diff --git a/client/cmd/vnc_agent.go b/client/cmd/vnc_agent.go index e8993baa6..e90c434e5 100644 --- a/client/cmd/vnc_agent.go +++ b/client/cmd/vnc_agent.go @@ -74,6 +74,8 @@ var vncAgentCmd = &cobra.Command{ log.Debugf("chmod %s: %v", vncAgentSocket, err) } + ctx := cmd.Context() + capturer, injector, err := newAgentResources() if err != nil { _ = ln.Close() @@ -87,12 +89,12 @@ var vncAgentCmd = &cobra.Command{ Listener: ln, }) - if err := srv.Start(cmd.Context(), netip.AddrPort{}, netip.Prefix{}); err != nil { + if err := srv.Start(ctx, netip.AddrPort{}, netip.Prefix{}); err != nil { return fmt.Errorf("start vnc server: %w", err) } log.Infof("vnc-agent listening on %s, ready", vncAgentSocket) - <-cmd.Context().Done() + <-ctx.Done() log.Info("vnc-agent context cancelled, shutting down") return srv.Stop() }, diff --git a/client/cmd/vnc_agent_darwin.go b/client/cmd/vnc_agent_darwin.go index e7fee4c92..3d9310c48 100644 --- a/client/cmd/vnc_agent_darwin.go +++ b/client/cmd/vnc_agent_darwin.go @@ -9,12 +9,12 @@ import ( ) func newAgentResources() (vncserver.ScreenCapturer, vncserver.InputInjector, error) { - // Ask for Screen Recording here and nowhere else: this process runs as the - // console user, which is what TCC requires for a user-scope service, and it - // is the point where somebody is demonstrably trying to view the screen. - // Granting it also requires the capturing process to restart, which comes - // for free since the agent is respawned per session. - vncserver.PrimeScreenCapturePermission() + // Ask for Screen Recording here and nowhere else. This process runs as the + // console user, which TCC requires for a user-scope service, and it is fresh + // per connection, which is what makes the dialog appear at all: TCC shows it + // once per process. The request blocks until the user answers, so it also + // keeps the Accessibility ask that follows the first input out of its way. + vncserver.RequestScreenRecording() capturer := vncserver.NewMacPoller() injector, err := vncserver.NewMacInputInjector() diff --git a/client/vnc/server/agent_darwin.go b/client/vnc/server/agent_darwin.go index f7efde0fe..13e06e66d 100644 --- a/client/vnc/server/agent_darwin.go +++ b/client/vnc/server/agent_darwin.go @@ -21,22 +21,28 @@ import ( "github.com/netbirdio/netbird/client/configs" ) -// darwinAgentManager spawns a per-user VNC agent on demand and keeps it -// alive across multiple client connections within the same console-user -// session. A new agent is spawned the first time a client connects, or -// whenever the console user changes underneath us. +// darwinAgentManager spawns a per-user VNC agent on demand and keeps it alive +// only while connections are using it. Concurrent connections share one agent; +// the last one to finish takes it down again. // -// Lifecycle is lazy by design: a daemon that never receives a VNC -// connection never spawns anything. The trade-off versus an eager spawn -// (the Windows model) is that the first VNC client pays the launchctl +// The agent is deliberately not reused across connections. A TCC prompt appears +// at most once in the lifetime of a process, so an agent whose permission the +// user takes away afterwards can never ask for it again: it just keeps serving a +// desktop picture with no windows in it, and only a service restart gets the +// question back on screen. A fresh process per connection can always ask. +// +// Lifecycle is lazy by design: a daemon that never receives a VNC connection +// never spawns anything. The cost is that every connection pays the launchctl // asuser + listen-readiness wait, ~hundreds of milliseconds in practice. -// That cost only repeats on user switch. type darwinAgentManager struct { mu sync.Mutex authToken string socketPath string uid uint32 running bool + // users counts the connections that resolved this agent and have not + // released it yet. + users int } func newDarwinAgentManager(ctx context.Context) *darwinAgentManager { @@ -101,8 +107,12 @@ func (m *darwinAgentManager) Resolve(ctx context.Context) (string, string, uint3 m.mu.Lock() defer m.mu.Unlock() if m.running && m.uid == consoleUID && vncAgentRunning() { + m.users++ return m.socketPath, m.authToken, m.uid, nil } + // Connections already holding this agent still owe a Release, so their claims + // carry over to the replacement rather than being dropped. + claims := m.users m.killLocked() // Reap stray agents so the new token is the only accepted one. killAllVNCAgents() @@ -131,10 +141,27 @@ func (m *darwinAgentManager) Resolve(ctx context.Context) (string, string, uint3 m.socketPath = socketPath m.uid = consoleUID m.running = true + m.users = claims + 1 log.Infof("spawned VNC agent for console uid=%d on %s", consoleUID, socketPath) return socketPath, token, consoleUID, nil } +// Release drops one connection's claim on the agent and stops it once none are +// left, so the next connection starts a process that sees the permissions as +// they are then rather than as they were when the first one connected. +func (m *darwinAgentManager) Release() { + m.mu.Lock() + defer m.mu.Unlock() + if m.users > 0 { + m.users-- + } + if m.users > 0 || !m.running { + return + } + m.killLocked() + log.Info("last VNC connection closed; agent stopped, a fresh one starts on the next connection") +} + // prepareAgentSocketDir creates a per-uid subdirectory under the netbird // runtime directory where the agent will bind its Unix socket. The leaf is // owned by uid with mode 0700, so only the target user and root can write diff --git a/client/vnc/server/agent_ipc.go b/client/vnc/server/agent_ipc.go index 283241e37..668d043f8 100644 --- a/client/vnc/server/agent_ipc.go +++ b/client/vnc/server/agent_ipc.go @@ -90,6 +90,7 @@ func (s *Server) handleServiceConnection(conn net.Conn, sa sessionAgent) { authedLog.Warnf("VNC connection rejected: agent unavailable: %v", err) return } + defer sa.Release() var initiator string if s.authorizer != nil { diff --git a/client/vnc/server/agent_windows.go b/client/vnc/server/agent_windows.go index 865ebd4d7..911a021a7 100644 --- a/client/vnc/server/agent_windows.go +++ b/client/vnc/server/agent_windows.go @@ -536,6 +536,12 @@ const agentResolveWait = 30 * time.Second var errAgentNotReady = errors.New("VNC agent not running yet") +// Release is a no-op here. The agent's lifetime follows the console session +// rather than individual connections: Windows has no per-process permission +// state to re-read, and CreateProcessAsUser into the console session is far too +// expensive to repeat per connection. +func (m *sessionManager) Release() {} + // Stop signals the session manager to exit its polling loop and closes the // Job Object handle, which Windows uses as the trigger to terminate every // agent process this manager spawned. diff --git a/client/vnc/server/capture_darwin.go b/client/vnc/server/capture_darwin.go index 8de941e56..9b5baecc0 100644 --- a/client/vnc/server/capture_darwin.go +++ b/client/vnc/server/capture_darwin.go @@ -37,9 +37,13 @@ var ( cfDataGetBytePtr func(uintptr) uintptr cfRelease func(uintptr) cgRequestScreenCaptureAccess func() bool - cgEventCreate func(uintptr) uintptr - cgEventGetLocation func(uintptr) cgPoint - darwinCaptureReady bool + // cgPreflightScreenCaptureAccess reads the decision without prompting. Kept + // for the diagnostic in RequestScreenRecording until it is known whether it + // can be trusted in the agent. + cgPreflightScreenCaptureAccess func() bool + cgEventCreate func(uintptr) uintptr + cgEventGetLocation func(uintptr) cgPoint + darwinCaptureReady bool ) // cgPoint mirrors CoreGraphics CGPoint: two doubles, 16 bytes, returned @@ -77,13 +81,16 @@ func initDarwinCapture() { purego.RegisterLibFunc(&cfDataGetBytePtr, cf, "CFDataGetBytePtr") purego.RegisterLibFunc(&cfRelease, cf, "CFRelease") - // CGRequestScreenCaptureAccess (macOS 11+) prompts on first call and - // is a cheap no-op once granted. The Preflight companion is unreliable - // on Sequoia (returns false even when access is granted), so we drive - // the permission flow from actual capture failures instead. + // CGRequestScreenCaptureAccess (macOS 11+) raises its dialog on the first + // call in a process and reports the decision as it stands, without waiting + // for the user. Its Preflight companion never prompts but has a reputation + // for lying on Sequoia, see RequestScreenRecording. if sym, err := purego.Dlsym(cg, "CGRequestScreenCaptureAccess"); err == nil { purego.RegisterFunc(&cgRequestScreenCaptureAccess, sym) } + if sym, err := purego.Dlsym(cg, "CGPreflightScreenCaptureAccess"); err == nil { + purego.RegisterFunc(&cgPreflightScreenCaptureAccess, sym) + } // CGEventCreate / CGEventGetLocation feed the cursor position used // by remote-cursor compositing. Optional; absence reports as a // position-source error and disables that feature on this host. @@ -114,52 +121,52 @@ type CGCapturer struct { cursor *cgCursor } -// PrimeScreenCapturePermission triggers the macOS Screen Recording -// permission prompt without creating a full capturer. The platform wiring -// calls this at VNC-server enable time so the user sees the prompt the -// moment they turn the feature on. CGRequestScreenCaptureAccess is a -// no-op when the grant already exists, so calling it on every enable is -// cheap and safe. -func PrimeScreenCapturePermission() { +// RequestScreenRecording asks the console user for Screen Recording and reports +// whether it is granted. Called once at agent startup, which is the only place it +// can work: TCC prompts at most once in the lifetime of a process, and the daemon +// is a LaunchDaemon, where a request for a user-scope service is dropped without +// ever showing a dialog. +// +// Capture cannot stand in for this check. Without the permission +// CGDisplayCreateImage still succeeds and returns the desktop picture with every +// window missing, so a failing capture is not how a missing grant shows up, and a +// successful one is no proof of having it. +// +// The call returns the decision as it stands and raises its dialog in the +// background, so the answer it reports is the state before the user gets a say. A +// grant takes effect immediately, so the session the dialog interrupted goes on to +// show the real screen without needing a new process. +// +// It deliberately does not open System Settings when the answer is no. The dialog +// is still on screen at that point, and putting the pane up alongside it leaves +// two things competing for one decision. A dismissed dialog needs no fallback +// either: the agent is recycled per connection, so the next one asks again. +func RequestScreenRecording() bool { initDarwinCapture() - if !darwinCaptureReady { - return - } - if cgRequestScreenCaptureAccess != nil { - cgRequestScreenCaptureAccess() - } -} - -// notifyScreenRecordingMissing nudges the user once per agent process to -// approve Screen Recording. The capturer init retries on backoff when the -// grant is missing; without the sync.Once we would reopen System Settings -// every tick and flood the daemon log with the same warning. -var screenRecordingNotifyOnce sync.Once - -func notifyScreenRecordingMissing() { - screenRecordingNotifyOnce.Do(func() { - if cgRequestScreenCaptureAccess != nil { - cgRequestScreenCaptureAccess() - } + if !darwinCaptureReady || cgRequestScreenCaptureAccess == nil { + // Nothing to ask with, so Settings is the only route left. openPrivacyPane("Privacy_ScreenCapture") - log.Warn("Screen Recording permission not granted. " + - "Opened System Settings > Privacy & Security > Screen Recording; enable netbird and restart.") - }) + log.Warn("cannot ask for Screen Recording permission on this macOS. " + + "Opened System Settings > Privacy & Security > Screen Recording; enable netbird there.") + return false + } + // Read the silent check first: the request below can change what it reports. + // Logged to settle whether it can be trusted here, in the console-user agent, + // having proven unreliable in the daemon where the request is dropped outright. + preflight := "unavailable" + if cgPreflightScreenCaptureAccess != nil { + preflight = strconv.FormatBool(cgPreflightScreenCaptureAccess()) + } + + granted := cgRequestScreenCaptureAccess() + log.Infof("Screen Recording: granted=%v (preflight reported %s)", granted, preflight) + if !granted { + log.Warn("Screen Recording permission not granted, the screen shows no windows until it is") + } + return granted } // NewCGCapturer creates a screen capturer for the main display. -// screenCaptureWorking records that a real capture succeeded, which is the only -// trustworthy signal that Screen Recording is granted: CGPreflight lies on -// Sequoia. The input side waits for this before asking for Accessibility, so the -// two permission panes never compete (macOS shows one at a time, and losing the -// Screen Recording pane is the worse outcome: without it there is no picture). -var screenCaptureWorking atomic.Bool - -// ScreenCaptureWorking reports whether a capture has succeeded in this process. -func ScreenCaptureWorking() bool { - return screenCaptureWorking.Load() -} - func NewCGCapturer() (*CGCapturer, error) { initDarwinCapture() if !darwinCaptureReady { @@ -171,10 +178,8 @@ func NewCGCapturer() (*CGCapturer, error) { img, err := c.Capture() if err != nil { - notifyScreenRecordingMissing() return nil, fmt.Errorf("probe capture: %w", err) } - screenCaptureWorking.Store(true) nativeW := img.Rect.Dx() nativeH := img.Rect.Dy() c.hasHash = false @@ -475,8 +480,8 @@ func convertBGRAToRGBA(dst []byte, dstStride int, src []byte, srcStride, w, h in // window so concurrent sessions coalesce into one capture. // // The capturer is allocated lazily on first use and released when all -// clients disconnect. Init is retried with backoff because the user may -// grant Screen Recording permission while the server is already running. +// clients disconnect. Init is retried with backoff because a display can be +// momentarily unable to hand out a frame, on a mode change or a display switch. type MacPoller struct { mu sync.Mutex @@ -493,8 +498,8 @@ type MacPoller struct { } // macInitRetryBackoffFor returns the delay we wait between init attempts -// after consecutive failures. Screen Recording permission is a one-shot -// user grant, so after several failures we back off aggressively. +// after consecutive failures. Repeated failures mean the display is not coming +// back on its own, so we back off aggressively rather than spin. func macInitRetryBackoffFor(fails int) time.Duration { switch { case fails > 15: diff --git a/client/vnc/server/input_darwin.go b/client/vnc/server/input_darwin.go index 9f8fe50e9..ec2569e62 100644 --- a/client/vnc/server/input_darwin.go +++ b/client/vnc/server/input_darwin.go @@ -82,6 +82,13 @@ var ( // CGEventCreateScrollWheelEvent is variadic, call via SyscallN. cgEventCreateScrollWheelEventAddr uintptr + // CGPreflight/RequestPostEventAccess (macOS 10.15+) read and ask for + // kTCCServicePostEvent, which is the service that actually governs + // CGEventPost. The AX calls below read kTCCServiceAccessibility, a different + // decision that can disagree with whether injected events land. + cgPreflightPostEventAccess func() bool + cgRequestPostEventAccess func() bool + axIsProcessTrusted func() bool // axIsProcessTrustedWithOptions takes a CFDictionary; when the dict's // kAXTrustedCheckOptionPrompt key is true, macOS shows the native @@ -148,6 +155,13 @@ func initDarwinInput() { cgEventCreateScrollWheelEventAddr = sym } + if sym, err := purego.Dlsym(cg, "CGPreflightPostEventAccess"); err == nil { + purego.RegisterFunc(&cgPreflightPostEventAccess, sym) + } + if sym, err := purego.Dlsym(cg, "CGRequestPostEventAccess"); err == nil { + purego.RegisterFunc(&cgRequestPostEventAccess, sym) + } + if ax, err := purego.Dlopen("/System/Library/Frameworks/ApplicationServices.framework/ApplicationServices", purego.RTLD_NOW|purego.RTLD_GLOBAL); err == nil { if sym, err := purego.Dlsym(ax, "AXIsProcessTrusted"); err == nil { purego.RegisterFunc(&axIsProcessTrusted, sym) @@ -302,9 +316,14 @@ type MacInputInjector struct { // field on each posted event, not from event timing. clickCount [5]int64 clickAt [5]time.Time - // axAsked is set once the Accessibility request has been made, so the - // per-event check is one atomic load. - axAsked atomic.Bool + // axDone is set once there is nothing left to ask, so the per-event check on + // the hot path is a single atomic load. + axDone atomic.Bool + // axNextTry is when the cold path may run again, and axAsks counts the asks + // made so far. Input arrives continuously, so the cold path is paced by time + // rather than by event count. + axNextTry atomic.Int64 + axAsks atomic.Int32 } // NewMacInputInjector creates a macOS input injector. @@ -313,9 +332,13 @@ func NewMacInputInjector() (*MacInputInjector, error) { if !darwinInputReady { return nil, fmt.Errorf("CoreGraphics not available for input injection") } - logAccessibilityStatus() - m := &MacInputInjector{} + if postEventAllowed() { + m.axDone.Store(true) + } else { + log.Info("input permission not granted yet, asking when input arrives") + } + if path, err := exec.LookPath("pbcopy"); err == nil { m.pbcopyPath = path } @@ -332,46 +355,93 @@ func NewMacInputInjector() (*MacInputInjector, error) { return m, nil } -// logAccessibilityStatus reports Accessibility state without prompting. Asking -// here would put the Accessibility pane on screen the moment a session starts, -// on top of the Screen Recording request, and macOS shows only one pane at a -// time: the Accessibility one wins and the more important request is buried. -// The ask happens on the first input instead, see ensureAccessibility. -func logAccessibilityStatus() { - if axIsProcessTrusted != nil && !axIsProcessTrusted() { - log.Info("Accessibility permission not granted yet; asking on the first input event") - } -} - -// ensureAccessibility asks for Accessibility at most once per process, on the -// first input that is actually delivered. Injection happens per event, so the -// common path has to be a single atomic load. +// ensureAccessibility asks for Accessibility while input is being delivered and +// the permission is missing. Injection happens per event, so the common path has +// to be a single atomic load. func (m *MacInputInjector) ensureAccessibility() { - if m.axAsked.Load() { + if m.axDone.Load() { return } m.askAccessibility() } +const ( + // axAskRetry is how long to leave between asks. macOS shows one permission + // dialog at a time, and the Screen Recording one raised at agent start is + // usually still up when the first input arrives, so the first ask is often + // dropped without a trace. Asking again is the only way to land it, since + // nothing in this process can observe either the dialog or the answer. + axAskRetry = 8 * time.Second + // axMaxAsks bounds that, so a user who wants neither is left alone. A later + // connection asks again from a fresh process anyway. + axMaxAsks = 3 +) + +// postEventAllowed reports whether injected events are allowed to land, reading +// kTCCServicePostEvent without prompting. This is the decision CGEventPost is +// judged by. +// +// The fallback is for hosts predating the call, where Accessibility was the gate. +// It is deliberately not used anywhere else: AXIsProcessTrusted has the side +// effect of filing the caller in the Accessibility list with the box unchecked, +// and a decision on file, even that one, stops macOS from ever showing the dialog. +func postEventAllowed() bool { + if cgPreflightPostEventAccess == nil { + return axProcessTrusted() + } + return cgPreflightPostEventAccess() +} + +// axProcessTrusted reads the Accessibility state without prompting, and registers +// the caller in the Accessibility list as a side effect. A host whose symbols +// failed to load counts as trusted: nothing can be asked or checked there. +func axProcessTrusted() bool { + if axIsProcessTrusted == nil { + return true + } + return axIsProcessTrusted() +} + // askAccessibility is the cold path of ensureAccessibility. // -// It waits for a capture to have succeeded before prompting: Screen Recording is -// the permission a session cannot do without, and requesting Accessibility while -// that pane is open replaces it. A session that never captures never gets here, -// which is the right outcome, since input on a black screen is not useful. +// The asks are blind. TCC answers at most once in the lifetime of a process, and +// the reads do not follow along either: both the post-event and the screen-capture +// preflight keep reporting the state the process started with, so a grant made +// during a session is invisible here. That rules out waiting for the Screen +// Recording question to be settled, and leaves repeating the ask as the only way +// to catch the moment its dialog goes away. System Settings is not opened on the +// way: the dialog carries that button itself. func (m *MacInputInjector) askAccessibility() { - if !ScreenCaptureWorking() { + now := time.Now() + if now.UnixNano() < m.axNextTry.Load() { return } - if !m.axAsked.CompareAndSwap(false, true) { + m.axNextTry.Store(now.Add(axAskRetry).UnixNano()) + if m.axAsks.Add(1) >= axMaxAsks { + m.axDone.Store(true) + } + + // AXIsProcessTrustedWithOptions is what actually puts the dialog on screen. + // CGRequestPostEventAccess asks about the right service but returns silently + // for a process like this one, so it is only the fallback. Neither prompts + // when the permission is already there, so this cannot produce a stray dialog. + switch { + case axIsProcessTrustedWithOptions != nil: + if axProcessIsTrusted() { + return + } + case cgRequestPostEventAccess != nil: + if cgRequestPostEventAccess() { + return + } + default: + // Nothing here can prompt, so Settings is the only route. + openPrivacyPane("Privacy_Accessibility") + log.Warn("cannot ask for input permission on this macOS. Opened System Settings > " + + "Privacy & Security > Accessibility; enable netbird there.") return } - if axProcessIsTrusted() { - return - } - log.Warn("Accessibility permission not granted. Input injection will not work. " + - "Approve the prompt or grant in System Settings > Privacy & Security > Accessibility.") - openPrivacyPane("Privacy_Accessibility") + log.Warn("asked for input permission; granting it makes remote input work right away") } // axProcessIsTrusted asks macOS whether netbird has Accessibility access, diff --git a/client/vnc/server/server.go b/client/vnc/server/server.go index 83d8e0268..ee06fa694 100644 --- a/client/vnc/server/server.go +++ b/client/vnc/server/server.go @@ -897,7 +897,11 @@ func (s *Server) handleConnection(conn net.Conn) { } ok, agentViewOnly := s.verifyAgentToken(conn, connLog) if !ok { - connLog.Info("VNC connection rejected: agent token check failed") + // Reported there already, at a level that tells a liveness probe apart + // from a bad token. The daemon dials the agent socket to wait for it to + // come up, so this fires on every spawn and is not a rejection worth + // putting in front of anyone. + connLog.Debug("VNC connection rejected: agent token check failed") return } header, err := s.readConnectionHeader(conn) diff --git a/client/vnc/server/service_agent.go b/client/vnc/server/service_agent.go index cd6bef4fb..f4715779b 100644 --- a/client/vnc/server/service_agent.go +++ b/client/vnc/server/service_agent.go @@ -8,8 +8,12 @@ import "context" // under (used to validate peer credentials before the daemon hands the // token to whoever is on the other end of the socket). Resolve may spawn // the agent lazily. +// Release reports that one proxied connection is done with the agent, so a +// platform that recycles the agent per connection can tear it down once the last +// one is gone. Every successful Resolve owes exactly one Release. type sessionAgent interface { Resolve(ctx context.Context) (socketPath, token string, peerUID uint32, err error) + Release() } // stopServiceAgent tears down the shared manager, if one was ever built, and