xds: xDS based SNI setting and SAN validation#12378
xds: xDS based SNI setting and SAN validation#12378kannanjgithub merged 86 commits intogrpc:masterfrom
Conversation
…e for SNI in ProtocolNegotiators.java
…channel authority itself.
…ootcerts-ignore-trusted-root-updates
…m-root-changes-needed # Conflicts: # xds/src/main/java/io/grpc/xds/internal/security/SslContextProviderSupplier.java # xds/src/test/java/io/grpc/xds/CdsLoadBalancer2Test.java # xds/src/test/java/io/grpc/xds/internal/security/SslContextProviderSupplierTest.java # xds/src/test/java/io/grpc/xds/internal/security/certprovider/CertProviderClientSslContextProviderTest.java
…lier and handle it in
|
I have done the changes discussed, but am yet to fix tests including compilation errors in a few of them. |
ejona86
left a comment
ejona86
left a comment
There was a problem hiding this comment.
About to get on a call, so sending what I have. This looks a lot better; I don't really have any reservations from what I've seen.
| HostPort hostPort = parseAuthority(authority); | ||
| this.host = hostPort.host; | ||
| this.port = hostPort.port; | ||
| // TODO: For empty authority and fallback flag |
There was a problem hiding this comment.
Half-baked thought (I haven't double checked with the expected behavior): it seems we should just pass sni = null here instead of empty string when GRPC_USE_CHANNEL_AUTHORITY_IF_NO_SNI_APPLICABLE is set. If the XDS config is saying to validate the SAN from the SNI and it also dosen't have a hostname, then that's a rather minor case that might pass when it should fail.
There was a problem hiding this comment.
We could either pass empty SNI here and change it to null when fallback flag is set (which is what I was going to do), or we could do that in the calling site at ClientTlsProtocolNegotiator itself and pass a null SNI in that case. The bigger question is at the time of SAN validation do we get the xDS channel authority from the SsllParameters.getServerNames(), and if so we should not use it for the SAN validation, and that would require setting some state in the XdsX509TrustManager.
ejona86
left a comment
ejona86
left a comment
There was a problem hiding this comment.
Moving ATTR_ADDRESS_NAME out of InternalXdsAttributes looks to be main thing. Everything else is small.
|
|
||
| private List<StringMatcher> getAutoSniSanMatchers(SSLParameters sslParams) { | ||
| List<StringMatcher> sniNamesToMatch = new ArrayList<>(); | ||
| if (CertificateUtils.isXdsSniEnabled && autoSniSanValidation) { |
There was a problem hiding this comment.
FYI: we strongly want to avoid checks like this deep in the code. It is very easy to miss a spot and generally wrong because verification checks should be impacted (e.g., XdsClusterResource.validateUpstreamTlsContext()). Instead, we'd want to handle this when parsing (XdsClusterResource) and then force autoSniSanValidation = false when CertificateUtils.isXdsSniEnabled == false.
Given this code is already failing to copy into our own data structures, it is sort of annoying to fix here. But keep it in mind in general.
There was a problem hiding this comment.
Acknowledged.
…er review comments.
…e it is causing package leakage check to fail.
…l to avoid the package leak.
| */ | ||
| public static InternalProtocolNegotiator.ProtocolNegotiator tls(SslContext sslContext) { | ||
| return tls(sslContext, null, Optional.absent()); | ||
| public static InternalProtocolNegotiator.ProtocolNegotiator tls( |
There was a problem hiding this comment.
Is this not an api breaking change? Should we do function overloading here?
There was a problem hiding this comment.
It is in an Internal* class, so we are free to make the change. Using overloads is convenient to avoid breaking usages. In this case I expect Kannan thought he was updating all usages (because the usages themselves were the cause of adding the new argument).
FWIW, I also contacted security team and they're looking at deleting the unused internal copy of s2a.
When using xDS credentials make SNI for the Tls handshake to be configured via xDS, rather than use the channel authority as the SNI, and make SAN validation to be able to use the SNI sent when so instructed via xDS. Implements A101.
When using xDS credentials make SNI for the Tls handshake to be configured via xDS, rather than use the channel authority as the SNI, and make SAN validation to be able to use the SNI sent when so instructed via xDS. Implements A101.
3 participants
When using xDS credentials make SNI for the Tls handshake to be configured via xDS, rather than use the channel authority as the SNI, and make SAN validation to be able to use the SNI sent when so instructed via xDS.
Implements A101.