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

crypto/sha3: import from x/crypto #69982

Closed
FiloSottile opened this issue Oct 22, 2024 · 15 comments
Closed

crypto/sha3: import from x/crypto #69982

FiloSottile opened this issue Oct 22, 2024 · 15 comments
Labels
Proposal Proposal-Accepted Proposal-Crypto Proposal related to crypto packages or other security issues
Milestone

Comments

@FiloSottile
Copy link
Contributor

FiloSottile commented Oct 22, 2024

As part of #65269 (and for #69536), I propose we migrate golang.org/x/crypto/sha3 to the standard library, with the following API.

package sha3

func Sum224(data []byte) [28]byte
func Sum256(data []byte) [32]byte
func Sum384(data []byte) [48]byte
func Sum512(data []byte) [64]byte

func SumSHAKE128(data []byte, length int) []byte
func SumSHAKE256(data []byte, length int) []byte

type SHA3 struct{}
func New224() *SHA3
func New256() *SHA3
func New384() *SHA3
func New512() *SHA3
func (*SHA3) Write(p []byte) (n int, err error)
func (*SHA3) Sum(b []byte) []byte
func (*SHA3) Reset()
func (*SHA3) Size() int
func (*SHA3) BlockSize() int
func (*SHA3) MarshalBinary() ([]byte, error)
func (*SHA3) AppendBinary([]byte) ([]byte, error)
func (*SHA3) UnmarshalBinary(data []byte) error

type SHAKE struct {}
func (*SHAKE) Write(p []byte) (n int, err error)
func (*SHAKE) Read(p []byte) (n int, err error)
func (*SHAKE) Reset()
func (*SHAKE) BlockSize() int
func (*SHAKE) MarshalBinary() ([]byte, error)
func (*SHAKE) AppendBinary([]byte) ([]byte, error)
func (*SHAKE) UnmarshalBinary(data []byte) error
func NewCSHAKE128(N, S []byte) *SHAKE
func NewCSHAKE256(N, S []byte) *SHAKE
func NewSHAKE128() *SHAKE
func NewSHAKE256() *SHAKE

/cc @golang/security @cpu

Changes from golang.org/x/crypto/sha3

  • Return concrete types *SHA3 and *SHAKE instead of hash.Hash and ShakeHash
    • This lets us add new methods in the future, but note that it makes it harder to use New functions with crypto/hmac, because they don't have type func() hash.Hash without wrapping. Feels worth it to me, and maybe we can make a v2 one day with generics, but if anyone has ideas to smooth this over let me know.
  • Expose MarshalBinary, AppendBinary, and UnmarshalBinary
  • Dropped Sum and Size from SHAKE
    • These were added recently by @mdempsky without a proposal in CL 526937, I see the value in making SHAKE usable as a hash, especially since the actual hashes NIST specified are so needlessly slow, but I found it confusing that SHAKE-128's Size is 256 bits, which doesn't come from any specs. We can add these back in a separate proposal.
  • Replaced ShakeSum/n/(hash, data []byte) with SumSHAKE/n/(data []byte, length int) []byte
    • I found the old one confusing with its two []byte arguments. The new one matches more closely Sum/n/ and can be made zero-allocations in most cases by outlining a fixed-size allocation of a reasonable length. We can also make the default recommendation "Just use SHAKE128 with a length of 32" and this would be the easiest to use API for that.
  • Dropped Clone() ShakeHash
  • Dropped NewLegacyKeccak256 and NewLegacyKeccak512
    • We can use a linkname for now to make x/crypto/sha3 a full wrapper of crypto/sha3, and can add them back to the public API if they are still necessary.
  • Renamed Shake to SHAKE to match standard spelling

For reference, here is the current x/crypto/sha3 API.

func New224() hash.Hash
func New256() hash.Hash
func New384() hash.Hash
func New512() hash.Hash
func NewLegacyKeccak256() hash.Hash
func NewLegacyKeccak512() hash.Hash
func ShakeSum128(hash, data []byte)
func ShakeSum256(hash, data []byte)
func Sum224(data []byte) (digest [28]byte)
func Sum256(data []byte) (digest [32]byte)
func Sum384(data []byte) (digest [48]byte)
func Sum512(data []byte) (digest [64]byte)
type ShakeHash interface {
	hash.Hash
	io.Reader
	Clone() ShakeHash
}
func NewCShake128(N, S []byte) ShakeHash
func NewCShake256(N, S []byte) ShakeHash
func NewShake128() ShakeHash
func NewShake256() ShakeHash
@FiloSottile FiloSottile added Proposal Proposal-Crypto Proposal related to crypto packages or other security issues labels Oct 22, 2024
@FiloSottile FiloSottile added this to the Proposal milestone Oct 22, 2024
@gabyhelp
Copy link

Related Issues and Documentation

(Emoji vote if this was helpful or unhelpful; more detailed feedback welcome in this discussion.)

@mateusz834
Copy link
Member

This lets us add new methods in the future, but note that it makes it harder to use New functions with crypto/hmac,

I guess this is going to work:

hmac.New(crypto.SHA3_256.New, make([]byte, 32))

@FiloSottile FiloSottile changed the title crypto/sha3: import from x/crypto proposal: crypto/sha3: import from x/crypto Oct 22, 2024
@mateusz834
Copy link
Member

