Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
184043021:
[webcrypto] JWK: Updated import(ext, key_ops) and added export of symmetric keys (Closed)
|
Created:
6 years, 9 months ago by padolph Modified:
6 years, 9 months ago CC:
chromium-reviews, joi+watch-content_chromium.org, darin-cc_chromium.org, jam Base URL:
https://chromium.googlesource.com/chromium/src.git@wcAesKw_nss1 Visibility:
Public. |
Description[webcrypto] JWK: Updated import(ext, key_ops) and added export of symmetric keys
See https://www.w3.org/Bugs/Public/show_bug.cgi?id=23796
BUG=245025
TEST=content_unittests --gtest_filter="SharedCryptoTest*"
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=256399
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=257078
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=257217
Patch Set 1 #
Total comments: 43
Patch Set 2 : fixes for eroman #
Total comments: 23
Patch Set 3 : rebase and fixes for eroman #Patch Set 4 : rebase #
Total comments: 16
Patch Set 5 : rebase #Patch Set 6 : fixes for eroman and rsleevi #
Total comments: 2
Patch Set 7 : fix for eroman: removed unneeded header includes in jwk.cc #Patch Set 8 : rebase #Patch Set 9 : runtime-conditional AES-GCM tests/DCHECK's plus cl lint fixes #
Total comments: 8
Patch Set 10 : fixes for eroman, and reverted revert to get original files back after rebase removed them #
Total comments: 2
Patch Set 11 : added missing AES-GCM runtime check #
Messages
Total messages: 38 (0 generated)
This CL replaces my previous 3 JWK-related CLs: Issue 111893010: [webcrypto] Update JWK import of AES and HMAC keys to latest spec. Issue 103713005: [webcrypto] Add JWK export of AES and HMAC keys. Issue 171693002: [webcrypto] Add JWK import/export of an AES-KW key.
https://codereview.chromium.org/184043021/diff/1/content/renderer/webcrypto/j... File content/renderer/webcrypto/jwk.cc (left): https://codereview.chromium.org/184043021/diff/1/content/renderer/webcrypto/j... content/renderer/webcrypto/jwk.cc:326: // | "A192CBC" | AES CBC using 192 bit keys [not yet part of JOSE] | thanks for updating these comments! https://codereview.chromium.org/184043021/diff/1/content/renderer/webcrypto/j... File content/renderer/webcrypto/jwk.cc (right): https://codereview.chromium.org/184043021/diff/1/content/renderer/webcrypto/j... content/renderer/webcrypto/jwk.cc:51: // TODO(padolph): include other AES symmetric algorithms once they are How about: return algorithm.paramsType() == blink::WebCryptoKeyAlgorithmParamsTypeAes; https://codereview.chromium.org/184043021/diff/1/content/renderer/webcrypto/j... content/renderer/webcrypto/jwk.cc:138: &BindAlgorithmId<CreateAlgorithm, blink::WebCryptoAlgorithmIdAesKw>); Include the key length as the final parameter, like you did for A192GCM. (This is used for verifying that the correct length key was given, rather than inferring algorithm from the key length) https://codereview.chromium.org/184043021/diff/1/content/renderer/webcrypto/j... content/renderer/webcrypto/jwk.cc:246: // |*property_exists| if it was found. Can you mention that the pointer *result is owned by the dictionary? https://codereview.chromium.org/184043021/diff/1/content/renderer/webcrypto/j... content/renderer/webcrypto/jwk.cc:300: // Returns true if the set bits in b are a strict subset of the set bits in a. I don't quite understand the comment "strict subset". In mathematical parlance for B to be a strict subset (or proper subset) of A means that A != B. However that is not the case for this helper. Suggest you remove or clarify that comment. https://codereview.chromium.org/184043021/diff/1/content/renderer/webcrypto/j... content/renderer/webcrypto/jwk.cc:327: // OPTIONAL here means that this code does not require the entry to be present Perhaps as a separate changelist, I suggest moving this big comment block to the top of the file. Since the JWK format documentation applies not just to this function, but the file in general. https://codereview.chromium.org/184043021/diff/1/content/renderer/webcrypto/j... content/renderer/webcrypto/jwk.cc:564: Status status = I prefer not shadowing the variable (can be tricky when reading). Can you instead overwrite status? https://codereview.chromium.org/184043021/diff/1/content/renderer/webcrypto/j... content/renderer/webcrypto/jwk.cc:595: if ((jwk_use_mask == kJwkEncUsage) && Can you express this in a more future proof way? My concern is that if jwk_use_mask were to ever be anything other than just kKwkEncUsage or jkwSigUsage then we stop doing the consistency check. Would something like this work: if (has_jwk_key_ops && has_jkw_use && !IsBitwiseSubset(jwk_use_mask, jkw_key_ops_mask)) return .... https://codereview.chromium.org/184043021/diff/1/content/renderer/webcrypto/j... content/renderer/webcrypto/jwk.cc:668: // JWK has the following JSON structure This seems like a duplication of the bigger comment block. You can leave it here if you like, but I think having the format documentation at the top of the file is sufficient. https://codereview.chromium.org/184043021/diff/1/content/renderer/webcrypto/j... content/renderer/webcrypto/jwk.cc:684: DCHECK_EQ(blink::WebCryptoKeyTypeSecret, key.type()); In fact, would this not be a simpler way to implement the symmetric check? https://codereview.chromium.org/184043021/diff/1/content/renderer/webcrypto/j... content/renderer/webcrypto/jwk.cc:701: } else { Please split this function up into smaller functions. * Delegate symmetric keys vs public/private keys to separate functions * Serialization of key_ops in separate function * Serialization of alg To structure it I suggest stack-allocating a DictionaryValue in the base function, and having the helper functions take a pointer to DictionaryValue and write into that. Pseudo-code: DictionaryValue jwk_dict; WriteKeyOps(key.usages(), &jwk_dict); WiteAlg(key.algorithm(), &jwk_dict); WriteExt(key.extractable(), &jwk_dict); if (key.type() == ..TypeSecret) { WriteSecretKey(key, &jwk_dict); } else { ... } JsonWriter::Write(..); https://codereview.chromium.org/184043021/diff/1/content/renderer/webcrypto/j... content/renderer/webcrypto/jwk.cc:708: if (key.usages() & blink::WebCryptoKeyUsageEncrypt) This is the opposite of what we use for import right? Rather than duplicate this in two places, you can create a static mapping like: struct JwkToWebCryptoUsageMapping { const char* jwk_usage; ... webcrypto_usage; }; const JwkToWebCryptoUsageMapping { {"encrypt", WebCryptoKeyUsageEncrypt}, {"decrypt", WebCryptoKeyUsageDecrypt}, ... }; The import vs export functions would be an iteration of this same table. https://codereview.chromium.org/184043021/diff/1/content/renderer/webcrypto/j... content/renderer/webcrypto/jwk.cc:745: switch (key.algorithm().id()) { Is there a good way to re-use the table mapping from import? (which similarly knows about the jwk name, and key length). A future idea would be to move to virtual methods on KeyAlgorithmParams. https://codereview.chromium.org/184043021/diff/1/content/renderer/webcrypto/j... content/renderer/webcrypto/jwk.cc:759: DCHECK(key.algorithm().hmacParams()); Have you considered making this outter switch be on the key.algorithm().paramsType() instead? The way the JWK name is built from the webcrypto information depends largely on the key type. https://codereview.chromium.org/184043021/diff/1/content/renderer/webcrypto/j... content/renderer/webcrypto/jwk.cc:772: return Status::ErrorJwkUnsupportedHmacHash(); This is in disagreement with the latest revision of WebCrypto spec, which also enumerates: HS1, HS224 https://codereview.chromium.org/184043021/diff/1/content/renderer/webcrypto/p... File content/renderer/webcrypto/platform_crypto_nss.cc (right): https://codereview.chromium.org/184043021/diff/1/content/renderer/webcrypto/p... content/renderer/webcrypto/platform_crypto_nss.cc:1200: key_algorithm, Not sure I understand this change --> wouldn't it have failed compile without this? Is this change an artifact of basing on an uncommitted git branch? (The change itself looks good, just trying to understand why it is here) https://codereview.chromium.org/184043021/diff/1/content/renderer/webcrypto/s... File content/renderer/webcrypto/shared_crypto_unittest.cc (right): https://codereview.chromium.org/184043021/diff/1/content/renderer/webcrypto/s... content/renderer/webcrypto/shared_crypto_unittest.cc:541: base::Value* value = base::JSONReader::Read(json_string); Read() can return NULL on failure. At a minimum EXPECT_TRUE() on it (the testing for NULL should be explicit somewhere) https://codereview.chromium.org/184043021/diff/1/content/renderer/webcrypto/s... content/renderer/webcrypto/shared_crypto_unittest.cc:547: bool CaseInsensitiveStringCompare(const std::string& a, const std::string& b) { I am surprised this flavor doesn't already exist in base. I suggested instead using: LowerCaseEqualsASCII() However you will have to call c_str() on one of the inputs. https://codereview.chromium.org/184043021/diff/1/content/renderer/webcrypto/s... content/renderer/webcrypto/shared_crypto_unittest.cc:1003: TEST_F(SharedCryptoTest, ImportJwkKeyUsage) { I haven't read through this in detail yet (will do it when reviewing next update). However my initial reaction is, whether much of this can be moved to a .json file like the other test data, and do the bulk of the tests as roundtrip import/export tests. Basically I am thinking: [ jwk_input: { ext: 0, alg: ... }, jwk_output: { ext: false, ... } ] Expressing the JWK input in the .json file is easy, since it takes JSON. After reading the Value it would have to be serialized to a string and then that is used for the input and output comparison. https://codereview.chromium.org/184043021/diff/1/content/renderer/webcrypto/s... content/renderer/webcrypto/shared_crypto_unittest.cc:1623: // HMAC SHA-1 and SHA-224 are not allowed by the JWK(JWA) spec. But they are allowed by webcrypto (see spec updates) https://codereview.chromium.org/184043021/diff/1/content/renderer/webcrypto/w... File content/renderer/webcrypto/webcrypto_util.h (right): https://codereview.chromium.org/184043021/diff/1/content/renderer/webcrypto/w... content/renderer/webcrypto/webcrypto_util.h:129: static Status ErrorJwkUnsupportedHmacHash(); I don't believe we need this https://codereview.chromium.org/184043021/diff/1/content/renderer/webcrypto/w... content/renderer/webcrypto/webcrypto_util.h:242: // Produces an unpadded 'base64url' encoding of the input data, the opposite of Can these be moved into jwk.cc? (Note that I have TODO more moving Base64DecodeUrlSafe() there too)
https://codereview.chromium.org/184043021/diff/1/content/renderer/webcrypto/p... File content/renderer/webcrypto/platform_crypto_nss.cc (right): https://codereview.chromium.org/184043021/diff/1/content/renderer/webcrypto/p... content/renderer/webcrypto/platform_crypto_nss.cc:1200: key_algorithm, On 2014/03/04 02:55:14, eroman wrote: > Not sure I understand this change --> wouldn't it have failed compile without > this? > > Is this change an artifact of basing on an uncommitted git branch? > > (The change itself looks good, just trying to understand why it is here) Yes, it's an artifact of my workflow. This is a fix for a compile bug from another git branch, upon which this branch is based. Sorry for the confusion. https://codereview.chromium.org/184043021/diff/1/content/renderer/webcrypto/w... File content/renderer/webcrypto/webcrypto_util.h (right): https://codereview.chromium.org/184043021/diff/1/content/renderer/webcrypto/w... content/renderer/webcrypto/webcrypto_util.h:242: // Produces an unpadded 'base64url' encoding of the input data, the opposite of On 2014/03/04 02:55:14, eroman wrote: > Can these be moved into jwk.cc? (Note that I have TODO more moving > Base64DecodeUrlSafe() there too) Base64DecodeUrlSafe() is used by the tests. I think this means it can't live in jwk.cc for linking reasons.
https://codereview.chromium.org/184043021/diff/1/content/renderer/webcrypto/j... File content/renderer/webcrypto/jwk.cc (right): https://codereview.chromium.org/184043021/diff/1/content/renderer/webcrypto/j... content/renderer/webcrypto/jwk.cc:51: // TODO(padolph): include other AES symmetric algorithms once they are On 2014/03/04 02:55:14, eroman wrote: > How about: > return algorithm.paramsType() == blink::WebCryptoKeyAlgorithmParamsTypeAes; Done. https://codereview.chromium.org/184043021/diff/1/content/renderer/webcrypto/j... content/renderer/webcrypto/jwk.cc:138: &BindAlgorithmId<CreateAlgorithm, blink::WebCryptoAlgorithmIdAesKw>); On 2014/03/04 02:55:14, eroman wrote: > Include the key length as the final parameter, like you did for A192GCM. > > (This is used for verifying that the correct length key was given, rather than > inferring algorithm from the key length) Done. https://codereview.chromium.org/184043021/diff/1/content/renderer/webcrypto/j... content/renderer/webcrypto/jwk.cc:246: // |*property_exists| if it was found. On 2014/03/04 02:55:14, eroman wrote: > Can you mention that the pointer *result is owned by the dictionary? Done. https://codereview.chromium.org/184043021/diff/1/content/renderer/webcrypto/j... content/renderer/webcrypto/jwk.cc:300: // Returns true if the set bits in b are a strict subset of the set bits in a. On 2014/03/04 02:55:14, eroman wrote: > I don't quite understand the comment "strict subset". > > In mathematical parlance for B to be a strict subset (or proper subset) of A > means that A != B. However that is not the case for this helper. > > Suggest you remove or clarify that comment. Done. https://codereview.chromium.org/184043021/diff/1/content/renderer/webcrypto/j... content/renderer/webcrypto/jwk.cc:327: // OPTIONAL here means that this code does not require the entry to be present On 2014/03/04 02:55:14, eroman wrote: > Perhaps as a separate changelist, I suggest moving this big comment block to the > top of the file. Since the JWK format documentation applies not just to this > function, but the file in general. Agreed. Added a TODO. https://codereview.chromium.org/184043021/diff/1/content/renderer/webcrypto/j... content/renderer/webcrypto/jwk.cc:564: Status status = On 2014/03/04 02:55:14, eroman wrote: > I prefer not shadowing the variable (can be tricky when reading). Can you > instead overwrite status? Done. https://codereview.chromium.org/184043021/diff/1/content/renderer/webcrypto/j... content/renderer/webcrypto/jwk.cc:595: if ((jwk_use_mask == kJwkEncUsage) && On 2014/03/04 02:55:14, eroman wrote: > Can you express this in a more future proof way? My concern is that if > jwk_use_mask were to ever be anything other than just kKwkEncUsage or > jkwSigUsage then we stop doing the consistency check. > > Would something like this work: > > if (has_jwk_key_ops && has_jkw_use && !IsBitwiseSubset(jwk_use_mask, > jkw_key_ops_mask)) > return .... Yes - that works perfectly. Thanks. https://codereview.chromium.org/184043021/diff/1/content/renderer/webcrypto/j... content/renderer/webcrypto/jwk.cc:668: // JWK has the following JSON structure On 2014/03/04 02:55:14, eroman wrote: > This seems like a duplication of the bigger comment block. You can leave it here > if you like, but I think having the format documentation at the top of the file > is sufficient. Removed. https://codereview.chromium.org/184043021/diff/1/content/renderer/webcrypto/j... content/renderer/webcrypto/jwk.cc:684: DCHECK_EQ(blink::WebCryptoKeyTypeSecret, key.type()); On 2014/03/04 02:55:14, eroman wrote: > In fact, would this not be a simpler way to implement the symmetric check? Done. https://codereview.chromium.org/184043021/diff/1/content/renderer/webcrypto/j... content/renderer/webcrypto/jwk.cc:701: } else { On 2014/03/04 02:55:14, eroman wrote: > Please split this function up into smaller functions. > > * Delegate symmetric keys vs public/private keys to separate functions > * Serialization of key_ops in separate function > * Serialization of alg > > To structure it I suggest stack-allocating a DictionaryValue in the base > function, and having the helper functions take a pointer to DictionaryValue and > write into that. Pseudo-code: > > > DictionaryValue jwk_dict; > > WriteKeyOps(key.usages(), &jwk_dict); > WiteAlg(key.algorithm(), &jwk_dict); > WriteExt(key.extractable(), &jwk_dict); > > if (key.type() == ..TypeSecret) { > WriteSecretKey(key, &jwk_dict); > } else { > ... > } > > JsonWriter::Write(..); Done. https://codereview.chromium.org/184043021/diff/1/content/renderer/webcrypto/j... content/renderer/webcrypto/jwk.cc:708: if (key.usages() & blink::WebCryptoKeyUsageEncrypt) On 2014/03/04 02:55:14, eroman wrote: > This is the opposite of what we use for import right? > > Rather than duplicate this in two places, you can create a static mapping like: > > > struct JwkToWebCryptoUsageMapping { > const char* jwk_usage; > ... webcrypto_usage; > }; > > const JwkToWebCryptoUsageMapping { > {"encrypt", WebCryptoKeyUsageEncrypt}, > {"decrypt", WebCryptoKeyUsageDecrypt}, > ... > }; > > The import vs export functions would be an iteration of this same table. Done, but I'm not sure I like the way it turned out. I like the single source of truth for the mapping, but the impl is ugly using a linear search through the map array. Also, I had to put the code in util because some of it is used by the tests. Please take a look. https://codereview.chromium.org/184043021/diff/1/content/renderer/webcrypto/j... content/renderer/webcrypto/jwk.cc:745: switch (key.algorithm().id()) { On 2014/03/04 02:55:14, eroman wrote: > Is there a good way to re-use the table mapping from import? (which similarly > knows about the jwk name, and key length). > > A future idea would be to move to virtual methods on KeyAlgorithmParams. Not as it stands now. First, reverse table lookup must be a linear search, often with functor, which can get ugly. Second, the value to be searched would have to be the AlgorithmCreationFunc pointer which is not possible to find here. We would likely have to add another field in the table to represent the algorithm and search on that plus key length. Or define a new lookup table with a flattened key that includes alg and key length. But I'm not sure that's any better than the nested switch we have here. Having the KeyAlgorithm able to generate it's own JWK string is interesting. But I'm not sure about building JWK-specific code into Blink. https://codereview.chromium.org/184043021/diff/1/content/renderer/webcrypto/j... content/renderer/webcrypto/jwk.cc:759: DCHECK(key.algorithm().hmacParams()); On 2014/03/04 02:55:14, eroman wrote: > Have you considered making this outter switch be on the > key.algorithm().paramsType() instead? The way the JWK name is built from the > webcrypto information depends largely on the key type. Done. Maybe a little more readable now, and does not compute aes_prefix except when required. Thanks. https://codereview.chromium.org/184043021/diff/1/content/renderer/webcrypto/j... content/renderer/webcrypto/jwk.cc:772: return Status::ErrorJwkUnsupportedHmacHash(); On 2014/03/04 02:55:14, eroman wrote: > This is in disagreement with the latest revision of WebCrypto spec, which also > enumerates: > > HS1, HS224 Done. https://codereview.chromium.org/184043021/diff/1/content/renderer/webcrypto/s... File content/renderer/webcrypto/shared_crypto_unittest.cc (right): https://codereview.chromium.org/184043021/diff/1/content/renderer/webcrypto/s... content/renderer/webcrypto/shared_crypto_unittest.cc:541: base::Value* value = base::JSONReader::Read(json_string); On 2014/03/04 02:55:14, eroman wrote: > Read() can return NULL on failure. > > At a minimum EXPECT_TRUE() on it (the testing for NULL should be explicit > somewhere) Done. https://codereview.chromium.org/184043021/diff/1/content/renderer/webcrypto/s... content/renderer/webcrypto/shared_crypto_unittest.cc:547: bool CaseInsensitiveStringCompare(const std::string& a, const std::string& b) { On 2014/03/04 02:55:14, eroman wrote: > I am surprised this flavor doesn't already exist in base. > > I suggested instead using: > LowerCaseEqualsASCII() > > However you will have to call c_str() on one of the inputs. Done. https://codereview.chromium.org/184043021/diff/1/content/renderer/webcrypto/s... content/renderer/webcrypto/shared_crypto_unittest.cc:1003: TEST_F(SharedCryptoTest, ImportJwkKeyUsage) { On 2014/03/04 02:55:14, eroman wrote: > I haven't read through this in detail yet (will do it when reviewing next > update). > > However my initial reaction is, whether much of this can be moved to a .json > file like the other test data, and do the bulk of the tests as roundtrip > import/export tests. > > Basically I am thinking: > > [ > jwk_input: { ext: 0, alg: ... }, > jwk_output: { ext: false, ... } > ] > > Expressing the JWK input in the .json file is easy, since it takes JSON. After > reading the Value it would have to be serialized to a string and then that is > used for the input and output comparison. I'm not sure I agree, for the following reasons: - Round tripping could mask certain errors. This test is about how import JWK works with varied input, specifically usage. Running through export hits a lot of other code paths that may influence the result. - Comparing stringified JSON is problematic. Minor formatting differences or ordering often gets in the way. This test wants to examine the resultant WebCrypto Key after an import, not its JWK exported representation. https://codereview.chromium.org/184043021/diff/1/content/renderer/webcrypto/s... content/renderer/webcrypto/shared_crypto_unittest.cc:1623: // HMAC SHA-1 and SHA-224 are not allowed by the JWK(JWA) spec. On 2014/03/04 02:55:14, eroman wrote: > But they are allowed by webcrypto (see spec updates) My mistake. Fixed. https://codereview.chromium.org/184043021/diff/1/content/renderer/webcrypto/w... File content/renderer/webcrypto/webcrypto_util.h (right): https://codereview.chromium.org/184043021/diff/1/content/renderer/webcrypto/w... content/renderer/webcrypto/webcrypto_util.h:129: static Status ErrorJwkUnsupportedHmacHash(); On 2014/03/04 02:55:14, eroman wrote: > I don't believe we need this Removed.
LGTM after these comments https://codereview.chromium.org/184043021/diff/20001/content/renderer/webcryp... File content/renderer/webcrypto/jwk.cc (right): https://codereview.chromium.org/184043021/diff/20001/content/renderer/webcryp... content/renderer/webcrypto/jwk.cc:283: bool IsBitwiseSubset(unsigned int a, unsigned int b) { return (a & b) == b; } I suggest making the parameters be blink::WebCryptoKeyUsageMask and giving it a more specific name like: ContainsKeyUsages() IncludesAllUsages() As much as possible I would like to hide the fact the the key usages is a bitfield. And ideally move this into the blink code so. https://codereview.chromium.org/184043021/diff/20001/content/renderer/webcryp... File content/renderer/webcrypto/shared_crypto_unittest.cc (right): https://codereview.chromium.org/184043021/diff/20001/content/renderer/webcryp... content/renderer/webcrypto/shared_crypto_unittest.cc:1509: // TODO(padolph): Move this data to external file? Not much unique data here. You can remove this comment. I agree fine to leave in the .cc file https://codereview.chromium.org/184043021/diff/20001/content/renderer/webcryp... content/renderer/webcrypto/shared_crypto_unittest.cc:1510: const char* const key_hex_128("3f1e7cd4f6f8543f6b1e16002e688623"); nit: in chromium code it is more common to use " = " assignment for POD. That is my preference, but up to you. https://codereview.chromium.org/184043021/diff/20001/content/renderer/webcryp... File content/renderer/webcrypto/webcrypto_util.cc (right): https://codereview.chromium.org/184043021/diff/20001/content/renderer/webcryp... content/renderer/webcrypto/webcrypto_util.cc:242: void Base64EncodeUrlSafe(const base::StringPiece& input, std::string* output) { [optional] Since no error can be returned, you might also consider directly returning an std::string. The compiler should be able to do "return value optimization" here. https://codereview.chromium.org/184043021/diff/20001/content/renderer/webcryp... content/renderer/webcrypto/webcrypto_util.cc:267: base::ListValue* jwk_key_ops_value, can this parameter be "const"? https://codereview.chromium.org/184043021/diff/20001/content/renderer/webcryp... content/renderer/webcrypto/webcrypto_util.cc:270: for (size_t i = 0; i < jwk_key_ops_value->GetSize(); ++i) { nit: ++i to match other code in webcrypto https://codereview.chromium.org/184043021/diff/20001/content/renderer/webcryp... content/renderer/webcrypto/webcrypto_util.cc:272: if (!jwk_key_ops_value->GetString(i, &key_op)) Please add a curly brace around this (because of line break it is no longer a "single-line if statement") https://codereview.chromium.org/184043021/diff/20001/content/renderer/webcryp... content/renderer/webcrypto/webcrypto_util.cc:276: const size_t map_size = ARRAYSIZE_UNSAFE(kJwkWebCryptoUsageMap); Is the UNSAFE variant needed here or does it work with ARRAYSIZE()? https://codereview.chromium.org/184043021/diff/20001/content/renderer/webcryp... content/renderer/webcrypto/webcrypto_util.cc:277: for (j = 0; j < map_size; ++j) { I have been burned by mixups on "i" vs "j" in the past, and encourage you to pull this out to a helper function, or choose different names for "i" and "j". For instance: bool JwkKeyOpToWebCryptoUsage(const std::string key_op, ...* usage) { for (size_t i = 0; i < ARRAY_SIZE(kJwkWebCryptoUsageMap); ++i) { if (kJwkWebCryptoUsageMap[i].jwk_key_op == key_op) { *usage = ....; return true; } } return false; } https://codereview.chromium.org/184043021/diff/20001/content/renderer/webcryp... content/renderer/webcrypto/webcrypto_util.cc:290: void GetJwkKeyOpsFromWebCryptoUsages(blink::WebCryptoKeyUsageMask usage_mask, I would call this "Fill" rather than "Get". Or alternately, call it "Create..." and have it return a new base::ListValue. https://codereview.chromium.org/184043021/diff/20001/content/renderer/webcryp... File content/renderer/webcrypto/webcrypto_util.h (right): https://codereview.chromium.org/184043021/diff/20001/content/renderer/webcryp... content/renderer/webcrypto/webcrypto_util.h:243: CONTENT_EXPORT Status GetWebCryptoUsagesFromJwkKeyOps( We should introduce a jwk.h header and move these utilities over to jwk.{cc,h}. ImportJwkKey/ExportJwkKey should be in that header too. I don't require that for this changelist, but something to keep in midnd,
https://codereview.chromium.org/184043021/diff/20001/content/renderer/webcryp... File content/renderer/webcrypto/jwk.cc (right): https://codereview.chromium.org/184043021/diff/20001/content/renderer/webcryp... content/renderer/webcrypto/jwk.cc:283: bool IsBitwiseSubset(unsigned int a, unsigned int b) { return (a & b) == b; } On 2014/03/07 19:33:06, eroman wrote: > I suggest making the parameters be blink::WebCryptoKeyUsageMask and giving it a > more specific name like: > > ContainsKeyUsages() > IncludesAllUsages() > > As much as possible I would like to hide the fact the the key usages is a > bitfield. And ideally move this into the blink code so. Done. https://codereview.chromium.org/184043021/diff/20001/content/renderer/webcryp... File content/renderer/webcrypto/shared_crypto_unittest.cc (right): https://codereview.chromium.org/184043021/diff/20001/content/renderer/webcryp... content/renderer/webcrypto/shared_crypto_unittest.cc:1509: // TODO(padolph): Move this data to external file? Not much unique data here. On 2014/03/07 19:33:06, eroman wrote: > You can remove this comment. I agree fine to leave in the .cc file Done. https://codereview.chromium.org/184043021/diff/20001/content/renderer/webcryp... content/renderer/webcrypto/shared_crypto_unittest.cc:1510: const char* const key_hex_128("3f1e7cd4f6f8543f6b1e16002e688623"); On 2014/03/07 19:33:06, eroman wrote: > nit: in chromium code it is more common to use " = " assignment for POD. That is > my preference, but up to you. Done. https://codereview.chromium.org/184043021/diff/20001/content/renderer/webcryp... File content/renderer/webcrypto/webcrypto_util.cc (right): https://codereview.chromium.org/184043021/diff/20001/content/renderer/webcryp... content/renderer/webcrypto/webcrypto_util.cc:242: void Base64EncodeUrlSafe(const base::StringPiece& input, std::string* output) { On 2014/03/07 19:33:06, eroman wrote: > [optional] Since no error can be returned, you might also consider directly > returning an std::string. > > The compiler should be able to do "return value optimization" here. Done. https://codereview.chromium.org/184043021/diff/20001/content/renderer/webcryp... content/renderer/webcrypto/webcrypto_util.cc:267: base::ListValue* jwk_key_ops_value, On 2014/03/07 19:33:06, eroman wrote: > can this parameter be "const"? Yes. Done. https://codereview.chromium.org/184043021/diff/20001/content/renderer/webcryp... content/renderer/webcrypto/webcrypto_util.cc:270: for (size_t i = 0; i < jwk_key_ops_value->GetSize(); ++i) { On 2014/03/07 19:33:06, eroman wrote: > nit: ++i to match other code in webcrypto I don't understand the comment. The code already has '++i'. https://codereview.chromium.org/184043021/diff/20001/content/renderer/webcryp... content/renderer/webcrypto/webcrypto_util.cc:272: if (!jwk_key_ops_value->GetString(i, &key_op)) On 2014/03/07 19:33:06, eroman wrote: > Please add a curly brace around this (because of line break it is no longer a > "single-line if statement") Done. https://codereview.chromium.org/184043021/diff/20001/content/renderer/webcryp... content/renderer/webcrypto/webcrypto_util.cc:276: const size_t map_size = ARRAYSIZE_UNSAFE(kJwkWebCryptoUsageMap); On 2014/03/07 19:33:06, eroman wrote: > Is the UNSAFE variant needed here or does it work with ARRAYSIZE()? Done. https://codereview.chromium.org/184043021/diff/20001/content/renderer/webcryp... content/renderer/webcrypto/webcrypto_util.cc:277: for (j = 0; j < map_size; ++j) { On 2014/03/07 19:33:06, eroman wrote: > I have been burned by mixups on "i" vs "j" in the past, and encourage you to > pull this out to a helper function, or choose different names for "i" and "j". > > For instance: > > bool JwkKeyOpToWebCryptoUsage(const std::string key_op, ...* usage) { > for (size_t i = 0; i < ARRAY_SIZE(kJwkWebCryptoUsageMap); ++i) { > if (kJwkWebCryptoUsageMap[i].jwk_key_op == key_op) { > *usage = ....; > return true; > } > } > return false; > } Done. https://codereview.chromium.org/184043021/diff/20001/content/renderer/webcryp... content/renderer/webcrypto/webcrypto_util.cc:290: void GetJwkKeyOpsFromWebCryptoUsages(blink::WebCryptoKeyUsageMask usage_mask, On 2014/03/07 19:33:06, eroman wrote: > I would call this "Fill" rather than "Get". > > Or alternately, call it "Create..." and have it return a new base::ListValue. Done. https://codereview.chromium.org/184043021/diff/20001/content/renderer/webcryp... File content/renderer/webcrypto/webcrypto_util.h (right): https://codereview.chromium.org/184043021/diff/20001/content/renderer/webcryp... content/renderer/webcrypto/webcrypto_util.h:243: CONTENT_EXPORT Status GetWebCryptoUsagesFromJwkKeyOps( On 2014/03/07 19:33:06, eroman wrote: > We should introduce a jwk.h header and move these utilities over to jwk.{cc,h}. > ImportJwkKey/ExportJwkKey should be in that header too. > > I don't require that for this changelist, but something to keep in midnd, Acknowledged.
L G T M after addressing: https://codereview.chromium.org/184043021/diff/60001/content/child/webcrypto/... https://codereview.chromium.org/184043021/diff/20001/content/renderer/webcryp... File content/renderer/webcrypto/webcrypto_util.cc (right): https://codereview.chromium.org/184043021/diff/20001/content/renderer/webcryp... content/renderer/webcrypto/webcrypto_util.cc:270: for (size_t i = 0; i < jwk_key_ops_value->GetSize(); ++i) { On 2014/03/09 22:06:36, padolph wrote: > On 2014/03/07 19:33:06, eroman wrote: > > nit: ++i to match other code in webcrypto > > I don't understand the comment. The code already has '++i'. My mistake, please disregard! https://codereview.chromium.org/184043021/diff/60001/content/child/webcrypto/... File content/child/webcrypto/jwk.cc (right): https://codereview.chromium.org/184043021/diff/60001/content/child/webcrypto/... content/child/webcrypto/jwk.cc:301: jwk_dict->SetString("k", webcrypto::Base64EncodeUrlSafe(key_str)); no need for webcrypto:: prefix https://codereview.chromium.org/184043021/diff/60001/content/child/webcrypto/... content/child/webcrypto/jwk.cc:307: base::ListValue* key_ops = CreateJwkKeyOpsFromWebCryptoUsages(key_usages); [optional] inline this directly into the Set() call below. https://codereview.chromium.org/184043021/diff/60001/content/child/webcrypto/... content/child/webcrypto/jwk.cc:341: base::StringPrintf("%s%s", aes_prefix, "CBC")); I suggest extracting the "base::StringPrintf("%s%s")" to the bottom and having this loop just get the algorithm suffix https://codereview.chromium.org/184043021/diff/60001/content/child/webcrypto/... content/child/webcrypto/jwk.cc:780: *buffer = webcrypto::CreateArrayBuffer( no need for "webcrypto::" prefix https://codereview.chromium.org/184043021/diff/60001/content/child/webcrypto/... File content/child/webcrypto/platform_crypto_nss.cc (left): https://codereview.chromium.org/184043021/diff/60001/content/child/webcrypto/... content/child/webcrypto/platform_crypto_nss.cc:1206: Status WrapSymKeyAesKw(SymKey* wrapping_key, IMPORTANT: Why is this showing as removed? https://codereview.chromium.org/184043021/diff/60001/content/child/webcrypto/... File content/child/webcrypto/shared_crypto_unittest.cc (right): https://codereview.chromium.org/184043021/diff/60001/content/child/webcrypto/... content/child/webcrypto/shared_crypto_unittest.cc:1041: for (size_t i = 0; i < ARRAYSIZE_UNSAFE(test_case); ++i) { consider calling this test_index for consistency.
https://codereview.chromium.org/184043021/diff/60001/content/child/webcrypto/... File content/child/webcrypto/jwk.cc (right): https://codereview.chromium.org/184043021/diff/60001/content/child/webcrypto/... content/child/webcrypto/jwk.cc:491: // | "HS224" | HMAC using SHA-224 hash algorithm | Note: This is being removed (all of SHA-224) https://codereview.chromium.org/184043021/diff/60001/content/child/webcrypto/... content/child/webcrypto/jwk.cc:515: // | | https://www.w3.org/Bugs/Public/show_bug.cgi?id=23796 | Remove this comment as well? [the not yet part]
https://codereview.chromium.org/184043021/diff/60001/content/child/webcrypto/... File content/child/webcrypto/jwk.cc (right): https://codereview.chromium.org/184043021/diff/60001/content/child/webcrypto/... content/child/webcrypto/jwk.cc:301: jwk_dict->SetString("k", webcrypto::Base64EncodeUrlSafe(key_str)); On 2014/03/10 21:55:34, eroman wrote: > no need for webcrypto:: prefix Done. https://codereview.chromium.org/184043021/diff/60001/content/child/webcrypto/... content/child/webcrypto/jwk.cc:307: base::ListValue* key_ops = CreateJwkKeyOpsFromWebCryptoUsages(key_usages); On 2014/03/10 21:55:34, eroman wrote: > [optional] inline this directly into the Set() call below. Done. https://codereview.chromium.org/184043021/diff/60001/content/child/webcrypto/... content/child/webcrypto/jwk.cc:341: base::StringPrintf("%s%s", aes_prefix, "CBC")); On 2014/03/10 21:55:34, eroman wrote: > I suggest extracting the "base::StringPrintf("%s%s")" to the bottom and having > this loop just get the algorithm suffix Done. https://codereview.chromium.org/184043021/diff/60001/content/child/webcrypto/... content/child/webcrypto/jwk.cc:491: // | "HS224" | HMAC using SHA-224 hash algorithm | On 2014/03/11 00:43:15, Ryan Sleevi wrote: > Note: This is being removed (all of SHA-224) Done. https://codereview.chromium.org/184043021/diff/60001/content/child/webcrypto/... content/child/webcrypto/jwk.cc:515: // | | https://www.w3.org/Bugs/Public/show_bug.cgi?id=23796 | On 2014/03/11 00:43:15, Ryan Sleevi wrote: > Remove this comment as well? [the not yet part] Done. https://codereview.chromium.org/184043021/diff/60001/content/child/webcrypto/... content/child/webcrypto/jwk.cc:780: *buffer = webcrypto::CreateArrayBuffer( On 2014/03/10 21:55:34, eroman wrote: > no need for "webcrypto::" prefix Done. https://codereview.chromium.org/184043021/diff/60001/content/child/webcrypto/... File content/child/webcrypto/platform_crypto_nss.cc (left): https://codereview.chromium.org/184043021/diff/60001/content/child/webcrypto/... content/child/webcrypto/platform_crypto_nss.cc:1206: Status WrapSymKeyAesKw(SymKey* wrapping_key, On 2014/03/10 21:55:34, eroman wrote: > IMPORTANT: Why is this showing as removed? This was a rebase problem that unfortunately got uploaded. This deleted code is an old copy of the same functions that are already in the file, above here. Sorry for the confusion. https://codereview.chromium.org/184043021/diff/60001/content/child/webcrypto/... File content/child/webcrypto/shared_crypto_unittest.cc (right): https://codereview.chromium.org/184043021/diff/60001/content/child/webcrypto/... content/child/webcrypto/shared_crypto_unittest.cc:1041: for (size_t i = 0; i < ARRAYSIZE_UNSAFE(test_case); ++i) { On 2014/03/10 21:55:34, eroman wrote: > consider calling this test_index for consistency. Done.
lgtm https://codereview.chromium.org/184043021/diff/100001/content/child/webcrypto... File content/child/webcrypto/jwk.cc (right): https://codereview.chromium.org/184043021/diff/100001/content/child/webcrypto... content/child/webcrypto/jwk.cc:8: #include "base/base64.h" Is this header necessary?
https://codereview.chromium.org/184043021/diff/100001/content/child/webcrypto... File content/child/webcrypto/jwk.cc (right): https://codereview.chromium.org/184043021/diff/100001/content/child/webcrypto... content/child/webcrypto/jwk.cc:8: #include "base/base64.h" On 2014/03/11 03:22:47, eroman wrote: > Is this header necessary? Done.
The CQ bit was checked by padolph@netflix.com
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/padolph@netflix.com/184043021/140001
Message was sent while issue was closed.
Change committed as 256399
Message was sent while issue was closed.
On 2014/03/12 02:06:24, I haz the power (commit-bot) wrote: > Change committed as 256399 This caused ImportExportJwkSymmetricKey test failure on dbg builds, e.g. http://build.chromium.org/p/chromium.linux/builders/Linux%20Clang%20%28dbg%29... Reverted at r256415.
On 2014/03/12 05:18:28, Takayoshi Kochi wrote: > On 2014/03/12 02:06:24, I haz the power (commit-bot) wrote: > > Change committed as 256399 > > This caused ImportExportJwkSymmetricKey test failure on dbg builds, e.g. > http://build.chromium.org/p/chromium.linux/builders/Linux%2520Clang%2520%2528... > > Reverted at r256415. That failure is most likely caused by old NSS libs on the target machine that do not include support for the AES-GCM cipher. Patch set 9 has a workaround for this issue. Problematic AES-GCM test are not run if a run-time existence check for AES-GCM support fails.
Can you try re-uploading? Something probably failed since only showing 1 file in patchset 9
The modifications to unittest look good. Let me know when the entire patchset is available and I will stamp it. https://codereview.chromium.org/184043021/diff/160001/content/child/webcrypto... File content/child/webcrypto/shared_crypto_unittest.cc (right): https://codereview.chromium.org/184043021/diff/160001/content/child/webcrypto... content/child/webcrypto/shared_crypto_unittest.cc:69: EXPECT_EQ(Status::ErrorUnsupported().ToString(), status.ToString()); [optional] update this to use EXPECT_STATUS() https://codereview.chromium.org/184043021/diff/160001/content/child/webcrypto... content/child/webcrypto/shared_crypto_unittest.cc:115: DCHECK(SupportsAesGcm()); Instead of DCHECK() please use EXPECT_TRUE(). DCHECK() is only run for debug builds, whereas EXPECT_TRUE() will give useful log messages for all builds. https://codereview.chromium.org/184043021/diff/160001/content/child/webcrypto... content/child/webcrypto/shared_crypto_unittest.cc:366: DCHECK(SupportsAesGcm()); ditto https://codereview.chromium.org/184043021/diff/160001/content/child/webcrypto... content/child/webcrypto/shared_crypto_unittest.cc:831: if (SupportsAesGcm()) Interesting. So generating AES-GCM key works without GCM support, however import doesn't. Yuck! I look forward to when Chromium has moved off NSS alltogether and we can rely on a particular version of OpenSSL :(
https://codereview.chromium.org/184043021/diff/160001/content/child/webcrypto... File content/child/webcrypto/shared_crypto_unittest.cc (right): https://codereview.chromium.org/184043021/diff/160001/content/child/webcrypto... content/child/webcrypto/shared_crypto_unittest.cc:69: EXPECT_EQ(Status::ErrorUnsupported().ToString(), status.ToString()); On 2014/03/13 22:31:13, eroman wrote: > [optional] update this to use EXPECT_STATUS() Done. https://codereview.chromium.org/184043021/diff/160001/content/child/webcrypto... content/child/webcrypto/shared_crypto_unittest.cc:115: DCHECK(SupportsAesGcm()); On 2014/03/13 22:31:13, eroman wrote: > Instead of DCHECK() please use EXPECT_TRUE(). DCHECK() is only run for debug > builds, whereas EXPECT_TRUE() will give useful log messages for all builds. Done. https://codereview.chromium.org/184043021/diff/160001/content/child/webcrypto... content/child/webcrypto/shared_crypto_unittest.cc:366: DCHECK(SupportsAesGcm()); On 2014/03/13 22:31:13, eroman wrote: > ditto Done. https://codereview.chromium.org/184043021/diff/160001/content/child/webcrypto... content/child/webcrypto/shared_crypto_unittest.cc:831: if (SupportsAesGcm()) On 2014/03/13 22:31:13, eroman wrote: > Interesting. So generating AES-GCM key works without GCM support, however import > doesn't. Yes, appears so. Strange. > Yuck! I look forward to when Chromium has moved off NSS alltogether and we can > rely on a particular version of OpenSSL :( Hmm. Something tells me I might be working on project for a long time. :-)
lgtm https://codereview.chromium.org/184043021/diff/180001/content/child/webcrypto... File content/child/webcrypto/shared_crypto_unittest.cc (right): https://codereview.chromium.org/184043021/diff/180001/content/child/webcrypto... content/child/webcrypto/shared_crypto_unittest.cc:2546: LOG(WARNING) << "AES GCM not supported, skipping tests"; I don't think we need the CheckAesGcm test since this one will already log anyway. Consider moving the warning text over here instead.
https://codereview.chromium.org/184043021/diff/180001/content/child/webcrypto... File content/child/webcrypto/shared_crypto_unittest.cc (right): https://codereview.chromium.org/184043021/diff/180001/content/child/webcrypto... content/child/webcrypto/shared_crypto_unittest.cc:2546: LOG(WARNING) << "AES GCM not supported, skipping tests"; On 2014/03/14 05:37:20, eroman wrote: > I don't think we need the CheckAesGcm test since this one will already log > anyway. Consider moving the warning text over here instead. My reasoning for putting in the CheckAesGcm test was to ensure something gets in the log independent of any other tests.
The CQ bit was checked by padolph@netflix.com
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/padolph@netflix.com/184043021/180001
Message was sent while issue was closed.
Change committed as 257078
Message was sent while issue was closed.
A revert of this CL has been created in https://codereview.chromium.org/200263002/ by finnur@chromium.org. The reason for reverting is: SharedCryptoTest.ImportExportJwkSymmetricKey test failing. http://build.chromium.org/p/chromium.linux/builders/Linux%20Clang%20%28dbg%29... .
Message was sent while issue was closed.
On 2014/03/14 12:31:54, Finnur wrote: > A revert of this CL has been created in > https://codereview.chromium.org/200263002/ by mailto:finnur@chromium.org. > > The reason for reverting is: SharedCryptoTest.ImportExportJwkSymmetricKey test > failing. > http://build.chromium.org/p/chromium.linux/builders/Linux%2520Clang%2520%2528... > > . I missed a runtime check to disable a test when the local system NSS lib does not support AES-GCM. Will patch and try again.
The CQ bit was checked by padolph@netflix.com
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/padolph@netflix.com/184043021/190001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: tryserver.chromium on linux_chromium_clang_dbg
The CQ bit was checked by padolph@netflix.com
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/padolph@netflix.com/184043021/190001
The CQ bit was unchecked by commit-bot@chromium.org
Retried try job too often on win_rel for step(s) net_unittests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...
The CQ bit was checked by padolph@netflix.com
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/padolph@netflix.com/184043021/190001
patchset 11 lgtm
Message was sent while issue was closed.
Change committed as 257217 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
