Recycle the macOS VNC agent per connection so permission prompts work

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()
 	},

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()

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

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 {

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.

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:

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,

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)

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