xds: Add SNI related field in handshake info#8965
xds: Add SNI related field in handshake info#8965eshitachandwani wants to merge 6 commits intogrpc:masterfrom
Conversation
eshitachandwani
added
Type: Feature
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #8965 +/- ##
==========================================
- Coverage 83.26% 83.10% -0.16%
==========================================
Files 410 411 +1
Lines 32576 32744 +168
==========================================
+ Hits 27123 27213 +90
- Misses 4062 4151 +89
+ Partials 1391 1380 -11
🚀 New features to boost your workflow:
|
We probably don't want to do that until interop tests pass. |
| sanMatchers []matcher.StringMatcher // Only on the client side. | ||
| requireClientCert bool // Only on server side. | ||
| sni string // Only on client side, used for Server Name Indication in TLS handshake. | ||
| autoSniSanValidation bool // Only on client side, indicates whether to perform validation of SANs based on SNI value. |
There was a problem hiding this comment.
Nit: In autoSniSanValidation, the Sni and San need to all caps. But that will make the field name be autoSNISANValidation which is not very readable. So, I think you need to get a little creative here.
Maybe sanValidationUsingSNI or something better?
There was a problem hiding this comment.
Changed to validateSANUsingSNI. Let me know if this sounds good.
| // NewHandshakeInfo returns a new handshake info configured with the provided | ||
| // options. | ||
| func NewHandshakeInfo(rootProvider certprovider.Provider, identityProvider certprovider.Provider, sanMatchers []matcher.StringMatcher, requireClientCert bool) *HandshakeInfo { | ||
| func NewHandshakeInfo(rootProvider certprovider.Provider, identityProvider certprovider.Provider, sanMatchers []matcher.StringMatcher, requireClientCert bool, sni string, autoSniSanValidation bool) *HandshakeInfo { |
There was a problem hiding this comment.
Same here with the autoSniSanValidation parameter?
| } | ||
| return fmt.Errorf("xds: received DNS SANs: %v do not match the SNI: %v", certs[0].DNSNames, hi.sni) | ||
| } | ||
| // The SANs sent by the MeshCA are encoded as SPIFFE IDs. We need to |
There was a problem hiding this comment.
Nit: While you are here, could you please remove mentions of MeshCA. That is not something that we use anymore. The SANs are sent to us by the xDS control plane.
There was a problem hiding this comment.
Changed the comment to say that.
| return nil | ||
| } | ||
| } | ||
| return fmt.Errorf("xds: received DNS SANs: %v do not match the SNI: %v", certs[0].DNSNames, hi.sni) |
There was a problem hiding this comment.
Please ensure that these %vs actually are readable in the error message.
There was a problem hiding this comment.
verified that it is readable
| if _, err := certs[0].Verify(opts); err != nil { | ||
| return err | ||
| } | ||
| if envconfig.XDSSNIEnabled && hi.autoSniSanValidation && hi.sni != "" { |
There was a problem hiding this comment.
This is the sentence from the gRFC:
Server SAN validation against SNI used: If auto_sni_san_validation is true in the UpstreamTlsContext gRPC client will perform matching for a SAN against the SNI used for the handshake if any. If auto_sni_san_validation is true but no SNI was sent, then validation will use any SAN matchers specified in the validation context instead. While XdsChannelCredentials without auto_sni_san_validation performs matching using any of DNS / URI / IPA SAN matchers specified in the validation context, when auto_sni_san_validation is set, validation will be performed using exact DNS matcher.
Based on the above, it is not clear to me whether we should perform matching against any of DNS/URI/IP SAN matchers specified in the validation context or should the validation be performed against exact DNS matcher, when auto_sni_san_validation is true but no SNI was sent.
Could you please confirm what other implementations do?
There was a problem hiding this comment.
From my discussion with Kannan , we should fallback to the matchers provided and continue doing what was being done earlier, and so does Envoy as mentioned here in chat
credentials/xds/xds_client_test.go
Outdated
| autoSniSanValidation bool | ||
| enableSniFlag bool |
There was a problem hiding this comment.
Nit: Go initialisms here as well.
Got it! Changed the description too. |
3 participants
This PR is part of A101 implementation.
This PR does the following changes:
sniandautoSniSanValidationfield to handshake info.sniis present (currently set as empty in handshake so will not be set).autoSniSanValidationis true (currently set to false by default).snito empty andautoSniSanValidationto false by default when creating handshake info inclusterimplIn the next PR :
clusterimplbalancer.RELEASE NOTES: None