From 3226d434c2b73be3774bbc3dd00acdb3fb9603c4 Mon Sep 17 00:00:00 2001 From: Thomas Eizinger Date: Thu, 27 Nov 2025 15:25:51 +1100 Subject: [PATCH] chore(connlib): clarify `Protocol::Icmp` as `IcmpEcho` (#10988) Firezone's DNS resource NAT can only route certain ICMP packets. In particular, we can only route ICMP _echo_ packets as only those have a stable identifier that we can use in our NAT table. To clarify that, rename the enum variant to `IcmpEcho` from `Icmp`. --- rust/libs/connlib/ip-packet/src/icmp_error.rs | 8 ++++---- rust/libs/connlib/ip-packet/src/lib.rs | 18 +++++++++--------- .../tunnel/src/gateway/filter_engine.rs | 4 ++-- .../connlib/tunnel/src/gateway/flow_tracker.rs | 4 ++-- .../connlib/tunnel/src/gateway/nat_table.rs | 4 ++-- .../tunnel/src/gateway/unroutable_packet.rs | 2 +- 6 files changed, 20 insertions(+), 20 deletions(-) diff --git a/rust/libs/connlib/ip-packet/src/icmp_error.rs b/rust/libs/connlib/ip-packet/src/icmp_error.rs index 79680991c..0031d2660 100644 --- a/rust/libs/connlib/ip-packet/src/icmp_error.rs +++ b/rust/libs/connlib/ip-packet/src/icmp_error.rs @@ -84,7 +84,7 @@ impl FailedPacket { match self.l4_proto { Layer4Protocol::Udp { src, .. } => Protocol::Udp(src), Layer4Protocol::Tcp { src, .. } => Protocol::Tcp(src), - Layer4Protocol::Icmp { id, .. } => Protocol::Icmp(id), + Layer4Protocol::Icmp { id, .. } => Protocol::IcmpEcho(id), } } @@ -181,9 +181,9 @@ fn translate_original_packet_protocol( inside_proto: Protocol, ) { let proto_offset = match inside_proto { - Protocol::Tcp(_) => 0, // source port is the first thing in a TCP packet. - Protocol::Udp(_) => 0, // source port is the first thing in a UDP packet. - Protocol::Icmp(_) => 4, // icmp identifier comes after type, code and checksum. + Protocol::Tcp(_) => 0, // source port is the first thing in a TCP packet. + Protocol::Udp(_) => 0, // source port is the first thing in a UDP packet. + Protocol::IcmpEcho(_) => 4, // icmp identifier comes after type, code and checksum. }; let proto_index = payload_start + proto_offset; diff --git a/rust/libs/connlib/ip-packet/src/lib.rs b/rust/libs/connlib/ip-packet/src/lib.rs index 88988196d..d4ae94a1a 100644 --- a/rust/libs/connlib/ip-packet/src/lib.rs +++ b/rust/libs/connlib/ip-packet/src/lib.rs @@ -61,8 +61,8 @@ pub enum Protocol { Tcp(u16), /// Contains either the source or destination port. Udp(u16), - /// Contains the `identifier` of the ICMP packet. - Icmp(u16), + /// Contains the `identifier` of the ICMP echo packet. + IcmpEcho(u16), } impl Protocol { @@ -71,7 +71,7 @@ impl Protocol { (self, other), (Protocol::Tcp(_), Protocol::Tcp(_)) | (Protocol::Udp(_), Protocol::Udp(_)) - | (Protocol::Icmp(_), Protocol::Icmp(_)) + | (Protocol::IcmpEcho(_), Protocol::IcmpEcho(_)) ) } @@ -79,7 +79,7 @@ impl Protocol { match self { Protocol::Tcp(v) => *v, Protocol::Udp(v) => *v, - Protocol::Icmp(v) => *v, + Protocol::IcmpEcho(v) => *v, } } @@ -87,7 +87,7 @@ impl Protocol { match self { Protocol::Tcp(_) => Protocol::Tcp(value), Protocol::Udp(_) => Protocol::Udp(value), - Protocol::Icmp(_) => Protocol::Icmp(value), + Protocol::IcmpEcho(_) => Protocol::IcmpEcho(value), } } } @@ -305,7 +305,7 @@ impl IpPacket { .ok_or_else(|| UnsupportedProtocol::UnsupportedIcmpv4Type(p.icmp_type()))? .id; - return Ok(Protocol::Icmp(id)); + return Ok(Protocol::IcmpEcho(id)); } if let Some(p) = self.as_icmpv6() { @@ -314,7 +314,7 @@ impl IpPacket { .ok_or_else(|| UnsupportedProtocol::UnsupportedIcmpv6Type(p.icmp_type()))? .id; - return Ok(Protocol::Icmp(id)); + return Ok(Protocol::IcmpEcho(id)); } Err(UnsupportedProtocol::UnsupportedIpPayload( @@ -337,7 +337,7 @@ impl IpPacket { .ok_or_else(|| UnsupportedProtocol::UnsupportedIcmpv4Type(p.icmp_type()))? .id; - return Ok(Protocol::Icmp(id)); + return Ok(Protocol::IcmpEcho(id)); } if let Some(p) = self.as_icmpv6() { @@ -346,7 +346,7 @@ impl IpPacket { .ok_or_else(|| UnsupportedProtocol::UnsupportedIcmpv6Type(p.icmp_type()))? .id; - return Ok(Protocol::Icmp(id)); + return Ok(Protocol::IcmpEcho(id)); } Err(UnsupportedProtocol::UnsupportedIpPayload( diff --git a/rust/libs/connlib/tunnel/src/gateway/filter_engine.rs b/rust/libs/connlib/tunnel/src/gateway/filter_engine.rs index d5d9188f1..d38db5b24 100644 --- a/rust/libs/connlib/tunnel/src/gateway/filter_engine.rs +++ b/rust/libs/connlib/tunnel/src/gateway/filter_engine.rs @@ -67,7 +67,7 @@ impl AllowRules { match protocol { Ok(Protocol::Tcp(port)) if self.tcp.contains(&port) => Ok(()), Ok(Protocol::Udp(port)) if self.udp.contains(&port) => Ok(()), - Ok(Protocol::Icmp(_)) if self.icmp => Ok(()), + Ok(Protocol::IcmpEcho(_)) if self.icmp => Ok(()), // If ICMP is allowed, we don't care about the specific ICMP type. // i.e. it doesn't have to be an echo request / reply. @@ -78,7 +78,7 @@ impl AllowRules { Ok(Protocol::Tcp(_)) => Err(Filtered::Tcp), Ok(Protocol::Udp(_)) => Err(Filtered::Udp), - Ok(Protocol::Icmp(_)) => Err(Filtered::Icmp), + Ok(Protocol::IcmpEcho(_)) => Err(Filtered::Icmp), Err(e) => Err(Filtered::UnsupportedProtocol(e)), } diff --git a/rust/libs/connlib/tunnel/src/gateway/flow_tracker.rs b/rust/libs/connlib/tunnel/src/gateway/flow_tracker.rs index 8d54ac6dc..5f3f28850 100644 --- a/rust/libs/connlib/tunnel/src/gateway/flow_tracker.rs +++ b/rust/libs/connlib/tunnel/src/gateway/flow_tracker.rs @@ -414,7 +414,7 @@ impl FlowTracker { } }; } - (Protocol::Icmp(_), Protocol::Icmp(_)) => {} + (Protocol::IcmpEcho(_), Protocol::IcmpEcho(_)) => {} _ => { tracing::error!("src and dst protocol must be the same"); } @@ -508,7 +508,7 @@ impl FlowTracker { } }; } - (Protocol::Icmp(_), Protocol::Icmp(_)) => {} + (Protocol::IcmpEcho(_), Protocol::IcmpEcho(_)) => {} _ => { tracing::error!("src and dst protocol must be the same"); } diff --git a/rust/libs/connlib/tunnel/src/gateway/nat_table.rs b/rust/libs/connlib/tunnel/src/gateway/nat_table.rs index f06a10b91..fc29cc985 100644 --- a/rust/libs/connlib/tunnel/src/gateway/nat_table.rs +++ b/rust/libs/connlib/tunnel/src/gateway/nat_table.rs @@ -218,7 +218,7 @@ impl EntryState { let ttl = match protocol { Protocol::Tcp(_) => TCP_TTL, Protocol::Udp(_) => UDP_TTL, - Protocol::Icmp(_) => ICMP_TTL, + Protocol::IcmpEcho(_) => ICMP_TTL, }; self.last_packet() + ttl @@ -381,7 +381,7 @@ mod tests { let ttl = match src { Protocol::Tcp(_) => 7200, Protocol::Udp(_) => 120, - Protocol::Icmp(_) => 120, + Protocol::IcmpEcho(_) => 120, }; // Assert diff --git a/rust/libs/connlib/tunnel/src/gateway/unroutable_packet.rs b/rust/libs/connlib/tunnel/src/gateway/unroutable_packet.rs index 7ca0012a5..d9f07f33b 100644 --- a/rust/libs/connlib/tunnel/src/gateway/unroutable_packet.rs +++ b/rust/libs/connlib/tunnel/src/gateway/unroutable_packet.rs @@ -119,7 +119,7 @@ impl FiveTuple { dst: MaybeIpOrSocket::Socket(SocketAddr::new(dst_ip, dst_port)), proto: MaybeProto::Udp, }, - (Ok(Protocol::Icmp(_)), Ok(Protocol::Icmp(_))) => Self { + (Ok(Protocol::IcmpEcho(_)), Ok(Protocol::IcmpEcho(_))) => Self { src: MaybeIpOrSocket::Ip(src_ip), dst: MaybeIpOrSocket::Ip(dst_ip), proto: MaybeProto::Icmp,