Skip to content

Commit

Permalink
Modify the cipher suite / TLS version selection behavior
Browse files Browse the repository at this point in the history
Motivation 1: Cipher selection and TLS protocol version selection
should be performed over "enabled", not "support" values. Otherwise,
ciphers / protocols that are disabled by default are re-enabled.
Android would like to avoid enabling values that are considered unsafe
and are disabled by the platform, apps or GMS core.

Motivation 2: Opinionated cipher suite selection makes sense for
OkHttp when bundled with apps. OkHttp probably has the best/newest
information available when compared with the socket factory it
encounters.

However, with the Android platform usecase the socket factory (i.e.
one from the platform, from GMS core, or installed by the app) might
have better information (or reasons why specific suites should not be
used and so it modifies the "enabled" set). Android would therefore
prefer OkHttp to select from a default set specified by the socket
factory, not a list hard-coded at release time.

Unlike ciphers, for protocols there is no current need to
go with the default enabled set so supporting "null" / "use default"
is not required for protocols.

Other change:

The ConnectionSpec caching (see supportedSpec) has been removed from
ConnectionSpec. This seems like an risky optimization to me in an
environment where different SocketFactory instances could be encountered
and doing it properly seems costly and of limited benefit.
  • Loading branch information
nfuller committed Jan 7, 2015
1 parent 5f44ca7 commit f695ea7
Show file tree
Hide file tree
Showing 2 changed files with 209 additions and 22 deletions.
172 changes: 172 additions & 0 deletions okhttp-tests/src/test/java/com/squareup/okhttp/ConnectionSpecTest.java
Original file line number Diff line number Diff line change
@@ -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));
}
}
59 changes: 37 additions & 22 deletions okhttp/src/main/java/com/squareup/okhttp/ConnectionSpec.java
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -89,6 +90,9 @@ public boolean isTls() {
}

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]);
Expand All @@ -110,11 +114,7 @@ public boolean supportsTlsExtensions() {

/** 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);

Expand All @@ -129,14 +129,20 @@ void apply(SSLSocket sslSocket, Route route) {
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);
newEnabledCipherSuites[newEnabledCipherSuites.length - 1] = fallbackScsv;
cipherSuitesToEnable = newEnabledCipherSuites;
}
}
sslSocket.setEnabledCipherSuites(cipherSuitesToEnable);
// null means "use default set".
if (cipherSuitesToEnable != null) {
sslSocket.setEnabledCipherSuites(cipherSuitesToEnable);
}

Platform platform = Platform.get();
if (specToApply.supportsTlsExtensions) {
Expand All @@ -146,16 +152,23 @@ void apply(SSLSocket sslSocket, Route route) {

/**
* 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();
}

Expand Down Expand Up @@ -186,7 +199,9 @@ private ConnectionSpec supportedSpec(SSLSocket sslSocket) {

@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
+ ")";
Expand All @@ -201,7 +216,7 @@ public static final class Builder {
private String[] tlsVersions;
private boolean supportsTlsExtensions;

private Builder(boolean tls) {
Builder(boolean tls) {
this.tls = tls;
}

Expand Down

0 comments on commit f695ea7

Please sign in to comment.