Refactoring AndroidKeyStore to support a KeyStore running in another process

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

[Yaron, 2014-02-14 07:47:35]

rsleevi: net/net.gyp (also free to look at other stuff but it's pretty
android-specific)
bulach (if you can spare time)/grace: * 

ps1 is from: https://codereview.chromium.org/164053004/ and this builds on top
of it. There's also internal patch
https://chrome-internal-review.googlesource.com/#/c/154799/1 which is far enough
to start review but not fully complete.

[bulach, 2014-02-14 10:45:28]

lgtm but ryan obviously need to review the ssl bits :)

I have some suggestions below, mostly to simplify the JNI layers:

https://codereview.chromium.org/166143002/diff/20001/chrome/android/java/src/...
File
chrome/android/java/src/org/chromium/chrome/browser/SSLClientCertificateRequest.java
(right):

https://codereview.chromium.org/166143002/diff/20001/chrome/android/java/src/...
chrome/android/java/src/org/chromium/chrome/browser/SSLClientCertificateRequest.java:41:
static final AndroidKeyStoreLocalImpl sLocalKeyStore = new
AndroidKeyStoreLocalImpl();
nit: can this be private, and wrapped in a GetInstance() or some such?

https://codereview.chromium.org/166143002/diff/20001/chrome/android/java/src/...
chrome/android/java/src/org/chromium/chrome/browser/SSLClientCertificateRequest.java:53:
private AndroidPrivateKey mAndroidKey;
nit: mAndroidPrivateKey, just to avoid confusion..

https://codereview.chromium.org/166143002/diff/20001/chrome/android/java/src/...
chrome/android/java/src/org/chromium/chrome/browser/SSLClientCertificateRequest.java:75:
Log.d(TAG, "got chain " + chain);
nit: unindent

https://codereview.chromium.org/166143002/diff/20001/net/android/java/src/org...
File net/android/java/src/org/chromium/net/AndroidKeyStore.java (right):

https://codereview.chromium.org/166143002/diff/20001/net/android/java/src/org...
net/android/java/src/org/chromium/net/AndroidKeyStore.java:8: * Abstract key
store. It can be implemented either as a local store operating on Java
PrivateKeys
nit: Key store interface :) but tbh, I'd remove this comment altogether... as
far as this interface goes, it can be implemented with return 'hardcodedkey' :)


also... I think @CalledByNative should be supported in interfaces? [1]
if it does work, we can then remove the following "AndroidKeyStoreBridge"?

probably need @CalledByNative for AndroidPrivateKey.getKeyStore() and the
corresponding .h/.cc, but I think it'd be overall simpler than keeping the
chunkier AndroidKeyStoreBridge... wdyt?

[1]
https://code.google.com/p/chromium/codesearch#chromium/src/android_webview/ja...

https://codereview.chromium.org/166143002/diff/20001/net/android/java/src/org...
net/android/java/src/org/chromium/net/AndroidKeyStore.java:15: * object,
obtained through reflection.
nit: again, the interface shouldn't specify how it's obtained.

https://codereview.chromium.org/166143002/diff/20001/net/android/java/src/org...
File net/android/java/src/org/chromium/net/AndroidKeyStoreBridge.java (right):

https://codereview.chromium.org/166143002/diff/20001/net/android/java/src/org...
net/android/java/src/org/chromium/net/AndroidKeyStoreBridge.java:11: * Static
wrapper for JNI functions that are needed for using a Java private key in the
C++ OpenSSL
see above, I think we can have the @CalledByNative in the interface, then have
the key exposing its "@CalledByNative getKeyStore()", and then in the following
.cc file we just do something like:
  Java_AndroidKeyStore_getRSAKeyModules(....,
Java_AndroidPrivateKey_getKeyStore());

https://codereview.chromium.org/166143002/diff/20001/net/android/java/src/org...
File net/android/java/src/org/chromium/net/AndroidKeyStoreLocalImpl.java
(right):

https://codereview.chromium.org/166143002/diff/20001/net/android/java/src/org...
net/android/java/src/org/chromium/net/AndroidKeyStoreLocalImpl.java:25: public
class AndroidKeyStoreLocalImpl implements AndroidKeyStore {
nit: perhaps "InProcess" instead of "Local"?

https://codereview.chromium.org/166143002/diff/20001/net/android/java/src/org...
net/android/java/src/org/chromium/net/AndroidKeyStoreLocalImpl.java:59: } else {
remove the "else" here and in the following methods too.

https://codereview.chromium.org/166143002/diff/20001/net/android/java/src/org...
File net/android/java/src/org/chromium/net/AndroidPrivateKey.java (right):

https://codereview.chromium.org/166143002/diff/20001/net/android/java/src/org...
net/android/java/src/org/chromium/net/AndroidPrivateKey.java:11: /** @return
AndroidKeyStore that handles this key. */
as above, I think this can be @CalledByNative

https://codereview.chromium.org/166143002/diff/20001/net/android/keystore.cc
File net/android/keystore.cc (right):

https://codereview.chromium.org/166143002/diff/20001/net/android/keystore.cc#...
net/android/keystore.cc:13: #include "jni/AndroidKeyStoreBridge_jni.h"
if my suggestion above works, then this will become again AndroidKeyStore_jni.h,
and the calls below would be something like:
Java_AndroidKeyStore_getRSAKeyModulus(env, 
  Java_AndroidKey_getKeyStore(env, private_key_ref)));

or rather:
Java_AndroidKeyStore_getRSAKeyModulus(env, 
  AndroidKey::GetKeyStore(env, private_key_ref)));

where AndroidKey is a tiny .h/.cc exposing the GetKeyStore from java, and the
AndroidKeyStoreBridge will be gone..

wdyt?

[bulach, 2014-02-14 12:48:59]

sorry, just saw the other side and have one question here:

https://codereview.chromium.org/166143002/diff/20001/net/android/java/src/org...
File net/android/java/src/org/chromium/net/interface.aidl (right):

https://codereview.chromium.org/166143002/diff/20001/net/android/java/src/org...
net/android/java/src/org/chromium/net/interface.aidl:5: interface
org.chomium.net.IAndroidKeyStoreRemote;
does this file need to be called "interface.aidl"?
can it be more descriptive?

[Yaron, 2014-02-14 17:29:28]

https://codereview.chromium.org/166143002/diff/20001/net/android/java/src/org...
File net/android/java/src/org/chromium/net/AndroidKeyStore.java (right):

https://codereview.chromium.org/166143002/diff/20001/net/android/java/src/org...
net/android/java/src/org/chromium/net/AndroidKeyStore.java:8: * Abstract key
store. It can be implemented either as a local store operating on Java
PrivateKeys
On 2014/02/14 10:45:29, bulach wrote:
> nit: Key store interface :) but tbh, I'd remove this comment altogether... as
> far as this interface goes, it can be implemented with return 'hardcodedkey'
:)
> 
> 
> also... I think @CalledByNative should be supported in interfaces? [1]
> if it does work, we can then remove the following "AndroidKeyStoreBridge"?
> 
> probably need @CalledByNative for AndroidPrivateKey.getKeyStore() and the
> corresponding .h/.cc, but I think it'd be overall simpler than keeping the
> chunkier AndroidKeyStoreBridge... wdyt?
> 
> [1]
>
https://code.google.com/p/chromium/codesearch#chromium/src/android_webview/ja...

My main reason to not added @CalledByNative is to keep this pure java. This
simplifies the helper process. Even if the helper process doesn't use the native
bits, it'd have to include the annotation. Not a big deal, and while I think
your suggestion is slightly cleaner, I think the bridge class is workable.

[Yaron, 2014-02-14 19:36:56]

done. I'm not convinced it was worth getting rid of the bridge but I won't fight
you on it. :)

Ryan's said previously that he can cover interactions with ssl but this is
mostly in android land and doesn't change the interaction with the rest of the
stack from previous impls so I think I'll tbr the gyp change

https://codereview.chromium.org/166143002/diff/20001/chrome/android/java/src/...
File
chrome/android/java/src/org/chromium/chrome/browser/SSLClientCertificateRequest.java
(right):

