[webcrypto] Add JWK import/export of RSA private keys (NSS).

[webcrypto] Add JWK import/export of RSA private keys (NSS).

BUG=373543, 373542, 245025
R=rsleevi@chromium.org

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

[eroman, 2014-05-17 03:33:02]

Updated this changelist to include JWK private key import as well (testing them
in combination is easier).

[Ryan Sleevi, 2014-05-19 00:10:34]

Glad it turned out to be fairly easy. A few comments here.

https://codereview.chromium.org/287133004/diff/30001/content/child/webcrypto/...
File content/child/webcrypto/jwk.cc (right):

https://codereview.chromium.org/287133004/diff/30001/content/child/webcrypto/...
content/child/webcrypto/jwk.cc:473: if (!*property_exists || status.IsError())
nit: Check status.IsError() before doing *property_exists

I would expect that if status.IsError() is true, GetOptionalJwkString may not
have updated property_exists (under the normal Chromium idiom of not mutating
return values on error). Now, this may not be true - you may always be updating
property_exists, but that would force me as a reader to check whether or not
that precondition is true (something i'll likely forget), whereas the logical
re-ordering provides some degree of reasonable expectation.

https://codereview.chromium.org/287133004/diff/30001/content/child/webcrypto/...
content/child/webcrypto/jwk.cc:569: // SHOULD include them.
Doesn't matter what JWA says. The WebCrypto spec is explicit that they MUST be
included.

https://codereview.chromium.org/287133004/diff/30001/content/child/webcrypto/...
content/child/webcrypto/jwk.cc:700: // TODO(eroman): Split this up into smaller
functions.
I would rather see RSA Private key import split out in this CL

Lines 836-912 seem like they can be fully encapsulated stand-alone.

https://codereview.chromium.org/287133004/diff/30001/content/child/webcrypto/...
File content/child/webcrypto/platform_crypto_nss.cc (right):

https://codereview.chromium.org/287133004/diff/30001/content/child/webcrypto/...
content/child/webcrypto/platform_crypto_nss.cc:631: #if defined(USE_NSS)
I would prefer a runtime check, rather than the hardcoding.

https://codereview.chromium.org/287133004/diff/30001/content/child/webcrypto/...
content/child/webcrypto/platform_crypto_nss.cc:1552: if (has_data) {
Two thoughts:
1) Short-circuit the error control:
if (!has_data)
  return

2) This doesn't seem to disambiguate when the property is absent, and when the
property is present and (empty or invalid)

Rather than having to track the has_data, I'd say just use if data.byte_length()
!= 0, and remove the bool entirely.

https://codereview.chromium.org/287133004/diff/30001/content/child/webcrypto/...
content/child/webcrypto/platform_crypto_nss.cc:1613: PK11_TypeGeneric,
key_object.get(), CKA_ID, &object_id) != SECSuccess)
Should document that this isn't guaranteed to be set by PKCS#11, but *is*
guaranteed by NSS's softoken -
https://code.google.com/p/chromium/codesearch#chromium-os/src/platform/login_...

https://codereview.chromium.org/287133004/diff/30001/content/child/webcrypto/...
File content/child/webcrypto/shared_crypto.h (right):

https://codereview.chromium.org/287133004/diff/30001/content/child/webcrypto/...
content/child/webcrypto/shared_crypto.h:175: platform::PrivateKey** out);
Feels like a layering violation. I'm not sure why you introduced these?

[eroman, 2014-05-19 18:51:09]

https://codereview.chromium.org/287133004/diff/30001/content/child/webcrypto/...
File content/child/webcrypto/jwk.cc (right):

