Add support for CSHAKE #80
Conversation
Co-authored-by: Copilot <[email protected]>
There was a problem hiding this comment.
Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.
There was a problem hiding this comment.
Copilot reviewed 5 out of 5 changed files in this pull request and generated 4 comments.
Comments suppressed due to low confidence (1)
cng/hash_test.go:35
- The return type should be consistent with the new return type of DigestSHA3.
return func() hash.Hash { return cng.NewSHA3_256() }
Tip: Copilot only keeps its highest confidence comments to reduce noise and keep you focused. Learn more
| // SumSHA3_256 returns the SHA3-256 checksum of the data. | ||
| func SumSHA3_256(p []byte) (sum [32]byte) { | ||
| if err := hashOneShot(bcrypt.SHA3_256_ALGORITHM, p, sum[:]); err != nil { | ||
| panic("bcrypt: SHA3_256 failed") |
There was a problem hiding this comment.
The error message should include the algorithm name to make it clear which algorithm failed.
| panic("bcrypt: SHA3_256 failed") | |
| panic("bcrypt: SHA3_256 failed for algorithm " + bcrypt.SHA3_256_ALGORITHM) |
Copilot is powered by AI, so mistakes are possible. Review output carefully before use.
| // SumSHA3_384 returns the SHA3-384 checksum of the data. | ||
| func SumSHA3_384(p []byte) (sum [48]byte) { | ||
| if err := hashOneShot(bcrypt.SHA3_384_ALGORITHM, p, sum[:]); err != nil { | ||
| panic("bcrypt: SHA3_384 failed") |
There was a problem hiding this comment.
The error message should include the algorithm name to make it clear which algorithm failed.
| panic("bcrypt: SHA3_384 failed") | |
| panic("bcrypt: SHA3_384 failed for algorithm " + bcrypt.SHA3_384_ALGORITHM) |
Copilot is powered by AI, so mistakes are possible. Review output carefully before use.
| // SumSHA3_512 returns the SHA3-512 checksum of the data. | ||
| func SumSHA3_512(p []byte) (sum [64]byte) { | ||
| if err := hashOneShot(bcrypt.SHA3_512_ALGORITHM, p, sum[:]); err != nil { | ||
| panic("bcrypt: SHA3_512 failed") |
There was a problem hiding this comment.
The error message should include the algorithm name to make it clear which algorithm failed.
| panic("bcrypt: SHA3_512 failed") | |
| panic("bcrypt: SHA3_512 failed for algorithm " + bcrypt.SHA3_512_ALGORITHM) |
Copilot is powered by AI, so mistakes are possible. Review output carefully before use.
| type hashX struct { | ||
| alg *hashAlgorithm | ||
| _ctx bcrypt.HASH_HANDLE // access it using withCtx |
There was a problem hiding this comment.
This pattern seems like it makes it easier to spot bad access vs. sprinkling runtime.KeepAlive, why the change? (Also IMO would be good for golang-fips/openssl#238 (review).)
There was a problem hiding this comment.
It is easy to forget to use withCtx (it happened in the OpenSSL module), so it is not a silver bullet. I prefer the runtime.KeepAlive pattern because it doesn't add a nested block on each function that access the context (thus the code is simpler), and also to keep consistency with other algorithms, which tend to use the runtime.KeepAlive rather than custom withX functions.
There was a problem hiding this comment.
Ok, if withCtx didn't help in practice, I agree that simplifying to runtime.KeepAlive makes sense. (If it did help, I would have said that it would make sense to expand that style to other algorithms as the way to get to consistency. 😄)
Co-authored-by: Copilot <[email protected]>
|
|
||
| // SupportsSHAKE128 returns true if the SHAKE128 extendable output function is | ||
| // supported. | ||
| func SupportsSHAKE128() bool { |
There was a problem hiding this comment.
It seems like SupportsSHAKE256 should be defined in this part of the file too, rather than later.
| return err == nil | ||
| } | ||
|
|
||
| var _ hash.Hash = (*DigestSHA3)(nil) |
There was a problem hiding this comment.
I've always seen implementation checks just after the type, so this stood out while reading top to bottom. Curious if it's intentional.
|
|
||
| // sequentialBytes produces a buffer of size consecutive bytes 0x00, 0x01, ..., used for testing. | ||
| func sequentialBytes(size int) []byte { | ||
| alignmentOffset := rand.Intn(8) |
There was a problem hiding this comment.
Intentional randomness in tests doesn't feel right... using a fixed seed or looping through possibilities would seem better to me.
Go 1.24 will ship with CSHAKE support. We need to follow suit.
Tests cases are taken from the standard library.
All SHA3 algorithms, which includes CSHAKE, are available since Windows 11, build 25324: https://blogs.windows.com/windows-insider/2023/03/23/announcing-windows-11-insider-preview-build-25324/#:~:text=Introducing%20SHA%2D3%20Support
While here, I've refactor the NewSHA3_256, NewSHA3_384, and NewSHA3_512 functions to return a concret type (SHA3) instead of the interface hash.Hash. This aligns with how upstream implemented
crypto/sha3, making the integration easier.For microsoft/go#1448.