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

nixos/acme: Must-Staple and extra flags #80900

Merged
merged 2 commits into from Mar 3, 2020

Conversation

emilazy
Copy link
Member

@emilazy emilazy commented Feb 23, 2020

Motivation for this change

Split out from #80856.

Responding to @m1cr0man from there:

The first two look good to me! I probably need this on my own setup with the 28 certs I'm trying to validate there, to avoid rate limits stuck_out_tongue

As for the additionalLegoRenewFlags, I actually had something similar (extraFlags) before I made the PR for lego (#77578, actually it was here) . I removed it because I felt it was too solution specific, and if we were to change acme clients again, this flag would have to be deprecated. With the other options like mustStaple and dnsPropagationCheck, at least if the next client has those features we can rewrite the arguments, but if people have random extra arguments then we can only break their configuration.

I am not against the idea either, for power users there is certainly desire to allow for custom flags to the service, but they run the above risk of it breaking some day. Would love to hear some other opinions on the matter. 🙂

I personally think that it's better for additionalLegoRenewFlags = ["--my-option"] to break cleanly at a later date than it is for people to be unable to use the NixOS module if they need additional configuration, but I realize that this is a somewhat philosophical difference. I made sure to include the name of the command in the configuration option so that there's no clash with a possible later additionalCertbotFlags or similar.

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.

@lukateras
Copy link
Member

I'd like to merge this on March 2 9 pm UTC unless anyone minds otherwise. cc @aanderse @m1cr0man

@emilazy
Copy link
Member Author

emilazy commented Feb 29, 2020

renamed mustStapleocspMustStaple for clarity

nixos/modules/security/acme.nix Outdated Show resolved Hide resolved
@lukateras lukateras merged commit c16f221 into NixOS:master Mar 3, 2020
@emilazy emilazy deleted the acme-must-staple branch March 3, 2020 01:19
dtzWill pushed a commit to dtzWill/nixpkgs that referenced this pull request Mar 3, 2020
nixos/acme: Must-Staple and extra flags

(cherry picked from commit c16f221)
@jktr jktr mentioned this pull request Jun 8, 2020
10 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants