[webcrypto] Add JWK import of RSA public key for NSS.

[webcrypto] Add JWK import of RSA public key for NSS.

This CL depends on Issue 25906002.

BUG=245025
TEST=content_unittests --gtest_filter="WebCryptoImpl*"

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

[eroman, 2013-11-20 23:37:12]

https://codereview.chromium.org/76363006/diff/1/content/renderer/webcrypto/we...
File content/renderer/webcrypto/webcrypto_impl_nss.cc (right):

https://codereview.chromium.org/76363006/diff/1/content/renderer/webcrypto/we...
content/renderer/webcrypto/webcrypto_impl_nss.cc:657: bool
WebCryptoImpl::ImportRsaPublicKeyInternal(
@rsleevi, dark lord of NSS: Can you review this function?

[Ryan Sleevi, 2013-11-21 01:41:34]

I only reviewed the file requested.

https://codereview.chromium.org/76363006/diff/1/content/renderer/webcrypto/we...
File content/renderer/webcrypto/webcrypto_impl_nss.cc (right):

https://codereview.chromium.org/76363006/diff/1/content/renderer/webcrypto/we...
content/renderer/webcrypto/webcrypto_impl_nss.cc:242: typedef scoped_ptr_malloc<
scoped_ptr_malloc is DEPRECATED, as scoped_ptr supports the case of passing
custom deleters now.

See
https://code.google.com/p/chromium/codesearch#chromium/src/base/memory/scoped...

https://codereview.chromium.org/76363006/diff/1/content/renderer/webcrypto/we...
content/renderer/webcrypto/webcrypto_impl_nss.cc:670: DCHECK(exponent_size);
Is Blink going to require these four fields be valid? I just want to confirm
they're fine as a DCHECK (eg: Blink guarantees it), and not something we should
be doing as actual checks.

https://codereview.chromium.org/76363006/diff/1/content/renderer/webcrypto/we...
content/renderer/webcrypto/webcrypto_impl_nss.cc:673: // modulus and exponent
values. But it can import an DER-encoded ASN.1 blob
"values. But" -> "values, but"

https://codereview.chromium.org/76363006/diff/1/content/renderer/webcrypto/we...
content/renderer/webcrypto/webcrypto_impl_nss.cc:697: {0, }};
nit: I seem to recall that clang-format/the style guide expects spaces following
the leading {

https://codereview.chromium.org/76363006/diff/1/content/renderer/webcrypto/we...
content/renderer/webcrypto/webcrypto_impl_nss.cc:701: // SECItem (pubkey_der) on
success, or NULL on failure.
A subtle element of this is the fact that the SECItem is allocated from the
PRArenaPool.

// DER-encode the public key. The returned SECItem* is allocated from
// the specified arena, and thus does not need to be explicitly freed.


I'm not a fan of documenting the function being called (eg: "on success, ... or
NULL of failure". Also, the "Do the ASN.1 encoding" is a bit redundant with the
function call; comments that start with "Do" can usually be reworded like I
tried.

That said, I don't understand why you manually create the arena. The
SEC_ASN1EncodeItem API permits you to supply NULL for the pool, and use the
'easier' APIs like ScopedSECItem - see
http://mxr.mozilla.org/nss/source/lib/util/secasn1.h#118

https://codereview.chromium.org/76363006/diff/1/content/renderer/webcrypto/we...
content/renderer/webcrypto/webcrypto_impl_nss.cc:709: // Import the DER to
create an RSA SECKEYPublicKey.
nit: // Import the DER-encoded public key to...

[eroman, 2013-11-21 01:55:41]

https://codereview.chromium.org/76363006/diff/1/content/renderer/webcrypto/we...
File content/renderer/webcrypto/webcrypto_impl_nss.cc (right):

https://codereview.chromium.org/76363006/diff/1/content/renderer/webcrypto/we...
content/renderer/webcrypto/webcrypto_impl_nss.cc:670: DCHECK(exponent_size);
On 2013/11/21 01:41:34, Ryan Sleevi wrote:
> Is Blink going to require these four fields be valid? I just want to confirm
> they're fine as a DCHECK (eg: Blink guarantees it), and not something we
should
> be doing as actual checks.

The caller here is the JWK parsing code, which validates that none of these are
empty. That said it could be more future proof to fail on empty data instead.

[padolph, 2013-11-21 03:38:12]

https://codereview.chromium.org/76363006/diff/1/content/renderer/webcrypto/we...
File content/renderer/webcrypto/webcrypto_impl_nss.cc (right):

https://codereview.chromium.org/76363006/diff/1/content/renderer/webcrypto/we...
content/renderer/webcrypto/webcrypto_impl_nss.cc:242: typedef scoped_ptr_malloc<
On 2013/11/21 01:41:34, Ryan Sleevi wrote:
> scoped_ptr_malloc is DEPRECATED, as scoped_ptr supports the case of passing
> custom deleters now.
> 
> See
>
https://code.google.com/p/chromium/codesearch#chromium/src/base/memory/scoped...

This typedef is no longer needed because of the change below.

https://codereview.chromium.org/76363006/diff/1/content/renderer/webcrypto/we...
content/renderer/webcrypto/webcrypto_impl_nss.cc:670: DCHECK(exponent_size);
On 2013/11/21 01:55:41, eroman wrote:
> On 2013/11/21 01:41:34, Ryan Sleevi wrote:
> > Is Blink going to require these four fields be valid? I just want to confirm
> > they're fine as a DCHECK (eg: Blink guarantees it), and not something we
> should
> > be doing as actual checks.
> 
> The caller here is the JWK parsing code, which validates that none of these
are
> empty. That said it could be more future proof to fail on empty data instead.

Done.

https://codereview.chromium.org/76363006/diff/1/content/renderer/webcrypto/we...
content/renderer/webcrypto/webcrypto_impl_nss.cc:673: // modulus and exponent
values. But it can import an DER-encoded ASN.1 blob
On 2013/11/21 01:41:34, Ryan Sleevi wrote:
> "values. But" -> "values, but"

Done.

https://codereview.chromium.org/76363006/diff/1/content/renderer/webcrypto/we...
content/renderer/webcrypto/webcrypto_impl_nss.cc:697: {0, }};
On 2013/11/21 01:41:34, Ryan Sleevi wrote:
> nit: I seem to recall that clang-format/the style guide expects spaces
following
> the leading {

The current format is what clang-format -style=Chromium produced. I could not
find info either way in the Chromium style guide. Should I change this?

https://codereview.chromium.org/76363006/diff/1/content/renderer/webcrypto/we...
content/renderer/webcrypto/webcrypto_impl_nss.cc:701: // SECItem (pubkey_der) on
success, or NULL on failure.
On 2013/11/21 01:41:34, Ryan Sleevi wrote:
> A subtle element of this is the fact that the SECItem is allocated from the
> PRArenaPool.
> 
> // DER-encode the public key. The returned SECItem* is allocated from
> // the specified arena, and thus does not need to be explicitly freed.
> 
> 
> I'm not a fan of documenting the function being called (eg: "on success, ...
or
> NULL of failure". Also, the "Do the ASN.1 encoding" is a bit redundant with
the
> function call; comments that start with "Do" can usually be reworded like I
> tried.

Done.
 
> That said, I don't understand why you manually create the arena. The
> SEC_ASN1EncodeItem API permits you to supply NULL for the pool, and use the
> 'easier' APIs like ScopedSECItem - see
> http://mxr.mozilla.org/nss/source/lib/util/secasn1.h#118

Copy/paste from the NSS tech note. Using the NULL calling method does make
things simpler. Thank you.

https://codereview.chromium.org/76363006/diff/1/content/renderer/webcrypto/we...
content/renderer/webcrypto/webcrypto_impl_nss.cc:709: // Import the DER to
create an RSA SECKEYPublicKey.
On 2013/11/21 01:41:34, Ryan Sleevi wrote:
> nit: // Import the DER-encoded public key to...

Done.

[eroman, 2013-11-26 06:11:29]

lgtm

https://codereview.chromium.org/76363006/diff/80001/content/renderer/webcrypt...
File content/renderer/webcrypto/webcrypto_impl_nss.cc (right):

https://codereview.chromium.org/76363006/diff/80001/content/renderer/webcrypt...
content/renderer/webcrypto/webcrypto_impl_nss.cc:684: const SEC_ASN1Template
RsaPublicKeyTemplate[] = {
style: RsaPublicKeyTemplate -> rsa_public_key_template

https://codereview.chromium.org/76363006/diff/80001/content/renderer/webcrypt...
content/renderer/webcrypto/webcrypto_impl_nss.cc:686: {SEC_ASN1_INTEGER,
offsetof(RsaPublicKeyData, modulus), },
@rsleevi: Do you know if leading zeros is an issue for NSS?

[padolph, 2013-11-27 04:02:12]

https://codereview.chromium.org/76363006/diff/80001/content/renderer/webcrypt...
File content/renderer/webcrypto/webcrypto_impl_nss.cc (right):

https://codereview.chromium.org/76363006/diff/80001/content/renderer/webcrypt...
content/renderer/webcrypto/webcrypto_impl_nss.cc:684: const SEC_ASN1Template
RsaPublicKeyTemplate[] = {
On 2013/11/26 06:11:29, eroman wrote:
> style: RsaPublicKeyTemplate -> rsa_public_key_template

Done.

[commit-bot: I haz the power, 2013-12-04 20:20:24]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/padolph@netflix.com/76363006/160001

[commit-bot: I haz the power, 2013-12-04 21:18:44]

Retried try job too often on android_clang_dbg for step(s) slave_steps
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_cl...

[commit-bot: I haz the power, 2013-12-04 21:26:18]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/padolph@netflix.com/76363006/180001

[commit-bot: I haz the power, 2013-12-04 23:54:34]

Change committed as 238821