From efeba55709cb1d982efd37eb3caa85c063901707 Mon Sep 17 00:00:00 2001
From: Thomas Eizinger <thomas@eizinger.io>
Date: Thu, 14 Nov 2024 02:43:17 +0000
Subject: [PATCH] chore(snownet): fail TURN connection on unknown attribute
 (#7341)

A TURN server that doesn't understand certain attributes should return
"Unknown attributes" as part of its response. Whilst we aim to be as
spec-compliant as possible, Firezone doesn't officially support other
TURN servers than our own relay.

If we encounter a TURN server that sends us an "Unknown attribute", we
now immediately fail this allocation and clear it as we cannot make any
more assumptions about what the connected relay actually supports.
---
 rust/connlib/snownet/src/allocation.rs | 31 +++++++++++++++++++++-----
 1 file changed, 26 insertions(+), 5 deletions(-)

diff --git a/rust/connlib/snownet/src/allocation.rs b/rust/connlib/snownet/src/allocation.rs
index a760feb58..fea494210 100644
--- a/rust/connlib/snownet/src/allocation.rs
+++ b/rust/connlib/snownet/src/allocation.rs
@@ -22,7 +22,7 @@ use stun_codec::{
         attributes::{
             ErrorCode, MessageIntegrity, Nonce, Realm, Software, Username, XorMappedAddress,
         },
-        errors::{StaleNonce, Unauthorized},
+        errors::{StaleNonce, Unauthorized, UnknownAttribute},
         methods::BINDING,
     },
     rfc5766::{
@@ -89,6 +89,8 @@ pub struct Allocation {
     last_now: Instant,
 
     credentials: Option<Credentials>,
+
+    explicit_failure: Option<FreeReason>,
 }
 
 #[derive(Debug, PartialEq)]
@@ -194,6 +196,8 @@ pub enum FreeReason {
     AuthenticationError,
     #[error("no response received. Is STUN blocked?")]
     NoResponseReceived,
+    #[error("TURN protocol failure")]
+    ProtocolFailure,
 }
 
 impl Allocation {
@@ -227,6 +231,7 @@ impl Allocation {
             buffered_channel_bindings: RingBuffer::new(100),
             software: Software::new(format!("snownet; session={session_id}"))
                 .expect("description has less then 128 chars"),
+            explicit_failure: Default::default(),
         };
 
         allocation.send_binding_requests();
@@ -396,6 +401,18 @@ impl Allocation {
                 return true;
             }
 
+            if error.code() == UnknownAttribute::CODEPOINT {
+                let attributes = message.unknown_attributes().collect::<Vec<_>>();
+
+                tracing::warn!(
+                    ?attributes,
+                    "Server did not understand one or more attributes in our request"
+                );
+                self.explicit_failure = Some(FreeReason::ProtocolFailure);
+
+                return true;
+            }
+
             // Catch-all error handling if none of the above apply.
             match message.method() {
                 ALLOCATE => {
@@ -420,13 +437,13 @@ impl Allocation {
                     self.channel_bindings.handle_failed_binding(channel);
 
                     // Duplicate log here because we want to attach "channel number" and "peer".
-                    tracing::warn!(error = %error.reason_phrase(), %channel, %peer);
+                    tracing::warn!(error = %error.reason_phrase(), %channel, %peer, "Channel bind failed");
                     return true;
                 }
                 _ => {}
             }
 
-            tracing::warn!(error = %error.reason_phrase());
+            tracing::warn!(error = %error.reason_phrase(), "TURN request failed");
 
             return true;
         }
@@ -763,7 +780,11 @@ impl Allocation {
     /// Whether this [`Allocation`] can be freed.
     ///
     /// This is tied to having our credentials cleared (i.e due to an authentication error) and having emitted all events or not having received a single response.
-    pub fn can_be_freed(&self) -> Option<FreeReason> {
+    pub fn can_be_freed(&mut self) -> Option<FreeReason> {
+        if let Some(reason) = self.explicit_failure.take() {
+            return Some(reason);
+        }
+
         let pending_work = !self.events.is_empty()
             || !self.buffered_transmits.is_empty()
             || !self.sent_requests.is_empty();
@@ -2375,7 +2396,7 @@ mod tests {
 
     #[test]
     fn allocation_is_not_freed_on_startup() {
-        let allocation = Allocation::for_test_ip4(Instant::now());
+        let mut allocation = Allocation::for_test_ip4(Instant::now());
 
         assert_eq!(allocation.can_be_freed(), None);
     }