luxolus
/
open-consul
2
0
Fork 0
Code Issues 1 Pull Requests Packages Releases Wiki Activity

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/`.
This commit is contained in:
Derek Menteer 2023-05-24 16:35:55 -05:00 committed by GitHub
parent f94f54a224
commit 1d42274870
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
4 changed files with 24 additions and 15 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

3
.changelog/17424.txt Normal file
View File

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

19
agent/consul/health_endpoint.go
View File

@ -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 {

7
agent/consul/health_endpoint_test.go
View File

@ -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

10
website/content/docs/upgrading/upgrade-specific.mdx
View File

@ -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)