diff --git a/cmd/tailscale/cli/up.go b/cmd/tailscale/cli/up.go index 3aa66865f..43f36f819 100644 --- a/cmd/tailscale/cli/up.go +++ b/cmd/tailscale/cli/up.go @@ -652,6 +652,7 @@ func upWorthyWarning(s string) bool { return strings.Contains(s, healthmsg.TailscaleSSHOnBut) || strings.Contains(s, healthmsg.WarnAcceptRoutesOff) || strings.Contains(s, healthmsg.LockedOut) || + strings.Contains(s, healthmsg.WarnExitNodeUsage) || strings.Contains(strings.ToLower(s), "update available: ") } diff --git a/health/health.go b/health/health.go index c040b8b03..c1ca46bc6 100644 --- a/health/health.go +++ b/health/health.go @@ -103,6 +103,16 @@ func WithMapDebugFlag(name string) WarnableOpt { }) } +// WithConnectivityImpact returns an option which makes a Warnable annotated as +// something that could be breaking external network connectivity on the +// machine. This will make the warnable returned by OverallError alongside +// network connectivity errors. +func WithConnectivityImpact() WarnableOpt { + return warnOptFunc(func(w *Warnable) { + w.hasConnectivityImpact = true + }) +} + type warnOptFunc func(*Warnable) func (f warnOptFunc) mod(w *Warnable) { f(w) } @@ -112,6 +122,10 @@ func (f warnOptFunc) mod(w *Warnable) { f(w) } type Warnable struct { debugFlag string // optional MapRequest.DebugFlag to send when unhealthy + // If true, this warning is related to configuration of networking stack + // on the machine that impacts connectivity. + hasConnectivityImpact bool + isSet atomic.Bool mu sync.Mutex err error @@ -442,9 +456,35 @@ func OverallError() error { var fakeErrForTesting = envknob.RegisterString("TS_DEBUG_FAKE_HEALTH_ERROR") +// networkErrorf creates an error that indicates issues with outgoing network +// connectivity. Any active warnings related to network connectivity will +// automatically be appended to it. +func networkErrorf(format string, a ...any) error { + errs := []error{ + fmt.Errorf(format, a...), + } + for w := range warnables { + if !w.hasConnectivityImpact { + continue + } + if err := w.get(); err != nil { + errs = append(errs, err) + } + } + if len(errs) == 1 { + return errs[0] + } + return multierr.New(errs...) +} + +var errNetworkDown = networkErrorf("network down") +var errNotInMapPoll = networkErrorf("not in map poll") +var errNoDERPHome = errors.New("no DERP home") +var errNoUDP4Bind = networkErrorf("no udp4 bind") + func overallErrorLocked() error { if !anyInterfaceUp { - return errors.New("network down") + return errNetworkDown } if localLogConfigErr != nil { return localLogConfigErr @@ -457,26 +497,26 @@ func overallErrorLocked() error { } now := time.Now() if !inMapPoll && (lastMapPollEndedAt.IsZero() || now.Sub(lastMapPollEndedAt) > 10*time.Second) { - return errors.New("not in map poll") + return errNotInMapPoll } const tooIdle = 2*time.Minute + 5*time.Second if d := now.Sub(lastStreamedMapResponse).Round(time.Second); d > tooIdle { - return fmt.Errorf("no map response in %v", d) + return networkErrorf("no map response in %v", d) } if !derpHomeless { rid := derpHomeRegion if rid == 0 { - return errors.New("no DERP home") + return errNoDERPHome } if !derpRegionConnected[rid] { - return fmt.Errorf("not connected to home DERP region %v", rid) + return networkErrorf("not connected to home DERP region %v", rid) } if d := now.Sub(derpRegionLastFrame[rid]).Round(time.Second); d > tooIdle { - return fmt.Errorf("haven't heard from home DERP region %v in %v", rid, d) + return networkErrorf("haven't heard from home DERP region %v in %v", rid, d) } } if udp4Unbound { - return errors.New("no udp4 bind") + return errNoUDP4Bind } // TODO: use diff --git a/health/healthmsg/healthmsg.go b/health/healthmsg/healthmsg.go index e2915a195..6c237678e 100644 --- a/health/healthmsg/healthmsg.go +++ b/health/healthmsg/healthmsg.go @@ -11,4 +11,5 @@ WarnAcceptRoutesOff = "Some peers are advertising routes but --accept-routes is false" TailscaleSSHOnBut = "Tailscale SSH enabled, but " // + ... something from caller LockedOut = "this node is locked out; it will not have connectivity until it is signed. For more info, see https://tailscale.com/s/locked-out" + WarnExitNodeUsage = "The following issues on your machine will likely make usage of exit nodes impossible" ) diff --git a/ipn/ipnlocal/local.go b/ipn/ipnlocal/local.go index 8b372e878..29c5a75ad 100644 --- a/ipn/ipnlocal/local.go +++ b/ipn/ipnlocal/local.go @@ -608,6 +608,7 @@ func (b *LocalBackend) linkChange(delta *netmon.ChangeDelta) { // If the local network configuration has changed, our filter may // need updating to tweak default routes. b.updateFilterLocked(b.netMap, b.pm.CurrentPrefs()) + updateExitNodeUsageWarning(b.pm.CurrentPrefs(), delta.New) if peerAPIListenAsync && b.netMap != nil && b.state == ipn.Running { want := b.netMap.GetAddresses().Len() @@ -3086,6 +3087,22 @@ func (b *LocalBackend) isDefaultServerLocked() bool { return prefs.ControlURLOrDefault() == ipn.DefaultControlURL } +var warnExitNodeUsage = health.NewWarnable(health.WithConnectivityImpact()) + +// updateExitNodeUsageWarning updates a warnable meant to notify users of +// configuration issues that could break exit node usage. +func updateExitNodeUsageWarning(p ipn.PrefsView, state *interfaces.State) { + var result error + if p.ExitNodeIP().IsValid() || p.ExitNodeID() != "" { + warn, _ := netutil.CheckReversePathFiltering(state) + const comment = "please set rp_filter=2 instead of rp_filter=1; see https://github.com/tailscale/tailscale/issues/3310" + if len(warn) > 0 { + result = fmt.Errorf("%s: %v, %s", healthmsg.WarnExitNodeUsage, warn, comment) + } + } + warnExitNodeUsage.Set(result) +} + func (b *LocalBackend) checkExitNodePrefsLocked(p *ipn.Prefs) error { if (p.ExitNodeIP.IsValid() || p.ExitNodeID != "") && p.AdvertisesExitNode() { return errors.New("Cannot advertise an exit node and use an exit node at the same time.") diff --git a/net/netutil/ip_forward.go b/net/netutil/ip_forward.go index 691743d80..9699f34c5 100644 --- a/net/netutil/ip_forward.go +++ b/net/netutil/ip_forward.go @@ -153,7 +153,7 @@ func CheckIPForwarding(routes []netip.Prefix, state *interfaces.State) (warn, er // This function returns an error if it is unable to determine whether reverse // path filtering is enabled, or a warning describing configuration issues if // reverse path fitering is non-functional or partly functional. -func CheckReversePathFiltering(routes []netip.Prefix, state *interfaces.State) (warn []string, err error) { +func CheckReversePathFiltering(state *interfaces.State) (warn []string, err error) { if runtime.GOOS != "linux" { return nil, nil } @@ -166,12 +166,6 @@ func CheckReversePathFiltering(routes []netip.Prefix, state *interfaces.State) ( } } - // Reverse path filtering as a syscall is only implemented on Linux for IPv4. - wantV4, _ := protocolsRequiredForForwarding(routes, state) - if !wantV4 { - return nil, nil - } - // The kernel uses the maximum value for rp_filter between the 'all' // setting and each per-interface config, so we need to fetch both. allSetting, err := reversePathFilterValueLinux("all") @@ -205,7 +199,7 @@ func CheckReversePathFiltering(routes []netip.Prefix, state *interfaces.State) ( iSetting = allSetting } if iSetting == filtStrict { - warn = append(warn, fmt.Sprintf("Interface %q has strict reverse-path filtering enabled", iface.Name)) + warn = append(warn, fmt.Sprintf("interface %q has strict reverse-path filtering enabled", iface.Name)) } } return warn, nil diff --git a/net/netutil/netutil_test.go b/net/netutil/netutil_test.go index 6c97f610a..3fc46f315 100644 --- a/net/netutil/netutil_test.go +++ b/net/netutil/netutil_test.go @@ -6,7 +6,6 @@ import ( "io" "net" - "net/netip" "runtime" "testing" ) @@ -71,9 +70,7 @@ func TestCheckReversePathFiltering(t *testing.T) { if runtime.GOOS != "linux" { t.Skipf("skipping on %s", runtime.GOOS) } - warn, err := CheckReversePathFiltering([]netip.Prefix{ - netip.MustParsePrefix("192.168.1.1/24"), - }, nil) + warn, err := CheckReversePathFiltering(nil) t.Logf("err: %v", err) t.Logf("warnings: %v", warn) }