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: make compatible with OpenBSD #46940

Closed
otrava7 opened this issue Jun 26, 2021 · 13 comments
Closed

proposal: x/crypto/bcrypt: make compatible with OpenBSD #46940

otrava7 opened this issue Jun 26, 2021 · 13 comments
Labels
FrozenDueToAge Proposal Proposal-Crypto Proposal related to crypto packages or other security issues
Milestone

Comments

@otrava7
Copy link

otrava7 commented Jun 26, 2021

Currently, the golang bcrypt implementation only allows for hash Generation using GenerateFromPassword which automatically generates the salt. The original OpenBSD implementation has the bcrypt_gensalt(u_int8_t log_rounds); and bcrypt(const char *key, const char *salt); functions. We already attempt to make the golang implementation compatible with the c implementation.
OpenBSD: https://nixdoc.net/man-pages/OpenBSD/man3/bcrypt.3.html

I propose we add equivalent functions to the OpenBSD ones to allow for interoperability. I have already made an equivalent for the bcrypt(const char *key, const char *salt); function, which you can find here

@gopherbot gopherbot added this to the Proposal milestone Jun 26, 2021
@seankhliao seankhliao added the Proposal-Crypto Proposal related to crypto packages or other security issues label Jun 26, 2021
@seankhliao
Copy link
Member

Are there any concrete use cases for this?

related #18737

@deltamualpha
Copy link

I have one!

I wrote a wrapper around the Enzoic password API, which, as part of its API, requires the caller to compute a bcrypt hash with a given salt. https://www.enzoic.com/docs-credentials-api/#hash-based-credentials-step-3.

In order to implement the client in go, I had to fork the bcrypt package and add a function very similar to the one linked above.

@ianlancetaylor ianlancetaylor added this to Incoming in Proposals (old) Jun 27, 2021
@rsc
Copy link
Contributor

rsc commented Oct 27, 2021

/cc @FiloSottile

@rsc
Copy link
Contributor

rsc commented Oct 27, 2021

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

@rsc rsc moved this from Incoming to Active in Proposals (old) Oct 27, 2021
@rsc
Copy link
Contributor

rsc commented Nov 3, 2021

@FiloSottile any opinions on adding GenerateFromPasswordAndSalt?

@deltamualpha
Copy link

A few other notes on this:

It has been proposed before: #18737. The use-cases presented in that proposal did not inspire... confidence... that this API will be used responsibly.

I personally think having the option is valuable (although it's really going to be valuable for people doing weird things with password management, as my use-case above illustrates), but it certainly should be gated with some big warning signs that "you probably just want to use GenerateFromPassword, only provide your own salt if you're certain it's sufficiently random, etc etc".

@rsc
Copy link
Contributor

rsc commented Nov 10, 2021

/cc @FiloSottile

@rsc
Copy link
Contributor

rsc commented Dec 1, 2021

@rolandshoemaker
Copy link
Member

It seems like there are still pretty limited use cases for this functionality, API compatibility with OpenBSD is nice, but I'm not really sure what else it gets us beyond API parity.

This is a dangerous API to add, and if we have to add a large "this is dangerous, please use it only if you know what you're doing" warning, it seems more prudent to just not implement it in the first place.

@rsc
Copy link
Contributor

rsc commented Dec 8, 2021

thanks @rolandshoemaker

@rsc rsc moved this from Active to Likely Decline in Proposals (old) Dec 8, 2021
@rsc
Copy link
Contributor

rsc commented Dec 8, 2021

Based on the discussion above, this proposal seems like a likely decline.
— rsc for the proposal review group

@deltamualpha
Copy link

Disappointing, but understandable. I suppose I'll have to maintain my little fork.

@rsc rsc moved this from Likely Decline to Declined in Proposals (old) Dec 15, 2021
@rsc
Copy link
Contributor

rsc commented Dec 15, 2021

No change in consensus, so declined.
— rsc for the proposal review group

@rsc rsc closed this as completed Dec 15, 2021
@golang golang locked and limited conversation to collaborators Dec 15, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
FrozenDueToAge Proposal Proposal-Crypto Proposal related to crypto packages or other security issues
Projects
No open projects
Development

No branches or pull requests

6 participants