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

@@ -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.
+```

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 {

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

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)