search

Colfenor / classic-mceliece-rust

This is a pure-rust safe-rust implementation of the Classic McEliece post-quantum scheme
MIT License
24 stars 11 forks source link

version 2 changes #28

Closed dkales closed 2 years ago

dkales commented 2 years ago

These are the proposed changes for v2. I would like to merge these, so we have the new, better interface in main, and as a public version on crates.

@prokls you would need to release this on crates I guess once merged. @faern any inputs from your side.

I'll work out the conflicts between this and main.

faern commented 2 years ago

Great! I'd love to get the stuff currently in v2 on crates.io! Can you just give me a day or so to try out the current v2 in our product and verify that the current API is actually as ergonomic as we think it is? :sweat_smile:

My only question is regarding #21. I'm for releasing 2.0 without doing that re-write, since it can be quite large. But I then want to first just make sure you still want #21 to get solved, and are OK with going 3.0 once we have fixed it? Just so we don't end up having a good solution to that problem in a month or two but are blocked because "not wanting to break the API so often" or something.

dkales commented 2 years ago

For me it doesn't really matter, its just a number and we probably don't want to backport things to the "broken" 1.0 anyway. For that reason I'm still partial to just yanking the 1.0 and 1.0.1 versions and pretending they never existed in the first place, since I'm pretty sure the only current consumers of this crate are in this thread :P.

Then the new API supporting all of the variants at once should just be the next higher major number, be it 2 or 3.

In the end, it's up to @prokls to decide when to release what.

dkales commented 2 years ago

TODO: Re-run the benchmarks & update the Readme.md. @prokls was this run on your machine?

faern commented 2 years ago

:warning: I realized one problem with the current API.. Lol. It's easy to see no one tried the v2 branch in practice yet :see_no_evil: ... There is no way to create a Ciphertext from raw data. When I receive a ciphertext over protocol x and I want to decapsulate it, I can't.

We need to implement From<[u8; CRYPTO_CIPHERTEXTBYTES]> for Ciphertext or similar.

EDIT: Same also goes for the PublicKey of course. Otherwise it's not possible to implement a server.

faern commented 2 years ago

I can create a PR for that. I just want to check something: decapsulate is infallible. So it should return a shared secret no matter what ciphertext you give it? There are no malformed ciphertexts? Of course it will yield the wrong shared secret if the ciphertext has been corrupted, but yeah.

faern commented 2 years ago

First PR, addressing one very large footgun in the new API: #29. This still does not solve the issue I described above. Still working on that.

dkales commented 2 years ago

I can create a PR for that. I just want to check something: decapsulate is infallible. So it should return a shared secret no matter what ciphertext you give it? There are no malformed ciphertexts? Of course it will yield the wrong shared secret if the ciphertext has been corrupted, but yeah.

If I don't remember wrongly, that is one intentional change in Classic McEliece, that decapsulation errors are removed and replaced with outputting a pseudorandom value.

dkales commented 2 years ago

⚠️ I realized one problem with the current API.. Lol. It's easy to see no one tried the v2 branch in practice yet 🙈 ... There is no way to create a Ciphertext from raw data. When I receive a ciphertext over protocol x and I want to decapsulate it, I can't.

We need to implement From<[u8; CRYPTO_BYTES]> for CRYPTO_CIPHERTEXTBYTES or similar.

EDIT: Same also goes for the PublicKey of course. Otherwise it's not possible to implemente servers.

This seems like a problem 🤦 . I guess for the PublicKey we also want both variants, owned and boxed on alloc. It is probably also good to do this for the secret key, since you probably want to write/load this to/from disk at some point too.

faern commented 2 years ago

All issues I'm aware of are now being fixed in #30

faern commented 2 years ago

I think I'm pretty happy with the state of the v2 branch right now! I have a draft PR up for upgrading to this version of the crate in our VPN client that uses the library: https://github.com/mullvad/mullvadvpn-app/pull/3877. It seems to work fine from my testing.

prokls commented 2 years ago

Review in progress. I hope to finish it in about 21 hours.

prokls commented 2 years ago

All tests passed. Ran the benchmarks. Tested features and their combinations. Reviewed alloc_boxed_array, zeroize, RngCore for AesState without resulting changes. Updated/fixed doc issues. It makes sense, what you created.

Congratulation to all of you for the v2 release! @dkales @faern @Colfenor :muscle: :partying_face: