[WebCrypto] Throw syntaxError if keyUsage is empty in ImportKey

[WebCrypto] Throw syntaxError if keyUsage is empty in ImportKey

Throws SyntaxError when KeyUsage is empty.

Have fixes in unit test, which was using empty key usage.

BUG=425646
R=eroman
TEST=Added unit test in HMAC and AES-CBC.

Committed: https://crrev.com/f4aa92ee7b9c7fe3ba28d04cc46d352bc5b4f98b
Cr-Commit-Position: refs/heads/master@{#309175}

[Habib Virji, 2014-12-05 18:26:54]

Changes to throw syntax error if key usage is empty.

[eroman, 2014-12-05 20:45:51]

Please take a similar approach to http://crbug.com/425645 -- do the checks on a
per-algorithm basis (since some key types allow empty usages)

https://codereview.chromium.org/777403004/diff/1/content/child/webcrypto/algo...
File content/child/webcrypto/algorithm_dispatch.cc (right):

https://codereview.chromium.org/777403004/diff/1/content/child/webcrypto/algo...
content/child/webcrypto/algorithm_dispatch.cc:132: if (usages == 0)
This is not correct. 

Some key types are allowed to have empty usages. For instance ECDH public keys
(https://dvcs.w3.org/hg/webcrypto-api/raw-file/tip/spec/Overview.html#ecdh)

https://codereview.chromium.org/777403004/diff/1/content/child/webcrypto/test...
File content/child/webcrypto/test/rsa_ssa_unittest.cc (right):

https://codereview.chromium.org/777403004/diff/1/content/child/webcrypto/test...
content/child/webcrypto/test/rsa_ssa_unittest.cc:358: true,
blink::WebCryptoKeyUsageVerify, &public_key));
sync the code, this has already changed:

https://codereview.chromium.org/757873003/diff/20001/content/child/webcrypto/...

[Habib Virji, 2014-12-08 15:05:21]

Updated to check usages only for AES and HMAC. Did not find in spec mentioning
about RSA algorithms.

https://codereview.chromium.org/777403004/diff/1/content/child/webcrypto/algo...
File content/child/webcrypto/algorithm_dispatch.cc (right):

https://codereview.chromium.org/777403004/diff/1/content/child/webcrypto/algo...
content/child/webcrypto/algorithm_dispatch.cc:132: if (usages == 0)
On 2014/12/05 20:45:51, eroman wrote:
> This is not correct. 
> 
> Some key types are allowed to have empty usages. For instance ECDH public keys
> (https://dvcs.w3.org/hg/webcrypto-api/raw-file/tip/spec/Overview.html#ecdh)

Updated. Have found usages only in AES and HMAC. 
Done.

https://codereview.chromium.org/777403004/diff/1/content/child/webcrypto/test...
File content/child/webcrypto/test/rsa_ssa_unittest.cc (right):

https://codereview.chromium.org/777403004/diff/1/content/child/webcrypto/test...
content/child/webcrypto/test/rsa_ssa_unittest.cc:358: true,
blink::WebCryptoKeyUsageVerify, &public_key));
On 2014/12/05 20:45:51, eroman wrote:
> sync the code, this has already changed:
> 
>
https://codereview.chromium.org/757873003/diff/20001/content/child/webcrypto/...

Done.

[eroman, 2014-12-08 18:46:33]

The spec says that RSA and EC algorithms must throw if the usages are empty when
importing private keys. Please read the spec carefully.

See for instance step 9 of "14.3.9. The importKey method"

https://codereview.chromium.org/777403004/diff/20001/content/child/webcrypto/...
File content/child/webcrypto/nss/aes_key_nss.cc (right):

https://codereview.chromium.org/777403004/diff/20001/content/child/webcrypto/...
content/child/webcrypto/nss/aes_key_nss.cc:76: return
Status::ErrorImportKeyEmptyUsages();
Move this into VerifyKeyUsagesBeforeImportKey() similar to the other usage
checks. (Necessary for early failure when unwrapping keys).

In fact it would be best to just modify the CheckKeyCreationUsages() helper to
do the check. Add for instance a bool parameter indicating if empty usages are
allowed.

[Habib Virji, 2014-12-08 19:07:56]

I was looking in section 18 in specific of each algorithm, that's why probably
missed it. 

Regarding CheckKeyCreationUsages(), it is used for GenerateKey too. So will not
be possible to differentiate between importKey and generateKey.

[Habib Virji, 2014-12-08 19:09:28]

Sorry forget to ask question, so should make changes in
checkKeyCreationsUsages() or VerifyKeyUsagesBeforeImportKey?

[eroman, 2014-12-08 19:15:47]

GenerateKey() could be refactored to something like:

  blink::WebCryptoKeyUsageMask public_usages;
  blink::WebCryptoKeyUsageMask private_usages;

  status = GetGenerateAsymmetricKeyUsages(combined_usages, all_public_usages,
all_private_usages, &public_usages, &private_usages);
  if (status.IsError())
    return status;

And GetGenerateAsymmetricKeyUsages() could call CheckKeyCreateUsages() twice,
once for the public usages and once for the private usages.

[eroman, 2014-12-08 19:16:56]

I can propose that latter refactor separately.

[Habib Virji, 2014-12-09 10:21:53]

Changes as per your suggestion in CheckKeyCreationUsages. It has extra parameter
which is set in required ImportKey case only.

https://codereview.chromium.org/777403004/diff/40001/content/child/webcrypto/...
File content/child/webcrypto/webcrypto_util.cc (right):

https://codereview.chromium.org/777403004/diff/40001/content/child/webcrypto/...
content/child/webcrypto/webcrypto_util.cc:233: bool checkImportEmptyUsage) {
Have initialized this variable to false by default, to avoid change existing
GenerateKey not requiring changes and only in case of algorithm that require
importKey usage check.

[eroman, 2014-12-09 17:42:56]

not lgtm

https://codereview.chromium.org/777403004/diff/40001/content/child/webcrypto/...
File content/child/webcrypto/nss/aes_key_nss.cc (right):

https://codereview.chromium.org/777403004/diff/40001/content/child/webcrypto/...
content/child/webcrypto/nss/aes_key_nss.cc:62: bool checkEmptyUsage = true;
Incorrect naming style, please read the chromium style guide.
Also no need for a variable, just pass the boolean directly to the function.

https://codereview.chromium.org/777403004/diff/40001/content/child/webcrypto/...
File content/child/webcrypto/nss/rsa_key_nss.cc (right):

https://codereview.chromium.org/777403004/diff/40001/content/child/webcrypto/...
content/child/webcrypto/nss/rsa_key_nss.cc:595: checkEmptyKeyUsage);
This is incorrect.

I am losing confidence in your ability to write this change. Please test and
verify your changes before sending them for review.

https://codereview.chromium.org/777403004/diff/40001/content/child/webcrypto/...
content/child/webcrypto/nss/rsa_key_nss.cc:604: checkEmptyKeyUsages).IsSuccess()
||
Incorrect for the same reason as above.

https://codereview.chromium.org/777403004/diff/40001/content/child/webcrypto/...
File content/child/webcrypto/test/rsa_ssa_unittest.cc (right):

https://codereview.chromium.org/777403004/diff/40001/content/child/webcrypto/...
content/child/webcrypto/test/rsa_ssa_unittest.cc:828:
ASSERT_EQ(Status::ErrorImportKeyEmptyUsages(),
Incorrect, this should pass per the spec.

https://codereview.chromium.org/777403004/diff/40001/content/child/webcrypto/...
File content/child/webcrypto/webcrypto_util.h (right):

https://codereview.chromium.org/777403004/diff/40001/content/child/webcrypto/...
content/child/webcrypto/webcrypto_util.h:72: bool checkImportKeyEmptyUsage =
false);
The style forbids default parameters, and also this is the wrong variable naming
style.

[Habib Virji, 2014-12-09 19:31:51]

I have corrected issue as highlighted in previous review. Have given a last try,
please see if they are corrected.

[eroman, 2014-12-09 21:04:47]

Thanks this is better, we are almost there.

I hope to commit two refactors before you land this, to simplify the changes you
need to make:

https://codereview.chromium.org/786363002/
https://codereview.chromium.org/787123003/

https://codereview.chromium.org/777403004/diff/60001/content/child/webcrypto/...
File content/child/webcrypto/nss/aes_key_nss.cc (right):

https://codereview.chromium.org/777403004/diff/60001/content/child/webcrypto/...
content/child/webcrypto/nss/aes_key_nss.cc:45: Status status =
CheckKeyCreationUsages(all_key_usages_, usages, false);
This may as well just pass true, and then remove the check in
GenerateSecretKeyNss/GenerateSecretKeyOpenssl

https://codereview.chromium.org/777403004/diff/60001/content/child/webcrypto/...
File content/child/webcrypto/nss/hmac_nss.cc (right):

https://codereview.chromium.org/777403004/diff/60001/content/child/webcrypto/...
content/child/webcrypto/nss/hmac_nss.cc:62: Status status =
CheckKeyCreationUsages(kAllKeyUsages, usages, false);
Same here.

https://codereview.chromium.org/777403004/diff/60001/content/child/webcrypto/...
File content/child/webcrypto/nss/rsa_key_nss.cc (right):

https://codereview.chromium.org/777403004/diff/60001/content/child/webcrypto/...
content/child/webcrypto/nss/rsa_key_nss.cc:507: all_public_key_usages_ |
all_private_key_usages_, combined_usages, false);
I am proposing a refactor in:

https://codereview.chromium.org/786363002/diff/1/content/child/webcrypto/webc...

Which will simplify the number of updates you need to do for this (will just
have to update the one function rather than 3)

https://codereview.chromium.org/777403004/diff/60001/content/child/webcrypto/...
content/child/webcrypto/nss/rsa_key_nss.cc:601: all_private_key_usages_, usages,
true).IsSuccess() ||
I am proposing a refactor in https://codereview.chromium.org/787123003/ which
will reduce the number of places you need to update.

https://codereview.chromium.org/777403004/diff/60001/content/child/webcrypto/...
content/child/webcrypto/nss/rsa_key_nss.cc:768: usages, jwk.is_private_key ?
true : false);
This is already a bool no need for ternary operator.

https://codereview.chromium.org/777403004/diff/60001/content/child/webcrypto/...
content/child/webcrypto/nss/rsa_key_nss.cc:770: return
Status::ErrorCreateKeyBadUsages();
On a side note, I wander whey this isn't just "return status"

https://codereview.chromium.org/777403004/diff/60001/content/child/webcrypto/...
File content/child/webcrypto/openssl/aes_key_openssl.cc (right):

https://codereview.chromium.org/777403004/diff/60001/content/child/webcrypto/...
content/child/webcrypto/openssl/aes_key_openssl.cc:37: Status status =
CheckKeyCreationUsages(all_key_usages_, usages, false);
Same comment as before, let's do the empty usage check here so it doesn't need
to be duplicated later.

https://codereview.chromium.org/777403004/diff/60001/content/child/webcrypto/...
File content/child/webcrypto/openssl/ec_key_openssl.cc (right):

https://codereview.chromium.org/777403004/diff/60001/content/child/webcrypto/...
content/child/webcrypto/openssl/ec_key_openssl.cc:286: bool checkEmptyUsage =
true;
Same comment as earlier.

https://codereview.chromium.org/777403004/diff/60001/content/child/webcrypto/...
File content/child/webcrypto/openssl/hmac_openssl.cc (right):

https://codereview.chromium.org/777403004/diff/60001/content/child/webcrypto/...
content/child/webcrypto/openssl/hmac_openssl.cc:73: Status status =
CheckKeyCreationUsages(kAllKeyUsages, usages, false);
Same comment as earlier.

https://codereview.chromium.org/777403004/diff/60001/content/child/webcrypto/...
File content/child/webcrypto/status.h (right):

https://codereview.chromium.org/777403004/diff/60001/content/child/webcrypto/...
content/child/webcrypto/status.h:227: static Status ErrorImportKeyEmptyUsages();
Why not use ErrorCreateKeyEmptyUsages() which is already defined?

https://codereview.chromium.org/777403004/diff/60001/content/child/webcrypto/...
File content/child/webcrypto/test/rsa_ssa_unittest.cc (right):

https://codereview.chromium.org/777403004/diff/60001/content/child/webcrypto/...
content/child/webcrypto/test/rsa_ssa_unittest.cc:826: TEST(WebCryptoRsaSsaTest,
ImportKeyEmptyUsages) {
Could you also add the same sort of test to ecdh_unittest.cc.

https://codereview.chromium.org/777403004/diff/60001/content/child/webcrypto/...
content/child/webcrypto/test/rsa_ssa_unittest.cc:836: // Private key usage will
generate error.
Could you add two more tests for JWK:
  (1) Import a JWK public key with empty usages (OK)
  (2) Import a JWK private key with empty usages (fails)

https://codereview.chromium.org/777403004/diff/60001/content/child/webcrypto/...
File content/child/webcrypto/webcrypto_util.cc (right):

https://codereview.chromium.org/777403004/diff/60001/content/child/webcrypto/...
content/child/webcrypto/webcrypto_util.cc:233: bool check_usage) {
This is poorly named -- the entire purpose of this function is in fact to check
usages.

|allow_empty_usages| for example would be a better name.

https://codereview.chromium.org/777403004/diff/60001/content/child/webcrypto/...
content/child/webcrypto/webcrypto_util.cc:234: // In case import key calls this
function, it checks for empty usage
This comment is unnecessary (will apply to both importKey and generateKey)

[Habib Virji, 2014-12-15 18:48:56]

Sorry for delay in updating.

Updated with your suggestions.

https://codereview.chromium.org/777403004/diff/60001/content/child/webcrypto/...
File content/child/webcrypto/nss/aes_key_nss.cc (right):

https://codereview.chromium.org/777403004/diff/60001/content/child/webcrypto/...
content/child/webcrypto/nss/aes_key_nss.cc:45: Status status =
CheckKeyCreationUsages(all_key_usages_, usages, false);
On 2014/12/09 at 21:04:46, eroman wrote:
> This may as well just pass true, and then remove the check in
GenerateSecretKeyNss/GenerateSecretKeyOpenssl

Done.

https://codereview.chromium.org/777403004/diff/60001/content/child/webcrypto/...
File content/child/webcrypto/nss/hmac_nss.cc (right):

https://codereview.chromium.org/777403004/diff/60001/content/child/webcrypto/...
content/child/webcrypto/nss/hmac_nss.cc:62: Status status =
CheckKeyCreationUsages(kAllKeyUsages, usages, false);
On 2014/12/09 at 21:04:46, eroman wrote:
> Same here.

Done.

https://codereview.chromium.org/777403004/diff/60001/content/child/webcrypto/...
File content/child/webcrypto/nss/rsa_key_nss.cc (right):

https://codereview.chromium.org/777403004/diff/60001/content/child/webcrypto/...
content/child/webcrypto/nss/rsa_key_nss.cc:507: all_public_key_usages_ |
all_private_key_usages_, combined_usages, false);
On 2014/12/09 at 21:04:46, eroman wrote:
> I am proposing a refactor in:
> 
>
https://codereview.chromium.org/786363002/diff/1/content/child/webcrypto/webc...
> 
> Which will simplify the number of updates you need to do for this (will just
have to update the one function rather than 3)

Adapted to your changes.

https://codereview.chromium.org/777403004/diff/60001/content/child/webcrypto/...
content/child/webcrypto/nss/rsa_key_nss.cc:601: all_private_key_usages_, usages,
true).IsSuccess() ||
On 2014/12/09 at 21:04:46, eroman wrote:
> I am proposing a refactor in https://codereview.chromium.org/787123003/ which
will reduce the number of places you need to update.

Adapted to your changes.

https://codereview.chromium.org/777403004/diff/60001/content/child/webcrypto/...
content/child/webcrypto/nss/rsa_key_nss.cc:768: usages, jwk.is_private_key ?
true : false);
On 2014/12/09 at 21:04:46, eroman wrote:
> This is already a bool no need for ternary operator.

Corrected.

https://codereview.chromium.org/777403004/diff/60001/content/child/webcrypto/...
content/child/webcrypto/nss/rsa_key_nss.cc:770: return
Status::ErrorCreateKeyBadUsages();
On 2014/12/09 at 21:04:46, eroman wrote:
> On a side note, I wander whey this isn't just "return status"

Adapted to just return status.

https://codereview.chromium.org/777403004/diff/60001/content/child/webcrypto/...
File content/child/webcrypto/openssl/aes_key_openssl.cc (right):

https://codereview.chromium.org/777403004/diff/60001/content/child/webcrypto/...
content/child/webcrypto/openssl/aes_key_openssl.cc:37: Status status =
CheckKeyCreationUsages(all_key_usages_, usages, false);
On 2014/12/09 at 21:04:46, eroman wrote:
> Same comment as before, let's do the empty usage check here so it doesn't need
to be duplicated later.

Done.

https://codereview.chromium.org/777403004/diff/60001/content/child/webcrypto/...
File content/child/webcrypto/status.h (right):

