Refactoring AndroidKeyStore to support a KeyStore running in another process

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

+// Copyright 2014 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+package org.chromium.net;
+
+import android.os.RemoteException;
+import android.util.Log;
+
+/**
+ * Provides a remoted implementation of AndroidKeyStore where all calls are forwarded via
+ * binder to an external process.
+ */
+public class AndroidKeyStoreRemoteImpl implements AndroidKeyStore {
+
+    private static class RemotePrivateKey implements AndroidPrivateKey {
+        // Reference to the key on a remote store.
+        final String mHandle;
+        // Key store handling this key.
+        final AndroidKeyStoreRemoteImpl mStore;
+
+        RemotePrivateKey(String handle, AndroidKeyStoreRemoteImpl store) {
+            mHandle = handle;
+            mStore = store;
+        }
+
+        public String getHandle() {
+            return mHandle;
+        }
+
+        @Override
+        public AndroidKeyStore getKeyStore() {
+            return mStore;
+        }
+    }
+
+    private static final String TAG = "AndroidKeyStoreRemoteImpl";

[Ryan Sleevi, 2014/02/14 20:57:51]

You ordering between the two classes is inconsistent - ex: the TAG appears first
in the other class.

[Yaron, 2014/02/14 22:06:59]

Done.

+
+    private final IAndroidKeyStoreRemote mRemoteManager;
+
+    public AndroidKeyStoreRemoteImpl(IAndroidKeyStoreRemote manager) {
+        mRemoteManager = manager;
+    }
+
+    @Override
+    public byte[] getRSAKeyModulus(AndroidPrivateKey key) {
+        RemotePrivateKey remoteKey = (RemotePrivateKey) key;
+        try {
+            Log.d(TAG, "getRSAKeyModulus");
+            return mRemoteManager.getRSAKeyModulus(remoteKey.getHandle());
+        } catch (RemoteException e) {
+            e.printStackTrace();
+            return null;
+        }
+     }
+
+    @Override
+    public byte[] getDSAKeyParamQ(AndroidPrivateKey key) {
+        RemotePrivateKey remoteKey = (RemotePrivateKey) key;
+        try {
+            Log.d(TAG, "getDSAKeyParamQ");
+            return mRemoteManager.getDSAKeyParamQ(remoteKey.getHandle());
+        } catch (RemoteException e) {
+            e.printStackTrace();
+            return null;
+        }
+    }
+
+    @Override
+    public byte[] getECKeyOrder(AndroidPrivateKey key) {
+        RemotePrivateKey remoteKey = (RemotePrivateKey) key;
+        try {
+            Log.d(TAG, "getECKeyOrder");
+            return mRemoteManager.getECKeyOrder(remoteKey.getHandle());
+        } catch (RemoteException e) {
+            e.printStackTrace();
+            return null;
+        }
+    }
+
+    @Override
+    public byte[] rawSignDigestWithPrivateKey(AndroidPrivateKey key, byte[] message) {
+        RemotePrivateKey remoteKey = (RemotePrivateKey) key;
+        try {
+            Log.d(TAG, "rawSignDigestWithPrivateKey");
+            return mRemoteManager.rawSignDigestWithPrivateKey(remoteKey.getHandle(), message);
+        } catch (RemoteException e) {
+            e.printStackTrace();
+            return null;
+        }
+    }
+
+    @Override
+    public int getPrivateKeyType(AndroidPrivateKey key) {
+        RemotePrivateKey remoteKey = (RemotePrivateKey) key;
+        try {
+            Log.d(TAG, "getPrivateKeyType");
+            return mRemoteManager.getPrivateKeyType(remoteKey.getHandle());
+        } catch (RemoteException e) {
+            e.printStackTrace();
+            return 0;
+        }
+    }
+
+    @Override
+    public byte[] getPrivateKeyEncodedBytes(AndroidPrivateKey key) {
+        // This should not be called as it's only for older versions of Android.
+        assert false;
+        return null;
+    }
+
+    @Override
+    public int getOpenSSLHandleForPrivateKey(AndroidPrivateKey privateKey) {

[Ryan Sleevi, 2014/02/14 20:57:51]

This seems like a real codesmell that you have to expose this but return
NULL/assert false. It seems to violate the IS-A principle. Should this method
really be exposed on the base if derived classes won't implement it?

[Yaron, 2014/02/14 22:06:59]

True but it's sort of dead code anyway. See the interface comment on not being
called on 4.2 (i.e. it shouldn't be called on Default* either).

+        // This should not be called as it's only for older versions of Android.
+        assert false;

[Ryan Sleevi, 2014/02/14 20:57:51]

Your ordering within this class does not match the interface order.

[Yaron, 2014/02/14 22:06:59]

Done.

+        return 0;
+    }
+
+    public AndroidPrivateKey createKey(String alias) {
+        try {
+            String handle = mRemoteManager.getPrivateKeyHandle(alias);
+            return new RemotePrivateKey(handle, this);
+        } catch (RemoteException e) {
+            e.printStackTrace();
+            return null;
+        }
+    }
+
+    @Override
+    public void releaseKey(AndroidPrivateKey key) {
+        RemotePrivateKey remoteKey = (RemotePrivateKey) key;
+        try {
+            Log.d(TAG, "releaseKey");
+            mRemoteManager.releaseKey(remoteKey.getHandle());
+        } catch (RemoteException e) {
+            e.printStackTrace();
+        }
+    }
+}