Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(46)

Issue 166143002: Refactoring AndroidKeyStore to support a KeyStore running in another process (Closed)

Created:
6 years, 10 months ago by Yaron
Modified:
6 years, 1 month ago
CC:
chromium-reviews, cbentzel+watch_chromium.org, ppi
Visibility:
Public.

Description

Refactoring AndroidKeyStore to support a KeyStore running in another process This CL does a number of things: 1) Extracts an AndroidKeyStore interface which specifies the API needed by the native OpenSSL engine from an AndroidKeyStore. Also changes from using PrivateKey to AndroidPrivateKey to provide a layer of indirection needed for a remote PrivateKey 2) Renames the previous AndroidKeyStore to AndroidKeyStoreLocalImpl as it's used for interacting with an in-process Android KeyStore 3) Provides a new class AndroidKeyStoreRemoteImpl and corresponding IAndroidKeyStoreRemote.aidl that together specify the interface and interaction with a remote process managing an Android KeyStore 4) Alters the PKCS11-based authentication flow to only use out a remote Android KeyStore 5) Adds a new method to the previous AndroidKeyStore interface to facilitate clean up of remote keys BUG=341500 CONTRIBUTOR=ppi@chromium.org R=bulach@chromium.org, klobag@chromium.org TBR=rsleevi NOTRY=true Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=251461

Patch Set 1 #

Patch Set 2 : #

Total comments: 24

Patch Set 3 : comments #

Patch Set 4 : #

Total comments: 10

Patch Set 5 : rename classes, add callback #

Patch Set 6 : rename InProcessAndroidKey #

Patch Set 7 : reupload cause of reitveld flakiness #

Total comments: 3

Patch Set 8 : add onInitializationCOmplete #

Patch Set 9 : shorter name #

Patch Set 10 : rebase #

Total comments: 2
Unified diffs Side-by-side diffs Delta from patch set Stats (+610 lines, -221 lines) Patch
M chrome/android/java/src/org/chromium/chrome/browser/PKCS11AuthenticationManager.java View 1 2 chunks +7 lines, -6 lines 0 comments Download
M chrome/android/java/src/org/chromium/chrome/browser/SSLClientCertificateRequest.java View 1 2 3 4 9 chunks +16 lines, -11 lines 0 comments Download
M chrome/android/testshell/java/src/org/chromium/chrome/testshell/TestShellPKCS11AuthenticationManager.java View 1 2 chunks +2 lines, -2 lines 0 comments Download
A net/android/android_private_key.h View 1 2 3 1 chunk +28 lines, -0 lines 0 comments Download
A net/android/android_private_key.cc View 1 2 3 1 chunk +25 lines, -0 lines 0 comments Download
M net/android/java/src/org/chromium/net/AndroidKeyStore.java View 1 2 3 4 8 chunks +21 lines, -182 lines 0 comments Download
A net/android/java/src/org/chromium/net/AndroidPrivateKey.java View 1 2 1 chunk +18 lines, -0 lines 0 comments Download
A net/android/java/src/org/chromium/net/DefaultAndroidKeyStore.java View 1 2 3 4 5 1 chunk +232 lines, -0 lines 0 comments Download
A net/android/java/src/org/chromium/net/IRemoteAndroidKeyStore.aidl View 1 2 3 4 1 chunk +33 lines, -0 lines 0 comments Download
A net/android/java/src/org/chromium/net/IRemoteAndroidKeyStoreCallbacks.aidl View 1 2 3 4 5 6 7 8 1 chunk +23 lines, -0 lines 0 comments Download
A + net/android/java/src/org/chromium/net/IRemoteAndroidKeyStoreInterface.aidl View 1 2 3 4 1 chunk +2 lines, -1 line 2 comments Download
A net/android/java/src/org/chromium/net/RemoteAndroidKeyStore.java View 1 2 3 4 1 chunk +139 lines, -0 lines 0 comments Download
M net/android/keystore.h View 1 1 chunk +2 lines, -0 lines 0 comments Download
M net/android/keystore.cc View 1 2 8 chunks +39 lines, -12 lines 0 comments Download
M net/android/keystore_openssl.cc View 1 3 chunks +3 lines, -7 lines 0 comments Download
M net/android/net_jni_registrar.cc View 1 2 2 chunks +2 lines, -0 lines 0 comments Download
M net/net.gyp View 1 2 3 4 5 6 7 8 9 3 chunks +18 lines, -0 lines 0 comments Download

Messages

