nixos/acme: adjust renewal timer options #80856

emilazy · 2020-02-23T02:23:42Z

Motivation for this change

The current weekly setting causes every NixOS server to try to renew its certificate at midnight on the dot on Monday. This contributes to the general problem of periodic load spikes for Let's Encrypt; NixOS is probably not a major contributor to that problem, but we can lead by example by picking good defaults here.

The values here were chosen after consulting with @yuriks, an SRE at Let's Encrypt:

Randomize the time certificates are renewed within a 24 hour period.
Check for renewal every 24 hours, to ensure the certificate is always renewed before an expiry notice is sent out.
Increase the AccuracySec (thus lowering the accuracy(!)), so that systemd can coalesce the renewal with other timers being run.

(You might be worried that this would defeat the purpose of the time skewing, but systemd is documented as avoiding this by picking a random time.)

For more information justifying these changes, see the following documents:

~~I also added trivial commits to expose the --must-staple option and allow users to specify additional custom renewal options, as free goodies to encourage merging :p~~

I've tested these changes on my server, and everything seems fine, but I'm just one person; please do look over this carefully!

Things done

emilazy · 2020-02-23T04:17:26Z

Confirmed that this still passes the acme tests for me.

aanderse · 2020-02-23T12:57:18Z

@emilazy I like how the first commit is nice and clean, simple, documented, and will be easy to test + validate. I like how in your backport you only include this commit. Thanks for doing the research and reaching out to the Let's Encrypt team!

I think it will be pretty easy to have the collective of relevant committers agree to merge the first commit. The other 2 commits might require some more discussion, and could potentially hold this PR up, which would hold up the backport PR. Given the stage we're at (with regards to the 20.03 release) I might recommend splitting this PR up. Just a recommendation.

m1cr0man · 2020-02-23T13:09:43Z

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 😛

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. 🙂

aanderse

Overall I think these changes are good 👍 My comment on code complexity here isn't really because of your code, but I think here and now might be the time to consider if we can simplify the command line argument logic at all... if anyone is up to the challenge, or has any great idea.

nixos/modules/security/acme.nix

aanderse · 2020-02-23T13:18:44Z

nixos/modules/security/acme.nix

+                runOpts = escapeShellArgs (globalOpts ++ [ "run" ] ++ certOpts);
+                renewOpts = escapeShellArgs (globalOpts ++
+                  [ "renew" "--days" (toString cfg.validMinDays) ] ++
+                  certOpts ++ data.additionalLegoRenewFlags);


globalOpts, certOpts, runOpts, and renewOpts - each of which is determined by a load of module options. I worry the code is becoming too complicated to properly test against all potential regressions on future changes.

Anyone have any ideas to simplify this, all while adding the flexibility @emilazy has added here? 🤷‍♂️

I spent an unreasonable amount of time thinking about this already. There's just a lot of different ways to configure your certs (multiple places for email, implicit selection between dns and http for example). Maybe using some of the lib.optional* functions would help clean it up a bit. The fact that you need 2 separate strings for "run" and "renew" is a bit annoying too.

I don't really have good ideas here, other than for lego to support proper configuration files itself or something. I currently have to do

