Argyle-Software / kyber

A rust implementation of the Kyber post-quantum KEM
https://docs.rs/pqc_kyber/
Apache License 2.0
171 stars 40 forks source link

Replace KAT feature with rustflags #26

Closed faern closed 1 year ago

faern commented 2 years ago

Exposing the test-only KAT stuff via a user facing cargo feature is IMHO not optimal. Users are never supposed to use it. So better to not expose it. Here I instead went the route that loom uses and recommends: https://docs.rs/loom/latest/loom/#running-loom-tests. To inject a --cfg <some name> to the build when you want the crate to compile for a certain type of testing.

faern commented 2 years ago

This is technically a breaking change. Since removing a feature is breaking the API. But it's a grey area since users are not supposed to use the KAT feature anyway. Could be considered a non-breaking change without too much fuzz I guess.

mberry commented 1 year ago

I was a bit ambivalent on this at first as it's still exposing the chance of misuse for the end-user.

Have come around to recognise this is actually quite neat and clean, never felt comfortable about the feature hack to expose what should always be private internals (it's there in the comments!).

Just for note, previously considered alternatives to the KAT feature:

1. Using the seed generation code itself rather than just generating it from the NIST submission C codebase into flat files. 2. Moving Known Answer Tests inline rather than as integration tests.

I'm not a fan of either and this is more elegant but just putting it out there for posterity.

faern commented 1 year ago

I'm glad you like it. I feel like the first priority is to get the KAT tests out of the public API of the crate (a feature is a public thing). Then there can be more work done to clean it up better without worrying about breaking changes. One way you/we can go is to do something similar to what classic-mceliece-rust does: https://github.com/Colfenor/classic-mceliece-rust/blob/ed63bb70de21d4cc66962660f5cf1d90a1b14d7d/src/lib.rs#L33-L34. It has a KAT-compatible RNG that is only included in the crate when #[cfg(test)] is true. And the KAT tests are isolated to tests and an example/ binary. That way it's never part of the crate API when building for not-tests.

faern commented 1 year ago

Is there anything left I need to do here? Any chance of getting it merged?

mberry commented 1 year ago

Sorry for the delay, I was hoping to roll up a few other changes and bump up to v0.4 but haven't had any time to work on the library lately.

Everything looks good, needs updating the main and testing readmes but I can fix that up.

Thanks for the PR, trying to keep them on dev branch though if you can remember for next time, cheers!

faern commented 1 year ago

Oh. Didn't know there was a development branch. I usually don't work that way and master was the default branch of the repo. Ok, will try to remember that.

mberry commented 1 year ago

No problems hey, just trying to keep it organised and stay on top of changes. Really I could lock the branch but also submit changes to main sometimes also, it's convenient.

Aiming for a stable 1.0 sometime mid next year, there's nothing outstanding with the internals apart from a migration to Rustcrypto's bitsliced AES for 90's mode. Still a bit of work on the exterior to find a nice balance with traits and library interop.

I do lament making breaking changes having been on the other end of them many times but there will probably be a few more leading up to v1.