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: crypto/tls: supported certificate types API #32426

Closed
tmthrgd opened this issue Jun 4, 2019 · 5 comments
Closed

proposal: crypto/tls: supported certificate types API #32426

tmthrgd opened this issue Jun 4, 2019 · 5 comments
Labels
FrozenDueToAge Proposal Proposal-Accepted Proposal-Crypto Proposal related to crypto packages or other security issues
Milestone

Comments

@tmthrgd
Copy link
Contributor

tmthrgd commented Jun 4, 2019

Presently there is no simple way to determine whether a client/connection will support a particular TLS certificate type from within a GetCertificate callback.

The logic within crypto/tls is complex, subtle and hard to reason with from without crypto/tls. With the addition of Ed25519 in CL 177698, this is now even harder to get right as there are now three distinct certificate types that may be supported: RSA, ECDSA and Ed25519.

What I'd like to propose is some addition to ClientHelloInfo that would allow the GetCertificate callback to determine which certificate types are supported by the client and server for the connection. I'm not sure what an appropriate API would look like, but perhaps a method (Supports(...) bool) or struct as a field (Supports struct { RSA, ECDSA, Ed25519 bool }) or added fields (SupportsRSA bool; SupportsECDSA bool; SupportsEd25519 bool) on ClientHelloInfo.

An incomplete implementation was added to acme/autocert in CL 114501. This is documented as differing from crypto/tls (see commit message), and could cause handshake failures if there isn't a mutually supported ECDSA cipher suite. I've also tried implementing this myself using just the information in ClientHelloInfo and it is entirely non-trivial[1].

It's impossible to correctly implement this safely without support from crypto/tls because it's dependant on the configured/default cipher suites (which aren't always easily accessible) and internal crypto/tls implementation details.

This is useful for servers that might want to support several certificate types–to allow for older clients to successful connect–like acme/autocert does. For example the server may wish to prioritise Ed25519 over ECDSA and use RSA as a fallback, or more simply prefer ECDSA over RSA.

I'm not sure whether this will conflict with the recently approved proposal for #28660 (#28660 (comment)), but it's likely that will need to be considered.


[1]: To successfully negotiate an ECDSA certificate, you need a valid point format (uncompressed), a mutually supported curve, a mutually supported ECDSA cipher suite or support for TLS 1.3, and a supported ECDSA signature scheme. I believe Ed25519 support is the same except for needing the Ed25519 signature scheme.

There's also the presently unsupported RSASSA-PSS algorithms with public key OID RSASSA-PSS block of signature schemes (rsa_pss_pss_*) for RSA certificates with RSA-PSS only public keys. I presume support for that could wind up in crypto/tls one-day. And finally the signature_algorithms_cert extension may be of relevance in some cases.

@gopherbot gopherbot added this to the Proposal milestone Jun 4, 2019
@tmthrgd
Copy link
Contributor Author

tmthrgd commented Jun 20, 2019

I'm not sure exactly what the protocol for proposals is, but cc @FiloSottile.

@FiloSottile FiloSottile added the Proposal-Crypto Proposal related to crypto packages or other security issues label Jun 20, 2019
@FiloSottile
Copy link
Contributor

I feel like the most future-proof and reusable API here would be

func (*ClientHelloInfo) SupportsCertificate(*Certificate) bool
func (*CertificateRequestInfo) SupportsCertificate(*Certificate) bool

I like this because the logic is getting more complex over time, I heard requests that would be fulfilled by this multiple times, and even the crypto/tls internals would benefit from this API.

@gopherbot
Copy link

Change https://golang.org/cl/205058 mentions this issue: crypto/tls: implement (*CertificateRequestInfo).SupportsCertificate

@gopherbot
Copy link

Change https://golang.org/cl/205057 mentions this issue: crypto/tls: implement (*ClientHelloInfo).SupportsCertificate

@gopherbot
Copy link

Change https://golang.org/cl/205061 mentions this issue: crypto/tls: refactor certificate and signature algorithm logic

gopherbot pushed a commit that referenced this issue Nov 12, 2019
This refactors a lot of the certificate support logic to make it cleaner
and reusable where possible. These changes will make the following CLs
much simpler.

In particular, the heavily overloaded pickSignatureAlgorithm is gone.
That function used to cover both signing and verifying side, would work
both for pre-signature_algorithms TLS 1.0/1.1 and TLS 1.2, and returned
sigalg, type and hash.

Now, TLS 1.0/1.1 and 1.2 are differentiated at the caller, as they have
effectively completely different logic. TLS 1.0/1.1 simply use
legacyTypeAndHashFromPublicKey as they employ a fixed hash function and
signature algorithm for each public key type. TLS 1.2 is instead routed
through selectSignatureScheme (on the signing side) or
isSupportedSignatureAlgorithm (on the verifying side) and
typeAndHashFromSignatureScheme, like TLS 1.3.

On the signing side, signatureSchemesForCertificate was already version
aware (for PKCS#1 v1.5 vs PSS support), so selectSignatureScheme just
had to learn the Section 7.4.1.4.1 defaults for a missing
signature_algorithms to replace pickSignatureAlgorithm.

On the verifying side, pickSignatureAlgorithm was also checking the
public key type, while isSupportedSignatureAlgorithm +
typeAndHashFromSignatureScheme are not, but that check was redundant
with the one in verifyHandshakeSignature.

There should be no major change in behavior so far. A few minor changes
came from the refactor: we now correctly require signature_algorithms in
TLS 1.3 when using a certificate; we won't use Ed25519 in TLS 1.2 if the
client didn't send signature_algorithms; and we don't send
ec_points_format in the ServerHello (a compatibility measure) if we are
not doing ECDHE anyway because there are no mutually supported curves.

The tests also got simpler because they test simpler functions. The
caller logic switching between TLS 1.0/1.1 and 1.2 is tested by the
transcript tests.

Updates #32426

Change-Id: Ice9dcaea78d204718f661f8d60efdb408ba41577
Reviewed-on: https://go-review.googlesource.com/c/go/+/205061
Reviewed-by: Katie Hockman <katie@golang.org>
gopherbot pushed a commit that referenced this issue Nov 12, 2019
We'll also use this function for a better selection logic from
Config.Certificates in a later CL.

Updates #32426

Change-Id: Ie239574d02eb7fd2cf025ec36721c8c7e082d0bc
Reviewed-on: https://go-review.googlesource.com/c/go/+/205057
Reviewed-by: Katie Hockman <katie@golang.org>
@golang golang locked and limited conversation to collaborators Nov 11, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
FrozenDueToAge Proposal Proposal-Accepted Proposal-Crypto Proposal related to crypto packages or other security issues
Projects
None yet
Development

No branches or pull requests

4 participants