From bf2fb2fd441d92fb3bcf896e4173534be4a9d502 Mon Sep 17 00:00:00 2001 From: Viktor Liu Date: Sun, 24 May 2026 18:52:57 +0200 Subject: [PATCH] Address CodeRabbit review on embedded VNC PR --- client/internal/engine_vnc.go | 20 +++++++++++++++---- client/vnc/server/agent_windows.go | 6 +++++- client/vnc/server/noise_auth_test.go | 2 ++ client/vnc/server/server.go | 14 ++++++++++++- client/vnc/server/server_windows.go | 15 ++++++++++---- .../server/types/networkmap_components.go | 10 ++++++++-- .../server/types/policy_authorized_users.go | 5 +++-- 7 files changed, 58 insertions(+), 14 deletions(-) diff --git a/client/internal/engine_vnc.go b/client/internal/engine_vnc.go index ab1f4f216..9cebd014e 100644 --- a/client/internal/engine_vnc.go +++ b/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 { diff --git a/client/vnc/server/agent_windows.go b/client/vnc/server/agent_windows.go index 64192d469..74fa7e0dc 100644 --- a/client/vnc/server/agent_windows.go +++ b/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) diff --git a/client/vnc/server/noise_auth_test.go b/client/vnc/server/noise_auth_test.go index 9ec30d442..aa9baad40 100644 --- a/client/vnc/server/noise_auth_test.go +++ b/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, }) diff --git a/client/vnc/server/server.go b/client/vnc/server/server.go index a19fbcdc2..d14584625 100644 --- a/client/vnc/server/server.go +++ b/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) diff --git a/client/vnc/server/server_windows.go b/client/vnc/server/server_windows.go index 7ad88eef7..0d13dc045 100644 --- a/client/vnc/server/server_windows.go +++ b/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 { diff --git a/management/server/types/networkmap_components.go b/management/server/types/networkmap_components.go index 0b9a2d505..b8572fc7b 100644 --- a/management/server/types/networkmap_components.go +++ b/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 } diff --git a/management/server/types/policy_authorized_users.go b/management/server/types/policy_authorized_users.go index 8ce0c48f7..486446f95 100644 --- a/management/server/types/policy_authorized_users.go +++ b/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