Shamir Secret Sharing over GF(2^8) #7891
Conversation
andrewjstone
changed the title
| /// implementations. | ||
| impl Mul for Gf256 { | ||
| type Output = Self; | ||
| fn mul(mut self, mut rhs: Self) -> Self::Output { |
There was a problem hiding this comment.
these both being mut seems strange, especially since the trait doesn't look to have it https://doc.rust-lang.org/stable/core/ops/trait.Mul.html . I know this is rust but are we going to run into surprises with the mut?
There was a problem hiding this comment.
The trait takes the parameters by value, so this shouldn't cause any problems. They'll get copied before being passed as arguments. I could create temporary locals inside the method I suppose. That may make the code a touch clearer.
There was a problem hiding this comment.
I completely failed the by value part. I think I was too focused on comparing against the other code which does use temporaries.
| /// implementations. | ||
| impl Mul for Gf256 { | ||
| type Output = Self; | ||
| fn mul(mut self, mut rhs: Self) -> Self::Output { |
There was a problem hiding this comment.
I completely failed the by value part. I think I was too focused on comparing against the other code which does use temporaries.
| // Can't divide by zero | ||
| for i in 1..=255u8 { | ||
| let a = Gf256::new(i); | ||
| assert_eq!((a * a.invert()).ct_eq(&ONE).unwrap_u8(), 1); |
There was a problem hiding this comment.
I know the unwrap_u8 isn't your code, but just thought I'd mention that I find the wording of that from sublte is unfortunate because it makes it seem like it could panic. I think your choice of into_u8 is much better. into_inner is also what I'd expect, though I also like into_u8 being that it's more idiomatic and better describes what you get.
| /// implementations. | ||
| impl Mul for Gf256 { | ||
| type Output = Self; | ||
| fn mul(mut self, mut rhs: Self) -> Self::Output { |
There was a problem hiding this comment.
I verified this implementation matches the link you provided (https://en.wikipedia.org/wiki/Finite_field_arithmetic#Rijndael's_(AES)_finite_field), though I think readability could be improved by using if statements instead of using wrapping_sub. Ex:
if rhs.0 & 1 == 1 {
product ^= self.0;
}
There was a problem hiding this comment.
While I 100% agree with the readability point, this was done intentionally to avoid data dependent code when dealing with secrets. I'm attempting to make this as constant time as possible in rust, barring the compiler harming me, which is extremely likely!
| } | ||
|
|
||
| /// Interpolate the points for each polynomial to find the value `y = f(x)` | ||
| /// and then concatenate them and return the corrseponding [`Share`]. |
There was a problem hiding this comment.
| /// and then concatenate them and return the corrseponding [`Share`]. | |
| /// and then concatenate them and return the corresponding [`Share`]. |
| /// | ||
| /// The secret is the concatenation of the y-coordinates at x=0. | ||
| pub fn compute_secret( | ||
| shares: &[Share], |
There was a problem hiding this comment.
Because it's part of the public API, it looks to me like shares should be behind a secrecry::Secret, considering this it all that's needed to construct the secret, where that only might be the case depending on the caller. Without enforcing that I think it adds potential for "holding it wrong" here. And so I could see an argument for making the underlying data in a public Share be wrapped in a secrecry::Secret.
This of course would have some big implications on the API, and I could imagine the ergonomic implications might have you cursing my name. I wanted to bounce this off you before thinking through all that further.
Thoughts?
There was a problem hiding this comment.
I don't think this is necessary. Shares themselves implement ZeroizeOnDrop and this API really shouldn't mandate how the user maintains them as it can indeed introduce serious ergonomic issues. Although not shown in any tests here, Shares are sent across nodes in messages, and must be collated over time. We'd need a mutable secret to store them, and we don't really want to force the collection in which they are stored on a caller. They can store them as values in maps, Vecs, etc... They can put their collection type l in a Secret, or not, but we don't really care about that here. In order to allow this work, we also need to update Secrecy because in the version we are on you can't have a mutable secret and can't build up shares retrieved over the network into a single collection. Also, I'm pretty sure this API would have to become generic over the collections to take a Secret of arbitrary inner type, which I"m definitely loathe to do.
Now, with that said, I actually think this API probably should be changed, except not to take a secret. It should instead take an Iterator<Item = &Share> to allow storage in various collection types without having to collect into a type that can be passed as a slice.
We could also wrap each individual Share in a Secret, but that just means for every damn math function we are constantly calling expose_secret, which will make things harder to read.
There was a problem hiding this comment.
Now, with that said, I actually think this API probably should be changed, except not to take a secret. It should instead take an Iterator<Item = &Share> to allow storage in various collection types without having to collect into a type that can be passed as a slice.
Actually doing this may be a mistake also, as they'd still have to be collected to build a ValidatedShares type. I think the best thing to do is to build on top of this library by using it and then make changes over time if they seem reasonable.
This code builds upon #7859, which itself builds upon #7891. Those need to be merged in order first. A `Node` is the sans-io entity driving the trust quorum protocol. This PR starts the work of creating the `Node` type and coordinating an initial configuration. There's a property based test that generates an initial `ReconfigureMsg` that is then used to call `Node::coordinate_reconfiguration`, which will internally setup a `CoordinatorState` and create a bunch of messages to be sent. We verify that a new `PersistentState` is returned and that messages are sent. This needs a bit more documentation and testing, so it's still WIP.
This code builds upon #7859, which itself builds upon #7891. Those need to be merged in order first. A `Node` is the sans-io entity driving the trust quorum protocol. This PR starts the work of creating the `Node` type and coordinating an initial configuration. There's a property based test that generates an initial `ReconfigureMsg` that is then used to call `Node::coordinate_reconfiguration`, which will internally setup a `CoordinatorState` and create a bunch of messages to be sent. We verify that a new `PersistentState` is returned and that messages are sent. This needs a bit more documentation and testing, so it's still WIP.
Please see the doc comments for details.