proposal: crypto/pbkdf2: add package

[qmuntal]

Proposal Details

I propose to move the golang.org/x/crypto/pbkdf2 package into the standard library with the name crypto/pbkdf2. This proposal is a #65269 spin off, as requested by the following comment from @rsc:

I agree with this in principle but there should be review of individual packages rather than a blank check to usher significant amounts of never-reviewed API into the standard library.
#65269 (comment)

pbkdf2 is a low-hanging fruit here as it only exports a single function. I propose to move it to the standard library as is, just adding an error return parameter in case we want to implement some additional parameter validation in the future (e.g. minimum salt length or minimum iterations). The proposed API looks like this:

// Key derives a key from the password, salt and iteration count,
// returning a []byte of length keyLen that can be used as cryptographic key.
// The key is derived based on the method described as PBKDF2 with the
// HMAC variant using the supplied hash function.
func Key(password, salt []byte, iter, keyLen int, h func() hash.Hash) ([]byte, error)

The pbkdf2.Key function is enough to cover most pbkdf2 use cases. As a data point, OpenSSL exports the PKCS5_PBKDF2_HMAC function, accepting the same inputs as pbkdf2.Key.

@golang/security

[gabyhelp]

Related Issues and Documentation

• proposal: crypto/hkdf: add package #61477

_{(Emoji vote if this was helpful or unhelpful; more detailed feedback welcome in this discussion.)}

[ericlagergren]

My one hangup about this is that new projects shouldn't be using PBKDF2 without a good reason. There are significantly better password-based KDFs, like Argon2 or even scrypt.

IMO, it's misleading to move this into the stdlib without updating the docs to point people to better algorithms.

[qmuntal]

My one hangup about this is that new projects shouldn't be using PBKDF2 without a good reason. There are significantly better password-based KDFs, like Argon2 or even scrypt.

Reasons aside, pbkdf2 is much more popular that scrypt and argon2, at least if we look the number of times the corresponding golang.org/x/crypto package is imported. As of writing this, pbkdf2 is imported by 7081 modules, scrypt 3227 modules and argon2 1559 modules.

Having said this, it is fine for me to add any doc explaining better alternatives with the corresponding reasoning.

[rolandshoemaker]

We've had higher level discussions of providing a more opaque password hashing API, wherein you just have a password you want to hash, and we return what we consider a secure hash (and also implement the verification side to transparently support multiple hashes, so we can easily rotate what we return). I think that is the long term solution to these kinds of problems where people are still using an older, less ideal method simply because they don't know they should update.

In the short-term adding pbkdf2 to the standard library seems mostly fine to me. In the long term we'll probably want to continue to provide these primitives anyway, since people will continue to need them for backwards and third-party compatibility.

I think the other option is to leave it in x/crypto as one of the "frozen" libraries we don't expect to see any further development. There are currently no other open issues for the library (and only 2 closed historical issues), so perhaps that would be reasonable if we don't expect any new API additions, and don't have any plans to make any other substantial changes to the package.

If we do add it, we should definitely add a note to the documentation though pointing out you should only be using this for compat reasons, and pointing to the better choices.

cc @FiloSottile

[qmuntal]

Thanks for the insights @rolandshoemaker.

I think the other option is to leave it in x/crypto as one of the "frozen" libraries we don't expect to see any further development. There are currently no other open issues for the library (and only 2 closed historical issues), so perhaps that would be reasonable if we don't expect any new API additions, and don't have any plans to make any other substantial changes to the package.

In the issue description I mentioned a small API change that could make pbkdf2 a little-bit harder to misuse: adding an error return value so we can validate the parameters and fail if they are not "secure" enough. I'm specifically thinking about implementing NIST SP 800-132 recommendations:

• Minimum iteration count of 1000.
• The length of the randomly-generated portion of the salt shall be at least
  128 bits.
• The key length shall be at least 112 bits in length.

[rolandshoemaker]

Oh sorry, I somehow completely glossed over this while reading through this. In that case I think it's reasonable to move into the standard library with these specific changes.

[meling]

In the issue description I mentioned a small API change that could make pbkdf2 a little-bit harder to misuse: adding an error return value so we can validate the parameters and fail if they are not "secure" enough. I'm specifically thinking about implementing NIST SP 800-132 recommendations:

• Minimum iteration count of 1000.
• The length of the randomly-generated portion of the salt shall be at least
  128 bits.
• The key length shall be at least 112 bits in length.

Would such bad inputs qualify for a panic, rather than an error? (My understanding is that these are programming mistakes, not user input, and thus it might be better to panic.)

[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.

[dpifke]

Would such bad inputs qualify for a panic, rather than an error? (My understanding is that these are programming mistakes, not user input, and thus it might be better to panic.)

For backwards compatibility, I think we need an "escape hatch" to derive a key using older, non-secure parameters. For instance, I have backups written with software that predates the SP 800-132 recommendations that I would like to still be able to read using modern tools.

[aclements]

For backwards compatibility, I think we need an "escape hatch" to derive a key using older, non-secure parameters.

There are two ways to deal with this. One is that we always compute and return the hash, but also return an error of a particular type if the parameters aren't secure. That way the caller can check for and discard that error if necessary. Another option would be a GODEBUG. (Possibly we do both and the GODEBUG would simply suppress the error return.)

[FiloSottile]

I'm specifically thinking about implementing NIST SP 800-132 recommendations:

• Minimum iteration count of 1000.
• The length of the randomly-generated portion of the salt shall be at least
  128 bits.
• The key length shall be at least 112 bits in length.

These are NIST recommendations and I am not sure we should enforce them across the board. For example salt-less PBKDF makes sense in some settings. This is what GODEBUG=fips140=enforce is for. We shouldn't let FIPS requirements infect the default mode. (Also, the minimum iteration count is a recommendation, not a shall.)

I guess adding the error return makes sense for future-proofing, although it makes switching package harder, but I don't think we need to think about GODEBUGs and new errors.

Since we're breaking the API anyway, should we move keyLen at the end like hkdf, scrypt, and argon2, and h at the beginning like hmac and hkdf?

func Key(h func() hash.Hash, password, salt []byte, iter, keyLen int) ([]byte, error)

Also, like in #61477 (comment), should we make it generic over hash.Hash?

And maybe password should be a string?

[qmuntal]

These are NIST recommendations and I am not sure we should enforce them across the board. For example salt-less PBKDF makes sense in some settings. This is what GODEBUG=fips140=enforce is for. We shouldn't let FIPS requirements infect the default mode.

I'm fine not implementing those FIPS restrictions by default if they are not mandatory. If we have the error return then we can implement whatever opt-in or opt-out requirements we want in the future.

Since we're breaking the API anyway, should we move keyLen at the end like hkdf, scrypt, and argon2, and h at the beginning like hmac and hkdf?

Good point, agree we should do this, else it will cause confusion when working with different KDFs.

Also, like in #61477 (comment), should we make it generic over hash.Hash?

And maybe password should be a string?

I don't have an opinion on these changes. Both would work for me.

[aclements]

Also, like in #61477 (comment), should we make it generic over hash.Hash?

In that case, the API would be

func Key[H hash.Hash](h func() H, password, salt []byte, iter, keyLen int) ([]byte, error)

right?

This seems pretty good to me.