diff --git a/client/internal/engine_vnc.go b/client/internal/engine_vnc.go index e03c3f471..c87d5e25d 100644 --- a/client/internal/engine_vnc.go +++ b/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, diff --git a/client/vnc/server/noise_auth_test.go b/client/vnc/server/noise_auth_test.go index 7eb862360..263cfb929 100644 --- a/client/vnc/server/noise_auth_test.go +++ b/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) +} diff --git a/client/vnc/server/server.go b/client/vnc/server/server.go index 81467e76f..a71b7e82d 100644 --- a/client/vnc/server/server.go +++ b/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 diff --git a/shared/sessionauth/auth.go b/shared/sessionauth/auth.go index b9d5cf2cb..2fa61b18d 100644 --- a/shared/sessionauth/auth.go +++ b/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, } } diff --git a/shared/sessionauth/auth_test.go b/shared/sessionauth/auth_test.go index a19985a31..2c134280a 100644 --- a/shared/sessionauth/auth_test.go +++ b/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") +}