[webcrypto] Implement PBKDF2 (blink-side)

Implement PBKDF2 (blink-side)

This CL implements password-based key derivation function 2 (PBKDF2) as described in the WebCrypto spec - https://dvcs.w3.org/hg/webcrypto-api/raw-file/tip/spec/Overview.html#pbkdf2.

The Chromium-side implementation is https://codereview.chromium.org/797723006/

BUG=399099
R=eroman
TESTS=crypto/ Layout tests

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

[xun.sun, 2015-01-07 06:23:07]

PTAL.

[eroman, 2015-01-07 06:48:52]

I only gave this an initial look over tonight (about to leave). Some initial
comments:

(1) Please add tests which test a variety of input lengths, including 0, and
lengths that are not a multiple of 8. In the case of non-multiple of 8 lengths
we want to zero out the unused bits like is currently done for ECDH.

(2) Nick is working on HKDF and there is some overlap when it comes to defining
the key parameters for structured cloning. I suggest seeing what he added in 
https://codereview.chromium.org/789733009/  -- I believe you can re-use the same
"Kdf" key parames (generalizes to HKDF, and PBKDF2).

(3) Layout tests should also include key structured cloning. See for instance
Nick's tests in https://codereview.chromium.org/822083003/

https://codereview.chromium.org/820523003/diff/20001/LayoutTests/crypto/subtl...
File LayoutTests/crypto/subtle/pbkdf2-deriveBits.html (right):

https://codereview.chromium.org/820523003/diff/20001/LayoutTests/crypto/subtl...
LayoutTests/crypto/subtle/pbkdf2-deriveBits.html:29: dkLen: 20,
What is the use of dkLen, it looks like it is always equal to
derived_key_full_length.length in these examples

[xun.sun, 2015-01-09 16:17:50]

https://codereview.chromium.org/820523003/diff/20001/LayoutTests/crypto/subtl...
File LayoutTests/crypto/subtle/pbkdf2-deriveBits.html (right):

https://codereview.chromium.org/820523003/diff/20001/LayoutTests/crypto/subtl...
LayoutTests/crypto/subtle/pbkdf2-deriveBits.html:29: dkLen: 20,
On 2015/01/07 06:48:52, eroman wrote:
> What is the use of dkLen, it looks like it is always equal to
> derived_key_full_length.length in these examples

It is used as key length input to deriveBits() and equal to
derived_key_full_length.length / 2. I think it is clearer to call out the length
than passing derived_key_full_length.length / 2 to deriveBits(). What do you
think?

[xun.sun, 2015-01-09 16:19:32]

On 2015/01/07 06:48:52, eroman wrote:
> I only gave this an initial look over tonight (about to leave). Some initial
> comments:
> 
> (1) Please add tests which test a variety of input lengths, including 0, and
> lengths that are not a multiple of 8. In the case of non-multiple of 8 lengths
> we want to zero out the unused bits like is currently done for ECDH.

Added tests for 0. http://www.w3.org/TR/WebCryptoAPI/#pbkdf2-operations says
non-multiple of 8 lengths should be rejected. Is this a spec bug?

> 
> (2) Nick is working on HKDF and there is some overlap when it comes to
defining
> the key parameters for structured cloning. I suggest seeing what he added in 
> https://codereview.chromium.org/789733009/  -- I believe you can re-use the
same
> "Kdf" key parames (generalizes to HKDF, and PBKDF2).
> 
> (3) Layout tests should also include key structured cloning. See for instance
> Nick's tests in https://codereview.chromium.org/822083003/
> 

I suppose I will need to rebase after Nick's CLs land. I will add structure
cloning implementation and tests then.

>
https://codereview.chromium.org/820523003/diff/20001/LayoutTests/crypto/subtl...
> File LayoutTests/crypto/subtle/pbkdf2-deriveBits.html (right):
> 
>
https://codereview.chromium.org/820523003/diff/20001/LayoutTests/crypto/subtl...
> LayoutTests/crypto/subtle/pbkdf2-deriveBits.html:29: dkLen: 20,
> What is the use of dkLen, it looks like it is always equal to
> derived_key_full_length.length in these examples

[eroman, 2015-01-09 19:59:09]

On Fri, Jan 9, 2015 at 8:19 AM, <xun.sun@intel.com> wrote:

> On 2015/01/07 06:48:52, eroman wrote:
>
>> I only gave this an initial look over tonight (about to leave). Some
>> initial
>> comments:
>>
>
>  (1) Please add tests which test a variety of input lengths, including 0,
>> and
>> lengths that are not a multiple of 8. In the case of non-multiple of 8
>> lengths
>> we want to zero out the unused bits like is currently done for ECDH.
>>
>
> Added tests for 0. http://www.w3.org/TR/WebCryptoAPI/#pbkdf2-operations
> says
> non-multiple of 8 lengths should be rejected. Is this a spec bug?


You are correct throwing an error is what the spec says, and what your
changelist should implement.
This is filed upstream as:
https://www.w3.org/Bugs/Public/show_bug.cgi?id=27403


>
>
>
>  (2) Nick is working on HKDF and there is some overlap when it comes to
>>
> defining
>
>> the key parameters for structured cloning. I suggest seeing what he added
>> in
>> https://codereview.chromium.org/789733009/  -- I believe you can re-use
>> the
>>
> same
>
>> "Kdf" key parames (generalizes to HKDF, and PBKDF2).
>>
>
>  (3) Layout tests should also include key structured cloning. See for
>> instance
>> Nick's tests in https://codereview.chromium.org/822083003/
>>
>
>
> I suppose I will need to rebase after Nick's CLs land. I will add structure
> cloning implementation and tests then.


Sounds good. Nick's CL should introduce the structured cloning support you
need, so then you will just need to add some tests.


>
>
>
> https://codereview.chromium.org/820523003/diff/20001/
> LayoutTests/crypto/subtle/pbkdf2-deriveBits.html
>
>> File LayoutTests/crypto/subtle/pbkdf2-deriveBits.html (right):
>>
>
>
> https://codereview.chromium.org/820523003/diff/20001/
> LayoutTests/crypto/subtle/pbkdf2-deriveBits.html#newcode29
>
>> LayoutTests/crypto/subtle/pbkdf2-deriveBits.html:29: dkLen: 20,
>> What is the use of dkLen, it looks like it is always equal to
>> derived_key_full_length.length in these examples
>>
>
>
> https://codereview.chromium.org/820523003/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to blink-reviews+unsubscribe@chromium.org.

[xun.sun, 2015-01-13 19:02:56]

On 2015/01/09 19:59:09, eroman wrote:
> On Fri, Jan 9, 2015 at 8:19 AM, <mailto:xun.sun@intel.com> wrote:
> 
> > On 2015/01/07 06:48:52, eroman wrote:
> >
> >> I only gave this an initial look over tonight (about to leave). Some
> >> initial
> >> comments:
> >>
> >
> >  (1) Please add tests which test a variety of input lengths, including 0,
> >> and
> >> lengths that are not a multiple of 8. In the case of non-multiple of 8
> >> lengths
> >> we want to zero out the unused bits like is currently done for ECDH.
> >>
> >
> > Added tests for 0. http://www.w3.org/TR/WebCryptoAPI/#pbkdf2-operations
> > says
> > non-multiple of 8 lengths should be rejected. Is this a spec bug?
> 
> 
> You are correct throwing an error is what the spec says, and what your
> changelist should implement.
> This is filed upstream as:
> https://www.w3.org/Bugs/Public/show_bug.cgi?id=27403
> 
> 
> >
> >
> >
> >  (2) Nick is working on HKDF and there is some overlap when it comes to
> >>
> > defining
> >
> >> the key parameters for structured cloning. I suggest seeing what he added
> >> in
> >> https://codereview.chromium.org/789733009/  -- I believe you can re-use
> >> the
> >>
> > same
> >
> >> "Kdf" key parames (generalizes to HKDF, and PBKDF2).
> >>
> >
> >  (3) Layout tests should also include key structured cloning. See for
> >> instance
> >> Nick's tests in https://codereview.chromium.org/822083003/
> >>
> >
> >
> > I suppose I will need to rebase after Nick's CLs land. I will add structure
> > cloning implementation and tests then.
> 
> 
> Sounds good. Nick's CL should introduce the structured cloning support you
> need, so then you will just need to add some tests.

Rebased and added structured cloning tests in clone-pbkdf2Key.html.

> 
> 
> >
> >
> >
> > https://codereview.chromium.org/820523003/diff/20001/
> > LayoutTests/crypto/subtle/pbkdf2-deriveBits.html
> >
> >> File LayoutTests/crypto/subtle/pbkdf2-deriveBits.html (right):
> >>
> >
> >
> > https://codereview.chromium.org/820523003/diff/20001/
> > LayoutTests/crypto/subtle/pbkdf2-deriveBits.html#newcode29
> >
> >> LayoutTests/crypto/subtle/pbkdf2-deriveBits.html:29: dkLen: 20,
> >> What is the use of dkLen, it looks like it is always equal to
> >> derived_key_full_length.length in these examples
> >>
> >
> >
> > https://codereview.chromium.org/820523003/
> >
> 
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:blink-reviews+unsubscribe@chromium.org.

[xun.sun, 2015-01-13 19:06:01]

On 2015/01/13 19:02:56, xun.sun wrote:
> On 2015/01/09 19:59:09, eroman wrote:
> > On Fri, Jan 9, 2015 at 8:19 AM, <mailto:xun.sun@intel.com> wrote:
> > 
> > > On 2015/01/07 06:48:52, eroman wrote:
> > >
> > >> I only gave this an initial look over tonight (about to leave). Some
> > >> initial
> > >> comments:
> > >>
> > >
> > >  (1) Please add tests which test a variety of input lengths, including 0,
> > >> and
> > >> lengths that are not a multiple of 8. In the case of non-multiple of 8
> > >> lengths
> > >> we want to zero out the unused bits like is currently done for ECDH.
> > >>
> > >
> > > Added tests for 0. http://www.w3.org/TR/WebCryptoAPI/#pbkdf2-operations
> > > says
> > > non-multiple of 8 lengths should be rejected. Is this a spec bug?
> > 
> > 
> > You are correct throwing an error is what the spec says, and what your
> > changelist should implement.
> > This is filed upstream as:
> > https://www.w3.org/Bugs/Public/show_bug.cgi?id=27403
> > 
> > 
> > >
> > >
> > >
> > >  (2) Nick is working on HKDF and there is some overlap when it comes to
> > >>
> > > defining
> > >
> > >> the key parameters for structured cloning. I suggest seeing what he added
> > >> in
> > >> https://codereview.chromium.org/789733009/  -- I believe you can re-use
> > >> the
> > >>
> > > same
> > >
> > >> "Kdf" key parames (generalizes to HKDF, and PBKDF2).
> > >>
> > >
> > >  (3) Layout tests should also include key structured cloning. See for
> > >> instance
> > >> Nick's tests in https://codereview.chromium.org/822083003/
> > >>
> > >
> > >
> > > I suppose I will need to rebase after Nick's CLs land. I will add
structure
> > > cloning implementation and tests then.
> > 
> > 
> > Sounds good. Nick's CL should introduce the structured cloning support you
> > need, so then you will just need to add some tests.
> 
> Rebased and added structured cloning tests in clone-pbkdf2Key.html.
> 
> > 
> > 
> > >
> > >
> > >
> > > https://codereview.chromium.org/820523003/diff/20001/
> > > LayoutTests/crypto/subtle/pbkdf2-deriveBits.html
> > >
> > >> File LayoutTests/crypto/subtle/pbkdf2-deriveBits.html (right):
> > >>
> > >
> > >
> > > https://codereview.chromium.org/820523003/diff/20001/
> > > LayoutTests/crypto/subtle/pbkdf2-deriveBits.html#newcode29
> > >
> > >> LayoutTests/crypto/subtle/pbkdf2-deriveBits.html:29: dkLen: 20,
> > >> What is the use of dkLen, it looks like it is always equal to
> > >> derived_key_full_length.length in these examples
> > >>
> > >
> > >
> > > https://codereview.chromium.org/820523003/
> > >
> > 
> > To unsubscribe from this group and stop receiving emails from it, send an
> email
> > to mailto:blink-reviews+unsubscribe@chromium.org.

