Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Changes to switch from NIST to IETF version of HKDF #16

Merged
merged 4 commits into from
Aug 29, 2016

Conversation

jimsch
Copy link
Contributor

@jimsch jimsch commented Mar 7, 2016

These are the top level changes to switch from using the HKDF-CTR
defined in NIST SP 800-108 to the IETF RFC 5869 HKDF function.

Both salt and info have been defined as being optional as per the IETF
algorithm.

This means that Issue 27473 in bugzilla is still open. We can easily address this by making the default behavior of salt be defined in this specification rather than relying on implementations to do it. (https://www.w3.org/Bugs/Public/show_bug.cgi?id=27473) The comment however may not be true in terms of implementations since we have switched the underlying algorithm.

Fixes 27600 with respect to this section. The indenting of IDL blocks now exists for HKDF.

Fixes 27425 - HKDF-CTR needs a new name. This has been renamed to be HKDF consistently throughout the document.

Fixes 27438 for this function, but not globally. Get key length now returns null rather than Integer or null since the algorithm as specified only allows for a null return value.

These are the top level changes to switch from using the HKDF-CTR
defined in NIST SP 800-108 to the IETF RFC 5869 HKDF function.

Both salt and info have been defined as being optional as per the IETF
algorithm.
@hhalpin
Copy link

hhalpin commented Jun 20, 2016

I agree to reference the IETF spec, but it seems like we need to add back in for salt behavior "if not provided, it is set to a string of HashLen zeros" behavior from RFC 5869 if no salt is provided.

dictionary <dfn id="dfn-HkdfParams">HkdfParams</dfn> : <a href="#dfn-Algorithm">Algorithm</a> {
<span class="comment">// The algorithm to use with HMAC (e.g.: <a href="#alg-sha-256">SHA-256</a>)</span>
required <a href="#dfn-HashAlgorithmIdentifier">HashAlgorithmIdentifier</a> <dfn id="dfn-HkdfParams-hash">hash</dfn>;
<span class="salt">// A bit string that corresponds to the salt used in the extract step.</span>
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should remain class="comment" - it causes the line to appear as a comment in the code block

@mwatson2
Copy link
Collaborator

@hhalpin According to RFC 5869, the salt is optional and the RFC specifies what to do if it is not provided (likewise info, though not as clearly). So, I don't think we need to specify that here.

@jimsch The changes look good to me, with one minor comment. We are also regenerating Overview.html with each commit, since the automation of that is not agreed yet. LMK if you would like me to make the changes and merge.

@engelke
Copy link

engelke commented Jul 18, 2016

I may be missing it, but the PR description of deriveBits does not seem to say how you convert the octet-string that the RFC algorithm creates to a bit string of length bits. The PBKDF2 operation just doesn't allow requesting anything that's not a multiple of 8 bits long, while ECDH specifies which bits to return. Which belongs here?

@ghost
Copy link

ghost commented Jul 22, 2016

Thanks for putting this together Jim!

Mostly looks good, but a couple observations from the perspective of Chrome's existing implementation:

  • Chrome treats non-multiple-of-8 bit lengths the same as ECDH (uses a truncated bit-string with zero-padding at the end). Per Charle's comment, I don't think the current PR specifies the behavior.
  • Chrome defines both "salt" and "info" as required whereas this pull request has them both as optional.

Here is how Chrome's existing implementation defines it:

dictionary HkdfParams : Algorithm {
    required HashAlgorithmIdentifier hash;
    required BufferSource salt;
    required BufferSource info;
};

While it is true that RFC 5869 specifically uses the term optional, we can accomplish the same thing conceptually in an API where the property is marked as required in the dictionary.

For instance without ambiguity here is how you can exercise the "optional-ness" in Chrome's API:

  • Pass a zero-length salt - This is equivalent to having not specified salt from the perspective of the RFC (follows from HMAC's definition), and doesn't require any special-casing in either the spec or in the implementation.
  • Passing a zero-length "info" - this is the same as not specifying it from the perspective of the RFC.

The advantage of having the fields be "required" is it makes two classes of error harder in the API:

  • Not passing a salt because you didn't realize you should have - use of a salt is definitely recommended
  • Intending to pass a "salt" however somehow getting it wrong.

There are a myriad of ways in which Javascript helps you to shoot your foot off. Here are two examples.

Example1

var alg = {
    name:"HKDF",
    info:myInfo,salts:mySalts,hash:"SHA-256",
};

crypto.subtle.deriveBits(alg, baseKey, 80);

Above I mistyped the salt attribute.

  • With an optional API the mistake goes unnoticed. HKDF will go ahead and use an empty salt, even though the developer had intended to use a salt but just typed it wrong.
  • With a required API the deriveBits() call simply fails and the developer notices and corrects the problem during development

Example2

var alg = {
    name: "HKDF",
    hash: "SHA-256",
    salt: mySalts,
    info: myInfo
};

crypto.subtle.deriveBits(alg, baseKey, 80);

In Example2 the problem is mySalts is mistyped, and hence undefined. In this case (I am pretty sure) it is treated by WebIDL as if the parameter were not specified. If it is optional, then we again have a bug with cryptographic consequence.

Certainly we can change Chrome's implementation if the spec decides to go with "optional" instead of "required". But I want to make the argument for why I think "optional" parameters are an anti-pattern, especially in Javascript where it is amazingly easy to introduce these sorts of bugs.

Making it easier to not specify a salt is IMO optimizing a use-case which is already dubious. Whereas making it a required parameter (but you can of course pass an empty buffer for it) is optimizing for correct use of the operation.

Cheers.

jimsch added 2 commits August 24, 2016 15:02
# Conflicts:
#	spec/Overview-WebCryptoAPI.xml
Change so that salt and info are now required fields
@jimsch
Copy link
Contributor Author

jimsch commented Aug 24, 2016

As requested, salt and info are now required elements rather than optional ones.

@ghost
Copy link

ghost commented Aug 26, 2016

PR #16 Looks good to me, thanks!

@mwatson2
Copy link
Collaborator

@jimsch Could you resolve the conflicts ?

@jimsch
Copy link
Contributor Author

jimsch commented Aug 26, 2016

Will do so again today (sigh)

# Conflicts:
#	spec/Overview-WebCryptoAPI.xml
#	spec/Overview.html
@mwatson2 mwatson2 merged commit 36bc668 into w3c:master Aug 29, 2016
saghul added a commit to saghul/TSJS-lib-generator that referenced this pull request Jun 30, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants