Skip to content

feat(gateway): vary DNS resource NAT TTL by protocol#9655

Merged
thomaseizinger merged 6 commits into
mainfrom
feat/dns-nat-mapping-2-hours
Jun 25, 2025
Merged

feat(gateway): vary DNS resource NAT TTL by protocol#9655
thomaseizinger merged 6 commits into
mainfrom
feat/dns-nat-mapping-2-hours

Conversation

@thomaseizinger

@thomaseizinger thomaseizinger commented Jun 24, 2025

Copy link
Copy Markdown
Member

Instead of a 1 minute TTL for all connections, we vary the TTL based on the protocol being used. For TCP, that is 2 hours. For UDP and ICMP, we use 2 minutes.

Resolves: #9645

@vercel

vercel Bot commented Jun 24, 2025

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 Jun 25, 2025 5:15pm

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull Request Overview

This PR increases the NAT table’s DNS resource TTL from 1 minute to 2 hours.

  • Changed TTL constant duration from 60 seconds to 2 hours.
Comments suppressed due to low confidence (1)

rust/connlib/tunnel/src/peer/nat_table.rs:28

  • Consider adding a doc comment above this constant to clarify its purpose and unit, e.g., /// NAT entry TTL (2 hours).
pub(crate) const TTL: Duration = Duration::from_secs(60 * 60 * 2);

jamilbk
jamilbk previously approved these changes Jun 24, 2025

@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.

Let's hope this does it! 💫

@thomaseizinger

Copy link
Copy Markdown
Member Author

We'll need to test this on staging. I hope this doesn't have any negative side-effect on e.g. resource usage.

@thomaseizinger thomaseizinger added this pull request to the merge queue Jun 24, 2025
@github-merge-queue github-merge-queue Bot removed this pull request from the merge queue due to failed status checks Jun 24, 2025
@jamilbk

jamilbk commented Jun 24, 2025

Copy link
Copy Markdown
Member

We'll need to test this on staging. I hope this doesn't have any negative side-effect on e.g. resource usage.

Any idea on what the behavior will be when the NAT table fills up?

And we don't evict these entries differently for UDP/TCP correct?

For TCP we can kill them before the 2-hour mark if we see a RST or FIN packet. For UDP we can use a much lower timeout like 2 minutes or something I think.

@thomaseizinger

Copy link
Copy Markdown
Member Author

Any idea on what the behavior will be when the NAT table fills up?

We will drop packets:

.context("Exhausted NAT")?;

@thomaseizinger

Copy link
Copy Markdown
Member Author

For TCP we can kill them before the 2-hour mark if we see a RST or FIN packet

I am not sure it is that easy. We will need at least some minimalistic tracking of the TCP state machine to correctly determine whether the connection is still alive.

@thomaseizinger

Copy link
Copy Markdown
Member Author

For UDP we can use a much lower timeout like 2 minutes or something I think.

This should be easy to do. The mappings are per protocol though so they don't conflict anyway. Hence, I am not sure it makes a big difference.

@jamilbk

jamilbk commented Jun 24, 2025

Copy link
Copy Markdown
Member

For UDP we can use a much lower timeout like 2 minutes or something I think.

This should be easy to do. The mappings are per protocol though so they don't conflict anyway. Hence, I am not sure it makes a big difference.

It might help if there are many UDP connections?

@jamilbk

jamilbk commented Jun 24, 2025

Copy link
Copy Markdown
Member

For TCP we can kill them before the 2-hour mark if we see a RST or FIN packet

I am not sure it is that easy. We will need at least some minimalistic tracking of the TCP state machine to correctly determine whether the connection is still alive.

Yeah I see that now. To do this robustly is quite a lot of state tracking. I'm not a TCP expert but just checking for a RST or a FIN-ACK might be good enough for some time?

Since we're already inside a tunnel we shouldn't need to worry about malicious RSTs as much I would think?

@jamilbk

jamilbk commented Jun 24, 2025

Copy link
Copy Markdown
Member

Oh I'm guessing this is probably tricky to do for packets coming back into the NAT. Technically we'd want those to keep this alive as well.

If that's possible then a 2-minute idle timeout for UDP and 2-hour for TCP plus RST/FIN-ACK check could work.

Alternatively, if we just use 2 hour and keep a pointer to the last allocated port, we could make this a ring and overwrite the oldest ones when we fill up.

If the allocation is 2 hours then we probably don't need to remap the same source port right?

@thomaseizinger

Copy link
Copy Markdown
Member Author

Oh I'm guessing this is probably tricky to do for packets coming back into the NAT. Technically we'd want those to keep this alive as well.

I don't think this is how NATs typically behave. AFAIK, NAT mappings are only kept alive based on outgoing traffic. Otherwise a malicious actor could keep a binding open indefinitely for as long as they are sending data.

If that's possible then a 2-minute idle timeout for UDP and 2-hour for TCP plus RST/FIN-ACK check could work.

Checking for RST should be easy. Doing only 2 min for UDP would also be easy. Properly tracking the closure of TCP connections requires a bit more work. We have practically all ports available as source ports for these connections, I think we can get away with not implementing any TCP state tracking here. The NAT is per client so a client would have to open ~7 TCP connections per second to exhaust this NAT.

@jamilbk

jamilbk commented Jun 25, 2025

Copy link
Copy Markdown
Member

I don't think this is how NATs typically behave. AFAIK, NAT mappings are only kept alive based on outgoing traffic. Otherwise a malicious actor could keep a binding open indefinitely for as long as they are sending data.

It depends. Incoming packets definitely do keep the connection alive in some cases. For UDP, once the connection is established, inbound packets will keep it alive.

For TCP, keepalives can be sent in either direction for an established connection too. I learned this after studying the behavior of SSH the last time we had TCP idle issues (for IP resources).

I don't know how accurately we need to model actual NAT behavior, but I'm certain that they do use (to some extent) inbound packets to maintain NAT sessions.

@jamilbk

jamilbk commented Jun 25, 2025

Copy link
Copy Markdown
Member

Unfortunately the RFC's aren't very helpful here. This section states NATs may use UDP inbound to keep sessions active, but doesn't require it: https://datatracker.ietf.org/doc/html/rfc4787#section-4.3. In practice, we should probably just follow the spirit of e.g. what conntrack does in Linux.

I did also find the 2-minute requirement for UDP NAT sessions, but network operators (as we've seen) in certain places of the world most likely configure this to 30 seconds instead. It appears that #9570 fixed the issue we were having there.

Taking a step back, I think the biggest take away is probably that it makes more sense to mirror what i.e. Linux or BSD does in its NAT implementation rather than relying on RFCs. Unfortunately not everyone follows the rules :-/.

@thomaseizinger thomaseizinger changed the title feat(gateway): set DNS resource NAT TTL to 2h feat(gateway): vary DNS resource NAT TTL by protocol Jun 25, 2025
@thomaseizinger

Copy link
Copy Markdown
Member Author

@jamilbk Implemented the varying TTL for different protocols. I opted to not implement any tracking for now because it would have to happen in multiple places. We can do that as a follow-up if we deem it necessary.

let ttl = match src {
Protocol::Tcp(_) => 7200,
Protocol::Udp(_) => 120,
Protocol::Icmp(_) => 120,

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.

Does ICMP incur a NAT entry? I would have thought not.

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.

I suppose it does for just the IP portion.

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.

We use one in our NAT because we operate on the source "protocol", i.e. we need some identifier from L4 for the NAT session. For UDP and TCP we use the port. For ICMP, we use the "identifier".

@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.

LGTM pending RST eviction.

let ttl = match src {
Protocol::Tcp(_) => 7200,
Protocol::Udp(_) => 120,
Protocol::Icmp(_) => 120,

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.

I suppose it does for just the IP portion.

@thomaseizinger

Copy link
Copy Markdown
Member Author

LGTM pending RST eviction.

I'll skip that for now for simplicity.

@thomaseizinger thomaseizinger added this pull request to the merge queue Jun 25, 2025
Merged via the queue into main with commit bf03e13 Jun 25, 2025
104 checks passed
@thomaseizinger thomaseizinger deleted the feat/dns-nat-mapping-2-hours branch June 25, 2025 17:29
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.

Keep DNS resource NAT mappings alive for 2 hours

3 participants