Address CodeRabbit review on embedded VNC PR

client/internal/engine_vnc.go

@@ -155,8 +155,10 @@ func (e *Engine) startVNCServer() error {
 }
 
 // updateVNCServerAuth updates VNC fine-grained access control from management.
+// A nil vncAuth clears all authorized users and session pubkeys so management
+// can revoke access by omitting the field on the next sync.
 func (e *Engine) updateVNCServerAuth(vncAuth *mgmProto.VNCAuth) {
-	if vncAuth == nil || e.vncSrv == nil {
+	if e.vncSrv == nil {
 		return
 	}
 
@@ -165,6 +167,11 @@ func (e *Engine) updateVNCServerAuth(vncAuth *mgmProto.VNCAuth) {
 		return
 	}
 
+	if vncAuth == nil {
+		vncSrv.UpdateVNCAuth(&sshauth.Config{})
+		return
+	}
+
 	protoUsers := vncAuth.GetAuthorizedUsers()
 	authorizedUsers := make([]sshuserhash.UserIDHash, len(protoUsers))
 	for i, hash := range protoUsers {
@@ -207,12 +214,17 @@ func (e *Engine) updateVNCServerAuth(vncAuth *mgmProto.VNCAuth) {
 }
 
 // GetVNCServerStatus returns whether the VNC server is running and the list
-// of active VNC sessions.
+// of active VNC sessions. The pointer is captured under syncMsgMux so a
+// concurrent updateVNC/stopVNCServer cannot swap it out between the nil
+// check and the ActiveSessions call.
 func (e *Engine) GetVNCServerStatus() (enabled bool, sessions []vncserver.ActiveSessionInfo) {
-	if e.vncSrv == nil {
+	e.syncMsgMux.Lock()
+	vncSrv := e.vncSrv
+	e.syncMsgMux.Unlock()
+	if vncSrv == nil {
 		return false, nil
 	}
-	return true, e.vncSrv.ActiveSessions()
+	return true, vncSrv.ActiveSessions()
 }
 
 func (e *Engine) stopVNCServer() error {

client/vnc/server/agent_windows.go

@@ -324,7 +324,11 @@ func spawnAgentInSession(sessionID uint32, socketPath, authToken string, jobHand
 	}
 
 	if _, err := windows.ResumeThread(pi.Thread); err != nil {
-		log.Warnf("resume agent main thread: %v", err)
+		_ = windows.CloseHandle(pi.Thread)
+		_ = windows.TerminateProcess(pi.Process, 1)
+		_ = windows.CloseHandle(pi.Process)
+		_ = windows.CloseHandle(stderrRead)
+		return 0, fmt.Errorf("ResumeThread: %w", err)
 	}
 	_ = windows.CloseHandle(pi.Thread)
 

client/vnc/server/noise_auth_test.go

@@ -243,6 +243,7 @@ func TestNoise_WrongServerStatic_HandshakeFails(t *testing.T) {
 		CipherSuite:   vncNoiseSuite,
 		Pattern:       noise.HandshakeIK,
 		Initiator:     true,
+		Prologue:      BuildVNCNoisePrologue(ModeAttach, ""),
 		StaticKeypair: clientKey,
 		PeerStatic:    bogusServerKey.Public,
 	})
@@ -382,6 +383,7 @@ func TestNoise_NoIdentityKey_FailsClosed(t *testing.T) {
 		CipherSuite:   vncNoiseSuite,
 		Pattern:       noise.HandshakeIK,
 		Initiator:     true,
+		Prologue:      BuildVNCNoisePrologue(ModeAttach, ""),
 		StaticKeypair: clientKey,
 		PeerStatic:    fakeServerKey.Public,
 	})

client/vnc/server/server.go

@@ -180,6 +180,10 @@ type Server struct {
 	netstackNet *netstack.Net
 	// agentToken holds the raw token bytes for agent-mode auth.
 	agentToken []byte
+	// invalidAgentToken latches when AgentTokenHex was provided but failed
+	// to decode. Start refuses to listen in that case so the daemon never
+	// silently downgrades the local IPC hop to unauthenticated access.
+	invalidAgentToken bool
 	// identityKey is the daemon's static X25519 private key used in the
 	// Noise_IK handshake. Nil disables the handshake.
 	identityKey []byte
@@ -356,6 +360,7 @@ func New(cfg Config) *Server {
 		if b, err := hex.DecodeString(cfg.AgentTokenHex); err == nil {
 			s.agentToken = b
 		} else {
+			s.invalidAgentToken = true
 			s.log.Warnf("invalid agent token: %v", err)
 		}
 	}
@@ -578,6 +583,9 @@ func (s *Server) Start(ctx context.Context, addr netip.AddrPort, network netip.P
 	if s.listener != nil {
 		return fmt.Errorf("server already running")
 	}
+	if s.invalidAgentToken {
+		return fmt.Errorf("invalid agent token configuration")
+	}
 
 	s.ctx, s.cancel = context.WithCancel(ctx)
 	s.vmgr = s.platformSessionManager()
@@ -686,13 +694,17 @@ func (s *Server) acceptLoop() {
 			continue
 		}
 
+		// Track before any early-reject path so a concurrent Stop's
+		// closeActiveSessions snapshot can never miss a just-accepted
+		// socket and let it survive shutdown.
+		s.trackConn(conn)
 		if !s.tryAcquireConnSlot() {
+			s.untrackConn(conn)
 			s.log.Warnf("rejecting VNC connection from %s: %d concurrent connections in flight", conn.RemoteAddr(), maxConcurrentVNCConns)
 			_ = conn.Close()
 			continue
 		}
 		enableTCPKeepAlive(conn, s.log)
-		s.trackConn(conn)
 		go func(c net.Conn) {
 			defer s.releaseConnSlot()
 			defer s.untrackConn(c)

client/vnc/server/server_windows.go

@@ -54,6 +54,11 @@ func sasSecurityAttributes() (*windows.SecurityAttributes, error) {
 type sasOriginalState struct {
 	had   bool   // true if the value existed before we wrote
 	value uint32 // its prior DWORD value, if had == true
+	// captured stays true once we have read the genuine pre-enable state
+	// for the first time, so a second enableSoftwareSAS call (e.g. after
+	// a daemon restart with no intervening disable) cannot overwrite the
+	// snapshot with our own forced value.
+	captured bool
 }
 
 var savedSASState sasOriginalState
@@ -74,10 +79,12 @@ func enableSoftwareSAS() {
 	}
 	defer key.Close()
 
-	if prev, _, err := key.GetIntegerValue("SoftwareSASGeneration"); err == nil {
-		savedSASState = sasOriginalState{had: true, value: uint32(prev)}
-	} else {
-		savedSASState = sasOriginalState{had: false}
+	if !savedSASState.captured {
+		if prev, _, err := key.GetIntegerValue("SoftwareSASGeneration"); err == nil {
+			savedSASState = sasOriginalState{had: true, value: uint32(prev), captured: true}
+		} else {
+			savedSASState = sasOriginalState{had: false, captured: true}
+		}
 	}
 
 	if err := key.SetDWordValue("SoftwareSASGeneration", 1); err != nil {

management/server/types/networkmap_components.go

@@ -243,7 +243,7 @@ func (c *NetworkMapComponents) resolveRuleEndpoint(
 	postureChecks []string,
 ) ([]*nbpeer.Peer, bool) {
 	if resource.Type == ResourceTypePeer && resource.ID != "" {
-		return c.getPeerFromResource(resource, peerID)
+		return c.getPeerFromResource(resource, peerID, postureChecks)
 	}
 	return c.getAllPeersFromGroups(groups, peerID, postureChecks)
 }
@@ -385,8 +385,11 @@ func (c *NetworkMapComponents) getUniquePeerIDsFromGroupsIDs(groups []string) []
 	return ids
 }
 
-func (c *NetworkMapComponents) getPeerFromResource(resource Resource, peerID string) ([]*nbpeer.Peer, bool) {
+func (c *NetworkMapComponents) getPeerFromResource(resource Resource, peerID string, postureChecks []string) ([]*nbpeer.Peer, bool) {
 	if resource.ID == peerID {
+		if len(postureChecks) > 0 && !c.ValidatePostureChecksOnPeer(peerID, postureChecks) {
+			return []*nbpeer.Peer{}, false
+		}
 		return []*nbpeer.Peer{}, true
 	}
 
@@ -394,6 +397,9 @@ func (c *NetworkMapComponents) getPeerFromResource(resource Resource, peerID str
 	if peerInfo == nil {
 		return []*nbpeer.Peer{}, false
 	}
+	if len(postureChecks) > 0 && !c.ValidatePostureChecksOnPeer(resource.ID, postureChecks) {
+		return []*nbpeer.Peer{}, false
+	}
 
 	return []*nbpeer.Peer{peerInfo}, false
 }

management/server/types/policy_authorized_users.go

@@ -59,9 +59,10 @@ func applyResolvedRuleToState(
 ) {
 	emitRuleDirections(rule, sourcePeers, destPeers, peerInSources, peerInDestinations, generateResources)
 
+	receivingPeer := peerInDestinations || (rule.Bidirectional && peerInSources)
 	switch {
 	case rule.Protocol == PolicyRuleProtocolNetbirdSSH:
-		if !peerInDestinations {
+		if !receivingPeer {
 			return
 		}
 		state.sshEnabled = true
@@ -69,7 +70,7 @@ func applyResolvedRuleToState(
 	case rule.Protocol == PolicyRuleProtocolNetbirdVNC:
 		cb.handleVNCRule(rule, peerInSources, peerInDestinations, state)
 	case policyRuleImpliesLegacySSH(rule) && targetPeerSSHEnabled:
-		if !peerInDestinations {
+		if !receivingPeer {
 			return
 		}
 		state.sshEnabled = true