diff --git a/client/internal/ipcauth/authz_gate_test.go b/client/internal/ipcauth/authz_gate_test.go index 5ff9d06ba..b9f53fbff 100644 --- a/client/internal/ipcauth/authz_gate_test.go +++ b/client/internal/ipcauth/authz_gate_test.go @@ -106,34 +106,42 @@ 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{targetErr: gstatus.Error(codes.NotFound, "profile not found")}) - - _, 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 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) { notFound := gstatus.Error(codes.NotFound, "profile not found") owned := Target{Path: "/profiles/some-profile.json", Owned: true} + someoneElse := Principal{Kind: KindUID, Value: "4242"} for _, tc := range []struct { - name string - st stubState + name string + method string + msg any + st stubState }{ - {"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")}}, + { + // A session somebody else holds stops at profile owner, so this + // needs a method profile owner is enough for. + name: "a live session it reports as owned", method: "GetConfig", + msg: &proto.GetConfigRequest{ProfileName: "some-profile"}, + st: stubState{target: owned, running: true, holder: someoneElse, targetErr: notFound}, + }, + { + name: "an idle daemon it reports as owned", method: "SwitchProfile", + msg: switchTo("some-profile"), + st: stubState{target: owned, targetErr: notFound}, + }, + { + name: "a daemon-side failure it reports as owned", method: "SwitchProfile", + msg: switchTo("some-profile"), + st: stubState{target: owned, targetErr: errors.New("read profile directory")}, + }, } { t.Run(tc.name, func(t *testing.T) { g := gateFor(t, tc.st) - _, err := g.authorize(transportCtx(unprivUser, nil), servicePath+"SwitchProfile", switchTo("some-profile")) + _, err := g.authorize(transportCtx(unprivUser, nil), servicePath+tc.method, tc.msg) assert.Error(t, err, "a failed resolution conferred a level it had no business conferring") }) } @@ -167,3 +175,21 @@ func TestAuthorizeCarriesTheTargetForAPrivilegedCaller(t *testing.T) { require.True(t, ok, "root resolved nothing to act on") assert.Equal(t, "/profiles/abcd1111.json", got) } + +func TestAuthorizeBlamesAHeldSession(t *testing.T) { + g := gateFor(t, stubState{ + target: Target{Path: "/profiles/mine.json", Owned: true}, + running: true, + holder: Principal{Kind: KindUID, Value: "4242"}, + }) + + _, err := g.authorize(transportCtx(unprivUser, nil), servicePath+"Up", &proto.UpRequest{}) + require.Error(t, err) + + denial, ok := DenialFrom(err) + require.True(t, ok, "a caller kept out by somebody else's session got no explanation") + assert.Equal(t, ErrorReasonSessionHeld, denial.Reason, + "the caller owns the profile, so the refusal is about the connection, not ownership") + assert.Contains(t, denial.Command, "netbird down", + "taking the connection down is the remedy this refusal points at") +} diff --git a/client/internal/ipcauth/privilege_denial_test.go b/client/internal/ipcauth/privilege_denial_test.go index ec62842aa..9132f8833 100644 --- a/client/internal/ipcauth/privilege_denial_test.go +++ b/client/internal/ipcauth/privilege_denial_test.go @@ -114,12 +114,16 @@ func (s stubState) ResolveTarget(Identity, string) (Target, error) { return s.ta // 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. +// +// Profile owner is the whole input: denyPolicyLevel reads the level and nothing +// else, and resolveLevel only ever hands it that level when a session is +// running and somebody else holds it. TestAuthorizeBlamesAHeldSession is what +// holds those two together. func TestDenyPolicyLevelExplainsAHeldSession(t *testing.T) { req := Request{ Identity: KnownForTest(Identity{UID: 1000}), Level: AuthzLevelProfileOwner, Method: servicePath + "Up", - State: stubState{holder: Principal{Kind: KindUID, Value: "4242"}, running: true}, } info := denialDetail(t, denyPolicyLevel(req, methodPolicies[servicePath+"Up"])) @@ -135,35 +139,20 @@ func TestDenyPolicyLevelExplainsAHeldSession(t *testing.T) { assert.Contains(t, info.GetMetadata()[ErrorMetaCommand], "netbird down") } -// With no session running, a caller short of session holder fell short on -// ownership instead, and the refusal says so rather than blaming a session. -func TestDenyPolicyLevelWithNoSessionBlamesOwnership(t *testing.T) { +// A caller who never owned the profile is refused for the profile, whatever the +// connection is doing. +func TestDenyPolicyLevelBelowProfileOwnerBlamesOwnership(t *testing.T) { req := Request{ Identity: KnownForTest(Identity{UID: 1000}), Level: AuthzLevelIdentified, Method: servicePath + "Up", - State: stubState{}, - } - - info := denialDetail(t, denyPolicyLevel(req, methodPolicies[servicePath+"Up"])) - assert.Equal(t, ErrorReasonNotProfileOwner, info.GetReason()) - assert.NotContains(t, info.GetMetadata()[ErrorMetaSummary], "connected") -} - -// Somebody else's session is not what stops a caller who never owned the -// profile: they are refused for the profile, and netbird down is neither theirs -// to run nor any help. -func TestDenyPolicyLevelBlamesOwnershipWhileASessionRuns(t *testing.T) { - req := Request{ - Identity: KnownForTest(Identity{UID: 1000}), - Level: AuthzLevelIdentified, - Method: servicePath + "Up", - State: stubState{holder: Principal{Kind: KindUID, Value: "4242"}, running: true}, } info := denialDetail(t, denyPolicyLevel(req, methodPolicies[servicePath+"Up"])) assert.Equal(t, ErrorReasonNotProfileOwner, info.GetReason()) assert.Contains(t, info.GetMetadata()[ErrorMetaSummary], "belongs to another user") + assert.NotContains(t, info.GetMetadata()[ErrorMetaSummary], "connected", + "the refusal is about the profile, so it must not blame a connection") _, hasCommand := info.GetMetadata()[ErrorMetaCommand] assert.False(t, hasCommand, "ending a session does not make the profile theirs")