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

Upgrade sha3 and use shake256 infallibly #17

Closed faern closed 2 years ago

faern commented 2 years ago

SHAKE256 output can be obtained infallibly. We only had an error branch here because the read was done using std::io::Read::read instead of directly from XofReader::read. The error that was here could not actually happen. Because the std::io::Read impl on XofReader was infallible, it was just forced to have a Result as the return type due to how the trait is defined.

I also took the opportunity to upgrade sha3. This is not strictly needed to make it infallible, but I figured we better get the latest dependencies anyway.

prokls commented 2 years ago

Even though, I agree with the change, I am afraid it does not have a chance.

  1. It changes the public API. Thus it won't make it into the 1.x branch.
  2. It seems to be solved already on the 2.x branch.
faern commented 2 years ago

How does it change the public API? The shake256 function is private and all errors exposed from the library are just opaque anyway.

prokls commented 2 years ago

Oh sorry, my bad. I did not read the (crate) suffix. Ready for merge.

faern commented 2 years ago

I forgot the v2 branch existed. I really think we need to rework fallibility there. The crypto operations are not infallible by themselves, even if they contained a lot of invalidly fallible operations (like this one).

faern commented 2 years ago

Sorry for pushing a bunch of stuff to the 1.0 branch. I really just want to help make 2.0 get out the door asap.

prokls commented 2 years ago

I think it is legitimate to improve branch 1.x where possible. I appreciate it.

I have difficulties to judge fallibility on branch 2.x. @dkales has a better overview on the changes of 2.x.

faern commented 2 years ago

The solution available in v2 is even better than here. I missed the digest_xof helper function :+1:

faern commented 2 years ago

I think it is legitimate to improve branch 1.x where possible. I appreciate it.

I don't think the v1 branch should be used by anyone in production. That RNG is not meant for real use. Which is why I want v2 got get ready. Because we already use this in production, albeit currently depending on git. https://github.com/mullvad/mullvadvpn-app/blob/master/talpid-tunnel-config-client/Cargo.toml#L20-L24

prokls commented 2 years ago

After running the testsuite, I merged the PR. I also added a commit for digest_xof. Thank you!