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

Closed
jameshartig opened this issue Nov 7, 2020 · 29 comments
Closed

x/crypto/acme: add ListCertAlternates #42437

jameshartig opened this issue Nov 7, 2020 · 29 comments
Labels
Milestone

Comments

@jameshartig
Copy link
Contributor

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.

@gopherbot gopherbot added this to the Proposal milestone Nov 7, 2020
@ianlancetaylor ianlancetaylor added the Proposal-Crypto Proposal related to crypto packages or other security issues label Nov 7, 2020
@ianlancetaylor
Copy link
Contributor

CC @FiloSottile @katiehockman

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

/cc @rolandshoemaker

@jameshartig
Copy link
Contributor Author

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

@rolandshoemaker any input here? Thanks!

@rsc
Copy link
Contributor

rsc commented Dec 2, 2020

ping @rolandshoemaker

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

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

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

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

@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 commented Dec 7, 2020 via email

@jameshartig
Copy link
Contributor Author

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

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

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

@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

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 commented Dec 16, 2020

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

@skoskav
Copy link

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

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

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 commented Jan 6, 2021

No change in consensus, so accepted.

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

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
@nbun
Copy link

nbun commented Aug 18, 2021

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?

It seems that the new date for this change is September 30th. In order to support devices stuck with OpenSSL 1.0.x and the DST X3 certificate, this change seems to be required to choose the shorter chain. Otherwise these devices will no longer be able to connect. Any chance that this change will be merged before that happens?

@jameshartig
Copy link
Contributor Author

@rolandshoemaker @rsc Can we get someone to review the mailed CL in hopes to get this merged before the end of the month?

@jameshartig
Copy link
Contributor Author

Actually (@nbun correct me if I'm wrong), after reading [1], it seems like nothing is changing on September 29th with the default or alternate chains. This change is still useful to fetch the shorter chain if you don't need early-Android compatibility but nothing seems to be breaking in the short-term.

[1] https://community.letsencrypt.org/t/production-chain-changes/150739

4a6f656c pushed a commit to 4a6f656c/go-crypto that referenced this issue Sep 17, 2021
Let's Encrypt is defaulting to a longer cross-signed chain on
May 4th, 2021 but will offer the ability to download the shorter chain via
an alternate URL via a link header [1]. The alternate relation is described
in section 7.4.2 of RFC 8555.

ListCertAlternates should be passed the original certificate chain URL and
will return a list of alternate chain URLs that can be passed to FetchCert
to download.

Fixes golang/go#42437

[1] https://community.letsencrypt.org/t/production-chain-changes/150739

Change-Id: Iaa32e49cb1322ac79ac1a5b4b7980d5401f4b86e
@nbun
Copy link

nbun commented Sep 17, 2021

@jameshartig There is a (admittedly rather small) group of devices that will be impacted by the change, see [1]. This is due to a bug in OpenSSL 1.0.x where the new default chain, which contains the soon to be expired DST X3 in last position, will be invalidated instead of accepting the certificate once the new and valid ISGR X1 is checked.

For devices where updating OpenSSL is not (easily) possible, being able to serve the alt chain without the invalid certificate at the end is critical.

[1] https://medium.com/geekculture/will-you-be-impacted-by-letsencrypt-dst-root-ca-x3-expiration-d54a018df257

@FiloSottile
Copy link
Contributor

OpenSSL 1.0.x has been EoL'd for almost two years now, so while we'll merge this change soon, anyone impacted by this should consider the security implications of running such an unsupported cryptography library.

@golang golang locked and limited conversation to collaborators Sep 21, 2022
LewiGoddard pushed a commit to LewiGoddard/crypto that referenced this issue Feb 16, 2023
Let's Encrypt is defaulting to a longer cross-signed chain on May 4th,
2021 but will offer the ability to download the shorter chain via an
alternate URL via a link header [1]. The shorter chain can be selected
to workaround a validation bug in legacy versions of OpenSSL, GnuTLS,
and LibreSSL. The alternate relation is described in section 7.4.2 of
RFC 8555.

ListCertAlternates should be passed the original certificate chain URL
and will return a list of alternate chain URLs that can be passed to
FetchCert to download.

Fixes golang/go#42437

[1] https://community.letsencrypt.org/t/production-chain-changes/150739

Change-Id: Iaa32e49cb1322ac79ac1a5b4b7980d5401f4b86e
Reviewed-on: https://go-review.googlesource.com/c/crypto/+/277294
Trust: Filippo Valsorda <filippo@golang.org>
Run-TryBot: Filippo Valsorda <filippo@golang.org>
Reviewed-by: Roland Shoemaker <roland@golang.org>
TryBot-Result: Go Bot <gobot@golang.org>
BiiChris pushed a commit to BiiChris/crypto that referenced this issue Sep 15, 2023
Let's Encrypt is defaulting to a longer cross-signed chain on May 4th,
2021 but will offer the ability to download the shorter chain via an
alternate URL via a link header [1]. The shorter chain can be selected
to workaround a validation bug in legacy versions of OpenSSL, GnuTLS,
and LibreSSL. The alternate relation is described in section 7.4.2 of
RFC 8555.

ListCertAlternates should be passed the original certificate chain URL
and will return a list of alternate chain URLs that can be passed to
FetchCert to download.

Fixes golang/go#42437

[1] https://community.letsencrypt.org/t/production-chain-changes/150739

Change-Id: Iaa32e49cb1322ac79ac1a5b4b7980d5401f4b86e
Reviewed-on: https://go-review.googlesource.com/c/crypto/+/277294
Trust: Filippo Valsorda <filippo@golang.org>
Run-TryBot: Filippo Valsorda <filippo@golang.org>
Reviewed-by: Roland Shoemaker <roland@golang.org>
TryBot-Result: Go Bot <gobot@golang.org>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
No open projects
Development

No branches or pull requests

9 participants