From 2d5d6f5403f34d47972c1955c989665ea1829807 Mon Sep 17 00:00:00 2001 From: Percy Wegmann Date: Wed, 28 Feb 2024 11:44:42 -0600 Subject: [PATCH] ipn,wgengine: only intercept TailFS traffic on quad 100 This fixes a regression introduced with 993acf4 and released in v1.60.0. The regression caused us to intercept all userspace traffic to port 8080 which prevented users from exposing their own services to their tailnet at port 8080. Now, we only intercept traffic to port 8080 if it's bound for 100.100.100.100 or fd7a:115c:a1e0::53. Fixes #11283 Signed-off-by: Percy Wegmann (cherry picked from commit 17cd0626f35dbc7948a78665d06a5862fc3dfdab) --- ipn/ipnlocal/local.go | 40 ++++++++++++-------- ipn/ipnlocal/local_test.go | 69 +++++++++++++++++++++++++++++++++++ ipn/ipnlocal/serve_test.go | 3 +- wgengine/netstack/netstack.go | 16 +------- 4 files changed, 97 insertions(+), 31 deletions(-) diff --git a/ipn/ipnlocal/local.go b/ipn/ipnlocal/local.go index 29c5a75ad..0fbc0a103 100644 --- a/ipn/ipnlocal/local.go +++ b/ipn/ipnlocal/local.go @@ -3329,13 +3329,24 @@ func (b *LocalBackend) isLocalIP(ip netip.Addr) bool { // TCPHandlerForDst returns a TCP handler for connections to dst, or nil if // no handler is needed. It also returns a list of TCP socket options to // apply to the socket before calling the handler. +// TCPHandlerForDst is called both for connections to our node's local IP +// as well as to the service IP (quad 100). func (b *LocalBackend) TCPHandlerForDst(src, dst netip.AddrPort) (handler func(c net.Conn) error, opts []tcpip.SettableSocketOption) { - if dst.Port() == 80 && (dst.Addr() == magicDNSIP || dst.Addr() == magicDNSIPv6) { - if b.ShouldRunWebClient() { - return b.handleWebClientConn, opts + // First handle internal connections to the service IP + hittingServiceIP := dst.Addr() == magicDNSIP || dst.Addr() == magicDNSIPv6 + if hittingServiceIP { + switch dst.Port() { + case 80: + if b.ShouldRunWebClient() { + return b.handleWebClientConn, opts + } + return b.HandleQuad100Port80Conn, opts + case TailFSLocalPort: + return b.handleTailFSConn, opts } - return b.HandleQuad100Port80Conn, opts } + + // Then handle external connections to the local IP. if !b.isLocalIP(dst.Addr()) { return nil, nil } @@ -3353,18 +3364,6 @@ func (b *LocalBackend) TCPHandlerForDst(src, dst netip.AddrPort) (handler func(c if dst.Port() == webClientPort && b.ShouldRunWebClient() { return b.handleWebClientConn, opts } - if dst.Port() == TailFSLocalPort { - fs, ok := b.sys.TailFSForLocal.GetOK() - if ok { - return func(conn net.Conn) error { - if !b.TailFSAccessEnabled() { - conn.Close() - return nil - } - return fs.HandleConn(conn, conn.RemoteAddr()) - }, opts - } - } if port, ok := b.GetPeerAPIPort(dst.Addr()); ok && dst.Port() == port { return func(c net.Conn) error { b.handlePeerAPIConn(src, dst, c) @@ -3377,6 +3376,15 @@ func (b *LocalBackend) TCPHandlerForDst(src, dst netip.AddrPort) (handler func(c return nil, nil } +func (b *LocalBackend) handleTailFSConn(conn net.Conn) error { + fs, ok := b.sys.TailFSForLocal.GetOK() + if !ok || !b.TailFSAccessEnabled() { + conn.Close() + return nil + } + return fs.HandleConn(conn, conn.RemoteAddr()) +} + func (b *LocalBackend) peerAPIServicesLocked() (ret []tailcfg.Service) { for _, pln := range b.peerAPIListeners { proto := tailcfg.PeerAPI4 diff --git a/ipn/ipnlocal/local_test.go b/ipn/ipnlocal/local_test.go index a3cb7e213..9d30cb77d 100644 --- a/ipn/ipnlocal/local_test.go +++ b/ipn/ipnlocal/local_test.go @@ -2173,3 +2173,72 @@ func TestOnTailnetDefaultAutoUpdate(t *testing.T) { }) } } + +func TestTCPHandlerForDst(t *testing.T) { + b := newTestBackend(t) + + tests := []struct { + desc string + dst string + intercept bool + }{ + { + desc: "intercept port 80 (Web UI) on quad100 IPv4", + dst: "100.100.100.100:80", + intercept: true, + }, + { + desc: "intercept port 80 (Web UI) on quad100 IPv6", + dst: "[fd7a:115c:a1e0::53]:80", + intercept: true, + }, + { + desc: "don't intercept port 80 on local ip", + dst: "100.100.103.100:80", + intercept: false, + }, + { + desc: "intercept port 8080 (TailFS) on quad100 IPv4", + dst: "100.100.100.100:8080", + intercept: true, + }, + { + desc: "intercept port 8080 (TailFS) on quad100 IPv6", + dst: "[fd7a:115c:a1e0::53]:8080", + intercept: true, + }, + { + desc: "don't intercept port 8080 on local ip", + dst: "100.100.103.100:8080", + intercept: false, + }, + { + desc: "don't intercept port 9080 on quad100 IPv4", + dst: "100.100.100.100:9080", + intercept: false, + }, + { + desc: "don't intercept port 9080 on quad100 IPv6", + dst: "[fd7a:115c:a1e0::53]:9080", + intercept: false, + }, + { + desc: "don't intercept port 9080 on local ip", + dst: "100.100.103.100:9080", + intercept: false, + }, + } + + for _, tt := range tests { + t.Run(tt.dst, func(t *testing.T) { + t.Log(tt.desc) + src := netip.MustParseAddrPort("100.100.102.100:51234") + h, _ := b.TCPHandlerForDst(src, netip.MustParseAddrPort(tt.dst)) + if !tt.intercept && h != nil { + t.Error("intercepted traffic we shouldn't have") + } else if tt.intercept && h == nil { + t.Error("failed to intercept traffic we should have") + } + }) + } +} diff --git a/ipn/ipnlocal/serve_test.go b/ipn/ipnlocal/serve_test.go index 45bd4bc2c..f2b3e303c 100644 --- a/ipn/ipnlocal/serve_test.go +++ b/ipn/ipnlocal/serve_test.go @@ -684,7 +684,8 @@ func newTestBackend(t *testing.T) *LocalBackend { b.netMap = &netmap.NetworkMap{ SelfNode: (&tailcfg.Node{ - Name: "example.ts.net", + Name: "example.ts.net", + Capabilities: []tailcfg.NodeCapability{tailcfg.NodeAttrsTailFSAccess}, }).View(), UserProfiles: map[tailcfg.UserID]tailcfg.UserProfile{ tailcfg.UserID(1): { diff --git a/wgengine/netstack/netstack.go b/wgengine/netstack/netstack.go index de7129bff..d7ca22422 100644 --- a/wgengine/netstack/netstack.go +++ b/wgengine/netstack/netstack.go @@ -1131,25 +1131,13 @@ func (ns *Impl) acceptTCP(r *tcp.ForwarderRequest) { // Local Services (DNS and WebDAV) hittingServiceIP := dialIP == serviceIP || dialIP == serviceIPv6 hittingDNS := hittingServiceIP && reqDetails.LocalPort == 53 - hittingTailFS := hittingServiceIP && ns.tailFSForLocal != nil && reqDetails.LocalPort == ipnlocal.TailFSLocalPort - if hittingDNS || hittingTailFS { + if hittingDNS { c := getConnOrReset() if c == nil { return } addrPort := netip.AddrPortFrom(clientRemoteIP, reqDetails.RemotePort) - if hittingDNS { - go ns.dns.HandleTCPConn(c, addrPort) - } else if hittingTailFS { - if !ns.lb.TailFSAccessEnabled() { - c.Close() - return - } - err := ns.tailFSForLocal.HandleConn(c, net.TCPAddrFromAddrPort(addrPort)) - if err != nil { - ns.logf("netstack: tailfs.HandleConn: %v", err) - } - } + go ns.dns.HandleTCPConn(c, addrPort) return }