Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Go client must verify ALPN selected h2 #2742

Closed
ejona86 opened this issue Apr 4, 2019 · 3 comments
Closed

Go client must verify ALPN selected h2 #2742

ejona86 opened this issue Apr 4, 2019 · 3 comments

Comments

@ejona86
Copy link
Member

ejona86 commented Apr 4, 2019

HTTP/2 uses ALPN to negotiate the protocol. But if ALPN is not supported on the server, the TLS library may let the connection progress, as the application will commonly use a fallback (typically HTTP/1).

It appears that the Go client does not detect the case where ALPN is unsupported on the server. The client should check the negotiated protocol and fail if it isn't supported (h2 or grpc-exp, etc).

This was discovered in grpc/grpc-java#5543. Fixing this may impact your users, but is also a requirement of HTTP/2.

I tested this with a hacked Java server that disabled ALPN. Wireshark is helpful to verify it is behaving as expected.

$ git clone https://github.com/grpc/grpc-java.git -b v1.19.0
$ cd grpc-java
$ patch -p1 <<EOF
diff --git a/netty/src/main/java/io/grpc/netty/GrpcSslContexts.java b/netty/src/main/java/io/grpc/netty/GrpcSslContexts.java
index b6aed29ab..70285d407 100644
--- a/netty/src/main/java/io/grpc/netty/GrpcSslContexts.java
+++ b/netty/src/main/java/io/grpc/netty/GrpcSslContexts.java
@@ -199,7 +199,7 @@ public class GrpcSslContexts {
         return builder
             .sslProvider(SslProvider.OPENSSL)
             .ciphers(Http2SecurityUtil.CIPHERS, SupportedCipherSuiteFilter.INSTANCE)
-            .applicationProtocolConfig(apc);
+            /*.applicationProtocolConfig(apc)*/;
       }
       default:
         throw new IllegalArgumentException("Unsupported provider: " + provider);
@@ -233,7 +233,7 @@ public class GrpcSslContexts {
     return builder
         .sslProvider(SslProvider.JDK)
         .ciphers(Http2SecurityUtil.CIPHERS, SupportedCipherSuiteFilter.INSTANCE)
-        .applicationProtocolConfig(apc)
+        //.applicationProtocolConfig(apc)
         .sslContextProvider(jdkProvider);
   }
 
