From edf4a45df9896edb4b083075c98d484510d812a2 Mon Sep 17 00:00:00 2001 From: Nico <32375741+nulmete@users.noreply.github.com> Date: Wed, 22 Jul 2026 11:12:16 -0300 Subject: [PATCH] Stop the delete host endpoint from revealing out-of-fleet host existence (#49645) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit **Related issue:** Resolves: N/A # Checklist for submitter - [x] Changes file added for user-visible changes in `changes/`, `orbit/changes/` or `ee/fleetd-chrome/changes`. See [Changes files](https://github.com/fleetdm/fleet/blob/main/docs/Contributing/guides/committing-changes.md#changes-files) for more information. ## Testing - [x] Added/updated automated tests - [x] QA'd all new/changed functionality manually ## Summary by CodeRabbit * **Bug Fixes** * Updated host deletion behavior to return consistent “not found” responses when the host doesn’t exist or isn’t visible to the requester. * Prevented out-of-scope delete attempts from disclosing whether the target host exists (now returns “not found” instead of “forbidden”). * Preserved “forbidden” errors when the host is visible but the requester lacks delete permission. * **Tests** * Added/updated authorization and deletion coverage to verify the new response-masking behavior. --- changes/fix-delete-host-response-mismatch | 1 + server/authz/authz.go | 28 +++++++++ server/authz/authz_test.go | 54 +++++++++++++++++ server/service/hosts.go | 14 +++-- server/service/hosts_test.go | 73 +++++++++++++++++++++-- 5 files changed, 162 insertions(+), 8 deletions(-) create mode 100644 changes/fix-delete-host-response-mismatch create mode 100644 server/authz/authz_test.go diff --git a/changes/fix-delete-host-response-mismatch b/changes/fix-delete-host-response-mismatch new file mode 100644 index 0000000000..a9c93ccc20 --- /dev/null +++ b/changes/fix-delete-host-response-mismatch @@ -0,0 +1 @@ +* Fixed the delete host endpoint returning inconsistent responses for a host outside the requester's fleet versus one that doesn't exist. diff --git a/server/authz/authz.go b/server/authz/authz.go index 24fc700dea..e9989dc64b 100644 --- a/server/authz/authz.go +++ b/server/authz/authz.go @@ -15,6 +15,7 @@ import ( "fmt" authz_ctx "github.com/fleetdm/fleet/v4/server/contexts/authz" + "github.com/fleetdm/fleet/v4/server/contexts/ctxerr" "github.com/fleetdm/fleet/v4/server/contexts/viewer" "github.com/fleetdm/fleet/v4/server/fleet" platform_authz "github.com/fleetdm/fleet/v4/server/platform/authz" @@ -132,6 +133,33 @@ func (a *Authorizer) Authorize(ctx context.Context, object, action interface{}) return nil } +// AuthorizeOrNotFound authorizes writeAction on writeObject. If that fails, +// it also checks fleet.ActionRead on readObject; if the caller can't even +// read it, notFoundErr is returned instead of the write failure, so a +// resource entirely outside the caller's visibility is indistinguishable +// from one that doesn't exist. If the caller CAN read it, or notFoundErr is +// nil, the original write-authorization failure is returned unchanged: in +// the first case no new information is disclosed by it (the caller already +// knows the resource exists); in the second, masking would incorrectly +// return nil (success) for a caller who was never authorized. +func (a *Authorizer) AuthorizeOrNotFound(ctx context.Context, writeObject, writeAction, readObject any, notFoundErr error) error { + writeErr := a.Authorize(ctx, writeObject, writeAction) + if writeErr == nil { + return nil + } + + if readErr := a.Authorize(ctx, readObject, fleet.ActionRead); readErr == nil || notFoundErr == nil { + return writeErr + } + + // The caller can't read this resource either: report notFoundErr instead + // of writeErr, so its existence isn't disclosed. Still record the real + // cause for observability, so a systemic authz/policy failure isn't + // silently reported as "not found" for every caller. + ctxerr.Handle(ctx, writeErr) + return notFoundErr +} + // ExtraAuthzer is the interface to implement extra fields for the policy. type ExtraAuthzer interface { // ExtraAuthz returns the extra key/value pairs for the type. diff --git a/server/authz/authz_test.go b/server/authz/authz_test.go new file mode 100644 index 0000000000..33aef6772a --- /dev/null +++ b/server/authz/authz_test.go @@ -0,0 +1,54 @@ +package authz + +import ( + "errors" + "testing" + + "github.com/fleetdm/fleet/v4/server/fleet" + "github.com/fleetdm/fleet/v4/server/test" + "github.com/stretchr/testify/require" +) + +func TestAuthorizeOrNotFound(t *testing.T) { + notFoundErr := errors.New("not found sentinel") + teamHost := &fleet.Host{TeamID: new(uint(1))} + + t.Run("write allowed", func(t *testing.T) { + ctx := test.UserContext(t.Context(), &fleet.User{Teams: []fleet.UserTeam{{Team: fleet.Team{ID: 1}, Role: fleet.RoleAdmin}}}) + err := auth.AuthorizeOrNotFound(ctx, teamHost, fleet.ActionWrite, teamHost, notFoundErr) + require.NoError(t, err) + }) + + t.Run("write denied but read allowed returns the write error, not masked", func(t *testing.T) { + // A team observer can read the host but can't write it: this is not + // an existence oracle (the caller already knows the host exists), so + // the real Forbidden should surface, not notFoundErr. + ctx := test.UserContext(t.Context(), &fleet.User{Teams: []fleet.UserTeam{{Team: fleet.Team{ID: 1}, Role: fleet.RoleObserver}}}) + err := auth.AuthorizeOrNotFound(ctx, teamHost, fleet.ActionWrite, teamHost, notFoundErr) + require.Error(t, err) + require.NotErrorIs(t, err, notFoundErr) + var forbidden *Forbidden + require.ErrorAs(t, err, &forbidden) + }) + + t.Run("write denied and read denied masks as notFoundErr", func(t *testing.T) { + // A caller with no relationship to the host's team can't read or + // write it: masking as notFoundErr prevents them from learning the + // host exists on some other team via a distinguishable Forbidden. + ctx := test.UserContext(t.Context(), &fleet.User{Teams: []fleet.UserTeam{{Team: fleet.Team{ID: 2}, Role: fleet.RoleObserver}}}) + err := auth.AuthorizeOrNotFound(ctx, teamHost, fleet.ActionWrite, teamHost, notFoundErr) + require.Error(t, err) + require.ErrorIs(t, err, notFoundErr) + }) + + t.Run("nil notFoundErr never fails open", func(t *testing.T) { + // A caller misusing this helper by passing a nil notFoundErr must + // never get nil (success) back for a caller who can neither read nor + // write the resource: that would silently bypass authorization. + ctx := test.UserContext(t.Context(), &fleet.User{Teams: []fleet.UserTeam{{Team: fleet.Team{ID: 2}, Role: fleet.RoleObserver}}}) + err := auth.AuthorizeOrNotFound(ctx, teamHost, fleet.ActionWrite, teamHost, nil) + require.Error(t, err) + var forbidden *Forbidden + require.ErrorAs(t, err, &forbidden) + }) +} diff --git a/server/service/hosts.go b/server/service/hosts.go index 0715227bbf..e0d46bb829 100644 --- a/server/service/hosts.go +++ b/server/service/hosts.go @@ -34,6 +34,7 @@ import ( "github.com/fleetdm/fleet/v4/server/mdm/assets" mdmlifecycle "github.com/fleetdm/fleet/v4/server/mdm/lifecycle" "github.com/fleetdm/fleet/v4/server/mdm/nanodep/godep" + common_mysql "github.com/fleetdm/fleet/v4/server/platform/mysql" "github.com/fleetdm/fleet/v4/server/ptr" "github.com/fleetdm/fleet/v4/server/worker" "github.com/gocarina/gocsv" @@ -915,8 +916,8 @@ func (svc *Service) checkWriteForHostIDs(ctx context.Context, ids []uint) error return ctxerr.Wrap(ctx, err, "get host for delete") } - // Authorize again with team loaded now that we have team_id - if err := svc.authz.Authorize(ctx, host, fleet.ActionWrite); err != nil { + notFoundErr := ctxerr.Wrap(ctx, common_mysql.NotFound("Host").WithID(id), "get host for delete") + if err := svc.authz.AuthorizeOrNotFound(ctx, host, fleet.ActionWrite, host, notFoundErr); err != nil { return err } } @@ -1113,8 +1114,13 @@ func (svc *Service) DeleteHost(ctx context.Context, id uint) error { return ctxerr.Wrap(ctx, err, "get host for delete") } - // Authorize again with team loaded now that we have team_id - if err := svc.authz.Authorize(ctx, host, fleet.ActionWrite); err != nil { + // Authorize again now that the host (and its team_id) is loaded. If the + // caller can't even read this host, it's entirely outside their + // visibility: report the same not-found error as a missing host above, + // rather than a forbidden that would confirm the host exists on some + // other team. + notFoundErr := ctxerr.Wrap(ctx, common_mysql.NotFound("Host").WithID(id), "get host for delete") + if err := svc.authz.AuthorizeOrNotFound(ctx, host, fleet.ActionWrite, host, notFoundErr); err != nil { return err } diff --git a/server/service/hosts_test.go b/server/service/hosts_test.go index ecec822b6a..00b49d4b61 100644 --- a/server/service/hosts_test.go +++ b/server/service/hosts_test.go @@ -1030,6 +1030,22 @@ func TestHostDetailsHostNameStatus(t *testing.T) { }) } +// checkHostWriteAuthErr asserts the result of a host-mutation authorization +// check. A caller with no read visibility into the host at all must see a +// NotFound (masking existence), not a Forbidden that would confirm the host +// exists on some other team; a caller who CAN read the host (e.g. same-team, +// wrong role) still gets the normal Forbidden, since no new information is +// disclosed by it. +func checkHostWriteAuthErr(t *testing.T, shouldFail, expectNotFound bool, err error) { + t.Helper() + if shouldFail && expectNotFound { + require.Error(t, err) + assert.True(t, fleet.IsNotFound(err)) + return + } + checkAuthErr(t, shouldFail, err) +} + // Fragile test: This test is fragile because of the large reliance on Datastore mocks. Consider refactoring test/logic or removing the test. It may be slowing us down more than helping us. func TestHostAuth(t *testing.T) { ds := new(mock.Store) @@ -1252,6 +1268,19 @@ func TestHostAuth(t *testing.T) { IncludePolicies: false, } + // A team-only role never has read visibility into a host outside + // its own team(s) (including the team_id-less "global" host used + // below): a write-authz failure in that case must surface as + // NotFound rather than Forbidden, so it doesn't confirm the + // host's existence to a caller with no view into it. + isTeamOnlyRole := tt.user.GlobalRole == nil + belongsToTeam1 := false + for _, ut := range tt.user.Teams { + if ut.Team.ID == 1 { + belongsToTeam1 = true + } + } + _, err := svc.GetHost(ctx, 1, opts) checkAuthErr(t, tt.shouldFailTeamRead, err) @@ -1277,16 +1306,16 @@ func TestHostAuth(t *testing.T) { checkAuthErr(t, tt.shouldFailGlobalRead, err) err = svc.DeleteHost(ctx, 1) - checkAuthErr(t, tt.shouldFailTeamWrite, err) + checkHostWriteAuthErr(t, tt.shouldFailTeamWrite, isTeamOnlyRole && !belongsToTeam1, err) err = svc.DeleteHost(ctx, 2) - checkAuthErr(t, tt.shouldFailGlobalWrite, err) + checkHostWriteAuthErr(t, tt.shouldFailGlobalWrite, isTeamOnlyRole, err) err = svc.DeleteHosts(ctx, []uint{1}, nil) - checkAuthErr(t, tt.shouldFailTeamWrite, err) + checkHostWriteAuthErr(t, tt.shouldFailTeamWrite, isTeamOnlyRole && !belongsToTeam1, err) err = svc.DeleteHosts(ctx, []uint{2}, nil) - checkAuthErr(t, tt.shouldFailGlobalWrite, err) + checkHostWriteAuthErr(t, tt.shouldFailGlobalWrite, isTeamOnlyRole, err) err = svc.AddHostsToTeam(ctx, new(uint(1)), []uint{1}, false) checkAuthErr(t, tt.shouldFailTeamWrite, err) @@ -1692,6 +1721,42 @@ func TestDeleteHost(t *testing.T) { }) } +func TestDeleteHostDoesNotLeakOutOfScopeExistence(t *testing.T) { + ds := new(mock.Store) + svc, ctx := newTestService(t, ds, nil, nil) + + teamHost := &fleet.Host{ID: 1, TeamID: new(uint(1))} + + ds.AppConfigFunc = func(ctx context.Context) (*fleet.AppConfig, error) { + return &fleet.AppConfig{}, nil + } + ds.HostLiteFunc = func(ctx context.Context, id uint) (*fleet.Host, error) { + return teamHost, nil + } + + // A team-scoped observer with no relationship to team 1 can neither read + // nor write host 1: the response must be indistinguishable from a + // nonexistent host (NotFound), not a Forbidden that would confirm the + // host exists on some other team. + outOfScopeUser := &fleet.User{Teams: []fleet.UserTeam{{Team: fleet.Team{ID: 2}, Role: fleet.RoleObserver}}} + err := svc.DeleteHost(viewer.NewContext(ctx, viewer.Viewer{User: outOfScopeUser}), 1) + require.Error(t, err) + assert.True(t, fleet.IsNotFound(err)) + + err = svc.DeleteHosts(viewer.NewContext(ctx, viewer.Viewer{User: outOfScopeUser}), []uint{1}, nil) + require.Error(t, err) + assert.True(t, fleet.IsNotFound(err)) + + // A team-scoped observer who belongs to team 1 can read host 1, just not + // write it: this must remain a normal Forbidden error, since no new + // information about the host's existence is disclosed by it. + inScopeObserver := &fleet.User{Teams: []fleet.UserTeam{{Team: fleet.Team{ID: 1}, Role: fleet.RoleObserver}}} + err = svc.DeleteHost(viewer.NewContext(ctx, viewer.Viewer{User: inScopeObserver}), 1) + require.Error(t, err) + assert.False(t, fleet.IsNotFound(err)) + assert.Contains(t, err.Error(), authz.ForbiddenErrorMessage) +} + func TestDeleteHostCreatesActivity(t *testing.T) { ds := mysqltest.CreateMySQLDS(t) defer ds.Close()