diff --git a/client/internal/ipcauth/authz_gate_test.go b/client/internal/ipcauth/authz_gate_test.go index c4f65a76a..ed23e721c 100644 --- a/client/internal/ipcauth/authz_gate_test.go +++ b/client/internal/ipcauth/authz_gate_test.go @@ -206,3 +206,32 @@ func TestAuthorizeBlamesAHeldSession(t *testing.T) { assert.Contains(t, denial.Command, "netbird down", "taking the connection down is the remedy this refusal points at") } + +// A live session on a profile that has no owner is held by the zero Principal. +// An owner of the target profile must not be taken for its holder as we want +// to fail close. +func TestResolveLevelUnownedLiveSessionFailsClosed(t *testing.T) { + g := gateFor(t, stubState{ + target: Target{Path: "/profiles/mine.json", Owned: true}, + running: true, + }) + + _, err := g.authorize(transportCtx(unprivUser, nil), servicePath+"Up", &proto.UpRequest{}) + require.Error(t, err, "a caller reached a live session nobody owns") + + denial, ok := DenialFrom(err) + require.True(t, ok, "the refusal carries no explanation") + assert.Equal(t, ErrorReasonSessionHeld, denial.Reason, + "the caller owns the target, so the refusal is about the live session") +} + +// With no session running there is nothing to hold, so the owner of the target +// profile may act on it. +func TestResolveLevelIdleDaemonGrantsSessionHolder(t *testing.T) { + g := gateFor(t, stubState{ + target: Target{Path: "/profiles/mine.json", Owned: true}, + }) + + _, err := g.authorize(transportCtx(unprivUser, nil), servicePath+"Up", &proto.UpRequest{}) + assert.NoError(t, err) +} diff --git a/client/server/server.go b/client/server/server.go index 9d0fe7698..d879ab2ca 100644 --- a/client/server/server.go +++ b/client/server/server.go @@ -3113,10 +3113,16 @@ func (s *Server) SessionHolder() (ipcauth.Principal, bool) { s.mutex.Lock() defer s.mutex.Unlock() - if !s.clientRunning || len(s.config.Owners) == 0 { + if !s.clientRunning { return ipcauth.Principal{}, false } + // A session with no ownership is held by the zero Principal, which matches + // nobody and fail close. + if len(s.config.Owners) == 0 { + return ipcauth.Principal{}, true + } + // The zero Principal matches nobody, so an unparseable owner locks the // session rather than opening it. principal, ok := ipcauth.ParsePrincipal(s.config.Owners[0]) diff --git a/client/server/server_resolvetarget_test.go b/client/server/server_resolvetarget_test.go index 75687bd22..b9809c107 100644 --- a/client/server/server_resolvetarget_test.go +++ b/client/server/server_resolvetarget_test.go @@ -44,8 +44,8 @@ func TestResolveTarget_RefreshesActiveConfigForAnyHandle(t *testing.T) { // The daemon's copy as it stood before the claim landed on disk. s.config = &profilemanager.Config{} s.clientRunning = true - _, running := s.SessionHolder() - require.False(t, running, "fixture is wrong: the stale copy already names an owner") + stale, _ := s.SessionHolder() + require.False(t, stale.Matches(owner), "fixture is wrong: the stale copy already names an owner") target, err := s.ResolveTarget(owner, tc.handle) require.NoError(t, err)