From 00fe8845b1e2f4ef5dedfe5989812d8beddfb1f9 Mon Sep 17 00:00:00 2001 From: Nick Khyl Date: Tue, 4 Feb 2025 14:20:06 -0600 Subject: [PATCH] ipn/{ipnauth,ipnlocal,ipnserver}: move the AlwaysOn policy check from ipnserver to ipnauth In this PR, we move the code that checks the AlwaysOn policy from ipnserver.actor to ipnauth. It is intended to be used by ipnauth.Actor implementations, and we temporarily make it exported while these implementations reside in ipnserver and in corp. We'll unexport it later. We also update [ipnauth.Actor.CheckProfileAccess] to accept an auditLogger, which is called to write details about the action to the audit log when required by the policy, and update LocalBackend.EditPrefsAs to use an auditLogger that writes to the regular backend log. Updates tailscale/corp#26146 Signed-off-by: Nick Khyl --- ipn/ipnauth/actor.go | 10 ++++++++- ipn/ipnauth/policy.go | 46 +++++++++++++++++++++++++++++++++++++++ ipn/ipnauth/self.go | 2 +- ipn/ipnauth/test_actor.go | 2 +- ipn/ipnlocal/local.go | 4 +++- ipn/ipnserver/actor.go | 17 +++------------ 6 files changed, 63 insertions(+), 18 deletions(-) create mode 100644 ipn/ipnauth/policy.go diff --git a/ipn/ipnauth/actor.go b/ipn/ipnauth/actor.go index 92e3b202f..446cb4635 100644 --- a/ipn/ipnauth/actor.go +++ b/ipn/ipnauth/actor.go @@ -10,6 +10,11 @@ import ( "tailscale.com/ipn" ) +// AuditLogFunc is any function that can be used to log audit actions performed by an [Actor]. +// +// TODO(nickkhyl,barnstar): define a named string type for the action (in tailcfg?) and use it here. +type AuditLogFunc func(action, details string) + // Actor is any actor using the [ipnlocal.LocalBackend]. // // It typically represents a specific OS user, indicating that an operation @@ -30,7 +35,10 @@ type Actor interface { // CheckProfileAccess checks whether the actor has the necessary access rights // to perform a given action on the specified Tailscale profile. // It returns an error if access is denied. - CheckProfileAccess(profile ipn.LoginProfileView, requestedAccess ProfileAccess) error + // + // If the auditLogger is non-nil, it is used to write details about the action + // to the audit log when required by the policy. + CheckProfileAccess(profile ipn.LoginProfileView, requestedAccess ProfileAccess, auditLogger AuditLogFunc) error // IsLocalSystem reports whether the actor is the Windows' Local System account. // diff --git a/ipn/ipnauth/policy.go b/ipn/ipnauth/policy.go new file mode 100644 index 000000000..c61f9cd89 --- /dev/null +++ b/ipn/ipnauth/policy.go @@ -0,0 +1,46 @@ +// Copyright (c) Tailscale Inc & AUTHORS +// SPDX-License-Identifier: BSD-3-Clause + +package ipnauth + +import ( + "errors" + "fmt" + + "tailscale.com/ipn" + "tailscale.com/util/syspolicy" +) + +// CheckDisconnectPolicy checks if the policy allows the specified actor to disconnect +// Tailscale with the given optional reason. It returns nil if the operation is allowed, +// or an error if it is not. If auditLogger is non-nil, it is called to log the action +// when required by the policy. +// +// Note: this function only checks the policy and does not check whether the actor has +// the necessary access rights to the device or profile. It is intended to be used by +// [Actor] implementations on platforms where [syspolicy] is supported. +// +// TODO(nickkhyl): unexport it when we move [ipn.Actor] implementations from [ipnserver] +// and corp to this package. +func CheckDisconnectPolicy(actor Actor, profile ipn.LoginProfileView, reason string, auditLogger AuditLogFunc) error { + if alwaysOn, _ := syspolicy.GetBoolean(syspolicy.AlwaysOn, false); !alwaysOn { + return nil + } + if allowWithReason, _ := syspolicy.GetBoolean(syspolicy.AlwaysOnOverrideWithReason, false); !allowWithReason { + return errors.New("disconnect not allowed: always-on mode is enabled") + } + if reason == "" { + return errors.New("disconnect not allowed: reason required") + } + if auditLogger != nil { + var details string + if username, _ := actor.Username(); username != "" { // best-effort; we don't have it on all platforms + details = fmt.Sprintf("%q is being disconnected by %q: %v", profile.Name(), username, reason) + } else { + details = fmt.Sprintf("%q is being disconnected: %v", profile.Name(), reason) + } + // TODO(nickkhyl,barnstar): use a const for DISCONNECT_NODE. + auditLogger("DISCONNECT_NODE", details) + } + return nil +} diff --git a/ipn/ipnauth/self.go b/ipn/ipnauth/self.go index d8ece45c5..271be9815 100644 --- a/ipn/ipnauth/self.go +++ b/ipn/ipnauth/self.go @@ -28,7 +28,7 @@ func (u unrestricted) Username() (string, error) { return "", nil } func (u unrestricted) ClientID() (_ ClientID, ok bool) { return NoClientID, false } // CheckProfileAccess implements [Actor]. -func (u unrestricted) CheckProfileAccess(_ ipn.LoginProfileView, _ ProfileAccess) error { +func (u unrestricted) CheckProfileAccess(_ ipn.LoginProfileView, _ ProfileAccess, _ AuditLogFunc) error { // Unrestricted access to all profiles. return nil } diff --git a/ipn/ipnauth/test_actor.go b/ipn/ipnauth/test_actor.go index 0d4a0e37d..ba4e03c93 100644 --- a/ipn/ipnauth/test_actor.go +++ b/ipn/ipnauth/test_actor.go @@ -31,7 +31,7 @@ func (a *TestActor) Username() (string, error) { return a.Name, a.NameErr } func (a *TestActor) ClientID() (_ ClientID, ok bool) { return a.CID, a.CID != NoClientID } // CheckProfileAccess implements [Actor]. -func (a *TestActor) CheckProfileAccess(profile ipn.LoginProfileView, _ ProfileAccess) error { +func (a *TestActor) CheckProfileAccess(profile ipn.LoginProfileView, _ ProfileAccess, _ AuditLogFunc) error { return errors.New("profile access denied") } diff --git a/ipn/ipnlocal/local.go b/ipn/ipnlocal/local.go index 373da9881..38bcfaaa2 100644 --- a/ipn/ipnlocal/local.go +++ b/ipn/ipnlocal/local.go @@ -4058,7 +4058,9 @@ func (b *LocalBackend) EditPrefsAs(mp *ipn.MaskedPrefs, actor ipnauth.Actor) (ip unlock := b.lockAndGetUnlock() defer unlock() if mp.WantRunningSet && !mp.WantRunning && b.pm.CurrentPrefs().WantRunning() { - if err := actor.CheckProfileAccess(b.pm.CurrentProfile(), ipnauth.Disconnect); err != nil { + // TODO(barnstar,nickkhyl): replace loggerFn with the actual audit logger. + loggerFn := func(action, details string) { b.logf("[audit]: %s: %s", action, details) } + if err := actor.CheckProfileAccess(b.pm.CurrentProfile(), ipnauth.Disconnect, loggerFn); err != nil { return ipn.PrefsView{}, err } diff --git a/ipn/ipnserver/actor.go b/ipn/ipnserver/actor.go index 652716670..6ee7a04d7 100644 --- a/ipn/ipnserver/actor.go +++ b/ipn/ipnserver/actor.go @@ -17,7 +17,6 @@ import ( "tailscale.com/types/logger" "tailscale.com/util/ctxkey" "tailscale.com/util/osuser" - "tailscale.com/util/syspolicy" "tailscale.com/version" ) @@ -80,7 +79,7 @@ func actorWithAccessOverride(baseActor *actor, reason string) *actor { } // CheckProfileAccess implements [ipnauth.Actor]. -func (a *actor) CheckProfileAccess(profile ipn.LoginProfileView, requestedAccess ipnauth.ProfileAccess) error { +func (a *actor) CheckProfileAccess(profile ipn.LoginProfileView, requestedAccess ipnauth.ProfileAccess, auditLogger ipnauth.AuditLogFunc) error { // TODO(nickkhyl): return errors of more specific types and have them // translated to the appropriate HTTP status codes in the API handler. if profile.LocalUserID() != a.UserID() { @@ -88,18 +87,8 @@ func (a *actor) CheckProfileAccess(profile ipn.LoginProfileView, requestedAccess } switch requestedAccess { case ipnauth.Disconnect: - if alwaysOn, _ := syspolicy.GetBoolean(syspolicy.AlwaysOn, false); alwaysOn { - if allowWithReason, _ := syspolicy.GetBoolean(syspolicy.AlwaysOnOverrideWithReason, false); !allowWithReason { - return errors.New("disconnect not allowed: always-on mode is enabled") - } - if a.accessOverrideReason == "" { - return errors.New("disconnect not allowed: reason required") - } - maybeUsername, _ := a.Username() // best-effort - a.logf("Tailscale (%q) is being disconnected by %q: %v", profile.Name(), maybeUsername, a.accessOverrideReason) - // TODO(nickkhyl): Log the reason to the audit log once we have one. - } - return nil // disconnect is allowed + // Disconnect is allowed if a user owns the profile and the policy permits it. + return ipnauth.CheckDisconnectPolicy(a, profile, a.accessOverrideReason, auditLogger) default: return errors.New("the requested operation is not allowed") }