Skip to content

Commit

Permalink
netty: Upgrade to Netty 4.1.52 and tcnative 2.0.34
Browse files Browse the repository at this point in the history
The tiny cache size was removed from the bytebuf allocator and so was
deprecated. TLSv1.3 was enabled by the upgrade, which fails mTLS
connections at different times. Conscrypt is incompatible with the
default TrustManager when TLSv1.3 is enabled so we explicitly disable
TLSv1.3 when Conscrypt is used for the moment.
  • Loading branch information
ejona86 committed Dec 29, 2020
1 parent cddc1a5 commit 8359d0b
Show file tree
Hide file tree
Showing 10 changed files with 70 additions and 28 deletions.
3 changes: 2 additions & 1 deletion SECURITY.md
Original file line number Diff line number Diff line change
Expand Up @@ -399,7 +399,8 @@ grpc-netty version | netty-handler version | netty-tcnative-boringssl-static ver
1.25.x-1.27.x | 4.1.42.Final | 2.0.26.Final
1.28.x | 4.1.45.Final | 2.0.28.Final
1.29.x-1.31.x | 4.1.48.Final | 2.0.30.Final
1.32.x- | 4.1.51.Final | 2.0.31.Final
1.32.x-1.34.x | 4.1.51.Final | 2.0.31.Final
1.35.x- | 4.1.52.Final | 2.0.34.Final

_(grpc-netty-shaded avoids issues with keeping these versions in sync.)_

