Let's Encrypt certs using DNS challenges #337

jvperrin · 2018-07-09T10:25:52Z

This uses dehydrated to request certs with a hook to use a DNS challenge and update bind9. I used these two because after messing around with them I was able to get them to work without too many problems and because they are both already packaged for Debian (need to use stretch-backports to get new enough versions, but they work fine).

This also removes ocf_ssl and refactors it into ocf::ssl instead, where there are both ocf::ssl::default (a bundle using LE certs and a DNS challenge) and ocf::ssl::default_incommon (a bundle using our previous method of using incommon certs), so it's quite easy to switch between the two.

One problem was making sure that when certs are renewed that the services that depend on those certs are restarted properly to re-read the new cert. I believe this is done for all modules, but let me know if you see a place where a subscribe isn't added or something.

Unfortunately I had to add a couple exec resources to actually request certs, but I didn't want it to run every time, so instead I use a bit of a hacky solution with two execs and an unless to make sure that neither exec executes unless the cert is close to expiring or the list of domains has changed. I'd be happy to explain more if you'd like an explanation, but it was a bit of a mess and I'd be open to suggestions if they are cleaner and meet the requirements.

NOTE: One problem is that the packages needed for this aren't available until stretch (meaning that death, and tsunami won't work with this, werewolves doesn't currently use it), so I don't think this should be merged until after we upgrade all hosts to stretch, meaning that ocf.io will probably have to be manually renewed one final time unless we migrate everything to stretch in the next ~10 days.

abizer

Nice job! You're on a roll :D

jvperrin · 2018-07-10T05:58:35Z

Alright, I backported the necessary packages for jessie from stretch and they worked for tsunami and after a few tries I got death working too (the regex for the dehydrated hook wasn't quite correct and the DNS entry for the root domain wasn't present, but those have been fixed now). I think this is ready to be shipped, I'd like some more people to look at the exec stuff to make sure that nobody else has a better way to do it, because I imagine there could be something nicer perhaps.

kpengboy

Thanks for getting this done, it's quite a large undertaking.

Regarding the two execs, I can neither think of a better solution. The other alternative I thought of is to have a fact that gathers expiration dates of certs under /var/lib/lets-encrypt, but that doesn't sound much better.

Or, of course, you could do it all with systemd...

kpengboy · 2018-07-10T08:29:48Z

modules/ocf/manifests/ssl/bundle.pp

+
+    file {
+      default:
+        group     => 'ssl-cert',


Do we wish to specify the owner here?

Yeah, good idea

kpengboy · 2018-07-10T08:39:52Z

modules/ocf/manifests/ssl/default.pp

+    [$::hostname],
+    delete($::dnsA, '@'),
+    $::dnsCname,
+  ), ''), '.ocf.io') + $extra_domains


...would suggest some code deduplication here....

I think the only way this could be really deduplicated is to use puppet functions, I'll try that and see how it goes, but it might be too complicated for our use here, idk

We can't assign the result of delete(concat([$::hostname], delete($::dnsA, '@'), $::dnsCname), '') to some variable?

Sure, that would work too lol

kpengboy · 2018-07-10T08:59:00Z

modules/ocf/manifests/ssl/lets_encrypt.pp

+
+    $letsencrypt_ddns_key = assert_type(Stdlib::Base64, hiera('letsencrypt::ddns::key'))
+
+    # TODO: Move these somewhere else so this defined resource can be used


/var/lib/lets-encrypt/$title perhaps?

No, like the certs directory can't really be moved unless we move the whole base directory, I meant to move this inside puppet to a different manifest so it could be included or something instead of redefined every time. This stuff only needs to be declared once, although I'm not sure how the domains list will work, it'll probably need to use concat or something.

kpengboy · 2018-07-10T09:35:05Z

modules/ocf/manifests/ssl/lets_encrypt.pp

+        File['/var/lib/lets-encrypt/domains.txt'],
+        File['/etc/dehydrated/config'],
+        File['/etc/dehydrated/dehydrated-hook-ddns-tsig.conf'],
+        File['/etc/ssl/lets-encrypt/le-account.key'],


Aren't these four File resource references redundant, because of their notify statements?

Yeah, probably

kpengboy · 2018-07-10T09:36:29Z

modules/ocf/manifests/ssl/lets_encrypt.pp

