Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merge certificates with a common second-level domain name into a sing… #57

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

Energy1190
Copy link

Merge certificates with a common second-level domain name into a single certificate with multiple sub-names.

This should help keep the limit on calls to let's encrypt api in the event that there are many third-level domains on the server.

@Eihrister
Copy link

If this is added, it should be made optional.

For instance, we are using this for 20000+ hosts, and looping over all of them every single time a certificate is renewed is not a good idea.

Apart from that is a privacy issue: A single host per certificate prevents other hosts a server listens on to be leaked in the same certificate. Adding all of them in one, could make third parties wiser than they need to be.

@GUI
Copy link
Collaborator

GUI commented Apr 12, 2017

@Energy1190: Thanks so much for the contribution and pull request! This will be great functionality to have! I'll try to take a closer look at the code this weekend.

@Eihrister: I think I agree that this should be an optional feature. Although, regarding the privacy concern, technically all certs are still discoverable by anyone via transparency logs. So I'm not sure if you knew about this (I only learned about it recently), but in light of this, does the privacy aspect still seem like as big of a deal to you? Personally, I think there's still an argument to be made that the transparency logs are less discoverable than everything being visible in a SAN certificate, but since all the info is technically out there, I just wasn't sure if you had any other thoughts.

And regarding your 20000+ hosts, out of curiosity, do you have any ballpark numbers for how many of those are subdomains versus unique domains? I need to look closer at the code in this implementation, but I think that may impact how many items are actually being looped over (since I think it might be trying to pull just the subdomains for a given domain first, so the number it's looping over might be much smaller, depending on your subdomain vs domain mix).

@Energy1190: As I mentioned, I need to give all this a closer look, but on a quick cursory glance, just a few notes:

  • As @Eihrister mentioned, we probably need to be careful about any potential performance impact of this, and ideally we'd make this behavior optional.
  • I think we need to account for public suffixes when determining the base domain. Right now, I believe the code is expecting the base domain to consist of the last 2 parts of the hostname, but it's trickier for TLDs like .co.uk (since in that case, the base domain needs to be the 3rd level domain, not the 2nd). Unfortunately, the public suffix list is pretty massive and evolving, so this might complicate things.
  • Let's Encrypt limits SAN certificates to 100 names per certificate, so ideally we'd be able to handle this intelligently, and request a new SAN certificate once the previous one has reached 100 items. But this might also complicate things, since we might need to keep track of more details on each cert and subdomain.
  • By appending all the domains as separate arguments to the dehyrated.sh call, we'll probably need to be careful of the limits in sockproc, which handles our shell calls. sockproc commands currently can't exceed 2040 characters. So things might start failing once a bunch of domains get added to the list. lua-resty-exec recently came on my radar as an alternative to lua-resty-shell, so that might be worth exploring to see if it's alternative to sockproc also has any length issues.
  • It would be great to add tests for this to the test suite, since this is pretty significant new functionality, and I think there's plenty of new edge-cases to consider.

So if you think you might be interested in tackling any of this, contributions are always welcome, but if this is more than you were bargaining for (the edge-cases for this unfortunately do seem rather thorny), I'm happy to try and collaborate on the functionality in this pull request (I'm just not sure when I'll have time personally). In any case, thanks again for getting the ball rolling on this feature! 🌟 🌟

@Eihrister
Copy link

@GUI First of all, don't get me wrong. As long as this is an optional feature, I'm all for it. Will likely be very useful to people. To us, however, not so much... and here's why:

As far as privacy: Maybe privacy wasn't the right term. We host roughly 10.000 shared hosting customers. They have say 20000 names connected to their sites. Some do, some don't like it when all of their names are clearly visible in the browser. Transparency logs only state that certificates have been given out, not where or for what they are in use (though easily found, sure). Again, with this being optional, it's a non-issue.

That being said, the current implementation has clear advantages as to its simplicity.
This added implementation will add a lot of layers of complexity, not just in terms of code, but also in logic, cleanup, keeping track of things, reissuances, etc.

Having one certificate per host makes it easy to just delete old certificates for that host after it's no longer in use on our platform. When a new host is added, a new certificate is created for that host, instead of having to revalidate all hosts within that domain.

A few notes, though:

With all these tens of thousands of hosts, we only found 2 customers that actually had more than the weekly rate limit of 20 certificates per domain. The rate limit only applies to newly generated certificates, renewals are exempted. Meaning those 2 customers only had to wait a week to get the rest of the certificates done.

Most of the actual problems regarding the rate limit, you hit during the initial stage. Not just the 20 per domain, but also the 500 per 3 hours limit. Before enabling it for everyone, we firewalled off port 443 to only allow Let's Encrypt to connect back for verification, then curl'd all of the hosts from the local machine. Throttled to stay under the limits.

This feature would reduce the time needed to generate all of those initial certificates also because of that reason: Less certificate issuances.

However, again, it's only the first time. Renewals are also exempted.

Also if you have only a few domains and they are quite static with so many hosts / subdomains under it, maybe this is not the solution for you and is a simple dehydrated install with a domains.txt a better option.

The main logic is moved to b. Added checks for the maximum length, for
the maximum number of names in the certificate.

The functional is now optional and is called by the parameter b. Through
it, you can also transfer the desired domain level.
@Energy1190
Copy link
Author

@GUI Thanks for answering. I corrected what you indicated.

  • Now all this is optional. Enabled with option auto_ssl:set("multiname_cert", true).
  • I did not find how to get around problems with public suffixes if using a sheet with them is undesirable. As an option, I made the ability to transfer to "multiname_cert" number, which will determine the level of the domain name.
  • I added a check for the limit of names, when the limit is reached, a new certificate is
  • I also added a check for the number of characters in the command line, although fairly lazy. In fact, I restricted the string with domains of 1000 characters, if this limit is reached, then a new certificate will be created by analogy with the restriction of 100 names.

I'm not sure if I can make such serious changes as replacing lua-resty-shell by lua-resty-exec. So I did not do it. And I need a lot more time to sort out the tests.
I changed the logic of the work, so that the already written code is almost not affected. I needed to check "multiname_cert" in three places: in the function "do_ssl", in order to initially replace the domain name, in function "issue_cert", that cancel the already existing check, because then, for all requests except for the first one, there was no certificate generation. And in "lets_encrypt.lua", to generate a list of names.

@luto
Copy link
Collaborator

luto commented Apr 19, 2017

Fixes #36

@luto
Copy link
Collaborator

luto commented May 6, 2017

@Energy1190 @GUI in my opinion the public suffix list is the right and only way to split domains into "suffix" and "prefix" parts. Someone even implemented a lua module for loading and parsing the list. So users of resty-auto-ssl only need to provide a PSL file in the setttings, which I find acceptable.

@ghost
Copy link

ghost commented May 7, 2017

Thank you for this great discussion and pull request!

For some of the more complex configurations would it be good idea if we simply created a direct communication link with dehydrated?

So that if a use case is not satisfied with auto-ssl the user can still instruct dehydrate to create certificates.

How will this work?

  1. The flags to dehydrate can be passed in the form of URL arguments.

  2. Thr URL arguments can be appended ,on a need by need basis, by an access by lua block in front of an incoming domain.

  3. If no URL arguments are passed auto-ssl works in a default setup. If arguments are passed auto-ssl just lets dehydrate do all the work and gets out of the way.

In this setup , the onus of error handling will be on the user.

Although the intention of this idea is to make things easier, I can't really say if they make it more complicated instead :)

@GUI
Copy link
Collaborator

GUI commented May 11, 2017

@Energy1190: Apologies for my delayed response on this. Thanks for making the various changes, though! Since this is a more significant change, I'd still like to mull this over a bit longer just so we understand the impact and the best way to go about all this. I'm hoping I can take a closer look at this over the weekend or next week. Hopefully this isn't holding anything up on your end, but thanks again for the contribution!

@luto: Thanks for the lua-psl link. That's definitely really good to know about, and could greatly simplify having to manage the huge publicsuffix list file ourselves.

I'm also wondering if perhaps deferring some of this logic to a callback users could implement might be another viable approach. Sort of similar to how we leave it up to users to implement allow_domain, we could have another callback controlling which domains can get combined (where users could either implement full-blown public-suffix handling, or if they have simpler cases, the logic might be much simpler)... Anyway, I still haven't fully thought all this through, but thanks to everyone for the feedback and ideas!

@MiniCodeMonkey
Copy link

bump

We could really use this feature as well (we keep getting "Error creating new cert :: too many certificates already issued").

For our use case, it would be perfectly find to have a callback to have users define how certificates should be grouped themselves -- love that idea!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants