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

sign: Adding SLH-DSA signature #512

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

Conversation

armfazh
Copy link
Contributor

@armfazh armfazh commented Sep 11, 2024

This implementation supports the twelve parameter sets approved at FIPS 205

Test vectors match the ones at ACVP-Server version 1.1.0.37. These test vectors only target the internal functions.

Pure and Prehash-based signatures are supported, but no known test vectors are available.

The last commit adds the twelve instances to the generic signing Scheme interface.

Implementation makes a good effort to avoid heap allocations that usually add a significant overhead.

@armfazh armfazh self-assigned this Sep 11, 2024
@armfazh
Copy link
Contributor Author

armfazh commented Sep 11, 2024

there is a timeout happening on the ARM build because tests are running in parallel, but it doesn't seems to be related to a failure in the code.

Copy link

@rozbb rozbb left a comment

Choose a reason for hiding this comment

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

This looks great! Almost all my comments are nitpicky bc I didn't have anything else to talk about :)

sign/slhdsa/wotsp.go Outdated Show resolved Hide resolved
sign/slhdsa/wotsp.go Show resolved Hide resolved
sign/slhdsa/wotsp.go Show resolved Hide resolved
sign/slhdsa/wotsp.go Show resolved Hide resolved
sign/slhdsa/wotsp.go Show resolved Hide resolved
sign/slhdsa/wotsp_test.go Show resolved Hide resolved
}

// See FIPS 205 -- Section 6.1 -- Algorithm 9 -- Iterative version.
func (s *statePriv) xmssNodeIter(
Copy link

Choose a reason for hiding this comment

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

Similar thing. I had trouble following along with this algorithm, since it differs so much from the paper. Also why is i a parameter at all if it's not a recursive algorithm?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Internally, the index of each node is used as part of the hash parameters. So, i denotes the first index of a sub-tree.

sign/slhdsa/xmss.go Show resolved Hide resolved
defer s.Clear()

s.forsSign(sig.forsSig, md, addr)
pkFors := s.forsPkFromSig(md, sig.forsSig, addr)
Copy link

Choose a reason for hiding this comment

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

Would it be possible to compute pkFors as a side effect of forsSign? Can save a few hashes if so. Similarly might be able to do this in the htSign function

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've tried to coalesce these functions, but didn't found any savings on the number of hashes.

@armfazh armfazh force-pushed the adding_slhdsa branch 3 times, most recently from 12020e3 to 32046e4 Compare October 10, 2024 10:07
@armfazh
Copy link
Contributor Author

armfazh commented Nov 13, 2024

ACVP test vectors updated to v1.1.0.37

@@ -36,6 +39,18 @@ var allSchemes = [...]sign.Scheme{
mldsa44.Scheme(),
mldsa65.Scheme(),
mldsa87.Scheme(),
slhdsa.ParamIDSHA2Small128,
Copy link
Member

Choose a reason for hiding this comment

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

Better to just use s and f instead of small and fast.


err = json.Unmarshal(input, &vector)
test.CheckNoErr(t, err, "error unmarshalling JSON file")
}
Copy link
Member

Choose a reason for hiding this comment

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

Let's open an issue to unify the ACVP code.

addressSizeNonCompressed = 32
)

type address struct {
Copy link
Member

Choose a reason for hiding this comment

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

Might be more efficient with respect to allocations to use [32]byte as address type.

s.xmssPkFromSig(node, node, sig[j], idxLeaf, addr)
}

return subtle.ConstantTimeCompare(node, root) == 1
Copy link
Member

@bwesterb bwesterb Dec 9, 2024

Choose a reason for hiding this comment

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

No need for constant time compare here

// Calling [NewMessageWithPreHash] with [NoPreHash] is equivalent.
// The calls to [Message.Write] copy the message into a internal buffer.
// To avoid copies of the message, use [NewMessage] instead.
type Message struct {
Copy link
Member

Choose a reason for hiding this comment

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

Why not use the regular Signer interface?

@bwesterb bwesterb self-requested a review December 9, 2024 13:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants