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

crypto/tls: consider enforcing KU bits on leaf certs #71842

Open
rolandshoemaker opened this issue Feb 19, 2025 · 3 comments
Open

crypto/tls: consider enforcing KU bits on leaf certs #71842

rolandshoemaker opened this issue Feb 19, 2025 · 3 comments
Labels
LibraryProposal Issues describing a requested change to the Go standard library or x/ libraries, but not to a tool NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made.

Comments

@rolandshoemaker
Copy link
Member

The crypto/x509 package has historically ignored KU, as CAs were historically terrible at setting it correctly (see https://cs.opensource.google/go/go/+/refs/tags/go1.24.0:src/crypto/x509/verify.go;l=717;drc=468fad45a27db0ec1fff4ae397d3670795b3f977.)

As such crypto/tls also ignores it when checking if the key in the presented leaf certificate is valid for the cipher being used. This means we ignore two things: RSA keys that are marked not to be used for RSA KEXs (lacking the keyAgreement bit) and differentiating ECDH and ECDSA keys (which otherwise have the same encoding).

We should revisit adding support for this. See https://github.com/google/boringssl/blob/main/ssl/handshake_client.cc#L1360-L1376 and https://github.com/google/boringssl/blob/main/ssl/handshake_server.cc#L1517-L1524 for how BoringSSL handles this.

cc @FiloSottile @cpu

@rolandshoemaker rolandshoemaker added the NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. label Feb 19, 2025
@cpu
Copy link
Contributor

cpu commented Feb 19, 2025

It sounds reasonable to me, but I also wouldn't be surprised to find it breaks oddball certs from outside the webpki (which I assume, is tolerable to some threshold of pain given the overall scope of crypto/tls).

Would you expect to land the RSA parts behind a GODEBUG similar to the BSSL enforce_rsa_key_usage flag?

FWIW we're also not validating KU in rustls/webpki...

@gabyhelp
Copy link

Related Code Changes

(Emoji vote if this was helpful or unhelpful; more detailed feedback welcome in this discussion.)

@gabyhelp gabyhelp added the LibraryProposal Issues describing a requested change to the Go standard library or x/ libraries, but not to a tool label Feb 19, 2025
@rolandshoemaker
Copy link
Member Author

Would you expect to land the RSA parts behind a GODEBUG similar to the BSSL enforce_rsa_key_usage flag?

I expect we'd probably gate the entire KU checking on a GODEBUG (default enabled, but with the ability to turn it off.)

FWIW we're also not validating KU in rustls/webpki...

I think it's probably the right thing to do for general usage of X.509, I'm sure there are enough legacy trusted roots that mess this up still (although perhaps worth a ecosystem survey to figure out.) We'd probably want to implement this as crypto/tls specific functionality.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
LibraryProposal Issues describing a requested change to the Go standard library or x/ libraries, but not to a tool NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made.
Projects
None yet
Development

No branches or pull requests

3 participants