From 19c5bc530a5231957e8e708c320d273262af4903 Mon Sep 17 00:00:00 2001 From: Thomas Eizinger Date: Thu, 27 Mar 2025 12:01:37 +1100 Subject: [PATCH] feat(gateway): deprecate the NAT64 module (#8383) At present, the Gateway implements a NAT64 conversion that can convert IPv4 packets to IPv6 and vice versa. Doing this efficiently creates a fair amount of complexity within our `ip-packet` crate. In addition, routing ICMP errors back through our NAT is also complicated by this because we may have to translate the packet embedded in the ICMP error as well. The NAT64 module was originally conceived as a result of the new stub resolver-based DNS architecture. When the Client resolves IPs for a domain, it doesn't know whether the domain will actually resolve to IPv4 AND IPv6 addresses so it simply assigns 4 of each to every domain. Thus, when receiving an IPv6 packet for such a DNS resource, the Gateway may only have IPv4 addresses available and can therefore not route the packet (unless it translates it). This problem is not novel. In fact, an IP being unroutable or a particular route disappearing happens all the time on the Internet. ICMP was conceived to handle this problem and it is doing a pretty good job at it. We can make use of that and simply return an ICMP unreachable error back to the client whenever it picks an IP that we cannot map to one that we resolved. In this PR, we leave all of the NAT64 code intact and only add a feature-flag that - when active - sends aforementioned ICMP error. While offline (and thus also for our tests), the feature-flag evaluates to false. It is however set to `true` in the backend, meaning on staging and later in production, we will send these ICMP errors. Once this is rolled out and indeed proving to be working as intended, we can simplify our codebase and rip out the NAT64 module. At that point, we will also have to adapt the test-suite. --- rust/connlib/tunnel/src/gateway.rs | 18 ++- rust/connlib/tunnel/src/peer.rs | 127 ++++++++++++++----- rust/ip-packet/src/make.rs | 2 +- website/src/components/Changelog/Gateway.tsx | 6 +- 4 files changed, 115 insertions(+), 38 deletions(-) diff --git a/rust/connlib/tunnel/src/gateway.rs b/rust/connlib/tunnel/src/gateway.rs index 9617480d0..e1fccf151 100644 --- a/rust/connlib/tunnel/src/gateway.rs +++ b/rust/connlib/tunnel/src/gateway.rs @@ -1,5 +1,6 @@ use crate::messages::gateway::ResourceDescription; use crate::messages::{Answer, IceCredentials, ResolveRequest, SecretKey}; +use crate::peer::TranslateOutboundResult; use crate::utils::earliest; use crate::{GatewayEvent, IpConfig, p2p_control}; use crate::{peer::ClientOnGateway, peer_store::PeerStore}; @@ -157,11 +158,22 @@ impl GatewayState { return Ok(None); } - let packet = peer + match peer .translate_outbound(packet, now) - .context("Failed to translate outbound packet")?; + .context("Failed to translate outbound packet")? + { + TranslateOutboundResult::Send(ip_packet) => Ok(Some(ip_packet)), + TranslateOutboundResult::DestinationUnreachable(reply) => { + let Some(transmit) = encrypt_packet(reply, cid, &mut self.node, now)? else { + return Ok(None); + }; - Ok(packet) + self.buffered_transmits.push_back(transmit); + + Ok(None) + } + TranslateOutboundResult::Filtered => Ok(None), + } } pub fn cleanup_connection(&mut self, id: &ClientId) { diff --git a/rust/connlib/tunnel/src/peer.rs b/rust/connlib/tunnel/src/peer.rs index 2c83982e3..d60a9888b 100644 --- a/rust/connlib/tunnel/src/peer.rs +++ b/rust/connlib/tunnel/src/peer.rs @@ -1,6 +1,6 @@ use std::collections::{BTreeMap, BTreeSet, HashMap, HashSet, VecDeque, hash_map}; use std::iter; -use std::net::{IpAddr, SocketAddr}; +use std::net::{IpAddr, Ipv4Addr, Ipv6Addr, SocketAddr}; use std::time::Instant; use crate::client::{IPV4_RESOURCES, IPV6_RESOURCES}; @@ -12,7 +12,7 @@ use dns_types::DomainName; use filter_engine::FilterEngine; use ip_network::{IpNetwork, Ipv4Network, Ipv6Network}; use ip_network_table::IpNetworkTable; -use ip_packet::{IpPacket, Protocol, UnsupportedProtocol}; +use ip_packet::{IpPacket, PacketBuilder, Protocol, UnsupportedProtocol, icmpv4, icmpv6}; use crate::utils::network_contains_network; use crate::{GatewayEvent, IpConfig}; @@ -272,16 +272,30 @@ impl ClientOnGateway { &mut self, packet: IpPacket, now: Instant, - ) -> anyhow::Result { + ) -> anyhow::Result { + let dst = packet.destination(); + // Packets to the TUN interface don't get transformed. - if self.gateway_tun.is_ip(packet.destination()) { - return Ok(packet); + 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(packet); + return self.dst_unreachable(&packet); }; + #[expect(clippy::collapsible_if, reason = "We want the feature flag separate.")] + if firezone_telemetry::feature_flags::icmp_unreachable_instead_of_nat64() { + if state.resolved_ip.is_ipv4() != dst.is_ipv4() { + return self.dst_unreachable(&packet); + } + } + let (source_protocol, real_ip) = self.nat_table .translate_outgoing(&packet, state.resolved_ip, now)?; @@ -296,24 +310,24 @@ impl ClientOnGateway { .context("Failed to translate packet to new destination")?; packet.update_checksum(); - Ok(packet) + Ok(TranslateOutboundResult::Send(packet)) } pub fn translate_outbound( &mut self, packet: IpPacket, now: Instant, - ) -> anyhow::Result> { + ) -> anyhow::Result { // 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(None); + return Ok(TranslateOutboundResult::Filtered); } // Failing to transform is an error we want to know about further up. - let packet = self.transform_network_to_tun(packet, now)?; + let result = self.transform_network_to_tun(packet, now)?; - Ok(Some(packet)) + Ok(result) } pub fn translate_inbound( @@ -433,6 +447,54 @@ impl ClientOnGateway { pub fn id(&self) -> ClientId { self.id } + + fn dst_unreachable(&self, packet: &IpPacket) -> Result { + let src = packet.source(); + + let icmp_error = match src { + IpAddr::V4(src) => icmpv4_network_unreachable(self.gateway_tun.v4, src, packet)?, + IpAddr::V6(src) => icmpv6_address_unreachable(self.gateway_tun.v6, src, packet)?, + }; + + Ok(TranslateOutboundResult::DestinationUnreachable(icmp_error)) + } +} + +fn icmpv4_network_unreachable( + src: Ipv4Addr, + dst: Ipv4Addr, + original_packet: &IpPacket, +) -> Result { + let builder = PacketBuilder::ipv4(src.octets(), dst.octets(), 20).icmpv4( + ip_packet::Icmpv4Type::DestinationUnreachable(icmpv4::DestUnreachableHeader::Network), + ); + let payload = original_packet.packet(); + + let ip_packet = ip_packet::build!(builder, payload)?; + + Ok(ip_packet) +} + +fn icmpv6_address_unreachable( + src: Ipv6Addr, + dst: Ipv6Addr, + original_packet: &IpPacket, +) -> Result { + let builder = PacketBuilder::ipv6(src.octets(), dst.octets(), 20).icmpv6( + ip_packet::Icmpv6Type::DestinationUnreachable(icmpv6::DestUnreachableCode::Address), + ); + let payload = original_packet.packet(); + + let ip_packet = ip_packet::build!(builder, payload)?; + + Ok(ip_packet) +} + +#[derive(Debug, PartialEq)] +pub enum TranslateOutboundResult { + Send(IpPacket), + DestinationUnreachable(IpPacket), + Filtered, } impl GatewayOnClient { @@ -640,7 +702,7 @@ mod tests { use crate::{ IpConfig, messages::gateway::{Filter, PortRange, ResourceDescription, ResourceDescriptionCidr}, - peer::nat_table, + peer::{TranslateOutboundResult, nat_table}, }; use chrono::Utc; use connlib_model::{ClientId, ResourceId}; @@ -771,11 +833,10 @@ mod tests { ) .unwrap(); - assert!( - peer.translate_outbound(request, Instant::now()) - .unwrap() - .is_some() - ); + assert!(matches!( + peer.translate_outbound(request, Instant::now()).unwrap(), + crate::peer::TranslateOutboundResult::Send(_) + )); assert!( peer.translate_inbound(response, Instant::now()) .unwrap() @@ -818,11 +879,10 @@ mod tests { ) .unwrap(); - assert!( - peer.translate_outbound(pkt, Instant::now()) - .unwrap() - .is_none() - ); + assert!(matches!( + peer.translate_outbound(pkt, Instant::now()).unwrap(), + crate::peer::TranslateOutboundResult::Filtered + )); let pkt = ip_packet::make::udp_packet( client_tun_ipv4(), @@ -833,11 +893,10 @@ mod tests { ) .unwrap(); - assert!( - peer.translate_outbound(pkt, Instant::now()) - .unwrap() - .is_none() - ); + assert!(matches!( + peer.translate_outbound(pkt, Instant::now()).unwrap(), + crate::peer::TranslateOutboundResult::Filtered + )); let pkt = ip_packet::make::udp_packet( client_tun_ipv4(), @@ -884,11 +943,10 @@ mod tests { ) .unwrap(); - assert!( - peer.translate_outbound(pkt, Instant::now()) - .unwrap() - .is_none() - ); + assert!(matches!( + peer.translate_outbound(pkt, Instant::now()).unwrap(), + crate::peer::TranslateOutboundResult::Filtered + )); let pkt = ip_packet::make::udp_packet( client_tun_ipv4(), @@ -927,7 +985,10 @@ mod tests { let mut now = Instant::now(); - assert!(matches!(peer.translate_outbound(request, now), Ok(Some(_)))); + assert!(matches!( + peer.translate_outbound(request, now), + Ok(TranslateOutboundResult::Send(_)) + )); let response = ip_packet::make::udp_packet( foo_real_ip(), diff --git a/rust/ip-packet/src/make.rs b/rust/ip-packet/src/make.rs index cbbcde5ca..57f61f190 100644 --- a/rust/ip-packet/src/make.rs +++ b/rust/ip-packet/src/make.rs @@ -20,7 +20,7 @@ macro_rules! build { let packet = IpPacket::new(ip, size).context("Failed to create IP packet")?; - Ok(packet) + ::anyhow::Ok(packet) }}; } diff --git a/website/src/components/Changelog/Gateway.tsx b/website/src/components/Changelog/Gateway.tsx index 5ec3a882c..3c5d68082 100644 --- a/website/src/components/Changelog/Gateway.tsx +++ b/website/src/components/Changelog/Gateway.tsx @@ -22,7 +22,11 @@ export default function Gateway() { return ( - + + + Deprecates the NAT64 functionality in favor of sending ICMP errors. + + Fixes a bug in the routing of DNS resources that would lead to "Source