+    # This exec can be notified to get it to check anyway, for instance, if the
+    # dehydrated config changes, or the domain list changes then this should
+    # run again, even if the cert will not expire soon.
+    exec { $title:


We're gonna get unspecific resource titles like Exec[www] at this rate; I would strongly recommend changing the title to be more specific here.

It'll be more like Exec[death.ocf.berkeley.edu], but sure, changing it would probably be fine, it should probably mention "cert" or maybe "ssl" somewhere in there, idk

Something like Exec[Renew $title cert] would work well IMO

kpengboy · 2018-07-10T09:37:04Z

modules/ocf/manifests/ssl/lets_encrypt.pp

+    # itself, but this should help with puppet error spam a bit (for instance
+    # if Let's Encrypt is down for any reason) and mean that services can
+    # subscribe to this resource to refresh after new certs are obtained.
+    exec { "${title}_check_expiration":


Exec titles can contain spaces.

kpengboy · 2018-07-10T09:50:14Z

modules/ocf/manifests/ssl/setup.pp

+  file {
+    default:
+      # the ssl-cert package creates the ssl-cert group
+      require => Package['ssl-cert'];


This seems redundant, given that the one file that actually uses this group already has the require set properly.

No, this was migrated from ocf_ssl, it's necessary because I don't believe /etc/ssl/certs exists without the ssl-cert package already being installed, but you're right that the file at the bottom of this block can have the require removed, I copy/pasted it from another manifest so that's why the require is duplicated.

The comment on this needed updating though, since it didn't explain the directories being created, so I see now why you would have thought it was just needed for the ssl-cert group.

kpengboy · 2018-07-10T09:51:11Z

modules/ocf/templates/ssl/dehydrated-hook-ddns-tsig.conf.erb

+[DEFAULT]
+name_server_ip = 169.229.226.22
+verbosity = 1
+# XXX: Do not put the key name in quotes, or it will not work


If it were me I would use something more specific like NOTE here. That's my personal preference.

Sure, the XXX was meant to be more of a warning than NOTE would be, since I've seen it like that before, but I don't mind changing it either

jvperrin · 2018-07-12T04:03:39Z

@kpengboy I thought about doing this via cron/systemd/whatever, but the issue with using some other solution is that when certs are renewed, daemons don't necessarily read in the new contents, so I wanted to have some kind of way for us to be able to reload/restart existing programs using the certs when they are renewed. This could also be done through cron/systemd, but we'd have to specify the exact program to reload when we already have those services specified in puppet, so it seemed like a natural fit to put this into puppet instead.

kpengboy · 2018-07-12T04:21:30Z

Or, of course, you could do it all with systemd...

@kpengboy I thought about doing this via cron/systemd/whatever

Lol that was sarcasm. I was referring to that you could theoretically make systemd path units for your four files, then use those units to trigger a systemd timer that renews the cert. I guess you could then use that to reload the service, although I didn't think that far. Of course, it'd be much easier to do this via Puppet anyway.

jvperrin · 2018-07-12T04:27:19Z

Ah, fair enough, I just meant this whole PR, not just the four files and the exec, but fair enough, I hadn't thought of using systemd path units lol

jvperrin · 2018-07-12T05:08:40Z

I just realized I haven't done anything with this for things hosted by Marathon, so I'll try and add that soon, once the DNS changes in ocf/dns@1cb09fa have propagated a bit more.

kpengboy · 2018-07-12T05:11:11Z

modules/ocf/functions/get_host_fqdns.pp

+# If there's a host with hostname 'death' and DNS A records 'dev-vhost' and DNS
+# CNAME records 'www', then this function (with the suffix 'ocf.io' would
+# return ['death.ocf.io', 'dev-vhost.ocf.io', 'www.ocf.io']
+function ocf::get_host_fqdns(String $suffix = 'ocf.berkeley.edu') {


What is our typename policy these days? Do we care to add a return type too?

I didn't add a return type because it doesn't work with older versions of puppet, and overheat doesn't have puppet available past 4.8, although maybe it could get a package from buster or something?

I'd ultimately like to add more types to this repo since they seem reasonably useful, but I'm not sure if we've really talked about it at all.

Wait, they actually work with 4.8, I thought it was with above 4.8, not 4.8 and above, so adding a return type to this sounds like a good idea.

kpengboy · 2018-07-12T05:17:21Z

modules/ocf/functions/get_host_fqdns.pp

+    [$::hostname],
+    delete($::dnsA, '@'),
+    $::dnsCname,
+  ), ''), ".${suffix}")


If we're going to refactor this into a function, we might as well reduce the number of lines it takes up. I honestly think it's easier to read as 1 or 2 lines than as 5.

I suppose so, my hesitation to put it on one line is because it exceeds the line length limit, but with variables that might be ok.

kpengboy · 2018-07-12T19:32:35Z

modules/ocf_mesos/manifests/master/load_balancer/http_vhost.pp

-  $ssl            = false,
-  $ssl_dir        = $title,
+  $ssl            = true,
+  $ssl_domains    = [$server_name],


Can we also add by default the server aliases which end in our domains?

Yeah, that would make sense, I'm just waiting on testing this since last time I was rate-limiting when working on it. I'll try again soon though

kpengboy

Thanks for getting this done! It's good to finally start being able to use automated LE to request certs for more of our servers.

kpengboy · 2018-07-20T22:20:03Z

@jvperrin bump? our cert's expired again

jvperrin · 2018-07-20T22:20:43Z

Ah, yeah I meant to merge this before today, sorry about that, I'll merge this now (after resolving conflicts)

This (along with some other refactoring) allows for being able to have multiple certificate bundles on one server without any problems.

kpengboy · 2018-07-20T23:43:50Z

modules/ocf_stats/manifests/munin.pp

@@ -42,6 +42,14 @@
  include apache::mod::fcgid
  include apache::mod::rewrite

+  $cname = $::host_env ? {


This is a duplicate declaration

Good catch, I got some merge conflicts when rebasing this, and it looks like I didn't resolve this correctly. I've fixed it in f3fdd59.

ethanhs · 2018-07-21T00:00:00Z

Congrats! And thanks!

jvperrin requested review from chriskuehl, dkess, abizer, kpengboy and kkuehlz July 9, 2018 10:25

jvperrin force-pushed the lets-encrypt-dns branch from 7f52756 to 6af52f3 Compare July 9, 2018 10:26

abizer approved these changes Jul 9, 2018

View reviewed changes

kpengboy reviewed Jul 10, 2018

View reviewed changes

kpengboy reviewed Jul 12, 2018

View reviewed changes

kpengboy approved these changes Jul 14, 2018

View reviewed changes

ethanhs approved these changes Jul 14, 2018

View reviewed changes

jvperrin added 11 commits July 20, 2018 15:22

Rewrite ocf_ssl into ocf::ssl

44e207d

Set up dehydrated config and automatic renewal

2365d5f

Switch a couple hosts to Let's Encrypt

3a6b53c

Update all staff VMs to use LE certs

15f7156

Fix permissions for Let's Encrypt certs

96ba975

Switch to Let's Encrypt on all hosts but ocf_mesos

16941bb

Special case certs for the DNS @ record

7ea3dbd

Migrate ocf_mesos to use Let's Encrypt certs

e99b4d0

Remove unused option in dehydrated config

b57a456

Fix dehydrated hook regex and cert alias issues

7bd0930

Split ocf::ssl::lets_encrypt into http and dns

598c018

This (along with some other refactoring) allows for being able to have multiple certificate bundles on one server without any problems.

jvperrin added 3 commits July 20, 2018 15:23

Refactor get_host_fqdns function

0ae46a9

Add vhosts with ocf_mesos to LE certs

b3985f6

ocf_mesos: Detect SSL-eligible vhosts for certs

6342ac7

jvperrin force-pushed the lets-encrypt-dns branch from 74297fc to 6342ac7 Compare July 20, 2018 22:23

jvperrin merged commit 7d46f23 into ocf:master Jul 20, 2018

jvperrin deleted the lets-encrypt-dns branch July 20, 2018 22:37

kpengboy reviewed Jul 20, 2018

View reviewed changes

jvperrin mentioned this pull request Jul 31, 2018

Retry obtaining certs if domains don't match #343

Closed


		$letsencrypt_ddns_key = assert_type(Stdlib::Base64, hiera('letsencrypt::ddns::key'))

		# TODO: Move these somewhere else so this defined resource can be used

Let's Encrypt certs using DNS challenges #337

Let's Encrypt certs using DNS challenges #337

Conversation

jvperrin commented Jul 9, 2018 • edited Loading

abizer left a comment

Choose a reason for hiding this comment

jvperrin commented Jul 10, 2018

kpengboy left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jvperrin commented Jul 12, 2018

kpengboy commented Jul 12, 2018

jvperrin commented Jul 12, 2018

jvperrin commented Jul 12, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kpengboy left a comment

Choose a reason for hiding this comment

kpengboy commented Jul 20, 2018

jvperrin commented Jul 20, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ethanhs commented Jul 21, 2018

jvperrin commented Jul 9, 2018 •

edited

Loading