open-quantum-safe / liboqs

C library for prototyping and experimenting with quantum-resistant cryptography
https://openquantumsafe.org/
Other
1.92k stars 466 forks source link

Use seed as private key format for ML-KEM #1994

Open bifurcation opened 2 weeks ago

bifurcation commented 2 weeks ago

This PR is a WIP demonstrating an approach to storing ML-KEM decapsulation keys in the form of a 64-byte seed, as opposed to the expanded form currently. Currently, the proposed scheme is implemented (a) for ML-KEM-512 only, and (b) in-place, as opposed to defining a new scheme. I am posting it in this state to get feedback on whether the structure of the change looks good, because the remainder of the PR will basically just be copy/pasting to cover the other parameter sets and maybe make a new KEM scheme.

If maintainers could answer the following questions, I can implement the rest of the PR:

Fixes #1985

Tests are currently failing because they are providing / looking for the expanded private key. The ACVP test vectors do provide the seed values (d and z), so it should be possible to fix the tests simply by using those values instead of the expanded values (dk).

kvanals commented 2 weeks ago

I'm tracking this closely, as we are heavily invested in ML-KEM at SoftIron. I'm not really sure what we could do here that would be backwards compatible with existing expanded keys.

As a side note, I'm suspecting changes will be needed in oqs-provider as well?

# openssl genpkey -algorithm mlkem1024 -outform pem -out mlkem.key malloc(): unaligned tcache chunk detected Aborted (core dumped)

bifurcation commented 2 weeks ago

@kvanals - The idea of doing it with backward compatibility would be to define a separate scheme using the same upstream library. And yes, there would probably be changes needed in oqs-provider as well, to enable callers to reference the additional scheme. You would have something like openssl genpkey -algorithm mlkem1024seed ....

kvanals commented 2 weeks ago

@kvanals - The idea of doing it with backward compatibility would be to define a separate scheme using the same upstream library. And yes, there would probably be changes needed in oqs-provider as well, to enable callers to reference the additional scheme. You would have something like openssl genpkey -algorithm mlkem1024seed ....

Completely sensible! Sorry I wasn't tracking that.

baentsch commented 1 week ago

Does the approach shown here look correct from an OQS structure / coding standards point of view?

Yes. A full implementation of course would have to use the code generation concepts of "copy_from_upstream" and pass all NIST tests.

Should this be implemented in the current ML-KEM provider, or as a new provider?

The overarching goal should be to create as little new original code (as opposed to generated code) as possible / be orthogonal to the current logic.

I'm tracking this closely, as we are heavily invested in ML-KEM at SoftIron.

In that case, allow me to invite "SoftIron" to contribute to the work in this project. Right now this project's mission is to support research and prototyping (only) as we don't have the resources to be "everything for everyone", e.g., maintain backwards compatibility (or other "product level" qualities). The proposal @bifurcation kindly works on here is the result of the discussion at #1985 with the goal to work with these limitations. Either way, currently, there is absolutely no guarantee that OQS will keep supporting both types of keys: For example, if the upstream ceases to support both modes, so will OQS. If the general consensus is to only use the seed-based approach, support for expanded keys most likely will vanish.

As a side note, I'm suspecting changes will be needed in oqs-provider as well?

This seems to indicate you want to use this code via OpenSSL. If so, you may want to follow the discussion (or --again-- even better, contribute to the project) at https://github.com/openssl/openssl/issues/25885 where we're using an entirely different code base and aim to only implement the relevant standards for exactly the same reason: Provide the best possible code with the smallest amount of development and maintenance resources possible.

kvanals commented 1 week ago

I'm tracking this closely, as we are heavily invested in ML-KEM at SoftIron.

In that case, allow me to invite "SoftIron" to contribute to the work in this project. Right now this project's mission is to support research and prototyping (only) as we don't have the resources to be "everything for everyone", e.g., maintain backwards compatibility (or other "product level" qualities). The proposal @bifurcation kindly works on here is the result of the discussion at #1985 with the goal to work with these limitations. Either way, currently, there is absolutely no guarantee that OQS will keep supporting both types of keys: For example, if the upstream ceases to support both modes, so will OQS. If the general consensus is to only use the seed-based approach, support for expanded keys most likely will vanish.

Cheers @baentsch, happy to upstream anything where applicable. Presently we're using oqs-provider as-is and only maintain one out-of-tree patch to liboqs which I don't believe would meet your copy_from_upstream requirements to build for some of our ARM build targets that are lacking some of the requisite ARM NEON intrinsics. Let me know if there is anything you'd like us to sign or review in this regard.

As a side note, I'm suspecting changes will be needed in oqs-provider as well?

This seems to indicate you want to use this code via OpenSSL. If so, you may want to follow the discussion (or --again-- even better, contribute to the project) at openssl/openssl#25885 where we're using an entirely different code base and aim to only implement the relevant standards for exactly the same reason: Provide the best possible code with the smallest amount of development and maintenance resources possible.

I absolutely am tracking that. And correct, we do very little work today with liboqs directly, but rather abstract its use behind high level OpenSSL EVP functions.

baentsch commented 1 week ago

Let me know if there is anything you'd like us to sign or review in this regard.

Regarding OQS contributions, all there is is (acceptance of) the license (by way of the DCO "signoff"; #1760). If you'd like to see your code/contributions also become used in OpenSSL, see here.

rather abstract its use behind high level OpenSSL EVP functions.

Good approach. Will allow use of the most well-maintained PQC code base -- whichever that is. FWIW, I like the approach of supporting both key formats via OSSL_PARAM. Which one(s) gets an encoder/decoder will probably be a matter of which format ultimately gets an official/standardized OID assigned. If both do, the approach taken in this PR (creating a "twin" alg set that each could get its own OIDs) is probably the most simple to integrate&use.

bifurcation commented 1 week ago

A full implementation of course would have to use the code generation concepts of "copy_from_upstream" and pass all NIST tests. The overarching goal should be to create as little new original code (as opposed to generated code) as possible / be orthogonal to the current logic.

@baentsch - Happy to work along these lines, but I'm not seeing documentation of how to do this. I see the copy_from_upstream script, but not what the concepts are or how to use it. Likewise, unclear to me what in this code base is generated vs. original code. Any pointers you could provide would be helpful. I'm also in the libOQS Discord if that would be a more convenient place to chat.

kvanals commented 1 week ago

It looks like @baentsch has been working on getting ML-KEM into OpenSSL from BoringSSL's implementation in https://github.com/openssl/openssl/tree/feature/ml-kem

If I'm reading this right, this supports using "OSSL_PKEY_PARAM_MLKEM_SEED" in the manner described above via an OSSL_PARAM.

baentsch commented 6 days ago

It looks like @baentsch has been working on getting ML-KEM into OpenSSL from BoringSSL's implementation in https://github.com/openssl/openssl/tree/feature/ml-kem

If I'm reading this right, this supports using "OSSL_PKEY_PARAM_MLKEM_SEED" in the manner described above via an OSSL_PARAM.

This is a community effort, but yes, that's what we're doing there. As always, feedback welcome to the approach taken (documentation proposal up at https://github.com/openssl/openssl/pull/26037).