Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
25906002:
[webcrypto] Add JWK import for HMAC and AES-CBC key. (Closed)
|
Created:
7 years, 2 months ago by padolph Modified:
6 years, 11 months ago CC:
chromium-reviews, joi+watch-content_chromium.org, darin-cc_chromium.org, jam, jochen (gone - plz use gerrit) Base URL:
https://chromium.googlesource.com/chromium/src.git@master Visibility:
Public. |
Description[webcrypto] Add JWK import for HMAC and AES-CBC key.
BUG=245025
TEST=content_unittests --gtest_filter="WebCryptoImpl*"
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=238574
Patch Set 1 #Patch Set 2 : Removed some DCHECK's and TODO's. #
Total comments: 8
Patch Set 3 : fixes for rsleevi: new input/JWK collision rules and related refactoring #Patch Set 4 : fixes for rsleevi: new input/JWK collision rules and related refactoring #Patch Set 5 : Rebase #Patch Set 6 : re-upload after rebase mistake #Patch Set 7 : re-upload #
Total comments: 51
Patch Set 8 : rebase #Patch Set 9 : fixes for eroman #
Total comments: 49
Patch Set 10 : fixes for eroman plus more tests #
Total comments: 10
Patch Set 11 : fixes for eroman and rsleevi, plus more tests #Patch Set 12 : rebase #
Total comments: 35
Patch Set 13 : use WebKit::WebCryptoKey::createNull() throughout #Patch Set 14 : fixes for eroman #
Total comments: 14
Patch Set 15 : rebase #Patch Set 16 : fixes for eroman, plus additional validation of AES key size #
Total comments: 7
Patch Set 17 : removed JWK AES alg key length validation, replaced with TODO #
Total comments: 61
Patch Set 18 : fixes for eroman and aproskuryakov #
Total comments: 5
Patch Set 19 : rebase #
Total comments: 1
Patch Set 20 : fixes for eroman #
Total comments: 3
Patch Set 21 : added comment reference to proposed JWK changes bug #Patch Set 22 : rebase #Patch Set 23 : namespaced util functions and renamed Start function #Patch Set 24 : fixed OpenSSL build break caused by new namespacing #Patch Set 25 : rebase #Patch Set 26 : minor comment-only changes related to rebase #Patch Set 27 : rebase #Patch Set 28 : rebase #Patch Set 29 : bugfix in top level WebCryptoImpl::importKey #
Total comments: 3
Patch Set 30 : fix for rsleevi #Patch Set 31 : fixed build error on windows #Patch Set 32 : added CONTENT_EXPORT to util functions used by unit tests, to fix unit test linkage problem #Patch Set 33 : missed a change from last upload #Messages
Total messages: 63 (0 generated)
Eric, This change requires minor additions to WebCryptoAlgorithmId in WebCryptoAlgorithm.h, but that file does not seem to be tracked in my local repo. How can I submit changes to this file?
On 2013/10/03 18:29:30, padolph wrote: > Eric, > > This change requires minor additions to WebCryptoAlgorithmId in > WebCryptoAlgorithm.h, but that file does not seem to be tracked in my local > repo. How can I submit changes to this file? Paul, by not tracked in your local repo, are you using the top-level chromium/src/ repo or the repo in chromium/src/third_party/WebKit/? Chromium uses git submodules which populate different levels of the tree, so if you modify this file and try to add it within chromium/src/third_party/WebKit/, then you should hit the right repo.
On 2013/10/04 21:07:25, Bryan Eyler wrote: > On 2013/10/03 18:29:30, padolph wrote: > > Eric, > > > > This change requires minor additions to WebCryptoAlgorithmId in > > WebCryptoAlgorithm.h, but that file does not seem to be tracked in my local > > repo. How can I submit changes to this file? > > Paul, by not tracked in your local repo, are you using the top-level > chromium/src/ repo or the repo in chromium/src/third_party/WebKit/? Chromium > uses git submodules which populate different levels of the tree, so if you > modify this file and try to add it within chromium/src/third_party/WebKit/, then > you should hit the right repo. I'm lost here. It appears that my .../chromium/src/third_party/WebKit/public/platform directory is not tracked at all. $ cd chromium/src/third_party/WebKit/public/platform; git status # On branch jwk2 # Your branch is ahead of 'master' by 11 commits. # nothing to commit (working directory clean) $ touch foo; git status # On branch jwk2 # Your branch is ahead of 'master' by 11 commits. # nothing to commit (working directory clean) How do I work with submodules? Is there some documentation I can look at?
I've taken a quick scan here. I think Eric should be the one to review this, since as you note, it touches on the Blink side of things as well. If you're having to touch something in third_party/WebKit, it means you've got a two-sided change to land - the Blink bits [presumably, first], then the Chromium bits. This is because they're still currently in two repositories and separately versioned. Eric can help with this, or advise on a design that may not make this necessary. My initial comments should not fundamentally alter the design too much, but Eric's comments may. https://codereview.chromium.org/25906002/diff/4001/content/renderer/webcrypto... File content/renderer/webcrypto/webcrypto_impl.cc (right): https://codereview.chromium.org/25906002/diff/4001/content/renderer/webcrypto... content/renderer/webcrypto/webcrypto_impl.cc:198: // .to(my_map); Why is this syntactic sugar needed, when my_map["a"] = 1 is 'just as' good. I note 'just as' because it "technically" is equiv to my_map.insert("a", int()).second->second = 1, but in terms of compiler generated code, my experience is it all collapses the same, and avoids the added syntactic sugars. https://codereview.chromium.org/25906002/diff/4001/content/renderer/webcrypto... content/renderer/webcrypto/webcrypto_impl.cc:372: // http://self-issued.info/docs/draft-ietf-jose-json-web-key.html (JOSE) Let's refer to a canonical draft URL at the IETF, rather than Mike's homepage. http://tools.ietf.org/html/draft-ietf-jose-json-web-key-16 (for example) https://codereview.chromium.org/25906002/diff/4001/content/renderer/webcrypto... content/renderer/webcrypto/webcrypto_impl.cc:473: // keyUsage: Use JWK value if present, otherwise use input value These are all things that should be in the spec before we ship something. I'm updating it this weekend, but per the WebCrypto mailing list, if a JWK value is present, and it is inconsistent with the input value, it's an error and the op fails. We should be sure to implement that algorithm. http://lists.w3.org/Archives/Public/public-webcrypto/2013Sep/0042.html https://codereview.chromium.org/25906002/diff/4001/content/renderer/webcrypto... content/renderer/webcrypto/webcrypto_impl.cc:475: std::string jsonStr(key_data, key_data + key_data_size); json_str However, you don't need to use std::string, but can use base::StringPiece instead to avoid the copy.
https://codereview.chromium.org/25906002/diff/4001/content/renderer/webcrypto... File content/renderer/webcrypto/webcrypto_impl.cc (right): https://codereview.chromium.org/25906002/diff/4001/content/renderer/webcrypto... content/renderer/webcrypto/webcrypto_impl.cc:198: // .to(my_map); On 2013/10/04 23:19:11, Ryan Sleevi wrote: > Why is this syntactic sugar needed, when > > my_map["a"] = 1 is 'just as' good. > > I note 'just as' because it "technically" is equiv to my_map.insert("a", > int()).second->second = 1, but in terms of compiler generated code, my > experience is it all collapses the same, and avoids the added syntactic sugars. Done. https://codereview.chromium.org/25906002/diff/4001/content/renderer/webcrypto... content/renderer/webcrypto/webcrypto_impl.cc:372: // http://self-issued.info/docs/draft-ietf-jose-json-web-key.html (JOSE) On 2013/10/04 23:19:11, Ryan Sleevi wrote: > Let's refer to a canonical draft URL at the IETF, rather than Mike's homepage. > > http://tools.ietf.org/html/draft-ietf-jose-json-web-key-16 (for example) Done. https://codereview.chromium.org/25906002/diff/4001/content/renderer/webcrypto... content/renderer/webcrypto/webcrypto_impl.cc:473: // keyUsage: Use JWK value if present, otherwise use input value On 2013/10/04 23:19:11, Ryan Sleevi wrote: > These are all things that should be in the spec before we ship something. > > I'm updating it this weekend, but per the WebCrypto mailing list, if a JWK value > is present, and it is inconsistent with the input value, it's an error and the > op fails. We should be sure to implement that algorithm. > > http://lists.w3.org/Archives/Public/public-webcrypto/2013Sep/0042.html The other parameters are easy, but can you please define "consistent" for algorithm? I assume this is just the general algorithm type, plus the inner hash type if the algorithm has one, but does not check consistency of other parameters like key length, exponent, etc. I'm concerned this requires the user knowing ahead of time the exact details of the JWK he is importing which seems like a big burden, especially if say it is inside a JWE received from a remote site. For example, the client does not normally have to know if a public key from a remote site is RSAES-PKCS1-v1_5 or RSA-OEAP to use it to encrypt something. Is that the kind of consistency my code needs to be aware of, so the API user can guess and use either one as the input algorithm? https://codereview.chromium.org/25906002/diff/4001/content/renderer/webcrypto... content/renderer/webcrypto/webcrypto_impl.cc:475: std::string jsonStr(key_data, key_data + key_data_size); On 2013/10/04 23:19:11, Ryan Sleevi wrote: > json_str > > However, you don't need to use std::string, but can use base::StringPiece > instead to avoid the copy. Done.
Paul, could you re-upload this change? It looks like the last upload failed, since when I try to view diffs it says: error: old chunk mismatch
On 2013/10/12 01:00:15, eroman wrote: > Paul, could you re-upload this change? It looks like the last upload failed, > since when I try to view diffs it says: > > error: old chunk mismatch Done.
Thanks Paul! https://codereview.chromium.org/25906002/diff/168001/content/renderer/webcryp... File content/renderer/webcrypto/webcrypto_impl.cc (right): https://codereview.chromium.org/25906002/diff/168001/content/renderer/webcryp... content/renderer/webcrypto/webcrypto_impl.cc:24: // FIXME! Need WebKit::WebCryptoAlgorithmIdNone There are other ways of accomplishing this that I think will be easier to maintain. Adding the possibility for an AlgorithmIdNone in the blink API means that every location which uses algorithm must check for it, when in practice the Algorithm should always be valid. I believe the situation you are running into here is that in the Blink API for ImportKey() I made algorithm a required parameter, whereas in the WebCrypto spec it is optional. I think the best course of action is to either turn that into an optional parameter (say by making it a const pointer which has the possibility of being NULL), or by adding a separate virtual method for importing JWK keys, in which the Algorithm is optional (and leaving ImportKey() as-is, with a non-optional Algorithm). https://codereview.chromium.org/25906002/diff/168001/content/renderer/webcryp... content/renderer/webcrypto/webcrypto_impl.cc:28: // TODO(padolph) Similar Create*Algorithm() methods are in What do you think about a renderer/webcrypto/webcrypto_util.{h,cc} file? Could contain these functions, as well as ShrinkBuffer(). OK to leave them in the one file for this changelist. https://codereview.chromium.org/25906002/diff/168001/content/renderer/webcryp... content/renderer/webcrypto/webcrypto_impl.cc:38: WebKit::WebCryptoAlgorithmId hashId; use hacker_style_variable_naming for chromium code. https://codereview.chromium.org/25906002/diff/168001/content/renderer/webcryp... content/renderer/webcrypto/webcrypto_impl.cc:56: return WebKit::WebCryptoAlgorithm::adoptParamsAndCreate( Please add a default case to the switch above (perhaps a CHECK or NOTREACHED()), to guard against an uninitialized value of hashId being used. https://codereview.chromium.org/25906002/diff/168001/content/renderer/webcryp... content/renderer/webcrypto/webcrypto_impl.cc:97: bool Base64DecodeUrlSafe(const std::string& input, std::string* output) { Please provide a comment pointing to the definition of "Base64url Encoding" in the jose spec. Or explaining that this is for decoding Base64 URLs where the padding has been omitted, and certain characters have been replaced. This is the paragraph I found in jose referring to it: Base64url Encoding Base64 encoding using the URL- and filename-safe character set defined in Section 5 of RFC 4648 [RFC4648], with all trailing '=' characters omitted (as permitted by Section 3.2). (See Appendix C of [JWS] for notes on implementing base64url encoding without padding.) https://codereview.chromium.org/25906002/diff/168001/content/renderer/webcryp... content/renderer/webcrypto/webcrypto_impl.cc:99: std::replace(base64EncodedText.begin(), base64EncodedText.end(), '-', '+'); Are these transformations enumerated int he JOSE spec? https://codereview.chromium.org/25906002/diff/168001/content/renderer/webcryp... content/renderer/webcrypto/webcrypto_impl.cc:105: // Identifiers for all JWK "alg" (algorithm) values handled by this code. The There is enough JWK stuff, that I suggest moving this code into its own file. Perhaps renderer/webcrypto/jwk.{h,cc} https://codereview.chromium.org/25906002/diff/168001/content/renderer/webcryp... content/renderer/webcrypto/webcrypto_impl.cc:106: // "enum trick" is used to force int type, so a user type is not required in Not sure I know what the "enum trick" refers to. You mean treating these just as integer constants? In which case, I would rather give this its own type. https://codereview.chromium.org/25906002/diff/168001/content/renderer/webcryp... content/renderer/webcrypto/webcrypto_impl.cc:127: WebKit::WebCryptoAlgorithmId JwkAlgIdToWebCryptoAlgId(int jwk_algorithm_id) { This function appears to be unused https://codereview.chromium.org/25906002/diff/168001/content/renderer/webcryp... content/renderer/webcrypto/webcrypto_impl.cc:185: WebKit::WebCryptoAlgorithm CreateWebCryptoAlgorithmFromJwkAlgorithmId( I think things would be more readable to have a single map from name --> function, and directly register the algorithm creation functions in there. https://codereview.chromium.org/25906002/diff/168001/content/renderer/webcryp... content/renderer/webcrypto/webcrypto_impl.cc:217: DCHECK(false); NOTREACHED() https://codereview.chromium.org/25906002/diff/168001/content/renderer/webcryp... content/renderer/webcrypto/webcrypto_impl.cc:224: void InitJwkAlgorithmMap(StringIntMap& jwk_algorithm_map) Chromium style (unlike Blink style) does not like non-const references, a pointer should be used instead. That said, I have a different suggestion for this initialization (explained where this function is used). https://codereview.chromium.org/25906002/diff/168001/content/renderer/webcryp... content/renderer/webcrypto/webcrypto_impl.cc:228: jwk_algorithm_map["HS256" ] = kJwkAlgorithmHs256; style nit: remove the space padding (i personally prefer with the spaces, but not in line with style guide). https://codereview.chromium.org/25906002/diff/168001/content/renderer/webcryp... content/renderer/webcrypto/webcrypto_impl.cc:252: bool operator==(const WebKit::WebCryptoAlgorithm& lhs, Chromium style guide doesn't like operator overloading. Please use a function instead like AlgorithmsMatch() https://codereview.chromium.org/25906002/diff/168001/content/renderer/webcryp... content/renderer/webcrypto/webcrypto_impl.cc:262: return true; For the sake of future proofing, I would rather have the default be "false", and only returning "true" for the hard-coded cases. https://codereview.chromium.org/25906002/diff/168001/content/renderer/webcryp... content/renderer/webcrypto/webcrypto_impl.cc:274: InitJwkAlgorithmMap(jwk_algorithm_map_); I would prefer lazy initialization, since JWK may not always be used. Can use the Singleton pattern or a function-level static for this case. For instance: const StringMap& JwkAlgorithms() { static StringMap algorithms = CreateJwkAlgorithms(); return algorithms; } https://codereview.chromium.org/25906002/diff/168001/content/renderer/webcryp... content/renderer/webcrypto/webcrypto_impl.cc:419: // JSON Web Key Format (JWK) This is a good comment block, thanks! https://codereview.chromium.org/25906002/diff/168001/content/renderer/webcryp... content/renderer/webcrypto/webcrypto_impl.cc:519: const char* const foo = reinterpret_cast<const char *>(key_data); Please use a variable name other than "foo". https://codereview.chromium.org/25906002/diff/168001/content/renderer/webcryp... content/renderer/webcrypto/webcrypto_impl.cc:543: const StringIntMap::iterator pos = jwk_algorithm_map_.find(jwk_alg_value); const_iterator? https://codereview.chromium.org/25906002/diff/168001/content/renderer/webcryp... content/renderer/webcrypto/webcrypto_impl.cc:571: if (!(jwk_usage_mask & usage_mask)) Do they need to match exactly? https://codereview.chromium.org/25906002/diff/168001/content/renderer/webcryp... content/renderer/webcrypto/webcrypto_impl.cc:586: const std::vector<uint8> data(jwk_k_value.begin(), jwk_k_value.end()); Rather than copying into a std::vector<>, I believe the string's data an be cast to uint8 and passed directly to ImportKeyInternal. https://codereview.chromium.org/25906002/diff/168001/content/renderer/webcryp... content/renderer/webcrypto/webcrypto_impl.cc:588: &data[0], Is data guaranteed to have at least 1 element?
https://codereview.chromium.org/25906002/diff/168001/content/renderer/webcryp... File content/renderer/webcrypto/webcrypto_impl.cc (right): https://codereview.chromium.org/25906002/diff/168001/content/renderer/webcryp... content/renderer/webcrypto/webcrypto_impl.cc:24: // FIXME! Need WebKit::WebCryptoAlgorithmIdNone On 2013/10/22 21:49:38, eroman wrote: > There are other ways of accomplishing this that I think will be easier to > maintain. > > Adding the possibility for an AlgorithmIdNone in the blink API means that every > location which uses algorithm must check for it, when in practice the Algorithm > should always be valid. > > I believe the situation you are running into here is that in the Blink API for > ImportKey() I made algorithm a required parameter, whereas in the WebCrypto spec > it is optional. I think the best course of action is to either turn that into an > optional parameter (say by making it a const pointer which has the possibility > of being NULL), or by adding a separate virtual method for importing JWK keys, > in which the Algorithm is optional (and leaving ImportKey() as-is, with a > non-optional Algorithm). There are other formats that may have embedded algorithms (eg: SPKI, PKCS#8), so let's make it a true optional parameter, as spec'd. https://codereview.chromium.org/25906002/diff/168001/content/renderer/webcryp... content/renderer/webcrypto/webcrypto_impl.cc:252: bool operator==(const WebKit::WebCryptoAlgorithm& lhs, On 2013/10/22 21:49:38, eroman wrote: > Chromium style guide doesn't like operator overloading. Please use a function > instead like AlgorithmsMatch() Should WebKit::WebCryptoAlgorithm have an Equals() method instead? This all seems like it should be in the Blink side of things? https://codereview.chromium.org/25906002/diff/168001/content/renderer/webcryp... content/renderer/webcrypto/webcrypto_impl.cc:274: InitJwkAlgorithmMap(jwk_algorithm_map_); On 2013/10/22 21:49:38, eroman wrote: > I would prefer lazy initialization, since JWK may not always be used. Can use > the Singleton pattern or a function-level static for this case. > > For instance: > > const StringMap& JwkAlgorithms() { > static StringMap algorithms = CreateJwkAlgorithms(); > return algorithms; > } Don't use a function-level static. They're forbidden by the style guide. Feel free to through fruit at eroman for suggesting it ;) Use a base::LazyInstance(::Leaky ? depends on threading lifetimes) https://codereview.chromium.org/25906002/diff/168001/content/renderer/webcryp... content/renderer/webcrypto/webcrypto_impl.cc:571: if (!(jwk_usage_mask & usage_mask)) On 2013/10/22 21:49:38, eroman wrote: > Do they need to match exactly? No. The spec merely says that if they conflict in ways the implementation disagrees with.
https://codereview.chromium.org/25906002/diff/168001/content/renderer/webcryp... File content/renderer/webcrypto/webcrypto_impl.cc (right): https://codereview.chromium.org/25906002/diff/168001/content/renderer/webcryp... content/renderer/webcrypto/webcrypto_impl.cc:24: // FIXME! Need WebKit::WebCryptoAlgorithmIdNone On 2013/10/24 01:45:58, Ryan Sleevi wrote: > On 2013/10/22 21:49:38, eroman wrote: > > There are other ways of accomplishing this that I think will be easier to > > maintain. > > > > Adding the possibility for an AlgorithmIdNone in the blink API means that > every > > location which uses algorithm must check for it, when in practice the > Algorithm > > should always be valid. > > > > I believe the situation you are running into here is that in the Blink API for > > ImportKey() I made algorithm a required parameter, whereas in the WebCrypto > spec > > it is optional. I think the best course of action is to either turn that into > an > > optional parameter (say by making it a const pointer which has the possibility > > of being NULL), or by adding a separate virtual method for importing JWK keys, > > in which the Algorithm is optional (and leaving ImportKey() as-is, with a > > non-optional Algorithm). > > There are other formats that may have embedded algorithms (eg: SPKI, PKCS#8), so > let's make it a true optional parameter, as spec'd. I will make the necessary changes to Blink side. https://codereview.chromium.org/25906002/diff/168001/content/renderer/webcryp... content/renderer/webcrypto/webcrypto_impl.cc:252: bool operator==(const WebKit::WebCryptoAlgorithm& lhs, On 2013/10/24 01:45:58, Ryan Sleevi wrote: > On 2013/10/22 21:49:38, eroman wrote: > > Chromium style guide doesn't like operator overloading. Please use a function > > instead like AlgorithmsMatch() > > Should WebKit::WebCryptoAlgorithm have an Equals() method instead? This all > seems like it should be in the Blink side of things? It isn't obvious what equality means, so I think this is better left to the consumers rather than in Blink. To give some examples: * If optional parameters for an algorithm are missing, does equality infer defaults? * Parameter types may not match. For instance one could be key generation parameters, another the algorithm parameters. Conceptually the same "algorithm", but different objects. * Do you normalize parameters? For instance BigIntegers can contain zero-padding but still represent the same number. I am receptive to the idea, just not sure how general of a mechanism you are thinking. https://codereview.chromium.org/25906002/diff/168001/content/renderer/webcryp... content/renderer/webcrypto/webcrypto_impl.cc:274: InitJwkAlgorithmMap(jwk_algorithm_map_); On 2013/10/24 01:45:58, Ryan Sleevi wrote: > On 2013/10/22 21:49:38, eroman wrote: > > I would prefer lazy initialization, since JWK may not always be used. Can use > > the Singleton pattern or a function-level static for this case. > > > > For instance: > > > > const StringMap& JwkAlgorithms() { > > static StringMap algorithms = CreateJwkAlgorithms(); > > return algorithms; > > } > > Don't use a function-level static. They're forbidden by the style guide. > > Feel free to through fruit at eroman for suggesting it ;) > > Use a base::LazyInstance(::Leaky ? depends on threading lifetimes) My mistake! I got muddled with Blink style vs Chromium style. https://codereview.chromium.org/25906002/diff/168001/content/renderer/webcryp... content/renderer/webcrypto/webcrypto_impl.cc:571: if (!(jwk_usage_mask & usage_mask)) On 2013/10/24 01:45:58, Ryan Sleevi wrote: > On 2013/10/22 21:49:38, eroman wrote: > > Do they need to match exactly? > > No. > > The spec merely says that if they conflict in ways the implementation disagrees > with. Thanks Ryan! My understanding then is that we want to enforce that "usages" given by user to importKey() is a subset of the jwk_usages, and if not fail the import. Paul, can you confirm? If that understanding is correct, then this line has a bug, since it is testing for *any* intersection. Instead I think you want this: if (jwk_usage_mask & usage_mask != usage_mask) { // usage_mask must be a subset of jwk_usage_mask return false; } Paul: Please add a corresponding unittest.
https://codereview.chromium.org/25906002/diff/168001/content/renderer/webcryp... File content/renderer/webcrypto/webcrypto_impl.cc (right): https://codereview.chromium.org/25906002/diff/168001/content/renderer/webcryp... content/renderer/webcrypto/webcrypto_impl.cc:24: // FIXME! Need WebKit::WebCryptoAlgorithmIdNone On 2013/10/22 21:49:38, eroman wrote: > There are other ways of accomplishing this that I think will be easier to > maintain. > > Adding the possibility for an AlgorithmIdNone in the blink API means that every > location which uses algorithm must check for it, when in practice the Algorithm > should always be valid. > > I believe the situation you are running into here is that in the Blink API for > ImportKey() I made algorithm a required parameter, whereas in the WebCrypto spec > it is optional. I think the best course of action is to either turn that into an > optional parameter (say by making it a const pointer which has the possibility > of being NULL), or by adding a separate virtual method for importing JWK keys, > in which the Algorithm is optional (and leaving ImportKey() as-is, with a > non-optional Algorithm). This is addressed by your recent Blink checkin. Thanks. https://codereview.chromium.org/25906002/diff/168001/content/renderer/webcryp... content/renderer/webcrypto/webcrypto_impl.cc:28: // TODO(padolph) Similar Create*Algorithm() methods are in On 2013/10/22 21:49:38, eroman wrote: > What do you think about a renderer/webcrypto/webcrypto_util.{h,cc} file? Could > contain these functions, as well as ShrinkBuffer(). > > OK to leave them in the one file for this changelist. Done. Should the stuff in webcrypto_util.[h|cc] be in a new 'util' sub-namespace? For now I just kept them in 'content'. Please advise. https://codereview.chromium.org/25906002/diff/168001/content/renderer/webcryp... content/renderer/webcrypto/webcrypto_impl.cc:38: WebKit::WebCryptoAlgorithmId hashId; On 2013/10/22 21:49:38, eroman wrote: > use hacker_style_variable_naming for chromium code. Done. https://codereview.chromium.org/25906002/diff/168001/content/renderer/webcryp... content/renderer/webcrypto/webcrypto_impl.cc:56: return WebKit::WebCryptoAlgorithm::adoptParamsAndCreate( On 2013/10/22 21:49:38, eroman wrote: > Please add a default case to the switch above (perhaps a CHECK or NOTREACHED()), > to guard against an uninitialized value of hashId being used. Done. https://codereview.chromium.org/25906002/diff/168001/content/renderer/webcryp... content/renderer/webcrypto/webcrypto_impl.cc:97: bool Base64DecodeUrlSafe(const std::string& input, std::string* output) { On 2013/10/22 21:49:38, eroman wrote: > Please provide a comment pointing to the definition of "Base64url Encoding" in > the jose spec. Or explaining that this is for decoding Base64 URLs where the > padding has been omitted, and certain characters have been replaced. > > This is the paragraph I found in jose referring to it: > > > Base64url Encoding Base64 encoding using the URL- and filename-safe > character set defined in Section 5 of RFC 4648 [RFC4648], with all > trailing '=' characters omitted (as permitted by Section 3.2). > (See Appendix C of [JWS] for notes on implementing base64url > encoding without padding.) Will do. 'base64url' encoding is relatively common, not only for JWK. Indeed RFC4648 section 5 is a good reference and lists the exact alphabet (regarding your question below). So I'll put in a link for that. https://codereview.chromium.org/25906002/diff/168001/content/renderer/webcryp... content/renderer/webcrypto/webcrypto_impl.cc:99: std::replace(base64EncodedText.begin(), base64EncodedText.end(), '-', '+'); On 2013/10/22 21:49:38, eroman wrote: > Are these transformations enumerated int he JOSE spec? Yes, in RFC4648 Section 5. https://codereview.chromium.org/25906002/diff/168001/content/renderer/webcryp... content/renderer/webcrypto/webcrypto_impl.cc:105: // Identifiers for all JWK "alg" (algorithm) values handled by this code. The On 2013/10/22 21:49:38, eroman wrote: > There is enough JWK stuff, that I suggest moving this code into its own file. > > Perhaps renderer/webcrypto/jwk.{h,cc} Most of this code went away with the function table refactoring. No need for a separate file. https://codereview.chromium.org/25906002/diff/168001/content/renderer/webcryp... content/renderer/webcrypto/webcrypto_impl.cc:106: // "enum trick" is used to force int type, so a user type is not required in On 2013/10/22 21:49:38, eroman wrote: > Not sure I know what the "enum trick" refers to. You mean treating these just as > integer constants? > > In which case, I would rather give this its own type. This code went away with the function table refactoring. https://codereview.chromium.org/25906002/diff/168001/content/renderer/webcryp... content/renderer/webcrypto/webcrypto_impl.cc:127: WebKit::WebCryptoAlgorithmId JwkAlgIdToWebCryptoAlgId(int jwk_algorithm_id) { On 2013/10/22 21:49:38, eroman wrote: > This function appears to be unused Done. https://codereview.chromium.org/25906002/diff/168001/content/renderer/webcryp... content/renderer/webcrypto/webcrypto_impl.cc:185: WebKit::WebCryptoAlgorithm CreateWebCryptoAlgorithmFromJwkAlgorithmId( On 2013/10/22 21:49:38, eroman wrote: > I think things would be more readable to have a single map from name --> > function, and directly register the algorithm creation functions in there. Done. But since the creation functions have different signatures I had to do some template stuff to get them all into a map. Much more concise now but is a different kind of ugly. Please take a look. https://codereview.chromium.org/25906002/diff/168001/content/renderer/webcryp... content/renderer/webcrypto/webcrypto_impl.cc:217: DCHECK(false); On 2013/10/22 21:49:38, eroman wrote: > NOTREACHED() This code is now gone. https://codereview.chromium.org/25906002/diff/168001/content/renderer/webcryp... content/renderer/webcrypto/webcrypto_impl.cc:224: void InitJwkAlgorithmMap(StringIntMap& jwk_algorithm_map) On 2013/10/22 21:49:38, eroman wrote: > Chromium style (unlike Blink style) does not like non-const references, a > pointer should be used instead. > > That said, I have a different suggestion for this initialization (explained > where this function is used). This code is now gone. https://codereview.chromium.org/25906002/diff/168001/content/renderer/webcryp... content/renderer/webcrypto/webcrypto_impl.cc:228: jwk_algorithm_map["HS256" ] = kJwkAlgorithmHs256; On 2013/10/22 21:49:38, eroman wrote: > style nit: remove the space padding (i personally prefer with the spaces, but > not in line with style guide). Done. https://codereview.chromium.org/25906002/diff/168001/content/renderer/webcryp... content/renderer/webcrypto/webcrypto_impl.cc:252: bool operator==(const WebKit::WebCryptoAlgorithm& lhs, On 2013/10/24 02:35:04, eroman wrote: > On 2013/10/24 01:45:58, Ryan Sleevi wrote: > > On 2013/10/22 21:49:38, eroman wrote: > > > Chromium style guide doesn't like operator overloading. Please use a > function > > > instead like AlgorithmsMatch() > > > > Should WebKit::WebCryptoAlgorithm have an Equals() method instead? This all > > seems like it should be in the Blink side of things? > > It isn't obvious what equality means, so I think this is better left to the > consumers rather than in Blink. To give some examples: > > * If optional parameters for an algorithm are missing, does equality infer > defaults? > * Parameter types may not match. For instance one could be key generation > parameters, another the algorithm parameters. Conceptually the same "algorithm", > but different objects. > * Do you normalize parameters? For instance BigIntegers can contain > zero-padding but still represent the same number. > > I am receptive to the idea, just not sure how general of a mechanism you are > thinking. Function name changed. But I am also very confused about the definition of "inconsistency" between algorithms in this case. Also, it seems like a big burden to put on the API user to have to guess what is inside the JWK and construct a matching algorithm to get the import to work. If the JWK came from inside a JWE received from a remote source, how will the user ever know the details? https://codereview.chromium.org/25906002/diff/168001/content/renderer/webcryp... content/renderer/webcrypto/webcrypto_impl.cc:262: return true; On 2013/10/22 21:49:38, eroman wrote: > For the sake of future proofing, I would rather have the default be "false", and > only returning "true" for the hard-coded cases. Done. https://codereview.chromium.org/25906002/diff/168001/content/renderer/webcryp... content/renderer/webcrypto/webcrypto_impl.cc:274: InitJwkAlgorithmMap(jwk_algorithm_map_); On 2013/10/24 02:35:04, eroman wrote: > On 2013/10/24 01:45:58, Ryan Sleevi wrote: > > On 2013/10/22 21:49:38, eroman wrote: > > > I would prefer lazy initialization, since JWK may not always be used. Can > use > > > the Singleton pattern or a function-level static for this case. > > > > > > For instance: > > > > > > const StringMap& JwkAlgorithms() { > > > static StringMap algorithms = CreateJwkAlgorithms(); > > > return algorithms; > > > } > > > > Don't use a function-level static. They're forbidden by the style guide. > > > > Feel free to through fruit at eroman for suggesting it ;) > > > > Use a base::LazyInstance(::Leaky ? depends on threading lifetimes) > > My mistake! I got muddled with Blink style vs Chromium style. LazyInstance used. https://codereview.chromium.org/25906002/diff/168001/content/renderer/webcryp... content/renderer/webcrypto/webcrypto_impl.cc:519: const char* const foo = reinterpret_cast<const char *>(key_data); On 2013/10/22 21:49:38, eroman wrote: > Please use a variable name other than "foo". Done. https://codereview.chromium.org/25906002/diff/168001/content/renderer/webcryp... content/renderer/webcrypto/webcrypto_impl.cc:543: const StringIntMap::iterator pos = jwk_algorithm_map_.find(jwk_alg_value); On 2013/10/22 21:49:38, eroman wrote: > const_iterator? Done. https://codereview.chromium.org/25906002/diff/168001/content/renderer/webcryp... content/renderer/webcrypto/webcrypto_impl.cc:571: if (!(jwk_usage_mask & usage_mask)) On 2013/10/24 02:35:04, eroman wrote: > On 2013/10/24 01:45:58, Ryan Sleevi wrote: > > On 2013/10/22 21:49:38, eroman wrote: > > > Do they need to match exactly? > > > > No. > > > > The spec merely says that if they conflict in ways the implementation > disagrees > > with. > > Thanks Ryan! > > My understanding then is that we want to enforce that "usages" given by user to > importKey() is a subset of the jwk_usages, and if not fail the import. Paul, can > you confirm? > > If that understanding is correct, then this line has a bug, since it is testing > for *any* intersection. > > Instead I think you want this: > > if (jwk_usage_mask & usage_mask != usage_mask) { > // usage_mask must be a subset of jwk_usage_mask > return false; > } > > Paul: Please add a corresponding unittest. Yea, after thinking about it more, subset makes more sense. Done. https://codereview.chromium.org/25906002/diff/168001/content/renderer/webcryp... content/renderer/webcrypto/webcrypto_impl.cc:586: const std::vector<uint8> data(jwk_k_value.begin(), jwk_k_value.end()); On 2013/10/22 21:49:38, eroman wrote: > Rather than copying into a std::vector<>, I believe the string's data an be cast > to uint8 and passed directly to ImportKeyInternal. Done. https://codereview.chromium.org/25906002/diff/168001/content/renderer/webcryp... content/renderer/webcrypto/webcrypto_impl.cc:588: &data[0], On 2013/10/22 21:49:38, eroman wrote: > Is data guaranteed to have at least 1 element? No. I added a check for that plus a unit test. Thanks.
https://codereview.chromium.org/25906002/diff/338001/content/content_renderer... File content/content_renderer.gypi (right): https://codereview.chromium.org/25906002/diff/338001/content/content_renderer... content/content_renderer.gypi:512: 'renderer/webcrypto/webcrypto_util.cc', Please add the .h file as well. It isn't strictly necessary (as the compiler infers the header dependencies), but it is consistent with the rest of the list. https://codereview.chromium.org/25906002/diff/338001/content/renderer/webcryp... File content/renderer/webcrypto/webcrypto_impl.cc (right): https://codereview.chromium.org/25906002/diff/338001/content/renderer/webcryp... content/renderer/webcrypto/webcrypto_impl.cc:29: WebKit::WebCryptoAlgorithm BindAlgFactoryWithKeyLen() { Ah, clever! I had been thinking something dumber -- defining a separate function for each one, which then passes the key length as parameter. ... But I like your way better. Even though I don't generally like templates, I think this is a good trade-off. https://codereview.chromium.org/25906002/diff/338001/content/renderer/webcryp... content/renderer/webcrypto/webcrypto_impl.cc:35: // WebKit::WebCryptoAlgorithm func(); [optional] Could reference the function typedef name instead. https://codereview.chromium.org/25906002/diff/338001/content/renderer/webcryp... content/renderer/webcrypto/webcrypto_impl.cc:70: JwkAlgFactoryMap& Map() { return map_; } Please make this a const-reference. (For mutable parameters chromium style prefers pointers). Also for accessors chromium style uses hacker_style(), so you can call this "map()" https://codereview.chromium.org/25906002/diff/338001/content/renderer/webcryp... content/renderer/webcrypto/webcrypto_impl.cc:78: // TODO(padolph) Verify this logic is sufficient to judge algorithm nit: semicolon after closing parenthesis. https://codereview.chromium.org/25906002/diff/338001/content/renderer/webcryp... content/renderer/webcrypto/webcrypto_impl.cc:86: if (WebCryptoAlgorithmsConsistent(lhs.hmacParams()->hash(), Unfortunately getting the hash() is not this easy :( Because all the parameters are strongly typed following the normalization, there is a different type corresponding to each algorithm. for instance the parameter type could be any of: WebCryptoHmacParams WebCryptoHmacKeyParams WebCryptoRsaSsaParams WebCryptoRsaOaepParams Written as is, this could return NULL for hmacParams and subsequently crash. I suggest adding a helper function in the utils such as say: WebCryptoAlgorithm GetHashAlgorithm(const WebCryptoAgorithm&) Which will explore the correct parameter type. Could you add a regression test? https://codereview.chromium.org/25906002/diff/338001/content/renderer/webcryp... content/renderer/webcrypto/webcrypto_impl.cc:103: WebCryptoImpl::~WebCryptoImpl() {} Doesn't seem like this is needed. https://codereview.chromium.org/25906002/diff/338001/content/renderer/webcryp... content/renderer/webcrypto/webcrypto_impl.cc:372: jwk_alg_factory_map.Get().Map().find(jwk_alg_value); How about moving this logic into the singleton class itself, instead of exposing the std::map directly: WebCryptoAlgorithm webcrypto_algorithm_from_jwk = jwk_alg_factory_.Get().CreateAlgorithmFromName(jwk_alg_value); if (algorithm_from_jkw.isNull()) return false; (I also shortened the name of the singleton in the above example). https://codereview.chromium.org/25906002/diff/338001/content/renderer/webcryp... content/renderer/webcrypto/webcrypto_impl.cc:379: if (!WebCryptoAlgorithmsConsistent(webcrypto_algorithm_from_jwk, algorithm)) Now that we have made WebCryptoAlgorithm nullable, it will be possible for "algorithm.isNull()". So it is necessary to either check for that somewhere in ImportKeyJwk(), or for WebCryptoAlgorithmsConsistent() to handle it. https://codereview.chromium.org/25906002/diff/338001/content/renderer/webcryp... content/renderer/webcrypto/webcrypto_impl.cc:400: // usage_mask must be a subset of jwk_usage_mask. Please capitalize comments. https://codereview.chromium.org/25906002/diff/338001/content/renderer/webcryp... File content/renderer/webcrypto/webcrypto_impl.h (right): https://codereview.chromium.org/25906002/diff/338001/content/renderer/webcryp... content/renderer/webcrypto/webcrypto_impl.h:9: #include <string> These two headers appear unused in this file. Add them to the specific C++ file instead. https://codereview.chromium.org/25906002/diff/338001/content/renderer/webcryp... content/renderer/webcrypto/webcrypto_impl.h:22: virtual ~WebCryptoImpl(); Any reason to add the destructor definition? Since there are no member fields doesn't seem like one is particularly interesting. https://codereview.chromium.org/25906002/diff/338001/content/renderer/webcryp... File content/renderer/webcrypto/webcrypto_impl_unittest.cc (right): https://codereview.chromium.org/25906002/diff/338001/content/renderer/webcryp... content/renderer/webcrypto/webcrypto_impl_unittest.cc:664: // Invalid JSON good test cases, thanks! https://codereview.chromium.org/25906002/diff/338001/content/renderer/webcryp... content/renderer/webcrypto/webcrypto_impl_unittest.cc:731: // TODO(padolph) RSA public key bad data: please add colon after paren. https://codereview.chromium.org/25906002/diff/338001/content/renderer/webcryp... content/renderer/webcrypto/webcrypto_impl_unittest.cc:735: // Value check on e?? We will need a lot of tests to cover all the full set of cases! This is a good start though. Another one that comes to mind is case-sensitivity of properties. (Which is different from the case-insensitivity of algorithm names in webcrypto). https://codereview.chromium.org/25906002/diff/338001/content/renderer/webcryp... content/renderer/webcrypto/webcrypto_impl_unittest.cc:804: usage_mask = WebKit::WebCryptoKeyUsageEncrypt | You must have read my mind! After reading the test case above, I was about to ask for this one ;) https://codereview.chromium.org/25906002/diff/338001/content/renderer/webcryp... File content/renderer/webcrypto/webcrypto_util.cc (right): https://codereview.chromium.org/25906002/diff/338001/content/renderer/webcryp... content/renderer/webcrypto/webcrypto_util.cc:7: #include "content/renderer/webcrypto/webcrypto_util.h" This header goes first, and then the rest (the header for the .cc file has special treatment in the sorting). https://codereview.chromium.org/25906002/diff/338001/content/renderer/webcryp... content/renderer/webcrypto/webcrypto_util.cc:51: } End with a comment " // namespace" to help matching https://codereview.chromium.org/25906002/diff/338001/content/renderer/webcryp... File content/renderer/webcrypto/webcrypto_util.h (right): https://codereview.chromium.org/25906002/diff/338001/content/renderer/webcryp... content/renderer/webcrypto/webcrypto_util.h:5: #ifndef CONTENT_RENDERER_WEBCRYPTO_WEBCRYPTO_UTI_H_ typo I presume: UTI --> UTIL https://codereview.chromium.org/25906002/diff/338001/content/renderer/webcryp... content/renderer/webcrypto/webcrypto_util.h:15: namespace content { I'm not entirely sure on whether we should put this into something like webcrypto_util namespace. Certainly some of the function names in here are very generic (like "Start").. Let me get back to you on this... https://codereview.chromium.org/25906002/diff/338001/content/renderer/webcryp... content/renderer/webcrypto/webcrypto_util.h:17: const uint8* Start(const std::vector<uint8>& data); I'm not sure how I feel about this being in the header, since it feels like "unittest" code. But I suppose it is used by webcrypto_util.cc as well as the unittest so makes sense. https://codereview.chromium.org/25906002/diff/338001/content/renderer/webcryp... content/renderer/webcrypto/webcrypto_util.h:21: bool Base64DecodeUrlSafe(const std::string& input, std::string* output); Please move the function-level comments from the .cc file to the .h file. https://codereview.chromium.org/25906002/diff/338001/content/renderer/webcryp... content/renderer/webcrypto/webcrypto_util.h:33: unsigned hash_length); side comment: We may want to change the parameter names to indicate whether they are bits or bytes. The WebCrypto API is inconsistent in that most key lengths are in bits, however for Hmac key generation it actually takes it in bytes. https://codereview.chromium.org/25906002/diff/338001/content/renderer/webcryp... content/renderer/webcrypto/webcrypto_util.h:46: const std::vector<uint8>& additionalData, unsigned char tagLength); use hacker_style_naming for variables in chromium
https://codereview.chromium.org/25906002/diff/338001/content/content_renderer... File content/content_renderer.gypi (right): https://codereview.chromium.org/25906002/diff/338001/content/content_renderer... content/content_renderer.gypi:512: 'renderer/webcrypto/webcrypto_util.cc', On 2013/10/28 23:02:00, eroman wrote: > Please add the .h file as well. > > It isn't strictly necessary (as the compiler infers the header dependencies), > but it is consistent with the rest of the list. Done. https://codereview.chromium.org/25906002/diff/338001/content/renderer/webcryp... File content/renderer/webcrypto/webcrypto_impl.cc (right): https://codereview.chromium.org/25906002/diff/338001/content/renderer/webcryp... content/renderer/webcrypto/webcrypto_impl.cc:29: WebKit::WebCryptoAlgorithm BindAlgFactoryWithKeyLen() { On 2013/10/28 23:02:00, eroman wrote: > Ah, clever! > > I had been thinking something dumber -- defining a separate function for each > one, which then passes the key length as parameter. > > ... But I like your way better. > > Even though I don't generally like templates, I think this is a good trade-off. The problem was that the create functions don't all have the same signature (e.g. not all take a key length). Reducing all to a simple no-argument signature with this type of non-type template binding was the only way to avoid making a bunch new create functions where some had ugly unused arguments. BTW I agree with you on template use. https://codereview.chromium.org/25906002/diff/338001/content/renderer/webcryp... content/renderer/webcrypto/webcrypto_impl.cc:35: // WebKit::WebCryptoAlgorithm func(); On 2013/10/28 23:02:00, eroman wrote: > [optional] Could reference the function typedef name instead. I just removed that part of the comment since it is obvious from the code. https://codereview.chromium.org/25906002/diff/338001/content/renderer/webcryp... content/renderer/webcrypto/webcrypto_impl.cc:70: JwkAlgFactoryMap& Map() { return map_; } On 2013/10/28 23:02:00, eroman wrote: > Please make this a const-reference. (For mutable parameters chromium style > prefers pointers). > > Also for accessors chromium style uses hacker_style(), so you can call this > "map()" Done. https://codereview.chromium.org/25906002/diff/338001/content/renderer/webcryp... content/renderer/webcrypto/webcrypto_impl.cc:78: // TODO(padolph) Verify this logic is sufficient to judge algorithm On 2013/10/28 23:02:00, eroman wrote: > nit: semicolon after closing parenthesis. Done. https://codereview.chromium.org/25906002/diff/338001/content/renderer/webcryp... content/renderer/webcrypto/webcrypto_impl.cc:86: if (WebCryptoAlgorithmsConsistent(lhs.hmacParams()->hash(), On 2013/10/28 23:02:00, eroman wrote: > Unfortunately getting the hash() is not this easy :( > > Because all the parameters are strongly typed following the normalization, there > is a different type corresponding to each algorithm. > > for instance the parameter type could be any of: > WebCryptoHmacParams > WebCryptoHmacKeyParams > WebCryptoRsaSsaParams > WebCryptoRsaOaepParams > > Written as is, this could return NULL for hmacParams and subsequently crash. > > I suggest adding a helper function in the utils such as say: > WebCryptoAlgorithm GetHashAlgorithm(const WebCryptoAgorithm&) > > Which will explore the correct parameter type. > > Could you add a regression test? Oops missed that. Thank you. Done and done. https://codereview.chromium.org/25906002/diff/338001/content/renderer/webcryp... content/renderer/webcrypto/webcrypto_impl.cc:103: WebCryptoImpl::~WebCryptoImpl() {} On 2013/10/28 23:02:00, eroman wrote: > Doesn't seem like this is needed. I think the same, but clang complained loudly; not sure why. https://codereview.chromium.org/25906002/diff/338001/content/renderer/webcryp... content/renderer/webcrypto/webcrypto_impl.cc:372: jwk_alg_factory_map.Get().Map().find(jwk_alg_value); On 2013/10/28 23:02:00, eroman wrote: > How about moving this logic into the singleton class itself, instead of exposing > the std::map directly: > > WebCryptoAlgorithm webcrypto_algorithm_from_jwk = > jwk_alg_factory_.Get().CreateAlgorithmFromName(jwk_alg_value); > > if (algorithm_from_jkw.isNull()) > return false; > > > (I also shortened the name of the singleton in the above example). Good idea, thank you. Done. https://codereview.chromium.org/25906002/diff/338001/content/renderer/webcryp... content/renderer/webcrypto/webcrypto_impl.cc:379: if (!WebCryptoAlgorithmsConsistent(webcrypto_algorithm_from_jwk, algorithm)) On 2013/10/28 23:02:00, eroman wrote: > Now that we have made WebCryptoAlgorithm nullable, it will be possible for > "algorithm.isNull()". > > So it is necessary to either check for that somewhere in ImportKeyJwk(), or for > WebCryptoAlgorithmsConsistent() to handle it. Done. Added input parameter sanity checks and corresponding unit tests. https://codereview.chromium.org/25906002/diff/338001/content/renderer/webcryp... content/renderer/webcrypto/webcrypto_impl.cc:400: // usage_mask must be a subset of jwk_usage_mask. On 2013/10/28 23:02:00, eroman wrote: > Please capitalize comments. Done. https://codereview.chromium.org/25906002/diff/338001/content/renderer/webcryp... File content/renderer/webcrypto/webcrypto_impl.h (right): https://codereview.chromium.org/25906002/diff/338001/content/renderer/webcryp... content/renderer/webcrypto/webcrypto_impl.h:9: #include <string> On 2013/10/28 23:02:00, eroman wrote: > These two headers appear unused in this file. Add them to the specific C++ file > instead. Done. https://codereview.chromium.org/25906002/diff/338001/content/renderer/webcryp... content/renderer/webcrypto/webcrypto_impl.h:22: virtual ~WebCryptoImpl(); On 2013/10/28 23:02:00, eroman wrote: > Any reason to add the destructor definition? Since there are no member fields > doesn't seem like one is particularly interesting. Clang complained when I didn't have this. Should I be paying attention to clang errors? https://codereview.chromium.org/25906002/diff/338001/content/renderer/webcryp... File content/renderer/webcrypto/webcrypto_impl_unittest.cc (right): https://codereview.chromium.org/25906002/diff/338001/content/renderer/webcryp... content/renderer/webcrypto/webcrypto_impl_unittest.cc:731: // TODO(padolph) RSA public key bad data: On 2013/10/28 23:02:00, eroman wrote: > please add colon after paren. Done. https://codereview.chromium.org/25906002/diff/338001/content/renderer/webcryp... content/renderer/webcrypto/webcrypto_impl_unittest.cc:735: // Value check on e?? On 2013/10/28 23:02:00, eroman wrote: > We will need a lot of tests to cover all the full set of cases! This is a good > start though. > > Another one that comes to mind is case-sensitivity of properties. (Which is > different from the case-insensitivity of algorithm names in webcrypto). Agreed. I wonder if the case-sensitive tests fit better on the blink side. They are easy tests though wherever they live. The JWK strings must be an exact case match as far as I can tell. https://codereview.chromium.org/25906002/diff/338001/content/renderer/webcryp... content/renderer/webcrypto/webcrypto_impl_unittest.cc:804: usage_mask = WebKit::WebCryptoKeyUsageEncrypt | On 2013/10/28 23:02:00, eroman wrote: > You must have read my mind! After reading the test case above, I was about to > ask for this one ;) Yes, a superset is strictly not a subset. :) https://codereview.chromium.org/25906002/diff/338001/content/renderer/webcryp... File content/renderer/webcrypto/webcrypto_util.cc (right): https://codereview.chromium.org/25906002/diff/338001/content/renderer/webcryp... content/renderer/webcrypto/webcrypto_util.cc:7: #include "content/renderer/webcrypto/webcrypto_util.h" On 2013/10/28 23:02:00, eroman wrote: > This header goes first, and then the rest (the header for the .cc file has > special treatment in the sorting). Done. https://codereview.chromium.org/25906002/diff/338001/content/renderer/webcryp... content/renderer/webcrypto/webcrypto_util.cc:51: } On 2013/10/28 23:02:00, eroman wrote: > End with a comment " // namespace" to help matching Done. https://codereview.chromium.org/25906002/diff/338001/content/renderer/webcryp... File content/renderer/webcrypto/webcrypto_util.h (right): https://codereview.chromium.org/25906002/diff/338001/content/renderer/webcryp... content/renderer/webcrypto/webcrypto_util.h:5: #ifndef CONTENT_RENDERER_WEBCRYPTO_WEBCRYPTO_UTI_H_ On 2013/10/28 23:02:00, eroman wrote: > typo I presume: UTI --> UTIL Done. https://codereview.chromium.org/25906002/diff/338001/content/renderer/webcryp... content/renderer/webcrypto/webcrypto_util.h:17: const uint8* Start(const std::vector<uint8>& data); On 2013/10/28 23:02:00, eroman wrote: > I'm not sure how I feel about this being in the header, since it feels like > "unittest" code. But I suppose it is used by webcrypto_util.cc as well as the > unittest so makes sense. I would like to remove this altogether, because I'm used to the std::vector / C array equivalence syntax using &vec[0]. Is that idiom not well known enough that we must wrap it in a helper? Or is this to get a unified syntax for a later move to C++11's vector::data()? https://codereview.chromium.org/25906002/diff/338001/content/renderer/webcryp... content/renderer/webcrypto/webcrypto_util.h:21: bool Base64DecodeUrlSafe(const std::string& input, std::string* output); On 2013/10/28 23:02:00, eroman wrote: > Please move the function-level comments from the .cc file to the .h file. Done. https://codereview.chromium.org/25906002/diff/338001/content/renderer/webcryp... content/renderer/webcrypto/webcrypto_util.h:33: unsigned hash_length); On 2013/10/28 23:02:00, eroman wrote: > side comment: We may want to change the parameter names to indicate whether they > are bits or bytes. The WebCrypto API is inconsistent in that most key lengths > are in bits, however for Hmac key generation it actually takes it in bytes. I thought that was going to be fixed in the spec. That said, I like including units in variable names. Please let me know if you want to make a bigger naming change. https://codereview.chromium.org/25906002/diff/338001/content/renderer/webcryp... content/renderer/webcrypto/webcrypto_util.h:46: const std::vector<uint8>& additionalData, unsigned char tagLength); On 2013/10/28 23:02:00, eroman wrote: > use hacker_style_naming for variables in chromium Done.
(manual email) 0 minutes ago #13 <https://codereview.chromium.org/25906002/#msg13> https://codereview.chromium.org/25906002/diff/338001/content/content_renderer... <https://codereview.chromium.org/25906002/diff/338001/content/content_renderer...> File content/content_renderer.gypi (right): https://codereview.chromium.org/25906002/diff/338001/content/content_renderer... <https://codereview.chromium.org/25906002/diff/338001/content/content_renderer...> content/content_renderer.gypi:512: 'renderer/webcrypto/webcrypto_util.cc', On 2013/10/28 23:02:00, eroman wrote: > Please add the .h file as well. > > It isn't strictly necessary (as the compiler infers the header dependencies), > but it is consistent with the rest of the list. Done. https://codereview.chromium.org/25906002/diff/338001/content/renderer/webcryp... <https://codereview.chromium.org/25906002/diff/338001/content/renderer/webcryp...> File content/renderer/webcrypto/webcrypto_impl.cc (right): https://codereview.chromium.org/25906002/diff/338001/content/renderer/webcryp... <https://codereview.chromium.org/25906002/diff/338001/content/renderer/webcryp...> content/renderer/webcrypto/webcrypto_impl.cc:29: WebKit::WebCryptoAlgorithm BindAlgFactoryWithKeyLen() { On 2013/10/28 23:02:00, eroman wrote: > Ah, clever! > > I had been thinking something dumber -- defining a separate function for each > one, which then passes the key length as parameter. > > ... But I like your way better. > > Even though I don't generally like templates, I think this is a good trade-off. The problem was that the create functions don't all have the same signature (e.g. not all take a key length). Reducing all to a simple no-argument signature with this type of non-type template binding was the only way to avoid making a bunch new create functions where some had ugly unused arguments. BTW I agree with you on template use. https://codereview.chromium.org/25906002/diff/338001/content/renderer/webcryp... <https://codereview.chromium.org/25906002/diff/338001/content/renderer/webcryp...> content/renderer/webcrypto/webcrypto_impl.cc:35: // WebKit::WebCryptoAlgorithm func(); On 2013/10/28 23:02:00, eroman wrote: > [optional] Could reference the function typedef name instead. I just removed that part of the comment since it is obvious from the code. https://codereview.chromium.org/25906002/diff/338001/content/renderer/webcryp... <https://codereview.chromium.org/25906002/diff/338001/content/renderer/webcryp...> content/renderer/webcrypto/webcrypto_impl.cc:70: JwkAlgFactoryMap& Map() { return map_; } On 2013/10/28 23:02:00, eroman wrote: > Please make this a const-reference. (For mutable parameters chromium style > prefers pointers). > > Also for accessors chromium style uses hacker_style(), so you can call this > "map()" Done. https://codereview.chromium.org/25906002/diff/338001/content/renderer/webcryp... <https://codereview.chromium.org/25906002/diff/338001/content/renderer/webcryp...> content/renderer/webcrypto/webcrypto_impl.cc:78: // TODO(padolph) Verify this logic is sufficient to judge algorithm On 2013/10/28 23:02:00, eroman wrote: > nit: semicolon after closing parenthesis. Done. https://codereview.chromium.org/25906002/diff/338001/content/renderer/webcryp... <https://codereview.chromium.org/25906002/diff/338001/content/renderer/webcryp...> content/renderer/webcrypto/webcrypto_impl.cc:86: if (WebCryptoAlgorithmsConsistent(lhs.hmacParams()->hash(), On 2013/10/28 23:02:00, eroman wrote: > Unfortunately getting the hash() is not this easy :( > > Because all the parameters are strongly typed following the normalization, there > is a different type corresponding to each algorithm. > > for instance the parameter type could be any of: > WebCryptoHmacParams > WebCryptoHmacKeyParams > WebCryptoRsaSsaParams > WebCryptoRsaOaepParams > > Written as is, this could return NULL for hmacParams and subsequently crash. > > I suggest adding a helper function in the utils such as say: > WebCryptoAlgorithm GetHashAlgorithm(const WebCryptoAgorithm&) > > Which will explore the correct parameter type. > > Could you add a regression test? Oops missed that. Thank you. Done and done. https://codereview.chromium.org/25906002/diff/338001/content/renderer/webcryp... <https://codereview.chromium.org/25906002/diff/338001/content/renderer/webcryp...> content/renderer/webcrypto/webcrypto_impl.cc:103: WebCryptoImpl::~WebCryptoImpl() {} On 2013/10/28 23:02:00, eroman wrote: > Doesn't seem like this is needed. I think the same, but clang complained loudly; not sure why. https://codereview.chromium.org/25906002/diff/338001/content/renderer/webcryp... <https://codereview.chromium.org/25906002/diff/338001/content/renderer/webcryp...> content/renderer/webcrypto/webcrypto_impl.cc:372: jwk_alg_factory_map.Get().Map().find(jwk_alg_value); On 2013/10/28 23:02:00, eroman wrote: > How about moving this logic into the singleton class itself, instead of exposing > the std::map directly: > > WebCryptoAlgorithm webcrypto_algorithm_from_jwk = > jwk_alg_factory_.Get().CreateAlgorithmFromName(jwk_alg_value); > > if (algorithm_from_jkw.isNull()) > return false; > > > (I also shortened the name of the singleton in the above example). Good idea, thank you. Done. https://codereview.chromium.org/25906002/diff/338001/content/renderer/webcryp... <https://codereview.chromium.org/25906002/diff/338001/content/renderer/webcryp...> content/renderer/webcrypto/webcrypto_impl.cc:379: if (!WebCryptoAlgorithmsConsistent(webcrypto_algorithm_from_jwk, algorithm)) On 2013/10/28 23:02:00, eroman wrote: > Now that we have made WebCryptoAlgorithm nullable, it will be possible for > "algorithm.isNull()". > > So it is necessary to either check for that somewhere in ImportKeyJwk(), or for > WebCryptoAlgorithmsConsistent() to handle it. Done. Added input parameter sanity checks and corresponding unit tests. https://codereview.chromium.org/25906002/diff/338001/content/renderer/webcryp... <https://codereview.chromium.org/25906002/diff/338001/content/renderer/webcryp...> content/renderer/webcrypto/webcrypto_impl.cc:400: // usage_mask must be a subset of jwk_usage_mask. On 2013/10/28 23:02:00, eroman wrote: > Please capitalize comments. Done. https://codereview.chromium.org/25906002/diff/338001/content/renderer/webcryp... <https://codereview.chromium.org/25906002/diff/338001/content/renderer/webcryp...> File content/renderer/webcrypto/webcrypto_impl.h (right): https://codereview.chromium.org/25906002/diff/338001/content/renderer/webcryp... <https://codereview.chromium.org/25906002/diff/338001/content/renderer/webcryp...> content/renderer/webcrypto/webcrypto_impl.h:9: #include <string> On 2013/10/28 23:02:00, eroman wrote: > These two headers appear unused in this file. Add them to the specific C++ file > instead. Done. https://codereview.chromium.org/25906002/diff/338001/content/renderer/webcryp... <https://codereview.chromium.org/25906002/diff/338001/content/renderer/webcryp...> content/renderer/webcrypto/webcrypto_impl.h:22: virtual ~WebCryptoImpl(); On 2013/10/28 23:02:00, eroman wrote: > Any reason to add the destructor definition? Since there are no member fields > doesn't seem like one is particularly interesting. Clang complained when I didn't have this. Should I be paying attention to clang errors? https://codereview.chromium.org/25906002/diff/338001/content/renderer/webcryp... <https://codereview.chromium.org/25906002/diff/338001/content/renderer/webcryp...> File content/renderer/webcrypto/webcrypto_impl_unittest.cc (right): https://codereview.chromium.org/25906002/diff/338001/content/renderer/webcryp... <https://codereview.chromium.org/25906002/diff/338001/content/renderer/webcryp...> content/renderer/webcrypto/webcrypto_impl_unittest.cc:731: // TODO(padolph) RSA public key bad data: On 2013/10/28 23:02:00, eroman wrote: > please add colon after paren. Done. https://codereview.chromium.org/25906002/diff/338001/content/renderer/webcryp... <https://codereview.chromium.org/25906002/diff/338001/content/renderer/webcryp...> content/renderer/webcrypto/webcrypto_impl_unittest.cc:735: // Value check on e?? On 2013/10/28 23:02:00, eroman wrote: > We will need a lot of tests to cover all the full set of cases! This is a good > start though. > > Another one that comes to mind is case-sensitivity of properties. (Which is > different from the case-insensitivity of algorithm names in webcrypto). Agreed. I wonder if the case-sensitive tests fit better on the blink side. They are easy tests though wherever they live. The JWK strings must be an exact case match as far as I can tell. https://codereview.chromium.org/25906002/diff/338001/content/renderer/webcryp... <https://codereview.chromium.org/25906002/diff/338001/content/renderer/webcryp...> content/renderer/webcrypto/webcrypto_impl_unittest.cc:804: usage_mask = WebKit::WebCryptoKeyUsageEncrypt | On 2013/10/28 23:02:00, eroman wrote: > You must have read my mind! After reading the test case above, I was about to > ask for this one ;) Yes, a superset is strictly not a subset. :) https://codereview.chromium.org/25906002/diff/338001/content/renderer/webcryp... <https://codereview.chromium.org/25906002/diff/338001/content/renderer/webcryp...> File content/renderer/webcrypto/webcrypto_util.cc (right): https://codereview.chromium.org/25906002/diff/338001/content/renderer/webcryp... <https://codereview.chromium.org/25906002/diff/338001/content/renderer/webcryp...> content/renderer/webcrypto/webcrypto_util.cc:7: #include "content/renderer/webcrypto/webcrypto_util.h" On 2013/10/28 23:02:00, eroman wrote: > This header goes first, and then the rest (the header for the .cc file has > special treatment in the sorting). Done. https://codereview.chromium.org/25906002/diff/338001/content/renderer/webcryp... <https://codereview.chromium.org/25906002/diff/338001/content/renderer/webcryp...> content/renderer/webcrypto/webcrypto_util.cc:51: } On 2013/10/28 23:02:00, eroman wrote: > End with a comment " // namespace" to help matching Done. https://codereview.chromium.org/25906002/diff/338001/content/renderer/webcryp... <https://codereview.chromium.org/25906002/diff/338001/content/renderer/webcryp...> File content/renderer/webcrypto/webcrypto_util.h (right): https://codereview.chromium.org/25906002/diff/338001/content/renderer/webcryp... <https://codereview.chromium.org/25906002/diff/338001/content/renderer/webcryp...> content/renderer/webcrypto/webcrypto_util.h:5: #ifndef CONTENT_RENDERER_WEBCRYPTO_WEBCRYPTO_UTI_H_ On 2013/10/28 23:02:00, eroman wrote: > typo I presume: UTI --> UTIL Done. https://codereview.chromium.org/25906002/diff/338001/content/renderer/webcryp... <https://codereview.chromium.org/25906002/diff/338001/content/renderer/webcryp...> content/renderer/webcrypto/webcrypto_util.h:17: const uint8* Start(const std::vector<uint8>& data); On 2013/10/28 23:02:00, eroman wrote: > I'm not sure how I feel about this being in the header, since it feels like > "unittest" code. But I suppose it is used by webcrypto_util.cc as well as the > unittest so makes sense. I would like to remove this altogether, because I'm used to the std::vector / C array equivalence syntax using &vec[0]. Is that idiom not well known enough that we must wrap it in a helper? Or is this to get a unified syntax for a later move to C++11's vector::data()? https://codereview.chromium.org/25906002/diff/338001/content/renderer/webcryp... <https://codereview.chromium.org/25906002/diff/338001/content/renderer/webcryp...> content/renderer/webcrypto/webcrypto_util.h:21: bool Base64DecodeUrlSafe(const std::string& input, std::string* output); On 2013/10/28 23:02:00, eroman wrote: > Please move the function-level comments from the .cc file to the .h file. Done. https://codereview.chromium.org/25906002/diff/338001/content/renderer/webcryp... <https://codereview.chromium.org/25906002/diff/338001/content/renderer/webcryp...> content/renderer/webcrypto/webcrypto_util.h:33: unsigned hash_length); On 2013/10/28 23:02:00, eroman wrote: > side comment: We may want to change the parameter names to indicate whether they > are bits or bytes. The WebCrypto API is inconsistent in that most key lengths > are in bits, however for Hmac key generation it actually takes it in bytes. I thought that was going to be fixed in the spec. That said, I like including units in variable names. Please let me know if you want to make a bigger naming change. https://codereview.chromium.org/25906002/diff/338001/content/renderer/webcryp... <https://codereview.chromium.org/25906002/diff/338001/content/renderer/webcryp...> content/renderer/webcrypto/webcrypto_util.h:46: const std::vector<uint8>& additionalData, unsigned char tagLength); On 2013/10/28 23:02:00, eroman wrote: > use hacker_style_naming for variables in chromium Done. On Mon, Oct 28, 2013 at 4:02 PM, <eroman@chromium.org> wrote: > > https://codereview.chromium.**org/25906002/diff/338001/** > content/content_renderer.gypi<https://codereview.chromium.org/25906002/diff/338001/content/content_renderer.gypi> > File content/content_renderer.gypi (right): > > https://codereview.chromium.**org/25906002/diff/338001/** > content/content_renderer.gypi#**newcode512<https://codereview.chromium.org/25906002/diff/338001/content/content_renderer.gypi#newcode512> > content/content_renderer.gypi:**512: > 'renderer/webcrypto/webcrypto_**util.cc', > Please add the .h file as well. > > It isn't strictly necessary (as the compiler infers the header > dependencies), but it is consistent with the rest of the list. > > https://codereview.chromium.**org/25906002/diff/338001/** > content/renderer/webcrypto/**webcrypto_impl.cc<https://codereview.chromium.org/25906002/diff/338001/content/renderer/webcrypto/webcrypto_impl.cc> > File content/renderer/webcrypto/**webcrypto_impl.cc (right): > > https://codereview.chromium.**org/25906002/diff/338001/** > content/renderer/webcrypto/**webcrypto_impl.cc#newcode29<https://codereview.chromium.org/25906002/diff/338001/content/renderer/webcrypto/webcrypto_impl.cc#newcode29> > content/renderer/webcrypto/**webcrypto_impl.cc:29: > WebKit::WebCryptoAlgorithm BindAlgFactoryWithKeyLen() { > Ah, clever! > > I had been thinking something dumber -- defining a separate function for > each one, which then passes the key length as parameter. > > ... But I like your way better. > > Even though I don't generally like templates, I think this is a good > trade-off. > > https://codereview.chromium.**org/25906002/diff/338001/** > content/renderer/webcrypto/**webcrypto_impl.cc#newcode35<https://codereview.chromium.org/25906002/diff/338001/content/renderer/webcrypto/webcrypto_impl.cc#newcode35> > content/renderer/webcrypto/**webcrypto_impl.cc:35: // > WebKit::WebCryptoAlgorithm func(); > [optional] Could reference the function typedef name instead. > > https://codereview.chromium.**org/25906002/diff/338001/** > content/renderer/webcrypto/**webcrypto_impl.cc#newcode70<https://codereview.chromium.org/25906002/diff/338001/content/renderer/webcrypto/webcrypto_impl.cc#newcode70> > content/renderer/webcrypto/**webcrypto_impl.cc:70: JwkAlgFactoryMap& Map() > { return map_; } > Please make this a const-reference. (For mutable parameters chromium > style prefers pointers). > > Also for accessors chromium style uses hacker_style(), so you can call > this "map()" > > https://codereview.chromium.**org/25906002/diff/338001/** > content/renderer/webcrypto/**webcrypto_impl.cc#newcode78<https://codereview.chromium.org/25906002/diff/338001/content/renderer/webcrypto/webcrypto_impl.cc#newcode78> > content/renderer/webcrypto/**webcrypto_impl.cc:78: // TODO(padolph) Verify > this logic is sufficient to judge algorithm > nit: semicolon after closing parenthesis. > > https://codereview.chromium.**org/25906002/diff/338001/** > content/renderer/webcrypto/**webcrypto_impl.cc#newcode86<https://codereview.chromium.org/25906002/diff/338001/content/renderer/webcrypto/webcrypto_impl.cc#newcode86> > content/renderer/webcrypto/**webcrypto_impl.cc:86: if > (**WebCryptoAlgorithmsConsistent(**lhs.hmacParams()->hash(), > Unfortunately getting the hash() is not this easy :( > > Because all the parameters are strongly typed following the > normalization, there is a different type corresponding to each > algorithm. > > for instance the parameter type could be any of: > WebCryptoHmacParams > WebCryptoHmacKeyParams > WebCryptoRsaSsaParams > WebCryptoRsaOaepParams > > Written as is, this could return NULL for hmacParams and subsequently > crash. > > I suggest adding a helper function in the utils such as say: > WebCryptoAlgorithm GetHashAlgorithm(const WebCryptoAgorithm&) > > Which will explore the correct parameter type. > > Could you add a regression test? > > https://codereview.chromium.**org/25906002/diff/338001/** > content/renderer/webcrypto/**webcrypto_impl.cc#newcode103<https://codereview.chromium.org/25906002/diff/338001/content/renderer/webcrypto/webcrypto_impl.cc#newcode103> > content/renderer/webcrypto/**webcrypto_impl.cc:103: > WebCryptoImpl::~WebCryptoImpl(**) {} > Doesn't seem like this is needed. > > https://codereview.chromium.**org/25906002/diff/338001/** > content/renderer/webcrypto/**webcrypto_impl.cc#newcode372<https://codereview.chromium.org/25906002/diff/338001/content/renderer/webcrypto/webcrypto_impl.cc#newcode372> > content/renderer/webcrypto/**webcrypto_impl.cc:372: > jwk_alg_factory_map.Get().Map(**).find(jwk_alg_value); > How about moving this logic into the singleton class itself, instead of > exposing the std::map directly: > > WebCryptoAlgorithm webcrypto_algorithm_from_jwk = > jwk_alg_factory_.Get().**CreateAlgorithmFromName(jwk_**alg_value); > > if (algorithm_from_jkw.isNull()) > return false; > > > (I also shortened the name of the singleton in the above example). > > https://codereview.chromium.**org/25906002/diff/338001/** > content/renderer/webcrypto/**webcrypto_impl.cc#newcode379<https://codereview.chromium.org/25906002/diff/338001/content/renderer/webcrypto/webcrypto_impl.cc#newcode379> > content/renderer/webcrypto/**webcrypto_impl.cc:379: if > (!**WebCryptoAlgorithmsConsistent(**webcrypto_algorithm_from_jwk, > algorithm)) > Now that we have made WebCryptoAlgorithm nullable, it will be possible > for "algorithm.isNull()". > > So it is necessary to either check for that somewhere in ImportKeyJwk(), > or for WebCryptoAlgorithmsConsistent(**) to handle it. > > https://codereview.chromium.**org/25906002/diff/338001/** > content/renderer/webcrypto/**webcrypto_impl.cc#newcode400<https://codereview.chromium.org/25906002/diff/338001/content/renderer/webcrypto/webcrypto_impl.cc#newcode400> > content/renderer/webcrypto/**webcrypto_impl.cc:400: // usage_mask must be > a subset of jwk_usage_mask. > Please capitalize comments. > > https://codereview.chromium.**org/25906002/diff/338001/** > content/renderer/webcrypto/**webcrypto_impl.h<https://codereview.chromium.org/25906002/diff/338001/content/renderer/webcrypto/webcrypto_impl.h> > File content/renderer/webcrypto/**webcrypto_impl.h (right): > > https://codereview.chromium.**org/25906002/diff/338001/** > content/renderer/webcrypto/**webcrypto_impl.h#newcode9<https://codereview.chromium.org/25906002/diff/338001/content/renderer/webcrypto/webcrypto_impl.h#newcode9> > content/renderer/webcrypto/**webcrypto_impl.h:9: #include <string> > These two headers appear unused in this file. Add them to the specific > C++ file instead. > > https://codereview.chromium.**org/25906002/diff/338001/** > content/renderer/webcrypto/**webcrypto_impl.h#newcode22<https://codereview.chromium.org/25906002/diff/338001/content/renderer/webcrypto/webcrypto_impl.h#newcode22> > content/renderer/webcrypto/**webcrypto_impl.h:22: virtual > ~WebCryptoImpl(); > Any reason to add the destructor definition? Since there are no member > fields doesn't seem like one is particularly interesting. > > https://codereview.chromium.**org/25906002/diff/338001/** > content/renderer/webcrypto/**webcrypto_impl_unittest.cc<https://codereview.chromium.org/25906002/diff/338001/content/renderer/webcrypto/webcrypto_impl_unittest.cc> > File content/renderer/webcrypto/**webcrypto_impl_unittest.cc (right): > > https://codereview.chromium.**org/25906002/diff/338001/** > content/renderer/webcrypto/**webcrypto_impl_unittest.cc#**newcode664<https://codereview.chromium.org/25906002/diff/338001/content/renderer/webcrypto/webcrypto_impl_unittest.cc#newcode664> > content/renderer/webcrypto/**webcrypto_impl_unittest.cc:**664: // Invalid > JSON > good test cases, thanks! > > https://codereview.chromium.**org/25906002/diff/338001/** > content/renderer/webcrypto/**webcrypto_impl_unittest.cc#**newcode731<https://codereview.chromium.org/25906002/diff/338001/content/renderer/webcrypto/webcrypto_impl_unittest.cc#newcode731> > content/renderer/webcrypto/**webcrypto_impl_unittest.cc:**731: // > TODO(padolph) RSA public key bad data: > please add colon after paren. > > https://codereview.chromium.**org/25906002/diff/338001/** > content/renderer/webcrypto/**webcrypto_impl_unittest.cc#**newcode735<https://codereview.chromium.org/25906002/diff/338001/content/renderer/webcrypto/webcrypto_impl_unittest.cc#newcode735> > content/renderer/webcrypto/**webcrypto_impl_unittest.cc:**735: // Value > check on e?? > We will need a lot of tests to cover all the full set of cases! This is > a good start though. > > Another one that comes to mind is case-sensitivity of properties. (Which > is different from the case-insensitivity of algorithm names in > webcrypto). > > https://codereview.chromium.**org/25906002/diff/338001/** > content/renderer/webcrypto/**webcrypto_impl_unittest.cc#**newcode804<https://codereview.chromium.org/25906002/diff/338001/content/renderer/webcrypto/webcrypto_impl_unittest.cc#newcode804> > content/renderer/webcrypto/**webcrypto_impl_unittest.cc:**804: usage_mask > = > WebKit::**WebCryptoKeyUsageEncrypt | > You must have read my mind! After reading the test case above, I was > about to ask for this one ;) > > https://codereview.chromium.**org/25906002/diff/338001/** > content/renderer/webcrypto/**webcrypto_util.cc<https://codereview.chromium.org/25906002/diff/338001/content/renderer/webcrypto/webcrypto_util.cc> > File content/renderer/webcrypto/**webcrypto_util.cc (right): > > https://codereview.chromium.**org/25906002/diff/338001/** > content/renderer/webcrypto/**webcrypto_util.cc#newcode7<https://codereview.chromium.org/25906002/diff/338001/content/renderer/webcrypto/webcrypto_util.cc#newcode7> > content/renderer/webcrypto/**webcrypto_util.cc:7: #include > "content/renderer/webcrypto/**webcrypto_util.h" > This header goes first, and then the rest (the header for the .cc file > has special treatment in the sorting). > > https://codereview.chromium.**org/25906002/diff/338001/** > content/renderer/webcrypto/**webcrypto_util.cc#newcode51<https://codereview.chromium.org/25906002/diff/338001/content/renderer/webcrypto/webcrypto_util.cc#newcode51> > content/renderer/webcrypto/**webcrypto_util.cc:51: } > End with a comment " // namespace" to help matching > > https://codereview.chromium.**org/25906002/diff/338001/** > content/renderer/webcrypto/**webcrypto_util.h<https://codereview.chromium.org/25906002/diff/338001/content/renderer/webcrypto/webcrypto_util.h> > File content/renderer/webcrypto/**webcrypto_util.h (right): > > https://codereview.chromium.**org/25906002/diff/338001/** > content/renderer/webcrypto/**webcrypto_util.h#newcode5<https://codereview.chromium.org/25906002/diff/338001/content/renderer/webcrypto/webcrypto_util.h#newcode5> > content/renderer/webcrypto/**webcrypto_util.h:5: #ifndef > CONTENT_RENDERER_WEBCRYPTO_**WEBCRYPTO_UTI_H_ > typo I presume: UTI --> UTIL > > https://codereview.chromium.**org/25906002/diff/338001/** > content/renderer/webcrypto/**webcrypto_util.h#newcode15<https://codereview.chromium.org/25906002/diff/338001/content/renderer/webcrypto/webcrypto_util.h#newcode15> > content/renderer/webcrypto/**webcrypto_util.h:15: namespace content { > I'm not entirely sure on whether we should put this into something like > webcrypto_util namespace. Certainly some of the function names in here > are very generic (like "Start").. Let me get back to you on this... > > https://codereview.chromium.**org/25906002/diff/338001/** > content/renderer/webcrypto/**webcrypto_util.h#newcode17<https://codereview.chromium.org/25906002/diff/338001/content/renderer/webcrypto/webcrypto_util.h#newcode17> > content/renderer/webcrypto/**webcrypto_util.h:17: const uint8* Start(const > std::vector<uint8>& data); > I'm not sure how I feel about this being in the header, since it feels > like "unittest" code. But I suppose it is used by webcrypto_util.cc as > well as the unittest so makes sense. > > https://codereview.chromium.**org/25906002/diff/338001/** > content/renderer/webcrypto/**webcrypto_util.h#newcode21<https://codereview.chromium.org/25906002/diff/338001/content/renderer/webcrypto/webcrypto_util.h#newcode21> > content/renderer/webcrypto/**webcrypto_util.h:21: bool > > Base64DecodeUrlSafe(const std::string& input, std::string* output); > Please move the function-level comments from the .cc file to the .h > file. > > https://codereview.chromium.**org/25906002/diff/338001/** > content/renderer/webcrypto/**webcrypto_util.h#newcode33<https://codereview.chromium.org/25906002/diff/338001/content/renderer/webcrypto/webcrypto_util.h#newcode33> > content/renderer/webcrypto/**webcrypto_util.h:33: unsigned hash_length); > side comment: We may want to change the parameter names to indicate > whether they are bits or bytes. The WebCrypto API is inconsistent in > that most key lengths are in bits, however for Hmac key generation it > actually takes it in bytes. > > https://codereview.chromium.**org/25906002/diff/338001/** > content/renderer/webcrypto/**webcrypto_util.h#newcode46<https://codereview.chromium.org/25906002/diff/338001/content/renderer/webcrypto/webcrypto_util.h#newcode46> > content/renderer/webcrypto/**webcrypto_util.h:46: const > std::vector<uint8>& additionalData, unsigned char tagLength); > use hacker_style_naming for variables in chromium > > https://codereview.chromium.**org/25906002/<https://codereview.chromium.org/2... > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
https://codereview.chromium.org/25906002/diff/338001/content/renderer/webcryp... File content/renderer/webcrypto/webcrypto_util.h (right): https://codereview.chromium.org/25906002/diff/338001/content/renderer/webcryp... content/renderer/webcrypto/webcrypto_util.h:17: const uint8* Start(const std::vector<uint8>& data); On 2013/10/29 02:25:40, padolph wrote: > On 2013/10/28 23:02:00, eroman wrote: > > I'm not sure how I feel about this being in the header, since it feels like > > "unittest" code. But I suppose it is used by webcrypto_util.cc as well as the > > unittest so makes sense. > > I would like to remove this altogether, because I'm used to the std::vector / C > array equivalence syntax using &vec[0]. Is that idiom not well known enough that > we must wrap it in a helper? Or is this to get a unified syntax for a later move > to C++11's vector::data()? The reason for Start() was to avoid doing &vec[0] on empty vectors. Since this pattern was necessary in several places of the unittests I extracted it to a helper. I am not particularly fond of it, and happy to see it go if you have a better solution. https://codereview.chromium.org/25906002/diff/448001/content/renderer/webcryp... File content/renderer/webcrypto/webcrypto_impl.cc (right): https://codereview.chromium.org/25906002/diff/448001/content/renderer/webcryp... content/renderer/webcrypto/webcrypto_impl.cc:353: if (!key_data || !key_data_size || algorithm.isNull() || !handle || !type) My interpretation is that it is legitimate for the input algorithm to be null. The algorithm only needs to be specified to "fill in" the data which cannot be inferred from the JWK. So for some imports it may be unnecessary for the caller to specify algorithm at all. When the algorithm is specified and it repeats information encoded by the JWK, then the implementation must verify that things match. It is fine not to handle null algorithm at this time, but in that case please write a TODO explaining that it needs to be handled. Lastly, please remove the tests on "!handle || !type". I view those as an illegal call to ImportKeyJwk(), and something that can DCHECK/crash instead. https://codereview.chromium.org/25906002/diff/448001/content/renderer/webcryp... File content/renderer/webcrypto/webcrypto_impl_unittest.cc (right): https://codereview.chromium.org/25906002/diff/448001/content/renderer/webcryp... content/renderer/webcrypto/webcrypto_impl_unittest.cc:727: dict.SetString("kty", key_val); I actually preferred the duplication of these constants that you had before (more readable). https://codereview.chromium.org/25906002/diff/448001/content/renderer/webcryp... File content/renderer/webcrypto/webcrypto_util.cc (right): https://codereview.chromium.org/25906002/diff/448001/content/renderer/webcryp... content/renderer/webcrypto/webcrypto_util.cc:84: const WebKit::WebCryptoHmacParams* const params = algorithm.hmacParams(); Cant get rid of the local variable, and consequently the need for curly braces in switch statement: return algorithm.hmacParams()->hash(); https://codereview.chromium.org/25906002/diff/448001/content/renderer/webcryp... content/renderer/webcrypto/webcrypto_util.cc:90: return params->hash(); WebCryptoAlgorithm can have different parameter types depending for the same algorithmid, depending on the context it was normalized in. The spec currently lists "None" as the parameters for importKey(), so params would end up being NULL: https://dvcs.w3.org/hg/webcrypto-api/raw-file/tip/spec/Overview.html#rsassa-p... A safer way to write this function would be: if (algorithm.hmacParams()) return algorithm.hmacParams()->hash(); if (algorithm.hmacKeyParams()) return algorithm.hmacKeyParams()->hash(); if (algorithm.rsaSsaParams()) return algorithm.rsaSsaParams()->hash(); if (algorithm.rsaOaepParams()) return algorithm.rsaOaepParams()->hash();
not lgtm, but possibly because I'm an idiot. However, I definitely don't understand a piece of code here, suggesting either a bug or a documentation issue. https://codereview.chromium.org/25906002/diff/448001/content/renderer/webcryp... File content/renderer/webcrypto/webcrypto_util.cc (right): https://codereview.chromium.org/25906002/diff/448001/content/renderer/webcryp... content/renderer/webcrypto/webcrypto_util.cc:133: WebKit::WebCryptoAlgorithm CreateRsaSsaAlgorithmByKeyLen( I don't understand why this function is calling CreateAlgorithmWithInnerHash Why are you passing hash_key_length? What relationship is that to KeyLen? Why not pass the actual inner hash?
https://codereview.chromium.org/25906002/diff/448001/content/renderer/webcryp... File content/renderer/webcrypto/webcrypto_impl.cc (right): https://codereview.chromium.org/25906002/diff/448001/content/renderer/webcryp... content/renderer/webcrypto/webcrypto_impl.cc:353: if (!key_data || !key_data_size || algorithm.isNull() || !handle || !type) On 2013/10/29 03:03:46, eroman wrote: > My interpretation is that it is legitimate for the input algorithm to be null. > > The algorithm only needs to be specified to "fill in" the data which cannot be > inferred from the JWK. So for some imports it may be unnecessary for the caller > to specify algorithm at all. > > When the algorithm is specified and it repeats information encoded by the JWK, > then the implementation must verify that things match. > > It is fine not to handle null algorithm at this time, but in that case please > write a TODO explaining that it needs to be handled. I put in logic to handle input and JWK algorithms when they are both optional plus corresponding tests. The logic works but is quite ugly, but I've stared at it too long now to have any more insights. Can you please take a look to see if can be simplified? > Lastly, please remove the tests on "!handle || !type". I view those as an > illegal call to ImportKeyJwk(), and something that can DCHECK/crash instead. Done. https://codereview.chromium.org/25906002/diff/448001/content/renderer/webcryp... File content/renderer/webcrypto/webcrypto_impl_unittest.cc (right): https://codereview.chromium.org/25906002/diff/448001/content/renderer/webcryp... content/renderer/webcrypto/webcrypto_impl_unittest.cc:727: dict.SetString("kty", key_val); On 2013/10/29 03:03:46, eroman wrote: > I actually preferred the duplication of these constants that you had before > (more readable). Done. https://codereview.chromium.org/25906002/diff/448001/content/renderer/webcryp... File content/renderer/webcrypto/webcrypto_util.cc (right): https://codereview.chromium.org/25906002/diff/448001/content/renderer/webcryp... content/renderer/webcrypto/webcrypto_util.cc:84: const WebKit::WebCryptoHmacParams* const params = algorithm.hmacParams(); On 2013/10/29 03:03:46, eroman wrote: > Cant get rid of the local variable, and consequently the need for curly braces > in switch statement: > > return algorithm.hmacParams()->hash(); replaced function with your version below. https://codereview.chromium.org/25906002/diff/448001/content/renderer/webcryp... content/renderer/webcrypto/webcrypto_util.cc:90: return params->hash(); On 2013/10/29 03:03:46, eroman wrote: > WebCryptoAlgorithm can have different parameter types depending for the same > algorithmid, depending on the context it was normalized in. > > The spec currently lists "None" as the parameters for importKey(), so params > would end up being NULL: > > https://dvcs.w3.org/hg/webcrypto-api/raw-file/tip/spec/Overview.html#rsassa-p... > > A safer way to write this function would be: > > if (algorithm.hmacParams()) > return algorithm.hmacParams()->hash(); > if (algorithm.hmacKeyParams()) > return algorithm.hmacKeyParams()->hash(); > if (algorithm.rsaSsaParams()) > return algorithm.rsaSsaParams()->hash(); > if (algorithm.rsaOaepParams()) > return algorithm.rsaOaepParams()->hash(); Done. https://codereview.chromium.org/25906002/diff/448001/content/renderer/webcryp... content/renderer/webcrypto/webcrypto_util.cc:133: WebKit::WebCryptoAlgorithm CreateRsaSsaAlgorithmByKeyLen( On 2013/10/29 18:05:13, Ryan Sleevi wrote: > I don't understand why this function is calling CreateAlgorithmWithInnerHash > > Why are you passing hash_key_length? What relationship is that to KeyLen? Why > not pass the actual inner hash? Sorry, I agree this is just wrong. Not sure what happened there. I replaced this code.
Thanks Paul! Because this is a complicated change, I will need to do another review pass after this batch of comments. https://codereview.chromium.org/25906002/diff/588001/content/renderer/webcryp... File content/renderer/webcrypto/webcrypto_impl.cc (right): https://codereview.chromium.org/25906002/diff/588001/content/renderer/webcryp... content/renderer/webcrypto/webcrypto_impl.cc:74: &BindAlgFactoryWithKeyLen<CreateAesGcmKeyGenAlgorithm, 256>; Shouldn't this be 128? https://codereview.chromium.org/25906002/diff/588001/content/renderer/webcryp... content/renderer/webcrypto/webcrypto_impl.cc:86: WebKit::WebCryptoAlgorithm CreateAlgorithmFromName( [optional] Can you add a newlinea above this? https://codereview.chromium.org/25906002/diff/588001/content/renderer/webcryp... content/renderer/webcrypto/webcrypto_impl.cc:97: base::LazyInstance<JwkAlgorithmFactoryMap> jwk_alg_factory = ditto on newline. https://codereview.chromium.org/25906002/diff/588001/content/renderer/webcryp... content/renderer/webcrypto/webcrypto_impl.cc:102: bool WebCryptoAlgorithmsConsistent(const WebKit::WebCryptoAlgorithm& lhs, Rather than "lhs" and "rhs" I suggest something more general like "a" and "b" or "agl1" / "alg2". https://codereview.chromium.org/25906002/diff/588001/content/renderer/webcryp... content/renderer/webcrypto/webcrypto_impl.cc:291: // - "extractable" (Key Exportability), OPTIONAL [NOTE: not yet part of JOSE] Is there a bug number that can be linked to here? Whatever we add should be on the standards track. https://codereview.chromium.org/25906002/diff/588001/content/renderer/webcryp... content/renderer/webcrypto/webcrypto_impl.cc:376: // Conflict resolution Just wanted to re-iterate, I found the comments for this giant comment block to be very helpful! https://codereview.chromium.org/25906002/diff/588001/content/renderer/webcryp... content/renderer/webcrypto/webcrypto_impl.cc:403: bool jwk_extractable_value; Paranoia: I would hate for future code to reference jwk_extractable_value and result in an access to uninitialized value. Could you put this in its own block to reduce the scope of this variable: { bool jwk_extractable_value; if (dict_value->GetBoolean(.. } https://codereview.chromium.org/25906002/diff/588001/content/renderer/webcryp... content/renderer/webcrypto/webcrypto_impl.cc:404: if (dict_value->GetBoolean("extractable", &jwk_extractable_value)) { Should we be doing stronger error checking here? What if a JWK contains: extractable: "false" or extractable: 0 In this case there is a type mismatch from what we are expecting (boolean), but by treating it as an optional parameter means we silently discard the information. Which could be a security problem for the caller. This same question applies to the other properties. https://codereview.chromium.org/25906002/diff/588001/content/renderer/webcryp... content/renderer/webcrypto/webcrypto_impl.cc:446: Purely for readability, I suggest adding here: DCHECK(!algorithm.isNull()); https://codereview.chromium.org/25906002/diff/588001/content/renderer/webcryp... content/renderer/webcrypto/webcrypto_impl.cc:450: unsigned jwk_usage_mask; Please use the type as WebKit::WebCryptoKeyUsageMask instead of "unsigned". Also can you defensively initialize it to 0? https://codereview.chromium.org/25906002/diff/588001/content/renderer/webcryp... content/renderer/webcrypto/webcrypto_impl.cc:463: if ((jwk_usage_mask & usage_mask) != usage_mask) { Side comment (on spec): having to duplicate specify things as the caller certainly seems inconvenient. Perhaps the usage should be optional as well allowing importKey to just use what JWK had. Or jwk import use a different function. https://codereview.chromium.org/25906002/diff/588001/content/renderer/webcryp... content/renderer/webcrypto/webcrypto_impl.cc:486: return false; What do you think about making this: return ImportKeyInternal(....); https://codereview.chromium.org/25906002/diff/588001/content/renderer/webcryp... File content/renderer/webcrypto/webcrypto_impl_unittest.cc (right): https://codereview.chromium.org/25906002/diff/588001/content/renderer/webcryp... content/renderer/webcrypto/webcrypto_impl_unittest.cc:48: void RestoreDictionary(base::DictionaryValue* dict) { nit: can you give it a more specific name? https://codereview.chromium.org/25906002/diff/588001/content/renderer/webcryp... content/renderer/webcrypto/webcrypto_impl_unittest.cc:49: dict->SetString("kty", "oct"); How about calling dict->Clear() first, to remove any other keys that got added? https://codereview.chromium.org/25906002/diff/588001/content/renderer/webcryp... content/renderer/webcrypto/webcrypto_impl_unittest.cc:642: CreateHmacKeyGenAlgorithm(WebKit::WebCryptoAlgorithmIdSha1, 0); thanks, that is a better name. https://codereview.chromium.org/25906002/diff/588001/content/renderer/webcryp... content/renderer/webcrypto/webcrypto_impl_unittest.cc:664: WebKit::WebCryptoKey key = WebCryptoImpl::NullKey(); please use WebKit::WebCryptoKey::createNull() https://codereview.chromium.org/25906002/diff/588001/content/renderer/webcryp... content/renderer/webcrypto/webcrypto_impl_unittest.cc:671: // complete. useful comment, thanks https://codereview.chromium.org/25906002/diff/588001/content/renderer/webcryp... content/renderer/webcrypto/webcrypto_impl_unittest.cc:754: WebKit::WebCryptoKey key = WebCryptoImpl::NullKey(); WebCryptoKey::createNull()
https://codereview.chromium.org/25906002/diff/588001/content/renderer/webcryp... File content/renderer/webcrypto/webcrypto_impl.cc (right): https://codereview.chromium.org/25906002/diff/588001/content/renderer/webcryp... content/renderer/webcrypto/webcrypto_impl.cc:404: if (dict_value->GetBoolean("extractable", &jwk_extractable_value)) { On 2013/11/01 20:59:58, eroman wrote: > Should we be doing stronger error checking here? > > What if a JWK contains: > > extractable: "false" > or > extractable: 0 > > In this case there is a type mismatch from what we are expecting (boolean), but > by treating it as an optional parameter means we silently discard the > information. Which could be a security problem for the caller. > > This same question applies to the other properties. This, I think, highlights some of the issues with "JWK-in-the-web". It means that JSON.parse() in ES will yield a different object than, say, this reader, because of the interpretations / conversions made. Same for conceptually mapping a JWK into WebIDL, which also has language bindings issues. Definitely, the JWK/JWT spec needs to be unambiguously clear on this, and given that I have seen no standardization efforts or discussions regarding 'extractable' (other than the JOSE group rejecting it), I do think we need to be extremely careful for the open web platform.
https://codereview.chromium.org/25906002/diff/588001/content/renderer/webcryp... File content/renderer/webcrypto/webcrypto_impl.cc (right): https://codereview.chromium.org/25906002/diff/588001/content/renderer/webcryp... content/renderer/webcrypto/webcrypto_impl.cc:74: &BindAlgFactoryWithKeyLen<CreateAesGcmKeyGenAlgorithm, 256>; On 2013/11/01 20:59:58, eroman wrote: > Shouldn't this be 128? Done. https://codereview.chromium.org/25906002/diff/588001/content/renderer/webcryp... content/renderer/webcrypto/webcrypto_impl.cc:86: WebKit::WebCryptoAlgorithm CreateAlgorithmFromName( On 2013/11/01 20:59:58, eroman wrote: > [optional] Can you add a newlinea above this? Done. https://codereview.chromium.org/25906002/diff/588001/content/renderer/webcryp... content/renderer/webcrypto/webcrypto_impl.cc:97: base::LazyInstance<JwkAlgorithmFactoryMap> jwk_alg_factory = On 2013/11/01 20:59:58, eroman wrote: > ditto on newline. Done. https://codereview.chromium.org/25906002/diff/588001/content/renderer/webcryp... content/renderer/webcrypto/webcrypto_impl.cc:102: bool WebCryptoAlgorithmsConsistent(const WebKit::WebCryptoAlgorithm& lhs, On 2013/11/01 20:59:58, eroman wrote: > Rather than "lhs" and "rhs" I suggest something more general like "a" and "b" or > "agl1" / "alg2". Done. https://codereview.chromium.org/25906002/diff/588001/content/renderer/webcryp... content/renderer/webcrypto/webcrypto_impl.cc:291: // - "extractable" (Key Exportability), OPTIONAL [NOTE: not yet part of JOSE] On 2013/11/01 20:59:58, eroman wrote: > Is there a bug number that can be linked to here? Whatever we add should be on > the standards track. Not that I know of. Do you know how we can follow up with the effort to add 'extractable' to the JWT spec? https://codereview.chromium.org/25906002/diff/588001/content/renderer/webcryp... content/renderer/webcrypto/webcrypto_impl.cc:376: // Conflict resolution On 2013/11/01 20:59:58, eroman wrote: > Just wanted to re-iterate, I found the comments for this giant comment block to > be very helpful! :-) They did take a while to type in. I needed a central reference for the implementation. Glad you find it useful. https://codereview.chromium.org/25906002/diff/588001/content/renderer/webcryp... content/renderer/webcrypto/webcrypto_impl.cc:403: bool jwk_extractable_value; On 2013/11/01 20:59:58, eroman wrote: > Paranoia: I would hate for future code to reference jwk_extractable_value and > result in an access to uninitialized value. > > Could you put this in its own block to reduce the scope of this variable: > { > bool jwk_extractable_value; > if (dict_value->GetBoolean(.. > } Done. https://codereview.chromium.org/25906002/diff/588001/content/renderer/webcryp... content/renderer/webcrypto/webcrypto_impl.cc:404: if (dict_value->GetBoolean("extractable", &jwk_extractable_value)) { On 2013/11/01 22:45:14, Ryan Sleevi wrote: > On 2013/11/01 20:59:58, eroman wrote: > > Should we be doing stronger error checking here? > > > > What if a JWK contains: > > > > extractable: "false" > > or > > extractable: 0 > > > > In this case there is a type mismatch from what we are expecting (boolean), > but > > by treating it as an optional parameter means we silently discard the > > information. Which could be a security problem for the caller. > > > > This same question applies to the other properties. > > This, I think, highlights some of the issues with "JWK-in-the-web". It means > that JSON.parse() in ES will yield a different object than, say, this reader, > because of the interpretations / conversions made. Same for conceptually mapping > a JWK into WebIDL, which also has language bindings issues. > > Definitely, the JWK/JWT spec needs to be unambiguously clear on this, and given > that I have seen no standardization efforts or discussions regarding > 'extractable' (other than the JOSE group rejecting it), I do think we need to be > extremely careful for the open web platform. What is the best way to move this forward? https://codereview.chromium.org/25906002/diff/588001/content/renderer/webcryp... content/renderer/webcrypto/webcrypto_impl.cc:446: On 2013/11/01 20:59:58, eroman wrote: > Purely for readability, I suggest adding here: > > DCHECK(!algorithm.isNull()); Done. https://codereview.chromium.org/25906002/diff/588001/content/renderer/webcryp... content/renderer/webcrypto/webcrypto_impl.cc:450: unsigned jwk_usage_mask; On 2013/11/01 20:59:58, eroman wrote: > Please use the type as WebKit::WebCryptoKeyUsageMask instead of "unsigned". Also > can you defensively initialize it to 0? Done. https://codereview.chromium.org/25906002/diff/588001/content/renderer/webcryp... content/renderer/webcrypto/webcrypto_impl.cc:463: if ((jwk_usage_mask & usage_mask) != usage_mask) { On 2013/11/01 20:59:58, eroman wrote: > Side comment (on spec): having to duplicate specify things as the caller > certainly seems inconvenient. I totally agree. Especially if this JWK comes from inside an opaque JWE received from a remote peer, and the protocol in use does not assume the receiver has any a. priori knowledge. > Perhaps the usage should be optional as well > allowing importKey to just use what JWK had. Or jwk import use a different > function. One thing we don't want to allow is for the key to be used for an operation different for what it was intended for when the JWK was built. For optional input, what about a special case for when usage_mask is 0, take the jwk value? I think this might require a WC spec change. https://codereview.chromium.org/25906002/diff/588001/content/renderer/webcryp... content/renderer/webcrypto/webcrypto_impl.cc:486: return false; On 2013/11/01 20:59:58, eroman wrote: > What do you think about making this: > > return ImportKeyInternal(....); Done. https://codereview.chromium.org/25906002/diff/588001/content/renderer/webcryp... File content/renderer/webcrypto/webcrypto_impl_unittest.cc (right): https://codereview.chromium.org/25906002/diff/588001/content/renderer/webcryp... content/renderer/webcrypto/webcrypto_impl_unittest.cc:48: void RestoreDictionary(base::DictionaryValue* dict) { On 2013/11/01 20:59:58, eroman wrote: > nit: can you give it a more specific name? Done. https://codereview.chromium.org/25906002/diff/588001/content/renderer/webcryp... content/renderer/webcrypto/webcrypto_impl_unittest.cc:49: dict->SetString("kty", "oct"); On 2013/11/01 20:59:58, eroman wrote: > How about calling dict->Clear() first, to remove any other keys that got added? Done. https://codereview.chromium.org/25906002/diff/588001/content/renderer/webcryp... content/renderer/webcrypto/webcrypto_impl_unittest.cc:664: WebKit::WebCryptoKey key = WebCryptoImpl::NullKey(); On 2013/11/01 20:59:58, eroman wrote: > please use WebKit::WebCryptoKey::createNull() Done. https://codereview.chromium.org/25906002/diff/588001/content/renderer/webcryp... content/renderer/webcrypto/webcrypto_impl_unittest.cc:754: WebKit::WebCryptoKey key = WebCryptoImpl::NullKey(); On 2013/11/01 20:59:58, eroman wrote: > WebCryptoKey::createNull() Done.
https://codereview.chromium.org/25906002/diff/338001/content/renderer/webcryp... File content/renderer/webcrypto/webcrypto_impl.cc (right): https://codereview.chromium.org/25906002/diff/338001/content/renderer/webcryp... content/renderer/webcrypto/webcrypto_impl.cc:103: WebCryptoImpl::~WebCryptoImpl() {} On 2013/10/29 02:25:40, padolph wrote: > On 2013/10/28 23:02:00, eroman wrote: > > Doesn't seem like this is needed. > > I think the same, but clang complained loudly; not sure why. What is the error you get without this? https://codereview.chromium.org/25906002/diff/678001/content/renderer/webcryp... File content/renderer/webcrypto/webcrypto_impl.cc (right): https://codereview.chromium.org/25906002/diff/678001/content/renderer/webcryp... content/renderer/webcrypto/webcrypto_impl.cc:52: map_["HS256"] = This is a duplication of the one above. https://codereview.chromium.org/25906002/diff/678001/content/renderer/webcryp... content/renderer/webcrypto/webcrypto_impl.cc:56: map_["HS384"] = This is a duplication of the key above. I presume you meant HS512 ? https://codereview.chromium.org/25906002/diff/678001/content/renderer/webcryp... content/renderer/webcrypto/webcrypto_impl.cc:74: &BindAlgFactoryWithKeyLen<CreateAesGcmKeyGenAlgorithm, 128>; Are you sure it is correct for these to be using the "Gen" version of the algorithm parameters? According to the registration tables in webcrypto spec importKey() doesn't expect any parameters on this (meaning the key length is implicit from the raw key data). https://codereview.chromium.org/25906002/diff/678001/content/renderer/webcryp... content/renderer/webcrypto/webcrypto_impl.cc:76: &BindAlgFactoryWithKeyLen<CreateAesGcmKeyGenAlgorithm, 256>; same comment here regarding the "Gen" version of the algorithm. Since importKey() is being called with this algorithm, the parameter type should match the expectations. https://codereview.chromium.org/25906002/diff/678001/content/renderer/webcryp... content/renderer/webcrypto/webcrypto_impl.cc:88: const std::string& alg_id) { please tag this function as "const". https://codereview.chromium.org/25906002/diff/678001/content/renderer/webcryp... content/renderer/webcrypto/webcrypto_impl.cc:279: const WebKit::WebCryptoAlgorithm& input_algorithm, This name differs from the declaration. Can they be unified? You may also consider calling this algorithm_or_null to match importKey()
https://codereview.chromium.org/25906002/diff/338001/content/renderer/webcryp... File content/renderer/webcrypto/webcrypto_impl.cc (right): https://codereview.chromium.org/25906002/diff/338001/content/renderer/webcryp... content/renderer/webcrypto/webcrypto_impl.cc:103: WebCryptoImpl::~WebCryptoImpl() {} On 2013/11/04 21:01:15, eroman wrote: > On 2013/10/29 02:25:40, padolph wrote: > > On 2013/10/28 23:02:00, eroman wrote: > > > Doesn't seem like this is needed. > > > > I think the same, but clang complained loudly; not sure why. > > What is the error you get without this? I rebuilt again with clang and don't see the error now. Not sure what happened. https://codereview.chromium.org/25906002/diff/678001/content/renderer/webcryp... File content/renderer/webcrypto/webcrypto_impl.cc (right): https://codereview.chromium.org/25906002/diff/678001/content/renderer/webcryp... content/renderer/webcrypto/webcrypto_impl.cc:52: map_["HS256"] = On 2013/11/04 21:01:15, eroman wrote: > This is a duplication of the one above. Done. https://codereview.chromium.org/25906002/diff/678001/content/renderer/webcryp... content/renderer/webcrypto/webcrypto_impl.cc:56: map_["HS384"] = On 2013/11/04 21:01:15, eroman wrote: > This is a duplication of the key above. I presume you meant HS512 ? Done. https://codereview.chromium.org/25906002/diff/678001/content/renderer/webcryp... content/renderer/webcrypto/webcrypto_impl.cc:74: &BindAlgFactoryWithKeyLen<CreateAesGcmKeyGenAlgorithm, 128>; On 2013/11/04 21:01:15, eroman wrote: > Are you sure it is correct for these to be using the "Gen" version of the > algorithm parameters? According to the registration tables in webcrypto spec > importKey() doesn't expect any parameters on this (meaning the key length is > implicit from the raw key data). The final call to ImportKey() at the end of ImportKeyJwk() requires an Algorithm, which gets bound to the subsequently-created Key object. I guess you are saying the best choice for the parameters struct for that Algorithm is NULL. I'll do that, but I thought the Gen version made more sense because we know the key length. https://codereview.chromium.org/25906002/diff/678001/content/renderer/webcryp... content/renderer/webcrypto/webcrypto_impl.cc:76: &BindAlgFactoryWithKeyLen<CreateAesGcmKeyGenAlgorithm, 256>; On 2013/11/04 21:01:15, eroman wrote: > same comment here regarding the "Gen" version of the algorithm. Since > importKey() is being called with this algorithm, the parameter type should match > the expectations. See above. https://codereview.chromium.org/25906002/diff/678001/content/renderer/webcryp... content/renderer/webcrypto/webcrypto_impl.cc:88: const std::string& alg_id) { On 2013/11/04 21:01:15, eroman wrote: > please tag this function as "const". Done. https://codereview.chromium.org/25906002/diff/678001/content/renderer/webcryp... content/renderer/webcrypto/webcrypto_impl.cc:279: const WebKit::WebCryptoAlgorithm& input_algorithm, On 2013/11/04 21:01:15, eroman wrote: > This name differs from the declaration. Can they be unified? You may also > consider calling this algorithm_or_null to match importKey() Done.
https://codereview.chromium.org/25906002/diff/738001/content/renderer/webcryp... File content/renderer/webcrypto/webcrypto_impl.cc (right): https://codereview.chromium.org/25906002/diff/738001/content/renderer/webcryp... content/renderer/webcrypto/webcrypto_impl.cc:98: &BindAlgFactoryAlgorithmId<CreateAlgorithm, A128GCM...A512CBC are not registered in JWA, why is it OK to support them as JWK algorithm names?
https://codereview.chromium.org/25906002/diff/738001/content/renderer/webcryp... File content/renderer/webcrypto/webcrypto_impl.cc (right): https://codereview.chromium.org/25906002/diff/738001/content/renderer/webcryp... content/renderer/webcrypto/webcrypto_impl.cc:98: &BindAlgFactoryAlgorithmId<CreateAlgorithm, On 2013/11/05 08:52:15, aproskuryakov wrote: > A128GCM...A512CBC are not registered in JWA, why is it OK to support them as JWK > algorithm names? Mark Watson at Netflix is working with JOSE to get these added, though I am not sure of the status of that effort. The current Netflix protocol uses AES-CBC 128 with JWE, and non-authenticated AES encryption is a often used in enveloping. We have a similar issue in this code with the JWK 'extractable' field which is required by the Web Crypto API but not mentioned yet in the JWK spec. https://codereview.chromium.org/25906002/diff/738001/content/renderer/webcryp... content/renderer/webcrypto/webcrypto_impl.cc:167: // Returns true if the input JWK alg string is one of the AES types enumerated eroman: What do you think of this approach? These four functions are just here to identify JWK AES alg ID's and extract the key length from them. I like the way it reads and it works, but it is not very efficient and probably too general. Alternative approaches would be a simple if-else stringmatch chain, or a map, but these each bring along ugliness of their own.
(manual email because rietveld does not always send emails for my comments) 0 minutes ago #24 <https://codereview.chromium.org/25906002/#msg24> https://codereview.chromium.org/25906002/diff/738001/content/renderer/webcryp... <https://codereview.chromium.org/25906002/diff/738001/content/renderer/webcryp...> File content/renderer/webcrypto/webcrypto_impl.cc (right): https://codereview.chromium.org/25906002/diff/738001/content/renderer/webcryp... <https://codereview.chromium.org/25906002/diff/738001/content/renderer/webcryp...> content/renderer/webcrypto/webcrypto_impl.cc:98: &BindAlgFactoryAlgorithmId<CreateAlgorithm, On 2013/11/05 08:52:15, aproskuryakov wrote: > A128GCM...A512CBC are not registered in JWA, why is it OK to support them as JWK > algorithm names? Mark Watson at Netflix is working with JOSE to get these added, though I am not sure of the status of that effort. The current Netflix protocol uses AES-CBC 128 with JWE, and non-authenticated AES encryption is a often used in enveloping. We have a similar issue in this code with the JWK 'extractable' field which is required by the Web Crypto API but not mentioned yet in the JWK spec. https://codereview.chromium.org/25906002/diff/738001/content/renderer/webcryp... <https://codereview.chromium.org/25906002/diff/738001/content/renderer/webcryp...> content/renderer/webcrypto/webcrypto_impl.cc:167: // Returns true if the input JWK alg string is one of the AES types enumerated eroman: What do you think of this approach? These four functions are just here to identify JWK AES alg ID's and extract the key length from them. I like the way it reads and it works, but it is not very efficient and probably too general. Alternative approaches would be a simple if-else stringmatch chain, or a map, but these each bring along ugliness of their own. 2013/11/5 <aproskuryakov@gmail.com> > > https://codereview.chromium.org/25906002/diff/738001/ > content/renderer/webcrypto/webcrypto_impl.cc > File content/renderer/webcrypto/webcrypto_impl.cc (right): > > https://codereview.chromium.org/25906002/diff/738001/ > content/renderer/webcrypto/webcrypto_impl.cc#newcode98 > content/renderer/webcrypto/webcrypto_impl.cc:98: > &BindAlgFactoryAlgorithmId<CreateAlgorithm, > A128GCM...A512CBC are not registered in JWA, why is it OK to support > them as JWK algorithm names? > > https://codereview.chromium.org/25906002/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On Nov 5, 2013 8:37 AM, "Paul Adolph" <padolph@netflix.com> wrote: > > (manual email because rietveld does not always send emails for my comments) > > 0 minutes ago #24 > > https://codereview.chromium.org/25906002/diff/738001/content/renderer/webcryp. .. > File content/renderer/webcrypto/webcrypto_impl.cc (right): > > https://codereview.chromium.org/25906002/diff/738001/content/renderer/webcryp. .. > content/renderer/webcrypto/webcrypto_impl.cc:98: > &BindAlgFactoryAlgorithmId<CreateAlgorithm, > On 2013/11/05 08:52:15, aproskuryakov wrote: > > A128GCM...A512CBC are not registered in JWA, why is it OK to support them as > JWK > > algorithm names? > > Mark Watson at Netflix is working with JOSE to get these added, though I am not > sure of the status of that effort. The current Netflix protocol uses AES-CBC 128 > with JWE, and non-authenticated AES encryption is a often used in enveloping. > > We have a similar issue in this code with the JWK 'extractable' field which is > required by the Web Crypto API but not mentioned yet in the JWK spec. To be clear, nothing in Web Crypto requires this. This is merely a field that has been proposed (by Netflix) for the JWK registry. > > https://codereview.chromium.org/25906002/diff/738001/content/renderer/webcryp. .. > content/renderer/webcrypto/webcrypto_impl.cc:167: // Returns true if the input > JWK alg string is one of the AES types enumerated > > eroman: What do you think of this approach? These four functions are just here > to identify JWK AES alg ID's and extract the key length from them. I like the > way it reads and it works, but it is not very efficient and probably too > general. Alternative approaches would be a simple if-else stringmatch chain, or > a map, but these each bring along ugliness of their own. > > > > 2013/11/5 <aproskuryakov@gmail.com> > >> >> https://codereview.chromium.org/25906002/diff/738001/content/renderer/webcryp... >> File content/renderer/webcrypto/webcrypto_impl.cc (right): >> >> https://codereview.chromium.org/25906002/diff/738001/content/renderer/webcryp... >> content/renderer/webcrypto/webcrypto_impl.cc:98: >> &BindAlgFactoryAlgorithmId<CreateAlgorithm, >> A128GCM...A512CBC are not registered in JWA, why is it OK to support >> them as JWK algorithm names? >> >> https://codereview.chromium.org/25906002/ > > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
https://codereview.chromium.org/25906002/diff/678001/content/renderer/webcryp... File content/renderer/webcrypto/webcrypto_impl.cc (right): https://codereview.chromium.org/25906002/diff/678001/content/renderer/webcryp... content/renderer/webcrypto/webcrypto_impl.cc:74: &BindAlgFactoryWithKeyLen<CreateAesGcmKeyGenAlgorithm, 128>; On 2013/11/05 03:30:55, padolph wrote: > On 2013/11/04 21:01:15, eroman wrote: > > Are you sure it is correct for these to be using the "Gen" version of the > > algorithm parameters? According to the registration tables in webcrypto spec > > importKey() doesn't expect any parameters on this (meaning the key length is > > implicit from the raw key data). > > The final call to ImportKey() at the end of ImportKeyJwk() requires an > Algorithm, which gets bound to the subsequently-created Key object. I guess you > are saying the best choice for the parameters struct for that Algorithm is NULL. > I'll do that, but I thought the Gen version made more sense because we know the > key length. Background: The spec alas does not define what if any normalization happens to key.algorithm. I have raised this in: https://www.w3.org/Bugs/Public/show_bug.cgi?id=23098 In my opinion it would be inconsistent for some keys to have parameters like the key length, whereas others do not -- merely by virtue of whether they were created using generateKey() vs importKey(). Right now in Blink we reflect the generation parameters, but honestly it seems backwards. If you generated the key yourself then the generation parameters will be known to you and this is mostly just extra API cruft which requires a bunch more bindings work in Blink to support (i.e. define interfaces for each of the possible parameter types, sigh). I am hoping that key.algorithm will ultimately be defined to not reflect parameters, since their usefulness seems rather limited. https://codereview.chromium.org/25906002/diff/738001/content/renderer/webcryp... File content/renderer/webcrypto/webcrypto_impl.cc (right): https://codereview.chromium.org/25906002/diff/738001/content/renderer/webcryp... content/renderer/webcrypto/webcrypto_impl.cc:167: // Returns true if the input JWK alg string is one of the AES types enumerated On 2013/11/05 16:35:57, padolph wrote: > eroman: What do you think of this approach? These four functions are just here > to identify JWK AES alg ID's and extract the key length from them. I like the > way it reads and it works, but it is not very efficient and probably too > general. Alternative approaches would be a simple if-else stringmatch chain, or > a map, but these each bring along ugliness of their own. Verifying the key bit length is a good addition to this changelist, thanks for thinking to add that. That said, I do not like the approach taken in this patchset. I am very wary of introducing additional string parsing, since such code is complex and hurts both readability and future proofing. There are 2 alternative approaches I would like for you to consider: (1) Rather than parsing the algorithm identifier, _generate_ it. Generating is typically simpler than parsing. I believe all of this code could be replaced with: string expected_jwk_alg; const char* aes_alg_suffix = NULL; if (algorithm.id() == WebCryptoAlgorithmIdAesCbc) aes_alg_suffix = "CBC"; else if (algorithm.id() == WebCryptoAlgorithmIdAesGCM) aes_alg_suffix = "GCM"; if (aes_alg_suffix && !jwk_alg.empty()) { int size_in_bits = jwk_k_value.size() * 8; // TODO: test overflow. string expected_jwk_alg = StringPrintf("A%d%s", size_in_bits, aes_alg_suffix); if (expected_jkw_alg != jwk_alg) return false; // Incorrect number of bytes given by "k". } (2) While generating is an improvement over parsing, I think the best strategy is to enumerate these in the registration tables. That makes it easier to update the code as there is just one source of truth. The most efficient organization would be to have a single map from algorith_name --> "properties". But I would also be OK with leaving the algorithm factory map as is and having a second one that maps (aes)algorithm_name --> key_length_bits. My suggestion for moving forward is to remove this, and replace it with a TODO which we can address in a follow-up changelist. It is easier to review smaller changes, and I would hate to drag this current one out for too much longer! (For instance, I will invariably be asking for new tests to accompany this validation step :)
https://codereview.chromium.org/25906002/diff/678001/content/renderer/webcryp... File content/renderer/webcrypto/webcrypto_impl.cc (right): https://codereview.chromium.org/25906002/diff/678001/content/renderer/webcryp... content/renderer/webcrypto/webcrypto_impl.cc:74: &BindAlgFactoryWithKeyLen<CreateAesGcmKeyGenAlgorithm, 128>; On 2013/11/07 05:49:41, eroman wrote: > On 2013/11/05 03:30:55, padolph wrote: > > On 2013/11/04 21:01:15, eroman wrote: > > > Are you sure it is correct for these to be using the "Gen" version of the > > > algorithm parameters? According to the registration tables in webcrypto spec > > > importKey() doesn't expect any parameters on this (meaning the key length is > > > implicit from the raw key data). > > > > The final call to ImportKey() at the end of ImportKeyJwk() requires an > > Algorithm, which gets bound to the subsequently-created Key object. I guess > you > > are saying the best choice for the parameters struct for that Algorithm is > NULL. > > I'll do that, but I thought the Gen version made more sense because we know > the > > key length. > > Background: The spec alas does not define what if any normalization happens to > key.algorithm. I have raised this in: > > https://www.w3.org/Bugs/Public/show_bug.cgi?id=23098 > > In my opinion it would be inconsistent for some keys to have parameters like the > key length, whereas others do not -- merely by virtue of whether they were > created using generateKey() vs importKey(). Right now in Blink we reflect the > generation parameters, but honestly it seems backwards. If you generated the key > yourself then the generation parameters will be known to you and this is mostly > just extra API cruft which requires a bunch more bindings work in Blink to > support (i.e. define interfaces for each of the possible parameter types, sigh). > > I am hoping that key.algorithm will ultimately be defined to not reflect > parameters, since their usefulness seems rather limited. Thanks for the background. I struggled with this in NfWebCrypto and eventually just gave up on normalization, which we obviously can't do here. https://codereview.chromium.org/25906002/diff/738001/content/renderer/webcryp... File content/renderer/webcrypto/webcrypto_impl.cc (right): https://codereview.chromium.org/25906002/diff/738001/content/renderer/webcryp... content/renderer/webcrypto/webcrypto_impl.cc:167: // Returns true if the input JWK alg string is one of the AES types enumerated On 2013/11/07 05:49:41, eroman wrote: > On 2013/11/05 16:35:57, padolph wrote: > > eroman: What do you think of this approach? These four functions are just here > > to identify JWK AES alg ID's and extract the key length from them. I like the > > way it reads and it works, but it is not very efficient and probably too > > general. Alternative approaches would be a simple if-else stringmatch chain, > or > > a map, but these each bring along ugliness of their own. > > Verifying the key bit length is a good addition to this changelist, thanks for > thinking to add that. > > That said, I do not like the approach taken in this patchset. I am very wary of > introducing additional string parsing, since such code is complex and hurts both > readability and future proofing. > > There are 2 alternative approaches I would like for you to consider: > > (1) Rather than parsing the algorithm identifier, _generate_ it. Generating is > typically simpler than parsing. I believe all of this code could be replaced > with: > > string expected_jwk_alg; > const char* aes_alg_suffix = NULL; > if (algorithm.id() == WebCryptoAlgorithmIdAesCbc) > aes_alg_suffix = "CBC"; > else if (algorithm.id() == WebCryptoAlgorithmIdAesGCM) > aes_alg_suffix = "GCM"; > > if (aes_alg_suffix && !jwk_alg.empty()) { > int size_in_bits = jwk_k_value.size() * 8; // TODO: test overflow. > string expected_jwk_alg = StringPrintf("A%d%s", size_in_bits, > aes_alg_suffix); > if (expected_jkw_alg != jwk_alg) > return false; // Incorrect number of bytes given by "k". > } > > (2) While generating is an improvement over parsing, I think the best strategy > is to enumerate these in the registration tables. That makes it easier to update > the code as there is just one source of truth. The most efficient organization > would be to have a single map from algorith_name --> "properties". But I would > also be OK with leaving the algorithm factory map as is and having a second one > that maps (aes)algorithm_name --> key_length_bits. > > > My suggestion for moving forward is to remove this, and replace it with a TODO > which we can address in a follow-up changelist. It is easier to review smaller > changes, and I would hate to drag this current one out for too much longer! (For > instance, I will invariably be asking for new tests to accompany this validation > step :) Thanks for the reply. I did not think of the generation approach. That's a cool idea that I will file away in my toolbox. Yes, I was thinking a alg->keyLen map would be the best, but I was put off by having to make a whole new class for a LazyInstance impl of it. Combining with the existing map is less preferable, because only the AES JWK alg ID's contain the key length, and dislike sparsely populated properties structs. OK, I will take this stuff out and replace it with a TODO.
(manual email) On Wed, Nov 6, 2013 at 9:49 PM, <eroman@chromium.org> wrote: > > https://codereview.chromium.org/25906002/diff/678001/ > content/renderer/webcrypto/webcrypto_impl.cc > File content/renderer/webcrypto/webcrypto_impl.cc (right): > > https://codereview.chromium.org/25906002/diff/678001/ > content/renderer/webcrypto/webcrypto_impl.cc#newcode74 > content/renderer/webcrypto/webcrypto_impl.cc:74: > &BindAlgFactoryWithKeyLen<CreateAesGcmKeyGenAlgorithm, 128>; > On 2013/11/05 03:30:55, padolph wrote: > >> On 2013/11/04 21:01:15, eroman wrote: >> > Are you sure it is correct for these to be using the "Gen" version >> > of the > >> > algorithm parameters? According to the registration tables in >> > webcrypto spec > >> > importKey() doesn't expect any parameters on this (meaning the key >> > length is > >> > implicit from the raw key data). >> > > The final call to ImportKey() at the end of ImportKeyJwk() requires an >> Algorithm, which gets bound to the subsequently-created Key object. I >> > guess you > >> are saying the best choice for the parameters struct for that >> > Algorithm is NULL. > >> I'll do that, but I thought the Gen version made more sense because we >> > know the > >> key length. >> > > Background: The spec alas does not define what if any normalization > happens to key.algorithm. I have raised this in: > > https://www.w3.org/Bugs/Public/show_bug.cgi?id=23098 > > In my opinion it would be inconsistent for some keys to have parameters > like the key length, whereas others do not -- merely by virtue of > whether they were created using generateKey() vs importKey(). Right now > in Blink we reflect the generation parameters, but honestly it seems > backwards. If you generated the key yourself then the generation > parameters will be known to you and this is mostly just extra API cruft > which requires a bunch more bindings work in Blink to support (i.e. > define interfaces for each of the possible parameter types, sigh). > > I am hoping that key.algorithm will ultimately be defined to not reflect > parameters, since their usefulness seems rather limited. > > > https://codereview.chromium.org/25906002/diff/738001/ > content/renderer/webcrypto/webcrypto_impl.cc > File content/renderer/webcrypto/webcrypto_impl.cc (right): > > https://codereview.chromium.org/25906002/diff/738001/ > content/renderer/webcrypto/webcrypto_impl.cc#newcode167 > > content/renderer/webcrypto/webcrypto_impl.cc:167: // Returns true if the > input JWK alg string is one of the AES types enumerated > On 2013/11/05 16:35:57, padolph wrote: > >> eroman: What do you think of this approach? These four functions are >> > just here > >> to identify JWK AES alg ID's and extract the key length from them. I >> > like the > >> way it reads and it works, but it is not very efficient and probably >> > too > >> general. Alternative approaches would be a simple if-else stringmatch >> > chain, or > >> a map, but these each bring along ugliness of their own. >> > > Verifying the key bit length is a good addition to this changelist, > thanks for thinking to add that. > > That said, I do not like the approach taken in this patchset. I am very > wary of introducing additional string parsing, since such code is > complex and hurts both readability and future proofing. > > There are 2 alternative approaches I would like for you to consider: > > (1) Rather than parsing the algorithm identifier, _generate_ it. > Generating is typically simpler than parsing. I believe all of this code > could be replaced with: > > string expected_jwk_alg; > const char* aes_alg_suffix = NULL; > if (algorithm.id() == WebCryptoAlgorithmIdAesCbc) > aes_alg_suffix = "CBC"; > else if (algorithm.id() == WebCryptoAlgorithmIdAesGCM) > aes_alg_suffix = "GCM"; > > if (aes_alg_suffix && !jwk_alg.empty()) { > int size_in_bits = jwk_k_value.size() * 8; // TODO: test overflow. > string expected_jwk_alg = StringPrintf("A%d%s", size_in_bits, > aes_alg_suffix); > if (expected_jkw_alg != jwk_alg) > return false; // Incorrect number of bytes given by "k". > } > > (2) While generating is an improvement over parsing, I think the best > strategy is to enumerate these in the registration tables. That makes it > easier to update the code as there is just one source of truth. The most > efficient organization would be to have a single map from algorith_name > --> "properties". But I would also be OK with leaving the algorithm > factory map as is and having a second one that maps (aes)algorithm_name > --> key_length_bits. > > > My suggestion for moving forward is to remove this, and replace it with > a TODO which we can address in a follow-up changelist. It is easier to > review smaller changes, and I would hate to drag this current one out > for too much longer! (For instance, I will invariably be asking for new > tests to accompany this validation step :) > > https://codereview.chromium.org/25906002/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
LGTM for patchset 17 after addressing these comments! There is more work to do, but I would prefer to tackle it as follow-up changelists. Once you have addressed these comments and re-uploaded, please ask either jamesr/jochen to approve content/content_renderer.gypi https://codereview.chromium.org/25906002/diff/738001/content/renderer/webcryp... File content/renderer/webcrypto/webcrypto_impl.cc (right): https://codereview.chromium.org/25906002/diff/738001/content/renderer/webcryp... content/renderer/webcrypto/webcrypto_impl.cc:167: // Returns true if the input JWK alg string is one of the AES types enumerated On 2013/11/07 17:58:21, padolph wrote: > On 2013/11/07 05:49:41, eroman wrote: > > On 2013/11/05 16:35:57, padolph wrote: > > > eroman: What do you think of this approach? These four functions are just > here > > > to identify JWK AES alg ID's and extract the key length from them. I like > the > > > way it reads and it works, but it is not very efficient and probably too > > > general. Alternative approaches would be a simple if-else stringmatch chain, > > or > > > a map, but these each bring along ugliness of their own. > > > > Verifying the key bit length is a good addition to this changelist, thanks for > > thinking to add that. > > > > That said, I do not like the approach taken in this patchset. I am very wary > of > > introducing additional string parsing, since such code is complex and hurts > both > > readability and future proofing. > > > > There are 2 alternative approaches I would like for you to consider: > > > > (1) Rather than parsing the algorithm identifier, _generate_ it. Generating is > > typically simpler than parsing. I believe all of this code could be replaced > > with: > > > > string expected_jwk_alg; > > const char* aes_alg_suffix = NULL; > > if (algorithm.id() == WebCryptoAlgorithmIdAesCbc) > > aes_alg_suffix = "CBC"; > > else if (algorithm.id() == WebCryptoAlgorithmIdAesGCM) > > aes_alg_suffix = "GCM"; > > > > if (aes_alg_suffix && !jwk_alg.empty()) { > > int size_in_bits = jwk_k_value.size() * 8; // TODO: test overflow. > > string expected_jwk_alg = StringPrintf("A%d%s", size_in_bits, > > aes_alg_suffix); > > if (expected_jkw_alg != jwk_alg) > > return false; // Incorrect number of bytes given by "k". > > } > > > > (2) While generating is an improvement over parsing, I think the best strategy > > is to enumerate these in the registration tables. That makes it easier to > update > > the code as there is just one source of truth. The most efficient organization > > would be to have a single map from algorith_name --> "properties". But I would > > also be OK with leaving the algorithm factory map as is and having a second > one > > that maps (aes)algorithm_name --> key_length_bits. > > > > > > My suggestion for moving forward is to remove this, and replace it with a TODO > > which we can address in a follow-up changelist. It is easier to review smaller > > changes, and I would hate to drag this current one out for too much longer! > (For > > instance, I will invariably be asking for new tests to accompany this > validation > > step :) > > Thanks for the reply. I did not think of the generation approach. That's a cool > idea that I will file away in my toolbox. > > Yes, I was thinking a alg->keyLen map would be the best, but I was put off by > having to make a whole new class for a LazyInstance impl of it. Combining with > the existing map is less preferable, because only the AES JWK alg ID's contain > the key length, and dislike sparsely populated properties structs. > > OK, I will take this stuff out and replace it with a TODO. Sounds good. Note also that you can rename JwkAlgorithmFactoryMap to something more general (like maybe JwkRegistry) and shove the extra map + method in there. https://codereview.chromium.org/25906002/diff/948001/content/renderer/webcryp... File content/renderer/webcrypto/webcrypto_impl.cc (right): https://codereview.chromium.org/25906002/diff/948001/content/renderer/webcryp... content/renderer/webcrypto/webcrypto_impl.cc:10: #include <sstream> remove https://codereview.chromium.org/25906002/diff/948001/content/renderer/webcryp... content/renderer/webcrypto/webcrypto_impl.cc:80: map_["A128KW"] = &WebKit::WebCryptoAlgorithm::createNull; Either add a TODO for these, or delete the "null" entries. https://codereview.chromium.org/25906002/diff/948001/content/renderer/webcryp... content/renderer/webcrypto/webcrypto_impl.cc:327: // - "use" (Key Use) Parameter, OPTIONAL It is worth also mentioning what OPTIONAL means. (Since right now optional parameters are disregarded when their type mismatches). I believe we will want to revisit or clarify this later. https://codereview.chromium.org/25906002/diff/948001/content/renderer/webcryp... content/renderer/webcrypto/webcrypto_impl.cc:425: if (!key_data || !key_data_size) Same comment as on the other CL, I would prefer this as: if (!key_data_size) return false DCHECK(key_data); To reinforce to the reader that NULL key data is not the right way to call it. https://codereview.chromium.org/25906002/diff/948001/content/renderer/webcryp... content/renderer/webcrypto/webcrypto_impl.cc:444: if (jwk_extractable_value != extractable) To be "consistent", it seems like it should alway be OK for the caller to say extractable=false. For instance if the JWK says extractable=true, I think it is legitimate for the caller to import it with extractable=false, and for the final key to be un-extractable. https://codereview.chromium.org/25906002/diff/948001/content/renderer/webcryp... content/renderer/webcrypto/webcrypto_impl.cc:477: return false; // case 3 Please indent this comment by just 2 spaces (style rules, not mine :) https://codereview.chromium.org/25906002/diff/948001/content/renderer/webcryp... content/renderer/webcrypto/webcrypto_impl.cc:483: return false; // case 5 same https://codereview.chromium.org/25906002/diff/948001/content/renderer/webcryp... File content/renderer/webcrypto/webcrypto_impl_nss.cc (right): https://codereview.chromium.org/25906002/diff/948001/content/renderer/webcryp... content/renderer/webcrypto/webcrypto_impl_nss.cc:222: bool BigIntegerToLong(const uint8* data, Side-comment: this would be a good candidate to move to the util class. Not necessarily because it will be used by OpenSSL, but because it will allow directly testing the function from the unit-tests. As I understand the validation on this is pretty weak right now. No change necessary; I may do this once the new file is in place. https://codereview.chromium.org/25906002/diff/948001/content/renderer/webcryp... File content/renderer/webcrypto/webcrypto_impl_unittest.cc (right): https://codereview.chromium.org/25906002/diff/948001/content/renderer/webcryp... content/renderer/webcrypto/webcrypto_impl_unittest.cc:69: content::Start(public_exponent), remove the "content::" prefix (we are already in content namespace). https://codereview.chromium.org/25906002/diff/948001/content/renderer/webcryp... content/renderer/webcrypto/webcrypto_impl_unittest.cc:693: CreateAesCbcAlgorithm(std::vector<uint8>()); or CreateAlgorithm(WebKit::WebCryptoAlgorithmIdAesCbc), since there doesn't need to be any parameters. https://codereview.chromium.org/25906002/diff/948001/content/renderer/webcryp... content/renderer/webcrypto/webcrypto_impl_unittest.cc:820: // Fail: Input extractable (false) is inconsistent with JWK value (true). Thanks for the test! As mentioned in other comment, my interpretation is that this is in fact "consistent". If you disagree then no change necessary. https://codereview.chromium.org/25906002/diff/948001/content/renderer/webcryp... File content/renderer/webcrypto/webcrypto_util.cc (right): https://codereview.chromium.org/25906002/diff/948001/content/renderer/webcryp... content/renderer/webcrypto/webcrypto_util.cc:99: return WebKit::WebCryptoAlgorithm::adoptParamsAndCreate( [optional] I propose adding DCHECK(IsHashAlgorithm(hash_id)); https://codereview.chromium.org/25906002/diff/948001/content/renderer/webcryp... content/renderer/webcrypto/webcrypto_util.cc:108: return WebKit::WebCryptoAlgorithm::adoptParamsAndCreate( [optional] I propose adding DCHECK(IsHashAlgorithm(hash_id)); https://codereview.chromium.org/25906002/diff/948001/content/renderer/webcryp... content/renderer/webcrypto/webcrypto_util.cc:111: CreateAlgorithm(hash_id), (hash_length != 0), hash_length)); 'hash_length" isn't the right name, this is more like key_length_bytes https://codereview.chromium.org/25906002/diff/948001/content/renderer/webcryp... content/renderer/webcrypto/webcrypto_util.cc:115: WebKit::WebCryptoAlgorithmId hash_algorithm_id) { hash_id https://codereview.chromium.org/25906002/diff/948001/content/renderer/webcryp... content/renderer/webcrypto/webcrypto_util.cc:116: return WebKit::WebCryptoAlgorithm::adoptParamsAndCreate( [optional] I propose adding DCHECK(IsHashAlgorithm(hash_id)); https://codereview.chromium.org/25906002/diff/948001/content/renderer/webcryp... content/renderer/webcrypto/webcrypto_util.cc:122: WebKit::WebCryptoAlgorithmId hash_algorithm_id) { hash_id https://codereview.chromium.org/25906002/diff/948001/content/renderer/webcryp... content/renderer/webcrypto/webcrypto_util.cc:123: return WebKit::WebCryptoAlgorithm::adoptParamsAndCreate( [optional] I propose adding DCHECK(IsHashAlgorithm(hash_id)); https://codereview.chromium.org/25906002/diff/948001/content/renderer/webcryp... File content/renderer/webcrypto/webcrypto_util.h (right): https://codereview.chromium.org/25906002/diff/948001/content/renderer/webcryp... content/renderer/webcrypto/webcrypto_util.h:35: WebKit::WebCryptoAlgorithm GetInnerHashAlgorithm( Can you add a comment similar to: // Returns the "hash" property for an algorithm if it exists, otherwise null. https://codereview.chromium.org/25906002/diff/948001/content/renderer/webcryp... content/renderer/webcrypto/webcrypto_util.h:38: WebKit::WebCryptoAlgorithm CreateAlgorithm(WebKit::WebCryptoAlgorithmId id); // Creates a WebCryptoAlgorithm without any parameters. https://codereview.chromium.org/25906002/diff/948001/content/renderer/webcryp... content/renderer/webcrypto/webcrypto_util.h:40: WebKit::WebCryptoAlgorithm CreateHmacAlgorithmByDigestLen( // It is an error to call this with an unsupported number of bits. https://codereview.chromium.org/25906002/diff/948001/content/renderer/webcryp... content/renderer/webcrypto/webcrypto_util.h:43: WebKit::WebCryptoAlgorithm CreateHmacAlgorithmByHashId( // It is an error to call this with a hash_id that is not SHA-* https://codereview.chromium.org/25906002/diff/948001/content/renderer/webcryp... content/renderer/webcrypto/webcrypto_util.h:46: WebKit::WebCryptoAlgorithm CreateHmacKeyGenAlgorithm( // It is an error to call this with a hash_id that is not a SHA-* // key_length_bytes is optional, and the value of 0 will be interpreted as "not specified" https://codereview.chromium.org/25906002/diff/948001/content/renderer/webcryp... content/renderer/webcrypto/webcrypto_util.h:48: unsigned hash_length); IMPORTANT: this parameter should be named key_length_bytes https://codereview.chromium.org/25906002/diff/948001/content/renderer/webcryp... content/renderer/webcrypto/webcrypto_util.h:50: WebKit::WebCryptoAlgorithm CreateRsaSsaAlgorithm( // It is an error to call this with a hash_id that is not a SHA-* https://codereview.chromium.org/25906002/diff/948001/content/renderer/webcryp... content/renderer/webcrypto/webcrypto_util.h:51: WebKit::WebCryptoAlgorithmId hash_algorithm_id); nit: elsewhere this is called hash_id https://codereview.chromium.org/25906002/diff/948001/content/renderer/webcryp... content/renderer/webcrypto/webcrypto_util.h:53: WebKit::WebCryptoAlgorithm CreateRsaOaepAlgorithm( // It is an error to call this with a hash_id that is not a SHA-* https://codereview.chromium.org/25906002/diff/948001/content/renderer/webcryp... content/renderer/webcrypto/webcrypto_util.h:54: WebKit::WebCryptoAlgorithmId hash_algorithm_id); nit: elsewhere this is called hash_id https://codereview.chromium.org/25906002/diff/948001/content/renderer/webcryp... content/renderer/webcrypto/webcrypto_util.h:63: WebKit::WebCryptoAlgorithm CreateAesCbcKeyGenAlgorithm(unsigned short length); would be good to name parameter length_bits or key_length_bits. https://codereview.chromium.org/25906002/diff/948001/content/renderer/webcryp... content/renderer/webcrypto/webcrypto_util.h:65: WebKit::WebCryptoAlgorithm CreateAesGcmKeyGenAlgorithm(unsigned short length); ditto.
> only the AES JWK alg ID's contain the key length I don't think that's accurate, HMAC algorithm identifiers have MUST level requirements on key length: " A key of the same size as the hash output (for instance, 256 bits for "HS256") or larger MUST be used with this algorithm. "
https://codereview.chromium.org/25906002/diff/738001/content/renderer/webcryp... File content/renderer/webcrypto/webcrypto_impl.cc (right): https://codereview.chromium.org/25906002/diff/738001/content/renderer/webcryp... content/renderer/webcrypto/webcrypto_impl.cc:98: &BindAlgFactoryAlgorithmId<CreateAlgorithm, One reason why I'm asking is that there is no such thing as AES-384 or AES-512 as far as I know. There are only 128, 192 and 256 ones. So, A512CBC doesn't appear to make any sense as an algorithm identifier. I tried to find the proposal to JOSE, and couldn't. Could you please post a link to it?
On 2013/11/07 21:45:39, aproskuryakov wrote: > > only the AES JWK alg ID's contain the key length > > I don't think that's accurate, HMAC algorithm identifiers have MUST level > requirements on key length: > > " > A key of the same size as the hash output (for instance, 256 bits for > "HS256") or larger MUST be used with this algorithm. > " Thank you. I fixed the TODO.
On 2013/11/07 22:05:41, aproskuryakov wrote: > https://codereview.chromium.org/25906002/diff/738001/content/renderer/webcryp... > File content/renderer/webcrypto/webcrypto_impl.cc (right): > > https://codereview.chromium.org/25906002/diff/738001/content/renderer/webcryp... > content/renderer/webcrypto/webcrypto_impl.cc:98: > &BindAlgFactoryAlgorithmId<CreateAlgorithm, > One reason why I'm asking is that there is no such thing as AES-384 or AES-512 > as far as I know. There are only 128, 192 and 256 ones. > > So, A512CBC doesn't appear to make any sense as an algorithm identifier. > > I tried to find the proposal to JOSE, and couldn't. Could you please post a link > to it? My mistake. I fixed the error. Thank you.
https://codereview.chromium.org/25906002/diff/948001/content/renderer/webcryp... File content/renderer/webcrypto/webcrypto_impl.cc (right): https://codereview.chromium.org/25906002/diff/948001/content/renderer/webcryp... content/renderer/webcrypto/webcrypto_impl.cc:10: #include <sstream> On 2013/11/07 20:54:37, eroman wrote: > remove Done. https://codereview.chromium.org/25906002/diff/948001/content/renderer/webcryp... content/renderer/webcrypto/webcrypto_impl.cc:80: map_["A128KW"] = &WebKit::WebCryptoAlgorithm::createNull; On 2013/11/07 20:54:37, eroman wrote: > Either add a TODO for these, or delete the "null" entries. Added a TODO. rsleevi: Would you consider adding AES-KW as an Algorithm to the Web Crypto API spec? Is this something I can enter a bug for on my own, or is this a Mark Watson thing? Please advise. https://codereview.chromium.org/25906002/diff/948001/content/renderer/webcryp... content/renderer/webcrypto/webcrypto_impl.cc:327: // - "use" (Key Use) Parameter, OPTIONAL On 2013/11/07 20:54:37, eroman wrote: > It is worth also mentioning what OPTIONAL means. (Since right now optional > parameters are disregarded when their type mismatches). I believe we will want > to revisit or clarify this later. I added some text to clarify this. Please take a look. https://codereview.chromium.org/25906002/diff/948001/content/renderer/webcryp... content/renderer/webcrypto/webcrypto_impl.cc:425: if (!key_data || !key_data_size) On 2013/11/07 20:54:37, eroman wrote: > Same comment as on the other CL, I would prefer this as: > > if (!key_data_size) > return false > DCHECK(key_data); > > To reinforce to the reader that NULL key data is not the right way to call it. Done. https://codereview.chromium.org/25906002/diff/948001/content/renderer/webcryp... content/renderer/webcrypto/webcrypto_impl.cc:444: if (jwk_extractable_value != extractable) On 2013/11/07 20:54:37, eroman wrote: > To be "consistent", it seems like it should alway be OK for the caller to say > extractable=false. For instance if the JWK says extractable=true, I think it is > legitimate for the caller to import it with extractable=false, and for the final > key to be un-extractable. Done. https://codereview.chromium.org/25906002/diff/948001/content/renderer/webcryp... content/renderer/webcrypto/webcrypto_impl.cc:477: return false; // case 3 On 2013/11/07 20:54:37, eroman wrote: > Please indent this comment by just 2 spaces (style rules, not mine :) Done. https://codereview.chromium.org/25906002/diff/948001/content/renderer/webcryp... content/renderer/webcrypto/webcrypto_impl.cc:483: return false; // case 5 On 2013/11/07 20:54:37, eroman wrote: > same Done. https://codereview.chromium.org/25906002/diff/948001/content/renderer/webcryp... File content/renderer/webcrypto/webcrypto_impl_unittest.cc (right): https://codereview.chromium.org/25906002/diff/948001/content/renderer/webcryp... content/renderer/webcrypto/webcrypto_impl_unittest.cc:693: CreateAesCbcAlgorithm(std::vector<uint8>()); On 2013/11/07 20:54:37, eroman wrote: > or CreateAlgorithm(WebKit::WebCryptoAlgorithmIdAesCbc), since there doesn't need > to be any parameters. Done. https://codereview.chromium.org/25906002/diff/948001/content/renderer/webcryp... content/renderer/webcrypto/webcrypto_impl_unittest.cc:820: // Fail: Input extractable (false) is inconsistent with JWK value (true). On 2013/11/07 20:54:37, eroman wrote: > Thanks for the test! As mentioned in other comment, my interpretation is that > this is in fact "consistent". > > If you disagree then no change necessary. I changed the logic and added new tests that match your interpretation. Please take a look. https://codereview.chromium.org/25906002/diff/948001/content/renderer/webcryp... File content/renderer/webcrypto/webcrypto_util.cc (right): https://codereview.chromium.org/25906002/diff/948001/content/renderer/webcryp... content/renderer/webcrypto/webcrypto_util.cc:99: return WebKit::WebCryptoAlgorithm::adoptParamsAndCreate( On 2013/11/07 20:54:37, eroman wrote: > [optional] I propose adding DCHECK(IsHashAlgorithm(hash_id)); Done. https://codereview.chromium.org/25906002/diff/948001/content/renderer/webcryp... content/renderer/webcrypto/webcrypto_util.cc:108: return WebKit::WebCryptoAlgorithm::adoptParamsAndCreate( On 2013/11/07 20:54:37, eroman wrote: > [optional] I propose adding DCHECK(IsHashAlgorithm(hash_id)); Done. https://codereview.chromium.org/25906002/diff/948001/content/renderer/webcryp... content/renderer/webcrypto/webcrypto_util.cc:111: CreateAlgorithm(hash_id), (hash_length != 0), hash_length)); On 2013/11/07 20:54:37, eroman wrote: > 'hash_length" isn't the right name, this is more like key_length_bytes Done. https://codereview.chromium.org/25906002/diff/948001/content/renderer/webcryp... content/renderer/webcrypto/webcrypto_util.cc:115: WebKit::WebCryptoAlgorithmId hash_algorithm_id) { On 2013/11/07 20:54:37, eroman wrote: > hash_id Done. https://codereview.chromium.org/25906002/diff/948001/content/renderer/webcryp... content/renderer/webcrypto/webcrypto_util.cc:116: return WebKit::WebCryptoAlgorithm::adoptParamsAndCreate( On 2013/11/07 20:54:37, eroman wrote: > [optional] I propose adding DCHECK(IsHashAlgorithm(hash_id)); Done. https://codereview.chromium.org/25906002/diff/948001/content/renderer/webcryp... content/renderer/webcrypto/webcrypto_util.cc:122: WebKit::WebCryptoAlgorithmId hash_algorithm_id) { On 2013/11/07 20:54:37, eroman wrote: > hash_id Done. https://codereview.chromium.org/25906002/diff/948001/content/renderer/webcryp... content/renderer/webcrypto/webcrypto_util.cc:123: return WebKit::WebCryptoAlgorithm::adoptParamsAndCreate( On 2013/11/07 20:54:37, eroman wrote: > [optional] I propose adding DCHECK(IsHashAlgorithm(hash_id)); Done. https://codereview.chromium.org/25906002/diff/948001/content/renderer/webcryp... File content/renderer/webcrypto/webcrypto_util.h (right): https://codereview.chromium.org/25906002/diff/948001/content/renderer/webcryp... content/renderer/webcrypto/webcrypto_util.h:35: WebKit::WebCryptoAlgorithm GetInnerHashAlgorithm( On 2013/11/07 20:54:37, eroman wrote: > Can you add a comment similar to: > > // Returns the "hash" property for an algorithm if it exists, otherwise null. Done. https://codereview.chromium.org/25906002/diff/948001/content/renderer/webcryp... content/renderer/webcrypto/webcrypto_util.h:38: WebKit::WebCryptoAlgorithm CreateAlgorithm(WebKit::WebCryptoAlgorithmId id); On 2013/11/07 20:54:37, eroman wrote: > // Creates a WebCryptoAlgorithm without any parameters. Done. https://codereview.chromium.org/25906002/diff/948001/content/renderer/webcryp... content/renderer/webcrypto/webcrypto_util.h:40: WebKit::WebCryptoAlgorithm CreateHmacAlgorithmByDigestLen( On 2013/11/07 20:54:37, eroman wrote: > // It is an error to call this with an unsupported number of bits. Done. https://codereview.chromium.org/25906002/diff/948001/content/renderer/webcryp... content/renderer/webcrypto/webcrypto_util.h:43: WebKit::WebCryptoAlgorithm CreateHmacAlgorithmByHashId( On 2013/11/07 20:54:37, eroman wrote: > // It is an error to call this with a hash_id that is not SHA-* Done. https://codereview.chromium.org/25906002/diff/948001/content/renderer/webcryp... content/renderer/webcrypto/webcrypto_util.h:46: WebKit::WebCryptoAlgorithm CreateHmacKeyGenAlgorithm( On 2013/11/07 20:54:37, eroman wrote: > // It is an error to call this with a hash_id that is not a SHA-* > // key_length_bytes is optional, and the value of 0 will be interpreted as "not > specified" Done. https://codereview.chromium.org/25906002/diff/948001/content/renderer/webcryp... content/renderer/webcrypto/webcrypto_util.h:48: unsigned hash_length); On 2013/11/07 20:54:37, eroman wrote: > IMPORTANT: this parameter should be named key_length_bytes Done. https://codereview.chromium.org/25906002/diff/948001/content/renderer/webcryp... content/renderer/webcrypto/webcrypto_util.h:50: WebKit::WebCryptoAlgorithm CreateRsaSsaAlgorithm( On 2013/11/07 20:54:37, eroman wrote: > // It is an error to call this with a hash_id that is not a SHA-* Done. https://codereview.chromium.org/25906002/diff/948001/content/renderer/webcryp... content/renderer/webcrypto/webcrypto_util.h:51: WebKit::WebCryptoAlgorithmId hash_algorithm_id); On 2013/11/07 20:54:37, eroman wrote: > nit: elsewhere this is called hash_id Done. https://codereview.chromium.org/25906002/diff/948001/content/renderer/webcryp... content/renderer/webcrypto/webcrypto_util.h:53: WebKit::WebCryptoAlgorithm CreateRsaOaepAlgorithm( On 2013/11/07 20:54:37, eroman wrote: > // It is an error to call this with a hash_id that is not a SHA-* Done. https://codereview.chromium.org/25906002/diff/948001/content/renderer/webcryp... content/renderer/webcrypto/webcrypto_util.h:54: WebKit::WebCryptoAlgorithmId hash_algorithm_id); On 2013/11/07 20:54:37, eroman wrote: > nit: elsewhere this is called hash_id Done.
lgtm https://codereview.chromium.org/25906002/diff/948001/content/renderer/webcryp... File content/renderer/webcrypto/webcrypto_impl.cc (right): https://codereview.chromium.org/25906002/diff/948001/content/renderer/webcryp... content/renderer/webcrypto/webcrypto_impl.cc:477: return false; // case 3 On 2013/11/09 00:33:38, padolph wrote: > On 2013/11/07 20:54:37, eroman wrote: > > Please indent this comment by just 2 spaces (style rules, not mine :) > > Done. not quite. What I meant is: return false; // case 3 https://codereview.chromium.org/25906002/diff/948001/content/renderer/webcryp... content/renderer/webcrypto/webcrypto_impl.cc:483: return false; // case 5 On 2013/11/09 00:33:38, padolph wrote: > On 2013/11/07 20:54:37, eroman wrote: > > same > > Done. Same. https://codereview.chromium.org/25906002/diff/948001/content/renderer/webcryp... File content/renderer/webcrypto/webcrypto_impl_unittest.cc (right): https://codereview.chromium.org/25906002/diff/948001/content/renderer/webcryp... content/renderer/webcrypto/webcrypto_impl_unittest.cc:69: content::Start(public_exponent), On 2013/11/07 20:54:37, eroman wrote: > remove the "content::" prefix (we are already in content namespace). Ah my mistake. Thanks for wrapping this section in content:: too. https://codereview.chromium.org/25906002/diff/1058001/content/renderer/webcry... File content/renderer/webcrypto/webcrypto_impl.cc (right): https://codereview.chromium.org/25906002/diff/1058001/content/renderer/webcry... content/renderer/webcrypto/webcrypto_impl.cc:426: // may be different the corresponding values inside the JWK. The Web Crypto "different the" --> "different than the" https://codereview.chromium.org/25906002/diff/1058001/content/renderer/webcry... File content/renderer/webcrypto/webcrypto_util.h (right): https://codereview.chromium.org/25906002/diff/1058001/content/renderer/webcry... content/renderer/webcrypto/webcrypto_util.h:35: // Returns the "hash" param for an algorithm if it exists, otherwise return nit: "otherwise return" --> "otherwise returns" https://codereview.chromium.org/25906002/diff/1058001/content/renderer/webcry... content/renderer/webcrypto/webcrypto_util.h:57: // The key_length_bytes parameter is optional, with zero meaning unspecified. Good comments. https://codereview.chromium.org/25906002/diff/1088001/content/renderer/webcry... File content/renderer/webcrypto/webcrypto_impl.cc (right): https://codereview.chromium.org/25906002/diff/1088001/content/renderer/webcry... content/renderer/webcrypto/webcrypto_impl.cc:100: blink::WebCryptoAlgorithm CreateAlgorithmFromName( Thanks for splitting up the patchsets with the rebase separate! Definitely makes it easier to review the changes without the WebKit --> blink noise.
@jochen or @jamesr: Could you review content/content_renderer.gypi? Thanks!
https://codereview.chromium.org/25906002/diff/1058001/content/renderer/webcry... File content/renderer/webcrypto/webcrypto_impl.cc (right): https://codereview.chromium.org/25906002/diff/1058001/content/renderer/webcry... content/renderer/webcrypto/webcrypto_impl.cc:426: // may be different the corresponding values inside the JWK. The Web Crypto On 2013/11/09 02:22:14, eroman wrote: > "different the" --> "different than the" Done. https://codereview.chromium.org/25906002/diff/1058001/content/renderer/webcry... File content/renderer/webcrypto/webcrypto_util.h (right): https://codereview.chromium.org/25906002/diff/1058001/content/renderer/webcry... content/renderer/webcrypto/webcrypto_util.h:35: // Returns the "hash" param for an algorithm if it exists, otherwise return On 2013/11/09 02:22:14, eroman wrote: > nit: "otherwise return" --> "otherwise returns" Done.
https://codereview.chromium.org/25906002/diff/948001/content/renderer/webcryp... File content/renderer/webcrypto/webcrypto_impl.cc (right): https://codereview.chromium.org/25906002/diff/948001/content/renderer/webcryp... content/renderer/webcrypto/webcrypto_impl.cc:477: return false; // case 3 On 2013/11/09 02:22:14, eroman wrote: > On 2013/11/09 00:33:38, padolph wrote: > > On 2013/11/07 20:54:37, eroman wrote: > > > Please indent this comment by just 2 spaces (style rules, not mine :) > > > > Done. > > not quite. What I meant is: > return false; // case 3 Ic. The current format is what clang-format told me to do. But will fix. https://codereview.chromium.org/25906002/diff/948001/content/renderer/webcryp... content/renderer/webcrypto/webcrypto_impl.cc:483: return false; // case 5 On 2013/11/09 02:22:14, eroman wrote: > On 2013/11/09 00:33:38, padolph wrote: > > On 2013/11/07 20:54:37, eroman wrote: > > > same > > > > Done. > > Same. Done.
This appears to grab a lot of the content:: namespace for free functions. Is there nowhere more specific where these could live? https://codereview.chromium.org/25906002/diff/1288001/content/renderer/webcry... File content/renderer/webcrypto/webcrypto_util.h (right): https://codereview.chromium.org/25906002/diff/1288001/content/renderer/webcry... content/renderer/webcrypto/webcrypto_util.h:20: const uint8* Start(const std::vector<uint8>& data); why is this not just std::vector<T>::data()? this is too generic of a function name and marginal utility to make it be a direct free function in the content:: namespace. I'd be really surprised if this isn't an ODR violation either today or in the near future
https://codereview.chromium.org/25906002/diff/1288001/content/renderer/webcry... File content/renderer/webcrypto/webcrypto_util.h (right): https://codereview.chromium.org/25906002/diff/1288001/content/renderer/webcry... content/renderer/webcrypto/webcrypto_util.h:20: const uint8* Start(const std::vector<uint8>& data); On 2013/11/12 02:33:09, jamesr wrote: > why is this not just std::vector<T>::data()? The vector::data() method is new to C++11 and causes build failures on platforms that do not support C++11 (e.g. Android). > this is too generic of a function name and marginal utility to make it be a > direct free function in the content:: namespace. I'd be really surprised if > this isn't an ODR violation either today or in the near future Good point about the name. eroman: What about renaming this to something that includes "vector", and templating the type?
On 2013/11/12 02:33:08, jamesr wrote: > This appears to grab a lot of the content:: namespace for free functions. Is > there nowhere more specific where these could live? All of the webcrypto stuff lives in content::, so this is consistent with that. eroman: Should we push all the webcrypto stuff, including the utils, into its own webcrypto:: namespace?
(manual email because rietveld does not send emails for me) 0 minutes ago #41 https://codereview.chromium.org/25906002/diff/1288001/content/renderer/webcry... File content/renderer/webcrypto/webcrypto_util.h (right): https://codereview.chromium.org/25906002/diff/1288001/content/renderer/webcry... content/renderer/webcrypto/webcrypto_util.h:20: const uint8* Start(const std::vector<uint8>& data); On 2013/11/12 02:33:09, jamesr wrote: > why is this not just std::vector<T>::data()? The vector::data() method is new to C++11 and causes build failures on platforms that do not support C++11 (e.g. Android). > this is too generic of a function name and marginal utility to make it be a > direct free function in the content:: namespace. I'd be really surprised if > this isn't an ODR violation either today or in the near future Good point about the name. eroman: What about renaming this to something that includes "vector", and templating the type? Reply me 0 minutes ago #42 On 2013/11/12 02:33:08, jamesr wrote: > This appears to grab a lot of the content:: namespace for free functions. Is > there nowhere more specific where these could live? All of the webcrypto stuff lives in content::, so this is consistent with that. eroman: Should we push all the webcrypto stuff, including the utils, into its own webcrypto:: namespace? On Mon, Nov 11, 2013 at 6:33 PM, <jamesr@chromium.org> wrote: > This appears to grab a lot of the content:: namespace for free functions. > Is > there nowhere more specific where these could live? > > > https://codereview.chromium.org/25906002/diff/1288001/content/renderer/webcry... > File content/renderer/webcrypto/webcrypto_util.h (right): > > https://codereview.chromium.org/25906002/diff/1288001/content/renderer/webcry... > content/renderer/webcrypto/webcrypto_util.h:20: const uint8* Start(const > > std::vector<uint8>& data); > why is this not just std::vector<T>::data()? > > this is too generic of a function name and marginal utility to make it > be a direct free function in the content:: namespace. I'd be really > surprised if this isn't an ODR violation either today or in the near > future > > https://codereview.chromium.org/25906002/ To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
+1 on making the name more specific (the general name came from being a helper function in test .cc file) Moving into a more specific namespace could be a bit tedious, since "using namespace webcrypto" is disallowed. @jamesr, would you propose moving WebCryptoImpl into the webcrypto namespace too?
Most of the webcrypto code so far is in classes, which namespace well. It's just generic free functions or other symbols directly in the content:: namespace i'm worried about
Paul, what is your plan to address James' review comments? I propose moving the functions in webcrypto_util.h into a new namespace, webcrypto.
On 2013/11/20 23:26:49, eroman wrote: > Paul, what is your plan to address James' review comments? > > I propose moving the functions in webcrypto_util.h into a new namespace, > webcrypto. Sorry, I was waiting for you to weigh in. Yes that seems to be the only option. What about also moving the rest of the code into webcrypto:: too? That would remove the requirement to prepend the webcrypto namespace to the util function calls (since using is not allowed). But would that cascade too many problems to callers of the webcrypto stuff?
Yes WebCryptoImpl should similarly be moved into the webcrypto namespace. I strongly suggest splitting that refactor off to a separate changelist (i.e. moving into a namespace and introducing the util file) to simplify the review.
On 2013/11/21 01:30:39, eroman wrote: > Yes WebCryptoImpl should similarly be moved into the webcrypto namespace. > > I strongly suggest splitting that refactor off to a separate changelist (i.e. > moving into a namespace and introducing the util file) to simplify the review. The util file has been part of this changelist almost from the beginning. Perhaps that was a mistake. But cutting it out now seems like busywork for a 6 week old CL. Alternatively, do you think we could get this CL committed by namespacing the util file functions and just attaching webcrypto:: at all the util functions callsites? Then in a subsequent CL I can properly namespace WebCryptoImpl and remove the util function callsite namespace prefix cruft. This would directly address @jamesr's namespace issue and would be a lot less work.
Free functions in webcrypto_util are now in a new webcrypto:: namespace. In a subsequent CL I will also put WebCryptoImpl into webcrypto:: https://codereview.chromium.org/25906002/diff/1288001/content/renderer/webcry... File content/renderer/webcrypto/webcrypto_util.h (right): https://codereview.chromium.org/25906002/diff/1288001/content/renderer/webcry... content/renderer/webcrypto/webcrypto_util.h:20: const uint8* Start(const std::vector<uint8>& data); On 2013/11/12 03:54:09, padolph wrote: > On 2013/11/12 02:33:09, jamesr wrote: > > why is this not just std::vector<T>::data()? > > The vector::data() method is new to C++11 and causes build failures on platforms > that do not support C++11 (e.g. Android). > > > this is too generic of a function name and marginal utility to make it be a > > direct free function in the content:: namespace. I'd be really surprised if > > this isn't an ODR violation either today or in the near future > > Good point about the name. > > eroman: What about renaming this to something that includes "vector", and > templating the type? Renamed.
lgtm
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/padolph@netflix.com/25906002/1588001
On 2013/11/14 02:45:30, jamesr wrote: > Most of the webcrypto code so far is in classes, which namespace well. It's just > generic free functions or other symbols directly in the content:: namespace i'm > worried about @jamesr: With the namespacing, can I get an lgtm from you? eroman's is not enough to pass the pre-submit. Thanks!
Retried try job too often on chromium_presubmit for step(s) presubmit http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_p...
Yes, lgtm. Did someone address Ryan Sleevi's concerns from earlier? He still shows up red on the reviewer list.
lgtm https://codereview.chromium.org/25906002/diff/1648001/content/renderer/webcry... File content/renderer/webcrypto/webcrypto_impl.cc (right): https://codereview.chromium.org/25906002/diff/1648001/content/renderer/webcry... content/renderer/webcrypto/webcrypto_impl.cc:137: return false; I don't know if this was previously raised, but let's do short-circuits first if (alg1.id() != alg2.id()) return false; if ((alg1.id() == ... || alg1.id() == ... || alg1.id() == ...) && !WebCryptoAlgorithmsConsistent(..., ...)) { return false; } return true;
https://codereview.chromium.org/25906002/diff/1648001/content/renderer/webcry... File content/renderer/webcrypto/webcrypto_impl.cc (right): https://codereview.chromium.org/25906002/diff/1648001/content/renderer/webcry... content/renderer/webcrypto/webcrypto_impl.cc:137: return false; On 2013/12/03 19:21:21, Ryan Sleevi wrote: > I don't know if this was previously raised, but let's do short-circuits first > > if (alg1.id() != alg2.id()) > return false; > if ((alg1.id() == ... || > alg1.id() == ... || > alg1.id() == ...) && > !WebCryptoAlgorithmsConsistent(..., ...)) { > return false; > } > return true; It was originally written that way. but Eric wanted the current version to help with future-proofing.
On 2013/12/03 19:42:53, padolph wrote: > https://codereview.chromium.org/25906002/diff/1648001/content/renderer/webcry... > File content/renderer/webcrypto/webcrypto_impl.cc (right): > > https://codereview.chromium.org/25906002/diff/1648001/content/renderer/webcry... > content/renderer/webcrypto/webcrypto_impl.cc:137: return false; > On 2013/12/03 19:21:21, Ryan Sleevi wrote: > > I don't know if this was previously raised, but let's do short-circuits first > > > > if (alg1.id() != alg2.id()) > > return false; > > if ((alg1.id() == ... || > > alg1.id() == ... || > > alg1.id() == ...) && > > !WebCryptoAlgorithmsConsistent(..., ...)) { > > return false; > > } > > return true; > > It was originally written that way. but Eric wanted the current version to help > with future-proofing. I don't think that's what Eric wanted. In general, for these sorts of algorithm checks, we should be doing a whitelist. That is, every supported algorithm should be explicitly enumerated when performing these checks, with the default behaviour (eg: not enumerated) to return false. You've currently implemented it as a blacklist, but one that is not as concise or compact as it should be. I think we should be changing this to an explicit whitelist (which requires enumerating every supported-in-Blink algorithm, as an explicit acknowledgement of equivalency). I'm not sure whether that makes more sense to do in a much smaller, compact follow-up or as a continuation of this patch. If you're going to keep it as a blacklist, though, use the concise form.
https://codereview.chromium.org/25906002/diff/1648001/content/renderer/webcry... File content/renderer/webcrypto/webcrypto_impl.cc (right): https://codereview.chromium.org/25906002/diff/1648001/content/renderer/webcry... content/renderer/webcrypto/webcrypto_impl.cc:137: return false; On 2013/12/03 19:42:53, padolph wrote: > On 2013/12/03 19:21:21, Ryan Sleevi wrote: > > I don't know if this was previously raised, but let's do short-circuits first > > > > if (alg1.id() != alg2.id()) > > return false; > > if ((alg1.id() == ... || > > alg1.id() == ... || > > alg1.id() == ...) && > > !WebCryptoAlgorithmsConsistent(..., ...)) { > > return false; > > } > > return true; > > It was originally written that way. but Eric wanted the current version to help > with future-proofing. Done.
whitelist LGTM! Thanks!
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/padolph@netflix.com/25906002/1668001
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/padolph@netflix.com/25906002/1728001
Message was sent while issue was closed.
Change committed as 238574 |