https://codereview.chromium.org/287133004/diff/30001/content/child/webcrypto/...
content/child/webcrypto/jwk.cc:473: if (!*property_exists || status.IsError())
On 2014/05/19 00:10:34, Ryan Sleevi wrote:
> nit: Check status.IsError() before doing *property_exists
> 
> I would expect that if status.IsError() is true, GetOptionalJwkString may not
> have updated property_exists (under the normal Chromium idiom of not mutating
> return values on error). Now, this may not be true - you may always be
updating
> property_exists, but that would force me as a reader to check whether or not
> that precondition is true (something i'll likely forget), whereas the logical
> re-ordering provides some degree of reasonable expectation.

Good point! Done.

Reading the boolean first could be an uninitialized memory read as the API is
currently defined (although not current implementation).

https://codereview.chromium.org/287133004/diff/30001/content/child/webcrypto/...
content/child/webcrypto/jwk.cc:569: // SHOULD include them.
On 2014/05/19 00:10:34, Ryan Sleevi wrote:
> Doesn't matter what JWA says. The WebCrypto spec is explicit that they MUST be
> included.

Done, updated the comment.

https://codereview.chromium.org/287133004/diff/30001/content/child/webcrypto/...
content/child/webcrypto/jwk.cc:700: // TODO(eroman): Split this up into smaller
functions.
On 2014/05/19 00:10:34, Ryan Sleevi wrote:
> I would rather see RSA Private key import split out in this CL
> 
> Lines 836-912 seem like they can be fully encapsulated stand-alone.

Done.

https://codereview.chromium.org/287133004/diff/30001/content/child/webcrypto/...
File content/child/webcrypto/platform_crypto_nss.cc (right):

https://codereview.chromium.org/287133004/diff/30001/content/child/webcrypto/...
content/child/webcrypto/platform_crypto_nss.cc:631: #if defined(USE_NSS)
On 2014/05/19 00:10:34, Ryan Sleevi wrote:
> I would prefer a runtime check, rather than the hardcoding.

Can you explain a bit more?

This ifdef is guarding code that is not compiled into the binary when using
PK11_ExportDERPrivateKeyInfo(). Are you proposing always compiling this in
instead?

(I had to loosen this ifdef since I am re-using the RSAPrivateKey struct and
helper functions.)

https://codereview.chromium.org/287133004/diff/30001/content/child/webcrypto/...
content/child/webcrypto/platform_crypto_nss.cc:1552: if (has_data) {
On 2014/05/19 00:10:34, Ryan Sleevi wrote:
> Two thoughts:
> 1) Short-circuit the error control:
> if (!has_data)
>   return
> 
> 2) This doesn't seem to disambiguate when the property is absent, and when the
> property is present and (empty or invalid)
> 
> Rather than having to track the has_data, I'd say just use if
data.byte_length()
> != 0, and remove the bool entirely.

I considered this initially, but wasn't sure about the correctness implications.
Certainly if I use the data being empty as a check for optional it simplifies
the ImportRsaPrivateKey() signature since I don't need to pass in all the
boolean values like "has_prime".

Several possibilities, which do you think are correct:

  * Check for specified but empty properties in jwk.cc and fail rather than
passing empty data to ImportRsaPrivateKey(). The failure in this case, should it
be OperationError or DataError?

  * Treat empty input the same as unspecified. There is no evidence of this in
the spec so this is likely wrong.

In terms of whether incorrect data leads to an error or not, it doesn't appear
that NSS validates it (well at least, errors if one is wrong). I presume NSS is
checking the inputs in some order and if it gets stuff working than it proceeds
happily. I was able to do imports with some of the properties being bogus. I
also was unable to do an import with just "n", "e", "d" -- I needed at least one
of "p" and "q" to succeed, is that expected?

https://codereview.chromium.org/287133004/diff/30001/content/child/webcrypto/...
content/child/webcrypto/platform_crypto_nss.cc:1613: PK11_TypeGeneric,
key_object.get(), CKA_ID, &object_id) != SECSuccess)
On 2014/05/19 00:10:34, Ryan Sleevi wrote:
> Should document that this isn't guaranteed to be set by PKCS#11, but *is*
> guaranteed by NSS's softoken -
>
https://code.google.com/p/chromium/codesearch#chromium-os/src/platform/login_...

Done, added comment.

https://codereview.chromium.org/287133004/diff/30001/content/child/webcrypto/...
File content/child/webcrypto/shared_crypto.h (right):

https://codereview.chromium.org/287133004/diff/30001/content/child/webcrypto/...
content/child/webcrypto/shared_crypto.h:175: platform::PrivateKey** out);
On 2014/05/19 00:10:34, Ryan Sleevi wrote:
> Feels like a layering violation. I'm not sure why you introduced these?

I exposed these for re-use in jwk.cc.

jwk.cc was already using ToPlatformPublicKey() via duplication. This CL now
required ToPlatformPrivateKey() as well. Rather than duplicate that as well, I
decided to expose them.

I agree shared_crypto.h is not the best place for these, however the other files
are also unsuited.

platform_crypto.h would be a reasonable place to expose the declaration, however
the issue is where to put the definition (there is no platform_crypto.cc, only
platform_crypto_nss.cc and platform_crypto_openssl.cc).

Suggestions? I could declare in platform_crypto.h and then define in
shared_crypto.cc but that feels strange too.

[Ryan Sleevi, 2014-05-19 18:58:34]

https://codereview.chromium.org/287133004/diff/30001/content/child/webcrypto/...
File content/child/webcrypto/platform_crypto_nss.cc (right):

