[webcrypto] Implement structured clone of keys (chromium-side).

[webcrypto] Implement structured clone of keys (chromium-side).

The serialized format saves keys as:
  * spki for public RSA keys
  * pkcs8 for private RSA keys
  * raw for AES and HMAC keys

The testing for this is done on the blink side by (see https://codereview.chromium.org/195543002/)

[2] PKCS8 import/export is not yet implemented. I will re-visit the serialization of private keys in a follow-up change.

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

[Ryan Sleevi, 2014-03-12 22:56:55]

https://codereview.chromium.org/196513002/diff/20001/content/child/webcrypto/...
File content/child/webcrypto/webcrypto_util.cc (right):

https://codereview.chromium.org/196513002/diff/20001/content/child/webcrypto/...
content/child/webcrypto/webcrypto_util.cc:262: DCHECK(IsHashAlgorithm(hash_id));
Does Blink also make this check?

If so, I agree that DCHECK is fine. Otherwise, I'm a little nervous about only
DCHECKing these conditions rather than explicitly hadling.

[eroman, 2014-03-12 23:10:10]

https://codereview.chromium.org/196513002/diff/20001/content/child/webcrypto/...
File content/child/webcrypto/webcrypto_util.cc (right):

https://codereview.chromium.org/196513002/diff/20001/content/child/webcrypto/...
content/child/webcrypto/webcrypto_util.cc:262: DCHECK(IsHashAlgorithm(hash_id));
On 2014/03/12 22:56:55, Ryan Sleevi wrote:
> Does Blink also make this check?
> 
> If so, I agree that DCHECK is fine. Otherwise, I'm a little nervous about only
> DCHECKing these conditions rather than explicitly hadling.

Yes. For the codepath I added that calls CreateRsaHashedImportAlgorithms() it is
guaranteed that the algorithm id is a hash.

I plan to clean this up in the future by using something like
WebCryptoHashAlgorithmId to describe hashes between blink <--> chromium rather
than a WebCryptoAlgorithm. Since it already validates that things are hashes
when normalizing algorithms.

[Ryan Sleevi, 2014-03-12 23:18:41]

https://codereview.chromium.org/196513002/diff/20001/content/child/webcrypto/...
File content/child/webcrypto/shared_crypto.cc (right):

https://codereview.chromium.org/196513002/diff/20001/content/child/webcrypto/...
content/child/webcrypto/shared_crypto.cc:664: reinterpret_cast<unsigned
char*>(buffer.data()), buffer.byteLength());
This seems to follow a double-allocation strategy

That is, first a new (temporary) blink::WebVector must be initialized, then the
copy constructor must be invoked, which then copies from the temporary

So in the present form, you have 1 allocation + 2 copies
  1 allocation to create the WebVector
  1 copy to create the WebVector from WebArrayBuffer
  1 copy to assign the WebVector from the (temporary) WebVector

You could use
  blink::WebVector<unsigned char>(reinterpret_cast<unsigned
char*>(buffer.data()), buffer.byteLength()).swap(*data);
which forces a new allocation, but doesn't force a copy.
  1 allocation to create the WebVector
  1 copy to create the WebVector from WebArrayBuffer


Alternatively, you could use
  data->assign(buffer.data(), buffer.byteLength());
which forces a copy, but doesn't force a new allocation.
  1 copy to assign the WebVector from WebArrayBuffer

[eroman, 2014-03-13 00:32:32]

https://codereview.chromium.org/196513002/diff/20001/content/child/webcrypto/...
File content/child/webcrypto/shared_crypto.cc (right):

