diff --git a/client/cmd/status.go b/client/cmd/status.go index bdadef2d7..4032c865c 100644 --- a/client/cmd/status.go +++ b/client/cmd/status.go @@ -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 { diff --git a/client/internal/ipcauth/interceptor.go b/client/internal/ipcauth/interceptor.go index de4caa911..e9ec5759d 100644 --- a/client/internal/ipcauth/interceptor.go +++ b/client/internal/ipcauth/interceptor.go @@ -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 { diff --git a/client/internal/ipcauth/interceptor_test.go b/client/internal/ipcauth/interceptor_test.go index 8f7fb8b4b..2391df3a4 100644 --- a/client/internal/ipcauth/interceptor_test.go +++ b/client/internal/ipcauth/interceptor_test.go @@ -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 { diff --git a/client/internal/ipcauth/policy.go b/client/internal/ipcauth/policy.go index a5bf67b16..3474544b2 100644 --- a/client/internal/ipcauth/policy.go +++ b/client/internal/ipcauth/policy.go @@ -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 diff --git a/client/server/ownership.go b/client/server/ownership.go index 1eee4fe2a..371f6faeb 100644 --- a/client/server/ownership.go +++ b/client/server/ownership.go @@ -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") } diff --git a/client/server/ownership_test.go b/client/server/ownership_test.go index 6f4e735c4..5ce1f32d0 100644 --- a/client/server/ownership_test.go +++ b/client/server/ownership_test.go @@ -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) {