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

Split lego state directories causes account rate limits #85152

Closed
m1cr0man opened this issue Apr 13, 2020 · 17 comments · Fixed by #85185
Closed

Split lego state directories causes account rate limits #85152

m1cr0man opened this issue Apr 13, 2020 · 17 comments · Fixed by #85185
Labels
0.kind: bug Something is broken 0.kind: regression Something that worked before working no longer
Milestone

Comments

@m1cr0man
Copy link
Contributor

Describe the bug
When writing #77578, it was important to ensure that the .lego state directory was common to all certs to ensure that new accounts were not being created for every certificate. The rate limit on new accounts is 10 every 3 hours. This is fine for most cases, but in my deployment I have 30 different domains to request certificates for, and that's after a lot of deduplication and grouping of domains.

As of #84781 this has been changed such that all certificates get their own state directories, and thus their own account credentials. As pointed out, the account directory can be symlinked using preStart scripts if necessary. I feel this isn't a matter of migrating user data but more about maintaining compatibility with existing setups like my own and also avoiding rate limits in LE's service. This should be part of the module's configuration by default.

@m1cr0man m1cr0man added the 0.kind: bug Something is broken label Apr 13, 2020
@aanderse
Copy link
Member

cc @flokli @arianvp @immae @emilazy @Mic92 and whoever else I'm missing...

@m1cr0man
Copy link
Contributor Author

I have been focused on getting a mail stack in Nixos working over the past week and I missed that PR and other things :/ sorry about that. I would've commented if I had seen it sooner.

@emilazy
Copy link
Member

emilazy commented Apr 13, 2020

Seems like reverting #84781 for now would probably be for the best; multiple certs for the same domain seems like a less important feature than not spamming LE with account creations. It should be possible to reuse the same account while keeping the certificates split, but maybe we'll have to make some ugly symlink forests?

I wish lego's CLI was more explicit.

@m1cr0man
Copy link
Contributor Author

Well that or, we can patch in symlinking of accounts directories back a folder. I think it wouldn't be too hard, I can prepare a PR now

@veprbl veprbl added this to the 20.03 milestone Apr 13, 2020
@veprbl veprbl added the 0.kind: regression Something that worked before working no longer label Apr 13, 2020
@emilazy
Copy link
Member

emilazy commented Apr 13, 2020

@Mic92 You posted about this issue before self-merging the PR and backporting it to 20.03; could you please subject your changes to critical infrastructure to wider review in future? It's quite frustrating to work with Let's Encrypt engineers to make the NixOS service well-behaved, only to have a change merged and backported that runs roughshod over rate limits and can potentially result in hundreds of certificates being renewed at once on upgrade just to avoid incorporating a few lines of migration code, with less than three hours between the PR being opened and merged, and only one recent contributor to the service being pinged. It's a lot easier to fix issues before merge than it is to rush to fight fires after something's hit the channels...

@immae
Copy link
Contributor

immae commented Apr 13, 2020

@m1cr0man : merging every certificates in a single directory leads to this issue: #84409
It was considered blocking for 20.03 because it broke a behavior that was authorized and expected in simp_le’s age. (The idea of symlinking the account folder was proposed, but since noone reacted noone implemented it I guess)

@m1cr0man
Copy link
Contributor Author

@immae I saw that issue and I don't intend to reintroduce it, but sharing just the accounts folder would fix this issue without regression. Of course, I will test and ensure that is the case on my system first but rest assured I have no intention to completely undo #84781

@immae
Copy link
Contributor

immae commented Apr 13, 2020

Thanks @m1cr0man 🙇

@m1cr0man
Copy link
Contributor Author

I created a PR...but I still don't fully understand the use case behind having multiple certs for the same domain? Surely, on a single system, you would only want one certificate per domain, even if they are for different services?

Also I did think of an issue with the .lego folder as a whole... if a user uses different user and group for 2 certificates then 1 of them will own the directory and the other will not be able to write to it. I don't know if we want to support that sort of configuration? It would certainly prevent us optimising around rate limits, but it is solvable if the .lego directory is under /var/lib/acme/${cert}

@emilazy
Copy link
Member

emilazy commented Apr 13, 2020

You can use different key types (to serve both RSA and ECDSA certificates for the same domain), or perhaps set other alternate names.

In terms of accounts directory sharing, I increasingly think that the only permissions option that makes sense from a privilege separation point of view is to have /var/lib/acme unconditionally owned by acme:acme and just selectively add other services like nginx to the acme group for read access (per #84633 (comment)).

@m1cr0man
Copy link
Contributor Author

Yeah I had read that earlier and I agree that an acme user + group would make sense. How do people feel about a PR for a complete revamp of the module? This was suggested back when I finished DNS-01 due to the number of changes and the complexity of the module at this stage. I would be tempted to at least try and do this (now that I have the time again), and see what can be simplified. It's becoming quite the monster.

@immae
Copy link
Contributor

immae commented Apr 13, 2020

@m1cr0man : if you want to completely rewrite the module, you might be interested in my recently listing of some issues regarding ACME: #84633

I think the last one might be very annoying: once a certificate is created, changing the list of domains in the certificate won’t work (in other words, in the current state of the module you cannot add or delete domains from a certificate after it’s generated)

@m1cr0man
Copy link
Contributor Author

@immae Yep also started tracking that issue already :) I think that one is resolvable by writing some file that hashes the domains + extraDomains, and takes action accordingly, but RemainAfterExit is a bigger bummer. I'll continue conversations about a rewrite over in that issue.

@Mic92
Copy link
Member

Mic92 commented Apr 14, 2020

@emilazy It was reviewed by @flokli and @immae. Catching ratelimits is not something one can easily test or fix. If one has 30 certificates they will run into certificate creation limits, in which only SAN certificates would help. The needed migration code was posted on Discourse and the Github issue for those that run unstable (Lego in ACME was never part of any stable release).

@rkoe
Copy link
Contributor

rkoe commented Apr 23, 2020

PLEASE backport this to 20.03, since it's a serious bug, with effectively completely breaks the lets-encrypt-certificate-renewal in 20.03 if more than 10 certificates are used on a server.

@arianvp
Copy link
Member

arianvp commented Apr 23, 2020

It was backported already. ecfd73d

Are you actually experiencing issues? If so; please open a new bug for that

@rkoe
Copy link
Contributor

rkoe commented Apr 23, 2020

I've updated a server to 20.03 today, and I'm experiencing exactly this issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
0.kind: bug Something is broken 0.kind: regression Something that worked before working no longer
Projects
None yet
Development

Successfully merging a pull request may close this issue.

8 participants