Skip to content

fix(gateway): translate ICMP destination unreachable errors#7398

Merged
thomaseizinger merged 10 commits into
mainfrom
fix/gateway/translate-icmp-errors
Dec 2, 2024
Merged

fix(gateway): translate ICMP destination unreachable errors#7398
thomaseizinger merged 10 commits into
mainfrom
fix/gateway/translate-icmp-errors

Conversation

@thomaseizinger

@thomaseizinger thomaseizinger commented Nov 26, 2024

Copy link
Copy Markdown
Member

Context

The Gateway implements a stateful NAT that translates the destination IP and source protocol of every packet that targets a DNS resource IP. This is necessary because the IPs for DNS resources are generated on the client without actually performing a DNS lookup, instead it always generates 4 IPv4 and 4 IPv6 addresses. On the Gateway, these IPs are then assigned in a round-robin fashion to the actual IPs that the domain resolves to, necessitating a NAT64/46 translation in case a domain only resolves to IPs of one family.

A domain may resolve to a set of IPs but not all of these IPs may be routable. Whilst an arguably poor practise of the domain administrator, routing problems can occur for all kinds of reasons and are well handled on the wider Internet.

When an IP packet cannot be routed further, the current routing node generates an ICMP error describing the routing failure and sends it back to the original sender. ICMP is a layer 4 protocol itself, same as TCP and UDP. As such, sending out a UDP packet may result in receiving an ICMP response. In order to allow the sender to learn, which packet failed to route, the ICMP error embeds parts of the original packet in its payload 0 1.

The Gateway's NAT table uses parts of the layer 4 protocol as part of its key; the UDP and TCP source port and the ICMP echo request identifier (further referred to as "source protocol"). An ICMP error message doesn't have any of these, meaning the lookup in the NAT table currently fails and the ICMP error is silently dropped.

A lot of software implements a happy-eyeballs approach and probs for IPv6 and IPv4 connectivity simulataneously. The absence of the ICMP errors confuses that algorithm as it detects the packet loss and starts retransmits instead of giving up.

Solution

Upon receiving an ICMP error on the Gateway, we now extract the partially embedded packet in the ICMP error payload. We use the destination IP and source protocol of that packet for the lookup in the NAT table. This returns us the original (client-assigned) destination IP and source protocol. In order for the Gateway's NAT to be transparent, we need to patch the packet embedded in the ICMP error to use the original destination and source protocol. We also have to account for the fact that the original packet may have been translated with NAT64/46 and translate it back. Finally, we generate an ICMP error with the appropriate code and embed the patched packet in its payload.

Test implementation

To test that this works for all kind of combinations, we extend tunnel_test to sample a list of unreachable IPs from all IPs sampled for DNS resources. Upon receiving a packet for one of these IPs, the Gateway will send an ICMP error back instead of invoking its regular echo reply logic. On the client-side, upon receiving an ICMP error, we extract the originally failed packet from the body and treat it as a successful response.

This may seem a bit hacky at first but is actually how operating systems would treat ICMP errors as well. For example, a TcpSocket::connect call (triggering a TCP SYN packet) may fail with an IO error if we receive an ICMP error packet. Thus, in a way, the original packet got answered, just not with what we expected.

In addition, by treating these ICMP errors as responses to the original packet, we automatically perform other assertions on them, like ensuring that they come from the right IP address, that there are no unexpected packets etc.

Test alternatives

It is tricky to solve this in other ways in the test suite because at the time of generating a packet for a DNS resource, we don't know the actual IP that is being targeted by a certain proxy IP unless we'd start reimplementing the round-robin algorithm employed by the Gateway. To "test" the transparency of the NAT, we'd like to avoid knowing about these implementation details in the test.

Future work

In this PR, we currently only deal with "Destination Unreachable" ICMP errors. There are other ICMP messages such as ICMPv6's PacketTooBig or ParameterProblem. We should eventually handle these as well. They are being deferred because translating those between the different IP versions is only partially implemented and would thus require more work. The most pressing need is to translate destination unreachable errors to enable happy-eyeballs algorithms to work correctly.

Resolves: #5614.
Resolves: #6371.

@vercel

vercel Bot commented Nov 26, 2024

Copy link
Copy Markdown

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
firezone ✅ Ready (Inspect) Visit Preview 💬 Add feedback Dec 2, 2024 10:55pm

@thomaseizinger thomaseizinger changed the title feat(gateway): translate ICMP errors back to client feat(gateway): translate ICMP dst unreachable errors back to client Nov 26, 2024
@thomaseizinger thomaseizinger changed the title feat(gateway): translate ICMP dst unreachable errors back to client fix(gateway): translate ICMP dst unreachable errors back to client Nov 26, 2024
@thomaseizinger thomaseizinger changed the title fix(gateway): translate ICMP dst unreachable errors back to client fix(gateway): translate ICMP errors back to client Nov 27, 2024
@thomaseizinger thomaseizinger changed the title fix(gateway): translate ICMP errors back to client fix(gateway): translate ICMP dst unreachble errors back to client Nov 27, 2024
@thomaseizinger thomaseizinger changed the title fix(gateway): translate ICMP dst unreachble errors back to client fix(gateway): translate ICMP destination unreachable errors Nov 27, 2024
@thomaseizinger thomaseizinger force-pushed the fix/gateway/translate-icmp-errors branch from 63dd240 to 59e542e Compare November 27, 2024 00:42
@thomaseizinger thomaseizinger marked this pull request as ready for review November 27, 2024 00:42
@thomaseizinger thomaseizinger force-pushed the fix/gateway/translate-icmp-errors branch from 59e542e to b157def Compare November 27, 2024 00:46
@thomaseizinger

thomaseizinger commented Nov 27, 2024

Copy link
Copy Markdown
Member Author

@jamilbk This is ready for review. The commits are atomic so you should be able to go through them one-by-one.

I also ran 50_000 test cases of this locally.

@jamilbk jamilbk left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good and makes sense. Nice work on this one!

Left some non-blocking comments / questions.

Comment on lines +157 to +160
/// Due to our NAT64/64 implementation, the ICMP error that we receive on the Gateway may not be what we want to forward to the client.
/// For example, in case we translate a TCP-SYN from IPv4 to IPv6 but the IPv6 address is unreachable, we need to:
/// - Translate the failed packet embedded in the ICMP error back to an IPv4 packet.
/// - Send an ICMPv4 error instead of an ICMPv6 error.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🫠

match res {
TranslateIncomingResult::Ok { proto, src } => (proto, src),
TranslateIncomingResult::NoNatSession
| TranslateIncomingResult::DestinationUnreachable(_) => panic!("Wrong result"),

@jamilbk jamilbk Dec 2, 2024

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is a panic the right approach here? I remember we were trying to avoid panicking in the packet processing paths.

Is this what we reach if the original packet is itself an ICMP destination error?

@thomaseizinger thomaseizinger Dec 2, 2024

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is just a unit-test where we never meant to see DST unreachable.

Comment thread website/src/components/Changelog/Gateway.tsx Outdated
@thomaseizinger thomaseizinger force-pushed the fix/gateway/translate-icmp-errors branch from 81a1559 to 7d824a6 Compare December 2, 2024 22:52
@thomaseizinger thomaseizinger added this pull request to the merge queue Dec 2, 2024
Merged via the queue into main with commit 9073bdd Dec 2, 2024
@thomaseizinger thomaseizinger deleted the fix/gateway/translate-icmp-errors branch December 2, 2024 23:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Resolve A or AAAA depending on Gateway's IPv4 and IPv6 connectivity gateway: allow NATing ICMP messages other than echo requests & replies

2 participants