Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(74)

Issue 195543002: [webcrypto] Implement structured clone of keys (blink-side). (Closed)

Created:
6 years, 9 months ago by eroman
Modified:
6 years, 9 months ago
Reviewers:
jsbell, abarth-chromium
CC:
blink-reviews, Nils Barth (inactive), jamesr, kojih, arv+blink, jsbell+bindings_chromium.org, kouhei+bindings_chromium.org, abarth-chromium, marja+watch_chromium.org, dglazkov+blink, adamk+blink_chromium.org, haraken, Nate Chapin, Inactive
Visibility:
Public.

Description

[webcrypto] Implement structured clone of keys (blink-side). The format looks like this: subtag:byte // The type of key keySpecificProperties // Block of key specific algorithm info usages:uint32 // Bitfield of usages + extractability keyDataLength:uint32 // Block of data controlled by embedder keyData:byte[keyDataLength] subtag influences how keySpecificProperties is interpreted: [If subtag=AesKeyTag] keyLengthBytes:uint32 // 16, 24, or 32 algorithmId:uint32 [If subtag=HmacKeyTag] keyLengthBytes:uint32 hashId:uint32 [If subtag=RsaKeyTag] algorithmId:uint32 type:uint32 // One of {PublicKeyType, PrivateKeyType} modulusLengthBits:uint32 publicExponentLength:uint32 publicExponent:byte[publicExponentLength] [If subtag=RsaHashedKeyTag] <Same as for RsaKeyTag> hashId:uint32 Note that uint32 is encoded as a variable length number. In practice it ends up being a single byte for most of the uses above. In this design, blink is responsible for serializing all of the key's attributes except for the actual key data which is left to the embedder. The included tests rely on the chromium side of structured clone landing: https://codereview.chromium.org/196513002/ The tests cover serialization of hmac, aes and rsa public keys. I haven't added tests for serialization of rsa private keys yet, since that part is not done on the chromium side. BUG=245025 Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=169633

Patch Set 1 : #

Total comments: 5

Patch Set 2 : Update comments #

Patch Set 3 : fix another comment #

Total comments: 30

Patch Set 4 : Fixes for jsbell #

Total comments: 3

Patch Set 5 : Fix broken test, and remove WebCryptoKeyUsage shift #

Patch Set 6 : Rebase #

Patch Set 7 : Rebase #

Patch Set 8 : Remove SHA224 and add HMAC key length #

Patch Set 9 : remove trailing space #

Patch Set 10 : Update serialized-script-value.html for version bump #

Unified diffs Side-by-side diffs Delta from patch set Stats (+4206 lines, -11 lines) Patch
A LayoutTests/crypto/clone-aesKey.html View 1 2 3 1 chunk +92 lines, -0 lines 0 comments Download
A LayoutTests/crypto/clone-aesKey-expected.txt View 1 2 3 4 5 6 7 1 chunk +849 lines, -0 lines 0 comments Download
A LayoutTests/crypto/clone-hmacKey.html View 1 2 3 4 5 6 7 1 chunk +96 lines, -0 lines 0 comments Download
A LayoutTests/crypto/clone-hmacKey-expected.txt View 1 2 3 4 5 6 7 1 chunk +945 lines, -0 lines 0 comments Download
A LayoutTests/crypto/clone-rsaHashedKey-public.html View 1 2 3 1 chunk +123 lines, -0 lines 0 comments Download
A LayoutTests/crypto/clone-rsaHashedKey-public-expected.txt View 1 2 3 4 5 6 7 1 chunk +1041 lines, -0 lines 0 comments Download
A LayoutTests/crypto/clone-rsaKey-public.html View 1 2 3 4 1 chunk +120 lines, -0 lines 0 comments Download
A LayoutTests/crypto/clone-rsaKey-public-expected.txt View 1 2 3 4 1 chunk +353 lines, -0 lines 0 comments Download
M LayoutTests/crypto/resources/common.js View 1 2 3 1 chunk +36 lines, -0 lines 0 comments Download
M LayoutTests/fast/storage/resources/serialized-script-value.js View 1 2 3 4 5 6 7 8 9 1 chunk +1 line, -1 line 0 comments Download
M Source/bindings/v8/SerializedScriptValue.h View 1 2 3 1 chunk +2 lines, -1 line 0 comments Download
M Source/bindings/v8/SerializedScriptValue.cpp View 1 2 3 4 5 6 7 11 chunks +430 lines, -1 line 0 comments Download
M Source/modules/crypto/NormalizeAlgorithm.cpp View 1 chunk +1 line, -1 line 0 comments Download
M Source/platform/exported/WebCryptoAlgorithm.cpp View 1 2 3 4 5 6 7 1 chunk +21 lines, -0 lines 0 comments Download
M Source/platform/exported/WebCryptoKeyAlgorithm.cpp View 1 2 3 4 5 6 7 2 chunks +36 lines, -0 lines 0 comments Download
M public/platform/WebCrypto.h View 1 2 3 4 5 6 7 8 9 2 chunks +49 lines, -0 lines 0 comments Download
M public/platform/WebCryptoAlgorithm.h View 1 2 3 4 5 6 2 chunks +4 lines, -1 line 0 comments Download
M public/platform/WebCryptoKey.h View 1 2 3 4 5 6 7 8 9 1 chunk +0 lines, -5 lines 0 comments Download
M public/platform/WebCryptoKeyAlgorithm.h View 1 2 3 4 5 6 7 1 chunk +6 lines, -0 lines 0 comments Download
M public/web/WebSerializedScriptValueVersion.h View 1 2 3 1 chunk +1 line, -1 line 0 comments Download

