Issue 822083003: Webcrypto: add layout tests for HKDF algorithm

nharper

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

5 years, 12 months ago (2014-12-23 22:50:38 UTC) #1

nharper

This is dependent on https://codereview.chromium.org/803173010/ which is dependent on https://codereview.chromium.org/789733009/

5 years, 12 months ago (2014-12-23 22:50:38 UTC) #2

eroman

(1) Please add a test for importing an empty key, and using it to derive. ...

5 years, 12 months ago (2014-12-23 23:29:39 UTC) #3

nharper

https://codereview.chromium.org/822083003/diff/1/LayoutTests/crypto/subtle/hkdf-deriveBits-failures.html File LayoutTests/crypto/subtle/hkdf-deriveBits-failures.html (right): https://codereview.chromium.org/822083003/diff/1/LayoutTests/crypto/subtle/hkdf-deriveBits-failures.html#newcode20 LayoutTests/crypto/subtle/hkdf-deriveBits-failures.html:20: // set up the test by creating an HKDF ...

5 years, 11 months ago (2015-01-06 23:52:11 UTC) #4

eroman

Please also add tests for bit lengths that are not multiples of 8. https://codereview.chromium.org/822083003/diff/1/LayoutTests/crypto/subtle/hkdf-deriveKey.html File ...

5 years, 11 months ago (2015-01-07 01:18:44 UTC) #5

eroman

https://codereview.chromium.org/822083003/diff/40001/LayoutTests/crypto/subtle/hkdf-failures-expected.txt File LayoutTests/crypto/subtle/hkdf-failures-expected.txt (right): https://codereview.chromium.org/822083003/diff/40001/LayoutTests/crypto/subtle/hkdf-failures-expected.txt#newcode8 LayoutTests/crypto/subtle/hkdf-failures-expected.txt:8: error is: NotSupportedError: Unsupported export key format for algorithm ...

5 years, 11 months ago (2015-01-09 03:16:02 UTC) #6

nharper

https://codereview.chromium.org/822083003/diff/1/LayoutTests/crypto/subtle/hkdf-deriveKey.html File LayoutTests/crypto/subtle/hkdf-deriveKey.html (right): https://codereview.chromium.org/822083003/diff/1/LayoutTests/crypto/subtle/hkdf-deriveKey.html#newcode25 LayoutTests/crypto/subtle/hkdf-deriveKey.html:25: return crypto.subtle.deriveKey({name: "HKDF", hash: "SHA-256", salt: new Uint8Array(), info: ...

5 years, 11 months ago (2015-01-09 17:54:35 UTC) #7

https://codereview.chromium.org/822083003/diff/1/LayoutTests/crypto/subtle/hk...
File LayoutTests/crypto/subtle/hkdf-deriveKey.html (right):

https://codereview.chromium.org/822083003/diff/1/LayoutTests/crypto/subtle/hk...
LayoutTests/crypto/subtle/hkdf-deriveKey.html:25: return
crypto.subtle.deriveKey({name: "HKDF", hash: "SHA-256", salt: new Uint8Array(),
info: new Uint8Array()}, baseKey, {name: "AES-GCM", length: 256}, extractable,
['encrypt']);
On 2015/01/07 01:18:42, eroman wrote:
> On 2015/01/06 23:51:59, nharper wrote:
> > On 2014/12/23 23:29:39, eroman wrote:
> > > Another test I would like to see is chaining ECDH and HKDF, since I think
> that
> > > will be a real use-case.
> > 
> > ECDH currently doesn't support the get key length operation, so I can't
derive
> > an ECDH key. Should it support this operation? (If so, that's s spec bug.)
> 
> Other way around. ECDH derives a key for HKDF, and then you call deriveBits()
/
> deriveKey() on HKDF for the final key.
> 
> The output of ECDH is not suited to used directly as key material, so common
> usage should feed it through a KDF first.
> 
> As far as ECDH having a get key length operation, I don't think that would
make
> sense for a asymmetric key algorithm. ECDH does support the "raw" format so
you
> could imagine it being used with deriveKey(), however the expectation for raw
> import is a public key.

That makes more sense (both the order of chaining ECDH->HKDF and that an
asymmetric algorithm like ECDH doesn't have a get key length operation).

I tried writing the test to derive an HKDF key from an ECDH key, but it fails
because HKDF doesn't have a get key length operation. (It makes sense that it
doesn't have a get key length operation, as you can use any length for your
IKM.) This means that if one wants to derive an HKDF key ECDH keys, one has to
call deriveBits with the ECDH keys and use the shared secret generated by
deriveBits as the input to HKDF's importKey. If we wanted to support the use
case of using deriveKey with ECDH and HKDF, we'd need to add the get key length
operation to HKDF, which would mean that we'd need a subclass of KeyAlgorithm
for HKDF that had a length parameter.

I would like to say that we should keep the spec the way it is (i.e. to make an
HKDF key from ECDH, one has to use deriveBits and importKey), but the more I
think about it, the less I think this is good for the webcrypto API. If ECDH in
webcrypto is used for static keys, the current API design will encourage people
to derive keys (like AES keys) directly from the shared secret of ECDH key
agreement, because one can call deriveKey with an ECDH keypair to generate an
AES key (for example), which is a Bad Idea(tm). Doing the correct thing of using
the shared secret from ECDH as input to a KDF and deriving their key through the
KDF is more work for the API user, because they must call deriveBits with the
ECDH keys, know what the length is of the secret generated from the curve
they're using, and then import the derivedBits into an HKDF key. (If the ECDH
keys in question are ephemeral and not static, then it might be acceptable to
use the secret straight from ECDH key agreement, but I don't know the crypto
well enough.)

