Skip to content

Commit 7ea4744

Browse files
xds: Introduce flag for fallback to use the xds channel authority if no SNI is determined to be used. (#12422)
This is to allow the previous behavior if needed, and when the xds channel authority is used as the SNI, it won't be used for the SAN validation.
1 parent ab20a12 commit 7ea4744

File tree

9 files changed

+39
-20
lines changed

9 files changed

+39
-20
lines changed

netty/src/main/java/io/grpc/netty/ProtocolNegotiators.java

Lines changed: 0 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -634,9 +634,6 @@ static final class ClientTlsHandler extends ProtocolNegotiationHandler {
634634
X509TrustManager x509TrustManager) {
635635
super(next, negotiationLogger);
636636
this.sslContext = Preconditions.checkNotNull(sslContext, "sslContext");
637-
// TODO: For empty authority and fallback flag
638-
// GRPC_USE_CHANNEL_AUTHORITY_IF_NO_SNI_APPLICABLE present, we should parse authority
639-
// but prevent it from being used for SAN validation in the TrustManager.
640637
if (authority != null) {
641638
HostPort hostPort = parseAuthority(authority);
642639
this.host = hostPort.host;

xds/src/main/java/io/grpc/xds/internal/security/DynamicSslContextProvider.java

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -44,6 +44,7 @@ public abstract class DynamicSslContextProvider extends SslContextProvider {
4444
@Nullable protected final CertificateValidationContext staticCertificateValidationContext;
4545
@Nullable protected AbstractMap.SimpleImmutableEntry<SslContext, X509TrustManager>
4646
sslContextAndTrustManager;
47+
protected boolean autoSniSanValidationDoesNotApply;
4748

4849
protected DynamicSslContextProvider(
4950
BaseTlsContext tlsContext, CertificateValidationContext staticCertValidationContext) {
@@ -59,6 +60,10 @@ protected DynamicSslContextProvider(
5960

6061
protected abstract CertificateValidationContext generateCertificateValidationContext();
6162

63+
public void setAutoSniSanValidationDoesNotApply() {
64+
autoSniSanValidationDoesNotApply = true;
65+
}
66+
6267
/** Gets a server or client side SslContextBuilder. */
6368
protected abstract AbstractMap.SimpleImmutableEntry<SslContextBuilder, X509TrustManager>
6469
getSslContextBuilderAndTrustManager(

xds/src/main/java/io/grpc/xds/internal/security/SecurityProtocolNegotiators.java

Lines changed: 15 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -194,6 +194,7 @@ static final class ClientSecurityHandler
194194
private final GrpcHttp2ConnectionHandler grpcHandler;
195195
private final SslContextProviderSupplier sslContextProviderSupplier;
196196
private final String sni;
197+
private final boolean autoSniSanValidationDoesNotApply;
197198

198199
ClientSecurityHandler(
199200
GrpcHttp2ConnectionHandler grpcHandler,
@@ -215,10 +216,19 @@ public void handlerAdded(ChannelHandlerContext ctx) throws Exception {
215216
EnvoyServerProtoData.BaseTlsContext tlsContext = sslContextProviderSupplier.getTlsContext();
216217
UpstreamTlsContext upstreamTlsContext = ((UpstreamTlsContext) tlsContext);
217218
if (CertificateUtils.isXdsSniEnabled) {
218-
sni = upstreamTlsContext.getAutoHostSni() && !Strings.isNullOrEmpty(endpointHostname)
219+
String sniToUse = upstreamTlsContext.getAutoHostSni()
220+
&& !Strings.isNullOrEmpty(endpointHostname)
219221
? endpointHostname : upstreamTlsContext.getSni();
222+
if (sniToUse.isEmpty() && CertificateUtils.useChannelAuthorityIfNoSniApplicable) {
223+
sniToUse = grpcHandler.getAuthority();
224+
autoSniSanValidationDoesNotApply = true;
225+
} else {
226+
autoSniSanValidationDoesNotApply = false;
227+
}
228+
sni = sniToUse;
220229
} else {
221230
sni = grpcHandler.getAuthority();
231+
autoSniSanValidationDoesNotApply = false;
222232
}
223233
}
224234

@@ -260,8 +270,8 @@ public void updateSslContextAndExtendedX509TrustManager(
260270
public void onException(Throwable throwable) {
261271
ctx.fireExceptionCaught(throwable);
262272
}
263-
}
264-
);
273+
},
274+
autoSniSanValidationDoesNotApply);
265275
}
266276

267277
@Override
@@ -399,8 +409,8 @@ public void updateSslContextAndExtendedX509TrustManager(
399409
public void onException(Throwable throwable) {
400410
ctx.fireExceptionCaught(throwable);
401411
}
402-
}
403-
);
412+
},
413+
false);
404414
}
405415
}
406416
}

xds/src/main/java/io/grpc/xds/internal/security/SslContextProviderSupplier.java

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -55,12 +55,15 @@ public BaseTlsContext getTlsContext() {
5555

5656
/** Updates SslContext via the passed callback. */
5757
public synchronized void updateSslContext(
58-
final SslContextProvider.Callback callback) {
58+
final SslContextProvider.Callback callback, boolean autoSniSanValidationDoesNotApply) {
5959
checkNotNull(callback, "callback");
6060
try {
6161
if (!shutdown) {
6262
if (sslContextProvider == null) {
6363
sslContextProvider = getSslContextProvider();
64+
if (tlsContext instanceof UpstreamTlsContext && autoSniSanValidationDoesNotApply) {
65+
((DynamicSslContextProvider) sslContextProvider).setAutoSniSanValidationDoesNotApply();
66+
}
6467
}
6568
}
6669
// we want to increment the ref-count so call findOrCreate again...

xds/src/main/java/io/grpc/xds/internal/security/certprovider/CertProviderClientSslContextProvider.java

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -64,13 +64,15 @@ final class CertProviderClientSslContextProvider extends CertProviderSslContextP
6464
new XdsTrustManagerFactory(
6565
savedSpiffeTrustMap,
6666
certificateValidationContext,
67-
((UpstreamTlsContext) tlsContext).getAutoSniSanValidation()));
67+
autoSniSanValidationDoesNotApply
68+
? false : ((UpstreamTlsContext) tlsContext).getAutoSniSanValidation()));
6869
} else if (savedTrustedRoots != null) {
6970
sslContextBuilder = sslContextBuilder.trustManager(
7071
new XdsTrustManagerFactory(
7172
savedTrustedRoots.toArray(new X509Certificate[0]),
7273
certificateValidationContext,
73-
((UpstreamTlsContext) tlsContext).getAutoSniSanValidation()));
74+
autoSniSanValidationDoesNotApply
75+
? false : ((UpstreamTlsContext) tlsContext).getAutoSniSanValidation()));
7476
} else {
7577
// Should be impossible because of the check in CertProviderClientSslContextProviderFactory
7678
throw new IllegalStateException("There must be trusted roots or a SPIFFE trust map");

xds/src/main/java/io/grpc/xds/internal/security/trust/CertificateUtils.java

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -31,6 +31,8 @@
3131
*/
3232
public final class CertificateUtils {
3333
public static boolean isXdsSniEnabled = GrpcUtil.getFlag("GRPC_EXPERIMENTAL_XDS_SNI", false);
34+
public static boolean useChannelAuthorityIfNoSniApplicable
35+
= GrpcUtil.getFlag("GRPC_USE_CHANNEL_AUTHORITY_IF_NO_SNI_APPLICABLE", false);
3436

3537
/**
3638
* Generates X509Certificate array from a file on disk.

xds/src/test/java/io/grpc/xds/XdsClientWrapperForServerSdsTestMisc.java

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -301,7 +301,7 @@ public void releaseOldSupplierOnTemporaryError_noClose() throws Exception {
301301
private void callUpdateSslContext(SslContextProviderSupplier sslContextProviderSupplier) {
302302
assertThat(sslContextProviderSupplier).isNotNull();
303303
SslContextProvider.Callback callback = mock(SslContextProvider.Callback.class);
304-
sslContextProviderSupplier.updateSslContext(callback);
304+
sslContextProviderSupplier.updateSslContext(callback, false);
305305
}
306306

307307
private void sendListenerUpdate(

xds/src/test/java/io/grpc/xds/internal/security/SecurityProtocolNegotiatorsTest.java

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -215,7 +215,7 @@ public void updateSslContextAndExtendedX509TrustManager(
215215
protected void onException(Throwable throwable) {
216216
future.set(throwable);
217217
}
218-
});
218+
}, false);
219219
assertThat(executor.runDueTasks()).isEqualTo(1);
220220
channel.runPendingTasks();
221221
Object fromFuture = future.get(2, TimeUnit.SECONDS);
@@ -401,7 +401,7 @@ public void updateSslContextAndExtendedX509TrustManager(
401401
protected void onException(Throwable throwable) {
402402
future.set(throwable);
403403
}
404-
});
404+
}, false);
405405
channel.runPendingTasks(); // need this for tasks to execute on eventLoop
406406
assertThat(executor.runDueTasks()).isEqualTo(1);
407407
Object fromFuture = future.get(2, TimeUnit.SECONDS);
@@ -540,7 +540,7 @@ public void updateSslContextAndExtendedX509TrustManager(
540540
protected void onException(Throwable throwable) {
541541
future.set(throwable);
542542
}
543-
});
543+
}, false);
544544
executor.runDueTasks();
545545
channel.runPendingTasks(); // need this for tasks to execute on eventLoop
546546
Object fromFuture = future.get(5, TimeUnit.SECONDS);

xds/src/test/java/io/grpc/xds/internal/security/SslContextProviderSupplierTest.java

Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -69,7 +69,7 @@ private void prepareSupplier() {
6969
private void callUpdateSslContext() {
7070
mockCallback = mock(SslContextProvider.Callback.class);
7171
doReturn(mockExecutor).when(mockCallback).getExecutor();
72-
supplier.updateSslContext(mockCallback);
72+
supplier.updateSslContext(mockCallback, false);
7373
}
7474

7575
@Test
@@ -94,7 +94,7 @@ public void get_updateSecret() {
9494
verify(mockTlsContextManager, times(1))
9595
.releaseClientSslContextProvider(eq(mockSslContextProvider));
9696
SslContextProvider.Callback mockCallback = mock(SslContextProvider.Callback.class);
97-
supplier.updateSslContext(mockCallback);
97+
supplier.updateSslContext(mockCallback, false);
9898
verify(mockTlsContextManager, times(3))
9999
.findOrCreateClientSslContextProvider(eq(upstreamTlsContext));
100100
}
@@ -121,7 +121,7 @@ public void autoHostSniFalse_usesSniFromUpstreamTlsContext() {
121121
verify(mockTlsContextManager, times(1))
122122
.releaseClientSslContextProvider(eq(mockSslContextProvider));
123123
SslContextProvider.Callback mockCallback = mock(SslContextProvider.Callback.class);
124-
supplier.updateSslContext(mockCallback);
124+
supplier.updateSslContext(mockCallback, false);
125125
verify(mockTlsContextManager, times(3))
126126
.findOrCreateClientSslContextProvider(eq(upstreamTlsContext));
127127
}
@@ -178,7 +178,7 @@ public void systemRootCertsWithMtls_callbackExecutedFromProvider() {
178178
verify(mockTlsContextManager, times(1))
179179
.releaseClientSslContextProvider(eq(mockSslContextProvider));
180180
SslContextProvider.Callback mockCallback = mock(SslContextProvider.Callback.class);
181-
supplier.updateSslContext(mockCallback);
181+
supplier.updateSslContext(mockCallback, false);
182182
verify(mockTlsContextManager, times(3))
183183
.findOrCreateClientSslContextProvider(eq(upstreamTlsContext));
184184
}
@@ -190,7 +190,7 @@ public void testClose() {
190190
supplier.close();
191191
verify(mockTlsContextManager, times(1))
192192
.releaseClientSslContextProvider(eq(mockSslContextProvider));
193-
supplier.updateSslContext(mockCallback);
193+
supplier.updateSslContext(mockCallback, false);
194194
verify(mockTlsContextManager, times(3))
195195
.findOrCreateClientSslContextProvider(eq(upstreamTlsContext));
196196
verify(mockTlsContextManager, times(1))

0 commit comments

Comments
 (0)