Merge pull request #1208 from nfuller/CipherSuiteSocketDefaultRevert

Modify the cipher suite / TLS version selection behavior

okhttp-tests/src/test/java/com/squareup/okhttp/ConnectionSpecTest.java

@@ -0,0 +1,172 @@
+/*
+ * Copyright (C) 2015 Square, Inc.
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *      http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package com.squareup.okhttp;
+
+import com.squareup.okhttp.internal.http.AuthenticatorAdapter;
+
+import org.junit.Test;
+
+import java.net.InetSocketAddress;
+import java.net.Proxy;
+import java.net.ProxySelector;
+import java.util.Arrays;
+import java.util.HashSet;
+import java.util.Set;
+import javax.net.ssl.SSLSocket;
+import javax.net.ssl.SSLSocketFactory;
+
+import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertFalse;
+import static org.junit.Assert.assertNull;
+import static org.junit.Assert.assertTrue;
+
+public final class ConnectionSpecTest {
+
+  private static final Proxy PROXY = Proxy.NO_PROXY;
+  private static final InetSocketAddress INET_SOCKET_ADDRESS =
+      InetSocketAddress.createUnresolved("host", 443);
+  private static final Address HTTPS_ADDRESS = new Address(
+      INET_SOCKET_ADDRESS.getHostString(), INET_SOCKET_ADDRESS.getPort(), null, null, null, null,
+      AuthenticatorAdapter.INSTANCE, PROXY, Arrays.asList(Protocol.HTTP_1_1),
+      Arrays.asList(ConnectionSpec.MODERN_TLS), ProxySelector.getDefault());
+
+  @Test
+  public void cleartextBuilder() throws Exception {
+    ConnectionSpec cleartextSpec = new ConnectionSpec.Builder(false).build();
+    assertFalse(cleartextSpec.isTls());
+  }
+
+  @Test
+  public void tlsBuilder_explicitCiphers() throws Exception {
+    ConnectionSpec tlsSpec = new ConnectionSpec.Builder(true)
+        .cipherSuites(CipherSuite.TLS_RSA_WITH_RC4_128_MD5)
+        .tlsVersions(TlsVersion.TLS_1_2)
+        .supportsTlsExtensions(true)
+        .build();
+    assertEquals(Arrays.asList(CipherSuite.TLS_RSA_WITH_RC4_128_MD5), tlsSpec.cipherSuites());
+    assertEquals(Arrays.asList(TlsVersion.TLS_1_2), tlsSpec.tlsVersions());
+    assertTrue(tlsSpec.supportsTlsExtensions());
+  }
+
+  @Test
+  public void tlsBuilder_defaultCiphers() throws Exception {
+    ConnectionSpec tlsSpec = new ConnectionSpec.Builder(true)
+        .tlsVersions(TlsVersion.TLS_1_2)
+        .supportsTlsExtensions(true)
+        .build();
+    assertNull(tlsSpec.cipherSuites());
+    assertEquals(Arrays.asList(TlsVersion.TLS_1_2), tlsSpec.tlsVersions());
+    assertTrue(tlsSpec.supportsTlsExtensions());
+  }
+
+  @Test
+  public void tls_defaultCiphers_noFallbackIndicator() throws Exception {
+    ConnectionSpec tlsSpec = new ConnectionSpec.Builder(true)
+        .tlsVersions(TlsVersion.TLS_1_2)
+        .supportsTlsExtensions(false)
+        .build();
+
+    SSLSocket socket = (SSLSocket) SSLSocketFactory.getDefault().createSocket();
+    socket.setEnabledCipherSuites(new String[] {
+        CipherSuite.TLS_RSA_WITH_RC4_128_MD5.javaName,
+        CipherSuite.TLS_RSA_WITH_RC4_128_SHA.javaName,
+    });
+    socket.setEnabledProtocols(new String[] {
+        TlsVersion.TLS_1_2.javaName,
+        TlsVersion.TLS_1_1.javaName,
+    });
+
+    Route route = new Route(HTTPS_ADDRESS, PROXY, INET_SOCKET_ADDRESS, tlsSpec,
+        false /* shouldSendTlsFallbackIndicator */);
+    tlsSpec.apply(socket, route);
+
+    assertEquals(createSet(TlsVersion.TLS_1_2.javaName), createSet(socket.getEnabledProtocols()));
+
+    Set<String> expectedCipherSet =
+        createSet(
+            CipherSuite.TLS_RSA_WITH_RC4_128_MD5.javaName,
+            CipherSuite.TLS_RSA_WITH_RC4_128_SHA.javaName);
+    assertEquals(expectedCipherSet, expectedCipherSet);
+  }
+
+  @Test
+  public void tls_defaultCiphers_withFallbackIndicator() throws Exception {
+    ConnectionSpec tlsSpec = new ConnectionSpec.Builder(true)
+        .tlsVersions(TlsVersion.TLS_1_2)
+        .supportsTlsExtensions(false)
+        .build();
+
+    SSLSocket socket = (SSLSocket) SSLSocketFactory.getDefault().createSocket();
+    socket.setEnabledCipherSuites(new String[] {
+        CipherSuite.TLS_RSA_WITH_RC4_128_MD5.javaName,
+        CipherSuite.TLS_RSA_WITH_RC4_128_SHA.javaName,
+    });
+    socket.setEnabledProtocols(new String[] {
+        TlsVersion.TLS_1_2.javaName,
+        TlsVersion.TLS_1_1.javaName,
+    });
+
+    Route route = new Route(HTTPS_ADDRESS, PROXY, INET_SOCKET_ADDRESS, tlsSpec,
+        true /* shouldSendTlsFallbackIndicator */);
+    tlsSpec.apply(socket, route);
+
+    assertEquals(createSet(TlsVersion.TLS_1_2.javaName), createSet(socket.getEnabledProtocols()));
+
+    Set<String> expectedCipherSet =
+        createSet(
+            CipherSuite.TLS_RSA_WITH_RC4_128_MD5.javaName,
+            CipherSuite.TLS_RSA_WITH_RC4_128_SHA.javaName);
+    if (Arrays.asList(socket.getSupportedCipherSuites()).contains("TLS_FALLBACK_SCSV")) {
+      expectedCipherSet.add("TLS_FALLBACK_SCSV");
+    }
+    assertEquals(expectedCipherSet, expectedCipherSet);
+  }
+
+  @Test
+  public void tls_explicitCiphers() throws Exception {
+    ConnectionSpec tlsSpec = new ConnectionSpec.Builder(true)
+        .cipherSuites(CipherSuite.TLS_RSA_WITH_RC4_128_MD5)
+        .tlsVersions(TlsVersion.TLS_1_2)
+        .supportsTlsExtensions(false)
+        .build();
+
+    SSLSocket socket = (SSLSocket) SSLSocketFactory.getDefault().createSocket();
+    socket.setEnabledCipherSuites(new String[] {
+        CipherSuite.TLS_RSA_WITH_RC4_128_MD5.javaName,
+        CipherSuite.TLS_RSA_WITH_RC4_128_SHA.javaName,
+    });
+    socket.setEnabledProtocols(new String[] {
+        TlsVersion.TLS_1_2.javaName,
+        TlsVersion.TLS_1_1.javaName,
+    });
+
+    Route route = new Route(HTTPS_ADDRESS, PROXY, INET_SOCKET_ADDRESS, tlsSpec,
+        true /* shouldSendTlsFallbackIndicator */);
+    tlsSpec.apply(socket, route);
+
+    assertEquals(createSet(TlsVersion.TLS_1_2.javaName), createSet(socket.getEnabledProtocols()));
+
+    Set<String> expectedCipherSet = createSet(CipherSuite.TLS_RSA_WITH_RC4_128_MD5.javaName);
+    if (Arrays.asList(socket.getSupportedCipherSuites()).contains("TLS_FALLBACK_SCSV")) {
+      expectedCipherSet.add("TLS_FALLBACK_SCSV");
+    }
+    assertEquals(expectedCipherSet, expectedCipherSet);
+  }
+
+  private static Set<String> createSet(String... values) {
+    return new HashSet<String>(Arrays.asList(values));
+  }
+}

