From 17cd0626f35dbc7948a78665d06a5862fc3dfdab 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 --- 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 1f11fa1a1..11ae186ee 100644 --- a/ipn/ipnlocal/local.go +++ b/ipn/ipnlocal/local.go @@ -3311,13 +3311,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 } @@ -3335,18 +3346,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) @@ -3359,6 +3358,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 c58f11023..fbd3ca56b 100644 --- a/wgengine/netstack/netstack.go +++ b/wgengine/netstack/netstack.go @@ -922,25 +922,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 }