x/crypto/scrypt,x/crypto/argon2: add high-level APIs

[elithrar]

Summary: The x/crypto/scrypt package has a very simple API that puts the onus of figuring out salt generation and sensible N/r/p values on the package user. We should attempt to mirror the bcrypt packages' API and provide sensible defaults.

Details:

• Add a GenerateFromPassword function that generates output in the form N$r$p$salt$dk (noting that there is no 'standard' for scrypt here)
• Add a CompareHashAndPassword function
• Add a Cost function that can return the cost of a given output (i.e. for determining whether to upgrade or not)
• Provide sensible default params that provide reasonable values of N, r, p and document why/when you may wish to change them.
• Potentially provide a way to automatically determine values of N, r, p given memory (MB) and time (ms) constraints.

Note that I've done most of this work in https://godoc.org/github.com/elithrar/simple-scrypt and would seek to bring most of this in.

[adg]

Seems reasonable to me, but @agl should take a look.

[bradfitz]

Ping @agl.

[kevinburke]

puts the onus of figuring out salt generation and sensible N/r/p values on the package user. Provide sensible default params that provide reasonable values of N, r, p and document why/when you may wish to change them.

I just added some better documentation here, so hopefully this is better now.

Add a GenerateFromPassword function that generates output in the form N$r$p$salt$dk (noting that there is no 'standard' for scrypt here)

I'd feel uncomfortable publishing this without having some sort of standard present on the scrypt website. You could ask Colin Percival if he is interested in it.

Add a CompareHashAndPassword function

This would be reasonable but we'd need to figure out what the hash input should be.

Add a Cost function that can return the cost of a given output (i.e. for determining whether to upgrade or not)

The API for bcrypt takes a []byte but []byte is also the output from scrypt.Key. You'd have to know to use the GenerateFromPassword output instead.

[elithrar]

Thanks for taking a look at this @kevinburke (I'd forgotten myself)

I'd feel uncomfortable publishing this without having some sort of standard present on the scrypt website. You could ask Colin Percival if he is interested in it.

Noted. I pinged Colin on Twitter, and as there isn't a specified output format (there isn't in the paper), have asked if there's any intent to specify. Unlikely, but doesn't hurt to ask.

The other two points are somewhat held up on a standard format. The alternative would be to return a struct of the params for the caller to then stringify as needed.

PS: I did note the improved params "for 2017" CL the other day, which is a good start.

[ianlancetaylor]

Ping @agl @FiloSottile

[FiloSottile]

Here's a proposal for a high-level password API for scrypt.

const RecommendedCost = 15
func NewHash(password string, cost int) (string, error)
func Cost(hash string) (int, error)
func Check(hash, password string) error
func Calibrate(t time.Duration, memoryMiB int) (cost int, err error)

We should just fix r = 8 and p = 1, as there is no reason to change those, and users can still use the low-level Key interface. We can support different values in Check.

For the $ identifier, we should probably just copy libsodium, but I'll put together a wider survey.

We should probably not have minimum and maximum costs, but the Check docs should recommend checking with Cost if the hash is not trusted. Cost needs to take r and p into account, by scaling the returned cost relatively to them.

While at it, let's also do argon2.

const RecommendedTime = 1
const RecommendedMemory = 32 * 1024
func NewHash(password string, time, memoryKiB uint32) (string, error)
func Cost(hash string) (time, memoryKiB uint32, threads uint8, err error)
func Check(hash, password string) error
func Calibrate(t time.Duration) (time uint32, err error)

We've left out threads because it doesn't seem to be widely used (is that true?) and one can still use Key, but had to return it from Cost because it affects the performance in a way that is not as easily aggregated as scrypt.Cost.

RecommendedMemory matches the memory of scrypt.RecommendedCost.

-- @FiloSottile and @katiehockman

[Jacalz]

I have been quite interested in cryptography recently and I am currently using bcrypt because of the super easy high level API that can be used. I would have preferred to use scrypt or argon2 due to them generally being advertised as more secure from what I can tell. That is precisely why I think this is a great idea and I think more users in Go would adopt usage of scrypt and argon2if they were easier to use.

Regarding the proposals, I really like the idea from @FiloSottile with the recommended values. That makes life a lot easier and it could enforce strong security because recommendations could ideally be bumped in future releases in case such a need should emerge. It looks like the recommendation is to have parallelism that corresponds to the amount of cores (or perhaps threads) on the computer system. Might be good to do this automatically in this high level API.

However, I would like that the names match those in bcrypt and I think that it makes a lot of sense to be concise. Thus I would appreciate if the NewHash() was GenerateFromPassword() and Check() wasCompareHashAndPassword(). It might also, as mentioned above, make sense to check the number of cores using runtime.NumCPU() for setting the parallelism. It might not be needed to be passed in as a function parameter, but it might make sense to check it based on cpu cores.

Might also make sense if the Argon2 implementation used Argon2ID because it seems to be recommended if the user doesn't know what to use.

[rsc]

