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

proposal: x/crypto/acme: support the ACME Renewal Information standard in Client #60958

Open
jmhodges opened this issue Jun 23, 2023 · 4 comments
Labels
Milestone

Comments

@jmhodges
Copy link
Contributor

jmhodges commented Jun 23, 2023

(This could be viewed as a preliminary ticket because, while Let's Encrypt supports the API in question and there are multiple ACME clients supporting it, the standard hasn't been finalized at time of creation. Recent events described below made me want to write this out.)

It would be nice for x/crypto/acme to give consumers of it a convenient way of knowing when their certificates need renewal especially when their CAs have revoked those certificates early.

One of the outcomes of the recent Let's Encrypt outage was a recognition that many ACME clients aren't renewing certificates that were revoked by the CA. @AGWA wrote a very nice piece on the outage, the renewal problem, and how ACME clients might solve it.

In that post, he mentions that Let's Encrypt currently supports the still-under-development ACME Renewal Information (ARI) standard. That standard and API allows ACME clients to hear from the CA that they need to renew a certificate they previously made. That renewal information also, helpfully, gives guidance on when to attempt a renewal even during normal operation.

It would be nice for x/crypto/acme to support the ARI standard.

One version of this work would be to:

  1. add a new method to acme.Client called GetRenewalInfo accepting a x509 certificate that it extract the necessary CertID from, and returning a RenewalInformation struct with the suggested liveness window and explanatory URL
  2. add a new method to acme.Client called UpdateRenewalInfo accepting a x509 certificate (a more minimal version would not include this method as its not required for correct operation of GetRenewalInfo)

This ticket includes a proposed API below. The method and type names below are meant only as reasonable first attempts at naming and others might have other better names.

The word "CertID" has specific meaning and importance here. The ARI APIs takes the certificate's "DER-encoded CertID ASN.1 sequence" (link). See section 4.1.1 of RFC6960 for the definition of "CertID". A CertID is a combination of what are currently multiple fields in crypto/x509.Certificate. That means GetRenewalInfo and UpdateRenewalInfo, unlike RevokeCert, needs to parse the x509 certificate given to it.

While GetRenewalInfo and UpdateRenewalInfo could take some CertID bytes or struct, that combination of data and encoding is a bit more complicated than we could expect users to re-create well. So, we're left with the choice of taking a []byte of the certificate that is parsed under the hood, or a *x509.Certificate.

A []byte for the cert seems preferable to a *x509.Certificate. First, because the []byte type fits the pattern set by RevokeCert. Second, a []byte would also likely result in less code written by most consumers as most don't care about the individual bytes inside a cert and would be passing it directly from their storage layer. Third, the []byte also avoids them having to learn how to parse x509 well. However, specifying the full x509.Certificate type might better convey what's being requested from the consumer and avoid this library from having to inform the consumer when their x509 certificate is ill-formed. On balance, the []byte seems better, but I'm open to hearing I'm wrong.

One last note: GetRenewalInfo doesn't require its request payload to be signed with the correct account key, but UpdateRenewalInfo does.

type SuggestedWindow struct {
  Start time.Time
  End time.Time
}

type RenewalInformation struct {
  SuggestedWindow SuggestedWindow
  ExplanationURL string
} 

func (c * Client) GetRenewalInfo(ctx context.Context, cert []byte) (*RenewalInformation, error)

func (c * Client) UpdateRenewalInfo(ctx context.Context, cert []byte) (error)
@gopherbot gopherbot added this to the Proposal milestone Jun 23, 2023
@ianlancetaylor
Copy link
Contributor

CC @golang/security

@FiloSottile
Copy link
Contributor

ARI is great, and we should definitely support it. What worries me is that the draft is not final and we already had one round of draft-to-RFC API churn in x/crypto/acme. Any sense how long that might take? If it's years I guess we'd have to bite the bullet.

I'd skip UpdateRenewalInfo. It's not necessary for most uses, and feels more likely to change than GetRenewalInfo. Also, I had to look at the spec to learn what it does. (Tells the server if the certificate has been renewed, and the bool SHOULD always be true, which is kinda weird.)

Nit, I wouldn't hoist SuggestedWindow out as its own type myself.

@AGWA
Copy link

AGWA commented Jul 2, 2023

Agree with @FiloSottile's points. Concretely, I would propose this API:

type RenewalInformation struct {
        WindowStart    time.Time
        WindowEnd      time.Time
        ExplanationURL string
} 

func (*Client) GetRenewalInfo(ctx context.Context, cert []byte) (*RenewalInformation, error)

I don't have a sense of how long it's going to take to finalize ARI, but I think it's likely that however it ends up, the above API will still work.

@noncombatant
Copy link

At Tailscale we have a fork of x/crypto (and a plan to upstream anything that upstream will accept). Into that fork, @awly and I added support for ARI as described in the current Internet-Draft.

Our API shape is close to what you all propose here. I do agree that it would be simpler, code-wise, to flatten the RenewalInformation struct, but that would require the JSON response as specified to be flattened, too. As specified, hoisting SuggestedWindow lets us use json.Unmarshal directly, which is nice.

Ideally, when ARI is finalized, we can all harmonize on an implementation in upstream x/crypto.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Status: Incoming
Development

No branches or pull requests

6 participants