diff --git a/appc/appconnector.go b/appc/appconnector.go index 9b8398811..d60d0198c 100644 --- a/appc/appconnector.go +++ b/appc/appconnector.go @@ -206,9 +206,8 @@ func (e *AppConnector) ObserveDNSResponse(res []byte) { if slices.Contains(addrs, addr) { continue } - // TODO(raggi): check for existing prefixes if err := e.routeAdvertiser.AdvertiseRoute(netip.PrefixFrom(addr, addr.BitLen())); err != nil { - e.logf("failed to advertise route for %v: %v", addr, err) + e.logf("failed to advertise route for %s: %v: %v", domain, addr, err) continue } e.logf("[v2] advertised route for %v: %v", domain, addr) @@ -217,5 +216,4 @@ func (e *AppConnector) ObserveDNSResponse(res []byte) { e.domains[domain] = append(addrs, addr) e.mu.Unlock() } - } diff --git a/ipn/ipnlocal/local.go b/ipn/ipnlocal/local.go index 1c81ba045..4f72a305e 100644 --- a/ipn/ipnlocal/local.go +++ b/ipn/ipnlocal/local.go @@ -774,7 +774,7 @@ func (b *LocalBackend) UpdateStatus(sb *ipnstate.StatusBuilder) { } if !prefs.ExitNodeID().IsZero() { if exitPeer, ok := b.netMap.PeerWithStableID(prefs.ExitNodeID()); ok { - var online = false + online := false if v := exitPeer.Online(); v != nil { online = *v } @@ -855,7 +855,7 @@ func (b *LocalBackend) populatePeerStatusLocked(sb *ipnstate.StatusBuilder) { if p.LastSeen() != nil { lastSeen = *p.LastSeen() } - var tailscaleIPs = make([]netip.Addr, 0, p.Addresses().Len()) + tailscaleIPs := make([]netip.Addr, 0, p.Addresses().Len()) for i := range p.Addresses().LenIter() { addr := p.Addresses().At(i) if addr.IsSingleIP() && tsaddr.IsTailscaleIP(addr.Addr()) { @@ -4200,7 +4200,6 @@ func (b *LocalBackend) enterStateLockedOnEntry(newState ipn.State) { default: b.logf("[unexpected] unknown newState %#v", newState) } - } // hasNodeKey reports whether a non-zero node key is present in the current @@ -5781,12 +5780,21 @@ func (b *LocalBackend) ObserveDNSResponse(res []byte) { appConnector.ObserveDNSResponse(res) } +// ErrDisallowedAutoRoute is returned by AdvertiseRoute when a route that is not allowed is requested. +var ErrDisallowedAutoRoute = errors.New("route is not allowed") + // AdvertiseRoute implements the appc.RouteAdvertiser interface. It sets a new // route advertisement if one is not already present in the existing routes. +// If the route is disallowed, ErrDisallowedAutoRoute is returned. func (b *LocalBackend) AdvertiseRoute(ipp netip.Prefix) error { + if !allowedAutoRoute(ipp) { + return ErrDisallowedAutoRoute + } currentRoutes := b.Prefs().AdvertiseRoutes() - // TODO(raggi): check if the new route is a subset of an existing route. - if currentRoutes.ContainsFunc(func(r netip.Prefix) bool { return r == ipp }) { + if currentRoutes.ContainsFunc(func(r netip.Prefix) bool { + // TODO(raggi): add support for subset checks and avoid subset route creations. + return ipp.IsSingleIP() && r.Contains(ipp.Addr()) || r == ipp + }) { return nil } routes := append(currentRoutes.AsSlice(), ipp) @@ -5799,6 +5807,36 @@ func (b *LocalBackend) AdvertiseRoute(ipp netip.Prefix) error { return err } +var ( + disallowedAddrs = []netip.Addr{ + netip.MustParseAddr("::1"), + netip.MustParseAddr("::"), + netip.MustParseAddr("0.0.0.0"), + } + disallowedRanges = []netip.Prefix{ + netip.MustParsePrefix("127.0.0.0/8"), + netip.MustParsePrefix("224.0.0.0/4"), + netip.MustParsePrefix("ff00::/8"), + } +) + +// allowedAutoRoute determines if the route being added via AdvertiseRoute (the app connector featuge) should be allowed. +func allowedAutoRoute(ipp netip.Prefix) bool { + // Note: blocking the addrs for globals, not solely the prefixes. + for _, addr := range disallowedAddrs { + if ipp.Addr() == addr { + return false + } + } + for _, pfx := range disallowedRanges { + if pfx.Overlaps(ipp) { + return false + } + } + // TODO(raggi): exclude tailscale service IPs and so on as well. + return true +} + // mayDeref dereferences p if non-nil, otherwise it returns the zero value. func mayDeref[T any](p *T) (v T) { if p == nil { diff --git a/ipn/ipnlocal/local_test.go b/ipn/ipnlocal/local_test.go index 71d08822b..17d374866 100644 --- a/ipn/ipnlocal/local_test.go +++ b/ipn/ipnlocal/local_test.go @@ -265,7 +265,6 @@ func TestPeerRoutes(t *testing.T) { } }) } - } func TestPeerAPIBase(t *testing.T) { @@ -700,7 +699,6 @@ func TestPacketFilterPermitsUnlockedNodes(t *testing.T) { } }) } - } func TestStatusWithoutPeers(t *testing.T) { @@ -1173,6 +1171,26 @@ func TestRouteAdvertiser(t *testing.T) { } } +func TestRouterAdvertiserIgnoresContainedRoutes(t *testing.T) { + b := newTestBackend(t) + testPrefix := netip.MustParsePrefix("192.0.0.0/24") + ra := appc.RouteAdvertiser(b) + must.Do(ra.AdvertiseRoute(testPrefix)) + + routes := b.Prefs().AdvertiseRoutes() + if routes.Len() != 1 || routes.At(0) != testPrefix { + t.Fatalf("got routes %v, want %v", routes, []netip.Prefix{testPrefix}) + } + + must.Do(ra.AdvertiseRoute(netip.MustParsePrefix("192.0.0.8/32"))) + + // the above /32 is not added as it is contained within the /24 + routes = b.Prefs().AdvertiseRoutes() + if routes.Len() != 1 || routes.At(0) != testPrefix { + t.Fatalf("got routes %v, want %v", routes, []netip.Prefix{testPrefix}) + } +} + func TestObserveDNSResponse(t *testing.T) { b := newTestBackend(t) @@ -1886,7 +1904,6 @@ func TestApplySysPolicy(t *testing.T) { }) t.Run("set prefs", func(t *testing.T) { - b := newTestBackend(t) b.SetPrefs(tt.prefs.Clone()) if !b.Prefs().Equals(tt.wantPrefs.View()) {