From f9dc62ccef447f3744e67ca46ea5d0b411979a62 Mon Sep 17 00:00:00 2001 From: Brendan McMillion Date: Sat, 10 Feb 2024 16:01:34 -0800 Subject: [PATCH] Add review comments. --- openmls/src/group/core_group/process.rs | 2 +- openmls/src/group/core_group/staged_commit.rs | 2 +- openmls/src/group/public_group/validation.rs | 22 ++++++++++++++++--- openmls/src/key_packages/key_package_in.rs | 1 + 4 files changed, 22 insertions(+), 5 deletions(-) diff --git a/openmls/src/group/core_group/process.rs b/openmls/src/group/core_group/process.rs index 9003a6bcb7..05f9065dc9 100644 --- a/openmls/src/group/core_group/process.rs +++ b/openmls/src/group/core_group/process.rs @@ -124,7 +124,7 @@ impl CoreGroup { FramedContentBody::Proposal(_) => { Err(ProcessMessageError::UnsupportedProposalType) } - FramedContentBody::Commit(_) => unimplemented!(), + FramedContentBody::Commit(_) => unimplemented!(), // NOTE: Should just be an error. } } } diff --git a/openmls/src/group/core_group/staged_commit.rs b/openmls/src/group/core_group/staged_commit.rs index 0b0f2e9a38..4b791598f9 100644 --- a/openmls/src/group/core_group/staged_commit.rs +++ b/openmls/src/group/core_group/staged_commit.rs @@ -226,7 +226,7 @@ impl CoreGroup { // Even if there is no path, we have to update the group context. diff.update_group_context( provider.crypto(), - apply_proposals_values.extensions.clone(), + apply_proposals_values.extensions.clone(), // NOTE: Could/should be None. )?; ( diff --git a/openmls/src/group/public_group/validation.rs b/openmls/src/group/public_group/validation.rs index 81ab909084..0d8f51f81b 100644 --- a/openmls/src/group/public_group/validation.rs +++ b/openmls/src/group/public_group/validation.rs @@ -168,7 +168,7 @@ impl PublicGroup { .signature_key() .as_slice() .to_vec() - }); + }); // NOTE: Why only Add proposals? Updates and the committer can also change their signature key. // Collect encryption keys from add proposals, update proposals, the // commit leaf node and path keys @@ -225,6 +225,7 @@ impl PublicGroup { // Validate uniqueness of signature keys // - ValSem101 for signature_key in signature_keys { + // NOTE: This prohibits doing a Remove+Add in the same commit for a client. if !signature_key_set.insert(signature_key) { return Err(ProposalValidationError::DuplicateSignatureKey); } @@ -237,9 +238,12 @@ impl PublicGroup { // - ValSem206 // - ValSem207 for encryption_key in encryption_keys { - if init_key_set.contains(&encryption_key) { + if init_key_set.contains(&encryption_key) { // NOTE: This doesn't seem to do anything? return Err(ProposalValidationError::InitEncryptionKeyCollision); } + // NOTE: This needs to get ALL encryption keys from ratchet tree if + // it is also meant to do the UpdatePath uniqueness check. Above + // only initializes with ratchet tree leaves. if !encryption_key_set.insert(encryption_key) { return Err(ProposalValidationError::DuplicateEncryptionKey); } @@ -288,8 +292,9 @@ impl PublicGroup { Proposal::Add(add_proposal) => Some(add_proposal.key_package().leaf_node()), Proposal::Update(update_proposal) => Some(update_proposal.leaf_node()), _ => None, - }); + }); // NOTE: This does not include committer's leaf node. + // NOTE: This should exclude members being removed. let mut group_leaf_nodes = self.treesync().full_leaves(); for leaf_node in leaf_nodes { @@ -306,6 +311,7 @@ impl PublicGroup { // If there is a required capabilities extension, check if that one // is supported. + // NOTE: This check should be skipped if there is a GroupContextExtensions proposal being committed. if let Some(required_capabilities) = self.group_context().extensions().required_capabilities() { @@ -326,6 +332,7 @@ impl PublicGroup { } // Check that the credential type is supported by all members of the group. + // NOTE: This should also check against the new members being added to the group. if !group_leaf_nodes.all(|node| { node.capabilities() .contains_credential(&leaf_node.credential().credential_type()) @@ -336,6 +343,7 @@ impl PublicGroup { // Check that the capabilities field of this LeafNode indicates // support for all the credential types currently in use by other // members. + // NOTE: This should also check against the new members being added to the group. if !group_leaf_nodes .all(|node| capabilities.contains_credential(&node.credential().credential_type())) { @@ -354,6 +362,7 @@ impl PublicGroup { let add_proposals = proposal_queue.add_proposals(); for add_proposal in add_proposals { + // NOTE: This check is already done elsewhere. // ValSem105: Check if ciphersuite and version of the group are correct: if add_proposal.add_proposal().key_package().ciphersuite() != self.ciphersuite() || add_proposal.add_proposal().key_package().protocol_version() != self.version() @@ -378,6 +387,7 @@ impl PublicGroup { for remove_proposal in remove_proposals { let removed = remove_proposal.remove_proposal().removed(); // ValSem107 + // NOTE: This should also be checking uniqueness among Updates. if !removes_set.insert(removed) { return Err(ProposalValidationError::DuplicateMemberRemoval); } @@ -476,12 +486,14 @@ impl PublicGroup { } // ValSem242: External Commit must only cover inline proposal in allowlist (ExternalInit, Remove, PreSharedKey) + // NOTE: Check for inline happens elsewhere. let contains_denied_proposal = proposal_queue.queued_proposals().any(|p| { let is_inline = p.proposal_or_ref_type() == ProposalOrRefType::Proposal; let is_allowed_type = matches!( p.proposal(), Proposal::ExternalInit(_) | Proposal::Remove(_) | Proposal::PreSharedKey(_) ); + // NOTE: Just !is_allowed_type is_inline && !is_allowed_type }); if contains_denied_proposal { @@ -503,6 +515,8 @@ impl PublicGroup { .treesync() .leaf(removed_leaf) .ok_or(ExternalCommitValidationError::UnknownMemberRemoval)?; + // NOTE: This is an application-defined check, I don't think you can implement it yourself. + // NOTE: This check should be skipped if this client is being removed by the commit. if removed_leaf.credential().identity() != new_leaf.credential().identity() { return Err(ExternalCommitValidationError::InvalidRemoveProposal); @@ -531,6 +545,8 @@ impl PublicGroup { .map(|ext| ext.extension_type()) .collect(); + // NOTE: This should be checking against all members of the new group. + // NOTE: RequiredCapabilities not checked. self.check_extension_support(&ext_type_list).map_err(|_| GroupContextExtensionsProposalValidationError::ExtensionNotSupportedByAllMembers)? } _ => { diff --git a/openmls/src/key_packages/key_package_in.rs b/openmls/src/key_packages/key_package_in.rs index 0b0d9ad3f7..e2c8aecd90 100644 --- a/openmls/src/key_packages/key_package_in.rs +++ b/openmls/src/key_packages/key_package_in.rs @@ -186,6 +186,7 @@ impl KeyPackageIn { // Ensure validity of the life time extension in the leaf node. if let Some(life_time) = key_package.payload.leaf_node.life_time() { + // NOTE: This must only be verified optionally. if !life_time.is_valid() { return Err(KeyPackageVerifyError::InvalidLifetime); }