[WebCrypto] Crypto tests updated to use KeyUsage

[WebCrypto] Crypto tests updated to use KeyUsage

As per update in the WebCrypto, keyUsage cannot be empty. Updates tests
to include appropriate keyUsage.

This is needed to avoid SyntaxError being generated and being spec
complaint.

R=eroman
BUG=425646
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=187487

[Habib Virji, 2014-12-02 14:55:47]

This is needed as KeyUsage cannot be empty.

[eroman, 2014-12-02 15:10:52]

The test changes look_good.

Please additionally add some test coverage for the case of de-serializing a key
which contain empty usages.

Upon completion of the bug it will no longer be possible to import a key with
empty usages, however existing keys with empty usages may have already been
serialized into persistent storage (IndexedDB), and I would like verification
that they can still be imported.

Such test can be written using: internals.deserializeBuffer() to avoid needing
to import the key first.

I will let you decide what the best way to introduce such coverage is -- perhaps
you can add a new test file which does this for a couple keys (AES-CBC and HMAC
for instance) and ensures that the de-serialized key is valid.

[Habib Virji, 2014-12-05 14:32:03]

Thanks eroman, uploaded new tests which stores key in indexdb with empty
keyusage and fetches from the db.

[eroman, 2014-12-05 17:16:00]

https://codereview.chromium.org/770363002/diff/20001/LayoutTests/crypto/expor...
File LayoutTests/crypto/exportKey-deserializedKey.html (right):

https://codereview.chromium.org/770363002/diff/20001/LayoutTests/crypto/expor...
LayoutTests/crypto/exportKey-deserializedKey.html:25: return
internals.deserializeBuffer(internals.serializeObject(keyDesc));
This isn't what I had in mind.

This test is serializing and de-serializing a javascript dictionary.

Whereas what is relevant to be testing here is de-serializing a CryptoKey.

Take a look at clone-aesKey.html and its calls to logSerializedKey().
Essentially what you want to do is run that test and log the serialized format
for a key imported with no usages.

Then copy that into the other test and pass it to deserializeBuffer(), then
verify that a CryptoKey was inflated which has no usages.

There i no need for indexeddb usage in the test since deserializeBuffer() can do
the work directly.

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

On 2014/12/05 at 17:16:00, eroman wrote:

Did not fathom correctly. 

So the steps are basically.
1. serialized key via clone-aesKey.html. I receive something of this form for
AES-CBS with keyUsage empty:
3f004b010120002000112233445546778899aabbccddeeff000102030405060708090a0b0c0d0e0f
2. use deserializeBuffer on the received serializedKey. Requires using
internals.deserializeBuffer(makeBuffer(serializedKey)) // make buffer is needed
as deserializeBuffer expects in arrayBuffer format.
3. Then result of the deserializeBuffer to be used in exportKey(or importKey)
and check the contents of the key.

Is these steps correct for the suggested tests.

[eroman, 2014-12-05 18:57:23]

Right. Once you have de-serialized to a key, the important verification is to
test the key.usages.

You can exportKey() too but that is less critical.

The important thing are that (1) de-serialization succeeds (2) the key.usages
reflects that it has empty usages.

Does that make sense?

[Habib Virji, 2014-12-08 18:46:11]

On 2014/12/05 18:57:23, eroman wrote:
> Right. Once you have de-serialized to a key, the important verification is to
> test the key.usages.
> 
> You can exportKey() too but that is less critical.
> 
> The important thing are that (1) de-serialization succeeds (2) the key.usages
> reflects that it has empty usages.
> 
> Does that make sense?

An update, still on it. 

It requires input data to be of a certain format for deserializeBuffer as in
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit....

Only issue is how to translate logSerializedKey output to relevant format.

[eroman, 2014-12-08 18:48:44]

The tests strip out the version number embedded in the serialized data. Your
test can probably just use the current hardcoded version number, or add it
through a helper function.

[eroman, 2014-12-09 20:54:47]

Any update on this? These test changes need to be committed before the
Chromium-side can land.

[Habib Virji, 2014-12-15 18:27:39]

Sorry for late response.

I am trying out following:
1. I have a key similar to value returned by logSerializeKey. (including version
number)
2. I use hexStringToUintArray, but it crashes content_shell.

crypto.subtle.importKey('raw',
hexStringToUint8Array("30112233445566778899aabbccddeeff"), {name: "AES-CBC"},
false, []).then(function(importedKey) {
   
console.log(JSON.stringify(internals.deserializeBuffer(internals.serializeObject(importedKey))));
--> This works fine.
    var imported = bytesToHexString(internals.serializeObject(importedKey)); -->
"ff073f004b010110001030112233445566778899aabbccddeeff"
   
console.log(JSON.stringify(internals.deserializeBuffer(hexStringToUint8Array(imported))));
    finishJSTest();
});

[eroman, 2014-12-15 21:38:14]

> crypto.subtle.importKey('raw',
hexStringToUint8Array("30112233445566778899aabbccddeeff"), {name: "AES-CBC"},
false, []).then(function(importedKey) {

Above line looks fine.

console.log(JSON.stringify(internals.deserializeBuffer(internals.serializeObject(importedKey))));
--> This works fine.

I don't understand the intent of the above line. serializeObject() should
convert it to a series of bytes, and then that is what you would want to log.
What is the purpose of calling deserializeBuffer(), won't that inflat it back to
a CryptoKey and be a no-op?

    var imported = bytesToHexString(internals.serializeObject(importedKey)); -->
"ff073f004b010110001030112233445566778899aabbccddeeff"

console.log(JSON.stringify(internals.deserializeBuffer(hexStringToUint8Array(imported))));
    finishJSTest();
});

deserializeBuffer expects a DOMArrayBuffer, however here you are passing a
DOMArrayBufferView. I suspect that is your problem.

Try passing the the .buffer of that arraybufferview.

[Habib Virji, 2014-12-16 10:34:55]

Thanks, it was error because of not passing DOMArrayBufferView. Uploaded new
tests.

[eroman, 2014-12-16 20:23:53]

https://codereview.chromium.org/770363002/diff/60001/LayoutTests/crypto/impor...
File LayoutTests/crypto/importKey-deserialized.html (right):

