fix: trust check failure of Sync message sent to non-elders

src/messages/mod.rs

@@ -214,11 +214,7 @@ impl Message {
                     .filter(|(known_prefix, _)| prefix.is_compatible(known_prefix))
                     .map(|(_, key)| key);
 
-                match proof_chain.check_trust(trusted_keys) {
-                    TrustStatus::Trusted => Ok(VerifyStatus::Full),
-                    TrustStatus::Unknown => Ok(VerifyStatus::Unknown),
-                    TrustStatus::Invalid => Err(Error::UntrustedMessage),
-                }
+                proof_chain.check_trust(trusted_keys).into()
             }
         }
     }
@@ -262,10 +258,12 @@ impl Message {
     pub(crate) fn extend_proof_chain(
         mut self,
         new_first_key: &bls::PublicKey,
-        section_proof_chain: &SectionProofChain,
+        full_chain: &SectionProofChain,
     ) -> Result<Self, ExtendProofChainError> {
-        if let Some(proof_chain) = self.proof_chain.as_mut() {
-            proof_chain.extend(new_first_key, section_proof_chain)?;
+        if let Variant::Sync { section, .. } = &mut self.variant {
+            section.extend_chain(new_first_key, full_chain)?
+        } else if let Some(proof_chain) = &mut self.proof_chain {
+            proof_chain.extend(new_first_key, full_chain)?
         } else {
             return Err(ExtendProofChainError::NoProofChain);
         }
@@ -313,6 +311,16 @@ pub enum VerifyStatus {
     Unknown,
 }
 
+impl Into<Result<VerifyStatus>> for TrustStatus {
+    fn into(self) -> Result<VerifyStatus> {
+        match self {
+            Self::Trusted => Ok(VerifyStatus::Full),
+            Self::Unknown => Ok(VerifyStatus::Unknown),
+            Self::Invalid => Err(Error::InvalidMessage),
+        }
+    }
+}
+
 /// Status of an incomming message.
 #[derive(Eq, PartialEq)]
 pub enum MessageStatus {

src/messages/variant.rs

@@ -13,7 +13,7 @@ use crate::{
     error::{Error, Result},
     network::Network,
     relocation::{RelocateDetails, RelocatePayload, RelocatePromise},
-    section::{EldersInfo, MemberInfo, Section, SectionProofChain, TrustStatus},
+    section::{EldersInfo, MemberInfo, Section, SectionProofChain},
 };
 use bls_dkg::key_gen::message::Message as DkgMessage;
 use bytes::Bytes;
@@ -136,31 +136,24 @@ impl Variant {
                 let proof_chain = proof_chain.ok_or(Error::InvalidMessage)?;
 
                 if !elders_info.verify(proof_chain) {
-                    return Err(Error::UntrustedMessage);
+                    return Err(Error::InvalidMessage);
                 }
 
                 if !member_info.verify(proof_chain) {
-                    return Err(Error::UntrustedMessage);
+                    return Err(Error::InvalidMessage);
                 }
 
-                match proof_chain.check_trust(trusted_keys) {
-                    TrustStatus::Trusted => Ok(VerifyStatus::Full),
-                    TrustStatus::Unknown => Ok(VerifyStatus::Unknown),
-                    TrustStatus::Invalid => Err(Error::UntrustedMessage),
-                }
+                proof_chain.check_trust(trusted_keys).into()
             }
+            Self::Sync { section, .. } => section.chain().check_trust(trusted_keys).into(),
             Self::NeighbourInfo { elders_info, .. } => {
                 let proof_chain = proof_chain.ok_or(Error::InvalidMessage)?;
 
                 if !elders_info.verify(proof_chain) {
-                    return Err(Error::UntrustedMessage);
+                    return Err(Error::InvalidMessage);
                 }
 
-                match proof_chain.check_trust(trusted_keys) {
-                    TrustStatus::Trusted => Ok(VerifyStatus::Full),
-                    TrustStatus::Unknown => Ok(VerifyStatus::Unknown),
-                    TrustStatus::Invalid => Err(Error::UntrustedMessage),
-                }
+                proof_chain.check_trust(trusted_keys).into()
             }
             _ => Ok(VerifyStatus::Full),
         }

src/routing/approved.rs

@@ -1536,7 +1536,7 @@ impl Approved {
 
         if old_is_elder && !new_is_elder {
             info!("Demoted");
-            self.section = self.section.to_minimal();
+            self.section = self.section.trimmed(1);
             self.network = Network::new();
             self.section_keys_provider = SectionKeysProvider::new(None);
             self.send_event(Event::Demoted);
@@ -1621,33 +1621,41 @@ impl Approved {
     }
 
     fn send_sync(&mut self, section: Section, network: Network) -> Result<Vec<Command>> {
-        let mut commands = vec![];
-
-        for peer in section.active_members() {
-            if peer.name() == &self.node.name() {
-                continue;
-            }
-
-            let variant = if section.is_elder(peer.name()) {
-                Variant::Sync {
-                    section: section.clone(),
-                    network: network.clone(),
-                }
-            } else {
-                Variant::Sync {
-                    section: section.to_minimal(),
-                    network: Network::new(),
-                }
-            };
+        let send = |variant, recipients: Vec<_>| -> Result<_> {
+            trace!("Send {:?} to {:?}", variant, recipients);
 
-            trace!("Send {:?} to {:?}", variant, peer);
             let message =
                 Message::single_src(&self.node, DstLocation::Direct, variant, None, None)?;
-            commands.push(Command::send_message_to_target(
-                peer.addr(),
+            let recipients: Vec<_> = recipients.iter().map(Peer::addr).copied().collect();
+
+            Ok(Command::send_message_to_targets(
+                &recipients,
+                recipients.len(),
                 message.to_bytes(),
             ))
-        }
+        };
+
+        let mut commands = vec![];
+
+        let (elders, non_elders): (Vec<_>, _) = section
+            .active_members()
+            .filter(|peer| peer.name() != &self.node.name())
+            .copied()
+            .partition(|peer| section.is_elder(peer.name()));
+
+        // Send the trimmed state to non-elders. The trimmed state contains only the latest
+        // section key and one key before that which is the key the recipients should know so they
+        // will be able to trust it.
+        let variant = Variant::Sync {
+            section: section.trimmed(2),
+            network: Network::new(),
+        };
+        commands.push(send(variant, non_elders)?);
+
+        // Send the full state to elders.
+        // The full state contains the whole section chain.
+        let variant = Variant::Sync { section, network };
+        commands.push(send(variant, elders)?);
 
         Ok(commands)
     }

src/routing/tests/mod.rs

@@ -1292,7 +1292,7 @@ async fn receive_message_with_invalid_proof_chain() -> Result<()> {
         })
         .await;
 
-    assert_matches!(result, Err(Error::UntrustedMessage));
+    assert_matches!(result, Err(Error::InvalidMessage));
 
     Ok(())
 }
@@ -1443,8 +1443,6 @@ async fn message_to_self(dst: MessageDst) -> Result<()> {
     Ok(())
 }
 
-// FIXME: currently failing
-#[ignore]
 #[tokio::test]
 async fn handle_elders_update() -> Result<()> {
     // Start with section that has `ELDER_SIZE` elders with age 6, 1 non-elder with age 5 and one
@@ -1475,14 +1473,17 @@ async fn handle_elders_update() -> Result<()> {
     let demoted_peer = other_elder_peers.remove(0);
 
     // Create `HandleConsensus` command for an `OurElders` vote. This will demote one of the
-    // current elders and promote the age 7 peer.
+    // current elders and promote the oldest peer.
     let elders_info1 = EldersInfo::new(
         iter::once(node.peer())
             .chain(other_elder_peers.clone())
             .chain(iter::once(promoted_peer)),
         Prefix::default(),
     );
+    let elder_names1: BTreeSet<_> = elders_info1.elders.keys().copied().collect();
+
     let sk_set1 = SecretKeySet::random();
+    let pk1 = sk_set1.secret_key().public_key();
 
     let proven_elders_info1 = proven(sk_set1.secret_key(), elders_info1)?;
     let vote = Vote::OurElders(proven_elders_info1);
@@ -1491,15 +1492,11 @@ async fn handle_elders_update() -> Result<()> {
         .sign(&bincode::serialize(&vote.as_signable())?);
     let proof = Proof {
         signature,
-        public_key: sk_set0.secret_key().public_key(),
+        public_key: pk0,
     };
 
-    let state = Approved::new(
-        node,
-        section0.clone(),
-        Some(section_key_share),
-        mpsc::unbounded_channel().0,
-    );
+    let (event_tx, mut event_rx) = mpsc::unbounded_channel();
+    let state = Approved::new(node, section0.clone(), Some(section_key_share), event_tx);
     let stage = Stage::new(state, create_comm()?);
 
     let commands = stage
@@ -1524,21 +1521,7 @@ async fn handle_elders_update() -> Result<()> {
             _ => continue,
         };
 
-        assert_eq!(recipients.len(), 1);
-        let recipient = recipients[0];
-
-        if &recipient == adult_peer.addr() || &recipient == demoted_peer.addr() {
-            // adults get only the latest key
-            assert_eq!(section.chain().len(), 1);
-        } else {
-            // elders get the full chain
-            assert_eq!(section.chain().len(), 2);
-        }
-
-        assert_eq!(
-            section.chain().last_key(),
-            &sk_set1.secret_key().public_key()
-        );
+        assert_eq!(section.chain().last_key(), &pk1);
 
         // The message is trusted even by peers who don't yet know the new section key.
         assert_matches!(
@@ -1549,7 +1532,7 @@ async fn handle_elders_update() -> Result<()> {
         // Merging the section contained in the message with the original section succeeds.
         assert_matches!(section0.clone().merge(section.clone()), Ok(()));
 
-        assert!(sync_actual_recipients.insert(recipient));
+        sync_actual_recipients.extend(recipients);
     }
 
     let sync_expected_recipients: HashSet<_> = other_elder_peers
@@ -1562,6 +1545,14 @@ async fn handle_elders_update() -> Result<()> {
 
     assert_eq!(sync_actual_recipients, sync_expected_recipients);
 
+    assert_matches!(
+        event_rx.try_recv(),
+        Ok(Event::EldersChanged { key, elders, .. }) => {
+            assert_eq!(key, pk1);
+            assert_eq!(elders, elder_names1);
+        }
+    );
+
     Ok(())
 }
 

src/section/mod.rs

@@ -150,8 +150,14 @@ impl Section {
         self.members.update(member_info)
     }
 
-    pub fn to_minimal(&self) -> Self {
-        let first_key_index = self.elders_info_signing_key_index();
+    // Returns a trimmed version of this `Section` which contains only the elders info and the
+    // section chain truncated to the given length (the chain is truncated from the end, so it
+    // always contains the latest key). If `chain_len` is zero, it is silently replaced with one.
+    pub fn trimmed(&self, chain_len: usize) -> Self {
+        let first_key_index = self
+            .chain
+            .last_key_index()
+            .saturating_sub(chain_len.saturating_sub(1) as u64);
 
         Self {
             elders_info: self.elders_info.clone(),
@@ -164,6 +170,15 @@ impl Section {
         &self.chain
     }
 
+    // Extend the section chain so it starts at `new_first_key` while keeping the last key intact.
+    pub(crate) fn extend_chain(
+        &mut self,
+        new_first_key: &bls::PublicKey,
+        full_chain: &SectionProofChain,
+    ) -> Result<(), ExtendError> {
+        self.chain.extend(new_first_key, full_chain)
+    }
+
     // Creates the shortest proof chain that includes both the key at `their_knowledge`
     // (if provided) and the key our current `elders_info` was signed with.
     pub fn create_proof_chain_for_our_info(

src/section/section_proof_chain.rs

@@ -177,25 +177,25 @@ impl SectionProofChain {
     pub(crate) fn extend(
         &mut self,
         new_first_key: &bls::PublicKey,
-        full_proof_chain: &Self,
+        full_chain: &Self,
     ) -> Result<(), ExtendError> {
         if self.has_key(new_first_key) {
             return Err(ExtendError::AlreadySufficient);
         }
 
-        let index_from = full_proof_chain
+        let index_from = full_chain
             .index_of(new_first_key)
             .ok_or(ExtendError::InvalidFirstKey)?;
 
-        let index_to = full_proof_chain
+        let index_to = full_chain
             .index_of(self.last_key())
             .ok_or(ExtendError::InvalidLastKey)?;
 
         if index_from > index_to {
             return Err(ExtendError::InvalidFirstKey);
         }
 
-        *self = full_proof_chain.slice(index_from..=index_to);
+        *self = full_chain.slice(index_from..=index_to);
 
         Ok(())
     }