Check that usage isn't empty when generateKey() is called

Check that usage isn't empty when generateKey() is called

BUG=425645
Committed: https://crrev.com/f723e1609b239219b3fb88e8db06562ad56a8af9
Cr-Commit-Position: refs/heads/master@{#305350}

[nharper, 2014-11-20 21:43:48]

nharper@chromium.org changed reviewers:
+ eroman@chromium.org, rsleevi@chromium.org

[eroman, 2014-11-20 23:40:10]

This change is off to a good start!

Have you also run the Blink LayoutTests? (I suspect they may also have
dependencies on empty usages for generation).

To run them, build the target in debug mode "blink_tests"

And then:

$ cd third_party/WebKit
$ ./Tools/Scripts/run-webkit-tests --debug crypto


... Also because of the Linux NSS horidness, if you are testing on that platform
you should also temporarily edit LayoutTests/TestExpectations and delete all of
the lines containing 'crypto/' so it doesn't mask any test failures.

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

https://codereview.chromium.org/745443002/diff/20001/content/child/webcrypto/...
content/child/webcrypto/nss/rsa_key_nss.cc:523: const
blink::WebCryptoKeyUsageMask private_usages =
Move the check up to here.

https://codereview.chromium.org/745443002/diff/20001/content/child/webcrypto/...
content/child/webcrypto/nss/rsa_key_nss.cc:605: if
(result->private_key().usages() == 0) {
Move the failure to before the key has been generated.

This is a bit different with respect to which errors will be given when there
are multiple problems, but should be fine.

(Alternately if we were OK doing the test after key generation, the logic could
be moved to the global GenerateKey() function so it doesn't need to be
duplicated into each algorithm implementation. However early failure is more
useful).

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

https://codereview.chromium.org/745443002/diff/20001/content/child/webcrypto/...
content/child/webcrypto/nss/sym_key_nss.cc:46: // TODO(nharper): mask usages
with allowed usages
Testing the allowed usages is the responsibility of the caller of
GenerateSecretKeyNss, so I don't think this TODO is necessary.

https://codereview.chromium.org/745443002/diff/20001/content/child/webcrypto/...
content/child/webcrypto/nss/sym_key_nss.cc:47: if (usages == 0) {
The style being used in these files is that single line if statements omit
braces. Please match it for new code.

https://codereview.chromium.org/745443002/diff/20001/content/child/webcrypto/...
content/child/webcrypto/nss/sym_key_nss.cc:48: return
Status::ErrorCreateKeyBadUsages();
Introduce a new error message so this is easier to understand for web
developers.

Perhaps:
 Status::ErrorGenerateKeyEmptyUsages()

https://codereview.chromium.org/745443002/diff/20001/content/child/webcrypto/...
File content/child/webcrypto/openssl/sym_key_openssl.cc (right):

https://codereview.chromium.org/745443002/diff/20001/content/child/webcrypto/...
content/child/webcrypto/openssl/sym_key_openssl.cc:35: if (usages == 0) {
Move this higher up -- should fail fast before doing the work of key generation.

https://codereview.chromium.org/745443002/diff/20001/content/child/webcrypto/...
File content/child/webcrypto/openssl/util_openssl.cc (right):

https://codereview.chromium.org/745443002/diff/20001/content/child/webcrypto/...
content/child/webcrypto/openssl/util_openssl.cc:183: if (key->usages() == 0) {
This test is not possible, as it will prevent import of keys with empty usages.
There is a separate bug to handle import (which is more involved because of
javascript's structured cloning).

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

https://codereview.chromium.org/745443002/diff/20001/content/child/webcrypto/...
content/child/webcrypto/test/rsa_ssa_unittest.cc:1081: if (key_format ==
blink::WebCryptoKeyFormatSpki) {
Omit the curlies.

[nharper, 2014-11-21 22:12:01]

I hadn't run the layout tests - the fix for them is in another cl. Once this
lands (which will be after the blink one lands), I can change the layout test to
check for the correct error when trying to generate a key with empty usage.

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

https://codereview.chromium.org/745443002/diff/20001/content/child/webcrypto/...
content/child/webcrypto/nss/rsa_key_nss.cc:523: const
blink::WebCryptoKeyUsageMask private_usages =
On 2014/11/20 23:40:09, eroman wrote:
> Move the check up to here.

Done.

https://codereview.chromium.org/745443002/diff/20001/content/child/webcrypto/...
content/child/webcrypto/nss/rsa_key_nss.cc:605: if
(result->private_key().usages() == 0) {
On 2014/11/20 23:40:09, eroman wrote:
> Move the failure to before the key has been generated.
> 
> This is a bit different with respect to which errors will be given when there
> are multiple problems, but should be fine.
> 
> (Alternately if we were OK doing the test after key generation, the logic
could
> be moved to the global GenerateKey() function so it doesn't need to be
> duplicated into each algorithm implementation. However early failure is more
> useful).

Done.

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

https://codereview.chromium.org/745443002/diff/20001/content/child/webcrypto/...
content/child/webcrypto/nss/sym_key_nss.cc:46: // TODO(nharper): mask usages
with allowed usages
On 2014/11/20 23:40:10, eroman wrote:
> Testing the allowed usages is the responsibility of the caller of
> GenerateSecretKeyNss, so I don't think this TODO is necessary.

Done.

https://codereview.chromium.org/745443002/diff/20001/content/child/webcrypto/...
content/child/webcrypto/nss/sym_key_nss.cc:47: if (usages == 0) {
On 2014/11/20 23:40:09, eroman wrote:
> The style being used in these files is that single line if statements omit
> braces. Please match it for new code.

Done.

https://codereview.chromium.org/745443002/diff/20001/content/child/webcrypto/...
content/child/webcrypto/nss/sym_key_nss.cc:48: return
Status::ErrorCreateKeyBadUsages();
On 2014/11/20 23:40:10, eroman wrote:
> Introduce a new error message so this is easier to understand for web
> developers.
> 
> Perhaps:
>  Status::ErrorGenerateKeyEmptyUsages()

Done.

https://codereview.chromium.org/745443002/diff/20001/content/child/webcrypto/...
File content/child/webcrypto/openssl/sym_key_openssl.cc (right):

https://codereview.chromium.org/745443002/diff/20001/content/child/webcrypto/...
content/child/webcrypto/openssl/sym_key_openssl.cc:35: if (usages == 0) {
On 2014/11/20 23:40:10, eroman wrote:
> Move this higher up -- should fail fast before doing the work of key
generation.

Done.

https://codereview.chromium.org/745443002/diff/20001/content/child/webcrypto/...
File content/child/webcrypto/openssl/util_openssl.cc (right):

https://codereview.chromium.org/745443002/diff/20001/content/child/webcrypto/...
content/child/webcrypto/openssl/util_openssl.cc:183: if (key->usages() == 0) {
On 2014/11/20 23:40:10, eroman wrote:
> This test is not possible, as it will prevent import of keys with empty
usages.
> There is a separate bug to handle import (which is more involved because of
> javascript's structured cloning).

Is it an issue that this affects behavior of key import as well? My
understanding of the spec is that importKey also must throw a SyntaxError if the
usages of the secret or private key is empty.

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

https://codereview.chromium.org/745443002/diff/20001/content/child/webcrypto/...
content/child/webcrypto/test/rsa_ssa_unittest.cc:1081: if (key_format ==
blink::WebCryptoKeyFormatSpki) {
On 2014/11/20 23:40:10, eroman wrote:
> Omit the curlies.

Done.

[eroman, 2014-11-21 22:55:47]

https://codereview.chromium.org/745443002/diff/20001/content/child/webcrypto/...
File content/child/webcrypto/openssl/util_openssl.cc (right):

https://codereview.chromium.org/745443002/diff/20001/content/child/webcrypto/...
content/child/webcrypto/openssl/util_openssl.cc:183: if (key->usages() == 0) {
On 2014/11/21 22:12:01, nharper wrote:
> On 2014/11/20 23:40:10, eroman wrote:
> > This test is not possible, as it will prevent import of keys with empty
> usages.
> > There is a separate bug to handle import (which is more involved because of
> > javascript's structured cloning).
> 
> Is it an issue that this affects behavior of key import as well? My
> understanding of the spec is that importKey also must throw a SyntaxError if
the
> usages of the secret or private key is empty.

Correct, importKey() must also throw a SyntaxError. The issue is that structured
cloning relies internally on the ImportKey*() methods, and there may exist
serialized CryptoKeys which have empty usages.

In more detail:

  (1) Structured cloning is a mechanism to serialize Javascript objects (in this
case  CryptoKey). This serialized form can be persisted into databases
(IndexedDB). Because CryptoKeys aren't vanilla Javascript objects (and in fact
they hide the key content from javascript) they implement their own structured
cloning/decloning. Search for "deserializeKeyForClone" for more information.

  (2) Because existing keys may have already been generated/imported with empty
usages, and because those keys may have already been persisted to IndexedDB, and
because we don't want to break deserializing such keys, we need to be prepared
to deal with it.

  ***(3)*** De-serializing structured-cloned keys is currently implemented using
ImportKey*(). You can see this by searching for "::DeserializeKeyForClone".
Hence some more surgery will be required before creating keys with empty usages
can be blocked.


The LayoutTests currently test serializing/deserializing keys with empty usages
(LayoutTests/crypto/clone-*.html). These will need to be refactored to just test
de-serialization of keys with empty usages since it will no longer be possible
to import them once the other bug is addressed.

Hope that helps to clarify!

[eroman, 2014-11-21 23:35:34]

Looksgood once you have adressed:
https://codereview.chromium.org/745443002/diff/40001/content/child/webcrypto/...

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

https://codereview.chromium.org/745443002/diff/40001/content/child/webcrypto/...
content/child/webcrypto/nss/sym_key_nss.cc:47: return
Status::ErrorCreateKeyEmptyUsages();
Please move this towards the top of the function (so it fails fast before more
work is done).

https://codereview.chromium.org/745443002/diff/40001/content/child/webcrypto/...
File content/child/webcrypto/openssl/sym_key_openssl.cc (right):

https://codereview.chromium.org/745443002/diff/40001/content/child/webcrypto/...
content/child/webcrypto/openssl/sym_key_openssl.cc:31: return
Status::ErrorCreateKeyEmptyUsages();
Move this higher up, so the failure happens before allocating the random_bytes
array.

https://codereview.chromium.org/745443002/diff/40001/content/child/webcrypto/...
File content/child/webcrypto/openssl/util_openssl.cc (right):

https://codereview.chromium.org/745443002/diff/40001/content/child/webcrypto/...
content/child/webcrypto/openssl/util_openssl.cc:183: if (key->usages() == 0)
Per our discussion this is best left to a follow-up patch since it has
implications on structured (de-)cloning.

Also we should fail faster (before doing the PKCS8 export)

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

https://codereview.chromium.org/745443002/diff/40001/content/child/webcrypto/...
content/child/webcrypto/status.h:217: // No usages were specified when creating
a key.
nit: "when creating a private or secret key" (since some public keys like ECDH
can have empty usages).

[nharper, 2014-11-22 00:26:45]

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

https://codereview.chromium.org/745443002/diff/40001/content/child/webcrypto/...
content/child/webcrypto/nss/sym_key_nss.cc:47: return
Status::ErrorCreateKeyEmptyUsages();
On 2014/11/21 23:35:34, eroman wrote:
> Please move this towards the top of the function (so it fails fast before more
> work is done).

Done.

https://codereview.chromium.org/745443002/diff/40001/content/child/webcrypto/...
File content/child/webcrypto/openssl/sym_key_openssl.cc (right):

https://codereview.chromium.org/745443002/diff/40001/content/child/webcrypto/...
content/child/webcrypto/openssl/sym_key_openssl.cc:31: return
Status::ErrorCreateKeyEmptyUsages();
On 2014/11/21 23:35:34, eroman wrote:
> Move this higher up, so the failure happens before allocating the random_bytes
> array.

Done.

https://codereview.chromium.org/745443002/diff/40001/content/child/webcrypto/...
File content/child/webcrypto/openssl/util_openssl.cc (right):

https://codereview.chromium.org/745443002/diff/40001/content/child/webcrypto/...
content/child/webcrypto/openssl/util_openssl.cc:183: if (key->usages() == 0)
On 2014/11/21 23:35:34, eroman wrote:
> Per our discussion this is best left to a follow-up patch since it has
> implications on structured (de-)cloning.
> 
> Also we should fail faster (before doing the PKCS8 export)

I moved this check up to the GenerateKey() functions that call
CreateWebCryptoPrivateKey(), and put it right after they define private_usages.

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

https://codereview.chromium.org/745443002/diff/40001/content/child/webcrypto/...
content/child/webcrypto/status.h:217: // No usages were specified when creating
a key.
On 2014/11/21 23:35:34, eroman wrote:
> nit: "when creating a private or secret key" (since some public keys like ECDH
> can have empty usages).

Done.

[eroman, 2014-11-22 00:29:52]

lgtm

[eroman, 2014-11-22 00:30:50]

(As you mentioned earlier, you should wait for the Blink side change to
LayoutTests to land + roll into Chromium before committing this changelist)

[nharper, 2014-11-22 02:08:49]

The CQ bit was checked by nharper@chromium.org

[commit-bot: I haz the power, 2014-11-22 02:09:08]

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

[commit-bot: I haz the power, 2014-11-22 03:03:57]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2014-11-22 03:04:50]

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

[pdr., 2014-11-22 21:33:46]

A revert of this CL (patchset #4 id:60001) has been created in
https://codereview.chromium.org/751883002/ by pdr@chromium.org.

The reason for reverting is: This patch caused the
crypto/rsa-oaep-generate-non-extractable-key.html layouttest to fail on
non-linux bots:
http://test-results.appspot.com/dashboards/flakiness_dashboard.html#tests=cry....