@@ -295,6 +295,9 @@ public class GrpcSslContexts {
   @SuppressWarnings("deprecation")
   static void ensureAlpnAndH2Enabled(
       io.netty.handler.ssl.ApplicationProtocolNegotiator alpnNegotiator) {
+    if (true) {
+      return;
+    }
     checkArgument(alpnNegotiator != null, "ALPN must be configured");
     checkArgument(alpnNegotiator.protocols() != null && !alpnNegotiator.protocols().isEmpty(),
         "ALPN must be enabled and list HTTP/2 as a supported protocol.");
diff --git a/netty/src/main/java/io/grpc/netty/ProtocolNegotiators.java b/netty/src/main/java/io/grpc/netty/ProtocolNegotiators.java
index ceda3013c..5266bf61d 100644
--- a/netty/src/main/java/io/grpc/netty/ProtocolNegotiators.java
+++ b/netty/src/main/java/io/grpc/netty/ProtocolNegotiators.java
@@ -160,7 +160,7 @@ public final class ProtocolNegotiators {
       if (evt instanceof SslHandshakeCompletionEvent) {
         SslHandshakeCompletionEvent handshakeEvent = (SslHandshakeCompletionEvent) evt;
         if (handshakeEvent.isSuccess()) {
-          if (NEXT_PROTOCOL_VERSIONS.contains(sslHandler(ctx.pipeline()).applicationProtocol())) {
+          if (true) {
             SSLSession session = sslHandler(ctx.pipeline()).engine().getSession();
             // Successfully negotiated the protocol.
             // Notify about completion and pass down SSLSession in attributes.
EOF
$ ./gradlew :grpc-interop-testing:installDist -PskipCodegen=true
$ ./run-test-server.sh
$ docker run --rm --net=host gcr.io/grpc-testing/grpc_interop_go1.11:v1.19.0 /go/bin/client --use_tls=true --server_host_override=foo.test.google.fr --use_test_ca=true --server_port=8080

The interop client should not succeed. For comparison:

$ docker run --rm --net=host gcr.io/grpc-testing/grpc_interop_java_oracle8:v1.19.0 /var/local/git/grpc-java/run-test-client.sh --use_tls=true --server_host_override=foo.test.google.fr --use_test_ca=true --server_port=8080
Running test empty_unary
Exception in thread "main" io.grpc.StatusRuntimeException: UNKNOWN
	at io.grpc.stub.ClientCalls.toStatusRuntimeException(ClientCalls.java:233)
	at io.grpc.stub.ClientCalls.getUnchecked(ClientCalls.java:214)
	at io.grpc.stub.ClientCalls.blockingUnaryCall(ClientCalls.java:139)
	at io.grpc.testing.integration.TestServiceGrpc$TestServiceBlockingStub.emptyCall(TestServiceGrpc.java:622)
	at io.grpc.testing.integration.AbstractInteropTest.emptyUnary(AbstractInteropTest.java:346)
	at io.grpc.testing.integration.TestServiceClient.runTest(TestServiceClient.java:224)
	at io.grpc.testing.integration.TestServiceClient.run(TestServiceClient.java:212)
	at io.grpc.testing.integration.TestServiceClient.main(TestServiceClient.java:70)
Caused by: java.lang.Exception: Failed ALPN negotiation: Unable to find compatible protocol.
	at io.grpc.netty.ProtocolNegotiators$BufferUntilTlsNegotiatedHandler.userEventTriggered(ProtocolNegotiators.java:680)
	at io.netty.channel.AbstractChannelHandlerContext.invokeUserEventTriggered(AbstractChannelHandlerContext.java:329)
	at io.netty.channel.AbstractChannelHandlerContext.invokeUserEventTriggered(AbstractChannelHandlerContext.java:315)
	at io.netty.channel.AbstractChannelHandlerContext.fireUserEventTriggered(AbstractChannelHandlerContext.java:307)
	at io.netty.handler.ssl.SslHandler.setHandshakeSuccess(SslHandler.java:1537)
	at io.netty.handler.ssl.SslHandler.unwrap(SslHandler.java:1372)
	at io.netty.handler.ssl.SslHandler.decodeJdkCompatible(SslHandler.java:1203)
	at io.netty.handler.ssl.SslHandler.decode(SslHandler.java:1247)
	at io.netty.handler.codec.ByteToMessageDecoder.decodeRemovalReentryProtection(ByteToMessageDecoder.java:502)
	at io.netty.handler.codec.ByteToMessageDecoder.callDecode(ByteToMessageDecoder.java:441)
	at io.netty.handler.codec.ByteToMessageDecoder.channelRead(ByteToMessageDecoder.java:278)
	at io.netty.channel.AbstractChannelHandlerContext.invokeChannelRead(AbstractChannelHandlerContext.java:362)
	at io.netty.channel.AbstractChannelHandlerContext.invokeChannelRead(AbstractChannelHandlerContext.java:348)
	at io.netty.channel.AbstractChannelHandlerContext.fireChannelRead(AbstractChannelHandlerContext.java:340)
	at io.netty.channel.DefaultChannelPipeline$HeadContext.channelRead(DefaultChannelPipeline.java:1434)
	at io.netty.channel.AbstractChannelHandlerContext.invokeChannelRead(AbstractChannelHandlerContext.java:362)
	at io.netty.channel.AbstractChannelHandlerContext.invokeChannelRead(AbstractChannelHandlerContext.java:348)
	at io.netty.channel.DefaultChannelPipeline.fireChannelRead(DefaultChannelPipeline.java:965)
	at io.netty.channel.nio.AbstractNioByteChannel$NioByteUnsafe.read(AbstractNioByteChannel.java:163)
	at io.netty.channel.nio.NioEventLoop.processSelectedKey(NioEventLoop.java:656)
	at io.netty.channel.nio.NioEventLoop.processSelectedKeysOptimized(NioEventLoop.java:591)
	at io.netty.channel.nio.NioEventLoop.processSelectedKeys(NioEventLoop.java:508)
	at io.netty.channel.nio.NioEventLoop.run(NioEventLoop.java:470)
	at io.netty.util.concurrent.SingleThreadEventExecutor$5.run(SingleThreadEventExecutor.java:909)
	at io.netty.util.concurrent.FastThreadLocalRunnable.run(FastThreadLocalRunnable.java:30)
	at java.lang.Thread.run(Thread.java:748)
Shutting down
@ejona86
Copy link
Member Author

ejona86 commented Apr 5, 2019

Related: grpc/grpc#9991

@hunterlxt
Copy link

hunterlxt commented Apr 3, 2020

Hi, I found the same problem when we intend to use rust as clients to connect go server. For example, when go server run in a mac os(don't support ALPN), rust client report that it trying to connect http/1.x server. But when using client with go, everything seems fine.... it's wired.
@ejona86

looks like we met a problem like those issues grpc/grpc#9991 grpc/grpc#18710

our problem tikv/tikv#7312

@dfawley
Copy link
Member

dfawley commented Mar 30, 2022

I believe this is the same as #434

@dfawley dfawley closed this as completed Mar 30, 2022
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 27, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants