Insulate the legacy Android client auth code from OpenSSL ABI changes.

Insulate the legacy Android client auth code from OpenSSL ABI changes.

The current implementation assumes the Android system OpenSSL and our copy have
compatible ABIs. This will be a problem when switching to BoringSSL which has
already changed from ABIs. Moreoever, it's already slightly off now as any
globals (error queue and locks) have different instances between the two.

Rework the code to never mix the two ABIs. We replicate the subset of OpenSSL
ABI we care about and use it to find the rsa_priv_enc implementation. This is
then mapped to Chromium OpenSSL via the custom RSA_METHOD. In addition, because
we cannot safely increase reference counts, retain references to Java wrappers
where appropriate in lieu of the C structures.

Also fix AndroidKeyStore unit tests on 4.1; they broke after an overzealous
NOTREACHED() in r278305.

BUG=389414
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=282713

[davidben, 2014-07-01 19:33:27]

Tested manually with MIT certificates on Android 4.1.2 (Galaxy Nexus) and
Android 4.4.2 (Nexus 4). Also ran net_unittests AndroidKeyStore.* on both.

[Yaron, 2014-07-08 20:01:27]

the java-y parts lgtm but i'm less familiar with the openssl and
android-internal parts

https://codereview.chromium.org/365503007/diff/60001/net/android/java/src/org...
File net/android/java/src/org/chromium/net/DefaultAndroidKeyStore.java (right):

https://codereview.chromium.org/365503007/diff/60001/net/android/java/src/org...
net/android/java/src/org/chromium/net/DefaultAndroidKeyStore.java:272: return 0;
return null

https://codereview.chromium.org/365503007/diff/60001/net/android/java/src/org...
net/android/java/src/org/chromium/net/DefaultAndroidKeyStore.java:295: return 0;
return null

[davidben, 2014-07-08 20:20:49]

https://codereview.chromium.org/365503007/diff/60001/net/android/java/src/org...
File net/android/java/src/org/chromium/net/DefaultAndroidKeyStore.java (right):

https://codereview.chromium.org/365503007/diff/60001/net/android/java/src/org...
net/android/java/src/org/chromium/net/DefaultAndroidKeyStore.java:272: return 0;
On 2014/07/08 20:01:27, Yaron wrote:
> return null

Yikes, that probably compiled thanks to autoboxing, didn't it? Done.

https://codereview.chromium.org/365503007/diff/60001/net/android/java/src/org...
net/android/java/src/org/chromium/net/DefaultAndroidKeyStore.java:295: return 0;
On 2014/07/08 20:01:27, Yaron wrote:
> return null

Done.

[Yaron, 2014-07-08 20:38:09]

On Tue, Jul 8, 2014 at 1:20 PM, <davidben@chromium.org> wrote:

>
> https://codereview.chromium.org/365503007/diff/60001/net/
> android/java/src/org/chromium/net/DefaultAndroidKeyStore.java
> File net/android/java/src/org/chromium/net/DefaultAndroidKeyStore.java
> (right):
>
> https://codereview.chromium.org/365503007/diff/60001/net/
> android/java/src/org/chromium/net/DefaultAndroidKeyStore.java#newcode272
> net/android/java/src/org/chromium/net/DefaultAndroidKeyStore.java:272:
> return 0;
> On 2014/07/08 20:01:27, Yaron wrote:
>
>> return null
>>
>
> Yikes, that probably compiled thanks to autoboxing, didn't it? Done.


Yep

>
>
> https://codereview.chromium.org/365503007/diff/60001/net/
> android/java/src/org/chromium/net/DefaultAndroidKeyStore.java#newcode295
> net/android/java/src/org/chromium/net/DefaultAndroidKeyStore.java:295:
> return 0;
> On 2014/07/08 20:01:27, Yaron wrote:
>
>> return null
>>
>
> Done.
>
> https://codereview.chromium.org/365503007/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[agl, 2014-07-10 16:33:44]

https://codereview.chromium.org/365503007/diff/80001/net/android/keystore.h
File net/android/keystore.h (right):

https://codereview.chromium.org/365503007/diff/80001/net/android/keystore.h#n...
net/android/keystore.h:111: // Returns a JNI reference to the OpenSSLEngine
object which used to back a
verb needed after "which".

https://codereview.chromium.org/365503007/diff/80001/net/android/keystore_ope...
File net/android/keystore_openssl.cc (right):

https://codereview.chromium.org/365503007/diff/80001/net/android/keystore_ope...
net/android/keystore_openssl.cc:179: RSAerr(RSA_F_RSA_PRIVATE_ENCRYPT,
ERR_R_INTERNAL_ERROR);
Do we need to clear the system's error queue?

https://codereview.chromium.org/365503007/diff/80001/net/android/keystore_ope...
net/android/keystore_openssl.cc:300: // |private_key| is the JNI reference
(local or global) to the object.
Probably needs to mention |legacy_rsa|.

[davidben, 2014-07-10 21:47:07]

https://codereview.chromium.org/365503007/diff/80001/net/android/keystore.h
File net/android/keystore.h (right):

https://codereview.chromium.org/365503007/diff/80001/net/android/keystore.h#n...
net/android/keystore.h:111: // Returns a JNI reference to the OpenSSLEngine
object which used to back a
On 2014/07/10 16:33:44, agl wrote:
> verb needed after "which".

Done.

https://codereview.chromium.org/365503007/diff/80001/net/android/keystore_ope...
File net/android/keystore_openssl.cc (right):

https://codereview.chromium.org/365503007/diff/80001/net/android/keystore_ope...
net/android/keystore_openssl.cc:179: RSAerr(RSA_F_RSA_PRIVATE_ENCRYPT,
ERR_R_INTERNAL_ERROR);
On 2014/07/10 16:33:44, agl wrote:
> Do we need to clear the system's error queue?

Added a TODO, but doing so seems to be difficult. I tried dlopen("libcrypto.so")
to get at the function, but libcrypto doesn't get loaded through dlopen, so we
end up with two copies of the library. I got some bizarre hangs that didn't
occur without the CL, so I assume something broke from that.

The other option I can see is to find some random Java function to call which
happens to dump the error queue as a side effect.

https://codereview.chromium.org/365503007/diff/80001/net/android/keystore_ope...
net/android/keystore_openssl.cc:300: // |private_key| is the JNI reference
(local or global) to the object.
On 2014/07/10 16:33:44, agl wrote:
> Probably needs to mention |legacy_rsa|.

Done.

[agl, 2014-07-11 19:11:37]

LGTM

[davidben, 2014-07-11 19:13:09]

The CQ bit was checked by davidben@chromium.org

[commit-bot: I haz the power, 2014-07-11 19:14:14]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/davidben@chromium.org/365503007/160001

[commit-bot: I haz the power, 2014-07-11 22:08:23]

Change committed as 282713