From bd9b66ffa986b4834217bed53e26bedcb830eae7 Mon Sep 17 00:00:00 2001 From: Carl Mastrangelo Date: Thu, 25 Jan 2018 11:51:24 -0800 Subject: [PATCH 1/2] netty: only add gRPC negotiator once SSL is established --- netty/src/main/java/io/grpc/netty/ProtocolNegotiators.java | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/netty/src/main/java/io/grpc/netty/ProtocolNegotiators.java b/netty/src/main/java/io/grpc/netty/ProtocolNegotiators.java index 973d7ec33b2..e5fd0b4ff23 100644 --- a/netty/src/main/java/io/grpc/netty/ProtocolNegotiators.java +++ b/netty/src/main/java/io/grpc/netty/ProtocolNegotiators.java @@ -609,7 +609,7 @@ private static class BufferUntilTlsNegotiatedHandler extends AbstractBufferingHa BufferUntilTlsNegotiatedHandler( ChannelHandler bootstrapHandler, GrpcHttp2ConnectionHandler grpcHandler) { - super(bootstrapHandler, grpcHandler); + super(bootstrapHandler); this.grpcHandler = grpcHandler; } @@ -628,6 +628,10 @@ public void userEventTriggered(ChannelHandlerContext ctx, Object evt) throws Exc // Successfully negotiated the protocol. logSslEngineDetails(Level.FINER, ctx, "TLS negotiation succeeded.", null); + // Wait until negotiation is complete to add gRPC. If added too early, HTTP/2 writes + // will fail before we see the userEvent, and the channel is closed down prematurely. + ctx.pipeline().addBefore(ctx.name(), null, grpcHandler); + // Successfully negotiated the protocol. // Notify about completion and pass down SSLSession in attributes. grpcHandler.handleProtocolNegotiationCompleted( From e7469d0938a8acf151bc3ffc07daf34afe70bbc9 Mon Sep 17 00:00:00 2001 From: Eric Anderson Date: Thu, 1 Feb 2018 07:25:10 -0800 Subject: [PATCH 2/2] netty: Add test to verify error for TLS failure This is to notice regressions like in #4016 --- .../grpc/netty/NettyClientTransportTest.java | 43 ++++++++++++++++++- 1 file changed, 41 insertions(+), 2 deletions(-) diff --git a/netty/src/test/java/io/grpc/netty/NettyClientTransportTest.java b/netty/src/test/java/io/grpc/netty/NettyClientTransportTest.java index dc4c44e7989..cd9845ab58d 100644 --- a/netty/src/test/java/io/grpc/netty/NettyClientTransportTest.java +++ b/netty/src/test/java/io/grpc/netty/NettyClientTransportTest.java @@ -17,6 +17,7 @@ package io.grpc.netty; import static com.google.common.base.Charsets.UTF_8; +import static com.google.common.truth.Truth.assertThat; import static io.grpc.internal.GrpcUtil.DEFAULT_MAX_MESSAGE_SIZE; import static io.grpc.internal.GrpcUtil.DEFAULT_SERVER_KEEPALIVE_TIMEOUT_NANOS; import static io.grpc.internal.GrpcUtil.DEFAULT_SERVER_KEEPALIVE_TIME_NANOS; @@ -67,6 +68,7 @@ import io.netty.channel.socket.nio.NioServerSocketChannel; import io.netty.channel.socket.nio.NioSocketChannel; import io.netty.handler.codec.http2.StreamBufferingEncoder; +import io.netty.handler.ssl.ClientAuth; import io.netty.handler.ssl.SslContext; import io.netty.handler.ssl.SupportedCipherSuiteFilter; import io.netty.util.AsciiString; @@ -83,6 +85,7 @@ import java.util.concurrent.ExecutionException; import java.util.concurrent.TimeUnit; import java.util.concurrent.TimeoutException; +import javax.net.ssl.SSLHandshakeException; import org.junit.After; import org.junit.Before; import org.junit.Test; @@ -269,6 +272,42 @@ public void run() { } } + @Test + public void tlsNegotiationFailurePropagatesToStatus() throws Exception { + File serverCert = TestUtils.loadCert("server1.pem"); + File serverKey = TestUtils.loadCert("server1.key"); + // Don't trust ca.pem, so that client auth fails + SslContext sslContext = GrpcSslContexts.forServer(serverCert, serverKey) + .ciphers(TestUtils.preferredTestCiphers(), SupportedCipherSuiteFilter.INSTANCE) + .clientAuth(ClientAuth.REQUIRE) + .build(); + negotiator = ProtocolNegotiators.serverTls(sslContext); + startServer(); + + File caCert = TestUtils.loadCert("ca.pem"); + File clientCert = TestUtils.loadCert("client.pem"); + File clientKey = TestUtils.loadCert("client.key"); + SslContext clientContext = GrpcSslContexts.forClient() + .trustManager(caCert) + .ciphers(TestUtils.preferredTestCiphers(), SupportedCipherSuiteFilter.INSTANCE) + .keyManager(clientCert, clientKey) + .build(); + ProtocolNegotiator negotiator = ProtocolNegotiators.tls(clientContext, authority); + final NettyClientTransport transport = newTransport(negotiator); + callMeMaybe(transport.start(clientTransportListener)); + + Rpc rpc = new Rpc(transport).halfClose(); + try { + rpc.waitForClose(); + fail("expected exception"); + } catch (ExecutionException ex) { + StatusException sre = (StatusException) ex.getCause(); + assertEquals(Status.Code.UNAVAILABLE, sre.getStatus().getCode()); + assertThat(sre.getCause()).isInstanceOf(SSLHandshakeException.class); + assertThat(sre.getCause().getMessage()).contains("SSLV3_ALERT_HANDSHAKE_FAILURE"); + } + } + @Test public void channelExceptionDuringNegotiatonPropagatesToStatus() throws Exception { negotiator = ProtocolNegotiators.serverPlaintext(); @@ -521,8 +560,8 @@ private Throwable getRootCause(Throwable t) { } private ProtocolNegotiator newNegotiator() throws IOException { - File clientCert = TestUtils.loadCert("ca.pem"); - SslContext clientContext = GrpcSslContexts.forClient().trustManager(clientCert) + File caCert = TestUtils.loadCert("ca.pem"); + SslContext clientContext = GrpcSslContexts.forClient().trustManager(caCert) .ciphers(TestUtils.preferredTestCiphers(), SupportedCipherSuiteFilter.INSTANCE).build(); return ProtocolNegotiators.tls(clientContext, authority); }