crypto/hkdf: add package

[qmuntal]

Important

Nov 20, 2024: The latest version of the proposal is here

I propose to move the golang.org/x/crypto/hkdf package into the standard library with the name crypto/hkdf. golang.org/x/crypto/hkdf would then be updated to just be a wrapper around crypto/hkdf.

I acknowledge that depending on golang.org/x/crypto/hkdf or crypto/hkdf doesn't make much difference in terms of usability, either the x/crypto package and the standard library promise backwards compatibility and are respected by the Go community. Yet, doing this move will bring two main benefits for the Go standard library and for the niche of users requiring FIPS 140 compliance:

• crypto/tls uses golang.org/x/crypto/hkdf to implement TLS 1.3, and there are some Go forks out there that either already provide FIPS compliant TLS 1.3 via OpenSSL, or plan to do so in the near term. I suppose that Google will eventually also provide it, but this is out of this proposal. Adding this support would be much easier if crypto/hkdf was part of the standard library, in which case it could be patched to forward calls to crypto/internal/boring as needed.
• hkdf is widely used outside the standard library. Users depending on it that also require FIPS 140 compliance will benefit from having it in the standard library as a package backed by BoringCrypto/OpenSSL/CNG, etc.

Worth noting that golang.org/x/crypto/hkdf API has remained the same for more than 5 years, and that its git log only contains 4 commits since it was added in 2014. It seems to already be in good shape, so I don't expect that moving it to the standard library would require much additional maintenance effort.

For completeness, this is the current golang.org/x/crypto/hkdf API that I'm proposing to add to the standard library:

// Expand returns a Reader, from which keys can be read, using the given
// pseudorandom key and optional context info, skipping the extraction step.
//
// The pseudorandomKey should have been generated by Extract, or be a uniformly
// random or pseudorandom cryptographically strong key. See RFC 5869, Section
// 3.3. Most common scenarios will want to use New instead.
func Expand(hash func() hash.Hash, pseudorandomKey, info []byte) io.Reader

// Extract generates a pseudorandom key for use with Expand from an input
// secret and an optional independent salt.
//
// Only use this function if you need to reuse the extracted key with multiple
// Expand invocations and different context values. Most common scenarios,
// including the generation of multiple keys, should use New instead.
func Extract(hash func() hash.Hash, secret, salt []byte) []byte

// New returns a Reader, from which keys can be read, using the given hash,
// secret, salt and context info. Salt and info can be nil.
func New(hash func() hash.Hash, secret, salt, info []byte) io.Reader

Related to #65269.

@golang/security

[qmuntal]

@rolandshoemaker @FiloSottile

[qmuntal]

I've been thinking a bit more about this. The x/crypto/hkdf API might be over-complicated given that most of hkdf.New and hkdf.Expand uses ends up with a single Read or io.ReadFull call.

A simpler and easier to use API could look like this:

// Expand derives a key from the given hash, pseudorandomKey, and optional context info,
// returning a []byte of length keyLen that can be used as cryptographic key.
// The extraction step is skipped.
//
// The pseudorandomKey should have been generated by Extract, or be a uniformly
// random or pseudorandom cryptographically strong key. See RFC 5869, Section
// 3.3. Most common scenarios will want to use Key instead.
func Expand(hash func() hash.Hash, pseudorandomKey, info []byte, keyLen int) ([]byte, error)

// Extract generates a pseudorandom key for use with Expand from an input
// secret and an optional independent salt.
//
// Only use this function if you need to reuse the extracted key with multiple
// Expand invocations and different context values. Most common scenarios,
// including the generation of multiple keys, should use Key instead.
func Extract(hash func() hash.Hash, secret, salt []byte) []byte

// Key derives a key from the given hash, secret, salt and context info,
// returning a []byte of length keyLen that can be used as cryptographic key.
// Salt and info can be nil.
func Key(hash func() hash.Hash, secret, salt, info []byte, keyLen int) ([]byte, error)

[ericlagergren]

@qmuntal Getting rid of the io.Reader is a good change, but not being able to generate a key into an existing slice is rather unfortunate.

[qmuntal]

Getting rid of the io.Reader is a good change, but not being able to generate a key into an existing slice is rather unfortunate.

I proposed the keyLen instead of passing am existing slice for symmetry with other kdf packages in x/crypto, but passing an slice instead also works for me.

[magical]

I was going to suggest using the mid-stack inlining trick to let us use the nice API (accept keyLen, return []byte) while avoiding the allocation, but it appears that that only works when the slice length is a const so it doesn't help in this case.

[qmuntal]

but it appears that that only works when the slice length is a const so it doesn't help in this case.

Yep, this is a known long-standing limitation of the compiler: #29095.

@rolandshoemaker @FiloSottile what do your think about this API proposal: #61477 (comment)

[FiloSottile]

Re: moving the package, we'll definitely want to do that in Go 1.24 for #69536, although I can't promise we'll want to expand the Go+BoringCrypto mode to cover it.

On removing the io.Reader I do like the keyLen API better, but I am worried about

1. leaving users behind by making the transition harder, and
2. not being able to implement x/crypto/hkdf in terms of crypto/hkdf.

(2) is a problem because we'd like to make moved x/crypto packages simple wrappers around the standard library ones.

(1) depends on how applications have been using HKDF. Have they taken to use it as a XOF? As you said, the API has been unchanged for many years...

A compromise could be to leave New and Expand as-is but add Key as the high-level helper. Do applications generally use New (and hence would be able to migrate to Key) or Expand? We could also have ExpandKey but that's starting to feel crowded.

The allocation can actually be largely mitigated by doing something like the following. I haven't tested but I am confident that some variation will work.

func Key(hash func() hash.Hash, secret, salt, info []byte, keyLen int) ([]byte, error) {
    b := make([]byte, 0, 128) // outlined allocation for keyLen <= 128
    return appendKey(b, hash, secret, salt, info, keyLen)
}

While we are making changes, I think I'd like to make info and maybe salt into strings. salt is largely misunderstood, and in many if not most cases it should be a fixed string, but sometimes like in TLS 1.3 it's used for high-entropy values. info is almost always ASCII AFAIK. Making them different types would also help against swapping them.

[mateusz834]

Do we still want to return an io.Reader, why not a concrete exported type like *hkdf.Reader?

[FiloSottile]

Do we still want to return an io.Reader, why not a concrete exported type like *hkdf.Reader?

Yes, strong +1. I think all new crypto APIs we add should return concrete types.

[qmuntal]

although I can't promise we'll want to expand the Go+BoringCrypto mode to cover it.

That's fine. Let's focus on the new API for now.

Do applications generally use New (and hence would be able to migrate to Key) or Expand? We could also have ExpandKey but that's starting to feel crowded.

On GitHub, There are 435 occurrences of hkdf.Expand (source) and 1.2k occurrences of hkdf.New (source). IMO hkdf.Expand is used enough to justify ExpandKey.

On removing the io.Reader I do like the keyLen API better, but I am worried about

1. leaving users behind by making the transition harder, and
2. not being able to implement x/crypto/hkdf in terms of crypto/hkdf.

Doing a quick GitHub sampling I would say that 99% of hkdf.New and hkdf.Expand end up calling either Read only one time or directly passing it to io.ReadFull. It would be a pity that wouldn't take advantage of this move to simplify the API. On the other hand, I see how not being able to implement x/crypto/hkdf in terms of crypto/hkdf might be a blocker to make the API simpler. Another compromise could be to implement x/crypto/hkdf by wrapping crypto/hkdf in a io.Reader. That reader could preallocate some bytes to make the common-case of only reading a handful keys.

I think all new crypto APIs we add should return concrete types.

If we drop the io.Reader then we don't have to expose this type. One point in favor of simplifying the API 😄.

Having said this, I'm not opposed to keeping the io.Reader, just trying to make some push back so that we discuss all alternatives.

[ericlagergren]

Making them different types would also help against swapping them.

I think it's a good idea, but it would be unfortunate if it ends up requiring an additional allocation for non-constant info parameters.

Actually, something like [[]byte]byte would be wonderful for implementing HPKE without allocating.

[ericlagergren]

Have they taken to use it as a XOF

Sample size n=1, but I've never once needed nor wanted to use HKDF as an XOF. That would raise some eyebrows. Also, I can't immediately think of any major crypto libraries that have the same behavior as Go here.

[FiloSottile]

Another compromise could be to implement x/crypto/hkdf by wrapping crypto/hkdf in a io.Reader. That reader could preallocate some bytes to make the common-case of only reading a handful keys.

The max output of Expand is 255 * HashLen (both by spec and because there is an 8-bit counter), so this is not impossible, but I'd argue that the code required to implement this Reader is more than what's needed to implement HKDF itself.

Still, the sampling is pretty compelling.

[gopherbot]

Change https://go.dev/cl/621275 mentions this issue: crypto/internal/fips/hkdf: new package

[aclements]

This proposal has been added to the active column of the proposals project
and will now be reviewed at the weekly proposal review meetings.

[aclements]

Replacing the io.Reader with []byte seems like a clearly worthwhile change for the vast majority of use cases that just want a one-shot API. If a caller does need multiple keys, they can generate a larger key and chop it up (or just use x/crypto). If we do get reports that people really need a multi-shot Expand/Key, we can always add a new function in the future, while still providing the more ergonomic []byte API for most users.

I propose we simply don't reimplement x/crypto/hkdf in terms of crypto/hkdf. It's 95 lines of code and it's not going to change.

[mateusz834]

Should it be generic over hash.Hash? This was a concern in #69982 for the crypto/hmac case.

[FiloSottile]

I propose we simply don't reimplement x/crypto/hkdf in terms of crypto/hkdf. It's 95 lines of code and it's not going to change.

The issue with that is that applications that use x/crypto/hkdf don't get redirected to the FIPS module.

Should it be generic over hash.Hash? This was a concern in #69982 for the crypto/hmac case.

Yeah, maybe! I ended up making the crypto/internal/fips/hkdf API generic because I wanted it to work for func() hash.Hash, func() fips.Hash, and func() *sha256.Hash.

package crypto/internal/fips/hkdf
func Extract[H fips.Hash](h func() H, secret, salt []byte) []byte
func Expand[H fips.Hash](h func() H, pseudorandomKey, info []byte, keyLen int) []byte
func Key[H fips.Hash](h func() H, secret, salt, info []byte, keyLen int) []byte

Sounds like we are converging on #61477 (comment), with three open questions (two from above, the last one new):

1. Should salt and/or info be string? info is almost always a string, and salt is often a string. How bad is it to put binary data in a string, if necessary? (I really don't like consecutive []byte parameters, I feel like I always get them the wrong way around.)
2. Should we make them generic over hash.Hash?
3. Why doesn't Extract return an error? (Or, why do any of them return errors? Naked []byte do make some code much easier to write, although I do always say that I always regret not adding an error.)

[ericlagergren]

... (Or, why do any of them return errors? Naked []byte do make some code much easier to write, although I do always say that I always regret not adding an error.)

Expand needs an error because HKDF has a maximum output size.

[qmuntal]

Why doesn't Extract return an error? (Or, why do any of them return errors?

I would add errors to all the functions defined here. It is more future proof and it would play better with the OpenSSL/CNG bindings I maintain, which are more likely to return an error.

In fact, Expand already returns an error, so the internal/fips/hkdf.Expand could error instead on panicking when the counter overflows.

The issue with that is that applications that use x/crypto/hkdf don't get redirected to the FIPS module.

I see how this is a desirable requirement. We can always linkname an unexported func crypto/hkdf.expandReader(...) io.Reader function or even export it to facilitate migrating from x/crypto/hkdf to crypto/hkdf. Note that there is no need to implement the reader counterpart of x/crypto/hkdf.New in the standard library, as it can could be implemented using hkdf.ExpandReader.

[FiloSottile]

HKDF salts are somewhat different from PBKDF2 salts, and often misunderstood, so they are more often strings than their PBKDF2 counterparts, which instead are almost always random.

(The difference is due to the fact that HKDF operates already on high-entropy inputs, and only needs the salt for domain separation and to make some proofs cleaner, while PBKDF2 is actually defending against precomputation.)

Anyway, info is more string-y than salt anyway, the usual pattern is to make it just a label for the purpose of the key, so let's make that a string and salt a byte slice, so it's consistent.

package hkdf
func Extract[Hash fips.Hash](h func() Hash, secret, salt []byte) []byte
func Expand[Hash fips.Hash](h func() Hash, key []byte, info string, length int) []byte
func Key[Hash fips.Hash](h func() Hash, secret, salt []byte, info string, length int) []byte

[FiloSottile]

Oh, and sure, let's return errors.

package hkdf
func Extract[Hash fips.Hash](h func() Hash, secret, salt []byte) ([]byte, error)
func Expand[Hash fips.Hash](h func() Hash, key []byte, info string, length int) ([]byte, error)
func Key[Hash fips.Hash](h func() Hash, secret, salt []byte, info string, length int) ([]byte, error)

[ericlagergren]

Oh, and sure, let's return errors.

package hkdf
func Extract[Hash fips.Hash](h func() Hash, secret, salt []byte) ([]byte, error)

How can Extract fail?

[FiloSottile]

In FIPS-only mode if the secret is too short.

In general I don’t want us to add error returns just for the benefit of FIPS-only mode, but since the other two already return errors for keyLen out of bounds, making them consistent feels fine.

[aclements]

Based on the discussion above, this proposal seems like a likely accept.

The proposal is to add a new crypto/hkdf package with the following API:

// Extract generates a pseudorandom key for use with [Expand] from an input
// secret and an optional independent salt.
//
// Only use this function if you need to reuse the extracted key with multiple
// Expand invocations and different context values. Most common scenarios,
// including the generation of multiple keys, should use [Key] instead.
func Extract[Hash hash.Hash](h func() Hash, secret, salt []byte) ([]byte, error)

// Expand derives a key from the given hash, key, and optional context info,
// returning a []byte of length keyLength that can be used as cryptographic key.
// The extraction step is skipped.
//
// The key should have been generated by [Extract], or be a uniformly
// random or pseudorandom cryptographically strong key. See RFC 5869, Section
// 3.3. Most common scenarios will want to use [Key] instead.
func Expand[Hash hash.Hash](h func() Hash, key []byte, info string, keyLength int) ([]byte, error)

// Key derives a key from the given hash, secret, salt and context info,
// returning a []byte of length keyLength that can be used as cryptographic key.
// Salt and info can be nil.
func Key[Hash hash.Hash](h func() Hash, secret, salt []byte, info string, keyLength int) ([]byte, error)

[magical]

The problem with adding an error return to a function like Extract which is expected to ~never fail is that it will be very tempting to ignore the error when calling it.

key, _ := hdkf.Extract(sha256.New, secret, salt) // can't fail

Especially if the only foreseeable error is "secret too small". That sounds like programmer error and it seems more appropriate to panic in that case than allow the program to continue.

[gopherbot]

Change https://go.dev/cl/630296 mentions this issue: crypto/hkdf: init package

[FiloSottile]

That sounds like programmer error and it seems more appropriate to panic in that case than allow the program to continue.

Hmm, not really, the secret could come from disk or network. I don't think this is the right place for a panic.

I do hear you about ignoring it being tempting, but how are Key and Expand different? If you put a fixed value as keyLength, they have the same failure mode as Extract.

There are good arguments to make all three return errors or not, but I don't think there's a strong argument to not make them consistent.

[magical]

I do hear you about ignoring it being tempting, but how are Key and Expand different?

I would argue that they are not, and that the error return isn't carrying its weight there either.

This seems supported by the fact that the initial implementation of crypto/internal/fips/hkdf did not require any error returns.

[aclements]

No change in consensus, so accepted. 🎉
This issue now tracks the work of implementing the proposal.

The proposal is to add a new crypto/hkdf package with the following API:

// Extract generates a pseudorandom key for use with [Expand] from an input
// secret and an optional independent salt.
//
// Only use this function if you need to reuse the extracted key with multiple
// Expand invocations and different context values. Most common scenarios,
// including the generation of multiple keys, should use [Key] instead.
func Extract[Hash hash.Hash](h func() Hash, secret, salt []byte) ([]byte, error)

// Expand derives a key from the given hash, key, and optional context info,
// returning a []byte of length keyLength that can be used as cryptographic key.
// The extraction step is skipped.
//
// The key should have been generated by [Extract], or be a uniformly
// random or pseudorandom cryptographically strong key. See RFC 5869, Section
// 3.3. Most common scenarios will want to use [Key] instead.
func Expand[Hash hash.Hash](h func() Hash, key []byte, info string, keyLength int) ([]byte, error)

// Key derives a key from the given hash, secret, salt and context info,
// returning a []byte of length keyLength that can be used as cryptographic key.
// Salt and info can be nil.
func Key[Hash hash.Hash](h func() Hash, secret, salt []byte, info string, keyLength int) ([]byte, error)

[cpu]

I think this can be closed since 630296 landed. I used the wrong linkage in the CR description, sorry!

[gopherbot]

Change https://go.dev/cl/634095 mentions this issue: crypto/hkdf: add package doc comment