integritychain / fips203

Pure Rust implementation of FIPS 203 Module-Lattice-based Key-Encapsulation Mechanism Standard for server, desktop, browser and embedded applications.
Apache License 2.0
6 stars 1 forks source link

Please update from IPD to released FIPS203 #16

Closed dkg closed 1 day ago

dkg commented 2 weeks ago

Earlier this month, NIST finalized the release of FIPS203.

According to ยงC.2 of the final FIPS203 the changes from the Initial Public Draft (IPD) are:

Based on comments submitted on the draft ML-KEM, domain separation was added to K-PKE.KeyGen to prevent the misuse of keys generated to target one security level from being used for a different security level when saving a key as a seed.

Additionally, FIPS 203 ipd had inadvertently swapped the indices of matrix ๐€ฬ‚ in K-PKE.KeyGen and K-PKE.Encrypt. This was changed back in the final version of ML-KEM to match CRYSTALS-KYBER.

other notes:

I believe that this doesn't change anything related to the usable public API of the crate, so it doesn't require an incompatible SemVer bump. That said, if you want to bump the version to 1.0 to reflect the release of the final version, or to acknowledge the incompatibility in ๐€ฬ‚, that could also be a reasonable approach.

eschorn1 commented 2 weeks ago

Yes, I expect to get an update underway fairly soon here.

Note that the new version does mention an additional API that provides seeds for keygen. I need to think about how to best 'express' that (as I don't care for exposing internal functions as the spec suggests).

Once I can get some time and initial test vectors I'll drop a new minor version update. I'll then make an extensive pass with far more testing resulting in a new major version update. At least that is my current thinking...having the current major version of 0 gives me some leeway.

integritychain commented 1 week ago

Hey @dkg -- I just made commit 269e974 which updates the functionality to the released FIPS 203 spec and passes the NIST vectors. It includes a new API keygen_with_seed(). It will become v0.4.0 once I get through updating the comments, revising some of the internal tests and supporting code.

dkg commented 1 week ago

thanks @integritychain !

In addition to keygen_with_seed, I'm wondering whether it makes sense to be able to have a keygen that exposes the seed directly, in addition to the enc/dec pairing. I'm hearing from LAMPS and OpenPGP folks that they're likely to try to use the seed itself as a storage format, and it might be preferable to help them generate a seed safely/automatically, without requiring them to know how to pick an RNG appropriately.

The other question (from a performance perspective) is whether it's cheaper to generate the full-form secret key material from the seed without generating the public key -- that'd be a different operation than keygen, read strictly, but i don't want to get into premature optimizations without measurements.

dkg commented 1 week ago

at the very least, exposing a gen_seed function could be useful for ensuring that the same RNG is used by an implementation that uses both in keygen and keygen_with_seed.

integritychain commented 1 week ago

Hey @dkg fyi the refactoring is progressing well, including aligning the internal code comments with spec updates, and the internal refactoring of the ml_kem functions. Hopefully, updating the supporting code should progress smoothly.

Btw, I realized that keygen_from_seed() is likely clearer than keygen_with_seed(). As for exposing the seed, the code re-exports the RNG functions/trait necessary to generate the seeds, so I was intended to heavily document how this can be (externally) done in the first instance. Unwinding public API isn't fun ;( so I wanted to think a bit further before adding.

dkg commented 5 days ago

i agree we don't want to expose anything fancy that we'd regret later. But it seems to me like there are two possible scenarios: โ“ generating seeds is super complicated, or โ“‘ generating seeds is trivial.

If it's trivial, then a simple trivial gen_seed wrapper doesn't seem like such a problem; it should map cleanly and obviously to whatever the trivial bits are. If turns out that it's complicated, then that's all the more reason to do it right and expose a simple interface. And of course as long as the seed type itself is straightforward (like an array of 32 or 64 u8s) then someone who wants to use their own custom seed generation routine can always come up with it externally too. No need to make the interface complex with passing in an RNG or anything like that. The point of the simple interface is to document a simple default, and let people with more complex needs figure them out for themselves. (that's how i see it anyway)

integritychain commented 1 day ago

Hey @dkg Generating the seeds is trivial, see the example https://github.com/integritychain/fips203/blob/main/src/traits.rs#L107

For now, I have published v0.4.0 aligning with the final spec, but without seed_gen as I'd like to ask around a bit further. Let's give it some time to settle and we can open another thread...thanks for your eyes on this!