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.

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

Add checks for ML-KEM keys #2009

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

abhinav-thales
Copy link
Contributor

This PR introduces the sanity checks for encapsulation and decapsulation keys introduced in the final specification of FIPS-203 in section 7.

The changes include:
- checking generated encaps and decaps key in the keypair step
- checking encaps key before the encapsulation step
- checking decaps key before the decapsulation step

Ideally this check should be in the ML-KEM source code but I am not sure if we allowed to make changes to the upstream code. Kindly let me know otherwise and will move the code accordingly.

Signed-off-by: Abhinav Saxena <[email protected]>
Signed-off-by: Abhinav Saxena <[email protected]>
Signed-off-by: Abhinav Saxena <[email protected]>
@baentsch
Copy link
Member

baentsch commented Dec 3, 2024

Thanks for this improvement @abhinav-thales . It seems the issue tracking this is #1951:

if we allowed to make changes to the upstream code

We are allowed to do anything :-) The question is whether the upstream takes such changes. More seriously: liboqs has a patch mechanism to insert code not yet made available by the upstreams but we're not over-eager to use that facility as it's introducing more work in a place where it conceptually doesn't belong.

Anyway, please see the discussion in the issue above. This PR could serve as a test stopgap measure and as such imo is mergeable as-is. What's your take @bhess: Also do a patch or wait for the upstream?

@bhess
Copy link
Member

bhess commented Dec 3, 2024

I agree that including key validation tests makes sense. As you pointed out, the pq-crystals upstream doesn't include these validations, so we’d need to rely on our patch mechanism, which would increase maintenance overhead on the liboqs side.

One alternative to consider is using this as an opportunity to start adopting the https://github.com/pq-code-package/mlkem-native implementation from PQCP. It recently had its first (alpha) release and is actively maintained. This implementation already includes the required key validations [1][2], and the advantage is that it would eliminate the need for us to maintain custom patches.

[1] https://github.com/pq-code-package/mlkem-native/blob/112dbd3d3c42aa6558a448bb80affe5f8c158ece/mlkem/kem.c#L36
[2] https://github.com/pq-code-package/mlkem-native/blob/112dbd3d3c42aa6558a448bb80affe5f8c158ece/mlkem/kem.c#L64

@baentsch
Copy link
Member

baentsch commented Dec 3, 2024

One alternative to consider is using this as an opportunity to start adopting the https://github.com/pq-code-package/mlkem-native implementation from PQCP

Why not -- not a new issue, though -- my thoughts stated half a year ago still stand/may be worth while addressing then: pq-code-package/tsc#15 (comment) : Technically, this could be simple, no? PQCP adds "copy-from-upstream" YML files and OQS adds it as another upstream (assuming some form of agreement on APIs, of course :) My strong preference would be to then drop other upstreams to indeed make life easier and not more complicated with this addition.

@baentsch
Copy link
Member

baentsch commented Dec 3, 2024

Additional thought: As and when there are different algorithm code bases becoming available and if OQS would still want to extend its utility, wouldn't this be the perfect opportunity to finally introduce the notion of "upstream maintained" to the YML file as a way to signal code (bases) that someone may rely on for use beyond research & prototyping? Given the various professionally maintained code bases for standardized algorithms already available or with a clear plan to do so, I'm personally less and less convinced this goal is a sensible one for OQS to pursue, though -- but wouldn't want to rule it out and not lay foundations for that -- like with such distinction of actively supported upstream code bases that would make it into a "reliable build" (vs. the current "amalgam/best effort" one).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants