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.

Sign up for GitHub

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

Jump to bottom

Add DeriveKeyPair API #2070

Draft
wants to merge 6 commits into
base: main
Choose a base branch
from
Draft

Add DeriveKeyPair API #2070

wants to merge 6 commits into from

Conversation

SWilson4
Copy link
Member

@SWilson4 SWilson4 commented Feb 7, 2025

Based on the work of @Eddy-M-K in #1877.

Closes #1206.

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

Eddy-M-K and others added 6 commits February 6, 2025 17:37
@Eddy-M-K @SWilson4
Initial derive keypair commit
8364da3 
Signed-off-by: Eddy Kim <Eddy.M.Kim@outlook.com>
Signed-off-by: Eddy Kim <e84kim@uwaterloo.ca>

Add pqcrystals-ml_kem_ipd.patch

Signed-off-by: Eddy Kim <Eddy.M.Kim@outlook.com>
Signed-off-by: Eddy Kim <e84kim@uwaterloo.ca>

Fix encaps key in scheme and revert whitespace changes

Signed-off-by: Eddy Kim <e84kim@uwaterloo.ca>

Hopefully corrected patch file

Signed-off-by: Eddy Kim <e84kim@uwaterloo.ca>

Corrected missing derand in kem_scheme

Signed-off-by: Eddy Kim <e84kim@uwaterloo.ca>

Fix indentation

Signed-off-by: Eddy Kim <e84kim@uwaterloo.ca>

Run copy_from_upstream

Signed-off-by: Eddy Kim <e84kim@uwaterloo.ca>

derand testing tentative changes

Signed-off-by: Eddy Kim <e84kim@uwaterloo.ca>

Add missing function declarations

Signed-off-by: Eddy Kim <e84kim@uwaterloo.ca>

Add template for avx2 derand functions

Signed-off-by: Eddy Kim <e84kim@uwaterloo.ca>

Run copy_from_upstream

Signed-off-by: Eddy Kim <e84kim@uwaterloo.ca>

WIP: Add changes for coin length

Signed-off-by: Eddy Kim <e84kim@uwaterloo.ca>

Update patch to include coin lengths

Signed-off-by: Eddy Kim <e84kim@uwaterloo.ca>
Signed-off-by: Spencer Wilson <spencer.wilson@uwaterloo.ca>

Bootstrap

Signed-off-by: Eddy Kim <e84kim@uwaterloo.ca>
Signed-off-by: Spencer Wilson <spencer.wilson@uwaterloo.ca>

Conditional copy

Signed-off-by: Eddy Kim <e84kim@uwaterloo.ca>
Signed-off-by: Spencer Wilson <spencer.wilson@uwaterloo.ca>

Run copy_from_upstream

Signed-off-by: Eddy Kim <e84kim@uwaterloo.ca>
Signed-off-by: Spencer Wilson <spencer.wilson@uwaterloo.ca>

Separate coins variable into two distinct variables

Signed-off-by: Eddy Kim <Eddy.M.Kim@outlook.com>
Signed-off-by: Eddy Kim <e84kim@uwaterloo.ca>
Signed-off-by: Spencer Wilson <spencer.wilson@uwaterloo.ca>

Add derand fixes

- Add support for BIKE, FrodoKEM, sntrup
- Add hooks for testing
- Add missing kem comment to documentation
- Don't run decaps() in test_kem_derand if encaps_derand() fails
- Add markdown documentation changes

Signed-off-by: Eddy Kim <e84kim@uwaterloo.ca>
Signed-off-by: Spencer Wilson <spencer.wilson@uwaterloo.ca>

WIP trying to fix build errors

Signed-off-by: Eddy Kim <e84kim@uwaterloo.ca>
Signed-off-by: Spencer Wilson <spencer.wilson@uwaterloo.ca>

Fix remaining build issues

Signed-off-by: Eddy Kim <e84kim@uwaterloo.ca>
Signed-off-by: Spencer Wilson <spencer.wilson@uwaterloo.ca>

Resolve unused parameter issues for BIKE

Signed-off-by: Eddy Kim <e84kim@uwaterloo.ca>
Signed-off-by: Spencer Wilson <spencer.wilson@uwaterloo.ca>

Resolve unused paramter issues for FrodoKEM

Signed-off-by: Eddy Kim <e84kim@uwaterloo.ca>
Signed-off-by: Spencer Wilson <spencer.wilson@uwaterloo.ca>

Fix whitespace inconsistency

Signed-off-by: Eddy Kim <e84kim@uwaterloo.ca>
Signed-off-by: Spencer Wilson <spencer.wilson@uwaterloo.ca>

Fix whitepace issue

Signed-off-by: Eddy Kim <e84kim@uwaterloo.ca>
Signed-off-by: Spencer Wilson <spencer.wilson@uwaterloo.ca>

Insert unused attributes

Signed-off-by: Eddy Kim <e84kim@uwaterloo.ca>
Signed-off-by: Spencer Wilson <spencer.wilson@uwaterloo.ca>

Void all unused parameters

Signed-off-by: Eddy Kim <e84kim@uwaterloo.ca>
Signed-off-by: Spencer Wilson <spencer.wilson@uwaterloo.ca>

Use tab instead of spaces in kem_scheme

Signed-off-by: Eddy Kim <e84kim@uwaterloo.ca>
Signed-off-by: Spencer Wilson <spencer.wilson@uwaterloo.ca>

Run copy_from_upstream

Signed-off-by: Eddy Kim <e84kim@uwaterloo.ca>
Signed-off-by: Spencer Wilson <spencer.wilson@uwaterloo.ca>

Fix kem_derand python tests