If the spec were changed to support deriving an HKDF key from ECDH keys, I would
suggest that the spec also be changed so that deriveKey for ECDH keys can only
be used to derive KDF keys. (If someone wanted the current behavior of being
able to generate an AES key directly from an ECDH secret, that would still be
possible, but it would require them using deriveBits and importKey instead.) Or,
the API can stay as it is - after all, it is understood that it is subtle (hence
the name crypto.subtle) and easy to shoot one's self in the foot with.

eroman

https://codereview.chromium.org/822083003/diff/1/LayoutTests/crypto/subtle/hkdf-deriveKey.html File LayoutTests/crypto/subtle/hkdf-deriveKey.html (right): https://codereview.chromium.org/822083003/diff/1/LayoutTests/crypto/subtle/hkdf-deriveKey.html#newcode25 LayoutTests/crypto/subtle/hkdf-deriveKey.html:25: return crypto.subtle.deriveKey({name: "HKDF", hash: "SHA-256", salt: new Uint8Array(), info: ...

5 years, 11 months ago (2015-01-09 19:27:47 UTC) #8

https://codereview.chromium.org/822083003/diff/1/LayoutTests/crypto/subtle/hk...
File LayoutTests/crypto/subtle/hkdf-deriveKey.html (right):

https://codereview.chromium.org/822083003/diff/1/LayoutTests/crypto/subtle/hk...
LayoutTests/crypto/subtle/hkdf-deriveKey.html:25: return
crypto.subtle.deriveKey({name: "HKDF", hash: "SHA-256", salt: new Uint8Array(),
info: new Uint8Array()}, baseKey, {name: "AES-GCM", length: 256}, extractable,
['encrypt']);
On 2015/01/09 17:54:34, nharper wrote:
> On 2015/01/07 01:18:42, eroman wrote:
> > On 2015/01/06 23:51:59, nharper wrote:
> > > On 2014/12/23 23:29:39, eroman wrote:
> > > > Another test I would like to see is chaining ECDH and HKDF, since I
think
> > that
> > > > will be a real use-case.
> > > 
> > > ECDH currently doesn't support the get key length operation, so I can't
> derive
> > > an ECDH key. Should it support this operation? (If so, that's s spec bug.)
> > 
> > Other way around. ECDH derives a key for HKDF, and then you call
deriveBits()
> /
> > deriveKey() on HKDF for the final key.
> > 
> > The output of ECDH is not suited to used directly as key material, so common
> > usage should feed it through a KDF first.
> > 
> > As far as ECDH having a get key length operation, I don't think that would
> make
> > sense for a asymmetric key algorithm. ECDH does support the "raw" format so
> you
> > could imagine it being used with deriveKey(), however the expectation for
raw
> > import is a public key.
> 
> That makes more sense (both the order of chaining ECDH->HKDF and that an
> asymmetric algorithm like ECDH doesn't have a get key length operation).
> 
> I tried writing the test to derive an HKDF key from an ECDH key, but it fails
> because HKDF doesn't have a get key length operation. (It makes sense that it
> doesn't have a get key length operation, as you can use any length for your
> IKM.) This means that if one wants to derive an HKDF key ECDH keys, one has to
> call deriveBits with the ECDH keys and use the shared secret generated by
> deriveBits as the input to HKDF's importKey. If we wanted to support the use
> case of using deriveKey with ECDH and HKDF, we'd need to add the get key
length
> operation to HKDF, which would mean that we'd need a subclass of KeyAlgorithm
> for HKDF that had a length parameter.
> 
> I would like to say that we should keep the spec the way it is (i.e. to make
an
> HKDF key from ECDH, one has to use deriveBits and importKey), but the more I
> think about it, the less I think this is good for the webcrypto API. If ECDH
in
> webcrypto is used for static keys, the current API design will encourage
people
> to derive keys (like AES keys) directly from the shared secret of ECDH key
> agreement, because one can call deriveKey with an ECDH keypair to generate an
> AES key (for example), which is a Bad Idea(tm). Doing the correct thing of
using
> the shared secret from ECDH as input to a KDF and deriving their key through
the
> KDF is more work for the API user, because they must call deriveBits with the
> ECDH keys, know what the length is of the secret generated from the curve
> they're using, and then import the derivedBits into an HKDF key. (If the ECDH
> keys in question are ephemeral and not static, then it might be acceptable to
> use the secret straight from ECDH key agreement, but I don't know the crypto
> well enough.)
> 
> If the spec were changed to support deriving an HKDF key from ECDH keys, I
would
> suggest that the spec also be changed so that deriveKey for ECDH keys can only
> be used to derive KDF keys. (If someone wanted the current behavior of being
> able to generate an AES key directly from an ECDH secret, that would still be
> possible, but it would require them using deriveBits and importKey instead.)
Or,
> the API can stay as it is - after all, it is understood that it is subtle
(hence
> the name crypto.subtle) and easy to shoot one's self in the foot with.

Not sure I understand your claim

(1) HKDF _does_ support get key length, and it is an omission of your current CL
that it is not implemented.
(2) Deriving KDF keys from ECDH is pretty much the whole point of the
deriveKey() operation
(3) Agreed that deriving an AES key from ECDH would be silly. However it is
called crypto.subtle.* so the burden is on the caller to do something that isn't
dumb.

nharper

https://codereview.chromium.org/822083003/diff/1/LayoutTests/crypto/subtle/hkdf-deriveKey.html File LayoutTests/crypto/subtle/hkdf-deriveKey.html (right): https://codereview.chromium.org/822083003/diff/1/LayoutTests/crypto/subtle/hkdf-deriveKey.html#newcode25 LayoutTests/crypto/subtle/hkdf-deriveKey.html:25: return crypto.subtle.deriveKey({name: "HKDF", hash: "SHA-256", salt: new Uint8Array(), info: ...

5 years, 11 months ago (2015-01-09 22:41:47 UTC) #9

https://codereview.chromium.org/822083003/diff/1/LayoutTests/crypto/subtle/hk...
File LayoutTests/crypto/subtle/hkdf-deriveKey.html (right):

https://codereview.chromium.org/822083003/diff/1/LayoutTests/crypto/subtle/hk...
LayoutTests/crypto/subtle/hkdf-deriveKey.html:25: return
crypto.subtle.deriveKey({name: "HKDF", hash: "SHA-256", salt: new Uint8Array(),
info: new Uint8Array()}, baseKey, {name: "AES-GCM", length: 256}, extractable,
['encrypt']);
On 2015/01/09 19:27:36, eroman wrote:
> On 2015/01/09 17:54:34, nharper wrote:
> > On 2015/01/07 01:18:42, eroman wrote:
> > > On 2015/01/06 23:51:59, nharper wrote:
> > > > On 2014/12/23 23:29:39, eroman wrote:
> > > > > Another test I would like to see is chaining ECDH and HKDF, since I
> think
> > > that
> > > > > will be a real use-case.
> > > > 
> > > > ECDH currently doesn't support the get key length operation, so I can't
> > derive
> > > > an ECDH key. Should it support this operation? (If so, that's s spec
bug.)
> > > 
> > > Other way around. ECDH derives a key for HKDF, and then you call
> deriveBits()
> > /
> > > deriveKey() on HKDF for the final key.
> > > 
> > > The output of ECDH is not suited to used directly as key material, so
common
> > > usage should feed it through a KDF first.
> > > 
> > > As far as ECDH having a get key length operation, I don't think that would
> > make
> > > sense for a asymmetric key algorithm. ECDH does support the "raw" format
so
> > you
> > > could imagine it being used with deriveKey(), however the expectation for
> raw
> > > import is a public key.
> > 
> > That makes more sense (both the order of chaining ECDH->HKDF and that an
> > asymmetric algorithm like ECDH doesn't have a get key length operation).
> > 
> > I tried writing the test to derive an HKDF key from an ECDH key, but it
fails
> > because HKDF doesn't have a get key length operation. (It makes sense that
it
> > doesn't have a get key length operation, as you can use any length for your
> > IKM.) This means that if one wants to derive an HKDF key ECDH keys, one has
to
> > call deriveBits with the ECDH keys and use the shared secret generated by
> > deriveBits as the input to HKDF's importKey. If we wanted to support the use
> > case of using deriveKey with ECDH and HKDF, we'd need to add the get key
> length
> > operation to HKDF, which would mean that we'd need a subclass of
KeyAlgorithm
> > for HKDF that had a length parameter.
> > 
> > I would like to say that we should keep the spec the way it is (i.e. to make
> an
> > HKDF key from ECDH, one has to use deriveBits and importKey), but the more I
> > think about it, the less I think this is good for the webcrypto API. If ECDH
> in
> > webcrypto is used for static keys, the current API design will encourage
> people
> > to derive keys (like AES keys) directly from the shared secret of ECDH key
> > agreement, because one can call deriveKey with an ECDH keypair to generate
an
> > AES key (for example), which is a Bad Idea(tm). Doing the correct thing of
> using
> > the shared secret from ECDH as input to a KDF and deriving their key through
> the
> > KDF is more work for the API user, because they must call deriveBits with
the
> > ECDH keys, know what the length is of the secret generated from the curve
> > they're using, and then import the derivedBits into an HKDF key. (If the
ECDH
> > keys in question are ephemeral and not static, then it might be acceptable
to
> > use the secret straight from ECDH key agreement, but I don't know the crypto
> > well enough.)
> > 
> > If the spec were changed to support deriving an HKDF key from ECDH keys, I
> would
> > suggest that the spec also be changed so that deriveKey for ECDH keys can
only
> > be used to derive KDF keys. (If someone wanted the current behavior of being
> > able to generate an AES key directly from an ECDH secret, that would still
be
> > possible, but it would require them using deriveBits and importKey instead.)
> Or,
> > the API can stay as it is - after all, it is understood that it is subtle
> (hence
> > the name crypto.subtle) and easy to shoot one's self in the foot with.
> 
> Not sure I understand your claim
> 
> (1) HKDF _does_ support get key length, and it is an omission of your current
CL
> that it is not implemented.
> (2) Deriving KDF keys from ECDH is pretty much the whole point of the
> deriveKey() operation
> (3) Agreed that deriving an AES key from ECDH would be silly. However it is
> called crypto.subtle.* so the burden is on the caller to do something that
isn't
> dumb.