https://codereview.chromium.org/287133004/diff/30001/content/child/webcrypto/...
content/child/webcrypto/platform_crypto_nss.cc:1552: if (has_data) {
On 2014/05/19 18:51:09, eroman wrote:
> On 2014/05/19 00:10:34, Ryan Sleevi wrote:
> > Two thoughts:
> > 1) Short-circuit the error control:
> > if (!has_data)
> >   return
> > 
> > 2) This doesn't seem to disambiguate when the property is absent, and when
the
> > property is present and (empty or invalid)
> > 
> > Rather than having to track the has_data, I'd say just use if
> data.byte_length()
> > != 0, and remove the bool entirely.
> 
> I considered this initially, but wasn't sure about the correctness
implications.
> Certainly if I use the data being empty as a check for optional it simplifies
> the ImportRsaPrivateKey() signature since I don't need to pass in all the
> boolean values like "has_prime".
> 
> Several possibilities, which do you think are correct:
> 
>   * Check for specified but empty properties in jwk.cc and fail rather than
> passing empty data to ImportRsaPrivateKey(). The failure in this case, should
it
> be OperationError or DataError?

This is not correct. It's a valid JWK to have these as empty properties, at
least according to the format. (That is, JWK dictates no consistency proofs on
the properties, nor does WebCrypto)

> 
>   * Treat empty input the same as unspecified. There is no evidence of this in
> the spec so this is likely wrong.

Why make a distinction between the two at all? The spec doesn't say anything
about special handling if they're not present.

> 
> In terms of whether incorrect data leads to an error or not, it doesn't appear
> that NSS validates it (well at least, errors if one is wrong). I presume NSS
is
> checking the inputs in some order and if it gets stuff working than it
proceeds
> happily. I was able to do imports with some of the properties being bogus. I
> also was unable to do an import with just "n", "e", "d" -- I needed at least
one
> of "p" and "q" to succeed, is that expected?

See http://mxr.mozilla.org/nss/source/lib/freebl/rsa.c#330

rsa_get_primes_from_exponents, which calculates the various properties, has the
requirement of

e, d, and either p or n, contingent upon hasModulus

So e, n, d "should" be sufficient for the computation of p/q.

This function is called by
http://mxr.mozilla.org/nss/source/lib/freebl/rsa.c#604

Which is documented as requiring either publicExponent or privateExponent

And two more of the following 5 parameters

So publicExponent (e), privateExponent (d), and modulus (n) should be
sufficient.

I suspect we'd need to step through a debugger on that function to find out why
it might be failing to populate the data.

[Ryan Sleevi, 2014-05-19 18:59:26]

https://codereview.chromium.org/287133004/diff/30001/content/child/webcrypto/...
File content/child/webcrypto/platform_crypto_nss.cc (right):

https://codereview.chromium.org/287133004/diff/30001/content/child/webcrypto/...
content/child/webcrypto/platform_crypto_nss.cc:631: #if defined(USE_NSS)
On 2014/05/19 18:51:09, eroman wrote:
> On 2014/05/19 00:10:34, Ryan Sleevi wrote:
> > I would prefer a runtime check, rather than the hardcoding.
> 
> Can you explain a bit more?
> 
> This ifdef is guarding code that is not compiled into the binary when using
> PK11_ExportDERPrivateKeyInfo(). Are you proposing always compiling this in
> instead?
> 
> (I had to loosen this ifdef since I am re-using the RSAPrivateKey struct and
> helper functions.)

Ah, right, missed that bit. No, this is fine.

[eroman, 2014-05-19 19:29:49]

PTAL: I removed the distinction between empty optional parameters and
unspecified ones.

[Ryan Sleevi, 2014-05-19 20:48:44]

LGTM, mod nits.

https://codereview.chromium.org/287133004/diff/70001/content/child/webcrypto/...
File content/child/webcrypto/jwk.cc (right):

https://codereview.chromium.org/287133004/diff/70001/content/child/webcrypto/...
content/child/webcrypto/jwk.cc:975: WriteExt(key.extractable(), &jwk_dict);
Huh. No clue why the spec requires this. This is silly, for the reason on line
929. But OK!

https://codereview.chromium.org/287133004/diff/70001/content/child/webcrypto/...
File content/child/webcrypto/shared_crypto_unittest.cc (right):

https://codereview.chromium.org/287133004/diff/70001/content/child/webcrypto/...
content/child/webcrypto/shared_crypto_unittest.cc:2102: // qi). The only
optional parameter included is "p" (without this import fails).
1: Re-align this comment
2: Indicate via a clear TODO why P is required (feel free to file a bug against
me on this)

