SVI/netbird
1
0
Fork 0
mirror of https://github.com/netbirdio/netbird.git synced 2026-07-28 00:42:37 -04:00
Code Issues 3.0k Packages Projects Releases 10 Wiki Activity

Fix up comments and improve readability

Browse Source
This commit is contained in:
Theodor S. Midtlien
2026-07-25 20:51:29 +02:00
parent 11642aeb44
commit bbf326e22c
6 changed files with 105 additions and 35 deletions
Download Patch File Download Diff File Expand all files Collapse all files

9
client/cmd/status.go
View File

@@ -174,12 +174,9 @@ func getStatus(ctx context.Context, fullPeerStatus bool, shouldRunProbes bool) (
return resp, nil
}
// getActiveProfileName asks the daemon for the active profile's display name.
// The daemon runs as root and can read the per-user profile files to resolve the
// ID to its human-readable name. When the active profile belongs to another local
// user, the name is annotated with that owner so the caller understands why the
// daemon is not under their control. Returns an empty string on any error so
// status output degrades gracefully.
// getActiveProfileName asks the daemon for the active profile's display name,
// annotated with the owning user when the active profile belongs to someone else.
// Returns an empty string on any error so status output degrades gracefully.
func getActiveProfileName(ctx context.Context) string {
conn, err := DialClientGRPCServer(ctx, daemonAddr)
if err != nil {

7
client/internal/ipcauth/interceptor.go
View File

@@ -46,6 +46,13 @@ func (i *Interceptor) StreamServerInterceptor() grpc.StreamServerInterceptor {
}
}
// authorize decides each RPC from the caller's identity, first match wins:
//
// 0. no identity DENY
// 1. self / privileged / forwarded ALLOW (root, elevated, daemon-self, JSON gateway)
// 2. in ownersAuthorizedMethods owner tier (gate on DaemonOwnership)
// 3. in handlerAuthorizedMethods profile tier (bypass, handler decides)
// 4. everything else default tier (gate on ActiveProfileOwnership)
func (i *Interceptor) authorize(ctx context.Context, fullMethod string) error {
id, ok := IdentityFromContext(ctx)
if !ok {

21
client/internal/ipcauth/interceptor_test.go
View File

@@ -56,13 +56,15 @@ func ctxWith(id Identity) context.Context {
}
const (
up = servicePath + "Up"
list = servicePath + "ListProfiles"
unkwn = servicePath + "SomeFutureMethod"
down = servicePath + "Down"
statusm = servicePath + "Status"
addp = servicePath + "AddProfile"
switchp = servicePath + "SwitchProfile"
up = servicePath + "Up"
list = servicePath + "ListProfiles"
unkwn = servicePath + "SomeFutureMethod"
down = servicePath + "Down"
statusm = servicePath + "Status"
addp = servicePath + "AddProfile"
switchp = servicePath + "SwitchProfile"
addowner = servicePath + "AddOwner"
sharep = servicePath + "ShareProfile"
)
func TestInterceptorAuthorize(t *testing.T) {
@@ -107,6 +109,11 @@ func TestInterceptorAuthorize(t *testing.T) {
{"add by daemon owner allowed", Ownership{}, Ownership{Owners: []string{"uid:1000"}}, nil, ctxWith(Identity{UID: 1000}), addp, false},
{"add by non-owner denied", Ownership{}, Ownership{Owners: []string{"uid:1000"}}, nil, ctxWith(Identity{UID: 2000}), addp, true},
{"owner-tier TOFU claims unowned daemon", Ownership{}, Ownership{}, nil, ctxWith(Identity{UID: 2000}), down, false},
// Owner-set mutations gate on daemon ownership, not the active profile.
{"add-owner by daemon owner allowed", Ownership{Owners: []string{"uid:9"}}, Ownership{Owners: []string{"uid:1000"}}, nil, ctxWith(Identity{UID: 1000}), addowner, false},
{"add-owner by active-profile owner (non daemon owner) denied", Ownership{Owners: []string{"uid:2000"}}, Ownership{Owners: []string{"uid:1000"}}, nil, ctxWith(Identity{UID: 2000}), addowner, true},
{"share by active-profile owner (non daemon owner) denied", Ownership{Owners: []string{"uid:2000"}}, Ownership{Owners: []string{"uid:1000"}}, nil, ctxWith(Identity{UID: 2000}), sharep, true},
}
for _, tt := range tests {

29
client/internal/ipcauth/policy.go
View File

@@ -26,22 +26,22 @@ type ProfilePolicy interface {
ClaimDaemonOwnerIfUnowned(id Identity) (bool, error)
}
// ownersAuthorizedMethods require a daemon-wide owner (or root). They are
// daemon-level operations independent of any single profile: creating profiles,
// tearing down the connection, and reading daemon status.
// ownersAuthorizedMethods gate on the daemon-wide owner set (or root): daemon-level
// ops independent of any profile. The owner-set mutations (AddOwner, ShareProfile,
// ResetOwner) must gate here, not on the active profile, else a per-profile owner
// could escalate via `owner add`. ResetOwner also requires root in its handler.
var ownersAuthorizedMethods = map[string]bool{
servicePath + "AddProfile": true,
servicePath + "Down": true,
servicePath + "Status": true,
servicePath + "AddProfile": true,
servicePath + "Down": true,
servicePath + "Status": true,
servicePath + "AddOwner": true,
servicePath + "ShareProfile": true,
servicePath + "ResetOwner": true,
}
// handlerAuthorizedMethods bypass the active-profile gate. Peer identity is still
// required to reach them. They either self-authorize in the handler against the
// profile they target (SwitchProfile, RemoveProfile, RenameProfile via
// authorizeTargetProfile) or against the caller's own profiles (ListProfiles via
// bindCallerUsername), or return only non-sensitive metadata that any local user
// may read (GetActiveProfile: the active profile's id, name, and owning username,
// so the CLI can show which profile, and whose, holds the daemon).
// handlerAuthorizedMethods bypass the ownership gate (identity still required)
// and let the handler authorize. GetActiveProfile bypasses only to return
// public metadata any local user may read.
var handlerAuthorizedMethods = map[string]bool{
servicePath + "ListProfiles": true,
servicePath + "RemoveProfile": true,
@@ -71,6 +71,9 @@ var auditMethods = map[string]bool{
servicePath + "Logout": true,
servicePath + "CleanState": true,
servicePath + "DeleteState": true,
servicePath + "AddOwner": true,
servicePath + "ResetOwner": true,
servicePath + "ShareProfile": true,
}
// ConfigAdapter is a ProfilePolicy whose backend is set lazily, once the daemon

33
client/server/ownership.go
View File

@@ -305,10 +305,23 @@ func (s *Server) authorizeTargetProfile(ctx context.Context, target *profilemana
return nil
}
// AddOwner adds a principal to the daemon-wide owner set. The interceptor has
// already confirmed the caller is an owner or privileged, the handler just
// validates and persists.
func (s *Server) AddOwner(_ context.Context, msg *proto.AddOwnerRequest) (*proto.AddOwnerResponse, error) {
// requireDaemonOwnerLocked fails closed unless the caller is a daemon owner or
// privileged. Defense in depth for the owner-set mutations, which the interceptor
// owner tier already gates. Caller must hold s.mutex.
func (s *Server) requireDaemonOwnerLocked(ctx context.Context) error {
id, ok := ipcauth.IdentityFromContext(ctx)
if !ok {
return gstatus.Error(codes.PermissionDenied, "caller identity could not be verified")
}
if id.IsPrivileged() || ipcauth.Authorize(s.owners, id, s.groupResolver) {
return nil
}
return gstatus.Error(codes.PermissionDenied, "not authorized: managing daemon owners requires a daemon owner or root/administrator")
}
// AddOwner adds a principal to the daemon-wide owner set. The owner tier gates
// this, the handler re-checks (defense in depth), validates, and persists.
func (s *Server) AddOwner(ctx context.Context, msg *proto.AddOwnerRequest) (*proto.AddOwnerResponse, error) {
principal := msg.GetPrincipal()
if _, ok := ipcauth.ParsePrincipal(principal); !ok {
return nil, gstatus.Errorf(codes.InvalidArgument, "invalid owner principal %q (expected uid:/gid:/group:/sid:)", principal)
@@ -317,6 +330,9 @@ func (s *Server) AddOwner(_ context.Context, msg *proto.AddOwnerRequest) (*proto
s.mutex.Lock()
defer s.mutex.Unlock()
if err := s.requireDaemonOwnerLocked(ctx); err != nil {
return nil, err
}
if s.daemonOwnerStore == nil {
return nil, gstatus.Error(codes.Unavailable, "daemon owner store unavailable")
}
@@ -356,12 +372,15 @@ func (s *Server) ResetOwner(ctx context.Context, _ *proto.ResetOwnerRequest) (*p
}
// ShareProfile marks the daemon shared or unshared. When shared, any authenticated
// local caller may control the daemon and its default profile. The interceptor has
// already confirmed the caller is an owner or privileged.
func (s *Server) ShareProfile(_ context.Context, msg *proto.ShareProfileRequest) (*proto.ShareProfileResponse, error) {
// local caller may control the daemon and its default profile. The owner tier
// gates this, the handler re-checks (defense in depth).
func (s *Server) ShareProfile(ctx context.Context, msg *proto.ShareProfileRequest) (*proto.ShareProfileResponse, error) {
s.mutex.Lock()
defer s.mutex.Unlock()
if err := s.requireDaemonOwnerLocked(ctx); err != nil {
return nil, err
}
if s.daemonOwnerStore == nil {
return nil, gstatus.Error(codes.Unavailable, "daemon owner store unavailable")
}

41
client/server/ownership_test.go
View File

@@ -71,8 +71,9 @@ func TestDaemonOwnerPolicyDefaultProfile(t *testing.T) {
require.NoError(t, err)
assert.False(t, claimed)
// AddOwner appends a daemon-wide principal (persisted).
_, err = s.AddOwner(context.Background(), &proto.AddOwnerRequest{Principal: "uid:1001"})
// AddOwner appends a daemon-wide principal (persisted). The caller must be a
// daemon owner: uid:1000 claimed ownership above.
_, err = s.AddOwner(ctxWithIdentity(ipcauth.Identity{UID: 1000}), &proto.AddOwnerRequest{Principal: "uid:1001"})
require.NoError(t, err)
assert.Equal(t, []string{"uid:1000", "uid:1001"}, store.owners)
@@ -83,6 +84,42 @@ func TestDaemonOwnerPolicyDefaultProfile(t *testing.T) {
assert.False(t, store.shared)
}
// TestOwnerMutationsRequireDaemonOwner checks the handler defense-in-depth: a
// caller who is neither a daemon owner nor privileged is denied at the handler.
func TestOwnerMutationsRequireDaemonOwner(t *testing.T) {
store := &fakeOwnerStore{owners: []string{"uid:1000"}}
s := &Server{groupResolver: ipcauth.NewDefaultGroupResolver()}
s.SetDaemonOwnerStore(store) // loads {uid:1000} into s.owners
owner := ctxWithIdentity(ipcauth.Identity{UID: 1000})
nonOwner := ctxWithIdentity(ipcauth.Identity{UID: 2000})
root := ctxWithIdentity(ipcauth.Identity{UID: 0})
t.Run("AddOwner denied for non-daemon-owner", func(t *testing.T) {
_, err := s.AddOwner(nonOwner, &proto.AddOwnerRequest{Principal: "uid:2000"})
assert.Equal(t, codes.PermissionDenied, gstatus.Code(err))
assert.Equal(t, []string{"uid:1000"}, store.owners, "owner set must be unchanged")
})
t.Run("ShareProfile denied for non-daemon-owner", func(t *testing.T) {
_, err := s.ShareProfile(nonOwner, &proto.ShareProfileRequest{Shared: true})
assert.Equal(t, codes.PermissionDenied, gstatus.Code(err))
assert.False(t, store.shared, "shared flag must be unchanged")
})
t.Run("AddOwner allowed for daemon owner", func(t *testing.T) {
_, err := s.AddOwner(owner, &proto.AddOwnerRequest{Principal: "uid:2000"})
require.NoError(t, err)
assert.Equal(t, []string{"uid:1000", "uid:2000"}, store.owners)
})
t.Run("ShareProfile allowed for root", func(t *testing.T) {
_, err := s.ShareProfile(root, &proto.ShareProfileRequest{Shared: true})
require.NoError(t, err)
assert.True(t, store.shared)
})
}
// TestDaemonOwnerAllOwnersUseDefault verifies every daemon owner is authorized
// for the default profile, while a non-owner is denied.
func TestDaemonOwnerAllOwnersUseDefault(t *testing.T) {