From 56b9ae2629b0f5e5d7fe07ceb2e36d019405f91d Mon Sep 17 00:00:00 2001 From: Eric Anderson Date: Sat, 24 Mar 2018 11:18:12 -0700 Subject: [PATCH 1/2] okhttp: Convert to internal ConnectionSpec eagerly This allows ProGuard to remove OkHttp's ConnectionSpec in most cases, saving about 40 methods. --- .../java/io/grpc/okhttp/OkHttpChannelBuilder.java | 13 +++++++------ .../io/grpc/okhttp/OkHttpClientTransportTest.java | 2 +- 2 files changed, 8 insertions(+), 7 deletions(-) diff --git a/okhttp/src/main/java/io/grpc/okhttp/OkHttpChannelBuilder.java b/okhttp/src/main/java/io/grpc/okhttp/OkHttpChannelBuilder.java index d74defcc8ce..11f7944915d 100644 --- a/okhttp/src/main/java/io/grpc/okhttp/OkHttpChannelBuilder.java +++ b/okhttp/src/main/java/io/grpc/okhttp/OkHttpChannelBuilder.java @@ -23,9 +23,6 @@ import com.google.common.annotations.VisibleForTesting; import com.google.common.base.Preconditions; -import com.squareup.okhttp.CipherSuite; -import com.squareup.okhttp.ConnectionSpec; -import com.squareup.okhttp.TlsVersion; import io.grpc.Attributes; import io.grpc.ExperimentalApi; import io.grpc.Internal; @@ -40,7 +37,10 @@ import io.grpc.internal.SharedResourceHolder; import io.grpc.internal.SharedResourceHolder.Resource; import io.grpc.internal.TransportTracer; +import io.grpc.okhttp.internal.CipherSuite; +import io.grpc.okhttp.internal.ConnectionSpec; import io.grpc.okhttp.internal.Platform; +import io.grpc.okhttp.internal.TlsVersion; import java.net.InetSocketAddress; import java.net.SocketAddress; import java.security.GeneralSecurityException; @@ -280,9 +280,10 @@ public final OkHttpChannelBuilder hostnameVerifier(@Nullable HostnameVerifier ho * @throws IllegalArgumentException * If {@code connectionSpec} is not with TLS */ - public final OkHttpChannelBuilder connectionSpec(ConnectionSpec connectionSpec) { + public final OkHttpChannelBuilder connectionSpec( + com.squareup.okhttp.ConnectionSpec connectionSpec) { Preconditions.checkArgument(connectionSpec.isTls(), "plaintext ConnectionSpec is not accepted"); - this.connectionSpec = connectionSpec; + this.connectionSpec = Utils.convertSpec(connectionSpec); return this; } @@ -481,7 +482,7 @@ public void run() { executor, socketFactory, hostnameVerifier, - Utils.convertSpec(connectionSpec), + connectionSpec, maxMessageSize, proxy, tooManyPingsRunnable, diff --git a/okhttp/src/test/java/io/grpc/okhttp/OkHttpClientTransportTest.java b/okhttp/src/test/java/io/grpc/okhttp/OkHttpClientTransportTest.java index bac6a8e67c2..5e018da8b53 100644 --- a/okhttp/src/test/java/io/grpc/okhttp/OkHttpClientTransportTest.java +++ b/okhttp/src/test/java/io/grpc/okhttp/OkHttpClientTransportTest.java @@ -228,7 +228,7 @@ public void testToString() throws Exception { executor, sslSocketFactory, hostnameVerifier, - Utils.convertSpec(OkHttpChannelBuilder.DEFAULT_CONNECTION_SPEC), + OkHttpChannelBuilder.DEFAULT_CONNECTION_SPEC, DEFAULT_MAX_MESSAGE_SIZE, NO_PROXY, tooManyPingsRunnable, From c896ffc47510c6b7512d1c973ec6eb945ec51c1c Mon Sep 17 00:00:00 2001 From: Eric Anderson Date: Mon, 26 Mar 2018 13:41:00 -0700 Subject: [PATCH 2/2] Deprecate DEFAULT_CONNECTION_SPEC --- .../testing/integration/Http2OkHttpTest.java | 4 +- .../io/grpc/okhttp/OkHttpChannelBuilder.java | 38 +++++++++++++++++-- .../okhttp/OkHttpClientTransportTest.java | 2 +- 3 files changed, 37 insertions(+), 7 deletions(-) diff --git a/interop-testing/src/test/java/io/grpc/testing/integration/Http2OkHttpTest.java b/interop-testing/src/test/java/io/grpc/testing/integration/Http2OkHttpTest.java index 2f6f6e80cc7..b5e207f5e6f 100644 --- a/interop-testing/src/test/java/io/grpc/testing/integration/Http2OkHttpTest.java +++ b/interop-testing/src/test/java/io/grpc/testing/integration/Http2OkHttpTest.java @@ -23,7 +23,6 @@ import com.google.common.base.Throwables; import com.google.protobuf.EmptyProtos.Empty; import com.squareup.okhttp.ConnectionSpec; -import com.squareup.okhttp.TlsVersion; import io.grpc.ManagedChannel; import io.grpc.internal.AbstractServerImplBuilder; import io.grpc.internal.GrpcUtil; @@ -94,9 +93,8 @@ protected ManagedChannel createChannel() { private OkHttpChannelBuilder createChannelBuilder() { OkHttpChannelBuilder builder = OkHttpChannelBuilder.forAddress("localhost", getPort()) .maxInboundMessageSize(AbstractInteropTest.MAX_MESSAGE_SIZE) - .connectionSpec(new ConnectionSpec.Builder(OkHttpChannelBuilder.DEFAULT_CONNECTION_SPEC) + .connectionSpec(new ConnectionSpec.Builder(ConnectionSpec.MODERN_TLS) .cipherSuites(TestUtils.preferredTestCiphers().toArray(new String[0])) - .tlsVersions(ConnectionSpec.MODERN_TLS.tlsVersions().toArray(new TlsVersion[0])) .build()) .overrideAuthority(GrpcUtil.authorityFromHostAndPort( TestUtils.TEST_SERVER_HOST, getPort())); diff --git a/okhttp/src/main/java/io/grpc/okhttp/OkHttpChannelBuilder.java b/okhttp/src/main/java/io/grpc/okhttp/OkHttpChannelBuilder.java index 11f7944915d..2ac8ea3537e 100644 --- a/okhttp/src/main/java/io/grpc/okhttp/OkHttpChannelBuilder.java +++ b/okhttp/src/main/java/io/grpc/okhttp/OkHttpChannelBuilder.java @@ -62,7 +62,39 @@ public class OkHttpChannelBuilder extends AbstractManagedChannelImplBuilder { - public static final ConnectionSpec DEFAULT_CONNECTION_SPEC = + /** + * ConnectionSpec closely matching the default configuration that could be used as a basis for + * modification. + * + *