https://codereview.chromium.org/770363002/diff/60001/LayoutTests/crypto/impor...
LayoutTests/crypto/importKey-deserialized.html:10: description("Check already
imported key, with empty usage can be deserialized.");
"Check that an already imported key with empty usages can be deserialized."

Also please provide some additional commentary explaining why this is relevant.

You can take a look at this test as an example:

https://chromium.googlesource.com/chromium/blink/+/master/LayoutTests/crypto/...

(Years from now it will not be obvious why this use case needs to be supported).

https://codereview.chromium.org/770363002/diff/60001/LayoutTests/crypto/impor...
LayoutTests/crypto/importKey-deserialized.html:11: jsTestIsAsync = true;
This test can be synchronous. Take a look at clone-hmacKey-empty.html as an
example

https://codereview.chromium.org/770363002/diff/60001/LayoutTests/crypto/impor...
LayoutTests/crypto/importKey-deserialized.html:15:
shouldBe("deserializedBuffer.usages", "[]");
Please add some other assertions for the key type, algorithm name,
extractability etc.

(The interesting point is to see that it is a secret or private key, with empty
usages).

https://codereview.chromium.org/770363002/diff/60001/LayoutTests/crypto/impor...
LayoutTests/crypto/importKey-deserialized.html:17: finishJSTest();
If the test is made synchronous this is not needed

[Habib Virji, 2014-12-17 11:54:36]

Updated the tests with comments and extra checks. 

Thanks.

https://codereview.chromium.org/770363002/diff/60001/LayoutTests/crypto/impor...
File LayoutTests/crypto/importKey-deserialized.html (right):

https://codereview.chromium.org/770363002/diff/60001/LayoutTests/crypto/impor...
LayoutTests/crypto/importKey-deserialized.html:10: description("Check already
imported key, with empty usage can be deserialized.");
On 2014/12/16 20:23:53, eroman wrote:
> "Check that an already imported key with empty usages can be deserialized."
> 
> Also please provide some additional commentary explaining why this is
relevant.
> 
> You can take a look at this test as an example:
> 
>
https://chromium.googlesource.com/chromium/blink/+/master/LayoutTests/crypto/...
> 
> (Years from now it will not be obvious why this use case needs to be
supported).

Done.

https://codereview.chromium.org/770363002/diff/60001/LayoutTests/crypto/impor...
LayoutTests/crypto/importKey-deserialized.html:11: jsTestIsAsync = true;
On 2014/12/16 20:23:53, eroman wrote:
> This test can be synchronous. Take a look at clone-hmacKey-empty.html as an
> example

Done.

https://codereview.chromium.org/770363002/diff/60001/LayoutTests/crypto/impor...
LayoutTests/crypto/importKey-deserialized.html:15:
shouldBe("deserializedBuffer.usages", "[]");
On 2014/12/16 20:23:53, eroman wrote:
> Please add some other assertions for the key type, algorithm name,
> extractability etc.
> 
> (The interesting point is to see that it is a secret or private key, with
empty
> usages).

Done.

https://codereview.chromium.org/770363002/diff/60001/LayoutTests/crypto/impor...
LayoutTests/crypto/importKey-deserialized.html:17: finishJSTest();
On 2014/12/16 20:23:53, eroman wrote:
> If the test is made synchronous this is not needed

Done.

[eroman, 2014-12-17 21:08:43]

not lgtm

https://codereview.chromium.org/770363002/diff/100001/LayoutTests/crypto/expo...
File LayoutTests/crypto/exportKey-deserializedKey-expected.txt (right):

https://codereview.chromium.org/770363002/diff/100001/LayoutTests/crypto/expo...
LayoutTests/crypto/exportKey-deserializedKey-expected.txt:1: Test storing a Raw
key in IndexedDB, and retrieving it which has empty key usage.
Where is this test? It is not attached to the changelist...

https://codereview.chromium.org/770363002/diff/100001/LayoutTests/crypto/impo...
File LayoutTests/crypto/importKey-deserialized.html (right):

https://codereview.chromium.org/770363002/diff/100001/LayoutTests/crypto/impo...
LayoutTests/crypto/importKey-deserialized.html:1: <!DOCTYPE html>
This name is still not good.
The test's name should indicate to readers what its purpose is.
The purpose of this test is de-serializing a secret key which contains empty
usages.

Rename this to for example:

deserialize-legacy-aes-key-empty-usages.html

[Habib Virji, 2014-12-18 09:36:39]

Renamed file and deleted one left out expected file.

https://codereview.chromium.org/770363002/diff/100001/LayoutTests/crypto/expo...
File LayoutTests/crypto/exportKey-deserializedKey-expected.txt (right):

https://codereview.chromium.org/770363002/diff/100001/LayoutTests/crypto/expo...
LayoutTests/crypto/exportKey-deserializedKey-expected.txt:1: Test storing a Raw
key in IndexedDB, and retrieving it which has empty key usage.
It is from first patch, somehow this file got included in the patch and did not
noticed.

https://codereview.chromium.org/770363002/diff/100001/LayoutTests/crypto/impo...
File LayoutTests/crypto/importKey-deserialized.html (right):

https://codereview.chromium.org/770363002/diff/100001/LayoutTests/crypto/impo...
LayoutTests/crypto/importKey-deserialized.html:1: <!DOCTYPE html>
On 2014/12/17 21:08:43, eroman wrote:
> This name is still not good.
> The test's name should indicate to readers what its purpose is.
> The purpose of this test is de-serializing a secret key which contains empty
> usages.
> 
> Rename this to for example:
> 
> deserialize-legacy-aes-key-empty-usages.html

Done.

[eroman, 2014-12-18 17:26:30]

The CQ bit was checked by eroman@chromium.org

[eroman, 2014-12-18 17:26:30]

lgtm

[commit-bot: I haz the power, 2014-12-18 17:27:18]

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

[commit-bot: I haz the power, 2014-12-18 19:47:17]

Committed patchset #7 (id:120001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=187487

[eroman, 2014-12-18 20:06:08]

It doesn't appear this change was complete, there are some places which weren't
updated:

crypto/clone-ecKey-private.html
crypto/clone-rsaHashedKey-private.html

Please test thoroughly!

If you are running on Linux, be sure to building using BoringSSL (OpenSSL) and
temporarily remove any exceptions from LayoutTests/TestExpectations.

For more details on how to do that see the developer section at:

http://www.chromium.org/blink/webcrypto

[Habib Virji, 2014-12-18 21:32:29]

On 2014/12/18 20:06:08, eroman wrote:
> It doesn't appear this change was complete, there are some places which
weren't
> updated:
> 
> crypto/clone-ecKey-private.html
> crypto/clone-rsaHashedKey-private.html
> 
> Please test thoroughly!
> 
> If you are running on Linux, be sure to building using BoringSSL (OpenSSL) and
> temporarily remove any exceptions from LayoutTests/TestExpectations.
> 
> For more details on how to do that see the developer section at:
> 
> http://www.chromium.org/blink/webcrypto
I did go through your docs instruction to enable boringSSL and was able to find
issues in unit tests. 

Is it okay to update for these two tests in other CL?

[eroman, 2014-12-18 21:39:18]

Yes, please add the other ones in a new changelist
(This one is already committed)

[Habib Virji, 2014-12-18 21:40:39]

On 2014/12/18 21:39:18, eroman wrote:
> Yes, please add the other ones in a new changelist
> (This one is already committed)

Making a b

[Habib Virji, 2014-12-18 21:40:39]

On 2014/12/18 21:39:18, eroman wrote:
> Yes, please add the other ones in a new changelist
> (This one is already committed)

Making a b

[Habib Virji, 2014-12-18 21:40:40]

On 2014/12/18 21:39:18, eroman wrote:
> Yes, please add the other ones in a new changelist
> (This one is already committed)

Making a b

[Habib Virji, 2014-12-18 21:41:00]

On 2014/12/18 21:40:40, Habib Virji wrote:
> On 2014/12/18 21:39:18, eroman wrote:
> > Yes, please add the other ones in a new changelist
> > (This one is already committed)
> 
Making a build with boring SSL and will send CL soon.

[Habib Virji, 2014-12-18 21:50:10]

Found issue it was due to these tests are present in TestExpectations  and both
these tests did not appear in failed tests.