https://codereview.chromium.org/287133004/diff/70001/content/child/webcrypto/...
content/child/webcrypto/shared_crypto_unittest.cc:2149: // instance what if p
was correct, but q was wrong?)
Update to point to https://www.w3.org/Bugs/Public/show_bug.cgi?id=25815

https://codereview.chromium.org/287133004/diff/70001/content/child/webcrypto/...
content/child/webcrypto/shared_crypto_unittest.cc:2184: // is handled
differently.
Update to point to https://www.w3.org/Bugs/Public/show_bug.cgi?id=25815

https://codereview.chromium.org/287133004/diff/90001/content/child/webcrypto/...
File content/child/webcrypto/jwk.cc (right):

https://codereview.chromium.org/287133004/diff/90001/content/child/webcrypto/...
content/child/webcrypto/jwk.cc:775: CryptoData(jwk_q_value),   // prime2
Two spaces, not three.

git cl format updated to your widest (which on 773-777 were three), but that was
wrong, because it should have been two.

[eroman, 2014-05-19 21:04:54]

https://codereview.chromium.org/287133004/diff/70001/content/child/webcrypto/...
File content/child/webcrypto/shared_crypto_unittest.cc (right):

https://codereview.chromium.org/287133004/diff/70001/content/child/webcrypto/...
content/child/webcrypto/shared_crypto_unittest.cc:2102: // qi). The only
optional parameter included is "p" (without this import fails).
On 2014/05/19 20:48:45, Ryan Sleevi wrote:
> 1: Re-align this comment
> 2: Indicate via a clear TODO why P is required (feel free to file a bug
against
> me on this)

Done.

https://codereview.chromium.org/287133004/diff/70001/content/child/webcrypto/...
content/child/webcrypto/shared_crypto_unittest.cc:2149: // instance what if p
was correct, but q was wrong?)
On 2014/05/19 20:48:45, Ryan Sleevi wrote:
> Update to point to https://www.w3.org/Bugs/Public/show_bug.cgi?id=25815

Done.

https://codereview.chromium.org/287133004/diff/70001/content/child/webcrypto/...
content/child/webcrypto/shared_crypto_unittest.cc:2184: // is handled
differently.
On 2014/05/19 20:48:45, Ryan Sleevi wrote:
> Update to point to https://www.w3.org/Bugs/Public/show_bug.cgi?id=25815

Done.

https://codereview.chromium.org/287133004/diff/90001/content/child/webcrypto/...
File content/child/webcrypto/jwk.cc (right):

https://codereview.chromium.org/287133004/diff/90001/content/child/webcrypto/...
content/child/webcrypto/jwk.cc:775: CryptoData(jwk_q_value),   // prime2
On 2014/05/19 20:48:45, Ryan Sleevi wrote:
> Two spaces, not three.
> 
> git cl format updated to your widest (which on 773-777 were three), but that
was
> wrong, because it should have been two.

git cl format insists on adding 3 spaces here, so that it lines up with those
below (line 776 and on).

My preference is to let git-cl format do its thing, and follow up with a git-cl
format bug if you think this is the wrong behavior.

[eroman, 2014-05-19 21:54:07]

The CQ bit was checked by eroman@chromium.org

[commit-bot: I haz the power, 2014-05-19 21:54:45]

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

[commit-bot: I haz the power, 2014-05-20 02:17:44]

FYI, CQ is re-trying this CL (attempt #1).
Please consider checking whether the failures are real,
and report flakes to chrome-troopers@google.com.
The failing builders are:
  android_dbg_triggered_tests on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_dbg_triggered...)
  win_chromium_compile_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_compile_...)
  win_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_rel/buil...)

[eroman, 2014-05-20 03:03:20]

The CQ bit was unchecked by eroman@chromium.org

[eroman, 2014-05-20 04:07:24]

I have updated to remove support for partial inclusion of p, q, dp, dq, qi.

It now more closely matches the spec as an all or nothing. If only a partial set
of p, q, dp, dq, qi, then it fails with a DataError.

[eroman, 2014-05-20 20:27:57]

The CQ bit was checked by eroman@chromium.org

[commit-bot: I haz the power, 2014-05-20 20:28:55]

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

[eroman, 2014-05-20 20:54:50]

The CQ bit was unchecked by eroman@chromium.org

[eroman, 2014-05-21 01:43:48]

The CQ bit was checked by eroman@chromium.org

[commit-bot: I haz the power, 2014-05-21 01:46:42]

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

[eroman, 2014-05-21 15:26:58]

Committed patchset #9 manually as r271901 (presubmit successful).