My claim was based on a) not realizing that HKDF does support get key length
(and it returns null), and b) not realizing that ECDH is clever and interprets a
null length in derive bits to mean return all bits of the secret. Now that I'm
aware of that, my point is moot.

https://codereview.chromium.org/822083003/diff/40001/LayoutTests/TestExpectat...
File LayoutTests/TestExpectations (right):

https://codereview.chromium.org/822083003/diff/40001/LayoutTests/TestExpectat...
LayoutTests/TestExpectations:588: #  (b) RSA-PSS, AES-CTR, ECDH and ECDSA have
only been implemented for BoringSSL
On 2015/01/07 01:18:42, eroman wrote:
> Add in HKDF to this list, if we are to keep it.

Done.

https://codereview.chromium.org/822083003/diff/40001/LayoutTests/TestExpectat...
LayoutTests/TestExpectations:626: crbug.com/338887 [ Linux ]
crypto/subtle/hkdf-failures.html [ Failure Pass ]
On 2015/01/07 01:18:42, eroman wrote:
> Naming issue: there is hkdf-deriveBits-failures and hdkf-failures. Unclear
what
> the intended difference is.

Renamed tests.

https://codereview.chromium.org/822083003/diff/40001/LayoutTests/TestExpectat...
LayoutTests/TestExpectations:627: crbug.com/338887 [ Linux ]
crypto/subtle/hkdf-importKey.html [ Failure Pass ]
On 2015/01/07 01:18:42, eroman wrote:
> Please also add export tests.

I'm not sure what export tests you want - calling exportKey() on an HKDF key
should fail (according to the spec), and this is tested in hkdf-exportKey.html
(formerly hkdf-failures.html).

https://codereview.chromium.org/822083003/diff/40001/LayoutTests/crypto/subtl...
File LayoutTests/crypto/subtle/clone-hkdfKey.html (right):

https://codereview.chromium.org/822083003/diff/40001/LayoutTests/crypto/subtl...
LayoutTests/crypto/subtle/clone-hkdfKey.html:16: // Tests the 48 permutations of
keys generated by:
On 2015/01/07 01:18:42, eroman wrote:
> This comment needs to be updated. (in fact the original comment doesn't seem
> right either)

Done. (The original comment being wrong is possibly my fault - if there were an
empty usage list in kPossibleKeyUsages, then the comment would be correct.)

https://codereview.chromium.org/822083003/diff/40001/LayoutTests/crypto/subtl...
LayoutTests/crypto/subtle/clone-hkdfKey.html:38: window.importedKey =
importedKey;
On 2015/01/07 01:18:43, eroman wrote:
> why change the code pattern from the original? Using a global on "window" has
> disadvantages:
> 
>   (1) The code cannot be run in WebWorker tests, as they have a different
global
> context.
> 
> 
>   (2) If combined with other promise-based code, there is no guarantee that
the
> then() runs directly afterwards. So relying on global state is less robust
then
> state from the enclosing environmet.

The reason for the change was because the local variable importedKey was
shadowing the global (used in shouldEvaluateAs). I've changed it back to be
mostly the way it was and renamed the local so it doesn't shadow a global.

https://codereview.chromium.org/822083003/diff/40001/LayoutTests/crypto/subtl...
File LayoutTests/crypto/subtle/hkdf-deriveBits-failures-expected.txt (right):

https://codereview.chromium.org/822083003/diff/40001/LayoutTests/crypto/subtl...
LayoutTests/crypto/subtle/hkdf-deriveBits-failures-expected.txt:10: deriveBits()
with a key that doesn't match the algorithm...
On 2015/01/07 01:18:43, eroman wrote:
> would be better to say here "with an ECDH key" or some such thing.

Done.

https://codereview.chromium.org/822083003/diff/40001/LayoutTests/crypto/subtl...
File LayoutTests/crypto/subtle/hkdf-deriveBits-failures.html (right):

https://codereview.chromium.org/822083003/diff/40001/LayoutTests/crypto/subtl...
LayoutTests/crypto/subtle/hkdf-deriveBits-failures.html:51:
debug("\nderiveBits() with requested length too long...");
On 2015/01/07 01:18:43, eroman wrote:
> nit: rather than saying "too long" can you say "of 65281". This makes it
easier
> when reviewing test outputs since the description says what the input was.

Done.

https://codereview.chromium.org/822083003/diff/40001/LayoutTests/crypto/subtl...
File LayoutTests/crypto/subtle/hkdf-deriveBits.html (right):