Expand Down
4 changes: 2 additions & 2 deletions build.gradle
Original file line number Diff line number Diff line change
Expand Up @@ -54,7 +54,7 @@ subprojects {
protocPluginBaseName = 'protoc-gen-grpc-java'
javaPluginPath = "$rootDir/compiler/build/exe/java_plugin/$protocPluginBaseName$exeSuffix"

nettyVersion = '4.1.51.Final'
nettyVersion = '4.1.52.Final'
guavaVersion = '30.0-android'
googleauthVersion = '0.22.2'
protobufVersion = '3.12.0'
Expand Down Expand Up @@ -174,7 +174,7 @@ subprojects {
// SECURITY.md (multiple occurrences)
// examples/example-tls/build.gradle
// examples/example-tls/pom.xml
netty_tcnative: 'io.netty:netty-tcnative-boringssl-static:2.0.31.Final',
netty_tcnative: 'io.netty:netty-tcnative-boringssl-static:2.0.34.Final',

conscrypt: 'org.conscrypt:conscrypt-openjdk-uber:2.5.1',
re2j: 'com.google.re2j:re2j:1.5',
Expand Down
2 changes: 1 addition & 1 deletion examples/example-tls/build.gradle
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@ targetCompatibility = 1.7
// Feel free to delete the comment at the next line. It is just for safely
// updating the version in our release process.
def grpcVersion = '1.35.0-SNAPSHOT' // CURRENT_GRPC_VERSION
def nettyTcNativeVersion = '2.0.31.Final'
def nettyTcNativeVersion = '2.0.34.Final'
def protocVersion = '3.12.0'

dependencies {
Expand Down
2 changes: 1 addition & 1 deletion examples/example-tls/pom.xml
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@
<project.build.sourceEncoding>UTF-8</project.build.sourceEncoding>
<grpc.version>1.35.0-SNAPSHOT</grpc.version><!-- CURRENT_GRPC_VERSION -->
<protoc.version>3.12.0</protoc.version>
<netty.tcnative.version>2.0.31.Final</netty.tcnative.version>
<netty.tcnative.version>2.0.34.Final</netty.tcnative.version>
<!-- required for jdk9 -->
<maven.compiler.source>1.7</maven.compiler.source>
<maven.compiler.target>1.7</maven.compiler.target>
Expand Down
3 changes: 3 additions & 0 deletions netty/src/main/java/io/grpc/netty/GrpcSslContexts.java
Original file line number Diff line number Diff line change
Expand Up @@ -208,6 +208,9 @@ public static SslContextBuilder configure(SslContextBuilder builder, Provider jd
}
} else if (ConscryptLoader.isConscrypt(jdkProvider)) {
apc = ALPN;
// TODO: Conscrypt triggers failures in the TrustManager.
// https://github.com/grpc/grpc-java/issues/7765
builder.protocols("TLSv1.2");
} else {
throw new IllegalArgumentException("Unknown provider; can't configure: " + jdkProvider);
}
Expand Down
6 changes: 5 additions & 1 deletion netty/src/main/java/io/grpc/netty/Utils.java
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,7 @@
import io.netty.channel.nio.NioEventLoopGroup;
import io.netty.channel.socket.nio.NioServerSocketChannel;
import io.netty.channel.socket.nio.NioSocketChannel;
import io.netty.handler.codec.DecoderException;
import io.netty.handler.codec.http2.Http2Exception;
import io.netty.handler.codec.http2.Http2Headers;
import io.netty.util.AsciiString;
Expand All @@ -62,6 +63,7 @@
import java.util.logging.Logger;
import javax.annotation.CheckReturnValue;
import javax.annotation.Nullable;
import javax.net.ssl.SSLException;

/**
* Common utility methods.
Expand Down Expand Up @@ -165,7 +167,6 @@ private static ByteBufAllocator createByteBufAllocator(boolean preferDirect) {
preferDirect ? PooledByteBufAllocator.defaultNumDirectArena() : 0,
PooledByteBufAllocator.defaultPageSize(),
maxOrder,
PooledByteBufAllocator.defaultTinyCacheSize(),
PooledByteBufAllocator.defaultSmallCacheSize(),
PooledByteBufAllocator.defaultNormalCacheSize(),
PooledByteBufAllocator.defaultUseCacheForAllThreads());
Expand Down Expand Up @@ -267,6 +268,9 @@ public static Status statusFromThrowable(Throwable t) {
extraT.initCause(t);
return Status.UNKNOWN.withDescription("channel closed").withCause(extraT);
}
if (t instanceof DecoderException && t.getCause() instanceof SSLException) {
return Status.UNAVAILABLE.withDescription("ssl exception").withCause(t);
}
if (t instanceof IOException) {
return Status.UNAVAILABLE.withDescription("io exception").withCause(t);
}
Expand Down
11 changes: 9 additions & 2 deletions netty/src/test/java/io/grpc/netty/NettyClientTransportTest.java
Original file line number Diff line number Diff line change
Expand Up @@ -101,6 +101,7 @@
import java.util.concurrent.TimeUnit;
import java.util.concurrent.TimeoutException;
import javax.annotation.Nullable;
import javax.net.ssl.SSLException;
import javax.net.ssl.SSLHandshakeException;
import org.junit.After;
import org.junit.Before;
Expand Down Expand Up @@ -325,8 +326,14 @@ public void tlsNegotiationFailurePropagatesToStatus() throws 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");
if (sre.getCause() instanceof SSLHandshakeException) {
assertThat(sre).hasCauseThat().isInstanceOf(SSLHandshakeException.class);
assertThat(sre).hasCauseThat().hasMessageThat().contains("SSLV3_ALERT_HANDSHAKE_FAILURE");
} else {
// Client cert verification is after handshake in TLSv1.3
assertThat(sre).hasCauseThat().hasCauseThat().isInstanceOf(SSLException.class);
assertThat(sre).hasCauseThat().hasMessageThat().contains("CERTIFICATE_REQUIRED");
}
}
}

Expand Down
22 changes: 18 additions & 4 deletions netty/src/test/java/io/grpc/netty/TlsTest.java
Original file line number Diff line number Diff line change
Expand Up @@ -16,11 +16,13 @@

package io.grpc.netty;

import static com.google.common.truth.Truth.assertThat;
import static org.junit.Assert.assertEquals;
import static org.junit.Assert.fail;

import com.google.common.base.Throwables;
import com.google.common.util.concurrent.MoreExecutors;
import io.grpc.ConnectivityState;
import io.grpc.ManagedChannel;
import io.grpc.Server;
import io.grpc.ServerBuilder;
Expand Down Expand Up @@ -114,10 +116,6 @@ public void setUp() throws NoSuchAlgorithmException {
// Java 9+
} catch (ClassNotFoundException ignored) {
// Before Java 9
// TODO(ejona): remove this assume once we upgrade to Netty 4.1.50.Final. GrpcSslContexts
// detects the Java 9 ALPN API in Java 8 u252, but Netty does not support it in our
// current version
Assume.assumeTrue("Jetty ALPN not found", JettyTlsUtil.isJettyAlpnConfigured());
try {
GrpcSslContexts.configure(SslContextBuilder.forClient(), jdkProvider);
} catch (IllegalArgumentException ex) {
Expand Down Expand Up @@ -230,6 +228,12 @@ public void serverRejectsUntrustedClientCert() throws Exception {
Throwables.getStackTraceAsString(e),
Status.Code.UNAVAILABLE, e.getStatus().getCode());
}
// We really want to see TRANSIENT_FAILURE here, but if the test runs slowly the 1s backoff
// may be exceeded by the time the failure happens (since it counts from the start of the
// attempt). Even so, CONNECTING is a strong indicator that the handshake failed; otherwise we'd
// expect READY or IDLE.
assertThat(channel.getState(false))
.isAnyOf(ConnectivityState.TRANSIENT_FAILURE, ConnectivityState.CONNECTING);
}


Expand Down Expand Up @@ -271,6 +275,11 @@ public void noClientAuthFailure() throws Exception {
Throwables.getStackTraceAsString(e),
Status.Code.UNAVAILABLE, e.getStatus().getCode());
}
// We really want to see TRANSIENT_FAILURE here, but if the test runs slowly the 1s backoff
// may be exceeded by the time the failure happens (since it counts from the start of the
// attempt). Even so, CONNECTING is a strong indicator that the handshake failed; otherwise we'd
assertThat(channel.getState(false))
.isAnyOf(ConnectivityState.TRANSIENT_FAILURE, ConnectivityState.CONNECTING);
}


Expand Down Expand Up @@ -316,6 +325,11 @@ public void clientRejectsUntrustedServerCert() throws Exception {
Throwables.getStackTraceAsString(e),
Status.Code.UNAVAILABLE, e.getStatus().getCode());
}
// We really want to see TRANSIENT_FAILURE here, but if the test runs slowly the 1s backoff
// may be exceeded by the time the failure happens (since it counts from the start of the
// attempt). Even so, CONNECTING is a strong indicator that the handshake failed; otherwise we'd
assertThat(channel.getState(false))
.isAnyOf(ConnectivityState.TRANSIENT_FAILURE, ConnectivityState.CONNECTING);
}


Expand Down
24 changes: 12 additions & 12 deletions repositories.bzl
Original file line number Diff line number Diff line change
Expand Up @@ -23,18 +23,18 @@ IO_GRPC_GRPC_JAVA_ARTIFACTS = [
"com.google.truth:truth:1.0.1",
"com.squareup.okhttp:okhttp:2.7.4",
"com.squareup.okio:okio:1.17.5",
"io.netty:netty-buffer:4.1.51.Final",
"io.netty:netty-codec-http2:4.1.51.Final",
"io.netty:netty-codec-http:4.1.51.Final",
"io.netty:netty-codec-socks:4.1.51.Final",
"io.netty:netty-codec:4.1.51.Final",
"io.netty:netty-common:4.1.51.Final",
"io.netty:netty-handler-proxy:4.1.51.Final",
"io.netty:netty-handler:4.1.51.Final",
"io.netty:netty-resolver:4.1.51.Final",
"io.netty:netty-tcnative-boringssl-static:2.0.31.Final",
"io.netty:netty-transport-native-epoll:jar:linux-x86_64:4.1.51.Final",
"io.netty:netty-transport:4.1.51.Final",
"io.netty:netty-buffer:4.1.52.Final",
"io.netty:netty-codec-http2:4.1.52.Final",
"io.netty:netty-codec-http:4.1.52.Final",
"io.netty:netty-codec-socks:4.1.52.Final",
"io.netty:netty-codec:4.1.52.Final",
"io.netty:netty-common:4.1.52.Final",
"io.netty:netty-handler-proxy:4.1.52.Final",
"io.netty:netty-handler:4.1.52.Final",
"io.netty:netty-resolver:4.1.52.Final",
"io.netty:netty-tcnative-boringssl-static:2.0.34.Final",
"io.netty:netty-transport-native-epoll:jar:linux-x86_64:4.1.52.Final",
"io.netty:netty-transport:4.1.52.Final",
"io.opencensus:opencensus-api:0.24.0",
"io.opencensus:opencensus-contrib-grpc-metrics:0.24.0",
"io.perfmark:perfmark-api:0.23.0",
Expand Down
21 changes: 17 additions & 4 deletions xds/src/test/java/io/grpc/xds/XdsSdsClientServerTest.java
Original file line number Diff line number Diff line change
Expand Up @@ -64,6 +64,7 @@
import java.util.ArrayList;
import java.util.Arrays;
import java.util.List;
import javax.net.ssl.SSLException;
import javax.net.ssl.SSLHandshakeException;
import org.junit.After;
import org.junit.Before;
Expand Down Expand Up @@ -175,8 +176,14 @@ public void requireClientAuth_noClientCert_expectException()
unaryRpc(/* requestMessage= */ "buddy", blockingStub);
fail("exception expected");
} catch (StatusRuntimeException sre) {
assertThat(sre).hasCauseThat().isInstanceOf(SSLHandshakeException.class);
assertThat(sre).hasCauseThat().hasMessageThat().contains("HANDSHAKE_FAILURE");
if (sre.getCause() instanceof SSLHandshakeException) {
assertThat(sre).hasCauseThat().isInstanceOf(SSLHandshakeException.class);
assertThat(sre).hasCauseThat().hasMessageThat().contains("HANDSHAKE_FAILURE");
} else {
// Client cert verification is after handshake in TLSv1.3
assertThat(sre).hasCauseThat().hasCauseThat().isInstanceOf(SSLException.class);
assertThat(sre).hasCauseThat().hasMessageThat().contains("CERTIFICATE_REQUIRED");
}
}
}

Expand Down Expand Up @@ -206,8 +213,14 @@ public void mtls_badClientCert_expectException() throws IOException, URISyntaxEx
false);
fail("exception expected");
} catch (StatusRuntimeException sre) {
assertThat(sre).hasCauseThat().isInstanceOf(SSLHandshakeException.class);
assertThat(sre).hasCauseThat().hasMessageThat().contains("HANDSHAKE_FAILURE");
if (sre.getCause() instanceof SSLHandshakeException) {
assertThat(sre).hasCauseThat().isInstanceOf(SSLHandshakeException.class);
assertThat(sre).hasCauseThat().hasMessageThat().contains("HANDSHAKE_FAILURE");
} else {
// Client cert verification is after handshake in TLSv1.3
assertThat(sre).hasCauseThat().hasCauseThat().isInstanceOf(SSLException.class);
assertThat(sre).hasCauseThat().hasMessageThat().contains("CERTIFICATE_REQUIRED");
}
}
}

Expand Down

0 comments on commit 8359d0b

Please sign in to comment.