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: set Certificate.Leaf from [Load]X509KeyPair #67065

Open
FiloSottile opened this issue Apr 26, 2024 · 2 comments
Open

proposal: crypto/tls: set Certificate.Leaf from [Load]X509KeyPair #67065

FiloSottile opened this issue Apr 26, 2024 · 2 comments
Labels
Proposal Proposal-Crypto Proposal related to crypto packages or other security issues Proposal-FinalCommentPeriod
Milestone

Comments

@FiloSottile
Copy link
Contributor

LoadX509KeyPair and X509KeyPair are documented to return a tls.Certificate with a nil Leaf *x509.Certificate field.

// X509KeyPair parses a public/private key pair from a pair of
// PEM encoded data. On successful return, Certificate.Leaf will be nil because
// the parsed form of the certificate is not retained.
func X509KeyPair(certPEMBlock, keyPEMBlock []byte) (Certificate, error)

This was done intentionally out of concern that servers that manage a lot of certificates would be wasting significant amounts of memory to keep the parsed version of the certificates around.

Originally, the parsed certificate was rarely used in crypto/tls. However, the new automatic certificate selection also needs that, causing significant slowdown if the certificates need to be re-parsed every time. See #35504.

Moreover, it seems that despite the documentation note, Leaf being nil catches some users by surprise. #35504 (comment)

I believe setting Leaf in LoadX509KeyPair and X509KeyPair would be a more natural API, and servers that load very large amounts of certificates and need to save memory can explicitly set it to nil.

Now that we have GODEBUGs, I think we should make the change. It's very unlikely there are programs that will break because a field is not nil.

/cc @golang/security @golang/proposal-review

@FiloSottile FiloSottile added Proposal Proposal-Crypto Proposal related to crypto packages or other security issues labels Apr 26, 2024
@FiloSottile FiloSottile added this to the Proposal milestone Apr 26, 2024
@rsc
Copy link
Contributor

rsc commented May 8, 2024

This seems minor enough to move to likely accept, especially with a GODEBUG.

@rsc
Copy link
Contributor

rsc commented May 8, 2024

Based on the discussion above, this proposal seems like a likely accept.
— rsc for the proposal review group

The proposal is to start including the Leaf field in the Certificate returned by LoadX509KeyPair and X509KeyPair.

A new GODEBUG setting will control the behavior. GODEBUG=x509keypairleaf=1 (the new default for go 1.23+ main modules) means generate the leaf; GODEBUG=x509keypairleaf=0 means don’t.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Proposal Proposal-Crypto Proposal related to crypto packages or other security issues Proposal-FinalCommentPeriod
Projects
Status: Likely Accept
Development

No branches or pull requests

2 participants