https://codereview.chromium.org/822083003/diff/40001/LayoutTests/crypto/subtl...
LayoutTests/crypto/subtle/hkdf-deriveBits.html:80: return
crypto.subtle.importKey("raw", hexStringToUint8Array(testCase.input), {name:
"HKDF"}, extractable, ['deriveKey','deriveBits']);
On 2015/01/07 01:18:43, eroman wrote:
> FYI: You can also just pass "HKDF" as the AlgorithmIdentifier now (don't need
to
> specify a dictionary).

Done.

https://codereview.chromium.org/822083003/diff/40001/LayoutTests/crypto/subtl...
LayoutTests/crypto/subtle/hkdf-deriveBits.html:85:
shouldEvaluateAs("key.extractable", true);
On 2015/01/07 01:18:43, eroman wrote:
> Since a variable "extractable" is used, would make sense to compare against
that
> instead of "true".

Done.

https://codereview.chromium.org/822083003/diff/40001/LayoutTests/crypto/subtl...
File LayoutTests/crypto/subtle/hkdf-deriveKey.html (right):

https://codereview.chromium.org/822083003/diff/40001/LayoutTests/crypto/subtl...
LayoutTests/crypto/subtle/hkdf-deriveKey.html:36:
shouldEvaluateAs("derivedKey.extractable", true);
On 2015/01/07 01:18:43, eroman wrote:
> extractable

Done.

https://codereview.chromium.org/822083003/diff/40001/LayoutTests/crypto/subtl...
LayoutTests/crypto/subtle/hkdf-deriveKey.html:60:
shouldEvaluateAs("derivedKey.extractable", true);
On 2015/01/07 01:18:43, eroman wrote:
> extractable

Done.

https://codereview.chromium.org/822083003/diff/40001/LayoutTests/crypto/subtl...
File LayoutTests/crypto/subtle/hkdf-failures-expected.txt (right):

https://codereview.chromium.org/822083003/diff/40001/LayoutTests/crypto/subtl...
LayoutTests/crypto/subtle/hkdf-failures-expected.txt:7: HKDF does not support
exportKey()...
On 2015/01/07 01:18:43, eroman wrote:
> Make this read what it is about to do:
>    calling exportKey() for HKDF key...

Done.

https://codereview.chromium.org/822083003/diff/40001/LayoutTests/crypto/subtl...
File LayoutTests/crypto/subtle/hkdf-failures.html (right):

https://codereview.chromium.org/822083003/diff/40001/LayoutTests/crypto/subtl...
LayoutTests/crypto/subtle/hkdf-failures.html:20: // set up the test by creating
an HKDF key and an RSA key
On 2015/01/07 01:18:43, eroman wrote:
> indent by 4.

Done.

https://codereview.chromium.org/822083003/diff/40001/LayoutTests/crypto/subtl...
File LayoutTests/crypto/subtle/hkdf-importKey.html (right):

https://codereview.chromium.org/822083003/diff/40001/LayoutTests/crypto/subtl...
LayoutTests/crypto/subtle/hkdf-importKey.html:17: raw_bytes = new Uint8Array([1,
2]);
On 2015/01/07 01:18:44, eroman wrote:
> rawBytes

Done.

