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/autocert: support ECDSA on TLS1.3-only connections #50522
Comments
@FiloSottile can you give this a look? |
/cc @golang/security |
I looked into this some more. I tried verifying that I would indeed get a error connecting with TLS1.3 with ECDSA only. I wanted to configure a tls.Config with only ECDSA as signature scheme. It looks like tls.Config does not allow setting signature schemes. I don't know if this is due to TLS1.3 the specification, or the Go implementation. I did notice this comment for CipherSuites: "Note that TLS 1.3 ciphersuites are not configurable.". I know modern crypto aims for fewer options/negotations. Perhaps RSA support is always required for TLS1.3. If so, the claim the title of this issue is incorrect (I extrapolated it from what I saw with autocert). At least it is clear that my knowledge about TLS1.3 is lacking. But there is more. I wrote this program to connect to my autocert server. To see what the tls.ClientHelloInfo looks like, and if connection succeeds:
It resulted in this tls.ClientHelloInfo:
I am surprised that cipher suites for TLS <=1.2 appear in the list for a TLS1.3-only connection. I tested with go1.17.6 and go1.18beta1, same result. I thought may be the crypto/tls server is inserting them. To check that, I tried with a different ecosystem:
No TLS<=1.2 cipher suites this time. So it looks like the crypto/tls client inserts the TLS<=1.2 cipher suites in a TLS1.3-only connection. And that causes the supportsECDSA check in autocert to succeed. The request made by curl results in supportECDSA returning false, so the (successful) connection uses RSA, not ECDSA. In the ClientHelloInfo's above, the signature schemes are different. So I'm concluding that it signature schemes are configurable with TLS1.3. But RSA support may always be required, and "ECDSA-only TLS1.3 connections" may not be a thing. For reference, here is the acmecert server I used for testing with https://github.com/letsencrypt/pebble:
And go.mod:
|
An ECDSA TLS1.3 connection indeed fails:
Logging from acmecert as posted above (with an additional log line added to "supportsECDSA", according to current detection of support):
|
supportsECDSA is indeed an extremely limited and partial check. The assumption is that RSA is always the safe fallback, too, which is not necessarily true. I added ClientHelloInfo.SupportsCertificate in Go 1.14 to address this. It's been in the standard library long enough that we should just switch autocert to it and delete supportsECDSA. |
Latest version of x/crypto/acme/autocert at the time of writing:
https://pkg.go.dev/golang.org/x/crypto@v0.0.0-20211215153901-e495a2d5b3d3/acme/autocert
What did you do?
Call Manager.GetCertificate to force an immediate retrieval of missing certificates (during application startup).
What did you expect to see?
I expected autocert to fetch an ECDSA certificate.
What did you see instead?
An RSA certificate was fetched instead.
Then, the first time I actually connect to my service (using standard tools like curl and openssl s_client), an ECDSA certificate was fetched. Because ECDSA is typically preferred over RSA by such tools.
Analysis
My initial tls.ClientHelloInfo had only ServerName set, no ciphersuites, versions, etc. This (correctly) fails the supportsECDSA check at: https://cs.opensource.google/go/x/crypto/+/e495a2d5:acme/autocert/autocert.go;drc=e495a2d5b3d3be43468d0ebb413f46eeaedf7eb3;l=322
That's fair, so I tried with an
hello
that supports only ECDSA:However, that still results in an RSA certificate being fetched: supportsECDSA does not take TLS1.3 configs into account, requiring an ECDSA-supporting ciphersuite for TLS <= 1.2. See: https://cs.opensource.google/go/x/crypto/+/e495a2d5:acme/autocert/autocert.go;l=353;drc=e495a2d5b3d3be43468d0ebb413f46eeaedf7eb3
Background info: With TLS1.3, the signature schemes have been taken/separated out of CipherSuite definitions.
I believe supportsECDSA should be updated to recognize TLS1.3, with a check for ECDSA in SignatureSchemes.
Furthermore, I believe supportsECDSA should be modified to also check for RSA support in the ClientHelloInfo: There is no point in fetching an RSA certificate if the client cannot use it. I understand RSA used to be the default signature scheme. But it may not be in the future, and clients are already free to select the signature schemes they wish to use.
I can work around my immediate issue (forcing a fetch of an ECDSA certificate) by simply including tls.TLS_ECDHE_ECDSA_WITH_AES_128_GCM_SHA256 in the CipherSuites slice. Then it passes the supportsECDSA check. But actual TLS1.3-only connections will still fetch an RSA certificate, not an ECDSA certificate, even if RSA isn't supported by the client.
I can write a patch if this sounds reasonable.
Another idea: We could change Manager.GetCertificate to be satisfied with an existing RSA certificate in case of a non-existing ECDSA certificate. We would have to be careful that the supported RSA-using-cipher-suites aren't otherwise weaker than the ECDSA cipher suites. That's why I'm not sure it is worth it. But if we were to expand the signature-scheme-compatibility to check for RSA too, perhaps it is worth it.
The text was updated successfully, but these errors were encountered: