SSH dial/tunnel/DNAT failures unconditionally drop the packet with no retry, and the 5s cooldown blanket-drops unrelated destinations too #31

New issue

Closed

opened 2026-07-07 14:35:01 +01:00 by danny · 9 comments

danny commented

2026-07-07 14:35:01 +01:00

(Migrated from gitlab.keyop.co.uk)

Found while investigating #4.

In getNfCallback (internal/nfq_forwarder/nfq_forwarder.go), three distinct error paths all issue an immediate, final nfqueue.NfDrop on the current packet with no retry of that packet:

SSH dial failure (sshConns.connection() error, ~line 641)
OpenTunnel failure other than ErrPortTunnelAlreadyOpen (~line 657)
addDNATRulesFunc failure (~line 664)

This compounds with sshConnections' sshRetryCooldown (5s, ~line 336, cache checked at ~lines 394-396): once one dial to a jump host fails, the cached error is returned for every subsequent packet for 5 whole seconds, for every destination sharing that SSH connection pool - not just the one that originally failed. A single transient failure (e.g. a jump host momentarily unreachable) therefore drops packets for otherwise-healthy, already-tunnelled destinations on the same connection for up to 5 seconds, with zero retry of any of those dropped packets.

Impact: a single transient SSH hiccup causes real, silent packet loss well beyond the destination that actually failed - not just a delay.

Investigate/consider: whether the cooldown should be scoped more narrowly (e.g. only the destination that failed, not the whole pool), and whether any of these error paths should retry the current packet (e.g. via a bounded requeue) rather than an unconditional, final drop.

Found while investigating #4. In `getNfCallback` (`internal/nfq_forwarder/nfq_forwarder.go`), three distinct error paths all issue an immediate, final `nfqueue.NfDrop` on the current packet with no retry of that packet: - SSH dial failure (`sshConns.connection()` error, ~line 641) - `OpenTunnel` failure other than `ErrPortTunnelAlreadyOpen` (~line 657) - `addDNATRulesFunc` failure (~line 664) This compounds with `sshConnections`' `sshRetryCooldown` (5s, ~line 336, cache checked at ~lines 394-396): once one dial to a jump host fails, the cached error is returned for every subsequent packet for **5 whole seconds**, for **every destination sharing that SSH connection pool** - not just the one that originally failed. A single transient failure (e.g. a jump host momentarily unreachable) therefore drops packets for otherwise-healthy, already-tunnelled destinations on the same connection for up to 5 seconds, with zero retry of any of those dropped packets. **Impact**: a single transient SSH hiccup causes real, silent packet loss well beyond the destination that actually failed - not just a delay. **Investigate/consider**: whether the cooldown should be scoped more narrowly (e.g. only the destination that failed, not the whole pool), and whether any of these error paths should retry the current packet (e.g. via a bounded requeue) rather than an unconditional, final drop.

danny commented

2026-07-07 14:36:04 +01:00

(Migrated from gitlab.keyop.co.uk)

mentioned in issue #4

danny commented

2026-07-07 14:46:08 +01:00

(Migrated from gitlab.keyop.co.uk)

mentioned in merge request !70

danny commented

2026-07-07 15:33:39 +01:00

(Migrated from gitlab.keyop.co.uk)

mentioned in merge request !71

danny commented

2026-07-07 16:07:17 +01:00

(Migrated from gitlab.keyop.co.uk)

mentioned in commit 6aa6c1b88d

mentioned in commit 6aa6c1b88d04de4ffedf001e94aed16532fa94cf

danny commented

2026-07-07 16:07:43 +01:00

(Migrated from gitlab.keyop.co.uk)

mentioned in merge request !72

danny commented

2026-07-07 16:07:54 +01:00

(Migrated from gitlab.keyop.co.uk)

Implementation is up for review: !72 (branch issue31_dont_leak_orphaned_tunnel_on_dnat_failure). Two fixes bundled: (1) rolling back the orphaned tunnel on a DNAT-insert failure (the more severe finding - traffic was silently going out unencrypted otherwise), and (2) distinguishing cooldown-suppressed dial failures from fresh ones in the logs. I'm deliberately not changing the cooldown's blast radius itself - see the MR description for why. Waiting on review before merging.

Implementation is up for review: !72 (branch `issue31_dont_leak_orphaned_tunnel_on_dnat_failure`). Two fixes bundled: (1) rolling back the orphaned tunnel on a DNAT-insert failure (the more severe finding - traffic was silently going out unencrypted otherwise), and (2) distinguishing cooldown-suppressed dial failures from fresh ones in the logs. I'm deliberately **not** changing the cooldown's blast radius itself - see the MR description for why. Waiting on review before merging.

danny commented

2026-07-07 17:24:46 +01:00

(Migrated from gitlab.keyop.co.uk)

mentioned in commit 067ef83730

mentioned in commit 067ef837304dce6fda7ddad51597745aed2fb8d9

danny (Migrated from gitlab.keyop.co.uk) closed this issue

2026-07-07 17:24:47 +01:00

claude commented

2026-07-07 17:26:19 +01:00

(Migrated from gitlab.keyop.co.uk)

Merged via !72 (merge commit 067ef83) and released as v1.1.3 (patch bump).

Merged via !72 (merge commit `067ef83`) and released as **v1.1.3** (patch bump).