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

x/crypto/acme: add ListCertAlternates #42437

Open
jameshartig opened this issue Nov 7, 2020 · 24 comments
Open

x/crypto/acme: add ListCertAlternates #42437

jameshartig opened this issue Nov 7, 2020 · 24 comments

Comments

@jameshartig
Copy link
Contributor

@jameshartig jameshartig commented Nov 7, 2020

Let's Encrypt is going to stop returning the cross-signed chain by default starting January 11, 2021 and with that they expect to break 33% of Android devices (and 1-5% of web traffic). If you want to continue getting the cross-signed root in your certificate chain you have to download the alternative certificate via the "alternative" link relation.

This also has implications on the autocert package since this is probably something that should be exposed as an option (or even the default) to not cause issues come January.

An example of the response from a certificate request is:

HTTP/2 200
server: nginx
date: Sat, 07 Nov 2020 08:28:02 GMT
content-type: application/pem-certificate-chain
content-length: 3551
cache-control: public, max-age=0, no-cache
link: <https://acme-v02.api.letsencrypt.org/directory>;rel="index"
link: <https://acme-v02.api.letsencrypt.org/acme/cert/certid/1>;rel="alternate"
x-frame-options: DENY
strict-transport-security: max-age=604800

... certificate pem ...

Right now FetchCert(ctx context.Context, url string, bundle bool) ([][]byte, error) provides no way to get the underlying headers or alternative chains. There's not an API-compatible way to extend that function. There are a few ways forward:

We could allow the user to make a request to get alternative links for a certificate and then they can manually call FetchCert on each one then parse them to determine which one they care about. This would happen after calling CreateOrderCert with the URL returned from that method.

// CertAlternatives returns a slice of alternative links for the given certificate
func (c *Client) CertAlternatives(ctx context.Context, url string) ([]string, error) {
  // HEAD request on URL then parse link headers and return where rel="alternative"
}

Alternatively to the above method, instead of returning a certificate as [][]byte from CreateOrderCert and FetchCert we could wrap it in a struct:

type Cert struct {
  DER []byte
  Chain [][]byte
  URL string
  AlternativeURLs []string
}

This would also give us the flexibility in the future for convenience methods for returning a bundle (getting rid of the current boolean option) or parsing the DER and returning *x509.Certificate or adding new fields in the future. The problem is that this would break the existing API or we'd need to add new methods and I'm not sure there's an obvious nomenclature for the new names.

My preference would be the second option if we're okay breaking the API (given that it's under x/) since it provides the most flexibility going forward. This would also cause all downstream users of this library to make a conscience decision on if they want to start using the alternative chain instead with the cross-signed root. It's not clear how many users will care about the old Android versions that the new root breaks. If we expect most people will just ignore the alternative chains then it makes sense to go with option 1 instead.

@ianlancetaylor
Copy link
Contributor

@ianlancetaylor ianlancetaylor commented Nov 7, 2020

@ianlancetaylor ianlancetaylor added this to Incoming in Proposals Nov 7, 2020
@FiloSottile
Copy link
Contributor

@FiloSottile FiloSottile commented Nov 14, 2020

@jameshartig
Copy link
Contributor Author

@jameshartig jameshartig commented Nov 18, 2020

The holidays are coming up and January 11th less than 60 days away, I'm willing to work on the changes for this if we can come to an agreement of how (or if) it should be solved/fixed. I brought up 2 different proposals, anyone have any thoughts on either? How comfortable are we with breaking the API of x/crypto/acme?

@jameshartig
Copy link
Contributor Author

@jameshartig jameshartig commented Dec 1, 2020

@rolandshoemaker any input here? Thanks!

@rsc
Copy link
Contributor

@rsc rsc commented Dec 2, 2020

@rsc rsc moved this from Incoming to Active in Proposals Dec 2, 2020
@rolandshoemaker
Copy link
Member

@rolandshoemaker rolandshoemaker commented Dec 2, 2020

Sorry for the lag. Given the number of users of this package I'd prefer to not break the API if we can avoid it (I suspect also that most people are unlikely to actually care about this change, and will just continue using whatever the default LE, or any other ACME provider, sends). As such I think the first option is probably preferable here.

Unfortunately there is very little context encoded in the alternative relations, so it's kind of hard to tell which of the various URLs you actually want (i.e. is the default the right chain for me, or alternative 1, or alternative 2, etc). This is complicated by there being no guarantee that the ordering is stable.

