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

x/crypto/acme/autocert: handle short cert lifetimes #64997

Open
mjl- opened this issue Jan 7, 2024 · 9 comments
Open

x/crypto/acme/autocert: handle short cert lifetimes #64997

mjl- opened this issue Jan 7, 2024 · 9 comments
Labels
FixPending Issues that have a fix which has not yet been reviewed or submitted. Proposal Proposal-Accepted Proposal-Crypto Proposal related to crypto packages or other security issues
Milestone
Unreleased

Comments

@mjl-
Copy link

mjl- commented Jan 7, 2024

Go version

n/a (x/crypto v0.17.0, latest at time of writing)

Output of go env in your module/workspace:

n/a

What did you do?

Use autocert with step-ca as ACME server (https://smallstep.com/docs/step-ca/) for internal testing. I created an acme.Client with a DirectoryURL pointing to step-ca, but I did not change autocert.Manager.RenewBefore.

What did you see happen?

Autocert retrieved a certificate, so that is working as intended.

However, autocert immediate starts and keeps refreshing the certificate. That's because the default RenewBefore is 30 days, which is sensible for Let's Encrypt's 90 day certificate validity period. However, step-ca hands out short-lived certificates valid for 1 day. So when a new certificate is retrieved, it is refreshed immediately, ad infinitum.

What did you expect to see?

No refresh loop.

Of course, I should have set autocert.Manager.RenewBefore to match the default 1 day certificate validity period of a new step-ca instance. That would have prevented this problem. However, I hadn't made RenewBefore configurable in my application, so instead I changed the step-ca config to issue certificates with a lifetime of 31 days. I suspect more people could run into this when configuring alternative ACME providers in the future.

I think it would be helpful to change the behavior of a zero RenewBefore value. Instead of using 30 days, we can set the time at which to renew to 2/3 of the certificate validity period. For Let's Encrypt, that would still be 30 days before expiration. For a 1 day certificate, it would be 8 hour before the expiration. If a certificate is valid for 12 months, it would be at 4 months before expiration (which feels like it may be too early, but not harmful, and also not likely: one of the advantages of ACME is enabling shorter-lived certificates, desirable due to revocation issues).

At the very least, we should probably add a check to not renew an issued certificate immediately. E.g. have a minimum "next refresh delay" of 1 hour.

I can make a CL if there is any interest in the idea.
@gopherbot gopherbot added this to the Unreleased milestone Jan 7, 2024
@dmitshur dmitshur added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Jan 9, 2024
@dmitshur
Copy link
Contributor

dmitshur commented Jan 9, 2024

CC @bradfitz, @rolandshoemaker, @golang/security.

@mcpherrinm
Copy link

Let's Encrypt will to begin offering short-term certificates as an option in the future as well, like 7-day certificates.
We are the default CA used by this package.

The current behaviour is going to be problematic for us, and so it would be good to have this changed well in advance of anything we do, to make that transition easier and avoid needing to block misbehaving clients.

I would recommend the 2/3 fraction instead of 30 days.

@rolandshoemaker
Copy link
Member

Since this will de facto be an API change it should go through the proposal process.

It sounds like a plausible change would be to make the doc comment for RevewBefore:

// RenewBefore optionally specifies how early certificates should
// be renewed before they expire.
//
// If zero, they're renewed the lesser of 30 days before expiration,
// or after 2/3 of the certificate lifetime (i.e. for a certificate
// with a 9 day validity period, 3 days before expiration).
RenewBefore time.Duration

This maintains the current behavior for very long lived certs (unlikely to be particularly common, but 🤷), while introducing the more ideal behavior for shorter lived ones.

@rolandshoemaker rolandshoemaker changed the title x/crypto/acme/autocert: change default behavior of Manager.RenewBefore to prevent certificate renew loop proposal: x/crypto/acme/autocert: change default behavior of Manager.RenewBefore to prevent certificate renew loop Feb 12, 2024
@dmitshur dmitshur removed the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Feb 12, 2024
@dmitshur dmitshur modified the milestones: Unreleased, Proposal Feb 12, 2024
@ianlancetaylor ianlancetaylor added the Proposal-Crypto Proposal related to crypto packages or other security issues label Feb 13, 2024
@mcpherrinm
Copy link

I believe some WebPKI CAs are issuing 1 year certificates via ACME today, and probably some private ones too, so it does make sense to me to include a "lesser of" clause here.

@rsc rsc changed the title proposal: x/crypto/acme/autocert: change default behavior of Manager.RenewBefore to prevent certificate renew loop proposal: x/crypto/acme/autocert: handle short cert lifetimes Mar 1, 2024
@rsc
Copy link
Contributor

rsc commented Mar 1, 2024

Based on the discussion above, this proposal seems like a likely accept.
— rsc for the proposal review group

The proposal is #64997 (comment).

@rsc
Copy link
Contributor

rsc commented Mar 8, 2024

No change in consensus, so accepted. 🎉
This issue now tracks the work of implementing the proposal.
— rsc for the proposal review group

The proposal is #64997 (comment).

@rsc rsc changed the title proposal: x/crypto/acme/autocert: handle short cert lifetimes x/crypto/acme/autocert: handle short cert lifetimes Mar 8, 2024
@rsc rsc modified the milestones: Proposal, Backlog Mar 8, 2024
@mjl-
Copy link
Author

mjl- commented Mar 13, 2024

Is anyone planning to work on this already? If not, I can give it a go.

@rolandshoemaker
Copy link
Member

I'm unlikely to get to this any time soon, if you'd like to send a CL for it I'd be happy to review.

@gopherbot
Copy link

Change https://go.dev/cl/579695 mentions this issue: acme/autocert: renew certs at 2/3 of lifetime by default, but not earlier than 30d before expiry

@dmitshur dmitshur added the FixPending Issues that have a fix which has not yet been reviewed or submitted. label Apr 17, 2024
@dmitshur dmitshur modified the milestones: Backlog, Unreleased Apr 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
FixPending Issues that have a fix which has not yet been reviewed or submitted. Proposal Proposal-Accepted Proposal-Crypto Proposal related to crypto packages or other security issues
Projects
Proposals
Status: Accepted
Milestone
Unreleased
Development

No branches or pull requests
7 participants
@mcpherrinm @rsc @mjl- @dmitshur @ianlancetaylor @rolandshoemaker @gopherbot