Implement TestRootCerts for Android

net/android/java/src/org/chromium/net/X509Util.java

Index: net/android/java/src/org/chromium/net/X509Util.java
diff --git a/net/android/java/src/org/chromium/net/X509Util.java b/net/android/java/src/org/chromium/net/X509Util.java
index 0c43b29b65e9bb860b5305d9a9dc357b940c4ad3..91acf3f36641a97c61ec65fc1b3ea9779274bd7e 100644
--- a/net/android/java/src/org/chromium/net/X509Util.java
+++ b/net/android/java/src/org/chromium/net/X509Util.java
@@ -7,6 +7,7 @@ package org.chromium.net;
 import android.util.Log;
 
 import java.io.ByteArrayInputStream;
+import java.io.IOException;
 import java.security.KeyStore;
 import java.security.KeyStoreException;
 import java.security.NoSuchAlgorithmException;
@@ -25,37 +26,56 @@ public class X509Util {
     private static CertificateFactory sCertificateFactory;
 
     /**
-     * Default sources of authentication trust decisions and certificate object
-     * creation.
+     * Trust manager backed up by the read-only system certificate store.
      */
     private static X509TrustManager sDefaultTrustManager;
 
     /**
-     * Ensures that |sCertificateFactory| and |sDefaultTrustManager| are
-     * initialized.
+     * Trust manager backed up by a custom certificate store. We need such manager to plant test
+     * root CA to the trust store in testing.
      */
-    private static synchronized void ensureInitialized() throws CertificateException,
+    private static X509TrustManager sTestTrustManager;
+    private static KeyStore sTestKeyStore;
+
+    /**
+     * Lock object used to synchronize all calls that modify or depend on the trust managers.
+     */
+    private static final Object sLock = new Object();
+
+    /**
+     * Ensures that the trust managers and certificate factory are initialized.
+     */
+    private static void ensureInitialized() throws CertificateException,
             KeyStoreException, NoSuchAlgorithmException {
-        if (sCertificateFactory == null) {
-            sCertificateFactory = CertificateFactory.getInstance("X.509");
-        }
-        if (sDefaultTrustManager == null) {
-            sDefaultTrustManager = X509Util.createDefaultTrustManager();
+        synchronized(sLock) {
+            if (sCertificateFactory == null) {
+                sCertificateFactory = CertificateFactory.getInstance("X.509");
+            }
+            if (sDefaultTrustManager == null) {
+                sDefaultTrustManager = X509Util.createTrustManager(null);
+            }
+            if (sTestKeyStore == null) {
+                sTestKeyStore = KeyStore.getInstance(KeyStore.getDefaultType());
+                try {
+                    sTestKeyStore.load(null);
+                } catch(IOException e) {}  // No IO operation is attempted.
+            }
+            if (sTestTrustManager == null) {
+                sTestTrustManager = X509Util.createTrustManager(sTestKeyStore);
+            }
         }
     }
 
     /**
-     * Creates a TrustManagerFactory and returns the X509TrustManager instance
-     * if one can be found.
-     *
-     * @throws CertificateException,KeyStoreException,NoSuchAlgorithmException
-     *             on error initializing the TrustManager.
+     * Creates a X509TrustManager backed up by the given key store. When null is passed as a key
+     * store, system default trust store is used.
+     * @throws KeyStoreException, NoSuchAlgorithmException on error initializing the TrustManager.
      */
-    private static X509TrustManager createDefaultTrustManager()
-            throws KeyStoreException, NoSuchAlgorithmException {
+    private static X509TrustManager createTrustManager(KeyStore keyStore) throws KeyStoreException,
+            NoSuchAlgorithmException {
         String algorithm = TrustManagerFactory.getDefaultAlgorithm();
         TrustManagerFactory tmf = TrustManagerFactory.getInstance(algorithm);
-        tmf.init((KeyStore) null);
+        tmf.init(keyStore);
 
         for (TrustManager tm : tmf.getTrustManagers()) {
             if (tm instanceof X509TrustManager) {
@@ -66,7 +86,7 @@ public class X509Util {
     }
 
     /**
-     * Convert a DER encoded certificate to an X509Certificate
+     * Convert a DER encoded certificate to an X509Certificate.
      */
     public static X509Certificate createCertificateFromBytes(byte[] derBytes) throws
             CertificateException, KeyStoreException, NoSuchAlgorithmException {
@@ -75,6 +95,26 @@ public class X509Util {
                 new ByteArrayInputStream(derBytes));
     }
 
+    public static void addTestRootCertificate(byte[] rootCertBytes) throws CertificateException,
+            KeyStoreException, NoSuchAlgorithmException {
+        ensureInitialized();
+        X509Certificate rootCert = createCertificateFromBytes(rootCertBytes);
+        synchronized(sLock) {
+            sTestKeyStore.setCertificateEntry(
+                    "root_cert_" + Integer.toString(sTestKeyStore.size()), rootCert);
+        }
+    }
+
+    public static void clearTestRootCertificates() throws NoSuchAlgorithmException,
+            CertificateException, KeyStoreException {
+        ensureInitialized();
+        synchronized(sLock) {
+            try {
+                sTestKeyStore.load(null);
+            } catch(IOException e) {}  // No IO operation is attempted.
+        }
+    }
+
     public static boolean verifyServerCertificates(byte[][] certChain, String authType)
             throws CertificateException, KeyStoreException, NoSuchAlgorithmException {
         if (certChain == null || certChain.length == 0 || certChain[0] == null) {
@@ -88,14 +128,24 @@ public class X509Util {
             serverCertificates[i] = createCertificateFromBytes(certChain[i]);
         }
 
-        try {
-            sDefaultTrustManager.checkServerTrusted(serverCertificates, authType);
-            return true;
-        } catch (CertificateException e) {
-            Log.i(TAG, "failed to validate the certificate chain, error: " +
-                    e.getMessage());
+        synchronized (sLock) {
+            try {
+                sDefaultTrustManager.checkServerTrusted(serverCertificates, authType);
+                return true;
+            } catch (CertificateException eDefaultManager) {
+                try {
+                    sTestTrustManager.checkServerTrusted(serverCertificates, authType);

[Ryan Sleevi, 2012/12/07 03:25:26]

My ignorance in Java is showing here, but does the sTestKeyStore also
consider/include system keys? Or is it an empty key store?

The only way this would become an issue is if sDefaultTrustManager also had an
intermediate certificate store that checkServerTrusted could use for path
building, but not trust. If that's not the case, then I agree, this
implementation should be fine.

[digit1, 2012/12/07 10:28:11]

The Java and Android API documentation is not very clear, so I looked at the
platform sources for details. A KeyStore is a pretty generic abstract interface,
and several implementations exist, differentiated by a "type" string.

KeyStore.getDefaultType() returns "jks", which is a file-backed KeyStore
implementation, while the Android system keystore (which runs in a separate
process) uses the "AndroidKeyStore" type.

So in the end, the test keystore in this existing code is completely empty, and
thus separated from the platform one.

I'm curious why we'd want to use platform certificate during unit testing
though? Isn't this bound to be sort of flaky? I understand that we might want to
check that a really well-known public CA certificate is installed on the test
device, but this would rather use a specialized test, rather than the
net::TestRootCerts class.

Or maybe there is a use case I don't envision?

[ppi, 2012/12/11 04:24:21]

That's an interesting remark, Ryan. I agree with David that the Java docs are
far from clear on these issues... 

TrustManagerFactory interface clearly does not expose any way of feeding
intermediate cert store. Moreover, KeyStore itself stores (all) its certificates
in instances of TrustedCertificateEntry - my working hypothesis is that that
Java does not support intermediate cert stores (in the context of trust
managers) at all - does it sound probable?

I second David's concern that relying on system trust roots in tests seems
controversial (I am not aware of any tests doing so).

[Ryan Sleevi, 2012/12/11 17:40:10]

The goal here with TestRootCerts is to make sure that, as consistently as
possible, the 'native' code path is being executed (in particular, chain
building and configuration)

It has less to do with trusting system keys (we don't want unit tests to depend
on external configuration), but DOES have to do with making sure that we're
exercising the same codepaths (that the test roots are not just some simple
'return true')

The Java abstraction may separate out these concepts, but on the other
platforms, it doesn't, hence why we intentionally "plug into" the system
implementation rather than "replace".

+                    return true;
+                } catch (CertificateException eTestManager) {
+                    /*
+                     *  Neither of the trust managers confirms the validity of the certificate
+                     *  chain, we emit the error message returned by the system trust manager.
+                     */
+                    Log.i(TAG, "failed to validate the certificate chain, error: " +
+                               eDefaultManager.getMessage());
+                }
+            }
         }
         return false;
     }
-
-}
+}