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.

Sign up for GitHub

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

Jump to bottom

acme: share accounts between certificates #85185

Merged
merged 1 commit into from Apr 14, 2020
Merged

acme: share accounts between certificates #85185

merged 1 commit into from Apr 14, 2020

Conversation

m1cr0man
Copy link
Contributor

Fixes #85152

Motivation for this change

There are strict rate limits on account creation for Let's Encrypt certificates. It is important to reuse credentials when possible.

This is a regression from #84781. Having tested on my own system which had this change committed, this new symlink logic will not affect already updated hosts and not trigger another account refresh.

I am tempted to check if /var/lib/acme/.lego/${cert} is a directory and remove it, thus undoing the effect of #85152 but I'm uneasy about deleting folders automatically :P Let me know what you think.

Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS linux)
  • Built on platform(s)
    • NixOS
    • macOS
    • other Linux distributions
  • [ ]Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
  • Tested compilation of all pkgs that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • Determined the impact on package closure size (by running nix path-info -S before and after)
  • Ensured that relevant documentation is up to date
  • Fits CONTRIBUTING.md.

@m1cr0man
acme: share accounts between certificates
827d5e6 
There are strict rate limits on account creation for Let's Encrypt
certificates. It is important to reuse credentails when possible.
emilazy
emilazy approved these changes Apr 13, 2020
View reviewed changes
Copy link
Member

@emilazy emilazy left a comment •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks!

@GrahamcOfBorg build nixosTests.acme

@emilazy
Copy link
Member

emilazy commented Apr 13, 2020 •
edited

I am tempted to check if /var/lib/acme/.lego/${cert} is a directory and remove it, thus undoing the effect of #85152

Guessing you mean /var/lib/acme/.lego/${cert}/accounts? I think it's a bad idea to delete people's account private keys without their consent; they're not terribly valuable, but they do matter for revocations, and CAA records can tie certificate issuance to account IDs, so although the window of opportunity here was small, we should probably support users who already got migrated to one account per certificate.

@GrahamcOfBorg test acme

@emilazy
Copy link
Member

emilazy commented Apr 13, 2020

(cc @grahamc; not sure whether I was just too impatient or whether ofborg failed to recognize my command until I posted it in a non-review comment?)

@m1cr0man
Copy link
Contributor Author

@emilazy I fully agree with that. I'll keep the PR as-is then, and support both scenarios. 🙂

@emilazy
Copy link
Member

emilazy commented Apr 13, 2020

Not sure what's up with either of the tests; the AArch64 test isn't running because I'm not a trusted user, the x86_64 test is failing cryptically. We know the ACME tests are flaky, so let's hope it's just nondeterminism and spin the wheel again...

@GrahamcOfBorg test acme

@lukateras
Copy link
Member

@GrahamcOfBorg test acme

@Mic92 Mic92 merged commit fd438d5 into NixOS:master Apr 14, 2020
@Mic92
Copy link
Member

Mic92 commented Apr 14, 2020

backport:

[detached HEAD ecfd73d] acme: share accounts between certificates
Author: Lucas Savva lucas@m1cr0man.com
Date: Mon Apr 13 23:54:44 2020 +0100
1 file changed, 2 insertions(+), 1 deletion(-)

This was referenced Apr 16, 2020
Closed
Closed
@m1cr0man m1cr0man deleted the legoaccounts branch October 6, 2020 20:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@emilazy emilazy emilazy approved these changes

Assignees
No one assigned
Labels
6.topic: nixos 8.has: module (update) 10.rebuild-darwin: 0 10.rebuild-linux: 0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Split lego state directories causes account rate limits
4 participants
@m1cr0man @emilazy @lukateras @Mic92