https://codereview.chromium.org/196513002/diff/20001/content/child/webcrypto/...
content/child/webcrypto/shared_crypto.cc:664: reinterpret_cast<unsigned
char*>(buffer.data()), buffer.byteLength());
On 2014/03/12 23:18:41, Ryan Sleevi wrote:
> This seems to follow a double-allocation strategy
> 
> That is, first a new (temporary) blink::WebVector must be initialized, then
the
> copy constructor must be invoked, which then copies from the temporary
> 
> So in the present form, you have 1 allocation + 2 copies
>   1 allocation to create the WebVector
>   1 copy to create the WebVector from WebArrayBuffer
>   1 copy to assign the WebVector from the (temporary) WebVector
> 
> You could use
>   blink::WebVector<unsigned char>(reinterpret_cast<unsigned
> char*>(buffer.data()), buffer.byteLength()).swap(*data);
> which forces a new allocation, but doesn't force a copy.
>   1 allocation to create the WebVector
>   1 copy to create the WebVector from WebArrayBuffer
> 
> 
> Alternatively, you could use
>   data->assign(buffer.data(), buffer.byteLength());
> which forces a copy, but doesn't force a new allocation.
>   1 copy to assign the WebVector from WebArrayBuffer

Changed to data->assign()

Oops, not sure what that silliness was about..

[eroman, 2014-03-13 01:16:04]

Also it occurs to me that there is at least one situation where using
"importKey()" for deserialization is a little bit different:

==> On linux importing an AES-GCM will fail if system NSS version doesn't
support it.

So it is possible to serialize a key, then downgrade the system NSS, and now
deserializing it will result in a deserialization error.

Instead, it should probably succeed deserialization, but then the next operation
using that key should fail.

I reckon I'll address this in a follow-up, since this will be natural behavior
once operations are moved to a worker thread.

[Ryan Sleevi, 2014-03-14 20:54:11]

LGTM, mod nit.

https://codereview.chromium.org/196513002/diff/40001/content/child/webcrypto/...
File content/child/webcrypto/webcrypto_util.cc (right):

https://codereview.chromium.org/196513002/diff/40001/content/child/webcrypto/...
content/child/webcrypto/webcrypto_util.cc:264: id ==
blink::WebCryptoAlgorithmIdRsaOaep);
No PSS?

[eroman, 2014-03-14 21:16:25]

https://codereview.chromium.org/196513002/diff/40001/content/child/webcrypto/...
File content/child/webcrypto/webcrypto_util.cc (right):

https://codereview.chromium.org/196513002/diff/40001/content/child/webcrypto/...
content/child/webcrypto/webcrypto_util.cc:264: id ==
blink::WebCryptoAlgorithmIdRsaOaep);
On 2014/03/14 20:54:11, Ryan Sleevi wrote:
> No PSS?

Not yet defined in blink, so no ID for such. Will expand it once those are added
(been adding the stuff piecemeal as it is needed)

[eroman, 2014-03-17 15:50:34]

The CQ bit was checked by eroman@chromium.org

[commit-bot: I haz the power, 2014-03-17 15:50:55]

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

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

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

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

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

[eroman, 2014-03-18 00:31:10]

The CQ bit was checked by eroman@chromium.org

[commit-bot: I haz the power, 2014-03-18 00:34:40]

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

[commit-bot: I haz the power, 2014-03-18 01:42:24]

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

[commit-bot: I haz the power, 2014-03-18 01:42:24]

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

[eroman, 2014-03-18 01:44:43]

The CQ bit was checked by eroman@chromium.org

[commit-bot: I haz the power, 2014-03-18 01:46:38]

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

[commit-bot: I haz the power, 2014-03-18 05:40:37]

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

[commit-bot: I haz the power, 2014-03-18 05:40:38]

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

[eroman, 2014-03-18 05:55:30]

The CQ bit was checked by eroman@chromium.org

[commit-bot: I haz the power, 2014-03-18 05:56:31]

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

[commit-bot: I haz the power, 2014-03-18 18:42:16]

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

[commit-bot: I haz the power, 2014-03-18 18:42:20]

Commit queue rejected this change because the description was changed
between the time the change entered the commit queue and the time it
was ready to commit. You can safely check the commit box again.

[eroman, 2014-03-18 19:12:07]

The CQ bit was checked by eroman@chromium.org

[commit-bot: I haz the power, 2014-03-18 19:30:04]

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

[commit-bot: I haz the power, 2014-03-18 20:27:15]

Change committed as 257723