Malicious sharded shuffle #1394
Conversation
| @@ -92,7 +92,7 @@ where | |||
| let mut x_2 = x_1.clone(); | |||
| add_single_shares_in_place(&mut x_2, z_31); | |||
| x_2.shuffle(&mut rng_perm_l); | |||
| send_to_peer(&x_2, ctx, &OPRFShuffleStep::TransferX2, Direction::Right).await?; | |||
| send_to_peer(&x_2, ctx, &OPRFShuffleStep::TransferXY, Direction::Right).await?; | |||
There was a problem hiding this comment.
To allow interceptor reuse between the sharded and non-sharded tests, steps are renamed to make TransferXY and TransferC have the same names for OPRFShuffleStep and ShardedShuffleStep.
| // The `From` and `Into` bounds are necessary to work with routines that have not been | ||
| // updated to use the `Shuffleable` trait. | ||
| pub trait Shuffleable: | ||
| From<AdditiveShare<Self::Share>> + Into<AdditiveShare<Self::Share>> + Send + 'static |
There was a problem hiding this comment.
The From and Into bounds are necessary to interoperate with the malicious shuffle routines that don't understand Shuffleable yet. I have a future change that updates things to use Shuffleable and gets rid of these bounds.
(At the moment, Shuffleable is only implemented for AdditiveShare<V: BooleanArray>, so these bounds are trivially satisfied. But we want to implement Shuffleable for other types.)
| @@ -288,11 +265,11 @@ where | |||
| // shared with the left helper. | |||
| let x2: Vec<S::Share> = ctx | |||
| .narrow(&ShuffleStep::Permute31) | |||
| .mask_and_shuffle(Direction::Left, &x1) | |||
| .mask_and_shuffle(Direction::Left, x1.iter().copied()) | |||
There was a problem hiding this comment.
With ShuffleShare restricted to BooleanArray, it acquires Copy, which makes the Borrow generic on mask_and_shuffle that I recently added unnecessary.
andyleiserson
force-pushed
the
malicious-sharded-shuffle
branch
from
fd7f3bd
to
21e954d
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1394 +/- ##
==========================================
- Coverage 93.68% 93.49% -0.20%
==========================================
Files 223 223
Lines 37737 38471 +734
==========================================
+ Hits 35353 35967 +614
- Misses 2384 2504 +120 ☔ View full report in Codecov by Sentry. |
| @@ -100,7 +104,7 @@ trait ShuffleContext: ShardedContext { | |||
| Direction::Right => r, | |||
| }; | |||
|
|
|||
| item.borrow().clone() + mask | |||
| item + mask | |||
There was a problem hiding this comment.
This works here, but with the revised bounds in #1397 (which remove the implication that ShuffleShare is Copy), it comes back in a slightly different form.
| @@ -234,12 +198,16 @@ impl<C: ShardedContext> ShuffleContext for C {} | |||
| /// | |||
| /// [`ShuffleShare`] and [`Shuffleable`] are added to bridge the gap. They can be implemented for | |||
| /// arbitrary structs as long as `Add` operation can be defined on them. | |||
| pub trait ShuffleShare: Sendable + Clone + FromRandom + Add<Output = Self> {} | |||
| pub trait ShuffleShare: BooleanArray + Serializable + FromRandom {} | |||
There was a problem hiding this comment.
I think BooleanArray bound may be problematic for structs like HybridReport, would it work if we ask implementors to give us bit representation of their data? something like fn as_bytes_iter(&self) -> impl Iterator<Item = [u8; 32]>. iirc we only need it to compute MACs
There was a problem hiding this comment.
Note that these are the trait bounds for ShuffleShare, not for Shuffleable itself (i.e. it is saying that any Shuffleable type needs to support conversion to boolean arrays, not that only boolean arrays are shuffleable). That said, after some back-and-forth with myself in #1395, I was able to arrive at a more precise set of bounds for ShuffleShare, which you can see in #1397 at line 209 of sharded.rs.
#1397 also has the implementations of Shuffleable for AttributionOutputs and OPRFIPAInputRow.
No description provided.