Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
789733009:
Implement HKDF for WebCrypto (blink-side) (Closed)
DescriptionAdd support for HKDF WebCryptoAlgorithm
This is for implementing the HKDF algorithm for WebCrypto
(https://www.w3.org/Bugs/Public/show_bug.cgi?id=27425). The main parts
of this are creating a new key type for KDFs, and adding a new class
WebCryptoHkdfParams. It also includes various bits of plumbing including
parsing HKDF params and serialization/deserialization of HKDF keys.
https://codereview.chromium.org/803173010/ is the chromium-side CL, and https://codereview.chromium.org/822083003/ are the layout tests.
BUG=399095
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=188247
Patch Set 1 #Patch Set 2 : #Patch Set 3 : rebase #
Total comments: 20
Patch Set 4 : #Patch Set 5 : add serialization/deserialization #
Total comments: 10
Patch Set 6 : remove unnecessary param, clean up comment, re-write description #
Total comments: 5
Patch Set 7 : iff -> if #Patch Set 8 : KdfKeyTag -> NoParamsKeyTag and associated semantic changes #
Total comments: 4
Patch Set 9 : move ASSERT(), add break #Messages
Total messages: 27 (6 generated)
nharper@chromium.org changed reviewers: + eroman@chromium.org
Also can you link to the accompanying LayoutTests (which will land later). https://codereview.chromium.org/789733009/diff/40001/Source/bindings/modules/... File Source/bindings/modules/v8/ScriptValueSerializerForModules.cpp (right): https://codereview.chromium.org/789733009/diff/40001/Source/bindings/modules/... Source/bindings/modules/v8/ScriptValueSerializerForModules.cpp:126: ASSERT_NOT_REACHED(); This is incomplete. Serialization of HKDF keys is going to fail. If you are going to be editing this file and introducing the HDKF key type, you should implement serialization/deserialization too. https://codereview.chromium.org/789733009/diff/40001/Source/modules/crypto/No... File Source/modules/crypto/NormalizeAlgorithm.cpp (right): https://codereview.chromium.org/789733009/diff/40001/Source/modules/crypto/No... Source/modules/crypto/NormalizeAlgorithm.cpp:777: // The WebCrypto spec hasn't been updated yet to define HKDF. I am assuming a Please add a FIXME or a link to a bug, so that there is an action-item somewhere to update this comment (and possibly the implementation). https://codereview.chromium.org/789733009/diff/40001/Source/modules/crypto/No... Source/modules/crypto/NormalizeAlgorithm.cpp:780: // dictionary HkdfParams : Algorithm { How confident are you that this API will match the spec changes? (Taking stuff back is problematic). The bigger deal would be if the reflected key parameters changed, since those can be persisted and requires backward support forever. https://codereview.chromium.org/789733009/diff/40001/Source/modules/crypto/No... Source/modules/crypto/NormalizeAlgorithm.cpp:786: // It is possible that salt will be changed to be optional. nit: no need for this comment, already covered by above in my opinion. https://codereview.chromium.org/789733009/diff/40001/Source/platform/exported... File Source/platform/exported/WebCryptoKeyAlgorithm.cpp (right): https://codereview.chromium.org/789733009/diff/40001/Source/platform/exported... Source/platform/exported/WebCryptoKeyAlgorithm.cpp:96: WebCryptoKeyAlgorithm WebCryptoKeyAlgorithm::create(WebCryptoAlgorithmId id) Call this createHkdf() instead and omit the ID parameter. (There is no other valid ID that can be passed). https://codereview.chromium.org/789733009/diff/40001/Source/platform/exported... Source/platform/exported/WebCryptoKeyAlgorithm.cpp:98: return WebCryptoKeyAlgorithm(id, nullptr); We should find out from the working group if this is likely to stick. It seems like at a minimum there should be a "length" attribute for the key to match how HMAC and AES keys work. The reason this is important, is once we support these keys and allow serializing them, if all the properties aren't there and are added later we will have to monkey that in, which is not great. https://codereview.chromium.org/789733009/diff/40001/public/platform/WebCrypt... File public/platform/WebCryptoAlgorithmParams.h (right): https://codereview.chromium.org/789733009/diff/40001/public/platform/WebCrypt... public/platform/WebCryptoAlgorithmParams.h:359: WebVector<unsigned char> m_salt; const
https://codereview.chromium.org/789733009/diff/40001/Source/platform/exported... File Source/platform/exported/WebCryptoKeyAlgorithm.cpp (right): https://codereview.chromium.org/789733009/diff/40001/Source/platform/exported... Source/platform/exported/WebCryptoKeyAlgorithm.cpp:98: return WebCryptoKeyAlgorithm(id, nullptr); On 2014/12/23 20:58:24, eroman wrote: > We should find out from the working group if this is likely to stick. It seems > like at a minimum there should be a "length" attribute for the key to match how > HMAC and AES keys work. Actually I am fairly confident a "length" attribute isn't a good idea. But would still be good to confirm with Ryan before this becomes set in stone. > The reason this is important, is once we support these keys and allow > serializing them, if all the properties aren't there and are added later we will > have to monkey that in, which is not great.
https://codereview.chromium.org/803173010 is the chromium CL to implement hkdf and https://codereview.chromium.org/822083003 is the blink CL with the layout tests. https://codereview.chromium.org/789733009/diff/40001/Source/bindings/modules/... File Source/bindings/modules/v8/ScriptValueSerializerForModules.cpp (right): https://codereview.chromium.org/789733009/diff/40001/Source/bindings/modules/... Source/bindings/modules/v8/ScriptValueSerializerForModules.cpp:126: ASSERT_NOT_REACHED(); On 2014/12/23 20:58:24, eroman wrote: > This is incomplete. Serialization of HKDF keys is going to fail. > > If you are going to be editing this file and introducing the HDKF key type, you > should implement serialization/deserialization too. Can you point me to an example CL that added serialization/deserialization for an algorithm? I'm not familiar with this part of webcrypto. https://codereview.chromium.org/789733009/diff/40001/Source/modules/crypto/No... File Source/modules/crypto/NormalizeAlgorithm.cpp (right): https://codereview.chromium.org/789733009/diff/40001/Source/modules/crypto/No... Source/modules/crypto/NormalizeAlgorithm.cpp:777: // The WebCrypto spec hasn't been updated yet to define HKDF. I am assuming a On 2014/12/23 20:58:24, eroman wrote: > Please add a FIXME or a link to a bug, so that there is an action-item somewhere > to update this comment (and possibly the implementation). Done. https://codereview.chromium.org/789733009/diff/40001/Source/modules/crypto/No... Source/modules/crypto/NormalizeAlgorithm.cpp:780: // dictionary HkdfParams : Algorithm { On 2014/12/23 20:58:24, eroman wrote: > How confident are you that this API will match the spec changes? (Taking stuff > back is problematic). > > The bigger deal would be if the reflected key parameters changed, since those > can be persisted and requires backward support forever. I'm not confident that this will match the spec. The names of the parameters salt and info will most likely stay the same (as I pulled them from the RFC). It's probably more likely that the salt will be optional instead of required. There's a chance that info will be optional, although considering that label and context were required for what the spec called "HKDF-CTR", info will probably be required. Assuming the names of the parameters stay the same, how much of a problem is it if a parameter changes from required to optional? https://codereview.chromium.org/789733009/diff/40001/Source/modules/crypto/No... Source/modules/crypto/NormalizeAlgorithm.cpp:786: // It is possible that salt will be changed to be optional. On 2014/12/23 20:58:24, eroman wrote: > nit: no need for this comment, already covered by above in my opinion. Done. https://codereview.chromium.org/789733009/diff/40001/Source/platform/exported... File Source/platform/exported/WebCryptoKeyAlgorithm.cpp (right): https://codereview.chromium.org/789733009/diff/40001/Source/platform/exported... Source/platform/exported/WebCryptoKeyAlgorithm.cpp:96: WebCryptoKeyAlgorithm WebCryptoKeyAlgorithm::create(WebCryptoAlgorithmId id) On 2014/12/23 20:58:24, eroman wrote: > Call this createHkdf() instead and omit the ID parameter. (There is no other > valid ID that can be passed). Done. https://codereview.chromium.org/789733009/diff/40001/Source/platform/exported... Source/platform/exported/WebCryptoKeyAlgorithm.cpp:98: return WebCryptoKeyAlgorithm(id, nullptr); On 2014/12/23 20:58:24, eroman wrote: > We should find out from the working group if this is likely to stick. It seems > like at a minimum there should be a "length" attribute for the key to match how > HMAC and AES keys work. > > The reason this is important, is once we support these keys and allow > serializing them, if all the properties aren't there and are added later we will > have to monkey that in, which is not great. PBKDF2 and CONCAT both have no extra params for KeyAlgorithm, like HKDF here. The only reason I can find for HMAC having a length attribute is for generateKey, a method which HKDF does not support. https://codereview.chromium.org/789733009/diff/40001/public/platform/WebCrypt... File public/platform/WebCryptoAlgorithmParams.h (right): https://codereview.chromium.org/789733009/diff/40001/public/platform/WebCrypt... public/platform/WebCryptoAlgorithmParams.h:359: WebVector<unsigned char> m_salt; On 2014/12/23 20:58:24, eroman wrote: > const Done.
https://codereview.chromium.org/789733009/diff/40001/Source/bindings/modules/... File Source/bindings/modules/v8/ScriptValueSerializerForModules.cpp (right): https://codereview.chromium.org/789733009/diff/40001/Source/bindings/modules/... Source/bindings/modules/v8/ScriptValueSerializerForModules.cpp:126: ASSERT_NOT_REACHED(); On 2014/12/23 22:46:59, nharper wrote: > On 2014/12/23 20:58:24, eroman wrote: > > This is incomplete. Serialization of HKDF keys is going to fail. > > > > If you are going to be editing this file and introducing the HDKF key type, > you > > should implement serialization/deserialization too. > > Can you point me to an example CL that added serialization/deserialization for > an algorithm? I'm not familiar with this part of webcrypto. Example: https://code.google.com/p/chromium/codesearch#chromium/src/content/child/webc... You can probably copy paste the code for those 2 functions and remove the params stuff, and should be good. https://codereview.chromium.org/789733009/diff/40001/Source/modules/crypto/No... File Source/modules/crypto/NormalizeAlgorithm.cpp (right): https://codereview.chromium.org/789733009/diff/40001/Source/modules/crypto/No... Source/modules/crypto/NormalizeAlgorithm.cpp:780: // dictionary HkdfParams : Algorithm { On 2014/12/23 22:46:59, nharper wrote: > On 2014/12/23 20:58:24, eroman wrote: > > How confident are you that this API will match the spec changes? (Taking stuff > > back is problematic). > > > > The bigger deal would be if the reflected key parameters changed, since those > > can be persisted and requires backward support forever. > > I'm not confident that this will match the spec. The names of the parameters > salt and info will most likely stay the same (as I pulled them from the RFC). > It's probably more likely that the salt will be optional instead of required. > There's a chance that info will be optional, although considering that label and > context were required for what the spec called "HKDF-CTR", info will probably be > required. > > Assuming the names of the parameters stay the same, how much of a problem is it > if a parameter changes from required to optional? Changing a required parameter from required -> optional is not a problem. Going the other direction would be. The rule of thumb, is we don't want web pages which previously worked, to suddenly stop working. Relaxing the constraints is pretty much always safe, however narrowing constraints will break consumers. https://codereview.chromium.org/789733009/diff/40001/Source/platform/exported... File Source/platform/exported/WebCryptoKeyAlgorithm.cpp (right): https://codereview.chromium.org/789733009/diff/40001/Source/platform/exported... Source/platform/exported/WebCryptoKeyAlgorithm.cpp:98: return WebCryptoKeyAlgorithm(id, nullptr); On 2014/12/23 22:46:59, nharper wrote: > On 2014/12/23 20:58:24, eroman wrote: > > We should find out from the working group if this is likely to stick. It seems > > like at a minimum there should be a "length" attribute for the key to match > how > > HMAC and AES keys work. > > > > The reason this is important, is once we support these keys and allow > > serializing them, if all the properties aren't there and are added later we > will > > have to monkey that in, which is not great. > > PBKDF2 and CONCAT both have no extra params for KeyAlgorithm, like HKDF here. > The only reason I can find for HMAC having a length attribute is for > generateKey, a method which HKDF does not support. The other reason HMAC needs it, is HMAC keys are not necessarily byte-lengthed, so without reflecting the key length there would be information loss when exporting / re-importing. (Even though algorithmically HMAC internally sets that stuff to zero...)
https://codereview.chromium.org/789733009/diff/40001/Source/bindings/modules/... File Source/bindings/modules/v8/ScriptValueSerializerForModules.cpp (right): https://codereview.chromium.org/789733009/diff/40001/Source/bindings/modules/... Source/bindings/modules/v8/ScriptValueSerializerForModules.cpp:126: ASSERT_NOT_REACHED(); On 2014/12/23 23:34:29, eroman wrote: > On 2014/12/23 22:46:59, nharper wrote: > > On 2014/12/23 20:58:24, eroman wrote: > > > This is incomplete. Serialization of HKDF keys is going to fail. > > > > > > If you are going to be editing this file and introducing the HDKF key type, > > you > > > should implement serialization/deserialization too. > > > > Can you point me to an example CL that added serialization/deserialization for > > an algorithm? I'm not familiar with this part of webcrypto. > > Example: > > https://code.google.com/p/chromium/codesearch#chromium/src/content/child/webc... > > You can probably copy paste the code for those 2 functions and remove the params > stuff, and should be good. Given that PBKDF2's key will be exactly the same, can probably invent a format that will generalize to both (i.e just save the algorithm id maybe?) I dunno, whatever approach ends up being cleaner.
https://codereview.chromium.org/789733009/diff/40001/Source/bindings/modules/... File Source/bindings/modules/v8/ScriptValueSerializerForModules.cpp (right): https://codereview.chromium.org/789733009/diff/40001/Source/bindings/modules/... Source/bindings/modules/v8/ScriptValueSerializerForModules.cpp:126: ASSERT_NOT_REACHED(); On 2014/12/24 00:38:27, eroman wrote: > On 2014/12/23 23:34:29, eroman wrote: > > On 2014/12/23 22:46:59, nharper wrote: > > > On 2014/12/23 20:58:24, eroman wrote: > > > > This is incomplete. Serialization of HKDF keys is going to fail. > > > > > > > > If you are going to be editing this file and introducing the HDKF key > type, > > > you > > > > should implement serialization/deserialization too. > > > > > > Can you point me to an example CL that added serialization/deserialization > for > > > an algorithm? I'm not familiar with this part of webcrypto. > > > > Example: > > > > > https://code.google.com/p/chromium/codesearch#chromium/src/content/child/webc... > > > > You can probably copy paste the code for those 2 functions and remove the > params > > stuff, and should be good. > > Given that PBKDF2's key will be exactly the same, can probably invent a format > that will generalize to both (i.e just save the algorithm id maybe?) > > I dunno, whatever approach ends up being cleaner. I added a Kdf params type (to be used for both HKDF and PBKDF2)
Please provide more detail in the change description on what this is implementing. Link to the relevant spec bug to give some extra confidence that this is planned to be specced. Otherwise this mostly looksgood. Also the comment description is somewhat in complete (it also introduces a new key type). You may want to work that in too. https://codereview.chromium.org/789733009/diff/80001/Source/modules/crypto/No... File Source/modules/crypto/NormalizeAlgorithm.cpp (right): https://codereview.chromium.org/789733009/diff/80001/Source/modules/crypto/No... Source/modules/crypto/NormalizeAlgorithm.cpp:780: // The WebCrypto spec hasn't been updated yet to define HKDF. I am assuming a Leave out things like "we" and "I" as it is unclear who is speaking. Phrase more generally like: "The WebCrypto spec hasn't been updated yet to define HKDF <Link to spec bug>. The assumed parameters are:" https://codereview.chromium.org/789733009/diff/80001/Source/platform/exported... File Source/platform/exported/WebCryptoKeyAlgorithm.cpp (right): https://codereview.chromium.org/789733009/diff/80001/Source/platform/exported... Source/platform/exported/WebCryptoKeyAlgorithm.cpp:166: if (m_private->params.get()) This change seems like it is sufficient, rather than defining an HkdfKeyParams. See later comments. (To be clear I am cool with this changed line, but not convinced on the addition of the new KdfKeyParams). https://codereview.chromium.org/789733009/diff/80001/public/platform/WebCrypt... File public/platform/WebCryptoKeyAlgorithm.h (right): https://codereview.chromium.org/789733009/diff/80001/public/platform/WebCrypt... public/platform/WebCryptoKeyAlgorithm.h:90: BLINK_PLATFORM_EXPORT WebCryptoKdfKeyAlgorithmParams* kdfParams() const; Why is this necessary? It looks like your other change was intended for KDF keys to have no params (which I think makes sense). https://codereview.chromium.org/789733009/diff/80001/public/platform/WebCrypt... File public/platform/WebCryptoKeyAlgorithmParams.h (right): https://codereview.chromium.org/789733009/diff/80001/public/platform/WebCrypt... public/platform/WebCryptoKeyAlgorithmParams.h:58: WebCryptoKeyAlgorithmParamsTypeKdf, Same comment as earlier. https://codereview.chromium.org/789733009/diff/80001/public/platform/WebCrypt... public/platform/WebCryptoKeyAlgorithmParams.h:210: class WebCryptoKdfKeyAlgorithmParams : public WebCryptoKeyAlgorithmParams { Same comment as earlier. I don't think there is value in having an empty params structure.
https://codereview.chromium.org/789733009/diff/80001/Source/modules/crypto/No... File Source/modules/crypto/NormalizeAlgorithm.cpp (right): https://codereview.chromium.org/789733009/diff/80001/Source/modules/crypto/No... Source/modules/crypto/NormalizeAlgorithm.cpp:780: // The WebCrypto spec hasn't been updated yet to define HKDF. I am assuming a On 2015/01/07 00:40:31, eroman wrote: > Leave out things like "we" and "I" as it is unclear who is speaking. Phrase more > generally like: > > "The WebCrypto spec hasn't been updated yet to define HKDF <Link to spec bug>. > The assumed parameters are:" Done. https://codereview.chromium.org/789733009/diff/80001/Source/platform/exported... File Source/platform/exported/WebCryptoKeyAlgorithm.cpp (right): https://codereview.chromium.org/789733009/diff/80001/Source/platform/exported... Source/platform/exported/WebCryptoKeyAlgorithm.cpp:166: if (m_private->params.get()) On 2015/01/07 00:40:31, eroman wrote: > This change seems like it is sufficient, rather than defining an HkdfKeyParams. > See later comments. (To be clear I am cool with this changed line, but not > convinced on the addition of the new KdfKeyParams). The introduction of KdfKeyParams was to make something marginally easier in ScriptValueSerializerForModules.cpp, but I agree that there's no need for creating a new class that stores nothing in it. https://codereview.chromium.org/789733009/diff/80001/public/platform/WebCrypt... File public/platform/WebCryptoKeyAlgorithm.h (right): https://codereview.chromium.org/789733009/diff/80001/public/platform/WebCrypt... public/platform/WebCryptoKeyAlgorithm.h:90: BLINK_PLATFORM_EXPORT WebCryptoKdfKeyAlgorithmParams* kdfParams() const; On 2015/01/07 00:40:31, eroman wrote: > Why is this necessary? It looks like your other change was intended for KDF keys > to have no params (which I think makes sense). It was added to make the switch statement in SerializedScriptValueWriterForModules::writeCryptoKey() to be a little more consistent, but I agree that it isn't needed (since KDF keys don't have any params). I've removed it, as I don't think the code symmetry is worth adding this otherwise useless class. https://codereview.chromium.org/789733009/diff/80001/public/platform/WebCrypt... File public/platform/WebCryptoKeyAlgorithmParams.h (right): https://codereview.chromium.org/789733009/diff/80001/public/platform/WebCrypt... public/platform/WebCryptoKeyAlgorithmParams.h:58: WebCryptoKeyAlgorithmParamsTypeKdf, On 2015/01/07 00:40:31, eroman wrote: > Same comment as earlier. Done. https://codereview.chromium.org/789733009/diff/80001/public/platform/WebCrypt... public/platform/WebCryptoKeyAlgorithmParams.h:210: class WebCryptoKdfKeyAlgorithmParams : public WebCryptoKeyAlgorithmParams { On 2015/01/07 00:40:31, eroman wrote: > Same comment as earlier. I don't think there is value in having an empty params > structure. Done.
FYI I don't expect to re-review this until tomorrow. Cheers
looksgood after my naming nits https://codereview.chromium.org/789733009/diff/100001/Source/bindings/modules... File Source/bindings/modules/v8/ScriptValueSerializerForModules.cpp (right): https://codereview.chromium.org/789733009/diff/100001/Source/bindings/modules... Source/bindings/modules/v8/ScriptValueSerializerForModules.cpp:128: doWriteKdfKey(key); Given the switch statement on params type, I think a better naming is to call these "key without params" or some such thing, rather than KdfKey. How about: doWriteKeyWithoutParams(key) I suggest this because an understanding of the KDF class of algorithms and what params they do and don't have is a more fuzzy concept in my opinion. Whereas a key without any params is something readers immediately get. (The crypto knowledge as much as possible is kept out of Blink). https://codereview.chromium.org/789733009/diff/100001/public/platform/WebCryp... File public/platform/WebCryptoAlgorithm.h (right): https://codereview.chromium.org/789733009/diff/100001/public/platform/WebCryp... public/platform/WebCryptoAlgorithm.h:200: // Returns true iff the provided algorithm ID is for a key derivation function "iff" as in "if and only if" ? I don't like using "iff" in comments. To readers who didn't study math in English it just reads like a typo. Drop the second "f" and it is still just as clear.
https://codereview.chromium.org/789733009/diff/100001/Source/bindings/modules... File Source/bindings/modules/v8/ScriptValueSerializerForModules.cpp (right): https://codereview.chromium.org/789733009/diff/100001/Source/bindings/modules... Source/bindings/modules/v8/ScriptValueSerializerForModules.cpp:128: doWriteKdfKey(key); On 2015/01/09 03:07:29, eroman wrote: > Given the switch statement on params type, I think a better naming is to call > these "key without params" or some such thing, rather than KdfKey. > > How about: > > doWriteKeyWithoutParams(key) > > I suggest this because an understanding of the KDF class of algorithms and what > params they do and don't have is a more fuzzy concept in my opinion. Whereas a > key without any params is something readers immediately get. (The crypto > knowledge as much as possible is kept out of Blink). I think this is reasonable, but the changes needed would be in more than just this file (otherwise we're just pushing this inconsistency around the serialization/deserialization code here). Specifically, it would be changing WebCryptoKeyAlgorithm::createKdf() to be WebCryptoKeyAlgorithm::create() (or create an explicit 1-arg constructor for WebCryptoKeyAlgorithm), and have it not care about what type of algorithm it is, just that it's an algorithm with no params. WebCryptoAlgorithm::isKdf() would also want to be changed to something like WebCryptoAlgorithm::hasNoParams(WebCryptoAlgorithmId id). Should I go ahead and make those changes (I don't know if I'll run into more changes that will need to be made, but I think it will make it all consistent), or should I leave it as is? https://codereview.chromium.org/789733009/diff/100001/public/platform/WebCryp... File public/platform/WebCryptoAlgorithm.h (right): https://codereview.chromium.org/789733009/diff/100001/public/platform/WebCryp... public/platform/WebCryptoAlgorithm.h:200: // Returns true iff the provided algorithm ID is for a key derivation function On 2015/01/09 03:07:29, eroman wrote: > "iff" as in "if and only if" ? > > I don't like using "iff" in comments. To readers who didn't study math in > English it just reads like a typo. > > Drop the second "f" and it is still just as clear. Done.
https://codereview.chromium.org/789733009/diff/100001/Source/bindings/modules... File Source/bindings/modules/v8/ScriptValueSerializerForModules.cpp (right): https://codereview.chromium.org/789733009/diff/100001/Source/bindings/modules... Source/bindings/modules/v8/ScriptValueSerializerForModules.cpp:128: doWriteKdfKey(key); On 2015/01/09 04:45:52, nharper wrote: > On 2015/01/09 03:07:29, eroman wrote: > > Given the switch statement on params type, I think a better naming is to call > > these "key without params" or some such thing, rather than KdfKey. > > > > How about: > > > > doWriteKeyWithoutParams(key) > > > > I suggest this because an understanding of the KDF class of algorithms and > what > > params they do and don't have is a more fuzzy concept in my opinion. Whereas a > > key without any params is something readers immediately get. (The crypto > > knowledge as much as possible is kept out of Blink). > > I think this is reasonable, but the changes needed would be in more than just > this file (otherwise we're just pushing this inconsistency around the > serialization/deserialization code here). Specifically, it would be changing > WebCryptoKeyAlgorithm::createKdf() to be WebCryptoKeyAlgorithm::create() (or > create an explicit 1-arg constructor for WebCryptoKeyAlgorithm), and have it not > care about what type of algorithm it is, just that it's an algorithm with no > params. WebCryptoAlgorithm::isKdf() would also want to be changed to something > like WebCryptoAlgorithm::hasNoParams(WebCryptoAlgorithmId id). > > Should I go ahead and make those changes (I don't know if I'll run into more > changes that will need to be made, but I think it will make it all consistent), > or should I leave it as is? We discussed this, and I've changed it.
Please link to the corresponding LayoutTests in the CL description. LGTM https://codereview.chromium.org/789733009/diff/140001/Source/bindings/modules... File Source/bindings/modules/v8/ScriptValueSerializerForModules.cpp (right): https://codereview.chromium.org/789733009/diff/140001/Source/bindings/modules... Source/bindings/modules/v8/ScriptValueSerializerForModules.cpp:127: ASSERT(WebCryptoAlgorithm::isKdf(key.algorithm().id())); Can you move this assertion into doWriteKeyWithoutParams() ? https://codereview.chromium.org/789733009/diff/140001/Source/bindings/modules... Source/bindings/modules/v8/ScriptValueSerializerForModules.cpp:128: doWriteKeyWithoutParams(key); Please add a break statement for symmetry with the other lines (more future proof in case someone adds a new case statement). (Although this is the last of the key types described by WebCrypto)
https://codereview.chromium.org/789733009/diff/140001/Source/bindings/modules... File Source/bindings/modules/v8/ScriptValueSerializerForModules.cpp (right): https://codereview.chromium.org/789733009/diff/140001/Source/bindings/modules... Source/bindings/modules/v8/ScriptValueSerializerForModules.cpp:127: ASSERT(WebCryptoAlgorithm::isKdf(key.algorithm().id())); On 2015/01/09 22:12:45, eroman wrote: > Can you move this assertion into doWriteKeyWithoutParams() ? Done. https://codereview.chromium.org/789733009/diff/140001/Source/bindings/modules... Source/bindings/modules/v8/ScriptValueSerializerForModules.cpp:128: doWriteKeyWithoutParams(key); On 2015/01/09 22:12:45, eroman wrote: > Please add a break statement for symmetry with the other lines (more future > proof in case someone adds a new case statement). > > (Although this is the last of the key types described by WebCrypto) Done.
The CQ bit was checked by nharper@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/789733009/160001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: blink_presubmit on tryserver.blink (http://build.chromium.org/p/tryserver.blink/builders/blink_presubmit/builds/2...)
The CQ bit was unchecked by nharper@chromium.org
nharper@chromium.org changed reviewers: + jochen@chromium.org
Adding jochen@ to review for OWNERS approval.
lgtm
The CQ bit was checked by nharper@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/789733009/160001
Message was sent while issue was closed.
Committed patchset #9 (id:160001) as https://src.chromium.org/viewvc/blink?view=rev&revision=188247 |
