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/argon2: support secret value and associated data #35481

Closed
except opened this issue Nov 9, 2019 · 16 comments
Closed

proposal: x/crypto/argon2: support secret value and associated data #35481

except opened this issue Nov 9, 2019 · 16 comments
Labels
NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. Proposal Proposal-Crypto Proposal related to crypto packages or other security issues
Milestone

Comments

@except
Copy link

except commented Nov 9, 2019

Hello,

It seems though the exported functions of x/crypto/argon2 only permits for customised parameters relating to generation and obviously the salt and password for hashing. Being able to use a secret key would in theory increase security.

@gopherbot gopherbot added this to the Unreleased milestone Nov 9, 2019
@ALTree ALTree added the Proposal-Crypto Proposal related to crypto packages or other security issues label Nov 9, 2019
@bcmills
Copy link
Contributor

bcmills commented Nov 10, 2019

CC @FiloSottile

@bcmills bcmills added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Nov 10, 2019
@FiloSottile FiloSottile changed the title x/crypto/argon2: export deriveKey to allow the usage of a secret key x/crypto/argon2: support secret value and associated data Nov 10, 2019
@FiloSottile
Copy link
Contributor

I assume you are referring to the inputs that the draft calls "Secret value K" and "Associated data X". The package API is already a handful as it is, I am not inclined to add support for them, unless there are protocols that require their use.

@except
Copy link
Author

except commented Nov 10, 2019

I just thought it was useful to be able to use the entirety of argon2, but I agree the package API does require many parameters already and it doesn’t seem as though any protocols need to use it.

@rsc rsc changed the title x/crypto/argon2: support secret value and associated data proposal: x/crypto/argon2: support secret value and associated data Feb 12, 2020
@rsc rsc added this to Active in Proposals (old) Feb 12, 2020
@rsc
Copy link
Contributor

rsc commented Feb 12, 2020

Adding to proposal minutes.

@rsc
Copy link
Contributor

rsc commented Feb 26, 2020

There doesn't seem to be much interest in this feature. Based on the discussion above, this seems like a likely decline.

@rsc rsc moved this from Active to Likely Decline in Proposals (old) Feb 26, 2020
@rsc
Copy link
Contributor

rsc commented Mar 4, 2020

No change in consensus, so declined.

@rsc rsc moved this from Likely Decline to Declined in Proposals (old) Mar 4, 2020
@marwan-at-work
Copy link
Contributor

Hi 👋 @rsc @FiloSottile

Not sure if my +1 here makes any difference, but I'd love to see the secret (K) value exposed in x/crypto/argon2.

The motivation here is that other argon2 implementations such as Ruby/C, allow users to pass K, and the fact that Go's implementation doesn't creates friction for developers who are migrating their code from said languages to Go.

For example, I am tasked with re-writing some Ruby code to Go where in the transition period, the Go code will generate the argon2 hashes and the Ruby code should continue to be able to verify it (as both Go and Ruby will share the same secret in Vault) until the Ruby code is fully swapped out. Of course, this is not possible unless x/crypto/argon2 allowed me to pass K.

From what I can see, the secret K implementation actually already exists in x/crypto/argon2 and is tested as well, it just needs to be exposed?

Thank you!

@marwan-at-work
Copy link
Contributor

Re-opening the issue for visibility. But please feel free to close again.

@except
Copy link
Author

except commented Feb 26, 2021

To avoid a breaking change, one could go down the route of argon2.IDKeyWithSecret.

@elgunhuseynli
Copy link

@rsc @FiloSottile

Deriving keys without secret allow attackers to crack passwords offline in case of database breach. Because salt and other parameters are stored in leaked database, and no secret is involved. Using secret and not storing it in database can add extra layer of security.

x/crypto/argon2 already implements this and all tests are passed. Why not expose secret parameter?

Also it is needed for compatibility with other libraries and existing projects.

Thanks.

@seankhliao
Copy link
Member

Closing as I see the decision has been made to not include this.

@seankhliao seankhliao closed this as not planned Won't fix, can't repro, duplicate, stale May 13, 2023
@peter021
Copy link

This is a bad decision. These parameters are required to be able to implement NIST SP 800-108r1 and NIST SP 800-56C REV. 2. Also good password storage practise (https://www.ietf.org/archive/id/draft-ietf-kitten-password-storage-04.html) is to use a pepper, which is the purpose of the "secret"-parameter. I wanted to use a memory safe language for this and chose go initially because https://go.googlesource.com/proposal/+/master/design/cryptography-principles.md promised that I could. But it seems now that "safe" and "modern" are just pretty words. I guess my python code won't get ported to go at this time.

@dpifke
Copy link
Contributor

dpifke commented May 15, 2023

I guess my python code won't get ported to go at this time.

While I agree with you that this decision is unfortunate, the less dramatic solution is to fork the Argon2 library out of /x/crypto. Speaking as someone who has had to do this (for compatibility with Argon2 hashes shared with a non-Go library—almost exactly the scenario described in #35481 (comment)), exposing those parameters is a pretty trivial modification, and shouldn't affect backporting any future changes from upstream.

Now that this decision is final, hopefully someone in the community will step up and maintain such a fork, similar to what happened with Proton Mail and /x/crypto/openpgp. I realistically don't have the bandwidth in the short-term, or else I would offer.

@peter021
Copy link

peter021 commented May 16, 2023

The bigger issue is that best practise, standards compliant featured doesn't seem to be asked for in this community or in the standard library. What other shortcuts and limitations is hidden in the standard library? If the standard library has no way of producing a peppered password, what conclusions can be drawn about resulting products? If there is a relevant module in the library then why shouldn't issues be fixed where they are? If forking is the natural reaction, what other parts of the standard library should not be used? Can this be clearly marked in the standard module documentation going forward?

@ianlancetaylor

This comment was marked as resolved.

@peter021
Copy link

peter021 commented May 17, 2023

You're right, I removed the sweeping accusations and generalisations. Sorry about that, got a little carried away.
Some of the remaining questions may be important to consider though. They don't need answers, and they represent a single point of view among others. Statistics show that if one person voice an oppinion there are generally another 24 people holding the very same oppinion. Also we are biased to think that everyone thinks the same way we do, which is usually only right about half of the time.
I write this not to downvalue the work that you guys do, I write this to try to convince people who are better than me to fix something important that is broken. I'm trying to help you guys improve the language and any resulting product that is built using it. I am a moonshine developer and a very recent beginner at Go and shouldn't be let near any standard library code. Security standards however is my back alley. My day job is about protecting critical infrastructure.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. Proposal Proposal-Crypto Proposal related to crypto packages or other security issues
Projects
Status: Declined
Development

No branches or pull requests