PTAL.

[xun.sun, 2015-01-13 19:16:30]

https://codereview.chromium.org/820523003/diff/60001/Source/platform/exported...
File Source/platform/exported/WebCryptoAlgorithm.cpp (right):

https://codereview.chromium.org/820523003/diff/60001/Source/platform/exported...
Source/platform/exported/WebCryptoAlgorithm.cpp:327: if (idInt >=
WTF_ARRAY_LENGTH(algorithmIdToInfo))
I made this change because I'm seeing the following build error very similar to
the ones in https://code.google.com/p/chromium/issues/detail?id=163104:

../../third_party/WebKit/Source/platform/exported/WebCryptoAlgorithm.cpp:327:11:
error: comparison of constant 16 with expression of type
'blink::WebCryptoAlgorithmId' is always false
[-Werror,-Wtautological-constant-out-of-range-compare]
    if (id>= WTF_ARRAY_LENGTH(algorithmIdToInfo))
        ~~^  ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

As soon as the list of AlgorithmId's grows to 16 this error starts to appear.
Not sure why adding an Algorithmid would make the difference.

[eroman, 2015-01-14 21:12:23]

Please update the CL description to:

(1) Link to the spec for PKBDF2:
https://dvcs.w3.org/hg/webcrypto-api/raw-file/tip/spec/Overview.html#pbkdf2

(2) Make the description more generic, like:
"Implement PBKDF2 (blink-side)"

There are more tests I would like to see for PBKDF2. Notably:

 * Test using a hash other than SHA-1
 * Test using a zero-length key
 * Test using a non-ASCII key
 * Test using empty salt

These tests can either be added to the current changelist (preferred) or
submitted as a follow-up CL after this has landed.

Other then that looksgood and I expect to sign off on this next pass.

https://codereview.chromium.org/820523003/diff/60001/LayoutTests/crypto/subtl...
File LayoutTests/crypto/subtle/clone-pbkdf2Key.html (right):

https://codereview.chromium.org/820523003/diff/60001/LayoutTests/crypto/subtl...
LayoutTests/crypto/subtle/clone-pbkdf2Key.html:22: var k256BitData =
"00112233445546778899aabbccddeeff000102030405060708090a0b0c0d0e0f";
Could you change the data lengths to some more interesting values?

  * 0 bytes
  * 1 byte
  * 11 bytes (or something other than 16 and 32)

I suggest this because the PBKDF2 key can be arbitrary length so a bit more
variety in the key lengths makes for a good test.

https://codereview.chromium.org/820523003/diff/60001/LayoutTests/crypto/subtl...
File LayoutTests/crypto/subtle/pbkdf2-deriveBits-failures-expected.txt (right):

https://codereview.chromium.org/820523003/diff/60001/LayoutTests/crypto/subtl...
LayoutTests/crypto/subtle/pbkdf2-deriveBits-failures-expected.txt:8: Deriving
100 bit...
bit --> bits

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

https://codereview.chromium.org/820523003/diff/60001/LayoutTests/crypto/subtl...
LayoutTests/crypto/subtle/pbkdf2-deriveBits.html:52: //  16777216 iterations
would timeout
Delete this commented out code (since we do not plan on running it).

https://codereview.chromium.org/820523003/diff/60001/LayoutTests/crypto/subtl...
LayoutTests/crypto/subtle/pbkdf2-deriveBits.html:68: derived_key_full_length:
"3d2eec4fe41c849b80c8d83662c0e44a8b291a964cf2f07038"
naming: I suggest either "derived_key" or "dk".
The "full_length" part doesn't add value in my opinion, since in every case it
matches dkLen

https://codereview.chromium.org/820523003/diff/60001/LayoutTests/crypto/subtl...
File LayoutTests/crypto/subtle/pbkdf2-deriveKey-aes.html (right):

https://codereview.chromium.org/820523003/diff/60001/LayoutTests/crypto/subtl...
LayoutTests/crypto/subtle/pbkdf2-deriveKey-aes.html:52: //  16777216 iterations
would timeout
Delete this testcase

https://codereview.chromium.org/820523003/diff/60001/Source/modules/crypto/No...
File Source/modules/crypto/NormalizeAlgorithm.cpp (right):

