fix: ratchet tree leaf node lifetime validation

Do not validate the lifetime in leaf nodes in the ratchet tree
when joining a group.

Author: franziskuskiefer

openmls/src/group/public_group/mod.rs

@@ -278,7 +278,7 @@ impl PublicGroup {
         public_group
             .treesync
             .full_leaves()
-            .try_for_each(|leaf_node| public_group.validate_leaf_node(leaf_node))?;
+            .try_for_each(|leaf_node| public_group.validate_leaf_node_inner(leaf_node, true))?;
 
         Ok((public_group, group_info))
     }

openmls/src/group/public_group/validation.rs

@@ -649,9 +649,24 @@ impl PublicGroup {
         Ok(())
     }
 
+    /// Validate a leaf node.
+    ///
+    /// This always validates the lifetime.
     pub(crate) fn validate_leaf_node(
         &self,
         leaf_node: &crate::treesync::LeafNode,
+    ) -> Result<(), LeafNodeValidationError> {
+        // Call the validation function and validate the lifetime
+        self.validate_leaf_node_inner(leaf_node, false)
+    }
+
+    /// Validate a leaf node.
+    ///
+    /// This may skip checking the lifetime when validating a ratchet tree.
+    pub(crate) fn validate_leaf_node_inner(
+        &self,
+        leaf_node: &crate::treesync::LeafNode,
+        ratchet_tree: bool,
     ) -> Result<(), LeafNodeValidationError> {
         // https://validation.openmls.tech/#valn0103
         // https://validation.openmls.tech/#valn0104
@@ -665,9 +680,18 @@ impl PublicGroup {
         // Only leaf nodes in key packages contain lifetimes, so this will return None for other
         // cases. Therefore we only check the lifetimes for leaf nodes in key packages.
         //
+        // We may want to check these in ratchet trees as well.
+        // However, this may lead to errors when leaf nodes don't get updated
+        // after being added to the tree. RFC 9420 recommends checking the lifetime
+        // but acknowledges already that this may cause issues.
+        // https://www.rfc-editor.org/rfc/rfc9420.html#section-7.3-4.5.1
+        // See #1810 for more background.
+        // We therefore DO NOT check the lifetime if this is a leaf node check
+        // for a ratchet tree.
+        //
         // Some KATs use key packages that are expired by now. In order to run these tests, we
         // provide a way to turn off this check.
-        if !crate::skip_validation::is_disabled::leaf_node_lifetime() {
+        if !ratchet_tree && !crate::skip_validation::is_disabled::leaf_node_lifetime() {
             if let Some(lifetime) = leaf_node.life_time() {
                 if !lifetime.is_valid() {
                     log::warn!("offending lifetime: {lifetime:?}");

openmls/src/group/tests_and_kats/tests/capabilities_check.rs

@@ -154,6 +154,7 @@ impl<'a, 'b: 'a, Provider: OpenMlsProvider> PreGroupPartyState<'b, Provider> {
             },
             Extensions::default(),
             &self.core_state.provider,
+            None,
             &self.signer,
         );
     }

openmls/src/key_packages/lifetime.rs

@@ -73,6 +73,14 @@ impl Lifetime {
         }
     }
 
+    /// Initialize raw lifetime without skew and explicit dates.
+    pub fn init(not_before: u64, not_after: u64) -> Self {
+        Self {
+            not_before,
+            not_after,
+        }
+    }
+
     /// Returns true if this lifetime is valid.
     pub fn is_valid(&self) -> bool {
         match SystemTime::now()

openmls/src/test_utils/single_group_test_framework/mod.rs

@@ -51,10 +51,16 @@ pub(crate) fn generate_key_package(
     credential_with_key: CredentialWithKey,
     extensions: Extensions,
     provider: &impl crate::storage::OpenMlsProvider,
+    lifetime: impl Into<Option<Lifetime>>,
     signer: &impl Signer,
 ) -> KeyPackageBundle {
-    KeyPackage::builder()
-        .key_package_extensions(extensions)
+    let mut builder = KeyPackage::builder().key_package_extensions(extensions);
+
+    if let Some(lifetime) = lifetime.into() {
+        builder = builder.key_package_lifetime(lifetime);
+    }
+
+    builder
         .build(ciphersuite, provider, signer, credential_with_key)
         .unwrap()
 }
@@ -83,9 +89,19 @@ pub struct PreGroupPartyState<'a, Provider> {
     pub core_state: &'a CorePartyState<Provider>,
 }
 
+// XXX: This should probably get a builder at some point.
 impl<Provider: OpenMlsProvider> CorePartyState<Provider> {
     /// Generates the pre-group state for a `CorePartyState`
     pub fn generate_pre_group(&self, ciphersuite: Ciphersuite) -> PreGroupPartyState<'_, Provider> {
+        self.generate_pre_group_lifetime(ciphersuite, None)
+    }
+
+    /// Generates the pre-group state for a `CorePartyState`
+    pub fn generate_pre_group_lifetime(
+        &self,
+        ciphersuite: Ciphersuite,
+        lifetime: impl Into<Option<Lifetime>>,
+    ) -> PreGroupPartyState<'_, Provider> {
         let (credential_with_key, signer) = generate_credential(
             self.name.into(),
             ciphersuite.signature_algorithm(),
@@ -97,6 +113,7 @@ impl<Provider: OpenMlsProvider> CorePartyState<Provider> {
             credential_with_key.clone(),
             Extensions::default(), // TODO: provide as argument?
             &self.provider,
+            lifetime,
             &signer,
         );
 

openmls/tests/join.rs

@@ -0,0 +1,97 @@
+use std::{
+    thread,
+    time::{Duration, SystemTime, UNIX_EPOCH},
+};
+
+use openmls::{prelude::*, test_utils::single_group_test_framework::*};
+use openmls_test::openmls_test;
+
+#[openmls_test]
+fn join_tree_with_outdated_leafnodes() {
+    // The validity of the key package into the future.
+    // This has to be short to keep the test run fast, but not too short to produce
+    // failing tests.
+    const VALIDITY: u64 = 2;
+
+    let alice_party = CorePartyState::<Provider>::new("alice");
+    let bob_party = CorePartyState::<Provider>::new("bob");
+    let charlie_party = CorePartyState::<Provider>::new("charlie");
+
+    // Create group
+    let create_config = MlsGroupCreateConfig::test_default_from_ciphersuite(ciphersuite);
+    let join_config = create_config.join_config().clone();
+    let mut group_state = {
+        let group_id = GroupId::from_slice(b"Test Group");
+
+        let group_state = GroupState::new_from_party(
+            group_id,
+            alice_party.generate_pre_group(ciphersuite),
+            create_config,
+        )
+        .unwrap();
+        group_state
+    };
+
+    // Create Charlie key package
+    let charlie_pre_group = charlie_party.generate_pre_group(ciphersuite);
+    let charlie_key_package = charlie_pre_group.key_package_bundle.key_package().clone();
+
+    // Generate a key package for Bob that is outdated when inviting Charlie.
+    // This test assumes that the setup goes through within VALIDITY seconds.
+    // After that the key package is invalid already.
+    let now = SystemTime::now()
+        .duration_since(UNIX_EPOCH)
+        .expect("SystemTime before UNIX EPOCH!")
+        .as_secs();
+    let bob_pre_group = bob_party
+        .generate_pre_group_lifetime(ciphersuite, Lifetime::init(now - 60, now + VALIDITY));
+    let bob_key_package = bob_pre_group.key_package_bundle.key_package().clone();
+
+    let [alice] = group_state.members_mut(&["alice"]);
+
+    // Alice adds Bob
+    let (_mls_message_out, _welcome, _group_info) = alice
+        .group
+        .add_members(
+            &alice_party.provider,
+            &alice.party.signer,
+            &[bob_key_package],
+        )
+        .expect("Could not add Bob.");
+
+    alice
+        .group
+        .merge_pending_commit(&alice_party.provider)
+        .unwrap();
+
+    // We don't care about Bob actually processing the Welcome.
+    // Let's wait for VALIDITY seconds to ensure that the leaf node is invalid.
+    thread::sleep(Duration::from_secs(VALIDITY));
+
+    // Alice adds Charlie
+    // At this point Bob's key package is outdated in the tree.
+    let (_mls_message_out, welcome, _group_info) = alice
+        .group
+        .add_members(
+            &alice_party.provider,
+            &alice.party.signer,
+            &[charlie_key_package],
+        )
+        .expect("Could not add Charlie.");
+
+    alice
+        .group
+        .merge_pending_commit(&alice_party.provider)
+        .unwrap();
+
+    // Charlie tries to join the group
+    let _charlie_group = StagedWelcome::new_from_welcome(
+        &charlie_party.provider,
+        &join_config,
+        MlsMessageIn::from(welcome).into_welcome().unwrap(),
+        None,
+    )
+    .expect("Failed to create group due to an invalid lifetime in a leaf node in the tree.")
+    .into_group(provider)
+    .unwrap();
+}