[Security] Supporting negotiation of certs based on signing algorithm - #9321
[Security] Supporting negotiation of certs based on signing algorithm#9321gtcooke94 wants to merge 12 commits into
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #9321 +/- ##
==========================================
+ Coverage 83.11% 83.14% +0.02%
==========================================
Files 423 423
Lines 35231 35269 +38
==========================================
+ Hits 29284 29326 +42
+ Misses 4432 4430 -2
+ Partials 1515 1513 -2 🚀 New features to boost your workflow:
|
|
@gtcooke94 : Could you please get someone from the security team to review this before the grpc team starts looking at this. Also, is there a gRFC/design that could be linked in the PR description. It is currently quite empty. |
easwars
added
Type: Feature
|
Will get the security team to review soon once I finalize a few items There's no public API change for this, but it's pre-work for post-quantum authenticity support. In fact, it's more of a bug in how we were handling things before, it's just that the use-case of negotiating the signature algorithm was never really used. |
…made in advancedtls
| if o.IdentityOptions.GetIdentityCertificatesForServer == nil { | ||
| var certificates []*tls.Certificate | ||
| switch { | ||
| case o.IdentityOptions.GetIdentityCertificatesForServer != nil: |
There was a problem hiding this comment.
To confirm my understanding, there is no production risk associated with this change because the current code requires that o.IdentityOptions.GetIdentityCertificatesForServer is non-nil. And now we are just allowing it to be nil. Is that correct?
There was a problem hiding this comment.
Let's discuss in the other comment - this diff looks like more than it is because of a moved switch statement
| if o.IdentityOptions.nonNilFieldCount() == 0 { | ||
| return nil, fmt.Errorf("needs to specify at least one field in IdentityCertificateOptions") | ||
| } | ||
| if o.IdentityOptions.Certificates != nil { |
There was a problem hiding this comment.
I don't understand how the changes to advancedtls.go and sni.go combine to enable negotiation of certs based on signing algorithm. Can you help explain this?
There was a problem hiding this comment.
In advancedtls.go and sni.go, we now call buildGetCertificates unconditionally on the server side as this is where we select the certificate based on the desired SNI and signature algorithm.
Previously, if Certificates was provided directly, we would not call buildGetCertificates.
Because we call unconditionally now, I moved the core switch impl into buildGetCertificates so we don't have to call it in each switch block in advancedtls.
buildGetCertificates calls clientHello.SupportsCertificate(cert) on each certificate which is what does the selection/negotiation, returning the first that is supported based on the clientHello (which includes checking signing algorithm).
Fundamentally, this already worked in the case of the identity provider and providing a function. However, if Certificates was provided directly, then this codepath would've been skipped (because buildGetCertificates wasn't callled)
4 participants
This PR fixes issues where gRPC-Go would not correctly support the negotiation of certs based on signing algorithm when multiple are provided. There is no API change.
In advancedtls.go and sni.go, we now call
buildGetCertificatesunconditionally as this is where we select the certificate based on the desired SNI and signature algorithm.