Simplify KeyChain API by removing now unneeded CA certificate lookup (3 of 3)

frameworks/base

   Remove getCaCertificates and findIssuer from IKeyChainService,
   these are now done via libcore's TrustedCertificateStore (as part
   of the default TrustManager implementation)

	keystore/java/android/security/IKeyChainService.aidl

   Simplify KeyChain API. Now that the CA certificates are visible
   through the default TrustManager, the KeyChain is solely focused on
   retrieving PrivateKeys and their associated certificates. The
   calling API for KeyChain to simply a single KeyChain.get() call
   that returns a KeyChainResult, removing the need for a KeyChain
   instance that needs to be closed.

	keystore/java/android/security/KeyChain.java
	keystore/java/android/security/KeyChainResult.java

master/libcore

    Remove getDefaultIndexedPKIXParameters and
    getIndexedPKIXParameters which was used as part of the prototype
    of looking up CAs via the KeyChain but is obsoleted by the new
    default TrustManager implementation.

	luni/src/main/java/org/apache/harmony/xnet/provider/jsse/SSLParametersImpl.java
	luni/src/main/java/org/apache/harmony/xnet/provider/jsse/TrustManagerImpl.java

packages/apps/KeyChain

    Tracking simplified IKeyChainService, removing now unneeded
    implementation, updating tests.

	src/com/android/keychain/KeyChainService.java
	tests/src/com/android/keychain/tests/KeyChainServiceTest.java
	tests/src/com/android/keychain/tests/KeyChainTestActivity.java

Change-Id: Ie2cb950783f897d87d39cc38a126068a9d68680a

3 files changed, 83 insertions, 204 deletions