{ # TODO: DNS validation security.acme = { email = "..."; acceptTerms = true; certs = let cfg = { user = "nginx"; group = "nginx"; mustStaple = true; }; in { "emily.moe" = cfg // { extraDomains = { "patchouli.emily.moe" = null; }; }; "mew.build" = cfg; }; }; }

to share options between my certificates, so the current configuration model certainly isn't ideal.

emilazy · 2020-02-23T16:44:05Z

Updated to split out the Must-Staple/extra flags commits into #80900, and also divide the 24h AccuracySec by the number of certificates, so that if you do have, say, 28 certificates, the renew requests don't all get coalesced into the same time.

nervously eyes https://github.com/NixOS/nixpkgs/pulls/emilazy, waiting for it to grow pagination...

emilazy · 2020-02-23T17:44:02Z

An explanation for the AccuracySec change, in case you find the timers as confusing as I do:

17:35 <m1cr0m4n> Hey emilazy are you about? Just wanted to ask about
the maths on AccuracySec. If I understand this correctly, does lowering
the value for more certs not mean that they will run closer to the
same time, rather than more spread out throughout a day?
17:35 <emily> m1cr0m4n: no: the larger AccuracySec is, the larger
the allowed skew
17:35 <emily> it's a really confusing name, but check the systemd.timer
manpage
17:35 <emily> AccuracySec = 1s means "the time must be accurate to
within 1s of what is specified"
17:36 <emily> AccuracySec = 24h means "the time can vary within a
24h period of what's determined"
17:36 <m1cr0m4n> Right but, in this situation do you actually want
to make it more accurate? I don't fully understand why you would
17:36 <emily> so if you have 3 certs, it'll pick 3 random times of
the day, and then coalesce them within 8h periods
17:36 <emily> m1cr0m4n: let's say you have 100 certificates that all
expire around the same time: hammering let's encrypt for all 100 of
them at once is bad
17:36 <emily> doing it 100 separate times throughout the day is better
17:37 <emily> they'll still potentially coalesce statistically due
to the random placement
17:37 <emily> but basically we want to coalesce with other periodic
timers for power management reasons, but not coalesce with other acme
renewals for load management reasons
17:37 <emily> (yuriks suggested this approach fwiw)
17:37 <m1cr0m4n> yes I agree! I just don't get how that is happening
here. The math is diving 24h by the number of certs, so you're gonna
have like 24h/100 = < an 15 mins AccuracySec between certs.
17:38 <emily> so, the cert renewal checks are initially set to run
"daily", which means at midnight. If we didn't do any AccuracySec
or skewing, it would renew all 100 at midnight. adding the 24h skew
means that all 100 will be renewed at random times throughout the day
17:38 <emily> adding AccuracySec = 24h means that it'll notice
that all of these renewal timers are within the same 24h period,
and coalesce them
17:38 <emily> thus defeating the skew (it's still skewed relative to
other NixOS systems, but the certs themselves don't get skewed)
17:39 <emily> dividing 24h by the number of certs means that we
"bucket" them approximately according to the number of certs
17:40 <m1cr0m4n> Ok yeah..I think I understand :P Sorry, very confusing
time maths
17:41 <emily> I totally agree
17:41 <emily> mind if I post this log to the PR to help others?
17:41 <m1cr0m4n> Fire away! :)

m1cr0man

Looks good. This is one heck of a module now, I'd love to try my hand at simplifying it at some point.

arianvp · 2020-02-24T09:20:20Z

At least we have a good chunk of people who are interested in this module now. I'm happy to set up some brainstorming session (Perhaps we can create an IRC channel; #nixos-acme) to think how we can tame this code a bit.

arianvp

nixos/modules/security/acme.nix

arianvp · 2020-02-24T17:43:13Z

Leave it. I'm also not convinced that assert will live forever

…

On Mon, Feb 24, 2020, 16:40 Emily ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In nixos/modules/security/acme.nix <#80856 (comment)>: > @@ -399,7 +399,17 @@ in systemd.tmpfiles.rules = map (data: "d ${data.webroot}/.well-known/acme-challenge - ${data.user} ${data.group}") (filter (data: data.webroot != null) (attrValues cfg.certs)); - systemd.timers = flip mapAttrs' cfg.certs (cert: data: nameValuePair + systemd.timers = let + # Allow systemd to pick a convenient time within the day + # to run the check. + # This allows the coalescing of multiple timer jobs. + # We divide by the number of certificates so that if you + # have many certificates, the renewals are distributed over + # the course of the day to avoid rate limits. + numCerts = length (attrNames cfg.certs); + _24hSecs = 60 * 60 * 24; + AccuracySec = if numCerts == 0 then null else "${toString (builtins.div _24hSecs numCerts)}s"; Makes sense; I was just defensively avoiding the division by zero, though I suppose with lazy evaluation there's not really much point. I can adjust this if you'd prefer, though I'm not entirely convinced that that assert will last forever :) — You are receiving this because your review was requested. Reply to this email directly, view it on GitHub <#80856?email_source=notifications&email_token=AAEZNI44A5REZ6XKUGTWPKDREPS5FA5CNFSM4KZWVUP2YY3PNVWWK3TUL52HS4DFWFIHK3DMKJSXC5LFON2FEZLWNFSXPKTDN5WW2ZLOORPWSZGOCWVFI6I#discussion_r383340084>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAEZNIYGBM7IL3SCTP6SR3LREPS5FANCNFSM4KZWVUPQ> .

nixos/modules/security/acme.nix

@yuriks

The current weekly setting causes every NixOS server to try to renew its certificate at midnight on the dot on Monday. This contributes to the general problem of periodic load spikes for Let's Encrypt; NixOS is probably not a major contributor to that problem, but we can lead by example by picking good defaults here. The values here were chosen after consulting with @yuriks, an SRE at Let's Encrypt: * Randomize the time certificates are renewed within a 24 hour period. * Check for renewal every 24 hours, to ensure the certificate is always renewed before an expiry notice is sent out. * Increase the AccuracySec (thus lowering the accuracy(!)), so that systemd can coalesce the renewal with other timers being run. (You might be worried that this would defeat the purpose of the time skewing, but systemd is documented as avoiding this by picking a random time.)

lukateras · 2020-02-29T14:52:30Z

@GrahamcOfBorg build nixosTests.acme

lukateras · 2020-02-29T16:37:17Z

Passes nixosTests.acme and works as expected locally! :)

@aanderse Now that numCerts == 0 condition is out, is this good to merge?

aanderse

@yegortimoshenko yeah, I'm a fan of this change 👍 I also think we should merge the backport, so long as the release managers agree.

EDIT: has someone addressed the test failure? I thought there was a conversation about that...

lukateras · 2020-02-29T16:46:33Z

EDIT: has someone addressed the test failure? I thought there was a conversation about that...

