Skip to content

Commit 2985cc3

Browse files
kannanjgithubkannanjgithub
committed
Fixes.
1 parent 2f5ba5d commit 2985cc3

File tree

8 files changed

+103
-45
lines changed

8 files changed

+103
-45
lines changed

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

Lines changed: 16 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -21,6 +21,7 @@
2121
import com.google.common.annotations.VisibleForTesting;
2222
import com.google.common.base.Optional;
2323
import com.google.common.base.Preconditions;
24+
import com.google.common.base.Strings;
2425
import com.google.errorprone.annotations.ForOverride;
2526
import io.grpc.Attributes;
2627
import io.grpc.CallCredentials;
@@ -89,6 +90,7 @@
8990
import javax.net.ssl.TrustManager;
9091
import javax.net.ssl.TrustManagerFactory;
9192
import javax.net.ssl.X509TrustManager;
93+
9294
import org.codehaus.mojo.animal_sniffer.IgnoreJRERequirement;
9395

9496
/**
@@ -641,16 +643,21 @@ static final class ClientTlsHandler extends ProtocolNegotiationHandler {
641643
private final X509TrustManager x509ExtendedTrustManager;
642644
private SSLEngine sslEngine;
643645

644-
ClientTlsHandler(ChannelHandler next, SslContext sslContext, String authority,
646+
ClientTlsHandler(ChannelHandler next, SslContext sslContext, String sniHostPort,
645647
Executor executor, ChannelLogger negotiationLogger,
646648
Optional<Runnable> handshakeCompleteRunnable,
647649
ClientTlsProtocolNegotiator clientTlsProtocolNegotiator,
648650
X509TrustManager x509ExtendedTrustManager) {
649651
super(next, negotiationLogger);
650652
this.sslContext = Preconditions.checkNotNull(sslContext, "sslContext");
651-
HostPort hostPort = parseAuthority(authority);
652-
this.host = hostPort.host;
653-
this.port = hostPort.port;
653+
if (!Strings.isNullOrEmpty(sniHostPort)) {
654+
HostPort hostPort = parseAuthority(sniHostPort);
655+
this.host = hostPort.host;
656+
this.port = hostPort.port;
657+
} else {
658+
this.host = null;
659+
this.port = 0;
660+
}
654661
this.executor = executor;
655662
this.handshakeCompleteRunnable = handshakeCompleteRunnable;
656663
this.x509ExtendedTrustManager = x509ExtendedTrustManager;
@@ -659,7 +666,11 @@ static final class ClientTlsHandler extends ProtocolNegotiationHandler {
659666
@Override
660667
@IgnoreJRERequirement
661668
protected void handlerAdded0(ChannelHandlerContext ctx) {
662-
sslEngine = sslContext.newEngine(ctx.alloc(), host, port);
669+
if (host != null) {
670+
sslEngine = sslContext.newEngine(ctx.alloc(), host, port);
671+
} else {
672+
sslEngine = sslContext.newEngine(ctx.alloc());
673+
}
663674
SSLParameters sslParams = sslEngine.getSSLParameters();
664675
sslParams.setEndpointIdentificationAlgorithm("HTTPS");
665676
sslEngine.setSSLParameters(sslParams);

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

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -33,6 +33,7 @@
3333
import io.grpc.netty.InternalProtocolNegotiators;
3434
import io.grpc.netty.ProtocolNegotiationEvent;
3535
import io.grpc.xds.EnvoyServerProtoData;
36+
import io.grpc.xds.internal.security.trust.CertificateUtils;
3637
import io.netty.channel.ChannelHandler;
3738
import io.netty.channel.ChannelHandlerAdapter;
3839
import io.netty.channel.ChannelHandlerContext;
@@ -193,7 +194,6 @@ public void exceptionCaught(ChannelHandlerContext ctx, Throwable cause) {
193194
@VisibleForTesting
194195
static final class ClientSecurityHandler
195196
extends InternalProtocolNegotiators.ProtocolNegotiationHandler {
196-
static boolean isXdsSniEnabled = GrpcUtil.getFlag("GRPC_EXPERIMENTAL_XDS_SNI", false);
197197

198198
private final GrpcHttp2ConnectionHandler grpcHandler;
199199
private final SslContextProviderSupplier sslContextProviderSupplier;
@@ -218,7 +218,7 @@ public void handlerAdded(ChannelHandlerContext ctx) throws Exception {
218218
this.sslContextProviderSupplier = sslContextProviderSupplier;
219219
EnvoyServerProtoData.BaseTlsContext tlsContext = sslContextProviderSupplier.getTlsContext();
220220
UpstreamTlsContext upstreamTlsContext = ((UpstreamTlsContext) tlsContext);
221-
if (isXdsSniEnabled) {
221+
if (CertificateUtils.isXdsSniEnabled) {
222222
sni = upstreamTlsContext.getAutoHostSni() && !Strings.isNullOrEmpty(endpointHostname)
223223
? endpointHostname : upstreamTlsContext.getSni();
224224
} else {

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

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -16,6 +16,8 @@
1616

1717
package io.grpc.xds.internal.security.trust;
1818

19+
import io.grpc.internal.GrpcUtil;
20+
1921
import java.io.BufferedInputStream;
2022
import java.io.File;
2123
import java.io.FileInputStream;
@@ -29,6 +31,8 @@
2931
* Contains certificate utility method(s).
3032
*/
3133
public final class CertificateUtils {
34+
public static boolean isXdsSniEnabled = GrpcUtil.getFlag("GRPC_EXPERIMENTAL_XDS_SNI", false);
35+
3236
/**
3337
* Generates X509Certificate array from a file on disk.
3438
*

‎xds/src/main/java/io/grpc/xds/internal/security/trust/XdsX509TrustManager.java‎

Lines changed: 1 addition & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -53,8 +53,6 @@
5353
*/
5454
final class XdsX509TrustManager extends X509ExtendedTrustManager implements X509TrustManager {
5555

56-
static boolean isXdsSniEnabled = GrpcUtil.getFlag("GRPC_EXPERIMENTAL_XDS_SNI", false);
57-
5856
// ref: io.grpc.okhttp.internal.OkHostnameVerifier and
5957
// sun.security.x509.GeneralNameInterface
6058
private static final int ALT_DNS_NAME = 2;
@@ -220,7 +218,7 @@ void verifySubjectAltNameInChain(X509Certificate[] peerCertChain) throws Certifi
220218
return;
221219
}
222220
@SuppressWarnings("deprecation") // gRFC A29 predates match_typed_subject_alt_names
223-
List<StringMatcher> verifyList = isXdsSniEnabled && !Strings.isNullOrEmpty(sniForSanMatching)
221+
List<StringMatcher> verifyList = CertificateUtils.isXdsSniEnabled && !Strings.isNullOrEmpty(sniForSanMatching)
224222
? ImmutableList.of(StringMatcher.newBuilder().setExact(sniForSanMatching).build())
225223
: certContext.getMatchSubjectAltNamesList();
226224
if (verifyList.isEmpty()) {

‎xds/src/test/java/io/grpc/xds/XdsSecurityClientServerTest.java‎

Lines changed: 10 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -76,6 +76,7 @@
7676
import io.grpc.xds.internal.security.SslContextProviderSupplier;
7777
import io.grpc.xds.internal.security.TlsContextManagerImpl;
7878
import io.grpc.xds.internal.security.certprovider.FileWatcherCertificateProviderProvider;
79+
import io.grpc.xds.internal.security.trust.CertificateUtils;
7980
import io.netty.handler.ssl.NotSslRecordException;
8081
import java.io.File;
8182
import java.io.FileOutputStream;
@@ -315,6 +316,7 @@ public void tlsClientServer_noAutoSniValidation_failureToMatchSubjAltNames()
315316
@Test
316317
public void tlsClientServer_autoSniValidation_sniInUTC()
317318
throws Exception {
319+
CertificateUtils.isXdsSniEnabled = true;
318320
Path trustStoreFilePath = getCacertFilePathForTestCa();
319321
try {
320322
setTrustStoreSystemProperties(trustStoreFilePath.toAbsolutePath().toString());
@@ -338,12 +340,14 @@ public void tlsClientServer_autoSniValidation_sniInUTC()
338340
} finally {
339341
Files.deleteIfExists(trustStoreFilePath);
340342
clearTrustStoreSystemProperties();
343+
CertificateUtils.isXdsSniEnabled = false;
341344
}
342345
}
343346

344347
@Test
345348
public void tlsClientServer_autoSniValidation_sniFromHostname()
346349
throws Exception {
350+
CertificateUtils.isXdsSniEnabled = true;
347351
Path trustStoreFilePath = getCacertFilePathForTestCa();
348352
try {
349353
setTrustStoreSystemProperties(trustStoreFilePath.toAbsolutePath().toString());
@@ -370,12 +374,14 @@ public void tlsClientServer_autoSniValidation_sniFromHostname()
370374
} finally {
371375
Files.deleteIfExists(trustStoreFilePath);
372376
clearTrustStoreSystemProperties();
377+
CertificateUtils.isXdsSniEnabled = false;
373378
}
374379
}
375380

376381
@Test
377382
public void tlsClientServer_autoSniValidation_noSNIApplicable_usesMatcherFromCmnVdnCtx()
378383
throws Exception {
384+
CertificateUtils.isXdsSniEnabled = true;
379385
Path trustStoreFilePath = getCacertFilePathForTestCa();
380386
try {
381387
setTrustStoreSystemProperties(trustStoreFilePath.toAbsolutePath().toString());
@@ -399,6 +405,7 @@ public void tlsClientServer_autoSniValidation_noSNIApplicable_usesMatcherFromCmn
399405
} finally {
400406
Files.deleteIfExists(trustStoreFilePath);
401407
clearTrustStoreSystemProperties();
408+
CertificateUtils.isXdsSniEnabled = false;
402409
}
403410
}
404411

@@ -815,7 +822,7 @@ private SimpleServiceGrpc.SimpleServiceBlockingStub getBlockingStub(
815822
// in the certificate SNI, which isn't implemented yet (https://github.com/grpc/grpc-java/pull/12345 implements it)
816823
// so use an exact match SAN such as waterzooi.test.google.be for SNI for this testcase.
817824
private SimpleServiceGrpc.SimpleServiceBlockingStub getBlockingStub(
818-
final UpstreamTlsContext upstreamTlsContext, String overrideAuthority, String addrAttribute) {
825+
final UpstreamTlsContext upstreamTlsContext, String overrideAuthority, String addrNameAttribute) {
819826
ManagedChannelBuilder<?> channelBuilder =
820827
Grpc.newChannelBuilder(
821828
"sectest://localhost:" + port,
@@ -833,8 +840,8 @@ private SimpleServiceGrpc.SimpleServiceBlockingStub getBlockingStub(
833840
new SslContextProviderSupplier(
834841
upstreamTlsContext, tlsContextManagerForClient))
835842
: Attributes.newBuilder();
836-
if (addrAttribute != null) {
837-
sslContextAttributesBuilder.set(SecurityProtocolNegotiators.ATTR_ADDRESS_NAME, addrAttribute);
843+
if (addrNameAttribute != null) {
844+
sslContextAttributesBuilder.set(SecurityProtocolNegotiators.ATTR_ADDRESS_NAME, addrNameAttribute);
838845
}
839846
sslContextAttributes = sslContextAttributesBuilder.build();
840847
fakeNameResolverFactory.setServers(

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

Lines changed: 16 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -51,6 +51,7 @@
5151
import io.grpc.xds.internal.security.SecurityProtocolNegotiators.ClientSecurityHandler;
5252
import io.grpc.xds.internal.security.SecurityProtocolNegotiators.ClientSecurityProtocolNegotiator;
5353
import io.grpc.xds.internal.security.certprovider.CommonCertProviderTestUtils;
54+
import io.grpc.xds.internal.security.trust.CertificateUtils;
5455
import io.netty.channel.ChannelHandler;
5556
import io.netty.channel.ChannelHandlerContext;
5657
import io.netty.channel.ChannelPipeline;
@@ -145,7 +146,7 @@ public void clientSecurityProtocolNegotiatorNewHandler_withTlsContextAttribute()
145146

146147
@Test
147148
public void clientSecurityProtocolNegotiatorNewHandler_autoHostSni_hostnameIsPassedToClientSecurityHandler() {
148-
ClientSecurityHandler.isXdsSniEnabled = true;
149+
CertificateUtils.isXdsSniEnabled = true;
149150
try {
150151
UpstreamTlsContext upstreamTlsContext =
151152
CommonTlsContextTestsUtil.buildUpstreamTlsContext(CommonTlsContext.newBuilder().build(), null, true, false);
@@ -168,7 +169,7 @@ public void clientSecurityProtocolNegotiatorNewHandler_autoHostSni_hostnameIsPas
168169
assertThat(newHandler).isInstanceOf(ClientSecurityHandler.class);
169170
assertThat(((ClientSecurityHandler) newHandler).getSni()).isEqualTo(FAKE_AUTHORITY);
170171
} finally {
171-
ClientSecurityHandler.isXdsSniEnabled = false;
172+
CertificateUtils.isXdsSniEnabled = false;
172173
}
173174
}
174175

@@ -207,7 +208,7 @@ public void updateSslContext(SslContext sslContext) {
207208
protected void onException(Throwable throwable) {
208209
future.set(throwable);
209210
}
210-
}, null);
211+
}, FAKE_AUTHORITY);
211212
assertThat(executor.runDueTasks()).isEqualTo(1);
212213
channel.runPendingTasks();
213214
Object fromFuture = future.get(2, TimeUnit.SECONDS);
@@ -227,7 +228,7 @@ protected void onException(Throwable throwable) {
227228

228229
@Test
229230
public void sniInClientSecurityHandler_autoHostSniIsTrue_usesEndpointHostname() {
230-
ClientSecurityHandler.isXdsSniEnabled = true;
231+
CertificateUtils.isXdsSniEnabled = true;
231232
try {
232233
Bootstrapper.BootstrapInfo bootstrapInfoForClient = CommonBootstrapperTestUtils
233234
.buildBootstrapInfo("google_cloud_private_spiffe-client", CLIENT_KEY_FILE, CLIENT_PEM_FILE,
@@ -244,13 +245,13 @@ public void sniInClientSecurityHandler_autoHostSniIsTrue_usesEndpointHostname()
244245

245246
assertThat(clientSecurityHandler.getSni()).isEqualTo(HOSTNAME);
246247
} finally {
247-
ClientSecurityHandler.isXdsSniEnabled = false;
248+
CertificateUtils.isXdsSniEnabled = false;
248249
}
249250
}
250251

251252
@Test
252253
public void sniInClientSecurityHandler_autoHostSniIsTrue_endpointHostnameIsEmpty_usesSniFromUpstreamTlsContext() {
253-
ClientSecurityHandler.isXdsSniEnabled = true;
254+
CertificateUtils.isXdsSniEnabled = true;
254255
try {
255256
Bootstrapper.BootstrapInfo bootstrapInfoForClient = CommonBootstrapperTestUtils
256257
.buildBootstrapInfo("google_cloud_private_spiffe-client", CLIENT_KEY_FILE, CLIENT_PEM_FILE,
@@ -267,13 +268,13 @@ public void sniInClientSecurityHandler_autoHostSniIsTrue_endpointHostnameIsEmpty
267268

268269
assertThat(clientSecurityHandler.getSni()).isEqualTo(SNI_IN_UTC);
269270
} finally {
270-
ClientSecurityHandler.isXdsSniEnabled = false;
271+
CertificateUtils.isXdsSniEnabled = false;
271272
}
272273
}
273274

274275
@Test
275276
public void sniInClientSecurityHandler_autoHostSniIsTrue_endpointHostnameIsNull_usesSniFromUpstreamTlsContext() {
276-
ClientSecurityHandler.isXdsSniEnabled = true;
277+
CertificateUtils.isXdsSniEnabled = true;
277278
try {
278279
Bootstrapper.BootstrapInfo bootstrapInfoForClient = CommonBootstrapperTestUtils
279280
.buildBootstrapInfo("google_cloud_private_spiffe-client", CLIENT_KEY_FILE, CLIENT_PEM_FILE,
@@ -290,13 +291,13 @@ public void sniInClientSecurityHandler_autoHostSniIsTrue_endpointHostnameIsNull_
290291

291292
assertThat(clientSecurityHandler.getSni()).isEqualTo(SNI_IN_UTC);
292293
} finally {
293-
ClientSecurityHandler.isXdsSniEnabled = false;
294+
CertificateUtils.isXdsSniEnabled = false;
294295
}
295296
}
296297

297298
@Test
298299
public void sniInClientSecurityHandler_autoHostSniIsFalse_usesSniFromUpstreamTlsContext() {
299-
ClientSecurityHandler.isXdsSniEnabled = true;
300+
CertificateUtils.isXdsSniEnabled = true;
300301
try {
301302
Bootstrapper.BootstrapInfo bootstrapInfoForClient = CommonBootstrapperTestUtils
302303
.buildBootstrapInfo("google_cloud_private_spiffe-client", CLIENT_KEY_FILE, CLIENT_PEM_FILE,
@@ -313,13 +314,13 @@ public void sniInClientSecurityHandler_autoHostSniIsFalse_usesSniFromUpstreamTls
313314

314315
assertThat(clientSecurityHandler.getSni()).isEqualTo(SNI_IN_UTC);
315316
} finally {
316-
ClientSecurityHandler.isXdsSniEnabled = false;
317+
CertificateUtils.isXdsSniEnabled = false;
317318
}
318319
}
319320

320321
@Test
321322
public void sniFeatureNotEnabled_usesChannelAuthorityForSni() {
322-
ClientSecurityHandler.isXdsSniEnabled = false;
323+
CertificateUtils.isXdsSniEnabled = false;
323324
Bootstrapper.BootstrapInfo bootstrapInfoForClient = CommonBootstrapperTestUtils
324325
.buildBootstrapInfo("google_cloud_private_spiffe-client", CLIENT_KEY_FILE, CLIENT_PEM_FILE,
325326
CA_PEM_FILE, null, null, null, null, null);
@@ -498,7 +499,7 @@ public void nullTlsContext_nullFallbackProtocolNegotiator_expectException() {
498499
@Test
499500
public void clientSecurityProtocolNegotiatorNewHandler_fireProtocolNegotiationEvent()
500501
throws InterruptedException, TimeoutException, ExecutionException {
501-
ClientSecurityHandler.isXdsSniEnabled = true;
502+
CertificateUtils.isXdsSniEnabled = true;
502503
try {
503504
FakeClock executor = new FakeClock();
504505
CommonCertProviderTestUtils.register(executor);
@@ -533,7 +534,7 @@ public void updateSslContext(SslContext sslContext) {
533534
protected void onException(Throwable throwable) {
534535
future.set(throwable);
535536
}
536-
}, null);
537+
}, "");
537538
executor.runDueTasks();
538539
channel.runPendingTasks(); // need this for tasks to execute on eventLoop
539540
Object fromFuture = future.get(5, TimeUnit.SECONDS);
@@ -548,7 +549,7 @@ protected void onException(Throwable throwable) {
548549
assertTrue(channel.isOpen());
549550
CommonCertProviderTestUtils.register0();
550551
} finally {
551-
ClientSecurityHandler.isXdsSniEnabled = false;
552+
CertificateUtils.isXdsSniEnabled = false;
552553
}
553554
}
554555

‎xds/src/test/java/io/grpc/xds/internal/security/TlsContextManagerTest.java‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -169,7 +169,7 @@ public void createClientSslContextProvider_releaseInstance() {
169169
TlsContextManagerImpl tlsContextManagerImpl =
170170
new TlsContextManagerImpl(mockClientFactory, mockServerFactory);
171171
SslContextProvider mockProvider = mock(SslContextProvider.class);
172-
when(mockClientFactory.create(new AbstractMap.SimpleImmutableEntry("sni", upstreamTlsContext)))
172+
when(mockClientFactory.create(new AbstractMap.SimpleImmutableEntry(upstreamTlsContext, SNI)))
173173
.thenReturn(mockProvider);
174174
SslContextProvider clientSecretProvider =
175175
tlsContextManagerImpl.findOrCreateClientSslContextProvider(upstreamTlsContext, SNI);

0 commit comments

Comments
 (0)