Total messages: 18 (1 generated)
Yaron
rsleevi: net/net.gyp (also free to look at other stuff but it's pretty android-specific) bulach (if ...
6 years, 10 months ago (2014-02-14 07:47:35 UTC) #1
bulach
lgtm but ryan obviously need to review the ssl bits :) I have some suggestions ...
6 years, 10 months ago (2014-02-14 10:45:28 UTC) #2
bulach
sorry, just saw the other side and have one question here: https://codereview.chromium.org/166143002/diff/20001/net/android/java/src/org/chromium/net/interface.aidl File net/android/java/src/org/chromium/net/interface.aidl (right): ...
6 years, 10 months ago (2014-02-14 12:48:59 UTC) #3
Yaron
https://codereview.chromium.org/166143002/diff/20001/net/android/java/src/org/chromium/net/AndroidKeyStore.java File net/android/java/src/org/chromium/net/AndroidKeyStore.java (right): https://codereview.chromium.org/166143002/diff/20001/net/android/java/src/org/chromium/net/AndroidKeyStore.java#newcode8 net/android/java/src/org/chromium/net/AndroidKeyStore.java:8: * Abstract key store. It can be implemented either ...
6 years, 10 months ago (2014-02-14 17:29:28 UTC) #4
Yaron
done. I'm not convinced it was worth getting rid of the bridge but I won't ...
6 years, 10 months ago (2014-02-14 19:36:56 UTC) #5
Ryan Sleevi
https://codereview.chromium.org/166143002/diff/270001/net/android/java/src/org/chromium/net/AndroidKeyStore.java File net/android/java/src/org/chromium/net/AndroidKeyStore.java (right): https://codereview.chromium.org/166143002/diff/270001/net/android/java/src/org/chromium/net/AndroidKeyStore.java#newcode48 net/android/java/src/org/chromium/net/AndroidKeyStore.java:48: int getOpenSSLHandleForPrivateKey(AndroidPrivateKey key); Why did you re-order these methods? ...
6 years, 10 months ago (2014-02-14 20:57:51 UTC) #6
Yaron
Updated based on your feedback. https://codereview.chromium.org/166143002/diff/270001/net/android/java/src/org/chromium/net/AndroidKeyStore.java File net/android/java/src/org/chromium/net/AndroidKeyStore.java (right): https://codereview.chromium.org/166143002/diff/270001/net/android/java/src/org/chromium/net/AndroidKeyStore.java#newcode48 net/android/java/src/org/chromium/net/AndroidKeyStore.java:48: int getOpenSSLHandleForPrivateKey(AndroidPrivateKey key); On ...
6 years, 10 months ago (2014-02-14 22:06:58 UTC) #7
klobag.chromium
Minor suggestions. https://chromiumcodereview.appspot.com/166143002/diff/160003/net/android/java/src/org/chromium/net/AndroidKeyStore.java File net/android/java/src/org/chromium/net/AndroidKeyStore.java (right): https://chromiumcodereview.appspot.com/166143002/diff/160003/net/android/java/src/org/chromium/net/AndroidKeyStore.java#newcode14 net/android/java/src/org/chromium/net/AndroidKeyStore.java:14: public interface AndroidKeyStore { If we keep ...
6 years, 10 months ago (2014-02-14 23:03:52 UTC) #8
Yaron
Working on other comment https://chromiumcodereview.appspot.com/166143002/diff/160003/net/android/java/src/org/chromium/net/AndroidKeyStore.java File net/android/java/src/org/chromium/net/AndroidKeyStore.java (right): https://chromiumcodereview.appspot.com/166143002/diff/160003/net/android/java/src/org/chromium/net/AndroidKeyStore.java#newcode14 net/android/java/src/org/chromium/net/AndroidKeyStore.java:14: public interface AndroidKeyStore { On ...
6 years, 10 months ago (2014-02-14 23:05:49 UTC) #9
klobag.chromium
https://chromiumcodereview.appspot.com/166143002/diff/20001/net/android/java/src/org/chromium/net/AndroidKeyStore.java File net/android/java/src/org/chromium/net/AndroidKeyStore.java (right): https://chromiumcodereview.appspot.com/166143002/diff/20001/net/android/java/src/org/chromium/net/AndroidKeyStore.java#newcode8 net/android/java/src/org/chromium/net/AndroidKeyStore.java:8: * Abstract key store. It can be implemented either ...
6 years, 10 months ago (2014-02-14 23:15:22 UTC) #10
klobag.chromium
lgtm Thanks.
6 years, 10 months ago (2014-02-14 23:30:15 UTC) #11
Yaron
On 2014/02/14 23:30:15, klobag.chromium wrote: > lgtm > > Thanks. Ryan gave approval for net/net.gyp ...
6 years, 10 months ago (2014-02-15 00:20:31 UTC) #12
Yaron
The CQ bit was checked by yfriedman@chromium.org
6 years, 10 months ago (2014-02-15 00:20:38 UTC) #13
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/yfriedman@chromium.org/166143002/710001
6 years, 10 months ago (2014-02-15 00:38:05 UTC) #14
Yaron
Committed patchset #10 manually as r251461 (presubmit successful).
6 years, 10 months ago (2014-02-15 00:54:31 UTC) #15
enrico.ros
https://codereview.chromium.org/166143002/diff/780001/net/android/java/src/org/chromium/net/IRemoteAndroidKeyStoreInterface.aidl File net/android/java/src/org/chromium/net/IRemoteAndroidKeyStoreInterface.aidl (right): https://codereview.chromium.org/166143002/diff/780001/net/android/java/src/org/chromium/net/IRemoteAndroidKeyStoreInterface.aidl#newcode5 net/android/java/src/org/chromium/net/IRemoteAndroidKeyStoreInterface.aidl:5: interface org.chomium.net.IRemoteAndroidKeyStore; The domain 'chomium.net' is incorrect (missing 'r'). ...
6 years, 1 month ago (2014-11-14 06:03:52 UTC) #17
ppi
6 years, 1 month ago (2014-11-14 10:03:34 UTC) #18
Message was sent while issue was closed.
https://codereview.chromium.org/166143002/diff/780001/net/android/java/src/or...
File net/android/java/src/org/chromium/net/IRemoteAndroidKeyStoreInterface.aidl
(right):

https://codereview.chromium.org/166143002/diff/780001/net/android/java/src/or...
net/android/java/src/org/chromium/net/IRemoteAndroidKeyStoreInterface.aidl:5:
interface org.chomium.net.IRemoteAndroidKeyStore;
On 2014/11/14 06:03:52, enrico.ros wrote:
> The domain 'chomium.net' is incorrect (missing 'r').
> What is the purpose of creating an invalid .aidl file?
> 
> It is not valid per the aidl syntax. The ninja rule that compiles aidl into
java
> should use the inclusion paths and not the pre-compiled checked in file.

Thanks for catching this - would you like to put together a patch to address the
issue?

Powered by Google App Engine
This is Rietveld 408576698