Using very long names because they already exist is not typically a good pattern. Cleaning up the API is almost certainly worth doing.

[rsc]

@FiloSottile posted a new API 19 days ago and there have not been many comments. Does anyone object to accepting this new API?

[Jacalz]

After my initial comment, I have changed the stance on this. I think it might be worth adding support for actually changing parallelism. Otherwise I have nothing to object.

I also thought of another thing. Wouldn’t it be useful to have the formatting functions exposed so people using the IDKey() and Key() functions could still encode and decode the standardized format for Argon2 hash storage?

[elithrar]

I’m in favor of @FiloSottile‘s API - I could nitpick, but I also appreciate there is no such thing as a perfect API:

• The word “hash” is important - “hashing” passwords is the common terminology and making the API clear to any user + searchable is helpful
• Calibrate is clear enough, but likely needs a good godoc to explain why you might use it
• Check might be the least clear - check what? Maybe CheckHash since we use NewHash.
• NewHash follows the NewXXX convention and does what it says on the tin.

The other comments w.r.t. these diverging from the bcrypt API are fair, but I agree with Russ that this is an opportunity to improve, with any new KDF following this new API as much as possible.

[rsc]

FWIW, it's not just Check. There is a signature:

func Check(hash, password string) error

That seems pretty clear.

[Jacalz]

I just want to elaborate more with what I propose as additions to the API suggested by @FiloSottile. My previous post was perhaps a bit too wage in my opinion, so I will try to make it a bit clearer.

• I suggest that the NewHash() function has a field for threads in the argon2 API.

  func NewHash(password string, time, memoryKiB uint32, threads uint8) (string, error)

• I also suggest that the Argon2 and Scrypt API implementations both provide the encode and decode functions as public in order to allow them to be used outside the high level API. With encode and decode I mean the functions to create and parse the $parameters$salt$hash strings that are created in these high level APIs. Users should not need to create their own insecure parsers. I think of something like this:

  
  func Encode(hash string, p Parameters) string

func Decode(encoded string) (hash string, p Parameters)

- I also think that both Scrypt and Argon2 apis could be beneficial to have key lengths supported in the high level APIs.

These are my suggestions. I think this would be a great addition to the proposed API and I would appreciate seeing this part of the implementation.

[magical]

What happens if Check is called with the arguments in the wrong order?

h.Check(password, hash)

One nice thing about the name CheckHashAndPassword, verbose though it be, is that it makes it obvious to anyone reading the code what the correct order is.

[FiloSottile]

I think it might be worth adding support for actually changing parallelism.

Can you show some examples of parallelism being used in the wild for password hashing? I don't think it's worth adding that decision to the developer's plate.

Wouldn’t it be useful to have the formatting functions exposed so people using the IDKey() and Key() functions could still encode and decode the standardized format for Argon2 hash storage?

Maybe, but I'm not sure how many people would need this, and there's always time to add APIs, none to remove them.

Check might be the least clear - check what? Maybe CheckHash since we use NewHash.

Not a strong opinion, but I feel like the argument names do that work, as Russ said, and CheckHash(hash, password) stutters.

What happens if Check is called with the arguments in the wrong order?

It always returns an error (unless someone intentionally makes a password in the format of a hash), so presumably never makes it to production.

I also think that both Scrypt and Argon2 apis could be beneficial to have key lengths supported in the high level APIs.

For what use cases? This is one of those things we can pick for the developer and avoid friction and issues. Something important to keep in mind is that extra flexibility is not intrinsically good, because it comes at a price in complexity and user confusion.

[elithrar]

Agree with Filippo on simplicity - key length, output length especially.

Improperly handled password hashing has a huge blast radius. Keeping it as opinionated as possible will only help reduce that.

(Folks who want to roll their own will roll their own - but let’s not make it easier!)

[SamWhited]

While we're at it, I frequently have to interface with applications that rely on the SCRAM family of SASL mechanisms (which uses PBKDF2), or which use PBKDF2 directly for password hashing to take advantage of the various FIPS validated implementations. Because of this it would be nice to have a similar API for PBKDF2 (recommended iteration count taken from the OWASP cheat sheet) :

const RecommendedIterations = 10000
func NewHash(password string, iter int) (string, error)
func Cost(hash string) (int, error)
func Check(hash, password string) error
func Calibrate(t time.Duration) (int, error)

I left off a way to set the hash since the point of this API is to be simple and not give the user too many knobs. I suspect we'd just want to use SHA256, document it, and call it a day.

[FiloSottile]

Made a couple changes based on feedback from @rsc: added memory to argon2.Calibrate, and made the constants typed. We should have a check for memory to be realistic, so we can detect if MB and KB are being swapped, and should have an example of upgrading the cost.

package scrypt

const RecommendedCost int = 15
func NewHash(password string, cost int) (string, error)
func Cost(hash string) (int, error)
func Check(hash, password string) error
func Calibrate(t time.Duration, memoryMB int) (cost int, err error)

package argon2

