liboqs icon indicating copy to clipboard operation
liboqs copied to clipboard

Add MAYO signature scheme from NIST onramp

Open bhess opened this issue 1 year ago • 12 comments

Adds MAYO signature scheme from the NIST onramp. The upstream implementation contains a C and an AVX2 implementation.

  • [x] C code import
  • [x] AVX2 code import
  • [x] docs update
  • [x] constant-time tests
  • [x] adds aes128ctr common code
  • [x] Merge some changes upstream
  • [x] tbc: add cmake switch to compile onramp algorithms: NIST_SIG_ONRAMP
  • [x] fix a few failing builds
  • [ ] 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 and OQS-OpenSSH will also need to be ready for review and merge by the time this is merged.)

bhess avatar Feb 26 '24 19:02 bhess

I'll also do a branch in oqs-provider to do the downstream test, other than this the PR is ready.

bhess avatar May 13 '24 11:05 bhess

I'll also do a branch in oqs-provider to do the downstream test, other than this the PR is ready.

That would be good. Please remember to use a "-tracker" branch name such as to trigger it from liboqs (this PR, best) prior to the merge (as per the wiki).

baentsch avatar May 13 '24 12:05 baentsch

I'll also do a branch in oqs-provider to do the downstream test, other than this the PR is ready.

That would be good. Please remember to use a "-tracker" branch name such as to trigger it from liboqs (this PR, best) prior to the merge (as per the wiki).

Added the -tracker branch in oqs-provider. Everything works when running locally. The [trigger downstream] test still fails, apparently because of access rights.

bhess avatar May 14 '24 09:05 bhess

apparently because of https://github.com/open-quantum-safe/liboqs/issues/1789#issuecomment-2109640801.

Tagging @ryjones

baentsch avatar May 15 '24 07:05 baentsch

Added the -tracker branch in oqs-provider. Everything works when running locally.

It would be good if PR could confirm this: OQS either uses and relies on CI (after getting it right) or it skips/does away with it and merges code based on local run results. The latter probably wouldn't be a step in a direction of a project aiming for "more productive use".

baentsch avatar May 17 '24 04:05 baentsch

Thank you @SWilson4 for the review! I'll update the PR per the discussion.

It would be good if PR could confirm this: OQS either uses and relies on CI (after getting it right) or it skips/does away with it and merges code based on local run results. The latter probably wouldn't be a step in a direction of a project aiming for "more productive use".

Agree with the former and ok to hold on for green CI.

bhess avatar May 17 '24 12:05 bhess

Hi,

I come across this PR and I have a few questions:

  1. Can we merge the MAYO_*_opt into a single folder, so it's easier to review?
  2. Can we merge the MAYO_*_avx2 into a single folder, share source code and different by macro, so it's easier to review?
  3. Is there a reason to have 6 directories instead of 2?

cothan avatar May 20 '24 01:05 cothan

I come across this PR and I have a few questions:

  1. Can we merge the MAYO_*_opt into a single folder, so it's easier to review?
  2. Can we merge the MAYO_*_avx2 into a single folder, share source code and different by macro, so it's easier to review?
  3. Is there a reason to have 6 directories instead of 2?

Hi @cothan, the integration basically follows the "pqclean style" of having a single folder per variant/optimization. The import of the MAYO code from upstream is automated with copy_from_upstream that also follows this pattern. I agree having a single folder sharing the implementation would be nice to avoid code duplication, but I think it would need quite some refactoring of copy_from_upstream. Do you want to open this as a separate issue (and tackle this separately)?

bhess avatar May 21 '24 08:05 bhess

Thanks for the answer @bhess . Well, it's from the copy_from_upstream then we don't need to change.

cothan avatar May 21 '24 08:05 cothan

Thanks for the answer @bhess . Well, it's from the copy_from_upstream then we don't need to change.

Well, the proposal by @bhess makes sense: liboqs is way too large/wasteful for practical deployment due to this limitation. It would be a good thing for the OQS team to consider such enhancement if it wanted to move the project to real world deployment (and limit the exposure to flaws in code by simply limiting the amount of code). For further experimentation-only use, I agree with @cothan though: Leave as-is.

baentsch avatar May 21 '24 09:05 baentsch

It would be good if PR could confirm this: OQS either uses and relies on CI (after getting it right) or it skips/does away with it and merges code based on local run results. The latter probably wouldn't be a step in a direction of a project aiming for "more productive use".

Agree with the former and ok to hold on for green CI.

CI is working again.

SWilson4 avatar May 23 '24 18:05 SWilson4

The PR is updated and now includes MAYO_5, handling it the same way as the other schemes with large stack usage. Not sure yet about the two failing macOS builds using gcc-13. PR https://github.com/open-quantum-safe/liboqs/pull/1708 seems to fail for the same reason, so I assume it's independent from the changes here. The difference I see from a previous working build is the update of gcc from 13.2.0 -> 13.3.0. I have the same config with gcc 13.3.0 on my local Mac which is working fine, it could be a hickup in the github runner image.

bhess avatar May 30 '24 11:05 bhess

Thanks for this work, @bhess! All of my comments are resolved and the oqs-provider integration tests are passing.

Thanks for the review @SWilson4 !

bhess avatar Jul 11 '24 06:07 bhess

Thanks for this integration, @bhess. Would it be conceivable to use an algorithm naming convention a bit more in line with what we've been using so far, i.e., either no or "-" (dash) name/strength separation? Reasons: 1) Easier to type; 2) easier to remember (for users accustomed to using other liboqs algs); 3) underscores require more backquotes (even in this PR); 4) in oqsprovider this breaks the convention that underscores separate classic from PQC algs... Or is the underscore an important feature for some reason?

I checked with the MAYO team and there was no objection changing the naming convention. The preference is to use "-" as a separator for consistency with the separators used in the FIPS drafts. I'll update this PR and https://github.com/open-quantum-safe/oqs-provider/pull/413.

bhess avatar Jul 11 '24 13:07 bhess

I'll update this PR and open-quantum-safe/oqs-provider#413.

Done

bhess avatar Jul 11 '24 18:07 bhess

Sorry for the merge conflicts introduced by https://github.com/open-quantum-safe/liboqs/pull/1832, @bhess. I think it's a straightforward resolution: just move the AES callback structure updates to the new aes_ops.h file.

SWilson4 avatar Jul 11 '24 18:07 SWilson4

Sorry for the merge conflicts introduced by #1832, @bhess. I think it's a straightforward resolution: just move the AES callback structure updates to the new aes_ops.h file.

Thanks for the hint @SWilson4, resolved the conflict accordingly.

bhess avatar Jul 11 '24 19:07 bhess

Added the -tracker branch in oqs-provider. Everything works when running locally.

Thanks for that downstream addition. Checking it, two questions:

  • Could you please trigger a re-run without "skip ci" so the latest code in this PR gets tested there?
  • It seems that tracker/https://github.com/open-quantum-safe/oqs-provider/pull/413 does not contain Mayo-5, right? Sensible to add and run CI?

baentsch avatar Jul 12 '24 06:07 baentsch

Thanks for the review @baentsch !

  • Could you please trigger a re-run without "skip ci" so the latest code in this PR gets tested there?

Done - the downstream test passes: https://github.com/open-quantum-safe/oqs-provider/actions/runs/9912829274/job/27388523412

Correct, I'll try this tomorrow.

bhess avatar Jul 12 '24 20:07 bhess

Correct, I'll try this tomorrow.

Added MAYO-5 to oqs-provider and downstream tests pass: https://github.com/open-quantum-safe/oqs-provider/actions/runs/9919738195

bhess avatar Jul 13 '24 19:07 bhess