[webcrypto] Wire up {spki, pkcs8} import/export for OpenSSL.

[webcrypto] Wire up {spki, pkcs8} import/export for OpenSSL.

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

[Ryan Sleevi, 2014-06-27 01:43:25]

https://codereview.chromium.org/353043005/diff/1/content/child/webcrypto/plat...
File content/child/webcrypto/platform_crypto_openssl.cc (right):

https://codereview.chromium.org/353043005/diff/1/content/child/webcrypto/plat...
content/child/webcrypto/platform_crypto_openssl.cc:229: DCHECK(EVP_PKEY_id(key)
== EVP_PKEY_RSA);
DCHECK_EQ

https://codereview.chromium.org/353043005/diff/1/content/child/webcrypto/plat...
content/child/webcrypto/platform_crypto_openssl.cc:241: if
(static_cast<int>(e.size()) != BN_bn2bin(rsa.get()->e, &e[0]))
I think these may differ dependent on leading bits?

https://codereview.chromium.org/353043005/diff/1/content/child/webcrypto/plat...
content/child/webcrypto/platform_crypto_openssl.cc:610:
crypto::ScopedOpenSSL<PKCS8_PRIV_KEY_INFO, PKCS8_PRIV_KEY_INFO_free> p8inf(
// TODO reminder - need to validate the EVP data according to the spec.

[eroman, 2014-06-27 02:12:47]

https://codereview.chromium.org/353043005/diff/1/content/child/webcrypto/plat...
File content/child/webcrypto/platform_crypto_openssl.cc (right):

https://codereview.chromium.org/353043005/diff/1/content/child/webcrypto/plat...
content/child/webcrypto/platform_crypto_openssl.cc:229: DCHECK(EVP_PKEY_id(key)
== EVP_PKEY_RSA);
On 2014/06/27 01:43:25, Ryan Sleevi wrote:
> DCHECK_EQ

Done.

https://codereview.chromium.org/353043005/diff/1/content/child/webcrypto/plat...
content/child/webcrypto/platform_crypto_openssl.cc:241: if
(static_cast<int>(e.size()) != BN_bn2bin(rsa.get()->e, &e[0]))
On 2014/06/27 01:43:25, Ryan Sleevi wrote:
> I think these may differ dependent on leading bits?

Looks like at least in the current implementation, the return value can be
nothing other than BN_num_bytes():

https://code.google.com/p/chromium/codesearch#chromium/src/third_party/openss...

I could remove the check alltogether and turn it into an assertion if this is
something I can assume will never have future failure modes.

https://codereview.chromium.org/353043005/diff/1/content/child/webcrypto/plat...
content/child/webcrypto/platform_crypto_openssl.cc:610:
crypto::ScopedOpenSSL<PKCS8_PRIV_KEY_INFO, PKCS8_PRIV_KEY_INFO_free> p8inf(
On 2014/06/27 01:43:24, Ryan Sleevi wrote:
> // TODO reminder - need to validate the EVP data according to the spec.

Done. I have added comments linking to the corresponding bugs.

[eroman, 2014-07-18 03:15:09]

PTAL, I have rebased this onto the upcoming webcrypto refactor

[agl, 2014-07-18 16:55:19]

https://codereview.chromium.org/353043005/diff/60001/content/child/webcrypto/...
File content/child/webcrypto/openssl/rsa_key_openssl.cc (right):

https://codereview.chromium.org/353043005/diff/60001/content/child/webcrypto/...
content/child/webcrypto/openssl/rsa_key_openssl.cc:32: if
(!i2d_PUBKEY_bio(bio.get(), key))
This is fine, although if it doesn't allow control of the OIDs as needed, it
might be a reason to rewrite it in BoringSSL with the CBB functions.

https://codereview.chromium.org/353043005/diff/60001/content/child/webcrypto/...
content/child/webcrypto/openssl/rsa_key_openssl.cc:36: long len =
BIO_get_mem_data(bio.get(), &data);
this will fail with BoringSSL because BIO_get_mem_data is now a function not a
macro and the second argument is a char**.

However, I agree that it should be a uint8_t. Also BIO_get_mem_data returns zero
on error and when the buffer is empty.

I've added BIO_mem_contents to fix this:
https://boringssl-review.googlesource.com/#/c/1261/

If it's not rolled into Chromium when this lands, I'll fix this up once it is.

https://codereview.chromium.org/353043005/diff/60001/content/child/webcrypto/...
content/child/webcrypto/openssl/rsa_key_openssl.cc:50: if
(!i2d_PKCS8PrivateKeyInfo_bio(bio.get(), key))
ditto here and about BIO_get_mem_data.

https://codereview.chromium.org/353043005/diff/60001/content/child/webcrypto/...
content/child/webcrypto/openssl/rsa_key_openssl.cc:76: unsigned int
modulus_length_bits = RSA_size(rsa.get()) * 8;
Since you are reaching in for |e| anyway, you might want
|BN_num_bits(rsa.get()->n))|.

https://codereview.chromium.org/353043005/diff/60001/content/child/webcrypto/...
content/child/webcrypto/openssl/rsa_key_openssl.cc:82: if
(static_cast<int>(e.size()) != BN_bn2bin(rsa.get()->e, &e[0]))
cast to int not needed with BoringSSL (which is currently landed).

[agl, 2014-07-18 16:55:33]

LGTM (I'll probably be at the IETF after this so don't wait for me.)

[eroman, 2014-07-18 20:17:31]

https://codereview.chromium.org/353043005/diff/60001/content/child/webcrypto/...
File content/child/webcrypto/openssl/rsa_key_openssl.cc (right):

https://codereview.chromium.org/353043005/diff/60001/content/child/webcrypto/...
content/child/webcrypto/openssl/rsa_key_openssl.cc:32: if
(!i2d_PUBKEY_bio(bio.get(), key))
On 2014/07/18 16:55:19, agl wrote:
> This is fine, although if it doesn't allow control of the OIDs as needed, it
> might be a reason to rewrite it in BoringSSL with the CBB functions.

Acknowledged.

https://codereview.chromium.org/353043005/diff/60001/content/child/webcrypto/...
content/child/webcrypto/openssl/rsa_key_openssl.cc:36: long len =
BIO_get_mem_data(bio.get(), &data);
On 2014/07/18 16:55:18, agl wrote:
> this will fail with BoringSSL because BIO_get_mem_data is now a function not a
> macro and the second argument is a char**.
> 
> However, I agree that it should be a uint8_t. Also BIO_get_mem_data returns
zero
> on error and when the buffer is empty.
> 
> I've added BIO_mem_contents to fix this:
> https://boringssl-review.googlesource.com/#/c/1261/
> 
> If it's not rolled into Chromium when this lands, I'll fix this up once it is.

Done.

https://codereview.chromium.org/353043005/diff/60001/content/child/webcrypto/...
content/child/webcrypto/openssl/rsa_key_openssl.cc:50: if
(!i2d_PKCS8PrivateKeyInfo_bio(bio.get(), key))
On 2014/07/18 16:55:19, agl wrote:
> ditto here and about BIO_get_mem_data.

Done.

https://codereview.chromium.org/353043005/diff/60001/content/child/webcrypto/...
content/child/webcrypto/openssl/rsa_key_openssl.cc:76: unsigned int
modulus_length_bits = RSA_size(rsa.get()) * 8;
On 2014/07/18 16:55:18, agl wrote:
> Since you are reaching in for |e| anyway, you might want
> |BN_num_bits(rsa.get()->n))|.

Done.

https://codereview.chromium.org/353043005/diff/60001/content/child/webcrypto/...
content/child/webcrypto/openssl/rsa_key_openssl.cc:82: if
(static_cast<int>(e.size()) != BN_bn2bin(rsa.get()->e, &e[0]))
On 2014/07/18 16:55:18, agl wrote:
> cast to int not needed with BoringSSL (which is currently landed).

Done.

[eroman, 2014-07-19 07:01:48]

The CQ bit was checked by eroman@chromium.org

[commit-bot: I haz the power, 2014-07-19 07:02:18]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/eroman@chromium.org/353043005/120001

[commit-bot: I haz the power, 2014-07-19 12:48:17]

Change committed as 284331