atunnel: support IPv6 original destination lookup - #753
atunnel: support IPv6 original destination lookup#753Bingtan Lu (lubingtan) wants to merge 3 commits into
Conversation
…o ipv6-integration
|
Deep-reviewed this against current Linux master kernel sources (net/netfilter/nf_conntrack_proto.c, net/ipv4/netfilter/ip_tables.c, net/ipv6/netfilter/ip6_tables.c, net/ipv6/af_inet6.c, include/uapi/linux/netfilter_ipv6/ip6_tables.h) and ran a live veth/netns/nftables repro. The core approach is correct; a few issues below. Verified correct
FindingsMAJOR-1: Error masking on ordinary IPv4 sockets
Fix: only attempt the v6 fallback when the socket is actually v6 ( MAJOR-2: Tests hard-fail (not skip) on hosts with a default-deny input firewall
Fix: have the test install its own filter-chain accept rule for the redirect port (own table, priority filter, before ufw), matching production wiring — or probe the connect and MINOR-3: Production IPv6 path is still dead code
NIT-4: ENOENT mechanism undocumentedAdd the why: pure-v6 sockets leave NIT-5:
|
|
Two asks; the rest of the PR I would take as is. 1. Gate the IPv6 fallback on the connection's family. krsnaSuraj's MAJOR-1, taking the first of the two options they offered — diff --git a/internal/atunnel/original_dst_linux.go b/internal/atunnel/original_dst_linux.go
index 8b430911..bd8b7646 100644
--- a/internal/atunnel/original_dst_linux.go
+++ b/internal/atunnel/original_dst_linux.go
@@ -43,14 +43,23 @@ func TCPOriginalDestination(conn net.Conn) (string, error) {
return "", fmt.Errorf("atunnel: acquiring TCP syscall connection: %w", err)
}
+ // The IPv6 option is only meaningful on an AF_INET6 socket: on AF_INET the
+ // kernel returns EOPNOTSUPP, which would mask the real IPv4 error. A
+ // v4-mapped local address still means an IPv4 flow, so To4 is the test.
+ local, ok := tcpConn.LocalAddr().(*net.TCPAddr)
+ if !ok {
+ return "", fmt.Errorf("atunnel: original destination requires a TCP local address, got %T", tcpConn.LocalAddr())
+ }
+ isIPv6 := local.IP.To4() == nil
+
var sockoptErr error
var destination string
if err := rawConn.Control(func(fd uintptr) {
destination, sockoptErr = originalIPv4Destination(fd)
- // Linux returns ENOENT when the IPv4 original-destination option is
- // queried on a redirected IPv6 connection. Only then try the IPv6
- // equivalent, so unrelated IPv4 failures retain their original error.
- if errors.Is(sockoptErr, unix.ENOENT) {
+ // A pure-IPv6 socket leaves the inet addresses zeroed, so the IPv4
+ // conntrack lookup always misses with ENOENT. That is the redirected
+ // IPv6 connection, and the only case worth retrying.
+ if isIPv6 && errors.Is(sockoptErr, unix.ENOENT) {
destination, sockoptErr = originalIPv6Destination(fd)
}
}); err != nil {2. A regression test. The non-obvious part is that it has to run in a fresh netns. Conntrack tracks loopback in any namespace carrying nftables rules, including the one Docker runs in, so in the host namespace Test diffdiff --git a/internal/atunnel/original_dst_linux_test.go b/internal/atunnel/original_dst_linux_test.go
index 4f26d980..36122c5d 100644
--- a/internal/atunnel/original_dst_linux_test.go
+++ b/internal/atunnel/original_dst_linux_test.go
@@ -22,6 +22,7 @@ import (
"fmt"
"net"
"os"
+ "runtime"
"strings"
"testing"
"time"
@@ -37,6 +38,63 @@ import (
"github.com/agent-substrate/substrate/internal/roottest"
)
+// TestTCPOriginalDestinationPreservesErrno covers the failure path on an
+// ordinary connection that no REDIRECT rule touched. The IPv4 lookup misses
+// and reports ENOENT; that error must reach the caller. Retrying the IPv6
+// option on an AF_INET socket would replace it with EOPNOTSUPP, which says
+// nothing about why the lookup failed.
+//
+// It runs in a fresh namespace because conntrack tracks loopback in any
+// namespace that has nftables rules — including the one docker runs in — and a
+// tracked connection returns its real destination instead of missing.
+func TestTCPOriginalDestinationPreservesErrno(t *testing.T) {
+ roottest.Require(t, "CAP_SYS_ADMIN for a network namespace with no conntrack hooks")
+
+ ns := newTestNetNS(t)
+ runtime.LockOSThread()
+ defer runtime.UnlockOSThread()
+ host, err := netns.Get()
+ if err != nil {
+ t.Fatal(err)
+ }
+ defer func() {
+ if err := netns.Set(host); err != nil {
+ t.Errorf("restoring host network namespace: %v", err)
+ }
+ _ = host.Close()
+ }()
+ if err := netns.Set(ns); err != nil {
+ t.Fatal(err)
+ }
+ loopback, err := netlink.LinkByName("lo")
+ if err != nil {
+ t.Fatal(err)
+ }
+ if err := netlink.LinkSetUp(loopback); err != nil {
+ t.Fatal(err)
+ }
+
+ listener, err := net.Listen("tcp4", "127.0.0.1:0")
+ if err != nil {
+ t.Fatal(err)
+ }
+ defer listener.Close()
+ client, err := net.Dial("tcp4", listener.Addr().String())
+ if err != nil {
+ t.Fatal(err)
+ }
+ defer client.Close()
+ server, err := listener.Accept()
+ if err != nil {
+ t.Fatal(err)
+ }
+ defer server.Close()
+
+ if _, err := TCPOriginalDestination(server); !errors.Is(err, unix.ENOENT) {
+ t.Fatalf("want the IPv4 lookup's ENOENT, got %v", err)
+ }
+}
+
func TestTCPOriginalDestination(t *testing.T) {
roottest.Require(t, "CAP_NET_ADMIN + CAP_SYS_ADMIN for an actor-like network namespace and nftables REDIRECT rule")On MAJOR-1's severity. I probed the errnos directly:
Row 3 is your design, working as intended. Row 2 is the bug — but it needs an IPv4 connection with no conntrack entry, and the one consumer in Your tests have run in CI, just not here. This PR's workflow has been queued in Diffs are illustration, not a patch to apply verbatim. |
There was a problem hiding this comment.
The IPv6 support looks right. One thing to fix before it merges: TestTCPOriginalDestinationIPv6 is flaky at about 15%.
The fix: raise the four hardcoded one-second deadlines in internal/atunnel/original_dst_linux_test.go to ten seconds — the dial timeout and the accept deadline in each of the two tests:
| line | today | proposed |
|---|---|---|
| 70 | net.DialTimeout("tcp4", …, time.Second) |
10*time.Second |
| 78 | SetDeadline(time.Now().Add(time.Second)) |
Add(10*time.Second) |
| 120 | net.DialTimeout("tcp6", …, time.Second) |
10*time.Second |
| 128 | SetDeadline(time.Now().Add(time.Second)) |
Add(10*time.Second) |
Nothing waits out the full timeout on a passing run, so this costs no wall clock.
Why: 60 runs on this branch alone gave 9 failures, always the same shape:
original_dst_linux_test.go:133: accepting redirected IPv6 connection:
accept tcp6 [fd00:198:18:b163::1]:41753: i/o timeout
The slow passes land at 1.05–1.13s — one dropped SYN retransmitted at TCP's 1s initial RTO while neighbour discovery resolves on a just-created veth. It isn't DAD; both addresses already carry IFA_F_NODAD. With all four raised I measured 0 failures in 60 runs.
This is consistent with the green run I cited in my earlier comment — one pass doesn't exclude a 15% rate.
The IPv4 test carries the same two deadlines and the same exposure — it just wins the race more often, because ARP resolves faster than ND. The root-gated suite runs rarely today so the rate is invisible upstream, but it will start failing unrelated PRs once this is in.
|
Thanks, Yuan Gao (@ygao-g) — addressed both points in the latest two commits:
Thanks for the detailed errno matrix and the flake investigation. |
| destination, sockoptErr = originalIPv4Destination(fd) | ||
| // A pure-IPv6 socket leaves the inet addresses zeroed, so the IPv4 | ||
| // conntrack lookup always misses with ENOENT. That is the redirected | ||
| // IPv6 connection, and the only case worth retrying. | ||
| if isIPv6 && errors.Is(sockoptErr, unix.ENOENT) { | ||
| destination, sockoptErr = originalIPv6Destination(fd) | ||
| } |
There was a problem hiding this comment.
Suggestion: dispatch on isIPv6 rather than falling back on ENOENT.
| destination, sockoptErr = originalIPv4Destination(fd) | |
| // A pure-IPv6 socket leaves the inet addresses zeroed, so the IPv4 | |
| // conntrack lookup always misses with ENOENT. That is the redirected | |
| // IPv6 connection, and the only case worth retrying. | |
| if isIPv6 && errors.Is(sockoptErr, unix.ENOENT) { | |
| destination, sockoptErr = originalIPv6Destination(fd) | |
| } | |
| if isIPv6 { | |
| destination, sockoptErr = originalIPv6Destination(fd) | |
| return | |
| } | |
| destination, sockoptErr = originalIPv4Destination(fd) |
As written the IPv4 lookup always runs first, and on a pure IPv6 socket it is guaranteed to miss — the PF_INET conntrack tuple is built from inet_rcv_saddr/inet_daddr, which are zeroed — so that guaranteed ENOENT is what signals the retry. Dispatching directly is one syscall on both paths and doesn't rest on that detail, since isIPv6 already holds the answer.
Worth landing together with a dual-stack test case: the unconditional IPv4 call currently covers for a misclassified v4-mapped flow, and this makes To4() load-bearing.
| return "", fmt.Errorf("atunnel: accessing TCP socket: %w", err) | ||
| } | ||
| if sockoptErr != nil { | ||
| return "", fmt.Errorf("atunnel: reading original TCP destination: %w", sockoptErr) |
There was a problem hiding this comment.
Nit: name the family that missed.
| return "", fmt.Errorf("atunnel: reading original TCP destination: %w", sockoptErr) | |
| family := "IPv4" | |
| if isIPv6 { | |
| family = "IPv6" | |
| } | |
| return "", fmt.Errorf("atunnel: reading original %s TCP destination: %w", family, sockoptErr) |
Both option levels report a miss as ENOENT, so the message as it stands doesn't say which lookup ran.
|
I put your three commits on a branch with the two suggested changes applied, so there's something concrete to look at rather than just prose: https://github.com/ygao-g/substrate/pull/8/files It's on my fork and isn't proposed for merge. Your three commits are unchanged and still authored by you; the two on top are the suggestions — Two suggestions are inline above. The rest: Run the redirect tests in their own netns. They currently build the veth, the NAT table and both listeners in the host namespace. Under a default-deny INPUT policy the redirected SYN is dropped and both tests fail on the accept deadline — 10s each, so the 1s→10s change made them fail slower rather than pass. In throwaway namespaces the same tests pass in 0.07s. Root-gated tests run on every PR now, so as written this reds for anyone with ufw or firewalld on. Private namespaces also make the PID-derived addresses and interface names unnecessary — Add a dual-stack case. atunnel listens on an unspecified address, which Go opens as one dual-stack AF_INET6 socket, so every IPv4 actor arrives with a v4-mapped local address. That's the case Cover Worth rebasing before any of this — you're 177 behind main, and One difference there: |
|
SURAJ KUMAR (@krsnaSuraj) on MINOR-3 — the conclusion holds, but one detail in the reasoning doesn't.
The other half of MINOR-3 is the load-bearing one and it's right: |
|
Yuan Gao (@ygao-g) — you're right, and I was wrong on the load-bearing detail. Confirmed empirically on Go 1.26.3: The conclusion still holds — production IPv6 remains dead code until the veth/nftables path gains dual-stack ( Also agreed on the dispatch suggestion and the family-in-error-message nit — both are strictly cleaner than the ENOENT-fallback and I'd take them. Thanks for the dual-stack test-case warning too; that's exactly the v4-mapped blind spot. |
3 participants
Fixes #686
Solution
TCPOriginalDestinationfirst queries the existing IPv4SOL_IP/SO_ORIGINAL_DSTsocket option. Linux returnsENOENTwhen that option is queried for a redirected IPv6 connection, so only in that case it falls back toSOL_IPV6/IP6T_SO_ORIGINAL_DST(value 80 fromlinux/netfilter_ipv6/ip6_tables.h). Other IPv4 errors are returned unchanged.The IPv6 lookup decodes
RawSockaddrInet6and formats the result withnet.JoinHostPort, preserving the required bracketed address form.Tests
The new root-gated integration tests model the production egress path for both address families: an actor-like network namespace sends TCP through a veth, an nftables
PREROUTINGrule redirects it to a local listener, and the listener verifies thatTCPOriginalDestinationreturns the actor's pre-redirect target. The IPv6 test also disables DAD for the test-only veth addresses so listeners can bind deterministically.Validation
go test ./internal/atunnel(privileged IPv4 and IPv6 redirect tests)NO_COLOR= GOCACHE=/tmp/substrate-go-build-user make verify