Page MenuHomePhabricator

Certificate Management for GitLab
Closed, ResolvedPublic

Description

Decide on Certificates for HTTPS for GitLab:

Option 1: GitLab can out of the box create (and renew) its own certificates with Let's Encrypt

Option 2: Integrate with acme-chief

  • Pro: used by other production systems with public ips
  • Con:

Event Timeline

Krinkle renamed this task from Certificate Management for Gitlab to Certificate Management for GitLab.Mar 6 2021, 4:54 PM
Krinkle updated the task description. (Show Details)

Change 670424 had a related patch set uploaded (by Jbond; owner: John Bond):
[operations/puppet@production] acme_chief: add gitlab certificate

https://gerrit.wikimedia.org/r/670424

Change 670427 had a related patch set uploaded (by Jbond; owner: John Bond):
[operations/puppet@production] P:gitlab: Deploy acme chief certificate

https://gerrit.wikimedia.org/r/670427

i think option 2 should be preferred as it allows us to make use of the shared config and monitoring space which makes things like changing algorithms and key sizes centrally a bit simpler to manage. Its fairly simple to add a certificate to acme chief and deploy it to the gitlab servers

Yeah, I tend to prefer option 2 as well. The other option could work in the short term, although maybe we'd want to evaluate whether it's sufficient in all respects (e.g. how early does it renew, and how does it retry on failures, and does it burn up ratelimits aggressively in a failure scenario, in a way that could impact acme-chief use of the same ratelimits, and how do we monitor for renewal failures, etc, etc). It seems easier to just use our standardized solution where we're solving these problems centrally.

I would vote for this intermediate solution:

Use acme-chief to get and renew the cert and let puppet drop it into the filesystem on the VM, but do not let puppet actually control and restart a service.

I think that if we manage the cert we have to manage reloading the daemon,
but agree we don’t need to manage the service. If not some new system
needs to be created to monitor the disks on file, check for changes and
then reload nginx.

Running the command above after a new very seems pretty harmless to me and
shouldn’t interfere a great deal with development.

The service has still to be notified somehow, to reload certificates at least. It can be implemented as simple as a hook (a predefined script for example) that can be called on a change. We will manage the script within the installation.

I think that if we manage the cert we have to manage reloading the daemon,

You are right and I like the new version with the simple reload exec and without service{}. Plus 1 to that.

The service has still to be notified somehow, to reload certificates at least. It can be implemented as simple as a hook (a predefined script for example) that can be called on a change. We will manage the script within the installation.

Agreed the most recent itteration of the proposed change calls gitlab-ctl hup nginx when the cert is renewed.

The service has still to be notified somehow, to reload certificates at least. It can be implemented as simple as a hook (a predefined script for example) that can be called on a change. We will manage the script within the installation.

The suggested change by John would take care of that by running command => '/usr/bin/gitlab-ctl hup nginx', if the cert changes. (But without letting puppet manage the entire service). I think that's the best solution.

fyi both theses changes are good to go, however can i get a +1 from @wkandek before merging. AFAICT nothing has been installed on gitlab1001 just yet so I'm guessing there is no immediate urgency but it dose not harm to push this out so its in place and ready.

Change 670424 merged by Dzahn:
[operations/puppet@production] acme_chief: add gitlab certificate

https://gerrit.wikimedia.org/r/670424

After I saw +1 from Valentin, I deployed the acme-chief part of this and checked puppet run on acmechief1001. Everything looked good. So acme-chief servers should now deliver the cert once asked for it.

The second change I did not merge yet because the application isn't installed yet on gitlab1001 and I did not want puppet to fail trying to run the restart command.

And then I saw John's comment here, so waiting with that part, ACK.

Change 670427 merged by Jbond:

[operations/puppet@production] P:gitlab: Deploy acme chief certificate

https://gerrit.wikimedia.org/r/670427

brennen claimed this task.