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: improve default performance of SupportsCertificate #35504

Open
FiloSottile opened this issue Nov 11, 2019 · 6 comments
Open

crypto/tls: improve default performance of SupportsCertificate #35504

FiloSottile opened this issue Nov 11, 2019 · 6 comments
Labels
Milestone

Comments

@FiloSottile
Copy link
Member

@FiloSottile FiloSottile commented Nov 11, 2019

As discussed in https://golang.org/cl/205059, SupportsCertificate requires c.Leaf to be set not to be extremely slow (because it needs to re-parse the leaf every time). This also impacts automatic selection from multiple Certificates candidates.

There are multiple solutions suggested on the CL, I will pick one and turn this into a proposal for further discussion.

@FiloSottile FiloSottile added this to the Go1.15 milestone Nov 11, 2019
@gopherbot
Copy link

@gopherbot gopherbot commented Nov 11, 2019

Change https://golang.org/cl/205059 mentions this issue: crypto/tls: select only compatible chains from Certificates

gopherbot pushed a commit that referenced this issue Nov 12, 2019
Now that we have a full implementation of the logic to check certificate
compatibility, we can let applications just list multiple chains in
Certificates (for example, an RSA and an ECDSA one) and choose the most
appropriate automatically.

NameToCertificate only maps each name to one chain, so simply deprecate
it, and while at it simplify its implementation by not stripping
trailing dots from the SNI (which is specified not to have any, see RFC
6066, Section 3) and by not supporting multi-level wildcards, which are
not a thing in the WebPKI (and in crypto/x509).

The performance of SupportsCertificate without Leaf is poor, but doesn't
affect current users. For now document that, and address it properly in
the next cycle. See #35504.

While cleaning up the Certificates/GetCertificate/GetConfigForClient
behavior, also support leaving Certificates/GetCertificate nil if
GetConfigForClient is set, and send unrecognized_name when there are no
available certificates.

Fixes #29139
Fixes #18377

Change-Id: I26604db48806fe4d608388e55da52f34b7ca4566
Reviewed-on: https://go-review.googlesource.com/c/go/+/205059
Run-TryBot: Filippo Valsorda <filippo@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Katie Hockman <katie@golang.org>
@FiloSottile FiloSottile modified the milestones: Go1.15, Backlog Mar 31, 2020
@rolandshoemaker
Copy link
Member

@rolandshoemaker rolandshoemaker commented May 19, 2020

Looking at golang.org/cl/205059 it seems like the proposed solutions were:

  1. build a fast SAN-only parser which would allow extracting the names to check against the requested server name
  2. make tls.Certificate a linked list, and continue to use Config.NameToCertificate to allow iterating through possible certificates on a per-name basis, rather than iterating through all possible certificates
  3. alter tls.LoadX509KeyPair to set Leaf itself

(1) would likely be quite fast, but introduces the need for a second special x509 parser, which while likely to be relatively self contained due to the limit of what it'd parse is still not ideal (it could also have relatively sharp edges, likely you'd need it to output a partially populated Certificate, in order to use VerifyHostname, so you'd have to be very careful not to use it anywhere else where you might expect anything other than the SAN fields to be populated).

(2) would introduce the typical problems with linked lists, circular lists, broken references, etc. This also is somewhat more likely to cause issues because you'd be relying on the user to properly build the list themselves (or use BuildNameToCertificate). Also given NameToCertificate/BuildNameToCertificate were deprecated in CL205059 it doesn't seem ideal to bring them back with new functionality in a subsequent release.

(3) would generally increase memory usage, which is unlikely to incur significant problems for most users who only have a handful of certificates, and would change documented behavior that people may have been relying on (specifically expecting Leaf not to be set). Assuming this is how most people load certificates this way this is likely the option to that'd cause the least pain without the user having to do anything in the typical case.

It seems there could also a third option, along similar lines to (3), which would be to add a once style lazy load in Certificate.leaf, where the leaf would still be parsed on first load if Leaf was nil, but instead of returning the result of the parsing directly to the caller it'd be stored and then returned. This would incur the same maximum memory footprint cost of (3), but would only load certificates as they were needed, so in theory if a server had a huge number of chains, but only actually used a handful of them they wouldn't all take up memory. Of course this could be somewhat confusing for users since memory usage would slowly rise over time as more certificates were loaded.

@rolandshoemaker
Copy link
Member

@rolandshoemaker rolandshoemaker commented May 20, 2020

Perhaps another solution would be a slight hybrid of above. Add a private SAN field to Certificate which is lazy loaded (or populated via LoadX509KeyPair) and only used when Leaf is nil. Loadx509KeyPair could then retain the property of not populating Leaf, and the overall memory footprint of storing this would be significantly smaller than storing the full certificate. A special parse wouldn't be needed because we could just use ParseCertificate and would only have to pay the penalty once per certificate.

@diogin
Copy link
Contributor

@diogin diogin commented Sep 10, 2020

I meet the same issue. Any progress on this?

@cespare
Copy link
Contributor

@cespare cespare commented Feb 16, 2021

I just ran into this as well. I noticed the deprecation warning on BuildNameToCertificate and, upon first reading the warning text, simply deleted the call. It's unfortunate that this introduces a huge performance degradation and I had to read other text elsewhere (the Go 1.14 release notes, say, or the Config.Certificates doc comment) to learn about it.

It also doesn't seem right that my code, which previously only called crypto/tls APIs, now needs to care about crypto/x509.

It also seems a little silly that it parses the certificate twice (though it was doing that before, too, via BuildNameToCertificate).

Personally @rolandshoemaker's option (3) sounds fine to me, though I guess I haven't contemplated use cases where the memory usage would matter at all. His alternative option where we lazily store Leaf seems okay too, though I'm not sure how we serialize the store to that field (since it's public and the caller could set Leaf = ... at any time).

Another (3) variation (though unpleasant) is tls.LoadX509KeyPairWithLeaves.

(1) and (2) would be okay as well, if they could be made invisible to me and were just as fast as what I'm currently doing, but they sound like an awful lot of mechanism to solve what seems like a simple problem.

@FiloSottile FiloSottile modified the milestones: Backlog, Go1.17 Mar 17, 2021
@rolandshoemaker
Copy link
Member

@rolandshoemaker rolandshoemaker commented Mar 17, 2021

Performance improvements from #44299 may magically fix this, if not it also includes a fast SAN parser we can use.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
5 participants