Messages

Total messages: 42 (0 generated)
eroman
Not asking for a real review yet, just the questions above. Thanks!
6 years, 9 months ago (2014-03-11 17:59:55 UTC) #1
abarth-chromium
> - What is the recommended way of organizing the code in SerializedScriptValue.cpp? My changelist ...
6 years, 9 months ago (2014-03-11 18:41:25 UTC) #2
Ryan Sleevi
pedantry https://codereview.chromium.org/195543002/diff/40001/public/platform/WebCrypto.h File public/platform/WebCrypto.h (right): https://codereview.chromium.org/195543002/diff/40001/public/platform/WebCrypto.h#newcode203 public/platform/WebCrypto.h:203: // In other words, blink takes care of ...
6 years, 9 months ago (2014-03-12 22:54:34 UTC) #3
eroman
Adam: This is now ready for review, thanks!
6 years, 9 months ago (2014-03-12 23:02:13 UTC) #4
abarth-chromium
@jsbell: Would you be willing to review the SerializedScriptValue interactions?
6 years, 9 months ago (2014-03-13 01:24:59 UTC) #5
sof
On 2014/03/11 18:41:25, abarth wrote: > > - What is the recommended way of organizing ...
6 years, 9 months ago (2014-03-13 10:39:15 UTC) #6
jsbell
On 2014/03/13 01:24:59, abarth wrote: > @jsbell: Would you be willing to review the SerializedScriptValue ...
6 years, 9 months ago (2014-03-13 18:22:31 UTC) #7
jsbell
Overall lgtm, with a handful of suggestions. Bumping the wire format version is the only ...
6 years, 9 months ago (2014-03-13 20:16:06 UTC) #8
jsbell
On 2014/03/13 10:39:15, sof wrote: > Is it worth considering defining an interface that allows ...
6 years, 9 months ago (2014-03-13 20:24:04 UTC) #9
eroman
Thanks for the comments! https://codereview.chromium.org/195543002/diff/80001/LayoutTests/crypto/clone-aesKey.html File LayoutTests/crypto/clone-aesKey.html (right): https://codereview.chromium.org/195543002/diff/80001/LayoutTests/crypto/clone-aesKey.html#newcode39 LayoutTests/crypto/clone-aesKey.html:39: shouldBe("importedKey.extractable", evalWrap(extractable)); On 2014/03/13 20:16:07, ...
6 years, 9 months ago (2014-03-14 05:24:32 UTC) #10
jsbell
Note error in layout test. Did you see my question about public/platform/WebCryptoKey.h:47 ? https://codereview.chromium.org/195543002/diff/100001/LayoutTests/crypto/clone-rsaKey-public.html File ...
6 years, 9 months ago (2014-03-14 16:22:16 UTC) #11
jsbell
BTW, changes look good other than that test glitch. You'll need an OWNER to approve ...
6 years, 9 months ago (2014-03-14 16:33:46 UTC) #12
eroman
https://codereview.chromium.org/195543002/diff/80001/public/platform/WebCryptoKey.h File public/platform/WebCryptoKey.h (right): https://codereview.chromium.org/195543002/diff/80001/public/platform/WebCryptoKey.h#newcode47 public/platform/WebCryptoKey.h:47: WebCryptoKeyUsageEncrypt = 1 << 1, On 2014/03/13 20:16:07, jsbell ...
6 years, 9 months ago (2014-03-14 18:28:08 UTC) #13
jsbell
https://codereview.chromium.org/195543002/diff/80001/public/platform/WebCryptoKey.h File public/platform/WebCryptoKey.h (right): https://codereview.chromium.org/195543002/diff/80001/public/platform/WebCryptoKey.h#newcode47 public/platform/WebCryptoKey.h:47: WebCryptoKeyUsageEncrypt = 1 << 1, On 2014/03/14 18:28:09, eroman ...
6 years, 9 months ago (2014-03-14 18:39:26 UTC) #14
eroman
https://codereview.chromium.org/195543002/diff/80001/public/platform/WebCryptoKey.h File public/platform/WebCryptoKey.h (right): https://codereview.chromium.org/195543002/diff/80001/public/platform/WebCryptoKey.h#newcode47 public/platform/WebCryptoKey.h:47: WebCryptoKeyUsageEncrypt = 1 << 1, On 2014/03/14 18:39:27, jsbell ...
6 years, 9 months ago (2014-03-14 19:32:50 UTC) #15
eroman
@abarth: I believe this is now ready for your review. Thanks!
6 years, 9 months ago (2014-03-15 00:08:16 UTC) #16
abarth-chromium
public/ LGTM
6 years, 9 months ago (2014-03-18 22:34:05 UTC) #17
eroman
BTW I had to make some additional (small) changes while rebasing: * SHA-224 was removed ...
6 years, 9 months ago (2014-03-19 19:37:12 UTC) #18
jsbell
still lgtm
6 years, 9 months ago (2014-03-19 21:19:18 UTC) #19
eroman
The CQ bit was checked by eroman@chromium.org
6 years, 9 months ago (2014-03-19 23:17:29 UTC) #20
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/eroman@chromium.org/195543002/200001
6 years, 9 months ago (2014-03-19 23:17:41 UTC) #21
commit-bot: I haz the power
The CQ bit was unchecked by commit-bot@chromium.org
6 years, 9 months ago (2014-03-19 23:28:26 UTC) #22
commit-bot: I haz the power
Try jobs failed on following builders: tryserver.blink on blink_presubmit
6 years, 9 months ago (2014-03-19 23:28:27 UTC) #23
eroman
The CQ bit was checked by eroman@chromium.org
6 years, 9 months ago (2014-03-19 23:31:14 UTC) #24
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/eroman@chromium.org/195543002/220001
6 years, 9 months ago (2014-03-19 23:31:23 UTC) #25
commit-bot: I haz the power
The CQ bit was unchecked by commit-bot@chromium.org
6 years, 9 months ago (2014-03-20 00:01:48 UTC) #26
commit-bot: I haz the power
Try jobs failed on following builders: tryserver.blink on linux_blink_rel
6 years, 9 months ago (2014-03-20 00:01:49 UTC) #27
jsbell
On 2014/03/20 00:01:49, I haz the power (commit-bot) wrote: > Try jobs failed on following ...
6 years, 9 months ago (2014-03-20 00:05:17 UTC) #28
jsbell
On 2014/03/20 00:05:17, jsbell wrote: > On 2014/03/20 00:01:49, I haz the power (commit-bot) wrote: ...
6 years, 9 months ago (2014-03-20 00:08:43 UTC) #29
jsbell
On 2014/03/20 00:08:43, jsbell wrote: > On 2014/03/20 00:05:17, jsbell wrote: > > On 2014/03/20 ...
6 years, 9 months ago (2014-03-20 00:17:31 UTC) #30
eroman
Thanks for the tip jsbell. Done. Will try again now :)
6 years, 9 months ago (2014-03-20 00:55:40 UTC) #31
eroman
The CQ bit was checked by eroman@chromium.org
6 years, 9 months ago (2014-03-20 00:55:46 UTC) #32
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/eroman@chromium.org/195543002/260001
6 years, 9 months ago (2014-03-20 00:55:55 UTC) #33
commit-bot: I haz the power
The CQ bit was unchecked by commit-bot@chromium.org
6 years, 9 months ago (2014-03-20 00:57:17 UTC) #34
commit-bot: I haz the power
Try jobs failed on following builders: tryserver.blink on mac_blink_compile_dbg
6 years, 9 months ago (2014-03-20 00:57:18 UTC) #35
eroman
The CQ bit was checked by eroman@chromium.org
6 years, 9 months ago (2014-03-20 02:38:36 UTC) #36
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/eroman@chromium.org/195543002/260001
6 years, 9 months ago (2014-03-20 02:38:40 UTC) #37
commit-bot: I haz the power
The CQ bit was unchecked by commit-bot@chromium.org
6 years, 9 months ago (2014-03-20 02:41:40 UTC) #38
commit-bot: I haz the power
Try jobs failed on following builders: tryserver.blink on linux_blink_dbg
6 years, 9 months ago (2014-03-20 02:41:41 UTC) #39
eroman
The CQ bit was checked by eroman@chromium.org
6 years, 9 months ago (2014-03-20 06:48:41 UTC) #40
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/eroman@chromium.org/195543002/260001
6 years, 9 months ago (2014-03-20 06:48:50 UTC) #41
commit-bot: I haz the power
6 years, 9 months ago (2014-03-20 07:51:15 UTC) #42
Message was sent while issue was closed.
Change committed as 169633

Powered by Google App Engine
This is Rietveld 408576698