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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

nixos/acme: lego run when account is missing #101482

Merged
merged 3 commits into from Dec 20, 2020
Merged

nixos/acme: lego run when account is missing #101482

merged 3 commits into from Dec 20, 2020

Conversation

@m1cr0man
Copy link
Contributor

@m1cr0man m1cr0man commented Oct 23, 2020

Motivation for this change

Closes #101445

It is possible that the wrong lego command will be issued if no account credentials exist. This can happen if you follow the new instructions to resolve JWS verification errors.

Annoyingly, I did not include the email option in the hash for the accounts directory. If I were to add it now, I would trigger a full certificate renewal + account creation for every certificated generated from this module 馃槈 Thus I will apply Irish "It'll be grand" logic and not change it now.

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
Copy link
Contributor Author

@m1cr0man m1cr0man commented Oct 23, 2020

Ping ye olde @NixOS/acme

@andir
Copy link
Member

@andir andir commented Oct 23, 2020

I wonder if we should use stateVersion to decide if we should use the "compatible" mode or the "proper" mode to handle this. IMHO we should carry a proper solution in the source tree.

@m1cr0man
Copy link
Contributor Author

@m1cr0man m1cr0man commented Oct 24, 2020

That sounds like a good solution. I'll add the conditional logic to cover that today.

othersHash = mkHash (
"${toString acmeServer} ${data.keyType}"
+ (
optionalString (versionOlder "20.09" config.system.stateVersion) data.email
Copy link
Member

@andir andir Oct 25, 2020

This looks good. I just had a thought that might also apply in general in these stateVersion checks.

Should we, at some point, throw a warning that a user should migrate (at their pace within a release) so we can realistically remove backwards compatibility at some point?

Copy link
Contributor

@flokli flokli Dec 16, 2020

@m1cr0man any opinions on that?

Copy link
Contributor Author

@m1cr0man m1cr0man Dec 16, 2020

So in the newer PR, I actually done away with this logic. This was written under the assumption that there was still rate limit issues with multiple certs being generated, but that's not the case now.

Copy link
Contributor Author

@m1cr0man m1cr0man Dec 16, 2020

Oh also worth noting I added a note on this matter to the release notes.

Copy link
Contributor

@flokli flokli Dec 16, 2020

So should this PR be changed before we merge it as requested in #101482 (comment)? Or do you want to close this one and incorporate it in the other?

Copy link
Contributor Author

@m1cr0man m1cr0man Dec 18, 2020

I will make the necessary changes to this PR so it can be merged. I worry it might take too long for the other PR to be merged.

@vincentbernat
Copy link
Member

@vincentbernat vincentbernat commented Dec 7, 2020

I have hit the bug fixed by this PR. Any chance it gets merged? Also, when requesting several certificates with the same account, it seems there is a race condition somewhere, with each certificate trying to create the same account.

arianvp
arianvp approved these changes Dec 7, 2020
@m1cr0man
Copy link
Contributor Author

@m1cr0man m1cr0man commented Dec 15, 2020

Doh! I totally forgot this PR existed. I've duplicated some of this work in #106857 . Can we get this merged so I can deal with the merge conflicts?

This means that all systems running from master will trigger
new certificate creation on next rebuild. Race conditions around
multiple account creation are fixed in NixOS#106857, not this commit.
@flokli flokli merged commit 49853c6 into NixOS:master Dec 20, 2020
17 checks passed
@symphorien
Copy link
Member

@symphorien symphorien commented Dec 22, 2020

Do the maintainers think this is fit for a backport to 20.09 ?

@m1cr0man
Copy link
Contributor Author

@m1cr0man m1cr0man commented Dec 22, 2020

I was going to propose the back port waits until #106857 is ready, as this PR alone does not fix account creation race conditions. However, it could be argued that has always been an issue so there's no harm in back porting it now. I'll let someone else cast the deciding vote.

@m1cr0man m1cr0man deleted the jwsfix branch Dec 22, 2020
@nh2
Copy link
Contributor

@nh2 nh2 commented Dec 27, 2020

However, it could be argued that has always been an issue so there's no harm in back porting it now. I'll let someone else cast the deciding vote.

@m1cr0man Please do backport it, I just found this to block me in the middle of a server migration, and I had to apply these patches to fix it.

@m1cr0man
Copy link
Contributor Author

@m1cr0man m1cr0man commented Dec 28, 2020

Ok, the backport is here: https://github.com/NixOS/nixpkgs/pull/107769/files

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

Successfully merging this pull request may close these issues.

7 participants