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

proposal: x/crypto/bcrypt: add WithContext-style functions #56416

Open
hf opened this issue Oct 25, 2022 · 2 comments
Open

proposal: x/crypto/bcrypt: add WithContext-style functions #56416

hf opened this issue Oct 25, 2022 · 2 comments
Labels
Proposal Proposal-Crypto Proposal related to crypto packages or other security issues
Milestone

Comments

@hf
Copy link

hf commented Oct 25, 2022

(This is my first time contributing to Go, so please let me know if I'm doing something wrong.)

What's the proposed change?

golang.org/x/crypto/bcrypt has two main functions:

  • GenerateFromPassword([]byte, int)([]byte, error)
  • CompareHashAndPassword([]byte, []byte) error

This proposal retains these functions, and adds two more:

  • GenerateFromPasswordWithContext(context.Context, []byte, int)([]byte, error)
  • CompareHashAndPasswordWithContext(context.Context, []byte, []byte) error

This is a common Go pattern for functions that obey semantics of a provided context. This context can be used to terminate password hashing early.

Why is this necessary?

Password hashing functions like bcrypt are incredibly CPU intensive. For example, hashing a password at the current bcrypt.DefaultCost of 10 on my Apple m1 chip takes about 75ms. Furthermore, the implementation of the algorithm is non-cooperative. Go does not preempt Goroutines. Thus, once a few of these Goroutines are scheduled on the majority of threads, all the other Goroutines will be penalized and will incur a latency of 75ms.

By providing this extended API, the algorithm can be changed to cooperate with the rest of the Goroutines by calling runtime.Gosched() periodically. The provided context can be thus used to check for cancellation or timeout, and terminate hashing early.

Who is most likely to benefit?

Any web server that does some sort of password hashing, typically authentication servers, suffers from unpredictable (and sometimes catastrophic) latencies due to the non-cooperative nature of the current bcrypt implementation. By giving access to this API which uses a cooperative algorithm, these web servers will no longer experience unpredictable latencies. In fact, non-password hashing workloads will be given their fair share of CPU time.

Who is going to implement this?

I've already done this in golang/crypto PR #236.

Further Notes

I've seen issues with this in previous experience. I'm also on the Supabase Auth team.

I wrote a large note some time ago. This isn't very well reviewed, so take all the math with a grain of salt, but I believe it illustrates the problem and solutions.

@hf hf added the Proposal label Oct 25, 2022
@gopherbot gopherbot added this to the Proposal milestone Oct 25, 2022
@FiloSottile FiloSottile changed the title proposal: add WithContext-style functions to bcrypt affected/package: x/crypto/bcrypt proposal: x/crypto/bcrypt: add WithContext-style functions to bcrypt Oct 25, 2022
@FiloSottile FiloSottile changed the title proposal: x/crypto/bcrypt: add WithContext-style functions to bcrypt proposal: x/crypto/bcrypt: add WithContext-style functions Oct 25, 2022
@FiloSottile
Copy link
Contributor

Shouldn't async preemption handle this, with no need for Gosched?

@hf
Copy link
Author

hf commented Oct 25, 2022

Shouldn't async preemption handle this, with no need for Gosched?

It should, though through some experience I've found the 10ms preemption too generous and not consistent. Not being able to cancel the workload (for example to time-out all password hashing at 2s) which in turn relieves CPU resources is the main issue that can't be dealt with outside of the current package.

I'm open to removing runtime.Gosched(). In fact it may just be that the select to check if the context is done is enough to schedule other Goroutines.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Proposal Proposal-Crypto Proposal related to crypto packages or other security issues
Projects
Status: Incoming
Development

No branches or pull requests

4 participants