Carry the VNC authorization across a listener rebind

client/internal/engine_vnc.go

@@ -27,6 +27,8 @@ type vncServer interface {
 	AddListener(ctx context.Context, addr netip.AddrPort, network netip.Prefix) error
 	Stop() error
 	ActiveSessions() []vncserver.ActiveSessionInfo
+	UpdateVNCAuth(config *sshauth.Config)
+	VNCAuth() *sshauth.Config
 }
 
 func (e *Engine) setupVNCPortRedirection() error {
@@ -97,26 +99,36 @@ func (e *Engine) updateVNC() error {
 		return nil
 	}
 
-	return e.startVNCServer()
+	return e.startVNCServer(nil)
 }
 
-// restartVNCListeners rebuilds the VNC server so it listens on new sockets.
-// No-op when it is not running. See Engine.rebindOverlayListeners for why this
-// is needed.
+// restartVNCListeners rebuilds the VNC server so it listens on new sockets, on
+// the same terms it was started with. No-op when it is not running. See
+// Engine.rebindOverlayListeners for why this is needed.
 func (e *Engine) restartVNCListeners() error {
 	if e.vncSrv == nil {
 		return nil
 	}
+	// Read from the server before it goes away. A rebuilt one starts with an
+	// empty authorizer, which fails closed, so without carrying the
+	// authorization over every authorized peer is refused until the next
+	// network map happens to bring one.
+	authConfig := e.vncSrv.VNCAuth()
 	if err := e.stopVNCServer(); err != nil {
 		return fmt.Errorf("rebind VNC listeners: %w", err)
 	}
-	if err := e.startVNCServer(); err != nil {
+	if err := e.startVNCServer(authConfig); err != nil {
 		return fmt.Errorf("rebind VNC listeners: %w", err)
 	}
 	return nil
 }
 
-func (e *Engine) startVNCServer() error {
+// startVNCServer builds and starts the VNC server. authConfig is the
+// fine-grained authorization to open with, and is applied before the server
+// accepts anything: a server that starts listening with an empty authorizer
+// refuses the connections that arrive in the meantime. Nil leaves it as
+// management has not sent one yet.
+func (e *Engine) startVNCServer(authConfig *sshauth.Config) error {
 	if e.wgInterface == nil {
 		return errors.New("wg interface not initialized")
 	}
@@ -166,6 +178,7 @@ func (e *Engine) startVNCServer() error {
 		ServiceMode:     serviceMode,
 		SessionRecorder: sessionRecorder,
 		NetstackNet:     e.wgInterface.GetNet(),
+		Auth:            authConfig,
 		RequireApproval: requireApproval,
 		Approver:        approver,
 		// Session start/stop is invisible to the peer status recorder, so push a
@@ -218,13 +231,8 @@ func (e *Engine) updateVNCServerAuth(vncAuth *mgmProto.VNCAuth) {
 		return
 	}
 
-	vncSrv, ok := e.vncSrv.(*vncserver.Server)
-	if !ok {
-		return
-	}
-
 	if vncAuth == nil {
-		vncSrv.UpdateVNCAuth(&sshauth.Config{})
+		e.vncSrv.UpdateVNCAuth(&sshauth.Config{})
 		return
 	}
 
@@ -262,7 +270,7 @@ func (e *Engine) updateVNCServerAuth(vncAuth *mgmProto.VNCAuth) {
 		})
 	}
 
-	vncSrv.UpdateVNCAuth(&sshauth.Config{
+	e.vncSrv.UpdateVNCAuth(&sshauth.Config{
 		AuthorizedUsers: authorizedUsers,
 		MachineUsers:    machineUsers,
 		SessionPubKeys:  sessionPubKeys,

client/vnc/server/noise_auth_test.go

@@ -458,3 +458,44 @@ func TestNoise_SessionMode_OSUserCheckRunsAfterHandshake(t *testing.T) {
 	assert.Contains(t, reason, "no machine user mapping")
 	assert.NotContains(t, reason, sshauth.ErrSessionKeyNotKnown.Error())
 }
+
+// TestNoise_ConfigAuth_InForceBeforeAccept covers the listener rebind: the
+// engine reads the running server's authorization and hands it to the
+// replacement through Config.Auth, so a session key enrolled before the
+// rebind must still authenticate immediately after it, with no
+// UpdateVNCAuth call in between.
+func TestNoise_ConfigAuth_InForceBeforeAccept(t *testing.T) {
+	kp, err := noise.DH25519.GenerateKeypair(nil)
+	require.NoError(t, err)
+
+	// The server the rebind replaces. Never started: only its authorization
+	// is of interest here.
+	previous := New(Config{
+		Capturer:    &testCapturer{},
+		Injector:    &StubInputInjector{},
+		IdentityKey: kp.Private,
+	})
+	clientKey := registerSessionKey(t, previous, "alice@example")
+
+	carried := previous.VNCAuth()
+	require.NotNil(t, carried)
+
+	rebuilt := New(Config{
+		Capturer:    &testCapturer{},
+		Injector:    &StubInputInjector{},
+		IdentityKey: kp.Private,
+		Auth:        carried,
+	})
+	require.NoError(t, rebuilt.Start(t.Context(), netip.MustParseAddrPort("127.0.0.1:0"), netip.MustParsePrefix("127.0.0.0/8")))
+	t.Cleanup(func() { _ = rebuilt.Stop() })
+
+	conn, err := net.Dial("tcp", rebuilt.listener.Addr().String())
+	require.NoError(t, err)
+	defer conn.Close()
+
+	writeHeaderPrefix(t, conn, ModeAttach)
+	performInitiator(t, conn, clientKey, kp.Public)
+	writeHeaderTail(t, conn)
+
+	readRFBGreetingNoFailure(t, conn)
+}

client/vnc/server/server.go

@@ -326,6 +326,11 @@ type Config struct {
 	DisableAuth     bool
 	AgentTokenHex   string
 	NetstackNet     *netstack.Net
+	// Auth is the fine-grained authorization to open with, applied before the
+	// server accepts anything: a server that starts listening with an empty
+	// authorizer refuses the connections that arrive in the meantime. Nil
+	// leaves it as management has not sent one yet.
+	Auth *sshauth.Config
 	// Listener, when set, is used instead of Start opening a TCP listener;
 	// addr/network args to Start are then ignored. The agent uses this to
 	// listen on a Unix socket.
@@ -437,6 +442,9 @@ func New(cfg Config) *Server {
 			s.log.Warnf("invalid agent token: %v", err)
 		}
 	}
+	if cfg.Auth != nil {
+		s.authorizer.Update(cfg.Auth)
+	}
 	return s
 }
 
@@ -749,6 +757,16 @@ func (s *Server) UpdateVNCAuth(config *sshauth.Config) {
 	s.revokeUnauthorizedSessions()
 }
 
+// VNCAuth returns the authorization currently in force, in a form that can be
+// handed to Config.Auth or UpdateVNCAuth to reproduce it. Nil when the server
+// has no authorizer.
+func (s *Server) VNCAuth() *sshauth.Config {
+	if s.authorizer == nil {
+		return nil
+	}
+	return s.authorizer.Config()
+}
+
 // Start begins listening for VNC connections on the given address.
 // network is the NetBird overlay prefix used to validate connection sources.
 // When Config.Listener was supplied, addr and network are ignored and the

shared/sessionauth/auth.go

@@ -217,8 +217,10 @@ func (a *Authorizer) GetUserIDClaim() string {
 	return a.userIDClaim
 }
 
-// Config returns the authorization currently in force. The user list and the
-// machine-user map are copies; the originals stay in use here.
+// Config returns the authorization currently in force, in a form that can be
+// fed back to Update to reproduce it. The user list, the machine-user map and
+// the session-key entries are copies; the originals stay in use here. The
+// session-key entries come out in unspecified order.
 func (a *Authorizer) Config() *Config {
 	a.mu.RLock()
 	defer a.mu.RUnlock()
@@ -228,10 +230,20 @@ func (a *Authorizer) Config() *Config {
 		machineUsers[osUser] = slices.Clone(indexes)
 	}
 
+	sessionPubKeys := make([]SessionPubKey, 0, len(a.sessionPubKeys))
+	for key, userIDHash := range a.sessionPubKeys {
+		sessionPubKeys = append(sessionPubKeys, SessionPubKey{
+			PubKey:      slices.Clone(key[:]),
+			UserIDHash:  userIDHash,
+			DisplayName: a.sessionDisplayNames[key],
+		})
+	}
+
 	return &Config{
 		UserIDClaim:     a.userIDClaim,
 		AuthorizedUsers: slices.Clone(a.authorizedUsers),
 		MachineUsers:    machineUsers,
+		SessionPubKeys:  sessionPubKeys,
 	}
 }
 

shared/sessionauth/auth_test.go

@@ -757,3 +757,61 @@ func TestAuthorizer_AuthorizeSessionKey_UnauthorizedUser(t *testing.T) {
 	_, _, err = a.AuthorizeSessionKey(pub, "alice")
 	require.ErrorIs(t, err, ErrUserNotAuthorized)
 }
+
+// Config must round-trip through Update: the VNC listener rebind reads the
+// running server's authorization and hands it to the replacement, so a session
+// key dropped here is an authorized peer refused until the next network map.
+func TestAuthorizer_Config_RoundTripsSessionPubKeys(t *testing.T) {
+	pub := bytesRepeat(0x77, sessionPubKeyLen)
+	userHash, err := sshauth.HashUserID("alice")
+	require.NoError(t, err)
+
+	original := NewAuthorizer()
+	original.Update(&Config{
+		UserIDClaim:     "email",
+		AuthorizedUsers: []sshauth.UserIDHash{userHash},
+		MachineUsers:    map[string][]uint32{Wildcard: {0}},
+		SessionPubKeys: []SessionPubKey{
+			{PubKey: pub, UserIDHash: userHash, DisplayName: "Alice"},
+		},
+	})
+
+	carried := original.Config()
+	require.NotNil(t, carried)
+	assert.Equal(t, "email", carried.UserIDClaim)
+	assert.Equal(t, []sshauth.UserIDHash{userHash}, carried.AuthorizedUsers)
+	assert.Equal(t, map[string][]uint32{Wildcard: {0}}, carried.MachineUsers)
+	require.Len(t, carried.SessionPubKeys, 1)
+	assert.Equal(t, pub, carried.SessionPubKeys[0].PubKey)
+	assert.Equal(t, userHash, carried.SessionPubKeys[0].UserIDHash)
+	assert.Equal(t, "Alice", carried.SessionPubKeys[0].DisplayName)
+
+	rebuilt := NewAuthorizer()
+	rebuilt.Update(carried)
+	gotHash, _, err := rebuilt.AuthorizeSessionKey(pub, "alice")
+	require.NoError(t, err)
+	assert.Equal(t, userHash, gotHash)
+	assert.Equal(t, "Alice", rebuilt.LookupSessionDisplayName(pub))
+}
+
+// The copies Config hands out must not alias the authorizer's own state, or a
+// caller mutating what it read changes the policy in force.
+func TestAuthorizer_Config_CopiesAreIndependent(t *testing.T) {
+	pub := bytesRepeat(0x78, sessionPubKeyLen)
+	userHash, err := sshauth.HashUserID("alice")
+	require.NoError(t, err)
+
+	a := NewAuthorizer()
+	a.Update(&Config{
+		AuthorizedUsers: []sshauth.UserIDHash{userHash},
+		MachineUsers:    map[string][]uint32{Wildcard: {0}},
+		SessionPubKeys:  []SessionPubKey{{PubKey: pub, UserIDHash: userHash}},
+	})
+
+	carried := a.Config()
+	carried.MachineUsers[Wildcard][0] = 42
+	carried.SessionPubKeys[0].PubKey[0] ^= 0xFF
+
+	_, _, err = a.AuthorizeSessionKey(pub, "alice")
+	require.NoError(t, err, "mutating the returned config must not affect the authorizer")
+}