Add a Replace proposal #174
Conversation
There was a problem hiding this comment.
Thanks @bifurcation !
I'm still reviewing, but I added some initial comments I had when looking.
| pub fn replace_member( | ||
| mut self, | ||
| to_replace: u32, | ||
| leaf_node: LeafNode, | ||
| ) -> Result<Self, MlsError> { |
There was a problem hiding this comment.
Is there a public interface to obtain the LeafNode? Maybe it would be easier to do replace_member(mut self, to_replace: u32, proposal: UpdateProposal) or even replace_member(mut self, proposal: MlsMessage) (message contains index) ?
There was a problem hiding this comment.
Agreed, to date we have made LeafNode private, maybe it should stay private?
| /// Abandon any cached state corresponding to a leaf node | ||
| #[cfg(feature = "replace_proposal")] | ||
| #[cfg_attr(not(mls_build_async), maybe_async::must_be_sync)] | ||
| pub async fn abandon_leaf_node(&mut self, leaf_node: &LeafNode) { | ||
| self.pending_updates.remove(&leaf_node.public_key); | ||
| } |
There was a problem hiding this comment.
Isn't it annoying for the application to track LeafNode's?
Can we make it automatic so that leaves with epoch <= current leaf epoch are deleted automatically since they can no longer be applied? Storing other leaves can't hurt security (since they haven't been applied) so this function isn't so crucial anymore.
Also, how about a function that deletes all cached leaves, or all leaves with not_before < given time T, or all leaves from epoch < given epoch E?
| #[cfg(not(feature = "replace_proposal"))] | ||
| let extension_types = Default::default(); | ||
|
|
||
| #[cfg(feature = "replace_proposal")] | ||
| let extension_types = vec![ExtensionType::LEAF_NODE_EPOCH]; |
There was a problem hiding this comment.
I'm curious if this should be default or not just because the feature is on? I think I view the feature more as "you can possibly do this" vs "you definitely want to do this"
| pub fn replace_member( | ||
| mut self, | ||
| to_replace: u32, | ||
| leaf_node: LeafNode, | ||
| ) -> Result<Self, MlsError> { |
There was a problem hiding this comment.
Agreed, to date we have made LeafNode private, maybe it should stay private?
| @@ -265,8 +280,7 @@ where | |||
| private_tree: TreeKemPrivate, | |||
| key_schedule: KeySchedule, | |||
| #[cfg(feature = "by_ref_proposal")] | |||
| pending_updates: | |||
| crate::map::SmallMap<HpkePublicKey, (HpkeSecretKey, Option<SignatureSecretKey>)>, // Hash of leaf node hpke public key to secret key | |||
| pending_updates: SmallMap<HpkePublicKey, PendingUpdate>, // Hash of leaf node hpke public key to secret key | |||
There was a problem hiding this comment.
@mulmarta I think that we persist this data to disk as part of the group state itself? If so making this change would break loading group states from before this version. We can probably just try to deserialize the new format followed by the old format somehow.
Issues:
Resolves #170
Description of changes:
This PR implements a Replace proposal long the lines discussed in #170 and draft-barnes-mls-replace. Changes are gated behind a feature flag
replace_proposal, as discussed in #171.Call-outs:
There are a few questions around error codes highlighted with
XXX(RLB)in the code.The current code does not reflect support for the Replace proposal in Capabilities when this feature is enabled, and probably should to be specification-compliant. Somewhat surprisingly, this does not cause tests to fail. I suspect there's a general bug to fix in that there doesn't seem to be any check that a proposal is supported by the group before it is applied.
Testing:
In general, the testing strategy is broadly parallel to testing of Update, since this new proposal type is closely related:
src/group/commit.rssrc/group/mod.rssrc/group/proposal_cache.rsAs a small drive-by, I also added tests that duplicate Updates are correctly filtered / detected, and the corresponding code to implement this policy.
I think these tests are complete and in the right places, but my familiarity with this code base is pretty rudimentary.
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 and MIT license.