Add CROSS - Githubissues

rtjk commented 3 months ago

This pull request adds the CROSS signature scheme. CROSS is part of NIST's Round 1 call for additional signatures. It comes with a reference implementation and an AVX2-optimized one.

Starting from the latest NIST submission package (version 1.2) we have:

Added support for parallel Keccak to speed up signing and verification.
Applied a few changes to comply with liboqs' API and coding conventions.

Thank you for considering this addition to the project. We hope it provides value and helps further the exploration of the on-ramp candidates.

NOTE: in GithHub Actions test_leaks.py is failing on the parameter set rsdp-192-avx2 when compiling on Ubuntu with clang-15. This seems specific to version 15, no leaks are detected with older and newer releases of Clang. If switching to a newer stable release seems reasonable we can add it to the PR.

[ ] 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.)
[x] 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.)

baentsch commented 3 months ago

Thank you very much @rtjk for this addition! At first glance I'm impressed with how well this PR maintains all OQS conventions . What beyond passing CI (just triggered a full run) is missing to move this to Ready For Review as far as you are concerned? Will you also provide a PR to integrate and test this in oqsprovider as per second checkbox above?

baentsch commented 3 months ago

in GithHub Actions test_leaks.py is failing on the parameter set rsdp-192-avx2 when compiling on Ubuntu with clang-15. T

The CI run confirms this. I'm tempted to resolve this by upgrading to a more current CI image: clang-15 is pretty dated (we're at v18 now) and the distro (focal) it is in also isn't really state of the art any more (2 LTS versions down to latest/Noble). What's your take @SWilson4 ? Doing that of course would mean updating PLATFORMS.md... Also, I'd like to suggest bringing this into 0.11.0 to avoid the hassle of yet another release cycle under the LF limitations.

rtjk commented 3 months ago

Thank you for your feedback @baentsch! I have just opened a PR to oqs-provider. Please note that the OIDs in there are currently placeholders. We are in the process of obtaining new, official OIDs for CROSS, do you have any suggestions or insights on the process? If everything else looks good to you, I'm ready to mark the PR for review.

SWilson4 commented 3 months ago

in GithHub Actions test_leaks.py is failing on the parameter set rsdp-192-avx2 when compiling on Ubuntu with clang-15. T

The CI run confirms this. I'm tempted to resolve this by upgrading to a more current CI image: clang-15 is pretty dated (we're at v18 now) and the distro (focal) it is in also isn't really state of the art any more (2 LTS versions down to latest/Noble). What's your take @SWilson4 ? Doing that of course would mean updating PLATFORMS.md... Also, I'd like to suggest bringing this into 0.11.0 to avoid the hassle of yet another release cycle under the LF limitations.

I agree that we ought to upgrade, and I'm fine with removing the clang-15 check when we do so. I also agree that it would be best done before the next release---it's in the milestone, after all.

I think at this point I'm the one most familiar with liboqs CI, so I'll volunteer to look at it.

baentsch commented 3 months ago

At first glance, this looks good. Before a final review (ideally after the CI upgrade landed that @SWilson4 volunteered doing (Thanks!)), though, can I ask you @rtjk to check why Zephyr fails (best, resolve that) and also to rebase the PR such as that the commits disappear that are not related to Cross as otherwise it seems we'd review already reviewed code?

rtjk commented 3 months ago

can I ask you @rtjk to check why Zephyr fails (best, resolve that) and also to rebase the PR

Zephyr tests are now passing after disabling CROSS variants with large-stack-usage, I also rebased the PR. Is it possible to trigger a new CI run @baentsch?

baentsch commented 3 months ago

Is it possible to trigger a new CI run @baentsch?

Just done

baentsch commented 3 months ago

@rtjk any updates from your side? It seems a re-base is now necessary as well as a CI failure seems relevant/requiring investigation. Please note the proposal to not wait for this PR before doing the next liboqs release. Please let me know if you'd need help to move this forward; also please note that https://github.com/open-quantum-safe/oqs-provider/pull/461 must be OK before this can merge. If you don't have time to work on this, letting us know would be nice so we can plan accordingly (and in my eyes thus "justify" including only Mayo as the only new NIST on-ramp algorithm in the next release).

rtjk commented 3 months ago

Hey @baentsch thanks for the heads up!

In liboqs the only test failing is on Ubuntu with clang-15, @SWilson4 has volunteered to upgrade to newer Ubuntu and clang releases.
I'm looking into the CI failures in oqs-provider, I count on solving this soon since all tests were passing locally.
Regarding the inclusion of CROSS in the next release (0.11.0 planned for early September), I fully trust your judgment since I'm new to the project.

Please let me know if there's anything else I can do.

rtjk commented 3 months ago

Thank you so much for the feedback @baentsch! I'm glad the integration was well received. The documentation is indeed well written and was a great help throughout the process. Regarding the comments on licensing and future maintenance, I'm waiting for internal discussion with the CROSS team and will respond soon.

baentsch commented 3 months ago

@rtjk see a possibly relevant CI failure above

baentsch commented 3 months ago

Thanks for the test harness update, @rtjk . Now really looking good to me (to merge). Again, reminder to @SWilson4 @praveksharma @dstebila to add second review.

praveksharma commented 3 months ago

Thank you for the PR @rtjk! The integration looks very well done.

I've not gone through all the code but I had question: I could not find a secure memory clearing function, instead the code makes use of memset throughout, is that intentional? Some of the other algorithms we import provide such a function which can then be patched out with OQS_MEM_cleanse. I'm not familiar at all with the CROSS algorithm but if you think secure memory clearing is necessary then implementing it upstream would be very helpful.

In any case, I don't think this ought to block merging, it wouldn't close #1864. Relevant fixes, if required, could be pulled in another PR.

I'll continue going through the code and follow up if necessary.

praveksharma commented 2 months ago

While I've not gone through all the cryptographic code in a lot of detail, the PR looks good to me.

praveksharma commented 2 months ago

There was a merge conflict related to cbom.json which I have resolved. I will merge now.

open-quantum-safe / liboqs

Add CROSS #1881