https://codereview.chromium.org/777403004/diff/60001/content/child/webcrypto/...
content/child/webcrypto/status.h:227: static Status ErrorImportKeyEmptyUsages();
On 2014/12/09 at 21:04:46, eroman wrote:
> Why not use ErrorCreateKeyEmptyUsages() which is already defined?

Was keeping it different as error message, explicitly said 
"create key", i have modified now error message to key usage empty for create or
import key is not allowed.

https://codereview.chromium.org/777403004/diff/60001/content/child/webcrypto/...
File content/child/webcrypto/test/rsa_ssa_unittest.cc (right):

https://codereview.chromium.org/777403004/diff/60001/content/child/webcrypto/...
content/child/webcrypto/test/rsa_ssa_unittest.cc:826: TEST(WebCryptoRsaSsaTest,
ImportKeyEmptyUsages) {
On 2014/12/09 at 21:04:46, eroman wrote:
> Could you also add the same sort of test to ecdh_unittest.cc.

Added test.

https://codereview.chromium.org/777403004/diff/60001/content/child/webcrypto/...
content/child/webcrypto/test/rsa_ssa_unittest.cc:836: // Private key usage will
generate error.
On 2014/12/09 at 21:04:46, eroman wrote:
> Could you add two more tests for JWK:
>   (1) Import a JWK public key with empty usages (OK)
>   (2) Import a JWK private key with empty usages (fails)

Done. Have done export key and import key as jwk. Bit extra steps, but getting
jwk structure looked bit more complicated.

https://codereview.chromium.org/777403004/diff/60001/content/child/webcrypto/...
File content/child/webcrypto/webcrypto_util.cc (right):

https://codereview.chromium.org/777403004/diff/60001/content/child/webcrypto/...
content/child/webcrypto/webcrypto_util.cc:233: bool check_usage) {
On 2014/12/09 at 21:04:46, eroman wrote:
> This is poorly named -- the entire purpose of this function is in fact to
check usages.
> 
> |allow_empty_usages| for example would be a better name.

Done.

https://codereview.chromium.org/777403004/diff/60001/content/child/webcrypto/...
content/child/webcrypto/webcrypto_util.cc:234: // In case import key calls this
function, it checks for empty usage
On 2014/12/09 at 21:04:46, eroman wrote:
> This comment is unnecessary (will apply to both importKey and generateKey)

Removed

[eroman, 2014-12-16 01:26:46]

https://codereview.chromium.org/777403004/diff/80001/content/child/webcrypto/...
File content/child/webcrypto/status.cc (right):

https://codereview.chromium.org/777403004/diff/80001/content/child/webcrypto/...
content/child/webcrypto/status.cc:271: Status Status::ErrorKeyEmptyUsages() {
Several issues here:
  (1) The header was not updated to remove ErrorCreateKeyEmptyUsages(), however
it's definition was removed.

  (2) I don't see the need for changing the method name or error text. "Creating
a key" seems like a good general name for both generating and importing (both
are ways of creating a key).

Please leave this unchanged. If you want to propose different text for the error
do so in a separate changelist.

https://codereview.chromium.org/777403004/diff/80001/content/child/webcrypto/...
File content/child/webcrypto/webcrypto_util.cc (left):

https://codereview.chromium.org/777403004/diff/80001/content/child/webcrypto/...
content/child/webcrypto/webcrypto_util.cc:399: return
Status::ErrorCreateKeyEmptyUsages();
The older mechanism was clearer, and correct. Just call the combined
CheckKeyCreationUsages() above with a static value (false, although depends if
you change it to actually meaning allow_empty_usage)

And then keep this line.

https://codereview.chromium.org/777403004/diff/80001/content/child/webcrypto/...
File content/child/webcrypto/webcrypto_util.cc (right):

https://codereview.chromium.org/777403004/diff/80001/content/child/webcrypto/...
content/child/webcrypto/webcrypto_util.cc:276: if (allow_empty_usages &&
actual_usages == 0)
This naming is reversed. You are passing true for places which do NOT allow
empty usages

https://codereview.chromium.org/777403004/diff/80001/content/child/webcrypto/...
content/child/webcrypto/webcrypto_util.cc:396: bool allow_empty_usage =
I don't understand this, doesn't seem right to me.

It is also confusing because allow_empty_usage is in fact reversed inside of
CheckKeyCreationusages() per my other comment.

[Habib Virji, 2014-12-16 09:59:42]

Updated with correct value for allow_empty_usages.

https://codereview.chromium.org/777403004/diff/80001/content/child/webcrypto/...
File content/child/webcrypto/status.cc (right):

https://codereview.chromium.org/777403004/diff/80001/content/child/webcrypto/...
content/child/webcrypto/status.cc:271: Status Status::ErrorKeyEmptyUsages() {
I have updated header and removed new function previously added.

https://codereview.chromium.org/777403004/diff/80001/content/child/webcrypto/...
File content/child/webcrypto/webcrypto_util.cc (left):

https://codereview.chromium.org/777403004/diff/80001/content/child/webcrypto/...
content/child/webcrypto/webcrypto_util.cc:399: return
Status::ErrorCreateKeyEmptyUsages();
I have updated above, please suggest if it is okay.

https://codereview.chromium.org/777403004/diff/80001/content/child/webcrypto/...
File content/child/webcrypto/webcrypto_util.cc (right):

https://codereview.chromium.org/777403004/diff/80001/content/child/webcrypto/...
content/child/webcrypto/webcrypto_util.cc:276: if (allow_empty_usages &&
actual_usages == 0)
Corrected now, it has now allow_empty_usages as true for scenarios it should not
check and false when it should check. I was trying to do reverse but naming was
not appropriate. Hopefully this is corrected now.

https://codereview.chromium.org/777403004/diff/80001/content/child/webcrypto/...
content/child/webcrypto/webcrypto_util.cc:396: bool allow_empty_usage =
Yes, i was passing true for scenario where it should check. But naming in the
function was making it mean different.

[eroman, 2014-12-17 01:23:51]

https://codereview.chromium.org/777403004/diff/100001/content/child/webcrypto...
File content/child/webcrypto/openssl/ec_key_openssl.cc (right):

https://codereview.chromium.org/777403004/diff/100001/content/child/webcrypto...
content/child/webcrypto/openssl/ec_key_openssl.cc:377: usages, true);
I don't believe this is correct. Shouldn't it be !is_private_key ?

I see that you have added unit-tests for this scenario, do those tests pass? Is
it possible you are running in an environment without ECDH support and hence
those tests you added were not run?

https://codereview.chromium.org/777403004/diff/100001/content/child/webcrypto...
File content/child/webcrypto/status.h (right):

https://codereview.chromium.org/777403004/diff/100001/content/child/webcrypto...
content/child/webcrypto/status.h:228: // No usages were specified when
creating/importing a secret or private key.
creating/importing --> generating/importing

https://codereview.chromium.org/777403004/diff/100001/content/child/webcrypto...
File content/child/webcrypto/test/rsa_ssa_unittest.cc (right):

https://codereview.chromium.org/777403004/diff/100001/content/child/webcrypto...
content/child/webcrypto/test/rsa_ssa_unittest.cc:826: TEST(WebCryptoRsaSsaTest,
ImportKeyEmptyUsages) {
Add a test for

if (!SupportsRsaPrivateKeyImport())
  return;

https://codereview.chromium.org/777403004/diff/100001/content/child/webcrypto...
content/child/webcrypto/test/rsa_ssa_unittest.cc:829: blink::WebCryptoKey key;
It is unclear what this variable means. Can you instead re-use private_key
later?

https://codereview.chromium.org/777403004/diff/100001/content/child/webcrypto...
content/child/webcrypto/test/rsa_ssa_unittest.cc:832:
ASSERT_EQ(Status::Success(),
It would be good if each of these successful ImportKey() were followed by a
verification of the usages. Something like:

EXPECT_EQ(0, public_key.usages());

[Habib Virji, 2014-12-17 15:41:54]

Updated with suggested changes.

https://codereview.chromium.org/777403004/diff/100001/content/child/webcrypto...
File content/child/webcrypto/openssl/ec_key_openssl.cc (right):

https://codereview.chromium.org/777403004/diff/100001/content/child/webcrypto...
content/child/webcrypto/openssl/ec_key_openssl.cc:377: usages, true);
Sorry I missed this file to update. 

You were right regarding my environment. Since I am running on Linux it was not
running ecdh and thus this was not invisible. I have now checked by changing
with build using boringSSL, it does run fine.

https://codereview.chromium.org/777403004/diff/100001/content/child/webcrypto...
File content/child/webcrypto/status.h (right):

https://codereview.chromium.org/777403004/diff/100001/content/child/webcrypto...
content/child/webcrypto/status.h:228: // No usages were specified when
creating/importing a secret or private key.
On 2014/12/17 01:23:51, eroman wrote:
> creating/importing --> generating/importing

Done.

https://codereview.chromium.org/777403004/diff/100001/content/child/webcrypto...
File content/child/webcrypto/test/rsa_ssa_unittest.cc (right):

https://codereview.chromium.org/777403004/diff/100001/content/child/webcrypto...
content/child/webcrypto/test/rsa_ssa_unittest.cc:826: TEST(WebCryptoRsaSsaTest,
ImportKeyEmptyUsages) {
On 2014/12/17 01:23:51, eroman wrote:
> Add a test for
> 
> if (!SupportsRsaPrivateKeyImport())
>   return;

Done.

https://codereview.chromium.org/777403004/diff/100001/content/child/webcrypto...
content/child/webcrypto/test/rsa_ssa_unittest.cc:829: blink::WebCryptoKey key;
On 2014/12/17 01:23:51, eroman wrote:
> It is unclear what this variable means. Can you instead re-use private_key
> later?

Done.

https://codereview.chromium.org/777403004/diff/100001/content/child/webcrypto...
content/child/webcrypto/test/rsa_ssa_unittest.cc:832:
ASSERT_EQ(Status::Success(),
On 2014/12/17 01:23:51, eroman wrote:
> It would be good if each of these successful ImportKey() were followed by a
> verification of the usages. Something like:
> 
> EXPECT_EQ(0, public_key.usages());

Done.

[eroman, 2014-12-17 21:17:42]

https://codereview.chromium.org/777403004/diff/120001/content/test/data/webcr...
File content/test/data/webcrypto/ecdh.json (left):

https://codereview.chromium.org/777403004/diff/120001/content/test/data/webcr...
content/test/data/webcrypto/ecdh.json:350: // The public key parameter is in
fact a private key
This test should not be removed.

If it is failing with this change, you must instead change the test expectation.

You will probably need to modify the test harness and introduce a
"public_key_error" field.

Search for "private_key_error" to see what I mean.

Once this has been addressed, the rest of the change looks good.

[eroman, 2014-12-17 21:21:03]

https://codereview.chromium.org/777403004/diff/120001/content/test/data/webcr...
File content/test/data/webcrypto/ecdh.json (left):

https://codereview.chromium.org/777403004/diff/120001/content/test/data/webcr...
content/test/data/webcrypto/ecdh.json:350: // The public key parameter is in
fact a private key
On 2014/12/17 21:17:42, eroman wrote:
> This test should not be removed.
> 
> If it is failing with this change, you must instead change the test
expectation.
> 
> You will probably need to modify the test harness and introduce a
> "public_key_error" field.
> 
> Search for "private_key_error" to see what I mean.
> 
> Once this has been addressed, the rest of the change looks good.

Actually I am OK with removing this test, as we have an equivalent one  in:

https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...

LGTM then

[eroman, 2014-12-17 21:21:10]

The CQ bit was checked by eroman@chromium.org

[commit-bot: I haz the power, 2014-12-17 21:22:34]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/777403004/120001

[eroman, 2014-12-17 21:24:10]

The CQ bit was unchecked by eroman@chromium.org

[eroman, 2014-12-17 21:24:59]

(The LayoutTest changes need to land first, so cancelling commit queue)

[eroman, 2014-12-18 19:52:04]

The CQ bit was checked by eroman@chromium.org

[commit-bot: I haz the power, 2014-12-18 20:02:01]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/777403004/120001

[eroman, 2014-12-18 20:02:27]

The CQ bit was unchecked by eroman@chromium.org

[eroman, 2014-12-18 20:02:52]

Nope, the LayoutTests still don't pass with this change, aborting the commit.

[commit-bot: I haz the power, 2014-12-18 20:03:20]

The CQ bit was unchecked by commit-bot@chromium.org

[Habib Virji, 2014-12-19 10:03:45]

The CQ bit was checked by habib.virji@samsung.com

[commit-bot: I haz the power, 2014-12-19 10:04:10]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/777403004/120001

[commit-bot: I haz the power, 2014-12-19 10:05:05]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2014-12-19 10:05:48]

Patchset 7 (id:??) landed as
https://crrev.com/f4aa92ee7b9c7fe3ba28d04cc46d352bc5b4f98b
Cr-Commit-Position: refs/heads/master@{#309175}