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

ml-kem: Adds feature flag to use key or seed #83

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

Conversation

supinie
Copy link
Contributor

@supinie supinie commented Jan 7, 2025

Resolves #53

I saw that @bifurcation mentioned that the test vectors should work, but they appeared to be in the wrong format. For now I have disabled them in the same way that deterministic does.

I have tried to write it in such a way that the user always deals with a DecapsulationKey type no matter whether this is a true key or seed to maximise backwards compatibility.

begin refactor

adds feature flag to use key or seed

minor fixes to no-default

disables acvp tests for seed

adds missing conditional

reset actions

properly handle tests

properly negate tests to stop clippy warnings
@tarcieri
Copy link
Member

tarcieri commented Jan 7, 2025

Really seeds should be the "default" API, not something that's enabled by a feature.

I think it's OK to make breaking changes towards that end.

@supinie
Copy link
Contributor Author

supinie commented Jan 7, 2025

Really seeds should be the "default" API, not something that's enabled by a feature.

I think it's OK to make breaking changes towards that end.

I can easily swap this around - will put together a new commit now.

@tarcieri tarcieri requested a review from bifurcation January 9, 2025 16:22
@supinie
Copy link
Contributor Author

supinie commented Jan 10, 2025

I just thought, since we now have two feature flags, is it also worth adding a short section on the docs homepage about them?

@supinie supinie changed the title Adds feature flag to use key or seed ml-kem: Adds feature flag to use key or seed Jan 10, 2025
@tarcieri
Copy link
Member

@supinie I would prefer a PR like this be purely additive, as opposed to changing things depending on if the feature is enabled by using cfg(not(...)) gates

@supinie
Copy link
Contributor Author

supinie commented Jan 22, 2025

@tarcieri in that case, would we rather that the DecapsulationKey object is the seed, and we then generate the "inner" key before use, or keep the DecapsulationKey as is, but with the addition of an extra B32 field to contain d?

@tarcieri
Copy link
Member

tarcieri commented Jan 22, 2025

That's a good question.

In ed25519-dalek, where Ed25519 uses a similar sort of seed-based expansion, the DecapsulationKey-alike type is based entirely on the seed (where the SecretKey type is an alias for the [u8; 32] seed), and there's a separate type for the expanded secret key which we conditionally export under the hazmat module when the hazmat feature is enabled.

As things are currently implemented, the seed is always expanded prior to use, and the expanded form is not cached but instead always computed on demand.

I think something similar could work here?

@supinie
Copy link
Contributor Author

supinie commented Jan 23, 2025

I've refactored it to fit this style, but am unable to get the KATs working (encap-decap.rs). Currently, the input is the bytes format of the inner key, which makes it trivial to recover the inner key, but going from the inner key to the seed is not possible. The d seed does not exist in the KAT input.

I have pushed these changes to https://github.com/supinie/KEMs/tree/additive so you can take a look if you'd like.

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.

ml-kem: seed support for DecapsulationKey
2 participants