fix(snownet): don't use unbound channels for relaying (#3474)

Currently, the `bound` flag is not considered when attempting to relay
data. This isn't actively harmful because the relay will drop them but
it causes warnings in the logs. This PR adds a check to make sure we
only try to relay data via channels that are bound. Additionally, we now
handle failed channel bind requests by clearing the local state.

rust/connlib/snownet/src/allocation.rs

@@ -142,11 +142,27 @@ impl Allocation {
                 return true;
             }
 
+            #[allow(clippy::single_match)] // There will be more eventually.
+            match message.method() {
+                CHANNEL_BIND => {
+                    let Some(channel) = original_request
+                        .get_attribute::<ChannelNumber>()
+                        .map(|c| c.value())
+                    else {
+                        tracing::warn!("Request did not contain a `CHANNEL-NUMBER`");
+                        return true;
+                    };
+
+                    self.channel_bindings.handle_failed_binding(channel);
+                }
+                _ => {}
+            }
+
             // TODO: Handle error codes such as:
             // - Failed allocations
-            // - Failed channel bindings
 
-            tracing::warn!(method = %original_request.method(), error = %error.reason_phrase(), "STUN request failed");
+            tracing::warn!(id = ?original_request.transaction_id(), method = %original_request.method(), error = %error.reason_phrase(), "STUN request failed");
+
             return true;
         }
 
@@ -655,6 +671,15 @@ impl ChannelBindings {
             .map(|(n, _)| *n)
     }
 
+    fn handle_failed_binding(&mut self, c: u16) {
+        let Some(channel) = self.inner.remove(&c) else {
+            debug_assert!(false, "No channel binding for {c}");
+            return;
+        };
+
+        debug_assert!(!channel.bound, "Channel should not yet be bound")
+    }
+
     fn set_confirmed(&mut self, c: u16, now: Instant) -> bool {
         let Some(channel) = self.inner.get_mut(&c) else {
             return false;
@@ -688,7 +713,7 @@ impl Channel {
     ///
     /// In case the channel is older than its lifetime (10 minutes), this returns false because the relay will have de-allocated the channel.
     fn connected_to_peer(&self, peer: SocketAddr, now: Instant) -> bool {
-        self.peer == peer && self.age(now) < Self::CHANNEL_LIFETIME
+        self.peer == peer && self.age(now) < Self::CHANNEL_LIFETIME && self.bound
     }
 
     fn can_rebind(&self, now: Instant) -> bool {
@@ -743,8 +768,10 @@ impl Channel {
 mod tests {
     use super::*;
     use std::net::{IpAddr, Ipv4Addr};
+    use stun_codec::rfc5389::errors::BadRequest;
 
     const PEER1: SocketAddr = SocketAddr::new(IpAddr::V4(Ipv4Addr::LOCALHOST), 10000);
+    const PEER2: SocketAddr = SocketAddr::new(IpAddr::V4(Ipv4Addr::LOCALHOST), 20000);
     const RELAY: SocketAddr = SocketAddr::new(IpAddr::V4(Ipv4Addr::LOCALHOST), 3478);
     const RELAY_ADDR: SocketAddr = SocketAddr::new(IpAddr::V4(Ipv4Addr::LOCALHOST), 9999);
     const MINUTE: Duration = Duration::from_secs(60);
@@ -930,23 +957,58 @@ mod tests {
             "no messages to be sent if we don't have an allocation"
         );
 
-        allocation.handle_timeout(Instant::now());
-        let message = next_stun_message(&mut allocation).unwrap();
-        assert_eq!(message.method(), ALLOCATE);
+        make_allocation(&mut allocation, PEER1);
 
-        assert!(
-            next_stun_message(&mut allocation).is_none(),
-            "no messages to be sent if we don't have an allocation"
+        let message = next_stun_message(&mut allocation).unwrap();
+        assert_eq!(message.method(), CHANNEL_BIND);
+    }
+
+    #[test]
+    fn does_not_relay_to_with_unbound_channel() {
+        let mut allocation = Allocation::new(
+            RELAY,
+            Username::new("foobar".to_owned()).unwrap(),
+            "baz".to_owned(),
+            Realm::new("firezone".to_owned()).unwrap(),
         );
 
+        make_allocation(&mut allocation, PEER1);
+        allocation.bind_channel(PEER2, Instant::now());
+
+        let message = allocation.encode_to_vec(PEER2, b"foobar", Instant::now());
+
+        assert!(message.is_none())
+    }
+
+    #[test]
+    fn failed_channel_binding_removes_state() {
+        let mut allocation = Allocation::new(
+            RELAY,
+            Username::new("foobar".to_owned()).unwrap(),
+            "baz".to_owned(),
+            Realm::new("firezone".to_owned()).unwrap(),
+        );
+
+        make_allocation(&mut allocation, PEER1);
+        allocation.bind_channel(PEER2, Instant::now());
+
+        let channel_bind_msg = next_stun_message(&mut allocation).unwrap();
+
         allocation.handle_input(
             RELAY,
             PEER1,
-            &encode(allocate_response(message.transaction_id())),
+            &encode(channel_bind_bad_request(channel_bind_msg.transaction_id())),
             Instant::now(),
         );
-        let message = next_stun_message(&mut allocation).unwrap();
-        assert_eq!(message.method(), CHANNEL_BIND);
+
+        // TODO: Not the best assertion because we are reaching into private state but better than nothing for now.
+        let channel = allocation
+            .channel_bindings
+            .inner
+            .values()
+            .find(|c| c.peer == PEER2);
+
+        assert!(channel.is_none());
     }
 
     fn ch(peer: SocketAddr, now: Instant) -> Channel {
@@ -964,6 +1026,17 @@ mod tests {
         Some(decode(&transmit.payload).unwrap().unwrap())
     }
 
+    fn make_allocation(allocation: &mut Allocation, local: SocketAddr) {
+        allocation.handle_timeout(Instant::now());
+        let message = next_stun_message(allocation).unwrap();
+        allocation.handle_input(
+            RELAY,
+            local,
+            &encode(allocate_response(message.transaction_id())),
+            Instant::now(),
+        );
+    }
+
     fn allocate_response(id: TransactionId) -> stun_codec::Message<Attribute> {
         let mut message = stun_codec::Message::new(MessageClass::SuccessResponse, ALLOCATE, id);
         message.add_attribute(XorMappedAddress::new(PEER1));
@@ -972,4 +1045,11 @@ mod tests {
 
         message
     }
+
+    fn channel_bind_bad_request(id: TransactionId) -> stun_codec::Message<Attribute> {
+        let mut message = stun_codec::Message::new(MessageClass::ErrorResponse, CHANNEL_BIND, id);
+        message.add_attribute(ErrorCode::from(BadRequest));
+
+        message
+    }
 }