diff --git a/server/service/integration_core_test.go b/server/service/integration_core_test.go index 46eaa391b8..cf85b2cc1c 100644 --- a/server/service/integration_core_test.go +++ b/server/service/integration_core_test.go @@ -535,15 +535,15 @@ func (s *integrationTestSuite) TestModifyAPIOnlyUser() { "name": "New Name", }, http.StatusUnprocessableEntity) - // An API-only user cannot reach this admin endpoint: the api_only middleware - // rejects it at the catalog check (the user-management endpoint is not in the catalog). + // An API-only user cannot modify itself: the service layer rejects the + // self-modify attempt with 422. // // This is to protect against privilege escalation vulnerability. s.token = apiUserToken defer func() { s.token = s.getTestAdminToken() }() s.Do("PATCH", fmt.Sprintf("/api/latest/fleet/users/api_only/%d", apiUserID), map[string]any{ "name": "Self Update", - }, http.StatusForbidden) + }, http.StatusUnprocessableEntity) s.token = s.getTestAdminToken() s.Do("PATCH", fmt.Sprintf("/api/latest/fleet/users/api_only/%d", apiUserID), map[string]any{ diff --git a/server/service/integration_enterprise_test.go b/server/service/integration_enterprise_test.go index 2896787c2e..b7d5b71ca3 100644 --- a/server/service/integration_enterprise_test.go +++ b/server/service/integration_enterprise_test.go @@ -29132,8 +29132,10 @@ func (s *integrationEnterpriseTestSuite) TestAPIOnlyUserEndpointMiddleware() { return createResp.Token } - // With no endpoint restrictions the user can reach any endpoint in the catalog. - t.Run("no restrictions allows all catalog endpoints", func(t *testing.T) { + // With no endpoint restrictions the api_only middleware skips entirely, so + // the user can reach any registered route — gated only by role-based authz + // further down the chain. + t.Run("no restrictions skips the middleware", func(t *testing.T) { s.token = createAPIOnlyUser("api-only-mw-no-restrictions", nil) s.Do("GET", "/api/latest/fleet/version", nil, http.StatusOK) @@ -29141,12 +29143,10 @@ func (s *integrationEnterpriseTestSuite) TestAPIOnlyUserEndpointMiddleware() { s.Do("GET", "/api/latest/fleet/hosts", nil, http.StatusOK) }) - // Paths not registered in the API endpoint catalog are always rejected for - // api-only users, regardless of whether they have endpoint restrictions. - t.Run("non-catalog path is rejected", func(t *testing.T) { - s.token = createAPIOnlyUser("api-only-mw-non-catalog-unrestricted", nil) - s.Do("PATCH", "/api/latest/fleet/users/api_only/1", map[string]any{"name": "x"}, http.StatusForbidden) - + // For api-only users with restrictions, requests to paths not in the API + // endpoint catalog are rejected by the middleware before reaching the + // service layer. + t.Run("non-catalog path is rejected for restricted users", func(t *testing.T) { s.token = createAPIOnlyUser("api-only-mw-non-catalog-restricted", []map[string]any{ {"method": "GET", "path": "/api/v1/fleet/version"}, }) diff --git a/server/service/middleware/auth/api_only.go b/server/service/middleware/auth/api_only.go index d92c1f84a6..9519e8f400 100644 --- a/server/service/middleware/auth/api_only.go +++ b/server/service/middleware/auth/api_only.go @@ -19,22 +19,24 @@ import ( var RouteTemplateRequestFunc = eu.RouteTemplateRequestFunc // APIOnlyEndpointCheck returns an endpoint.Endpoint middleware that enforces -// access control for API-only users (api_only=true). It must be wired inside -// AuthenticatedUser (so a Viewer is already in context when it runs) and the -// enclosing transport must register RouteTemplateRequestFunc as a ServerBefore -// option so the mux route template is available in context. +// access control for API-only users (api_only=true) that have configured +// endpoint restrictions. It must be wired inside AuthenticatedUser (so a Viewer +// is already in context when it runs) and the enclosing transport must register +// RouteTemplateRequestFunc as a ServerBefore option so the mux route template +// is available in context. // -// For non-API-only users the check is skipped entirely. When there is no Viewer -// in context, the call passes through — AuthenticatedUser guarantees that any -// request that needs a Viewer has already been rejected before reaching here. +// The check is skipped entirely for: non-API-only users, requests with no +// Viewer in context (AuthenticatedUser already rejects those), and API-only +// users with no endpoint restrictions configured — the latter are granted +// access to every registered route, gated only by role-based authz further +// down the chain. // -// For API-only users two checks are applied in order: +// For API-only users with a non-empty restriction list (rows in +// user_api_endpoints), two checks are applied in order: // 1. The requested route must appear in the API endpoint catalog. If not, a // permission error (403) is returned. -// 2. If the user has configured endpoint restrictions (rows in -// user_api_endpoints), the route must match one of them. If not, a -// permission error (403) is returned. An empty restriction list grants -// full access to all catalog endpoints. +// 2. The route must match one of the user's allowed endpoints. If not, a +// permission error (403) is returned. func APIOnlyEndpointCheck(next endpoint.Endpoint) endpoint.Endpoint { return apiOnlyEndpointCheck(apiendpoints.IsInCatalog, next) } @@ -42,7 +44,7 @@ func APIOnlyEndpointCheck(next endpoint.Endpoint) endpoint.Endpoint { func apiOnlyEndpointCheck(isInCatalog func(string) bool, next endpoint.Endpoint) endpoint.Endpoint { return func(ctx context.Context, request any) (any, error) { v, ok := viewer.FromContext(ctx) - if !ok || v.User == nil || !v.User.APIOnly { + if !ok || v.User == nil || !v.User.APIOnly || len(v.User.APIEndpoints) == 0 { return next(ctx, request) } @@ -55,11 +57,6 @@ func apiOnlyEndpointCheck(isInCatalog func(string) bool, next endpoint.Endpoint) return nil, permissionDenied(ctx) } - // No endpoint restrictions: full access to all catalog endpoints. - if len(v.User.APIEndpoints) == 0 { - return next(ctx, request) - } - // Check whether the requested endpoint matches any of the user's allowed endpoints. for _, ep := range v.User.APIEndpoints { if fleet.NewAPIEndpointFromTpl(ep.Method, ep.Path).Fingerprint() == fp { diff --git a/server/service/middleware/auth/api_only_test.go b/server/service/middleware/auth/api_only_test.go index 853af0dd1d..d4014a7915 100644 --- a/server/service/middleware/auth/api_only_test.go +++ b/server/service/middleware/auth/api_only_test.go @@ -125,10 +125,15 @@ func TestAPIOnlyEndpointCheck(t *testing.T) { require.True(t, *called) }) - t.Run("api-only user, endpoint not in catalog", func(t *testing.T) { + t.Run("api-only user with restrictions, endpoint not in catalog", func(t *testing.T) { next, called := newNext() ctx := ctxWithMethod("GET", muxTemplate("fleet/secret_admin_endpoint")) - ctx = viewer.NewContext(ctx, viewer.Viewer{User: &fleet.User{APIOnly: true}}) + ctx = viewer.NewContext(ctx, viewer.Viewer{User: &fleet.User{ + APIOnly: true, + // Non-empty restrictions force the middleware to run; the route + // is not in the catalog, so the catalog check rejects it. + APIEndpoints: []fleet.APIEndpointRef{{Method: "GET", Path: "/api/v1/fleet/hosts"}}, + }}) _, err := newEndpoint(next)(ctx, nil) require.Error(t, err) @@ -137,12 +142,15 @@ func TestAPIOnlyEndpointCheck(t *testing.T) { require.ErrorAs(t, err, &permErr) }) - t.Run("api-only user, missing route template in context is rejected", func(t *testing.T) { + t.Run("api-only user with restrictions, missing route template in context is rejected", func(t *testing.T) { next, called := newNext() // routeTemplateKey deliberately not set (simulates RouteTemplateRequestFunc failure). ctx := context.Background() ctx = context.WithValue(ctx, kithttp.ContextKeyRequestMethod, "GET") - ctx = viewer.NewContext(ctx, viewer.Viewer{User: &fleet.User{APIOnly: true}}) + ctx = viewer.NewContext(ctx, viewer.Viewer{User: &fleet.User{ + APIOnly: true, + APIEndpoints: []fleet.APIEndpointRef{{Method: "GET", Path: "/api/v1/fleet/hosts"}}, + }}) _, err := newEndpoint(next)(ctx, nil) require.Error(t, err) @@ -151,11 +159,14 @@ func TestAPIOnlyEndpointCheck(t *testing.T) { require.ErrorAs(t, err, &permErr) }) - t.Run("api-only user, missing method and template are both rejected", func(t *testing.T) { + t.Run("api-only user with restrictions, missing method and template are both rejected", func(t *testing.T) { next, called := newNext() // Neither method nor template set — empty fingerprint never matches catalog. ctx := context.Background() - ctx = viewer.NewContext(ctx, viewer.Viewer{User: &fleet.User{APIOnly: true}}) + ctx = viewer.NewContext(ctx, viewer.Viewer{User: &fleet.User{ + APIOnly: true, + APIEndpoints: []fleet.APIEndpointRef{{Method: "GET", Path: "/api/v1/fleet/hosts"}}, + }}) _, err := newEndpoint(next)(ctx, nil) require.Error(t, err) @@ -165,12 +176,13 @@ func TestAPIOnlyEndpointCheck(t *testing.T) { }) t.Run("api-only user, method normalization is case-insensitive", func(t *testing.T) { - // Lower-case method must normalize to the same fingerprint as upper-case. + // Lower-case method must normalize to the same fingerprint as upper-case + // when matching against the catalog and the user's allow-list. next, called := newNext() ctx := ctxWithMethod("get", muxTemplate("fleet/hosts")) // lower-case ctx = viewer.NewContext(ctx, viewer.Viewer{User: &fleet.User{ APIOnly: true, - APIEndpoints: nil, + APIEndpoints: []fleet.APIEndpointRef{{Method: "GET", Path: "/api/v1/fleet/hosts"}}, }}) _, err := newEndpoint(next)(ctx, nil) @@ -178,7 +190,7 @@ func TestAPIOnlyEndpointCheck(t *testing.T) { require.True(t, *called) }) - t.Run("api-only user, rejection marks authz context as checked", func(t *testing.T) { + t.Run("api-only user with restrictions, rejection marks authz context as checked", func(t *testing.T) { // Ensures authzcheck middleware does not emit a spurious "Missing // authorization check" log when we deny an api_only user. next, called := newNext() @@ -186,7 +198,10 @@ func TestAPIOnlyEndpointCheck(t *testing.T) { ctx := authzctx.NewContext(context.Background(), ac) ctx = context.WithValue(ctx, kithttp.ContextKeyRequestMethod, "GET") ctx = eu.WithRouteTemplate(ctx, muxTemplate("fleet/secret_admin_endpoint")) - ctx = viewer.NewContext(ctx, viewer.Viewer{User: &fleet.User{APIOnly: true}}) + ctx = viewer.NewContext(ctx, viewer.Viewer{User: &fleet.User{ + APIOnly: true, + APIEndpoints: []fleet.APIEndpointRef{{Method: "GET", Path: "/api/v1/fleet/hosts"}}, + }}) _, err := newEndpoint(next)(ctx, nil) require.Error(t, err)