Hold packets until the tunnel is open - fix NFQUEUE/DNAT hook-ordering bug #70

Merged

danny merged 2 commits from issue4_hold_packets_until_tunnel_open into main 2026-07-07 15:28:22 +01:00

Conversation 10 Commits 2 Files changed 4 +105 -30

danny commented 2026-07-07 14:46:05 +01:00 (Migrated from gitlab.keyop.co.uk)

Copy link

Closes #4

Background

The NFQUEUE interception rule lived in filter/OUTPUT, while the DNAT rule (added once the tunnel is up) is inserted into nat/OUTPUT. Linux evaluates a single packet's traversal of the OUTPUT hook across tables in a fixed priority order - nat runs before filter. By the time a brand-new destination's first packet reached the NFQUEUE rule, nat/OUTPUT had already run for it (with no match, since the DNAT rule didn't exist yet) and could not run again for that same packet - a verdict (ACCEPT/REPEAT/QUEUE) can only continue forward or repeat the same table, never rewind to an earlier-priority one.

So the very first packet of every new destination was structurally unrecoverable once queued: no verdict this code could issue would make that packet benefit from the DNAT rule it had just inserted. It was silently lost, and the connection only survived because the client's TCP stack retransmitted the initial packet a moment later - masking the bug for ordinary TCP traffic, but a real, guaranteed failure for anything that doesn't retry.

Fix

• initNfqIptables now inserts the NFQUEUE rule into mangle/OUTPUT instead of filter/OUTPUT - mangle runs before nat, so once the tunnel and DNAT rule are ready, getNfCallback can verdict with nfqueue.NfAccept (previously nfqueue.NfQeueue) and the packet continues forward into nat/OUTPUT in the same traversal, actually getting DNAT'd and forwarded itself.
• Both getNfCallback verdict sites affected by this (the success path, and the ErrPortTunnelAlreadyOpen branch) switched from NfQeueue to NfAccept.
• deleteNfqIptablesRules's crash-recovery cleanup now scans mangle (the new location) and filter (where an older, not-yet-upgraded binary's rules could still be sitting) alongside nat, so upgrading in place can't leave a stale rule behind that cleanup would never find.

