fix: avoid invalidating signature when resending bounced Sync message

src/messages/mod.rs

@@ -326,14 +326,14 @@ impl Message {
 
     // Extend the current message proof chain so it starts at `new_first_key` while keeping the
     // last key (and therefore the signature) intact.
+    // NOTE: This operation doesn't invalidate the signatures because the proof chain is not part of
+    // the signed data.
     pub(crate) fn extend_proof_chain(
         mut self,
         new_first_key: &bls::PublicKey,
         full_chain: &SectionChain,
     ) -> Result<Self, ExtendProofChainError> {
-        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 {
+        if let Some(proof_chain) = &mut self.proof_chain {
             *proof_chain = proof_chain.extend(new_first_key, full_chain)?
         } else {
             return Err(ExtendProofChainError::NoProofChain);

src/routing/approved.rs

@@ -675,14 +675,11 @@ impl Approved {
             Variant::UserMessage(content) => self.handle_user_message(&msg, content.clone()),
             Variant::BouncedUntrustedMessage(message) => {
                 let sender = sender.ok_or(Error::InvalidSrcLocation)?;
-                Ok(self
-                    .handle_bounced_untrusted_message(
-                        msg.src().to_node_peer(sender)?,
-                        *msg.dst_key(),
-                        *message.clone(),
-                    )
-                    .into_iter()
-                    .collect())
+                Ok(vec![self.handle_bounced_untrusted_message(
+                    msg.src().to_node_peer(sender)?,
+                    *msg.dst_key(),
+                    *message.clone(),
+                )?])
             }
             Variant::BouncedUnknownMessage { src_key, message } => {
                 let sender = sender.ok_or(Error::InvalidSrcLocation)?;
@@ -887,28 +884,52 @@ impl Approved {
         sender: Peer,
         dst_key: Option<bls::PublicKey>,
         bounced_msg: Message,
-    ) -> Option<Command> {
+    ) -> Result<Command> {
         let span = trace_span!("Received BouncedUntrustedMessage", ?bounced_msg, %sender);
         let _span_guard = span.enter();
 
-        if let Some(dst_key) = dst_key {
-            let resend_msg = match bounced_msg.extend_proof_chain(&dst_key, self.section.chain()) {
-                Ok(msg) => msg,
-                Err(err) => {
-                    trace!("extending proof failed, discarding: {:?}", err);
-                    return None;
-                }
-            };
+        let dst_key = dst_key.ok_or_else(|| {
+            error!("missing dst key");
+            Error::InvalidMessage
+        })?;
 
-            trace!("resending with extended proof");
-            Some(Command::send_message_to_node(
-                sender.addr(),
-                resend_msg.to_bytes(),
-            ))
-        } else {
-            trace!("missing dst key, discarding");
-            None
-        }
+        let resend_msg = match bounced_msg.variant() {
+            Variant::Sync { section, network } => {
+                // `Sync` messages are handled specially, because they don't carry a proof chain.
+                // Instead we use the section chain that's part of the included `Section` struct.
+                // Problem is we can't extend that chain as it would invalidate the signature. We
+                // must construct a new message instead.
+                let section = section
+                    .extend_chain(&dst_key, self.section.chain())
+                    .map_err(|err| {
+                        error!("extending section chain failed: {:?}", err);
+                        Error::InvalidMessage // TODO: more specific error
+                    })?;
+
+                Message::single_src(
+                    &self.node,
+                    DstLocation::Direct,
+                    Variant::Sync {
+                        section,
+                        network: network.clone(),
+                    },
+                    None,
+                    None,
+                )?
+            }
+            _ => bounced_msg
+                .extend_proof_chain(&dst_key, self.section.chain())
+                .map_err(|err| {
+                    error!("extending proof chain failed: {:?}", err);
+                    Error::InvalidMessage // TODO: more specific error
+                })?,
+        };
+
+        trace!("resending with extended proof");
+        Ok(Command::send_message_to_node(
+            sender.addr(),
+            resend_msg.to_bytes(),
+        ))
     }
 
     fn handle_bounced_unknown_message(

src/routing/bootstrap.rs

@@ -186,7 +186,11 @@ impl<'a> State<'a> {
         recipients: Vec<SocketAddr>,
         relocate_details: Option<&SignedRelocateDetails>,
     ) -> Result<()> {
-        debug!("{} Sending GetSectionQuery to {:?}", self.node, recipients);
+        if recipients.is_empty() {
+            return Ok(());
+        }
+
+        debug!("Sending GetSectionQuery to {:?}", recipients);
 
         let destination = relocate_details
             .map(|details| *details.destination())

src/routing/tests/mod.rs

@@ -1355,6 +1355,94 @@ async fn handle_untrusted_sync() -> Result<()> {
     Ok(())
 }
 
+#[tokio::test]
+async fn handle_bounced_untrusted_sync() -> Result<()> {
+    let sk0 = bls::SecretKey::random();
+    let pk0 = sk0.public_key();
+
+    let sk1 = bls::SecretKey::random();
+    let pk1 = sk1.public_key();
+    let sig1 = sk0.sign(&bincode::serialize(&pk1)?);
+
+    let sk2_set = SecretKeySet::random();
+    let sk2 = sk2_set.secret_key();
+    let pk2 = sk2.public_key();
+    let sig2 = sk1.sign(&bincode::serialize(&pk2)?);
+
+    let mut chain = SectionChain::new(pk0);
+    chain.insert(&pk0, pk1, sig1)?;
+    chain.insert(&pk1, pk2, sig2)?;
+
+    let (elders_info, mut nodes) = create_elders_info();
+    let proven_elders_info = proven(&sk0, elders_info.clone())?;
+    let section_full = Section::new(chain, proven_elders_info)?;
+    let section_trimmed = section_full.trimmed(2);
+
+    let (event_tx, _) = mpsc::unbounded_channel();
+    let node = nodes.remove(0);
+    let section_key_share = create_section_key_share(&sk2_set, 0);
+    let state = Approved::new(
+        node.clone(),
+        section_full,
+        Some(section_key_share),
+        event_tx,
+    );
+    let stage = Stage::new(state, create_comm().await?);
+
+    let orig_message = Message::single_src(
+        &node,
+        DstLocation::Direct,
+        Variant::Sync {
+            section: section_trimmed,
+            network: Network::new(),
+        },
+        None,
+        None,
+    )?;
+
+    let sender = create_node();
+    let bounced_message = Message::single_src(
+        &sender,
+        DstLocation::Node(node.name()),
+        Variant::BouncedUntrustedMessage(Box::new(orig_message)),
+        None,
+        Some(pk0),
+    )?;
+
+    let commands = stage
+        .handle_command(Command::HandleMessage {
+            message: bounced_message,
+            sender: Some(sender.addr),
+        })
+        .await?;
+
+    let mut message_resent = false;
+
+    for command in commands {
+        let (recipients, message) = match command {
+            Command::SendMessage {
+                recipients,
+                message: MessageType::NodeMessage(NodeMessage(msg_bytes)),
+                ..
+            } => (recipients, Message::from_bytes(Bytes::from(msg_bytes))?),
+            _ => continue,
+        };
+
+        match message.variant() {
+            Variant::Sync { section, .. } => {
+                assert_eq!(recipients, [sender.addr]);
+                assert!(section.chain().has_key(&pk0));
+                message_resent = true;
+            }
+            _ => continue,
+        }
+    }
+
+    assert!(message_resent);
+
+    Ok(())
+}
+
 #[tokio::test]
 async fn relocation_of_non_elder() -> Result<()> {
     relocation(RelocatedPeerRole::NonElder).await

src/section/mod.rs

@@ -175,14 +175,19 @@ impl Section {
         &self.chain
     }
 
-    // Extend the section chain so it starts at `new_first_key` while keeping the last key intact.
+    // Extend the section chain so it starts at `trusted_key` while keeping the last key intact.
     pub(crate) fn extend_chain(
-        &mut self,
-        new_first_key: &bls::PublicKey,
+        &self,
+        trusted_key: &bls::PublicKey,
         full_chain: &SectionChain,
-    ) -> Result<(), SectionChainError> {
-        self.chain = self.chain.extend(new_first_key, full_chain)?;
-        Ok(())
+    ) -> Result<Self, SectionChainError> {
+        let chain = self.chain.extend(trusted_key, full_chain)?;
+
+        Ok(Self {
+            elders_info: self.elders_info.clone(),
+            chain,
+            members: self.members.clone(),
+        })
     }
 
     pub fn elders_info(&self) -> &EldersInfo {