okhttp/src/main/java/com/squareup/okhttp/ConnectionSpec.java

@@ -66,16 +66,17 @@ public final class ConnectionSpec {
   public static final ConnectionSpec CLEARTEXT = new Builder(false).build();
 
   final boolean tls;
-  private final String[] cipherSuites;
-  private final String[] tlsVersions;
-  final boolean supportsTlsExtensions;
 
   /**
-   * Caches the subset of this spec that's supported by the host platform. It's possible that the
-   * platform hosts multiple implementations of {@link SSLSocket}, in which case this cache will be
-   * incorrect.
+   * Used if tls == true. The cipher suites to set on the SSLSocket. {@code null} means "use
+   * default set".
    */
-  private ConnectionSpec supportedSpec;
+  private final String[] cipherSuites;
+
+  /** Used if tls == true. The TLS protocol versions to use. */
+  private final String[] tlsVersions;
+
+  final boolean supportsTlsExtensions;
 
   private ConnectionSpec(Builder builder) {
     this.tls = builder.tls;
@@ -89,6 +90,9 @@ public final class ConnectionSpec {
   }
 
   public List<CipherSuite> cipherSuites() {
+    if (cipherSuites == null) {
+      return null;
+    }
     CipherSuite[] result = new CipherSuite[cipherSuites.length];
     for (int i = 0; i < cipherSuites.length; i++) {
       result[i] = CipherSuite.forJavaName(cipherSuites[i]);
@@ -110,11 +114,7 @@ public final class ConnectionSpec {
 
   /** Applies this spec to {@code sslSocket} for {@code route}. */
   void apply(SSLSocket sslSocket, Route route) {
-    ConnectionSpec specToApply = supportedSpec;
-    if (specToApply == null) {
-      specToApply = supportedSpec(sslSocket);
-      supportedSpec = specToApply;
-    }
+    ConnectionSpec specToApply = supportedSpec(sslSocket);
 
     sslSocket.setEnabledProtocols(specToApply.tlsVersions);
 
@@ -129,6 +129,9 @@ public final class ConnectionSpec {
       if (socketSupportsFallbackScsv) {
         // Add the SCSV cipher to the set of enabled ciphers iff it is supported.
         String[] oldEnabledCipherSuites = cipherSuitesToEnable;
+        if (cipherSuitesToEnable == null) {
+          oldEnabledCipherSuites = sslSocket.getEnabledCipherSuites();
+        }
         String[] newEnabledCipherSuites = new String[oldEnabledCipherSuites.length + 1];
         System.arraycopy(oldEnabledCipherSuites, 0,
             newEnabledCipherSuites, 0, oldEnabledCipherSuites.length);
@@ -136,7 +139,10 @@ public final class ConnectionSpec {
         cipherSuitesToEnable = newEnabledCipherSuites;
       }
     }
-    sslSocket.setEnabledCipherSuites(cipherSuitesToEnable);
+    // null means "use default set".
+    if (cipherSuitesToEnable != null) {
+      sslSocket.setEnabledCipherSuites(cipherSuitesToEnable);
+    }
 
     Platform platform = Platform.get();
     if (specToApply.supportsTlsExtensions) {
@@ -146,16 +152,23 @@ public final class ConnectionSpec {
 
   /**
    * Returns a copy of this that omits cipher suites and TLS versions not
-   * supported by {@code sslSocket}.
+   * enabled by {@code sslSocket}.
    */
   private ConnectionSpec supportedSpec(SSLSocket sslSocket) {
-    List<String> supportedCipherSuites =
-        Util.intersect(cipherSuites, sslSocket.getSupportedCipherSuites());
-    List<String> supportedTlsVersions =
-        Util.intersect(tlsVersions, sslSocket.getSupportedProtocols());
+    String[] cipherSuitesToEnable = null;
+    if (cipherSuites != null) {
+      String[] cipherSuitesToSelectFrom = sslSocket.getEnabledCipherSuites();
+      List<String> cipherSuitesToEnableList =
+          Util.intersect(cipherSuites, cipherSuitesToSelectFrom);
+      cipherSuitesToEnable = cipherSuitesToEnableList.toArray(
+          new String[cipherSuitesToEnableList.size()]);
+    }
+
+    String[] protocolsToSelectFrom = sslSocket.getEnabledProtocols();
+    List<String> tlsVersionsToEnable = Util.intersect(tlsVersions, protocolsToSelectFrom);
     return new Builder(this)
-        .cipherSuites(supportedCipherSuites.toArray(new String[supportedCipherSuites.size()]))
-        .tlsVersions(supportedTlsVersions.toArray(new String[supportedTlsVersions.size()]))
+        .cipherSuites(cipherSuitesToEnable)
+        .tlsVersions(tlsVersionsToEnable.toArray(new String[tlsVersionsToEnable.size()]))
         .build();
   }
 
@@ -186,7 +199,9 @@ public final class ConnectionSpec {
 
   @Override public String toString() {
     if (tls) {
-      return "ConnectionSpec(cipherSuites=" + cipherSuites()
+      List<CipherSuite> cipherSuites = cipherSuites();
+      String cipherSuitesString = cipherSuites == null ? "[use default]" : cipherSuites.toString();
+      return "ConnectionSpec(cipherSuites=" + cipherSuitesString
           + ", tlsVersions=" + tlsVersions()
           + ", supportsTlsExtensions=" + supportsTlsExtensions
           + ")";
@@ -201,7 +216,7 @@ public final class ConnectionSpec {
     private String[] tlsVersions;
     private boolean supportsTlsExtensions;
 
-    private Builder(boolean tls) {
+    Builder(boolean tls) {
       this.tls = tls;
     }