From 9bb601ebe849fd8492e8fbec351a7630643dee76 Mon Sep 17 00:00:00 2001 From: Brad Fitzpatrick Date: Fri, 4 Feb 2022 09:32:53 -0800 Subject: [PATCH] ipn/ipnlocal, wgengine/netstack: use netstack for peerapi server We're finding a bunch of host operating systems/firewalls interact poorly with peerapi. We either get ICMP errors from the host or users need to run commands to allow the peerapi port: https://github.com/tailscale/tailscale/issues/3842#issuecomment-1025133727 ... even though the peerapi should be an internal implementation detail. Rather than fight the host OS & firewalls, this change handles the server side of peerapi entirely in netstack (except on iOS), so it never makes its way to the host OS where it might be messed with. Two main downsides are: 1) netstack isn't as fast, but we don't really need speed for peerapi. And actually, with fewer trips to/from the kernel, we might actually make up for some of the netstack performance loss by staying in userspace. 2) tcpdump / Wireshark etc packet captures will no longer see the peerapi traffic. Oh well. Crawshaw's been wanting to add packet capture server support to tailscaled, so we'll probably do that sooner now. A future change might also then use peerapi for the client-side (except on iOS). Updates #3842 (probably fixes, as well as many exit node issues I bet) Change-Id: Ibc25edbb895dc083d1f07bd3cab614134705aa39 Signed-off-by: Brad Fitzpatrick (cherry picked from commit bd90781b34d2558f27196194ba871f1cda60ffe6) + edits (and cherry picked part of commit f3c0023addc13ea4212db9daf04fd317585d28c3) --- cmd/tailscaled/tailscaled.go | 7 +++-- ipn/ipnlocal/local.go | 27 ++++++++++++++-- ipn/ipnlocal/peerapi.go | 45 ++++++++++++++------------ wgengine/netstack/netstack.go | 59 +++++++++++++++++++++++++++++++---- 4 files changed, 107 insertions(+), 31 deletions(-) diff --git a/cmd/tailscaled/tailscaled.go b/cmd/tailscaled/tailscaled.go index 6c8a1bc43..db6320d3c 100644 --- a/cmd/tailscaled/tailscaled.go +++ b/cmd/tailscaled/tailscaled.go @@ -328,9 +328,6 @@ func run() error { } ns.ProcessLocalIPs = useNetstack ns.ProcessSubnets = useNetstack || wrapNetstack - if err := ns.Start(); err != nil { - return fmt.Errorf("failed to start netstack: %w", err) - } if useNetstack { dialer.UseNetstackForIP = func(ip netaddr.IP) bool { @@ -391,6 +388,10 @@ func run() error { if err != nil { return fmt.Errorf("ipnserver.New: %w", err) } + ns.SetLocalBackend(srv.LocalBackend()) + if err := ns.Start(); err != nil { + return fmt.Errorf("failed to start netstack: %w", err) + } if debugMux != nil { debugMux.HandleFunc("/debug/ipn", srv.ServeHTMLStatus) diff --git a/ipn/ipnlocal/local.go b/ipn/ipnlocal/local.go index 7c9db799a..120260c7f 100644 --- a/ipn/ipnlocal/local.go +++ b/ipn/ipnlocal/local.go @@ -212,7 +212,7 @@ func NewLocalBackend(logf logger.Logf, logid string, store ipn.StateStore, diale wiredPeerAPIPort := false if ig, ok := e.(wgengine.InternalsGetter); ok { if tunWrap, _, ok := ig.GetInternals(); ok { - tunWrap.PeerAPIPort = b.getPeerAPIPortForTSMPPing + tunWrap.PeerAPIPort = b.GetPeerAPIPort wiredPeerAPIPort = true } } @@ -1779,7 +1779,9 @@ func (b *LocalBackend) setPrefsLockedOnEntry(caller string, newp *ipn.Prefs) { b.send(ipn.Notify{Prefs: newp}) } -func (b *LocalBackend) getPeerAPIPortForTSMPPing(ip netaddr.IP) (port uint16, ok bool) { +// GetPeerAPIPort returns the port number for the peerapi server +// running on the provided IP. +func (b *LocalBackend) GetPeerAPIPort(ip netaddr.IP) (port uint16, ok bool) { b.mu.Lock() defer b.mu.Unlock() for _, pln := range b.peerAPIListeners { @@ -1790,6 +1792,27 @@ func (b *LocalBackend) getPeerAPIPortForTSMPPing(ip netaddr.IP) (port uint16, ok return 0, false } +// ServePeerAPIConnection serves an already-accepted connection c. +// +// The remote parameter is the remote address. +// The local paramater is the local address (either a Tailscale IPv4 +// or IPv6 IP and the peerapi port for that address). +// +// The connection will be closed by ServePeerAPIConnection. +func (b *LocalBackend) ServePeerAPIConnection(remote, local netaddr.IPPort, c net.Conn) { + b.mu.Lock() + defer b.mu.Unlock() + for _, pln := range b.peerAPIListeners { + if pln.ip == local.IP() { + go pln.ServeConn(remote, c) + return + } + } + b.logf("[unexpected] no peerAPI listener found for %v", local) + c.Close() + return +} + func (b *LocalBackend) peerAPIServicesLocked() (ret []tailcfg.Service) { for _, pln := range b.peerAPIListeners { proto := tailcfg.PeerAPI4 diff --git a/ipn/ipnlocal/peerapi.go b/ipn/ipnlocal/peerapi.go index 19aeb2240..8c2fe946b 100644 --- a/ipn/ipnlocal/peerapi.go +++ b/ipn/ipnlocal/peerapi.go @@ -481,27 +481,32 @@ func (pln *peerAPIListener) serve() { c.Close() continue } - peerNode, peerUser, ok := pln.lb.WhoIs(ipp) - if !ok { - logf("peerapi: unknown peer %v", ipp) - c.Close() - continue - } - h := &peerAPIHandler{ - ps: pln.ps, - isSelf: pln.ps.selfNode.User == peerNode.User, - remoteAddr: ipp, - peerNode: peerNode, - peerUser: peerUser, - } - httpServer := &http.Server{ - Handler: h, - } - if addH2C != nil { - addH2C(httpServer) - } - go httpServer.Serve(&oneConnListener{Listener: pln.ln, conn: c}) + pln.ServeConn(ipp, c) + } +} + +func (pln *peerAPIListener) ServeConn(src netaddr.IPPort, c net.Conn) { + logf := pln.lb.logf + peerNode, peerUser, ok := pln.lb.WhoIs(src) + if !ok { + logf("peerapi: unknown peer %v", src) + c.Close() + return + } + h := &peerAPIHandler{ + ps: pln.ps, + isSelf: pln.ps.selfNode.User == peerNode.User, + remoteAddr: src, + peerNode: peerNode, + peerUser: peerUser, + } + httpServer := &http.Server{ + Handler: h, + } + if addH2C != nil { + addH2C(httpServer) } + go httpServer.Serve(&oneConnListener{Listener: pln.ln, conn: c}) } type oneConnListener struct { diff --git a/wgengine/netstack/netstack.go b/wgengine/netstack/netstack.go index f92007059..5978b63f1 100644 --- a/wgengine/netstack/netstack.go +++ b/wgengine/netstack/netstack.go @@ -34,11 +34,13 @@ import ( "inet.af/netstack/tcpip/transport/tcp" "inet.af/netstack/tcpip/transport/udp" "inet.af/netstack/waiter" + "tailscale.com/ipn/ipnlocal" "tailscale.com/net/packet" "tailscale.com/net/tsaddr" "tailscale.com/net/tsdial" "tailscale.com/net/tstun" "tailscale.com/syncs" + "tailscale.com/types/ipproto" "tailscale.com/types/logger" "tailscale.com/types/netmap" "tailscale.com/version/distro" @@ -79,8 +81,12 @@ type Impl struct { mc *magicsock.Conn logf logger.Logf dialer *tsdial.Dialer - ctx context.Context // alive until Close - ctxCancel context.CancelFunc // called on Close + ctx context.Context // alive until Close + ctxCancel context.CancelFunc // called on Close + lb *ipnlocal.LocalBackend // or nil + + peerapiPort4Atomic uint32 // uint16 port number for IPv4 peerapi + peerapiPort6Atomic uint32 // uint16 port number for IPv6 peerapi // atomicIsLocalIPFunc holds a func that reports whether an IP // is a local (non-subnet) Tailscale IP address of this @@ -164,6 +170,12 @@ func (ns *Impl) Close() error { return nil } +// SetLocalBackend sets the LocalBackend; it should only be run before +// the Start method is called. +func (ns *Impl) SetLocalBackend(lb *ipnlocal.LocalBackend) { + ns.lb = lb +} + // wrapProtoHandler returns protocol handler h wrapped in a version // that dynamically reconfigures ns's subnet addresses as needed for // outbound traffic. @@ -251,8 +263,9 @@ func (ns *Impl) updateIPs(nm *netmap.NetworkMap) { ap := protocolAddr.AddressWithPrefix ip := netaddrIPFromNetstackIP(ap.Address) if ip == v4broadcast && ap.PrefixLen == 32 { - // Don't delete this one later. It seems to be important. - // Related to Issue 2642? Likely. + // Don't add 255.255.255.255/32 to oldIPs so we don't + // delete it later. We didn't install it, so it's not + // ours to delete. continue } oldIPs[ap] = true @@ -263,10 +276,10 @@ func (ns *Impl) updateIPs(nm *netmap.NetworkMap) { if nm.SelfNode != nil { for _, ipp := range nm.SelfNode.Addresses { isAddr[ipp] = true + newIPs[ipPrefixToAddressWithPrefix(ipp)] = true } for _, ipp := range nm.SelfNode.AllowedIPs { - local := isAddr[ipp] - if local && ns.ProcessLocalIPs || !local && ns.ProcessSubnets { + if !isAddr[ipp] && ns.ProcessSubnets { newIPs[ipPrefixToAddressWithPrefix(ipp)] = true } } @@ -389,9 +402,33 @@ func (ns *Impl) isLocalIP(ip netaddr.IP) bool { return ns.atomicIsLocalIPFunc.Load().(func(netaddr.IP) bool)(ip) } +func (ns *Impl) peerAPIPortAtomic(ip netaddr.IP) *uint32 { + if ip.Is4() { + return &ns.peerapiPort4Atomic + } else { + return &ns.peerapiPort6Atomic + } +} + // shouldProcessInbound reports whether an inbound packet should be // handled by netstack. func (ns *Impl) shouldProcessInbound(p *packet.Parsed, t *tstun.Wrapper) bool { + // Handle incoming peerapi connections in netstack. + if ns.lb != nil && p.IPProto == ipproto.TCP { + var peerAPIPort uint16 + dstIP := p.Dst.IP() + if p.TCPFlags&packet.TCPSynAck == packet.TCPSyn && ns.isLocalIP(dstIP) { + if port, ok := ns.lb.GetPeerAPIPort(p.Dst.IP()); ok { + peerAPIPort = port + atomic.StoreUint32(ns.peerAPIPortAtomic(dstIP), uint32(port)) + } + } else { + peerAPIPort = uint16(atomic.LoadUint32(ns.peerAPIPortAtomic(dstIP))) + } + if p.IPProto == ipproto.TCP && p.Dst.Port() == peerAPIPort { + return true + } + } if !ns.ProcessLocalIPs && !ns.ProcessSubnets { // Fast path for common case (e.g. Linux server in TUN mode) where // netstack isn't used at all; don't even do an isLocalIP lookup. @@ -584,6 +621,16 @@ func (ns *Impl) acceptTCP(r *tcp.ForwarderRequest) { // block until the TCP handshake is complete. c := gonet.NewTCPConn(&wq, ep) + if ns.lb != nil { + if port, ok := ns.lb.GetPeerAPIPort(dialIP); ok { + if reqDetails.LocalPort == port && ns.isLocalIP(dialIP) { + src := netaddr.IPPortFrom(clientRemoteIP, reqDetails.RemotePort) + dst := netaddr.IPPortFrom(dialIP, port) + ns.lb.ServePeerAPIConnection(src, dst, c) + return + } + } + } if ns.ForwardTCPIn != nil { ns.ForwardTCPIn(c, reqDetails.LocalPort) return