From a90c9ce2b0e2a49b0d07ca17ce7b77e4f9476af9 Mon Sep 17 00:00:00 2001 From: Derek Menteer <105233703+hashi-derek@users.noreply.github.com> Date: Wed, 24 May 2023 16:35:55 -0500 Subject: [PATCH] Fix ACL check on health endpoint (#17424) Fix ACL check on health endpoint Prior to this change, the service health API would not explicitly return an error whenever a token with invalid permissions was given, and it would instead return empty results. With this change, a "Permission denied" error is returned whenever data is queried. This is done to better support the agent cache, which performs a fetch backoff sleep whenever ACL errors are encountered. Affected endpoints are: `/v1/health/connect/` and `/v1/health/ingress/`. --- .changelog/17424.txt | 3 +++ agent/consul/health_endpoint.go | 19 ++++++++----------- agent/consul/health_endpoint_test.go | 7 +++---- .../docs/upgrading/upgrade-specific.mdx | 10 ++++++++++ 4 files changed, 24 insertions(+), 15 deletions(-) create mode 100644 .changelog/17424.txt diff --git a/.changelog/17424.txt b/.changelog/17424.txt new file mode 100644 index 0000000000..cd35f9aa33 --- /dev/null +++ b/.changelog/17424.txt @@ -0,0 +1,3 @@ +```release-note:breaking-change +api: The `/v1/health/connect/` and `/v1/health/ingress/` endpoints now immediately return 403 "Permission Denied" errors whenever a token with insufficient `service:read` permissions is provided. Prior to this change, the endpoints returned a success code with an empty result list when a token with insufficient permissions was provided. +``` diff --git a/agent/consul/health_endpoint.go b/agent/consul/health_endpoint.go index 945b3b2eb8..c1286cce17 100644 --- a/agent/consul/health_endpoint.go +++ b/agent/consul/health_endpoint.go @@ -226,6 +226,14 @@ func (h *Health) ServiceNodes(args *structs.ServiceSpecificRequest, reply *struc return err } + // If we're doing a connect or ingress query, we need read access to the service + // we're trying to find proxies for, so check that. + if args.Connect || args.Ingress { + if authz.ServiceRead(args.ServiceName, &authzContext) != acl.Allow { + return acl.ErrPermissionDenied + } + } + filter, err := bexpr.CreateFilter(args.Filter, nil, reply.Nodes) if err != nil { return err @@ -247,17 +255,6 @@ func (h *Health) ServiceNodes(args *structs.ServiceSpecificRequest, reply *struc return err } - // If we're doing a connect or ingress query, we need read access to the service - // we're trying to find proxies for, so check that. - if args.Connect || args.Ingress { - // TODO(acl-error-enhancements) Look for ways to percolate this information up to give any feedback to the user. - if authz.ServiceRead(args.ServiceName, &authzContext) != acl.Allow { - // Return the index here so that the agent cache does not infinitely loop. - reply.Index = index - return nil - } - } - resolvedNodes := nodes if args.MergeCentralConfig { for _, node := range resolvedNodes { diff --git a/agent/consul/health_endpoint_test.go b/agent/consul/health_endpoint_test.go index 1981332d86..cd37b5ec4c 100644 --- a/agent/consul/health_endpoint_test.go +++ b/agent/consul/health_endpoint_test.go @@ -1125,9 +1125,8 @@ node "foo" { QueryOptions: structs.QueryOptions{Token: token}, } var resp structs.IndexedCheckServiceNodes - assert.Nil(t, msgpackrpc.CallWithCodec(codec, "Health.ServiceNodes", &req, &resp)) + assert.ErrorContains(t, msgpackrpc.CallWithCodec(codec, "Health.ServiceNodes", &req, &resp), "Permission denied") assert.Len(t, resp.Nodes, 0) - assert.Greater(t, resp.Index, uint64(0)) // List w/ token. This should work since we're requesting "foo", but should // also only contain the proxies with names that adhere to our ACL. @@ -1465,7 +1464,7 @@ func TestHealth_ServiceNodes_Ingress_ACL(t *testing.T) { ServiceName: "db", Ingress: true, } - require.Nil(t, msgpackrpc.CallWithCodec(codec, "Health.ServiceNodes", &req, &out2)) + require.ErrorContains(t, msgpackrpc.CallWithCodec(codec, "Health.ServiceNodes", &req, &out2), "Permission denied") require.Len(t, out2.Nodes, 0) // Requesting a service that is not covered by the token's policy @@ -1475,7 +1474,7 @@ func TestHealth_ServiceNodes_Ingress_ACL(t *testing.T) { Ingress: true, QueryOptions: structs.QueryOptions{Token: token.SecretID}, } - require.Nil(t, msgpackrpc.CallWithCodec(codec, "Health.ServiceNodes", &req, &out2)) + require.ErrorContains(t, msgpackrpc.CallWithCodec(codec, "Health.ServiceNodes", &req, &out2), "Permission denied") require.Len(t, out2.Nodes, 0) // Requesting service covered by the token's policy diff --git a/website/content/docs/upgrading/upgrade-specific.mdx b/website/content/docs/upgrading/upgrade-specific.mdx index 6a18a604be..62e466d668 100644 --- a/website/content/docs/upgrading/upgrade-specific.mdx +++ b/website/content/docs/upgrading/upgrade-specific.mdx @@ -16,6 +16,16 @@ upgrade flow. ## Consul 1.16.x +#### API health endpoints return different status code + +Consul versions 1.16.0+ now return an error 403 "Permission denied" status +whenever the `/v1/health/connect/` and `/v1/health/ingress/` endpoints are +queried with insufficient ACL `service:read` privileges. + +In Consul versions older than 1.16.0, the service health API did not return an explicit error when given a token with invalid permissions. Instead, it returned an empty list with a success status code. + +Before upgrading, ensure that all of your applications can handle this new API behavior properly. An update is not required unless you have a custom application that is querying one of these API endpoints directly. + #### Remove deprecated service-defaults peer upstream override behavior When configuring a service defaults configuration entry, the [`UpstreamConfig.Overrides` configuration](/consul/docs/connect/config-entries/service-defaults#upstreamconfig-overrides)