From 1d32ea3df2eba6eaa21a0d24c3b5851b9eef36da Mon Sep 17 00:00:00 2001 From: Jan Winkelmann <146678+keks@users.noreply.github.com> Date: Wed, 17 Jan 2024 17:24:35 +0100 Subject: [PATCH 01/17] Process GroupContextExtensions Proposals (#1475) Adds support for GroupContextExtensions proposals. The relevant validation has been added both to the code and the documentation. Co-authored-by: Jan Winkelmann (keks) --- book/src/message_validation.md | 2 + openmls/src/framing/validation.rs | 1 + openmls/src/group/core_group/mod.rs | 17 ++-- openmls/src/group/core_group/proposals.rs | 11 +-- openmls/src/group/core_group/staged_commit.rs | 10 ++- .../src/group/core_group/test_proposals.rs | 53 ----------- openmls/src/group/errors.rs | 31 ++++++- openmls/src/group/group_context.rs | 4 + openmls/src/group/mls_group/errors.rs | 12 ++- openmls/src/group/mls_group/proposal.rs | 9 +- openmls/src/group/mls_group/test_mls_group.rs | 89 ++++++++++++++++++- openmls/src/group/public_group/diff.rs | 5 ++ .../public_group/diff/apply_proposals.rs | 13 +++ .../group/public_group/diff/compute_path.rs | 5 +- .../src/group/public_group/staged_commit.rs | 5 +- openmls/src/group/public_group/validation.rs | 40 ++++++++- openmls/src/treesync/node/leaf_node.rs | 29 +++--- .../treesync/node/leaf_node/capabilities.rs | 14 ++- .../kats/kat_tree_operations.rs | 5 +- 19 files changed, 252 insertions(+), 103 deletions(-) diff --git a/book/src/message_validation.md b/book/src/message_validation.md index efdaa92761..8c4e2cf37e 100644 --- a/book/src/message_validation.md +++ b/book/src/message_validation.md @@ -80,6 +80,8 @@ The following is a list of the individual semantic validation steps performed by | `ValSem205` | Confirmation tag must be successfully verified | ✅ | ✅ | `openmls/src/group/tests/test_commit_validation.rs` | | `ValSem206` | Path leaf node encryption key must be unique among proposals & members | ✅ | ✅ | `openmls/src/group/tests/test_commit_validation.rs` | | `ValSem207` | Path encryption keys must be unique among proposals & members | ✅ | ✅ | `openmls/src/group/tests/test_commit_validation.rs` | +| `ValSem208` | Only one GroupContextExtensions proposal in a commit | ✅ | | | +| `ValSem209` | GroupContextExtensions proposals may only contain extensions support by all members | ✅ | | | ### External Commit message validation diff --git a/openmls/src/framing/validation.rs b/openmls/src/framing/validation.rs index 7108b70b73..391cdc2af7 100644 --- a/openmls/src/framing/validation.rs +++ b/openmls/src/framing/validation.rs @@ -55,6 +55,7 @@ use super::{ /// - ValSem005 /// - ValSem007 /// - ValSem009 +#[derive(Debug)] pub(crate) struct DecryptedMessage { verifiable_content: VerifiableAuthenticatedContentIn, } diff --git a/openmls/src/group/core_group/mod.rs b/openmls/src/group/core_group/mod.rs index b56b53f8b6..827966452c 100644 --- a/openmls/src/group/core_group/mod.rs +++ b/openmls/src/group/core_group/mod.rs @@ -452,10 +452,9 @@ impl CoreGroup { let required_capabilities = required_extension.as_required_capabilities_extension()?; // Ensure we support all the capabilities. required_capabilities.check_support()?; - // TODO #566/#1361: This needs to be re-enabled once we support GCEs - /* self.own_leaf_node()? - .capabilities() - .supports_required_capabilities(required_capabilities)?; */ + self.own_leaf_node()? + .capabilities() + .supports_required_capabilities(required_capabilities)?; // Ensure that all other leaf nodes support all the required // extensions as well. @@ -890,6 +889,11 @@ impl CoreGroup { .validate_update_proposals(&proposal_queue, *sender_index)?; } + // ValSem208 + // ValSem209 + self.public_group + .validate_group_context_extensions_proposal(&proposal_queue)?; + // Make a copy of the public group to apply proposals safely let mut diff = self.public_group.empty_diff(); @@ -915,12 +919,13 @@ impl CoreGroup { apply_proposals_values.exclusion_list(), params.commit_type(), signer, - params.take_credential_with_key() + params.take_credential_with_key(), + apply_proposals_values.extensions.clone() )? } else { // If path is not needed, update the group context and return // empty path processing results - diff.update_group_context(provider.crypto())?; + diff.update_group_context(provider.crypto(), apply_proposals_values.extensions.clone())?; PathComputationResult::default() }; diff --git a/openmls/src/group/core_group/proposals.rs b/openmls/src/group/core_group/proposals.rs index e5344133b6..57920e43f8 100644 --- a/openmls/src/group/core_group/proposals.rs +++ b/openmls/src/group/core_group/proposals.rs @@ -507,7 +507,7 @@ impl ProposalQueue { } } Proposal::GroupContextExtensions(_) => { - // TODO: Validate proposal? + valid_proposals.add(queued_proposal.proposal_reference()); proposal_pool.insert(queued_proposal.proposal_reference(), queued_proposal); } Proposal::AppAck(_) => unimplemented!("See #291"), @@ -530,10 +530,11 @@ impl ProposalQueue { // Only retain `adds` and `valid_proposals` let mut proposal_queue = ProposalQueue::default(); for proposal_reference in adds.iter().chain(valid_proposals.iter()) { - proposal_queue.add(match proposal_pool.get(proposal_reference) { - Some(queued_proposal) => queued_proposal.clone(), - None => return Err(ProposalQueueError::ProposalNotFound), - }); + let queued_proposal = proposal_pool + .get(proposal_reference) + .cloned() + .ok_or(ProposalQueueError::ProposalNotFound)?; + proposal_queue.add(queued_proposal); } Ok((proposal_queue, contains_own_updates)) } diff --git a/openmls/src/group/core_group/staged_commit.rs b/openmls/src/group/core_group/staged_commit.rs index 8fccbf42b2..0b69aa3f37 100644 --- a/openmls/src/group/core_group/staged_commit.rs +++ b/openmls/src/group/core_group/staged_commit.rs @@ -165,7 +165,10 @@ impl CoreGroup { )?; // Update group context - diff.update_group_context(provider.crypto())?; + diff.update_group_context( + provider.crypto(), + apply_proposals_values.extensions.clone(), + )?; let decryption_keypairs: Vec<&EncryptionKeyPair> = old_epoch_keypairs .iter() @@ -221,7 +224,10 @@ impl CoreGroup { } // Even if there is no path, we have to update the group context. - diff.update_group_context(provider.crypto())?; + diff.update_group_context( + provider.crypto(), + apply_proposals_values.extensions.clone(), + )?; ( CommitSecret::zero_secret(ciphersuite, self.version()), diff --git a/openmls/src/group/core_group/test_proposals.rs b/openmls/src/group/core_group/test_proposals.rs index 397237bda1..4f9812c9a1 100644 --- a/openmls/src/group/core_group/test_proposals.rs +++ b/openmls/src/group/core_group/test_proposals.rs @@ -22,7 +22,6 @@ use crate::{ messages::proposals::{AddProposal, Proposal, ProposalOrRef, ProposalType}, schedule::psk::store::ResumptionPskStore, test_utils::*, - treesync::errors::LeafNodeValidationError, versions::ProtocolVersion, }; @@ -310,58 +309,6 @@ fn test_required_unsupported_proposals(ciphersuite: Ciphersuite, provider: &impl ) } -#[apply(ciphersuites_and_providers)] -fn test_required_extension_key_package_mismatch( - ciphersuite: Ciphersuite, - provider: &impl OpenMlsProvider, -) { - // Basic group setup. - let group_aad = b"Alice's test group"; - let framing_parameters = FramingParameters::new(group_aad, WireFormat::PublicMessage); - - let (alice_credential, _, alice_signer, _alice_pk) = - setup_client("Alice", ciphersuite, provider); - let (_bob_credential_with_key, bob_key_package_bundle, _, _) = - setup_client("Bob", ciphersuite, provider); - let bob_key_package = bob_key_package_bundle.key_package(); - - // Set required capabilities - let extensions = &[ - ExtensionType::RequiredCapabilities, - ExtensionType::ApplicationId, - ]; - let proposals = &[ - ProposalType::GroupContextExtensions, - ProposalType::Add, - ProposalType::Remove, - ProposalType::Update, - ]; - let credentials = &[CredentialType::Basic]; - let required_capabilities = - RequiredCapabilitiesExtension::new(extensions, proposals, credentials); - - let alice_group = CoreGroup::builder( - GroupId::random(provider.rand()), - CryptoConfig::with_default_version(ciphersuite), - alice_credential, - ) - .with_required_capabilities(required_capabilities) - .build(provider, &alice_signer) - .expect("Error creating CoreGroup."); - - let e = alice_group - .create_add_proposal( - framing_parameters, - bob_key_package.clone(), - &alice_signer, - ) - .expect_err("Proposal was created even though the key package didn't support the required extensions."); - assert_eq!( - e, - CreateAddProposalError::LeafNodeValidation(LeafNodeValidationError::UnsupportedExtensions) - ); -} - #[apply(ciphersuites_and_providers)] fn test_group_context_extensions(ciphersuite: Ciphersuite, provider: &impl OpenMlsProvider) { // Basic group setup. diff --git a/openmls/src/group/errors.rs b/openmls/src/group/errors.rs index 1432c494a0..fbc1296710 100644 --- a/openmls/src/group/errors.rs +++ b/openmls/src/group/errors.rs @@ -189,6 +189,11 @@ pub enum StageCommitError { /// See [`UpdatePathError`] for more details. #[error(transparent)] VerifiedUpdatePathError(#[from] UpdatePathError), + /// See [`GroupContextExtensionsProposalValidationError`] for more details. + #[error(transparent)] + GroupContextExtensionsProposalValidationError( + #[from] GroupContextExtensionsProposalValidationError, + ), } /// Create commit error @@ -233,6 +238,11 @@ pub enum CreateCommitError { /// See [`InvalidExtensionError`] for more details. #[error(transparent)] InvalidExtensionError(#[from] InvalidExtensionError), + /// See [`GroupContextExtensionsProposalValidationError`] for more details. + #[error(transparent)] + GroupContextExtensionsProposalValidationError( + #[from] GroupContextExtensionsProposalValidationError, + ), } /// Validation error @@ -500,16 +510,13 @@ pub(crate) enum CoreGroupParseMessageError { /// Create group context ext proposal error #[derive(Error, Debug, PartialEq, Clone)] -pub(crate) enum CreateGroupContextExtProposalError { +pub enum CreateGroupContextExtProposalError { /// See [`LibraryError`] for more details. #[error(transparent)] LibraryError(#[from] LibraryError), /// See [`KeyPackageExtensionSupportError`] for more details. #[error(transparent)] KeyPackageExtensionSupport(#[from] KeyPackageExtensionSupportError), - /// See [`TreeSyncError`] for more details. - #[error(transparent)] - TreeSyncError(#[from] TreeSyncError), /// See [`ExtensionError`] for more details. #[error(transparent)] Extension(#[from] ExtensionError), @@ -528,3 +535,19 @@ pub enum MergeCommitError { #[error("Error accessing the key store.")] KeyStoreError(KeyStoreError), } + +/// Error validation a GroupContextExtensions proposal. +#[derive(Error, Debug, PartialEq, Clone)] +pub enum GroupContextExtensionsProposalValidationError { + /// Commit has more than one GroupContextExtensions proposal. + #[error("Commit has more than one GroupContextExtensions proposal.")] + TooManyGCEProposals, + /// See [`LibraryError`] for more details. + #[error(transparent)] + LibraryError(#[from] LibraryError), + /// The new extensions set contails extensions that are not supported by all group members. + #[error( + "The new extensions set contails extensions that are not supported by all group members." + )] + ExtensionNotSupportedByAllMembers, +} diff --git a/openmls/src/group/group_context.rs b/openmls/src/group/group_context.rs index 8344e07625..d6b1cc93a9 100644 --- a/openmls/src/group/group_context.rs +++ b/openmls/src/group/group_context.rs @@ -159,6 +159,10 @@ impl GroupContext { self.confirmed_transcript_hash.as_slice() } + pub(crate) fn set_extensions(&mut self, extensions: Extensions) { + self.extensions = extensions; + } + /// Return the extensions. pub fn extensions(&self) -> &Extensions { &self.extensions diff --git a/openmls/src/group/mls_group/errors.rs b/openmls/src/group/mls_group/errors.rs index a23e47667e..2b7a4fc207 100644 --- a/openmls/src/group/mls_group/errors.rs +++ b/openmls/src/group/mls_group/errors.rs @@ -10,9 +10,12 @@ use thiserror::Error; use crate::{ error::LibraryError, extensions::errors::InvalidExtensionError, - group::errors::{ - CreateAddProposalError, CreateCommitError, MergeCommitError, StageCommitError, - ValidationError, + group::{ + errors::{ + CreateAddProposalError, CreateCommitError, MergeCommitError, StageCommitError, + ValidationError, + }, + CreateGroupContextExtProposalError, }, schedule::errors::PskError, treesync::errors::{LeafNodeValidationError, PublicTreeError}, @@ -317,4 +320,7 @@ pub enum ProposalError { /// See [`ValidationError`] for more details. #[error(transparent)] ValidationError(#[from] ValidationError), + /// See [`CreateGroupContextExtProposalError`] for more details. + #[error(transparent)] + CreateGroupContextExtProposalError(#[from] CreateGroupContextExtProposalError), } diff --git a/openmls/src/group/mls_group/proposal.rs b/openmls/src/group/mls_group/proposal.rs index b15b21d75d..bc00077640 100644 --- a/openmls/src/group/mls_group/proposal.rs +++ b/openmls/src/group/mls_group/proposal.rs @@ -328,10 +328,11 @@ impl MlsGroup { ) -> Result<(MlsMessageOut, ProposalRef), ProposalError<()>> { self.is_operational()?; - let proposal = self - .group - .create_group_context_ext_proposal(self.framing_parameters(), extensions, signer) - .unwrap(); + let proposal = self.group.create_group_context_ext_proposal( + self.framing_parameters(), + extensions, + signer, + )?; let queued_proposal = QueuedProposal::from_authenticated_content_by_ref( self.ciphersuite(), diff --git a/openmls/src/group/mls_group/test_mls_group.rs b/openmls/src/group/mls_group/test_mls_group.rs index b22ec84b6a..2587a2066e 100644 --- a/openmls/src/group/mls_group/test_mls_group.rs +++ b/openmls/src/group/mls_group/test_mls_group.rs @@ -721,7 +721,7 @@ fn test_pending_commit_logic(ciphersuite: Ciphersuite, provider: &impl OpenMlsPr // Let's add bob let (proposal, _) = alice_group .propose_add_member(provider, &alice_signer, bob_key_package) - .expect("error creating self-update proposal"); + .expect("error creating add-bob proposal"); let alice_processed_message = alice_group .process_message(provider, proposal.into_protocol_message().unwrap()) @@ -1002,6 +1002,93 @@ fn remove_prosposal_by_ref(ciphersuite: Ciphersuite, provider: &impl OpenMlsProv _ => unreachable!("Expected a StagedCommit."), } } +// +// Test that the builder pattern accurately configures the new group. +#[apply(ciphersuites_and_providers)] +fn group_context_extensions_proposal(ciphersuite: Ciphersuite, provider: &impl OpenMlsProvider) { + let (alice_credential_with_key, _alice_kpb, alice_signer, _alice_pk) = + setup_client("Alice", ciphersuite, provider); + + // === Alice creates a group === + let mut alice_group = MlsGroup::builder() + .build(provider, &alice_signer, alice_credential_with_key) + .expect("error creating group using builder"); + + let required_capabilities = alice_group + .group() + .group_context_extensions() + .required_capabilities() + .expect("couldn't get required_capabilities"); + + // no required extensions + assert!(required_capabilities.extension_types().is_empty()); + + let new_extensions = Extensions::single(Extension::RequiredCapabilities( + RequiredCapabilitiesExtension::new(&[ExtensionType::RequiredCapabilities], &[], &[]), + )); + + let new_extensions_2 = Extensions::single(Extension::RequiredCapabilities( + RequiredCapabilitiesExtension::new(&[ExtensionType::RatchetTree], &[], &[]), + )); + + alice_group + .propose_group_context_extensions(provider, new_extensions.clone(), &alice_signer) + .expect("failed to build group context extensions proposal"); + + assert_eq!(alice_group.pending_proposals().count(), 1); + + alice_group + .commit_to_pending_proposals(provider, &alice_signer) + .expect("failed to commit to pending proposals"); + + alice_group + .merge_pending_commit(provider) + .expect("error merging pending commit"); + + let required_capabilities = alice_group + .group() + .group_context_extensions() + .required_capabilities() + .expect("couldn't get required_capabilities"); + + // has required_capabilities as required capability + assert!(required_capabilities.extension_types() == [ExtensionType::RequiredCapabilities]); + + // === committing to two group context extensions should fail + + alice_group + .propose_group_context_extensions(provider, new_extensions, &alice_signer) + .expect("failed to build group context extensions proposal"); + + // the proposals need to be different or they will be deduplicated + alice_group + .propose_group_context_extensions(provider, new_extensions_2, &alice_signer) + .expect("failed to build group context extensions proposal"); + + assert_eq!(alice_group.pending_proposals().count(), 2); + + alice_group + .commit_to_pending_proposals(provider, &alice_signer) + .expect_err( + "expected error when committing to multiple group context extensions proposals", + ); + + // === can't update required required_capabilities to extensions that existing group members + // are not capable of + + // contains unsupported extension + let new_extensions = Extensions::single(Extension::RequiredCapabilities( + RequiredCapabilitiesExtension::new(&[ExtensionType::Unknown(0xf042)], &[], &[]), + )); + + alice_group + .propose_group_context_extensions(provider, new_extensions, &alice_signer) + .expect_err("expected an error building GCE proposal with bad required_capabilities"); + + // TODO: we need to test that processing a commit with multiple group context extensions + // proposal also fails. however, we can't generate this commit, because our functions for + // constructing commits does not permit it. See #1476 +} // Test that the builder pattern accurately configures the new group. #[apply(ciphersuites_and_providers)] diff --git a/openmls/src/group/public_group/diff.rs b/openmls/src/group/public_group/diff.rs index d9c7e94e01..e2b3eafb75 100644 --- a/openmls/src/group/public_group/diff.rs +++ b/openmls/src/group/public_group/diff.rs @@ -13,6 +13,7 @@ use super::PublicGroup; use crate::{ binary_tree::{array_representation::TreeSize, LeafNodeIndex}, error::LibraryError, + extensions::Extensions, framing::{mls_auth_content::AuthenticatedContent, public_message::InterimTranscriptHashInput}, group::GroupContext, messages::{proposals::AddProposal, ConfirmationTag, EncryptedGroupSecrets}, @@ -201,6 +202,7 @@ impl<'a> PublicGroupDiff<'a> { pub(crate) fn update_group_context( &mut self, crypto: &impl OpenMlsCrypto, + extensions: Option, ) -> Result<(), LibraryError> { // Calculate tree hash let new_tree_hash = self @@ -208,6 +210,9 @@ impl<'a> PublicGroupDiff<'a> { .compute_tree_hashes(crypto, self.group_context().ciphersuite())?; self.group_context.update_tree_hash(new_tree_hash); self.group_context.increment_epoch(); + if let Some(extensions) = extensions { + self.group_context.set_extensions(extensions); + } Ok(()) } diff --git a/openmls/src/group/public_group/diff/apply_proposals.rs b/openmls/src/group/public_group/diff/apply_proposals.rs index cf25af9d94..470c41a183 100644 --- a/openmls/src/group/public_group/diff/apply_proposals.rs +++ b/openmls/src/group/public_group/diff/apply_proposals.rs @@ -12,12 +12,14 @@ use crate::{ use super::*; /// This struct contain the return values of the `apply_proposals()` function +#[derive(Debug)] pub(crate) struct ApplyProposalsValues { pub(crate) path_required: bool, pub(crate) self_removed: bool, pub(crate) invitation_list: Vec<(LeafNodeIndex, AddProposal)>, pub(crate) presharedkeys: Vec, pub(crate) external_init_proposal_option: Option, + pub(crate) extensions: Option, } impl ApplyProposalsValues { @@ -140,6 +142,16 @@ impl<'a> PublicGroupDiff<'a> { }) .collect(); + // apply group context extension proposal + let extensions = proposal_queue + .filtered_by_type(ProposalType::GroupContextExtensions) + .find_map(|queued_proposal| match queued_proposal.proposal() { + Proposal::GroupContextExtensions(extensions) => { + Some(extensions.extensions().clone()) + } + _ => None, + }); + let proposals_require_path = proposal_queue .queued_proposals() .any(|p| p.proposal().is_path_required()); @@ -159,6 +171,7 @@ impl<'a> PublicGroupDiff<'a> { invitation_list, presharedkeys, external_init_proposal_option, + extensions, }) } } diff --git a/openmls/src/group/public_group/diff/compute_path.rs b/openmls/src/group/public_group/diff/compute_path.rs index 5561dabfed..b41d8784b9 100644 --- a/openmls/src/group/public_group/diff/compute_path.rs +++ b/openmls/src/group/public_group/diff/compute_path.rs @@ -7,6 +7,7 @@ use crate::{ binary_tree::LeafNodeIndex, credentials::CredentialWithKey, error::LibraryError, + extensions::Extensions, group::{ config::CryptoConfig, core_group::create_commit_params::CommitType, errors::CreateCommitError, @@ -35,6 +36,7 @@ pub(crate) struct PathComputationResult { } impl<'a> PublicGroupDiff<'a> { + #[allow(clippy::too_many_arguments)] pub(crate) fn compute_path( &mut self, provider: &impl OpenMlsProvider, @@ -43,6 +45,7 @@ impl<'a> PublicGroupDiff<'a> { commit_type: CommitType, signer: &impl Signer, credential_with_key: Option, + extensions: Option, ) -> Result> { let version = self.group_context().protocol_version(); let ciphersuite = self.group_context().ciphersuite(); @@ -101,7 +104,7 @@ impl<'a> PublicGroupDiff<'a> { // After we've processed the path, we can update the group context s.t. // the updated group context is used for path secret encryption. Note // that we have not yet updated the confirmed transcript hash. - self.update_group_context(provider.crypto())?; + self.update_group_context(provider.crypto(), extensions)?; let serialized_group_context = self .group_context() diff --git a/openmls/src/group/public_group/staged_commit.rs b/openmls/src/group/public_group/staged_commit.rs index c7cfa5a25b..2e583017e0 100644 --- a/openmls/src/group/public_group/staged_commit.rs +++ b/openmls/src/group/public_group/staged_commit.rs @@ -87,6 +87,9 @@ impl PublicGroup { // ValSem107 // ValSem108 self.validate_remove_proposals(&proposal_queue)?; + // ValSem208 + // ValSem209 + self.validate_group_context_extensions_proposal(&proposal_queue)?; // ValSem401 // ValSem402 // ValSem403 @@ -226,7 +229,7 @@ impl PublicGroup { }; // Update group context - diff.update_group_context(crypto)?; + diff.update_group_context(crypto, apply_proposals_values.extensions.clone())?; // Update the confirmed transcript hash before we compute the confirmation tag. diff.update_confirmed_transcript_hash(crypto, mls_content)?; diff --git a/openmls/src/group/public_group/validation.rs b/openmls/src/group/public_group/validation.rs index 6db6328592..81ab909084 100644 --- a/openmls/src/group/public_group/validation.rs +++ b/openmls/src/group/public_group/validation.rs @@ -6,7 +6,8 @@ use std::collections::{BTreeSet, HashSet}; use openmls_traits::types::VerifiableCiphersuite; use super::PublicGroup; -#[cfg(test)] +use crate::group::GroupContextExtensionsProposalValidationError; +use crate::prelude::LibraryError; use crate::treesync::errors::LeafNodeValidationError; use crate::{ binary_tree::array_representation::LeafNodeIndex, @@ -515,7 +516,42 @@ impl PublicGroup { /// Returns a [`LeafNodeValidationError`] if an [`ExtensionType`] /// in `extensions` is not supported by a leaf in this tree. - #[cfg(test)] + pub(crate) fn validate_group_context_extensions_proposal( + &self, + proposal_queue: &ProposalQueue, + ) -> Result<(), GroupContextExtensionsProposalValidationError> { + let mut iter = proposal_queue.filtered_by_type(ProposalType::GroupContextExtensions); + + match iter.next() { + Some(queued_proposal) => match queued_proposal.proposal() { + Proposal::GroupContextExtensions(extensions) => { + let ext_type_list: Vec<_> = extensions + .extensions() + .iter() + .map(|ext| ext.extension_type()) + .collect(); + + self.check_extension_support(&ext_type_list).map_err(|_| GroupContextExtensionsProposalValidationError::ExtensionNotSupportedByAllMembers)? + } + _ => { + return Err(GroupContextExtensionsProposalValidationError::LibraryError( + LibraryError::custom( + "found non-gce proposal when filtered for gce proposals", + ), + )) + } + }, + None => return Ok(()), + } + + match iter.next() { + Some(_) => Err(GroupContextExtensionsProposalValidationError::TooManyGCEProposals), + None => Ok(()), + } + } + + /// Returns a [`LeafNodeValidationError`] if an [`ExtensionType`] + /// in `extensions` is not supported by a leaf in this tree. pub(crate) fn check_extension_support( &self, extensions: &[crate::extensions::ExtensionType], diff --git a/openmls/src/treesync/node/leaf_node.rs b/openmls/src/treesync/node/leaf_node.rs index b50aa7bf67..fd1f47cf4f 100644 --- a/openmls/src/treesync/node/leaf_node.rs +++ b/openmls/src/treesync/node/leaf_node.rs @@ -27,7 +27,6 @@ use crate::{ versions::ProtocolVersion, }; -#[cfg(test)] use crate::treesync::errors::LeafNodeValidationError; mod capabilities; @@ -382,6 +381,20 @@ impl LeafNode { .contains(extension_type) || default_extensions().iter().any(|et| et == extension_type) } + /// + /// Check whether the this leaf node supports all the required extensions + /// in the provided list. + pub(crate) fn check_extension_support( + &self, + extensions: &[ExtensionType], + ) -> Result<(), LeafNodeValidationError> { + for required in extensions.iter() { + if !self.supports_extension(required) { + return Err(LeafNodeValidationError::UnsupportedExtensions); + } + } + Ok(()) + } } #[cfg(test)] @@ -411,20 +424,6 @@ impl LeafNode { pub fn capabilities_mut(&mut self) -> &mut Capabilities { &mut self.payload.capabilities } - - /// Check whether the this leaf node supports all the required extensions - /// in the provided list. - pub(crate) fn check_extension_support( - &self, - extensions: &[ExtensionType], - ) -> Result<(), LeafNodeValidationError> { - for required in extensions.iter() { - if !self.supports_extension(required) { - return Err(LeafNodeValidationError::UnsupportedExtensions); - } - } - Ok(()) - } } #[cfg(any(feature = "test-utils", test))] diff --git a/openmls/src/treesync/node/leaf_node/capabilities.rs b/openmls/src/treesync/node/leaf_node/capabilities.rs index e07ff477c8..f7eb9c5ac0 100644 --- a/openmls/src/treesync/node/leaf_node/capabilities.rs +++ b/openmls/src/treesync/node/leaf_node/capabilities.rs @@ -137,7 +137,7 @@ impl Capabilities { if required_capabilities .extension_types() .iter() - .any(|e| !self.extensions().contains(e)) + .any(|e| !(self.extensions().contains(e) || default_extensions().contains(e))) { return Err(LeafNodeValidationError::UnsupportedExtensions); } @@ -145,7 +145,7 @@ impl Capabilities { if required_capabilities .proposal_types() .iter() - .any(|p| !self.proposals().contains(p)) + .any(|p| !(self.proposals().contains(p) || default_proposals().contains(p))) { return Err(LeafNodeValidationError::UnsupportedProposals); } @@ -153,7 +153,7 @@ impl Capabilities { if required_capabilities .credential_types() .iter() - .any(|c| !self.credentials().contains(c)) + .any(|c| !(self.credentials().contains(c) || default_credentials().contains(c))) { return Err(LeafNodeValidationError::UnsupportedCredentials); } @@ -216,7 +216,13 @@ pub(super) fn default_ciphersuites() -> Vec { /// All extensions defined in the MLS spec are considered "default" by the spec. pub(super) fn default_extensions() -> Vec { - vec![ExtensionType::ApplicationId] + vec![ + ExtensionType::ApplicationId, + ExtensionType::RatchetTree, + ExtensionType::RequiredCapabilities, + ExtensionType::ExternalPub, + ExtensionType::ExternalSenders, + ] } /// All proposals defined in the MLS spec are considered "default" by the spec. diff --git a/openmls/src/treesync/tests_and_kats/kats/kat_tree_operations.rs b/openmls/src/treesync/tests_and_kats/kats/kat_tree_operations.rs index 1857191f8b..7726940212 100644 --- a/openmls/src/treesync/tests_and_kats/kats/kat_tree_operations.rs +++ b/openmls/src/treesync/tests_and_kats/kats/kat_tree_operations.rs @@ -111,8 +111,9 @@ fn run_test_vector(test: TestElement, provider: &impl OpenMlsProvider) -> Result let mut diff = group.empty_diff(); - diff.apply_proposals(&proposal_queue, None).unwrap(); - diff.update_group_context(provider.crypto()).unwrap(); + let apply_proposal_values = diff.apply_proposals(&proposal_queue, None).unwrap(); + diff.update_group_context(provider.crypto(), apply_proposal_values.extensions.clone()) + .unwrap(); let staged_diff = diff .into_staged_diff(provider.crypto(), ciphersuite) From 4d5716e0eda31d8e84c8a6c3135f2ec3e49c36ba Mon Sep 17 00:00:00 2001 From: Konrad Kohbrok Date: Thu, 18 Jan 2024 13:07:28 +0100 Subject: [PATCH 02/17] Add leaf node configuration options to MlsGroup builder (#1477) --- book/src/user_manual/group_config.md | 2 + openmls/src/extensions/errors.rs | 5 ++ openmls/src/group/core_group/mod.rs | 17 +++++ openmls/src/group/mls_group/builder.rs | 23 +++++- openmls/src/group/mls_group/config.rs | 29 +++++++- openmls/src/group/mls_group/test_mls_group.rs | 32 +++++++++ openmls/src/group/public_group/builder.rs | 71 +++++++++++++------ openmls/tests/book_code.rs | 15 +++- 8 files changed, 168 insertions(+), 26 deletions(-) diff --git a/book/src/user_manual/group_config.md b/book/src/user_manual/group_config.md index cd826f9adc..2f498569d3 100644 --- a/book/src/user_manual/group_config.md +++ b/book/src/user_manual/group_config.md @@ -19,6 +19,8 @@ Two very similar structs can help configure groups upon their creation: `MlsGrou | ------------------------------ | ------------------------------- | ------------------------------------------------------------------------------------------------ | | `required_capabilities` | `RequiredCapabilitiesExtension` | Required capabilities (extensions and proposal types). | | `external_senders` | `ExternalSendersExtensions` | List credentials of non-group members that are allowed to send proposals to the group. | +| `capabilities` . | `Capabilities` | Lists the capabilities of the group's creator. | +| `leaf_extensions` . | `Extensions` | Extensions to be included in the group creator's leaf | Both ways of group configurations can be specified by using the struct's builder pattern, or choosing their default values. The default value contains safe values for all parameters and is suitable for scenarios without particular requirements. diff --git a/openmls/src/extensions/errors.rs b/openmls/src/extensions/errors.rs index bdc7f9d58a..e8eb93a310 100644 --- a/openmls/src/extensions/errors.rs +++ b/openmls/src/extensions/errors.rs @@ -97,4 +97,9 @@ pub enum InvalidExtensionError { "The provided extension list contains an extension that is not allowed in group contexts." )] IllegalInGroupContext, + /// The provided extension list contains an extension that is not allowed in leaf nodes + #[error( + "The provided extension list contains an extension that is not allowed in leaf nodes." + )] + IllegalInLeafNodes, } diff --git a/openmls/src/group/core_group/mod.rs b/openmls/src/group/core_group/mod.rs index 827966452c..5dcfd3226d 100644 --- a/openmls/src/group/core_group/mod.rs +++ b/openmls/src/group/core_group/mod.rs @@ -39,6 +39,7 @@ use tls_codec::Serialize as TlsSerializeTrait; use self::{ create_commit_params::{CommitType, CreateCommitParams}, + node::leaf_node::Capabilities, past_secrets::MessageSecretsStore, staged_commit::{MemberStagedCommitState, StagedCommit, StagedCommitState}, }; @@ -189,6 +190,11 @@ impl CoreGroupBuilder { .with_required_capabilities(required_capabilities); self } + /// Set the [`Capabilities`] of the group's creator. + pub(crate) fn with_capabilities(mut self, capabilities: Capabilities) -> Self { + self.public_group_builder = self.public_group_builder.with_capabilities(capabilities); + self + } /// Set the [`ExternalSendersExtension`] of the [`CoreGroup`]. pub(crate) fn with_external_senders( mut self, @@ -217,6 +223,17 @@ impl CoreGroupBuilder { Ok(self) } + /// Sets extensions of the group creator's [`LeafNode`]. + pub(crate) fn with_leaf_node_extensions( + mut self, + extensions: Extensions, + ) -> Result { + self.public_group_builder = self + .public_group_builder + .with_leaf_node_extensions(extensions)?; + Ok(self) + } + /// Set the number of past epochs the group should keep secrets. pub fn with_max_past_epoch_secrets(mut self, max_past_epochs: usize) -> Self { self.max_past_epochs = max_past_epochs; diff --git a/openmls/src/group/mls_group/builder.rs b/openmls/src/group/mls_group/builder.rs index ed8ec42069..bcd27d8f66 100644 --- a/openmls/src/group/mls_group/builder.rs +++ b/openmls/src/group/mls_group/builder.rs @@ -8,7 +8,7 @@ use crate::{ CoreGroupBuildError, CoreGroupConfig, GroupId, MlsGroupCreateConfig, MlsGroupCreateConfigBuilder, NewGroupError, ProposalStore, WireFormatPolicy, }, - prelude::{LibraryError, Lifetime, SenderRatchetConfiguration}, + prelude::{Capabilities, LibraryError, Lifetime, SenderRatchetConfiguration}, }; use super::{InnerState, MlsGroup, MlsGroupState}; @@ -73,6 +73,8 @@ impl MlsGroupBuilder { .with_required_capabilities(mls_group_create_config.required_capabilities.clone()) .with_external_senders(mls_group_create_config.external_senders.clone()) .with_group_context_extensions(mls_group_create_config.group_context_extensions.clone())? + .with_leaf_node_extensions(mls_group_create_config.leaf_node_extensions.clone())? + .with_capabilities(mls_group_create_config.capabilities.clone()) .with_max_past_epoch_secrets(mls_group_create_config.join_config.max_past_epochs) .with_lifetime(*mls_group_create_config.lifetime()) .build(provider, signer) @@ -200,6 +202,14 @@ impl MlsGroupBuilder { self } + /// Sets the group creator's [`Capabilities`] + pub fn with_capabilities(mut self, capabilities: Capabilities) -> Self { + self.mls_group_create_config_builder = self + .mls_group_create_config_builder + .capabilities(capabilities); + self + } + /// Sets the initial group context extensions pub fn with_group_context_extensions( mut self, @@ -210,4 +220,15 @@ impl MlsGroupBuilder { .with_group_context_extensions(extensions)?; Ok(self) } + + /// Sets the initial leaf node extensions + pub fn with_leaf_node_extensions( + mut self, + extensions: Extensions, + ) -> Result { + self.mls_group_create_config_builder = self + .mls_group_create_config_builder + .with_leaf_node_extensions(extensions)?; + Ok(self) + } } diff --git a/openmls/src/group/mls_group/config.rs b/openmls/src/group/mls_group/config.rs index c270ece7d2..673548ad5d 100644 --- a/openmls/src/group/mls_group/config.rs +++ b/openmls/src/group/mls_group/config.rs @@ -30,7 +30,7 @@ use super::*; use crate::{ extensions::errors::InvalidExtensionError, group::config::CryptoConfig, key_packages::Lifetime, - tree::sender_ratchet::SenderRatchetConfiguration, + tree::sender_ratchet::SenderRatchetConfiguration, treesync::node::leaf_node::Capabilities, }; use serde::{Deserialize, Serialize}; @@ -85,6 +85,8 @@ impl MlsGroupJoinConfig { pub struct MlsGroupCreateConfig { /// Required capabilities (extensions and proposal types) pub(crate) required_capabilities: RequiredCapabilitiesExtension, + /// Capabilities advertised in the creator's leaf node + pub(crate) capabilities: Capabilities, /// Senders authorized to send external remove proposals pub(crate) external_senders: ExternalSendersExtension, /// Lifetime of the own leaf node @@ -95,6 +97,8 @@ pub struct MlsGroupCreateConfig { pub(crate) join_config: MlsGroupJoinConfig, /// List of initial group context extensions pub(crate) group_context_extensions: Extensions, + /// List of initial leaf node extensions + pub(crate) leaf_node_extensions: Extensions, } /// Builder struct for an [`MlsGroupJoinConfig`]. @@ -298,6 +302,12 @@ impl MlsGroupCreateConfigBuilder { self } + /// Sets the `capabilities` of the group creator's leaf node. + pub fn capabilities(mut self, capabilities: Capabilities) -> Self { + self.config.capabilities = capabilities; + self + } + /// Sets the `sender_ratchet_configuration` property of the MlsGroupCreateConfig. /// See [`SenderRatchetConfiguration`] for more information. pub fn sender_ratchet_configuration( @@ -345,6 +355,23 @@ impl MlsGroupCreateConfigBuilder { Ok(self) } + /// Sets extensions of the group creator's [`LeafNode`]. + pub fn with_leaf_node_extensions( + mut self, + extensions: Extensions, + ) -> Result { + // None of the default extensions are leaf node extensions, so only + // unknown extensions can be leaf node extensions. + let is_valid_in_leaf_node = extensions + .iter() + .all(|e| matches!(e.extension_type(), ExtensionType::Unknown(_))); + if !is_valid_in_leaf_node { + return Err(InvalidExtensionError::IllegalInLeafNodes); + } + self.config.leaf_node_extensions = extensions; + Ok(self) + } + /// Finalizes the builder and retursn an `[MlsGroupCreateConfig`]. pub fn build(self) -> MlsGroupCreateConfig { self.config diff --git a/openmls/src/group/mls_group/test_mls_group.rs b/openmls/src/group/mls_group/test_mls_group.rs index 2587a2066e..d97c9d39d9 100644 --- a/openmls/src/group/mls_group/test_mls_group.rs +++ b/openmls/src/group/mls_group/test_mls_group.rs @@ -5,10 +5,12 @@ use tls_codec::{Deserialize, Serialize}; use crate::{ binary_tree::LeafNodeIndex, + extensions::errors::InvalidExtensionError, framing::*, group::{config::CryptoConfig, errors::*, *}, key_packages::*, messages::proposals::*, + prelude::Capabilities, test_utils::test_framework::{ errors::ClientError, noop_authentication_service, ActionType::Commit, CodecUse, MlsGroupTestSetup, @@ -1109,6 +1111,17 @@ fn builder_pattern(ciphersuite: Ciphersuite, provider: &impl OpenMlsProvider) { let test_sender_ratchet_config = SenderRatchetConfiguration::new(10, 2000); let test_max_past_epochs = 10; let test_number_of_resumption_psks = 5; + let test_capabilities = Capabilities::new( + None, + None, + Some(&[ExtensionType::Unknown(0xff00)]), + None, + None, + ); + let test_leaf_extensions = Extensions::single(Extension::Unknown( + 0xff00, + UnknownExtension(vec![0x00, 0x01, 0x02]), + )); // === Alice creates a group === let alice_group = MlsGroup::builder() @@ -1122,6 +1135,9 @@ fn builder_pattern(ciphersuite: Ciphersuite, provider: &impl OpenMlsProvider) { .use_ratchet_tree_extension(true) .max_past_epochs(test_max_past_epochs) .number_of_resumption_psks(test_number_of_resumption_psks) + .with_leaf_node_extensions(test_leaf_extensions.clone()) + .unwrap() + .with_capabilities(test_capabilities.clone()) .build(provider, &alice_signer, alice_credential_with_key) .expect("error creating group using builder"); @@ -1161,4 +1177,20 @@ fn builder_pattern(ciphersuite: Ciphersuite, provider: &impl OpenMlsProvider) { .life_time() .expect("leaf doesn't have a lifetime"); assert_eq!(lifetime, &test_lifetime); + let own_leaf = alice_group.own_leaf_node().expect("can't find own leaf"); + let capabilities = own_leaf.capabilities(); + assert_eq!(capabilities, &test_capabilities); + let leaf_extensions = own_leaf.extensions(); + assert_eq!(leaf_extensions, &test_leaf_extensions); + + // Make sure that building with an invalid leaf node extension fails + let invalid_leaf_extensions = + Extensions::single(Extension::ApplicationId(ApplicationIdExtension::new(&[ + 0x00, 0x01, 0x02, + ]))); + + let builder_err = MlsGroup::builder() + .with_leaf_node_extensions(invalid_leaf_extensions) + .expect_err("successfully built group with invalid leaf extensions"); + assert_eq!(builder_err, InvalidExtensionError::IllegalInLeafNodes); } diff --git a/openmls/src/group/public_group/builder.rs b/openmls/src/group/public_group/builder.rs index 3da16bdf98..54d1edd453 100644 --- a/openmls/src/group/public_group/builder.rs +++ b/openmls/src/group/public_group/builder.rs @@ -8,7 +8,7 @@ use crate::{ errors::{ExtensionError, InvalidExtensionError}, Extension, Extensions, ExternalSendersExtension, RequiredCapabilitiesExtension, }, - group::{config::CryptoConfig, GroupContext, GroupId}, + group::{config::CryptoConfig, ExtensionType, GroupContext, GroupId}, key_packages::Lifetime, messages::ConfirmationTag, schedule::CommitSecret, @@ -25,9 +25,10 @@ pub(crate) struct TempBuilderPG1 { credential_with_key: CredentialWithKey, lifetime: Option, required_capabilities: Option, + capabilities: Option, external_senders: Option, - leaf_extensions: Option, - group_context_extensions: Option, + leaf_node_extensions: Extensions, + group_context_extensions: Extensions, } impl TempBuilderPG1 { @@ -44,6 +45,11 @@ impl TempBuilderPG1 { self } + pub(crate) fn with_capabilities(mut self, capabilities: Capabilities) -> Self { + self.capabilities = Some(capabilities); + self + } + pub(crate) fn with_external_senders( mut self, external_senders: ExternalSendersExtension, @@ -64,7 +70,23 @@ impl TempBuilderPG1 { if !is_valid_in_group_context { return Err(InvalidExtensionError::IllegalInGroupContext); } - self.group_context_extensions = Some(extensions); + self.group_context_extensions = extensions; + Ok(self) + } + + pub(crate) fn with_leaf_node_extensions( + mut self, + extensions: Extensions, + ) -> Result { + // None of the default extensions are leaf node extensions, so only + // unknown extensions can be leaf node extensions. + let is_valid_in_leaf_node = extensions + .iter() + .all(|e| matches!(e.extension_type(), ExtensionType::Unknown(_))); + if !is_valid_in_leaf_node { + return Err(InvalidExtensionError::IllegalInLeafNodes); + } + self.leaf_node_extensions = extensions; Ok(self) } @@ -73,24 +95,30 @@ impl TempBuilderPG1 { provider: &impl OpenMlsProvider, signer: &impl Signer, ) -> Result<(TempBuilderPG2, CommitSecret, EncryptionKeyPair), PublicGroupBuildError> { - let capabilities = self - .required_capabilities - .as_ref() - .map(|re| re.extension_types()); + // Capabilities are either provided by the builder or a minimal set of + // capabilities is created from the crypto config and the required + // capabilities + let capabilities = self.capabilities.unwrap_or(Capabilities::new( + Some(&[self.crypto_config.version]), + Some(&[self.crypto_config.ciphersuite]), + self.required_capabilities + .as_ref() + .map(|re| re.extension_types()), + self.required_capabilities + .as_ref() + .map(|re| re.proposal_types()), + self.required_capabilities + .as_ref() + .map(|re| re.credential_types()), + )); let (treesync, commit_secret, leaf_keypair) = TreeSync::new( provider, signer, self.crypto_config, self.credential_with_key, self.lifetime.unwrap_or_default(), - Capabilities::new( - Some(&[self.crypto_config.version]), // TODO: Allow more versions - Some(&[self.crypto_config.ciphersuite]), // TODO: allow more ciphersuites - capabilities, - None, - None, - ), - self.leaf_extensions.unwrap_or(Extensions::empty()), + capabilities, + self.leaf_node_extensions, )?; let required_capabilities = self.required_capabilities.unwrap_or_default(); required_capabilities.check_support().map_err(|e| match e { @@ -104,11 +132,7 @@ impl TempBuilderPG1 { })?; let required_capabilities = Extension::RequiredCapabilities(required_capabilities); - let mut group_context_extensions = if let Some(exts) = self.group_context_extensions { - exts - } else { - Extensions::empty() - }; + let mut group_context_extensions = self.group_context_extensions; group_context_extensions.add_or_replace(required_capabilities); if let Some(ext_senders) = self.external_senders { group_context_extensions.add_or_replace(Extension::ExternalSenders(ext_senders)); @@ -191,9 +215,10 @@ impl PublicGroup { credential_with_key, lifetime: None, required_capabilities: None, + capabilities: None, external_senders: None, - leaf_extensions: None, - group_context_extensions: None, + leaf_node_extensions: Extensions::empty(), + group_context_extensions: Extensions::empty(), } } } diff --git a/openmls/tests/book_code.rs b/openmls/tests/book_code.rs index b6b8c1399b..c3ef6832a0 100644 --- a/openmls/tests/book_code.rs +++ b/openmls/tests/book_code.rs @@ -133,11 +133,24 @@ fn book_operations(ciphersuite: Ciphersuite, provider: &impl OpenMlsProvider) { 10, // out_of_order_tolerance 2000, // maximum_forward_distance )) + .crypto_config(CryptoConfig::with_default_version(ciphersuite)) + .capabilities(Capabilities::new( + None, // Defaults to the group's protocol version + None, // Defaults to the group's ciphersuite + None, // Defaults to all basic extension types + None, // Defaults to all basic proposal types + Some(&[CredentialType::Basic]), + )) + // Example leaf extension + .with_leaf_node_extensions(Extensions::single(Extension::Unknown( + 0xff00, + UnknownExtension(vec![0, 1, 2, 3]), + ))) + .expect("failed to configure leaf extensions") .external_senders(vec![ExternalSender::new( ds_credential_with_key.signature_key.clone(), ds_credential_with_key.credential.clone(), )]) - .crypto_config(CryptoConfig::with_default_version(ciphersuite)) .use_ratchet_tree_extension(true) .build(); // ANCHOR_END: mls_group_create_config_example From 0d67da863b88f19226558229a6aeba021338914e Mon Sep 17 00:00:00 2001 From: Konrad Kohbrok Date: Thu, 18 Jan 2024 15:50:15 +0100 Subject: [PATCH 03/17] Clean up `MlsGroup*` builders (#1478) --- CHANGELOG.md | 4 + book/src/user_manual/group_config.md | 3 +- openmls/src/group/core_group/mod.rs | 23 +---- .../src/group/core_group/test_proposals.rs | 15 +++- openmls/src/group/mls_group/builder.rs | 33 +++----- openmls/src/group/mls_group/config.rs | 35 +------- openmls/src/group/mls_group/test_mls_group.rs | 13 +-- openmls/src/group/public_group/builder.rs | 83 +++++++------------ .../group/tests/external_remove_proposal.rs | 7 +- openmls/tests/book_code.rs | 15 ++-- 10 files changed, 81 insertions(+), 150 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 16712e2efc..a5f0809f13 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -10,6 +10,10 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Changed - [#1464](https://github.com/openmls/openmls/pull/1464): Add builder pattern for `MlsGroup`; split `MlsGroupJoinConfig` into `MlsGroupCreateConfig` and `MlsGroupJoinConfig` +- [#1473](https://github.com/openmls/openmls/pull/1473): Allow setting group context extensions when building an MlsGroup(Config). +- [#1475](https://github.com/openmls/openmls/pull/1475): Fully process GroupContextExtension proposals +- [#1477](https://github.com/openmls/openmls/pull/1477): Allow setting leaf node extensions and capabilities of the group creator when creating an MlsGroup(Config) +- [#1478](https://github.com/openmls/openmls/pull/1478): Remove explicit functions to set `RequiredCapabilitiesExtension` and `ExternalSendersExtension` when building an MlsGroup(Config) in favor of the more general function to set group context extensions ## 0.5.0 (XXXX-XX-XX) diff --git a/book/src/user_manual/group_config.md b/book/src/user_manual/group_config.md index 2f498569d3..55e6f5b742 100644 --- a/book/src/user_manual/group_config.md +++ b/book/src/user_manual/group_config.md @@ -17,8 +17,7 @@ Two very similar structs can help configure groups upon their creation: `MlsGrou | Name | Type | Explanation | | ------------------------------ | ------------------------------- | ------------------------------------------------------------------------------------------------ | -| `required_capabilities` | `RequiredCapabilitiesExtension` | Required capabilities (extensions and proposal types). | -| `external_senders` | `ExternalSendersExtensions` | List credentials of non-group members that are allowed to send proposals to the group. | +| `group_context_extensions` | `Extensions` | Optional group-level extensions, e.g. `RequiredCapabilitiesExtension`. | | `capabilities` . | `Capabilities` | Lists the capabilities of the group's creator. | | `leaf_extensions` . | `Extensions` | Extensions to be included in the group creator's leaf | diff --git a/openmls/src/group/core_group/mod.rs b/openmls/src/group/core_group/mod.rs index 5dcfd3226d..b17891cf1e 100644 --- a/openmls/src/group/core_group/mod.rs +++ b/openmls/src/group/core_group/mod.rs @@ -180,33 +180,12 @@ impl CoreGroupBuilder { self.psk_ids = psk_ids; self } - /// Set the [`RequiredCapabilitiesExtension`] of the [`CoreGroup`]. - pub(crate) fn with_required_capabilities( - mut self, - required_capabilities: RequiredCapabilitiesExtension, - ) -> Self { - self.public_group_builder = self - .public_group_builder - .with_required_capabilities(required_capabilities); - self - } + /// Set the [`Capabilities`] of the group's creator. pub(crate) fn with_capabilities(mut self, capabilities: Capabilities) -> Self { self.public_group_builder = self.public_group_builder.with_capabilities(capabilities); self } - /// Set the [`ExternalSendersExtension`] of the [`CoreGroup`]. - pub(crate) fn with_external_senders( - mut self, - external_senders: ExternalSendersExtension, - ) -> Self { - if !external_senders.is_empty() { - self.public_group_builder = self - .public_group_builder - .with_external_senders(external_senders); - } - self - } /// Sets initial group context extensions. Note that RequiredCapabilities /// extensions will be overwritten, and should be set using a call to diff --git a/openmls/src/group/core_group/test_proposals.rs b/openmls/src/group/core_group/test_proposals.rs index 4f9812c9a1..21209b115c 100644 --- a/openmls/src/group/core_group/test_proposals.rs +++ b/openmls/src/group/core_group/test_proposals.rs @@ -298,7 +298,10 @@ fn test_required_unsupported_proposals(ciphersuite: Ciphersuite, provider: &impl CryptoConfig::with_default_version(ciphersuite), alice_credential, ) - .with_required_capabilities(required_capabilities) + .with_group_context_extensions(Extensions::single(Extension::RequiredCapabilities( + required_capabilities, + ))) + .unwrap() .build(provider, &alice_signer) .expect_err( "CoreGroup creation must fail because AppAck proposals aren't supported in OpenMLS yet.", @@ -339,7 +342,10 @@ fn test_group_context_extensions(ciphersuite: Ciphersuite, provider: &impl OpenM CryptoConfig::with_default_version(ciphersuite), alice_credential, ) - .with_required_capabilities(required_capabilities) + .with_group_context_extensions(Extensions::single(Extension::RequiredCapabilities( + required_capabilities, + ))) + .unwrap() .build(provider, &alice_signer) .expect("Error creating CoreGroup."); @@ -417,7 +423,10 @@ fn test_group_context_extension_proposal_fails( CryptoConfig::with_default_version(ciphersuite), alice_credential, ) - .with_required_capabilities(required_capabilities) + .with_group_context_extensions(Extensions::single(Extension::RequiredCapabilities( + required_capabilities, + ))) + .unwrap() .build(provider, &alice_signer) .expect("Error creating CoreGroup."); diff --git a/openmls/src/group/mls_group/builder.rs b/openmls/src/group/mls_group/builder.rs index bcd27d8f66..6607727dbb 100644 --- a/openmls/src/group/mls_group/builder.rs +++ b/openmls/src/group/mls_group/builder.rs @@ -2,13 +2,16 @@ use openmls_traits::{key_store::OpenMlsKeyStore, signatures::Signer, OpenMlsProv use crate::{ credentials::CredentialWithKey, - extensions::{errors::InvalidExtensionError, Extensions, ExternalSendersExtension}, + error::LibraryError, + extensions::{errors::InvalidExtensionError, Extensions}, group::{ config::CryptoConfig, public_group::errors::PublicGroupBuildError, CoreGroup, CoreGroupBuildError, CoreGroupConfig, GroupId, MlsGroupCreateConfig, MlsGroupCreateConfigBuilder, NewGroupError, ProposalStore, WireFormatPolicy, }, - prelude::{Capabilities, LibraryError, Lifetime, SenderRatchetConfiguration}, + key_packages::Lifetime, + tree::sender_ratchet::SenderRatchetConfiguration, + treesync::node::leaf_node::Capabilities, }; use super::{InnerState, MlsGroup, MlsGroupState}; @@ -70,8 +73,6 @@ impl MlsGroupBuilder { credential_with_key, ) .with_config(group_config) - .with_required_capabilities(mls_group_create_config.required_capabilities.clone()) - .with_external_senders(mls_group_create_config.external_senders.clone()) .with_group_context_extensions(mls_group_create_config.group_context_extensions.clone())? .with_leaf_node_extensions(mls_group_create_config.leaf_node_extensions.clone())? .with_capabilities(mls_group_create_config.capabilities.clone()) @@ -194,22 +195,6 @@ impl MlsGroupBuilder { self } - /// Sets the `external_senders` property of the MlsGroup. - pub fn external_senders(mut self, external_senders: ExternalSendersExtension) -> Self { - self.mls_group_create_config_builder = self - .mls_group_create_config_builder - .external_senders(external_senders); - self - } - - /// Sets the group creator's [`Capabilities`] - pub fn with_capabilities(mut self, capabilities: Capabilities) -> Self { - self.mls_group_create_config_builder = self - .mls_group_create_config_builder - .capabilities(capabilities); - self - } - /// Sets the initial group context extensions pub fn with_group_context_extensions( mut self, @@ -231,4 +216,12 @@ impl MlsGroupBuilder { .with_leaf_node_extensions(extensions)?; Ok(self) } + + /// Sets the group creator's [`Capabilities`] + pub fn with_capabilities(mut self, capabilities: Capabilities) -> Self { + self.mls_group_create_config_builder = self + .mls_group_create_config_builder + .capabilities(capabilities); + self + } } diff --git a/openmls/src/group/mls_group/config.rs b/openmls/src/group/mls_group/config.rs index 673548ad5d..884e160388 100644 --- a/openmls/src/group/mls_group/config.rs +++ b/openmls/src/group/mls_group/config.rs @@ -83,12 +83,8 @@ impl MlsGroupJoinConfig { /// more information about the different configuration values. #[derive(Clone, Debug, Default, PartialEq, Eq, Serialize, Deserialize)] pub struct MlsGroupCreateConfig { - /// Required capabilities (extensions and proposal types) - pub(crate) required_capabilities: RequiredCapabilitiesExtension, /// Capabilities advertised in the creator's leaf node pub(crate) capabilities: Capabilities, - /// Senders authorized to send external remove proposals - pub(crate) external_senders: ExternalSendersExtension, /// Lifetime of the own leaf node pub(crate) lifetime: Lifetime, /// Ciphersuite and protocol version @@ -191,21 +187,11 @@ impl MlsGroupCreateConfig { self.join_config.use_ratchet_tree_extension } - /// Returns the [`MlsGroupCreateConfig`] required capabilities extension. - pub fn required_capabilities(&self) -> &RequiredCapabilitiesExtension { - &self.required_capabilities - } - /// Returns the [`MlsGroupCreateConfig`] sender ratchet configuration. pub fn sender_ratchet_configuration(&self) -> &SenderRatchetConfiguration { &self.join_config.sender_ratchet_configuration } - /// Returns the [`MlsGroupCreateConfig`] external senders extension - pub fn external_senders(&self) -> &ExternalSendersExtension { - &self.external_senders - } - /// Returns the [`Extensions`] set as the initial group context. /// This does not contain the initial group context extensions /// added from builder calls to `external_senders` or `required_capabilities`. @@ -293,15 +279,6 @@ impl MlsGroupCreateConfigBuilder { self } - /// Sets the `required_capabilities` property of the MlsGroupCreateConfig. - pub fn required_capabilities( - mut self, - required_capabilities: RequiredCapabilitiesExtension, - ) -> Self { - self.config.required_capabilities = required_capabilities; - self - } - /// Sets the `capabilities` of the group creator's leaf node. pub fn capabilities(mut self, capabilities: Capabilities) -> Self { self.config.capabilities = capabilities; @@ -330,17 +307,7 @@ impl MlsGroupCreateConfigBuilder { self } - /// Sets the `external_senders` property of the MlsGroupCreateConfig. - pub fn external_senders(mut self, external_senders: ExternalSendersExtension) -> Self { - self.config.external_senders = external_senders; - self - } - - /// Sets initial group context extensions. Note that RequiredCapabilities - /// extensions will be overwritten, and should be set using a call to - /// `required_capabilities`. If `ExternalSenders` extensions are provided - /// both in this call, and a call to `external_senders`, only the one from - /// the call to `external_senders` will be included. + /// Sets initial group context extensions. pub fn with_group_context_extensions( mut self, extensions: Extensions, diff --git a/openmls/src/group/mls_group/test_mls_group.rs b/openmls/src/group/mls_group/test_mls_group.rs index d97c9d39d9..3052980b8f 100644 --- a/openmls/src/group/mls_group/test_mls_group.rs +++ b/openmls/src/group/mls_group/test_mls_group.rs @@ -1016,14 +1016,12 @@ fn group_context_extensions_proposal(ciphersuite: Ciphersuite, provider: &impl O .build(provider, &alice_signer, alice_credential_with_key) .expect("error creating group using builder"); - let required_capabilities = alice_group + // No required capabilities, so no specifically required extensions. + assert!(alice_group .group() .group_context_extensions() .required_capabilities() - .expect("couldn't get required_capabilities"); - - // no required extensions - assert!(required_capabilities.extension_types().is_empty()); + .is_none()); let new_extensions = Extensions::single(Extension::RequiredCapabilities( RequiredCapabilitiesExtension::new(&[ExtensionType::RequiredCapabilities], &[], &[]), @@ -1128,7 +1126,10 @@ fn builder_pattern(ciphersuite: Ciphersuite, provider: &impl OpenMlsProvider) { .with_group_id(test_group_id.clone()) .padding_size(test_padding_size) .sender_ratchet_configuration(test_sender_ratchet_config.clone()) - .external_senders(test_external_senders.clone()) + .with_group_context_extensions(Extensions::single(Extension::ExternalSenders( + test_external_senders.clone(), + ))) + .unwrap() .crypto_config(test_crypto_config) .with_wire_format_policy(test_wire_format_policy) .lifetime(test_lifetime) diff --git a/openmls/src/group/public_group/builder.rs b/openmls/src/group/public_group/builder.rs index 54d1edd453..0e02a525fc 100644 --- a/openmls/src/group/public_group/builder.rs +++ b/openmls/src/group/public_group/builder.rs @@ -6,7 +6,7 @@ use crate::{ error::LibraryError, extensions::{ errors::{ExtensionError, InvalidExtensionError}, - Extension, Extensions, ExternalSendersExtension, RequiredCapabilitiesExtension, + Extensions, }, group::{config::CryptoConfig, ExtensionType, GroupContext, GroupId}, key_packages::Lifetime, @@ -24,9 +24,7 @@ pub(crate) struct TempBuilderPG1 { crypto_config: CryptoConfig, credential_with_key: CredentialWithKey, lifetime: Option, - required_capabilities: Option, capabilities: Option, - external_senders: Option, leaf_node_extensions: Extensions, group_context_extensions: Extensions, } @@ -37,29 +35,11 @@ impl TempBuilderPG1 { self } - pub(crate) fn with_required_capabilities( - mut self, - required_capabilities: RequiredCapabilitiesExtension, - ) -> Self { - self.required_capabilities = Some(required_capabilities); - self - } - pub(crate) fn with_capabilities(mut self, capabilities: Capabilities) -> Self { self.capabilities = Some(capabilities); self } - pub(crate) fn with_external_senders( - mut self, - external_senders: ExternalSendersExtension, - ) -> Self { - if !external_senders.is_empty() { - self.external_senders = Some(external_senders); - } - self - } - pub(crate) fn with_group_context_extensions( mut self, extensions: Extensions, @@ -95,21 +75,37 @@ impl TempBuilderPG1 { provider: &impl OpenMlsProvider, signer: &impl Signer, ) -> Result<(TempBuilderPG2, CommitSecret, EncryptionKeyPair), PublicGroupBuildError> { - // Capabilities are either provided by the builder or a minimal set of - // capabilities is created from the crypto config and the required - // capabilities + // If there are no capabilities, we want to provide a default version + // plus anything in the required capabilities. + let (required_extensions, required_proposals, required_credentials) = + if let Some(required_capabilities) = + self.group_context_extensions.required_capabilities() + { + // Also, while we're at it, check if we support all required + // capabilities ourselves. + required_capabilities.check_support().map_err(|e| match e { + ExtensionError::UnsupportedProposalType => { + PublicGroupBuildError::UnsupportedProposalType + } + ExtensionError::UnsupportedExtensionType => { + PublicGroupBuildError::UnsupportedExtensionType + } + _ => LibraryError::custom("Unexpected ExtensionError").into(), + })?; + ( + Some(required_capabilities.extension_types()), + Some(required_capabilities.proposal_types()), + Some(required_capabilities.credential_types()), + ) + } else { + (None, None, None) + }; let capabilities = self.capabilities.unwrap_or(Capabilities::new( Some(&[self.crypto_config.version]), Some(&[self.crypto_config.ciphersuite]), - self.required_capabilities - .as_ref() - .map(|re| re.extension_types()), - self.required_capabilities - .as_ref() - .map(|re| re.proposal_types()), - self.required_capabilities - .as_ref() - .map(|re| re.credential_types()), + required_extensions, + required_proposals, + required_credentials, )); let (treesync, commit_secret, leaf_keypair) = TreeSync::new( provider, @@ -120,29 +116,12 @@ impl TempBuilderPG1 { capabilities, self.leaf_node_extensions, )?; - let required_capabilities = self.required_capabilities.unwrap_or_default(); - required_capabilities.check_support().map_err(|e| match e { - ExtensionError::UnsupportedProposalType => { - PublicGroupBuildError::UnsupportedProposalType - } - ExtensionError::UnsupportedExtensionType => { - PublicGroupBuildError::UnsupportedExtensionType - } - _ => LibraryError::custom("Unexpected ExtensionError").into(), - })?; - let required_capabilities = Extension::RequiredCapabilities(required_capabilities); - - let mut group_context_extensions = self.group_context_extensions; - group_context_extensions.add_or_replace(required_capabilities); - if let Some(ext_senders) = self.external_senders { - group_context_extensions.add_or_replace(Extension::ExternalSenders(ext_senders)); - } let group_context = GroupContext::create_initial_group_context( self.crypto_config.ciphersuite, self.group_id, treesync.tree_hash().to_vec(), - group_context_extensions, + self.group_context_extensions, ); let next_builder = TempBuilderPG2 { treesync, @@ -214,9 +193,7 @@ impl PublicGroup { crypto_config, credential_with_key, lifetime: None, - required_capabilities: None, capabilities: None, - external_senders: None, leaf_node_extensions: Extensions::empty(), group_context_extensions: Extensions::empty(), } diff --git a/openmls/src/group/tests/external_remove_proposal.rs b/openmls/src/group/tests/external_remove_proposal.rs index bb057e4e35..e29d3d707b 100644 --- a/openmls/src/group/tests/external_remove_proposal.rs +++ b/openmls/src/group/tests/external_remove_proposal.rs @@ -30,7 +30,10 @@ fn new_test_group( let mls_group_config = MlsGroupCreateConfig::builder() .wire_format_policy(wire_format_policy) .crypto_config(CryptoConfig::with_default_version(ciphersuite)) - .external_senders(external_senders) + .with_group_context_extensions(Extensions::single(Extension::ExternalSenders( + external_senders, + ))) + .unwrap() .build(); ( @@ -336,6 +339,6 @@ fn external_remove_proposal_should_fail_when_no_external_senders( .unwrap_err(); assert_eq!( error, - ProcessMessageError::ValidationError(ValidationError::NoExternalSendersExtension) + ProcessMessageError::ValidationError(ValidationError::UnauthorizedExternalSender) ); } diff --git a/openmls/tests/book_code.rs b/openmls/tests/book_code.rs index c3ef6832a0..9bf48cc137 100644 --- a/openmls/tests/book_code.rs +++ b/openmls/tests/book_code.rs @@ -133,6 +133,13 @@ fn book_operations(ciphersuite: Ciphersuite, provider: &impl OpenMlsProvider) { 10, // out_of_order_tolerance 2000, // maximum_forward_distance )) + .with_group_context_extensions(Extensions::single(Extension::ExternalSenders(vec![ + ExternalSender::new( + ds_credential_with_key.signature_key.clone(), + ds_credential_with_key.credential.clone(), + ), + ]))) + .expect("error adding external senders extension to group context extensions") .crypto_config(CryptoConfig::with_default_version(ciphersuite)) .capabilities(Capabilities::new( None, // Defaults to the group's protocol version @@ -147,10 +154,6 @@ fn book_operations(ciphersuite: Ciphersuite, provider: &impl OpenMlsProvider) { UnknownExtension(vec![0, 1, 2, 3]), ))) .expect("failed to configure leaf extensions") - .external_senders(vec![ExternalSender::new( - ds_credential_with_key.signature_key.clone(), - ds_credential_with_key.credential.clone(), - )]) .use_ratchet_tree_extension(true) .build(); // ANCHOR_END: mls_group_create_config_example @@ -214,10 +217,6 @@ fn book_operations(ciphersuite: Ciphersuite, provider: &impl OpenMlsProvider) { 10, // out_of_order_tolerance 2000, // maximum_forward_distance )) - .external_senders(vec![ExternalSender::new( - ds_credential_with_key.signature_key, - ds_credential_with_key.credential, - )]) .crypto_config(CryptoConfig::with_default_version(ciphersuite)) .use_ratchet_tree_extension(true) .build(provider, &alice_signature_keys, alice_credential.clone()) From 303967cf2304c78e9d622b0cd381b7a15dcb5fb9 Mon Sep 17 00:00:00 2001 From: Konrad Kohbrok Date: Fri, 19 Jan 2024 14:19:13 +0100 Subject: [PATCH 04/17] Enable the use of unknown/opaque extensions (#1479) --- CHANGELOG.md | 1 + book/src/user_manual/group_config.md | 4 + openmls/src/extensions/mod.rs | 15 --- .../src/extensions/required_capabilities.rs | 5 - .../src/group/core_group/test_proposals.rs | 72 ++++++++---- openmls/src/group/mls_group/test_mls_group.rs | 111 ++++++++++++++++-- 6 files changed, 157 insertions(+), 51 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index a5f0809f13..11a1376e1d 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -14,6 +14,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - [#1475](https://github.com/openmls/openmls/pull/1475): Fully process GroupContextExtension proposals - [#1477](https://github.com/openmls/openmls/pull/1477): Allow setting leaf node extensions and capabilities of the group creator when creating an MlsGroup(Config) - [#1478](https://github.com/openmls/openmls/pull/1478): Remove explicit functions to set `RequiredCapabilitiesExtension` and `ExternalSendersExtension` when building an MlsGroup(Config) in favor of the more general function to set group context extensions +- [#1479](https://github.com/openmls/openmls/pull/1479): Allow the use of extensions with `ExtensionType::Unknown` in group context, key packages and leaf nodes ## 0.5.0 (XXXX-XX-XX) diff --git a/book/src/user_manual/group_config.md b/book/src/user_manual/group_config.md index 55e6f5b742..dbaf943c14 100644 --- a/book/src/user_manual/group_config.md +++ b/book/src/user_manual/group_config.md @@ -34,3 +34,7 @@ Example create configuration: ```rust,no_run,noplayground {{#include ../../../openmls/tests/book_code.rs:mls_group_create_config_example}} ``` + +## Unknown extensions + +Some extensions carry data, but don't alter the behaviour of the protocol (e.g. the application_id extension). OpenMLS allows the use of arbitrary such extensions in the group context, key packages and leaf nodes. Such extensions can be instantiated and retrieved through the use of the `UnknownExtension` struct and the `ExtensionType::Unknown` extension type. Such "unknown" extensions are handled transparently by OpenMLS, but can be used by the application, e.g. to have a group agree on pieces of data. diff --git a/openmls/src/extensions/mod.rs b/openmls/src/extensions/mod.rs index 9c2554d015..20f46beec5 100644 --- a/openmls/src/extensions/mod.rs +++ b/openmls/src/extensions/mod.rs @@ -167,21 +167,6 @@ impl From for u16 { } } -impl ExtensionType { - /// Check whether an [`ExtensionType`] is supported or not. - pub fn is_supported(&self) -> bool { - matches!( - self, - ExtensionType::ApplicationId - | ExtensionType::RatchetTree - | ExtensionType::RequiredCapabilities - | ExtensionType::ExternalPub - | ExtensionType::ExternalSenders - | ExtensionType::LastResort - ) - } -} - /// # Extension /// /// An extension is one of the [`Extension`] enum values. diff --git a/openmls/src/extensions/required_capabilities.rs b/openmls/src/extensions/required_capabilities.rs index d00081beaf..9dd5b0f9bb 100644 --- a/openmls/src/extensions/required_capabilities.rs +++ b/openmls/src/extensions/required_capabilities.rs @@ -78,11 +78,6 @@ impl RequiredCapabilitiesExtension { /// Check if all extension and proposal types are supported. pub(crate) fn check_support(&self) -> Result<(), ExtensionError> { - for extension in self.extension_types() { - if !extension.is_supported() { - return Err(ExtensionError::UnsupportedExtensionType); - } - } for proposal in self.proposal_types() { if !proposal.is_supported() { return Err(ExtensionError::UnsupportedProposalType); diff --git a/openmls/src/group/core_group/test_proposals.rs b/openmls/src/group/core_group/test_proposals.rs index 21209b115c..197c2d3272 100644 --- a/openmls/src/group/core_group/test_proposals.rs +++ b/openmls/src/group/core_group/test_proposals.rs @@ -312,6 +312,56 @@ fn test_required_unsupported_proposals(ciphersuite: Ciphersuite, provider: &impl ) } +#[apply(ciphersuites_and_providers)] +fn test_required_extension_key_package_mismatch( + ciphersuite: Ciphersuite, + provider: &impl OpenMlsProvider, +) { + // Basic group setup. + let group_aad = b"Alice's test group"; + let framing_parameters = FramingParameters::new(group_aad, WireFormat::PublicMessage); + + let (alice_credential, _, alice_signer, _alice_pk) = + setup_client("Alice", ciphersuite, provider); + let (_bob_credential_with_key, bob_key_package_bundle, _, _) = + setup_client("Bob", ciphersuite, provider); + let bob_key_package = bob_key_package_bundle.key_package(); + + // Set required capabilities + let extensions = &[ExtensionType::Unknown(0xff00)]; + // We don't support unknown proposals (yet) + let proposals = &[]; + let credentials = &[CredentialType::Basic]; + let required_capabilities = + RequiredCapabilitiesExtension::new(extensions, proposals, credentials); + + let alice_group = CoreGroup::builder( + GroupId::random(provider.rand()), + CryptoConfig::with_default_version(ciphersuite), + alice_credential, + ) + .with_group_context_extensions(Extensions::single(Extension::RequiredCapabilities( + required_capabilities, + ))) + .expect("error adding group context extensions") + .build(provider, &alice_signer) + .expect("Error creating CoreGroup."); + + let e = alice_group + .create_add_proposal( + framing_parameters, + bob_key_package.clone(), + &alice_signer, + ) + .expect_err("Proposal was created even though the key package didn't support the required extensions."); + assert_eq!( + e, + CreateAddProposalError::LeafNodeValidation( + crate::treesync::errors::LeafNodeValidationError::UnsupportedExtensions + ) + ); +} + #[apply(ciphersuites_and_providers)] fn test_group_context_extensions(ciphersuite: Ciphersuite, provider: &impl OpenMlsProvider) { // Basic group setup. @@ -430,28 +480,6 @@ fn test_group_context_extension_proposal_fails( .build(provider, &alice_signer) .expect("Error creating CoreGroup."); - // TODO: openmls/openmls#1130 add a test for unsupported required capabilities. - // We can't test this right now because we don't have a capability - // that is not a "default" proposal or extension. - // // Alice tries to add a required capability she doesn't support herself. - // let required_application_id = Extension::RequiredCapabilities( - // RequiredCapabilitiesExtension::new(&[ExtensionType::ApplicationId], &[]), - // ); - // let e = alice_group.create_group_context_ext_proposal( - // framing_parameters, - // &alice_credential_bundle, - // &[required_application_id.clone()], - // provider, - // ).expect_err("Alice was able to create a gce proposal with a required extensions she doesn't support."); - // assert_eq!( - // e, - // CreateGroupContextExtProposalError::TreeSyncError( - // crate::treesync::errors::TreeSyncError::UnsupportedExtension - // ) - // ); - // - // // Well, this failed luckily. - // Adding Bob let bob_add_proposal = alice_group .create_add_proposal(framing_parameters, bob_key_package.clone(), &alice_signer) diff --git a/openmls/src/group/mls_group/test_mls_group.rs b/openmls/src/group/mls_group/test_mls_group.rs index 3052980b8f..55658d3ce6 100644 --- a/openmls/src/group/mls_group/test_mls_group.rs +++ b/openmls/src/group/mls_group/test_mls_group.rs @@ -1101,10 +1101,19 @@ fn builder_pattern(ciphersuite: Ciphersuite, provider: &impl OpenMlsProvider) { let test_lifetime = Lifetime::new(3600); let test_wire_format_policy = PURE_CIPHERTEXT_WIRE_FORMAT_POLICY; let test_padding_size = 100; - let test_external_senders = vec![ExternalSender::new( + let test_external_senders = Extension::ExternalSenders(vec![ExternalSender::new( alice_credential_with_key.signature_key.clone(), alice_credential_with_key.credential.clone(), - )]; + )]); + let test_required_capabilities = Extension::RequiredCapabilities( + RequiredCapabilitiesExtension::new(&[ExtensionType::Unknown(0xff00)], &[], &[]), + ); + let test_gc_extensions = Extensions::from_vec(vec![ + test_external_senders.clone(), + test_required_capabilities.clone(), + ]) + .expect("error creating group context extensions"); + let test_crypto_config = CryptoConfig::with_default_version(ciphersuite); let test_sender_ratchet_config = SenderRatchetConfiguration::new(10, 2000); let test_max_past_epochs = 10; @@ -1126,10 +1135,8 @@ fn builder_pattern(ciphersuite: Ciphersuite, provider: &impl OpenMlsProvider) { .with_group_id(test_group_id.clone()) .padding_size(test_padding_size) .sender_ratchet_configuration(test_sender_ratchet_config.clone()) - .with_group_context_extensions(Extensions::single(Extension::ExternalSenders( - test_external_senders.clone(), - ))) - .unwrap() + .with_group_context_extensions(test_gc_extensions.clone()) + .expect("error adding group context extension to builder") .crypto_config(test_crypto_config) .with_wire_format_policy(test_wire_format_policy) .lifetime(test_lifetime) @@ -1137,7 +1144,7 @@ fn builder_pattern(ciphersuite: Ciphersuite, provider: &impl OpenMlsProvider) { .max_past_epochs(test_max_past_epochs) .number_of_resumption_psks(test_number_of_resumption_psks) .with_leaf_node_extensions(test_leaf_extensions.clone()) - .unwrap() + .expect("error adding leaf node extension to builder") .with_capabilities(test_capabilities.clone()) .build(provider, &alice_signer, alice_credential_with_key) .expect("error creating group using builder"); @@ -1165,13 +1172,19 @@ fn builder_pattern(ciphersuite: Ciphersuite, provider: &impl OpenMlsProvider) { let external_senders = group_context .extensions() .external_senders() - .expect("error getting external senders"); - assert_eq!(external_senders, &test_external_senders); + .expect("error getting external senders") + .to_vec(); + assert_eq!( + Extension::ExternalSenders(external_senders), + test_external_senders + ); let crypto_config = CryptoConfig { ciphersuite, version: group_context.protocol_version(), }; assert_eq!(crypto_config, test_crypto_config); + let extensions = group_context.extensions(); + assert_eq!(extensions, &test_gc_extensions); let lifetime = alice_group .own_leaf() .expect("error getting own leaf") @@ -1195,3 +1208,83 @@ fn builder_pattern(ciphersuite: Ciphersuite, provider: &impl OpenMlsProvider) { .expect_err("successfully built group with invalid leaf extensions"); assert_eq!(builder_err, InvalidExtensionError::IllegalInLeafNodes); } + +// Test that unknown group context and leaf node extensions can be used in groups +#[apply(ciphersuites_and_providers)] +fn unknown_extensions(ciphersuite: Ciphersuite, provider: &impl OpenMlsProvider) { + let (alice_credential_with_key, _alice_kpb, alice_signer, _alice_pk) = + setup_client("Alice", ciphersuite, provider); + + let unknown_gc_extension = Extension::Unknown(0xff00, UnknownExtension(vec![0, 1, 2, 3])); + let unknown_leaf_extension = Extension::Unknown(0xff01, UnknownExtension(vec![4, 5, 6, 7])); + let unknown_kp_extension = Extension::Unknown(0xff02, UnknownExtension(vec![8, 9, 10, 11])); + let required_extensions = &[ + ExtensionType::Unknown(0xff00), + ExtensionType::Unknown(0xff01), + ]; + let required_capabilities = + Extension::RequiredCapabilities(RequiredCapabilitiesExtension::new(&[], &[], &[])); + let capabilities = Capabilities::new(None, None, Some(required_extensions), None, None); + let test_gc_extensions = Extensions::from_vec(vec![ + unknown_gc_extension.clone(), + required_capabilities.clone(), + ]) + .expect("error creating group context extensions"); + let test_kp_extensions = Extensions::single(unknown_kp_extension.clone()); + + // === Alice creates a group === + let config = CryptoConfig { + ciphersuite, + version: crate::versions::ProtocolVersion::default(), + }; + let mut alice_group = MlsGroup::builder() + .crypto_config(config) + .with_capabilities(capabilities.clone()) + .with_leaf_node_extensions(Extensions::single(unknown_leaf_extension.clone())) + .expect("error adding unknown leaf extension to builder") + .with_group_context_extensions(test_gc_extensions.clone()) + .expect("error adding unknown extension to builder") + .build(provider, &alice_signer, alice_credential_with_key) + .expect("error creating group using builder"); + + // Check that everything was added successfully + let group_context = alice_group.export_group_context(); + assert_eq!(group_context.extensions(), &test_gc_extensions); + let leaf_node = alice_group.own_leaf().expect("error getting own leaf"); + assert_eq!( + leaf_node.extensions(), + &Extensions::single(unknown_leaf_extension) + ); + + // Now let's add Bob to the group and make sure that he joins the group successfully + + // === Alice adds Bob === + let (bob_credential_with_key, _bob_kpb, bob_signer, _bob_pk) = + setup_client("Bob", ciphersuite, provider); + + // Generate a KP that supports the unknown extensions + let bob_key_package = KeyPackage::builder() + .leaf_node_capabilities(capabilities) + .key_package_extensions(test_kp_extensions.clone()) + .build(config, provider, &bob_signer, bob_credential_with_key) + .expect("error building key package"); + + assert_eq!( + bob_key_package.extensions(), + &Extensions::single(unknown_kp_extension) + ); + + // alice adds bob and bob processes the welcome to ensure that the unknown + // extensions are processed correctly + let (_, welcome, _) = alice_group + .add_members(provider, &alice_signer, &[bob_key_package.clone()]) + .unwrap(); + alice_group.merge_pending_commit(provider).unwrap(); + let _bob_group = MlsGroup::new_from_welcome( + provider, + &MlsGroupJoinConfig::default(), + welcome.into_welcome().unwrap(), + Some(alice_group.export_ratchet_tree().into()), + ) + .expect("Error creating group from Welcome"); +} From cbb4c11e3e19ca68e68ac5bbaf833c12f268c2c6 Mon Sep 17 00:00:00 2001 From: "Jan Winkelmann (keks)" Date: Wed, 10 Jan 2024 14:45:55 +0100 Subject: [PATCH 05/17] Add first draft of mutable metadata extension This is similar to the ProtectedMetadata extension, with the exception that the payload is not signed. Whether and how the payload should be processed is up to the application. --- openmls/src/extensions/codec.rs | 10 +++++++++- openmls/src/extensions/mod.rs | 11 +++++++++++ openmls/src/extensions/mutable_metadata.rs | 19 +++++++++++++++++++ 3 files changed, 39 insertions(+), 1 deletion(-) create mode 100644 openmls/src/extensions/mutable_metadata.rs diff --git a/openmls/src/extensions/codec.rs b/openmls/src/extensions/codec.rs index 15e0e3a48e..eaa59409d1 100644 --- a/openmls/src/extensions/codec.rs +++ b/openmls/src/extensions/codec.rs @@ -8,7 +8,10 @@ use crate::extensions::{ UnknownExtension, }; -use super::{last_resort::LastResortExtension, protected_metadata::ProtectedMetadata}; +use super::{ + last_resort::LastResortExtension, mutable_metadata::MutableMetadata, + protected_metadata::ProtectedMetadata, +}; fn vlbytes_len_len(length: usize) -> usize { if length < 0x40 { @@ -38,6 +41,7 @@ impl Size for Extension { Extension::ExternalSenders(e) => e.tls_serialized_len(), Extension::LastResort(e) => e.tls_serialized_len(), Extension::ProtectedMetadata(e) => e.tls_serialized_len(), + Extension::MutableMetadata(e) => e.tls_serialized_len(), Extension::Unknown(_, e) => e.0.len(), }; @@ -71,6 +75,7 @@ impl Serialize for Extension { Extension::ExternalSenders(e) => e.tls_serialize(&mut extension_data), Extension::LastResort(e) => e.tls_serialize(&mut extension_data), Extension::ProtectedMetadata(e) => e.tls_serialize(&mut extension_data), + Extension::MutableMetadata(e) => e.tls_serialize(&mut extension_data), Extension::Unknown(_, e) => extension_data .write_all(e.0.as_slice()) .map(|_| e.0.len()) @@ -123,6 +128,9 @@ impl Deserialize for Extension { ExtensionType::ProtectedMetadata => Extension::ProtectedMetadata( ProtectedMetadata::tls_deserialize(&mut extension_data)?, ), + ExtensionType::MutableMetadata => { + Extension::MutableMetadata(MutableMetadata::tls_deserialize(&mut extension_data)?) + } ExtensionType::Unknown(unknown) => { Extension::Unknown(unknown, UnknownExtension(extension_data.to_vec())) } diff --git a/openmls/src/extensions/mod.rs b/openmls/src/extensions/mod.rs index 6ca68e983e..798408e682 100644 --- a/openmls/src/extensions/mod.rs +++ b/openmls/src/extensions/mod.rs @@ -32,6 +32,7 @@ mod codec; mod external_pub_extension; mod external_sender_extension; mod last_resort; +mod mutable_metadata; mod protected_metadata; mod ratchet_tree_extension; mod required_capabilities; @@ -56,6 +57,8 @@ use tls_codec::{ pub use protected_metadata::ProtectedMetadata; +use self::mutable_metadata::MutableMetadata; + #[cfg(test)] mod test_extensions; @@ -104,6 +107,8 @@ pub enum ExtensionType { /// extension ProtectedMetadata, + MutableMetadata, + /// A currently unknown extension type. Unknown(u16), } @@ -156,6 +161,7 @@ impl From for ExtensionType { 5 => ExtensionType::ExternalSenders, 10 => ExtensionType::LastResort, 11 => ExtensionType::ProtectedMetadata, + 0xf001 => ExtensionType::MutableMetadata, unknown => ExtensionType::Unknown(unknown), } } @@ -171,6 +177,7 @@ impl From for u16 { ExtensionType::ExternalSenders => 5, ExtensionType::LastResort => 10, ExtensionType::ProtectedMetadata => 11, + ExtensionType::MutableMetadata => 0xf001, ExtensionType::Unknown(unknown) => unknown, } } @@ -188,6 +195,7 @@ impl ExtensionType { | ExtensionType::ExternalSenders | ExtensionType::LastResort | ExtensionType::ProtectedMetadata + | ExtensionType::MutableMetadata ) } } @@ -229,6 +237,8 @@ pub enum Extension { /// A [`ProtectedMetadata`] extension ProtectedMetadata(ProtectedMetadata), + MutableMetadata(MutableMetadata), + /// A currently unknown extension. Unknown(u16, UnknownExtension), } @@ -518,6 +528,7 @@ impl Extension { Extension::ExternalSenders(_) => ExtensionType::ExternalSenders, Extension::LastResort(_) => ExtensionType::LastResort, Extension::ProtectedMetadata(_) => ExtensionType::ProtectedMetadata, + Extension::MutableMetadata(_) => ExtensionType::MutableMetadata, Extension::Unknown(kind, _) => ExtensionType::Unknown(*kind), } } diff --git a/openmls/src/extensions/mutable_metadata.rs b/openmls/src/extensions/mutable_metadata.rs new file mode 100644 index 0000000000..be24578682 --- /dev/null +++ b/openmls/src/extensions/mutable_metadata.rs @@ -0,0 +1,19 @@ +use super::{Deserialize, Serialize}; +use tls_codec::{TlsDeserialize, TlsSerialize, TlsSize}; + +#[derive( + PartialEq, Eq, Clone, Debug, Serialize, Deserialize, TlsDeserialize, TlsSerialize, TlsSize, +)] +pub struct MutableMetadata { + metadata: Vec, +} + +impl MutableMetadata { + pub fn new(metadata: Vec) -> Self { + Self { metadata } + } + + pub fn metadata(&self) -> &Vec { + &self.metadata + } +} From b646b47ba824c388dbf8f92ca474e983a8c1e6a1 Mon Sep 17 00:00:00 2001 From: "Jan Winkelmann (keks)" Date: Wed, 10 Jan 2024 16:50:58 +0100 Subject: [PATCH 06/17] rename to Metadata, add test # Conflicts: # openmls/src/extensions/test_extensions.rs --- openmls/src/extensions/codec.rs | 11 +++--- .../{mutable_metadata.rs => metadata.rs} | 4 +- openmls/src/extensions/mod.rs | 24 ++++++++---- openmls/src/extensions/test_extensions.rs | 38 +++++++++++++++++++ 4 files changed, 61 insertions(+), 16 deletions(-) rename openmls/src/extensions/{mutable_metadata.rs => metadata.rs} (87%) diff --git a/openmls/src/extensions/codec.rs b/openmls/src/extensions/codec.rs index eaa59409d1..a2962eab31 100644 --- a/openmls/src/extensions/codec.rs +++ b/openmls/src/extensions/codec.rs @@ -9,8 +9,7 @@ use crate::extensions::{ }; use super::{ - last_resort::LastResortExtension, mutable_metadata::MutableMetadata, - protected_metadata::ProtectedMetadata, + last_resort::LastResortExtension, metadata::Metadata, protected_metadata::ProtectedMetadata, }; fn vlbytes_len_len(length: usize) -> usize { @@ -41,7 +40,7 @@ impl Size for Extension { Extension::ExternalSenders(e) => e.tls_serialized_len(), Extension::LastResort(e) => e.tls_serialized_len(), Extension::ProtectedMetadata(e) => e.tls_serialized_len(), - Extension::MutableMetadata(e) => e.tls_serialized_len(), + Extension::Metadata(e) => e.tls_serialized_len(), Extension::Unknown(_, e) => e.0.len(), }; @@ -75,7 +74,7 @@ impl Serialize for Extension { Extension::ExternalSenders(e) => e.tls_serialize(&mut extension_data), Extension::LastResort(e) => e.tls_serialize(&mut extension_data), Extension::ProtectedMetadata(e) => e.tls_serialize(&mut extension_data), - Extension::MutableMetadata(e) => e.tls_serialize(&mut extension_data), + Extension::Metadata(e) => e.tls_serialize(&mut extension_data), Extension::Unknown(_, e) => extension_data .write_all(e.0.as_slice()) .map(|_| e.0.len()) @@ -128,8 +127,8 @@ impl Deserialize for Extension { ExtensionType::ProtectedMetadata => Extension::ProtectedMetadata( ProtectedMetadata::tls_deserialize(&mut extension_data)?, ), - ExtensionType::MutableMetadata => { - Extension::MutableMetadata(MutableMetadata::tls_deserialize(&mut extension_data)?) + ExtensionType::Metadata => { + Extension::Metadata(Metadata::tls_deserialize(&mut extension_data)?) } ExtensionType::Unknown(unknown) => { Extension::Unknown(unknown, UnknownExtension(extension_data.to_vec())) diff --git a/openmls/src/extensions/mutable_metadata.rs b/openmls/src/extensions/metadata.rs similarity index 87% rename from openmls/src/extensions/mutable_metadata.rs rename to openmls/src/extensions/metadata.rs index be24578682..31dd9e217b 100644 --- a/openmls/src/extensions/mutable_metadata.rs +++ b/openmls/src/extensions/metadata.rs @@ -4,11 +4,11 @@ use tls_codec::{TlsDeserialize, TlsSerialize, TlsSize}; #[derive( PartialEq, Eq, Clone, Debug, Serialize, Deserialize, TlsDeserialize, TlsSerialize, TlsSize, )] -pub struct MutableMetadata { +pub struct Metadata { metadata: Vec, } -impl MutableMetadata { +impl Metadata { pub fn new(metadata: Vec) -> Self { Self { metadata } } diff --git a/openmls/src/extensions/mod.rs b/openmls/src/extensions/mod.rs index 798408e682..594d26b48b 100644 --- a/openmls/src/extensions/mod.rs +++ b/openmls/src/extensions/mod.rs @@ -32,7 +32,7 @@ mod codec; mod external_pub_extension; mod external_sender_extension; mod last_resort; -mod mutable_metadata; +mod metadata; mod protected_metadata; mod ratchet_tree_extension; mod required_capabilities; @@ -57,7 +57,7 @@ use tls_codec::{ pub use protected_metadata::ProtectedMetadata; -use self::mutable_metadata::MutableMetadata; +pub use metadata::Metadata; #[cfg(test)] mod test_extensions; @@ -107,7 +107,7 @@ pub enum ExtensionType { /// extension ProtectedMetadata, - MutableMetadata, + Metadata, /// A currently unknown extension type. Unknown(u16), @@ -161,7 +161,7 @@ impl From for ExtensionType { 5 => ExtensionType::ExternalSenders, 10 => ExtensionType::LastResort, 11 => ExtensionType::ProtectedMetadata, - 0xf001 => ExtensionType::MutableMetadata, + 0xf001 => ExtensionType::Metadata, unknown => ExtensionType::Unknown(unknown), } } @@ -177,7 +177,7 @@ impl From for u16 { ExtensionType::ExternalSenders => 5, ExtensionType::LastResort => 10, ExtensionType::ProtectedMetadata => 11, - ExtensionType::MutableMetadata => 0xf001, + ExtensionType::Metadata => 0xf001, ExtensionType::Unknown(unknown) => unknown, } } @@ -195,7 +195,7 @@ impl ExtensionType { | ExtensionType::ExternalSenders | ExtensionType::LastResort | ExtensionType::ProtectedMetadata - | ExtensionType::MutableMetadata + | ExtensionType::Metadata ) } } @@ -237,7 +237,7 @@ pub enum Extension { /// A [`ProtectedMetadata`] extension ProtectedMetadata(ProtectedMetadata), - MutableMetadata(MutableMetadata), + Metadata(Metadata), /// A currently unknown extension. Unknown(u16, UnknownExtension), @@ -438,6 +438,14 @@ impl Extensions { _ => None, }) } + + pub fn metadata(&self) -> Option<&Metadata> { + self.find_by_type(ExtensionType::Metadata) + .and_then(|e| match e { + Extension::Metadata(e) => Some(e), + _ => None, + }) + } } impl Extension { @@ -528,7 +536,7 @@ impl Extension { Extension::ExternalSenders(_) => ExtensionType::ExternalSenders, Extension::LastResort(_) => ExtensionType::LastResort, Extension::ProtectedMetadata(_) => ExtensionType::ProtectedMetadata, - Extension::MutableMetadata(_) => ExtensionType::MutableMetadata, + Extension::Metadata(_) => ExtensionType::Metadata, Extension::Unknown(kind, _) => ExtensionType::Unknown(*kind), } } diff --git a/openmls/src/extensions/test_extensions.rs b/openmls/src/extensions/test_extensions.rs index dd0f2c62e2..4489da7bca 100644 --- a/openmls/src/extensions/test_extensions.rs +++ b/openmls/src/extensions/test_extensions.rs @@ -369,6 +369,44 @@ fn wrong_extension_with_group_context_extensions( assert_eq!(err, InvalidExtensionError::IllegalInGroupContext); } +fn test_metadata(ciphersuite: Ciphersuite, provider: &impl OpenMlsProvider) { + // Create credentials and keys + //let (alice_credential_with_key, alice_signature_keys) = + let alice_credential_with_key_and_signer = tests::utils::generate_credential_with_key( + b"Alice".into(), + ciphersuite.signature_algorithm(), + provider, + ); + + // example metadata (opaque data -- test hex string is "1cedc0ffee") + let metadata = vec![0x1c, 0xed, 0xc0, 0xff, 0xee]; + let ext = Extension::Metadata(Metadata::new(metadata.clone())); + let extensions = Extensions::from_vec(vec![ext]).expect("could not build extensions struct"); + + let config = MlsGroupConfig::builder() + .group_context_extensions(extensions) + .build(); + + // === Alice creates a group with the ratchet tree extension === + let alice_group = MlsGroup::new( + provider, + &alice_credential_with_key_and_signer.signer, + &config, + alice_credential_with_key_and_signer + .credential_with_key + .clone(), + ) + .expect("failed to build group"); + + let got_metadata = alice_group + .export_group_context() + .extensions() + .metadata() + .expect("failed to read group metadata"); + + assert_eq!(got_metadata.metadata(), &metadata); +} + #[apply(ciphersuites_and_providers)] fn last_resort_extension(ciphersuite: Ciphersuite, provider: &impl OpenMlsProvider) { let last_resort = Extension::LastResort(LastResortExtension::default()); From acfbee2796c3662c6fa76170954fad535deb3222 Mon Sep 17 00:00:00 2001 From: "Jan Winkelmann (keks)" Date: Thu, 11 Jan 2024 10:54:47 +0100 Subject: [PATCH 07/17] add comments --- openmls/src/extensions/metadata.rs | 4 ++++ openmls/src/extensions/mod.rs | 5 +++-- 2 files changed, 7 insertions(+), 2 deletions(-) diff --git a/openmls/src/extensions/metadata.rs b/openmls/src/extensions/metadata.rs index 31dd9e217b..f2e1ab53b8 100644 --- a/openmls/src/extensions/metadata.rs +++ b/openmls/src/extensions/metadata.rs @@ -1,6 +1,8 @@ use super::{Deserialize, Serialize}; use tls_codec::{TlsDeserialize, TlsSerialize, TlsSize}; +/// Metadata is an extension that keeps arbitrary application-specific metadata, in the form of a +/// byte sequence. The application is responsible for specifying a format and parsing the contents. #[derive( PartialEq, Eq, Clone, Debug, Serialize, Deserialize, TlsDeserialize, TlsSerialize, TlsSize, )] @@ -9,10 +11,12 @@ pub struct Metadata { } impl Metadata { + /// Create a new [`Metadata`] extension. pub fn new(metadata: Vec) -> Self { Self { metadata } } + /// Get the metadata bytes. pub fn metadata(&self) -> &Vec { &self.metadata } diff --git a/openmls/src/extensions/mod.rs b/openmls/src/extensions/mod.rs index 594d26b48b..e6e8493ca1 100644 --- a/openmls/src/extensions/mod.rs +++ b/openmls/src/extensions/mod.rs @@ -55,9 +55,8 @@ use tls_codec::{ Size, TlsSize, }; -pub use protected_metadata::ProtectedMetadata; - pub use metadata::Metadata; +pub use protected_metadata::ProtectedMetadata; #[cfg(test)] mod test_extensions; @@ -107,6 +106,7 @@ pub enum ExtensionType { /// extension ProtectedMetadata, + /// Metadata extension for policies and other metadata. GroupContext Extension. Metadata, /// A currently unknown extension type. @@ -237,6 +237,7 @@ pub enum Extension { /// A [`ProtectedMetadata`] extension ProtectedMetadata(ProtectedMetadata), + // A [`Metadata`] extension Metadata(Metadata), /// A currently unknown extension. From 09f1832a75243948a98adbb353354589ee3e2b94 Mon Sep 17 00:00:00 2001 From: "Jan Winkelmann (keks)" Date: Thu, 11 Jan 2024 10:58:58 +0100 Subject: [PATCH 08/17] change ProtectedMetadata extension id to 0xf002 --- openmls/src/extensions/mod.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/openmls/src/extensions/mod.rs b/openmls/src/extensions/mod.rs index e6e8493ca1..4087c44385 100644 --- a/openmls/src/extensions/mod.rs +++ b/openmls/src/extensions/mod.rs @@ -160,8 +160,8 @@ impl From for ExtensionType { 4 => ExtensionType::ExternalPub, 5 => ExtensionType::ExternalSenders, 10 => ExtensionType::LastResort, - 11 => ExtensionType::ProtectedMetadata, 0xf001 => ExtensionType::Metadata, + 0xf002 => ExtensionType::ProtectedMetadata, unknown => ExtensionType::Unknown(unknown), } } @@ -176,8 +176,8 @@ impl From for u16 { ExtensionType::ExternalPub => 4, ExtensionType::ExternalSenders => 5, ExtensionType::LastResort => 10, - ExtensionType::ProtectedMetadata => 11, ExtensionType::Metadata => 0xf001, + ExtensionType::ProtectedMetadata => 0xf002, ExtensionType::Unknown(unknown) => unknown, } } From f94a4c276edd088887bdd6733efc1c07b1da5675 Mon Sep 17 00:00:00 2001 From: "Jan Winkelmann (keks)" Date: Wed, 17 Jan 2024 17:06:38 +0100 Subject: [PATCH 09/17] update test that assumed 0xf000 was an unknown extension --- openmls/src/extensions/mod.rs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/openmls/src/extensions/mod.rs b/openmls/src/extensions/mod.rs index 4087c44385..ad74b54faf 100644 --- a/openmls/src/extensions/mod.rs +++ b/openmls/src/extensions/mod.rs @@ -160,8 +160,8 @@ impl From for ExtensionType { 4 => ExtensionType::ExternalPub, 5 => ExtensionType::ExternalSenders, 10 => ExtensionType::LastResort, + 0xf000 => ExtensionType::ProtectedMetadata, 0xf001 => ExtensionType::Metadata, - 0xf002 => ExtensionType::ProtectedMetadata, unknown => ExtensionType::Unknown(unknown), } } @@ -176,8 +176,8 @@ impl From for u16 { ExtensionType::ExternalPub => 4, ExtensionType::ExternalSenders => 5, ExtensionType::LastResort => 10, + ExtensionType::ProtectedMetadata => 0xf000, ExtensionType::Metadata => 0xf001, - ExtensionType::ProtectedMetadata => 0xf002, ExtensionType::Unknown(unknown) => unknown, } } @@ -642,7 +642,7 @@ mod test { #[test] fn that_unknown_extensions_are_de_serialized_correctly() { - let extension_types = [0x0000u16, 0x0A0A, 0x7A7A, 0xF000, 0xFFFF]; + let extension_types = [0x0000u16, 0x0A0A, 0x7A7A, 0xF100, 0xFFFF]; let extension_datas = [vec![], vec![0], vec![1, 2, 3]]; for extension_type in extension_types.into_iter() { From ed849bdf1d013d6bdd5a8ed14ba5cb81772db1f6 Mon Sep 17 00:00:00 2001 From: Franziskus Kiefer Date: Fri, 19 Jan 2024 09:44:50 +0100 Subject: [PATCH 10/17] fixup after merge from upstream main --- openmls/src/extensions/test_extensions.rs | 77 ++++++++++++----------- 1 file changed, 39 insertions(+), 38 deletions(-) diff --git a/openmls/src/extensions/test_extensions.rs b/openmls/src/extensions/test_extensions.rs index 4489da7bca..0488d88e1d 100644 --- a/openmls/src/extensions/test_extensions.rs +++ b/openmls/src/extensions/test_extensions.rs @@ -251,6 +251,45 @@ fn required_capabilities() { assert_eq!(extension_bytes, encoded); } +#[apply(ciphersuites_and_providers)] +fn test_metadata(ciphersuite: Ciphersuite, provider: &impl OpenMlsProvider) { + // Create credentials and keys + let alice_credential_with_key_and_signer = tests::utils::generate_credential_with_key( + b"Alice".into(), + ciphersuite.signature_algorithm(), + provider, + ); + + // example metadata (opaque data -- test hex string is "1cedc0ffee") + let metadata = vec![0x1c, 0xed, 0xc0, 0xff, 0xee]; + let ext = Extension::Metadata(Metadata::new(metadata.clone())); + let extensions = Extensions::from_vec(vec![ext]).expect("could not build extensions struct"); + + let config = MlsGroupCreateConfig::builder() + .with_group_context_extensions(extensions) + .unwrap() + .build(); + + // === Alice creates a group with the ratchet tree extension === + let alice_group = MlsGroup::new( + provider, + &alice_credential_with_key_and_signer.signer, + &config, + alice_credential_with_key_and_signer + .credential_with_key + .clone(), + ) + .expect("failed to build group"); + + let got_metadata = alice_group + .export_group_context() + .extensions() + .metadata() + .expect("failed to read group metadata"); + + assert_eq!(got_metadata.metadata(), &metadata); +} + #[apply(ciphersuites_and_providers)] fn with_group_context_extensions(ciphersuite: Ciphersuite, provider: &impl OpenMlsProvider) { // create an extension that we can check for later @@ -369,44 +408,6 @@ fn wrong_extension_with_group_context_extensions( assert_eq!(err, InvalidExtensionError::IllegalInGroupContext); } -fn test_metadata(ciphersuite: Ciphersuite, provider: &impl OpenMlsProvider) { - // Create credentials and keys - //let (alice_credential_with_key, alice_signature_keys) = - let alice_credential_with_key_and_signer = tests::utils::generate_credential_with_key( - b"Alice".into(), - ciphersuite.signature_algorithm(), - provider, - ); - - // example metadata (opaque data -- test hex string is "1cedc0ffee") - let metadata = vec![0x1c, 0xed, 0xc0, 0xff, 0xee]; - let ext = Extension::Metadata(Metadata::new(metadata.clone())); - let extensions = Extensions::from_vec(vec![ext]).expect("could not build extensions struct"); - - let config = MlsGroupConfig::builder() - .group_context_extensions(extensions) - .build(); - - // === Alice creates a group with the ratchet tree extension === - let alice_group = MlsGroup::new( - provider, - &alice_credential_with_key_and_signer.signer, - &config, - alice_credential_with_key_and_signer - .credential_with_key - .clone(), - ) - .expect("failed to build group"); - - let got_metadata = alice_group - .export_group_context() - .extensions() - .metadata() - .expect("failed to read group metadata"); - - assert_eq!(got_metadata.metadata(), &metadata); -} - #[apply(ciphersuites_and_providers)] fn last_resort_extension(ciphersuite: Ciphersuite, provider: &impl OpenMlsProvider) { let last_resort = Extension::LastResort(LastResortExtension::default()); From b184e1a902119cdef6b684431d543d55d4029fc9 Mon Sep 17 00:00:00 2001 From: Franziskus Kiefer Date: Fri, 19 Jan 2024 09:52:03 +0100 Subject: [PATCH 11/17] rename to immutable metadata --- openmls/src/extensions/codec.rs | 14 +- openmls/src/extensions/mod.rs | 38 ++- openmls/src/extensions/protected_metadata.rs | 246 ------------------- openmls/src/group/errors.rs | 3 + openmls/src/group/public_group/validation.rs | 16 ++ 5 files changed, 43 insertions(+), 274 deletions(-) delete mode 100644 openmls/src/extensions/protected_metadata.rs diff --git a/openmls/src/extensions/codec.rs b/openmls/src/extensions/codec.rs index a2962eab31..1ba2ad498c 100644 --- a/openmls/src/extensions/codec.rs +++ b/openmls/src/extensions/codec.rs @@ -8,9 +8,7 @@ use crate::extensions::{ UnknownExtension, }; -use super::{ - last_resort::LastResortExtension, metadata::Metadata, protected_metadata::ProtectedMetadata, -}; +use super::{last_resort::LastResortExtension, metadata::Metadata}; fn vlbytes_len_len(length: usize) -> usize { if length < 0x40 { @@ -39,7 +37,7 @@ impl Size for Extension { Extension::ExternalPub(e) => e.tls_serialized_len(), Extension::ExternalSenders(e) => e.tls_serialized_len(), Extension::LastResort(e) => e.tls_serialized_len(), - Extension::ProtectedMetadata(e) => e.tls_serialized_len(), + Extension::ImmutableMetadata(e) => e.tls_serialized_len(), Extension::Metadata(e) => e.tls_serialized_len(), Extension::Unknown(_, e) => e.0.len(), }; @@ -73,7 +71,7 @@ impl Serialize for Extension { Extension::ExternalPub(e) => e.tls_serialize(&mut extension_data), Extension::ExternalSenders(e) => e.tls_serialize(&mut extension_data), Extension::LastResort(e) => e.tls_serialize(&mut extension_data), - Extension::ProtectedMetadata(e) => e.tls_serialize(&mut extension_data), + Extension::ImmutableMetadata(e) => e.tls_serialize(&mut extension_data), Extension::Metadata(e) => e.tls_serialize(&mut extension_data), Extension::Unknown(_, e) => extension_data .write_all(e.0.as_slice()) @@ -124,9 +122,9 @@ impl Deserialize for Extension { ExtensionType::LastResort => { Extension::LastResort(LastResortExtension::tls_deserialize(&mut extension_data)?) } - ExtensionType::ProtectedMetadata => Extension::ProtectedMetadata( - ProtectedMetadata::tls_deserialize(&mut extension_data)?, - ), + ExtensionType::ImmutableMetadata => { + Extension::ImmutableMetadata(Metadata::tls_deserialize(&mut extension_data)?) + } ExtensionType::Metadata => { Extension::Metadata(Metadata::tls_deserialize(&mut extension_data)?) } diff --git a/openmls/src/extensions/mod.rs b/openmls/src/extensions/mod.rs index ad74b54faf..1263c148c9 100644 --- a/openmls/src/extensions/mod.rs +++ b/openmls/src/extensions/mod.rs @@ -33,7 +33,6 @@ mod external_pub_extension; mod external_sender_extension; mod last_resort; mod metadata; -mod protected_metadata; mod ratchet_tree_extension; mod required_capabilities; use errors::*; @@ -56,7 +55,6 @@ use tls_codec::{ }; pub use metadata::Metadata; -pub use protected_metadata::ProtectedMetadata; #[cfg(test)] mod test_extensions; @@ -102,9 +100,9 @@ pub enum ExtensionType { /// scenario. LastResort, - /// Protected metadata extension for policies of the group. GroupContext - /// extension - ProtectedMetadata, + /// Immutable metadata extension for the GroupContext. + /// This can only be set on creation of the group. + ImmutableMetadata, /// Metadata extension for policies and other metadata. GroupContext Extension. Metadata, @@ -160,7 +158,7 @@ impl From for ExtensionType { 4 => ExtensionType::ExternalPub, 5 => ExtensionType::ExternalSenders, 10 => ExtensionType::LastResort, - 0xf000 => ExtensionType::ProtectedMetadata, + 0xf000 => ExtensionType::ImmutableMetadata, 0xf001 => ExtensionType::Metadata, unknown => ExtensionType::Unknown(unknown), } @@ -176,7 +174,7 @@ impl From for u16 { ExtensionType::ExternalPub => 4, ExtensionType::ExternalSenders => 5, ExtensionType::LastResort => 10, - ExtensionType::ProtectedMetadata => 0xf000, + ExtensionType::ImmutableMetadata => 0xf000, ExtensionType::Metadata => 0xf001, ExtensionType::Unknown(unknown) => unknown, } @@ -194,7 +192,7 @@ impl ExtensionType { | ExtensionType::ExternalPub | ExtensionType::ExternalSenders | ExtensionType::LastResort - | ExtensionType::ProtectedMetadata + | ExtensionType::ImmutableMetadata | ExtensionType::Metadata ) } @@ -234,8 +232,8 @@ pub enum Extension { /// A [`LastResortExtension`] LastResort(LastResortExtension), - /// A [`ProtectedMetadata`] extension - ProtectedMetadata(ProtectedMetadata), + /// An [`ImmutableMetadata`] extension + ImmutableMetadata(Metadata), // A [`Metadata`] extension Metadata(Metadata), @@ -431,11 +429,11 @@ impl Extensions { }) } - /// Get a reference to the [`ProtectedMetadata`] if there is any. - pub fn protected_metadata(&self) -> Option<&ProtectedMetadata> { - self.find_by_type(ExtensionType::ProtectedMetadata) + /// Get a reference to the immutable [`Metadata`] if there is any. + pub fn immutable_metadata(&self) -> Option<&Metadata> { + self.find_by_type(ExtensionType::ImmutableMetadata) .and_then(|e| match e { - Extension::ProtectedMetadata(e) => Some(e), + Extension::ImmutableMetadata(e) => Some(e), _ => None, }) } @@ -514,14 +512,14 @@ impl Extension { } } - /// Get a reference to this extension as [`ProtectedMetadata`]. + /// Get a reference to this extension as immutable [`Metadata`]. /// Returns an [`ExtensionError::InvalidExtensionType`] error if called on - /// an [`Extension`] that's not a [`ProtectedMetadata`] extension. - pub fn as_protected_metadata_extension(&self) -> Result<&ProtectedMetadata, ExtensionError> { + /// an [`Extension`] that's not an immutable [`Metadata`] extension. + pub fn as_protected_metadata_extension(&self) -> Result<&Metadata, ExtensionError> { match self { - Self::ProtectedMetadata(e) => Ok(e), + Self::ImmutableMetadata(e) => Ok(e), _ => Err(ExtensionError::InvalidExtensionType( - "This is not an ProtectedMetadata".into(), + "This is not an immutable metadata extensions".into(), )), } } @@ -536,7 +534,7 @@ impl Extension { Extension::ExternalPub(_) => ExtensionType::ExternalPub, Extension::ExternalSenders(_) => ExtensionType::ExternalSenders, Extension::LastResort(_) => ExtensionType::LastResort, - Extension::ProtectedMetadata(_) => ExtensionType::ProtectedMetadata, + Extension::ImmutableMetadata(_) => ExtensionType::ImmutableMetadata, Extension::Metadata(_) => ExtensionType::Metadata, Extension::Unknown(kind, _) => ExtensionType::Unknown(*kind), } diff --git a/openmls/src/extensions/protected_metadata.rs b/openmls/src/extensions/protected_metadata.rs deleted file mode 100644 index 593367a36b..0000000000 --- a/openmls/src/extensions/protected_metadata.rs +++ /dev/null @@ -1,246 +0,0 @@ -use std::time::{SystemTime, UNIX_EPOCH}; - -use openmls_traits::signatures::Signer; -use tls_codec::{Serialize as TlsSerializeTrait, TlsDeserialize, TlsSerialize, TlsSize}; - -use super::{Deserialize, Serialize}; -use crate::{ - ciphersuite::{ - signable::{Signable, SignatureError, SignedStruct, Verifiable, VerifiedStruct}, - signature::Signature, - }, - credentials::Credential, -}; - -/// # Protected Metadata -/// -/// ```c -/// struct { -/// opaque signer_application_id; -/// Credential signer_credential; -/// SignaturePublicKey signature_key; -/// uint64 signing_time; -/// opaque metadata; -/// /* SignWithLabel(., "ProtectedMetadataTBS",ProtectedMetadata) */ -/// opaque signature; -/// } ProtectedMetadata; -/// ``` -/// -/// This extension must be verified by the application every time it is set or -/// changed. -/// The application **MUST** verify that -/// * the signature is valid -/// * the credential has been valid at `signing_time` -/// * the `signer_application_id` is equal to the `creator_application_id`. -/// -/// FIXME: This should NOT be deserializable. But we need to change more code for -/// that to be possible. -#[derive( - PartialEq, Eq, Clone, Debug, Serialize, Deserialize, TlsDeserialize, TlsSerialize, TlsSize, -)] -pub struct ProtectedMetadata { - payload: ProtectedMetadataTbs, - signature: Signature, -} - -impl ProtectedMetadata { - /// Create a new protected metadata extension and sign it. - pub fn new( - signer: &impl Signer, - signer_application_id: Vec, - signer_credential: Credential, - signature_key: Vec, - metadata: Vec, - ) -> Result { - let tbs = ProtectedMetadataTbs::new( - signer_application_id, - signer_credential, - signature_key, - metadata, - ); - tbs.sign(signer) - } - - /// Get the signer application ID as slice. - pub fn signer_application_id(&self) -> &[u8] { - self.payload.signer_application_id.as_ref() - } - - /// Get the signer [`Credential`]. - pub fn signer_credential(&self) -> &Credential { - &self.payload.signer_credential - } - - /// Get the signature key as slize. - pub fn signature_key(&self) -> &[u8] { - self.payload.signature_key.as_ref() - } - - /// Get the signing time as UNIX timestamp. - pub fn signing_time(&self) -> u64 { - self.payload.signing_time - } - - /// Get the serialized metadata as slice. - /// - /// This is opaque to OpenMLS. The caller must handle it appropriately. - pub fn metadata(&self) -> &[u8] { - self.payload.metadata.as_ref() - } -} - -impl SignedStruct for ProtectedMetadata { - fn from_payload(payload: ProtectedMetadataTbs, signature: Signature) -> Self { - ProtectedMetadata { payload, signature } - } -} - -/// # Protected Metadata -/// -/// ```c -/// /* SignWithLabel(., "ProtectedMetadataTBS",ProtectedMetadata) */ -/// struct { -/// opaque signer_application_id; -/// Credential signer_credential; -/// SignaturePublicKey signature_key; -/// uint64 signing_time; -/// opaque metadata; -/// } ProtectedMetadataTBS; -/// ``` -#[derive( - PartialEq, Eq, Clone, Debug, Serialize, Deserialize, TlsSerialize, TlsDeserialize, TlsSize, -)] -pub struct ProtectedMetadataTbs { - signer_application_id: Vec, - signer_credential: Credential, - signature_key: Vec, - signing_time: u64, - metadata: Vec, -} - -impl ProtectedMetadataTbs { - /// Create a protected metadata extension tbs. - fn new( - signer_application_id: Vec, - signer_credential: Credential, - signature_key: Vec, - metadata: impl Into>, - ) -> Self { - Self { - signer_application_id, - signer_credential, - signature_key, - signing_time: SystemTime::now() - .duration_since(UNIX_EPOCH) - .expect("SystemTime before UNIX EPOCH!") - .as_secs(), - metadata: metadata.into(), - } - } -} - -const SIGNATURE_LABEL: &str = "ProtectedMetadataTbs"; - -impl Signable for ProtectedMetadataTbs { - type SignedOutput = ProtectedMetadata; - - fn unsigned_payload(&self) -> Result, tls_codec::Error> { - self.tls_serialize_detached() - } - - fn label(&self) -> &str { - SIGNATURE_LABEL - } -} - -/// XXX: This really should not be implemented on [`ProtectedMetadata`] but on -/// the verifiable version. -mod verifiable { - use openmls_traits::crypto::OpenMlsCrypto; - - use crate::prelude::OpenMlsSignaturePublicKey; - - use super::*; - - impl Verifiable for ProtectedMetadata { - type VerifiedStruct = ProtectedMetadata; - - fn unsigned_payload(&self) -> Result, tls_codec::Error> { - self.payload.tls_serialize_detached() - } - - fn signature(&self) -> &Signature { - &self.signature - } - - fn label(&self) -> &str { - SIGNATURE_LABEL - } - - fn verify( - self, - _crypto: &impl OpenMlsCrypto, - _pk: &OpenMlsSignaturePublicKey, - ) -> Result { - Ok(self) - } - } - - impl VerifiedStruct for ProtectedMetadata {} - - mod private_mod { - #[derive(Default)] - pub struct Seal; - } -} - -#[cfg(test)] -mod tests { - use tls_codec::{Deserialize, Serialize}; - - use crate::{ - credentials::test_utils::new_credential, extensions::protected_metadata::ProtectedMetadata, - prelude_test::OpenMlsSignaturePublicKey, test_utils::*, - }; - - use super::*; - - #[apply(ciphersuites_and_providers)] - fn serialize_extension(ciphersuite: Ciphersuite, provider: &impl OpenMlsProvider) { - let creator_application_id = b"MetadataTestAppId".to_vec(); - - // Create metadata - let metadata = vec![1, 2, 3]; - - // Setup crypto - let (credential_with_key, signer) = new_credential( - provider, - b"Kreator", - crate::credentials::CredentialType::Basic, - ciphersuite.signature_algorithm(), - ); - let signature_key = - OpenMlsSignaturePublicKey::new(signer.public().into(), ciphersuite.into()).unwrap(); - - let signer_application_id = creator_application_id.clone(); - let extension = ProtectedMetadata::new( - &signer, - signer_application_id, - credential_with_key.credential.clone(), - signature_key.as_slice().to_vec(), - metadata.clone(), - ) - .unwrap(); - - // serialize and deserialize + verify - let serialized = extension.tls_serialize_detached().unwrap(); - let unverified = ProtectedMetadata::tls_deserialize_exact(serialized).unwrap(); - let deserialized: ProtectedMetadata = unverified - .verify(provider.crypto(), &signature_key) - .unwrap(); - assert_eq!(deserialized, extension); - - let xmtp_metadata = deserialized.metadata(); - assert_eq!(xmtp_metadata, metadata); - } -} diff --git a/openmls/src/group/errors.rs b/openmls/src/group/errors.rs index fbc1296710..fb31308e9a 100644 --- a/openmls/src/group/errors.rs +++ b/openmls/src/group/errors.rs @@ -550,4 +550,7 @@ pub enum GroupContextExtensionsProposalValidationError { "The new extensions set contails extensions that are not supported by all group members." )] ExtensionNotSupportedByAllMembers, + /// Proposal changes the immutable metadata extension, which is not allowed. + #[error("Proposal changes the immutable metadata extension, which is not allowed.")] + ChangedImmutableMetadata, } diff --git a/openmls/src/group/public_group/validation.rs b/openmls/src/group/public_group/validation.rs index 81ab909084..8893e3901e 100644 --- a/openmls/src/group/public_group/validation.rs +++ b/openmls/src/group/public_group/validation.rs @@ -525,6 +525,22 @@ impl PublicGroup { match iter.next() { Some(queued_proposal) => match queued_proposal.proposal() { Proposal::GroupContextExtensions(extensions) => { + // Ensure that the [`ImmutableMetadata`] extension did not change. + if let Some(new_immutable_metadata) = + extensions.extensions().immutable_metadata() + { + // Get existing extension. + if let Some(old_immutable_metadata) = + self.group_context.extensions().immutable_metadata() + { + // If the extension changed, or there hasn't been one before, + // this proposal is invalid. + if new_immutable_metadata != old_immutable_metadata { + return Err(GroupContextExtensionsProposalValidationError::ChangedImmutableMetadata); + } + } + } + let ext_type_list: Vec<_> = extensions .extensions() .iter() From f6503ce4139c2b8b8868bdc3997191268d0b0e78 Mon Sep 17 00:00:00 2001 From: Franziskus Kiefer Date: Mon, 22 Jan 2024 15:57:39 +0100 Subject: [PATCH 12/17] reject if there was no immutable metadata before --- openmls/src/group/public_group/validation.rs | 2 ++ 1 file changed, 2 insertions(+) diff --git a/openmls/src/group/public_group/validation.rs b/openmls/src/group/public_group/validation.rs index 8893e3901e..107a9215ea 100644 --- a/openmls/src/group/public_group/validation.rs +++ b/openmls/src/group/public_group/validation.rs @@ -538,6 +538,8 @@ impl PublicGroup { if new_immutable_metadata != old_immutable_metadata { return Err(GroupContextExtensionsProposalValidationError::ChangedImmutableMetadata); } + } else { + return Err(GroupContextExtensionsProposalValidationError::ChangedImmutableMetadata); } } From d7e059a2ee62458868522937dede7522ec2a0ed5 Mon Sep 17 00:00:00 2001 From: "Jan Winkelmann (keks)" Date: Wed, 24 Jan 2024 16:06:57 +0100 Subject: [PATCH 13/17] simplify validation --- openmls/src/group/public_group/validation.rs | 32 ++++++++------------ 1 file changed, 12 insertions(+), 20 deletions(-) diff --git a/openmls/src/group/public_group/validation.rs b/openmls/src/group/public_group/validation.rs index 107a9215ea..d07f2c4c88 100644 --- a/openmls/src/group/public_group/validation.rs +++ b/openmls/src/group/public_group/validation.rs @@ -522,25 +522,17 @@ impl PublicGroup { ) -> Result<(), GroupContextExtensionsProposalValidationError> { let mut iter = proposal_queue.filtered_by_type(ProposalType::GroupContextExtensions); - match iter.next() { - Some(queued_proposal) => match queued_proposal.proposal() { + if let Some(queued_proposal) = iter.next() { + match queued_proposal.proposal() { Proposal::GroupContextExtensions(extensions) => { - // Ensure that the [`ImmutableMetadata`] extension did not change. - if let Some(new_immutable_metadata) = - extensions.extensions().immutable_metadata() - { - // Get existing extension. - if let Some(old_immutable_metadata) = - self.group_context.extensions().immutable_metadata() - { - // If the extension changed, or there hasn't been one before, - // this proposal is invalid. - if new_immutable_metadata != old_immutable_metadata { - return Err(GroupContextExtensionsProposalValidationError::ChangedImmutableMetadata); - } - } else { - return Err(GroupContextExtensionsProposalValidationError::ChangedImmutableMetadata); - } + // Check that immutable metadata didn't change and wasn't added or removed. + let new_immutable_metadata = extensions.extensions().immutable_metadata(); + let current_immutable_metadata = + self.group_context.extensions().immutable_metadata(); + if new_immutable_metadata != current_immutable_metadata { + return Err( + GroupContextExtensionsProposalValidationError::ChangedImmutableMetadata, + ); } let ext_type_list: Vec<_> = extensions @@ -558,10 +550,10 @@ impl PublicGroup { ), )) } - }, - None => return Ok(()), + } } + // make sure that there is at most one proposal of this type match iter.next() { Some(_) => Err(GroupContextExtensionsProposalValidationError::TooManyGCEProposals), None => Ok(()), From cd91f6c6fd36beb9709f18ad0cd3b1fa1467a32a Mon Sep 17 00:00:00 2001 From: "Jan Winkelmann (keks)" Date: Thu, 25 Jan 2024 08:48:10 +0100 Subject: [PATCH 14/17] add test --- openmls/src/group/mls_group/test_mls_group.rs | 127 +++++++++++++++++- 1 file changed, 126 insertions(+), 1 deletion(-) diff --git a/openmls/src/group/mls_group/test_mls_group.rs b/openmls/src/group/mls_group/test_mls_group.rs index 55658d3ce6..ffcff7cb01 100644 --- a/openmls/src/group/mls_group/test_mls_group.rs +++ b/openmls/src/group/mls_group/test_mls_group.rs @@ -1004,7 +1004,132 @@ fn remove_prosposal_by_ref(ciphersuite: Ciphersuite, provider: &impl OpenMlsProv _ => unreachable!("Expected a StagedCommit."), } } -// + +// Test that the builder pattern accurately configures the new group. +#[apply(ciphersuites_and_providers)] +fn immutable_metadata(ciphersuite: Ciphersuite, provider: &impl OpenMlsProvider) { + let (alice_credential_with_key, _alice_kpb, alice_signer, _alice_pk) = + setup_client("Alice", ciphersuite, provider); + + let metadata = Metadata::new(b"this is a test group".to_vec()); + + let extensions_with_metadata = + Extensions::single(Extension::ImmutableMetadata(metadata.clone())); + + // === Create a Group with Metadata === + let var_name = Capabilities::new( + None, + None, + Some(&[ExtensionType::ImmutableMetadata]), + None, + None, + ); + let mut group_with_metadata = MlsGroup::builder() + .with_group_context_extensions(extensions_with_metadata.clone()) + .expect("error when setting initial metadata group extension") + .with_capabilities(var_name) + .build(provider, &alice_signer, alice_credential_with_key.clone()) + .expect("error creating group using builder"); + + let got_metadata = group_with_metadata + .group() + .group_context_extensions() + .immutable_metadata() + .expect("error getting metadata from group"); + + // check we get back the same metadata we initially set + assert_eq!(got_metadata, &metadata); + + // changing the metadata should fail + let new_metadata = Metadata::new(b"this is a not test group".to_vec()); + + let new_extensions_with_metadata = + Extensions::single(Extension::ImmutableMetadata(new_metadata.clone())); + + group_with_metadata + .propose_group_context_extensions(provider, new_extensions_with_metadata, &alice_signer) + .expect("error proposing GCE proposal with same metadata"); + + assert_eq!(group_with_metadata.pending_proposals().count(), 1); + + group_with_metadata + .commit_to_pending_proposals(provider, &alice_signer) + .expect_err("should not have been able to commit to proposal that changes metadata"); + + group_with_metadata.clear_pending_proposals(); + + assert_eq!(group_with_metadata.pending_proposals().count(), 0); + + // using the same metadata should succeed + group_with_metadata + .propose_group_context_extensions(provider, extensions_with_metadata.clone(), &alice_signer) + .expect("error proposing GCE proposal with same metadata"); + + assert_eq!(group_with_metadata.pending_proposals().count(), 1); + + group_with_metadata + .commit_to_pending_proposals(provider, &alice_signer) + .expect("failed to commit to pending proposals"); + + group_with_metadata + .merge_pending_commit(provider) + .expect("error merging pending commit"); + + let got_metadata = group_with_metadata + .group() + .group_context_extensions() + .immutable_metadata() + .expect("couldn't get immutable_metadata"); + + // check we get back the same metadata we initially set + assert_eq!(got_metadata, &metadata); + + // === Create a Group without Metadata === + let mut group_without_metadata = MlsGroup::builder() + .build(provider, &alice_signer, alice_credential_with_key) + .expect("error creating group using builder"); + + assert!(group_without_metadata + .group() + .group_context_extensions() + .immutable_metadata() + .is_none()); + + // changing the metadata should fail + let new_metadata = Metadata::new(b"this is a new metadata".to_vec()); + + let new_extensions_with_metadata = + Extensions::single(Extension::ImmutableMetadata(new_metadata.clone())); + + group_without_metadata + .propose_group_context_extensions(provider, new_extensions_with_metadata, &alice_signer) + .expect("error proposing GCE proposal with metadata"); + + // since the GCEs are the same as befroe, the proposal handling logic "deduplicates" the proposal + // (with the implicit "identity proposal" I guess), so there isn't actually a proposal here + assert_eq!(group_with_metadata.pending_proposals().count(), 0); + + // using the same metadata should succeed + group_without_metadata + .propose_group_context_extensions(provider, Extensions::empty(), &alice_signer) + .expect("error proposing GCE proposal with no metadata"); + + // since the GCEs are empty, the proposal handling logic "deduplicates" the proposal + // (with the implicit "identity proposal" I guess), so there isn't actually a proposal here + assert_eq!(group_with_metadata.pending_proposals().count(), 0); + + // check we still get no metadata + assert!(group_without_metadata + .group() + .group_context_extensions() + .immutable_metadata() + .is_none()); + + // TODO: we need to test that processing an invalid commit also fails. + // however, we can't generate this commit, because our functions for + // constructing commits does not permit it. See #1476 +} + // Test that the builder pattern accurately configures the new group. #[apply(ciphersuites_and_providers)] fn group_context_extensions_proposal(ciphersuite: Ciphersuite, provider: &impl OpenMlsProvider) { From 305b7610ad600446919a15f68d4225fdf610ab37 Mon Sep 17 00:00:00 2001 From: "Jan Winkelmann (keks)" Date: Thu, 25 Jan 2024 09:20:01 +0100 Subject: [PATCH 15/17] address feedback --- openmls/src/extensions/mod.rs | 3 ++- openmls/src/group/mls_group/test_mls_group.rs | 4 ++-- 2 files changed, 4 insertions(+), 3 deletions(-) diff --git a/openmls/src/extensions/mod.rs b/openmls/src/extensions/mod.rs index 1263c148c9..292142001b 100644 --- a/openmls/src/extensions/mod.rs +++ b/openmls/src/extensions/mod.rs @@ -438,6 +438,7 @@ impl Extensions { }) } + /// Get a reference to the mutable [`Metadata`] if there is any. pub fn metadata(&self) -> Option<&Metadata> { self.find_by_type(ExtensionType::Metadata) .and_then(|e| match e { @@ -515,7 +516,7 @@ impl Extension { /// Get a reference to this extension as immutable [`Metadata`]. /// Returns an [`ExtensionError::InvalidExtensionType`] error if called on /// an [`Extension`] that's not an immutable [`Metadata`] extension. - pub fn as_protected_metadata_extension(&self) -> Result<&Metadata, ExtensionError> { + pub fn as_immutable_metadata_extension(&self) -> Result<&Metadata, ExtensionError> { match self { Self::ImmutableMetadata(e) => Ok(e), _ => Err(ExtensionError::InvalidExtensionType( diff --git a/openmls/src/group/mls_group/test_mls_group.rs b/openmls/src/group/mls_group/test_mls_group.rs index ffcff7cb01..23eb00c826 100644 --- a/openmls/src/group/mls_group/test_mls_group.rs +++ b/openmls/src/group/mls_group/test_mls_group.rs @@ -1017,7 +1017,7 @@ fn immutable_metadata(ciphersuite: Ciphersuite, provider: &impl OpenMlsProvider) Extensions::single(Extension::ImmutableMetadata(metadata.clone())); // === Create a Group with Metadata === - let var_name = Capabilities::new( + let capabilities = Capabilities::new( None, None, Some(&[ExtensionType::ImmutableMetadata]), @@ -1027,7 +1027,7 @@ fn immutable_metadata(ciphersuite: Ciphersuite, provider: &impl OpenMlsProvider) let mut group_with_metadata = MlsGroup::builder() .with_group_context_extensions(extensions_with_metadata.clone()) .expect("error when setting initial metadata group extension") - .with_capabilities(var_name) + .with_capabilities(capabilities) .build(provider, &alice_signer, alice_credential_with_key.clone()) .expect("error creating group using builder"); From 17989cf93b19b97b66110d74b05e4f95cf1e2ce5 Mon Sep 17 00:00:00 2001 From: "Jan Winkelmann (keks)" Date: Mon, 29 Jan 2024 10:37:58 +0100 Subject: [PATCH 16/17] remove Metadata extension in favour of using Unknown extensions --- openmls/src/extensions/codec.rs | 5 ----- openmls/src/extensions/mod.rs | 19 ------------------- openmls/src/extensions/test_extensions.rs | 10 +++++++--- 3 files changed, 7 insertions(+), 27 deletions(-) diff --git a/openmls/src/extensions/codec.rs b/openmls/src/extensions/codec.rs index 1ba2ad498c..00408aa994 100644 --- a/openmls/src/extensions/codec.rs +++ b/openmls/src/extensions/codec.rs @@ -38,7 +38,6 @@ impl Size for Extension { Extension::ExternalSenders(e) => e.tls_serialized_len(), Extension::LastResort(e) => e.tls_serialized_len(), Extension::ImmutableMetadata(e) => e.tls_serialized_len(), - Extension::Metadata(e) => e.tls_serialized_len(), Extension::Unknown(_, e) => e.0.len(), }; @@ -72,7 +71,6 @@ impl Serialize for Extension { Extension::ExternalSenders(e) => e.tls_serialize(&mut extension_data), Extension::LastResort(e) => e.tls_serialize(&mut extension_data), Extension::ImmutableMetadata(e) => e.tls_serialize(&mut extension_data), - Extension::Metadata(e) => e.tls_serialize(&mut extension_data), Extension::Unknown(_, e) => extension_data .write_all(e.0.as_slice()) .map(|_| e.0.len()) @@ -125,9 +123,6 @@ impl Deserialize for Extension { ExtensionType::ImmutableMetadata => { Extension::ImmutableMetadata(Metadata::tls_deserialize(&mut extension_data)?) } - ExtensionType::Metadata => { - Extension::Metadata(Metadata::tls_deserialize(&mut extension_data)?) - } ExtensionType::Unknown(unknown) => { Extension::Unknown(unknown, UnknownExtension(extension_data.to_vec())) } diff --git a/openmls/src/extensions/mod.rs b/openmls/src/extensions/mod.rs index 292142001b..6bdd31f952 100644 --- a/openmls/src/extensions/mod.rs +++ b/openmls/src/extensions/mod.rs @@ -104,9 +104,6 @@ pub enum ExtensionType { /// This can only be set on creation of the group. ImmutableMetadata, - /// Metadata extension for policies and other metadata. GroupContext Extension. - Metadata, - /// A currently unknown extension type. Unknown(u16), } @@ -159,7 +156,6 @@ impl From for ExtensionType { 5 => ExtensionType::ExternalSenders, 10 => ExtensionType::LastResort, 0xf000 => ExtensionType::ImmutableMetadata, - 0xf001 => ExtensionType::Metadata, unknown => ExtensionType::Unknown(unknown), } } @@ -175,7 +171,6 @@ impl From for u16 { ExtensionType::ExternalSenders => 5, ExtensionType::LastResort => 10, ExtensionType::ImmutableMetadata => 0xf000, - ExtensionType::Metadata => 0xf001, ExtensionType::Unknown(unknown) => unknown, } } @@ -193,7 +188,6 @@ impl ExtensionType { | ExtensionType::ExternalSenders | ExtensionType::LastResort | ExtensionType::ImmutableMetadata - | ExtensionType::Metadata ) } } @@ -235,9 +229,6 @@ pub enum Extension { /// An [`ImmutableMetadata`] extension ImmutableMetadata(Metadata), - // A [`Metadata`] extension - Metadata(Metadata), - /// A currently unknown extension. Unknown(u16, UnknownExtension), } @@ -437,15 +428,6 @@ impl Extensions { _ => None, }) } - - /// Get a reference to the mutable [`Metadata`] if there is any. - pub fn metadata(&self) -> Option<&Metadata> { - self.find_by_type(ExtensionType::Metadata) - .and_then(|e| match e { - Extension::Metadata(e) => Some(e), - _ => None, - }) - } } impl Extension { @@ -536,7 +518,6 @@ impl Extension { Extension::ExternalSenders(_) => ExtensionType::ExternalSenders, Extension::LastResort(_) => ExtensionType::LastResort, Extension::ImmutableMetadata(_) => ExtensionType::ImmutableMetadata, - Extension::Metadata(_) => ExtensionType::Metadata, Extension::Unknown(kind, _) => ExtensionType::Unknown(*kind), } } diff --git a/openmls/src/extensions/test_extensions.rs b/openmls/src/extensions/test_extensions.rs index 0488d88e1d..50eeeda8d2 100644 --- a/openmls/src/extensions/test_extensions.rs +++ b/openmls/src/extensions/test_extensions.rs @@ -262,7 +262,7 @@ fn test_metadata(ciphersuite: Ciphersuite, provider: &impl OpenMlsProvider) { // example metadata (opaque data -- test hex string is "1cedc0ffee") let metadata = vec![0x1c, 0xed, 0xc0, 0xff, 0xee]; - let ext = Extension::Metadata(Metadata::new(metadata.clone())); + let ext = Extension::Unknown(0xf001, UnknownExtension(metadata.clone())); let extensions = Extensions::from_vec(vec![ext]).expect("could not build extensions struct"); let config = MlsGroupCreateConfig::builder() @@ -284,10 +284,14 @@ fn test_metadata(ciphersuite: Ciphersuite, provider: &impl OpenMlsProvider) { let got_metadata = alice_group .export_group_context() .extensions() - .metadata() + .find_by_type(ExtensionType::Unknown(0xf001)) .expect("failed to read group metadata"); - assert_eq!(got_metadata.metadata(), &metadata); + if let Extension::Unknown(0xf001, UnknownExtension(got_metadata)) = got_metadata { + assert_eq!(got_metadata, &metadata); + } else { + panic!("metadata extension has wrong extension enum variant") + } } #[apply(ciphersuites_and_providers)] From cbf2b71e39da5a94c4eb38d7dbbf4637efb37b02 Mon Sep 17 00:00:00 2001 From: "Jan Winkelmann (keks)" Date: Mon, 29 Jan 2024 10:50:01 +0100 Subject: [PATCH 17/17] fix doc link --- openmls/src/extensions/mod.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/openmls/src/extensions/mod.rs b/openmls/src/extensions/mod.rs index 6bdd31f952..df9d2414dc 100644 --- a/openmls/src/extensions/mod.rs +++ b/openmls/src/extensions/mod.rs @@ -226,7 +226,7 @@ pub enum Extension { /// A [`LastResortExtension`] LastResort(LastResortExtension), - /// An [`ImmutableMetadata`] extension + /// An immutable [`Metadata`] extension ImmutableMetadata(Metadata), /// A currently unknown extension.