Trade-off (documented in initNfqIptables's doc comment, README.md, and CLAUDE.md): nat can no longer transparently keep an already-tunnelled destination's packets from reaching our rule - that only worked because filter ran after nat. Every new connection to an already-tunnelled destination (not only the very first one ever) now takes a fast userspace round-trip: a cheap map lookup via sshtunnel.Conn.OpenTunnel's already-open check, then an immediate NfAccept - no SSH dial, no iptables mutation. This is a deliberate trade for never silently dropping a connection's first packet, matching the acceptance criteria from the ticket discussion: delays are fine, silent drops are not.

Testing

• TestGetNfCallbackDuplicateTunnelAccepts/TestGetNfCallbackNewTunnelDNATSuccessAccepts (renamed from ...Requeues) now assert nfqueue.NfAccept specifically - a regression test against reintroducing the old NfQeueue-based mechanism.
• New TestInitNfqIptablesTargetsMangleTable confirms the real initNfqIptables call announces (and attempts) inserting into mangle, not filter - safe regardless of sandbox privilege, same pattern as the existing TestDeleteIPTablesTableChainTagRulesIsSafe.
• make lint and make test (go test -race -cover ./...) both clean.

Caveat: I can't verify netfilter hook-ordering behaviour against a real kernel in this sandbox - this should be confirmed with a live packet capture (tcpdump + iptables -t nat -L OUTPUT -v --line-numbers timing) on a real host, logged as an outstanding item in CLAUDE.md's Known Issues checklist alongside this project's other sandbox-unverifiable web UI/networking items.

Docs

Updated README.md's "Main methodology" section (previously described the old, broken mechanism in detail) and added a CLAUDE.md Design notes bullet explaining the fix and its trade-off.

Related tickets

Split out from the same investigation into their own tickets, to be worked in order after this one: #32 (a malformed packet permanently kills its queue's read loop), #31 (unconditional drop with no retry + a 5s blanket cooldown affecting unrelated destinations), #33 (NfQeueue verdicts always target queue 0, likely made moot for the primary path by this fix but worth auditing/fixing for any other call sites).

Closes #4 ## Background The NFQUEUE interception rule lived in `filter/OUTPUT`, while the DNAT rule (added once the tunnel is up) is inserted into `nat/OUTPUT`. Linux evaluates a single packet's traversal of the `OUTPUT` hook across tables in a fixed priority order - `nat` runs *before* `filter`. By the time a brand-new destination's first packet reached the NFQUEUE rule, `nat/OUTPUT` had already run for it (with no match, since the DNAT rule didn't exist yet) and could not run again for that same packet - a verdict (`ACCEPT`/`REPEAT`/`QUEUE`) can only continue forward or repeat the same table, never rewind to an earlier-priority one. So the very first packet of every new destination was structurally unrecoverable once queued: no verdict this code could issue would make *that packet* benefit from the DNAT rule it had just inserted. It was silently lost, and the connection only survived because the client's TCP stack retransmitted the initial packet a moment later - masking the bug for ordinary TCP traffic, but a real, guaranteed failure for anything that doesn't retry. ## Fix - `initNfqIptables` now inserts the NFQUEUE rule into `mangle/OUTPUT` instead of `filter/OUTPUT` - `mangle` runs *before* `nat`, so once the tunnel and DNAT rule are ready, `getNfCallback` can verdict with `nfqueue.NfAccept` (previously `nfqueue.NfQeueue`) and the packet continues forward into `nat/OUTPUT` in the *same* traversal, actually getting DNAT'd and forwarded itself. - Both `getNfCallback` verdict sites affected by this (the success path, and the `ErrPortTunnelAlreadyOpen` branch) switched from `NfQeueue` to `NfAccept`. - `deleteNfqIptablesRules`'s crash-recovery cleanup now scans `mangle` (the new location) *and* `filter` (where an older, not-yet-upgraded binary's rules could still be sitting) alongside `nat`, so upgrading in place can't leave a stale rule behind that cleanup would never find. **Trade-off** (documented in `initNfqIptables`'s doc comment, `README.md`, and `CLAUDE.md`): `nat` can no longer transparently keep an already-tunnelled destination's packets from reaching our rule - that only worked because `filter` ran *after* `nat`. Every *new* connection to an already-tunnelled destination (not only the very first one ever) now takes a fast userspace round-trip: a cheap map lookup via `sshtunnel.Conn.OpenTunnel`'s already-open check, then an immediate `NfAccept` - no SSH dial, no iptables mutation. This is a deliberate trade for never silently dropping a connection's first packet, matching the acceptance criteria from the ticket discussion: delays are fine, silent drops are not. ## Testing - `TestGetNfCallbackDuplicateTunnelAccepts`/`TestGetNfCallbackNewTunnelDNATSuccessAccepts` (renamed from `...Requeues`) now assert `nfqueue.NfAccept` specifically - a regression test against reintroducing the old `NfQeueue`-based mechanism. - New `TestInitNfqIptablesTargetsMangleTable` confirms the real `initNfqIptables` call announces (and attempts) inserting into `mangle`, not `filter` - safe regardless of sandbox privilege, same pattern as the existing `TestDeleteIPTablesTableChainTagRulesIsSafe`. - `make lint` and `make test` (`go test -race -cover ./...`) both clean. **Caveat**: I can't verify netfilter hook-ordering behaviour against a real kernel in this sandbox - this should be confirmed with a live packet capture (tcpdump + `iptables -t nat -L OUTPUT -v --line-numbers` timing) on a real host, logged as an outstanding item in `CLAUDE.md`'s Known Issues checklist alongside this project's other sandbox-unverifiable web UI/networking items. ## Docs Updated `README.md`'s "Main methodology" section (previously described the old, broken mechanism in detail) and added a `CLAUDE.md` Design notes bullet explaining the fix and its trade-off. ## Related tickets Split out from the same investigation into their own tickets, to be worked in order after this one: #32 (a malformed packet permanently kills its queue's read loop), #31 (unconditional drop with no retry + a 5s blanket cooldown affecting unrelated destinations), #33 (`NfQeueue` verdicts always target queue 0, likely made moot for the primary path by this fix but worth auditing/fixing for any other call sites).

danny (Migrated from gitlab.keyop.co.uk) approved these changes 2026-07-07 14:46:05 +01:00

danny commented 2026-07-07 14:46:20 +01:00 (Migrated from gitlab.keyop.co.uk)

Copy link

mentioned in issue #4

mentioned in issue #4

danny commented 2026-07-07 14:48:32 +01:00 (Migrated from gitlab.keyop.co.uk)

Copy link

Real-host verification procedure for this fix.

Goal: prove the very first packet of a brand-new connection now actually reaches the tunnel, rather than being silently dropped and only surviving via a kernel-level TCP retransmit ~1s later (the old, broken behaviour). Needs a real host with the fixed binary running (--debug/--verbose recommended so getNfCallback's log lines are visible) and root for tcpdump/iptables -L -v.

0. Confirm rule placement

sudo iptables -t mangle -L OUTPUT -v -n --line-numbers | grep NFQUEUE   # should show our rule
sudo iptables -t filter -L OUTPUT -v -n --line-numbers                  # should NOT show an nfq_forwarder-tagged rule (unless cleaning up a stale one from an old binary)

1. Start a capture covering both the real egress interface and loopback, for the duration of the test:

sudo tcpdump -i any -n -w /tmp/nfq_test.pcap 'tcp and port <DEST_PORT>'

2. Pick a destination:port from your config that's genuinely cold - i.e. nfq_forwarder hasn't tunnelled it since it last started (restart the service, or use an entry you know hasn't been touched yet), so there's no already-open tunnel/DNAT rule to short-circuit the test.

3. Note the 'before' packet counters on both rules:

sudo iptables -t mangle -L OUTPUT -v -n --line-numbers   # note the NFQUEUE rule's packet count
sudo iptables -t nat -L OUTPUT -v -n --line-numbers      # note count for this destination's DNAT rule (won't exist yet on a cold destination)

4. From the SAME host (nfq_forwarder only intercepts locally-originated packets, via the OUTPUT chain - the 'client' has to be a process on this same box), time a single connection attempt:

time nc -zv -w 5 <DEST_IP> <DEST_PORT>

(or curl --max-time 5 -v telnet://<DEST_IP>:<DEST_PORT> if nc isn't available)

Expected with the fix: connects in well under a second - bounded only by the SSH dial + tunnel-open time (typically tens to a couple hundred ms against a healthy jump host). A time of ~1s or more (Linux's default initial SYN retransmit timeout) would indicate the first SYN was still lost and only a retransmit got through - i.e. the bug is still present.

5. Inspect the capture for the SYNs to this destination:

tcpdump -r /tmp/nfq_test.pcap -n 'tcp[tcpflags] & tcp-syn != 0 and tcp[tcpflags] & tcp-ack == 0'

Expect exactly one SYN to the real destination IP (captured before DNAT rewrite, on the egress interface) and its SYN-ACK coming back - no second, retransmitted SYN a second or so later with the same source port/sequence number. A retransmitted SYN here means the first attempt was still lost.

6. Re-check the 'after' packet counters from step 3 - both the mangle NFQUEUE rule and the newly-inserted nat DNAT rule for this destination should show exactly +1 packet, confirming the same packet was counted by both rules in sequence - only possible if it genuinely traversed mangle -> nat in one pass, which is the whole mechanism this fix relies on.

7. Sanity-check kernel queue drops (unrelated to this fix specifically, but worth a glance given the queue-overflow risk discussed under #31):

cat /proc/net/netfilter/nfnetlink_queue

queue_dropped/queue_user_dropped (the last two numeric columns) shouldn't increase across the test.

8. Repeat step 2-6 against a second, different cold destination:port to confirm it's not a fluke.

9. Confirm the trade-off doesn't regress steady-state use: immediately open a second new connection to the same now-already-tunnelled destination:port from step 2-6, and confirm it still succeeds quickly, with getNfCallback's "Mapping already exists for ..." log line appearing (the fast userspace round-trip path) rather than any error/delay.

Optional but the strongest possible evidence: check out main at the commit just before this fix (or the running production binary, if it predates this change), repeat steps 0-6 once, and confirm you do see the ~1s+ delay and the retransmitted SYN in that case - a clean before/after contrast.

**Real-host verification procedure for this fix.** Goal: prove the *very first* packet of a brand-new connection now actually reaches the tunnel, rather than being silently dropped and only surviving via a kernel-level TCP retransmit ~1s later (the old, broken behaviour). Needs a real host with the fixed binary running (`--debug`/`--verbose` recommended so `getNfCallback`'s log lines are visible) and root for `tcpdump`/`iptables -L -v`. **0. Confirm rule placement** ``` sudo iptables -t mangle -L OUTPUT -v -n --line-numbers | grep NFQUEUE # should show our rule sudo iptables -t filter -L OUTPUT -v -n --line-numbers # should NOT show an nfq_forwarder-tagged rule (unless cleaning up a stale one from an old binary) ``` **1. Start a capture covering both the real egress interface and loopback**, for the duration of the test: ``` sudo tcpdump -i any -n -w /tmp/nfq_test.pcap 'tcp and port <DEST_PORT>' ``` **2. Pick a destination:port from your config that's genuinely cold** - i.e. nfq_forwarder hasn't tunnelled it since it last started (restart the service, or use an entry you know hasn't been touched yet), so there's no already-open tunnel/DNAT rule to short-circuit the test. **3. Note the 'before' packet counters** on both rules: ``` sudo iptables -t mangle -L OUTPUT -v -n --line-numbers # note the NFQUEUE rule's packet count sudo iptables -t nat -L OUTPUT -v -n --line-numbers # note count for this destination's DNAT rule (won't exist yet on a cold destination) ``` **4. From the SAME host** (nfq_forwarder only intercepts locally-originated packets, via the OUTPUT chain - the 'client' has to be a process on this same box), time a single connection attempt: ``` time nc -zv -w 5 <DEST_IP> <DEST_PORT> ``` (or `curl --max-time 5 -v telnet://<DEST_IP>:<DEST_PORT>` if nc isn't available) **Expected with the fix**: connects in well under a second - bounded only by the SSH dial + tunnel-open time (typically tens to a couple hundred ms against a healthy jump host). A time of ~1s or more (Linux's default initial SYN retransmit timeout) would indicate the first SYN was still lost and only a retransmit got through - i.e. the bug is still present. **5. Inspect the capture** for the SYNs to this destination: ``` tcpdump -r /tmp/nfq_test.pcap -n 'tcp[tcpflags] & tcp-syn != 0 and tcp[tcpflags] & tcp-ack == 0' ``` Expect exactly **one** SYN to the real destination IP (captured before DNAT rewrite, on the egress interface) and its SYN-ACK coming back - no second, retransmitted SYN a second or so later with the same source port/sequence number. A retransmitted SYN here means the first attempt was still lost. **6. Re-check the 'after' packet counters** from step 3 - both the `mangle` NFQUEUE rule and the newly-inserted `nat` DNAT rule for this destination should show exactly **+1 packet**, confirming the *same* packet was counted by both rules in sequence - only possible if it genuinely traversed mangle -> nat in one pass, which is the whole mechanism this fix relies on. **7. Sanity-check kernel queue drops** (unrelated to this fix specifically, but worth a glance given the queue-overflow risk discussed under #31): ``` cat /proc/net/netfilter/nfnetlink_queue ``` `queue_dropped`/`queue_user_dropped` (the last two numeric columns) shouldn't increase across the test. **8. Repeat step 2-6 against a second, different cold destination:port** to confirm it's not a fluke. **9. Confirm the trade-off doesn't regress steady-state use**: immediately open a *second* new connection to the *same* now-already-tunnelled destination:port from step 2-6, and confirm it still succeeds quickly, with `getNfCallback`'s "Mapping already exists for ..." log line appearing (the fast userspace round-trip path) rather than any error/delay. **Optional but the strongest possible evidence**: check out `main` at the commit just before this fix (or the running production binary, if it predates this change), repeat steps 0-6 once, and confirm you *do* see the ~1s+ delay and the retransmitted SYN in that case - a clean before/after contrast.

danny commented 2026-07-07 15:20:01 +01:00 (Migrated from gitlab.keyop.co.uk)

Copy link

assigned to @danny

assigned to @danny

danny commented 2026-07-07 15:20:02 +01:00 (Migrated from gitlab.keyop.co.uk)

Copy link

requested review from @danny

requested review from @danny

danny commented 2026-07-07 15:20:31 +01:00 (Migrated from gitlab.keyop.co.uk)

Copy link

All looks sane, and seems to work Ok. Testing shows this seems to no longer drop due to the ordering bug.

All looks sane, and seems to work Ok. Testing shows this seems to no longer drop due to the ordering bug.

danny commented 2026-07-07 15:20:32 +01:00 (Migrated from gitlab.keyop.co.uk)

Copy link

approved this merge request

approved this merge request

danny commented 2026-07-07 15:23:02 +01:00 (Migrated from gitlab.keyop.co.uk)

Copy link

added 1 commit

• 68a4eaef - Record real-host verification results for the issue #4 fix

Compare with previous version

added 1 commit <ul><li>68a4eaef - Record real-host verification results for the issue #4 fix</li></ul> [Compare with previous version](/keyop/go/nfq_forwarder/-/merge_requests/29/diffs?diff_id=315&start_sha=b8717269e9287a56991b891f78f02abf6a7d5986)

danny (Migrated from gitlab.keyop.co.uk) scheduled this pull request to auto merge when all checks succeed 2026-07-07 15:23:26 +01:00

danny commented 2026-07-07 15:23:46 +01:00 (Migrated from gitlab.keyop.co.uk)

Copy link

Real-host verification results (from testing.txt) - the fix checks out. Thanks for running this.

Rule placement (step 0/3): confirmed - the NFQUEUE rule is in mangle/OUTPUT (queue 21086, matching 10.66.16.0/20:9100), and filter/OUTPUT is clean (no leftover rule).

Packet counters (step 6) - the cleanest evidence in the whole test:

mangle NFQUEUE rule: 0 -> 1 packet
nat DNAT rule:      (didn't exist) -> 1 packet

Both show exactly +1 packet for the same connection attempt. That's only possible if the identical first packet was queued in mangle, accepted, and then actually traversed nat/OUTPUT and got DNAT'd there in the same pass - precisely the mechanism this fix is supposed to guarantee. This on its own is solid confirmation the fix works as designed.

Packet capture (step 5) - my test methodology had a gap, not a bug: the empty result for a SYN filtered on port 9100 isn't a problem - it's expected, and I should have anticipated it. DNAT rewrites the packet's destination before it's ever actually put on a real interface within that same netfilter traversal, so there's no wire-level capture point where the pre-DNAT form of that first packet (destined to port 9100) ever exists to be captured - by the time it would hit a device for real transmission, its destination is already 127.0.0.1:<tunnel-port>. The three packets that did show up (SYN-ACK/ACK/FIN between 10.66.16.2:9100 and 10.79.24.110:56562 on lo) are consistent with a working connection, just reflecting loopback+conntrack's un-DNAT view for the return direction - not something to chase further. The packet-counter comparison above is the reliable way to confirm this on a real host; I'll drop the SYN-capture step from the procedure for next time.

Timing - this is what actually nails it down. Your first run (2.999s) understandably looked like a possible problem, but once you controlled for it by restarting nfq_forwarder (forcing a genuinely cold tunnel) with the jump host's SSSD auth cache already warm, a very clean pattern emerged:

Binary	Run 1	Run 2
main (v1.1.0, pre-fix)	1.06s	1.01s
this branch (fix)	0.28s	0.24s

~1.0-1.06s is essentially Linux's default initial TCP SYN retransmission timeout - that's the signature of a silently dropped first SYN, only rescued by the kernel's automatic retransmit a second later. That's exactly the bug this ticket describes. The fix consistently completes in ~0.25s - no retransmit-shaped delay at all, roughly 4x faster, and consistent across both fixed-branch runs.

The initial 2.999s reading was a real but unrelated confound: an SSSD lookup delay on the jump host's own PAM auth after a period of inactivity, not anything in this code - it's why the clean comparison above always restarts nfq_forwarder (forcing a fresh tunnel dial) rather than the jump host's own auth state.

Conclusion: both lines of evidence (packet counters and timing) independently confirm the fix resolves the reported bug. I've recorded this verification in CLAUDE.md's Known Issues checklist (commit 68a4eae on this branch) so it's not re-litigated later, and noted the tcpdump methodology gap there too. Ready for your final review/merge whenever you're happy.

**Real-host verification results (from `testing.txt`) - the fix checks out.** Thanks for running this. **Rule placement (step 0/3):** confirmed - the NFQUEUE rule is in `mangle`/OUTPUT (queue 21086, matching `10.66.16.0/20:9100`), and `filter`/OUTPUT is clean (no leftover rule). **Packet counters (step 6) - the cleanest evidence in the whole test:** ``` mangle NFQUEUE rule: 0 -> 1 packet nat DNAT rule: (didn't exist) -> 1 packet ``` Both show exactly **+1 packet** for the same connection attempt. That's only possible if the *identical* first packet was queued in `mangle`, accepted, and then actually traversed `nat`/OUTPUT and got DNAT'd there in the same pass - precisely the mechanism this fix is supposed to guarantee. This on its own is solid confirmation the fix works as designed. **Packet capture (step 5) - my test methodology had a gap, not a bug:** the empty result for a SYN filtered on port 9100 isn't a problem - it's expected, and I should have anticipated it. DNAT rewrites the packet's destination *before* it's ever actually put on a real interface within that same netfilter traversal, so there's no wire-level capture point where the pre-DNAT form of that first packet (destined to port 9100) ever exists to be captured - by the time it would hit a device for real transmission, its destination is already `127.0.0.1:<tunnel-port>`. The three packets that *did* show up (SYN-ACK/ACK/FIN between `10.66.16.2:9100` and `10.79.24.110:56562` on `lo`) are consistent with a working connection, just reflecting loopback+conntrack's un-DNAT view for the return direction - not something to chase further. The packet-counter comparison above is the reliable way to confirm this on a real host; I'll drop the SYN-capture step from the procedure for next time. **Timing - this is what actually nails it down.** Your first run (2.999s) understandably looked like a possible problem, but once you controlled for it by restarting `nfq_forwarder` (forcing a genuinely cold tunnel) with the jump host's SSSD auth cache already warm, a very clean pattern emerged: | Binary | Run 1 | Run 2 | |---|---|---| | `main` (v1.1.0, pre-fix) | 1.06s | 1.01s | | this branch (fix) | 0.28s | 0.24s | ~1.0-1.06s is essentially Linux's default initial TCP SYN retransmission timeout - that's the signature of a *silently dropped first SYN*, only rescued by the kernel's automatic retransmit a second later. That's exactly the bug this ticket describes. The fix consistently completes in ~0.25s - no retransmit-shaped delay at all, roughly 4x faster, and consistent across both fixed-branch runs. The initial 2.999s reading was a real but unrelated confound: an SSSD lookup delay on the jump host's own PAM auth after a period of inactivity, not anything in this code - it's why the clean comparison above always restarts `nfq_forwarder` (forcing a fresh tunnel dial) rather than the jump host's own auth state. **Conclusion**: both lines of evidence (packet counters and timing) independently confirm the fix resolves the reported bug. I've recorded this verification in `CLAUDE.md`'s Known Issues checklist (commit `68a4eae` on this branch) so it's not re-litigated later, and noted the tcpdump methodology gap there too. Ready for your final review/merge whenever you're happy.

danny (Migrated from gitlab.keyop.co.uk) merged commit a6eaee7e01 into main 2026-07-07 15:28:22 +01:00

danny commented 2026-07-07 15:28:23 +01:00 (Migrated from gitlab.keyop.co.uk)

Copy link

mentioned in commit a6eaee7e01

mentioned in commit a6eaee7e0195abdfbfd745288a382835f87acb16

Sign in to join this conversation.

Reviewers

No reviewers

danny

Labels

Clear labels   

Bug

  

BuildIssue

  

Claude-fixed

  

Enhancement

  

In Progress

  

Low priority

  

On Hold

Blocked on something outside this project (e.g. external infrastructure) - not actionable right now

  

Security

No labels

Bug

BuildIssue

Claude-fixed

Enhancement

In Progress

Low priority

On Hold

Security

Milestone

Clear milestone

No items

No milestone

Projects

Clear projects

No items

No project

Assignees

Clear assignees

No assignees

1 participant

Notifications

Subscribe

Due date

The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference

keyop-go/nfq_forwarder!70

No description provided.