[WebCrypto] Move cloneKey test to respective algorithm directory

[WebCrypto] Move cloneKey test to respective algorithm directory

Creates a cloneKey.js per algorithm classes (symmetric/RSA/EC), which is called by each algorithm falling in respective class.
- In symmetric-cloneKey.js, hashing and length are extra parameter which are present for HMAC and for HMAC/AES algorithm respectively. They have been handled via if case.
- In ec-cloneKey.js, EC specific return values are checked. Similarly for RSA-cloneKey.js handles RSA specific return values.
- Tests iterates for each particular algorithm depending on the extractable, key usage,  algorithm parameters(i.e. hashing algorithm, named curve) and key data.

The input data is present in the test file. HMAC has 8/64/88/128 bit data test. Similar for PBKDF2 & HKDF but also includes empty bit data test.

Old files that has been updated:
- clone-aesKey.html has been split in AES-CBC and AES-GCM. There were no cloneKey tests for AES-CTR and AES-KW, they have been added.
- clone-ecKey-private/public has been split into ECDH and ECDSA. ECDH public test was missing, and has been added.
- clone-hkdf and clone-pbkdf2, has been moved to HKDF and PBKDF2 respective directories.
- clone-rsaHashedKey, holds tests only for RSASSA-PKCS1, tests has been  moved to the algorithm directory.

BUG=452269
R=eroman

Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=191772

[Habib Virji, 2015-02-27 17:55:19]

Patchset #2 (id:20001) has been deleted

[Habib Virji, 2015-02-27 17:56:36]

It only contains cloneKey tests.

[eroman, 2015-02-27 19:13:36]

I like the general direction of this change, however find that cloneKey.js is
too complex (too many branches for specific algorithms and properties) which
makes it hard to read and evaluate.

I think this could be solved by having different entry-points for different
algorithm classes.

We have the following key types:
  EC keys (public/private)
  RSA keys (public/private)
  Symmetric keys (AES keys/*KDF keys/HMAC keys)

How about having an entry point for the key type, which implies the data to be
imported.

-----------------
For instance:
-----------------

Testing ECDSA (ecdsa/cloneKey.html):
  testCloneEcPublicKeys("ECDSA", kPossiblePublicUsages)
  testCloneEcPrivateKeys("ECDSA", kPossiblePrivateUsages)

Testing ECDH (ecdh/cloneKey.html):
  testCloneEcPublicKeys("ECDH", kPossiblePublicUsages)
  testCloneEcPrivateKeys("ECDH", kPossiblePrivateUsages)

Testing RSA-SSA (rsassa-pkcs1-v1_5/cloneKey.html):
  testCloneRsaPublicKeys("RSASSA-PKCS1-v1_5", kPossiblePublicKeyUsages)
  testCloneRsaPrivateKeys("RSASSA-PKCS1-v1_5", kPossiblePrivateKeyUsages)

Testing AES-GCM (aes-gcm/cloneKey.html):
  testCloneAesKeys("AES-KW", kPossibleUsages);

Testing AES-CTR (aes-ctr/cloneKey.html):
  testCloneAesKeys("aes-ctr", kPossibleUsages);

Testing AES-CTR (aes-ctr/cloneKey.html):
  testCloneAesKeys("aes-ctr", kPossibleUsages);

... So on and so forth.


testCloneEcPublicKeys():
   * Takes care of import format, import data, and iterating through curves


testCloneAesKeys():
   * Could call out to a helper testCloneSymmetricKeys() that is shared by HMAC
and AES
   * Would iterate through 128-bit and 256-bit data


testCloneHmacKeys():
   * Could call out to testCloneSymmetricKeys() giving it data of a few
different lenths (whereas AES tests are just for 128-bit and 256-bit data).


testCloneKdfKeys():
  * This could be the basis of testing PKBDF2 and HKDF. It is essentially like
testCloneSymmetricKeys() only there is no length property on the keys.




A few other general comments I have towards tests:


   * Try to define the key data as close as possible to where it is used. For
instance the EC key data can be defined beside the testEcPublicKeys() helper. I
prefer this over putting in a shared keys.js since it is hard to reason about
what those constants are, and in general would like to get rid of keys.js as
much as possible for better readability.

  * I don't mind duplication in test code when it helps readability. Making test
code too general in my opinion can be problematic as it hurts readability, and
even worse sometimes refactors end up breaking what the test is supposed to be
testing because of the reduced readability.

[eroman, 2015-02-27 19:17:01]

Oh one more comment on organization:

If we find we have lots of shared code we may consider adding
per-algorithm-class directories like:

  crypto/subtle/aes/*  --> Contains .js files used by AES algorithms (i.e.
aes-kw/ aes-gcm/ aes-ctr/ etc..)

  crypto/subtle/rsa/*  --> Contains shared .js files for RSA algorithms


At a minimum we are going to have clone key javascript files (specific to each
algorithm class if you follow my other suggestion).

We might expand on this and find it useful to have JWK specific javascript files
for each algorithm class too (certainly AES key JWK import/export is pretty
common, however is mainly just being tested for AES-CBC at the moment).

Cheers

[Habib Virji, 2015-03-03 17:23:44]

Thanks eroman for your suggestions.

Have uploaded a new patch with division based on algorithm classes. 

So now there are three different cloneKey.js
- symmetric-cloneKey.js
- rsa-cloneKey.js
- ec-cloneKey.js

Public and private key entry point have been implemented as suggested. 

Only difference is implemented single symmetric-cloneKey.js instead of dividing
between HMAC, AES and KDF. HMAC and AES difference was one has hash parameters
and other does not have, same for HMAC and KDF as they share same number of
input parameters but length and hash name are not present in the importedKey.
Option was either create 3 different files for AES, HMAC and KDF or create one
which handle extra parameters for HMAC and AES functions. I have opted for
second option as it does not have much if else and quite readable, but if you
think not I can change code to 3 different files.

Structures are removed from keys.js and are now part of from where they are
called.

Regarding shared code does make sense but did not organize in the order
suggested in this patch as usually js files are located in resources, might make
sense to move organization inside resources such as : 
- resources/rsa/cloneKey.js
- resources/ec/cloneKey.js
- resources/symmetric/cloneKey.js (if current single file solution is
acceptable)

[eroman, 2015-03-05 19:58:51]

LGTM once you update the CL description (whose comments reference the old way of
doing things; for instance "Creates a single file cloneKey.js").

Other than that I have a couple of comments, but none of them are essential.
Feel free to submit as is and re-visit them in a follow-up changelist.

https://codereview.chromium.org/958353003/diff/40001/LayoutTests/crypto/subtl...
File LayoutTests/crypto/subtle/ecdh/cloneKey.html (right):

https://codereview.chromium.org/958353003/diff/40001/LayoutTests/crypto/subtl...
LayoutTests/crypto/subtle/ecdh/cloneKey.html:14: testCloneEcPublicKeys('ECDH',
[[]]).then(runPrivateECDHTest, failAndFinishJSTest);
I would suggest writing this as:

function testClonePublicKeys() {
  return testCloneEcPublicKeys('ECDH' ...);
}

function testClonePrivateKeys() {
  return testCloneEcPrivateKeys('ECDH", ...);
}

testClonePublicKeys().then(testClonePrivateKeys).then(finishJSTest,
failAndFinishJSTest);

https://codereview.chromium.org/958353003/diff/40001/LayoutTests/crypto/subtl...
File LayoutTests/crypto/subtle/ecdsa/cloneKey-expected.txt (right):

https://codereview.chromium.org/958353003/diff/40001/LayoutTests/crypto/subtl...
LayoutTests/crypto/subtle/ecdsa/cloneKey-expected.txt:1: Tests structured
cloning of ECDSA private and public keys
This description is fine. Another way would be "Tests structured cloning of
ECDSA keys"

https://codereview.chromium.org/958353003/diff/40001/LayoutTests/crypto/subtl...
File LayoutTests/crypto/subtle/ecdsa/cloneKey.html (right):

https://codereview.chromium.org/958353003/diff/40001/LayoutTests/crypto/subtl...
LayoutTests/crypto/subtle/ecdsa/cloneKey.html:14: testCloneEcPublicKeys('ECDSA',
[[],['verify']]).then(runPrivateECDSATest, failAndFinishJSTest);
If you follow the recommendation for ECDH keys, then use that structure for all
of the tests.

https://codereview.chromium.org/958353003/diff/40001/LayoutTests/crypto/subtl...
File LayoutTests/crypto/subtle/hkdf/cloneKey.html (right):

https://codereview.chromium.org/958353003/diff/40001/LayoutTests/crypto/subtl...
LayoutTests/crypto/subtle/hkdf/cloneKey.html:20: var kKeyData = [k128BitData,
k256BitData];
[Optional]: Perhaps as a follow-up, we could change the key data used. 128bit
and 256bit data is not as interesting as empty data or non-multiple-of-8 data
would be.

https://codereview.chromium.org/958353003/diff/40001/LayoutTests/crypto/subtl...
File LayoutTests/crypto/subtle/hmac/cloneKey.html (right):

https://codereview.chromium.org/958353003/diff/40001/LayoutTests/crypto/subtl...
LayoutTests/crypto/subtle/hmac/cloneKey.html:20: var kKeyData = [k128BitData,
k256BitData];
Same comment as for HDKF, in the future we may want to change this to be
different lengthed data (since HMAC is not limited to 128 bit or 256 bit data.

https://codereview.chromium.org/958353003/diff/40001/LayoutTests/crypto/subtl...
File LayoutTests/crypto/subtle/resources/ec-cloneKey.js (right):

https://codereview.chromium.org/958353003/diff/40001/LayoutTests/crypto/subtl...
LayoutTests/crypto/subtle/resources/ec-cloneKey.js:70: "P-256":
"3059301306072A8648CE3D020106082A8648CE3D030107034200049CB0CF69303DAFC761D4E4687B4ECF039E6D34AB964AF80810D8D558A4A8D6F72D51233A1788920A86EE08A1962C79EFA317FB7879E297DAD2146DB995FA1C78",
nit: indent these a bit.

https://codereview.chromium.org/958353003/diff/40001/LayoutTests/crypto/subtl...
LayoutTests/crypto/subtle/resources/ec-cloneKey.js:80: "P-256": 
"308187020100301306072A8648CE3D020106082A8648CE3D030107046D306B02010104201FE33950C5F461124AE992C2BDFDF1C73B1615F571BD567E60D19AA1F48CDF42A144034200047C110C66DCFDA807F6E69E45DDB3C74F69A1484D203E8DC5ADA8E9A9DD7CB3C70DF448986E51BDE5D1576F99901F9C2C6A806A47FD907643A72B835597EFC8C6",
indent

https://codereview.chromium.org/958353003/diff/40001/LayoutTests/crypto/subtl...
LayoutTests/crypto/subtle/resources/ec-cloneKey.js:84:
"3081EE020100301006072A8648CE3D020106052B810400230481D63081D3020101044201BD56BD106118EDA246155BD43B42B8E13F0A6E25DD3BB376026FAB4DC92B6157BC6DFEC2D15DD3D0CF2A39AA68494042AF48BA9601118DA82C6F2108A3A203AD74A181890381860004012FBCAEFFA6A51F3EE4D3D2B51C5DEC6D7C726CA353FC014EA2BF7CFBB9B910D32CBFA6A00FE39B6CDB8946F22775398B2E233C0CF144D78C8A7742B5C7A3BB5D23009CDEF823DD7BF9A79E8CCEACD2E4527C231D0AE5967AF0958E931D7DDCCF2805A3E618DC3039FEC9FEBBD33052FE4C0FEE98F033106064982D88F4E03549D4A64D"
// P-521
nit: This comment isn't needed :)

https://codereview.chromium.org/958353003/diff/40001/LayoutTests/crypto/subtl...
File LayoutTests/crypto/subtle/resources/rsa-cloneKey.js (right):

https://codereview.chromium.org/958353003/diff/40001/LayoutTests/crypto/subtl...
LayoutTests/crypto/subtle/resources/rsa-cloneKey.js:8: var importAlgorithm =  {
name: algorithmName, hash: { name: hashName } };
nit: remove extra space on the left. The other extra spaces however are good.

https://codereview.chromium.org/958353003/diff/40001/LayoutTests/crypto/subtl...
LayoutTests/crypto/subtle/resources/rsa-cloneKey.js:12: return
crypto.subtle.importKey(format, importData, importAlgorithm, extractable,
keyUsages).then(function(importedKey) {
Good generalization!

https://codereview.chromium.org/958353003/diff/40001/LayoutTests/crypto/subtl...
File LayoutTests/crypto/subtle/resources/symmetric-cloneKey.js (right):

https://codereview.chromium.org/958353003/diff/40001/LayoutTests/crypto/subtl...
LayoutTests/crypto/subtle/resources/symmetric-cloneKey.js:9: if (hashName)
I agree, this is a good tradeoff (having hash as a single parameter in a generic
"symmetric key" method).

https://codereview.chromium.org/958353003/diff/40001/LayoutTests/crypto/subtl...
LayoutTests/crypto/subtle/resources/symmetric-cloneKey.js:72: switch
(algorithmName)
I do not like this, since I would like to avoid having to enumerate algorithm
names anywhere.

How about solving this instead as follows:

  (1) Only test the hash if "runCloneSymmetricTests" was given a "hash"
parameter

  (2) Pass an extra boolean to "runCloneSymmetricTests", named something like
"keyHasLength". Only test the "length" parameter when this is true (will be true
for everything except the KDFs).

https://codereview.chromium.org/958353003/diff/40001/LayoutTests/crypto/subtl...
File LayoutTests/crypto/subtle/rsassa-pkcs1-v1_5/cloneKey.html (right):

https://codereview.chromium.org/958353003/diff/40001/LayoutTests/crypto/subtl...
LayoutTests/crypto/subtle/rsassa-pkcs1-v1_5/cloneKey.html:11: description('Tests
structured cloning of RSA private and public keys (with a hash)');
How about renaming this to:

"Tests structured cloning of RSASSA-PKCS1-v1_5 keys."

(Note that "with a hash" is also a historical legacy, as webcrypto no longer
supports non-hashed RSA keys.)

As a follow-up we could also add similar structured clone tests for RSA-PSS
(which we currently lack).

[eroman, 2015-03-05 20:22:50]

https://codereview.chromium.org/958353003/diff/40001/LayoutTests/crypto/subtl...
File LayoutTests/crypto/subtle/resources/symmetric-cloneKey.js (right):

https://codereview.chromium.org/958353003/diff/40001/LayoutTests/crypto/subtl...
LayoutTests/crypto/subtle/resources/symmetric-cloneKey.js:52: function
testCloneSymmetricKeys(algorithmName, possibleHashAlgorithms,
possibleExtractable, possibleKeyUsages, possibleKeyData)
I believe we can remove the parameter "possibleExtractable".

The only cases where it is not being invoked with [true, false] is for the KDFs
(PBKDF2 and HKDF).

However in both of these cases, I believe importing/cloning with
extractable=true is permitted, so we may as well be testing it.

[Habib Virji, 2015-03-09 12:04:09]

Sorry has been bit sluggish last week. Updated with as per the review:

New parameter added in symmetric-cloneKey.js, hasKeyLength to enable checking
algorithmLength as this is not present in KDF.
- KDF algorithm with extraction value as true, fails with "Unsupported export
key format for algorithm". Hence possibleExtractionValue is still kept.
- Instead of checking "hash" parameter, algorithmName is used to check if "hash"
parameter is present, because does have "hash" parameter but does not have
algorithm.hash.name set.

Input data changed for:
- HMAC input data uses 8/64/88/128 bit data
- HKDF and PBKDF2 uses 0/8/64/88/128 bit data. 

Structure changes has been updated in RSA, ECDH and ECDSA. Also the description
has been updated and format input data.

Two new tests for Non-multiple-of-8 data and RSA-PSS are needed to be added. I
am working on them and will upload in a separate CL.

[eroman, 2015-03-10 00:11:06]

lgtm

https://codereview.chromium.org/958353003/diff/60001/LayoutTests/crypto/subtl...
File LayoutTests/crypto/subtle/hkdf/cloneKey.html (right):

https://codereview.chromium.org/958353003/diff/60001/LayoutTests/crypto/subtl...
LayoutTests/crypto/subtle/hkdf/cloneKey.html:25: testCloneSymmetricKeys('HKDF',
kPossibleHashAlgorithms, kPossibleExtractable, kPossibleKeyUsage,
kKeyData).then(finishJSTest, failAndFinishJSTest);
Can you explicitly pass false for the "keyHasLength" argument?

You could do this similarly to what you did in the other files:

var kKeyHasLength = false;

https://codereview.chromium.org/958353003/diff/60001/LayoutTests/crypto/subtl...
File LayoutTests/crypto/subtle/pbkdf2/cloneKey.html (right):

https://codereview.chromium.org/958353003/diff/60001/LayoutTests/crypto/subtl...
LayoutTests/crypto/subtle/pbkdf2/cloneKey.html:25:
testCloneSymmetricKeys('PBKDF2', kPossibleHashAlgorithms, kPossibleExtractable,
kPossibleKeyUsage, kKeyData).then(finishJSTest, failAndFinishJSTest);
Same comment as earlier (pass the keyHasLength parameter rather than having it
default to undefined)

https://codereview.chromium.org/958353003/diff/60001/LayoutTests/crypto/subtl...
File LayoutTests/crypto/subtle/resources/symmetric-cloneKey.js (right):

https://codereview.chromium.org/958353003/diff/60001/LayoutTests/crypto/subtl...
LayoutTests/crypto/subtle/resources/symmetric-cloneKey.js:33: if (algorithmName
=== "HMAC")
I suggest doing a case-insensitive match here:


var expectingHash = algorithmName.toLowerCase() == "hmac";
if (expectingHash)
  ....

Or I suppose it would also work as:

var expectingHash = hashName && keyHasLength;

But that might be hard to understand.

https://codereview.chromium.org/958353003/diff/60001/LayoutTests/crypto/subtl...
LayoutTests/crypto/subtle/resources/symmetric-cloneKey.js:45: if (algorithmName
=== "HMAC")
if (expectingHash)

so the algorithm name comparison is only done in one place.

[Habib Virji, 2015-03-12 10:30:25]

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

[Habib Virji, 2015-03-12 10:30:25]

The patchset sent to the CQ was uploaded after l-g-t-m from eroman@chromium.org
Link to the patchset: https://codereview.chromium.org/958353003/#ps80001 (title:
" ")

[commit-bot: I haz the power, 2015-03-12 10:30:36]

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

[Habib Virji, 2015-03-12 10:30:50]

Addressed all the comments. Thanks.

[commit-bot: I haz the power, 2015-03-12 13:25:48]

Committed patchset #4 (id:80001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=191772

[eroman, 2015-03-12 17:40:06]

committed patchset LGTM