Couldn't get it to fail locally; https://github.com/NixOS/nixpkgs/pull/80856/checks?check_run_id=476781247 also passes. https://github.com/NixOS/nixpkgs/pull/80856/checks?check_run_id=476784925 (another attempt) does fail. Looks like a race condition? Don't think that it's introduced by this PR.

aanderse · 2020-02-29T16:48:59Z

I thought @m1cr0man and @arianvp had some insight into the failed test... or maybe it was @flokli?

worldofpeace · 2020-03-01T12:49:42Z

This is a very nice change with well substantiated evidence for improvement. Backport is fine with me.

flokli · 2020-03-02T22:51:51Z

I thought @m1cr0man and @arianvp had some insight into the failed test... or maybe it was @flokli?

No, I mostly grumbled about the acme tests (in nixpkgs) not being very understandable currently, and requiring a bit more love - no knowledge about why they might or might not be flaky.

ofborg bot added 6.topic: nixos 8.has: module (update) labels Feb 23, 2020

emilazy mentioned this pull request Feb 23, 2020

[20.03] nixos/acme: adjust renewal timer options #80857

Merged

10 tasks

ofborg bot added 10.rebuild-darwin: 0 10.rebuild-linux: 1-10 labels Feb 23, 2020

emilazy force-pushed the adjust-acme branch 2 times, most recently from 4379912 to 6e404aa Compare February 23, 2020 02:35

emilazy requested review from aanderse and m1cr0man February 23, 2020 02:48

aanderse requested review from flokli, fpletz and arianvp February 23, 2020 03:04

aanderse reviewed Feb 23, 2020

View reviewed changes

emilazy force-pushed the adjust-acme branch from 6e404aa to cb04ec3 Compare February 23, 2020 16:37

emilazy mentioned this pull request Feb 23, 2020

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

Merged

10 tasks

m1cr0man approved these changes Feb 23, 2020

View reviewed changes

arianvp approved these changes Feb 24, 2020

View reviewed changes

aanderse reviewed Feb 24, 2020

View reviewed changes

nixos/modules/security/acme.nix Outdated Show resolved Hide resolved

lukateras reviewed Feb 29, 2020

View reviewed changes

nixos/modules/security/acme.nix Outdated Show resolved Hide resolved

nixos/modules/security/acme.nix Outdated Show resolved Hide resolved

emilazy force-pushed the adjust-acme branch from cb04ec3 to 7b14bbd Compare February 29, 2020 14:03

lukateras approved these changes Feb 29, 2020

View reviewed changes

aanderse approved these changes Feb 29, 2020

View reviewed changes

lukateras merged commit 31aefc7 into NixOS:master Mar 3, 2020

emilazy deleted the adjust-acme branch March 3, 2020 01:19

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

nixos/acme: adjust renewal timer options #80856

nixos/acme: adjust renewal timer options #80856

emilazy commented Feb 23, 2020 •

edited

emilazy commented Feb 23, 2020

aanderse commented Feb 23, 2020 •

edited

m1cr0man commented Feb 23, 2020

aanderse left a comment

aanderse Feb 23, 2020

m1cr0man Feb 23, 2020

emilazy Feb 23, 2020

emilazy commented Feb 23, 2020 •

edited

emilazy commented Feb 23, 2020

m1cr0man left a comment

arianvp commented Feb 24, 2020

arianvp left a comment

arianvp commented Feb 24, 2020 via email

lukateras commented Feb 29, 2020

lukateras commented Feb 29, 2020

aanderse left a comment •

edited

lukateras commented Feb 29, 2020 •

edited

aanderse commented Feb 29, 2020

worldofpeace commented Mar 1, 2020

flokli commented Mar 2, 2020

nixos/acme: adjust renewal timer options #80856

nixos/acme: adjust renewal timer options #80856

Conversation

emilazy commented Feb 23, 2020 • edited

Motivation for this change

Things done

emilazy commented Feb 23, 2020

aanderse commented Feb 23, 2020 • edited

m1cr0man commented Feb 23, 2020

aanderse left a comment

Choose a reason for hiding this comment

aanderse Feb 23, 2020

Choose a reason for hiding this comment

m1cr0man Feb 23, 2020

Choose a reason for hiding this comment

emilazy Feb 23, 2020

Choose a reason for hiding this comment

emilazy commented Feb 23, 2020 • edited

emilazy commented Feb 23, 2020

m1cr0man left a comment

Choose a reason for hiding this comment

arianvp commented Feb 24, 2020

arianvp left a comment

Choose a reason for hiding this comment

arianvp commented Feb 24, 2020 via email

lukateras commented Feb 29, 2020

lukateras commented Feb 29, 2020

aanderse left a comment • edited

Choose a reason for hiding this comment

lukateras commented Feb 29, 2020 • edited

aanderse commented Feb 29, 2020

worldofpeace commented Mar 1, 2020

flokli commented Mar 2, 2020

emilazy commented Feb 23, 2020 •

edited

aanderse commented Feb 23, 2020 •

edited

emilazy commented Feb 23, 2020 •

edited

aanderse left a comment •

edited

lukateras commented Feb 29, 2020 •

edited