Since this field is the only reference in gRPC to ConnectionSpec that may not be ProGuarded, + * we are removing the field to reduce method count. We've been unable to find any existing users + * of the field, and any such user would highly likely at least be changing the cipher suites, + * which is sort of the only part that's non-obvious. Any existing user should instead create + * their own spec from scratch or base it off ConnectionSpec.MODERN_TLS if believed to be + * necessary. If this was providing you with value and don't want to see it removed, open a GitHub + * issue to discuss keeping it. + * + * @deprecated Deemed of little benefit and users weren't using it. Just define one yourself + */ + @Deprecated + public static final com.squareup.okhttp.ConnectionSpec DEFAULT_CONNECTION_SPEC = + new com.squareup.okhttp.ConnectionSpec.Builder(com.squareup.okhttp.ConnectionSpec.MODERN_TLS) + .cipherSuites( + // The following items should be sync with Netty's Http2SecurityUtil.CIPHERS. + com.squareup.okhttp.CipherSuite.TLS_ECDHE_ECDSA_WITH_AES_256_GCM_SHA384, + com.squareup.okhttp.CipherSuite.TLS_ECDHE_ECDSA_WITH_AES_128_GCM_SHA256, + com.squareup.okhttp.CipherSuite.TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384, + com.squareup.okhttp.CipherSuite.TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256, + com.squareup.okhttp.CipherSuite.TLS_DHE_RSA_WITH_AES_128_GCM_SHA256, + com.squareup.okhttp.CipherSuite.TLS_DHE_DSS_WITH_AES_128_GCM_SHA256, + com.squareup.okhttp.CipherSuite.TLS_DHE_RSA_WITH_AES_256_GCM_SHA384, + com.squareup.okhttp.CipherSuite.TLS_DHE_DSS_WITH_AES_256_GCM_SHA384) + .tlsVersions(com.squareup.okhttp.TlsVersion.TLS_1_2) + .supportsTlsExtensions(true) + .build(); + + @VisibleForTesting + static final ConnectionSpec INTERNAL_DEFAULT_CONNECTION_SPEC = new ConnectionSpec.Builder(ConnectionSpec.MODERN_TLS) .cipherSuites( // The following items should be sync with Netty's Http2SecurityUtil.CIPHERS. @@ -110,7 +142,7 @@ public static OkHttpChannelBuilder forTarget(String target) { private SSLSocketFactory sslSocketFactory; private HostnameVerifier hostnameVerifier; - private ConnectionSpec connectionSpec = DEFAULT_CONNECTION_SPEC; + private ConnectionSpec connectionSpec = INTERNAL_DEFAULT_CONNECTION_SPEC; private NegotiationType negotiationType = NegotiationType.TLS; private long keepAliveTimeNanos = KEEPALIVE_TIME_NANOS_DISABLED; private long keepAliveTimeoutNanos = DEFAULT_KEEPALIVE_TIMEOUT_NANOS; @@ -272,7 +304,7 @@ public final OkHttpChannelBuilder hostnameVerifier(@Nullable HostnameVerifier ho * For secure connection, provides a ConnectionSpec to specify Cipher suite and * TLS versions. * - *

By default {@link #DEFAULT_CONNECTION_SPEC} will be used. + *

By default a modern, HTTP/2-compatible spec will be used. * *

This method is only used when building a secure connection. For plaintext * connection, use {@link #usePlaintext()} instead. diff --git a/okhttp/src/test/java/io/grpc/okhttp/OkHttpClientTransportTest.java b/okhttp/src/test/java/io/grpc/okhttp/OkHttpClientTransportTest.java index 5e018da8b53..5be80b13700 100644 --- a/okhttp/src/test/java/io/grpc/okhttp/OkHttpClientTransportTest.java +++ b/okhttp/src/test/java/io/grpc/okhttp/OkHttpClientTransportTest.java @@ -228,7 +228,7 @@ public void testToString() throws Exception { executor, sslSocketFactory, hostnameVerifier, - OkHttpChannelBuilder.DEFAULT_CONNECTION_SPEC, + OkHttpChannelBuilder.INTERNAL_DEFAULT_CONNECTION_SPEC, DEFAULT_MAX_MESSAGE_SIZE, NO_PROXY, tooManyPingsRunnable,