diff --git a/ipn/ipnauth/self.go b/ipn/ipnauth/self.go index 9b430dc6d..adee06964 100644 --- a/ipn/ipnauth/self.go +++ b/ipn/ipnauth/self.go @@ -13,6 +13,11 @@ // has unlimited access. var Self Actor = unrestricted{} +// TODO is a caller identity used when the operation is performed on behalf of a user, +// rather than by tailscaled itself, but the surrounding function is not yet extended +// to accept an [Actor] parameter. It grants the same unrestricted access as [Self]. +var TODO Actor = unrestricted{} + // unrestricted is an [Actor] that has unlimited access to the currently running // tailscaled instance. It's typically used for operations performed by tailscaled // on its own, or upon a request from the control plane, rather on behalf of a user. @@ -49,3 +54,10 @@ func (unrestricted) IsLocalSystem() bool { return false } // Deprecated: this method exists for compatibility with the current (as of 2025-01-28) // permission model and will be removed as we progress on tailscale/corp#18342. func (unrestricted) IsLocalAdmin(operatorUID string) bool { return false } + +// IsTailscaled reports whether the given Actor represents Tailscaled itself, +// such as [Self] or a [TODO] placeholder actor. +func IsTailscaled(a Actor) bool { + _, ok := a.(unrestricted) + return ok +} diff --git a/ipn/ipnlocal/local.go b/ipn/ipnlocal/local.go index 0ee249dfb..03a0709e2 100644 --- a/ipn/ipnlocal/local.go +++ b/ipn/ipnlocal/local.go @@ -178,6 +178,10 @@ type watchSession struct { // It is used as a context cancellation cause for the old context // and can be returned when an operation is performed on it. errNodeContextChanged = errors.New("profile changed") + + // errManagedByPolicy indicates the operation is blocked + // because the target state is managed by a GP/MDM policy. + errManagedByPolicy = errors.New("managed by policy") ) // LocalBackend is the glue between the major pieces of the Tailscale @@ -3477,12 +3481,14 @@ func (b *LocalBackend) onTailnetDefaultAutoUpdate(au bool) { b.logf("using tailnet default auto-update setting: %v", au) prefsClone := prefs.AsStruct() prefsClone.AutoUpdate.Apply = opt.NewBool(au) - _, err := b.editPrefsLockedOnEntry(&ipn.MaskedPrefs{ - Prefs: *prefsClone, - AutoUpdateSet: ipn.AutoUpdatePrefsMask{ - ApplySet: true, - }, - }, unlock) + _, err := b.editPrefsLockedOnEntry( + ipnauth.Self, + &ipn.MaskedPrefs{ + Prefs: *prefsClone, + AutoUpdateSet: ipn.AutoUpdatePrefsMask{ + ApplySet: true, + }, + }, unlock) if err != nil { b.logf("failed to apply tailnet-wide default for auto-updates (%v): %v", au, err) return @@ -4224,7 +4230,7 @@ func (b *LocalBackend) checkAutoUpdatePrefsLocked(p *ipn.Prefs) error { // On success, it returns the resulting prefs (or current prefs, in the case of no change). // Setting the value to false when use of an exit node is already false is not an error, // nor is true when the exit node is already in use. -func (b *LocalBackend) SetUseExitNodeEnabled(v bool) (ipn.PrefsView, error) { +func (b *LocalBackend) SetUseExitNodeEnabled(actor ipnauth.Actor, v bool) (ipn.PrefsView, error) { unlock := b.lockAndGetUnlock() defer unlock() @@ -4267,7 +4273,7 @@ func (b *LocalBackend) SetUseExitNodeEnabled(v bool) (ipn.PrefsView, error) { mp.InternalExitNodePrior = p0.ExitNodeID() } } - return b.editPrefsLockedOnEntry(mp, unlock) + return b.editPrefsLockedOnEntry(actor, mp, unlock) } // MaybeClearAppConnector clears the routes from any AppConnector if @@ -4296,30 +4302,83 @@ func (b *LocalBackend) EditPrefsAs(mp *ipn.MaskedPrefs, actor ipnauth.Actor) (ip return ipn.PrefsView{}, errors.New("can't set Internal fields") } - // Zeroing the ExitNodeId via localAPI must also zero the prior exit node. - if mp.ExitNodeIDSet && mp.ExitNodeID == "" { + return b.editPrefsLockedOnEntry(actor, mp, b.lockAndGetUnlock()) +} + +// checkEditPrefsAccessLocked checks whether the current user has access +// to apply the prefs changes in mp. +// +// It returns an error if the user is not allowed, or nil otherwise. +// +// b.mu must be held. +func (b *LocalBackend) checkEditPrefsAccessLocked(actor ipnauth.Actor, mp *ipn.MaskedPrefs) error { + var errs []error + + if mp.RunSSHSet && mp.RunSSH && !envknob.CanSSHD() { + errs = append(errs, errors.New("Tailscale SSH server administratively disabled")) + } + + // Check if the user is allowed to disconnect Tailscale. + if mp.WantRunningSet && !mp.WantRunning && b.pm.CurrentPrefs().WantRunning() { + if err := actor.CheckProfileAccess(b.pm.CurrentProfile(), ipnauth.Disconnect, b.extHost.AuditLogger()); err != nil { + errs = append(errs, err) + } + } + + // Prevent users from changing exit node preferences + // when exit node usage is managed by policy. + if mp.ExitNodeIDSet || mp.ExitNodeIPSet || mp.AutoExitNodeSet { + // TODO(nickkhyl): Allow users to override ExitNode policy settings + // if the ExitNode.AllowUserOverride policy permits it. + // (Policy setting name and details are TBD. See tailscale/corp#29969) + isManaged, err := syspolicy.HasAnyOf(syspolicy.ExitNodeID, syspolicy.ExitNodeIP) + if err != nil { + err = fmt.Errorf("policy check failed: %w", err) + } else if isManaged { + err = errManagedByPolicy + } + if err != nil { + errs = append(errs, fmt.Errorf("exit node cannot be changed: %w", err)) + } + } + + return multierr.New(errs...) +} + +// adjustEditPrefsLocked applies additional changes to mp if necessary, +// such as zeroing out mutually exclusive fields. +// +// It must not assume that the changes in mp will actually be applied. +// +// b.mu must be held. +func (b *LocalBackend) adjustEditPrefsLocked(_ ipnauth.Actor, mp *ipn.MaskedPrefs) { + // Zeroing the ExitNodeID via localAPI must also zero the prior exit node. + if mp.ExitNodeIDSet && mp.ExitNodeID == "" && !mp.InternalExitNodePriorSet { mp.InternalExitNodePrior = "" mp.InternalExitNodePriorSet = true } // Disable automatic exit node selection if the user explicitly sets // ExitNodeID or ExitNodeIP. - if mp.ExitNodeIDSet || mp.ExitNodeIPSet { + if (mp.ExitNodeIDSet || mp.ExitNodeIPSet) && !mp.AutoExitNodeSet { mp.AutoExitNodeSet = true mp.AutoExitNode = "" } +} - // Acquire the lock before checking the profile access to prevent - // TOCTOU issues caused by the current profile changing between the - // check and the actual edit. - unlock := b.lockAndGetUnlock() - defer unlock() - if mp.WantRunningSet && !mp.WantRunning && b.pm.CurrentPrefs().WantRunning() { - if err := actor.CheckProfileAccess(b.pm.CurrentProfile(), ipnauth.Disconnect, b.extHost.AuditLogger()); err != nil { - b.logf("check profile access failed: %v", err) - return ipn.PrefsView{}, err - } - +// onEditPrefsLocked is called when prefs are edited (typically, via LocalAPI), +// just before the changes in newPrefs are set for the current profile. +// +// The changes in mp have been allowed, but the resulting [ipn.Prefs] +// have not yet been applied and may be subject to reconciliation +// by [LocalBackend.reconcilePrefsLocked], either before or after being set. +// +// This method handles preference edits, typically initiated by the user, +// as opposed to reconfiguring the backend when the final prefs are set. +// +// b.mu must be held; mp must not be mutated by this method. +func (b *LocalBackend) onEditPrefsLocked(_ ipnauth.Actor, mp *ipn.MaskedPrefs, oldPrefs, newPrefs ipn.PrefsView) { + if mp.WantRunningSet && !mp.WantRunning && oldPrefs.WantRunning() { // If a user has enough rights to disconnect, such as when [syspolicy.AlwaysOn] // is disabled, or [syspolicy.AlwaysOnOverrideWithReason] is also set and the user // provides a reason for disconnecting, then we should not force the "always on" @@ -4331,7 +4390,18 @@ func (b *LocalBackend) EditPrefsAs(mp *ipn.MaskedPrefs, actor ipnauth.Actor) (ip } } - return b.editPrefsLockedOnEntry(mp, unlock) + // This is recorded here in the EditPrefs path, not the setPrefs path on purpose. + // recordForEdit records metrics related to edits and changes, not the final state. + // If, in the future, we want to record gauge-metrics related to the state of prefs, + // that should be done in the setPrefs path. + e := prefsMetricsEditEvent{ + change: mp, + pNew: newPrefs, + pOld: oldPrefs, + node: b.currentNode(), + lastSuggestedExitNode: b.lastSuggestedExitNode, + } + e.record() } // startReconnectTimerLocked sets a timer to automatically set WantRunning to true @@ -4368,7 +4438,7 @@ func (b *LocalBackend) startReconnectTimerLocked(d time.Duration) { } mp := &ipn.MaskedPrefs{WantRunningSet: true, Prefs: ipn.Prefs{WantRunning: true}} - if _, err := b.editPrefsLockedOnEntry(mp, unlock); err != nil { + if _, err := b.editPrefsLockedOnEntry(ipnauth.Self, mp, unlock); err != nil { b.logf("failed to automatically reconnect as %q after %v: %v", cp.Name(), d, err) } else { b.logf("automatically reconnected as %q after %v", cp.Name(), d) @@ -4399,9 +4469,19 @@ func (b *LocalBackend) stopReconnectTimerLocked() { // Warning: b.mu must be held on entry, but it unlocks it on the way out. // TODO(bradfitz): redo the locking on all these weird methods like this. -func (b *LocalBackend) editPrefsLockedOnEntry(mp *ipn.MaskedPrefs, unlock unlockOnce) (ipn.PrefsView, error) { +func (b *LocalBackend) editPrefsLockedOnEntry(actor ipnauth.Actor, mp *ipn.MaskedPrefs, unlock unlockOnce) (ipn.PrefsView, error) { defer unlock() // for error paths + // Check if the changes in mp are allowed. + if err := b.checkEditPrefsAccessLocked(actor, mp); err != nil { + b.logf("EditPrefs(%v): %v", mp.Pretty(), err) + return ipn.PrefsView{}, err + } + + // Apply additional changes to mp if necessary, + // such as clearing mutually exclusive fields. + b.adjustEditPrefsLocked(actor, mp) + if mp.EggSet { mp.EggSet = false b.egg = true @@ -4416,29 +4496,18 @@ func (b *LocalBackend) editPrefsLockedOnEntry(mp *ipn.MaskedPrefs, unlock unlock b.logf("EditPrefs check error: %v", err) return ipn.PrefsView{}, err } - if p1.RunSSH && !envknob.CanSSHD() { - b.logf("EditPrefs requests SSH, but disabled by envknob; returning error") - return ipn.PrefsView{}, errors.New("Tailscale SSH server administratively disabled.") - } + if p1.View().Equals(p0) { return stripKeysFromPrefs(p0), nil } b.logf("EditPrefs: %v", mp.Pretty()) - newPrefs := b.setPrefsLockedOnEntry(p1, unlock) - // This is recorded here in the EditPrefs path, not the setPrefs path on purpose. - // recordForEdit records metrics related to edits and changes, not the final state. - // If, in the future, we want to record gauge-metrics related to the state of prefs, - // that should be done in the setPrefs path. - e := prefsMetricsEditEvent{ - change: mp, - pNew: p1.View(), - pOld: p0, - node: b.currentNode(), - lastSuggestedExitNode: b.lastSuggestedExitNode, - } - e.record() + // Perform any actions required when prefs are edited (typically by a user), + // before the modified prefs are actually set for the current profile. + b.onEditPrefsLocked(actor, mp, p0, p1.View()) + + newPrefs := b.setPrefsLockedOnEntry(p1, unlock) // Note: don't perform any actions for the new prefs here. Not // every prefs change goes through EditPrefs. Put your actions @@ -5829,11 +5898,16 @@ func (b *LocalBackend) Logout(ctx context.Context) error { // delete it later. profile := b.pm.CurrentProfile() - _, err := b.editPrefsLockedOnEntry(&ipn.MaskedPrefs{ - WantRunningSet: true, - LoggedOutSet: true, - Prefs: ipn.Prefs{WantRunning: false, LoggedOut: true}, - }, unlock) + // TODO(nickkhyl): change [LocalBackend.Logout] to accept an [ipnauth.Actor]. + // This will allow enforcing Always On mode when a user tries to log out + // while logged in and connected. See tailscale/corp#26249. + _, err := b.editPrefsLockedOnEntry( + ipnauth.TODO, + &ipn.MaskedPrefs{ + WantRunningSet: true, + LoggedOutSet: true, + Prefs: ipn.Prefs{WantRunning: false, LoggedOut: true}, + }, unlock) if err != nil { return err } diff --git a/ipn/ipnlocal/local_test.go b/ipn/ipnlocal/local_test.go index 3a2258cc6..1e1b7663a 100644 --- a/ipn/ipnlocal/local_test.go +++ b/ipn/ipnlocal/local_test.go @@ -501,29 +501,30 @@ func TestLazyMachineKeyGeneration(t *testing.T) { func TestZeroExitNodeViaLocalAPI(t *testing.T) { lb := newTestLocalBackend(t) + user := &ipnauth.TestActor{} // Give it an initial exit node in use. - if _, err := lb.EditPrefs(&ipn.MaskedPrefs{ + if _, err := lb.EditPrefsAs(&ipn.MaskedPrefs{ ExitNodeIDSet: true, Prefs: ipn.Prefs{ ExitNodeID: "foo", }, - }); err != nil { + }, user); err != nil { t.Fatalf("enabling first exit node: %v", err) } // SetUseExitNodeEnabled(false) "remembers" the prior exit node. - if _, err := lb.SetUseExitNodeEnabled(false); err != nil { + if _, err := lb.SetUseExitNodeEnabled(user, false); err != nil { t.Fatal("expected failure") } // Zero the exit node - pv, err := lb.EditPrefs(&ipn.MaskedPrefs{ + pv, err := lb.EditPrefsAs(&ipn.MaskedPrefs{ ExitNodeIDSet: true, Prefs: ipn.Prefs{ ExitNodeID: "", }, - }) + }, user) if err != nil { t.Fatalf("enabling first exit node: %v", err) @@ -539,29 +540,30 @@ func TestZeroExitNodeViaLocalAPI(t *testing.T) { func TestSetUseExitNodeEnabled(t *testing.T) { lb := newTestLocalBackend(t) + user := &ipnauth.TestActor{} // Can't turn it on if it never had an old value. - if _, err := lb.SetUseExitNodeEnabled(true); err == nil { + if _, err := lb.SetUseExitNodeEnabled(user, true); err == nil { t.Fatal("expected success") } // But we can turn it off when it's already off. - if _, err := lb.SetUseExitNodeEnabled(false); err != nil { + if _, err := lb.SetUseExitNodeEnabled(user, false); err != nil { t.Fatal("expected failure") } // Give it an initial exit node in use. - if _, err := lb.EditPrefs(&ipn.MaskedPrefs{ + if _, err := lb.EditPrefsAs(&ipn.MaskedPrefs{ ExitNodeIDSet: true, Prefs: ipn.Prefs{ ExitNodeID: "foo", }, - }); err != nil { + }, user); err != nil { t.Fatalf("enabling first exit node: %v", err) } // Now turn off that exit node. - if prefs, err := lb.SetUseExitNodeEnabled(false); err != nil { + if prefs, err := lb.SetUseExitNodeEnabled(user, false); err != nil { t.Fatal("expected failure") } else { if g, w := prefs.ExitNodeID(), tailcfg.StableNodeID(""); g != w { @@ -573,7 +575,7 @@ func TestSetUseExitNodeEnabled(t *testing.T) { } // And turn it back on. - if prefs, err := lb.SetUseExitNodeEnabled(true); err != nil { + if prefs, err := lb.SetUseExitNodeEnabled(user, true); err != nil { t.Fatal("expected failure") } else { if g, w := prefs.ExitNodeID(), tailcfg.StableNodeID("foo"); g != w { @@ -585,9 +587,9 @@ func TestSetUseExitNodeEnabled(t *testing.T) { } // Verify we block setting an Internal field. - if _, err := lb.EditPrefs(&ipn.MaskedPrefs{ + if _, err := lb.EditPrefsAs(&ipn.MaskedPrefs{ InternalExitNodePriorSet: true, - }); err == nil { + }, user); err == nil { t.Fatalf("unexpected success; want an error trying to set an internal field") } } @@ -612,16 +614,18 @@ func TestConfigureExitNode(t *testing.T) { } tests := []struct { - name string - prefs ipn.Prefs - netMap *netmap.NetworkMap - report *netcheck.Report - changePrefs *ipn.MaskedPrefs - useExitNodeEnabled *bool - exitNodeIDPolicy *tailcfg.StableNodeID - exitNodeIPPolicy *netip.Addr - exitNodeAllowedIDs []tailcfg.StableNodeID // nil if all IDs are allowed for auto exit nodes - wantPrefs ipn.Prefs + name string + prefs ipn.Prefs + netMap *netmap.NetworkMap + report *netcheck.Report + changePrefs *ipn.MaskedPrefs + useExitNodeEnabled *bool + exitNodeIDPolicy *tailcfg.StableNodeID + exitNodeIPPolicy *netip.Addr + exitNodeAllowedIDs []tailcfg.StableNodeID // nil if all IDs are allowed for auto exit nodes + wantChangePrefsErr error // if non-nil, the error we expect from [LocalBackend.EditPrefsAs] + wantPrefs ipn.Prefs + wantExitNodeToggleErr error // if non-nil, the error we expect from [LocalBackend.SetUseExitNodeEnabled] }{ { name: "exit-node-id-via-prefs", // set exit node ID via prefs @@ -804,6 +808,7 @@ func TestConfigureExitNode(t *testing.T) { ControlURL: controlURL, ExitNodeID: exitNode1.StableID(), }, + wantChangePrefsErr: errManagedByPolicy, }, { name: "id-via-policy/cannot-override-via-prefs/by-ip", // syspolicy should take precedence over prefs @@ -822,6 +827,7 @@ func TestConfigureExitNode(t *testing.T) { ControlURL: controlURL, ExitNodeID: exitNode1.StableID(), }, + wantChangePrefsErr: errManagedByPolicy, }, { name: "id-via-policy/cannot-override-via-prefs/by-auto-expr", // syspolicy should take precedence over prefs @@ -840,6 +846,7 @@ func TestConfigureExitNode(t *testing.T) { ControlURL: controlURL, ExitNodeID: exitNode1.StableID(), }, + wantChangePrefsErr: errManagedByPolicy, }, { name: "ip-via-policy", // set exit node IP via syspolicy (should be resolved to an ID) @@ -999,15 +1006,16 @@ func TestConfigureExitNode(t *testing.T) { prefs: ipn.Prefs{ ControlURL: controlURL, }, - netMap: clientNetmap, - report: report, - exitNodeIDPolicy: ptr.To(tailcfg.StableNodeID("auto:any")), - useExitNodeEnabled: ptr.To(false), // should be ignored + netMap: clientNetmap, + report: report, + exitNodeIDPolicy: ptr.To(tailcfg.StableNodeID("auto:any")), + useExitNodeEnabled: ptr.To(false), // should fail with an error + wantExitNodeToggleErr: errManagedByPolicy, wantPrefs: ipn.Prefs{ ControlURL: controlURL, ExitNodeID: exitNode1.StableID(), // still enforced by the policy setting AutoExitNode: "any", - InternalExitNodePrior: "auto:any", + InternalExitNodePrior: "", }, }, } @@ -1046,14 +1054,17 @@ func TestConfigureExitNode(t *testing.T) { lb.SetControlClientStatus(lb.cc, controlclient.Status{NetMap: tt.netMap}) } + user := &ipnauth.TestActor{} // If we have a changePrefs, apply it. if tt.changePrefs != nil { - lb.EditPrefs(tt.changePrefs) + _, err := lb.EditPrefsAs(tt.changePrefs, user) + checkError(t, err, tt.wantChangePrefsErr, true) } // If we need to flip exit node toggle on or off, do it. if tt.useExitNodeEnabled != nil { - lb.SetUseExitNodeEnabled(*tt.useExitNodeEnabled) + _, err := lb.SetUseExitNodeEnabled(user, *tt.useExitNodeEnabled) + checkError(t, err, tt.wantExitNodeToggleErr, true) } // Now check the prefs. @@ -6218,6 +6229,18 @@ type test struct { } } +func checkError(tb testing.TB, got, want error, fatal bool) { + tb.Helper() + f := tb.Errorf + if fatal { + f = tb.Fatalf + } + if (want == nil) != (got == nil) || + (want != nil && got != nil && want.Error() != got.Error() && !errors.Is(got, want)) { + f("gotErr: %v; wantErr: %v", got, want) + } +} + func toStrings[T ~string](in []T) []string { out := make([]string, len(in)) for i, v := range in { diff --git a/ipn/localapi/localapi.go b/ipn/localapi/localapi.go index a90ae5d84..d4b4b443e 100644 --- a/ipn/localapi/localapi.go +++ b/ipn/localapi/localapi.go @@ -1910,7 +1910,7 @@ func (h *Handler) serveSetUseExitNodeEnabled(w http.ResponseWriter, r *http.Requ http.Error(w, "invalid 'enabled' parameter", http.StatusBadRequest) return } - prefs, err := h.b.SetUseExitNodeEnabled(v) + prefs, err := h.b.SetUseExitNodeEnabled(h.Actor, v) if err != nil { http.Error(w, err.Error(), http.StatusInternalServerError) return diff --git a/util/syspolicy/syspolicy.go b/util/syspolicy/syspolicy.go index a84afa5db..6555a58ac 100644 --- a/util/syspolicy/syspolicy.go +++ b/util/syspolicy/syspolicy.go @@ -72,7 +72,10 @@ func HasAnyOf(keys ...Key) (bool, error) { if errors.Is(err, setting.ErrNotConfigured) || errors.Is(err, setting.ErrNoSuchKey) { continue } - return err == nil, err // err may be nil or non-nil + if err != nil { + return false, err + } + return true, nil } return false, nil }