Signed-off-by: Eddy Kim <e84kim@uwaterloo.ca>
Signed-off-by: Spencer Wilson <spencer.wilson@uwaterloo.ca>

Initialize coins in test_kem_derand

Signed-off-by: Eddy Kim <e84kim@uwaterloo.ca>
Signed-off-by: Spencer Wilson <spencer.wilson@uwaterloo.ca>
@SWilson4
Update patch to work with mlkem-native
38106c1 
Signed-off-by: Spencer Wilson <spencer.wilson@uwaterloo.ca>
@SWilson4
Update docs generation and templating
aec9812 
Signed-off-by: Spencer Wilson <spencer.wilson@uwaterloo.ca>
@SWilson4
Run copy_from_upstream [full tests] [extended tests]
38b38dc 
Signed-off-by: Spencer Wilson <spencer.wilson@uwaterloo.ca>
@SWilson4
Don't call randombytes on zero-length arrays
e07eba3 
Signed-off-by: Spencer Wilson <spencer.wilson@uwaterloo.ca>
@SWilson4
Run format script
eee5e13 
Signed-off-by: Spencer Wilson <spencer.wilson@uwaterloo.ca>
bifurcation
bifurcation approved these changes Feb 7, 2025
View reviewed changes
Copy link

@bifurcation bifurcation left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall, this looks right to me. Couple of minor naming things and questions.

docs/algorithms/kem/bike.md
| BIKE-L1 | NA | IND-CPA | 1 | 1541 | 5223 | 1573 | 32 |
| BIKE-L3 | NA | IND-CPA | 3 | 3083 | 10105 | 3115 | 32 |
| BIKE-L5 | NA | IND-CPA | 5 | 5122 | 16494 | 5154 | 32 |
| Parameter set | Parameter set alias | Security model | Claimed NIST Level | Public key size (bytes) | Secret key size (bytes) | Ciphertext size (bytes) | Shared secret size (bytes) | Keypair coins (bytes) | Encapsulation coins (bytes) |
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
| Parameter set | Parameter set alias | Security model | Claimed NIST Level | Public key size (bytes) | Secret key size (bytes) | Ciphertext size (bytes) | Shared secret size (bytes) | Keypair coins (bytes) | Encapsulation coins (bytes) |
| Parameter set | Parameter set alias | Security model | Claimed NIST Level | Public key size (bytes) | Secret key size (bytes) | Ciphertext size (bytes) | Shared secret size (bytes) | Keypair seed (bytes) | Encapsulation seed (bytes) |

Nit: "Seed" seems to be the terminology that folks are coalescing around. See, e.g., this IETF presentation

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it necessary to have the encapsulation coins / seed as well? As far as I am aware, only the deterministic keypair generation is used in real applications; deterministic encapsulation is only done for testing.

docs/algorithms/kem/kyber.md
| Kyber1024 | NA | IND-CCA2 | 5 | 1568 | 3168 | 1568 | 32 |
| Parameter set | Parameter set alias | Security model | Claimed NIST Level | Public key size (bytes) | Secret key size (bytes) | Ciphertext size (bytes) | Shared secret size (bytes) | Keypair coins (bytes) | Encapsulation coins (bytes) |
|:---------------:|:----------------------|:-----------------|---------------------:|--------------------------:|--------------------------:|--------------------------:|-----------------------------:|:------------------------|:------------------------------|
| Kyber512 | NA | IND-CCA2 | 1 | 800 | 1632 | 768 | 32 | NA | NA |
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't these be the same as the ML-KEM entries? Though I guess it would make sense to introduce the new API service with ML-KEM as the sole example; clearly we can upgrade other KEMs later.

scripts/copy_from_upstream/copy_from_upstream.py
@@ -200,6 +200,10 @@ def load_instructions(file='copy_from_upstream.yml'):
scheme['upstream_location'] = family['upstream_location']
if (not 'arch_specific_upstream_locations' in scheme) and 'arch_specific_upstream_locations' in family:
scheme['arch_specific_upstream_locations'] = family['arch_specific_upstream_locations']
if (not 'derandomized_keypair' in scheme) and 'derandomized_keypair' in family:
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: I would change "derandomized" to "deterministic".

scripts/copy_from_upstream/src/kem/family/kem_scheme.c
Comment on lines +196 to +199
(void)public_key;
(void)secret_key;
(void)coins;
return OQS_ERROR;
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just to confirm -- this provides a default always-error implementation if the back-end KEM doesn't supply one, right? And likewise below for encap().

@bifurcation
Copy link

This PR seems pretty mature. What still needs to be done to merge it?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bifurcation bifurcation bifurcation approved these changes

@bhess bhess Awaiting requested review from bhess bhess will be requested when the pull request is marked ready for review bhess is a code owner

@dstebila dstebila Awaiting requested review from dstebila dstebila will be requested when the pull request is marked ready for review dstebila is a code owner

@brian-jarvis-aws brian-jarvis-aws Awaiting requested review from brian-jarvis-aws brian-jarvis-aws will be requested when the pull request is marked ready for review brian-jarvis-aws is a code owner

@baentsch baentsch Awaiting requested review from baentsch baentsch will be requested when the pull request is marked ready for review baentsch is a code owner

@alexrow alexrow Awaiting requested review from alexrow alexrow will be requested when the pull request is marked ready for review alexrow is a code owner

@praveksharma praveksharma Awaiting requested review from praveksharma praveksharma will be requested when the pull request is marked ready for review praveksharma is a code owner

At least 2 approving reviews are required to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Adding a DeriveKeyPair functionality
3 participants
@SWilson4 @bifurcation @Eddy-M-K