If you care about using a specific chain it seems like you'd end up needing to fetch all of the chains available in order to pick the right one correctly, since there is no way to know which of the URLs returned by CertAlternatives is the one you want. This makes me think that perhaps a FetchAllChains(url string) ([][][]byte, error) type method is perhaps more appropriate? Although I guess we could just push off the iterating and fetching of these chains to the user.

@jameshartig
Copy link
Contributor Author

@jameshartig jameshartig commented Dec 2, 2020

Thanks for the reply! It sounds like there are only 2 possible options left.

I suspect also that most people are unlikely to actually care about this change, and will just continue using whatever the default LE, or any other ACME provider, sends

Unfortunately I don't think many people are aware of the change to the LE roots and therefore won't realize what's happening. Therefore, I agree with your suspicion to some level. There will definitely be larger or more cognizant users that will want to prevent breakage of these older clients.

If you care about using a specific chain it seems like you'd end up needing to fetch all of the chains available in order to pick the right one correctly, since there is no way to know which of the URLs returned by CertAlternatives is the one you want.

In the more general case, that makes sense. In this one specific instance though we'd always want to use the alternative. However, given that there could be many alternatives it doesn't seem like we should make the only option to fetch all of them.

Unfortunately there is very little context encoded in the alternative relations, so it's kind of hard to tell which of the various URLs you actually want (i.e. is the default the right chain for me, or alternative 1, or alternative 2, etc). This is complicated by there being no guarantee that the ordering is stable.

Maybe for LE but other providers might have useful suffixes on the URLs that might be useful to the user to understand.

Although I guess we could just push off the iterating and fetching of these chains to the user.

I think this makes sense given the above. Especially since someone could build a FetchAllCerts method as a wrapper around this method if they desire that.
Should I start to work on a CL for:

CertAlternatives(ctx context.Context, url string) ([] string, error)

If there are no alternatives should it just return nil, nil?

The above solution doesn't have any API breakage which has the sole downside that people might be surprised when there are issues with older clients. I don't necessarily think it should be on Go to let them know about that though.

@FiloSottile
Copy link
Contributor

@FiloSottile FiloSottile commented Dec 5, 2020

So this API would take the certURL from CreateOrderCert, right?

All methods are currently verbs, so we should probably follow that convention. Maybe ListCertAlternates?

@jameshartig
Copy link
Contributor Author

@jameshartig jameshartig commented Dec 5, 2020

So this API would take the certURL from CreateOrderCert, right?

Correct

All methods are currently verbs, so we should probably follow that convention. Maybe ListCertAlternates?

Good point, will do

@jameshartig
Copy link
Contributor Author

@jameshartig jameshartig commented Dec 6, 2020

@rolandshoemaker @FiloSottile so I had originally planned on making a HEAD request to the certificate URL but I'm concerned that, despite it working for Let's Encrypt, since it isn't RFC 8555 compliant it might break randomly or not work with other providers. If I follow the RFC and do a POST-as-GET then it'll get the original certificate anyways which seems like a waste to just throw it away. This is making me instead lean towards FetchAllChains but since there might be many alternatives a better option might be:

RangeAllChains(ctx context.Context, url string, f func(der [][]byte, url string) bool) error

This would first fetch the initial URL and then call the callback function. If the function returned false it would stop, otherwise continue with the next alternative chain until the callback returned false. This would allow you to keep fetching until a suitable chain was returned. For non-compliant sources we'd just fetch the initial cert internally and call the callback once.

The way that cert-manager handles this is allowing you to specify preferredChain: "DST Root CA X3" [1]. Similarly certbot has --preferred-chain option [2]. If you wanted to do something similar with RangeAllChains in your callback you'd parse the certificates and if it was a chain to the correct root you'd return false and use that chain.

[1] https://cert-manager.io/docs/configuration/acme/#use-an-alternative-certificate-chain
[2] certbot/certbot#8080

@FiloSottile
Copy link
Contributor

@FiloSottile FiloSottile commented Dec 7, 2020

@jameshartig
Copy link
Contributor Author

@jameshartig jameshartig commented Dec 7, 2020

What happens if you pass an alternate chain URL to ListCertAlternates?

It looks like Let's Encrypt returns an alternative header with a URL of the main certificate.

@rsc
Copy link
Contributor

@rsc rsc commented Dec 9, 2020

Thanks for the good discussion. I hope it continues. Please try to converge to a crisp statement of what the new API (or, less great, API change) is. Thanks.

@rolandshoemaker
Copy link
Member

@rolandshoemaker rolandshoemaker commented Dec 9, 2020

I see the redundancy issue, but I feel like the certificate response body
is not large enough to justify the more complex API.

Agreed.

What happens if you pass an alternate chain URL to ListCertAlternates?

It looks like Let's Encrypt returns an alternative header with a URL of the main certificate.

This is unfortunately underspecified, but I'd assume implementations would typically return all chains that aren't the one being requested, as the LE one does.

@FiloSottile
Copy link
Contributor

@FiloSottile FiloSottile commented Dec 9, 2020

What happens if you pass an alternate chain URL to ListCertAlternates?

It looks like Let's Encrypt returns an alternative header with a URL of the main certificate.

That's good, so all certURLs work the same.

I think my suggestion is

// ListCertAlternates return alternate certificate chain URLs for an already issued certificate.
func (c *Client) ListCertAlternates(ctx context.Context, url string) ([]string, error)
@jameshartig
Copy link
Contributor Author

@jameshartig jameshartig commented Dec 9, 2020

Yep! I have a change almost ready to mail just working on tests. The actual change is pretty small.

I'm not sure how the proposal gets accepted or what the process is for that specifically but this seems to be the consensus:

// ListCertAlternates return alternate certificate chain URLs for an already issued certificate.
func (c *Client) ListCertAlternates(ctx context.Context, url string) ([]string, error)

This method accepts a certificate URL and then returns a slice of alternate URLs or an empty slice of there are none. It assumes you're using an RFC-compliant provider since there's no concept of "alternate" URLs previously.

@rolandshoemaker?

@FiloSottile
Copy link
Contributor

@FiloSottile FiloSottile commented Dec 11, 2020

@jameshartig You can go ahead and mail a CL! The proposal will transition to likely accept and then to accepted, there is nothing you need to do on that side. We can do the code review in parallel with the proposal approval.

@gopherbot
Copy link

@gopherbot gopherbot commented Dec 12, 2020

Change https://golang.org/cl/277294 mentions this issue: acme: implement ListCertAlternates

@rsc rsc changed the title proposal: x/crypto/acme: Add support for alternative chains to support cross-signed root with Let's Encrypt proposal: x/crypto/acme: add ListCertAlternates Dec 16, 2020
@rsc
Copy link
Contributor

@rsc rsc commented Dec 16, 2020

Based on the discussion above, this seems like a likely accept.

@skoskav
Copy link

@skoskav skoskav commented Dec 22, 2020

Hello friends. The default certificate chain switch by Let's Encrypt on January 11th is approaching fast. Is there still a reasonable chance that this will be merged and released before that?

@FiloSottile
Copy link
Contributor

@FiloSottile FiloSottile commented Dec 22, 2020

The chain switch has been postponed and it significantly changed in nature: the new chain will be compatible with old and new devices, and the only reason to use the alternate will be in order to drop compatibility is exchange for a shorter chain.

https://letsencrypt.org/2020/12/21/extending-android-compatibility.html

We'll still land this change, but there is no rush now.

@jameshartig
Copy link
Contributor Author

@jameshartig jameshartig commented Dec 22, 2020

Specifically the change was pushed back to June:

Prior to that, perhaps as early as June 2021, we will be making a similar change to what we intended to make this January. When we make that change, subscribers will have the option to continue using DST Root CA X3 by configuring their ACME client to specifically request it.

Also, the alternate chain will still be offered if you don't want the new longer chain.

We currently provide the option of getting the chain: Subscriber Certificate < – R3 < – ISRG Root X1 We will continue to offer this same chain as an alternate.

@rsc
Copy link
Contributor

@rsc rsc commented Jan 6, 2021

No change in consensus, so accepted.

@rsc rsc moved this from Likely Accept to Accepted in Proposals Jan 6, 2021
@rsc
Copy link
Contributor

@rsc rsc commented Jan 6, 2021

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

@rsc rsc changed the title proposal: x/crypto/acme: add ListCertAlternates x/crypto/acme: add ListCertAlternates Jan 6, 2021
@rsc rsc modified the milestones: Proposal, Backlog Jan 6, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Proposals
Accepted
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
8 participants
@rsc @jameshartig @FiloSottile @ianlancetaylor @rolandshoemaker @skoskav @gopherbot and others