From 6dcb1374e9f3a8f7527a161fa649582bdb3f6b0e Mon Sep 17 00:00:00 2001 From: "Theodor S. Midtlien" Date: Fri, 18 Sep 2026 11:19:35 +0200 Subject: [PATCH] Move profile resolution to the authz gate --- client/internal/ipcauth/authz_gate.go | 59 +++--- client/internal/ipcauth/authz_gate_test.go | 76 +++++--- client/internal/ipcauth/authz_level.go | 37 ++-- .../internal/ipcauth/privilege_denial_test.go | 13 +- client/internal/ipcauth/target.go | 26 +++ client/internal/profilemanager/prefs_test.go | 2 +- client/internal/profilemanager/service.go | 121 +++++++----- .../internal/profilemanager/service_test.go | 154 +++++++-------- client/mobile/profile_manager.go | 2 +- client/server/claim_profile_test.go | 23 +-- client/server/login_gate_test.go | 12 +- client/server/logout_gate_test.go | 24 ++- client/server/server.go | 180 +++++++++++------- client/server/server_jwt_test.go | 5 +- ...e_test.go => server_resolvetarget_test.go} | 98 ++++++++-- client/server/setconfig_mdm_test.go | 6 + client/server/setconfig_test.go | 4 + client/server/ssh_gate_test.go | 8 + 18 files changed, 518 insertions(+), 332 deletions(-) create mode 100644 client/internal/ipcauth/target.go rename client/server/{server_ownsprofile_test.go => server_resolvetarget_test.go} (54%) diff --git a/client/internal/ipcauth/authz_gate.go b/client/internal/ipcauth/authz_gate.go index 40f8325e8..b94f6081e 100644 --- a/client/internal/ipcauth/authz_gate.go +++ b/client/internal/ipcauth/authz_gate.go @@ -18,11 +18,12 @@ type DaemonState interface { // whether one is held. SessionHolder() (Principal, bool) - // OwnsProfile reports whether id owns the profile a request names. An empty - // handle is the active profile, which is what a method that acts on the - // live session resolves against. The error says what was wrong with the - // handle itself. - OwnsProfile(id Identity, handle string) (bool, error) + // ResolveTarget resolves the profile a request names to a concrete profile + // and reports whether the caller may address it. An empty handle is the + // active profile. + // + // The error says what was wrong with the handle itself. + ResolveTarget(id Identity, handle string) (Target, error) } // AuthzGate authorizes every RPC call before its handler run. @@ -118,8 +119,9 @@ func denyPolicyLevel(r Request, p MethodPolicy) error { // target-scoped. func (g *AuthzGate) StreamPolicyInterceptor() grpc.StreamServerInterceptor { return func(srv any, ss grpc.ServerStream, info *grpc.StreamServerInfo, handler grpc.StreamHandler) error { - authErr := g.authorize(ss.Context(), info.FullMethod, nil) - if authErr != nil { + // A stream's context cannot be replaced from here. We use context to pass + // resolved target profile and no streaming method may be target-scoped. + if _, authErr := g.authorize(ss.Context(), info.FullMethod, nil); authErr != nil { return authErr } return handler(srv, ss) @@ -129,7 +131,7 @@ func (g *AuthzGate) StreamPolicyInterceptor() grpc.StreamServerInterceptor { // UnaryPolicyInterceptor authorizes each unary RPC before the handler runs. func (g *AuthzGate) UnaryPolicyInterceptor() grpc.UnaryServerInterceptor { return func(ctx context.Context, req any, info *grpc.UnaryServerInfo, handler grpc.UnaryHandler) (resp any, err error) { - authErr := g.authorize(ctx, info.FullMethod, req) + ctx, authErr := g.authorize(ctx, info.FullMethod, req) if authErr != nil { return nil, authErr } @@ -137,55 +139,68 @@ func (g *AuthzGate) UnaryPolicyInterceptor() grpc.UnaryServerInterceptor { } } -func (g *AuthzGate) authorize(ctx context.Context, method string, msg any) error { +func (g *AuthzGate) authorize(ctx context.Context, method string, msg any) (context.Context, error) { id, ok := CallerIdentity(ctx) if !ok { log.Warnf("ipc authz: DENY %s, caller identity unavailable", method) - return status.Error(codes.PermissionDenied, + return ctx, status.Error(codes.PermissionDenied, "caller identity could not be verified on the daemon control channel") } st := g.state() if st == nil { log.Warnf("ipc authz: DENY %s for %s, daemon state not attached", method, id) - return status.Error(codes.Unavailable, "daemon not initialized") + return ctx, status.Error(codes.Unavailable, "daemon not initialized") } policy := methodPolicyFor(method) - // Only a target-scoped method reads a profile off the request. - var target string + // Only a target-scoped method reads a profile off the request. Everything + // else acts on the active profile, which an empty handle resolves to. + var handle string if policy.TargetsProfile { named, ok := targetProfile(msg) if !ok { - return status.Errorf(codes.Internal, "%s is declared target-scoped but names no profile", method) + return ctx, status.Errorf(codes.Internal, "%s is declared target-scoped but names no profile", method) } - target = named + handle = named } - level, resolveErr := resolveLevel(id, target, st) + target, handleErr := st.ResolveTarget(id, handle) + + level := resolveLevel(id, target, st) + + if handleErr != nil && handle != "" { + level = AuthzLevelIdentified + } req := Request{ Identity: id, Level: level, - Target: target, + Target: handle, Method: method, State: st, Msg: msg, } if req.Level < policy.Level { log.Warnf("ipc authz: DENY %s for %s (%s), requires %s", method, id, req.Level, policy.Level) - if resolveErr != nil { - return resolveErr + if presentable := presentableHandleError(handle, handleErr); presentable != nil { + return ctx, presentable } - return denyPolicyLevel(req, policy) + return ctx, denyPolicyLevel(req, policy) } for _, rule := range policy.Rules { if err := rule(req); err != nil { log.Warnf("ipc authz: DENY %s for %s (%s): error", method, id, req.Level) - return err + return ctx, err } } if policy.Audit { log.Infof("ipc authz: allow %s for %s (%s)", method, id, req.Level) } - return nil + if !target.Owned { + // Reaching here means the method was open to the caller's level + // without owning anything, so there is no authorized profile to hand + // the handler. + return ctx, nil + } + return ContextWithTarget(ctx, target.Path), nil } diff --git a/client/internal/ipcauth/authz_gate_test.go b/client/internal/ipcauth/authz_gate_test.go index 600a15c86..5ff9d06ba 100644 --- a/client/internal/ipcauth/authz_gate_test.go +++ b/client/internal/ipcauth/authz_gate_test.go @@ -35,9 +35,9 @@ func switchTo(handle string) *proto.SwitchProfileRequest { // exists and belongs to somebody, which a mistyped handle does not. func TestAuthorizeSurfacesWhatIsWrongWithTheHandle(t *testing.T) { notFound := gstatus.Errorf(codes.NotFound, "profile %q not found", "asdfasdfasdf") - g := gateFor(t, stubState{ownsErr: notFound}) + g := gateFor(t, stubState{targetErr: notFound}) - err := g.authorize(transportCtx(unprivUser, nil), servicePath+"SwitchProfile", switchTo("asdfasdfasdf")) + _, err := g.authorize(transportCtx(unprivUser, nil), servicePath+"SwitchProfile", switchTo("asdfasdfasdf")) require.Error(t, err) st := gstatus.Convert(err) @@ -52,9 +52,9 @@ func TestAuthorizeSurfacesWhatIsWrongWithTheHandle(t *testing.T) { // error, and the CLI reformats it into a hint. It has to reach the CLI. func TestAuthorizeSurfacesAnAmbiguousHandle(t *testing.T) { ambiguous := gstatus.Errorf(codes.InvalidArgument, "handle %q matches 2 profiles", "ab") - g := gateFor(t, stubState{ownsErr: ambiguous}) + g := gateFor(t, stubState{targetErr: ambiguous}) - err := g.authorize(transportCtx(unprivUser, nil), servicePath+"SwitchProfile", switchTo("ab")) + _, err := g.authorize(transportCtx(unprivUser, nil), servicePath+"SwitchProfile", switchTo("ab")) require.Error(t, err) assert.Equal(t, codes.InvalidArgument, gstatus.Convert(err).Code()) } @@ -64,9 +64,9 @@ func TestAuthorizeSurfacesAnAmbiguousHandle(t *testing.T) { // the refusal stays about who the profile belongs to. func TestAuthorizeBlamesOwnershipForTheActiveProfile(t *testing.T) { notFound := gstatus.Errorf(codes.NotFound, "profile %q not found", "active-profile-id") - g := gateFor(t, stubState{ownsErr: notFound}) + g := gateFor(t, stubState{targetErr: notFound}) - err := g.authorize(transportCtx(unprivUser, nil), servicePath+"SwitchProfile", switchTo("")) + _, err := g.authorize(transportCtx(unprivUser, nil), servicePath+"SwitchProfile", switchTo("")) require.Error(t, err) denial, ok := DenialFrom(err) @@ -78,9 +78,9 @@ func TestAuthorizeBlamesOwnershipForTheActiveProfile(t *testing.T) { // A daemon-side failure is not something the caller can correct, and putting it // on the wire would describe the daemon rather than the request. func TestAuthorizeKeepsADaemonFailureOffTheWire(t *testing.T) { - g := gateFor(t, stubState{ownsErr: errors.New("read profile directory: permission denied")}) + g := gateFor(t, stubState{targetErr: errors.New("read profile directory: permission denied")}) - err := g.authorize(transportCtx(unprivUser, nil), servicePath+"SwitchProfile", switchTo("some-profile")) + _, err := g.authorize(transportCtx(unprivUser, nil), servicePath+"SwitchProfile", switchTo("some-profile")) require.Error(t, err) denial, ok := DenialFrom(err) @@ -93,14 +93,15 @@ func TestAuthorizeKeepsADaemonFailureOffTheWire(t *testing.T) { // identified caller may make. A failure there must not take those down. func TestAuthorizeAllowsIdentifiedMethodsDespiteAResolveFailure(t *testing.T) { notFound := gstatus.Errorf(codes.NotFound, "profile %q not found", "active-profile-id") - g := gateFor(t, stubState{ownsErr: notFound}) + g := gateFor(t, stubState{targetErr: notFound}) for _, method := range []string{"ListProfiles", "AddProfile", "GetActiveProfile", "GetFeatures"} { t.Run(method, func(t *testing.T) { require.Equal(t, AuthzLevelIdentified, methodPolicies[servicePath+method].Level, "fixture is wrong: %s is no longer open to any identified caller", method) - assert.NoError(t, g.authorize(transportCtx(unprivUser, nil), servicePath+method, nil)) + _, err := g.authorize(transportCtx(unprivUser, nil), servicePath+method, nil) + assert.NoError(t, err) }) } } @@ -108,32 +109,61 @@ func TestAuthorizeAllowsIdentifiedMethodsDespiteAResolveFailure(t *testing.T) { // Ownership is the gate's answer, never the error's: a resolution that failed is // a no whatever it returned alongside. func TestAuthorizeRefusesWhenResolutionFails(t *testing.T) { - g := gateFor(t, stubState{owns: false, ownsErr: gstatus.Error(codes.NotFound, "profile not found")}) + g := gateFor(t, stubState{targetErr: gstatus.Error(codes.NotFound, "profile not found")}) - err := g.authorize(transportCtx(unprivUser, nil), servicePath+"SwitchProfile", switchTo("some-profile")) + _, err := g.authorize(transportCtx(unprivUser, nil), servicePath+"SwitchProfile", switchTo("some-profile")) assert.Error(t, err, "an error from the resolution cannot be read as ownership") } -// A resolution that failed established nothing about the profile, so no level -// returned alongside the error may be acted on. This is the invariant the gate -// clamps, pinned at the function that has to hold it. +// A resolution that failed established nothing about the profile, so ownership +// reported alongside the error may not be acted on. A state that answers both +// at once is exactly what this refuses to trust. func TestResolveLevelNeverRaisesTheLevelOnAFailure(t *testing.T) { - asDaemon(t, root) - notFound := gstatus.Error(codes.NotFound, "profile not found") + owned := Target{Path: "/profiles/some-profile.json", Owned: true} for _, tc := range []struct { name string st stubState }{ - {"a live session it reports as owned", stubState{owns: true, running: true, ownsErr: notFound}}, - {"an idle daemon it reports as owned", stubState{owns: true, ownsErr: notFound}}, - {"a daemon-side failure it reports as owned", stubState{owns: true, ownsErr: errors.New("read profile directory")}}, + {"a live session it reports as owned", stubState{target: owned, running: true, targetErr: notFound}}, + {"an idle daemon it reports as owned", stubState{target: owned, targetErr: notFound}}, + {"a daemon-side failure it reports as owned", stubState{target: owned, targetErr: errors.New("read profile directory")}}, } { t.Run(tc.name, func(t *testing.T) { - level, _ := resolveLevel(unprivUser, "some-profile", tc.st) - assert.Equal(t, AuthzLevelIdentified, level, - "a failed resolution cannot confer %s", level) + g := gateFor(t, tc.st) + + _, err := g.authorize(transportCtx(unprivUser, nil), servicePath+"SwitchProfile", switchTo("some-profile")) + assert.Error(t, err, "a failed resolution conferred a level it had no business conferring") }) } } + +// The profile the gate resolved is what the handler acts on, so it has to reach +// the handler. Resolving the handle a second time downstream is what this +// exists to make unnecessary. +func TestAuthorizeCarriesTheResolvedTargetToTheHandler(t *testing.T) { + g := gateFor(t, stubState{target: Target{Path: "/profiles/abcd1111.json", Owned: true}}) + + ctx, err := g.authorize(transportCtx(unprivUser, nil), servicePath+"SwitchProfile", switchTo("work")) + require.NoError(t, err) + + got, ok := TargetFromContext(ctx) + require.True(t, ok, "the handler has no profile to act on") + assert.Equal(t, "/profiles/abcd1111.json", got, + "the handler would act on a different profile than the one authorized") +} + +// A privileged caller skips the ownership question but still needs the profile +// their handle named, or every target-scoped RPC breaks under sudo. +func TestAuthorizeCarriesTheTargetForAPrivilegedCaller(t *testing.T) { + g := gateFor(t, stubState{target: Target{Path: "/profiles/abcd1111.json", Owned: true}}) + + ctx, err := g.authorize(transportCtx(root, nil), servicePath+"ClaimProfile", + &proto.ClaimProfileRequest{Handle: "work"}) + require.NoError(t, err) + + got, ok := TargetFromContext(ctx) + require.True(t, ok, "root resolved nothing to act on") + assert.Equal(t, "/profiles/abcd1111.json", got) +} diff --git a/client/internal/ipcauth/authz_level.go b/client/internal/ipcauth/authz_level.go index 9f6744984..53711276e 100644 --- a/client/internal/ipcauth/authz_level.go +++ b/client/internal/ipcauth/authz_level.go @@ -47,41 +47,42 @@ func (l AuthzLevel) String() string { } // resolveLevel is the authority the caller holds over the profile the request -// names. The second return is what was wrong with the handle, when that is -// worth showing the caller instead of a refusal. It never raises the level: a -// resolution that failed still denies. -func resolveLevel(id Identity, target string, st DaemonState) (AuthzLevel, error) { +// resolved to. A profile the caller does not own confers nothing beyond being +// identified, which is also what an unresolved handle leaves them with. +func resolveLevel(id Identity, target Target, st DaemonState) AuthzLevel { if !id.Known() { - return AuthzLevelNone, nil + return AuthzLevelNone } if IsPrivilegedCaller(id) { - return AuthzLevelPrivileged, nil + return AuthzLevelPrivileged } - ownsProfile, err := st.OwnsProfile(id, target) - if err != nil { - return AuthzLevelIdentified, presentableHandleError(target, err) - } - if !ownsProfile { - return AuthzLevelIdentified, nil + if !target.Owned { + return AuthzLevelIdentified } if holder, running := st.SessionHolder(); !running || holder.Matches(id) { - return AuthzLevelSessionHolder, nil + return AuthzLevelSessionHolder } - return AuthzLevelProfileOwner, nil + return AuthzLevelProfileOwner } // presentableHandleError keeps a resolution failure only when the gate can put // it in front of the caller in place of its own refusal. Everything else is // dropped, and the caller gets the refusal their level earned. -func presentableHandleError(target string, err error) error { - // An empty target is the active profile rather than something the caller +func presentableHandleError(handle string, err error) error { + if err == nil { + return nil + } + + // An empty handle is the active profile rather than something the caller // typed, so a failure to resolve it is not theirs to correct. - if target == "" { + if handle == "" { return nil } // Only a gRPC status reaches the caller as a sentence the CLI and the UI - // render. + // render. A plain error is a daemon-side failure, and putting it on the + // wire would tell the caller about the daemon rather than about the handle + // they gave. if _, ok := gstatus.FromError(err); !ok { return nil } diff --git a/client/internal/ipcauth/privilege_denial_test.go b/client/internal/ipcauth/privilege_denial_test.go index ee2fc716f..ec62842aa 100644 --- a/client/internal/ipcauth/privilege_denial_test.go +++ b/client/internal/ipcauth/privilege_denial_test.go @@ -102,14 +102,15 @@ func TestDenyPolicyLevelWithoutGuidanceStaysBare(t *testing.T) { // stubState stands in for the daemon so a denial can be built without a server. type stubState struct { - holder Principal - running bool - owns bool - ownsErr error + holder Principal + running bool + target Target + targetErr error } -func (s stubState) SessionHolder() (Principal, bool) { return s.holder, s.running } -func (s stubState) OwnsProfile(Identity, string) (bool, error) { return s.owns, s.ownsErr } +func (s stubState) SessionHolder() (Principal, bool) { return s.holder, s.running } + +func (s stubState) ResolveTarget(Identity, string) (Target, error) { return s.target, s.targetErr } // A refusal caused by somebody else's connection explains itself and offers no // command, since the caller cannot end a session that is not theirs. diff --git a/client/internal/ipcauth/target.go b/client/internal/ipcauth/target.go new file mode 100644 index 000000000..5c0075a4a --- /dev/null +++ b/client/internal/ipcauth/target.go @@ -0,0 +1,26 @@ +package ipcauth + +import "context" + +// Target is the profile a request resolved to. +type Target struct { + Path string + Owned bool +} + +type targetKey struct{} + +// ContextWithTarget carries the profile the gate resolved, so a handler acts on +// the profile that was authorized rather than resolving the caller's handle a +// second time. +func ContextWithTarget(ctx context.Context, path string) context.Context { + return context.WithValue(ctx, targetKey{}, path) +} + +// TargetFromContext returns the file of the profile the gate resolved for this +// request. It reports false when nothing resolved, which a handler acting on a +// named profile must treat as a refusal rather than as the active profile. +func TargetFromContext(ctx context.Context) (string, bool) { + path, ok := ctx.Value(targetKey{}).(string) + return path, ok && path != "" +} diff --git a/client/internal/profilemanager/prefs_test.go b/client/internal/profilemanager/prefs_test.go index 6d1b6022c..d3807f684 100644 --- a/client/internal/profilemanager/prefs_test.go +++ b/client/internal/profilemanager/prefs_test.go @@ -151,7 +151,7 @@ func TestRemoveProfile_DeletesPrefsFile(t *testing.T) { _, err = os.Stat(prefsPath) require.NoError(t, err) - require.NoError(t, sm.RemoveProfile(created.ID, userID)) + require.NoError(t, sm.RemoveProfile(created.ID)) _, err = os.Stat(prefsPath) assert.True(t, errors.Is(err, os.ErrNotExist), "prefs file should be removed") }) diff --git a/client/internal/profilemanager/service.go b/client/internal/profilemanager/service.go index 726de8236..545b281f3 100644 --- a/client/internal/profilemanager/service.go +++ b/client/internal/profilemanager/service.go @@ -49,6 +49,13 @@ type ErrAmbiguousHandle struct { Kind AmbiguityKind } +// HandleMatch is the set of profiles a handle matched and which matcher found +// them. Kind only carries meaning when more than one profile matched. +type HandleMatch struct { + Profiles []Profile + Kind AmbiguityKind +} + // AmbiguityKind describes which matcher produced the ambiguity, so callers // can tailor the error message. type AmbiguityKind int @@ -436,20 +443,9 @@ func (s *ServiceManager) RenameProfile(id ID, newName string) error { return fmt.Errorf("invalid profile ID: %q", id) } - profiles, err := s.loadAllProfiles() + target, err := s.ProfileByID(id) if err != nil { - return fmt.Errorf("load profiles: %w", err) - } - - var target *Profile - for i := range profiles { - if profiles[i].ID == id { - target = &profiles[i] - break - } - } - if target == nil { - return ErrProfileNotFound + return err } return writeProfileName(target.Path, displayName) @@ -470,20 +466,9 @@ func (s *ServiceManager) RemoveProfile(id ID) error { return fmt.Errorf("invalid profile ID: %q", id) } - profiles, err := s.loadAllProfiles() + target, err := s.ProfileByID(id) if err != nil { - return fmt.Errorf("load profiles: %w", err) - } - - var target *Profile - for i := range profiles { - if profiles[i].ID == id { - target = &profiles[i] - break - } - } - if target == nil { - return ErrProfileNotFound + return err } activeProf, err := s.GetActiveProfileState() @@ -676,7 +661,7 @@ func (s *ServiceManager) ClaimDefaultProfileIfNeeded(id ipcauth.Identity) { profiles, err := s.loadAllProfiles() if err != nil { - log.Warnf("could not load all profiles: %w", err) + log.Warnf("could not load all profiles: %v", err) return } @@ -1061,25 +1046,31 @@ func (s *ServiceManager) activeProfileID() (ID, bool) { return state.ID, false } -// ResolveProfile turns a user-supplied handle into a Profile. Resolution -// precedence is: exact ID match, then unique exact name, then unique ID -// prefix. Ambiguous matches return *ErrAmbiguousHandle so callers can -// surface the candidates. -func (s *ServiceManager) ResolveProfile(handle string) (*Profile, error) { +// MatchProfiles returns every profile a user-supplied handle matches, at the +// highest precedence tier that matched at all: exact ID, then exact name, then +// ID prefix. It answers existence and nothing else, so choosing between several +// matches is left to the caller that knows who is asking. +func (s *ServiceManager) MatchProfiles(handle string) (HandleMatch, error) { if handle == "" { - return nil, fmt.Errorf("profile handle is empty") + return HandleMatch{}, fmt.Errorf("profile handle is empty") } profiles, err := s.loadAllProfiles() if err != nil { - return nil, err + return HandleMatch{}, err } + // A legacy ID is a display name two accounts can hold in their own profile + // directories, so even an exact ID can match more than one file. + var idMatches []Profile for i := range profiles { if profiles[i].ID == ID(handle) { - return &profiles[i], nil + idMatches = append(idMatches, profiles[i]) } } + if len(idMatches) > 0 { + return HandleMatch{Profiles: idMatches, Kind: AmbiguityKindName}, nil + } var nameMatches []Profile for i := range profiles { @@ -1087,15 +1078,8 @@ func (s *ServiceManager) ResolveProfile(handle string) (*Profile, error) { nameMatches = append(nameMatches, profiles[i]) } } - if len(nameMatches) == 1 { - return &nameMatches[0], nil - } - if len(nameMatches) > 1 { - return nil, &ErrAmbiguousHandle{ - Handle: handle, - Candidates: nameMatches, - Kind: AmbiguityKindName, - } + if len(nameMatches) > 0 { + return HandleMatch{Profiles: nameMatches, Kind: AmbiguityKindName}, nil } // ID prefix match. Skip the default profile so `select d` does not @@ -1109,14 +1093,49 @@ func (s *ServiceManager) ResolveProfile(handle string) (*Profile, error) { prefixMatches = append(prefixMatches, profiles[i]) } } - if len(prefixMatches) == 1 { - return &prefixMatches[0], nil + if len(prefixMatches) > 0 { + return HandleMatch{Profiles: prefixMatches, Kind: AmbiguityKindIDPrefix}, nil } - if len(prefixMatches) > 1 { - return nil, &ErrAmbiguousHandle{ - Handle: handle, - Candidates: prefixMatches, - Kind: AmbiguityKindIDPrefix, + + return HandleMatch{}, ErrProfileNotFound +} + +// ProfileByPath returns the profile stored at this path. +func (s *ServiceManager) ProfileByPath(path string) (*Profile, error) { + if path == "" { + return nil, fmt.Errorf("profile path is empty") + } + + profiles, err := s.loadAllProfiles() + if err != nil { + return nil, err + } + + for i := range profiles { + if profiles[i].Path == path { + return &profiles[i], nil + } + } + + return nil, ErrProfileNotFound +} + +// ProfileByID returns the first profile with this ID. Only a caller that has no +// second profile to confuse it with may use this: a legacy ID is a display name +// and two accounts can hold the same one. Prefer ProfileByPath. +func (s *ServiceManager) ProfileByID(id ID) (*Profile, error) { + if id == "" { + return nil, fmt.Errorf("profile ID is empty") + } + + profiles, err := s.loadAllProfiles() + if err != nil { + return nil, err + } + + for i := range profiles { + if profiles[i].ID == id { + return &profiles[i], nil } } diff --git a/client/internal/profilemanager/service_test.go b/client/internal/profilemanager/service_test.go index 09a829f69..3dca0e528 100644 --- a/client/internal/profilemanager/service_test.go +++ b/client/internal/profilemanager/service_test.go @@ -39,6 +39,31 @@ func withTestSM(t *testing.T, fn func(sm *ServiceManager, id ipcauth.Identity)) // The identity the helper hands out has to work as a profile owner on the // platform the suite is running on. +// matchOne is the single profile a handle matches, for the tests that are about +// the matcher's precedence rather than about who is asking. +func matchOne(t *testing.T, sm *ServiceManager, handle string) Profile { + t.Helper() + + match, err := sm.MatchProfiles(handle) + require.NoError(t, err) + require.Len(t, match.Profiles, 1, "handle %q did not match exactly one profile", handle) + return match.Profiles[0] +} + +// claimAndList is the sequence a request goes through now: the gate stamps +// whatever the caller can claim, and only then is the listing filtered by what +// they own. Listing on its own no longer claims. +func claimAndList(t *testing.T, sm *ServiceManager, id ipcauth.Identity) []Profile { + t.Helper() + + sm.ClaimDefaultProfileIfNeeded(id) + sm.ClaimLegacyProfiles(id) + + profiles, err := sm.ListProfiles(id) + require.NoError(t, err) + return profiles +} + func TestWithTestSM_ScopesToAUsableOwner(t *testing.T) { withTestSM(t, func(sm *ServiceManager, id ipcauth.Identity) { require.True(t, id.Known(), "every test in this package authorizes against this identity") @@ -46,7 +71,7 @@ func TestWithTestSM_ScopesToAUsableOwner(t *testing.T) { created, err := sm.AddProfile("owned", &id) require.NoError(t, err) - got, err := sm.ResolveProfile(created.ID.String(), id) + got, err := sm.ProfileByID(created.ID) require.NoError(t, err) require.Len(t, got.Owners, 1, "the profile records the identity it was created for") assert.True(t, got.Owners[0].Matches(id), @@ -59,8 +84,7 @@ func TestServiceProfile_ExactID(t *testing.T) { created, err := sm.AddProfile("work", nil) require.NoError(t, err) - got, err := sm.ResolveProfile(created.ID.String(), userID) - require.NoError(t, err) + got := matchOne(t, sm, created.ID.String()) assert.Equal(t, created.ID, got.ID) assert.Equal(t, "work", got.Name) }) @@ -72,8 +96,7 @@ func TestServiceProfile_IDPrefix(t *testing.T) { require.NoError(t, err) prefix := created.ID[:4] - got, err := sm.ResolveProfile(prefix.String(), userID) - require.NoError(t, err) + got := matchOne(t, sm, prefix.String()) assert.Equal(t, created.ID, got.ID) }) } @@ -91,11 +114,12 @@ func TestServiceProfile_AmbiguousPrefix(t *testing.T) { require.NoError(t, util.WriteJson(context.Background(), path, &Config{Name: id})) } - _, err = sm.ResolveProfile("abcd", userID) - var amb *ErrAmbiguousHandle - require.ErrorAs(t, err, &amb) - assert.Equal(t, AmbiguityKindIDPrefix, amb.Kind) - assert.Len(t, amb.Candidates, 2) + // Deciding between the two belongs to whoever knows who is asking, so + // the matcher hands both back rather than refusing. + match, err := sm.MatchProfiles("abcd") + require.NoError(t, err) + assert.Equal(t, AmbiguityKindIDPrefix, match.Kind) + assert.Len(t, match.Profiles, 2) }) } @@ -104,8 +128,7 @@ func TestServiceProfile_ExactNameUnique(t *testing.T) { _, err := sm.AddProfile("work", &userID) require.NoError(t, err) - got, err := sm.ResolveProfile("work", userID) - require.NoError(t, err) + got := matchOne(t, sm, "work") assert.Equal(t, "work", got.Name) }) } @@ -117,25 +140,23 @@ func TestServiceProfile_AmbiguousName(t *testing.T) { _, err = sm.AddProfile("work", &userID) require.NoError(t, err) - _, err = sm.ResolveProfile("work", userID) - var amb *ErrAmbiguousHandle - require.ErrorAs(t, err, &amb) - assert.Equal(t, AmbiguityKindName, amb.Kind) - assert.Len(t, amb.Candidates, 2) + match, err := sm.MatchProfiles("work") + require.NoError(t, err) + assert.Equal(t, AmbiguityKindName, match.Kind) + assert.Len(t, match.Profiles, 2) }) } func TestServiceProfile_NotFound(t *testing.T) { withTestSM(t, func(sm *ServiceManager, userID ipcauth.Identity) { - _, err := sm.ResolveProfile("nope", userID) + _, err := sm.MatchProfiles("nope") assert.ErrorIs(t, err, ErrProfileNotFound) }) } func TestServiceProfile_DefaultByExactID(t *testing.T) { withTestSM(t, func(sm *ServiceManager, userID ipcauth.Identity) { - got, err := sm.ResolveProfile(defaultProfileName, userID) - require.NoError(t, err) + got := matchOne(t, sm, defaultProfileName) assert.Equal(t, defaultProfileName, got.ID.String()) }) } @@ -151,8 +172,7 @@ func TestServiceProfile_LegacyFilenameCoexists(t *testing.T) { path := filepath.Join(configDir, "legacy.json") require.NoError(t, util.WriteJson(context.Background(), path, &Config{})) - got, err := sm.ResolveProfile("legacy", userID) - require.NoError(t, err) + got := matchOne(t, sm, "legacy") assert.Equal(t, "legacy", got.ID.String()) // Name falls back to the filename stem when JSON omits it. assert.Equal(t, "legacy", got.Name) @@ -187,7 +207,7 @@ func TestAddProfile_RejectsInvalidNames(t *testing.T) { func TestRemoveProfile_RejectsInvalidID(t *testing.T) { withTestSM(t, func(sm *ServiceManager, userID ipcauth.Identity) { - err := sm.RemoveProfile("../escape", userID) + err := sm.RemoveProfile("../escape") assert.Error(t, err) }) } @@ -253,7 +273,7 @@ func TestRemoveProfile_DeletesStateFile(t *testing.T) { statePath := filepath.Join(configDir, created.ID.String()+".state.json") require.NoError(t, os.WriteFile(statePath, []byte(`{"email":"a@b"}`), 0600)) - require.NoError(t, sm.RemoveProfile(created.ID, userID)) + require.NoError(t, sm.RemoveProfile(created.ID)) _, err = os.Stat(statePath) assert.True(t, errors.Is(err, os.ErrNotExist), "state file should be removed") }) @@ -318,16 +338,14 @@ func TestListProfiles_UnownedProfilesArePrivilegedOnly(t *testing.T) { require.NoError(t, err) alice := ipcauth.KnownForTest(ipcauth.Identity{UID: 4242}) - got, err := sm.ListProfiles(alice) - require.NoError(t, err) + got := claimAndList(t, sm, alice) assert.NotContains(t, profileIDs(got), defaultProfileName, "the default profile has no exemption, being claimed is what opens it") assert.NotContains(t, profileIDs(got), unowned.ID.String(), "every profile needs an owner before anyone can address it") root := ipcauth.KnownForTest(ipcauth.Identity{UID: 0}) - got, err = sm.ListProfiles(root) - require.NoError(t, err) + got = claimAndList(t, sm, root) assert.Contains(t, profileIDs(got), defaultProfileName, "root still reaches both, which is how an unowned profile gets assigned") assert.Contains(t, profileIDs(got), unowned.ID.String()) @@ -392,15 +410,13 @@ func TestListProfiles_UnownedLegacyProfileIsPrivilegedOnly(t *testing.T) { stubLegacyDir(t, "bob") bob := ipcauth.KnownForTest(ipcauth.Identity{UID: 4242}) - got, err := sm.ListProfiles(bob) - require.NoError(t, err) + got := claimAndList(t, sm, bob) assert.NotContains(t, profileIDs(got), "work", "a profile sitting in someone else's directory is not free to take") assert.Empty(t, readOwners(t, path), "and it is not claimed on the way past") root := ipcauth.KnownForTest(ipcauth.Identity{UID: 0}) - got, err = sm.ListProfiles(root) - require.NoError(t, err) + got = claimAndList(t, sm, root) assert.Contains(t, profileIDs(got), "work", "root still reaches it, which is how it gets reassigned") }) @@ -412,18 +428,16 @@ func TestListProfiles_ClaimsLegacyProfileForItsOwnAccount(t *testing.T) { stubLegacyDir(t, "alice") alice := ipcauth.KnownForTest(ipcauth.Identity{UID: 4242}) - got, err := sm.ListProfiles(alice) - require.NoError(t, err) + got := claimAndList(t, sm, alice) assert.Contains(t, profileIDs(got), "work", - "the claim lands before the listing is filtered, so the gap closes in one call") + "the profile in the caller's own directory is claimed for them and then listed") assert.Equal(t, []string{"uid:4242"}, readOwners(t, path)) // The claim is on disk now, so it is the owner check and not the // directory name that keeps the next caller out. stubLegacyDir(t, "alice") other := ipcauth.KnownForTest(ipcauth.Identity{UID: 5252}) - got, err = sm.ListProfiles(other) - require.NoError(t, err) + got = claimAndList(t, sm, other) assert.NotContains(t, profileIDs(got), "work") assert.Equal(t, []string{"uid:4242"}, readOwners(t, path), "a second caller does not overwrite a stamped owner") @@ -439,8 +453,7 @@ func TestClaimLegacyProfile_LeavesTheProfileWhereItIs(t *testing.T) { stubLegacyDir(t, "alice") alice := ipcauth.KnownForTest(ipcauth.Identity{UID: 4242}) - got, err := sm.ListProfiles(alice) - require.NoError(t, err) + got := claimAndList(t, sm, alice) claimed := ownedProfile(t, got, "uid:4242") assert.Equal(t, ID("work"), claimed.ID, "claiming does not re-key the profile") @@ -463,8 +476,7 @@ func TestClaimLegacyProfile_NamesakeInAnotherDirectoryIsUntouched(t *testing.T) stubLegacyDir(t, "alice") alice := ipcauth.KnownForTest(ipcauth.Identity{UID: 4242}) - got, err := sm.ListProfiles(alice) - require.NoError(t, err) + got := claimAndList(t, sm, alice) claimed := ownedProfile(t, got, "uid:4242") assert.Equal(t, filepath.Join(configDir, "alice", "work.json"), claimed.Path, @@ -496,8 +508,7 @@ func TestClaimLegacyProfile_SkipsOneAlreadyOwnedInTheSameDirectory(t *testing.T) stubLegacyDir(t, "alice") alice := ipcauth.KnownForTest(ipcauth.Identity{UID: 4242}) - _, err := sm.ListProfiles(alice) - require.NoError(t, err) + claimAndList(t, sm, alice) assert.Equal(t, []string{"uid:9999"}, readOwners(t, taken), "a profile that already has an owner is not restamped") @@ -510,9 +521,9 @@ func TestRenameProfile(t *testing.T) { created, err := sm.AddProfile("work", &userID) require.NoError(t, err) - require.NoError(t, sm.RenameProfile(created.ID, userID, "weekend")) + require.NoError(t, sm.RenameProfile(created.ID, "weekend")) - got, err := sm.ResolveProfile(created.ID.String(), userID) + got, err := sm.ProfileByID(created.ID) require.NoError(t, err) assert.Equal(t, "weekend", got.Name, "the new name is on disk") assert.Equal(t, created.ID, got.ID, "renaming does not re-key the profile") @@ -520,47 +531,16 @@ func TestRenameProfile(t *testing.T) { }) } -func TestRenameProfile_NotTheCallersProfile(t *testing.T) { - withTestSM(t, func(sm *ServiceManager, userID ipcauth.Identity) { - created, err := sm.AddProfile("work", &userID) - require.NoError(t, err) - - stranger := ipcauth.KnownForTest(ipcauth.Identity{UID: 4242}) - require.Error(t, sm.RenameProfile(created.ID, stranger, "weekend"), - "a profile the caller cannot address is not theirs to rename") - - got, err := sm.ResolveProfile(created.ID.String(), userID) - require.NoError(t, err) - assert.Equal(t, "work", got.Name) - }) -} - -func TestResolveProfile_ClaimsOnTheWayThrough(t *testing.T) { - withLegacyLayout(t, func(sm *ServiceManager, configDir string) { - path := writeLegacyProfile(t, configDir, "alice", "work", nil) - stubLegacyDir(t, "alice") - - // Resolution is what switching a profile goes through, so the claim has - // to land here and not only when something lists profiles. - alice := ipcauth.KnownForTest(ipcauth.Identity{UID: 4242}) - got, err := sm.ResolveProfile("work", alice) - require.NoError(t, err) - assert.Equal(t, path, got.Path) - assert.Equal(t, []string{"uid:4242"}, readOwners(t, path)) - }) -} - func TestListProfiles_PrivilegedCallerDoesNotClaim(t *testing.T) { withLegacyLayout(t, func(sm *ServiceManager, configDir string) { path := writeLegacyProfile(t, configDir, "root", "work", nil) stubLegacyDir(t, "root") root := ipcauth.KnownForTest(ipcauth.Identity{UID: 0}) - got, err := sm.ListProfiles(root) - require.NoError(t, err) + got := claimAndList(t, sm, root) assert.Contains(t, profileIDs(got), "work") assert.Empty(t, readOwners(t, path), - "root reaches every profile anyway, so a listing must not stamp one") + "root reaches every profile anyway, so the claim must not stamp one") }) } @@ -598,8 +578,7 @@ func TestClaimLegacyProfile_LeavesAnOwnerItCannotParse(t *testing.T) { stubLegacyDir(t, "alice") alice := ipcauth.KnownForTest(ipcauth.Identity{UID: 4242}) - got, err := sm.ListProfiles(alice) - require.NoError(t, err) + got := claimAndList(t, sm, alice) assert.NotContains(t, profileIDs(got), "work") assert.Equal(t, []string{"group:devs"}, readOwners(t, path), @@ -803,8 +782,7 @@ func TestListProfiles_ClaimKeepsFieldsThisVersionDoesNotModel(t *testing.T) { stubLegacyDir(t, "alice") alice := ipcauth.KnownForTest(ipcauth.Identity{UID: 4242}) - _, err := sm.ListProfiles(alice) - require.NoError(t, err) + claimAndList(t, sm, alice) assert.Equal(t, []string{"uid:4242"}, readOwners(t, path), "the claim still lands") data, err := os.ReadFile(path) @@ -834,8 +812,7 @@ func TestClaimDefaultProfile_ConsoleUserClaimsIt(t *testing.T) { stubConsoleUser(t, true) alice := ipcauth.KnownForTest(ipcauth.Identity{UID: 4242}) - _, err := sm.ListProfiles(alice) - require.NoError(t, err) + claimAndList(t, sm, alice) assert.Equal(t, []string{"uid:4242"}, readOwners(t, DefaultConfigPath), "the first caller at the console closes the window the default profile is open in") }) @@ -918,8 +895,7 @@ func TestClaimDefaultProfile_CallerAwayFromTheConsoleDoesNotClaimIt(t *testing.T stubConsoleUser(t, false) alice := ipcauth.KnownForTest(ipcauth.Identity{UID: 4242}) - _, err := sm.ListProfiles(alice) - require.NoError(t, err) + claimAndList(t, sm, alice) assert.Empty(t, readOwners(t, DefaultConfigPath), "a local caller who is not at the console must not take the machine's profile") }) @@ -931,8 +907,7 @@ func TestClaimDefaultProfile_DisableEnvWithholdsTheClaim(t *testing.T) { t.Setenv(EnvDisableDefaultProfileClaim, "true") alice := ipcauth.KnownForTest(ipcauth.Identity{UID: 4242}) - _, err := sm.ListProfiles(alice) - require.NoError(t, err) + claimAndList(t, sm, alice) assert.Empty(t, readOwners(t, DefaultConfigPath), "the flag withholds the claim even from a caller who would otherwise get it") }) @@ -944,8 +919,7 @@ func TestClaimDefaultProfile_UnparseableDisableEnvLeavesTheClaimOn(t *testing.T) t.Setenv(EnvDisableDefaultProfileClaim, "yes please") alice := ipcauth.KnownForTest(ipcauth.Identity{UID: 4242}) - _, err := sm.ListProfiles(alice) - require.NoError(t, err) + claimAndList(t, sm, alice) assert.Equal(t, []string{"uid:4242"}, readOwners(t, DefaultConfigPath), "a typo must not be what turns a safety mechanism off") }) diff --git a/client/mobile/profile_manager.go b/client/mobile/profile_manager.go index 04c5df644..d0384bce5 100644 --- a/client/mobile/profile_manager.go +++ b/client/mobile/profile_manager.go @@ -132,7 +132,7 @@ func (pm *ProfileManager) GetActiveProfile() (*Profile, error) { return nil, fmt.Errorf("get active profile: %w", err) } - prof, err := pm.serviceMgr.ResolveProfile(activeState.ID.String()) + prof, err := pm.serviceMgr.ProfileByID(activeState.ID) if err != nil { return nil, fmt.Errorf("resolve active profile %q: %w", activeState.ID, err) } diff --git a/client/server/claim_profile_test.go b/client/server/claim_profile_test.go index 0a3dfd095..15d369943 100644 --- a/client/server/claim_profile_test.go +++ b/client/server/claim_profile_test.go @@ -1,7 +1,6 @@ package server import ( - "context" "fmt" "os/user" "path/filepath" @@ -59,7 +58,7 @@ func TestClaimProfile_RecordsTheOwner(t *testing.T) { srv := claimTestServer(t) owner := claimOwner(4242) - resp, err := srv.ClaimProfile(rootCtx(), &proto.ClaimProfileRequest{ + resp, err := srv.ClaimProfile(withTarget(rootCtx(), profilemanager.DefaultConfigPath), &proto.ClaimProfileRequest{ Handle: "default", Owner: owner, }) @@ -119,10 +118,7 @@ func TestClaimProfile_RequiresBothArguments(t *testing.T) { func TestClaimProfile_RefusesAnUnknownProfile(t *testing.T) { srv := claimTestServer(t) - _, err := srv.ClaimProfile(rootCtx(), &proto.ClaimProfileRequest{ - Handle: "no-such-profile", - Owner: claimOwner(4242), - }) + _, err := srv.ResolveTarget(privilegedIdentity(), "no-such-profile") require.Error(t, err) assert.Equal(t, codes.NotFound, gstatus.Convert(err).Code()) } @@ -130,12 +126,11 @@ func TestClaimProfile_RefusesAnUnknownProfile(t *testing.T) { func TestClaimProfile_NeedsAnIdentifiedCaller(t *testing.T) { srv := claimTestServer(t) - _, err := srv.ClaimProfile(context.Background(), &proto.ClaimProfileRequest{ - Handle: "default", - Owner: claimOwner(4242), - }) - require.Error(t, err) - assert.Equal(t, codes.Unauthenticated, gstatus.Convert(err).Code()) + // A caller the kernel did not vouch for reaches no profile, so the gate has + // nothing to authorize and the handler is never entered. + target, err := srv.ResolveTarget(ipcauth.Identity{}, "default") + require.NoError(t, err) + assert.False(t, target.Owned, "an unattested caller must not be able to claim") } // A principal is taken as given. The account deliberately does not exist, which @@ -146,7 +141,7 @@ func TestClaimProfile_TakesAPrincipalWithoutResolvingIt(t *testing.T) { t.Run(owner, func(t *testing.T) { srv := claimTestServer(t) - resp, err := srv.ClaimProfile(rootCtx(), &proto.ClaimProfileRequest{ + resp, err := srv.ClaimProfile(withTarget(rootCtx(), profilemanager.DefaultConfigPath), &proto.ClaimProfileRequest{ Handle: "default", Owner: owner, }) @@ -164,7 +159,7 @@ func TestClaimProfile_ResolvesAnAccountName(t *testing.T) { want, ok := profilemanager.PrincipalForUser(u) require.True(t, ok) - resp, err := srv.ClaimProfile(rootCtx(), &proto.ClaimProfileRequest{ + resp, err := srv.ClaimProfile(withTarget(rootCtx(), profilemanager.DefaultConfigPath), &proto.ClaimProfileRequest{ Handle: "default", Owner: u.Username, }) diff --git a/client/server/login_gate_test.go b/client/server/login_gate_test.go index cff894ac7..05b2712a8 100644 --- a/client/server/login_gate_test.go +++ b/client/server/login_gate_test.go @@ -27,15 +27,16 @@ func TestLogin_RefusedChangeLeavesTheProfileAlone(t *testing.T) { // A second profile that runs the SSH server, which is what makes repointing // its management binding a privileged change. target := "ssh-enabled" + targetPath := filepath.Join(profilemanager.DefaultConfigPathDir, target+".json") _, err := profilemanager.UpdateOrCreateConfig(profilemanager.ConfigInput{ - ConfigPath: filepath.Join(profilemanager.DefaultConfigPathDir, target+".json"), + ConfigPath: targetPath, ManagementURL: "https://api.netbird.io:443", ServerSSHAllowed: boolPtr(true), Owner: testProfileOwner(), }) require.NoError(t, err) - _, err = s.Login(userCtx(), &proto.LoginRequest{ + _, err = s.Login(withTarget(userCtx(), targetPath), &proto.LoginRequest{ ProfileName: &target, Username: &username, ManagementUrl: "https://mgmt.attacker.example:443", @@ -82,7 +83,7 @@ func TestLogin_ChangeThatBecomesPrivilegedMidRequestHasNoSideEffects(t *testing. } t.Cleanup(func() { afterLoginPreCheck = nil }) - _, err = s.Login(userCtx(), &proto.LoginRequest{ + _, err = s.Login(withTarget(userCtx(), targetPath), &proto.LoginRequest{ ProfileName: &target, Username: &username, ManagementUrl: "https://mgmt.attacker.example:443", @@ -108,8 +109,9 @@ func TestLogin_RefusedChangeLeavesAnInProgressLoginAlone(t *testing.T) { s.rootCtx = internal.CtxInitState(context.Background()) target := "ssh-enabled" + targetPath := filepath.Join(profilemanager.DefaultConfigPathDir, target+".json") _, err := profilemanager.UpdateOrCreateConfig(profilemanager.ConfigInput{ - ConfigPath: filepath.Join(profilemanager.DefaultConfigPathDir, target+".json"), + ConfigPath: targetPath, ManagementURL: "https://api.netbird.io:443", ServerSSHAllowed: boolPtr(true), Owner: testProfileOwner(), @@ -119,7 +121,7 @@ func TestLogin_RefusedChangeLeavesAnInProgressLoginAlone(t *testing.T) { cancelled := false s.actCancel = func() { cancelled = true } - _, err = s.Login(userCtx(), &proto.LoginRequest{ + _, err = s.Login(withTarget(userCtx(), targetPath), &proto.LoginRequest{ ProfileName: &target, Username: &username, ManagementUrl: "https://mgmt.attacker.example:443", diff --git a/client/server/logout_gate_test.go b/client/server/logout_gate_test.go index 4c9da943e..7bad1b3b3 100644 --- a/client/server/logout_gate_test.go +++ b/client/server/logout_gate_test.go @@ -48,7 +48,7 @@ func TestLogout_ActiveProfileAllowedWhenProfilesDisabled(t *testing.T) { s.profilesDisabled = true - _, err := s.Logout(userCtx(), &proto.LogoutRequest{ + _, err := s.Logout(withTarget(userCtx(), cfgPath), &proto.LogoutRequest{ ProfileName: &activeProfile, Username: &username, }) @@ -67,8 +67,9 @@ func TestLogout_OtherProfileStaysGatedWhenProfilesDisabled(t *testing.T) { s.rootCtx = internal.CtxInitState(context.Background()) other := "other-profile" + otherPath := filepath.Join(profilemanager.DefaultConfigPathDir, other+".json") _, err := profilemanager.UpdateOrCreateConfig(profilemanager.ConfigInput{ - ConfigPath: filepath.Join(profilemanager.DefaultConfigPathDir, other+".json"), + ConfigPath: otherPath, ManagementURL: unreachableManagementURL, Owner: testProfileOwner(), }) @@ -76,7 +77,7 @@ func TestLogout_OtherProfileStaysGatedWhenProfilesDisabled(t *testing.T) { s.profilesDisabled = true - _, err = s.Logout(userCtx(), &proto.LogoutRequest{ + _, err = s.Logout(withTarget(userCtx(), otherPath), &proto.LogoutRequest{ ProfileName: &other, Username: &username, }) @@ -95,11 +96,11 @@ func TestLogout_ForeignUserProfileStaysGatedWhenProfilesDisabled(t *testing.T) { s.rootCtx = internal.CtxInitState(context.Background()) shared := "shared-legacy-name" - plantNamesakeProfiles(t, s, shared) + ownPath := plantNamesakeProfiles(t, s, shared) s.profilesDisabled = true - _, err := s.Logout(userCtx(), &proto.LogoutRequest{ + _, err := s.Logout(withTarget(userCtx(), ownPath), &proto.LogoutRequest{ ProfileName: &shared, Username: &username, }) @@ -114,7 +115,7 @@ func TestLogout_ForeignUserProfileStaysGatedWhenProfilesDisabled(t *testing.T) { // which is the one made active. Only the caller's copy carries an owner, so // that is the one a handle resolves to, while the active profile stays the // other file. -func plantNamesakeProfiles(t *testing.T, s *Server, id string) { +func plantNamesakeProfiles(t *testing.T, s *Server, id string) string { t.Helper() foreignDir := filepath.Join(profilemanager.DefaultConfigPathDir, "someone-else") @@ -125,8 +126,9 @@ func plantNamesakeProfiles(t *testing.T, s *Server, id string) { }) require.NoError(t, err) + ownPath := filepath.Join(profilemanager.DefaultConfigPathDir, id+".json") _, err = profilemanager.UpdateOrCreateConfig(profilemanager.ConfigInput{ - ConfigPath: filepath.Join(profilemanager.DefaultConfigPathDir, id+".json"), + ConfigPath: ownPath, ManagementURL: unreachableManagementURL, Owner: testProfileOwner(), }) @@ -136,6 +138,8 @@ func plantNamesakeProfiles(t *testing.T, s *Server, id string) { ID: profilemanager.ID(id), Username: "someone-else", })) + + return ownPath } // Deregistering a namesake profile must not go out with the running config. @@ -156,11 +160,11 @@ func TestLogout_ForeignUserProfileDoesNotUseTheRunningConfig(t *testing.T) { s.connectClient = newDummyConnectClient(context.Background()) shared := "shared-legacy-name" - plantNamesakeProfiles(t, s, shared) + ownPath := plantNamesakeProfiles(t, s, shared) // Bounded so the deregistration the fixed path attempts fails on the dial // rather than sitting in gRPC backoff for the whole test timeout. - ctx, cancel := context.WithTimeout(userCtx(), 2*time.Second) + ctx, cancel := context.WithTimeout(withTarget(userCtx(), ownPath), 2*time.Second) t.Cleanup(cancel) _, err = s.Logout(ctx, &proto.LogoutRequest{ @@ -206,7 +210,7 @@ func TestLogout_ActiveProfileAllowedWhenProfilesEnabled(t *testing.T) { s.rootCtx = internal.CtxInitState(context.Background()) enableSSHOnProfile(t, cfgPath) - _, err := s.Logout(userCtx(), &proto.LogoutRequest{ + _, err := s.Logout(withTarget(userCtx(), cfgPath), &proto.LogoutRequest{ ProfileName: &activeProfile, Username: &username, }) diff --git a/client/server/server.go b/client/server/server.go index 6b30db26d..4e34aab46 100644 --- a/client/server/server.go +++ b/client/server/server.go @@ -533,7 +533,12 @@ func (s *Server) SetConfig(callerCtx context.Context, msg *proto.SetConfigReques return nil, err } - stored, err := s.storedProfileConfig(msg.ProfileName) + resolved, err := s.targetProfile(callerCtx) + if err != nil { + return nil, err + } + + stored, err := s.storedProfileConfig(resolved) if err != nil { return nil, err } @@ -541,7 +546,7 @@ func (s *Server) SetConfig(callerCtx context.Context, msg *proto.SetConfigReques return nil, err } - config, err := s.setConfigInputFromRequest(msg) + config, err := s.setConfigInputFromRequest(msg, resolved) if err != nil { return nil, err } @@ -571,14 +576,9 @@ func (s *Server) SetConfig(callerCtx context.Context, msg *proto.SetConfigReques // field is its own optional case. Returns the resolved ConfigInput // and a non-nil error only when the active profile file path cannot // be determined. -func (s *Server) setConfigInputFromRequest(msg *proto.SetConfigRequest) (profilemanager.ConfigInput, error) { +func (s *Server) setConfigInputFromRequest(msg *proto.SetConfigRequest, resolved *profilemanager.Profile) (profilemanager.ConfigInput, error) { var config profilemanager.ConfigInput - resolved, err := s.resolveProfileHandle(msg.ProfileName) - if err != nil { - log.Errorf("failed to resolve profile %q: %v", msg.ProfileName, err) - return config, err - } profPath := resolved.Path if profPath == "" { profPath = profilemanager.DefaultConfigPath @@ -688,7 +688,7 @@ func (s *Server) Login(callerCtx context.Context, msg *proto.LoginRequest) (*pro // refused login neither switches the profile nor cancels a login already in // progress, and it reads the profile the request targets, which is the one the // switch below would activate. - stored, err := s.storedLoginConfig(activeProf, msg) + stored, err := s.storedLoginConfig(callerCtx, activeProf, msg) if err != nil { return nil, err } @@ -1090,7 +1090,12 @@ func (s *Server) Up(callerCtx context.Context, msg *proto.UpRequest) (*proto.UpR } if msg != nil && msg.ProfileName != nil { - if _, err := s.switchProfileIfNeeded(*msg.ProfileName, activeProf); err != nil { + resolved, err := s.targetProfile(callerCtx) + if err != nil { + s.mutex.Unlock() + return nil, err + } + if _, err := s.switchProfileIfNeeded(resolved, activeProf); err != nil { s.mutex.Unlock() log.Errorf("failed to switch profile: %v", err) return nil, err @@ -1156,12 +1161,7 @@ func (s *Server) waitForUp(callerCtx context.Context) (*proto.UpResponse, error) // targets, so a privileged-change decision can be made against the values the // profile currently holds. A profile that has no config file yet yields nil, // which every caller must read as "nothing enabled yet". -func (s *Server) storedProfileConfig(handle string) (*profilemanager.Config, error) { - resolved, err := s.resolveProfileHandle(handle) - if err != nil { - return nil, err - } - +func (s *Server) storedProfileConfig(resolved *profilemanager.Profile) (*profilemanager.Config, error) { path := resolved.Path if path == "" { path = profilemanager.DefaultConfigPath @@ -1173,7 +1173,7 @@ func (s *Server) storedProfileConfig(handle string) (*profilemanager.Config, err // storedLoginConfig loads the on-disk config of the profile a login request // targets: the one it names, or the active one when it names none. Used to decide // a privileged change before the request is allowed to switch profiles. -func (s *Server) storedLoginConfig(activeProf *profilemanager.ActiveProfileState, msg *proto.LoginRequest) (*profilemanager.Config, error) { +func (s *Server) storedLoginConfig(ctx context.Context, activeProf *profilemanager.ActiveProfileState, msg *proto.LoginRequest) (*profilemanager.Config, error) { if msg.ProfileName == nil { cfgPath, err := s.profileManager.ActiveProfilePath(activeProf) if err != nil { @@ -1184,7 +1184,11 @@ func (s *Server) storedLoginConfig(activeProf *profilemanager.ActiveProfileState // Mirrors switchProfileIfNeeded, so this reads the very profile the switch // would activate. - return s.storedProfileConfig(*msg.ProfileName) + resolved, err := s.targetProfile(ctx) + if err != nil { + return nil, err + } + return s.storedProfileConfig(resolved) } // storedConfigAtPath reads a profile config file, yielding nil when it does not @@ -1218,33 +1222,10 @@ func callerIdentity(ctx context.Context) (ipcauth.Identity, error) { return id, nil } -// resolveProfileHandle resolves a wire-level profile handle (display -// name, ID, or unique ID prefix). Returns gRPC status errors so -// handlers can return them directly. -func (s *Server) resolveProfileHandle(handle string) (*profilemanager.Profile, error) { - p, err := s.profileManager.ResolveProfile(handle) - if err == nil { - return p, nil - } - var amb *profilemanager.ErrAmbiguousHandle - if errors.As(err, &amb) { - return nil, gstatus.Errorf(codes.InvalidArgument, "%v", amb) - } - if errors.Is(err, profilemanager.ErrProfileNotFound) { - return nil, gstatus.Errorf(codes.NotFound, "profile %q not found", handle) - } - return nil, fmt.Errorf("resolve profile: %w", err) -} - -// switchProfileIfNeeded resolves the user-supplied handle, updates the -// active profile state if it differs from the current one, and returns -// the resolved profile so callers can include its ID in RPC responses. -func (s *Server) switchProfileIfNeeded(handle string, activeProf *profilemanager.ActiveProfileState) (*profilemanager.Profile, error) { - resolved, err := s.resolveProfileHandle(handle) - if err != nil { - return nil, err - } - +// switchProfileIfNeeded updates the active profile state when the profile the +// gate resolved differs from the current one, and returns it so callers can +// include its ID in RPC responses. +func (s *Server) switchProfileIfNeeded(resolved *profilemanager.Profile, activeProf *profilemanager.ActiveProfileState) (*profilemanager.Profile, error) { if s.isActiveProfile(activeProf, resolved) { return resolved, nil } @@ -1278,7 +1259,11 @@ func (s *Server) SwitchProfile(callerCtx context.Context, msg *proto.SwitchProfi } if msg != nil && msg.ProfileName != nil { - if _, err := s.switchProfileIfNeeded(*msg.ProfileName, activeProf); err != nil { + resolved, err := s.targetProfile(callerCtx) + if err != nil { + return nil, err + } + if _, err := s.switchProfileIfNeeded(resolved, activeProf); err != nil { log.Errorf("failed to switch profile: %v", err) return nil, err } @@ -1431,7 +1416,7 @@ func (s *Server) Logout(ctx context.Context, msg *proto.LogoutRequest) (*proto.L } func (s *Server) handleProfileLogout(ctx context.Context, msg *proto.LogoutRequest) (*proto.LogoutResponse, error) { - resolved, err := s.resolveProfileHandle(*msg.ProfileName) + resolved, err := s.targetProfile(ctx) if err != nil { return nil, err } @@ -2243,9 +2228,8 @@ func (s *Server) GetConfig(ctx context.Context, req *proto.GetConfigRequest) (*p return nil, ctx.Err() } - resolved, err := s.resolveProfileHandle(req.ProfileName) + resolved, err := s.targetProfile(ctx) if err != nil { - log.Errorf("failed to resolve profile %q: %v", req.ProfileName, err) return nil, err } cfgPath := resolved.Path @@ -2388,7 +2372,7 @@ func (s *Server) RenameProfile(ctx context.Context, msg *proto.RenameProfileRequ return nil, gstatus.Errorf(codes.InvalidArgument, "profile name and new profile name must be provided") } - resolved, err := s.resolveProfileHandle(msg.Handle) + resolved, err := s.targetProfile(ctx) if err != nil { return nil, err } @@ -2417,7 +2401,7 @@ func (s *Server) RemoveProfile(ctx context.Context, msg *proto.RemoveProfileRequ return nil, gstatus.Errorf(codes.InvalidArgument, "profile name must be provided") } - resolved, err := s.resolveProfileHandle(msg.ProfileName) + resolved, err := s.targetProfile(ctx) if err != nil { return nil, err } @@ -2464,7 +2448,7 @@ func (s *Server) ClaimProfile(ctx context.Context, msg *proto.ClaimProfileReques return nil, gstatus.Errorf(codes.InvalidArgument, "%v", err) } - resolved, err := s.resolveProfileHandle(msg.Handle) + resolved, err := s.targetProfile(ctx) if err != nil { return nil, err } @@ -2831,7 +2815,7 @@ func (s *Server) authorizeAndPrepareLogin(callerCtx context.Context, msg *proto. s.guardedConfigMu.Lock() defer s.guardedConfigMu.Unlock() - stored, err := s.storedLoginConfig(activeProf, msg) + stored, err := s.storedLoginConfig(callerCtx, activeProf, msg) if err != nil { return nil, nil, err } @@ -2855,7 +2839,11 @@ func (s *Server) authorizeAndPrepareLogin(callerCtx context.Context, msg *proto. } if msg.ProfileName != nil { - if _, err := s.switchProfileIfNeeded(*msg.ProfileName, activeProf); err != nil { + resolved, err := s.targetProfile(callerCtx) + if err != nil { + return nil, nil, err + } + if _, err := s.switchProfileIfNeeded(resolved, activeProf); err != nil { return nil, nil, fmt.Errorf("switch profile: %w", err) } } @@ -2896,16 +2884,16 @@ func (s *Server) SessionHolder() (ipcauth.Principal, bool) { return principal, true } -// OwnsProfile reports whether the profile the handle resolves to answers to -// this identity, and what was wrong with the handle when resolution failed. +// ResolveTarget resolves the profile a request names to a concrete profile and +// reports whether this identity may address it. // -// This triggers stamping of legacy profiles, and reloads the active profile's -// config so the stamp is visible to SessionHolder. -func (s *Server) OwnsProfile(id ipcauth.Identity, handle string) (bool, error) { +// This triggers stamping of legacy and default profiles, and reloads the active +// profile's config so the stamp is visible to SessionHolder. +func (s *Server) ResolveTarget(id ipcauth.Identity, handle string) (ipcauth.Target, error) { s.profileManager.ClaimDefaultProfileIfNeeded(id) s.profileManager.ClaimLegacyProfiles(id) - // The daemon's copy of the active profile's config goes stale after a the - // possible profile claims above. + // The claims above stamp owners on profiles, so the daemon's copy of the + // active profile's config might go stale. s.reloadActiveConfig() // Without the active profile there is nothing to fall back to and nothing @@ -2914,27 +2902,83 @@ func (s *Server) OwnsProfile(id ipcauth.Identity, handle string) (bool, error) { activeProfile, err := s.profileManager.GetActiveProfileState() if err != nil { log.Warnf("failed to get active profile: %v", err) - return false, nil + return ipcauth.Target{}, nil } if activeProfile == nil { log.Warn("no active profile to authorize against") - return false, nil + return ipcauth.Target{}, nil } if handle == "" { handle = activeProfile.ID.String() } - resolved, resolveErr := s.resolveProfileHandle(handle) + match, matchErr := s.profileManager.MatchProfiles(handle) if afterProfileResolve != nil { afterProfileResolve() } - if resolveErr != nil { - log.Debugf("failed to resolve profile %q: %v", handle, resolveErr) - return false, resolveErr + if matchErr != nil { + log.Debugf("failed to match profile %q: %v", handle, matchErr) + return ipcauth.Target{}, matchHandleError(handle, matchErr) } - return resolved.AccessibleBy(id), nil + + return targetFromMatch(id, handle, match) +} + +// targetFromMatch picks the profile the caller meant out of everything the +// handle matched. Their own profile wins, only a handle matching two of the +// caller's own profiles is ambiguous. +func targetFromMatch(id ipcauth.Identity, handle string, match profilemanager.HandleMatch) (ipcauth.Target, error) { + var owned []profilemanager.Profile + for _, p := range match.Profiles { + if p.AccessibleBy(id) { + owned = append(owned, p) + } + } + + switch { + case len(owned) == 1: + return ipcauth.Target{Path: owned[0].Path, Owned: true}, nil + + case len(owned) > 1: + return ipcauth.Target{}, gstatus.Errorf(codes.InvalidArgument, "%v", &profilemanager.ErrAmbiguousHandle{ + Handle: handle, + Candidates: owned, + Kind: match.Kind, + }) + + case len(match.Profiles) > 0: + // The handle names a profile that is real but not the caller's. + return ipcauth.Target{Path: match.Profiles[0].Path}, nil + + default: + return ipcauth.Target{}, gstatus.Errorf(codes.NotFound, "profile %q not found", handle) + } +} + +// matchHandleError renders a failed match as something the caller can act on. +func matchHandleError(handle string, err error) error { + if errors.Is(err, profilemanager.ErrProfileNotFound) { + return gstatus.Errorf(codes.NotFound, "profile %q not found", handle) + } + return fmt.Errorf("match profile: %w", err) +} + +// targetProfile returns the profile the gate resolved and authorized for this +// request. +func (s *Server) targetProfile(ctx context.Context) (*profilemanager.Profile, error) { + path, ok := ipcauth.TargetFromContext(ctx) + if !ok { + return nil, gstatus.Error(codes.Internal, "no target profile was resolved for this request") + } + + resolved, err := s.profileManager.ProfileByPath(path) + if err != nil { + log.Debugf("the profile the gate resolved at %q is gone: %v", path, err) + return nil, gstatus.Error(codes.NotFound, "the profile this request names is no longer there") + } + return resolved, nil } // afterProfileResolve is a seam for tests to run a concurrent profile switch diff --git a/client/server/server_jwt_test.go b/client/server/server_jwt_test.go index 1907ebf77..8170881bc 100644 --- a/client/server/server_jwt_test.go +++ b/client/server/server_jwt_test.go @@ -103,8 +103,9 @@ func TestSwitchProfile_ClearsJWTCache(t *testing.T) { const target = "second" username := "tester" owner := unprivilegedIdentity() + targetPath := filepath.Join(profilemanager.DefaultConfigPathDir, target+".json") _, err := profilemanager.UpdateOrCreateConfig(profilemanager.ConfigInput{ - ConfigPath: filepath.Join(profilemanager.DefaultConfigPathDir, target+".json"), + ConfigPath: targetPath, ManagementURL: "https://api.netbird.io:443", Owner: &owner, }) @@ -115,7 +116,7 @@ func TestSwitchProfile_ClearsJWTCache(t *testing.T) { name := target // The handler scopes the switch to the caller's identity, which a real // caller gets from the daemon's transport credentials. - _, err = s.SwitchProfile(ctxWithIdentity(owner), &proto.SwitchProfileRequest{ProfileName: &name, Username: &username}) + _, err = s.SwitchProfile(withTarget(ctxWithIdentity(owner), targetPath), &proto.SwitchProfileRequest{ProfileName: &name, Username: &username}) require.NoError(t, err) active, err := s.profileManager.GetActiveProfileState() diff --git a/client/server/server_ownsprofile_test.go b/client/server/server_resolvetarget_test.go similarity index 54% rename from client/server/server_ownsprofile_test.go rename to client/server/server_resolvetarget_test.go index b7a24debd..92b15b52e 100644 --- a/client/server/server_ownsprofile_test.go +++ b/client/server/server_resolvetarget_test.go @@ -3,6 +3,7 @@ package server import ( "os" "path/filepath" + "runtime" "testing" "github.com/stretchr/testify/require" @@ -15,10 +16,9 @@ import ( // Resolving a handle claims every legacy profile the caller can take, the // active one included, whatever profile the handle itself names. SessionHolder -// answers from the daemon's in-memory config, so OwnsProfile has to refresh it -// for any handle: a copy taken before the claim reports no owner at all, and a -// session with no owner is one every identified caller may take over. -func TestOwnsProfile_RefreshesActiveConfigForAnyHandle(t *testing.T) { +// answers from the daemon's in-memory config, so ResolveTarget has to refresh it +// for any handle. +func TestResolveTarget_RefreshesActiveConfigForAnyHandle(t *testing.T) { other := "second-profile" for _, tc := range []struct { @@ -47,9 +47,9 @@ func TestOwnsProfile_RefreshesActiveConfigForAnyHandle(t *testing.T) { _, running := s.SessionHolder() require.False(t, running, "fixture is wrong: the stale copy already names an owner") - owns, err := s.OwnsProfile(owner, tc.handle) + target, err := s.ResolveTarget(owner, tc.handle) require.NoError(t, err) - require.True(t, owns, "the caller owns every profile in this fixture") + require.True(t, target.Owned, "the caller owns every profile in this fixture") holder, running := s.SessionHolder() require.True(t, running, "the claimed owner never reached the daemon's config, so the live session is unowned") @@ -60,20 +60,18 @@ func TestOwnsProfile_RefreshesActiveConfigForAnyHandle(t *testing.T) { // A caller whose profile the daemon cannot read is not the owner of anything. // The answer has to be no rather than a panic in the authorization path. -func TestOwnsProfile_UnreadableActiveProfileStateDenies(t *testing.T) { +func TestResolveTarget_UnreadableActiveProfileStateDenies(t *testing.T) { s, _, _, _, _ := setupServerWithProfile(t) require.NoError(t, os.WriteFile(profilemanager.ActiveProfileStatePath, []byte("{"), 0600)) - owns, err := s.OwnsProfile(unprivilegedIdentity(), "") + target, err := s.ResolveTarget(unprivilegedIdentity(), "") require.NoError(t, err, "an unreadable active profile is not the caller's handle to fix") - require.False(t, owns) + require.False(t, target.Owned) } // A config the daemon cannot re-read leaves the one it already has in place. -// Dropping a nil in its stead would take down every reader of it, SessionHolder -// among them, which is the authorization path itself. -func TestOwnsProfile_UnreadableConfigKeepsTheOneInPlace(t *testing.T) { +func TestResolveTarget_UnreadableConfigKeepsTheOneInPlace(t *testing.T) { s, _, _, _, _ := setupServerWithProfile(t) // An ID no path can be built for, which is what a hand-edited or @@ -84,9 +82,9 @@ func TestOwnsProfile_UnreadableConfigKeepsTheOneInPlace(t *testing.T) { s.config = kept s.clientRunning = true - owns, err := s.OwnsProfile(unprivilegedIdentity(), "") - require.False(t, owns, "a profile that did not resolve is nobody's") - require.Equal(t, codes.NotFound, gstatus.Code(err), "resolution failed") + target, err := s.ResolveTarget(unprivilegedIdentity(), "") + require.NoError(t, err, "an active profile the daemon cannot place is not the caller's handle to fix") + require.False(t, target.Owned, "a profile the caller does not own is nobody's to act on") require.Same(t, kept, s.config, "a failed reload replaced the daemon's config") holder, running := s.SessionHolder() @@ -98,7 +96,7 @@ func TestOwnsProfile_UnreadableConfigKeepsTheOneInPlace(t *testing.T) { // reads every profile off disk, and SwitchProfile only needs the daemon lock, // which the gate does not hold. The config the reload publishes has to be the // one the daemon is now on, not the one the check started out reading. -func TestOwnsProfile_ReloadFollowsASwitchThatLandsMidCheck(t *testing.T) { +func TestResolveTarget_ReloadFollowsASwitchThatLandsMidCheck(t *testing.T) { s, _, activeProfile, _, _ := setupServerWithProfile(t) owner := unprivilegedIdentity() @@ -123,9 +121,9 @@ func TestOwnsProfile_ReloadFollowsASwitchThatLandsMidCheck(t *testing.T) { } t.Cleanup(func() { afterProfileResolve = nil }) - owns, err := s.OwnsProfile(owner, activeProfile) + target, err := s.ResolveTarget(owner, activeProfile) require.NoError(t, err) - require.True(t, owns) + require.True(t, target.Owned) require.NotNil(t, s.config.ManagementURL) require.Equal(t, switchedToURL, s.config.ManagementURL.String(), @@ -133,15 +131,73 @@ func TestOwnsProfile_ReloadFollowsASwitchThatLandsMidCheck(t *testing.T) { } // The handlers that start a session read their config off disk themselves. -func TestOwnsProfile_IdleDaemonKeepsItsConfig(t *testing.T) { +func TestResolveTarget_IdleDaemonKeepsItsConfig(t *testing.T) { s, _, activeProfile, _, _ := setupServerWithProfile(t) untouched := &profilemanager.Config{} s.config = untouched s.clientRunning = false - owns, err := s.OwnsProfile(unprivilegedIdentity(), activeProfile) + target, err := s.ResolveTarget(unprivilegedIdentity(), activeProfile) require.NoError(t, err) - require.True(t, owns) + require.True(t, target.Owned) require.Same(t, untouched, s.config) } + +// foreignIdentity is a caller that is neither this process nor the one the test +// fixtures own profiles for, so a profile stamped for either is somebody else. +func foreignIdentity() ipcauth.Identity { + if runtime.GOOS == "windows" { + return ipcauth.KnownForTest(ipcauth.Identity{SID: "S-1-5-21-1-2-3-4242"}) + } + return ipcauth.KnownForTest(ipcauth.Identity{UID: unprivUID + 1, GID: unprivUID + 1}) +} + +// Two accounts can hold the same legacy profile ID in their own directories, +// and a handle is resolved against every profile on the machine. +func TestResolveTarget_PrefersTheCallersOwnOverANamesake(t *testing.T) { + s, _, _, _, _ := setupServerWithProfile(t) + + shared := "shared-legacy-name" + ownPath := plantNamesakeProfiles(t, s, shared) + + target, err := s.ResolveTarget(unprivilegedIdentity(), shared) + require.NoError(t, err, "another account's namesake must not make the handle ambiguous") + require.True(t, target.Owned) + require.Equal(t, ownPath, target.Path, "the handle resolved to the other account's profile") +} + +// A profile that exists but belongs to somebody else resolves, so the refusal +// is about ownership rather than about the handle. +func TestResolveTarget_AnotherUsersProfileIsNotOwned(t *testing.T) { + s, _, activeProfile, _, _ := setupServerWithProfile(t) + + target, err := s.ResolveTarget(foreignIdentity(), activeProfile) + require.NoError(t, err, "the profile is there, so nothing about the handle is wrong") + require.False(t, target.Owned, "a profile the caller does not own is not theirs to act on") +} + +// Only a handle matching two of the caller's own profiles is genuinely +// ambiguous. Anything else has an answer, and the candidates named are theirs. +func TestResolveTarget_AmbiguousOnlyAmongTheCallersOwn(t *testing.T) { + s, _, _, _, _ := setupServerWithProfile(t) + + shared := "shared-legacy-name" + plantNamesakeProfiles(t, s, shared) + + // A second copy of the same ID in a directory this caller also claims, + // which is the case no owner can settle. + secondDir := filepath.Join(profilemanager.DefaultConfigPathDir, "second") + require.NoError(t, os.MkdirAll(secondDir, 0700)) + _, err := profilemanager.UpdateOrCreateConfig(profilemanager.ConfigInput{ + ConfigPath: filepath.Join(secondDir, shared+".json"), + ManagementURL: "https://api.netbird.io:443", + Owner: testProfileOwner(), + }) + require.NoError(t, err) + + _, err = s.ResolveTarget(unprivilegedIdentity(), shared) + require.Error(t, err) + require.Equal(t, codes.InvalidArgument, gstatus.Code(err), + "two profiles of the caller's own under one handle is theirs to disambiguate") +} diff --git a/client/server/setconfig_mdm_test.go b/client/server/setconfig_mdm_test.go index e2e7240b5..f6a909181 100644 --- a/client/server/setconfig_mdm_test.go +++ b/client/server/setconfig_mdm_test.go @@ -100,6 +100,12 @@ func setupServerWithProfile(t *testing.T) (s *Server, ctx context.Context, profN // without an identity the gate would (correctly) refuse the SSH fields. ctx = privilegedTestCtx() s = New(ctx, "console", "", false, false, false, false) + + // The gate resolves the request's handle before the handler runs and hands + // the profile down in the context. Driving the handler directly skips the + // gate, so the fixture stands in for it with the profile these requests + // name. + ctx = withTarget(ctx, cfgPath) return s, ctx, profName, currUser.Username, cfgPath } diff --git a/client/server/setconfig_test.go b/client/server/setconfig_test.go index 7442b718e..333f6b791 100644 --- a/client/server/setconfig_test.go +++ b/client/server/setconfig_test.go @@ -58,6 +58,10 @@ func TestSetConfig_AllFieldsSaved(t *testing.T) { ctx := privilegedTestCtx() s := New(ctx, "console", "", false, false, false, false) + // The gate resolves the handle and hands the profile down in the context; + // driving the handler directly skips it, so the test stands in for it. + ctx = withTarget(ctx, ic.ConfigPath) + rosenpassEnabled := true rosenpassPermissive := true serverSSHAllowed := true diff --git a/client/server/ssh_gate_test.go b/client/server/ssh_gate_test.go index b730ffaa6..da8d338ed 100644 --- a/client/server/ssh_gate_test.go +++ b/client/server/ssh_gate_test.go @@ -38,6 +38,14 @@ var unprivUID = uint32(os.Geteuid() + 1) // The fabricated identities have to be shaped like the platform's: a uid says // nothing on Windows, and a zero uid there would read as root and be privileged. +// withTarget is a request context as the gate leaves it: carrying the profile +// the gate resolved and authorized out of the request's handle. A test that +// calls a target-scoped handler directly supplies it, since the interceptor +// that normally would is not in play. +func withTarget(ctx context.Context, profilePath string) context.Context { + return ipcauth.ContextWithTarget(ctx, profilePath) +} + func rootCtx() context.Context { return ctxWithIdentity(privilegedIdentity()) } func userCtx() context.Context { return ctxWithIdentity(unprivilegedIdentity()) }