Require privilege for VNC server and approval config changes

2026-09-01 20:41:28 +02:00 · 2026-07-30 10:43:26 +02:00
parent 2b051ad250
commit abe8bc1b53
9 changed files with 401 additions and 118 deletions
--- a/client/server/ssh_gate.go
+++ b/client/server/ssh_gate.go
@@ -26,40 +26,51 @@ import (
 //     daemon's SSH server into a root (or unauthenticated) shell.
 //   - Enabling the SSH server at all is what makes the above reachable, and a
 //     profile the caller owns is not a privilege they hold.
-//   - While the SSH server is enabled, repointing the profile at another
-//     management identity hands SSH authorization decisions, including which
-//     keys and users are accepted, to whoever controls that identity. Changing
-//     the management URL and deregistering the peer are both ways to do that.
+//   - Enabling the VNC server exposes the console session, which on a
+//     multi-user host belongs to another user, and disabling its approval
+//     prompt removes that user's only say in it.
+//   - While a remote-access server (SSH or VNC) is enabled, repointing the
+//     profile at another management identity hands authorization decisions,
+//     including which keys and users are accepted, to whoever controls that
+//     identity. Changing the management URL and deregistering the peer are both
+//     ways to do that.
 //
 // Everything else stays unauthenticated, so this is not an authorization model:
-// it only refuses the changes that would let a local user become root. A caller
-// whose identity cannot be established is refused as well.
+// it only refuses the changes that would let a local user become root, or reach
+// another user's desktop. A caller whose identity cannot be established is
+// refused as well.

 // privilegedConfigChange is the subset of a config request that crosses the
 // user-to-root boundary. Fields are nil or empty when the request leaves them
 // untouched.
 type privilegedConfigChange struct {
-	managementURL    string
-	serverSSHAllowed *bool
-	enableSSHRoot    *bool
-	disableSSHAuth   *bool
+	managementURL      string
+	serverSSHAllowed   *bool
+	enableSSHRoot      *bool
+	disableSSHAuth     *bool
+	serverVNCAllowed   *bool
+	disableVNCApproval *bool
 }

 func privilegedChangeFromSetConfig(msg *proto.SetConfigRequest) privilegedConfigChange {
 	return privilegedConfigChange{
-		managementURL:    msg.GetManagementUrl(),
-		serverSSHAllowed: msg.ServerSSHAllowed,
-		enableSSHRoot:    msg.EnableSSHRoot,
-		disableSSHAuth:   msg.DisableSSHAuth,
+		managementURL:      msg.GetManagementUrl(),
+		serverSSHAllowed:   msg.ServerSSHAllowed,
+		enableSSHRoot:      msg.EnableSSHRoot,
+		disableSSHAuth:     msg.DisableSSHAuth,
+		serverVNCAllowed:   msg.ServerVNCAllowed,
+		disableVNCApproval: msg.DisableVNCApproval,
 	}
 }

 func privilegedChangeFromLogin(msg *proto.LoginRequest) privilegedConfigChange {
 	return privilegedConfigChange{
-		managementURL:    msg.GetManagementUrl(),
-		serverSSHAllowed: msg.ServerSSHAllowed,
-		enableSSHRoot:    msg.EnableSSHRoot,
-		disableSSHAuth:   msg.DisableSSHAuth,
+		managementURL:      msg.GetManagementUrl(),
+		serverSSHAllowed:   msg.ServerSSHAllowed,
+		enableSSHRoot:      msg.EnableSSHRoot,
+		disableSSHAuth:     msg.DisableSSHAuth,
+		serverVNCAllowed:   msg.ServerVNCAllowed,
+		disableVNCApproval: msg.DisableVNCApproval,
 	}
 }

@@ -83,15 +94,21 @@ func requirePrivilegeForConfigChange(ctx context.Context, stored *profilemanager
 		return denyPrivileged(ctx, "enabling the NetBird SSH server", ipcauth.UpCommand("--allow-server-ssh"))
 	}

-	// Only guard the management binding while the SSH server is enabled: that is
-	// when the management identity decides who may open a shell here.
-	if !sshServerEnabled(stored) {
+	if err := requirePrivilegeForVNCChange(ctx, stored, change); err != nil {
+		return err
+	}
+
+	// Only guard the management binding while a remote-access server is enabled:
+	// that is when the management identity decides who may open a shell or reach
+	// the desktop here.
+	server, enabled := enabledRemoteAccessServer(stored)
+	if !enabled {
 		return nil
 	}

 	if change.managementURL != "" && !sameManagementURL(stored.ManagementURL, change.managementURL) {
 		return denyPrivileged(ctx,
-			"changing the management URL while the NetBird SSH server is enabled",
+			fmt.Sprintf("changing the management URL while the NetBird %s server is enabled", server),
 			ipcauth.UpCommand("-m "+change.managementURL))
 	}

@@ -99,20 +116,21 @@ func requirePrivilegeForConfigChange(ctx context.Context, stored *profilemanager
 }

 // requirePrivilegeForDeregistration refuses to deregister the peer from the
-// management server when the caller is not privileged and the profile has the
-// SSH server enabled. Deregistering frees the peer's key to be registered
-// against another management identity, which is the same handover the
+// management server when the caller is not privileged and the profile has a
+// remote-access server enabled. Deregistering frees the peer's key to be
+// registered against another management identity, which is the same handover the
 // management URL check refuses.
 //
 // Callers that treat deregistration as best-effort (profile removal) continue
 // without it; callers that were asked to deregister surface the error.
 func requirePrivilegeForDeregistration(ctx context.Context, cfg *profilemanager.Config) error {
-	if !sshServerEnabled(cfg) {
+	server, enabled := enabledRemoteAccessServer(cfg)
+	if !enabled {
 		return nil
 	}

 	return denyPrivileged(ctx,
-		"deregistering this peer while the NetBird SSH server is enabled",
+		fmt.Sprintf("deregistering this peer while the NetBird %s server is enabled", server),
 		ipcauth.ElevatedCommand("netbird logout"))
 }

--- a/client/server/vnc_gate.go
+++ b/client/server/vnc_gate.go
@@ -0,0 +1,58 @@
+package server
+
+import (
+	"context"
+
+	"github.com/netbirdio/netbird/client/internal/ipcauth"
+	"github.com/netbirdio/netbird/client/internal/profilemanager"
+)
+
+// The VNC half of the privilege gate described in ssh_gate.go. The daemon runs
+// as root/LocalSystem and the VNC server it hosts attaches to the console
+// session, so both of these cross the user-to-root boundary:
+//
+//   - Enabling the VNC server publishes the console desktop, keyboard and
+//     mouse to whoever the account authorizes. On a multi-user host that
+//     desktop belongs to another user, and the profile the caller owns is not a
+//     privilege they hold over it.
+//   - Disabling the approval prompt removes the console user's per-connection
+//     consent, turning an authorized VNC session into a silent one.
+func requirePrivilegeForVNCChange(ctx context.Context, stored *profilemanager.Config, change privilegedConfigChange) error {
+	if enables(storedFlag(stored, func(c *profilemanager.Config) *bool { return c.DisableVNCApproval }), change.disableVNCApproval) {
+		return denyPrivileged(ctx, "disabling VNC connection approval", ipcauth.UpCommand("--disable-vnc-approval"))
+	}
+
+	if enables(storedFlag(stored, func(c *profilemanager.Config) *bool { return c.ServerVNCAllowed }), change.serverVNCAllowed) {
+		return denyPrivileged(ctx, "enabling the NetBird VNC server", ipcauth.UpCommand("--allow-server-vnc"))
+	}
+
+	return nil
+}
+
+// vncServerEnabled reports whether the profile currently runs the VNC server.
+//
+// Unlike the SSH server, VNC defaults to off: the flag was introduced with the
+// server itself, so a nil value is a config written before VNC existed and
+// means the server is not running.
+func vncServerEnabled(cfg *profilemanager.Config) bool {
+	if cfg == nil || cfg.ServerVNCAllowed == nil {
+		return false
+	}
+	return *cfg.ServerVNCAllowed
+}
+
+// enabledRemoteAccessServer names a remote-access server the profile currently
+// runs, and whether it runs any. It decides whether the management-binding and
+// deregistration guards apply, since either server hands authorization
+// decisions to the management identity the profile points at. SSH is reported
+// first when both are on: it is the more privileged of the two.
+func enabledRemoteAccessServer(cfg *profilemanager.Config) (string, bool) {
+	switch {
+	case sshServerEnabled(cfg):
+		return "SSH", true
+	case vncServerEnabled(cfg):
+		return "VNC", true
+	default:
+		return "", false
+	}
+}
--- a/client/server/vnc_gate_test.go
+++ b/client/server/vnc_gate_test.go
@@ -0,0 +1,178 @@
+package server
+
+import (
+	"testing"
+
+	"github.com/netbirdio/netbird/client/internal/profilemanager"
+)
+
+func TestRequirePrivilegeForConfigChange_VNCFlags(t *testing.T) {
+	tests := []struct {
+		name       string
+		stored     *profilemanager.Config
+		change     privilegedConfigChange
+		privileged bool
+		wantDeny   bool
+	}{
+		{
+			name:     "enabling the vnc server unprivileged is refused",
+			stored:   &profilemanager.Config{ServerVNCAllowed: boolPtr(false)},
+			change:   privilegedConfigChange{serverVNCAllowed: boolPtr(true)},
+			wantDeny: true,
+		},
+		{
+			name:       "enabling the vnc server as root is allowed",
+			stored:     &profilemanager.Config{ServerVNCAllowed: boolPtr(false)},
+			change:     privilegedConfigChange{serverVNCAllowed: boolPtr(true)},
+			privileged: true,
+		},
+		{
+			name:   "restating an already enabled vnc server is not a change",
+			stored: &profilemanager.Config{ServerVNCAllowed: boolPtr(true)},
+			change: privilegedConfigChange{serverVNCAllowed: boolPtr(true)},
+		},
+		{
+			name:   "turning the vnc server off is not guarded",
+			stored: &profilemanager.Config{ServerVNCAllowed: boolPtr(true)},
+			change: privilegedConfigChange{serverVNCAllowed: boolPtr(false)},
+		},
+		{
+			// Unlike SSH, a nil flag means off: the flag shipped with the server.
+			name:     "a config written before vnc existed counts as off, so enabling is refused",
+			stored:   &profilemanager.Config{},
+			change:   privilegedConfigChange{serverVNCAllowed: boolPtr(true)},
+			wantDeny: true,
+		},
+		{
+			name:     "a profile with no config yet counts as off, so enabling is refused",
+			stored:   nil,
+			change:   privilegedConfigChange{serverVNCAllowed: boolPtr(true)},
+			wantDeny: true,
+		},
+		{
+			name:     "disabling the vnc approval prompt unprivileged is refused",
+			stored:   &profilemanager.Config{DisableVNCApproval: boolPtr(false)},
+			change:   privilegedConfigChange{disableVNCApproval: boolPtr(true)},
+			wantDeny: true,
+		},
+		{
+			name:       "disabling the vnc approval prompt as root is allowed",
+			stored:     &profilemanager.Config{DisableVNCApproval: boolPtr(false)},
+			change:     privilegedConfigChange{disableVNCApproval: boolPtr(true)},
+			privileged: true,
+		},
+		{
+			name:   "re-enabling the vnc approval prompt is not guarded",
+			stored: &profilemanager.Config{DisableVNCApproval: boolPtr(true)},
+			change: privilegedConfigChange{disableVNCApproval: boolPtr(false)},
+		},
+		{
+			name:   "restating a disabled approval prompt is not a change",
+			stored: &profilemanager.Config{DisableVNCApproval: boolPtr(true)},
+			change: privilegedConfigChange{disableVNCApproval: boolPtr(true)},
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			ctx := userCtx()
+			if tt.privileged {
+				ctx = rootCtx()
+			}
+			err := requirePrivilegeForConfigChange(ctx, tt.stored, tt.change)
+			if tt.wantDeny {
+				assertDenied(t, err)
+				return
+			}
+			assertAllowed(t, err)
+		})
+	}
+}
+
+// The management binding and deregistration guards protect either remote-access
+// server, so the VNC server alone must arm them even with SSH off.
+func TestRequirePrivilegeForConfigChange_ManagementURLWithVNCOnly(t *testing.T) {
+	vncOnly := &profilemanager.Config{
+		ServerSSHAllowed:   boolPtr(false),
+		ServerVNCAllowed:   boolPtr(true),
+		ManagementURL:      mustURL(t, "https://api.netbird.io:443"),
+		DisableVNCApproval: boolPtr(false),
+	}
+
+	err := requirePrivilegeForConfigChange(userCtx(), vncOnly,
+		privilegedConfigChange{managementURL: "https://attacker.example.com:443"})
+	assertDenied(t, err)
+
+	// The same move is the administrator's to make.
+	assertAllowed(t, requirePrivilegeForConfigChange(rootCtx(), vncOnly,
+		privilegedConfigChange{managementURL: "https://selfhosted.example.com:443"}))
+
+	// Restating the stored binding is not a change, so it is never refused.
+	assertAllowed(t, requirePrivilegeForConfigChange(userCtx(), vncOnly,
+		privilegedConfigChange{managementURL: "https://api.netbird.io"}))
+}
+
+func TestRequirePrivilegeForDeregistration_VNCOnly(t *testing.T) {
+	vncOnly := &profilemanager.Config{ServerSSHAllowed: boolPtr(false), ServerVNCAllowed: boolPtr(true)}
+
+	assertDenied(t, requirePrivilegeForDeregistration(userCtx(), vncOnly))
+	assertAllowed(t, requirePrivilegeForDeregistration(rootCtx(), vncOnly))
+
+	bothOff := &profilemanager.Config{ServerSSHAllowed: boolPtr(false), ServerVNCAllowed: boolPtr(false)}
+	assertAllowed(t, requirePrivilegeForDeregistration(userCtx(), bothOff))
+}
+
+// enabledRemoteAccessServer names the server in the refusal, so the user is told
+// which one is holding the binding down. SSH wins when both are on: it is the
+// more privileged of the two.
+func TestEnabledRemoteAccessServer(t *testing.T) {
+	tests := []struct {
+		name       string
+		cfg        *profilemanager.Config
+		wantServer string
+		wantOn     bool
+	}{
+		{
+			name:       "ssh only",
+			cfg:        &profilemanager.Config{ServerSSHAllowed: boolPtr(true), ServerVNCAllowed: boolPtr(false)},
+			wantServer: "SSH",
+			wantOn:     true,
+		},
+		{
+			name:       "vnc only",
+			cfg:        &profilemanager.Config{ServerSSHAllowed: boolPtr(false), ServerVNCAllowed: boolPtr(true)},
+			wantServer: "VNC",
+			wantOn:     true,
+		},
+		{
+			name:       "both on reports ssh",
+			cfg:        &profilemanager.Config{ServerSSHAllowed: boolPtr(true), ServerVNCAllowed: boolPtr(true)},
+			wantServer: "SSH",
+			wantOn:     true,
+		},
+		{
+			name: "both off",
+			cfg:  &profilemanager.Config{ServerSSHAllowed: boolPtr(false), ServerVNCAllowed: boolPtr(false)},
+		},
+		{
+			// A nil SSH flag means on (legacy configs), so it still arms the guard.
+			name:       "legacy config with no flags at all reports ssh",
+			cfg:        &profilemanager.Config{},
+			wantServer: "SSH",
+			wantOn:     true,
+		},
+		{
+			name: "no config",
+			cfg:  nil,
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			server, on := enabledRemoteAccessServer(tt.cfg)
+			if on != tt.wantOn || server != tt.wantServer {
+				t.Fatalf("enabledRemoteAccessServer() = (%q, %v), want (%q, %v)", server, on, tt.wantServer, tt.wantOn)
+			}
+		})
+	}
+}