From 874db2173b26894b6b48de95fcb462a8c006f7e4 Mon Sep 17 00:00:00 2001 From: Nick Khyl Date: Sun, 13 Oct 2024 11:36:46 -0500 Subject: [PATCH] ipn/{ipnauth,ipnlocal,ipnserver}: send the auth URL to the user who started interactive login We add the ClientID() method to the ipnauth.Actor interface and updated ipnserver.actor to implement it. This method returns a unique ID of the connected client if the actor represents one. It helps link a series of interactions initiated by the client, such as when a notification needs to be sent back to a specific session, rather than all active sessions, in response to a certain request. We also add LocalBackend.WatchNotificationsAs and LocalBackend.StartLoginInteractiveAs methods, which are like WatchNotifications and StartLoginInteractive but accept an additional parameter specifying an ipnauth.Actor who initiates the operation. We store these actor identities in watchSession.owner and LocalBackend.authActor, respectively,and implement LocalBackend.sendTo and related helper methods to enable sending notifications to watchSessions associated with actors (or, more broadly, identifiable recipients). We then use the above to change who receives the BrowseToURL notifications: - For user-initiated, interactive logins, the notification is delivered only to the user who initiated the process. If the initiating actor represents a specific connected client, the URL notification is sent back to the same LocalAPI client that called StartLoginInteractive. Otherwise, the notification is sent to all clients connected as that user. Currently, we only differentiate between users on Windows, as it is inherently a multi-user OS. - In all other cases (e.g., node key expiration), we send the notification to all connected users. Updates tailscale/corp#18342 Signed-off-by: Nick Khyl --- ipn/ipnauth/actor.go | 31 ++ ipn/ipnauth/ipnauth_notwindows.go | 4 +- ipn/ipnauth/test_actor.go | 36 ++ ipn/ipnlocal/local.go | 158 +++++++-- ipn/ipnlocal/local_test.go | 540 ++++++++++++++++++++++++++++++ ipn/ipnserver/actor.go | 23 +- ipn/localapi/localapi.go | 4 +- ipn/localapi/localapi_test.go | 19 +- 8 files changed, 762 insertions(+), 53 deletions(-) create mode 100644 ipn/ipnauth/test_actor.go diff --git a/ipn/ipnauth/actor.go b/ipn/ipnauth/actor.go index db3192c91..107017268 100644 --- a/ipn/ipnauth/actor.go +++ b/ipn/ipnauth/actor.go @@ -4,6 +4,8 @@ package ipnauth import ( + "fmt" + "tailscale.com/ipn" ) @@ -20,6 +22,9 @@ type Actor interface { // Username returns the user name associated with the receiver, // or "" if the actor does not represent a specific user. Username() (string, error) + // ClientID returns a non-zero ClientID and true if the actor represents + // a connected LocalAPI client. Otherwise, it returns a zero value and false. + ClientID() (_ ClientID, ok bool) // IsLocalSystem reports whether the actor is the Windows' Local System account. // @@ -45,3 +50,29 @@ type ActorCloser interface { // Close releases resources associated with the receiver. Close() error } + +// ClientID is an opaque, comparable value used to identify a connected LocalAPI +// client, such as a connected Tailscale GUI or CLI. It does not necessarily +// correspond to the same [net.Conn] or any physical session. +// +// Its zero value is valid, but does not represent a specific connected client. +type ClientID struct { + v any +} + +// NoClientID is the zero value of [ClientID]. +var NoClientID ClientID + +// ClientIDFrom returns a new [ClientID] derived from the specified value. +// ClientIDs derived from equal values are equal. +func ClientIDFrom[T comparable](v T) ClientID { + return ClientID{v} +} + +// String implements [fmt.Stringer]. +func (id ClientID) String() string { + if id.v == nil { + return "(none)" + } + return fmt.Sprint(id.v) +} diff --git a/ipn/ipnauth/ipnauth_notwindows.go b/ipn/ipnauth/ipnauth_notwindows.go index 3dad8233a..d9d11bd0a 100644 --- a/ipn/ipnauth/ipnauth_notwindows.go +++ b/ipn/ipnauth/ipnauth_notwindows.go @@ -18,7 +18,9 @@ import ( func GetConnIdentity(_ logger.Logf, c net.Conn) (ci *ConnIdentity, err error) { ci = &ConnIdentity{conn: c, notWindows: true} _, ci.isUnixSock = c.(*net.UnixConn) - ci.creds, _ = peercred.Get(c) + if ci.creds, _ = peercred.Get(c); ci.creds != nil { + ci.pid, _ = ci.creds.PID() + } return ci, nil } diff --git a/ipn/ipnauth/test_actor.go b/ipn/ipnauth/test_actor.go new file mode 100644 index 000000000..d38aa2196 --- /dev/null +++ b/ipn/ipnauth/test_actor.go @@ -0,0 +1,36 @@ +// Copyright (c) Tailscale Inc & AUTHORS +// SPDX-License-Identifier: BSD-3-Clause + +package ipnauth + +import ( + "tailscale.com/ipn" +) + +var _ Actor = (*TestActor)(nil) + +// TestActor is an [Actor] used exclusively for testing purposes. +type TestActor struct { + UID ipn.WindowsUserID // OS-specific UID of the user, if the actor represents a local Windows user + Name string // username associated with the actor, or "" + NameErr error // error to be returned by [TestActor.Username] + CID ClientID // non-zero if the actor represents a connected LocalAPI client + LocalSystem bool // whether the actor represents the special Local System account on Windows + LocalAdmin bool // whether the actor has local admin access + +} + +// UserID implements [Actor]. +func (a *TestActor) UserID() ipn.WindowsUserID { return a.UID } + +// Username implements [Actor]. +func (a *TestActor) Username() (string, error) { return a.Name, a.NameErr } + +// ClientID implements [Actor]. +func (a *TestActor) ClientID() (_ ClientID, ok bool) { return a.CID, a.CID != NoClientID } + +// IsLocalSystem implements [Actor]. +func (a *TestActor) IsLocalSystem() bool { return a.LocalSystem } + +// IsLocalAdmin implements [Actor]. +func (a *TestActor) IsLocalAdmin(operatorUID string) bool { return a.LocalAdmin } diff --git a/ipn/ipnlocal/local.go b/ipn/ipnlocal/local.go index c7df4333b..b01f3a0c0 100644 --- a/ipn/ipnlocal/local.go +++ b/ipn/ipnlocal/local.go @@ -155,10 +155,12 @@ func RegisterNewSSHServer(fn newSSHServerFunc) { newSSHServer = fn } -// watchSession represents a WatchNotifications channel +// watchSession represents a WatchNotifications channel, +// an [ipnauth.Actor] that owns it (e.g., a connected GUI/CLI), // and sessionID as required to close targeted buses. type watchSession struct { ch chan *ipn.Notify + owner ipnauth.Actor // or nil sessionID string cancel func() // call to signal that the session must be terminated } @@ -265,9 +267,9 @@ type LocalBackend struct { endpoints []tailcfg.Endpoint blocked bool keyExpired bool - authURL string // non-empty if not Running - authURLTime time.Time // when the authURL was received from the control server - interact bool // indicates whether a user requested interactive login + authURL string // non-empty if not Running + authURLTime time.Time // when the authURL was received from the control server + authActor ipnauth.Actor // an actor who called [LocalBackend.StartLoginInteractive] last, or nil egg bool prevIfState *netmon.State peerAPIServer *peerAPIServer // or nil @@ -2129,10 +2131,10 @@ func (b *LocalBackend) Start(opts ipn.Options) error { blid := b.backendLogID.String() b.logf("Backend: logs: be:%v fe:%v", blid, opts.FrontendLogID) - b.sendLocked(ipn.Notify{ + b.sendToLocked(ipn.Notify{ BackendLogID: &blid, Prefs: &prefs, - }) + }, allClients) if !loggedOut && (b.hasNodeKeyLocked() || confWantRunning) { // If we know that we're either logged in or meant to be @@ -2657,10 +2659,15 @@ func applyConfigToHostinfo(hi *tailcfg.Hostinfo, c *conffile.Config) { // notifications. There is currently (2022-11-22) no mechanism provided to // detect when a message has been dropped. func (b *LocalBackend) WatchNotifications(ctx context.Context, mask ipn.NotifyWatchOpt, onWatchAdded func(), fn func(roNotify *ipn.Notify) (keepGoing bool)) { - ch := make(chan *ipn.Notify, 128) + b.WatchNotificationsAs(ctx, nil, mask, onWatchAdded, fn) +} +// WatchNotificationsAs is like WatchNotifications but takes an [ipnauth.Actor] +// as an additional parameter. If non-nil, the specified callback is invoked +// only for notifications relevant to this actor. +func (b *LocalBackend) WatchNotificationsAs(ctx context.Context, actor ipnauth.Actor, mask ipn.NotifyWatchOpt, onWatchAdded func(), fn func(roNotify *ipn.Notify) (keepGoing bool)) { + ch := make(chan *ipn.Notify, 128) sessionID := rands.HexString(16) - origFn := fn if mask&ipn.NotifyNoPrivateKeys != 0 { fn = func(n *ipn.Notify) bool { @@ -2712,6 +2719,7 @@ func (b *LocalBackend) WatchNotifications(ctx context.Context, mask ipn.NotifyWa session := &watchSession{ ch: ch, + owner: actor, sessionID: sessionID, cancel: cancel, } @@ -2834,13 +2842,71 @@ func (b *LocalBackend) DebugPickNewDERP() error { // // b.mu must not be held. func (b *LocalBackend) send(n ipn.Notify) { + b.sendTo(n, allClients) +} + +// notificationTarget describes a notification recipient. +// A zero value is valid and indicate that the notification +// should be broadcast to all active [watchSession]s. +type notificationTarget struct { + // userID is the OS-specific UID of the target user. + // If empty, the notification is not user-specific and + // will be broadcast to all connected users. + // TODO(nickkhyl): make this field cross-platform rather + // than Windows-specific. + userID ipn.WindowsUserID + // clientID identifies a client that should be the exclusive recipient + // of the notification. A zero value indicates that notification should + // be sent to all sessions of the specified user. + clientID ipnauth.ClientID +} + +var allClients = notificationTarget{} // broadcast to all connected clients + +// toNotificationTarget returns a [notificationTarget] that matches only actors +// representing the same user as the specified actor. If the actor represents +// a specific connected client, the [ipnauth.ClientID] must also match. +// If the actor is nil, the [notificationTarget] matches all actors. +func toNotificationTarget(actor ipnauth.Actor) notificationTarget { + t := notificationTarget{} + if actor != nil { + t.userID = actor.UserID() + t.clientID, _ = actor.ClientID() + } + return t +} + +// match reports whether the specified actor should receive notifications +// targeting t. If the actor is nil, it should only receive notifications +// intended for all users. +func (t notificationTarget) match(actor ipnauth.Actor) bool { + if t == allClients { + return true + } + if actor == nil { + return false + } + if t.userID != "" && t.userID != actor.UserID() { + return false + } + if t.clientID != ipnauth.NoClientID { + clientID, ok := actor.ClientID() + if !ok || clientID != t.clientID { + return false + } + } + return true +} + +// sendTo is like [LocalBackend.send] but allows specifying a recipient. +func (b *LocalBackend) sendTo(n ipn.Notify, recipient notificationTarget) { b.mu.Lock() defer b.mu.Unlock() - b.sendLocked(n) + b.sendToLocked(n, recipient) } -// sendLocked is like send, but assumes b.mu is already held. -func (b *LocalBackend) sendLocked(n ipn.Notify) { +// sendToLocked is like [LocalBackend.sendTo], but assumes b.mu is already held. +func (b *LocalBackend) sendToLocked(n ipn.Notify, recipient notificationTarget) { if n.Prefs != nil { n.Prefs = ptr.To(stripKeysFromPrefs(*n.Prefs)) } @@ -2854,10 +2920,12 @@ func (b *LocalBackend) sendLocked(n ipn.Notify) { } for _, sess := range b.notifyWatchers { - select { - case sess.ch <- &n: - default: - // Drop the notification if the channel is full. + if recipient.match(sess.owner) { + select { + case sess.ch <- &n: + default: + // Drop the notification if the channel is full. + } } } } @@ -2892,15 +2960,18 @@ func (b *LocalBackend) sendFileNotify() { // This method is called when a new authURL is received from the control plane, meaning that either a user // has started a new interactive login (e.g., by running `tailscale login` or clicking Login in the GUI), // or the control plane was unable to authenticate this node non-interactively (e.g., due to key expiration). -// b.interact indicates whether an interactive login is in progress. +// A non-nil b.authActor indicates that an interactive login is in progress and was initiated by the specified actor. // If url is "", it is equivalent to calling [LocalBackend.resetAuthURLLocked] with b.mu held. func (b *LocalBackend) setAuthURL(url string) { var popBrowser, keyExpired bool + var recipient ipnauth.Actor b.mu.Lock() switch { case url == "": b.resetAuthURLLocked() + b.mu.Unlock() + return case b.authURL != url: b.authURL = url b.authURLTime = b.clock.Now() @@ -2909,26 +2980,27 @@ func (b *LocalBackend) setAuthURL(url string) { popBrowser = true default: // Otherwise, only open it if the user explicitly requests interactive login. - popBrowser = b.interact + popBrowser = b.authActor != nil } keyExpired = b.keyExpired + recipient = b.authActor // or nil // Consume the StartLoginInteractive call, if any, that caused the control // plane to send us this URL. - b.interact = false + b.authActor = nil b.mu.Unlock() if popBrowser { - b.popBrowserAuthNow(url, keyExpired) + b.popBrowserAuthNow(url, keyExpired, recipient) } } -// popBrowserAuthNow shuts down the data plane and sends an auth URL -// to the connected frontend, if any. +// popBrowserAuthNow shuts down the data plane and sends the URL to the recipient's +// [watchSession]s if the recipient is non-nil; otherwise, it sends the URL to all watchSessions. // keyExpired is the value of b.keyExpired upon entry and indicates // whether the node's key has expired. // It must not be called with b.mu held. -func (b *LocalBackend) popBrowserAuthNow(url string, keyExpired bool) { - b.logf("popBrowserAuthNow: url=%v, key-expired=%v, seamless-key-renewal=%v", url != "", keyExpired, b.seamlessRenewalEnabled()) +func (b *LocalBackend) popBrowserAuthNow(url string, keyExpired bool, recipient ipnauth.Actor) { + b.logf("popBrowserAuthNow(%q): url=%v, key-expired=%v, seamless-key-renewal=%v", maybeUsernameOf(recipient), url != "", keyExpired, b.seamlessRenewalEnabled()) // Deconfigure the local network data plane if: // - seamless key renewal is not enabled; @@ -2937,7 +3009,7 @@ func (b *LocalBackend) popBrowserAuthNow(url string, keyExpired bool) { b.blockEngineUpdates(true) b.stopEngineAndWait() } - b.tellClientToBrowseToURL(url) + b.tellRecipientToBrowseToURL(url, toNotificationTarget(recipient)) if b.State() == ipn.Running { b.enterState(ipn.Starting) } @@ -2978,8 +3050,13 @@ func (b *LocalBackend) validPopBrowserURL(urlStr string) bool { } func (b *LocalBackend) tellClientToBrowseToURL(url string) { + b.tellRecipientToBrowseToURL(url, allClients) +} + +// tellRecipientToBrowseToURL is like tellClientToBrowseToURL but allows specifying a recipient. +func (b *LocalBackend) tellRecipientToBrowseToURL(url string, recipient notificationTarget) { if b.validPopBrowserURL(url) { - b.send(ipn.Notify{BrowseToURL: &url}) + b.sendTo(ipn.Notify{BrowseToURL: &url}, recipient) } } @@ -3251,6 +3328,15 @@ func (b *LocalBackend) tryLookupUserName(uid string) string { // StartLoginInteractive attempts to pick up the in-progress flow where it left // off. func (b *LocalBackend) StartLoginInteractive(ctx context.Context) error { + return b.StartLoginInteractiveAs(ctx, nil) +} + +// StartLoginInteractiveAs is like StartLoginInteractive but takes an [ipnauth.Actor] +// as an additional parameter. If non-nil, the specified user is expected to complete +// the interactive login, and therefore will receive the BrowseToURL notification once +// the control plane sends us one. Otherwise, the notification will be delivered to all +// active [watchSession]s. +func (b *LocalBackend) StartLoginInteractiveAs(ctx context.Context, user ipnauth.Actor) error { b.mu.Lock() if b.cc == nil { panic("LocalBackend.assertClient: b.cc == nil") @@ -3264,17 +3350,17 @@ func (b *LocalBackend) StartLoginInteractive(ctx context.Context) error { hasValidURL := url != "" && timeSinceAuthURLCreated < ((7*24*time.Hour)-(1*time.Hour)) if !hasValidURL { // A user wants to log in interactively, but we don't have a valid authURL. - // Set a flag to indicate that interactive login is in progress, forcing - // a BrowseToURL notification once the authURL becomes available. - b.interact = true + // Remember the user who initiated the login, so that we can notify them + // once the authURL is available. + b.authActor = user } cc := b.cc b.mu.Unlock() - b.logf("StartLoginInteractive: url=%v", hasValidURL) + b.logf("StartLoginInteractiveAs(%q): url=%v", maybeUsernameOf(user), hasValidURL) if hasValidURL { - b.popBrowserAuthNow(url, keyExpired) + b.popBrowserAuthNow(url, keyExpired, user) } else { cc.Login(b.loginFlags | controlclient.LoginInteractive) } @@ -5124,7 +5210,7 @@ func (b *LocalBackend) resetControlClientLocked() controlclient.Client { func (b *LocalBackend) resetAuthURLLocked() { b.authURL = "" b.authURLTime = time.Time{} - b.interact = false + b.authActor = nil } // ResetForClientDisconnect resets the backend for GUI clients running @@ -7369,3 +7455,13 @@ func (b *LocalBackend) srcIPHasCapForFilter(srcIP netip.Addr, cap tailcfg.NodeCa } return n.HasCap(cap) } + +// maybeUsernameOf returns the actor's username if the actor +// is non-nil and its username can be resolved. +func maybeUsernameOf(actor ipnauth.Actor) string { + var username string + if actor != nil { + username, _ = actor.Username() + } + return username +} diff --git a/ipn/ipnlocal/local_test.go b/ipn/ipnlocal/local_test.go index b0e12d500..9a8fa5e02 100644 --- a/ipn/ipnlocal/local_test.go +++ b/ipn/ipnlocal/local_test.go @@ -15,6 +15,7 @@ import ( "os" "reflect" "slices" + "strings" "sync" "testing" "time" @@ -31,6 +32,7 @@ import ( "tailscale.com/health" "tailscale.com/hostinfo" "tailscale.com/ipn" + "tailscale.com/ipn/ipnauth" "tailscale.com/ipn/store/mem" "tailscale.com/net/netcheck" "tailscale.com/net/netmon" @@ -3998,3 +4000,541 @@ func TestFillAllowedSuggestions(t *testing.T) { }) } } + +func TestNotificationTargetMatch(t *testing.T) { + tests := []struct { + name string + target notificationTarget + actor ipnauth.Actor + wantMatch bool + }{ + { + name: "AllClients/Nil", + target: allClients, + actor: nil, + wantMatch: true, + }, + { + name: "AllClients/NoUID/NoCID", + target: allClients, + actor: &ipnauth.TestActor{}, + wantMatch: true, + }, + { + name: "AllClients/WithUID/NoCID", + target: allClients, + actor: &ipnauth.TestActor{UID: "S-1-5-21-1-2-3-4", CID: ipnauth.NoClientID}, + wantMatch: true, + }, + { + name: "AllClients/NoUID/WithCID", + target: allClients, + actor: &ipnauth.TestActor{CID: ipnauth.ClientIDFrom("A")}, + wantMatch: true, + }, + { + name: "AllClients/WithUID/WithCID", + target: allClients, + actor: &ipnauth.TestActor{UID: "S-1-5-21-1-2-3-4", CID: ipnauth.ClientIDFrom("A")}, + wantMatch: true, + }, + { + name: "FilterByUID/Nil", + target: notificationTarget{userID: "S-1-5-21-1-2-3-4"}, + actor: nil, + wantMatch: false, + }, + { + name: "FilterByUID/NoUID/NoCID", + target: notificationTarget{userID: "S-1-5-21-1-2-3-4"}, + actor: &ipnauth.TestActor{}, + wantMatch: false, + }, + { + name: "FilterByUID/NoUID/WithCID", + target: notificationTarget{userID: "S-1-5-21-1-2-3-4"}, + actor: &ipnauth.TestActor{CID: ipnauth.ClientIDFrom("A")}, + wantMatch: false, + }, + { + name: "FilterByUID/SameUID/NoCID", + target: notificationTarget{userID: "S-1-5-21-1-2-3-4"}, + actor: &ipnauth.TestActor{UID: "S-1-5-21-1-2-3-4"}, + wantMatch: true, + }, + { + name: "FilterByUID/DifferentUID/NoCID", + target: notificationTarget{userID: "S-1-5-21-1-2-3-4"}, + actor: &ipnauth.TestActor{UID: "S-1-5-21-5-6-7-8"}, + wantMatch: false, + }, + { + name: "FilterByUID/SameUID/WithCID", + target: notificationTarget{userID: "S-1-5-21-1-2-3-4"}, + actor: &ipnauth.TestActor{UID: "S-1-5-21-1-2-3-4", CID: ipnauth.ClientIDFrom("A")}, + wantMatch: true, + }, + { + name: "FilterByUID/DifferentUID/WithCID", + target: notificationTarget{userID: "S-1-5-21-1-2-3-4"}, + actor: &ipnauth.TestActor{UID: "S-1-5-21-5-6-7-8", CID: ipnauth.ClientIDFrom("A")}, + wantMatch: false, + }, + { + name: "FilterByCID/Nil", + target: notificationTarget{clientID: ipnauth.ClientIDFrom("A")}, + actor: nil, + wantMatch: false, + }, + { + name: "FilterByCID/NoUID/NoCID", + target: notificationTarget{clientID: ipnauth.ClientIDFrom("A")}, + actor: &ipnauth.TestActor{}, + wantMatch: false, + }, + { + name: "FilterByCID/NoUID/SameCID", + target: notificationTarget{clientID: ipnauth.ClientIDFrom("A")}, + actor: &ipnauth.TestActor{CID: ipnauth.ClientIDFrom("A")}, + wantMatch: true, + }, + { + name: "FilterByCID/NoUID/DifferentCID", + target: notificationTarget{clientID: ipnauth.ClientIDFrom("A")}, + actor: &ipnauth.TestActor{CID: ipnauth.ClientIDFrom("B")}, + wantMatch: false, + }, + { + name: "FilterByCID/WithUID/NoCID", + target: notificationTarget{clientID: ipnauth.ClientIDFrom("A")}, + actor: &ipnauth.TestActor{UID: "S-1-5-21-1-2-3-4"}, + wantMatch: false, + }, + { + name: "FilterByCID/WithUID/SameCID", + target: notificationTarget{clientID: ipnauth.ClientIDFrom("A")}, + actor: &ipnauth.TestActor{UID: "S-1-5-21-1-2-3-4", CID: ipnauth.ClientIDFrom("A")}, + wantMatch: true, + }, + { + name: "FilterByCID/WithUID/DifferentCID", + target: notificationTarget{clientID: ipnauth.ClientIDFrom("A")}, + actor: &ipnauth.TestActor{UID: "S-1-5-21-1-2-3-4", CID: ipnauth.ClientIDFrom("B")}, + wantMatch: false, + }, + { + name: "FilterByUID+CID/Nil", + target: notificationTarget{userID: "S-1-5-21-1-2-3-4"}, + actor: nil, + wantMatch: false, + }, + { + name: "FilterByUID+CID/NoUID/NoCID", + target: notificationTarget{userID: "S-1-5-21-1-2-3-4", clientID: ipnauth.ClientIDFrom("A")}, + actor: &ipnauth.TestActor{}, + wantMatch: false, + }, + { + name: "FilterByUID+CID/NoUID/SameCID", + target: notificationTarget{userID: "S-1-5-21-1-2-3-4", clientID: ipnauth.ClientIDFrom("A")}, + actor: &ipnauth.TestActor{CID: ipnauth.ClientIDFrom("A")}, + wantMatch: false, + }, + { + name: "FilterByUID+CID/NoUID/DifferentCID", + target: notificationTarget{userID: "S-1-5-21-1-2-3-4", clientID: ipnauth.ClientIDFrom("A")}, + actor: &ipnauth.TestActor{CID: ipnauth.ClientIDFrom("B")}, + wantMatch: false, + }, + { + name: "FilterByUID+CID/SameUID/NoCID", + target: notificationTarget{userID: "S-1-5-21-1-2-3-4", clientID: ipnauth.ClientIDFrom("A")}, + actor: &ipnauth.TestActor{UID: "S-1-5-21-1-2-3-4"}, + wantMatch: false, + }, + { + name: "FilterByUID+CID/SameUID/SameCID", + target: notificationTarget{userID: "S-1-5-21-1-2-3-4", clientID: ipnauth.ClientIDFrom("A")}, + actor: &ipnauth.TestActor{UID: "S-1-5-21-1-2-3-4", CID: ipnauth.ClientIDFrom("A")}, + wantMatch: true, + }, + { + name: "FilterByUID+CID/SameUID/DifferentCID", + target: notificationTarget{userID: "S-1-5-21-1-2-3-4", clientID: ipnauth.ClientIDFrom("A")}, + actor: &ipnauth.TestActor{UID: "S-1-5-21-1-2-3-4", CID: ipnauth.ClientIDFrom("B")}, + wantMatch: false, + }, + { + name: "FilterByUID+CID/DifferentUID/NoCID", + target: notificationTarget{userID: "S-1-5-21-1-2-3-4", clientID: ipnauth.ClientIDFrom("A")}, + actor: &ipnauth.TestActor{UID: "S-1-5-21-5-6-7-8"}, + wantMatch: false, + }, + { + name: "FilterByUID+CID/DifferentUID/SameCID", + target: notificationTarget{userID: "S-1-5-21-1-2-3-4", clientID: ipnauth.ClientIDFrom("A")}, + actor: &ipnauth.TestActor{UID: "S-1-5-21-5-6-7-8", CID: ipnauth.ClientIDFrom("A")}, + wantMatch: false, + }, + { + name: "FilterByUID+CID/DifferentUID/DifferentCID", + target: notificationTarget{userID: "S-1-5-21-1-2-3-4", clientID: ipnauth.ClientIDFrom("A")}, + actor: &ipnauth.TestActor{UID: "S-1-5-21-5-6-7-8", CID: ipnauth.ClientIDFrom("B")}, + wantMatch: false, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + gotMatch := tt.target.match(tt.actor) + if gotMatch != tt.wantMatch { + t.Errorf("match: got %v; want %v", gotMatch, tt.wantMatch) + } + }) + } +} + +type newTestControlFn func(tb testing.TB, opts controlclient.Options) controlclient.Client + +func newLocalBackendWithTestControl(t *testing.T, enableLogging bool, newControl newTestControlFn) *LocalBackend { + logf := logger.Discard + if enableLogging { + logf = tstest.WhileTestRunningLogger(t) + } + sys := new(tsd.System) + store := new(mem.Store) + sys.Set(store) + e, err := wgengine.NewFakeUserspaceEngine(logf, sys.Set, sys.HealthTracker(), sys.UserMetricsRegistry()) + if err != nil { + t.Fatalf("NewFakeUserspaceEngine: %v", err) + } + t.Cleanup(e.Close) + sys.Set(e) + + b, err := NewLocalBackend(logf, logid.PublicID{}, sys, 0) + if err != nil { + t.Fatalf("NewLocalBackend: %v", err) + } + b.DisablePortMapperForTest() + + b.SetControlClientGetterForTesting(func(opts controlclient.Options) (controlclient.Client, error) { + return newControl(t, opts), nil + }) + return b +} + +// notificationHandler is any function that can process (e.g., check) a notification. +// It returns whether the notification has been handled or should be passed to the next handler. +// The handler may be called from any goroutine, so it must avoid calling functions +// that are restricted to the goroutine running the test or benchmark function, +// such as [testing.common.FailNow] and [testing.common.Fatalf]. +type notificationHandler func(testing.TB, ipnauth.Actor, *ipn.Notify) bool + +// wantedNotification names a [notificationHandler] that processes a notification +// the test expects and wants to receive. The name is used to report notifications +// that haven't been received within the expected timeout. +type wantedNotification struct { + name string + cond notificationHandler +} + +// notificationWatcher observes [LocalBackend] notifications as the specified actor, +// reporting missing but expected notifications using [testing.common.Error], +// and delegating the handling of unexpected notifications to the [notificationHandler]s. +type notificationWatcher struct { + tb testing.TB + lb *LocalBackend + actor ipnauth.Actor + + mu sync.Mutex + mask ipn.NotifyWatchOpt + want []wantedNotification // notifications we want to receive + unexpected []notificationHandler // funcs that are called to check any other notifications + ctxCancel context.CancelFunc // cancels the outstanding [LocalBackend.WatchNotificationsAs] call + got []*ipn.Notify // all notifications, both wanted and unexpected, we've received so far + gotWanted []*ipn.Notify // only the expected notifications; holds nil for any notification that hasn't been received + gotWantedCh chan struct{} // closed when we have received the last wanted notification + doneCh chan struct{} // closed when [LocalBackend.WatchNotificationsAs] returns +} + +func newNotificationWatcher(tb testing.TB, lb *LocalBackend, actor ipnauth.Actor) *notificationWatcher { + return ¬ificationWatcher{tb: tb, lb: lb, actor: actor} +} + +func (w *notificationWatcher) watch(mask ipn.NotifyWatchOpt, wanted []wantedNotification, unexpected ...notificationHandler) { + w.tb.Helper() + + // Cancel any outstanding [LocalBackend.WatchNotificationsAs] calls. + w.mu.Lock() + ctxCancel := w.ctxCancel + doneCh := w.doneCh + w.mu.Unlock() + if doneCh != nil { + ctxCancel() + <-doneCh + } + + doneCh = make(chan struct{}) + gotWantedCh := make(chan struct{}) + ctx, ctxCancel := context.WithCancel(context.Background()) + w.tb.Cleanup(func() { + ctxCancel() + <-doneCh + }) + + w.mu.Lock() + w.mask = mask + w.want = wanted + w.unexpected = unexpected + w.ctxCancel = ctxCancel + w.got = nil + w.gotWanted = make([]*ipn.Notify, len(wanted)) + w.gotWantedCh = gotWantedCh + w.doneCh = doneCh + w.mu.Unlock() + + watchAddedCh := make(chan struct{}) + go func() { + defer close(doneCh) + if len(wanted) == 0 { + close(gotWantedCh) + if len(unexpected) == 0 { + close(watchAddedCh) + return + } + } + + var nextWantIdx int + w.lb.WatchNotificationsAs(ctx, w.actor, w.mask, func() { close(watchAddedCh) }, func(notify *ipn.Notify) (keepGoing bool) { + w.tb.Helper() + + w.mu.Lock() + defer w.mu.Unlock() + w.got = append(w.got, notify) + + wanted := false + for i := nextWantIdx; i < len(w.want); i++ { + if wanted = w.want[i].cond(w.tb, w.actor, notify); wanted { + w.gotWanted[i] = notify + nextWantIdx = i + 1 + break + } + } + + if wanted && nextWantIdx == len(w.want) { + close(w.gotWantedCh) + if len(w.unexpected) == 0 { + // If we have received the last wanted notification, + // and we don't have any handlers for the unexpected notifications, + // we can stop the watcher right away. + return false + } + + } + + if !wanted { + // If we've received a notification we didn't expect, + // it could either be an unwanted notification caused by a bug + // or just a miscellaneous one that's irrelevant for the current test. + // Call unexpected notification handlers, if any, to + // check and fail the test if necessary. + for _, h := range w.unexpected { + if h(w.tb, w.actor, notify) { + break + } + } + } + + return true + }) + + }() + <-watchAddedCh +} + +func (w *notificationWatcher) check() []*ipn.Notify { + w.tb.Helper() + + w.mu.Lock() + cancel := w.ctxCancel + gotWantedCh := w.gotWantedCh + checkUnexpected := len(w.unexpected) != 0 + doneCh := w.doneCh + w.mu.Unlock() + + // Wait for up to 10 seconds to receive expected notifications. + timeout := 10 * time.Second + for { + select { + case <-gotWantedCh: + if checkUnexpected { + gotWantedCh = nil + // But do not wait longer than 500ms for unexpected notifications after + // the expected notifications have been received. + timeout = 500 * time.Millisecond + continue + } + case <-doneCh: + // [LocalBackend.WatchNotificationsAs] has already returned, so no further + // notifications will be received. There's no reason to wait any longer. + case <-time.After(timeout): + } + cancel() + <-doneCh + break + } + + // Report missing notifications, if any, and log all received notifications, + // including both expected and unexpected ones. + w.mu.Lock() + defer w.mu.Unlock() + if hasMissing := slices.Contains(w.gotWanted, nil); hasMissing { + want := make([]string, len(w.want)) + got := make([]string, 0, len(w.want)) + for i, wn := range w.want { + want[i] = wn.name + if w.gotWanted[i] != nil { + got = append(got, wn.name) + } + } + w.tb.Errorf("Notifications(%s): got %q; want %q", actorDescriptionForTest(w.actor), strings.Join(got, ", "), strings.Join(want, ", ")) + for i, n := range w.got { + w.tb.Logf("%d. %v", i, n) + } + return nil + } + + return w.gotWanted +} + +func actorDescriptionForTest(actor ipnauth.Actor) string { + var parts []string + if actor != nil { + if name, _ := actor.Username(); name != "" { + parts = append(parts, name) + } + if uid := actor.UserID(); uid != "" { + parts = append(parts, string(uid)) + } + if clientID, _ := actor.ClientID(); clientID != ipnauth.NoClientID { + parts = append(parts, clientID.String()) + } + } + return fmt.Sprintf("Actor{%s}", strings.Join(parts, ", ")) +} + +func TestLoginNotifications(t *testing.T) { + const ( + enableLogging = true + controlURL = "https://localhost:1/" + loginURL = "https://localhost:1/1" + ) + + wantBrowseToURL := wantedNotification{ + name: "BrowseToURL", + cond: func(t testing.TB, actor ipnauth.Actor, n *ipn.Notify) bool { + if n.BrowseToURL != nil && *n.BrowseToURL != loginURL { + t.Errorf("BrowseToURL (%s): got %q; want %q", actorDescriptionForTest(actor), *n.BrowseToURL, loginURL) + return false + } + return n.BrowseToURL != nil + }, + } + unexpectedBrowseToURL := func(t testing.TB, actor ipnauth.Actor, n *ipn.Notify) bool { + if n.BrowseToURL != nil { + t.Errorf("Unexpected BrowseToURL(%s): %v", actorDescriptionForTest(actor), n) + return true + } + return false + } + + tests := []struct { + name string + logInAs ipnauth.Actor + urlExpectedBy []ipnauth.Actor + urlUnexpectedBy []ipnauth.Actor + }{ + { + name: "NoObservers", + logInAs: &ipnauth.TestActor{UID: "A"}, + urlExpectedBy: []ipnauth.Actor{}, // ensure that it does not panic if no one is watching + }, + { + name: "SingleUser", + logInAs: &ipnauth.TestActor{UID: "A"}, + urlExpectedBy: []ipnauth.Actor{&ipnauth.TestActor{UID: "A"}}, + }, + { + name: "SameUser/TwoSessions/NoCID", + logInAs: &ipnauth.TestActor{UID: "A"}, + urlExpectedBy: []ipnauth.Actor{&ipnauth.TestActor{UID: "A"}, &ipnauth.TestActor{UID: "A"}}, + }, + { + name: "SameUser/TwoSessions/OneWithCID", + logInAs: &ipnauth.TestActor{UID: "A", CID: ipnauth.ClientIDFrom("123")}, + urlExpectedBy: []ipnauth.Actor{&ipnauth.TestActor{UID: "A", CID: ipnauth.ClientIDFrom("123")}}, + urlUnexpectedBy: []ipnauth.Actor{&ipnauth.TestActor{UID: "A"}}, + }, + { + name: "SameUser/TwoSessions/BothWithCID", + logInAs: &ipnauth.TestActor{UID: "A", CID: ipnauth.ClientIDFrom("123")}, + urlExpectedBy: []ipnauth.Actor{&ipnauth.TestActor{UID: "A", CID: ipnauth.ClientIDFrom("123")}}, + urlUnexpectedBy: []ipnauth.Actor{&ipnauth.TestActor{UID: "A", CID: ipnauth.ClientIDFrom("456")}}, + }, + { + name: "DifferentUsers/NoCID", + logInAs: &ipnauth.TestActor{UID: "A"}, + urlExpectedBy: []ipnauth.Actor{&ipnauth.TestActor{UID: "A"}}, + urlUnexpectedBy: []ipnauth.Actor{&ipnauth.TestActor{UID: "B"}}, + }, + { + name: "DifferentUsers/SameCID", + logInAs: &ipnauth.TestActor{UID: "A"}, + urlExpectedBy: []ipnauth.Actor{&ipnauth.TestActor{UID: "A", CID: ipnauth.ClientIDFrom("123")}}, + urlUnexpectedBy: []ipnauth.Actor{&ipnauth.TestActor{UID: "B", CID: ipnauth.ClientIDFrom("123")}}, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + + lb := newLocalBackendWithTestControl(t, enableLogging, func(tb testing.TB, opts controlclient.Options) controlclient.Client { + return newClient(tb, opts) + }) + if _, err := lb.EditPrefs(&ipn.MaskedPrefs{ControlURLSet: true, Prefs: ipn.Prefs{ControlURL: controlURL}}); err != nil { + t.Fatalf("(*EditPrefs).Start(): %v", err) + } + if err := lb.Start(ipn.Options{}); err != nil { + t.Fatalf("(*LocalBackend).Start(): %v", err) + } + + sessions := make([]*notificationWatcher, 0, len(tt.urlExpectedBy)+len(tt.urlUnexpectedBy)) + for _, actor := range tt.urlExpectedBy { + session := newNotificationWatcher(t, lb, actor) + session.watch(0, []wantedNotification{wantBrowseToURL}) + sessions = append(sessions, session) + } + for _, actor := range tt.urlUnexpectedBy { + session := newNotificationWatcher(t, lb, actor) + session.watch(0, nil, unexpectedBrowseToURL) + sessions = append(sessions, session) + } + + if err := lb.StartLoginInteractiveAs(context.Background(), tt.logInAs); err != nil { + t.Fatal(err) + } + + lb.cc.(*mockControl).send(nil, loginURL, false, nil) + + var wg sync.WaitGroup + wg.Add(len(sessions)) + for _, sess := range sessions { + go func() { // check all sessions in parallel + sess.check() + wg.Done() + }() + } + wg.Wait() + }) + } +} diff --git a/ipn/ipnserver/actor.go b/ipn/ipnserver/actor.go index 761c9816c..63d4b183c 100644 --- a/ipn/ipnserver/actor.go +++ b/ipn/ipnserver/actor.go @@ -31,6 +31,7 @@ type actor struct { logf logger.Logf ci *ipnauth.ConnIdentity + clientID ipnauth.ClientID isLocalSystem bool // whether the actor is the Windows' Local System identity. } @@ -39,7 +40,22 @@ func newActor(logf logger.Logf, c net.Conn) (*actor, error) { if err != nil { return nil, err } - return &actor{logf: logf, ci: ci, isLocalSystem: connIsLocalSystem(ci)}, nil + var clientID ipnauth.ClientID + if pid := ci.Pid(); pid != 0 { + // Derive [ipnauth.ClientID] from the PID of the connected client process. + // TODO(nickkhyl): This is transient and will be re-worked as we + // progress on tailscale/corp#18342. At minimum, we should use a 2-tuple + // (PID + StartTime) or a 3-tuple (PID + StartTime + UID) to identify + // the client process. This helps prevent security issues where a + // terminated client process's PID could be reused by a different + // process. This is not currently an issue as we allow only one user to + // connect anyway. + // Additionally, we should consider caching authentication results since + // operations like retrieving a username by SID might require network + // connectivity on domain-joined devices and/or be slow. + clientID = ipnauth.ClientIDFrom(pid) + } + return &actor{logf: logf, ci: ci, clientID: clientID, isLocalSystem: connIsLocalSystem(ci)}, nil } // IsLocalSystem implements [ipnauth.Actor]. @@ -61,6 +77,11 @@ func (a *actor) pid() int { return a.ci.Pid() } +// ClientID implements [ipnauth.Actor]. +func (a *actor) ClientID() (_ ipnauth.ClientID, ok bool) { + return a.clientID, a.clientID != ipnauth.NoClientID +} + // Username implements [ipnauth.Actor]. func (a *actor) Username() (string, error) { if a.ci == nil { diff --git a/ipn/localapi/localapi.go b/ipn/localapi/localapi.go index 528304bab..25ec19121 100644 --- a/ipn/localapi/localapi.go +++ b/ipn/localapi/localapi.go @@ -1231,7 +1231,7 @@ func (h *Handler) serveWatchIPNBus(w http.ResponseWriter, r *http.Request) { w.Header().Set("Content-Type", "application/json") ctx := r.Context() enc := json.NewEncoder(w) - h.b.WatchNotifications(ctx, mask, f.Flush, func(roNotify *ipn.Notify) (keepGoing bool) { + h.b.WatchNotificationsAs(ctx, h.Actor, mask, f.Flush, func(roNotify *ipn.Notify) (keepGoing bool) { err := enc.Encode(roNotify) if err != nil { h.logf("json.Encode: %v", err) @@ -1251,7 +1251,7 @@ func (h *Handler) serveLoginInteractive(w http.ResponseWriter, r *http.Request) http.Error(w, "want POST", http.StatusBadRequest) return } - h.b.StartLoginInteractive(r.Context()) + h.b.StartLoginInteractiveAs(r.Context(), h.Actor) w.WriteHeader(http.StatusNoContent) return } diff --git a/ipn/localapi/localapi_test.go b/ipn/localapi/localapi_test.go index fa54a1e75..d89c46261 100644 --- a/ipn/localapi/localapi_test.go +++ b/ipn/localapi/localapi_test.go @@ -39,23 +39,6 @@ import ( "tailscale.com/wgengine" ) -var _ ipnauth.Actor = (*testActor)(nil) - -type testActor struct { - uid ipn.WindowsUserID - name string - isLocalSystem bool - isLocalAdmin bool -} - -func (u *testActor) UserID() ipn.WindowsUserID { return u.uid } - -func (u *testActor) Username() (string, error) { return u.name, nil } - -func (u *testActor) IsLocalSystem() bool { return u.isLocalSystem } - -func (u *testActor) IsLocalAdmin(operatorUID string) bool { return u.isLocalAdmin } - func TestValidHost(t *testing.T) { tests := []struct { host string @@ -207,7 +190,7 @@ func TestWhoIsArgTypes(t *testing.T) { func TestShouldDenyServeConfigForGOOSAndUserContext(t *testing.T) { newHandler := func(connIsLocalAdmin bool) *Handler { - return &Handler{Actor: &testActor{isLocalAdmin: connIsLocalAdmin}, b: newTestLocalBackend(t)} + return &Handler{Actor: &ipnauth.TestActor{LocalAdmin: connIsLocalAdmin}, b: newTestLocalBackend(t)} } tests := []struct { name string