https://codereview.chromium.org/822083003/diff/40001/LayoutTests/crypto/subtl...
LayoutTests/crypto/subtle/hkdf-importKey.html:21: debug("\nimportKey() should
fail with 'encrypt' usage...");
On 2015/01/07 01:18:43, eroman wrote:
> I find these comments more useful when it says what is about to be done, not
> what the expectation is (the result is printed right afterwards, and different
> baselines can capture different expectations).
> 
> Example:
> 
>   importKey() with 'encrypt' usage...

Done.

https://codereview.chromium.org/822083003/diff/40001/LayoutTests/crypto/subtl...
LayoutTests/crypto/subtle/hkdf-importKey.html:22: return
crypto.subtle.importKey("raw", raw_bytes, "HKDF", extractable, ['encrypt']);
On 2015/01/07 01:18:44, eroman wrote:
> indent by 4

Done.

https://codereview.chromium.org/822083003/diff/40001/LayoutTests/crypto/subtl...
LayoutTests/crypto/subtle/hkdf-importKey.html:26: debug("\nimportKey() with null
key...");
On 2015/01/07 01:18:44, eroman wrote:
> null key --> null data (or null key data)

Done.

https://codereview.chromium.org/822083003/diff/40001/LayoutTests/crypto/subtl...
LayoutTests/crypto/subtle/hkdf-importKey.html:31: debug("\nimportKey() with jwk
type...");
On 2015/01/07 01:18:44, eroman wrote:
> type --> format

Done.

https://codereview.chromium.org/822083003/diff/40001/LayoutTests/crypto/subtl...
LayoutTests/crypto/subtle/hkdf-importKey.html:36: debug("\nimportKey() with spki
type...");
On 2015/01/07 01:18:44, eroman wrote:
> type -> format

Done.

https://codereview.chromium.org/822083003/diff/40001/LayoutTests/crypto/subtl...
LayoutTests/crypto/subtle/hkdf-importKey.html:47: return
crypto.subtle.importKey("raw", raw_bytes, "HKDF", extractable, ['deriveKey']);
On 2015/01/07 01:18:44, eroman wrote:
> Consider naming this import-failures? (and removing the 1 success case).

Done.

https://codereview.chromium.org/822083003/diff/40001/LayoutTests/crypto/subtl...
File LayoutTests/crypto/subtle/hkdf-unwrapKey.html (right):

https://codereview.chromium.org/822083003/diff/40001/LayoutTests/crypto/subtl...
LayoutTests/crypto/subtle/hkdf-unwrapKey.html:38: // key operation, I wrap it by
calling encrypt.
On 2015/01/07 01:18:44, eroman wrote:
> Same comment as in another review: Avoid the use of "I" and "we". There was a
> chromium-dev thread somewhere about this...

Done.

eroman

https://codereview.chromium.org/822083003/diff/40001/LayoutTests/TestExpectations File LayoutTests/TestExpectations (right): https://codereview.chromium.org/822083003/diff/40001/LayoutTests/TestExpectations#newcode627 LayoutTests/TestExpectations:627: crbug.com/338887 [ Linux ] crypto/subtle/hkdf-importKey.html [ Failure Pass ] ...

5 years, 11 months ago (2015-01-12 22:35:23 UTC) #10

nharper

https://codereview.chromium.org/822083003/diff/60001/LayoutTests/crypto/subtle/derive-hkdf-keys.html File LayoutTests/crypto/subtle/derive-hkdf-keys.html (right): https://codereview.chromium.org/822083003/diff/60001/LayoutTests/crypto/subtle/derive-hkdf-keys.html#newcode49 LayoutTests/crypto/subtle/derive-hkdf-keys.html:49: return crypto.subtle.deriveKey(hkdfAlgorithm, hkdfKey, "HKDF", true, ['deriveBits']); On 2015/01/12 22:35:23, ...

5 years, 11 months ago (2015-01-13 00:11:16 UTC) #11

https://codereview.chromium.org/822083003/diff/60001/LayoutTests/crypto/subtl...
File LayoutTests/crypto/subtle/derive-hkdf-keys.html (right):

https://codereview.chromium.org/822083003/diff/60001/LayoutTests/crypto/subtl...
LayoutTests/crypto/subtle/derive-hkdf-keys.html:49: return
crypto.subtle.deriveKey(hkdfAlgorithm, hkdfKey, "HKDF", true, ['deriveBits']);
On 2015/01/12 22:35:23, eroman wrote:
> This test is a weak test IMO:
>   (1) using an imported ECDH keys rather than generated one would allow
> verifying the final HKDF key in a more meaningful way (either by structured
> cloning it or by using it in a derive bits operation)
> 
>   (2) this deriveKey() is not the interesting case. The more interesting case
> would be if hkdfKey had the 'deriveKey' usages. As it is, this fails because
of
> a usages violation. We should have a test (either in addition to or in place
of)
> which tests the get key length mismatch.

1) I'm now importing 2 ECDH keys and deriving an HKDF key from that. I then
create another HKDF key from the ECDH secret (pre-computed), and on both HKDF
keys call deriveBits and verify that they derive the same bits. This is a test
that calling deriveKey with an ECDH key and a derived key type of HKDF will
result in importing the correct material into the HKDF key - it doesn't check
that HKDF is implemented correctly (that's tested by the test vectors test).

2) I changed the usage from deriveBits to deriveKey to test what I meant to
test. I noticed that the test case of derive HKDF key from HKDF key is both in
this file and hkdf-deriveKey.html - do you have a preference for which one it
lives in?

https://codereview.chromium.org/822083003/diff/60001/LayoutTests/crypto/subtl...
File LayoutTests/crypto/subtle/hkdf-deriveBits-test-vectors.html (right):

https://codereview.chromium.org/822083003/diff/60001/LayoutTests/crypto/subtl...
LayoutTests/crypto/subtle/hkdf-deriveBits-test-vectors.html:71: length: 42,
On 2015/01/12 22:35:23, eroman wrote:
> Can you add some variations with truncated lengths? And make the inputs be in
> bits rather than bytes so can add partial bit variants as well?

See previous comment - I want to keep this file containing only the RFC 5869
test vectors.

https://codereview.chromium.org/822083003/diff/60001/LayoutTests/crypto/subtl...
File LayoutTests/crypto/subtle/hkdf-deriveBits.html (right):

https://codereview.chromium.org/822083003/diff/60001/LayoutTests/crypto/subtl...
LayoutTests/crypto/subtle/hkdf-deriveBits.html:45:
shouldEvaluateAs("derivedBits.getUint8(0) % 16", 0);
On 2015/01/12 22:35:23, eroman wrote:
> Can you do an equality test instead?
> 
> Should verify that the other bits are correct in the case of deriving partial
> lengths.
> 
> In fact, why put this in its own test file? This could just as easily be
> expressed as another test case in the test-vectors.html.
> 
> I suggest renaming test-vectors.html to simply deriveBits.html, and exercising
> the 0 length derivation, and non-multiple of 8 derivation in there.

shouldEvaluateAs changed to shouldBe.

I have test-vectors as its own file because all it contains are the test vectors
from RFC 5869 (I could rename it rfc-5869-test-vectors). I'd prefer to keep any
other tests for deriveBits out of that file. Also, the format of the tests here
doesn't match that of the test-vectors - basically all the test vectors test
does is check that the output is correct for the given input, while here I'm
doing more specific checks (like checking the length of the returned buffer).

https://codereview.chromium.org/822083003/diff/60001/LayoutTests/crypto/subtl...
File LayoutTests/crypto/subtle/hkdf-deriveKey-expected.txt (right):

https://codereview.chromium.org/822083003/diff/60001/LayoutTests/crypto/subtl...
LayoutTests/crypto/subtle/hkdf-deriveKey-expected.txt:13: error is:
NotSupportedError: The requested operation is unsupported
On 2015/01/12 22:35:23, eroman wrote:
> Why is this not supported error. Shouldn't it be a typeerror or something?

You're correct. This was an issue with the -expected.txt file (I forgot to
update it after updating the test.)

https://codereview.chromium.org/822083003/diff/60001/LayoutTests/crypto/subtl...
File LayoutTests/crypto/subtle/hkdf-exportKey.html (right):

https://codereview.chromium.org/822083003/diff/60001/LayoutTests/crypto/subtl...
LayoutTests/crypto/subtle/hkdf-exportKey.html:20: // set up the test by creating
an HKDF key and an RSA key
On 2015/01/12 22:35:23, eroman wrote:
> This comment is false.

Done.

https://codereview.chromium.org/822083003/diff/60001/LayoutTests/crypto/subtl...
File LayoutTests/crypto/subtle/hkdf-unwrapKey-expected.txt (right):

https://codereview.chromium.org/822083003/diff/60001/LayoutTests/crypto/subtl...
LayoutTests/crypto/subtle/hkdf-unwrapKey-expected.txt:15: Unwrapping an HKDF key
from a format other than raw should fail.
On 2015/01/12 22:35:23, eroman wrote:
> Please describe the action being done, not the expected result.
> (The expectation reads on its own in this -expected.txt file.)

Done.

eroman

https://codereview.chromium.org/822083003/diff/60001/LayoutTests/crypto/subtle/derive-hkdf-keys.html File LayoutTests/crypto/subtle/derive-hkdf-keys.html (right): https://codereview.chromium.org/822083003/diff/60001/LayoutTests/crypto/subtle/derive-hkdf-keys.html#newcode49 LayoutTests/crypto/subtle/derive-hkdf-keys.html:49: return crypto.subtle.deriveKey(hkdfAlgorithm, hkdfKey, "HKDF", true, ['deriveBits']); On 2015/01/13 00:11:15, ...

5 years, 11 months ago (2015-01-13 00:45:35 UTC) #12

https://codereview.chromium.org/822083003/diff/60001/LayoutTests/crypto/subtl...
File LayoutTests/crypto/subtle/derive-hkdf-keys.html (right):

https://codereview.chromium.org/822083003/diff/60001/LayoutTests/crypto/subtl...
LayoutTests/crypto/subtle/derive-hkdf-keys.html:49: return
crypto.subtle.deriveKey(hkdfAlgorithm, hkdfKey, "HKDF", true, ['deriveBits']);
On 2015/01/13 00:11:15, nharper wrote:
> On 2015/01/12 22:35:23, eroman wrote:
> > This test is a weak test IMO:
> >   (1) using an imported ECDH keys rather than generated one would allow
> > verifying the final HKDF key in a more meaningful way (either by structured
> > cloning it or by using it in a derive bits operation)
> > 
> >   (2) this deriveKey() is not the interesting case. The more interesting
case
> > would be if hkdfKey had the 'deriveKey' usages. As it is, this fails because
> of
> > a usages violation. We should have a test (either in addition to or in place
> of)
> > which tests the get key length mismatch.
> 
> 1) I'm now importing 2 ECDH keys and deriving an HKDF key from that. I then
> create another HKDF key from the ECDH secret (pre-computed), and on both HKDF
> keys call deriveBits and verify that they derive the same bits. This is a test
> that calling deriveKey with an ECDH key and a derived key type of HKDF will
> result in importing the correct material into the HKDF key - it doesn't check
> that HKDF is implemented correctly (that's tested by the test vectors test).
> 
> 2) I changed the usage from deriveBits to deriveKey to test what I meant to
> test. I noticed that the test case of derive HKDF key from HKDF key is both in
> this file and hkdf-deriveKey.html - do you have a preference for which one it
> lives in?

Either location is fine as long as there is one source of truth. How about
keeping it in hkdf-derivekey ?

https://codereview.chromium.org/822083003/diff/60001/LayoutTests/crypto/subtl...
File LayoutTests/crypto/subtle/hkdf-deriveBits.html (right):

https://codereview.chromium.org/822083003/diff/60001/LayoutTests/crypto/subtl...
LayoutTests/crypto/subtle/hkdf-deriveBits.html:45:
shouldEvaluateAs("derivedBits.getUint8(0) % 16", 0);
On 2015/01/13 00:11:16, nharper wrote:
> On 2015/01/12 22:35:23, eroman wrote:
> > Can you do an equality test instead?
> > 
> > Should verify that the other bits are correct in the case of deriving
partial
> > lengths.
> > 
> > In fact, why put this in its own test file? This could just as easily be
> > expressed as another test case in the test-vectors.html.
> > 
> > I suggest renaming test-vectors.html to simply deriveBits.html, and
exercising
> > the 0 length derivation, and non-multiple of 8 derivation in there.
> 
> shouldEvaluateAs changed to shouldBe.
> 
> I have test-vectors as its own file because all it contains are the test
vectors
> from RFC 5869 (I could rename it rfc-5869-test-vectors). I'd prefer to keep
any

If you keep the file please rename it as you suggested, rfc-5879-test-vectors
Otherwise with the generic test-vectors name I don't see why we can't put other
stuff in it.

> other tests for deriveBits out of that file. Also, the format of the tests
here
> doesn't match that of the test-vectors - basically all the test vectors test
> does is check that the output is correct for the given input, while here I'm
> doing more specific checks (like checking the length of the returned buffer).

These tests aren't any more specific than in the other one. The other one
compares the output buffer to a known value, and so is implicitly testing the
length as well.

https://codereview.chromium.org/822083003/diff/80001/LayoutTests/crypto/subtl...
File LayoutTests/crypto/subtle/derive-hkdf-keys.html (right):

https://codereview.chromium.org/822083003/diff/80001/LayoutTests/crypto/subtl...
LayoutTests/crypto/subtle/derive-hkdf-keys.html:42: name: "HKDF",
indent by 4.

https://codereview.chromium.org/822083003/diff/80001/LayoutTests/crypto/subtl...
File LayoutTests/crypto/subtle/hkdf-deriveBits-failures.html (right):

https://codereview.chromium.org/822083003/diff/80001/LayoutTests/crypto/subtl...
LayoutTests/crypto/subtle/hkdf-deriveBits-failures.html:50: // bits, so
8160*8=65280 and add 1 to exceed the maximum length.
nit: I would have suggested adding 8 so it is 1 byte larger instead of 1 bit.
But either one is testing something valuable.

https://codereview.chromium.org/822083003/diff/80001/LayoutTests/crypto/subtl...
File LayoutTests/crypto/subtle/hkdf-deriveBits-test-vectors.html (right):

https://codereview.chromium.org/822083003/diff/80001/LayoutTests/crypto/subtl...
LayoutTests/crypto/subtle/hkdf-deriveBits-test-vectors.html:31: length: 82,
What is the purpose of the length parameter, doesn't it duplicate knowledge
already present in the "output"?

Unless this test is modified to include truncations, should remove the length.

https://codereview.chromium.org/822083003/diff/80001/LayoutTests/crypto/subtl...
File LayoutTests/crypto/subtle/hkdf-deriveBits.html (right):

https://codereview.chromium.org/822083003/diff/80001/LayoutTests/crypto/subtl...
LayoutTests/crypto/subtle/hkdf-deriveBits.html:45:
shouldBe("derivedBits.getUint8(0) % 16", "0");
My earlier comment is unresolved. Why not test the exact value rather than just
the last 4 bits?

for instance:

shouldBe("derivedBits.getUint8(0)", 0xE0);

(0xE0 just placeholder for what actual value is supposed to be).

nharper

https://codereview.chromium.org/822083003/diff/80001/LayoutTests/crypto/subtle/derive-hkdf-keys.html File LayoutTests/crypto/subtle/derive-hkdf-keys.html (right): https://codereview.chromium.org/822083003/diff/80001/LayoutTests/crypto/subtle/derive-hkdf-keys.html#newcode42 LayoutTests/crypto/subtle/derive-hkdf-keys.html:42: name: "HKDF", On 2015/01/13 00:45:34, eroman wrote: > indent ...

5 years, 11 months ago (2015-01-13 01:43:26 UTC) #13

eroman

Any update on the other comments? (renaming the file) https://codereview.chromium.org/822083003/diff/80001/LayoutTests/crypto/subtle/hkdf-deriveBits.html File LayoutTests/crypto/subtle/hkdf-deriveBits.html (right): https://codereview.chromium.org/822083003/diff/80001/LayoutTests/crypto/subtle/hkdf-deriveBits.html#newcode45 LayoutTests/crypto/subtle/hkdf-deriveBits.html:45: ...

5 years, 11 months ago (2015-01-14 02:45:11 UTC) #14

nharper

I believe the other comments are addressed in patch sets 7 and 8. https://codereview.chromium.org/822083003/diff/80001/LayoutTests/crypto/subtle/hkdf-deriveBits.html File ...

5 years, 11 months ago (2015-01-14 02:53:42 UTC) #15

commit-bot: I haz the power

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

5 years, 11 months ago (2015-01-14 02:57:02 UTC) #18

commit-bot: I haz the power

5 years, 11 months ago (2015-01-14 04:32:01 UTC) #19

Message was sent while issue was closed.

Committed patchset #8 (id:140001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=188354

Issue 822083003: Webcrypto: add layout tests for HKDF algorithm (Closed)

Description

Patch Set 1 #

Patch Set 2 : #

Patch Set 3 : use correct diffbase #

Patch Set 4 : #

Patch Set 5 : #

Patch Set 6 : #

Patch Set 7 : rename hkdf-deriveBits-test-vectors -> hkdf-deriveBits-rfc5869-test-vectors #

Patch Set 8 : remove duplicate test case #

Messages