Skip to content

Commit

Permalink
Merge pull request square#2445 from square/jwilson_0328_android_cert_…
Browse files Browse the repository at this point in the history 
…chain_cleaner

Use X509TrustManagerExtensions on Android 17+.
  • Loading branch information
@swankjesse
swankjesse committed Mar 28, 2016
2 parents 338ad7f + cbd3309 commit e5b7409
Show file tree
Hide file tree
Showing 4 changed files with 71 additions and 20 deletions.
33 changes: 18 additions & 15 deletions okhttp-tests/src/test/java/okhttp3/CertificateChainCleanerTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,7 @@ public final class CertificateChainCleanerTest {
.serialNumber("1")
.build();
CertificateChainCleaner cleaner = CertificateChainCleaner.get(root.certificate);
assertEquals(list(root), cleaner.clean(list(root)));
assertEquals(list(root), cleaner.clean(list(root), "hostname"));
}

@Test public void normalizeUnknownSelfSignedCertificate() throws Exception {
Expand All @@ -44,7 +44,7 @@ public final class CertificateChainCleanerTest {
CertificateChainCleaner cleaner = CertificateChainCleaner.get();

try {
cleaner.clean(list(root));
cleaner.clean(list(root), "hostname");
fail();
} catch (SSLPeerUnverifiedException expected) {
}
Expand All @@ -64,7 +64,7 @@ public final class CertificateChainCleanerTest {
.build();

CertificateChainCleaner cleaner = CertificateChainCleaner.get(root.certificate);
assertEquals(list(certB, certA, root), cleaner.clean(list(certB, certA, root)));
assertEquals(list(certB, certA, root), cleaner.clean(list(certB, certA, root), "hostname"));
}

@Test public void orderedChainOfCertificatesWithoutRoot() throws Exception {
Expand All @@ -81,7 +81,8 @@ public final class CertificateChainCleanerTest {
.build();

CertificateChainCleaner cleaner = CertificateChainCleaner.get(root.certificate);
assertEquals(list(certB, certA, root), cleaner.clean(list(certB, certA))); // Root is added!
assertEquals(list(certB, certA, root),
cleaner.clean(list(certB, certA), "hostname")); // Root is added!
}

@Test public void unorderedChainOfCertificatesWithRoot() throws Exception {
Expand All @@ -102,7 +103,8 @@ public final class CertificateChainCleanerTest {
.build();

CertificateChainCleaner cleaner = CertificateChainCleaner.get(root.certificate);
assertEquals(list(certC, certB, certA, root), cleaner.clean(list(certC, certA, root, certB)));
assertEquals(list(certC, certB, certA, root),
cleaner.clean(list(certC, certA, root, certB), "hostname"));
}

@Test public void unorderedChainOfCertificatesWithoutRoot() throws Exception {
Expand All @@ -123,7 +125,8 @@ public final class CertificateChainCleanerTest {
.build();

CertificateChainCleaner cleaner = CertificateChainCleaner.get(root.certificate);
assertEquals(list(certC, certB, certA, root), cleaner.clean(list(certC, certA, certB)));
assertEquals(list(certC, certB, certA, root),
cleaner.clean(list(certC, certA, certB), "hostname"));
}

@Test public void unrelatedCertificatesAreOmitted() throws Exception {
Expand All @@ -144,7 +147,7 @@ public final class CertificateChainCleanerTest {

CertificateChainCleaner cleaner = CertificateChainCleaner.get(root.certificate);
assertEquals(list(certB, certA, root),
cleaner.clean(list(certB, certUnnecessary, certA, root)));
cleaner.clean(list(certB, certUnnecessary, certA, root), "hostname"));
}

@Test public void chainGoesAllTheWayToSelfSignedRoot() throws Exception {
Expand All @@ -167,11 +170,11 @@ public final class CertificateChainCleanerTest {
CertificateChainCleaner cleaner = CertificateChainCleaner.get(
selfSigned.certificate, trusted.certificate);
assertEquals(list(certB, certA, trusted, selfSigned),
cleaner.clean(list(certB, certA)));
cleaner.clean(list(certB, certA), "hostname"));
assertEquals(list(certB, certA, trusted, selfSigned),
cleaner.clean(list(certB, certA, trusted)));
cleaner.clean(list(certB, certA, trusted), "hostname"));
assertEquals(list(certB, certA, trusted, selfSigned),
cleaner.clean(list(certB, certA, trusted, selfSigned)));
cleaner.clean(list(certB, certA, trusted, selfSigned), "hostname"));
}

@Test public void trustedRootNotSelfSigned() throws Exception {
Expand All @@ -193,9 +196,9 @@ public final class CertificateChainCleanerTest {

CertificateChainCleaner cleaner = CertificateChainCleaner.get(trusted.certificate);
assertEquals(list(certificate, intermediateCa, trusted),
cleaner.clean(list(certificate, intermediateCa)));
cleaner.clean(list(certificate, intermediateCa), "hostname"));
assertEquals(list(certificate, intermediateCa, trusted),
cleaner.clean(list(certificate, intermediateCa, trusted)));
cleaner.clean(list(certificate, intermediateCa, trusted), "hostname"));
}

@Test public void chainMaxLength() throws Exception {
Expand All @@ -207,8 +210,8 @@ public final class CertificateChainCleanerTest {

X509Certificate root = heldCertificates.get(heldCertificates.size() - 1).certificate;
CertificateChainCleaner cleaner = CertificateChainCleaner.get(root);
assertEquals(certificates, cleaner.clean(certificates));
assertEquals(certificates, cleaner.clean(certificates.subList(0, 9)));
assertEquals(certificates, cleaner.clean(certificates, "hostname"));
assertEquals(certificates, cleaner.clean(certificates.subList(0, 9), "hostname"));
}

@Test public void chainTooLong() throws Exception {
Expand All @@ -221,7 +224,7 @@ public final class CertificateChainCleanerTest {
X509Certificate root = heldCertificates.get(heldCertificates.size() - 1).certificate;
CertificateChainCleaner cleaner = CertificateChainCleaner.get(root);
try {
cleaner.clean(certificates);
cleaner.clean(certificates, "hostname");
fail();
} catch (SSLPeerUnverifiedException expected) {
}
Expand Down
2 changes: 1 addition & 1 deletion okhttp/src/main/java/okhttp3/CertificatePinner.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -145,7 +145,7 @@ public void check(String hostname, List<Certificate> peerCertificates)
if (pins.isEmpty()) return;

if (certificateChainCleaner != null) {
peerCertificates = certificateChainCleaner.clean(peerCertificates);
peerCertificates = certificateChainCleaner.clean(peerCertificates, hostname);
}

for (int c = 0, certsSize = peerCertificates.size(); c < certsSize; c++) {
Expand Down
52 changes: 48 additions & 4 deletions okhttp/src/main/java/okhttp3/internal/tls/CertificateChainCleaner.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,9 @@
*/
package okhttp3.internal.tls;

import java.lang.reflect.Constructor;
import java.lang.reflect.InvocationTargetException;
import java.lang.reflect.Method;
import java.security.GeneralSecurityException;
import java.security.cert.Certificate;
import java.security.cert.X509Certificate;
Expand All @@ -38,11 +41,20 @@
* pinning.
*/
public abstract class CertificateChainCleaner {
public abstract List<Certificate> clean(List<Certificate> chain)
public abstract List<Certificate> clean(List<Certificate> chain, String hostname)
throws SSLPeerUnverifiedException;

public static CertificateChainCleaner get(X509TrustManager trustManager) {
return new BasicCertificateChainCleaner(TrustRootIndex.get(trustManager));
try {
Class<?> extensionsClass = Class.forName("android.net.http.X509TrustManagerExtensions");
Constructor<?> constructor = extensionsClass.getConstructor(X509TrustManager.class);
Object extensions = constructor.newInstance(trustManager);
Method checkServerTrusted = extensionsClass.getMethod(
"checkServerTrusted", X509Certificate[].class, String.class, String.class);
return new AndroidCertificateChainCleaner(extensions, checkServerTrusted);
} catch (Exception e) {
return new BasicCertificateChainCleaner(TrustRootIndex.get(trustManager));
}
}

public static CertificateChainCleaner get(X509Certificate... caCerts) {
Expand All @@ -51,7 +63,8 @@ public static CertificateChainCleaner get(X509Certificate... caCerts) {

/**
* A certificate chain cleaner that uses a set of trusted root certificates to build the trusted
* chain.
* chain. This class duplicates the clean chain building performed during the TLS handshake. We
* prefer other mechanisms where they exist, such as with {@link AndroidCertificateChainCleaner}.
*
* <p>This class includes code from <a href="https://conscrypt.org/">Conscrypt's</a> {@code
* TrustManagerImpl} and {@code TrustedCertificateIndex}.
Expand All @@ -73,7 +86,7 @@ public BasicCertificateChainCleaner(TrustRootIndex trustRootIndex) {
* constructed. This is unexpected unless the trust root index in this class has a different
* trust manager than what was used to establish {@code chain}.
*/
@Override public List<Certificate> clean(List<Certificate> chain)
@Override public List<Certificate> clean(List<Certificate> chain, String hostname)
throws SSLPeerUnverifiedException {
Deque<Certificate> queue = new ArrayDeque<>(chain);
List<Certificate> result = new ArrayList<>();
Expand Down Expand Up @@ -134,4 +147,35 @@ private boolean verifySignature(X509Certificate toVerify, X509Certificate signin
}
}
}

/**
* X509TrustManagerExtensions was added to Android in API 17 (Android 4.2, released in late 2012).
* This is the best way to get a clean chain on Android because it uses the same code as the TLS
* handshake.
*/
static final class AndroidCertificateChainCleaner extends CertificateChainCleaner {
private final Object x509TrustManagerExtensions;
private final Method checkServerTrusted;

AndroidCertificateChainCleaner(Object x509TrustManagerExtensions, Method checkServerTrusted) {
this.x509TrustManagerExtensions = x509TrustManagerExtensions;
this.checkServerTrusted = checkServerTrusted;
}

@SuppressWarnings({"unchecked", "SuspiciousToArrayCall"}) // Reflection on List<Certificate>.
@Override public List<Certificate> clean(List<Certificate> chain, String hostname)
throws SSLPeerUnverifiedException {
try {
X509Certificate[] certificates = chain.toArray(new X509Certificate[chain.size()]);
return (List<Certificate>) checkServerTrusted.invoke(
x509TrustManagerExtensions, certificates, "RSA", hostname);
} catch (InvocationTargetException e) {
SSLPeerUnverifiedException exception = new SSLPeerUnverifiedException(e.getMessage());
exception.initCause(e);
throw exception;
} catch (IllegalAccessException e) {
throw new AssertionError(e);
}
}
}
}
4 changes: 4 additions & 0 deletions okhttp/src/main/java/okhttp3/internal/tls/TrustRootIndex.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,10 @@ public static TrustRootIndex get(X509Certificate... caCerts) {
* An index of trusted root certificates that exploits knowledge of Android implementation
* details. This class is potentially much faster to initialize than {@link BasicTrustRootIndex}
* because it doesn't need to load and index trusted CA certificates.
*
* <p>This class uses APIs added to Android in API 14 (Android 4.0, released October 2011). This
* class shouldn't be used in Android API 17 or better because those releases are better served by
* {@link CertificateChainCleaner.AndroidCertificateChainCleaner}.
*/
static final class AndroidTrustRootIndex extends TrustRootIndex {
private final X509TrustManager trustManager;
Expand Down

0 comments on commit e5b7409

Please sign in to comment.