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

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

Open
elithrar opened this issue Sep 2, 2016 · 26 comments
Open

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

elithrar opened this issue Sep 2, 2016 · 26 comments
Labels
NeedsFix Proposal-Accepted Proposal-Crypto
Projects
Milestone

Comments

@elithrar
Copy link

@elithrar elithrar commented Sep 2, 2016

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.

@quentinmit quentinmit added this to the Proposal milestone Sep 6, 2016
@adg
Copy link
Contributor

@adg adg commented Oct 3, 2016

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

@bradfitz
Copy link
Contributor

@bradfitz bradfitz commented Nov 7, 2016

Ping @agl.

@rsc rsc changed the title Proposal: Make x/crypto/scrypt API match x/crypto/bcrypt proposal: x/crypto/scrypt: make API match x/crypto/bcrypt Nov 7, 2016
@kevinburke
Copy link
Contributor

@kevinburke kevinburke commented Oct 5, 2017

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
Copy link
Author

@elithrar elithrar commented Oct 6, 2017

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
Copy link
Contributor

@ianlancetaylor ianlancetaylor commented Mar 19, 2018

@ianlancetaylor ianlancetaylor added the Proposal-Crypto label Mar 20, 2018
@FiloSottile FiloSottile changed the title proposal: x/crypto/scrypt: make API match x/crypto/bcrypt proposal: x/crypto/scrypt,x/crypto/argon2: add high-level APIs Apr 3, 2020
@FiloSottile
Copy link
Contributor

@FiloSottile FiloSottile commented Apr 3, 2020

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

This comment has been minimized.

@rsc
Copy link
Contributor

@rsc rsc commented Apr 22, 2020

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

@rsc
Copy link
Contributor

@rsc rsc commented Apr 22, 2020

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

@rsc rsc added this to Active in Proposals Apr 22, 2020
@Jacalz
Copy link
Contributor

@Jacalz Jacalz commented Apr 22, 2020

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
Copy link
Author

@elithrar elithrar commented Apr 22, 2020

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
Copy link
Contributor

@rsc rsc commented Apr 22, 2020

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

func Check(hash, password string) error

That seems pretty clear.

@Jacalz
Copy link
Contributor

@Jacalz Jacalz commented Apr 22, 2020

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
Copy link
Contributor

@magical magical commented Apr 22, 2020

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
Copy link
Contributor

@FiloSottile FiloSottile commented Apr 23, 2020

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
Copy link
Author

@elithrar elithrar commented Apr 23, 2020

@SamWhited
Copy link
Member

@SamWhited SamWhited commented Apr 24, 2020

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
Copy link
Contributor

@FiloSottile FiloSottile commented Apr 24, 2020

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
Copy link
Contributor

@Jacalz Jacalz commented Apr 25, 2020

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
Copy link
Contributor

@FiloSottile FiloSottile commented Apr 26, 2020

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
Copy link
Contributor

@rsc rsc commented May 5, 2020

@FiloSottile proposed a revised API in #16971 (comment) 11 days ago.

Does anyone object to this API? Thanks.

@bojanz
Copy link

@bojanz bojanz commented May 15, 2020

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
Copy link
Contributor

@rsc rsc commented May 20, 2020

@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
Copy link
Contributor

@FiloSottile FiloSottile commented May 26, 2020

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.

  1. 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
Copy link
Contributor

@rsc rsc commented May 27, 2020

No change in consensus, so accepted.

@rsc rsc moved this from Likely Accept to Accepted in Proposals May 27, 2020
@rsc rsc unassigned agl May 27, 2020
@rsc rsc removed this from the Proposal milestone May 27, 2020
@rsc rsc added this to the Unreleased milestone May 27, 2020
@rsc rsc changed the title proposal: x/crypto/scrypt,x/crypto/argon2: add high-level APIs x/crypto/scrypt,x/crypto/argon2: add high-level APIs May 27, 2020
@bojanz
Copy link

@bojanz bojanz commented May 27, 2020

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
NeedsFix Proposal-Accepted Proposal-Crypto
Projects
Proposals
Accepted
Development

No branches or pull requests