[webcrypto] Add JWK RSA public key export for NSS.

[webcrypto] Add JWK RSA public key export for NSS.

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

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

[eroman, 2014-03-21 02:30:51]

BTW I am gone tomorrow, so may not do the next review pass until Monday.

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

https://codereview.chromium.org/205913002/diff/60001/content/child/webcrypto/...
content/child/webcrypto/jwk.cc:312: const base::StringPiece
modulus_str(static_cast<const char*>(modulus.data()),
There are now 3 places in the code which do this in the file. I suggest either
adding a helper:

base::StringPiece ToStringPiece(const blink::WebArrayBuffer&);

Or adding an overload for Base64EncodeUrlSafe

https://codereview.chromium.org/205913002/diff/60001/content/child/webcrypto/...
content/child/webcrypto/jwk.cc:316: DCHECK(!public_exponent.isNull());
my preference is to put all input dchecks at the top of the function

https://codereview.chromium.org/205913002/diff/60001/content/child/webcrypto/...
content/child/webcrypto/jwk.cc:406: case blink::WebCryptoAlgorithmIdRsaOaep:
I don't believe this is correct.

In WebCrypto RSA-OAEP must be imported with a hash, so its algorithm params type
will be TypeRsaHashed and not TypeRsa.

https://codereview.chromium.org/205913002/diff/60001/content/child/webcrypto/...
File content/child/webcrypto/platform_crypto.h (right):

https://codereview.chromium.org/205913002/diff/60001/content/child/webcrypto/...
content/child/webcrypto/platform_crypto.h:182: blink::WebArrayBuffer* modulus,
Please use a "std::vector<uint8>*" as the output type instead.

Since these buffers don't need to be sent back to Blink, it is both more
convenient and efficient to make them be std::vectors<>.

(WebArrayBuffer is shared by the javascript engine and has extra issues).

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

https://codereview.chromium.org/205913002/diff/60001/content/child/webcrypto/...
content/child/webcrypto/platform_crypto_nss.cc:708: DCHECK(modulus_item.data);
Do these need to be proper returns?

[padolph, 2014-03-24 04:24:06]

https://codereview.chromium.org/205913002/diff/60001/content/child/webcrypto/...
File content/child/webcrypto/platform_crypto.h (right):

https://codereview.chromium.org/205913002/diff/60001/content/child/webcrypto/...
content/child/webcrypto/platform_crypto.h:182: blink::WebArrayBuffer* modulus,
On 2014/03/21 02:30:51, eroman wrote:
> Please use a "std::vector<uint8>*" as the output type instead.
> 
> Since these buffers don't need to be sent back to Blink, it is both more
> convenient and efficient to make them be std::vectors<>.
> 
> (WebArrayBuffer is shared by the javascript engine and has extra issues).

Done.

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

https://codereview.chromium.org/205913002/diff/60001/content/child/webcrypto/...
content/child/webcrypto/platform_crypto_nss.cc:708: DCHECK(modulus_item.data);
On 2014/03/21 02:30:51, eroman wrote:
> Do these need to be proper returns?

Yes.

[padolph, 2014-03-24 04:28:28]

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

https://codereview.chromium.org/205913002/diff/60001/content/child/webcrypto/...
content/child/webcrypto/jwk.cc:312: const base::StringPiece
modulus_str(static_cast<const char*>(modulus.data()),
On 2014/03/21 02:30:51, eroman wrote:
> There are now 3 places in the code which do this in the file. I suggest either
> adding a helper:
> 
> base::StringPiece ToStringPiece(const blink::WebArrayBuffer&);
> 
> Or adding an overload for Base64EncodeUrlSafe

Done.

https://codereview.chromium.org/205913002/diff/60001/content/child/webcrypto/...
content/child/webcrypto/jwk.cc:316: DCHECK(!public_exponent.isNull());
On 2014/03/21 02:30:51, eroman wrote:
> my preference is to put all input dchecks at the top of the function

Done.

https://codereview.chromium.org/205913002/diff/60001/content/child/webcrypto/...
content/child/webcrypto/jwk.cc:406: case blink::WebCryptoAlgorithmIdRsaOaep:
On 2014/03/21 02:30:51, eroman wrote:
> I don't believe this is correct.
> 
> In WebCrypto RSA-OAEP must be imported with a hash, so its algorithm params
type
> will be TypeRsaHashed and not TypeRsa.

Done.

[eroman, 2014-03-24 22:05:51]

patchset 7 LGTM after these comments:

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

https://codereview.chromium.org/205913002/diff/120001/content/child/webcrypto...
content/child/webcrypto/jwk.cc:326: unsigned int raw_key_length_bytes,
Please delete this parameter (makes the caller uglier).

https://codereview.chromium.org/205913002/diff/120001/content/child/webcrypto...
content/child/webcrypto/jwk.cc:331: switch (raw_key_length_bytes) {
Instead we can use:

algorithm.aesParams()->lengthBits()

https://codereview.chromium.org/205913002/diff/120001/content/child/webcrypto...
content/child/webcrypto/jwk.cc:783: unsigned int key_length_bytes = 0;
Delete this variable WriteAlg() no longer needs it, and it is weird to only
write it for the sym keys.

https://codereview.chromium.org/205913002/diff/120001/content/child/webcrypto...
content/child/webcrypto/jwk.cc:802: std::vector<uint8> modulus, public_exponent;
Please put these on two separate lines:

std::vector<uint8> modulus;
std::vector<uint8> public_exponent;

Although more verbose, I am pretty sure that is in the style guide somewhere.

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

https://codereview.chromium.org/205913002/diff/120001/content/child/webcrypto...
content/child/webcrypto/platform_crypto_nss.cc:708: if (!modulus_item.data ||
!modulus_item.len)
Consider extracting this to a function:

CopySECItemToVector(const SECItem& item, std::vector<uint8>* out);

Then you have:

CopySECItemToVector(key->key()->u.rsa.publicExponent, public_exponent);
CopySECItemToVector(key->key()->u.rsa.modulus, modulus);

if (public_exponent->empty() || modulus->empty())
  return Status::ErrorUnexpected();

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

https://codereview.chromium.org/205913002/diff/120001/content/child/webcrypto...
content/child/webcrypto/shared_crypto_unittest.cc:543: // Verifies that the JSON
in the input ArrayBuffer contains the provided
This comment needs to be updated as well (no longer an input ArrayBuffer)

https://codereview.chromium.org/205913002/diff/120001/content/child/webcrypto...
content/child/webcrypto/shared_crypto_unittest.cc:644: // TODO(padolph):
LowerCaseEqualsASCII() does not work for above!
Not sure I follow, can you explain this comment? Should it be doing a
case-insensitive comparison?

[padolph, 2014-03-25 01:10:28]

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

https://codereview.chromium.org/205913002/diff/120001/content/child/webcrypto...
content/child/webcrypto/jwk.cc:326: unsigned int raw_key_length_bytes,
On 2014/03/24 22:05:51, eroman wrote:
> Please delete this parameter (makes the caller uglier).

Done.

https://codereview.chromium.org/205913002/diff/120001/content/child/webcrypto...
content/child/webcrypto/jwk.cc:331: switch (raw_key_length_bytes) {
On 2014/03/24 22:05:51, eroman wrote:
> Instead we can use:
> 
> algorithm.aesParams()->lengthBits()

Done.

https://codereview.chromium.org/205913002/diff/120001/content/child/webcrypto...
content/child/webcrypto/jwk.cc:783: unsigned int key_length_bytes = 0;
On 2014/03/24 22:05:51, eroman wrote:
> Delete this variable WriteAlg() no longer needs it, and it is weird to only
> write it for the sym keys.

Done.

https://codereview.chromium.org/205913002/diff/120001/content/child/webcrypto...
content/child/webcrypto/jwk.cc:802: std::vector<uint8> modulus, public_exponent;
On 2014/03/24 22:05:51, eroman wrote:
> Please put these on two separate lines:
> 
> std::vector<uint8> modulus;
> std::vector<uint8> public_exponent;
> 
> Although more verbose, I am pretty sure that is in the style guide somewhere.

Done.

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

https://codereview.chromium.org/205913002/diff/120001/content/child/webcrypto...
content/child/webcrypto/platform_crypto_nss.cc:708: if (!modulus_item.data ||
!modulus_item.len)
On 2014/03/24 22:05:51, eroman wrote:
> Consider extracting this to a function:
> 
> CopySECItemToVector(const SECItem& item, std::vector<uint8>* out);
> 
> Then you have:
> 
> CopySECItemToVector(key->key()->u.rsa.publicExponent, public_exponent);
> CopySECItemToVector(key->key()->u.rsa.modulus, modulus);
> 
> if (public_exponent->empty() || modulus->empty())
>   return Status::ErrorUnexpected();

Done.

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

https://codereview.chromium.org/205913002/diff/120001/content/child/webcrypto...
content/child/webcrypto/shared_crypto_unittest.cc:543: // Verifies that the JSON
in the input ArrayBuffer contains the provided
On 2014/03/24 22:05:51, eroman wrote:
> This comment needs to be updated as well (no longer an input ArrayBuffer)

Done.

https://codereview.chromium.org/205913002/diff/120001/content/child/webcrypto...
content/child/webcrypto/shared_crypto_unittest.cc:644: // TODO(padolph):
LowerCaseEqualsASCII() does not work for above!
On 2014/03/24 22:05:51, eroman wrote:
> Not sure I follow, can you explain this comment? Should it be doing a
> case-insensitive comparison?

LowerCaseEqual() did not behave properly in this code. Using clearly equal
strings, it did not return the correct value. This is either a bug or some kind
of size limit in the implementation of LowerCaseEqual(), since I've seen it work
correctly with shorter strings.

The new code as it stands works, but is making assumptions of the case of string
returned by base::HexEncode() and the input string, so is less than ideal.

What is your opinion on the right course of action? I did not think it was
worthwhile or appropriate for me to debug the general Chromium string stuff.

[eroman, 2014-03-25 01:21:11]

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

https://codereview.chromium.org/205913002/diff/120001/content/child/webcrypto...
content/child/webcrypto/shared_crypto_unittest.cc:644: // TODO(padolph):
LowerCaseEqualsASCII() does not work for above!
On 2014/03/25 01:10:28, padolph wrote:
> On 2014/03/24 22:05:51, eroman wrote:
> > Not sure I follow, can you explain this comment? Should it be doing a
> > case-insensitive comparison?
> 
> LowerCaseEqual() did not behave properly in this code. Using clearly equal
> strings, it did not return the correct value. This is either a bug or some
kind
> of size limit in the implementation of LowerCaseEqual(), since I've seen it
work
> correctly with shorter strings.
> 
> The new code as it stands works, but is making assumptions of the case of
string
> returned by base::HexEncode() and the input string, so is less than ideal.
> 
> What is your opinion on the right course of action? I did not think it was
> worthwhile or appropriate for me to debug the general Chromium string stuff.

I would be very surprised if LowerCaseEqual() was not working. Can you send me
the inputs/outputs?

Or what modifications I can make to reproduce locally.

[eroman, 2014-03-25 01:22:45]

patchset 8 LGTM

[padolph, 2014-03-25 16:19:55]

The CQ bit was checked by padolph@netflix.com

[padolph, 2014-03-25 16:19:59]

The CQ bit was unchecked by padolph@netflix.com

[padolph, 2014-03-25 16:44:52]

The CQ bit was checked by padolph@netflix.com

[commit-bot: I haz the power, 2014-03-25 16:45:03]

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

[commit-bot: I haz the power, 2014-03-25 17:13:24]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-03-25 17:13:25]

Try jobs failed on following builders:
  tryserver.chromium on mac_chromium_compile_dbg

[eroman, 2014-03-25 17:20:46]

The issue is that either CreateRsaSsaImportAlgorithm() has to be marked as
CONTENT_EXPORT in webcrypto_util.h, or use the already exported
CreateRsaHashedImportAlgorithm()

[padolph, 2014-03-25 17:35:18]

On 2014/03/25 17:20:46, eroman wrote:
> The issue is that either CreateRsaSsaImportAlgorithm() has to be marked as
> CONTENT_EXPORT in webcrypto_util.h, or use the already exported
> CreateRsaHashedImportAlgorithm()

I chose the latter so as not to conflict with your TODO to move
CreateRsaSsaImportAlgorithm() to jwk.cc.

[padolph, 2014-03-25 18:12:14]

On 2014/03/25 17:20:46, eroman wrote:
> The issue is that either CreateRsaSsaImportAlgorithm() has to be marked as
> CONTENT_EXPORT in webcrypto_util.h, or use the already exported
> CreateRsaHashedImportAlgorithm()

I created a test case that demonstrates the apparent failure:
https://codereview.chromium.org/206613004/

[padolph, 2014-03-25 18:15:34]

On 2014/03/25 01:21:11, eroman wrote:
>
https://codereview.chromium.org/205913002/diff/120001/content/child/webcrypto...
> File content/child/webcrypto/shared_crypto_unittest.cc (right):
> 
>
https://codereview.chromium.org/205913002/diff/120001/content/child/webcrypto...
> content/child/webcrypto/shared_crypto_unittest.cc:644: // TODO(padolph):
> LowerCaseEqualsASCII() does not work for above!
> On 2014/03/25 01:10:28, padolph wrote:
> > On 2014/03/24 22:05:51, eroman wrote:
> > > Not sure I follow, can you explain this comment? Should it be doing a
> > > case-insensitive comparison?
> > 
> > LowerCaseEqual() did not behave properly in this code. Using clearly equal
> > strings, it did not return the correct value. This is either a bug or some
> kind
> > of size limit in the implementation of LowerCaseEqual(), since I've seen it
> work
> > correctly with shorter strings.
> > 
> > The new code as it stands works, but is making assumptions of the case of
> string
> > returned by base::HexEncode() and the input string, so is less than ideal.
> > 
> > What is your opinion on the right course of action? I did not think it was
> > worthwhile or appropriate for me to debug the general Chromium string stuff.
> 
> I would be very surprised if LowerCaseEqual() was not working. Can you send me
> the inputs/outputs?
> 
> Or what modifications I can make to reproduce locally.

I created a test case that demonstrates the apparent failure:
https://codereview.chromium.org/206613004/

[eroman, 2014-03-25 19:22:33]

lgtm

patchset 11 LGTM

[commit-bot: I haz the power, 2014-03-25 19:24:09]

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

[commit-bot: I haz the power, 2014-03-25 21:35:06]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-03-25 21:35:07]

Retried try job too often on win_rel for step(s) content_browsertests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[eroman, 2014-03-25 21:37:35]

The CQ bit was checked by eroman@chromium.org

[commit-bot: I haz the power, 2014-03-25 21:38:24]

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

[commit-bot: I haz the power, 2014-03-26 01:39:35]

Change committed as 259432