From 520dd0aa31d35b120cb70b1aeb6c3c49b0703a5b Mon Sep 17 00:00:00 2001 From: Thomas Eizinger Date: Fri, 11 Jul 2025 15:54:41 +0200 Subject: [PATCH] feat(gateway): respond with ICMP error for filtered packets (#9816) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit When defining a resource, a Firezone admin can define traffic filters to only allow traffic on certain TCP and/or UDP ports and/or restrict traffic on the ICMP protocol. Presently, when a packet is filtered out on the Gateway, we simply drop it. Dropping packets means the sending application can only react to timeouts and has no other means on error handling. ICMP was conceived to deal with these kind of situations. In particular, the "destination unreachable" type has a dedicated code for filtered packets: "Communication administratively prohibited". Instead of just dropping the not-allowed packet, we now send back an ICMP error with this particular code set, thus informing the sending application that the packet did not get lost but was in fact not routed for policy reasons. When setting a traffic filter that does not allow TCP traffic, attempting to `curl` such a resource now results in the following: ``` ❯ sudo docker compose exec --env RUST_LOG=info -it client /bin/sh -c 'curl -v example.com' * Host example.com:80 was resolved. * IPv6: fd00:2021:1111:8000::, fd00:2021:1111:8000::1, fd00:2021:1111:8000::2, fd00:2021:1111:8000::3 * IPv4: 100.96.0.1, 100.96.0.2, 100.96.0.3, 100.96.0.4 * Trying [fd00:2021:1111:8000::]:80... * connect to fd00:2021:1111:8000:: port 80 from fd00:2021:1111::1e:7658 port 34560 failed: Permission denied * Trying [fd00:2021:1111:8000::1]:80... * connect to fd00:2021:1111:8000::1 port 80 from fd00:2021:1111::1e:7658 port 34828 failed: Permission denied * Trying [fd00:2021:1111:8000::2]:80... * connect to fd00:2021:1111:8000::2 port 80 from fd00:2021:1111::1e:7658 port 44314 failed: Permission denied * Trying [fd00:2021:1111:8000::3]:80... * connect to fd00:2021:1111:8000::3 port 80 from fd00:2021:1111::1e:7658 port 37628 failed: Permission denied * Trying 100.96.0.1:80... * connect to 100.96.0.1 port 80 from 100.66.110.26 port 53780 failed: Host is unreachable * Trying 100.96.0.2:80... * connect to 100.96.0.2 port 80 from 100.66.110.26 port 60748 failed: Host is unreachable * Trying 100.96.0.3:80... * connect to 100.96.0.3 port 80 from 100.66.110.26 port 38378 failed: Host is unreachable * Trying 100.96.0.4:80... * connect to 100.96.0.4 port 80 from 100.66.110.26 port 49866 failed: Host is unreachable * Failed to connect to example.com port 80 after 9 ms: Could not connect to server * closing connection #0 curl: (7) Failed to connect to example.com port 80 after 9 ms: Could not connect to server ``` --- rust/connlib/ip-packet/src/make.rs | 40 ++++--- rust/connlib/tunnel/src/gateway.rs | 4 +- rust/connlib/tunnel/src/peer.rs | 106 +++++++++++-------- rust/connlib/tunnel/src/tests/reference.rs | 9 +- rust/connlib/tunnel/src/tests/sim_client.rs | 22 ---- website/src/components/Changelog/Gateway.tsx | 3 + 6 files changed, 95 insertions(+), 89 deletions(-) diff --git a/rust/connlib/ip-packet/src/make.rs b/rust/connlib/ip-packet/src/make.rs index b639d45dc..659634dc1 100644 --- a/rust/connlib/ip-packet/src/make.rs +++ b/rust/connlib/ip-packet/src/make.rs @@ -172,16 +172,20 @@ where } } -pub fn icmp_dst_unreachable(original_packet: &IpPacket) -> Result { +pub fn icmp_dest_unreachable( + original_packet: &IpPacket, + icmpv4: icmpv4::DestUnreachableHeader, + icmpv6: icmpv6::DestUnreachableCode, +) -> Result { let src = original_packet.source(); let dst = original_packet.destination(); let icmp_error = match (src, dst) { (IpAddr::V4(src), IpAddr::V4(dst)) => { - icmpv4_network_unreachable(dst, src, original_packet)? + icmpv4_unreachable(dst, src, original_packet, icmpv4)? } (IpAddr::V6(src), IpAddr::V6(dst)) => { - icmpv6_address_unreachable(dst, src, original_packet)? + icmpv6_unreachable(dst, src, original_packet, icmpv6)? } (IpAddr::V4(_), IpAddr::V6(_)) => { bail!("Invalid IP packet: Inconsistent IP address versions") @@ -194,14 +198,14 @@ pub fn icmp_dst_unreachable(original_packet: &IpPacket) -> Result { Ok(icmp_error) } -fn icmpv4_network_unreachable( +fn icmpv4_unreachable( src: Ipv4Addr, dst: Ipv4Addr, original_packet: &IpPacket, + code: icmpv4::DestUnreachableHeader, ) -> Result { - let builder = PacketBuilder::ipv4(src.octets(), dst.octets(), 20).icmpv4( - crate::Icmpv4Type::DestinationUnreachable(icmpv4::DestUnreachableHeader::Network), - ); + let builder = PacketBuilder::ipv4(src.octets(), dst.octets(), 20) + .icmpv4(crate::Icmpv4Type::DestinationUnreachable(code)); let payload = original_packet.packet(); let header_len = original_packet @@ -218,16 +222,16 @@ fn icmpv4_network_unreachable( Ok(ip_packet) } -fn icmpv6_address_unreachable( +fn icmpv6_unreachable( src: Ipv6Addr, dst: Ipv6Addr, original_packet: &IpPacket, + code: icmpv6::DestUnreachableCode, ) -> Result { const MAX_ICMP_ERROR_PAYLOAD_LEN: usize = MAX_IP_SIZE - Ipv6Header::LEN - Icmpv6Header::MAX_LEN; - let builder = PacketBuilder::ipv6(src.octets(), dst.octets(), 20).icmpv6( - crate::Icmpv6Type::DestinationUnreachable(icmpv6::DestUnreachableCode::Address), - ); + let builder = PacketBuilder::ipv6(src.octets(), dst.octets(), 20) + .icmpv6(crate::Icmpv6Type::DestinationUnreachable(code)); let payload = original_packet.packet(); let actual_payload_len = std::cmp::min(payload.len(), MAX_ICMP_ERROR_PAYLOAD_LEN); @@ -267,7 +271,12 @@ mod tests { ) .unwrap(); - let icmp_error = icmp_dst_unreachable(&unreachable_packet).unwrap(); + let icmp_error = icmp_dest_unreachable( + &unreachable_packet, + icmpv4::DestUnreachableHeader::Network, + icmpv6::DestUnreachableCode::Address, + ) + .unwrap(); assert_eq!( icmp_error.destination(), @@ -293,7 +302,12 @@ mod tests { ) .unwrap(); - let icmp_error = icmp_dst_unreachable(&unreachable_packet).unwrap(); + let icmp_error = icmp_dest_unreachable( + &unreachable_packet, + icmpv4::DestUnreachableHeader::Network, + icmpv6::DestUnreachableCode::Address, + ) + .unwrap(); assert_eq!( icmp_error.destination(), diff --git a/rust/connlib/tunnel/src/gateway.rs b/rust/connlib/tunnel/src/gateway.rs index 776c3478e..575595239 100644 --- a/rust/connlib/tunnel/src/gateway.rs +++ b/rust/connlib/tunnel/src/gateway.rs @@ -163,7 +163,8 @@ impl GatewayState { .context("Failed to translate outbound packet")? { TranslateOutboundResult::Send(ip_packet) => Ok(Some(ip_packet)), - TranslateOutboundResult::DestinationUnreachable(reply) => { + TranslateOutboundResult::DestinationUnreachable(reply) + | TranslateOutboundResult::Filtered(reply) => { let Some(transmit) = encrypt_packet(reply, cid, &mut self.node, now)? else { return Ok(None); }; @@ -172,7 +173,6 @@ impl GatewayState { Ok(None) } - TranslateOutboundResult::Filtered => Ok(None), } } diff --git a/rust/connlib/tunnel/src/peer.rs b/rust/connlib/tunnel/src/peer.rs index ed47b9a3d..1117c88e0 100644 --- a/rust/connlib/tunnel/src/peer.rs +++ b/rust/connlib/tunnel/src/peer.rs @@ -276,47 +276,6 @@ impl ClientOnGateway { } } - fn transform_network_to_tun( - &mut self, - packet: IpPacket, - now: Instant, - ) -> anyhow::Result { - let dst = packet.destination(); - - // Packets to the TUN interface don't get transformed. - if self.gateway_tun.is_ip(dst) { - return Ok(TranslateOutboundResult::Send(packet)); - } - - // Packets for CIDR resources / Internet resource are forwarded as is. - if !is_dns_addr(dst) { - return Ok(TranslateOutboundResult::Send(packet)); - } - - let Some(state) = self.permanent_translations.get_mut(&packet.destination()) else { - return Ok(TranslateOutboundResult::DestinationUnreachable( - ip_packet::make::icmp_dst_unreachable(&packet)?, - )); - }; - - if state.resolved_ip.is_ipv4() != dst.is_ipv4() { - return Ok(TranslateOutboundResult::DestinationUnreachable( - ip_packet::make::icmp_dst_unreachable(&packet)?, - )); - } - - let (source_protocol, real_ip) = - self.nat_table - .translate_outgoing(&packet, state.resolved_ip, now)?; - - let mut packet = packet - .translate_destination(source_protocol, real_ip) - .context("Failed to translate packet to new destination")?; - packet.update_checksum(); - - Ok(TranslateOutboundResult::Send(packet)) - } - pub fn translate_outbound( &mut self, packet: IpPacket, @@ -325,7 +284,13 @@ impl ClientOnGateway { // Filtering a packet is not an error. if let Err(e) = self.ensure_allowed_src_and_dst(&packet) { tracing::debug!(filtered_packet = ?packet, "{e:#}"); - return Ok(TranslateOutboundResult::Filtered); + return Ok(TranslateOutboundResult::Filtered( + ip_packet::make::icmp_dest_unreachable( + &packet, + ip_packet::icmpv4::DestUnreachableHeader::FilterProhibited, + ip_packet::icmpv6::DestUnreachableCode::Prohibited, + )?, + )); } // Failing to transform is an error we want to know about further up. @@ -380,6 +345,55 @@ impl ClientOnGateway { Ok(Some(packet)) } + fn transform_network_to_tun( + &mut self, + packet: IpPacket, + now: Instant, + ) -> anyhow::Result { + let dst = packet.destination(); + + // Packets to the TUN interface don't get transformed. + if self.gateway_tun.is_ip(dst) { + return Ok(TranslateOutboundResult::Send(packet)); + } + + // Packets for CIDR resources / Internet resource are forwarded as is. + if !is_dns_addr(dst) { + return Ok(TranslateOutboundResult::Send(packet)); + } + + let Some(state) = self.permanent_translations.get_mut(&packet.destination()) else { + return Ok(TranslateOutboundResult::DestinationUnreachable( + ip_packet::make::icmp_dest_unreachable( + &packet, + ip_packet::icmpv4::DestUnreachableHeader::Network, + ip_packet::icmpv6::DestUnreachableCode::Address, + )?, + )); + }; + + if state.resolved_ip.is_ipv4() != dst.is_ipv4() { + return Ok(TranslateOutboundResult::DestinationUnreachable( + ip_packet::make::icmp_dest_unreachable( + &packet, + ip_packet::icmpv4::DestUnreachableHeader::Network, + ip_packet::icmpv6::DestUnreachableCode::Address, + )?, + )); + } + + let (source_protocol, real_ip) = + self.nat_table + .translate_outgoing(&packet, state.resolved_ip, now)?; + + let mut packet = packet + .translate_destination(source_protocol, real_ip) + .context("Failed to translate packet to new destination")?; + packet.update_checksum(); + + Ok(TranslateOutboundResult::Send(packet)) + } + fn transform_tun_to_network( &mut self, packet: IpPacket, @@ -483,7 +497,7 @@ impl ClientOnGateway { pub enum TranslateOutboundResult { Send(IpPacket), DestinationUnreachable(IpPacket), - Filtered, + Filtered(IpPacket), } impl GatewayOnClient { @@ -874,7 +888,7 @@ mod tests { assert!(matches!( peer.translate_outbound(pkt, Instant::now()).unwrap(), - crate::peer::TranslateOutboundResult::Filtered + crate::peer::TranslateOutboundResult::Filtered(_) )); let pkt = ip_packet::make::udp_packet( @@ -888,7 +902,7 @@ mod tests { assert!(matches!( peer.translate_outbound(pkt, Instant::now()).unwrap(), - crate::peer::TranslateOutboundResult::Filtered + crate::peer::TranslateOutboundResult::Filtered(_) )); let pkt = ip_packet::make::udp_packet( @@ -938,7 +952,7 @@ mod tests { assert!(matches!( peer.translate_outbound(pkt, Instant::now()).unwrap(), - crate::peer::TranslateOutboundResult::Filtered + crate::peer::TranslateOutboundResult::Filtered(_) )); let pkt = ip_packet::make::udp_packet( diff --git a/rust/connlib/tunnel/src/tests/reference.rs b/rust/connlib/tunnel/src/tests/reference.rs index fbc80ba1c..4dfb44e3d 100644 --- a/rust/connlib/tunnel/src/tests/reference.rs +++ b/rust/connlib/tunnel/src/tests/reference.rs @@ -394,14 +394,13 @@ impl ReferenceState { } } Transition::SendIcmpPacket { - src, dst, seq, identifier, payload, + .. } => state.client.exec_mut(|client| { client.on_icmp_packet( - *src, dst.clone(), *seq, *identifier, @@ -411,15 +410,14 @@ impl ReferenceState { ) }), Transition::SendUdpPacket { - src, dst, sport, dport, payload, + .. } => { state.client.exec_mut(|client| { client.on_udp_packet( - *src, dst.clone(), *sport, *dport, @@ -430,15 +428,14 @@ impl ReferenceState { }); } Transition::SendTcpPayload { - src, dst, sport, dport, payload, + .. } => { state.client.exec_mut(|client| { client.on_tcp_packet( - *src, dst.clone(), *sport, *dport, diff --git a/rust/connlib/tunnel/src/tests/sim_client.rs b/rust/connlib/tunnel/src/tests/sim_client.rs index da865bdd2..97f91a429 100644 --- a/rust/connlib/tunnel/src/tests/sim_client.rs +++ b/rust/connlib/tunnel/src/tests/sim_client.rs @@ -597,13 +597,6 @@ impl RefClient { .collect() } - pub(crate) fn is_tunnel_ip(&self, ip: IpAddr) -> bool { - match ip { - IpAddr::V4(ip4) => self.tunnel_ip4 == ip4, - IpAddr::V6(ip6) => self.tunnel_ip6 == ip6, - } - } - pub(crate) fn tunnel_ip_for(&self, dst: IpAddr) -> IpAddr { match dst { IpAddr::V4(_) => self.tunnel_ip4.into(), @@ -611,10 +604,8 @@ impl RefClient { } } - #[expect(clippy::too_many_arguments, reason = "We don't care.")] pub(crate) fn on_icmp_packet( &mut self, - src: IpAddr, dst: Destination, seq: Seq, identifier: Identifier, @@ -623,7 +614,6 @@ impl RefClient { gateway_by_ip: impl Fn(IpAddr) -> Option, ) { self.on_packet( - src, dst.clone(), (dst, seq, identifier), |ref_client| &mut ref_client.expected_icmp_handshakes, @@ -633,10 +623,8 @@ impl RefClient { ); } - #[expect(clippy::too_many_arguments, reason = "We don't care.")] pub(crate) fn on_udp_packet( &mut self, - src: IpAddr, dst: Destination, sport: SPort, dport: DPort, @@ -645,7 +633,6 @@ impl RefClient { gateway_by_ip: impl Fn(IpAddr) -> Option, ) { self.on_packet( - src, dst.clone(), (dst, sport, dport), |ref_client| &mut ref_client.expected_udp_handshakes, @@ -655,10 +642,8 @@ impl RefClient { ); } - #[expect(clippy::too_many_arguments, reason = "We don't care.")] pub(crate) fn on_tcp_packet( &mut self, - src: IpAddr, dst: Destination, sport: SPort, dport: DPort, @@ -667,7 +652,6 @@ impl RefClient { gateway_by_ip: impl Fn(IpAddr) -> Option, ) { self.on_packet( - src, dst.clone(), (dst, sport, dport), |ref_client| &mut ref_client.expected_tcp_exchanges, @@ -677,11 +661,9 @@ impl RefClient { ); } - #[expect(clippy::too_many_arguments, reason = "We don't care.")] #[tracing::instrument(level = "debug", skip_all, fields(dst, resource, gateway))] fn on_packet( &mut self, - src: IpAddr, dst: Destination, packet_id: E, map: impl FnOnce(&mut Self) -> &mut BTreeMap>, @@ -718,10 +700,6 @@ impl RefClient { gateway }; - if !self.is_tunnel_ip(src) { - return; - } - tracing::debug!(%payload, "Sending packet"); map(self) diff --git a/website/src/components/Changelog/Gateway.tsx b/website/src/components/Changelog/Gateway.tsx index 87a64e8a3..1d1d2a2dd 100644 --- a/website/src/components/Changelog/Gateway.tsx +++ b/website/src/components/Changelog/Gateway.tsx @@ -27,6 +27,9 @@ export default function Gateway() { Excludes ICMP errors from the ICMP traffic filter. Those are now always routed back to the client. + + Responds with ICMP errors for filtered packets. +