https://codereview.chromium.org/820523003/diff/60001/Source/modules/crypto/No...
Source/modules/crypto/NormalizeAlgorithm.cpp:762: //     dictionary Pbkdf2Params
: Algorithm {
Add a newline separating this from the previous line. Also add a line:
// Defined by the WebCrypto spec as:
To match the other parameter parsing functions

https://codereview.chromium.org/820523003/diff/60001/Source/platform/exported...
File Source/platform/exported/WebCryptoAlgorithm.cpp (right):

https://codereview.chromium.org/820523003/diff/60001/Source/platform/exported...
Source/platform/exported/WebCryptoAlgorithm.cpp:327: if (idInt >=
WTF_ARRAY_LENGTH(algorithmIdToInfo))
On 2015/01/13 19:16:29, xun.sun wrote:
> I made this change because I'm seeing the following build error very similar
to
> the ones in https://code.google.com/p/chromium/issues/detail?id=163104:
> 
>
../../third_party/WebKit/Source/platform/exported/WebCryptoAlgorithm.cpp:327:11:
> error: comparison of constant 16 with expression of type
> 'blink::WebCryptoAlgorithmId' is always false
> [-Werror,-Wtautological-constant-out-of-range-compare]
>     if (id>= WTF_ARRAY_LENGTH(algorithmIdToInfo))
>         ~~^  ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> 
> As soon as the list of AlgorithmId's grows to 16 this error starts to appear.
> Not sure why adding an Algorithmid would make the difference.

Not sure why it would do that.

https://codereview.chromium.org/820523003/diff/60001/public/platform/WebCrypt...
File public/platform/WebCryptoAlgorithmParams.h (right):

https://codereview.chromium.org/820523003/diff/60001/public/platform/WebCrypt...
public/platform/WebCryptoAlgorithmParams.h:365: explicit
WebCryptoPbkdf2Params(const WebCryptoAlgorithm& hash, const unsigned char* salt,
remove "explicit" (we only use that for single-parameter constructors)

https://codereview.chromium.org/820523003/diff/60001/public/platform/WebCrypt...
public/platform/WebCryptoAlgorithmParams.h:366: unsigned saltLength, unsigned
long iterations)
unsigned long --> unsigned int

https://codereview.chromium.org/820523003/diff/60001/public/platform/WebCrypt...
public/platform/WebCryptoAlgorithmParams.h:376: unsigned long iterations() const
{ return m_iterations; }
unsigned long --> unsigned int

(unsigned long in WebIDL is a 32-bit value. Blink compiles with 32-bit ints on
all platforms, whereas long could be 64-bit depending on the platform).

https://codereview.chromium.org/820523003/diff/60001/public/platform/WebCrypt...
public/platform/WebCryptoAlgorithmParams.h:380: const unsigned long
m_iterations;
unsigned int

[xun.sun, 2015-01-15 17:13:29]

https://codereview.chromium.org/820523003/diff/60001/LayoutTests/crypto/subtl...
File LayoutTests/crypto/subtle/clone-pbkdf2Key.html (right):

https://codereview.chromium.org/820523003/diff/60001/LayoutTests/crypto/subtl...
LayoutTests/crypto/subtle/clone-pbkdf2Key.html:22: var k256BitData =
"00112233445546778899aabbccddeeff000102030405060708090a0b0c0d0e0f";
On 2015/01/14 21:12:22, eroman wrote:
> Could you change the data lengths to some more interesting values?
> 
>   * 0 bytes
>   * 1 byte
>   * 11 bytes (or something other than 16 and 32)
> 
> I suggest this because the PBKDF2 key can be arbitrary length so a bit more
> variety in the key lengths makes for a good test.

Done. Now testing with passwords of 1 byte, 8 bytes, and 11 bytes. I'm a bit
hesitant with 0 bytes because I think empty passwords don't make sense but the
spec does not request to reject them?

https://codereview.chromium.org/820523003/diff/60001/LayoutTests/crypto/subtl...
File LayoutTests/crypto/subtle/pbkdf2-deriveBits-failures-expected.txt (right):

https://codereview.chromium.org/820523003/diff/60001/LayoutTests/crypto/subtl...
LayoutTests/crypto/subtle/pbkdf2-deriveBits-failures-expected.txt:8: Deriving
100 bit...
On 2015/01/14 21:12:22, eroman wrote:
> bit --> bits

Done.

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

https://codereview.chromium.org/820523003/diff/60001/LayoutTests/crypto/subtl...
LayoutTests/crypto/subtle/pbkdf2-deriveBits.html:52: //  16777216 iterations
would timeout
On 2015/01/14 21:12:22, eroman wrote:
> Delete this commented out code (since we do not plan on running it).

Done.

https://codereview.chromium.org/820523003/diff/60001/LayoutTests/crypto/subtl...
LayoutTests/crypto/subtle/pbkdf2-deriveBits.html:68: derived_key_full_length:
"3d2eec4fe41c849b80c8d83662c0e44a8b291a964cf2f07038"
On 2015/01/14 21:12:22, eroman wrote:
> naming: I suggest either "derived_key" or "dk".
> The "full_length" part doesn't add value in my opinion, since in every case it
> matches dkLen

Done.

https://codereview.chromium.org/820523003/diff/60001/LayoutTests/crypto/subtl...
File LayoutTests/crypto/subtle/pbkdf2-deriveKey-aes.html (right):

https://codereview.chromium.org/820523003/diff/60001/LayoutTests/crypto/subtl...
LayoutTests/crypto/subtle/pbkdf2-deriveKey-aes.html:52: //  16777216 iterations
would timeout
On 2015/01/14 21:12:22, eroman wrote:
> Delete this testcase

Done.

https://codereview.chromium.org/820523003/diff/60001/Source/modules/crypto/No...
File Source/modules/crypto/NormalizeAlgorithm.cpp (right):

https://codereview.chromium.org/820523003/diff/60001/Source/modules/crypto/No...
Source/modules/crypto/NormalizeAlgorithm.cpp:762: //     dictionary Pbkdf2Params
: Algorithm {
On 2015/01/14 21:12:22, eroman wrote:
> Add a newline separating this from the previous line. Also add a line:
> // Defined by the WebCrypto spec as:
> To match the other parameter parsing functions

Done.

https://codereview.chromium.org/820523003/diff/60001/public/platform/WebCrypt...
File public/platform/WebCryptoAlgorithmParams.h (right):

https://codereview.chromium.org/820523003/diff/60001/public/platform/WebCrypt...
public/platform/WebCryptoAlgorithmParams.h:365: explicit
WebCryptoPbkdf2Params(const WebCryptoAlgorithm& hash, const unsigned char* salt,
On 2015/01/14 21:12:22, eroman wrote:
> remove "explicit" (we only use that for single-parameter constructors)

Done.

https://codereview.chromium.org/820523003/diff/60001/public/platform/WebCrypt...
public/platform/WebCryptoAlgorithmParams.h:366: unsigned saltLength, unsigned
long iterations)
On 2015/01/14 21:12:22, eroman wrote:
> unsigned long --> unsigned int

Done.

https://codereview.chromium.org/820523003/diff/60001/public/platform/WebCrypt...
public/platform/WebCryptoAlgorithmParams.h:376: unsigned long iterations() const
{ return m_iterations; }
On 2015/01/14 21:12:22, eroman wrote:
> unsigned long --> unsigned int
> 
> (unsigned long in WebIDL is a 32-bit value. Blink compiles with 32-bit ints on
> all platforms, whereas long could be 64-bit depending on the platform).

Done.

https://codereview.chromium.org/820523003/diff/60001/public/platform/WebCrypt...
public/platform/WebCryptoAlgorithmParams.h:380: const unsigned long
m_iterations;
On 2015/01/14 21:12:22, eroman wrote:
> unsigned int

Done.

[xun.sun, 2015-01-15 17:17:13]

On 2015/01/14 21:12:23, eroman wrote:
> Please update the CL description to:
> 
> (1) Link to the spec for PKBDF2:
> https://dvcs.w3.org/hg/webcrypto-api/raw-file/tip/spec/Overview.html#pbkdf2
> 
> (2) Make the description more generic, like:
> "Implement PBKDF2 (blink-side)"
> 

Done.

> There are more tests I would like to see for PBKDF2. Notably:
> 
>  * Test using a hash other than SHA-1
>  * Test using a zero-length key
>  * Test using a non-ASCII key
>  * Test using empty salt
> 

Done. I added several tests in pbkdf2-deriveBits.html to cover SHA-256, SHA-512,
non-ascii password, and empty salt. I think zero-length password should be
rejected but the spec does not seem to require this. Can I follow up on this
later?

> These tests can either be added to the current changelist (preferred) or
> submitted as a follow-up CL after this has landed.
> 
> Other then that looksgood and I expect to sign off on this next pass.
> 
>
https://codereview.chromium.org/820523003/diff/60001/LayoutTests/crypto/subtl...
> File LayoutTests/crypto/subtle/clone-pbkdf2Key.html (right):
> 
>
https://codereview.chromium.org/820523003/diff/60001/LayoutTests/crypto/subtl...
> LayoutTests/crypto/subtle/clone-pbkdf2Key.html:22: var k256BitData =
> "00112233445546778899aabbccddeeff000102030405060708090a0b0c0d0e0f";
> Could you change the data lengths to some more interesting values?
> 
>   * 0 bytes
>   * 1 byte
>   * 11 bytes (or something other than 16 and 32)
> 
> I suggest this because the PBKDF2 key can be arbitrary length so a bit more
> variety in the key lengths makes for a good test.
> 
>
https://codereview.chromium.org/820523003/diff/60001/LayoutTests/crypto/subtl...
> File LayoutTests/crypto/subtle/pbkdf2-deriveBits-failures-expected.txt
(right):
> 
>
https://codereview.chromium.org/820523003/diff/60001/LayoutTests/crypto/subtl...
> LayoutTests/crypto/subtle/pbkdf2-deriveBits-failures-expected.txt:8: Deriving
> 100 bit...
> bit --> bits
> 
>
https://codereview.chromium.org/820523003/diff/60001/LayoutTests/crypto/subtl...
> File LayoutTests/crypto/subtle/pbkdf2-deriveBits.html (right):
> 
>
https://codereview.chromium.org/820523003/diff/60001/LayoutTests/crypto/subtl...
> LayoutTests/crypto/subtle/pbkdf2-deriveBits.html:52: //  16777216 iterations
> would timeout
> Delete this commented out code (since we do not plan on running it).
> 
>
https://codereview.chromium.org/820523003/diff/60001/LayoutTests/crypto/subtl...
> LayoutTests/crypto/subtle/pbkdf2-deriveBits.html:68: derived_key_full_length:
> "3d2eec4fe41c849b80c8d83662c0e44a8b291a964cf2f07038"
> naming: I suggest either "derived_key" or "dk".
> The "full_length" part doesn't add value in my opinion, since in every case it
> matches dkLen
> 
>
https://codereview.chromium.org/820523003/diff/60001/LayoutTests/crypto/subtl...
> File LayoutTests/crypto/subtle/pbkdf2-deriveKey-aes.html (right):
> 
>
https://codereview.chromium.org/820523003/diff/60001/LayoutTests/crypto/subtl...
> LayoutTests/crypto/subtle/pbkdf2-deriveKey-aes.html:52: //  16777216
iterations
> would timeout
> Delete this testcase
> 
>
https://codereview.chromium.org/820523003/diff/60001/Source/modules/crypto/No...
> File Source/modules/crypto/NormalizeAlgorithm.cpp (right):
> 
>
https://codereview.chromium.org/820523003/diff/60001/Source/modules/crypto/No...
> Source/modules/crypto/NormalizeAlgorithm.cpp:762: //     dictionary
Pbkdf2Params
> : Algorithm {
> Add a newline separating this from the previous line. Also add a line:
> // Defined by the WebCrypto spec as:
> To match the other parameter parsing functions
> 
>
https://codereview.chromium.org/820523003/diff/60001/Source/platform/exported...
> File Source/platform/exported/WebCryptoAlgorithm.cpp (right):
> 
>
https://codereview.chromium.org/820523003/diff/60001/Source/platform/exported...
> Source/platform/exported/WebCryptoAlgorithm.cpp:327: if (idInt >=
> WTF_ARRAY_LENGTH(algorithmIdToInfo))
> On 2015/01/13 19:16:29, xun.sun wrote:
> > I made this change because I'm seeing the following build error very similar
> to
> > the ones in https://code.google.com/p/chromium/issues/detail?id=163104:
> > 
> >
>
../../third_party/WebKit/Source/platform/exported/WebCryptoAlgorithm.cpp:327:11:
> > error: comparison of constant 16 with expression of type
> > 'blink::WebCryptoAlgorithmId' is always false
> > [-Werror,-Wtautological-constant-out-of-range-compare]
> >     if (id>= WTF_ARRAY_LENGTH(algorithmIdToInfo))
> >         ~~^  ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> > 
> > As soon as the list of AlgorithmId's grows to 16 this error starts to
appear.
> > Not sure why adding an Algorithmid would make the difference.
> 
> Not sure why it would do that.
> 
>
https://codereview.chromium.org/820523003/diff/60001/public/platform/WebCrypt...
> File public/platform/WebCryptoAlgorithmParams.h (right):
> 
>
https://codereview.chromium.org/820523003/diff/60001/public/platform/WebCrypt...
> public/platform/WebCryptoAlgorithmParams.h:365: explicit
> WebCryptoPbkdf2Params(const WebCryptoAlgorithm& hash, const unsigned char*
salt,
> remove "explicit" (we only use that for single-parameter constructors)
> 
>
https://codereview.chromium.org/820523003/diff/60001/public/platform/WebCrypt...
> public/platform/WebCryptoAlgorithmParams.h:366: unsigned saltLength, unsigned
> long iterations)
> unsigned long --> unsigned int
> 
>
https://codereview.chromium.org/820523003/diff/60001/public/platform/WebCrypt...
> public/platform/WebCryptoAlgorithmParams.h:376: unsigned long iterations()
const
> { return m_iterations; }
> unsigned long --> unsigned int
> 
> (unsigned long in WebIDL is a 32-bit value. Blink compiles with 32-bit ints on
> all platforms, whereas long could be 64-bit depending on the platform).
> 
>
https://codereview.chromium.org/820523003/diff/60001/public/platform/WebCrypt...
> public/platform/WebCryptoAlgorithmParams.h:380: const unsigned long
> m_iterations;
> unsigned int

PTAL

[eroman, 2015-01-15 22:28:58]

Patchset 7 LGTM after the following comments are adressed

https://codereview.chromium.org/820523003/diff/60001/LayoutTests/crypto/subtl...
File LayoutTests/crypto/subtle/clone-pbkdf2Key.html (right):

https://codereview.chromium.org/820523003/diff/60001/LayoutTests/crypto/subtl...
LayoutTests/crypto/subtle/clone-pbkdf2Key.html:22: var k256BitData =
"00112233445546778899aabbccddeeff000102030405060708090a0b0c0d0e0f";
On 2015/01/15 17:13:29, xun.sun wrote:
> On 2015/01/14 21:12:22, eroman wrote:
> > Could you change the data lengths to some more interesting values?
> > 
> >   * 0 bytes
> >   * 1 byte
> >   * 11 bytes (or something other than 16 and 32)
> > 
> > I suggest this because the PBKDF2 key can be arbitrary length so a bit more
> > variety in the key lengths makes for a good test.
> 
> Done. Now testing with passwords of 1 byte, 8 bytes, and 11 bytes. I'm a bit
> hesitant with 0 bytes because I think empty passwords don't make sense but the
> spec does not request to reject them?

It should be easy to add an empty password case, can you add it to this CL? If
you are having problems then it is OK to do so in a follow-up CL.

Independent of whether empty length is allowed or rejected by the spec, it is
important to have a test for it, since empty input is the sort of boundary case
that has a good chance of causing problems in the implementation.

Since these inputs are user-controlled, we need to be very careful that they
cannot lead to problems. Historically crypto library haven't been hardened
against invalid inputs, so testing these cases is important.

https://codereview.chromium.org/820523003/diff/100001/LayoutTests/TestExpecta...
File LayoutTests/TestExpectations (right):

https://codereview.chromium.org/820523003/diff/100001/LayoutTests/TestExpecta...
LayoutTests/TestExpectations:631: # These PBKDF2 tests are failing without the
Chromium implementation
Reword this to explain this is a temporary situation:

# These tests are temporary failures until the chromium-side of PBKDF2 lands

https://codereview.chromium.org/820523003/diff/100001/LayoutTests/crypto/subt...
File LayoutTests/crypto/subtle/clone-pbkdf2Key.html (right):

https://codereview.chromium.org/820523003/diff/100001/LayoutTests/crypto/subt...
LayoutTests/crypto/subtle/clone-pbkdf2Key.html:16: // Tests the 18 permutations
of keys generated by:
27 permutations now

https://codereview.chromium.org/820523003/diff/100001/LayoutTests/crypto/subt...
LayoutTests/crypto/subtle/clone-pbkdf2Key.html:73:
kPossibleKeyUsages.forEach(function(keyUsages) {
no need for his extra indentation

https://codereview.chromium.org/820523003/diff/100001/LayoutTests/crypto/subt...
File LayoutTests/crypto/subtle/pbkdf2-deriveBits.html (right):

https://codereview.chromium.org/820523003/diff/100001/LayoutTests/crypto/subt...
LayoutTests/crypto/subtle/pbkdf2-deriveBits.html:21: //  
https://tools.ietf.org/html/rfc6070
Not all of the test vectors are from here, please make the comment clearer on
which tests are from rfc6070

https://codereview.chromium.org/820523003/diff/100001/LayoutTests/crypto/subt...
LayoutTests/crypto/subtle/pbkdf2-deriveBits.html:70: // Non-ascii password
From here on the tests are not from rfc6070.

Either indicate as such, or move the rfc6070 tests to their own file.

HKDF tests use the naming scheme:
  hkdf-deriveBits-rfc5869-test-vectors.html

So you could consider moving the rfc6070 tests to a similarly named file and
then keep these ones here.

https://codereview.chromium.org/820523003/diff/100001/LayoutTests/crypto/subt...
LayoutTests/crypto/subtle/pbkdf2-deriveBits.html:108: },
Please add the empty password case too.

[xun.sun, 2015-01-16 03:36:59]

https://codereview.chromium.org/820523003/diff/100001/LayoutTests/TestExpecta...
File LayoutTests/TestExpectations (right):

https://codereview.chromium.org/820523003/diff/100001/LayoutTests/TestExpecta...
LayoutTests/TestExpectations:631: # These PBKDF2 tests are failing without the
Chromium implementation
On 2015/01/15 22:28:57, eroman wrote:
> Reword this to explain this is a temporary situation:
> 
> # These tests are temporary failures until the chromium-side of PBKDF2 lands

Done.

https://codereview.chromium.org/820523003/diff/100001/LayoutTests/crypto/subt...
File LayoutTests/crypto/subtle/clone-pbkdf2Key.html (right):

https://codereview.chromium.org/820523003/diff/100001/LayoutTests/crypto/subt...
LayoutTests/crypto/subtle/clone-pbkdf2Key.html:73:
kPossibleKeyUsages.forEach(function(keyUsages) {
On 2015/01/15 22:28:57, eroman wrote:
> no need for his extra indentation

Done.

https://codereview.chromium.org/820523003/diff/100001/LayoutTests/crypto/subt...
File LayoutTests/crypto/subtle/pbkdf2-deriveBits.html (right):

https://codereview.chromium.org/820523003/diff/100001/LayoutTests/crypto/subt...
LayoutTests/crypto/subtle/pbkdf2-deriveBits.html:21: //  
https://tools.ietf.org/html/rfc6070
On 2015/01/15 22:28:57, eroman wrote:
> Not all of the test vectors are from here, please make the comment clearer on
> which tests are from rfc6070

Done. Separated the tests into 2 files, one for rfc6070, and one for the others.

https://codereview.chromium.org/820523003/diff/100001/LayoutTests/crypto/subt...
LayoutTests/crypto/subtle/pbkdf2-deriveBits.html:70: // Non-ascii password
On 2015/01/15 22:28:57, eroman wrote:
> From here on the tests are not from rfc6070.
> 
> Either indicate as such, or move the rfc6070 tests to their own file.
> 
> HKDF tests use the naming scheme:
>   hkdf-deriveBits-rfc5869-test-vectors.html
> 
> So you could consider moving the rfc6070 tests to a similarly named file and
> then keep these ones here.

Done.

https://codereview.chromium.org/820523003/diff/100001/LayoutTests/crypto/subt...
LayoutTests/crypto/subtle/pbkdf2-deriveBits.html:108: },
On 2015/01/15 22:28:58, eroman wrote:
> Please add the empty password case too.

Done. Added in pbkdf2-deriveBits-empty-password.html.

The keys derived from empty password turn out to be random. Is this known /
expected?

[xun.sun, 2015-01-16 03:42:10]

On 2015/01/15 22:28:58, eroman wrote:
> Patchset 7 LGTM after the following comments are adressed
> 
>
https://codereview.chromium.org/820523003/diff/60001/LayoutTests/crypto/subtl...
> File LayoutTests/crypto/subtle/clone-pbkdf2Key.html (right):
> 
>
https://codereview.chromium.org/820523003/diff/60001/LayoutTests/crypto/subtl...
> LayoutTests/crypto/subtle/clone-pbkdf2Key.html:22: var k256BitData =
> "00112233445546778899aabbccddeeff000102030405060708090a0b0c0d0e0f";
> On 2015/01/15 17:13:29, xun.sun wrote:
> > On 2015/01/14 21:12:22, eroman wrote:
> > > Could you change the data lengths to some more interesting values?
> > > 
> > >   * 0 bytes
> > >   * 1 byte
> > >   * 11 bytes (or something other than 16 and 32)
> > > 
> > > I suggest this because the PBKDF2 key can be arbitrary length so a bit
more
> > > variety in the key lengths makes for a good test.
> > 
> > Done. Now testing with passwords of 1 byte, 8 bytes, and 11 bytes. I'm a bit
> > hesitant with 0 bytes because I think empty passwords don't make sense but
the
> > spec does not request to reject them?
> 
> It should be easy to add an empty password case, can you add it to this CL? If
> you are having problems then it is OK to do so in a follow-up CL.
> 
> Independent of whether empty length is allowed or rejected by the spec, it is
> important to have a test for it, since empty input is the sort of boundary
case
> that has a good chance of causing problems in the implementation.
> 
> Since these inputs are user-controlled, we need to be very careful that they
> cannot lead to problems. Historically crypto library haven't been hardened
> against invalid inputs, so testing these cases is important.
> 

Added pbkdf2-deriveBits-empty-password.html.

PTAL.

>
https://codereview.chromium.org/820523003/diff/100001/LayoutTests/TestExpecta...
> File LayoutTests/TestExpectations (right):
> 
>
https://codereview.chromium.org/820523003/diff/100001/LayoutTests/TestExpecta...
> LayoutTests/TestExpectations:631: # These PBKDF2 tests are failing without the
> Chromium implementation
> Reword this to explain this is a temporary situation:
> 
> # These tests are temporary failures until the chromium-side of PBKDF2 lands
> 
>
https://codereview.chromium.org/820523003/diff/100001/LayoutTests/crypto/subt...
> File LayoutTests/crypto/subtle/clone-pbkdf2Key.html (right):
> 
>
https://codereview.chromium.org/820523003/diff/100001/LayoutTests/crypto/subt...
> LayoutTests/crypto/subtle/clone-pbkdf2Key.html:16: // Tests the 18
permutations
> of keys generated by:
> 27 permutations now
> 
>
https://codereview.chromium.org/820523003/diff/100001/LayoutTests/crypto/subt...
> LayoutTests/crypto/subtle/clone-pbkdf2Key.html:73:
> kPossibleKeyUsages.forEach(function(keyUsages) {
> no need for his extra indentation
> 
>
https://codereview.chromium.org/820523003/diff/100001/LayoutTests/crypto/subt...
> File LayoutTests/crypto/subtle/pbkdf2-deriveBits.html (right):
> 
>
https://codereview.chromium.org/820523003/diff/100001/LayoutTests/crypto/subt...
> LayoutTests/crypto/subtle/pbkdf2-deriveBits.html:21: //  
> https://tools.ietf.org/html/rfc6070
> Not all of the test vectors are from here, please make the comment clearer on
> which tests are from rfc6070
> 
>
https://codereview.chromium.org/820523003/diff/100001/LayoutTests/crypto/subt...
> LayoutTests/crypto/subtle/pbkdf2-deriveBits.html:70: // Non-ascii password
> From here on the tests are not from rfc6070.
> 
> Either indicate as such, or move the rfc6070 tests to their own file.
> 
> HKDF tests use the naming scheme:
>   hkdf-deriveBits-rfc5869-test-vectors.html
> 
> So you could consider moving the rfc6070 tests to a similarly named file and
> then keep these ones here.
> 
>
https://codereview.chromium.org/820523003/diff/100001/LayoutTests/crypto/subt...
> LayoutTests/crypto/subtle/pbkdf2-deriveBits.html:108: },
> Please add the empty password case too.

[eroman, 2015-01-16 04:05:23]

https://codereview.chromium.org/820523003/diff/100001/LayoutTests/crypto/subt...
File LayoutTests/crypto/subtle/pbkdf2-deriveBits.html (right):

https://codereview.chromium.org/820523003/diff/100001/LayoutTests/crypto/subt...
LayoutTests/crypto/subtle/pbkdf2-deriveBits.html:108: },
On 2015/01/16 03:36:59, xun.sun wrote:
> On 2015/01/15 22:28:58, eroman wrote:
> > Please add the empty password case too.
> 
> Done. Added in pbkdf2-deriveBits-empty-password.html.
> 
> The keys derived from empty password turn out to be random. Is this known /
> expected?

Not expected, it looks like a bug in BoringSSL (which is precisely why I wanted
this test :)
I will follow-up on that bug.

[xun.sun, 2015-01-16 05:38:47]

On 2015/01/16 04:05:23, eroman wrote:
>
https://codereview.chromium.org/820523003/diff/100001/LayoutTests/crypto/subt...
> File LayoutTests/crypto/subtle/pbkdf2-deriveBits.html (right):
> 
>
https://codereview.chromium.org/820523003/diff/100001/LayoutTests/crypto/subt...
> LayoutTests/crypto/subtle/pbkdf2-deriveBits.html:108: },
> On 2015/01/16 03:36:59, xun.sun wrote:
> > On 2015/01/15 22:28:58, eroman wrote:
> > > Please add the empty password case too.
> > 
> > Done. Added in pbkdf2-deriveBits-empty-password.html.
> > 
> > The keys derived from empty password turn out to be random. Is this known /
> > expected?
> 
> Not expected, it looks like a bug in BoringSSL (which is precisely why I
wanted
> this test :)
> I will follow-up on that bug.

Test revised.

[eroman, 2015-01-16 21:21:38]

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

[eroman, 2015-01-16 21:21:39]

LGTM

@jochen: Could you review for Source/platform/* and public/platform/* approval?
Thanks!

[jochen (gone - plz use gerrit), 2015-01-19 12:51:22]

lgtm with nit

https://codereview.chromium.org/820523003/diff/190001/public/platform/WebCryp...
File public/platform/WebCryptoAlgorithmParams.h (right):

https://codereview.chromium.org/820523003/diff/190001/public/platform/WebCryp...
public/platform/WebCryptoAlgorithmParams.h:366: unsigned saltLength, unsigned
iterations)
nit. no line length limit in blink, should all go into one line

[xun.sun, 2015-01-19 13:40:31]

On 2015/01/19 12:51:22, jochen (slow) wrote:
> lgtm with nit
> 
>
https://codereview.chromium.org/820523003/diff/190001/public/platform/WebCryp...
> File public/platform/WebCryptoAlgorithmParams.h (right):
> 
>
https://codereview.chromium.org/820523003/diff/190001/public/platform/WebCryp...
> public/platform/WebCryptoAlgorithmParams.h:366: unsigned saltLength, unsigned
> iterations)
> nit. no line length limit in blink, should all go into one line

Done.

Feel free to CQ this.

[eroman, 2015-01-19 19:32:06]

The CQ bit was checked by eroman@chromium.org

[commit-bot: I haz the power, 2015-01-19 19:33:05]

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

[commit-bot: I haz the power, 2015-01-19 21:30:47]

Committed patchset #12 (id:210001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=188649