-
Notifications
You must be signed in to change notification settings - Fork 63
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
Update PQ Algorithms #165
Update PQ Algorithms #165
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks again for this substantial contribution! Conceptually, this LGTM, particularly the thoroughness of explanations. These IMO should be reflected in the README and in CI for users not normally checking PR comments, though: For example the use of the final alg name mlkem combined with the guarantee that it'll move off the ipd variant as/when available. Also, what about adding a CI job to do what you "locally" did (enable other sig algs) to ensure all are triggered (automatically)?
The CI jobs are failing due to a timeout from Circle CI. Cut issue #166 to track. Not sure how you want to deal with that in the short term.
Good idea, I'll update the README with some more notes.
This limitation predates me and is exactly why the flags to disable algorithms exist in the generator. That said, I'm inclined to cut this as an independent issue. It's more complicated now that we have to deal with issue #166. Setting that aside though, it would require some non-trivial development to either refactor the logic around the bit-mask to support more algorithms or build out some CI logic to patch/regenerate/recompile/test against a different slice of algorithms. |
9599761
to
4fc0fda
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
PR looks good to me. Couple of questions:
- We used to document this update process in this wiki page. It hasn't been updated in a while. Is it still current? Could you update it if need be (add new or remove stale info...)
- We should also bump the version per this (low on details!) wiki page
- I'm approving now with merging blocked by failing CI. As discussed in the meeting today, we can change to GH Actions before merging this PR and re-running the tests, but if this is too much effort and we'd like to land this first, we could discuss alternative options (e.g., only test one param set for each alg).
First time seeing that. It is still up to date and would've been helpful for me figuring this all out from source. 😅
Yes, I plan on cutting a new release and making all the little edits once all these bigger changes are merges. In addition to this PR, I still want to get x25519 hybrid support and the sntrup761 interop merged.
We've got a path forward on CI so I'm hopeful we can just wait on that. |
Sigh & apologies @geedo0 -- when editing that the last time I still thought highly of Wikis. By now I have moved such information into the repo itself, e.g., https://github.com/open-quantum-safe/oqs-provider/blob/main/DEVELOPMENT.md and would recommend you consider doing that here too to avoid "posterity"/future new contributors from falling into the same trap again. |
This aligns the algorithms supported by the OQS OpenSSH fork with the algorithms available in liboqs. - Added support for ML-KEM-IPD using the ML-KEM alias and the names/curves documented by the [PQ SSH draft RFC](https://datatracker.ietf.org/doc/draft-kampanakis-curdle-ssh-pq-ke/). - Added support for ML-DSA-IPD using the ML-DSA alias following the conventions from the Dilithium implementation. The Dilithium signatures have been disabled. - Updated from BIKE Round 3 to Round 4. This adds a new parameter set for BIKE-L5. - Added support for the Falcon PADDED format. It is disabled by default to preserve entries in the bit-mask. - Added support for MAYO. The level 1 variant is left disabled. The KEX algorithms specified by the draft RFC do not match the security level pairing convention followed by Kyber so we chose to prefer the pairings specified in the document. There were no proposed parameter sets for ML-KEM-512 so we defaulted to the conventional parameters. x25519 hybrids are currently not handled by the oqs-openssh fork so we've omitted `mlkem768x25519-sha256` support for now. This does not add support for sntrup761 as that requires handling the conflict with the upstream OpenSSH implementation. Related to Issue open-quantum-safe#163 Signed-off-by: Gerardo Ravago <[email protected]>
This aligns the algorithms supported by the OQS OpenSSH fork with the algorithms available in liboqs.
The KEX algorithms specified by the draft RFC do not match the security
level pairing convention followed by Kyber so we chose to prefer the
pairings specified in the document. There were no proposed parameter
sets for ML-KEM-512 so we defaulted to the conventional parameters.
x25519 hybrids are currently not handled by the oqs-openssh fork so
we've omitted
mlkem768x25519-sha256
support for now.This does not add support for sntrup761 as that requires handling the conflict with the upstream OpenSSH implementation.
Related to Issue #163
Testing
Since we are limited (by the size of the bitmask) in the number of signatures we can support at compile time, we can't test compile the full set of signatures at all times. To work around this, I privately enabled all new algorithms which are disabled in this PR (MAYO-1, Falcon-padded) and disabled a few of the existing algorithms to make space. After that, I simply generated, compiled, and ran the standard test script to make sure they all worked.