diff --git a/src/com/android/keychain/KeyChainService.java b/src/com/android/keychain/KeyChainService.java
index 1190368..3b3d144 100644
--- a/src/com/android/keychain/KeyChainService.java
+++ b/src/com/android/keychain/KeyChainService.java
@@ -64,39 +64,15 @@ public class KeyChainService extends Service {
         private final TrustedCertificateStore mTrustedCertificateStore
                 = new TrustedCertificateStore();
 
-        private boolean isKeyStoreUnlocked() {
-            return (mKeyStore.test() == KeyStore.NO_ERROR);
-        }
-
-        @Override public byte[] getPrivate(String alias, String authToken) {
-            if (alias == null) {
-                throw new NullPointerException("alias == null");
-            }
-            if (authToken == null) {
-                throw new NullPointerException("authToken == null");
-            }
-            if (!isKeyStoreUnlocked()) {
-                throw new IllegalStateException("keystore locked");
-            }
-            if (!mAccountManager.peekAuthToken(mAccount, alias).equals(authToken)) {
-                throw new IllegalStateException("authtoken mismatch");
-            }
-            String key = Credentials.USER_PRIVATE_KEY + alias;
-            byte[] bytes = mKeyStore.get(key.getBytes(Charsets.UTF_8));
-            if (bytes == null) {
-                throw new IllegalStateException("keystore value missing");
-            }
-            return bytes;
+        @Override public byte[] getPrivateKey(String alias, String authToken) {
+            return getKeyStoreEntry(Credentials.USER_PRIVATE_KEY, alias, authToken);
         }
 
         @Override public byte[] getCertificate(String alias, String authToken) {
-            return getCert(Credentials.USER_CERTIFICATE, alias, authToken);
-        }
-        @Override public byte[] getCaCertificate(String alias, String authToken) {
-            return getCert(Credentials.CA_CERTIFICATE, alias, authToken);
+            return getKeyStoreEntry(Credentials.USER_CERTIFICATE, alias, authToken);
         }
 
-        private byte[] getCert(String type, String alias, String authToken) {
+        private byte[] getKeyStoreEntry(String type, String alias, String authToken) {
             if (alias == null) {
                 throw new NullPointerException("alias == null");
             }
@@ -106,10 +82,7 @@ public class KeyChainService extends Service {
             if (!isKeyStoreUnlocked()) {
                 throw new IllegalStateException("keystore locked");
             }
-            String authAlias = (type.equals(Credentials.CA_CERTIFICATE))
-                    ? (alias + KeyChain.CA_SUFFIX)
-                    : alias;
-            if (!mAccountManager.peekAuthToken(mAccount, authAlias).equals(authToken)) {
+            if (!mAccountManager.peekAuthToken(mAccount, alias).equals(authToken)) {
                 throw new IllegalStateException("authtoken mismatch");
             }
             String key = type + alias;
@@ -120,57 +93,8 @@ public class KeyChainService extends Service {
             return bytes;
         }
 
-        @Override public String findIssuer(Bundle bundle) {
-            if (bundle == null) {
-                throw new NullPointerException("bundle == null");
-            }
-            X509Certificate cert = KeyChain.toCertificate(bundle);
-            if (cert == null) {
-                throw new IllegalArgumentException("no cert in bundle");
-            }
-            X500Principal issuer = cert.getIssuerX500Principal();
-            if (issuer == null) {
-                throw new IllegalStateException();
-            }
-            byte[] aliasPrefix = Credentials.CA_CERTIFICATE.getBytes(Charsets.UTF_8);
-            byte[][] aliasSuffixes = mKeyStore.saw(aliasPrefix);
-            if (aliasSuffixes == null) {
-                return null;
-            }
-
-            // TODO if the keystore would notify us of changes, we
-            // could cache the certs and perform a lookup by issuer
-            for (byte[] aliasSuffix : aliasSuffixes) {
-                byte[] alias = concatenate(aliasPrefix, aliasSuffix);
-                byte[] bytes = mKeyStore.get(alias);
-                try {
-                    // TODO we could at least cache the byte to cert parsing
-                    X509Certificate caCert = parseCertificate(bytes);
-                    if (issuer.equals(caCert.getSubjectX500Principal())) {
-                        // will throw exception on failure to verify.
-                        // this can happen if there are two CAs with
-                        // the same name but with different public
-                        // keys, which does in fact happen, so we will
-                        // try to continue and not just fail fast.
-                        cert.verify(caCert.getPublicKey());
-                        return new String(aliasSuffix, Charsets.UTF_8);
-                    }
-                } catch (Exception ignored) {
-                }
-            }
-            return null;
-        }
-
-        private X509Certificate parseCertificate(byte[] bytes) throws CertificateException {
-            CertificateFactory cf = CertificateFactory.getInstance("X.509");
-            return (X509Certificate) cf.generateCertificate(new ByteArrayInputStream(bytes));
-        }
-
-        private byte[] concatenate(byte[] a, byte[] b) {
-            byte[] result = new byte[a.length + b.length];
-            System.arraycopy(a, 0, result, 0, a.length);
-            System.arraycopy(b, 0, result, a.length, b.length);
-            return result;
+        private boolean isKeyStoreUnlocked() {
+            return (mKeyStore.test() == KeyStore.NO_ERROR);
         }
 
         @Override public void installCaCertificate(byte[] caCertificate) {
@@ -190,6 +114,12 @@ public class KeyChainService extends Service {
                 throw new IllegalStateException(e);
             }
         }
+
+        private X509Certificate parseCertificate(byte[] bytes) throws CertificateException {
+            CertificateFactory cf = CertificateFactory.getInstance("X.509");
+            return (X509Certificate) cf.generateCertificate(new ByteArrayInputStream(bytes));
+        }
+
         @Override public boolean reset() {
             // only Settings should be able to reset
             final String expectedPackage = "android.uid.system:1000";
diff --git a/tests/src/com/android/keychain/tests/KeyChainServiceTest.java b/tests/src/com/android/keychain/tests/KeyChainServiceTest.java
index 75883b2..8f34a0c 100644
--- a/tests/src/com/android/keychain/tests/KeyChainServiceTest.java
+++ b/tests/src/com/android/keychain/tests/KeyChainServiceTest.java
@@ -191,15 +191,7 @@ public class KeyChainServiceTest extends Service {
             assertNotNull(accountManager);
             for (Account account : accountManager.getAccountsByType(KeyChain.ACCOUNT_TYPE)) {
                 mSupport.revokeAppPermission(account, alias1, getApplicationInfo().uid);
-                mSupport.revokeAppPermission(
-                        account, alias1Intermediate + KeyChain.CA_SUFFIX, getApplicationInfo().uid);
-                mSupport.revokeAppPermission(
-                        account, alias1Root + KeyChain.CA_SUFFIX, getApplicationInfo().uid);
                 mSupport.revokeAppPermission(account, alias2, getApplicationInfo().uid);
-                mSupport.revokeAppPermission(
-                        account, alias2Intermediate + KeyChain.CA_SUFFIX, getApplicationInfo().uid);
-                mSupport.revokeAppPermission(
-                        account, alias2Root + KeyChain.CA_SUFFIX, getApplicationInfo().uid);
             }
 
             Log.d(TAG, "test_KeyChainService bind service");
@@ -244,7 +236,7 @@ public class KeyChainServiceTest extends Service {
             assertNotNull(authToken);
             assertFalse(authToken.isEmpty());
 
-            byte[] privateKey = mService.getPrivate(alias1, authToken);
+            byte[] privateKey = mService.getPrivateKey(alias1, authToken);
             assertNotNull(privateKey);
             assertEquals(Arrays.toString(pke1.getPrivateKey().getEncoded()),
                          Arrays.toString(privateKey));
@@ -254,29 +246,6 @@ public class KeyChainServiceTest extends Service {
             assertEquals(Arrays.toString(pke1.getCertificate().getEncoded()),
                          Arrays.toString(certificate));
 
-            String aliasI = mService.findIssuer(KeyChain.fromCertificate(pke1.getCertificate()));
-            assertNotNull(aliasI);
-            assertEquals(alias1Intermediate, aliasI);
-
-            String aliasR = mService.findIssuer(KeyChain.fromCertificate(intermediate1));
-            assertNotNull(aliasR);
-            assertEquals(alias1Root, aliasR);
-
-            String aliasRR = mService.findIssuer(KeyChain.fromCertificate(intermediate1));
-            assertNotNull(aliasRR);
-            assertEquals(alias1Root, aliasRR);
-
-            try {
-                mService.findIssuer(new Bundle());
-                fail();
-            } catch (IllegalArgumentException expected) {
-            }
-            try {
-                mService.findIssuer(null);
-                fail();
-            } catch (NullPointerException expected) {
-            }
-
             Log.d(TAG, "test_KeyChainService unbind");
             unbindServices();
             assertFalse(mIsBoundSupport);
diff --git a/tests/src/com/android/keychain/tests/KeyChainTestActivity.java b/tests/src/com/android/keychain/tests/KeyChainTestActivity.java
index b7610e3..eed08d4 100644
--- a/tests/src/com/android/keychain/tests/KeyChainTestActivity.java
+++ b/tests/src/com/android/keychain/tests/KeyChainTestActivity.java
@@ -20,7 +20,9 @@ import android.app.Activity;
 import android.content.Intent;
 import android.os.AsyncTask;
 import android.os.Bundle;
+import android.os.RemoteException;
 import android.security.KeyChain;
+import android.security.KeyChainResult;
 import android.text.method.ScrollingMovementMethod;
 import android.util.Log;
 import android.widget.TextView;
@@ -89,10 +91,45 @@ public class KeyChainTestActivity extends Activity {
         log("Starting test...");
 
         try {
-            KeyChain.getInstance(this);
+            KeyChain.get(null, null);
             throw new AssertionError();
         } catch (InterruptedException e) {
-            throw new RuntimeException(e);
+            throw new AssertionError(e);
+        } catch (RemoteException e) {
+            throw new AssertionError(e);
+        } catch (NullPointerException expected) {
+            log("KeyChain failed as expected with null argument.");
+        }
+
+        try {
+            KeyChain.get(this, null);
+            throw new AssertionError();
+        } catch (InterruptedException e) {
+            throw new AssertionError(e);
+        } catch (RemoteException e) {
+            throw new AssertionError(e);
+        } catch (NullPointerException expected) {
+            log("KeyChain failed as expected with null argument.");
+        }
+
+        try {
+            KeyChain.get(null, "");
+            throw new AssertionError();
+        } catch (InterruptedException e) {
+            throw new AssertionError(e);
+        } catch (RemoteException e) {
+            throw new AssertionError(e);
+        } catch (NullPointerException expected) {
+            log("KeyChain failed as expected with null argument.");
+        }
+
+        try {
+            KeyChain.get(this, "");
+            throw new AssertionError();
+        } catch (InterruptedException e) {
+            throw new AssertionError(e);
+        } catch (RemoteException e) {
+            throw new AssertionError(e);
         } catch (IllegalStateException expected) {
             log("KeyChain failed as expected on main thread.");
         }
@@ -100,7 +137,6 @@ public class KeyChainTestActivity extends Activity {
         new AsyncTask<Void, Void, Void>() {
             @Override protected Void doInBackground(Void... params) {
                 try {
-                    mKeyChain = KeyChain.getInstance(KeyChainTestActivity.this);
                     log("Starting web server...");
                     URL url = startWebServer();
                     log("Making https request to " + url);
@@ -132,9 +168,7 @@ public class KeyChainTestActivity extends Activity {
             }
             private void makeHttpsRequest(URL url) throws Exception {
                 SSLContext clientContext = SSLContext.getInstance("SSL");
-                clientContext.init(new KeyManager[] { new KeyChainKeyManager() },
-                                   new TrustManager[] { new KeyChainTrustManager() },
-                                   null);
+                clientContext.init(new KeyManager[] { new KeyChainKeyManager() }, null, null);
                 HttpsURLConnection connection = (HttpsURLConnection) url.openConnection();
                 connection.setSSLSocketFactory(clientContext.getSocketFactory());
                 if (connection.getResponseCode() != 200) {
@@ -190,16 +224,23 @@ public class KeyChainTestActivity extends Activity {
             throw new UnsupportedOperationException();
         }
         @Override public X509Certificate[] getCertificateChain(String alias) {
-            log("KeyChainKeyManager getCertificateChain...");
-            Bundle cert = mKeyChain.getCertificate(alias);
-            Intent intent = cert.getParcelable(KeyChain.KEY_INTENT);
-            if (intent != null) {
-                waitForGrant(intent);
-                cert = mKeyChain.getCertificate(alias);
+            try {
+                log("KeyChainKeyManager getCertificateChain...");
+                KeyChainResult keyChainResult = KeyChain.get(KeyChainTestActivity.this, alias);
+                Intent intent = keyChainResult.getIntent();
+                if (intent != null) {
+                    waitForGrant(intent);
+                    keyChainResult = KeyChain.get(KeyChainTestActivity.this, alias);
+                }
+                X509Certificate certificate = keyChainResult.getCertificate();
+                log("certificate=" + certificate);
+                return new X509Certificate[] { certificate };
+            } catch (InterruptedException e) {
+                Thread.currentThread().interrupt();
+                return null;
+            } catch (RemoteException e) {
+                throw new RuntimeException(e);
             }
-            X509Certificate certificate = KeyChain.toCertificate(cert);
-            log("certificate=" + certificate);
-            return new X509Certificate[] { certificate };
         }
         @Override public String[] getClientAliases(String keyType, Principal[] issuers) {
             // not a client SSLSocket callback
@@ -210,84 +251,23 @@ public class KeyChainTestActivity extends Activity {
             throw new UnsupportedOperationException();
         }
         @Override public PrivateKey getPrivateKey(String alias) {
-            log("KeyChainKeyManager getPrivateKey...");
-            Bundle pkey = mKeyChain.getPrivate(alias);
-            Intent intent = pkey.getParcelable(KeyChain.KEY_INTENT);
-            if (intent != null) {
-                waitForGrant(intent);
-                pkey = mKeyChain.getPrivate(alias);
-            }
-            PrivateKey privateKey = KeyChain.toPrivateKey(pkey);
-            log("privateKey=" + privateKey);
-            return privateKey;
-        }
-    }
-
-    private class KeyChainTrustManager implements X509TrustManager {
-        private final X509TrustManager trustManager = SSLParametersImpl.getDefaultTrustManager();
-        private final IndexedPKIXParameters index
-                = SSLParametersImpl.getDefaultIndexedPKIXParameters();
-
-        @Override public void checkClientTrusted(X509Certificate[] chain, String authType)
-                throws CertificateException {
-            // not a client SSLSocket callback
-            throw new UnsupportedOperationException();
-        }
-
-        @Override public void checkServerTrusted(X509Certificate[] chain, String authType)
-                throws CertificateException {
-            log("KeyChainTrustManager checkServerTrusted...");
-            // start at the end of the chain and make sure we have a trust anchor.
-            // if not, ask KeyChain for one.
-            X509Certificate end = chain[chain.length-1];
-            if (findTrustAnchor(end)) {
-                trustManager.checkServerTrusted(chain, authType);
-                return;
-            }
-
-            // try to extend the chain
-            List<X509Certificate> list = new ArrayList<X509Certificate>(Arrays.asList(chain));
-            do {
-                Bundle ca = mKeyChain.findIssuer(end);
-                if (ca == null) {
-                    break;
-                }
-                Intent intent = ca.getParcelable(KeyChain.KEY_INTENT);
+            try {
+                log("KeyChainKeyManager getPrivateKey...");
+                KeyChainResult keyChainResult = KeyChain.get(KeyChainTestActivity.this, alias);
+                Intent intent = keyChainResult.getIntent();
                 if (intent != null) {
                     waitForGrant(intent);
-                    ca = mKeyChain.findIssuer(end);
-                }
-                end = KeyChain.toCertificate(ca);
-                list.add(end);
-            } while (!findTrustAnchor(end));
-
-            // convert extended chain back to array
-            if (list.size() != chain.length) {
-                chain = list.toArray(new X509Certificate[list.size()]);
-            }
-            trustManager.checkServerTrusted(chain, authType);
-        }
-
-        /**
-         * Returns true if we have found a trust anchor, with or
-         * without error, indicating that we should call the
-         * underlying TrustManager to verify the chain in its current
-         * state. Otherwise, returns false to indicate the chain
-         * should be extended.
-         */
-        private boolean findTrustAnchor(X509Certificate cert) {
-            try {
-                if (index.findTrustAnchor(cert) == null) {
-                    return false;
+                    keyChainResult = KeyChain.get(KeyChainTestActivity.this, alias);
                 }
-            } catch (CertPathValidatorException ignored) {
+                PrivateKey privateKey = keyChainResult.getPrivateKey();
+                log("privateKey=" + privateKey);
+                return privateKey;
+            } catch (InterruptedException e) {
+                Thread.currentThread().interrupt();
+                return null;
+            } catch (RemoteException e) {
+                throw new RuntimeException(e);
             }
-            return true;
-        }
-
-        @Override public X509Certificate[] getAcceptedIssuers() {
-            // not a client SSLSocket callback
-            throw new UnsupportedOperationException();
         }
     }