Speaking of crypto.SHA3_256.New, once this proposal gets accepted, the x/crypto/sha3 is going to be implemented in terms of crypto/sha3 (for go1.24), but if someone is going to use crypto/sha3 AND an old version of x/crypto package, then crypto.SHA3_256.New is going to be random (depending on the import order) crypto/sha3 or x/crypto/sha3, right?

go/src/crypto/crypto.go

Lines 145 to 150 in 38f8596

func RegisterHash(h Hash, f func() hash.Hash) {
if h >= maxHash {
panic("crypto: RegisterHash of unknown hash function")
}
hashes[h] = f
}

At least it does not panic. I assume that this is fine, right?

@FiloSottile
Copy link
Contributor Author

Yeah, I was just thinking about that, and indeed it doesn't really matter since they will be equivalent and they don't panic. Newer versions of x/crypto will simply not register the hash if running on Go 1.24 (and import crypto/sha3 which will register its version).

@magical
Copy link
Contributor

magical commented Oct 22, 2024

This lets us add new methods in the future, but note that it makes it harder to use New functions with crypto/hmac

Indeed, though it's worth noting that SHA-3 has its own MAC construction, KMAC, which should probably be preferred over HMAC-SHA3. So making the package slightly harder to use with crypto/hmac is not necessarily a bad thing.

@gopherbot
Copy link
Contributor

Change https://go.dev/cl/616717 mentions this issue: crypto/internal/fips/sha3: import x/crypto/sha3@750a45fe5e4

@aclements aclements moved this to Incoming in Proposals Oct 23, 2024
@aclements aclements moved this from Incoming to Active in Proposals Oct 23, 2024
@aclements
Copy link
Member

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

gopherbot pushed a commit that referenced this issue Oct 28, 2024
For now just internally, pending a dedicated proposal for the exposed
package API.

In this CL the code is copied verbatim, for ease of review. Only the
imports were replaced with the corresponding internal ones, and
crypto.RegisterHash calls were disabled. Also, the 0.5MB keccakkats file
was dropped, supplanted by TestCSHAKEAccumulated and ACVP tests.

Updates #65269
Updates #69982
For #69536

Change-Id: Ia4735b50c99b9573a5c4889733c4a119930fe658
Reviewed-on: https://go-review.googlesource.com/c/go/+/616717
Reviewed-by: Daniel McCarney <daniel@binaryparadox.net>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Reviewed-by: Roland Shoemaker <roland@golang.org>
Auto-Submit: Filippo Valsorda <filippo@golang.org>
Reviewed-by: Carlos Amedee <carlos@golang.org>
This was referenced Oct 30, 2024
@aclements
Copy link
Member

Speaking of crypto.SHA3_256.New, once this proposal gets accepted, the x/crypto/sha3 is going to be implemented in terms of crypto/sha3 (for go1.24), but if someone is going to use crypto/sha3 AND an old version of x/crypto package, then crypto.SHA3_256.New is going to be random (depending on the import order) crypto/sha3 or x/crypto/sha3, right?

Packages are initialized in order by import path (constrained by the package DAG), so because crypto/sha3 sorts because golang.org/x/crypto/sha3, crypto/sha3 will always be initialized first. Since RegisterHash just takes the latest one, that means projects that import an old version of golang.org/x/crypto/sha3 will always get the x/crypto version.

As @FiloSottile pointed out, it shouldn't be a problem either way, but it's not non-deterministic.

@aclements
Copy link
Member

As @FiloSottile pointed out, it shouldn't be a problem either way, but it's not non-deterministic.

If this is an issue for FIPS, we could check the caller in RegisterHash and prefer the built-in sha3. It's ugly.

@FiloSottile
Copy link
Contributor Author

I don't think it's a problem: we can just communicate "you need to use at least x/crypto@v0.X.0 for it to redirect calls to the FIPS module". After all, if an application uses cryptography from some other external module there's nothing we can do about it.

@aclements aclements moved this from Active to Likely Accept in Proposals Nov 6, 2024
@aclements
Copy link
Member

Based on the discussion above, this proposal seems like a likely accept.

The proposal details are in #69982 (comment)

@FiloSottile
Copy link
Contributor Author

FiloSottile commented Nov 12, 2024

Chatting with @rsc we thought about renaming the SHAKE helpers to have a Sum prefix, so it doesn't look like the type returned by NewSHAKE128/256.

func SumSHAKE128(data []byte, length int) []byte
func SumSHAKE256(data []byte, length int) []byte

[Edited the issue body.]

@aclements aclements moved this from Likely Accept to Accepted in Proposals Nov 13, 2024
@aclements
Copy link
Member

No change in consensus, so accepted. 🎉
This issue now tracks the work of implementing the proposal.

The proposal details are in #69982 (comment)

@aclements aclements changed the title proposal: crypto/sha3: import from x/crypto crypto/sha3: import from x/crypto Nov 13, 2024
@gopherbot
Copy link
Contributor

Change https://go.dev/cl/629176 mentions this issue: crypto/sha3: new package

@gopherbot
Copy link
Contributor

Change https://go.dev/cl/636255 mentions this issue: crypto/sha3: reduce cSHAKE allocations

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

No branches or pull requests

7 participants