Fix Android cryptographic key pair storage.

Fix Android cryptographic key pair storage.

This patch fixes an issue with the way public/private key pairs
that were generated from keygen are installed on the system.

- First, the generated data bytes were not in a format that
  the system supports.

- Second, the public and private key pairs were swapped when
  they were sent to the CertInstaller activity.

This fixes both issues at the same time.

BUG=124660
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=164088

[digit1, 2012-10-24 12:35:55]

Note that this was isolated from the larger patch at
https://chromiumcodereview.appspot.com/11031043/ to make it easier to review.

[joth, 2012-10-24 19:40:28]

lgtm.

(I believe the main substance of the change was already reviewed in the m18
branch?).

https://codereview.chromium.org/11260015/diff/1/net/android/java/src/org/chro...
File net/android/java/src/org/chromium/net/AndroidNetworkLibrary.java (right):

https://codereview.chromium.org/11260015/diff/1/net/android/java/src/org/chro...
net/android/java/src/org/chromium/net/AndroidNetworkLibrary.java:35: * @param
context: current activity context.
question: I think native code normally passes over the application not activity
context?

https://codereview.chromium.org/11260015/diff/1/net/android/java/src/org/chro...
net/android/java/src/org/chromium/net/AndroidNetworkLibrary.java:47: byte[]
private_key) {
nit: java style param wrap is double indent (not alighn with paren) and bunch up
multiple params per line.

https://codereview.chromium.org/11260015/diff/1/net/base/openssl_private_key_...
File net/base/openssl_private_key_store_android.cc (right):

https://codereview.chromium.org/11260015/diff/1/net/base/openssl_private_key_...
net/base/openssl_private_key_store_android.cc:26: // Important: Do not use
i2d_PublicKey() here, which returns data in
nit: this may be a little easier on eyes to put a newline before each of the
Important: blocks.

[Ryan Sleevi, 2012-10-24 21:16:18]

LGTM, mod nits.

http://codereview.chromium.org/11260015/diff/1/net/base/openssl_private_key_s...
File net/base/openssl_private_key_store_android.cc (right):

http://codereview.chromium.org/11260015/diff/1/net/base/openssl_private_key_s...
net/base/openssl_private_key_store_android.cc:26: // Important: Do not use
i2d_PublicKey() here, which returns data in
On 2012/10/24 19:40:28, joth wrote:
> nit: this may be a little easier on eyes to put a newline before each of the
> Important: blocks.

+1

http://codereview.chromium.org/11260015/diff/1/net/base/openssl_private_key_s...
net/base/openssl_private_key_store_android.cc:27: // PKKCS#1 format, use
i2d_PUBKEY() which returns it as DER-encoded
typo: PKCS#1

http://codereview.chromium.org/11260015/diff/1/net/base/openssl_private_key_s...
net/base/openssl_private_key_store_android.cc:30: int public_len =
i2d_PUBKEY(pkey, &public_key);
nit: I feel slightly better with a ScopedOpenSSL<uint8>, but not enough to
require nit

(PEDANTIC) BUG: Relying on unsigned char == uint8 is one of those pedantry
issues. It would be better to use "unsigned char*" here, rather than uint8, at
least when supplying a pointer-to-a-pointer.

http://codereview.chromium.org/11260015/diff/1/net/base/openssl_private_key_s...
net/base/openssl_private_key_store_android.cc:34: // "error:0D0680A8:asn1
encoding routines:ASN1_CHECK_TLEN:wrong tag"
nit: This feels a leftover comment of the investigation, and suggests "We don't
know why it's doing this".

Seems like we should know why it was doing that, and comment appropriately.

http://codereview.chromium.org/11260015/diff/1/net/base/openssl_private_key_s...
net/base/openssl_private_key_store_android.cc:37: PKCS8_PRIV_KEY_INFO* pkcs8 =
EVP_PKEY2PKCS8(pkey);
nit: ScopedOpenSSL<EVP_PKEY2PKCS8>

[digit1, 2012-10-25 14:34:32]

https://codereview.chromium.org/11260015/diff/1/net/android/java/src/org/chro...
File net/android/java/src/org/chromium/net/AndroidNetworkLibrary.java (right):

https://codereview.chromium.org/11260015/diff/1/net/android/java/src/org/chro...
net/android/java/src/org/chromium/net/AndroidNetworkLibrary.java:35: * @param
context: current activity context.
That's correct, updated.

https://codereview.chromium.org/11260015/diff/1/net/android/java/src/org/chro...
net/android/java/src/org/chromium/net/AndroidNetworkLibrary.java:47: byte[]
private_key) {
On 2012/10/24 19:40:28, joth wrote:
> nit: java style param wrap is double indent (not alighn with paren) and bunch
up
> multiple params per line.

Done.

https://codereview.chromium.org/11260015/diff/1/net/base/openssl_private_key_...
File net/base/openssl_private_key_store_android.cc (right):

https://codereview.chromium.org/11260015/diff/1/net/base/openssl_private_key_...
net/base/openssl_private_key_store_android.cc:26: // Important: Do not use
i2d_PublicKey() here, which returns data in
On 2012/10/24 19:40:28, joth wrote:
> nit: this may be a little easier on eyes to put a newline before each of the
> Important: blocks.

Done.

https://codereview.chromium.org/11260015/diff/1/net/base/openssl_private_key_...
net/base/openssl_private_key_store_android.cc:27: // PKKCS#1 format, use
i2d_PUBKEY() which returns it as DER-encoded
On 2012/10/24 21:16:18, Ryan Sleevi wrote:
> typo: PKCS#1

Done.

https://codereview.chromium.org/11260015/diff/1/net/base/openssl_private_key_...
net/base/openssl_private_key_store_android.cc:30: int public_len =
i2d_PUBKEY(pkey, &public_key);
On 2012/10/24 21:16:18, Ryan Sleevi wrote:
> nit: I feel slightly better with a ScopedOpenSSL<uint8>, but not enough to
> require nit
> 
That's not going to work because the ScopedOpenSSL<> template doesn't allow you
to get the address of its internal pointer, which is required by the call to
i2d_PUBKEY(). I could add a method to it to do that though if you feel it's
important enough.

> (PEDANTIC) BUG: Relying on unsigned char == uint8 is one of those pedantry
> issues. It would be better to use "unsigned char*" here, rather than uint8, at
> least when supplying a pointer-to-a-pointer.

Done.

https://codereview.chromium.org/11260015/diff/1/net/base/openssl_private_key_...
net/base/openssl_private_key_store_android.cc:34: // "error:0D0680A8:asn1
encoding routines:ASN1_CHECK_TLEN:wrong tag"
When I investigated the issue, I didn't really understand the difference, even
after peeking into the OpenSSL source code. I didn't have really time to extract
the data and compare it so that was the best I could come up with. For now, I'll
just remove the above two lasts lines.

https://codereview.chromium.org/11260015/diff/1/net/base/openssl_private_key_...
net/base/openssl_private_key_store_android.cc:37: PKCS8_PRIV_KEY_INFO* pkcs8 =
EVP_PKEY2PKCS8(pkey);
On 2012/10/24 21:16:18, Ryan Sleevi wrote:
> nit: ScopedOpenSSL<EVP_PKEY2PKCS8>

Done.

[commit-bot: I haz the power, 2012-10-25 14:39:03]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/digit@chromium.org/11260015/6002

[commit-bot: I haz the power, 2012-10-25 16:28:47]

Change committed as 164088