Add per-connection user-approval prompts for VNC

This commit is contained in:
Viktor Liu
2026-05-23 18:33:55 +02:00
parent c29ef638f4
commit 8e72967bbe
38 changed files with 2183 additions and 492 deletions
+20 -9
View File
@@ -72,6 +72,11 @@ func (s *Server) handleServiceConnection(conn net.Conn, sa sessionAgent) {
}
s.registerConnAuth(conn, header)
allow, decision := s.gateApproval(conn, header, authedLog)
if !allow {
return
}
socketPath, token, err := sa.Resolve(s.ctx)
if err != nil {
code := RejectCodeCapturerError
@@ -87,7 +92,7 @@ func (s *Server) handleServiceConnection(conn net.Conn, sa sessionAgent) {
Reader: io.MultiReader(&headerBuf, conn),
Conn: conn,
}
if err := proxyToAgent(s.ctx, replayConn, socketPath, token); err != nil {
if err := proxyToAgent(s.ctx, replayConn, socketPath, token, decision.ViewOnly); err != nil {
rejectConnection(conn, codeMessage(RejectCodeCapturerError, err.Error()))
authedLog.Warnf("VNC connection rejected: agent unreachable: %v", err)
return
@@ -124,12 +129,13 @@ func generateAuthToken() (string, error) {
}
// proxyToAgent dials the per-session agent's Unix socket, writes the
// raw token bytes, then copies bytes both ways until either side closes.
// The token must precede any RFB byte so the agent's verifyAgentToken
// can run first. Returns nil once a stream is established; the caller is
// responsible for sending an RFB-level rejection on error so the client
// sees a reason instead of a bare timeout.
func proxyToAgent(ctx context.Context, client net.Conn, socketPath, authToken string) error {
// raw token bytes plus a single view-only flag byte, then copies bytes
// both ways until either side closes. The token + flag prefix must
// precede any RFB byte so the agent's verifyAgentToken can run first.
// Returns nil once a stream is established; the caller is responsible
// for sending an RFB-level rejection on error so the client sees a
// reason instead of a bare timeout.
func proxyToAgent(ctx context.Context, client net.Conn, socketPath, authToken string, viewOnly bool) error {
tokenBytes, err := hex.DecodeString(authToken)
if err != nil || len(tokenBytes) != agentTokenLen {
return fmt.Errorf("invalid auth token (len=%d): %w", len(tokenBytes), err)
@@ -140,9 +146,14 @@ func proxyToAgent(ctx context.Context, client net.Conn, socketPath, authToken st
return fmt.Errorf("dial agent at %s: %w", socketPath, err)
}
if _, err := agentConn.Write(tokenBytes); err != nil {
preamble := make([]byte, len(tokenBytes)+1)
copy(preamble, tokenBytes)
if viewOnly {
preamble[len(tokenBytes)] = 1
}
if _, err := agentConn.Write(preamble); err != nil {
_ = agentConn.Close()
return fmt.Errorf("send auth token to agent: %w", err)
return fmt.Errorf("send auth preamble to agent: %w", err)
}
defer client.Close()
+1 -1
View File
@@ -15,7 +15,7 @@ import (
"github.com/stretchr/testify/require"
"golang.org/x/crypto/curve25519"
sshauth "github.com/netbirdio/netbird/client/ssh/auth"
sshauth "github.com/netbirdio/netbird/shared/sessionauth"
sshuserhash "github.com/netbirdio/netbird/shared/sshauth"
)
+137 -28
View File
@@ -23,7 +23,7 @@ import (
"golang.org/x/crypto/curve25519"
"golang.zx2c4.com/wireguard/tun/netstack"
sshauth "github.com/netbirdio/netbird/client/ssh/auth"
sshauth "github.com/netbirdio/netbird/shared/sessionauth"
)
// Connection modes sent by the client in the session header.
@@ -36,12 +36,14 @@ const (
// stable so clients can branch on them without parsing free text.
// Format: "CODE: human message".
const (
RejectCodeAuthForbidden = "AUTH_FORBIDDEN"
RejectCodeSessionError = "SESSION_ERROR"
RejectCodeCapturerError = "CAPTURER_ERROR"
RejectCodeUnsupportedOS = "UNSUPPORTED"
RejectCodeBadRequest = "BAD_REQUEST"
RejectCodeNoConsoleUser = "NO_CONSOLE_USER"
RejectCodeAuthForbidden = "AUTH_FORBIDDEN"
RejectCodeSessionError = "SESSION_ERROR"
RejectCodeCapturerError = "CAPTURER_ERROR"
RejectCodeUnsupportedOS = "UNSUPPORTED"
RejectCodeBadRequest = "BAD_REQUEST"
RejectCodeNoConsoleUser = "NO_CONSOLE_USER"
RejectCodeApprovalDenied = "APPROVAL_DENIED"
RejectCodeNoApprover = "NO_APPROVER"
)
// EnvVNCDisableDownscale disables any platform-specific framebuffer
@@ -173,11 +175,11 @@ type Server struct {
network netip.Prefix
log *log.Entry
mu sync.Mutex
listener net.Listener
ctx context.Context
cancel context.CancelFunc
vmgr virtualSessionManager
mu sync.Mutex
listener net.Listener
ctx context.Context
cancel context.CancelFunc
vmgr virtualSessionManager
authorizer *sshauth.Authorizer
netstackNet *netstack.Net
// agentToken holds the raw token bytes for agent-mode auth.
@@ -216,6 +218,14 @@ type Server struct {
// this to its metrics framework.
sessionRecorder func(SessionTick)
// requireApproval enables the per-connection user-accept gate. When
// true and approver is nil (or returns an error), the connection is
// rejected before any agent or session work.
requireApproval bool
// approver prompts the local user (via the daemon→UI event channel)
// to accept or deny each incoming connection.
approver Approver
// preListener, when non-nil, replaces the TCP listener Start would
// open; addr/network args to Start are ignored. Used by the agent's
// Unix-socket path.
@@ -275,6 +285,40 @@ type Config struct {
// addr/network args to Start are then ignored. The agent uses this to
// listen on a Unix socket.
Listener net.Listener
// RequireApproval gates each accepted connection on a user-side accept
// prompt before the proxy/session starts. Requires Approver to be set;
// otherwise the gate fails closed.
RequireApproval bool
// Approver brokers the per-connection prompt to the local user via the
// daemon→UI event channel. Nil disables the gate.
Approver Approver
}
// Approver decouples the VNC server from the approval broker. A non-nil
// error means "do not proceed".
type Approver interface {
Request(ctx context.Context, info ApprovalInfo) (ApprovalDecision, error)
}
// ApprovalDecision carries the parts of the user's response the VNC
// server acts on. Accept is implicit (errors signal deny). ViewOnly puts
// the session into read-only mode: the server drops input events.
type ApprovalDecision struct {
ViewOnly bool
}
// ApprovalInfo describes the pending connection passed to the approver.
// Fields are best-effort; any may be empty.
type ApprovalInfo struct {
PeerName string
PeerPubKey string
SourceIP string
Mode string
Username string
// Initiator is the display name of the user who initiated the
// connection (typically the dashboard user). Resolved from the
// Noise-verified client static pubkey.
Initiator string
}
// New creates a VNC server from the provided Config. IdentityKey is the
@@ -287,6 +331,8 @@ func New(cfg Config) *Server {
identityKey: cfg.IdentityKey,
serviceMode: cfg.ServiceMode,
sessionRecorder: cfg.SessionRecorder,
requireApproval: cfg.RequireApproval,
approver: cfg.Approver,
disableAuth: cfg.DisableAuth,
netstackNet: cfg.NetstackNet,
preListener: cfg.Listener,
@@ -377,6 +423,59 @@ func (s *Server) untrackConn(c net.Conn) {
s.sessionsMu.Unlock()
}
// gateApproval prompts the local user to accept or deny conn before any
// session resources are allocated. On rejection the conn already received
// an RFB reject reason; the gate does not close it.
func (s *Server) gateApproval(conn net.Conn, header *connectionHeader, connLog *log.Entry) (bool, ApprovalDecision) {
if !s.requireApproval {
return true, ApprovalDecision{}
}
if s.approver == nil {
rejectConnection(conn, codeMessage(RejectCodeNoApprover, "approval required but no approver configured"))
connLog.Warn("VNC connection rejected: approval required but no approver")
return false, ApprovalDecision{}
}
info := ApprovalInfo{
SourceIP: sourceIPString(conn.RemoteAddr()),
Mode: modeString(header.mode),
Username: header.username,
}
if len(header.clientStatic) == 32 {
info.PeerPubKey = hex.EncodeToString(header.clientStatic)
if s.authorizer != nil {
info.Initiator = s.authorizer.LookupSessionDisplayName(header.clientStatic)
}
}
decision, err := s.approver.Request(s.ctx, info)
if err != nil {
rejectConnection(conn, codeMessage(RejectCodeApprovalDenied, err.Error()))
connLog.Infof("VNC connection rejected: approval %v", err)
return false, ApprovalDecision{}
}
if decision.ViewOnly {
connLog.Info("VNC connection approved by user (view-only)")
} else {
connLog.Info("VNC connection approved by user")
}
return true, decision
}
// sourceIPString returns the IP portion of a remote address, or the full
// string when no port is present (e.g. unix sockets).
func sourceIPString(addr net.Addr) string {
if addr == nil {
return ""
}
if ta, ok := addr.(*net.TCPAddr); ok && ta != nil {
return ta.IP.String()
}
host, _, err := net.SplitHostPort(addr.String())
if err != nil {
return addr.String()
}
return host
}
// registerConnAuth records the verified Noise_IK identity for a live
// connection so UpdateVNCAuth can later revoke it if policy changes.
// No-op when auth is disabled (e.g. agent-mode loopback connections).
@@ -673,7 +772,8 @@ func (s *Server) handleConnection(conn net.Conn) {
_ = conn.Close()
return
}
if !s.verifyAgentToken(conn, connLog) {
ok, agentViewOnly := s.verifyAgentToken(conn, connLog)
if !ok {
connLog.Info("VNC connection rejected: agent token check failed")
return
}
@@ -683,13 +783,19 @@ func (s *Server) handleConnection(conn net.Conn) {
_ = conn.Close()
return
}
connLog, sessionUserID, ok := s.authorizeSession(conn, header, connLog)
var sessionUserID string
connLog, sessionUserID, ok = s.authorizeSession(conn, header, connLog)
if !ok {
connLog.Info("VNC connection rejected: auth failed")
return
}
s.registerConnAuth(conn, header)
allow, decision := s.gateApproval(conn, header, connLog)
if !allow {
return
}
capturer, injector, sessionCleanup, ok := s.acquireSessionResources(conn, header, &connLog)
if !ok {
connLog.Warn("VNC connection rejected: capturer/injector unavailable")
@@ -726,6 +832,7 @@ func (s *Server) handleConnection(conn net.Conn) {
serverW: w,
serverH: h,
log: connLog,
viewOnly: decision.ViewOnly || agentViewOnly,
}
sess.serve()
connLog.Infof("VNC connection closed (%dms)", time.Since(start).Milliseconds())
@@ -791,8 +898,9 @@ func (s *Server) authenticateSession(header *connectionHeader) (string, error) {
var vncIdentityMagic = []byte("NBV3")
// Noise_IK_25519_ChaChaPoly_SHA256 message sizes (with empty payloads).
// msg1 = e(32) + s_AEAD(32+16) + payload_AEAD(0+16) = 96 bytes
// msg2 = e(32) + payload_AEAD(0+16) = 48 bytes
//
// msg1 = e(32) + s_AEAD(32+16) + payload_AEAD(0+16) = 96 bytes
// msg2 = e(32) + payload_AEAD(0+16) = 48 bytes
const (
noiseInitiatorMsgLen = 96
noiseResponderMsgLen = 48
@@ -929,39 +1037,40 @@ func (s *Server) maybeRunNoiseHandshake(conn net.Conn, br *bufio.Reader) ([]byte
return clientStatic, true, nil
}
// verifyAgentToken validates the agent token prefix when configured. Returns
// false when the token is invalid or unreadable; the connection is closed.
func (s *Server) verifyAgentToken(conn net.Conn, connLog *log.Entry) bool {
// verifyAgentToken validates the agent token prefix when configured and
// reads the trailing view-only flag byte the daemon writes alongside it.
// Returns (ok, viewOnly). ok=false closes the connection.
func (s *Server) verifyAgentToken(conn net.Conn, connLog *log.Entry) (bool, bool) {
if len(s.agentToken) == 0 {
return true
return true, false
}
buf := make([]byte, len(s.agentToken))
buf := make([]byte, len(s.agentToken)+1)
if err := conn.SetReadDeadline(time.Now().Add(5 * time.Second)); err != nil {
connLog.Debugf("set agent token deadline: %v", err)
conn.Close()
return false
return false, false
}
if _, err := io.ReadFull(conn, buf); err != nil {
if errors.Is(err, io.EOF) || errors.Is(err, io.ErrUnexpectedEOF) {
// Connect-then-close probes (port liveness checks) hit this
// path on every dial; logging them would just flood the
// daemon log without surfacing a real failure.
connLog.Tracef("agent auth: read token: %v", err)
connLog.Tracef("agent auth: read preamble: %v", err)
} else {
connLog.Warnf("agent auth: read token: %v", err)
connLog.Warnf("agent auth: read preamble: %v", err)
}
conn.Close()
return false
return false, false
}
if err := conn.SetReadDeadline(time.Time{}); err != nil {
connLog.Debugf("clear agent token deadline: %v", err)
}
if subtle.ConstantTimeCompare(buf, s.agentToken) != 1 {
if subtle.ConstantTimeCompare(buf[:len(s.agentToken)], s.agentToken) != 1 {
connLog.Warn("agent auth: invalid token, rejecting")
conn.Close()
return false
return false, false
}
return true
return true, buf[len(s.agentToken)] != 0
}
// authorizeSession runs the Noise_IK handshake when auth is enabled.
+181
View File
@@ -3,15 +3,19 @@
package server
import (
"context"
"encoding/binary"
"encoding/hex"
"errors"
"image"
"io"
"net"
"net/netip"
"sync/atomic"
"testing"
"time"
log "github.com/sirupsen/logrus"
"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/require"
)
@@ -333,3 +337,180 @@ func TestSessionMode_RejectedWhenNoVMGR(t *testing.T) {
require.NoError(t, err)
assert.Contains(t, string(reason), RejectCodeUnsupportedOS)
}
// recordingApprover lets gate tests choose the outcome of the approval
// prompt and verify how often (and with what info) the gate calls it.
type recordingApprover struct {
calls atomic.Int32
lastIn ApprovalInfo
decision ApprovalDecision
respond error
}
func (r *recordingApprover) Request(_ context.Context, info ApprovalInfo) (ApprovalDecision, error) {
r.calls.Add(1)
r.lastIn = info
if r.respond != nil {
return ApprovalDecision{}, r.respond
}
return r.decision, nil
}
// drainRejectClient simulates a remote VNC client just enough that
// rejectConnection's handshake-half completes promptly: it reads the
// server's "RFB 003.008\n", writes back a placeholder client version, and
// drains until EOF. Without this the rejectConnection path would block
// for up to two seconds on its SetReadDeadline.
func drainRejectClient(t *testing.T, c net.Conn) {
t.Helper()
go func() {
defer c.Close()
var srvVer [12]byte
if _, err := io.ReadFull(c, srvVer[:]); err != nil {
return
}
_, _ = c.Write([]byte("RFB 003.008\n"))
_, _ = io.Copy(io.Discard, c)
}()
}
// newGateConn returns a server-side conn and a client-side conn linked by
// net.Pipe, with the client-side already draining so gateApproval's
// rejectConnection path completes without blocking the test.
func newGateConn(t *testing.T) net.Conn {
t.Helper()
srv, cli := net.Pipe()
drainRejectClient(t, cli)
t.Cleanup(func() { _ = srv.Close() })
return srv
}
func gateTestServer(requireApproval bool, approver Approver) *Server {
return &Server{
log: log.WithField("test", "gate"),
requireApproval: requireApproval,
approver: approver,
}
}
// TestGateApproval_Disabled_NoApproverCall: when the feature is off the
// gate must short-circuit before consulting any approver. A nil approver
// must NOT mean "deny" here — that would break upgrades for peers that
// haven't opted in yet.
func TestGateApproval_Disabled_NoApproverCall(t *testing.T) {
app := &recordingApprover{}
srv := gateTestServer(false, app)
conn := newGateConn(t)
defer conn.Close()
header := &connectionHeader{mode: ModeAttach}
allowed, _ := srv.gateApproval(conn, header, srv.log)
assert.True(t, allowed, "gate must pass through when requireApproval is false")
assert.Equal(t, int32(0), app.calls.Load(), "approver must not be called when disabled")
}
// TestGateApproval_Enabled_NilApproverDenies is the most important
// regression test for "no silent bypass": if the feature is enabled but
// the broker wasn't wired (a misconfiguration), the gate must REJECT,
// not pass through. The reject code must be the dedicated NO_APPROVER so
// the failure is unambiguous in logs and on the client side.
func TestGateApproval_Enabled_NilApproverDenies(t *testing.T) {
srv := gateTestServer(true, nil)
srvConn, cliConn := net.Pipe()
defer srvConn.Close()
defer cliConn.Close()
// Capture the reject reason the gate sends.
rejectReason := make(chan string, 1)
go func() {
var srvVer [12]byte
_, _ = io.ReadFull(cliConn, srvVer[:])
_, _ = cliConn.Write([]byte("RFB 003.008\n"))
// Server sends: 1 byte (numTypes=0), 4 bytes (reason len), reason.
var numTypes [1]byte
_, _ = io.ReadFull(cliConn, numTypes[:])
var lenBuf [4]byte
_, _ = io.ReadFull(cliConn, lenBuf[:])
reason := make([]byte, binary.BigEndian.Uint32(lenBuf[:]))
_, _ = io.ReadFull(cliConn, reason)
rejectReason <- string(reason)
}()
header := &connectionHeader{mode: ModeAttach}
allowed, _ := srv.gateApproval(srvConn, header, srv.log)
assert.False(t, allowed, "missing approver MUST deny; never silently pass")
select {
case reason := <-rejectReason:
assert.Contains(t, reason, RejectCodeNoApprover, "reject code must surface the misconfiguration cause")
case <-time.After(2 * time.Second):
t.Fatal("did not observe rejection reason")
}
}
// TestGateApproval_ApproverDenies maps every approver error to a deny.
// We assert against every Err* the broker can produce so a future caller
// adding a new error doesn't accidentally fall into a default-allow.
func TestGateApproval_ApproverDenies(t *testing.T) {
cases := []struct {
name string
err error
}{
{"denied", errors.New("user denied")},
{"timeout", errors.New("approval timed out")},
{"no_subscriber", errors.New("no UI subscriber connected for approval")},
{"ctx_canceled", context.Canceled},
{"misc", errors.New("anything else")},
}
for _, tc := range cases {
t.Run(tc.name, func(t *testing.T) {
app := &recordingApprover{respond: tc.err}
srv := gateTestServer(true, app)
conn := newGateConn(t)
defer conn.Close()
header := &connectionHeader{mode: ModeAttach}
allowed, _ := srv.gateApproval(conn, header, srv.log)
assert.False(t, allowed, "approver error %v must deny", tc.err)
assert.Equal(t, int32(1), app.calls.Load())
})
}
}
// TestGateApproval_ApproverAccepts confirms the happy path actually
// returns true so we know the deny path is not the only outcome the
// gate can produce.
func TestGateApproval_ApproverAccepts(t *testing.T) {
app := &recordingApprover{respond: nil}
srv := gateTestServer(true, app)
conn := newGateConn(t)
defer conn.Close()
header := &connectionHeader{mode: ModeAttach, username: "alice"}
allowed, _ := srv.gateApproval(conn, header, srv.log)
assert.True(t, allowed, "approver returning nil must let the gate pass")
assert.Equal(t, int32(1), app.calls.Load())
assert.Equal(t, "alice", app.lastIn.Username, "header username must reach the approver")
}
// TestGateApproval_PassesPubKeyHex confirms the gate hex-encodes the
// 32-byte client static key into ApprovalInfo.PeerPubKey so the prompt's
// metadata identifies which peer is connecting. A wrong-length key must
// NOT bypass the gate; it just won't populate the field.
func TestGateApproval_PassesPubKeyHex(t *testing.T) {
app := &recordingApprover{respond: nil}
srv := gateTestServer(true, app)
conn := newGateConn(t)
defer conn.Close()
pub := make([]byte, 32)
for i := range pub {
pub[i] = byte(i)
}
header := &connectionHeader{mode: ModeAttach, clientStatic: pub}
allowed, _ := srv.gateApproval(conn, header, srv.log)
assert.True(t, allowed)
assert.Equal(t, hex.EncodeToString(pub), app.lastIn.PeerPubKey)
}
+44 -6
View File
@@ -55,6 +55,11 @@ type session struct {
serverH int
desktopName string
log *log.Entry
// viewOnly drops KeyEvent / PointerEvent (legacy + QEMU + extended)
// without invoking the injector when the user approved the
// connection in view-only mode. The bytes are still consumed off the
// wire so the protocol stays in sync.
viewOnly bool
writeMu sync.Mutex
// encMu guards the negotiated pixel format and encoding state below.
@@ -161,6 +166,15 @@ func (s *session) serve() {
}
s.log.Infof("client connected: %s", s.addr())
// View-only clients can't move the pointer, so default to compositing
// the host cursor into the framebuffer. The client can still send
// ShowRemoteCursor to turn it off.
if s.viewOnly {
s.encMu.Lock()
s.showRemoteCursor = true
s.encMu.Unlock()
}
// On any exit path (clean disconnect, transport error, panic) release
// modifier keys and mouse buttons so the host doesn't end up with
// Shift/Ctrl/Alt or a mouse button stuck because the client dropped
@@ -226,8 +240,10 @@ func (s *session) handshake() error {
// mode, username) that precedes the RFB handshake; the protocol-level
// password scheme is not supported.
func (s *session) sendSecurityTypes() error {
_, err := s.conn.Write([]byte{1, secNone})
return err
if _, err := s.conn.Write([]byte{1, secNone}); err != nil {
return err
}
return nil
}
func (s *session) handleSecurity(secType byte) error {
@@ -237,11 +253,20 @@ func (s *session) handleSecurity(secType byte) error {
return binary.Write(s.conn, binary.BigEndian, uint32(0))
}
// ViewOnlyDesktopNamePrefix tags the RFB desktop name when the host
// approved the connection in view-only mode, so a NetBird-aware client
// can switch its UI into read-only state. NUL framing guarantees no
// collision with a user-set name.
const ViewOnlyDesktopNamePrefix = "\x00NB-VIEW-ONLY\x00"
func (s *session) sendServerInit() error {
desktop := s.desktopName
if desktop == "" {
desktop = "NetBird VNC"
}
if s.viewOnly {
desktop = ViewOnlyDesktopNamePrefix + desktop
}
name := []byte(desktop)
buf := make([]byte, 0, 4+16+4+len(name))
@@ -259,8 +284,10 @@ func (s *session) sendServerInit() error {
)
buf = append(buf, name...)
_, err := s.conn.Write(buf)
return err
if _, err := s.conn.Write(buf); err != nil {
return err
}
return nil
}
func (s *session) messageLoop() error {
@@ -536,8 +563,10 @@ func (s *session) SendDesktopName(name string) error {
if _, err := s.conn.Write(header); err != nil {
return err
}
_, err := s.conn.Write(body)
return err
if _, err := s.conn.Write(body); err != nil {
return err
}
return nil
}
func (s *session) handleKeyEvent() error {
@@ -545,6 +574,9 @@ func (s *session) handleKeyEvent() error {
if _, err := io.ReadFull(s.conn, data[:]); err != nil {
return fmt.Errorf("read KeyEvent: %w", err)
}
if s.viewOnly {
return nil
}
down := data[0] == 1
keysym := binary.BigEndian.Uint32(data[3:7])
s.injector.InjectKey(keysym, down)
@@ -565,6 +597,9 @@ func (s *session) handleQEMUMessage() error {
s.log.Tracef("ignoring QEMU subtype %d", subtype)
return nil
}
if s.viewOnly {
return nil
}
down := binary.BigEndian.Uint16(data[1:3]) != 0
keysym := binary.BigEndian.Uint32(data[3:7])
scancode := binary.BigEndian.Uint32(data[7:11])
@@ -598,6 +633,9 @@ func (s *session) handlePointerEvent() error {
s.lastPointerX = x
s.lastPointerY = y
s.pointerMu.Unlock()
if s.viewOnly {
return nil
}
s.injector.InjectPointer(mask, x, y, s.serverW, s.serverH)
return nil
}