sendnrw/netbird
2
0
Fork 0
mirror of https://github.com/netbirdio/netbird.git synced 2026-09-16 03:39:07 +02:00
Code Issues Packages Projects Releases Wiki Activity

Close view-only input, approval-responder and inbound-block gaps in the VNC path

Browse Source
This commit is contained in:
Viktor Liu
2026-08-27 21:07:03 +02:00
parent e426401941
commit ab472f572d
5 changed files with 148 additions and 2 deletions
Download Patch File Download Diff File Expand all files Collapse all files
client/server/server.go
+19 -1
View File
@@ -25,6 +25,7 @@ import (
"github.com/netbirdio/netbird/client/internal/expose"
"github.com/prometheus/client_golang/prometheus"
"github.com/netbirdio/netbird/client/internal/ipcauth"
"github.com/netbirdio/netbird/client/internal/localmetrics"
"github.com/netbirdio/netbird/client/internal/profilemanager"
sleephandler "github.com/netbirdio/netbird/client/internal/sleep/handler"
@@ -2080,10 +2081,27 @@ func (s *Server) ExposeService(req *proto.ExposeServiceRequest, srv proto.Daemon
// approval prompt to the engine's broker. Unknown or already-resolved
// request_ids are silently no-op'd so a slow UI cannot deny a prompt the
// user already handled (or that already timed out).
func (s *Server) RespondApproval(_ context.Context, msg *proto.RespondApprovalRequest) (*proto.RespondApprovalResponse, error) {
//
// The answer is the whole of the consent the prompt asks for, so a caller the
// daemon cannot name is refused: on a control channel that carries no identity
// (loopback TCP) anything that can open a socket could accept a session on the
// console user's behalf. An identified local caller is accepted and logged.
// Restricting it further, to the user who owns the console, needs an
// owner-gated IPC channel that does not exist yet, and on a multi-seat Linux
// host there is no single such user to check against.
func (s *Server) RespondApproval(ctx context.Context, msg *proto.RespondApprovalRequest) (*proto.RespondApprovalResponse, error) {
if msg.GetRequestId() == "" {
return nil, gstatus.Errorf(codes.InvalidArgument, "request_id is required")
}
id, ok := ipcauth.CallerIdentity(ctx)
if !ok {
log.Warnf("refusing approval response for %s: the caller's identity cannot be verified on this control channel", msg.GetRequestId())
return nil, gstatus.Errorf(codes.PermissionDenied,
"answering a connection approval requires a control channel that carries the caller's identity. "+
"Reinstall the service on a socket that does: %s", reinstallCommand())
}
log.Infof("approval response for %s from caller %s: accept=%t view_only=%t",
msg.GetRequestId(), id, msg.GetAccept(), msg.GetViewOnly())
s.mutex.Lock()
connectClient := s.connectClient
s.mutex.Unlock()
client/server/ssh_gate.go
+36
View File
@@ -35,6 +35,10 @@ import (
// 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.
// - Unblocking inbound connections while a remote-access server is enabled
// starts that server: the engine keeps SSH and VNC down for as long as
// inbound traffic is blocked, so clearing the block is another way to hand
// out a shell or a desktop.
// - Binding the local metrics endpoint to a non-loopback address publishes
// peer names and connectivity state to the network without authentication.
//
@@ -53,6 +57,7 @@ type privilegedConfigChange struct {
disableSSHAuth *bool
serverVNCAllowed *bool
disableVNCApproval *bool
blockInbound *bool
enableLocalMetrics *bool
localMetricsAddress *string
}
@@ -65,6 +70,7 @@ func privilegedChangeFromSetConfig(msg *proto.SetConfigRequest) privilegedConfig
disableSSHAuth: msg.DisableSSHAuth,
serverVNCAllowed: msg.ServerVNCAllowed,
disableVNCApproval: msg.DisableVNCApproval,
blockInbound: msg.BlockInbound,
enableLocalMetrics: msg.EnableLocalMetrics,
localMetricsAddress: msg.LocalMetricsAddress,
}
@@ -78,6 +84,7 @@ func privilegedChangeFromLogin(msg *proto.LoginRequest) privilegedConfigChange {
disableSSHAuth: msg.DisableSSHAuth,
serverVNCAllowed: msg.ServerVNCAllowed,
disableVNCApproval: msg.DisableVNCApproval,
blockInbound: msg.BlockInbound,
enableLocalMetrics: msg.EnableLocalMetrics,
localMetricsAddress: msg.LocalMetricsAddress,
}
@@ -127,6 +134,15 @@ func requirePrivilegeForConfigChange(ctx context.Context, stored *profilemanager
ipcauth.UpCommand("-m "+change.managementURL))
}
// The engine keeps a remote-access server down while inbound connections
// are blocked, so lifting the block on a profile that has one enabled
// starts it. That is the same privilege as enabling it in the first place.
if disables(storedBlockInbound(stored), change.blockInbound) {
return denyPrivileged(ctx,
fmt.Sprintf("unblocking inbound connections while the NetBird %s server is enabled", server),
ipcauth.UpCommand("--block-inbound=false"))
}
return nil
}
@@ -243,6 +259,26 @@ func enables(stored, requested *bool) bool {
return stored == nil || !*stored
}
// disables reports whether requested turns a flag off that is currently on. The
// mirror of enables, for a flag whose privileged direction is being cleared.
func disables(stored, requested *bool) bool {
if requested == nil || *requested {
return false
}
return stored != nil && *stored
}
// storedBlockInbound reads the inbound-block flag from the stored config. It
// defaults to off, which is the engine's own default, so a config written
// before the flag existed is not read as blocking.
func storedBlockInbound(cfg *profilemanager.Config) *bool {
if cfg == nil {
return nil
}
blocked := cfg.BlockInbound
return &blocked
}
// storedFlag reads a flag from the stored config, tolerating a config that does
// not exist yet.
func storedFlag(cfg *profilemanager.Config, get func(*profilemanager.Config) *bool) *bool {
client/server/ssh_gate_test.go
+70
View File
@@ -444,3 +444,73 @@ func TestRequirePrivilegeForDeregistration(t *testing.T) {
// directly have no transport credentials, and the privileged-change gate refuses
// a caller it cannot identify.
func privilegedTestCtx() context.Context { return rootCtx() }
// The engine keeps SSH and VNC down while inbound connections are blocked, so
// clearing that block on a profile with one enabled starts the server. An
// unprivileged caller must not be able to do it by that route either.
func TestRequirePrivilegeForConfigChange_UnblockInbound(t *testing.T) {
tests := []struct {
name string
stored *profilemanager.Config
change privilegedConfigChange
privileged bool
wantDeny bool
}{
{
name: "clearing the block with the SSH server enabled",
stored: &profilemanager.Config{ServerSSHAllowed: boolPtr(true), BlockInbound: true},
change: privilegedConfigChange{blockInbound: boolPtr(false)},
wantDeny: true,
},
{
name: "clearing the block with only the VNC server enabled",
stored: &profilemanager.Config{
ServerSSHAllowed: boolPtr(false),
ServerVNCAllowed: boolPtr(true),
BlockInbound: true,
},
change: privilegedConfigChange{blockInbound: boolPtr(false)},
wantDeny: true,
},
{
name: "an administrator may clear it",
stored: &profilemanager.Config{ServerSSHAllowed: boolPtr(true), BlockInbound: true},
change: privilegedConfigChange{blockInbound: boolPtr(false)},
privileged: true,
},
{
name: "restating the stored block is not a change",
stored: &profilemanager.Config{ServerSSHAllowed: boolPtr(true), BlockInbound: true},
change: privilegedConfigChange{blockInbound: boolPtr(true)},
},
{
name: "turning the block on is never privileged",
stored: &profilemanager.Config{ServerSSHAllowed: boolPtr(true), BlockInbound: false},
change: privilegedConfigChange{blockInbound: boolPtr(true)},
},
{
name: "with no remote-access server enabled it is the user's own business",
stored: &profilemanager.Config{
ServerSSHAllowed: boolPtr(false),
ServerVNCAllowed: boolPtr(false),
BlockInbound: true,
},
change: privilegedConfigChange{blockInbound: boolPtr(false)},
},
}
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)
})
}
}