Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
23569007:
WebCrypto: Implement importKey() and sign() for HMAC in NSS (Closed)
|
Created:
7 years, 3 months ago by Bryan Eyler Modified:
7 years, 3 months ago CC:
chromium-reviews, joi+watch-content_chromium.org, darin-cc_chromium.org, jam Base URL:
https://chromium.googlesource.com/chromium/src.git@master Visibility:
Public. |
DescriptionWebCrypto: Implement importKey() and sign() for HMAC in NSS
BUG=245025
R=eroman@chromium.org,ellyjones@chromium.org,jamesr@chromium.org,mmoss@chromium.org,phajdan.jr@chromium.org
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=223813
Patch Set 1 #
Total comments: 63
Patch Set 2 : Fixes to review from eroman #
Total comments: 21
Patch Set 3 : More fixes to review from eroman #
Total comments: 17
Patch Set 4 : Fixes to NSS #
Total comments: 13
Patch Set 5 : More fixes to NSS. #Patch Set 6 : Fix try failures. #Patch Set 7 : Rebase. Another attempt to fix try errors. #
Total comments: 4
Patch Set 8 : Remove duplication of usage, fix naming, add length sanity check. Rebase. #
Total comments: 18
Patch Set 9 : Fixes to NSS; removal of redundant storage. Rebased. #
Total comments: 10
Patch Set 10 : A few small clean-up tasks before sending for owners. #Patch Set 11 : Rebase and re-upload. #Patch Set 12 : Rebase and re-upload. #Patch Set 13 : Re-submit with fixed external dependencies. #
Total comments: 1
Messages
Total messages: 42 (0 generated)
I tried to keep this as close to easy to merge with https://codereview.chromium.org/23533010/ as possible.
Also, not sure what I should do about that data in the unit tests. Should this still reflect the 80-character line limits and just break it up?
Ryan, you don't need to review this yet. But I do have a few inline comments prefixed with "sleevi:" which would be great if you can answer. https://codereview.chromium.org/23569007/diff/1/content/renderer/webcrypto_im... File content/renderer/webcrypto_impl.cc (right): https://codereview.chromium.org/23569007/diff/1/content/renderer/webcrypto_im... content/renderer/webcrypto_impl.cc:37: type = WebKit::WebCryptoKeyTypeSecret; For the future, we are better off having the "type" be set by imortKeyIntenal(), just like how Elly does generateKeyInternal(). Later down the road when importing other key types, it may not be known until the import has completed whether it was a private or public key. https://codereview.chromium.org/23569007/diff/1/content/renderer/webcrypto_im... content/renderer/webcrypto_impl.cc:52: result.completeWithError(); You need a return statement after this https://codereview.chromium.org/23569007/diff/1/content/renderer/webcrypto_im... File content/renderer/webcrypto_impl.h (right): https://codereview.chromium.org/23569007/diff/1/content/renderer/webcrypto_im... content/renderer/webcrypto_impl.h:26: const unsigned char* keyData, style: key_data (unfortunately things can get confusing here because Blink and Chromium have different styles!) https://codereview.chromium.org/23569007/diff/1/content/renderer/webcrypto_im... content/renderer/webcrypto_impl.h:27: size_t keyDataSize, ditto https://codereview.chromium.org/23569007/diff/1/content/renderer/webcrypto_im... content/renderer/webcrypto_impl.h:53: bool extractable, Can remove this it is unused. https://codereview.chromium.org/23569007/diff/1/content/renderer/webcrypto_im... content/renderer/webcrypto_impl.h:55: WebKit::WebCryptoKeyHandle** handle); I asked on Elly's code review for generateKeyInternal() to be a scoped_ptr<>* instead, I recommend the same approach for this function. I also didn't notice on our previous code review which added digestIntenal(), but these function names should be in chromium style. i.e.: ImportKeyInternal(). https://codereview.chromium.org/23569007/diff/1/content/renderer/webcrypto_im... File content/renderer/webcrypto_impl_nss.cc (right): https://codereview.chromium.org/23569007/diff/1/content/renderer/webcrypto_im... content/renderer/webcrypto_impl_nss.cc:22: mechanism_ = mechanism; What is the initial value? I would rather this be an attribute of the constructor so it has to be specified. https://codereview.chromium.org/23569007/diff/1/content/renderer/webcrypto_im... content/renderer/webcrypto_impl_nss.cc:24: void set_slot(PK11SlotInfo* slot) { I don't see any current need to specify a slot other than that from GetInternalSlot(). To that end, can the slot be set automatically by the constructor? https://codereview.chromium.org/23569007/diff/1/content/renderer/webcrypto_im... content/renderer/webcrypto_impl_nss.cc:27: void set_key(PK11SymKey* key) { This effectively passes ownership of the key. I think it would be more expressive to have it take a ScopedPK11SymKey key here, and do a "move". Just thinking out loud right now, but to avoid "moving" stuff around while building the key, another code organization would be to have a factory method on WebCryptoSymKeyHandle, like WebCryptoSymKeyHandle* CreateByImporting(...). Just throwing it out as an idea, not sure which design is cleaner :) https://codereview.chromium.org/23569007/diff/1/content/renderer/webcrypto_im... content/renderer/webcrypto_impl_nss.cc:109: WebKit::WebCryptoHmacKeyParams* params = algorithm.hmacKeyParams(); Ah! I should have defined these as "const" pointers in the API. Please assume they are "const", I will update the public API to fix this :) https://codereview.chromium.org/23569007/diff/1/content/renderer/webcrypto_im... content/renderer/webcrypto_impl_nss.cc:114: switch (params->hash().id()) { You will have to coordinate with Elly's changelist here (she is added an algorithm information table which enumerates these properties). You can merge that once it ends up landing (tough timing!) https://codereview.chromium.org/23569007/diff/1/content/renderer/webcrypto_im... content/renderer/webcrypto_impl_nss.cc:137: ((usage_mask & WebKit::WebCryptoKeyUsageEncrypt) ? CKF_ENCRYPT : 0) | sleevi: My understanding is the NSS usage flags are not really respected. It certainly seems prudent to set them, in case it has any effect on some internals for how the key is provisioned. But curious if this has an effect. bryaneyler: I suggest extracting this to a helper function since it will surely be used elsewhere. Also as an aside, the Blink layer enforces the key usage before delegating operations to the embedding layer, so it will not be necessary to verify key usage mask in each operation (other than asserting if you wanted to). https://codereview.chromium.org/23569007/diff/1/content/renderer/webcrypto_im... content/renderer/webcrypto_impl_nss.cc:152: memcpy(raw_key_data, key_data, raw_key_data_size); This is unnecessary. You can just pass a SECItem which references the data directly, no copying necessary. sleevi: Right? https://codereview.chromium.org/23569007/diff/1/content/renderer/webcrypto_im... content/renderer/webcrypto_impl_nss.cc:176: delete[] raw_key_data; See my comment above. We don't want to duplicate the key data. Moreover if we did, you would want to use a scoped container to handle the subsequent deletion. https://codereview.chromium.org/23569007/diff/1/content/renderer/webcrypto_im... content/renderer/webcrypto_impl_nss.cc:182: delete[] raw_key_data; See above. We try hard in Chromium not to do manual memory management of this sort, as it is very easy to get wrong. https://codereview.chromium.org/23569007/diff/1/content/renderer/webcrypto_im... content/renderer/webcrypto_impl_nss.cc:196: WebKit::WebCryptoHmacParams* params = algorithm.hmacParams(); const-ify please. I will change the API to disallow non-const accesses :) https://codereview.chromium.org/23569007/diff/1/content/renderer/webcrypto_im... content/renderer/webcrypto_impl_nss.cc:207: digest_length = 20; Rather than hardcode this, can you use a function to retrieve it? For instance, HASH_ResultLen(). Note that the digest() code above already has a switch statement to convert WebCryptoAlgorithmId --> HASH_HashType, so you can probably just extract that to a helper function. sleevi: Or is there a better way that you recommend? https://codereview.chromium.org/23569007/diff/1/content/renderer/webcrypto_im... content/renderer/webcrypto_impl_nss.cc:234: if (PK11_SignWithSymKey(const_cast<PK11SymKey*>(sym_key->key()), Please make key() return a non-const PK11SymKey rather than const-casting. https://codereview.chromium.org/23569007/diff/1/content/renderer/webcrypto_im... File content/renderer/webcrypto_impl_unittest.cc (right): https://codereview.chromium.org/23569007/diff/1/content/renderer/webcrypto_im... content/renderer/webcrypto_impl_unittest.cc:128: // For this data, see http://csrc.nist.gov/groups/STM/cavp/index.html#07 I suggest additionally having a test for the "empty" data case. Just cuz it can sometimes be a bit different. https://codereview.chromium.org/23569007/diff/1/content/renderer/webcrypto_im... content/renderer/webcrypto_impl_unittest.cc:135: "a3ce8899df1022e8d2d539b47bf0e309c66f84095e21438ec355bf119ce5fdcb4e73a619cdf36f25b369d8c38ff419997f0c59830108223606e31223483fd39edeaa4d3f0d21198862d239c9fd26074130ff6c86493f5227ab895c8f244bd42c7afce5d147a20a590798c68e708e964902d124dadecdbda9dbd0051ed710e9bf", regarding comment on line length: I think it is fine for test data, and I wouldn't worry about it too much. To be on the safe side, you could always break it up to fit on 80 character lines tho since it is easy enough. i.e use C's auto-line joiner for string literals (whatever it is called) "Yo dawg, this string is long" can be broken up as: "Yo dawg, " "this string is long" https://codereview.chromium.org/23569007/diff/1/content/renderer/webcrypto_im... content/renderer/webcrypto_impl_unittest.cc:164: input_set[index].algorithm, "HASH", NULL)); Remove the "HASH" parameter. I am in the process of getting rid of that from the Blink API (https://codereview.chromium.org/23672024/) since it was annoying to pass in the unit tests like this :) https://codereview.chromium.org/23569007/diff/1/content/renderer/webcrypto_im... content/renderer/webcrypto_impl_unittest.cc:166: scoped_ptr<WebKit::WebCryptoHmacKeyParams> key_params( Use a WebCryptoHmacParams instead of a WebCryptoHmacKeyParams. Things should work fine with either, however to simulate how the code will be getting called, better to use the HmacParams (HmacKeyParams is only used for generateKey). You can tell this by looking at the operations table here: https://dvcs.w3.org/hg/webcrypto-api/raw-file/tip/spec/Overview.html#hmac Notice how the input to importKey has "Parameters" of HmacParams. The Blink side of the code which does algorithm parameter normalization will guarantee that the Chromium code only gets called with the expected parameter type. https://codereview.chromium.org/23569007/diff/1/content/renderer/webcrypto_im... content/renderer/webcrypto_impl_unittest.cc:171: WebKit::WebCryptoAlgorithmIdHmac, "HMAC", key_params.release())); Ditto. Remove the "HMAC" string, no longer needed. Yay https://codereview.chromium.org/23569007/diff/1/content/renderer/webcrypto_im... content/renderer/webcrypto_impl_unittest.cc:173: WebKit::WebCryptoKeyHandle* handle; This is being leaked. That is why I would like the internal* methods to use scoped_ptr<>, so it is very clear how the ownership of the returned pointer works :) https://codereview.chromium.org/23569007/diff/1/content/renderer/webcrypto_im... content/renderer/webcrypto_impl_unittest.cc:182: &key_raw.front(), It is more typical in chromium code to use .data() https://codereview.chromium.org/23569007/diff/1/content/renderer/webcrypto_im... content/renderer/webcrypto_impl_unittest.cc:193: new WebKit::WebCryptoHmacParams(hash_algorithm)); Optional: creating the WebCryptoAlgorithm for tests is clumsy enough, that I suggest extracting that code to helpers. For example: WebCryptoAlgorithm sup_dawg = HmacAlgorithm(WebCryptoAlgorithmIdSha1); And another extractable method that might be useful for the pre-existing digest tests: WebCryptoAlgorithm magnets = ShaAlgorithm(256) (OK, maybe using an integer parameter is crazy, but just spitballing some ideas) https://codereview.chromium.org/23569007/diff/1/content/renderer/webcrypto_im... content/renderer/webcrypto_impl_unittest.cc:197: WebKit::WebCryptoAlgorithmIdHmac, "HMAC", sign_params.release())); remove "HMAC" https://codereview.chromium.org/23569007/diff/1/content/renderer/webcrypto_im... content/renderer/webcrypto_impl_unittest.cc:204: &msg_raw.front(), msg_raw.data() is more concise in my opinion.
Thanks for the review. You caught a lot of good issues. https://codereview.chromium.org/23569007/diff/1/content/renderer/webcrypto_im... File content/renderer/webcrypto_impl.cc (right): https://codereview.chromium.org/23569007/diff/1/content/renderer/webcrypto_im... content/renderer/webcrypto_impl.cc:37: type = WebKit::WebCryptoKeyTypeSecret; On 2013/09/05 01:57:51, eroman wrote: > For the future, we are better off having the "type" be set by imortKeyIntenal(), > just like how Elly does generateKeyInternal(). Later down the road when > importing other key types, it may not be known until the import has completed > whether it was a private or public key. Done. https://codereview.chromium.org/23569007/diff/1/content/renderer/webcrypto_im... content/renderer/webcrypto_impl.cc:52: result.completeWithError(); On 2013/09/05 01:57:51, eroman wrote: > You need a return statement after this Done. https://codereview.chromium.org/23569007/diff/1/content/renderer/webcrypto_im... File content/renderer/webcrypto_impl.h (right): https://codereview.chromium.org/23569007/diff/1/content/renderer/webcrypto_im... content/renderer/webcrypto_impl.h:26: const unsigned char* keyData, On 2013/09/05 01:57:51, eroman wrote: > style: key_data (unfortunately things can get confusing here because Blink and > Chromium have different styles!) Done. https://codereview.chromium.org/23569007/diff/1/content/renderer/webcrypto_im... content/renderer/webcrypto_impl.h:27: size_t keyDataSize, On 2013/09/05 01:57:51, eroman wrote: > ditto Done. https://codereview.chromium.org/23569007/diff/1/content/renderer/webcrypto_im... content/renderer/webcrypto_impl.h:53: bool extractable, On 2013/09/05 01:57:51, eroman wrote: > Can remove this it is unused. Done. https://codereview.chromium.org/23569007/diff/1/content/renderer/webcrypto_im... content/renderer/webcrypto_impl.h:55: WebKit::WebCryptoKeyHandle** handle); On 2013/09/05 01:57:51, eroman wrote: > I asked on Elly's code review for generateKeyInternal() to be a scoped_ptr<>* > instead, I recommend the same approach for this function. > > I also didn't notice on our previous code review which added digestIntenal(), > but these function names should be in chromium style. i.e.: > > ImportKeyInternal(). Done. And fixed DigestInternal. https://codereview.chromium.org/23569007/diff/1/content/renderer/webcrypto_im... File content/renderer/webcrypto_impl_nss.cc (right): https://codereview.chromium.org/23569007/diff/1/content/renderer/webcrypto_im... content/renderer/webcrypto_impl_nss.cc:22: mechanism_ = mechanism; On 2013/09/05 01:57:51, eroman wrote: > What is the initial value? I would rather this be an attribute of the > constructor so it has to be specified. Yeah, initial value appears to be not clear. Moved to constructor. https://codereview.chromium.org/23569007/diff/1/content/renderer/webcrypto_im... content/renderer/webcrypto_impl_nss.cc:24: void set_slot(PK11SlotInfo* slot) { On 2013/09/05 01:57:51, eroman wrote: > I don't see any current need to specify a slot other than that from > GetInternalSlot(). To that end, can the slot be set automatically by the > constructor? Sure, makes sense. https://codereview.chromium.org/23569007/diff/1/content/renderer/webcrypto_im... content/renderer/webcrypto_impl_nss.cc:27: void set_key(PK11SymKey* key) { On 2013/09/05 01:57:51, eroman wrote: > This effectively passes ownership of the key. I think it would be more > expressive to have it take a ScopedPK11SymKey key here, and do a "move". > > Just thinking out loud right now, but to avoid "moving" stuff around while > building the key, another code organization would be to have a factory method on > WebCryptoSymKeyHandle, like WebCryptoSymKeyHandle* CreateByImporting(...). Just > throwing it out as an idea, not sure which design is cleaner :) It gets complicated as the slot is needed for creating the key, so if we want the GetInternalSlot() to be part of the constructor, we can't have a factory whose arguments depend on that. We could have a factory that takes in the slot? In the meantime, I've made this a ScopedPK11SymKey to make the tracking better. https://codereview.chromium.org/23569007/diff/1/content/renderer/webcrypto_im... content/renderer/webcrypto_impl_nss.cc:109: WebKit::WebCryptoHmacKeyParams* params = algorithm.hmacKeyParams(); On 2013/09/05 01:57:51, eroman wrote: > Ah! I should have defined these as "const" pointers in the API. Please assume > they are "const", I will update the public API to fix this :) Done. https://codereview.chromium.org/23569007/diff/1/content/renderer/webcrypto_im... content/renderer/webcrypto_impl_nss.cc:114: switch (params->hash().id()) { On 2013/09/05 01:57:51, eroman wrote: > You will have to coordinate with Elly's changelist here (she is added an > algorithm information table which enumerates these properties). You can merge > that once it ends up landing (tough timing!) Yeah, I realized there would be a bit of overlap/merge issues. https://codereview.chromium.org/23569007/diff/1/content/renderer/webcrypto_im... content/renderer/webcrypto_impl_nss.cc:137: ((usage_mask & WebKit::WebCryptoKeyUsageEncrypt) ? CKF_ENCRYPT : 0) | On 2013/09/05 01:57:51, eroman wrote: > sleevi: My understanding is the NSS usage flags are not really respected. > > It certainly seems prudent to set them, in case it has any effect on some > internals for how the key is provisioned. > > But curious if this has an effect. > > bryaneyler: I suggest extracting this to a helper function since it will surely > be used elsewhere. Also as an aside, the Blink layer enforces the key usage > before delegating operations to the embedding layer, so it will not be necessary > to verify key usage mask in each operation (other than asserting if you wanted > to). I tried not setting the appropriate flags and it failed, so I think this is necessary to set them. Alternatively, we could just set them all if you think we should just rely on Blink? https://codereview.chromium.org/23569007/diff/1/content/renderer/webcrypto_im... content/renderer/webcrypto_impl_nss.cc:152: memcpy(raw_key_data, key_data, raw_key_data_size); On 2013/09/05 01:57:51, eroman wrote: > This is unnecessary. You can just pass a SECItem which references the data > directly, no copying necessary. > > sleevi: Right? Yes, true. Done. https://codereview.chromium.org/23569007/diff/1/content/renderer/webcrypto_im... content/renderer/webcrypto_impl_nss.cc:176: delete[] raw_key_data; On 2013/09/05 01:57:51, eroman wrote: > See my comment above. We don't want to duplicate the key data. Moreover if we > did, you would want to use a scoped container to handle the subsequent deletion. Done. https://codereview.chromium.org/23569007/diff/1/content/renderer/webcrypto_im... content/renderer/webcrypto_impl_nss.cc:182: delete[] raw_key_data; On 2013/09/05 01:57:51, eroman wrote: > See above. We try hard in Chromium not to do manual memory management of this > sort, as it is very easy to get wrong. Done. https://codereview.chromium.org/23569007/diff/1/content/renderer/webcrypto_im... content/renderer/webcrypto_impl_nss.cc:196: WebKit::WebCryptoHmacParams* params = algorithm.hmacParams(); On 2013/09/05 01:57:51, eroman wrote: > const-ify please. I will change the API to disallow non-const accesses :) Done. https://codereview.chromium.org/23569007/diff/1/content/renderer/webcrypto_im... content/renderer/webcrypto_impl_nss.cc:207: digest_length = 20; On 2013/09/05 01:57:51, eroman wrote: > Rather than hardcode this, can you use a function to retrieve it? > > For instance, HASH_ResultLen(). Note that the digest() code above already has a > switch statement to convert WebCryptoAlgorithmId --> HASH_HashType, so you can > probably just extract that to a helper function. > > sleevi: Or is there a better way that you recommend? Done. In order to do the DCHECK, I also needed to extract out the mechanism, which may also come in handy later on. https://codereview.chromium.org/23569007/diff/1/content/renderer/webcrypto_im... content/renderer/webcrypto_impl_nss.cc:234: if (PK11_SignWithSymKey(const_cast<PK11SymKey*>(sym_key->key()), On 2013/09/05 01:57:51, eroman wrote: > Please make key() return a non-const PK11SymKey rather than const-casting. I need to const_cast at some point because the key passed in is a const. Should I const_cast when casting to sym_key? https://codereview.chromium.org/23569007/diff/1/content/renderer/webcrypto_im... File content/renderer/webcrypto_impl_unittest.cc (right): https://codereview.chromium.org/23569007/diff/1/content/renderer/webcrypto_im... content/renderer/webcrypto_impl_unittest.cc:128: // For this data, see http://csrc.nist.gov/groups/STM/cavp/index.html#07 On 2013/09/05 01:57:51, eroman wrote: > I suggest additionally having a test for the "empty" data case. Just cuz it can > sometimes be a bit different. Done. https://codereview.chromium.org/23569007/diff/1/content/renderer/webcrypto_im... content/renderer/webcrypto_impl_unittest.cc:135: "a3ce8899df1022e8d2d539b47bf0e309c66f84095e21438ec355bf119ce5fdcb4e73a619cdf36f25b369d8c38ff419997f0c59830108223606e31223483fd39edeaa4d3f0d21198862d239c9fd26074130ff6c86493f5227ab895c8f244bd42c7afce5d147a20a590798c68e708e964902d124dadecdbda9dbd0051ed710e9bf", On 2013/09/05 01:57:51, eroman wrote: > regarding comment on line length: I think it is fine for test data, and I > wouldn't worry about it too much. To be on the safe side, you could always break > it up to fit on 80 character lines tho since it is easy enough. > > i.e use C's auto-line joiner for string literals (whatever it is called) > > "Yo dawg, this string is long" can be broken up as: > > "Yo dawg, " > "this string is long" Done. https://codereview.chromium.org/23569007/diff/1/content/renderer/webcrypto_im... content/renderer/webcrypto_impl_unittest.cc:164: input_set[index].algorithm, "HASH", NULL)); On 2013/09/05 01:57:51, eroman wrote: > Remove the "HASH" parameter. I am in the process of getting rid of that from the > Blink API (https://codereview.chromium.org/23672024/) since it was annoying to > pass in the unit tests like this :) Done. https://codereview.chromium.org/23569007/diff/1/content/renderer/webcrypto_im... content/renderer/webcrypto_impl_unittest.cc:166: scoped_ptr<WebKit::WebCryptoHmacKeyParams> key_params( On 2013/09/05 01:57:51, eroman wrote: > Use a WebCryptoHmacParams instead of a WebCryptoHmacKeyParams. > > Things should work fine with either, however to simulate how the code will be > getting called, better to use the HmacParams (HmacKeyParams is only used for > generateKey). > > You can tell this by looking at the operations table here: > > https://dvcs.w3.org/hg/webcrypto-api/raw-file/tip/spec/Overview.html#hmac > > Notice how the input to importKey has "Parameters" of HmacParams. The Blink side > of the code which does algorithm parameter normalization will guarantee that the > Chromium code only gets called with the expected parameter type. Done. https://codereview.chromium.org/23569007/diff/1/content/renderer/webcrypto_im... content/renderer/webcrypto_impl_unittest.cc:171: WebKit::WebCryptoAlgorithmIdHmac, "HMAC", key_params.release())); On 2013/09/05 01:57:51, eroman wrote: > Ditto. Remove the "HMAC" string, no longer needed. Yay Done. https://codereview.chromium.org/23569007/diff/1/content/renderer/webcrypto_im... content/renderer/webcrypto_impl_unittest.cc:173: WebKit::WebCryptoKeyHandle* handle; On 2013/09/05 01:57:51, eroman wrote: > This is being leaked. > > That is why I would like the internal* methods to use scoped_ptr<>, so it is > very clear how the ownership of the returned pointer works :) Made into scoped_ptr. https://codereview.chromium.org/23569007/diff/1/content/renderer/webcrypto_im... content/renderer/webcrypto_impl_unittest.cc:182: &key_raw.front(), On 2013/09/05 01:57:51, eroman wrote: > It is more typical in chromium code to use .data() Done. https://codereview.chromium.org/23569007/diff/1/content/renderer/webcrypto_im... content/renderer/webcrypto_impl_unittest.cc:193: new WebKit::WebCryptoHmacParams(hash_algorithm)); On 2013/09/05 01:57:51, eroman wrote: > Optional: creating the WebCryptoAlgorithm for tests is clumsy enough, that I > suggest extracting that code to helpers. For example: > > WebCryptoAlgorithm sup_dawg = HmacAlgorithm(WebCryptoAlgorithmIdSha1); > > And another extractable method that might be useful for the pre-existing digest > tests: > WebCryptoAlgorithm magnets = ShaAlgorithm(256) > > (OK, maybe using an integer parameter is crazy, but just spitballing some ideas) This duplication is actually a result of the mistake above where I was using HmacKeyParams instead of HmacParams. I've removed this code and unified the algorithm used. Should I break it out only if it's necessary in the future? https://codereview.chromium.org/23569007/diff/1/content/renderer/webcrypto_im... content/renderer/webcrypto_impl_unittest.cc:197: WebKit::WebCryptoAlgorithmIdHmac, "HMAC", sign_params.release())); On 2013/09/05 01:57:51, eroman wrote: > remove "HMAC" Done. https://codereview.chromium.org/23569007/diff/1/content/renderer/webcrypto_im... content/renderer/webcrypto_impl_unittest.cc:204: &msg_raw.front(), On 2013/09/05 01:57:51, eroman wrote: > msg_raw.data() is more concise in my opinion. Done.
https://codereview.chromium.org/23569007/diff/1/content/renderer/webcrypto_im... File content/renderer/webcrypto_impl_nss.cc (right): https://codereview.chromium.org/23569007/diff/1/content/renderer/webcrypto_im... content/renderer/webcrypto_impl_nss.cc:137: ((usage_mask & WebKit::WebCryptoKeyUsageEncrypt) ? CKF_ENCRYPT : 0) | On 2013/09/05 01:57:51, eroman wrote: > sleevi: My understanding is the NSS usage flags are not really respected. > > It certainly seems prudent to set them, in case it has any effect on some > internals for how the key is provisioned. > > But curious if this has an effect. Not for softoken (effectively), but definitely for other token types. The correct masks should be set. > > bryaneyler: I suggest extracting this to a helper function since it will surely > be used elsewhere. Also as an aside, the Blink layer enforces the key usage > before delegating operations to the embedding layer, so it will not be necessary > to verify key usage mask in each operation (other than asserting if you wanted > to). https://codereview.chromium.org/23569007/diff/1/content/renderer/webcrypto_im... content/renderer/webcrypto_impl_nss.cc:152: memcpy(raw_key_data, key_data, raw_key_data_size); On 2013/09/05 01:57:51, eroman wrote: > This is unnecessary. You can just pass a SECItem which references the data > directly, no copying necessary. > > sleevi: Right? Right https://codereview.chromium.org/23569007/diff/1/content/renderer/webcrypto_im... content/renderer/webcrypto_impl_nss.cc:207: digest_length = 20; On 2013/09/05 01:57:51, eroman wrote: > Rather than hardcode this, can you use a function to retrieve it? > > For instance, HASH_ResultLen(). Note that the digest() code above already has a > switch statement to convert WebCryptoAlgorithmId --> HASH_HashType, so you can > probably just extract that to a helper function. > > sleevi: Or is there a better way that you recommend? +1 to your recommended way.
looks good after this comment batch! https://codereview.chromium.org/23569007/diff/1/content/renderer/webcrypto_im... File content/renderer/webcrypto_impl_nss.cc (right): https://codereview.chromium.org/23569007/diff/1/content/renderer/webcrypto_im... content/renderer/webcrypto_impl_nss.cc:33: const PK11SymKey* key() const { return key_.get(); } Don't make the pointer const. Then there is no need to const-cast later! https://codereview.chromium.org/23569007/diff/1/content/renderer/webcrypto_im... content/renderer/webcrypto_impl_nss.cc:234: if (PK11_SignWithSymKey(const_cast<PK11SymKey*>(sym_key->key()), On 2013/09/06 01:27:51, Bryan Eyler wrote: > On 2013/09/05 01:57:51, eroman wrote: > > Please make key() return a non-const PK11SymKey rather than const-casting. > > I need to const_cast at some point because the key passed in is a const. Should > I const_cast when casting to sym_key? Yep, make sym_key non-const, and then change the method on SymKeyHandle to return a non-const pointer. Shouldn't need a const-cast here. https://codereview.chromium.org/23569007/diff/9001/content/renderer/webcrypto... File content/renderer/webcrypto_impl_nss.cc (right): https://codereview.chromium.org/23569007/diff/9001/content/renderer/webcrypto... content/renderer/webcrypto_impl_nss.cc:21: WebCryptoSymKeyHandle(CK_MECHANISM_TYPE mechanism) single-parameter ctors should be marked as "explicit" https://codereview.chromium.org/23569007/diff/9001/content/renderer/webcrypto... content/renderer/webcrypto_impl_nss.cc:24: DCHECK(slot_ != NULL); It is customary in chrome to omit the != NULL. just: DCHECK(slot_); or if that doesn't compile then: DCHECK(slot_.get()); Just to show my NSS ignorance for a moment, is failure possible in the API? (in which case, this should be a handled error case rather than a DCHECK). https://codereview.chromium.org/23569007/diff/9001/content/renderer/webcrypto... content/renderer/webcrypto_impl_nss.cc:27: void set_key(crypto::ScopedPK11SymKey key) { Good. I suggest adding a DCHECK(!key_) for good measure, as this is only supposed to be called once. https://codereview.chromium.org/23569007/diff/9001/content/renderer/webcrypto... content/renderer/webcrypto_impl_nss.cc:35: private: Please also add DISALLOW_COPY_AND_ASSIGN (or whatever it is). Not strictly necessary since the scoped slot makes this uncopiable, but still good practice. https://codereview.chromium.org/23569007/diff/9001/content/renderer/webcrypto... content/renderer/webcrypto_impl_nss.cc:41: static CK_FLAGS WebCryptoKeyUsageMaskToNSSFlags( I recommend moving all of this section into an unnamed namespace rather than labeling them static. Thanks for moving this to a standalone function! https://codereview.chromium.org/23569007/diff/9001/content/renderer/webcrypto... content/renderer/webcrypto_impl_nss.cc:161: SECItem key_item = { nit: inconsistent style (later on you initialize a SecItem just like this all on one line). https://codereview.chromium.org/23569007/diff/9001/content/renderer/webcrypto... content/renderer/webcrypto_impl_nss.cc:170: key_item.len = key_data_size; Hmm. NSS expects "unsigned int" for lengths, however here we are setting it with a size_t. Under the LP64 datamodel, size_t will be 64-bits whereas unsigned int will be 32-bits, so that could result in truncation. Unfortunately chromium doesn't treat integer narrowing warnings as an error, so it isn't easy to notice. In practice the blink side will not pass individual buffers exceeding 32-bit so this isn't a problem. I hadn't considered this issue when I designed the API. Probably the best thing is for me to change the api from "size_t" to "unsigned" and then we don't have to worry about this anymore :) https://codereview.chromium.org/23569007/diff/9001/content/renderer/webcrypto... content/renderer/webcrypto_impl_nss.cc:220: size_t digest_length = HASH_ResultLen(hash_type); unsigned int for consistency https://codereview.chromium.org/23569007/diff/9001/content/renderer/webcrypto... content/renderer/webcrypto_impl_nss.cc:236: digest_length I think this is more readable as result.byteLength(). https://codereview.chromium.org/23569007/diff/9001/content/renderer/webcrypto... content/renderer/webcrypto_impl_nss.cc:239: if (PK11_SignWithSymKey(const_cast<PK11SymKey*>(sym_key->key()), Commented on other patchset https://codereview.chromium.org/23569007/diff/9001/content/renderer/webcrypto... File content/renderer/webcrypto_impl_unittest.cc (right): https://codereview.chromium.org/23569007/diff/9001/content/renderer/webcrypto... content/renderer/webcrypto_impl_unittest.cc:228: ASSERT_TRUE(handle.get() != NULL); nit: typical to leave off "!= NULL" in chromiumland
https://codereview.chromium.org/23569007/diff/1/content/renderer/webcrypto_im... File content/renderer/webcrypto_impl_nss.cc (right): https://codereview.chromium.org/23569007/diff/1/content/renderer/webcrypto_im... content/renderer/webcrypto_impl_nss.cc:33: const PK11SymKey* key() const { return key_.get(); } On 2013/09/06 23:03:16, eroman wrote: > Don't make the pointer const. Then there is no need to const-cast later! Done. https://codereview.chromium.org/23569007/diff/1/content/renderer/webcrypto_im... content/renderer/webcrypto_impl_nss.cc:234: if (PK11_SignWithSymKey(const_cast<PK11SymKey*>(sym_key->key()), On 2013/09/06 23:03:16, eroman wrote: > On 2013/09/06 01:27:51, Bryan Eyler wrote: > > On 2013/09/05 01:57:51, eroman wrote: > > > Please make key() return a non-const PK11SymKey rather than const-casting. > > > > I need to const_cast at some point because the key passed in is a const. > Should > > I const_cast when casting to sym_key? > > Yep, make sym_key non-const, and then change the method on SymKeyHandle to > return a non-const pointer. Shouldn't need a const-cast here. Done. https://codereview.chromium.org/23569007/diff/9001/content/renderer/webcrypto... File content/renderer/webcrypto_impl_nss.cc (right): https://codereview.chromium.org/23569007/diff/9001/content/renderer/webcrypto... content/renderer/webcrypto_impl_nss.cc:21: WebCryptoSymKeyHandle(CK_MECHANISM_TYPE mechanism) On 2013/09/06 23:03:16, eroman wrote: > single-parameter ctors should be marked as "explicit" Done. https://codereview.chromium.org/23569007/diff/9001/content/renderer/webcrypto... content/renderer/webcrypto_impl_nss.cc:24: DCHECK(slot_ != NULL); On 2013/09/06 23:03:16, eroman wrote: > It is customary in chrome to omit the != NULL. just: > > DCHECK(slot_); > > or if that doesn't compile then: > > DCHECK(slot_.get()); > > Just to show my NSS ignorance for a moment, is failure possible in the API? (in > which case, this should be a handled error case rather than a DCHECK). Not much documentation on this, but crypto/ seems to handle the error case, so changed from DCHECK to error handling. https://codereview.chromium.org/23569007/diff/9001/content/renderer/webcrypto... content/renderer/webcrypto_impl_nss.cc:27: void set_key(crypto::ScopedPK11SymKey key) { On 2013/09/06 23:03:16, eroman wrote: > Good. > I suggest adding a DCHECK(!key_) for good measure, as this is only supposed to > be called once. Done. https://codereview.chromium.org/23569007/diff/9001/content/renderer/webcrypto... content/renderer/webcrypto_impl_nss.cc:35: private: On 2013/09/06 23:03:16, eroman wrote: > Please also add DISALLOW_COPY_AND_ASSIGN (or whatever it is). > > Not strictly necessary since the scoped slot makes this uncopiable, but still > good practice. Done. https://codereview.chromium.org/23569007/diff/9001/content/renderer/webcrypto... content/renderer/webcrypto_impl_nss.cc:41: static CK_FLAGS WebCryptoKeyUsageMaskToNSSFlags( On 2013/09/06 23:03:16, eroman wrote: > I recommend moving all of this section into an unnamed namespace rather than > labeling them static. > > Thanks for moving this to a standalone function! Done. https://codereview.chromium.org/23569007/diff/9001/content/renderer/webcrypto... content/renderer/webcrypto_impl_nss.cc:161: SECItem key_item = { On 2013/09/06 23:03:16, eroman wrote: > nit: inconsistent style (later on you initialize a SecItem just like this all on > one line). Done. https://codereview.chromium.org/23569007/diff/9001/content/renderer/webcrypto... content/renderer/webcrypto_impl_nss.cc:170: key_item.len = key_data_size; On 2013/09/06 23:03:16, eroman wrote: > Hmm. > > NSS expects "unsigned int" for lengths, however here we are setting it with a > size_t. > > Under the LP64 datamodel, size_t will be 64-bits whereas unsigned int will be > 32-bits, so that could result in truncation. > > Unfortunately chromium doesn't treat integer narrowing warnings as an error, so > it isn't easy to notice. > > In practice the blink side will not pass individual buffers exceeding 32-bit so > this isn't a problem. > > I hadn't considered this issue when I designed the API. Probably the best thing > is for me to change the api from "size_t" to "unsigned" and then we don't have > to worry about this anymore :) Will re-based once https://codereview.chromium.org/23537027/ lands. https://codereview.chromium.org/23569007/diff/9001/content/renderer/webcrypto... content/renderer/webcrypto_impl_nss.cc:220: size_t digest_length = HASH_ResultLen(hash_type); On 2013/09/06 23:03:16, eroman wrote: > unsigned int for consistency This is something James requested I change in my previous CL. In DigestInternal, I do a similar thing with HASH_ResultLenContext(), and it was requested to be made into size_t. https://codereview.chromium.org/23569007/diff/9001/content/renderer/webcrypto... content/renderer/webcrypto_impl_nss.cc:236: digest_length On 2013/09/06 23:03:16, eroman wrote: > I think this is more readable as result.byteLength(). Done. https://codereview.chromium.org/23569007/diff/9001/content/renderer/webcrypto... File content/renderer/webcrypto_impl_unittest.cc (right): https://codereview.chromium.org/23569007/diff/9001/content/renderer/webcrypto... content/renderer/webcrypto_impl_unittest.cc:228: ASSERT_TRUE(handle.get() != NULL); On 2013/09/06 23:03:16, eroman wrote: > nit: typical to leave off "!= NULL" in chromiumland Done.
LGTM. sleevi: Could you take a quick look to verify NSS side? https://codereview.chromium.org/23569007/diff/18001/content/renderer/webcrypt... File content/renderer/webcrypto_impl.h (right): https://codereview.chromium.org/23569007/diff/18001/content/renderer/webcrypt... content/renderer/webcrypto_impl.h:52: size_t key_data_size, I suggest making the internal method "unsigned int" in the meantime. I will update the virtual method once blink side gets rolled in. https://codereview.chromium.org/23569007/diff/18001/content/renderer/webcrypt... content/renderer/webcrypto_impl.h:61: size_t data_size, same here https://codereview.chromium.org/23569007/diff/18001/content/renderer/webcrypt... File content/renderer/webcrypto_impl_nss.cc (right): https://codereview.chromium.org/23569007/diff/18001/content/renderer/webcrypt... content/renderer/webcrypto_impl_nss.cc:8: #include <nss/pk11pub.h> keep these in sorted order (unless there is a header ordering dependency) https://codereview.chromium.org/23569007/diff/18001/content/renderer/webcrypt... content/renderer/webcrypto_impl_nss.cc:92: } nit: please add the comment: } // namespace https://codereview.chromium.org/23569007/diff/18001/content/renderer/webcrypt... File content/renderer/webcrypto_impl_openssl.cc (right): https://codereview.chromium.org/23569007/diff/18001/content/renderer/webcrypt... content/renderer/webcrypto_impl_openssl.cc:36: size_t data_size, ditto here if you make the change to "unsigned int"
https://codereview.chromium.org/23569007/diff/18001/content/renderer/webcrypt... File content/renderer/webcrypto_impl_nss.cc (right): https://codereview.chromium.org/23569007/diff/18001/content/renderer/webcrypt... content/renderer/webcrypto_impl_nss.cc:85: return CKM_SHA256_HMAC; FYI: In order to verify truncated HMACs, you'll need to use the CK_MAC_GENERAL_PARAMS with CKM_SHA*_HMAC_GENERAL See 6.19.3 of PKCS#11 v 2.30 draft 7 - namely, it behaves exactly the same as CKM_SHA256_HMAC, but is more general. https://codereview.chromium.org/23569007/diff/18001/content/renderer/webcrypt... content/renderer/webcrypto_impl_nss.cc:227: HASH_HashType hash_type = WebCryptoAlgorithmToNSSHashType(params->hash()); BUG: You have a lack of consistency between WebCryptoAlgorithmToNSSHashType and WebCryptoAlgorithmToNSSMechanism In particular, NSSMechanism only allows CKM_SHA1/256_HMAC, but NSSHashType allows the SHA-224/384/512 types. https://codereview.chromium.org/23569007/diff/18001/content/renderer/webcrypt... content/renderer/webcrypto_impl_nss.cc:232: size_t digest_length = HASH_ResultLen(hash_type); Rather than computing the expected length via digest_length, if you supply a NULL buffer for the output (namely, sig->data), then what will happen is sig->len will be updated with the appropriate length needed for the signature. This is because PK11_SignWithSymKey is a wrapper for C_Sign, and C_Sign (and C_SignFinal) uses the convention described in Section 11.2 for producing output. This will be the most flexible solution for working with any form of symmetric key, and avoids the whole hash concerns raised in the BUG above.
https://codereview.chromium.org/23569007/diff/18001/content/renderer/webcrypt... File content/renderer/webcrypto_impl_nss.cc (right): https://codereview.chromium.org/23569007/diff/18001/content/renderer/webcrypt... content/renderer/webcrypto_impl_nss.cc:227: HASH_HashType hash_type = WebCryptoAlgorithmToNSSHashType(params->hash()); On 2013/09/10 00:42:49, Ryan Sleevi wrote: > BUG: You have a lack of consistency between WebCryptoAlgorithmToNSSHashType and > WebCryptoAlgorithmToNSSMechanism > > In particular, NSSMechanism only allows CKM_SHA1/256_HMAC, but NSSHashType > allows the SHA-224/384/512 types. In practice this isn't a bug, since the Blink side verifies that key.algorithm.hash matches algorithm.hash for HMAC before passing it to the embedder (hence it will only be called for the subset of hashes that are supported). That being said, I do like your alternative to not calling HASH_ResultLen() in the first place. +1
https://codereview.chromium.org/23569007/diff/18001/content/renderer/webcrypt... File content/renderer/webcrypto_impl.h (right): https://codereview.chromium.org/23569007/diff/18001/content/renderer/webcrypt... content/renderer/webcrypto_impl.h:52: size_t key_data_size, On 2013/09/09 23:00:48, eroman wrote: > I suggest making the internal method "unsigned int" in the meantime. I will > update the virtual method once blink side gets rolled in. Done. https://codereview.chromium.org/23569007/diff/18001/content/renderer/webcrypt... content/renderer/webcrypto_impl.h:61: size_t data_size, On 2013/09/09 23:00:48, eroman wrote: > same here Done. https://codereview.chromium.org/23569007/diff/18001/content/renderer/webcrypt... File content/renderer/webcrypto_impl_nss.cc (right): https://codereview.chromium.org/23569007/diff/18001/content/renderer/webcrypt... content/renderer/webcrypto_impl_nss.cc:8: #include <nss/pk11pub.h> On 2013/09/09 23:00:48, eroman wrote: > keep these in sorted order (unless there is a header ordering dependency) Done. https://codereview.chromium.org/23569007/diff/18001/content/renderer/webcrypt... content/renderer/webcrypto_impl_nss.cc:85: return CKM_SHA256_HMAC; On 2013/09/10 00:42:49, Ryan Sleevi wrote: > FYI: In order to verify truncated HMACs, you'll need to use the > CK_MAC_GENERAL_PARAMS with CKM_SHA*_HMAC_GENERAL > > See 6.19.3 of PKCS#11 v 2.30 draft 7 - namely, it behaves exactly the same as > CKM_SHA256_HMAC, but is more general. If I rely on the signing to get the length (as described in the comments later in the file), then I should keep this as the non-general versions, correct? https://codereview.chromium.org/23569007/diff/18001/content/renderer/webcrypt... content/renderer/webcrypto_impl_nss.cc:92: } On 2013/09/09 23:00:48, eroman wrote: > nit: please add the comment: > } // namespace Done. https://codereview.chromium.org/23569007/diff/18001/content/renderer/webcrypt... content/renderer/webcrypto_impl_nss.cc:227: HASH_HashType hash_type = WebCryptoAlgorithmToNSSHashType(params->hash()); On 2013/09/10 00:42:49, Ryan Sleevi wrote: > BUG: You have a lack of consistency between WebCryptoAlgorithmToNSSHashType and > WebCryptoAlgorithmToNSSMechanism > > In particular, NSSMechanism only allows CKM_SHA1/256_HMAC, but NSSHashType > allows the SHA-224/384/512 types. Done as per next comment. https://codereview.chromium.org/23569007/diff/18001/content/renderer/webcrypt... content/renderer/webcrypto_impl_nss.cc:232: size_t digest_length = HASH_ResultLen(hash_type); On 2013/09/10 00:42:49, Ryan Sleevi wrote: > Rather than computing the expected length via digest_length, if you supply a > NULL buffer for the output (namely, sig->data), then what will happen is > sig->len will be updated with the appropriate length needed for the signature. > > This is because PK11_SignWithSymKey is a wrapper for C_Sign, and C_Sign (and > C_SignFinal) uses the convention described in Section 11.2 for producing output. > > This will be the most flexible solution for working with any form of symmetric > key, and avoids the whole hash concerns raised in the BUG above. Done. https://codereview.chromium.org/23569007/diff/18001/content/renderer/webcrypt... File content/renderer/webcrypto_impl_openssl.cc (right): https://codereview.chromium.org/23569007/diff/18001/content/renderer/webcrypt... content/renderer/webcrypto_impl_openssl.cc:36: size_t data_size, On 2013/09/09 23:00:48, eroman wrote: > ditto here if you make the change to "unsigned int" Done.
https://codereview.chromium.org/23569007/diff/29001/content/renderer/webcrypt... File content/renderer/webcrypto_impl_nss.cc (right): https://codereview.chromium.org/23569007/diff/29001/content/renderer/webcrypt... content/renderer/webcrypto_impl_nss.cc:261: BUG: You need to update the length of |result| to be the returned length. This is more of a general sanity thing - in the case of HMAC, it will always be the length, but as you move to add stuff like AES-CBC etc, you may have variable block sizes.
https://codereview.chromium.org/23569007/diff/29001/content/renderer/webcrypt... File content/renderer/webcrypto_impl_nss.cc (right): https://codereview.chromium.org/23569007/diff/29001/content/renderer/webcrypt... content/renderer/webcrypto_impl_nss.cc:57: ((mask & WebKit::WebCryptoKeyUsageUnwrapKey) ? CKF_UNWRAP : 0); DESIGN: On second thought, we should be tracking these usages independent of the PKCS#11 usages. This is especially important for supporting the JWK and wrapped key use cases. Because PKCS#11 does not support unwrapping JWK, we'll need to decrypt (CKF_DECRYPT) and then import by hand in the UA. My suggestion is that the usages should be stored on the SymKeyHandle / KeyHandle, so that they can then be persisted (eg: as part of the Structured Clone, if/when its implemented) https://codereview.chromium.org/23569007/diff/29001/content/renderer/webcrypt... content/renderer/webcrypto_impl_nss.cc:79: CK_MECHANISM_TYPE WebCryptoAlgorithmToNSSMechanism( STYLE: I feel like this method should be renamed, since it's for mapping HMAC mechanisms. For example, you're not mapping to CKM_SHA_1, which is what this method name would suggest. https://codereview.chromium.org/23569007/diff/29001/content/renderer/webcrypt... content/renderer/webcrypto_impl_nss.cc:123: HASH_End(context, digest, &result_length, hash_result_length); BUG: 1) result_length should be unsigned int, not uint32. What about platforms where sizeof(unsigned int) != sizeof(uint32) BUG: 2) hash_result_length is a size_t, but HASH_End expects an unsigned int. https://codereview.chromium.org/23569007/diff/29001/content/renderer/webcrypt... content/renderer/webcrypto_impl_nss.cc:228: WebCryptoAlgorithmToNSSMechanism(params->hash())); Why is this DCHECK still needed? You're no longer constructing the output based on ->hash(), so this seems entirely unnecessary.
https://codereview.chromium.org/23569007/diff/29001/content/renderer/webcrypt... File content/renderer/webcrypto_impl_nss.cc (right): https://codereview.chromium.org/23569007/diff/29001/content/renderer/webcrypt... content/renderer/webcrypto_impl_nss.cc:57: ((mask & WebKit::WebCryptoKeyUsageUnwrapKey) ? CKF_UNWRAP : 0); On 2013/09/10 19:25:54, Ryan Sleevi wrote: > DESIGN: On second thought, we should be tracking these usages independent of the > PKCS#11 usages. > > This is especially important for supporting the JWK and wrapped key use cases. > Because PKCS#11 does not support unwrapping JWK, we'll need to decrypt > (CKF_DECRYPT) and then import by hand in the UA. > > My suggestion is that the usages should be stored on the SymKeyHandle / > KeyHandle, so that they can then be persisted (eg: as part of the Structured > Clone, if/when its implemented) Added a base class for the usages, as I believe this will be necessary for asymmetric keys as well. Also moved the mechanism and slot to this base class, as I believe those will also be needed. https://codereview.chromium.org/23569007/diff/29001/content/renderer/webcrypt... content/renderer/webcrypto_impl_nss.cc:79: CK_MECHANISM_TYPE WebCryptoAlgorithmToNSSMechanism( On 2013/09/10 19:25:54, Ryan Sleevi wrote: > STYLE: I feel like this method should be renamed, since it's for mapping HMAC > mechanisms. For example, you're not mapping to CKM_SHA_1, which is what this > method name would suggest. Renamed to WebCryptoAlgorithmToHMACMechanism. https://codereview.chromium.org/23569007/diff/29001/content/renderer/webcrypt... content/renderer/webcrypto_impl_nss.cc:123: HASH_End(context, digest, &result_length, hash_result_length); On 2013/09/10 19:25:54, Ryan Sleevi wrote: > BUG: 1) result_length should be unsigned int, not uint32. What about platforms > where sizeof(unsigned int) != sizeof(uint32) > BUG: 2) hash_result_length is a size_t, but HASH_End expects an unsigned int. These were both requests from James; I have made them back into unsigned ints, but I think there may be some debate about this. https://codereview.chromium.org/23569007/diff/29001/content/renderer/webcrypt... content/renderer/webcrypto_impl_nss.cc:228: WebCryptoAlgorithmToNSSMechanism(params->hash())); On 2013/09/10 19:25:54, Ryan Sleevi wrote: > Why is this DCHECK still needed? You're no longer constructing the output based > on ->hash(), so this seems entirely unnecessary. Sign is provided with the hash algorithm, so I figured it might be useful to validate that the algorithm matches. Does this make sense? Or should I just ignore that algorithm value and trust the Blink layer to catch an invalid use? https://codereview.chromium.org/23569007/diff/29001/content/renderer/webcrypt... content/renderer/webcrypto_impl_nss.cc:261: On 2013/09/10 01:22:36, Ryan Sleevi wrote: > BUG: You need to update the length of |result| to be the returned length. > > This is more of a general sanity thing - in the case of HMAC, it will always be > the length, but as you move to add stuff like AES-CBC etc, you may have variable > block sizes. Sorry, I'm not exactly sure what you're looking for here. Do you want me to make sure the resulting length matches the data length with a DCHECK?
https://codereview.chromium.org/23569007/diff/29001/content/renderer/webcrypt... File content/renderer/webcrypto_impl_nss.cc (right): https://codereview.chromium.org/23569007/diff/29001/content/renderer/webcrypt... content/renderer/webcrypto_impl_nss.cc:123: HASH_End(context, digest, &result_length, hash_result_length); On 2013/09/10 21:21:56, Bryan Eyler wrote: > On 2013/09/10 19:25:54, Ryan Sleevi wrote: > > BUG: 1) result_length should be unsigned int, not uint32. What about platforms > > where sizeof(unsigned int) != sizeof(uint32) > > BUG: 2) hash_result_length is a size_t, but HASH_End expects an unsigned int. > > These were both requests from James; I have made them back into unsigned ints, > but I think there may be some debate about this. This is absolutely a security bug when the sizeof's don't match, because then you can have stack smashing. Whenever talking to NSS, the API contract MUST be preserved. Any uses of size_t (which I think are wrong anyways, since it'll eventually need to cross IPC boundaries) should be done in the Chrome-controlled interfaces. https://codereview.chromium.org/23569007/diff/29001/content/renderer/webcrypt... content/renderer/webcrypto_impl_nss.cc:261: On 2013/09/10 21:21:56, Bryan Eyler wrote: > On 2013/09/10 01:22:36, Ryan Sleevi wrote: > > BUG: You need to update the length of |result| to be the returned length. > > > > This is more of a general sanity thing - in the case of HMAC, it will always > be > > the length, but as you move to add stuff like AES-CBC etc, you may have > variable > > block sizes. > > Sorry, I'm not exactly sure what you're looking for here. Do you want me to > make sure the resulting length matches the data length with a DCHECK? PK11_SignWithSymKey will update signature_item.len to match the length of the actual signature item, which may be less than the signature_item.len at creation. WebArrayBuffer's length then needs to be updated to reflect that returned size.
https://codereview.chromium.org/23569007/diff/29001/content/renderer/webcrypt... File content/renderer/webcrypto_impl_nss.cc (right): https://codereview.chromium.org/23569007/diff/29001/content/renderer/webcrypt... content/renderer/webcrypto_impl_nss.cc:261: On 2013/09/10 21:25:41, Ryan Sleevi wrote: > On 2013/09/10 21:21:56, Bryan Eyler wrote: > > On 2013/09/10 01:22:36, Ryan Sleevi wrote: > > > BUG: You need to update the length of |result| to be the returned length. > > > > > > This is more of a general sanity thing - in the case of HMAC, it will always > > be > > > the length, but as you move to add stuff like AES-CBC etc, you may have > > variable > > > block sizes. > > > > Sorry, I'm not exactly sure what you're looking for here. Do you want me to > > make sure the resulting length matches the data length with a DCHECK? > > PK11_SignWithSymKey will update signature_item.len to match the length of the > actual signature item, which may be less than the signature_item.len at > creation. > > WebArrayBuffer's length then needs to be updated to reflect that returned size. It doesn't look like there's a way to update the length of the WebArrayBuffer, but I can create a new one. This probably makes the most sense to have this logic when the length is expected to be different from the computed length, but I could add logic to check if the length differs and create a new WebArrayBuffer if it does?
https://codereview.chromium.org/23569007/diff/54001/content/renderer/webcrypt... File content/renderer/webcrypto_impl_nss.cc (right): https://codereview.chromium.org/23569007/diff/54001/content/renderer/webcrypt... content/renderer/webcrypto_impl_nss.cc:28: WebKit::WebCryptoKeyUsageMask usage() { return usage_; } Note that this is a duplication from. What about changing SignInternal() to take a "const WebCryptoKey&" instead so it can access this directly? https://codereview.chromium.org/23569007/diff/54001/content/renderer/webcrypt... content/renderer/webcrypto_impl_nss.cc:44: class WebCryptoSymKeyHandle : public WebCryptoKeyHandleBase { You can drop the "WebCrypto" prefix on these classes. They live in an anonymous namespace.
https://codereview.chromium.org/23569007/diff/54001/content/renderer/webcrypt... File content/renderer/webcrypto_impl_nss.cc (right): https://codereview.chromium.org/23569007/diff/54001/content/renderer/webcrypt... content/renderer/webcrypto_impl_nss.cc:28: WebKit::WebCryptoKeyUsageMask usage() { return usage_; } On 2013/09/11 19:42:07, eroman wrote: > Note that this is a duplication from. What about changing SignInternal() to take > a "const WebCryptoKey&" instead so it can access this directly? Done. https://codereview.chromium.org/23569007/diff/54001/content/renderer/webcrypt... content/renderer/webcrypto_impl_nss.cc:44: class WebCryptoSymKeyHandle : public WebCryptoKeyHandleBase { On 2013/09/11 19:42:07, eroman wrote: > You can drop the "WebCrypto" prefix on these classes. They live in an anonymous > namespace. Done.
https://codereview.chromium.org/23569007/diff/55006/content/renderer/webcrypt... File content/renderer/webcrypto_impl_nss.cc (right): https://codereview.chromium.org/23569007/diff/55006/content/renderer/webcrypt... content/renderer/webcrypto_impl_nss.cc:36: CK_MECHANISM_TYPE mechanism_; This is duplicating what the PK11SymKey already stores as well. Any reason for that? https://codereview.chromium.org/23569007/diff/55006/content/renderer/webcrypt... content/renderer/webcrypto_impl_nss.cc:54: crypto::ScopedPK11SymKey key_; The PK11SymKey holds onto its PK11SlotInfoStr. The PK11SlotInfoStr is a ref-counted object. So you shouldn't need to independently store the key. https://codereview.chromium.org/23569007/diff/55006/content/renderer/webcrypt... content/renderer/webcrypto_impl_nss.cc:67: ((mask & WebKit::WebCryptoKeyUsageUnwrapKey) ? CKF_UNWRAP : 0); design: Don't use this? https://codereview.chromium.org/23569007/diff/55006/content/renderer/webcrypt... content/renderer/webcrypto_impl_nss.cc:241: DCHECK_NE(0, key.usages() & WebKit::WebCryptoKeyUsageSign); What's this DCHECK guarding against? The WebKit API? https://codereview.chromium.org/23569007/diff/55006/content/renderer/webcrypt... File content/renderer/webcrypto_impl_unittest.cc (right): https://codereview.chromium.org/23569007/diff/55006/content/renderer/webcrypt... content/renderer/webcrypto_impl_unittest.cc:127: const char* msg; naming nit: msg -> message http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Genera... https://codereview.chromium.org/23569007/diff/55006/content/renderer/webcrypt... content/renderer/webcrypto_impl_unittest.cc:158: "3c8162589aafaee024fc9a5ca50dd2336fe3eb28", From a readability, it's very difficult to note the commas versus the extended continuations. At the cost of duplication, I would suggest adding explicit comments // key // message // mac https://codereview.chromium.org/23569007/diff/55006/content/renderer/webcrypt... content/renderer/webcrypto_impl_unittest.cc:197: for (size_t index = 0; index < ARRAYSIZE_UNSAFE(input_set); index++) { In general, I have a preference for seeing parameterized tests, since they allow you to see more information about exactly what test is failing and (with the use of a PrintTo overload) provide pretty-printing of outputs. That said, if a loop is appropriate here long term, please make sure to add a SCOPED_TRACE() to the iteration to make sure that the failure reasons are clear - otherwise, it will be a pain to debug. See https://code.google.com/p/googletest/wiki/V1_6_AdvancedGuide#Using_Assertions... https://codereview.chromium.org/23569007/diff/55006/content/renderer/webcrypt... content/renderer/webcrypto_impl_unittest.cc:238: std::vector<uint8> msg_raw; naming nit: msg -> message
https://codereview.chromium.org/23569007/diff/55006/content/renderer/webcrypt... File content/renderer/webcrypto_impl_nss.cc (right): https://codereview.chromium.org/23569007/diff/55006/content/renderer/webcrypt... content/renderer/webcrypto_impl_nss.cc:54: crypto::ScopedPK11SymKey key_; On 2013/09/12 22:01:56, Ryan Sleevi wrote: > The PK11SymKey holds onto its PK11SlotInfoStr. The PK11SlotInfoStr is a > ref-counted object. So you shouldn't need to independently store the key. s/key/slot https://codereview.chromium.org/23569007/diff/55006/content/renderer/webcrypt... content/renderer/webcrypto_impl_nss.cc:67: ((mask & WebKit::WebCryptoKeyUsageUnwrapKey) ? CKF_UNWRAP : 0); On 2013/09/12 22:01:56, Ryan Sleevi wrote: > design: Don't use this? eric pointed out some confusion: We should not be tracking CKF flags on the NSS side - or at least, generating the keys with all flags. In the end, it won't matter - the Blink side is responsible for enforcing usages. The example of why we shouldn't be doing this is a key being used to unwrap JWKs. NSS will never support JWK, so it will need to CKF_DECRYPT the JWK and then import that, but if this is used, it'll only have the CKF_UNWRAP. If it won't let you supply no usages, supply all usages (that are valid for that key type) - eg: no sign/verify for AES keys.
https://codereview.chromium.org/23569007/diff/55006/content/renderer/webcrypt... File content/renderer/webcrypto_impl_nss.cc (right): https://codereview.chromium.org/23569007/diff/55006/content/renderer/webcrypt... content/renderer/webcrypto_impl_nss.cc:36: CK_MECHANISM_TYPE mechanism_; On 2013/09/12 22:01:56, Ryan Sleevi wrote: > This is duplicating what the PK11SymKey already stores as well. > > Any reason for that? None, other than I didn't realize that could be retrieved from the key. Removed. https://codereview.chromium.org/23569007/diff/55006/content/renderer/webcrypt... content/renderer/webcrypto_impl_nss.cc:54: crypto::ScopedPK11SymKey key_; On 2013/09/12 22:15:02, Ryan Sleevi wrote: > On 2013/09/12 22:01:56, Ryan Sleevi wrote: > > The PK11SymKey holds onto its PK11SlotInfoStr. The PK11SlotInfoStr is a > > ref-counted object. So you shouldn't need to independently store the key. > > s/key/slot Removed tracking of slot. With the removal of tracking of slot and mechanism, there's nothing else to store in a KeyHandleBase class. I believe there should eventually be something, like a type indicating whether it's symmetric of asymmetric, but perhaps this should be left until it's needed and the base class removed? I removed it in the next patch set. https://codereview.chromium.org/23569007/diff/55006/content/renderer/webcrypt... content/renderer/webcrypto_impl_nss.cc:67: ((mask & WebKit::WebCryptoKeyUsageUnwrapKey) ? CKF_UNWRAP : 0); On 2013/09/12 22:15:02, Ryan Sleevi wrote: > On 2013/09/12 22:01:56, Ryan Sleevi wrote: > > design: Don't use this? > > eric pointed out some confusion: We should not be tracking CKF flags on the NSS > side - or at least, generating the keys with all flags. > > In the end, it won't matter - the Blink side is responsible for enforcing > usages. > > The example of why we shouldn't be doing this is a key being used to unwrap > JWKs. NSS will never support JWK, so it will need to CKF_DECRYPT the JWK and > then import that, but if this is used, it'll only have the CKF_UNWRAP. > > If it won't let you supply no usages, supply all usages (that are valid for that > key type) - eg: no sign/verify for AES keys. Done. https://codereview.chromium.org/23569007/diff/55006/content/renderer/webcrypt... content/renderer/webcrypto_impl_nss.cc:241: DCHECK_NE(0, key.usages() & WebKit::WebCryptoKeyUsageSign); On 2013/09/12 22:01:56, Ryan Sleevi wrote: > What's this DCHECK guarding against? > > The WebKit API? I guess - just a sanity check that the params in match up. Should I just remove it? Same as above - just validating that what's passed in is as expected. https://codereview.chromium.org/23569007/diff/55006/content/renderer/webcrypt... File content/renderer/webcrypto_impl_unittest.cc (right): https://codereview.chromium.org/23569007/diff/55006/content/renderer/webcrypt... content/renderer/webcrypto_impl_unittest.cc:127: const char* msg; On 2013/09/12 22:01:56, Ryan Sleevi wrote: > naming nit: msg -> message > > http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Genera... Done. https://codereview.chromium.org/23569007/diff/55006/content/renderer/webcrypt... content/renderer/webcrypto_impl_unittest.cc:158: "3c8162589aafaee024fc9a5ca50dd2336fe3eb28", On 2013/09/12 22:01:56, Ryan Sleevi wrote: > From a readability, it's very difficult to note the commas versus the extended > continuations. > > At the cost of duplication, I would suggest adding explicit comments > > // key > // message > // mac Done. https://codereview.chromium.org/23569007/diff/55006/content/renderer/webcrypt... content/renderer/webcrypto_impl_unittest.cc:197: for (size_t index = 0; index < ARRAYSIZE_UNSAFE(input_set); index++) { On 2013/09/12 22:01:56, Ryan Sleevi wrote: > In general, I have a preference for seeing parameterized tests, since they allow > you to see more information about exactly what test is failing and (with the use > of a PrintTo overload) provide pretty-printing of outputs. > > That said, if a loop is appropriate here long term, please make sure to add a > SCOPED_TRACE() to the iteration to make sure that the failure reasons are clear > - otherwise, it will be a pain to debug. > > See > https://code.google.com/p/googletest/wiki/V1_6_AdvancedGuide#Using_Assertions... Done, and added to digest tests as well. https://codereview.chromium.org/23569007/diff/55006/content/renderer/webcrypt... content/renderer/webcrypto_impl_unittest.cc:238: std::vector<uint8> msg_raw; On 2013/09/12 22:01:56, Ryan Sleevi wrote: > naming nit: msg -> message Done.
lgtm https://codereview.chromium.org/23569007/diff/72001/content/renderer/webcrypt... File content/renderer/webcrypto_impl_unittest.cc (right): https://codereview.chromium.org/23569007/diff/72001/content/renderer/webcrypt... content/renderer/webcrypto_impl_unittest.cc:97: SCOPED_TRACE(base::StringPrintf("id_index: %zu", id_index)); BUG: Don't use %zu. Use PRIuS http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=64-bit... base::StringPrintf("id_index: %" PRIuS, id_index) That said, given that SCOPED_TRACE includes both the File and the Line, and Message can be any streamable type, I'm inclined to suggest just using SCOPED_TRACE(id_index) and SCOPED_TRACE(set_index)
lgtm https://codereview.chromium.org/23569007/diff/72001/content/renderer/webcrypt... File content/renderer/webcrypto_impl_nss.cc (right): https://codereview.chromium.org/23569007/diff/72001/content/renderer/webcrypt... content/renderer/webcrypto_impl_nss.cc:25: void set_key(crypto::ScopedPK11SymKey key) { This could be an argument to the ctor instead https://codereview.chromium.org/23569007/diff/72001/content/renderer/webcrypt... content/renderer/webcrypto_impl_nss.cc:180: if (!sym_key->key()) { I think it would be more readable to do this test on pk11_sym_key above. And then construct the key afterwards if it passes. https://codereview.chromium.org/23569007/diff/72001/content/renderer/webcrypt... content/renderer/webcrypto_impl_nss.cc:206: const_cast<SymKeyHandle*>( no need for the const-cast; just don't add "const" in the re-interpret cast below. https://codereview.chromium.org/23569007/diff/72001/content/renderer/webcrypt... content/renderer/webcrypto_impl_nss.cc:231: DCHECK_LT(0u, signature_item.len); This check can never fail: signature_item.len is unsigned.
Adding James as reviewer for owners approval. I'm working on figuring out the Windows link issue, but in the meantime, if you could take a look at the latest CL, that would be appreciated. https://codereview.chromium.org/23569007/diff/72001/content/renderer/webcrypt... File content/renderer/webcrypto_impl_nss.cc (right): https://codereview.chromium.org/23569007/diff/72001/content/renderer/webcrypt... content/renderer/webcrypto_impl_nss.cc:25: void set_key(crypto::ScopedPK11SymKey key) { On 2013/09/13 00:48:38, eroman wrote: > This could be an argument to the ctor instead Done. https://codereview.chromium.org/23569007/diff/72001/content/renderer/webcrypt... content/renderer/webcrypto_impl_nss.cc:180: if (!sym_key->key()) { On 2013/09/13 00:48:38, eroman wrote: > I think it would be more readable to do this test on pk11_sym_key above. And > then construct the key afterwards if it passes. Done. https://codereview.chromium.org/23569007/diff/72001/content/renderer/webcrypt... content/renderer/webcrypto_impl_nss.cc:206: const_cast<SymKeyHandle*>( On 2013/09/13 00:48:38, eroman wrote: > no need for the const-cast; just don't add "const" in the re-interpret cast > below. Done. https://codereview.chromium.org/23569007/diff/72001/content/renderer/webcrypt... content/renderer/webcrypto_impl_nss.cc:231: DCHECK_LT(0u, signature_item.len); On 2013/09/13 00:48:38, eroman wrote: > This check can never fail: signature_item.len is unsigned. This is DCHECK_LT, not DCHECK_LE. I'm validating that the length should be greater than 0. Changed to NE to help with this understanding. https://codereview.chromium.org/23569007/diff/72001/content/renderer/webcrypt... File content/renderer/webcrypto_impl_unittest.cc (right): https://codereview.chromium.org/23569007/diff/72001/content/renderer/webcrypt... content/renderer/webcrypto_impl_unittest.cc:97: SCOPED_TRACE(base::StringPrintf("id_index: %zu", id_index)); On 2013/09/12 23:25:20, Ryan Sleevi wrote: > BUG: Don't use %zu. Use PRIuS > > http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=64-bit... > > base::StringPrintf("id_index: %" PRIuS, id_index) > > That said, given that SCOPED_TRACE includes both the File and the Line, and > Message can be any streamable type, I'm inclined to suggest just using > > SCOPED_TRACE(id_index) > > and SCOPED_TRACE(set_index) Done.
I'm just getting "error: old chunk mismatch" on each diff, mind re-uploading? :/
On 2013/09/13 21:37:13, jamesr wrote: > I'm just getting "error: old chunk mismatch" on each diff, mind re-uploading? :/ Re-uploaded; twice it seems.
lgtm
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/bryaneyler@google.com/23569007/94001
Retried try job too often on win_rel for step(s) browser_tests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/bryaneyler@google.com/23569007/94001
Message was sent while issue was closed.
Change committed as 223506
Message was sent while issue was closed.
Is it possible this caused the following error on Linux? FAILED: cd ../../chrome; flock -- /tmp/linux_package_lock bash ../out/Release/installer/debian/build.sh -o../out/Release -b ../out/Release -a x64 -c stable 18c18 < libnss3 (>= 3.12.6) --- > libnss3 (>= 3.14.3) ERROR: Shared library dependencies changed! If this is intentional, please update: chrome/installer/linux/debian/expected_deps See: http://build.chromium.org/p/chromium.chrome/builders/Google%20Chrome%20Linux%... I think I'll try reverting to see if it fixes it.
That was an unrelated, but 'safe' break (we already require a new NSS), due to how our official build scripts manage dependencies. Just need to update the expected deps as well while we're at it. On Sep 17, 2013 7:27 AM, <finnur@chromium.org> wrote: > Is it possible this caused the following error on Linux? > > FAILED: cd ../../chrome; flock -- /tmp/linux_package_lock bash > ../out/Release/installer/**debian/build.sh -o../out/Release -b > ../out/Release -a > x64 -c stable > 18c18 > < libnss3 (>= 3.12.6) > --- > >> libnss3 (>= 3.14.3) >> > > ERROR: Shared library dependencies changed! > If this is intentional, please update: > chrome/installer/linux/debian/**expected_deps > > > See: > http://build.chromium.org/p/**chromium.chrome/builders/** > Google%20Chrome%20Linux%20x64<http://build.chromium.org/p/chromium.chrome/builders/Google%20Chrome%20Linux%20x64> > > I think I'll try reverting to see if it fixes it. > > https://chromiumcodereview.**appspot.com/23569007/<https://chromiumcodereview... > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
No problem. Feel free to revert my revert and add the dependency as a followup. On Tue, Sep 17, 2013 at 2:40 PM, Ryan Sleevi <rsleevi@chromium.org> wrote: > That was an unrelated, but 'safe' break (we already require a new NSS), > due to how our official build scripts manage dependencies. > > Just need to update the expected deps as well while we're at it. > On Sep 17, 2013 7:27 AM, <finnur@chromium.org> wrote: > >> Is it possible this caused the following error on Linux? >> >> FAILED: cd ../../chrome; flock -- /tmp/linux_package_lock bash >> ../out/Release/installer/**debian/build.sh -o../out/Release -b >> ../out/Release -a >> x64 -c stable >> 18c18 >> < libnss3 (>= 3.12.6) >> --- >> >>> libnss3 (>= 3.14.3) >>> >> >> ERROR: Shared library dependencies changed! >> If this is intentional, please update: >> chrome/installer/linux/debian/**expected_deps >> >> >> See: >> http://build.chromium.org/p/**chromium.chrome/builders/** >> Google%20Chrome%20Linux%20x64<http://build.chromium.org/p/chromium.chrome/builders/Google%20Chrome%20Linux%20x64> >> >> I think I'll try reverting to see if it fixes it. >> >> https://chromiumcodereview.**appspot.com/23569007/<https://chromiumcodereview... >> > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Message was sent while issue was closed.
On 2013/09/17 14:45:54, Finnur wrote: > No problem. Feel free to revert my revert and add the dependency as a > followup. FWIW, probably better to add the dependency expectation to the same CL, so there won't be an avoidable build break (which has already pushed LKGR way behind). If you were thinking that it needed to be done separately because one part is in chrome/ and one part is chrome-internal/, the packaging stuff all moved to chrome/ a while ago.
Message was sent while issue was closed.
Fine with me either way. I was just trying to save you some work. This sounded like an easy change and if I recall this failure didn't even cause the tree to close.
Message was sent while issue was closed.
Hi Michael and Pawel, Would you please take a look at the external deps change for this CL? Thanks
FYI, re-opened as the previous CL was reverted due to dependency error.
lgtm
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/bryaneyler@google.com/23569007/54002
Message was sent while issue was closed.
Change committed as 223813
Message was sent while issue was closed.
https://chromiumcodereview.appspot.com/23569007/diff/54002/chrome/installer/l... File chrome/installer/linux/debian/expected_deps (right): https://chromiumcodereview.appspot.com/23569007/diff/54002/chrome/installer/l... chrome/installer/linux/debian/expected_deps:18: libnss3 (>= 3.14.3) Could you also remove "libnss3 (>= 3.14.3)" from debian/build.sh now that we have this? |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
