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
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 @@
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 @@
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 @@
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 @@
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 @@
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 @@
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 @@
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.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 @@
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 @@
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 @@
}
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 @@
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 @@
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);
+ keyChainResult = KeyChain.get(KeyChainTestActivity.this, alias);
}
- 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()]);
+ PrivateKey privateKey = keyChainResult.getPrivateKey();
+ log("privateKey=" + privateKey);
+ return privateKey;
+ } catch (InterruptedException e) {
+ Thread.currentThread().interrupt();
+ return null;
+ } catch (RemoteException e) {
+ throw new RuntimeException(e);
}
- 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;
- }
- } catch (CertPathValidatorException ignored) {
- }
- return true;
- }
-
- @Override public X509Certificate[] getAcceptedIssuers() {
- // not a client SSLSocket callback
- throw new UnsupportedOperationException();
}
}