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

ACME: default to Let's Encrypt's staging CA. #31122

Merged
merged 4 commits into from Nov 2, 2017
Merged

ACME: default to Let's Encrypt's staging CA. #31122

merged 4 commits into from Nov 2, 2017

Conversation

P-E-Meunier
Copy link
Contributor

@P-E-Meunier P-E-Meunier commented Nov 2, 2017

Motivation for this change

The goal is to avoid hitting rate limits too soon by default, which is
pretty bad when it hits you (for instance when NixOps forced you to
reinstall your instance more than five times in a week)

Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option build-use-sandbox in nix.conf on non-NixOS)
  • 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 nox --run "nox-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • Fits CONTRIBUTING.md.

The goal is to avoid hitting rate limits too soon by default, which is
pretty bad when it hits you (for instance when NixOps forced you to
reinstall your instance more than five times in a week)
@@ -139,6 +139,19 @@ in
'';
};

production = mkOption {
type = types.bool;
default = false;
Copy link
Member

Choose a reason for hiding this comment

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

This at least needs to default to true, we can't just break everyone's LE installs.

++ concatLists (mapAttrsToList (name: root: [ "-d" (if root == null then name else "${name}:${root}")]) data.extraDomains);
++ concatLists (mapAttrsToList (name: root: [ "-d" (if root == null then name else "${name}:${root}")]) data.extraDomains)
++ (if cfg.production then []
else ["--server" "https://acme-staging.api.letsencrypt.org/directory"]);
Copy link
Member

@Mic92 Mic92 Nov 2, 2017

Choose a reason for hiding this comment

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

That change would at least requiring a release note entry.

UPDATE: now production defaults to true so old behavior is restored.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

How do I do this?


See
<literal>https://letsencrypt.org/docs/staging-environment</literal>
for more detail. ''; };
Copy link
Member

Choose a reason for hiding this comment

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

This is some weird closing of quotes and braces, please keep to the style used in the rest of this file :)

@globin globin merged commit 538acd9 into NixOS:master Nov 2, 2017
@P-E-Meunier P-E-Meunier deleted the letsencrypt-staging branch November 2, 2017 10:13
@edolstra
Copy link
Member

edolstra commented Nov 2, 2017

What is the staging CA and what are the consequences of switching to it?

@globin
Copy link
Member

globin commented Nov 2, 2017

We didn't switch it, the outcome was to add a production option: 538acd9

@P-E-Meunier
Copy link
Contributor Author

The staging CA is a CA that is not in browsers, for debugging purposes.

The motivation for this change is that I hit the limit of Let's Encrypt certificate requests after NixOps crashed my GCE instances five times over three consecutive days, screwing my SSH access every single time. Now I have to wait four more days before I can ask for a certificate again.

ali-abrar pushed a commit to obsidiansystems/nixpkgs that referenced this pull request Feb 22, 2018
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

6 participants