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

Add support for CSHAKE #80

Open
wants to merge 9 commits into
base: main
Choose a base branch
from
Open

Add support for CSHAKE #80

wants to merge 9 commits into from

Conversation

qmuntal
Copy link
Member

@qmuntal qmuntal commented Dec 18, 2024

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.

Copy link

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.

cng/hash.go Outdated Show resolved Hide resolved
@qmuntal qmuntal requested a review from Copilot December 18, 2024 17:59

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.

@qmuntal qmuntal requested a review from Copilot December 20, 2024 11:49
Copy link

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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")
Copy link
Preview

Copilot AI Dec 20, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The error message should include the algorithm name to make it clear which algorithm failed.

Suggested change
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.

Positive Feedback
Negative Feedback

Provide additional feedback

Please help us improve GitHub Copilot by sharing more details about this comment.

Please select one or more of the options
// 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")
Copy link
Preview

Copilot AI Dec 20, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The error message should include the algorithm name to make it clear which algorithm failed.

Suggested change
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.

Positive Feedback
Negative Feedback

Provide additional feedback

Please help us improve GitHub Copilot by sharing more details about this comment.

Please select one or more of the options
// 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")
Copy link
Preview

Copilot AI Dec 20, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The error message should include the algorithm name to make it clear which algorithm failed.

Suggested change
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.

Positive Feedback
Negative Feedback

Provide additional feedback

Please help us improve GitHub Copilot by sharing more details about this comment.

Please select one or more of the options
@@ -16,6 +16,9 @@ import (
"github.com/microsoft/go-crypto-winnative/internal/bcrypt"
)

// maxHashSize is the size of SHA52 and SHA3_512, the largest hashes we support.
Copy link
Preview

Copilot AI Dec 20, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The comment has a typo. It should be 'SHA512' instead of 'SHA52'.

Suggested change
// maxHashSize is the size of SHA52 and SHA3_512, the largest hashes we support.
// maxHashSize is the size of SHA512 and SHA3_512, the largest hashes we support.

Copilot is powered by AI, so mistakes are possible. Review output carefully before use.

Positive Feedback
Negative Feedback

Provide additional feedback

Please help us improve GitHub Copilot by sharing more details about this comment.

Please select one or more of the options
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants