From 412193c602c6a2bf7697d8c92d0154d1a45ae792 Mon Sep 17 00:00:00 2001 From: Viktor Liu Date: Thu, 21 May 2026 18:09:07 +0200 Subject: [PATCH] Address CodeRabbit VNC review feedback --- client/ssh/auth/auth.go | 10 +++++++++ client/status/status.go | 2 ++ client/vnc/server/input_uinput_freebsd.go | 17 ++++++++++++++ client/vnc/server/input_windows.go | 4 ---- client/vnc/server/noise_auth_test.go | 13 ++++++----- client/wasm/internal/vnc/proxy.go | 22 +++++++------------ .../http/handlers/peers/peers_handler.go | 10 +++++++++ management/server/types/policyrule.go | 8 +++---- shared/management/http/api/openapi.yml | 2 +- 9 files changed, 59 insertions(+), 29 deletions(-) create mode 100644 client/vnc/server/input_uinput_freebsd.go diff --git a/client/ssh/auth/auth.go b/client/ssh/auth/auth.go index 52a548a81..782c816af 100644 --- a/client/ssh/auth/auth.go +++ b/client/ssh/auth/auth.go @@ -117,12 +117,22 @@ func (a *Authorizer) Update(config *Config) { a.machineUsers = machineUsers sessionPubKeys := make(map[[sessionPubKeyLen]byte]sshuserhash.UserIDHash, len(config.SessionPubKeys)) + conflicted := make(map[[sessionPubKeyLen]byte]struct{}) for _, e := range config.SessionPubKeys { if len(e.PubKey) != sessionPubKeyLen { continue } var key [sessionPubKeyLen]byte copy(key[:], e.PubKey) + if _, bad := conflicted[key]; bad { + continue + } + if existing, ok := sessionPubKeys[key]; ok && existing != e.UserIDHash { + log.Warnf("SSH auth: session pubkey bound to conflicting user hashes; dropping binding") + delete(sessionPubKeys, key) + conflicted[key] = struct{}{} + continue + } sessionPubKeys[key] = e.UserIDHash } a.sessionPubKeys = sessionPubKeys diff --git a/client/status/status.go b/client/status/status.go index d3aaadf51..30760efe1 100644 --- a/client/status/status.go +++ b/client/status/status.go @@ -1075,5 +1075,7 @@ func anonymizeServerSessions(a *anonymize.Anonymizer, overview *OutputOverview) } for i, sess := range overview.VNCServerState.Sessions { overview.VNCServerState.Sessions[i].RemoteAddress = anonymizeRemoteAddress(a, sess.RemoteAddress) + overview.VNCServerState.Sessions[i].Username = a.AnonymizeString(sess.Username) + overview.VNCServerState.Sessions[i].UserID = a.AnonymizeString(sess.UserID) } } diff --git a/client/vnc/server/input_uinput_freebsd.go b/client/vnc/server/input_uinput_freebsd.go new file mode 100644 index 000000000..08ccc32b2 --- /dev/null +++ b/client/vnc/server/input_uinput_freebsd.go @@ -0,0 +1,17 @@ +//go:build freebsd + +package server + +import "fmt" + +// UInputInjector is a freebsd placeholder; the linux uinput implementation +// uses Linux-only ioctls (UI_DEV_CREATE etc.) and is not portable. +type UInputInjector struct { + StubInputInjector +} + +// NewUInputInjector always returns an error on freebsd so callers fall back +// to a stub or platform-appropriate injector. +func NewUInputInjector(_, _ int) (*UInputInjector, error) { + return nil, fmt.Errorf("uinput not implemented on freebsd") +} diff --git a/client/vnc/server/input_windows.go b/client/vnc/server/input_windows.go index cf3e2505e..c9479538a 100644 --- a/client/vnc/server/input_windows.go +++ b/client/vnc/server/input_windows.go @@ -169,10 +169,6 @@ func (w *WindowsInputInjector) Close() { func (w *WindowsInputInjector) tryEnqueue(cmd inputCmd) { select { case <-w.closed: - return - default: - } - select { case w.ch <- cmd: default: } diff --git a/client/vnc/server/noise_auth_test.go b/client/vnc/server/noise_auth_test.go index 2da2d817b..711dc5bcf 100644 --- a/client/vnc/server/noise_auth_test.go +++ b/client/vnc/server/noise_auth_test.go @@ -268,19 +268,20 @@ func TestNoise_TruncatedMsg1_ClosesConnection(t *testing.T) { conn, err := net.Dial("tcp", addr.String()) require.NoError(t, err) + defer conn.Close() writeHeaderPrefix(t, conn, ModeAttach) _, err = conn.Write([]byte("NBV3")) require.NoError(t, err) _, err = conn.Write(make([]byte, 8)) require.NoError(t, err) - require.NoError(t, conn.Close()) + require.NoError(t, conn.(*net.TCPConn).CloseWrite()) - // Re-dial just to confirm the listener is alive (the previous - // connection terminated server-side without affecting the listener). - probe, err := net.Dial("tcp", addr.String()) - require.NoError(t, err) - require.NoError(t, probe.Close()) + require.NoError(t, conn.SetReadDeadline(time.Now().Add(2*time.Second))) + buf := make([]byte, 64) + n, err := conn.Read(buf) + require.Equal(t, 0, n, "server must not emit RFB bytes after a truncated handshake") + require.ErrorIs(t, err, io.EOF, "server must close the connection on truncated msg1") } // TestNoise_AuthEnabled_NoHandshake_Rejected proves that with auth on, diff --git a/client/wasm/internal/vnc/proxy.go b/client/wasm/internal/vnc/proxy.go index e6ced7ca1..60f03b212 100644 --- a/client/wasm/internal/vnc/proxy.go +++ b/client/wasm/internal/vnc/proxy.go @@ -58,22 +58,19 @@ func NewSessionKey() (string, []byte, error) { return id, kp.Public, nil } -// lookupSessionKey returns the keypair for id, or false if unknown. -func lookupSessionKey(id string) (noise.DHKey, bool) { +// consumeSessionKey atomically retrieves and removes the keypair for id. +// A session handle is single-use; combining lookup and delete under one +// critical section prevents concurrent callers from observing the same key. +func consumeSessionKey(id string) (noise.DHKey, bool) { sessionKeyStore.mu.Lock() defer sessionKeyStore.mu.Unlock() kp, ok := sessionKeyStore.keys[id] + if ok { + delete(sessionKeyStore.keys, id) + } return kp, ok } -// dropSessionKey removes the keypair for id. Called after the VNC -// connection closes (or after a connect attempt fails terminally). -func dropSessionKey(id string) { - sessionKeyStore.mu.Lock() - delete(sessionKeyStore.keys, id) - sessionKeyStore.mu.Unlock() -} - const ( vncProxyHost = "vnc.proxy.local" vncProxyScheme = "ws" @@ -190,13 +187,10 @@ func (p *VNCProxy) CreateProxy(req ProxyRequest) js.Value { height: height, } if req.KeySessionID != "" { - kp, ok := lookupSessionKey(req.KeySessionID) + kp, ok := consumeSessionKey(req.KeySessionID) if !ok { return rejectedPromise("unknown VNC session id") } - // A session handle is single-use; drop it before the destination - // holds the private bytes so a leaked handle can't be replayed. - dropSessionKey(req.KeySessionID) dest.sessionPriv = kp.Private dest.sessionPub = kp.Public pub, err := decodePeerPubKey(req.PeerPublicKey) diff --git a/management/server/http/handlers/peers/peers_handler.go b/management/server/http/handlers/peers/peers_handler.go index 0da2db5de..60263db4e 100644 --- a/management/server/http/handlers/peers/peers_handler.go +++ b/management/server/http/handlers/peers/peers_handler.go @@ -2,6 +2,7 @@ package peers import ( "context" + "encoding/base64" "encoding/json" "fmt" "net/http" @@ -518,6 +519,15 @@ func (h *Handler) CreateTemporaryAccess(w http.ResponseWriter, r *http.Request) policy.Rules[0].AuthorizedUser = userAuth.UserId } if protocol == types.PolicyRuleProtocolNetbirdVNC && req.SessionPubKey != nil { + pub, err := base64.StdEncoding.DecodeString(*req.SessionPubKey) + if err != nil { + util.WriteError(r.Context(), status.Errorf(status.InvalidArgument, "session_pub_key is not valid base64: %v", err), w) + return + } + if len(pub) != 32 { + util.WriteError(r.Context(), status.Errorf(status.InvalidArgument, "session_pub_key must decode to 32 bytes, got %d", len(pub)), w) + return + } policy.Rules[0].SessionPubKey = *req.SessionPubKey } diff --git a/management/server/types/policyrule.go b/management/server/types/policyrule.go index ceb58dd0f..054a08266 100644 --- a/management/server/types/policyrule.go +++ b/management/server/types/policyrule.go @@ -89,10 +89,10 @@ type PolicyRule struct { // AuthorizedUser is a list of userIDs that are authorized to access local resources via ssh AuthorizedUser string - // SessionPubKey is the base64 Ed25519 public key the AuthorizedUser - // will sign session-binding challenges with. Set together with - // AuthorizedUser when the rule was created via temporary-access for - // a VNC scope; empty otherwise. + // SessionPubKey is the base64 X25519 public key used with Noise_IK to + // bind a VNC session to the AuthorizedUser. Set together with + // AuthorizedUser when the rule was created via temporary-access for a + // VNC scope; empty otherwise. SessionPubKey string } diff --git a/shared/management/http/api/openapi.yml b/shared/management/http/api/openapi.yml index 049a1b26d..4a6e1d7d3 100644 --- a/shared/management/http/api/openapi.yml +++ b/shared/management/http/api/openapi.yml @@ -1008,7 +1008,7 @@ components: type: string example: "tcp/80" session_pub_key: - description: Ephemeral Ed25519 public key the requester will sign session-binding challenges with. Required for VNC rules; ignored for SSH and L4. + description: Ephemeral base64-encoded X25519 public key used with Noise_IK to bind the VNC session. Required for VNC rules; ignored for SSH and L4. type: string example: "n0r3pL4c3h0ld3rK3y==" required: