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.

Sign up for GitHub

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

Jump to bottom

Add XMSS-SHA256_{10, 16, 20}_192 parameters #1817

Merged
merged 4 commits into from
Jun 18, 2024
Merged

Add XMSS-SHA256_{10, 16, 20}_192 parameters #1817

merged 4 commits into from
Jun 18, 2024

Conversation

cothan
Copy link
Contributor

@cothan cothan commented Jun 8, 2024 •
edited
Loading

Signed-off-by: Duc Tri Nguyen [email protected]

  • Add additional XMSS parameters to meet NIST SP 800 208.
  • Add emoji to Table parameters sig_stfl_xmss.h, I hope it's helpful and save developer time.
  • Add KATs.
  • Add tests.
  • Does this PR change the input/output behaviour of a cryptographic algorithm (i.e., does it change known answer test values)? (If so, a version bump will be required from x.y.z to x.(y+1).0.)
  • Does this PR change the list of algorithms available -- either adding, removing, or renaming? Does this PR otherwise change an API? (If so, PRs in fully supported downstream projects dependent on these, i.e., oqs-provider will also need to be ready for review and merge by the time this is merged.)

@cothan cothan requested a review from dstebila as a code owner June 8, 2024 20:19
@cothan cothan changed the title Add XMSS-SHA2_{10, 16, 20}_192 parameters Add XMSS-SHA256_{10, 16, 20}_192 parameters Jun 8, 2024
@cothan cothan requested review from ashman-p, SWilson4 and baentsch June 8, 2024 21:19
@cothan cothan mentioned this pull request Jun 8, 2024
Merged
2 tasks
@cothan
Copy link
Contributor Author

cothan commented Jun 8, 2024

So the 1 failed check because it's timeout :((

baentsch
baentsch previously requested changes Jun 9, 2024
View reviewed changes
Copy link
Member

@baentsch baentsch left a comment •
edited
Loading

Choose a reason for hiding this comment

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

If I'm not mistaken (?) the second question checkbox needs to be checked as this adds algs. If you'd agree, please activate downstream testing: I'm pretty certain it won't pass (as the current liboqs "main" breaks oqsprovider building) -- still undecided whether this should be fixed in liboqs or oqsprovider. Opinions welcome.

Add: See open-quantum-safe/oqs-demos#281 and open-quantum-safe/oqs-provider#427 (same problem)

@baentsch baentsch mentioned this pull request Jun 10, 2024
Closed
ashman-p
ashman-p reviewed Jun 10, 2024
View reviewed changes
Copy link
Contributor

@ashman-p ashman-p left a comment

Choose a reason for hiding this comment

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

Probably should update docs/algorithms/sig_stfl/xmss.md etc?

@cothan
Copy link
Contributor Author

cothan commented Jun 10, 2024

@ashman-p , thank you. I forgot to update the doc. I will make an update soon.

@cothan cothan force-pushed the add_xmss-sha2_10-16-20_192_parameters branch from 00bc880 to d48fe88 Compare June 12, 2024 19:56
@cothan cothan force-pushed the add_xmss-sha2_10-16-20_192_parameters branch from d48fe88 to 3ae54cb Compare June 12, 2024 20:01
@cothan
Copy link
Contributor Author

cothan commented Jun 12, 2024

Thanks @ashman-p , I've updated README.md, xmss.yml, and xmss.md.

@cothan
Copy link
Contributor Author

cothan commented Jun 12, 2024

@baentsch . I think I miss something, I check the downstream oqs-provider and I don't see XMSS/LMS to be inserted like open-quantum-safe/oqs-provider#413 .
This PR add additional parameters to XMSS, so it changes the algorithms list (if we count the parameters in).
And it doesn't change APIs. So I think I will remove the 2nd checkbox.
What do you think?

@ashman-p
Copy link
Contributor

Thanks @ashman-p , I've updated README.md, xmss.yml, and xmss.md.

@cothan Thanks.

@baentsch baentsch dismissed their stale review June 12, 2024 20:57

No XMSS support in oqs-provider, no API change impacting downstream

@SWilson4
Copy link
Member

This looks good to me, but I'll hold off on approving until CI is green. Also, it's probably a good idea to trigger the oqs-provider tests by putting "[trigger downstream]" in a commit message in case of a subtle bug like #1815.

cothan added 2 commits June 13, 2024 17:22
@cothan
add XMSS-SHA2_*_192 parameters
d3ecc3e 
Signed-off-by: Duc Tri Nguyen <[email protected]>

make astyle happy

Signed-off-by: Duc Tri Nguyen <[email protected]>

update xmss.md

Signed-off-by: Duc Tri Nguyen <[email protected]>

update algorithm list

Signed-off-by: Duc Tri Nguyen <[email protected]>
@cothan
[trigger downstream]
9ff5efb 
Signed-off-by: Duc Tri Nguyen <[email protected]>
@cothan cothan force-pushed the add_xmss-sha2_10-16-20_192_parameters branch from 7e2b97a to 9ff5efb Compare June 13, 2024 21:22
cothan added 2 commits June 17, 2024 14:39
@cothan
format xmss.md
8659166 
Signed-off-by: Duc Tri Nguyen <[email protected]>
@cothan
[trigger downstream]
ae975e5 
Signed-off-by: Duc Tri Nguyen <[email protected]>
@cothan
Copy link
Contributor Author

cothan commented Jun 17, 2024

@SWilson4 , Fixed the [trigger downstream] test. All CIs are green. I'll merge when I have your approval.

SWilson4
SWilson4 approved these changes Jun 18, 2024
View reviewed changes
Copy link
Member

@SWilson4 SWilson4 left a comment

Choose a reason for hiding this comment

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

LGTM! Thanks, Duc.

@SWilson4 SWilson4 merged commit 5e31116 into main Jun 18, 2024
114 checks passed
@cothan cothan deleted the add_xmss-sha2_10-16-20_192_parameters branch June 18, 2024 18:32
@BrewTestBot BrewTestBot mentioned this pull request Sep 27, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@SWilson4 SWilson4 SWilson4 approved these changes

@ashman-p ashman-p ashman-p approved these changes

@baentsch baentsch baentsch left review comments

@dstebila dstebila Awaiting requested review from dstebila dstebila is a code owner

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@cothan @ashman-p @SWilson4 @baentsch