Repository navigation
feat(gateway): don't apply traffic filters to ICMP errors - #9834
Conversation
In case the allow-rules do get called with an unsupported ICMP type, we still want to do the right thing and not filter them.
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
There was a problem hiding this comment.
Pull Request Overview
Adds special handling so that ICMP error packets are not blocked by existing traffic filters.
- Extend the
AllowRules::applymethod to treat unsupported ICMPv4/v6 types as allowed when ICMP is enabled. - Introduce an early return in
ClientOnGatewayto always pass through ICMP unreachable error packets before checking resource filters.
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| rust/connlib/tunnel/src/peer/filter_engine.rs | Added match arms to accept UnsupportedIcmpv4Type and UnsupportedIcmpv6Type under self.icmp. |
| rust/connlib/tunnel/src/peer.rs | Inserted an early-return block to bypass resource filtering for ICMP unreachable error packets. |
Comments suppressed due to low confidence (3)
rust/connlib/tunnel/src/peer/filter_engine.rs:77
- Add a unit test to verify that both
UnsupportedIcmpv4TypeandUnsupportedIcmpv6Typebranches returnOk(())whenself.icmpis true to prevent regressions.
) if self.icmp => Ok(()),
rust/connlib/tunnel/src/peer.rs:355
- [nitpick] Clarify in the comment that this bypass specifically applies to ICMP "unreachable" error messages rather than all ICMP traffic.
// Always allow ICMP errors to pass through, even in the presence of filters that don't allow ICMP.
rust/connlib/tunnel/src/peer.rs:356
- Add a test covering the early-return path when
icmp_unreachable_destination()yieldsSome, ensuring that unreachable errors bypass filters as intended.
if packet
| // 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. | ||
| Err( | ||
| UnsupportedProtocol::UnsupportedIcmpv4Type(_) | ||
| | UnsupportedProtocol::UnsupportedIcmpv6Type(_), | ||
| ) if self.icmp => Ok(()), |
There was a problem hiding this comment.
This is a defense-in-depth kind of change: The other change ensures we don't actually call this filter at all for ICMP errors. Yet, I still opted to fix this here to ensure it is correct in isolation, i.e. IF we apply the filter, we allow any kind of ICMP message, not just echo requests and replies.
This is as correct as we can be in here. We don't know whether the entire packet is an ICMP error or not so from the perspective of this component, all we know is whether we should allow ICMP traffic.
6d7de83 to
012bb69
Compare
|
I am not sure it worth going this deep on ICMP traffic filters. We have not gotten any complaints on usability there. Is there a GitHub issue or customer ask for these? Since this is technically a change in how traffic filters work (and due to the security-related nature), we probably should send an announcement and bump the Gateway minor if we decide to roll this out. |
That said, I do agree with the bugfix nature of this PR in that we should not be filtering out ICMP messages that we ourselves generate (i.e. the source address is our tunnel address). |
|
|
||
| // Always allow ICMP errors to pass through, even in the presence of filters that don't allow ICMP. | ||
| if packet | ||
| .icmp_unreachable_destination() |
| title="Allow ICMP traffic" | ||
| title="Allow ICMP echo requests/replies" | ||
| type="checkbox" | ||
| field={@forms_by_protocol[:icmp]} | ||
| name={"#{@form.name}[icmp][enabled]"} | ||
| checked={Map.has_key?(@forms_by_protocol, :icmp)} | ||
| value="true" | ||
| disabled={!@traffic_filters_enabled?} | ||
| label="ICMP" | ||
| label="ICMP echo" |
There was a problem hiding this comment.
@jamilbk Just making sure you've seen these changes in here?
There was a problem hiding this comment.
Yeah thanks for adding that. I think that's probably a good way to summarize the effect this change has.
Firezone uses ICMP errors to signal to client applications that e.g. a certain IP is not reachable. This happens for example if a DNS resource only resolves to IPv4 addresses yet the client application attempted to use an IPv6 proxy address to connect to it.
In the presence of traffic filters for such a resource that does not allow ICMP, we currently filter out these ICMP errors because - well - ICMP traffic is not allowed! However, even in the presence of ICMP traffic being allowed, we would fail to evaluate this filter because the ICMP error packet is not an ICMP echo reply and therefore doesn't have an ICMP identifier. We require this in the DNS resource NAT to identify "connections" and NAT them correctly. The same L4 component is used to evaluate the traffic filters.
ICMP errors are critical to many usage scenarios and algorithms like happy-eyeballs. Dropping them usually results in weird behaviour as client applications can then only react to timeouts.