From 2eedc23b82cfeb0af23dbee15c0e57514fdca47a Mon Sep 17 00:00:00 2001 From: Thomas Eizinger Date: Thu, 26 Jun 2025 16:49:07 +0100 Subject: [PATCH] chore(snownet): embed more context in WireGuard errors (#9687) --- rust/Cargo.lock | 1 + rust/connlib/snownet/Cargo.toml | 1 + rust/connlib/snownet/src/lib.rs | 2 +- rust/connlib/snownet/src/node.rs | 55 +++++++++++++----------------- rust/connlib/tunnel/src/client.rs | 4 +-- rust/connlib/tunnel/src/gateway.rs | 2 +- 6 files changed, 29 insertions(+), 36 deletions(-) diff --git a/rust/Cargo.lock b/rust/Cargo.lock index 608c2e672..005a55991 100644 --- a/rust/Cargo.lock +++ b/rust/Cargo.lock @@ -6804,6 +6804,7 @@ dependencies = [ name = "snownet" version = "0.1.0" dependencies = [ + "anyhow", "boringtun", "bufferpool", "bytecodec", diff --git a/rust/connlib/snownet/Cargo.toml b/rust/connlib/snownet/Cargo.toml index c70b5683a..7672fe37c 100644 --- a/rust/connlib/snownet/Cargo.toml +++ b/rust/connlib/snownet/Cargo.toml @@ -5,6 +5,7 @@ edition = { workspace = true } license = { workspace = true } [dependencies] +anyhow = { workspace = true } boringtun = { workspace = true } bufferpool = { workspace = true } bytecodec = { workspace = true } diff --git a/rust/connlib/snownet/src/lib.rs b/rust/connlib/snownet/src/lib.rs index 4569d0f8b..2b0fadedd 100644 --- a/rust/connlib/snownet/src/lib.rs +++ b/rust/connlib/snownet/src/lib.rs @@ -16,7 +16,7 @@ pub use allocation::RelaySocket; #[allow(deprecated)] // Rust bug: `expect` doesn't seem to work on imports? pub use node::{Answer, Offer}; pub use node::{ - Client, ClientNode, Credentials, Error, Event, HANDSHAKE_TIMEOUT, NoTurnServers, Node, Server, + Client, ClientNode, Credentials, Event, HANDSHAKE_TIMEOUT, NoTurnServers, Node, Server, ServerNode, Transmit, }; pub use stats::{ConnectionStats, NodeStats}; diff --git a/rust/connlib/snownet/src/node.rs b/rust/connlib/snownet/src/node.rs index 29398bc18..44c0427cd 100644 --- a/rust/connlib/snownet/src/node.rs +++ b/rust/connlib/snownet/src/node.rs @@ -3,6 +3,7 @@ use crate::candidate_set::CandidateSet; use crate::index::IndexLfsr; use crate::stats::{ConnectionStats, NodeStats}; use crate::utils::{channel_data_packet_buffer, earliest}; +use anyhow::{Context, Result, anyhow}; use boringtun::noise::errors::WireGuardError; use boringtun::noise::{Tunn, TunnResult}; use boringtun::x25519::PublicKey; @@ -136,22 +137,6 @@ pub struct Node { rng: StdRng, } -#[derive(thiserror::Error, Debug)] -pub enum Error { - #[error("Unknown interface")] - UnknownInterface, - #[error("Failed to decapsulate: {0}")] - Decapsulate(boringtun::noise::errors::WireGuardError), - #[error("Failed to encapsulate: {0}")] - Encapsulate(boringtun::noise::errors::WireGuardError), - #[error("Packet has unknown format")] - UnknownPacketFormat, - #[error("Not connected")] - NotConnected, - #[error("Invalid local address: {0}")] - BadLocalAddress(#[from] str0m::error::IceError), -} - #[derive(thiserror::Error, Debug)] #[error("No TURN servers available")] pub struct NoTurnServers {} @@ -337,7 +322,7 @@ where /// └─────────┘ │ │ └────┘ /// └──┘ /// ``` - pub fn add_local_host_candidate(&mut self, address: SocketAddr) -> Result<(), Error> { + pub fn add_local_host_candidate(&mut self, address: SocketAddr) -> Result<()> { self.add_local_as_host_candidate(address)?; Ok(()) @@ -410,7 +395,7 @@ where from: SocketAddr, packet: &[u8], now: Instant, - ) -> Result, Error> { + ) -> Result> { self.add_local_as_host_candidate(local)?; let (from, packet, relayed) = match self.allocations_try_handle(from, local, packet, now) { @@ -446,11 +431,11 @@ where connection: TId, packet: IpPacket, now: Instant, - ) -> Result, Error> { + ) -> Result> { let conn = self .connections .get_established_mut(&connection) - .ok_or(Error::NotConnected)?; + .with_context(|| format!("Unknown connection {connection}"))?; if self.mode.is_server() && !conn.state.has_nominated_socket() { tracing::debug!( @@ -466,7 +451,8 @@ where // Encode the packet with an offset of 4 bytes, in case we need to wrap it in a channel-data message. let Some(packet_len) = conn - .encapsulate(packet, &mut buffer[4..], now)? + .encapsulate(packet, &mut buffer[4..], now) + .with_context(|| format!("cid={connection}"))? .map(|p| p.len()) // Mapping to len() here terminate the mutable borrow of buffer, allowing re-borrowing further down. else { @@ -489,7 +475,9 @@ where } ConnectionState::Connected { peer_socket, .. } => peer_socket, ConnectionState::Idle { peer_socket } => peer_socket, - ConnectionState::Failed => return Err(Error::NotConnected), + ConnectionState::Failed => { + return Err(anyhow!("Connection {connection} failed")); + } }; match *socket { @@ -783,8 +771,9 @@ where /// /// Receiving traffic on a certain interface means we at least have a connection to a relay via this interface. /// Thus, it is also a viable interface to attempt a connection to a gateway. - fn add_local_as_host_candidate(&mut self, local: SocketAddr) -> Result<(), Error> { - let host_candidate = Candidate::host(local, Protocol::Udp)?; + fn add_local_as_host_candidate(&mut self, local: SocketAddr) -> Result<()> { + let host_candidate = + Candidate::host(local, Protocol::Udp).context("Failed to parse host candidate")?; if !self.shared_candidates.insert(host_candidate.clone()) { return Ok(()); @@ -868,7 +857,7 @@ where destination: SocketAddr, packet: &[u8], now: Instant, - ) -> ControlFlow> { + ) -> ControlFlow> { let Ok(message) = StunMessage::parse(packet) else { return ControlFlow::Continue(()); }; @@ -899,7 +888,7 @@ where from: SocketAddr, packet: &[u8], now: Instant, - ) -> ControlFlow, (TId, IpPacket)> { + ) -> ControlFlow, (TId, IpPacket)> { for (cid, conn) in self.connections.iter_established_mut() { if !conn.accepts(&from) { continue; @@ -927,7 +916,9 @@ where return match control_flow { ControlFlow::Continue(c) => ControlFlow::Continue((cid, c)), - ControlFlow::Break(b) => ControlFlow::Break(b), + ControlFlow::Break(b) => ControlFlow::Break( + b.with_context(|| format!("cid={cid} length={}", packet.len())), + ), }; } @@ -939,7 +930,7 @@ where return ControlFlow::Break(Ok(())); } - ControlFlow::Break(Err(Error::UnknownPacketFormat)) + ControlFlow::Break(Err(anyhow!("Packet has unknown format"))) } fn allocations_drain_events(&mut self) { @@ -2086,14 +2077,14 @@ where packet: IpPacket, buffer: &'b mut [u8], now: Instant, - ) -> Result, Error> { + ) -> Result> { let _guard = self.span.enter(); self.state.on_outgoing(&mut self.agent, &packet, now); let len = match self.tunnel.encapsulate_at(packet.packet(), buffer, now) { TunnResult::Done => return Ok(None), - TunnResult::Err(e) => return Err(Error::Encapsulate(e)), + TunnResult::Err(e) => return Err(anyhow::Error::new(e)), TunnResult::WriteToNetwork(packet) => packet.len(), TunnResult::WriteToTunnelV4(_, _) | TunnResult::WriteToTunnelV6(_, _) => { unreachable!("never returned from encapsulate") @@ -2110,7 +2101,7 @@ where allocations: &mut BTreeMap, transmits: &mut VecDeque, now: Instant, - ) -> ControlFlow, IpPacket> { + ) -> ControlFlow, IpPacket> { let _guard = self.span.enter(); let mut ip_packet = IpPacketBuf::new(); @@ -2119,7 +2110,7 @@ where .decapsulate_at(Some(src), packet, ip_packet.buf(), now) { TunnResult::Done => ControlFlow::Break(Ok(())), - TunnResult::Err(e) => ControlFlow::Break(Err(Error::Decapsulate(e))), + TunnResult::Err(e) => ControlFlow::Break(Err(anyhow::Error::new(e))), // For WriteToTunnel{V4,V6}, boringtun returns the source IP of the packet that was tunneled to us. // I am guessing this was done for convenience reasons. diff --git a/rust/connlib/tunnel/src/client.rs b/rust/connlib/tunnel/src/client.rs index 4a66b091d..837064cf9 100644 --- a/rust/connlib/tunnel/src/client.rs +++ b/rust/connlib/tunnel/src/client.rs @@ -415,7 +415,7 @@ impl ClientState { packet.as_ref(), now, ) - .inspect_err(|e| tracing::debug!(%local, num_bytes = %packet.len(), "Failed to decapsulate incoming packet: {}", err_with_src(e))) + .inspect_err(|e| tracing::debug!(%local, num_bytes = %packet.len(), "Failed to decapsulate: {e:#}")) .ok()??; if self.tcp_dns_client.accepts(&packet) { @@ -547,7 +547,7 @@ impl ClientState { let transmit = self .node .encapsulate(gid, packet, now) - .inspect_err(|e| tracing::debug!(%gid, "Failed to encapsulate: {}", err_with_src(e))) + .inspect_err(|e| tracing::debug!(%gid, "Failed to encapsulate: {e:#}")) .ok()??; Some(transmit) diff --git a/rust/connlib/tunnel/src/gateway.rs b/rust/connlib/tunnel/src/gateway.rs index 0e242d8a6..643fcea6c 100644 --- a/rust/connlib/tunnel/src/gateway.rs +++ b/rust/connlib/tunnel/src/gateway.rs @@ -500,7 +500,7 @@ fn encrypt_packet( ) -> Result> { let transmit = node .encapsulate(cid, packet, now) - .context("Failed to encapsulate packet")?; + .context("Failed to encapsulate")?; Ok(transmit) }