Skip to content

Commit 2858db2

Browse files
NianJiuZstNianJiuZst
authored
netty: make hostname verification configurable (grpc#13002)
Fixes grpc#12343. This configures HTTPS endpoint identification in GrpcSslContexts and updates ClientTlsHandler to preserve an explicit empty-string opt-out. A null algorithm still falls back to HTTPS, retaining hostname verification for custom contexts that do not select an algorithm. The public Javadoc warns callers that disabling DNS hostname verification requires alternative identity verification. Aided by the above changes, now - `CertProviderClientSslContextProvider` now explicitly sets `.endpointIdentificationAlgorithm("")`. - `XdsX509TrustManager` no longer mutates endpoint-identification settings on SSLEngine or SSLSocket.
1 parent f97248f commit 2858db2

8 files changed

Lines changed: 143 additions & 15 deletions

File tree

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

Lines changed: 16 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -47,6 +47,7 @@
4747
@ExperimentalApi("https://github.com/grpc/grpc-java/issues/1784")
4848
public class GrpcSslContexts {
4949
private static final Logger logger = Logger.getLogger(GrpcSslContexts.class.getName());
50+
static final String DEFAULT_ENDPOINT_IDENTIFICATION_ALGORITHM = "HTTPS";
5051

5152
private GrpcSslContexts() {}
5253

@@ -89,6 +90,11 @@ private GrpcSslContexts() {}
8990
/**
9091
* Creates an SslContextBuilder with ciphers and APN appropriate for gRPC.
9192
*
93+
* <p>HTTPS endpoint identification is enabled by default. Clients that authenticate a non-DNS
94+
* identity may explicitly disable it with {@link
95+
* SslContextBuilder#endpointIdentificationAlgorithm(String) endpointIdentificationAlgorithm("")},
96+
* but must perform alternative identity verification.
97+
*
9298
* @see SslContextBuilder#forClient()
9399
* @see #configure(SslContextBuilder)
94100
*/
@@ -140,7 +146,8 @@ public static SslContextBuilder forServer(
140146

141147
/**
142148
* Set ciphers and APN appropriate for gRPC. Precisely what is set is permitted to change, so if
143-
* an application requires particular settings it should override the options set here.
149+
* an application requires particular settings it should override the options set here. For
150+
* client builders, HTTPS endpoint identification is enabled by default.
144151
*/
145152
@CanIgnoreReturnValue
146153
public static SslContextBuilder configure(SslContextBuilder builder) {
@@ -149,7 +156,8 @@ public static SslContextBuilder configure(SslContextBuilder builder) {
149156

150157
/**
151158
* Set ciphers and APN appropriate for gRPC. Precisely what is set is permitted to change, so if
152-
* an application requires particular settings it should override the options set here.
159+
* an application requires particular settings it should override the options set here. For
160+
* client builders, HTTPS endpoint identification is enabled by default.
153161
*/
154162
@ExperimentalApi("https://github.com/grpc/grpc-java/issues/1784")
155163
@CanIgnoreReturnValue
@@ -173,7 +181,8 @@ public static SslContextBuilder configure(SslContextBuilder builder, SslProvider
173181
return builder
174182
.sslProvider(SslProvider.OPENSSL)
175183
.ciphers(Http2SecurityUtil.CIPHERS, SupportedCipherSuiteFilter.INSTANCE)
176-
.applicationProtocolConfig(apc);
184+
.applicationProtocolConfig(apc)
185+
.endpointIdentificationAlgorithm(DEFAULT_ENDPOINT_IDENTIFICATION_ALGORITHM);
177186
}
178187
default:
179188
throw new IllegalArgumentException("Unsupported provider: " + provider);
@@ -182,7 +191,8 @@ public static SslContextBuilder configure(SslContextBuilder builder, SslProvider
182191

183192
/**
184193
* Set ciphers and APN appropriate for gRPC. Precisely what is set is permitted to change, so if
185-
* an application requires particular settings it should override the options set here.
194+
* an application requires particular settings it should override the options set here. For
195+
* client builders, HTTPS endpoint identification is enabled by default.
186196
*/
187197
@CanIgnoreReturnValue
188198
public static SslContextBuilder configure(SslContextBuilder builder, Provider jdkProvider) {
@@ -220,7 +230,8 @@ public static SslContextBuilder configure(SslContextBuilder builder, Provider jd
220230
.sslProvider(SslProvider.JDK)
221231
.ciphers(Http2SecurityUtil.CIPHERS, SupportedCipherSuiteFilter.INSTANCE)
222232
.applicationProtocolConfig(apc)
223-
.sslContextProvider(jdkProvider);
233+
.sslContextProvider(jdkProvider)
234+
.endpointIdentificationAlgorithm(DEFAULT_ENDPOINT_IDENTIFICATION_ALGORITHM);
224235
}
225236

226237
/**

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

Lines changed: 5 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -680,8 +680,11 @@ protected void handlerAdded0(ChannelHandlerContext ctx) {
680680
sslEngine = sslContext.newEngine(ctx.alloc());
681681
}
682682
SSLParameters sslParams = sslEngine.getSSLParameters();
683-
sslParams.setEndpointIdentificationAlgorithm("HTTPS");
684-
sslEngine.setSSLParameters(sslParams);
683+
if (sslParams.getEndpointIdentificationAlgorithm() == null) {
684+
sslParams.setEndpointIdentificationAlgorithm(
685+
GrpcSslContexts.DEFAULT_ENDPOINT_IDENTIFICATION_ALGORITHM);
686+
sslEngine.setSSLParameters(sslParams);
687+
}
685688
ctx.pipeline().addBefore(ctx.name(), /* name= */ null, this.executor != null
686689
? new SslHandler(sslEngine, false, this.executor)
687690
: new SslHandler(sslEngine, false));

‎netty/src/test/java/io/grpc/netty/ProtocolNegotiatorsTest.java‎

Lines changed: 42 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -111,6 +111,7 @@
111111
import io.netty.handler.proxy.ProxyConnectException;
112112
import io.netty.handler.ssl.ApplicationProtocolConfig;
113113
import io.netty.handler.ssl.SslContext;
114+
import io.netty.handler.ssl.SslContextBuilder;
114115
import io.netty.handler.ssl.SslHandler;
115116
import io.netty.handler.ssl.SslHandshakeCompletionEvent;
116117
import java.io.File;
@@ -936,6 +937,47 @@ public String applicationProtocol() {
936937
assertNotNull(grpcHandlerCtx);
937938
}
938939

940+
@Test
941+
public void grpcSslContextsConfigure_enablesEndpointIdentification() throws Exception {
942+
SslContext clientSslContext = GrpcSslContexts.configure(
943+
SslContextBuilder.forClient().endpointIdentificationAlgorithm(null)).build();
944+
945+
SSLEngine sslEngine = clientSslContext.newEngine(channel.alloc(), "localhost", 443);
946+
947+
assertThat(sslEngine.getSSLParameters().getEndpointIdentificationAlgorithm())
948+
.isEqualTo("HTTPS");
949+
}
950+
951+
@Test
952+
public void clientTlsHandler_nullEndpointIdentificationUsesHttps() throws Exception {
953+
SslContext clientSslContext = GrpcSslContexts.forClient()
954+
.endpointIdentificationAlgorithm(null)
955+
.build();
956+
ClientTlsHandler handler = new ClientTlsHandler(grpcHandler, clientSslContext,
957+
"authority", null, noopLogger, Optional.absent(),
958+
getClientTlsProtocolNegotiator(), null);
959+
960+
pipeline.addLast(handler);
961+
962+
assertThat(pipeline.get(SslHandler.class).engine().getSSLParameters()
963+
.getEndpointIdentificationAlgorithm()).isEqualTo("HTTPS");
964+
}
965+
966+
@Test
967+
public void clientTlsHandler_emptyEndpointIdentificationRemainsDisabled() throws Exception {
968+
SslContext clientSslContext = GrpcSslContexts.forClient()
969+
.endpointIdentificationAlgorithm("")
970+
.build();
971+
ClientTlsHandler handler = new ClientTlsHandler(grpcHandler, clientSslContext,
972+
"authority", null, noopLogger, Optional.absent(),
973+
getClientTlsProtocolNegotiator(), null);
974+
975+
pipeline.addLast(handler);
976+
977+
assertThat(pipeline.get(SslHandler.class).engine().getSSLParameters()
978+
.getEndpointIdentificationAlgorithm()).isEmpty();
979+
}
980+
939981
@Test
940982
public void clientTlsHandler_userEventTriggeredSslEvent_supportedProtocolCustom()
941983
throws Exception {

‎netty/src/test/java/io/grpc/netty/TlsTest.java‎

Lines changed: 37 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -237,6 +237,43 @@ public void clientRejectedMismatchedHostname() throws Exception {
237237
}
238238
}
239239

240+
/**
241+
* Test that a client can explicitly disable hostname verification when it authenticates the
242+
* server using a non-DNS identity.
243+
*/
244+
@Test
245+
public void clientCanDisableHostnameVerification() throws Exception {
246+
// Create & start a server.
247+
File serverCertFile = TestUtils.loadCert("server1.pem");
248+
File serverPrivateKeyFile = TestUtils.loadCert("server1.key");
249+
X509Certificate[] serverTrustedCaCerts = {
250+
TestUtils.loadX509Cert("ca.pem")
251+
};
252+
server = serverBuilder(0, serverCertFile, serverPrivateKeyFile, serverTrustedCaCerts)
253+
.addService(new SimpleServiceImpl())
254+
.build()
255+
.start();
256+
257+
// Use a mismatched authority, but explicitly disable DNS hostname verification.
258+
File clientCertChainFile = TestUtils.loadCert("client.pem");
259+
File clientPrivateKeyFile = TestUtils.loadCert("client.key");
260+
X509Certificate[] clientTrustedCaCerts = {
261+
TestUtils.loadX509Cert("ca.pem")
262+
};
263+
channel = NettyChannelBuilder.forAddress("localhost", server.getPort())
264+
.overrideAuthority("i.am.a.bad.hostname")
265+
.negotiationType(NegotiationType.TLS)
266+
.sslContext(clientContextBuilder
267+
.endpointIdentificationAlgorithm("")
268+
.keyManager(clientCertChainFile, clientPrivateKeyFile)
269+
.trustManager(clientTrustedCaCerts)
270+
.build())
271+
.build();
272+
SimpleServiceGrpc.SimpleServiceBlockingStub client = SimpleServiceGrpc.newBlockingStub(channel);
273+
274+
client.unaryRpc(SimpleRequest.getDefaultInstance());
275+
}
276+
240277
/**
241278
* Tests that a server configured to require client authentication refuses to accept connections
242279
* from a client that has an untrusted certificate.

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

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -76,7 +76,9 @@ final class CertProviderClientSslContextProvider extends CertProviderSslContextP
7676
}
7777

7878
SslContextBuilder sslContextBuilder =
79-
GrpcSslContexts.forClient().trustManager(trustManagerFactory);
79+
GrpcSslContexts.forClient()
80+
.endpointIdentificationAlgorithm("")
81+
.trustManager(trustManagerFactory);
8082
if (isMtls()) {
8183
sslContextBuilder.keyManager(savedKey, savedCertChain);
8284
}

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

Lines changed: 0 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -263,10 +263,6 @@ public void checkServerTrusted(X509Certificate[] chain, String authType, Socket
263263
if (socket instanceof SSLSocket) {
264264
SSLSocket sslSocket = (SSLSocket) socket;
265265
SSLParameters sslParams = sslSocket.getSSLParameters();
266-
if (sslParams != null) {
267-
sslParams.setEndpointIdentificationAlgorithm("");
268-
sslSocket.setSSLParameters(sslParams);
269-
}
270266
sniMatchers = getAutoSniSanMatchers(sslParams);
271267
}
272268
if (sniMatchers.isEmpty() && certContext != null) {
@@ -284,8 +280,6 @@ public void checkServerTrusted(X509Certificate[] chain, String authType, SSLEngi
284280
List<StringMatcher> sniMatchers = null;
285281
SSLParameters sslParams = sslEngine.getSSLParameters();
286282
if (sslParams != null) {
287-
sslParams.setEndpointIdentificationAlgorithm("");
288-
sslEngine.setSSLParameters(sslParams);
289283
sniMatchers = getAutoSniSanMatchers(sslParams);
290284
}
291285
if (sniMatchers.isEmpty() && certContext != null) {

‎xds/src/test/java/io/grpc/xds/internal/security/certprovider/CertProviderClientSslContextProviderTest.java‎

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -38,6 +38,7 @@
3838
import io.grpc.xds.client.CommonBootstrapperTestUtils;
3939
import io.grpc.xds.internal.security.CommonTlsContextTestsUtil;
4040
import io.grpc.xds.internal.security.CommonTlsContextTestsUtil.TestCallback;
41+
import io.netty.buffer.UnpooledByteBufAllocator;
4142
import java.util.Queue;
4243
import java.util.concurrent.ConcurrentLinkedQueue;
4344
import java.util.concurrent.Executor;
@@ -393,6 +394,10 @@ public void testProviderForClient_tls() throws Exception {
393394
CommonTlsContextTestsUtil.getValueThruCallback(provider);
394395

395396
doChecksOnSslContext(false, testCallback.updatedSslContext, /* expectedApnProtos= */ null);
397+
assertThat(testCallback.updatedSslContext.getKey()
398+
.newEngine(UnpooledByteBufAllocator.DEFAULT)
399+
.getSSLParameters()
400+
.getEndpointIdentificationAlgorithm()).isEmpty();
396401
}
397402

398403
@Test

‎xds/src/test/java/io/grpc/xds/internal/security/trust/XdsX509TrustManagerTest.java‎

Lines changed: 35 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -573,6 +573,23 @@ public void checkServerTrustedSslEngineSpiffeTrustMap()
573573
assertThat(sslEngine.getSSLParameters().getEndpointIdentificationAlgorithm()).isEmpty();
574574
}
575575

576+
@Test
577+
public void checkServerTrustedSslEngine_doesNotModifyEndpointIdentificationAlgorithm()
578+
throws CertificateException, IOException {
579+
trustManager = new XdsX509TrustManager(null, mockDelegate, false);
580+
SSLParameters sslParams = new SSLParameters();
581+
sslParams.setEndpointIdentificationAlgorithm("HTTPS");
582+
TestSslEngine sslEngine = mock(TestSslEngine.class, CALLS_REAL_METHODS);
583+
sslEngine.setSSLParameters(sslParams);
584+
X509Certificate[] serverCerts =
585+
CertificateUtils.toX509Certificates(TlsTesting.loadCert(SERVER_1_PEM_FILE));
586+
587+
trustManager.checkServerTrusted(serverCerts, "ECDHE_ECDSA", sslEngine);
588+
589+
assertThat(sslEngine.getSSLParameters().getEndpointIdentificationAlgorithm())
590+
.isEqualTo("HTTPS");
591+
}
592+
576593
@Test
577594
public void checkServerTrustedSslEngineSpiffeTrustMap_missing_spiffe_id()
578595
throws CertificateException, IOException, CertStoreException {
@@ -667,6 +684,23 @@ public void checkServerTrustedSslSocketSpiffeTrustMap()
667684
assertThat(sslSocket.getSSLParameters().getEndpointIdentificationAlgorithm()).isEmpty();
668685
}
669686

687+
@Test
688+
public void checkServerTrustedSslSocket_doesNotModifyEndpointIdentificationAlgorithm()
689+
throws CertificateException, IOException {
690+
trustManager = new XdsX509TrustManager(null, mockDelegate, false);
691+
SSLParameters sslParams = new SSLParameters();
692+
sslParams.setEndpointIdentificationAlgorithm("HTTPS");
693+
TestSslSocket sslSocket = mock(TestSslSocket.class, CALLS_REAL_METHODS);
694+
sslSocket.setSSLParameters(sslParams);
695+
X509Certificate[] serverCerts =
696+
CertificateUtils.toX509Certificates(TlsTesting.loadCert(SERVER_1_PEM_FILE));
697+
698+
trustManager.checkServerTrusted(serverCerts, "ECDHE_ECDSA", sslSocket);
699+
700+
assertThat(sslSocket.getSSLParameters().getEndpointIdentificationAlgorithm())
701+
.isEqualTo("HTTPS");
702+
}
703+
670704
@Test
671705
public void checkServerTrustedSslSocket_untrustedServer_expectException()
672706
throws CertificateException, IOException, CertStoreException {
@@ -785,7 +819,7 @@ private SSLParameters buildTrustManagerAndGetSslParameters()
785819
when(mockSession.getProtocol()).thenReturn("TLSv1.2");
786820
when(mockSession.getPeerHost()).thenReturn("peer-host-from-mock");
787821
SSLParameters sslParams = new SSLParameters();
788-
sslParams.setEndpointIdentificationAlgorithm("HTTPS");
822+
sslParams.setEndpointIdentificationAlgorithm("");
789823
return sslParams;
790824
}
791825

0 commit comments

Comments
 (0)