Add CROSS

[rtjk]

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.)
• 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]

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]

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]

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]

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]

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?

[bhess]

Thank you @rtjk for this work integrating CROSS!
The diff is a bit difficult to follow because there seem to be some commits included that are already on main. Is it possible to do a rebase/merge so that only the actual changes are visible? (edit: just noticed this is a duplicate request from @baentsch's above)

[rtjk]

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]

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

Just done

[baentsch]

@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 open-quantum-safe/oqs-provider#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]

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.

[baentsch]

Nicely done enabling use of our upstream integration logic! Do you intend to eventually contribute this to PQClean itself, too (i.e., change this link to the PQClean repo), adding some more testing and maintenance by the team there? What statement regarding maintenance of Cross can you give? Would you agree becoming a liboqs contributor and codeowner for Cross? If so, please indicate this by adding yourself to the CONTRIBUTORS file in an updated PR. Please also add the commit tag/message "[trigger downstream]" such as to trigger testing within oqs-provider.

[rtjk]

The team is definitely considering opening a PR to PQClean as well. We'll make sure to update the upstream if/when this happens.

Regarding maintenance: I'm working on this integration for my master thesis, I'd like to also add my advisors to the CONTRIBUTORS file. You can see them as authors in CROSS' header comments and they would be the main point of reference for any future development. Is this ok @baentsch?

[baentsch]

If those advisors are not really (continued/committed) contributors in the sense of OQS (regularly willing to do PRs as well as review those within OQS) (?) it'd indeed be best to add them (like yourself, given you're probably not continuing to work on this after your thesis (or are you?)) by reference only to the CONTRIBUTORS file (probably best with email address in case one needs to reach out privately, e.g., in case of security problems) but not to CODEOWNERS (implying a longer-term commitment): OK?

[baentsch]

Just in case the question above didn't hit your Inbox, tagging @rtjk explicitly on this (to me final for this PR) open question.

[rtjk]

Apologies for the delay @baentsch, I'm waiting for feedback from the team on this, I will get back to you soon and update codeowners/committers here and in oqs-provider. Thanks for your patience!

[rtjk]

@alexrow from the CROSS team, who actively maintains the main repo, has assisted me throughout this integration and is willing to oversee CROSS' evolution in liboqs, review pull reuqests, and manage the OIDs in oqs-provider. I just added him as codeowner for copy_from_upstream.yml and the CROSS subdirectory.

[baentsch]

Thanks for this commitment and welcome @alexrow! The upstream reference I see though, is to a repo owned by @rtjk : Is that something that should be changed then in a future PR or before Cross is integrated to a liboqs release (happening very soon)?

[rtjk]

Yes,

• Repo 1 hosts the unaltered NIST submission package.

• Repo 2 hosts the lboqs upstream, with all the changes that were necessary (e.g. switching to fixed sized integer types), as well as the code generation script that produces the 18 subdirectories (one for each parameter set) and applyes namespacing.

we might merge the two in the future, in which case we'll make sure to open a new PR to change the upstream. Tagging @alexrow to confirm this.

[alexrow]

I do confirm that we're planning a restructuration of the repositories owned by the CROSS github organization; the expected plan is that, we will move Repo 2 under its ownership, while keeping the structure, so that it can act as an upstream for liboqs.

I also confirm that, once the state of the repos is changed, we'll submit a PR to change the upstream.

Thanks for all the work on liboqs!

[baentsch]

LGTM. I'm impressed by your "unassisted" integration of a new algorithm family, @rtjk ! Maybe our documentation is good :) Please see the single comments to improve things further (in the future, if you cannot resolve them right away). I also manually ascertained that open-quantum-safe/oqs-provider#461 passes : Well done there, too. @dstebila @SWilson4 @praveksharma Second review, please. Please also note that I did not review the actual code but leave that to a cryptographer.

[rtjk]

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]

@rtjk see a possibly relevant CI failure above

[baentsch]

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]

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]

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

[praveksharma]

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