const RecommendedTime uint32 = 1
const RecommendedMemory uint32 = 32 * 1024
func NewHash(password string, time, memoryKB uint32) (string, error)
func Cost(hash string) (time, memoryKB uint32, threads uint8, err error)
func Check(hash, password string) error
func Calibrate(t time.Duration, memoryKB uint32) (time uint32, err error)

I'd love to do PBKDF2 at the same time, but is there a crypt prefix for it?

[Jacalz]

Can you show some examples of parallelism being used in the wild for password hashing? I don't think it's worth adding that decision to the developer's plate. The official documentation says that it should be set to the highest possible value on threads. There are people using it, https://github.com/charlesportwoodii/php-argon2-ext for example. Otherwise it might be a good idea to have it set to runtime.NumCPU() automatically. Since I guess we are using goroutines, it might be a viable solution. Looking at https://crypto.stackexchange.com/questions/53400/independently-choose-lanes-and-threads-param-on-argon2 might also be useful.

For what use cases? This is one of those things we can pick for the developer and avoid friction and issues. Something important to keep in mind is that extra flexibility is not intrinsically good, because it comes at a price in complexity and user confusion.

Take my use case as an example. I am producing a 64byte long hash and splitting it to have one half for AES encryption and one for storage. Perhaps having a setting but only allowing output of hashes longer than 32 bytes? I mean, one of the reasons for using argon2 or scrypt over bcrypt is the option to set output key length.

My implementation is also the reason that I would prefer to have functions for decoding and encoding strings with parameters to avoid having to write my own parsing code, which feel wrong from a security standpoint.I could see other users having use of it as well.

[FiloSottile]

Take my use case as an example. I am producing a 64byte long hash and splitting it to have one half for AES encryption and one for storage.

I think you are firmly in "use the Key API" territory, as you actually want to derive key material, not just do password authentication. It's ok for more specific use-cases to have to drop down to lower-level APIs.

[rsc]

@FiloSottile proposed a revised API in https://github.com/golang/go/issues/16971#issuecomment-619261508 11 days ago.

Does anyone object to this API? Thanks.

[bojanz]

The proposal in #16971 sounds great to me. It would remove the need to use wrappers such as this one in applications.

Two questions around the argon2 implementation: 1) Would this always use the (recommended) argon2id variant? That would make sense to me.

2) Assuming that we're using argon2id and not argon2i, the default RecommendedMemory would make more sense as 64kb, since that is the current default in argon2.IDKey() and is recommended by the spec.

[rsc]

@FiloSottile can you reply to @bojanz's comment right above this one?

That clarification comment aside, it sounds like this is a likely accept.

[FiloSottile]

Sorry I had missed this.

Two questions around the argon2 implementation:

1. Would this always use the (recommended) argon2id variant? That would make sense to me.

Yes, there's consensus on using that for password hashes, so we can just choose for the user.

2. Assuming that we're using argon2id and not argon2i, the default RecommendedMemory would make more sense as 64kb, since that is the current default in argon2.IDKey() and is recommended by the spec.

argon2.IDKey doesn't have a default, just an example. I had picked 32 MB here to match the recommended scrypt value. The latest Internet-Draft doesn't actually make any recommendation, it just says an unfortunate "maximum available".

The Argon2id variant with t=1 and maximum available memory is RECOMMENDED as a default setting for all environments.

I think this is a sad mistake, because using literally all the available memory is not at all practical in multi-user shared environments like servers, and application developers don't have (nor should have) the information to assess the marginal value of more memory against hardware attackers.

Lacking a recommendation from the authors, I think being consistent with scrypt so no one is surprised when (or dissuaded from) switching is best.

[rsc]

No change in consensus, so accepted.

[bojanz]

@FiloSottile Thanks for the clarification! Your reasoning makes sense.

[muhlemmer]

I would like to work on this. Just one thing to clarify from the original proposal:

...generates output in the form N$r$p$salt$dk

The argon2 (reference) command and PHC string format spec have a leading $. IMO we should stick with that to remain portability with the reference implementation.

$argon2d$v=19$m=4096,t=3,p=1$c2FsdHNhbHQ$pQdtwf2+LnVUvPb4ldeVk1rKTL+xTirfXtMwML8orOM

And @kevinburke wrote:

I'd feel uncomfortable publishing this without having some sort of standard present on the scrypt website. You could ask Colin Percival if he is interested in it.

Apparently the hash-storing universe lacks a standard in general. For this implementation I would encode it the same way as python's passlib implementation, which is consistent with PHC / argon2. At least to have some kind of portability.

$scrypt$ln=16,r=8,p=1$aM15713r3Xsvxbi31lqr1Q$nFNh2CVHVjNldFVKDHDlm4CbdRSCdEBsjjJxD+iCs5E

[gopherbot]

Change https://go.dev/cl/431595 mentions this issue: argon2: add high-level API

[muhlemmer]

Abandoned work on this issue, no responses in review. Implemented and using my own solution meanwhile.