https://codereview.chromium.org/166143002/diff/20001/chrome/android/java/src/...
chrome/android/java/src/org/chromium/chrome/browser/SSLClientCertificateRequest.java:41:
static final AndroidKeyStoreLocalImpl sLocalKeyStore = new
AndroidKeyStoreLocalImpl();
On 2014/02/14 10:45:29, bulach wrote:
> nit: can this be private, and wrapped in a GetInstance() or some such?

Done.

https://codereview.chromium.org/166143002/diff/20001/chrome/android/java/src/...
chrome/android/java/src/org/chromium/chrome/browser/SSLClientCertificateRequest.java:53:
private AndroidPrivateKey mAndroidKey;
On 2014/02/14 10:45:29, bulach wrote:
> nit: mAndroidPrivateKey, just to avoid confusion..

Done.

https://codereview.chromium.org/166143002/diff/20001/chrome/android/java/src/...
chrome/android/java/src/org/chromium/chrome/browser/SSLClientCertificateRequest.java:75:
Log.d(TAG, "got chain " + chain);
On 2014/02/14 10:45:29, bulach wrote:
> nit: unindent

Done.

https://codereview.chromium.org/166143002/diff/20001/net/android/java/src/org...
File net/android/java/src/org/chromium/net/AndroidKeyStore.java (right):

https://codereview.chromium.org/166143002/diff/20001/net/android/java/src/org...
net/android/java/src/org/chromium/net/AndroidKeyStore.java:8: * Abstract key
store. It can be implemented either as a local store operating on Java
PrivateKeys
On 2014/02/14 17:29:28, Yaron wrote:
> On 2014/02/14 10:45:29, bulach wrote:
> > nit: Key store interface :) but tbh, I'd remove this comment altogether...
as
> > far as this interface goes, it can be implemented with return 'hardcodedkey'
> :)
> > 
> > 
> > also... I think @CalledByNative should be supported in interfaces? [1]
> > if it does work, we can then remove the following "AndroidKeyStoreBridge"?
> > 
> > probably need @CalledByNative for AndroidPrivateKey.getKeyStore() and the
> > corresponding .h/.cc, but I think it'd be overall simpler than keeping the
> > chunkier AndroidKeyStoreBridge... wdyt?
> > 
> > [1]
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/android_webview/ja...
> 
> My main reason to not added @CalledByNative is to keep this pure java. This
> simplifies the helper process. Even if the helper process doesn't use the
native
> bits, it'd have to include the annotation. Not a big deal, and while I think
> your suggestion is slightly cleaner, I think the bridge class is workable.

Done.

https://codereview.chromium.org/166143002/diff/20001/net/android/java/src/org...
net/android/java/src/org/chromium/net/AndroidKeyStore.java:15: * object,
obtained through reflection.
On 2014/02/14 10:45:29, bulach wrote:
> nit: again, the interface shouldn't specify how it's obtained.

I had just moved up the comments from the concrete class but fair point.

https://codereview.chromium.org/166143002/diff/20001/net/android/java/src/org...
File net/android/java/src/org/chromium/net/AndroidKeyStoreBridge.java (right):

https://codereview.chromium.org/166143002/diff/20001/net/android/java/src/org...
net/android/java/src/org/chromium/net/AndroidKeyStoreBridge.java:11: * Static
wrapper for JNI functions that are needed for using a Java private key in the
C++ OpenSSL
On 2014/02/14 10:45:29, bulach wrote:
> see above, I think we can have the @CalledByNative in the interface, then have
> the key exposing its "@CalledByNative getKeyStore()", and then in the
following
> .cc file we just do something like:
>   Java_AndroidKeyStore_getRSAKeyModules(....,
> Java_AndroidPrivateKey_getKeyStore());

Done.

https://codereview.chromium.org/166143002/diff/20001/net/android/java/src/org...
File net/android/java/src/org/chromium/net/AndroidKeyStoreLocalImpl.java
(right):

https://codereview.chromium.org/166143002/diff/20001/net/android/java/src/org...
net/android/java/src/org/chromium/net/AndroidKeyStoreLocalImpl.java:25: public
class AndroidKeyStoreLocalImpl implements AndroidKeyStore {
On 2014/02/14 10:45:29, bulach wrote:
> nit: perhaps "InProcess" instead of "Local"?

Done.

https://codereview.chromium.org/166143002/diff/20001/net/android/java/src/org...
net/android/java/src/org/chromium/net/AndroidKeyStoreLocalImpl.java:59: } else {
On 2014/02/14 10:45:29, bulach wrote:
> remove the "else" here and in the following methods too.

Done.

https://codereview.chromium.org/166143002/diff/20001/net/android/java/src/org...
File net/android/java/src/org/chromium/net/AndroidPrivateKey.java (right):

https://codereview.chromium.org/166143002/diff/20001/net/android/java/src/org...
net/android/java/src/org/chromium/net/AndroidPrivateKey.java:11: /** @return
AndroidKeyStore that handles this key. */
On 2014/02/14 10:45:29, bulach wrote:
> as above, I think this can be @CalledByNative

Done.

https://codereview.chromium.org/166143002/diff/20001/net/android/java/src/org...
File net/android/java/src/org/chromium/net/interface.aidl (right):

https://codereview.chromium.org/166143002/diff/20001/net/android/java/src/org...
net/android/java/src/org/chromium/net/interface.aidl:5: interface
org.chomium.net.IAndroidKeyStoreRemote;
On 2014/02/14 12:48:59, bulach wrote:
> does this file need to be called "interface.aidl"?
> can it be more descriptive?

Done.

https://codereview.chromium.org/166143002/diff/20001/net/android/keystore.cc
File net/android/keystore.cc (right):

https://codereview.chromium.org/166143002/diff/20001/net/android/keystore.cc#...
net/android/keystore.cc:13: #include "jni/AndroidKeyStoreBridge_jni.h"
On 2014/02/14 10:45:29, bulach wrote:
> if my suggestion above works, then this will become again
AndroidKeyStore_jni.h,
> and the calls below would be something like:
> Java_AndroidKeyStore_getRSAKeyModulus(env, 
>   Java_AndroidKey_getKeyStore(env, private_key_ref)));
> 
> or rather:
> Java_AndroidKeyStore_getRSAKeyModulus(env, 
>   AndroidKey::GetKeyStore(env, private_key_ref)));
> 
> where AndroidKey is a tiny .h/.cc exposing the GetKeyStore from java, and the
> AndroidKeyStoreBridge will be gone..
> 
> wdyt?

Done.

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

https://codereview.chromium.org/166143002/diff/270001/net/android/java/src/or...
File net/android/java/src/org/chromium/net/AndroidKeyStore.java (right):

https://codereview.chromium.org/166143002/diff/270001/net/android/java/src/or...
net/android/java/src/org/chromium/net/AndroidKeyStore.java:48: int
getOpenSSLHandleForPrivateKey(AndroidPrivateKey key);
Why did you re-order these methods? Makes it much harder to read the diff

https://codereview.chromium.org/166143002/diff/270001/net/android/java/src/or...
File net/android/java/src/org/chromium/net/AndroidKeyStoreInProcessImpl.java
(right):

https://codereview.chromium.org/166143002/diff/270001/net/android/java/src/or...
net/android/java/src/org/chromium/net/AndroidKeyStoreInProcessImpl.java:25:
public class AndroidKeyStoreInProcessImpl implements AndroidKeyStore {
Naming wise, this seems unduly verbose.

I would just go with DefaultAndroidKeyStore and RemoteAndroidKeyStore

The "impl" seems needed if-f you're keeping the name the same (eg: an
alternative scheme is "AndroidKeyStoreImpl" and "AndroidKeyStoreRemote")

https://codereview.chromium.org/166143002/diff/270001/net/android/java/src/or...
File net/android/java/src/org/chromium/net/AndroidKeyStoreRemoteImpl.java
(right):

https://codereview.chromium.org/166143002/diff/270001/net/android/java/src/or...
net/android/java/src/org/chromium/net/AndroidKeyStoreRemoteImpl.java:37: private
static final String TAG = "AndroidKeyStoreRemoteImpl";
You ordering between the two classes is inconsistent - ex: the TAG appears first
in the other class.

https://codereview.chromium.org/166143002/diff/270001/net/android/java/src/or...
net/android/java/src/org/chromium/net/AndroidKeyStoreRemoteImpl.java:113: public
int getOpenSSLHandleForPrivateKey(AndroidPrivateKey privateKey) {
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?

https://codereview.chromium.org/166143002/diff/270001/net/android/java/src/or...
net/android/java/src/org/chromium/net/AndroidKeyStoreRemoteImpl.java:115: assert
false;
Your ordering within this class does not match the interface order.

[Yaron, 2014-02-14 22:06:58]

Updated based on your feedback.

https://codereview.chromium.org/166143002/diff/270001/net/android/java/src/or...
File net/android/java/src/org/chromium/net/AndroidKeyStore.java (right):

https://codereview.chromium.org/166143002/diff/270001/net/android/java/src/or...
net/android/java/src/org/chromium/net/AndroidKeyStore.java:48: int
getOpenSSLHandleForPrivateKey(AndroidPrivateKey key);
On 2014/02/14 20:57:51, Ryan Sleevi wrote:
> Why did you re-order these methods? Makes it much harder to read the diff

Not intentional, sorry. Can't honestly say when it happened between patches but
restored the ordering.

https://codereview.chromium.org/166143002/diff/270001/net/android/java/src/or...
File net/android/java/src/org/chromium/net/AndroidKeyStoreInProcessImpl.java
(right):

https://codereview.chromium.org/166143002/diff/270001/net/android/java/src/or...
net/android/java/src/org/chromium/net/AndroidKeyStoreInProcessImpl.java:25:
public class AndroidKeyStoreInProcessImpl implements AndroidKeyStore {
On 2014/02/14 20:57:51, Ryan Sleevi wrote:
> Naming wise, this seems unduly verbose.
> 
> I would just go with DefaultAndroidKeyStore and RemoteAndroidKeyStore
> 
> The "impl" seems needed if-f you're keeping the name the same (eg: an
> alternative scheme is "AndroidKeyStoreImpl" and "AndroidKeyStoreRemote")

Fair enough. Default* and Remote* it is. ALso updated the aidl files..

https://codereview.chromium.org/166143002/diff/270001/net/android/java/src/or...
File net/android/java/src/org/chromium/net/AndroidKeyStoreRemoteImpl.java
(right):

https://codereview.chromium.org/166143002/diff/270001/net/android/java/src/or...
net/android/java/src/org/chromium/net/AndroidKeyStoreRemoteImpl.java:37: private
static final String TAG = "AndroidKeyStoreRemoteImpl";
On 2014/02/14 20:57:51, Ryan Sleevi wrote:
> You ordering between the two classes is inconsistent - ex: the TAG appears
first
> in the other class.

Done.

https://codereview.chromium.org/166143002/diff/270001/net/android/java/src/or...
net/android/java/src/org/chromium/net/AndroidKeyStoreRemoteImpl.java:113: public
int getOpenSSLHandleForPrivateKey(AndroidPrivateKey privateKey) {
On 2014/02/14 20:57:51, Ryan Sleevi wrote:
> 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?

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).

https://codereview.chromium.org/166143002/diff/270001/net/android/java/src/or...
net/android/java/src/org/chromium/net/AndroidKeyStoreRemoteImpl.java:115: assert
false;
On 2014/02/14 20:57:51, Ryan Sleevi wrote:
> Your ordering within this class does not match the interface order.

Done.

[klobag.chromium, 2014-02-14 23:03:52]

Minor suggestions.

https://chromiumcodereview.appspot.com/166143002/diff/160003/net/android/java...
File net/android/java/src/org/chromium/net/AndroidKeyStore.java (right):

https://chromiumcodereview.appspot.com/166143002/diff/160003/net/android/java...
net/android/java/src/org/chromium/net/AndroidKeyStore.java:14: public interface
AndroidKeyStore {
If we keep AndroidKeyStore as class, and introduce a new interface class
AndroidKeyStoreBase, we don't need to expose AndroidPrivateKey to native.

e.g 

public static byte[] getRSAKeyModulus(AndroidPrivateKey key) {
  key.getKeyStore().getRSAKeyModulus(key);
}

https://chromiumcodereview.appspot.com/166143002/diff/160003/net/android/java...
File net/android/java/src/org/chromium/net/IRemoteAndroidKeyStoreCallbacks.aidl
(right):

https://chromiumcodereview.appspot.com/166143002/diff/160003/net/android/java...
net/android/java/src/org/chromium/net/IRemoteAndroidKeyStoreCallbacks.aidl:15:
void onDisabled();
Add onInitComplete().

[Yaron, 2014-02-14 23:05:49]

Working on other comment

https://chromiumcodereview.appspot.com/166143002/diff/160003/net/android/java...
File net/android/java/src/org/chromium/net/AndroidKeyStore.java (right):

https://chromiumcodereview.appspot.com/166143002/diff/160003/net/android/java...
net/android/java/src/org/chromium/net/AndroidKeyStore.java:14: public interface
AndroidKeyStore {
On 2014/02/14 23:03:52, klobag.chromium wrote:
> If we keep AndroidKeyStore as class, and introduce a new interface class
> AndroidKeyStoreBase, we don't need to expose AndroidPrivateKey to native.
> 
> e.g 
> 
> public static byte[] getRSAKeyModulus(AndroidPrivateKey key) {
>   key.getKeyStore().getRSAKeyModulus(key);
> }

That's essentially what I had before form ppi's AndroidKeyStoreBridge but Marcus
suggested this. Not switching back at this point... I think Ryan's suggestion on
internal CL is more on point of hiding this better into EVP_PKEY

[klobag.chromium, 2014-02-14 23:15:22]

https://chromiumcodereview.appspot.com/166143002/diff/20001/net/android/java/...
File net/android/java/src/org/chromium/net/AndroidKeyStore.java (right):

https://chromiumcodereview.appspot.com/166143002/diff/20001/net/android/java/...
net/android/java/src/org/chromium/net/AndroidKeyStore.java:8: * Abstract key
store. It can be implemented either as a local store operating on Java
PrivateKeys
Sorry I didn't see this.

I agree with Yaron on keeping KeyStore pure Java. In the new code, we have to
add a new JNI.

Anyway we can decide the style later. Let us check this in first.

thanks.

On 2014/02/14 17:29:28, Yaron wrote:
> On 2014/02/14 10:45:29, bulach wrote:
> > nit: Key store interface :) but tbh, I'd remove this comment altogether...
as
> > far as this interface goes, it can be implemented with return 'hardcodedkey'
> :)
> > 
> > 
> > also... I think @CalledByNative should be supported in interfaces? [1]
> > if it does work, we can then remove the following "AndroidKeyStoreBridge"?
> > 
> > probably need @CalledByNative for AndroidPrivateKey.getKeyStore() and the
> > corresponding .h/.cc, but I think it'd be overall simpler than keeping the
> > chunkier AndroidKeyStoreBridge... wdyt?
> > 
> > [1]
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/android_webview/ja...
> 
> My main reason to not added @CalledByNative is to keep this pure java. This
> simplifies the helper process. Even if the helper process doesn't use the
native
> bits, it'd have to include the annotation. Not a big deal, and while I think
> your suggestion is slightly cleaner, I think the bridge class is workable.

[klobag.chromium, 2014-02-14 23:30:15]

lgtm

Thanks.

[Yaron, 2014-02-15 00:20:31]

On 2014/02/14 23:30:15, klobag.chromium wrote:
> lgtm
> 
> Thanks.

Ryan gave approval for net/net.gyp off-thread pending follow-ups to clean up
abstraction of AndroidPrivateKey and fit better into EVP_PKEY

THanks for reviews. Landing

[Yaron, 2014-02-15 00:20:38]

The CQ bit was checked by yfriedman@chromium.org

[commit-bot: I haz the power, 2014-02-15 00:38:05]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/yfriedman@chromium.org/166143002/710001

[Yaron, 2014-02-15 00:54:31]

Committed patchset #10 manually as r251461 (presubmit successful).

[enrico.ros, 2014-11-14 06:03:51]

enrico.ros@gmail.com changed reviewers:
+ enrico.ros@gmail.com

[enrico.ros, 2014-11-14 06:03:52]

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;
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.

[ppi, 2014-11-14 10:03:34]

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?