From b8f01eed34889d20631718a50abbd66c8fc7f58f Mon Sep 17 00:00:00 2001 From: David Anderson Date: Fri, 8 May 2020 01:07:13 +0000 Subject: [PATCH] wgengine/router: remove wireguard-go config from settings. Instead, pass in only exactly the relevant configuration pieces that the OS network stack cares about. Signed-off-by: David Anderson --- wgengine/router/ifconfig_windows.go | 85 ++++++++--------- wgengine/router/router.go | 29 ++---- wgengine/router/router_darwin.go | 2 +- wgengine/router/router_darwin_support.go | 4 +- wgengine/router/router_fake.go | 4 +- wgengine/router/router_freebsd.go | 8 +- wgengine/router/router_linux.go | 115 +++++++++-------------- wgengine/router/router_openbsd.go | 8 +- wgengine/router/router_windows.go | 4 +- wgengine/userspace.go | 42 ++++----- 10 files changed, 122 insertions(+), 179 deletions(-) diff --git a/wgengine/router/ifconfig_windows.go b/wgengine/router/ifconfig_windows.go index 7b5a6352f..94f1baf67 100644 --- a/wgengine/router/ifconfig_windows.go +++ b/wgengine/router/ifconfig_windows.go @@ -20,7 +20,6 @@ import ( winipcfg "github.com/tailscale/winipcfg-go" "github.com/tailscale/wireguard-go/device" "github.com/tailscale/wireguard-go/tun" - "github.com/tailscale/wireguard-go/wgcfg" "golang.org/x/sys/windows" "golang.org/x/sys/windows/registry" "tailscale.com/wgengine/winnet" @@ -237,7 +236,7 @@ func setFirewall(ifcGUID *windows.GUID) (bool, error) { return false, nil } -func configureInterface(m *wgcfg.Config, tun *tun.NativeTun, dns []wgcfg.IP, dnsDomains []string) error { +func configureInterface(rs Settings, tun *tun.NativeTun) error { const mtu = 0 guid := tun.GUID() log.Printf("wintun GUID is %v\n", guid) @@ -263,13 +262,13 @@ func configureInterface(m *wgcfg.Config, tun *tun.NativeTun, dns []wgcfg.IP, dns } }() - setDNSDomains(guid, dnsDomains) + setDNSDomains(guid, rs.DNSDomains) routes := []winipcfg.RouteData{} var firstGateway4 *net.IP var firstGateway6 *net.IP - addresses := make([]*net.IPNet, len(m.Addresses)) - for i, addr := range m.Addresses { + addresses := make([]*net.IPNet, len(rs.LocalAddrs)) + for i, addr := range rs.LocalAddrs { ipnet := addr.IPNet() addresses[i] = ipnet gateway := ipnet.IP @@ -282,48 +281,46 @@ func configureInterface(m *wgcfg.Config, tun *tun.NativeTun, dns []wgcfg.IP, dns foundDefault4 := false foundDefault6 := false - for _, peer := range m.Peers { - for _, allowedip := range peer.AllowedIPs { - if (allowedip.IP.Is4() && firstGateway4 == nil) || (allowedip.IP.Is6() && firstGateway6 == nil) { - return errors.New("Due to a Windows limitation, one cannot have interface routes without an interface address") - } + for _, route := range rs.Routes { + if (route.IP.Is4() && firstGateway4 == nil) || (route.IP.Is6() && firstGateway6 == nil) { + return errors.New("Due to a Windows limitation, one cannot have interface routes without an interface address") + } - ipn := allowedip.IPNet() - var gateway net.IP - if allowedip.IP.Is4() { - gateway = *firstGateway4 - } else if allowedip.IP.Is6() { - gateway = *firstGateway6 - } - r := winipcfg.RouteData{ - Destination: net.IPNet{ - IP: ipn.IP.Mask(ipn.Mask), - Mask: ipn.Mask, - }, - NextHop: gateway, - Metric: 0, - } - if bytes.Compare(r.Destination.IP, gateway) == 0 { - // no need to add a route for the interface's - // own IP. The kernel does that for us. - // If we try to replace it, we'll fail to - // add the route unless NextHop is set, but - // then the interface's IP won't be pingable. - continue + ipn := route.IPNet() + var gateway net.IP + if route.IP.Is4() { + gateway = *firstGateway4 + } else if route.IP.Is6() { + gateway = *firstGateway6 + } + r := winipcfg.RouteData{ + Destination: net.IPNet{ + IP: ipn.IP.Mask(ipn.Mask), + Mask: ipn.Mask, + }, + NextHop: gateway, + Metric: 0, + } + if bytes.Compare(r.Destination.IP, gateway) == 0 { + // no need to add a route for the interface's + // own IP. The kernel does that for us. + // If we try to replace it, we'll fail to + // add the route unless NextHop is set, but + // then the interface's IP won't be pingable. + continue + } + if route.IP.Is4() { + if route.Mask == 0 { + foundDefault4 = true } - if allowedip.IP.Is4() { - if allowedip.Mask == 0 { - foundDefault4 = true - } - r.NextHop = *firstGateway4 - } else if allowedip.IP.Is6() { - if allowedip.Mask == 0 { - foundDefault6 = true - } - r.NextHop = *firstGateway6 + r.NextHop = *firstGateway4 + } else if route.IP.Is6() { + if route.Mask == 0 { + foundDefault6 = true } - routes = append(routes, r) + r.NextHop = *firstGateway6 } + routes = append(routes, r) } err = iface.SyncAddresses(addresses) @@ -362,7 +359,7 @@ func configureInterface(m *wgcfg.Config, tun *tun.NativeTun, dns []wgcfg.IP, dns } var dnsIPs []net.IP - for _, ip := range dns { + for _, ip := range rs.DNS { dnsIPs = append(dnsIPs, ip.IP()) } err = iface.SetDNS(dnsIPs) diff --git a/wgengine/router/router.go b/wgengine/router/router.go index c4486c519..93275ba88 100644 --- a/wgengine/router/router.go +++ b/wgengine/router/router.go @@ -7,8 +7,6 @@ package router import ( - "fmt" - "github.com/tailscale/wireguard-go/device" "github.com/tailscale/wireguard-go/tun" "github.com/tailscale/wireguard-go/wgcfg" @@ -22,10 +20,10 @@ type Router interface { // Up brings the router up. Up() error - // SetRoutes is called regularly on network map updates. - // It's how you kernel route table entries are populated for - // each peer. - SetRoutes(RouteSettings) error + // Set updates the OS network stack with new settings. It may be + // called multiple times with identical Settings, which the + // implementation should handle gracefully. + Set(Settings) error // Close closes the router. Close() error @@ -37,23 +35,12 @@ func New(logf logger.Logf, wgdev *device.Device, tundev tun.Device) (Router, err return newUserspaceRouter(logf, wgdev, tundev) } -// RouteSettings is the full WireGuard config data (set of peers keys, -// IP, etc in wgcfg.Config) plus the things that WireGuard doesn't do -// itself, like DNS stuff. -type RouteSettings struct { +// Settings is the subset of Tailscale configuration that is relevant +// to the OS's network stack. +type Settings struct { LocalAddrs []wgcfg.CIDR DNS []wgcfg.IP DNSDomains []string + Routes []wgcfg.CIDR // routes to point into the Tailscale interface SubnetRoutes []wgcfg.CIDR // subnets being advertised to other Tailscale nodes - Cfg *wgcfg.Config -} - -// OnlyRelevantParts returns a string minimally describing the route settings. -func (rs *RouteSettings) OnlyRelevantParts() string { - var peers [][]wgcfg.CIDR - for _, p := range rs.Cfg.Peers { - peers = append(peers, p.AllowedIPs) - } - return fmt.Sprintf("%v %v %v %v %v", - rs.LocalAddrs, rs.DNS, rs.DNSDomains, rs.SubnetRoutes, peers) } diff --git a/wgengine/router/router_darwin.go b/wgengine/router/router_darwin.go index f1c80d619..c28adb6f0 100644 --- a/wgengine/router/router_darwin.go +++ b/wgengine/router/router_darwin.go @@ -26,7 +26,7 @@ func (r *darwinRouter) Up() error { return nil } -func (r *darwinRouter) SetRoutes(rs RouteSettings) error { +func (r *darwinRouter) Set(rs Settings) error { if SetRoutesFunc != nil { return SetRoutesFunc(rs) } diff --git a/wgengine/router/router_darwin_support.go b/wgengine/router/router_darwin_support.go index 031c4942d..364f66650 100644 --- a/wgengine/router/router_darwin_support.go +++ b/wgengine/router/router_darwin_support.go @@ -6,7 +6,7 @@ package router -// SetRoutesFunc applies the given route settings to the OS network +// SetRoutesFunc applies the given router settings to the OS network // stack. // // This is logically part of the router_darwin.go implementation, and @@ -22,4 +22,4 @@ package router // as MacOS, so that we don't have to wait until the Mac CI to // discover that we broke it. So this one definition needs to exist in // both the darwin and linux builds. Hence this file and build tag. -var SetRoutesFunc func(rs RouteSettings) error +var SetRoutesFunc func(rs Settings) error diff --git a/wgengine/router/router_fake.go b/wgengine/router/router_fake.go index f3d55b8ff..b60cc9428 100644 --- a/wgengine/router/router_fake.go +++ b/wgengine/router/router_fake.go @@ -25,8 +25,8 @@ func (r fakeRouter) Up() error { return nil } -func (r fakeRouter) SetRoutes(rs RouteSettings) error { - r.logf("Warning: fakeRouter.SetRoutes: not implemented.") +func (r fakeRouter) Set(rs Settings) error { + r.logf("Warning: fakeRouter.Set: not implemented.") return nil } diff --git a/wgengine/router/router_freebsd.go b/wgengine/router/router_freebsd.go index d120044ea..86011df93 100644 --- a/wgengine/router/router_freebsd.go +++ b/wgengine/router/router_freebsd.go @@ -55,7 +55,7 @@ func (r *freebsdRouter) Up() error { return nil } -func (r *freebsdRouter) SetRoutes(rs RouteSettings) error { +func (r *freebsdRouter) Set(rs Settings) error { if len(rs.LocalAddrs) == 0 { return nil } @@ -95,10 +95,8 @@ func (r *freebsdRouter) SetRoutes(rs RouteSettings) error { } newRoutes := make(map[wgcfg.CIDR]struct{}) - for _, peer := range rs.Cfg.Peers { - for _, route := range peer.AllowedIPs { - newRoutes[route] = struct{}{} - } + for _, route := range rs.Routes { + newRoutes[route] = struct{}{} } // Delete any pre-existing routes. for route := range r.routes { diff --git a/wgengine/router/router_linux.go b/wgengine/router/router_linux.go index 389b09c2c..389264203 100644 --- a/wgengine/router/router_linux.go +++ b/wgengine/router/router_linux.go @@ -138,90 +138,59 @@ func (r *linuxRouter) Close() error { return ret } -func (r *linuxRouter) SetRoutes(rs RouteSettings) error { - var errq error - - newAddrs := make(map[wgcfg.CIDR]bool) - for _, addr := range rs.LocalAddrs { - newAddrs[addr] = true - } - for addr := range r.addrs { - if newAddrs[addr] { - continue +// Set implements the Router interface. +func (r *linuxRouter) Set(rs Settings) error { + // cidrDiff calls add and del as needed to make the set of prefixes in + // old and new match. Returns a map version of new, and the first + // error encountered while reconfiguring, if any. + cidrDiff := func(kind string, old map[wgcfg.CIDR]bool, new []wgcfg.CIDR, add, del func(wgcfg.CIDR) error) (map[wgcfg.CIDR]bool, error) { + var ( + ret = make(map[wgcfg.CIDR]bool, len(new)) + errq error + ) + + for _, cidr := range new { + ret[cidr] = true } - if err := r.delAddress(addr); err != nil { - r.logf("addr del failed: %v", err) - if errq == nil { - errq = err + for cidr := range old { + if ret[cidr] { + continue } - } - } - for addr := range newAddrs { - if r.addrs[addr] { - continue - } - if err := r.addAddress(addr); err != nil { - r.logf("addr add failed: %v", err) - if errq == nil { - errq = err + if err := del(cidr); err != nil { + r.logf("%s del failed: %v", kind, err) + if errq == nil { + errq = err + } } } - } - - newRoutes := make(map[wgcfg.CIDR]bool) - for _, peer := range rs.Cfg.Peers { - for _, route := range peer.AllowedIPs { - newRoutes[route] = true - } - } - for route := range r.routes { - if newRoutes[route] { - continue - } - if err := r.delRoute(route); err != nil { - r.logf("route del failed: %v", err) - if errq == nil { - errq = err + for cidr := range ret { + if old[cidr] { + continue } - } - } - for route := range newRoutes { - if r.routes[route] { - continue - } - if err := r.addRoute(route); err != nil { - r.logf("route add failed: %v", err) - if errq == nil { - errq = err + if err := add(cidr); err != nil { + r.logf("%s add failed: %v", kind, err) + if errq == nil { + errq = err + } } } + + return ret, errq } - newSubnetRoutes := map[wgcfg.CIDR]bool{} - for _, route := range rs.SubnetRoutes { - newSubnetRoutes[route] = true + var errq error + + newAddrs, err := cidrDiff("addr", r.addrs, rs.LocalAddrs, r.addAddress, r.delAddress) + if err != nil && errq == nil { + errq = err } - for route := range r.subnetRoutes { - if newSubnetRoutes[route] { - continue - } - if err := r.delSubnetRule(route); err != nil { - r.logf("subnet rule del failed: %v", err) - if errq == nil { - errq = err - } - } + newRoutes, err := cidrDiff("route", r.routes, rs.Routes, r.addRoute, r.delRoute) + if err != nil && errq == nil { + errq = err } - for route := range newSubnetRoutes { - if r.subnetRoutes[route] { - continue - } - if err := r.addSubnetRule(route); err != nil { - r.logf("subnet rule add failed: %v", err) - if errq == nil { - errq = err - } - } + newSubnetRoutes, err := cidrDiff("subnet rule", r.subnetRoutes, rs.SubnetRoutes, r.addSubnetRule, r.delSubnetRule) + if err != nil && errq == nil { + errq = err } r.addrs = newAddrs diff --git a/wgengine/router/router_openbsd.go b/wgengine/router/router_openbsd.go index 84b54effe..53fdc7731 100644 --- a/wgengine/router/router_openbsd.go +++ b/wgengine/router/router_openbsd.go @@ -60,7 +60,7 @@ func (r *openbsdRouter) Up() error { return nil } -func (r *openbsdRouter) SetRoutes(rs RouteSettings) error { +func (r *openbsdRouter) Set(rs Settings) error { // TODO: support configuring multiple local addrs on interface. if len(rs.LocalAddrs) != 1 { return errors.New("freebsd doesn't support setting multiple local addrs yet") @@ -114,10 +114,8 @@ func (r *openbsdRouter) SetRoutes(rs RouteSettings) error { } newRoutes := make(map[wgcfg.CIDR]struct{}) - for _, peer := range rs.Cfg.Peers { - for _, route := range peer.AllowedIPs { - newRoutes[route] = struct{}{} - } + for _, route := range rs.Routes { + newRoutes[route] = struct{}{} } for route := range r.routes { if _, keep := newRoutes[route]; !keep { diff --git a/wgengine/router/router_windows.go b/wgengine/router/router_windows.go index f44fb2db5..6e9d9c853 100644 --- a/wgengine/router/router_windows.go +++ b/wgengine/router/router_windows.go @@ -45,8 +45,8 @@ func (r *winRouter) Up() error { return nil } -func (r *winRouter) SetRoutes(rs RouteSettings) error { - err := configureInterface(rs.Cfg, r.nativeTun, rs.DNS, rs.DNSDomains) +func (r *winRouter) Set(rs Settings) error { + err := configureInterface(rs, r.nativeTun) if err != nil { r.logf("ConfigureInterface: %v\n", err) return err diff --git a/wgengine/userspace.go b/wgengine/userspace.go index 010592aa8..79dc4d518 100644 --- a/wgengine/userspace.go +++ b/wgengine/userspace.go @@ -58,7 +58,6 @@ type userspaceEngine struct { wgLock sync.Mutex // serializes all wgdev operations; see lock order comment below lastReconfig string lastCfg wgcfg.Config - lastRoutes string mu sync.Mutex // guards following; see lock order comment below filt *filter.Filter @@ -241,7 +240,7 @@ func newUserspaceEngineAdvanced(logf logger.Logf, tundev tun.Device, routerGen R e.wgdev.Close() return nil, err } - if err := e.router.SetRoutes(router.RouteSettings{Cfg: new(wgcfg.Config)}); err != nil { + if err := e.router.Set(router.Settings{}); err != nil { e.wgdev.Close() return nil, err } @@ -324,6 +323,16 @@ func (e *userspaceEngine) pinger(peerKey wgcfg.Key, ips []wgcfg.IP) { } } +func configSignature(cfg *wgcfg.Config, dnsDomains []string, localRoutes []wgcfg.CIDR) (string, error) { + // TODO(apenwarr): get rid of uapi stuff for in-process comms + uapi, err := cfg.ToUAPI() + if err != nil { + return "", err + } + + return fmt.Sprintf("%s %v %v", uapi, dnsDomains, localRoutes), nil +} + // TODO(apenwarr): dnsDomains really ought to be in wgcfg.Config. // However, we don't actually ever provide it to wireguard and it's not in // the traditional wireguard config format. On the other hand, wireguard @@ -346,13 +355,10 @@ func (e *userspaceEngine) Reconfig(cfg *wgcfg.Config, dnsDomains []string, local } e.mu.Unlock() - // TODO(apenwarr): get rid of uapi stuff for in-process comms - uapi, err := cfg.ToUAPI() + rc, err := configSignature(cfg, dnsDomains, localRoutes) if err != nil { return err } - - rc := uapi + "\x00" + strings.Join(dnsDomains, "\x00") if rc == e.lastReconfig { return ErrNoChanges } @@ -390,30 +396,18 @@ func (e *userspaceEngine) Reconfig(cfg *wgcfg.Config, dnsDomains []string, local }) } - rs := router.RouteSettings{ + rs := router.Settings{ LocalAddrs: addrs, - Cfg: cfg, DNS: cfg.DNS, DNSDomains: dnsDomains, SubnetRoutes: localRoutes, } + for _, peer := range cfg.Peers { + rs.Routes = append(rs.Routes, peer.AllowedIPs...) + } - // TODO(apenwarr): all the parts of RouteSettings should be "relevant." - // We're checking only the "relevant" parts to see if they have - // changed, and if not, skipping SetRoutes(). But if SetRoutes() - // is getting the non-relevant parts of Cfg, it might act on them, - // and this optimization is unsafe. Probably we should not pass - // a whole Cfg object as part of RouteSettings; instead, trim it to - // just what's absolutely needed (the set of actual routes). - rss := rs.OnlyRelevantParts() - if rss != e.lastRoutes { - e.logf("wgengine: Reconfig: reconfiguring router. la=%v dns=%v dom=%v; new routes: %v", - rs.LocalAddrs, rs.DNS, rs.DNSDomains, rss) - e.lastRoutes = rss - err = e.router.SetRoutes(rs) - if err != nil { - return err - } + if err := e.router.Set(rs); err != nil { + return err } e.logf("wgengine: Reconfig done")