Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
803173010:
Implement HKDF for webcrypto (Closed)
DescriptionImplement HKDF for webcrypto
The webcrypto spec doesn't currently define HKDF as is implemented in this CL (https://dvcs.w3.org/hg/webcrypto-api/raw-file/tip/spec/Overview.html#hkdf-ctr is incorrect, as it refers to HKDF by name but has SP800-108 as the implementation). The HKDF implemented in this CL is the one from RFC 5869. The rest of the implementation tries to match the webcrypto spec as closely as possible (in terms of operations supported, parameters, which errors to throw under what circumstances), with the following notable change: The HkdfCtrParams dictionary is changed to have required BufferSources info and salt instead of label and context.
Layout tests for this change are in https://codereview.chromium.org/822083003/.
BUG=399095
Committed: https://crrev.com/6510317950146a4af1df4bac9bf4c52212b92766
Cr-Commit-Position: refs/heads/master@{#311291}
Patch Set 1 #
Total comments: 20
Patch Set 2 : #
Total comments: 19
Patch Set 3 : #
Total comments: 11
Patch Set 4 : #
Total comments: 6
Patch Set 5 : implement GetKeyLength #Patch Set 6 : fix variable names #Patch Set 7 : be consistent in using front() instead of [0] #Patch Set 8 : blink::WebCryptoKeyAlgorithm::createKdf() is now createWithoutParams() #
Total comments: 7
Patch Set 9 : add curlies and OpenSSLErrStackTracer #
Messages
Total messages: 23 (3 generated)
nharper@chromium.org changed reviewers: + eroman@chromium.org
This is dependent on https://codereview.chromium.org/809913003/
https://codereview.chromium.org/803173010/diff/1/content/child/webcrypto/open... File content/child/webcrypto/openssl/hkdf_openssl.cc (right): https://codereview.chromium.org/803173010/diff/1/content/child/webcrypto/open... content/child/webcrypto/openssl/hkdf_openssl.cc:8: // TODO: figure out which headers belong here and remove ones that were (1) TODO(nharper) (2) Address the TODO now :) https://codereview.chromium.org/803173010/diff/1/content/child/webcrypto/open... content/child/webcrypto/openssl/hkdf_openssl.cc:40: // need to check the format here. You should do the check, otherwise this is inconsistent with how other algorithms are implemented. In particular consider the case of unwrapping a key with a format other than raw. The errors will differ. https://codereview.chromium.org/803173010/diff/1/content/child/webcrypto/open... content/child/webcrypto/openssl/hkdf_openssl.cc:46: This also needs support for cloning/de-cloning. https://codereview.chromium.org/803173010/diff/1/content/child/webcrypto/open... content/child/webcrypto/openssl/hkdf_openssl.cc:64: // and converted to a number. It won't be called as part of deriveKey, I don't understand this comment. HKDF does support deriveKey(). Seems like this is reachable by calling deriveKey() for HDKF, and specifying the derivedKeyType as also HKDF, right? The spec says to throw a TypeError in this case. https://codereview.chromium.org/803173010/diff/1/content/child/webcrypto/open... content/child/webcrypto/openssl/hkdf_openssl.cc:74: GetDigest(algorithm.hkdfParams()->hash().id()); It is guaranteed to already be a digest, since the Blink Layer ensures it is a HashAlgorithmIdentifier when parsing. https://codereview.chromium.org/803173010/diff/1/content/child/webcrypto/open... content/child/webcrypto/openssl/hkdf_openssl.cc:79: unsigned int derived_bytes_len = (optional_length_bits + 7) / 8; Use NumBitsToBytes() instead. It is more readable, but more importantly the input is user controlled and we should not allow integer overflowing. https://codereview.chromium.org/803173010/diff/1/content/child/webcrypto/open... content/child/webcrypto/openssl/hkdf_openssl.cc:84: std::vector<uint8_t> raw_key = Don't make a copy of the key material. https://codereview.chromium.org/803173010/diff/1/content/child/webcrypto/open... content/child/webcrypto/openssl/hkdf_openssl.cc:86: blink::WebVector<unsigned char> salt = algorithm.hkdfParams()->salt(); Don't make copies here. https://codereview.chromium.org/803173010/diff/1/content/child/webcrypto/open... content/child/webcrypto/openssl/hkdf_openssl.cc:95: } else { nit: omit "else" when short-circuiting.
https://codereview.chromium.org/803173010/diff/1/content/child/webcrypto/open... File content/child/webcrypto/openssl/hkdf_openssl.cc (right): https://codereview.chromium.org/803173010/diff/1/content/child/webcrypto/open... content/child/webcrypto/openssl/hkdf_openssl.cc:8: // TODO: figure out which headers belong here and remove ones that were On 2014/12/23 23:13:36, eroman wrote: > (1) TODO(nharper) > (2) Address the TODO now :) Done. https://codereview.chromium.org/803173010/diff/1/content/child/webcrypto/open... content/child/webcrypto/openssl/hkdf_openssl.cc:40: // need to check the format here. On 2014/12/23 23:13:36, eroman wrote: > You should do the check, otherwise this is inconsistent with how other > algorithms are implemented. > > In particular consider the case of unwrapping a key with a format other than > raw. The errors will differ. I added a check here for this. It turns out in both cases, the correct thing to do is throw a NotSupportedError, so there's no difference in the errors. https://codereview.chromium.org/803173010/diff/1/content/child/webcrypto/open... content/child/webcrypto/openssl/hkdf_openssl.cc:46: On 2014/12/23 23:13:36, eroman wrote: > This also needs support for cloning/de-cloning. Done. https://codereview.chromium.org/803173010/diff/1/content/child/webcrypto/open... content/child/webcrypto/openssl/hkdf_openssl.cc:64: // and converted to a number. It won't be called as part of deriveKey, On 2014/12/23 23:13:36, eroman wrote: > I don't understand this comment. HKDF does support deriveKey(). > > Seems like this is reachable by calling deriveKey() for HDKF, and specifying the > derivedKeyType as also HKDF, right? > > The spec says to throw a TypeError in this case. Done. https://codereview.chromium.org/803173010/diff/1/content/child/webcrypto/open... content/child/webcrypto/openssl/hkdf_openssl.cc:74: GetDigest(algorithm.hkdfParams()->hash().id()); On 2014/12/23 23:13:36, eroman wrote: > It is guaranteed to already be a digest, since the Blink Layer ensures it is a > HashAlgorithmIdentifier when parsing. This same check is done in the HMAC code - any objection to leaving it here? https://codereview.chromium.org/803173010/diff/1/content/child/webcrypto/open... content/child/webcrypto/openssl/hkdf_openssl.cc:79: unsigned int derived_bytes_len = (optional_length_bits + 7) / 8; On 2014/12/23 23:13:36, eroman wrote: > Use NumBitsToBytes() instead. > > It is more readable, but more importantly the input is user controlled and we > should not allow integer overflowing. Done. https://codereview.chromium.org/803173010/diff/1/content/child/webcrypto/open... content/child/webcrypto/openssl/hkdf_openssl.cc:84: std::vector<uint8_t> raw_key = On 2014/12/23 23:13:36, eroman wrote: > Don't make a copy of the key material. Done. https://codereview.chromium.org/803173010/diff/1/content/child/webcrypto/open... content/child/webcrypto/openssl/hkdf_openssl.cc:86: blink::WebVector<unsigned char> salt = algorithm.hkdfParams()->salt(); On 2014/12/23 23:13:36, eroman wrote: > Don't make copies here. Done. https://codereview.chromium.org/803173010/diff/1/content/child/webcrypto/open... content/child/webcrypto/openssl/hkdf_openssl.cc:95: } else { On 2014/12/23 23:13:36, eroman wrote: > nit: omit "else" when short-circuiting. Done. Also removed the curly braces from the one-line if statement, even though I disagree with that style.
In the description please provide more details on what this is implementing. Ideally link to the WebCrypto spec. However since it isn't specified... Now you see why it needs more description :) https://codereview.chromium.org/803173010/diff/1/content/child/webcrypto/open... File content/child/webcrypto/openssl/hkdf_openssl.cc (right): https://codereview.chromium.org/803173010/diff/1/content/child/webcrypto/open... content/child/webcrypto/openssl/hkdf_openssl.cc:74: GetDigest(algorithm.hkdfParams()->hash().id()); On 2015/01/06 22:53:42, nharper wrote: > On 2014/12/23 23:13:36, eroman wrote: > > It is guaranteed to already be a digest, since the Blink Layer ensures it is a > > HashAlgorithmIdentifier when parsing. > > This same check is done in the HMAC code - any objection to leaving it here? Nope. Although we should be consistent about when it is Unexpected vs NotSupported. We should change the others to be NotSupported as well. https://codereview.chromium.org/803173010/diff/20001/content/child/webcrypto/... File content/child/webcrypto/openssl/hkdf_openssl.cc (right): https://codereview.chromium.org/803173010/diff/20001/content/child/webcrypto/... content/child/webcrypto/openssl/hkdf_openssl.cc:1: // Copyright 2014 The Chromium Authors. All rights reserved. 2015 perhaps? https://codereview.chromium.org/803173010/diff/20001/content/child/webcrypto/... content/child/webcrypto/openssl/hkdf_openssl.cc:45: *key = blink::WebCryptoKey::create( Simplify to: return CreateWebCryptoSecretKey(...); https://codereview.chromium.org/803173010/diff/20001/content/child/webcrypto/... content/child/webcrypto/openssl/hkdf_openssl.cc:58: // TODO(nharper): The spec might change so that an OperationError should be Link to the spec bug if you keep this comment. That said: (1) This TODO would be better if it were in Status::ErrorHkdfDeriveBitsLengthNotSpecified(), since that is where the code change will be. (2) A bug on crbug.com would alternately be sufficient https://codereview.chromium.org/803173010/diff/20001/content/child/webcrypto/... content/child/webcrypto/openssl/hkdf_openssl.cc:64: // "If the hash member of normalizedAlgorithm does not describe a recognized It is unclear where this quote comes from (the spec). Given the changing nature of the spec, I would omit this comment altogether (should be able to look this up in the spec if needing to validate behaviors). https://codereview.chromium.org/803173010/diff/20001/content/child/webcrypto/... content/child/webcrypto/openssl/hkdf_openssl.cc:78: const std::vector<uint8_t>* raw_key = Please use const reference rather than pointers throughout: const std::vector<uint8_t>& raw_key = ...; https://codereview.chromium.org/803173010/diff/20001/content/child/webcrypto/... content/child/webcrypto/openssl/hkdf_openssl.cc:80: const blink::WebVector<unsigned char>* salt = Rather than making these aliases, I think it would be cleaner to have this near the start: const blink::.... params = algorithm.hkdfParams(); And then use expressions like: params->hash() params->salt() etc.. https://codereview.chromium.org/803173010/diff/20001/content/child/webcrypto/... content/child/webcrypto/openssl/hkdf_openssl.cc:84: if (!HKDF(derived_bytes->data(), derived_bytes_len, digest_algorithm, I don't believe we can use std::vector::data(), as that is new to c++11. I recall having trouble compiling such code on Windows. https://codereview.chromium.org/803173010/diff/20001/content/child/webcrypto/... File content/child/webcrypto/status.h (right): https://codereview.chromium.org/803173010/diff/20001/content/child/webcrypto/... content/child/webcrypto/status.h:262: // No length parameter was provided for deriveBits in hkdf. nit: unclear what deriveBits references (as it is a not a function name at this layer). In javascript it is a method (crypto.subtle.deriveBits()), however it is not possible to pass no length to it so the comment doesn't make sense interpreted that way either. Howe about: No length parameter was provided for HKDF's Derive Bits operation. (DeriveBits is the "operation", which backs both deriveBits() implementation and deriveKey()).
https://codereview.chromium.org/803173010/diff/20001/content/child/webcrypto/... File content/child/webcrypto/openssl/hkdf_openssl.cc (right): https://codereview.chromium.org/803173010/diff/20001/content/child/webcrypto/... content/child/webcrypto/openssl/hkdf_openssl.cc:92: } Also, the HKDF spec is woefully underwritten. However there is an additional concern here that the input is in bits, but the output is in bytes. For ECDH which similar has this style, I opted to set the unused bits to zero: https://www.w3.org/Bugs/Public/show_bug.cgi?id=27402 Also need tests for this case.
https://codereview.chromium.org/803173010/diff/1/content/child/webcrypto/open... File content/child/webcrypto/openssl/hkdf_openssl.cc (right): https://codereview.chromium.org/803173010/diff/1/content/child/webcrypto/open... content/child/webcrypto/openssl/hkdf_openssl.cc:74: GetDigest(algorithm.hkdfParams()->hash().id()); On 2015/01/07 00:23:42, eroman wrote: > On 2015/01/06 22:53:42, nharper wrote: > > On 2014/12/23 23:13:36, eroman wrote: > > > It is guaranteed to already be a digest, since the Blink Layer ensures it is > a > > > HashAlgorithmIdentifier when parsing. > > > > This same check is done in the HMAC code - any objection to leaving it here? > > Nope. Although we should be consistent about when it is Unexpected vs > NotSupported. We should change the others to be NotSupported as well. It looks like sha_openssl.cc is the only one (code that calls GetDigest() in src/content/child/webcrypto/openssl) that returns Status::ErrorUnexpected() instead of Status::ErrorUnsupported(). I'll change that in a separate cl. https://codereview.chromium.org/803173010/diff/20001/content/child/webcrypto/... File content/child/webcrypto/openssl/hkdf_openssl.cc (right): https://codereview.chromium.org/803173010/diff/20001/content/child/webcrypto/... content/child/webcrypto/openssl/hkdf_openssl.cc:1: // Copyright 2014 The Chromium Authors. All rights reserved. On 2015/01/07 00:23:42, eroman wrote: > 2015 perhaps? Done. Is there a doc that describes the appropriate style for copyright/license headers in chromium? https://codereview.chromium.org/803173010/diff/20001/content/child/webcrypto/... content/child/webcrypto/openssl/hkdf_openssl.cc:45: *key = blink::WebCryptoKey::create( On 2015/01/07 00:23:42, eroman wrote: > Simplify to: > > return CreateWebCryptoSecretKey(...); Done. https://codereview.chromium.org/803173010/diff/20001/content/child/webcrypto/... content/child/webcrypto/openssl/hkdf_openssl.cc:58: // TODO(nharper): The spec might change so that an OperationError should be On 2015/01/07 00:23:43, eroman wrote: > Link to the spec bug if you keep this comment. > > That said: > (1) This TODO would be better if it were in > Status::ErrorHkdfDeriveBitsLengthNotSpecified(), since that is where the code > change will be. > > (2) A bug on http://crbug.com would alternately be sufficient I've moved the comment to the appropriate part of status.cc and added a link to the spec bug. https://codereview.chromium.org/803173010/diff/20001/content/child/webcrypto/... content/child/webcrypto/openssl/hkdf_openssl.cc:64: // "If the hash member of normalizedAlgorithm does not describe a recognized On 2015/01/07 00:23:42, eroman wrote: > It is unclear where this quote comes from (the spec). Given the changing nature > of the spec, I would omit this comment altogether (should be able to look this > up in the spec if needing to validate behaviors). Done. https://codereview.chromium.org/803173010/diff/20001/content/child/webcrypto/... content/child/webcrypto/openssl/hkdf_openssl.cc:78: const std::vector<uint8_t>* raw_key = On 2015/01/07 00:23:42, eroman wrote: > Please use const reference rather than pointers throughout: > > const std::vector<uint8_t>& raw_key = ...; Done. https://codereview.chromium.org/803173010/diff/20001/content/child/webcrypto/... content/child/webcrypto/openssl/hkdf_openssl.cc:80: const blink::WebVector<unsigned char>* salt = On 2015/01/07 00:23:42, eroman wrote: > Rather than making these aliases, I think it would be cleaner to have this near > the start: > > const blink::.... params = algorithm.hkdfParams(); > > > And then use expressions like: > > params->hash() > params->salt() > > etc.. Done. https://codereview.chromium.org/803173010/diff/20001/content/child/webcrypto/... content/child/webcrypto/openssl/hkdf_openssl.cc:84: if (!HKDF(derived_bytes->data(), derived_bytes_len, digest_algorithm, On 2015/01/07 00:23:43, eroman wrote: > I don't believe we can use std::vector::data(), as that is new to c++11. I > recall having trouble compiling such code on Windows. I've removed use of std::vector::data(), although the code is less clean now. https://codereview.chromium.org/803173010/diff/20001/content/child/webcrypto/... content/child/webcrypto/openssl/hkdf_openssl.cc:92: } On 2015/01/07 01:00:59, eroman wrote: > Also, the HKDF spec is woefully underwritten. However there is an additional > concern here that the input is in bits, but the output is in bytes. > > For ECDH which similar has this style, I opted to set the unused bits to zero: > > https://www.w3.org/Bugs/Public/show_bug.cgi?id=27402 > > Also need tests for this case. I'm truncating the bits now. I'll add a test for bit length % 8 != 0 as part of https://codereview.chromium.org/822083003/. https://codereview.chromium.org/803173010/diff/20001/content/child/webcrypto/... File content/child/webcrypto/status.h (right): https://codereview.chromium.org/803173010/diff/20001/content/child/webcrypto/... content/child/webcrypto/status.h:262: // No length parameter was provided for deriveBits in hkdf. On 2015/01/07 00:23:43, eroman wrote: > nit: unclear what deriveBits references (as it is a not a function name at this > layer). In javascript it is a method (crypto.subtle.deriveBits()), however it is > not possible to pass no length to it so the comment doesn't make sense > interpreted that way either. > > Howe about: > > No length parameter was provided for HKDF's Derive Bits operation. > > > (DeriveBits is the "operation", which backs both deriveBits() implementation and > deriveKey()). Done.
https://codereview.chromium.org/803173010/diff/20001/content/child/webcrypto/... File content/child/webcrypto/openssl/hkdf_openssl.cc (right): https://codereview.chromium.org/803173010/diff/20001/content/child/webcrypto/... content/child/webcrypto/openssl/hkdf_openssl.cc:1: // Copyright 2014 The Chromium Authors. All rights reserved. On 2015/01/08 01:31:24, nharper wrote: > On 2015/01/07 00:23:42, eroman wrote: > > 2015 perhaps? > > Done. Is there a doc that describes the appropriate style for copyright/license > headers in chromium? Search for File Headers in: http://www.chromium.org/developers/coding-style https://codereview.chromium.org/803173010/diff/40001/content/child/webcrypto/... File content/child/webcrypto/nss/util_nss.cc (right): https://codereview.chromium.org/803173010/diff/40001/content/child/webcrypto/... content/child/webcrypto/nss/util_nss.cc:105: // HKDF is only being imlemented for boringssl. BoringSSL is how it is referred to elsewhere in code I believe. https://codereview.chromium.org/803173010/diff/40001/content/child/webcrypto/... File content/child/webcrypto/openssl/hkdf_openssl.cc (right): https://codereview.chromium.org/803173010/diff/40001/content/child/webcrypto/... content/child/webcrypto/openssl/hkdf_openssl.cc:73: const uint8_t* raw_key_voidp = raw_key.size() ? &raw_key[0] : nullptr; (1) This is called "voidp" however it is not a void pointer (2) raw_key.size() --> !raw_key.empty() is clearer I think (3) other code in webcrypto is using NULL not nullptr, can you match that style? (switching everything to nullptr should be a separate refactor) https://codereview.chromium.org/803173010/diff/40001/content/child/webcrypto/... content/child/webcrypto/openssl/hkdf_openssl.cc:74: if (!HKDF(&(*derived_bytes)[0], derived_bytes_len, digest_algorithm, derived_bytes might be empty, so dereferncing element 0 is not correct (certainly was on advantage of using .data(). (1) Add a test deriving 0 bits (2) Maybe things have changed and .data() is supported on all platforms now? Try sending some jobs to trybots, would certainly be cleaner using .data() in these places. https://codereview.chromium.org/803173010/diff/40001/content/child/webcrypto/... File content/child/webcrypto/status.cc (right): https://codereview.chromium.org/803173010/diff/40001/content/child/webcrypto/... content/child/webcrypto/status.cc:339: // (https://www.w3.org/Bugs/Public/show_bug.cgi?id=27771) If the spec is nit: The last sentence seems obvious, suggest removing it. https://codereview.chromium.org/803173010/diff/40001/content/child/webcrypto/... content/child/webcrypto/status.cc:342: "Length must be specified for deriveBits."); Can you suggest something better for this wording? When reading this it looks like the deriveBits() method, however the length parameter is mandatory for deriveBits(). This is the same discussion we had on the comment. Perhaps using similar language to the comment (although it won't make much sense to callers who haven't read the spec)?
https://codereview.chromium.org/803173010/diff/40001/content/child/webcrypto/... File content/child/webcrypto/nss/util_nss.cc (right): https://codereview.chromium.org/803173010/diff/40001/content/child/webcrypto/... content/child/webcrypto/nss/util_nss.cc:105: // HKDF is only being imlemented for boringssl. On 2015/01/08 02:39:43, eroman wrote: > BoringSSL is how it is referred to elsewhere in code I believe. Done. That's how it's capitalized in the rest of the code. https://codereview.chromium.org/803173010/diff/40001/content/child/webcrypto/... File content/child/webcrypto/openssl/hkdf_openssl.cc (right): https://codereview.chromium.org/803173010/diff/40001/content/child/webcrypto/... content/child/webcrypto/openssl/hkdf_openssl.cc:73: const uint8_t* raw_key_voidp = raw_key.size() ? &raw_key[0] : nullptr; On 2015/01/08 02:39:43, eroman wrote: > (1) This is called "voidp" however it is not a void pointer > (2) raw_key.size() --> !raw_key.empty() is clearer I think > (3) other code in webcrypto is using NULL not nullptr, can you match that style? > (switching everything to nullptr should be a separate refactor) 1) it's now raw_key_p 2) I used raw_key.empty() and switched the 2nd and 3rd args to the ternary to make it clearer 3) switched to NULL. (After enough time in google3, I finally got trained to use nullptr instead of NULL, and now I have to unlearn that.) https://codereview.chromium.org/803173010/diff/40001/content/child/webcrypto/... content/child/webcrypto/openssl/hkdf_openssl.cc:74: if (!HKDF(&(*derived_bytes)[0], derived_bytes_len, digest_algorithm, On 2015/01/08 02:39:43, eroman wrote: > derived_bytes might be empty, so dereferncing element 0 is not correct > (certainly was on advantage of using .data(). > > (1) Add a test deriving 0 bits > (2) Maybe things have changed and .data() is supported on all platforms now? > Try sending some jobs to trybots, would certainly be cleaner using .data() in > these places. > Good catch on derived_bytes possibly being empty. I changed it to use the same trick as raw_key_p (I copied that trick from the hmac code). I'd like to leave it using this trick for now, and later I can refactor this (and hmac, and possibly other places if there are others) to use .data() if it's supported. https://codereview.chromium.org/803173010/diff/40001/content/child/webcrypto/... File content/child/webcrypto/status.cc (right): https://codereview.chromium.org/803173010/diff/40001/content/child/webcrypto/... content/child/webcrypto/status.cc:339: // (https://www.w3.org/Bugs/Public/show_bug.cgi?id=27771) If the spec is On 2015/01/08 02:39:43, eroman wrote: > nit: The last sentence seems obvious, suggest removing it. Done. https://codereview.chromium.org/803173010/diff/40001/content/child/webcrypto/... content/child/webcrypto/status.cc:342: "Length must be specified for deriveBits."); On 2015/01/08 02:39:43, eroman wrote: > Can you suggest something better for this wording? When reading this it looks > like the deriveBits() method, however the length parameter is mandatory for > deriveBits(). > > This is the same discussion we had on the comment. > > Perhaps using similar language to the comment (although it won't make much sense > to callers who haven't read the spec)? How about "No length was specified for the HKDF Derive Bits operation." I also doubt that a caller would ever see this, because it would only happen in DeriveKey if GetKeyLength returns a successful status and also sets has_length_bits to false, and it looks like none of our algorithms do that.
https://codereview.chromium.org/803173010/diff/40001/content/child/webcrypto/... File content/child/webcrypto/status.cc (right): https://codereview.chromium.org/803173010/diff/40001/content/child/webcrypto/... content/child/webcrypto/status.cc:342: "Length must be specified for deriveBits."); On 2015/01/09 18:35:55, nharper wrote: > On 2015/01/08 02:39:43, eroman wrote: > > Can you suggest something better for this wording? When reading this it looks > > like the deriveBits() method, however the length parameter is mandatory for > > deriveBits(). > > > > This is the same discussion we had on the comment. > > > > Perhaps using similar language to the comment (although it won't make much > sense > > to callers who haven't read the spec)? > > How about "No length was specified for the HKDF Derive Bits operation." I also > doubt that a caller would ever see this, because it would only happen in > DeriveKey if GetKeyLength returns a successful status and also sets > has_length_bits to false, and it looks like none of our algorithms do that. Can't you reproduce by calling deriveKey() on HKDF to derive an HKDF key? Can you add a test that exercises this? https://codereview.chromium.org/803173010/diff/60001/content/child/webcrypto/... File content/child/webcrypto/openssl/hkdf_openssl.cc (right): https://codereview.chromium.org/803173010/diff/60001/content/child/webcrypto/... content/child/webcrypto/openssl/hkdf_openssl.cc:73: const uint8_t* raw_key_p = raw_key.empty() ? NULL : &raw_key[0]; I don't like "_p". I would prefer "_ptr" if you stick to this naming style. Or alternately "_start" or "_bytes" or "_begin" https://codereview.chromium.org/803173010/diff/60001/content/child/webcrypto/... content/child/webcrypto/openssl/hkdf_openssl.cc:75: derived_bytes->empty() ? NULL : &(*derived_bytes)[0]; optional: &derive_bytes->front() may be easier to read. https://codereview.chromium.org/803173010/diff/60001/content/child/webcrypto/... content/child/webcrypto/openssl/hkdf_openssl.cc:105: } You are missing the implementing of get key length operation.
https://codereview.chromium.org/803173010/diff/60001/content/child/webcrypto/... File content/child/webcrypto/openssl/hkdf_openssl.cc (right): https://codereview.chromium.org/803173010/diff/60001/content/child/webcrypto/... content/child/webcrypto/openssl/hkdf_openssl.cc:73: const uint8_t* raw_key_p = raw_key.empty() ? NULL : &raw_key[0]; On 2015/01/09 19:31:53, eroman wrote: > I don't like "_p". > I would prefer "_ptr" if you stick to this naming style. > > Or alternately "_start" or "_bytes" or "_begin" Done. https://codereview.chromium.org/803173010/diff/60001/content/child/webcrypto/... content/child/webcrypto/openssl/hkdf_openssl.cc:75: derived_bytes->empty() ? NULL : &(*derived_bytes)[0]; On 2015/01/09 19:31:53, eroman wrote: > optional: &derive_bytes->front() may be easier to read. I thought that there was some reason why ->front() didn't work, but I tried it again and it works. (I agree it's easier to read.) https://codereview.chromium.org/803173010/diff/60001/content/child/webcrypto/... content/child/webcrypto/openssl/hkdf_openssl.cc:105: } On 2015/01/09 19:31:53, eroman wrote: > You are missing the implementing of get key length operation. Done.
Updated to match the change in https://codereview.chromium.org/789733009/
LGTM. In the description please link to the LayoutTests changelist. Since a CL without tests would be a no-go :) https://codereview.chromium.org/803173010/diff/140001/content/child/webcrypto... File content/child/webcrypto/openssl/hkdf_openssl.cc (right): https://codereview.chromium.org/803173010/diff/140001/content/child/webcrypto... content/child/webcrypto/openssl/hkdf_openssl.cc:47: blink::WebCryptoAlgorithmIdHkdf), did you run git cl format? not sure this is correct.
https://codereview.chromium.org/803173010/diff/140001/content/child/webcrypto... File content/child/webcrypto/openssl/hkdf_openssl.cc (right): https://codereview.chromium.org/803173010/diff/140001/content/child/webcrypto... content/child/webcrypto/openssl/hkdf_openssl.cc:76: if (!HKDF(derived_bytes_ptr, derived_bytes_len, digest_algorithm, @davidben: Could you review the use of BoringSSL (which is limited to this file).
https://codereview.chromium.org/803173010/diff/140001/content/child/webcrypto... File content/child/webcrypto/openssl/hkdf_openssl.cc (right): https://codereview.chromium.org/803173010/diff/140001/content/child/webcrypto... content/child/webcrypto/openssl/hkdf_openssl.cc:47: blink::WebCryptoAlgorithmIdHkdf), On 2015/01/09 21:58:07, eroman wrote: > did you run git cl format? not sure this is correct. I did run it. I agree that it looks weird. (I tried changing it to be a 4-space indent past the previous line, and git cl format changed it back to this.)
davidben@chromium.org changed reviewers: + davidben@chromium.org
content/child/webcrypto/openssl/hkdf_openssl.cc lgtm with two comments. https://codereview.chromium.org/803173010/diff/140001/content/child/webcrypto... File content/child/webcrypto/openssl/hkdf_openssl.cc (right): https://codereview.chromium.org/803173010/diff/140001/content/child/webcrypto... content/child/webcrypto/openssl/hkdf_openssl.cc:55: std::vector<uint8_t>* derived_bytes) const override { This function should have a crypto::OpenSSLErrStackTracer at the top. https://codereview.chromium.org/803173010/diff/140001/content/child/webcrypto... content/child/webcrypto/openssl/hkdf_openssl.cc:82: ERR_GET_REASON(error) == HKDF_R_OUTPUT_TOO_LARGE) Style nit: Since the conditional is multiple lines, I think we put curlies in this case. (clang-format won't do anything about adding or removing curlies.)
https://codereview.chromium.org/803173010/diff/140001/content/child/webcrypto... File content/child/webcrypto/openssl/hkdf_openssl.cc (right): https://codereview.chromium.org/803173010/diff/140001/content/child/webcrypto... content/child/webcrypto/openssl/hkdf_openssl.cc:55: std::vector<uint8_t>* derived_bytes) const override { On 2015/01/09 22:54:46, David Benjamin wrote: > This function should have a crypto::OpenSSLErrStackTracer at the top. Done. https://codereview.chromium.org/803173010/diff/140001/content/child/webcrypto... content/child/webcrypto/openssl/hkdf_openssl.cc:82: ERR_GET_REASON(error) == HKDF_R_OUTPUT_TOO_LARGE) On 2015/01/09 22:54:46, David Benjamin wrote: > Style nit: Since the conditional is multiple lines, I think we put curlies in > this case. (clang-format won't do anything about adding or removing curlies.) Done. I'm a fan of curlies.
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/803173010/160001
Message was sent while issue was closed.
Committed patchset #9 (id:160001)
Message was sent while issue was closed.
Patchset 9 (id:??) landed as https://crrev.com/6510317950146a4af1df4bac9bf4c52212b92766 Cr-Commit-Position: refs/heads/master@{#311291} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
