From c2d167d06e0689f0ef6fb7867802bd24de559139 Mon Sep 17 00:00:00 2001 From: "R.B. Boyer" Date: Wed, 4 Nov 2020 12:50:03 -0600 Subject: [PATCH] agent: protect the ui metrics proxy endpoint behind ACLs (#9099) This ensures the metrics proxy endpoint is ACL protected behind a wildcard `service:read` and `node:read` set of rules. For Consul Enterprise these will need to span all namespaces: ``` service_prefix "" { policy = "read" } node_prefix "" { policy = "read" } namespace_prefix "" { service_prefix "" { policy = "read" } node_prefix "" { policy = "read" } } ``` This PR contains just the backend changes. The frontend changes to actually pass the consul token header to the proxy through the JS plugin will come in another PR. --- acl/acl_test.go | 112 +++++++++++++++++++++++ acl/authorizer.go | 6 ++ acl/authorizer_test.go | 12 +++ acl/chained_authorizer.go | 12 +++ acl/chained_authorizer_test.go | 8 ++ acl/policy_authorizer.go | 10 ++- acl/static_authorizer.go | 14 +++ agent/http.go | 35 ++++++-- agent/ui_endpoint.go | 28 ++++++ agent/ui_endpoint_oss_test.go | 159 +++++++++++++++++++++++++++++++++ 10 files changed, 387 insertions(+), 9 deletions(-) create mode 100644 agent/ui_endpoint_oss_test.go diff --git a/acl/acl_test.go b/acl/acl_test.go index 8e50f75f5..becc3b709 100644 --- a/acl/acl_test.go +++ b/acl/acl_test.go @@ -100,6 +100,10 @@ func checkAllowNodeRead(t *testing.T, authz Authorizer, prefix string, entCtx *A require.Equal(t, Allow, authz.NodeRead(prefix, entCtx)) } +func checkAllowNodeReadAll(t *testing.T, authz Authorizer, _ string, entCtx *AuthorizerContext) { + require.Equal(t, Allow, authz.NodeReadAll(entCtx)) +} + func checkAllowNodeWrite(t *testing.T, authz Authorizer, prefix string, entCtx *AuthorizerContext) { require.Equal(t, Allow, authz.NodeWrite(prefix, entCtx)) } @@ -124,6 +128,10 @@ func checkAllowServiceRead(t *testing.T, authz Authorizer, prefix string, entCtx require.Equal(t, Allow, authz.ServiceRead(prefix, entCtx)) } +func checkAllowServiceReadAll(t *testing.T, authz Authorizer, _ string, entCtx *AuthorizerContext) { + require.Equal(t, Allow, authz.ServiceReadAll(entCtx)) +} + func checkAllowServiceWrite(t *testing.T, authz Authorizer, prefix string, entCtx *AuthorizerContext) { require.Equal(t, Allow, authz.ServiceWrite(prefix, entCtx)) } @@ -204,6 +212,10 @@ func checkDenyNodeRead(t *testing.T, authz Authorizer, prefix string, entCtx *Au require.Equal(t, Deny, authz.NodeRead(prefix, entCtx)) } +func checkDenyNodeReadAll(t *testing.T, authz Authorizer, _ string, entCtx *AuthorizerContext) { + require.Equal(t, Deny, authz.NodeReadAll(entCtx)) +} + func checkDenyNodeWrite(t *testing.T, authz Authorizer, prefix string, entCtx *AuthorizerContext) { require.Equal(t, Deny, authz.NodeWrite(prefix, entCtx)) } @@ -228,6 +240,10 @@ func checkDenyServiceRead(t *testing.T, authz Authorizer, prefix string, entCtx require.Equal(t, Deny, authz.ServiceRead(prefix, entCtx)) } +func checkDenyServiceReadAll(t *testing.T, authz Authorizer, _ string, entCtx *AuthorizerContext) { + require.Equal(t, Deny, authz.ServiceReadAll(entCtx)) +} + func checkDenyServiceWrite(t *testing.T, authz Authorizer, prefix string, entCtx *AuthorizerContext) { require.Equal(t, Deny, authz.ServiceWrite(prefix, entCtx)) } @@ -308,6 +324,10 @@ func checkDefaultNodeRead(t *testing.T, authz Authorizer, prefix string, entCtx require.Equal(t, Default, authz.NodeRead(prefix, entCtx)) } +func checkDefaultNodeReadAll(t *testing.T, authz Authorizer, _ string, entCtx *AuthorizerContext) { + require.Equal(t, Default, authz.NodeReadAll(entCtx)) +} + func checkDefaultNodeWrite(t *testing.T, authz Authorizer, prefix string, entCtx *AuthorizerContext) { require.Equal(t, Default, authz.NodeWrite(prefix, entCtx)) } @@ -332,6 +352,10 @@ func checkDefaultServiceRead(t *testing.T, authz Authorizer, prefix string, entC require.Equal(t, Default, authz.ServiceRead(prefix, entCtx)) } +func checkDefaultServiceReadAll(t *testing.T, authz Authorizer, _ string, entCtx *AuthorizerContext) { + require.Equal(t, Default, authz.ServiceReadAll(entCtx)) +} + func checkDefaultServiceWrite(t *testing.T, authz Authorizer, prefix string, entCtx *AuthorizerContext) { require.Equal(t, Default, authz.ServiceWrite(prefix, entCtx)) } @@ -381,12 +405,14 @@ func TestACL(t *testing.T) { {name: "DenyKeyringWrite", check: checkDenyKeyringWrite}, {name: "DenyKeyWrite", check: checkDenyKeyWrite}, {name: "DenyNodeRead", check: checkDenyNodeRead}, + {name: "DenyNodeReadAll", check: checkDenyNodeReadAll}, {name: "DenyNodeWrite", check: checkDenyNodeWrite}, {name: "DenyOperatorRead", check: checkDenyOperatorRead}, {name: "DenyOperatorWrite", check: checkDenyOperatorWrite}, {name: "DenyPreparedQueryRead", check: checkDenyPreparedQueryRead}, {name: "DenyPreparedQueryWrite", check: checkDenyPreparedQueryWrite}, {name: "DenyServiceRead", check: checkDenyServiceRead}, + {name: "DenyServiceReadAll", check: checkDenyServiceReadAll}, {name: "DenyServiceWrite", check: checkDenyServiceWrite}, {name: "DenySessionRead", check: checkDenySessionRead}, {name: "DenySessionWrite", check: checkDenySessionWrite}, @@ -411,12 +437,14 @@ func TestACL(t *testing.T) { {name: "AllowKeyringWrite", check: checkAllowKeyringWrite}, {name: "AllowKeyWrite", check: checkAllowKeyWrite}, {name: "AllowNodeRead", check: checkAllowNodeRead}, + {name: "AllowNodeReadAll", check: checkAllowNodeReadAll}, {name: "AllowNodeWrite", check: checkAllowNodeWrite}, {name: "AllowOperatorRead", check: checkAllowOperatorRead}, {name: "AllowOperatorWrite", check: checkAllowOperatorWrite}, {name: "AllowPreparedQueryRead", check: checkAllowPreparedQueryRead}, {name: "AllowPreparedQueryWrite", check: checkAllowPreparedQueryWrite}, {name: "AllowServiceRead", check: checkAllowServiceRead}, + {name: "AllowServiceReadAll", check: checkAllowServiceReadAll}, {name: "AllowServiceWrite", check: checkAllowServiceWrite}, {name: "AllowSessionRead", check: checkAllowSessionRead}, {name: "AllowSessionWrite", check: checkAllowSessionWrite}, @@ -441,12 +469,14 @@ func TestACL(t *testing.T) { {name: "AllowKeyringWrite", check: checkAllowKeyringWrite}, {name: "AllowKeyWrite", check: checkAllowKeyWrite}, {name: "AllowNodeRead", check: checkAllowNodeRead}, + {name: "AllowNodeReadAll", check: checkAllowNodeReadAll}, {name: "AllowNodeWrite", check: checkAllowNodeWrite}, {name: "AllowOperatorRead", check: checkAllowOperatorRead}, {name: "AllowOperatorWrite", check: checkAllowOperatorWrite}, {name: "AllowPreparedQueryRead", check: checkAllowPreparedQueryRead}, {name: "AllowPreparedQueryWrite", check: checkAllowPreparedQueryWrite}, {name: "AllowServiceRead", check: checkAllowServiceRead}, + {name: "AllowServiceReadAll", check: checkAllowServiceReadAll}, {name: "AllowServiceWrite", check: checkAllowServiceWrite}, {name: "AllowSessionRead", check: checkAllowSessionRead}, {name: "AllowSessionWrite", check: checkAllowSessionWrite}, @@ -995,6 +1025,7 @@ func TestACL(t *testing.T) { }), }, checks: []aclCheck{ + {name: "ReadAllDenied", prefix: "", check: checkDenyNodeReadAll}, {name: "DefaultReadDenied", prefix: "nope", check: checkDenyNodeRead}, {name: "DefaultWriteDenied", prefix: "nope", check: checkDenyNodeWrite}, {name: "DenyReadDenied", prefix: "root-nope", check: checkDenyNodeRead}, @@ -1075,6 +1106,7 @@ func TestACL(t *testing.T) { }), }, checks: []aclCheck{ + {name: "ReadAllDenied", prefix: "", check: checkDenyNodeReadAll}, {name: "DefaultReadAllowed", prefix: "nope", check: checkAllowNodeRead}, {name: "DefaultWriteAllowed", prefix: "nope", check: checkAllowNodeWrite}, {name: "DenyReadDenied", prefix: "root-nope", check: checkDenyNodeRead}, @@ -1335,6 +1367,7 @@ func TestACL(t *testing.T) { }), }, checks: []aclCheck{ + {name: "ServiceReadAllDenied", prefix: "", check: checkDenyServiceReadAll}, {name: "KeyReadDenied", prefix: "other", check: checkDenyKeyRead}, {name: "KeyWriteDenied", prefix: "other", check: checkDenyKeyWrite}, {name: "KeyWritePrefixDenied", prefix: "other", check: checkDenyKeyWritePrefix}, @@ -1464,6 +1497,7 @@ func TestACL(t *testing.T) { }), }, checks: []aclCheck{ + {name: "ServiceReadAllDenied", prefix: "", check: checkDenyServiceReadAll}, {name: "KeyReadAllowed", prefix: "other", check: checkAllowKeyRead}, {name: "KeyWriteAllowed", prefix: "other", check: checkAllowKeyWrite}, {name: "KeyWritePrefixAllowed", prefix: "other", check: checkAllowKeyWritePrefix}, @@ -1708,6 +1742,9 @@ func TestACL(t *testing.T) { }, }, checks: []aclCheck{ + {name: "NodeReadAllDenied", prefix: "", check: checkDenyNodeReadAll}, + {name: "ServiceReadAllDenied", prefix: "", check: checkDenyServiceReadAll}, + {name: "AgentReadPrefixAllowed", prefix: "fo", check: checkAllowAgentRead}, {name: "AgentWritePrefixDenied", prefix: "fo", check: checkDenyAgentWrite}, {name: "AgentReadPrefixAllowed", prefix: "for", check: checkAllowAgentRead}, @@ -2101,3 +2138,78 @@ func TestACLEnforce(t *testing.T) { }) } } + +func TestACL_ReadAll(t *testing.T) { + type testcase struct { + name string + rules string + check func(t *testing.T, authz Authorizer, prefix string, entCtx *AuthorizerContext) + } + + tests := []testcase{ + { + name: "node:bar:read", + rules: `node "bar" { policy = "read" }`, + check: checkDenyNodeReadAll, + }, + { + name: "node:bar:write", + rules: `node "bar" { policy = "write" }`, + check: checkDenyNodeReadAll, + }, + { + name: "node:*:read", + rules: `node_prefix "" { policy = "read" }`, + check: checkAllowNodeReadAll, + }, + { + name: "node:*:write", + rules: `node_prefix "" { policy = "write" }`, + check: checkAllowNodeReadAll, + }, + { + name: "service:bar:read", + rules: `service "bar" { policy = "read" }`, + check: checkDenyServiceReadAll, + }, + { + name: "service:bar:write", + rules: `service "bar" { policy = "write" }`, + check: checkDenyServiceReadAll, + }, + { + name: "service:*:read", + rules: `service_prefix "" { policy = "read" }`, + check: checkAllowServiceReadAll, + }, + { + name: "service:*:write", + rules: `service_prefix "" { policy = "write" }`, + check: checkAllowServiceReadAll, + }, + } + + body := func(t *testing.T, rules string, defaultPolicy Authorizer, check func(t *testing.T, authz Authorizer, prefix string, entCtx *AuthorizerContext)) { + t.Helper() + + policy, err := NewPolicyFromSource("", 0, rules, SyntaxCurrent, nil, nil) + require.NoError(t, err) + + acl, err := NewPolicyAuthorizerWithDefaults(defaultPolicy, []*Policy{policy}, nil) + require.NoError(t, err) + + check(t, acl, "", nil) + } + + for _, tc := range tests { + tc := tc + t.Run(tc.name, func(t *testing.T) { + t.Run("default deny", func(t *testing.T) { + body(t, tc.rules, DenyAll(), tc.check) + }) + t.Run("default allow", func(t *testing.T) { + body(t, tc.rules, AllowAll(), checkAllowNodeReadAll) + }) + }) + } +} diff --git a/acl/authorizer.go b/acl/authorizer.go index fb3abcc34..9693786f0 100644 --- a/acl/authorizer.go +++ b/acl/authorizer.go @@ -107,6 +107,9 @@ type Authorizer interface { // NodeRead checks for permission to read (discover) a given node. NodeRead(string, *AuthorizerContext) EnforcementDecision + // NodeReadAll checks for permission to read (discover) all nodes. + NodeReadAll(*AuthorizerContext) EnforcementDecision + // NodeWrite checks for permission to create or update (register) a // given node. NodeWrite(string, *AuthorizerContext) EnforcementDecision @@ -130,6 +133,9 @@ type Authorizer interface { // ServiceRead checks for permission to read a given service ServiceRead(string, *AuthorizerContext) EnforcementDecision + // ServiceReadAll checks for permission to read all services + ServiceReadAll(*AuthorizerContext) EnforcementDecision + // ServiceWrite checks for permission to create or update a given // service ServiceWrite(string, *AuthorizerContext) EnforcementDecision diff --git a/acl/authorizer_test.go b/acl/authorizer_test.go index 8ce3ce516..b5534b00c 100644 --- a/acl/authorizer_test.go +++ b/acl/authorizer_test.go @@ -12,6 +12,8 @@ type mockAuthorizer struct { mock.Mock } +var _ Authorizer = (*mockAuthorizer)(nil) + // ACLRead checks for permission to list all the ACLs func (m *mockAuthorizer) ACLRead(ctx *AuthorizerContext) EnforcementDecision { ret := m.Called(ctx) @@ -115,6 +117,11 @@ func (m *mockAuthorizer) NodeRead(segment string, ctx *AuthorizerContext) Enforc return ret.Get(0).(EnforcementDecision) } +func (m *mockAuthorizer) NodeReadAll(ctx *AuthorizerContext) EnforcementDecision { + ret := m.Called(ctx) + return ret.Get(0).(EnforcementDecision) +} + // NodeWrite checks for permission to create or update (register) a // given node. func (m *mockAuthorizer) NodeWrite(segment string, ctx *AuthorizerContext) EnforcementDecision { @@ -156,6 +163,11 @@ func (m *mockAuthorizer) ServiceRead(segment string, ctx *AuthorizerContext) Enf return ret.Get(0).(EnforcementDecision) } +func (m *mockAuthorizer) ServiceReadAll(ctx *AuthorizerContext) EnforcementDecision { + ret := m.Called(ctx) + return ret.Get(0).(EnforcementDecision) +} + // ServiceWrite checks for permission to create or update a given // service func (m *mockAuthorizer) ServiceWrite(segment string, ctx *AuthorizerContext) EnforcementDecision { diff --git a/acl/chained_authorizer.go b/acl/chained_authorizer.go index 66ab18cd3..391563003 100644 --- a/acl/chained_authorizer.go +++ b/acl/chained_authorizer.go @@ -152,6 +152,12 @@ func (c *ChainedAuthorizer) NodeRead(node string, entCtx *AuthorizerContext) Enf }) } +func (c *ChainedAuthorizer) NodeReadAll(entCtx *AuthorizerContext) EnforcementDecision { + return c.executeChain(func(authz Authorizer) EnforcementDecision { + return authz.NodeReadAll(entCtx) + }) +} + // NodeWrite checks for permission to create or update (register) a // given node. func (c *ChainedAuthorizer) NodeWrite(node string, entCtx *AuthorizerContext) EnforcementDecision { @@ -199,6 +205,12 @@ func (c *ChainedAuthorizer) ServiceRead(name string, entCtx *AuthorizerContext) }) } +func (c *ChainedAuthorizer) ServiceReadAll(entCtx *AuthorizerContext) EnforcementDecision { + return c.executeChain(func(authz Authorizer) EnforcementDecision { + return authz.ServiceReadAll(entCtx) + }) +} + // ServiceWrite checks for permission to create or update a given // service func (c *ChainedAuthorizer) ServiceWrite(name string, entCtx *AuthorizerContext) EnforcementDecision { diff --git a/acl/chained_authorizer_test.go b/acl/chained_authorizer_test.go index 155d9d808..4ff7b2e4f 100644 --- a/acl/chained_authorizer_test.go +++ b/acl/chained_authorizer_test.go @@ -6,6 +6,8 @@ import ( type testAuthorizer EnforcementDecision +var _ Authorizer = testAuthorizer(Allow) + func (authz testAuthorizer) ACLRead(*AuthorizerContext) EnforcementDecision { return EnforcementDecision(authz) } @@ -54,6 +56,9 @@ func (authz testAuthorizer) KeyringWrite(*AuthorizerContext) EnforcementDecision func (authz testAuthorizer) NodeRead(string, *AuthorizerContext) EnforcementDecision { return EnforcementDecision(authz) } +func (authz testAuthorizer) NodeReadAll(*AuthorizerContext) EnforcementDecision { + return EnforcementDecision(authz) +} func (authz testAuthorizer) NodeWrite(string, *AuthorizerContext) EnforcementDecision { return EnforcementDecision(authz) } @@ -72,6 +77,9 @@ func (authz testAuthorizer) PreparedQueryWrite(string, *AuthorizerContext) Enfor func (authz testAuthorizer) ServiceRead(string, *AuthorizerContext) EnforcementDecision { return EnforcementDecision(authz) } +func (authz testAuthorizer) ServiceReadAll(*AuthorizerContext) EnforcementDecision { + return EnforcementDecision(authz) +} func (authz testAuthorizer) ServiceWrite(string, *AuthorizerContext) EnforcementDecision { return EnforcementDecision(authz) } diff --git a/acl/policy_authorizer.go b/acl/policy_authorizer.go index 01f688488..af52418c2 100644 --- a/acl/policy_authorizer.go +++ b/acl/policy_authorizer.go @@ -350,7 +350,7 @@ type enforceCallback func(raw interface{}, prefixOnly bool) EnforcementDecision func anyAllowed(tree *radix.Tree, enforceFn enforceCallback) EnforcementDecision { decision := Default - // special case for handling a catch-all prefix rule. If the rule woul Deny access then our default decision + // special case for handling a catch-all prefix rule. If the rule would Deny access then our default decision // should be to Deny, but this decision should still be overridable with other more specific rules. if raw, found := tree.Get(""); found { decision = enforceFn(raw, true) @@ -686,6 +686,10 @@ func (p *policyAuthorizer) NodeRead(name string, _ *AuthorizerContext) Enforceme return Default } +func (p *policyAuthorizer) NodeReadAll(_ *AuthorizerContext) EnforcementDecision { + return p.allAllowed(p.nodeRules, AccessRead) +} + // NodeWrite checks if writing (registering) a node is allowed func (p *policyAuthorizer) NodeWrite(name string, _ *AuthorizerContext) EnforcementDecision { if rule, ok := getPolicy(name, p.nodeRules); ok { @@ -720,6 +724,10 @@ func (p *policyAuthorizer) ServiceRead(name string, _ *AuthorizerContext) Enforc return Default } +func (p *policyAuthorizer) ServiceReadAll(_ *AuthorizerContext) EnforcementDecision { + return p.allAllowed(p.serviceRules, AccessRead) +} + // ServiceWrite checks if writing (registering) a service is allowed func (p *policyAuthorizer) ServiceWrite(name string, _ *AuthorizerContext) EnforcementDecision { if rule, ok := getPolicy(name, p.serviceRules); ok { diff --git a/acl/static_authorizer.go b/acl/static_authorizer.go index 2339a2fe2..4523f0636 100644 --- a/acl/static_authorizer.go +++ b/acl/static_authorizer.go @@ -142,6 +142,13 @@ func (s *staticAuthorizer) NodeRead(string, *AuthorizerContext) EnforcementDecis return Deny } +func (s *staticAuthorizer) NodeReadAll(*AuthorizerContext) EnforcementDecision { + if s.defaultAllow { + return Allow + } + return Deny +} + func (s *staticAuthorizer) NodeWrite(string, *AuthorizerContext) EnforcementDecision { if s.defaultAllow { return Allow @@ -184,6 +191,13 @@ func (s *staticAuthorizer) ServiceRead(string, *AuthorizerContext) EnforcementDe return Deny } +func (s *staticAuthorizer) ServiceReadAll(*AuthorizerContext) EnforcementDecision { + if s.defaultAllow { + return Allow + } + return Deny +} + func (s *staticAuthorizer) ServiceWrite(string, *AuthorizerContext) EnforcementDecision { if s.defaultAllow { return Allow diff --git a/agent/http.go b/agent/http.go index dc8cdbc37..10233fa6b 100644 --- a/agent/http.go +++ b/agent/http.go @@ -895,12 +895,26 @@ func (s *HTTPHandlers) parseDC(req *http.Request, dc *string) { // parseTokenInternal is used to parse the ?token query param or the X-Consul-Token header or // Authorization Bearer token (RFC6750). func (s *HTTPHandlers) parseTokenInternal(req *http.Request, token *string) { - tok := "" if other := req.URL.Query().Get("token"); other != "" { - tok = other - } else if other := req.Header.Get("X-Consul-Token"); other != "" { - tok = other - } else if other := req.Header.Get("Authorization"); other != "" { + *token = other + return + } + + if ok := s.parseTokenFromHeaders(req, token); ok { + return + } + + *token = "" + return +} + +func (s *HTTPHandlers) parseTokenFromHeaders(req *http.Request, token *string) bool { + if other := req.Header.Get("X-Consul-Token"); other != "" { + *token = other + return true + } + + if other := req.Header.Get("Authorization"); other != "" { // HTTP Authorization headers are in the format: [SPACE] // Ref. https://tools.ietf.org/html/rfc7236#section-3 parts := strings.Split(other, " ") @@ -916,13 +930,18 @@ func (s *HTTPHandlers) parseTokenInternal(req *http.Request, token *string) { if strings.ToLower(scheme) == "bearer" { // Since Bearer tokens shouldn't contain spaces (rfc6750#section-2.1) // "value" is tokenized, only the first item is used - tok = strings.TrimSpace(strings.Split(value, " ")[0]) + *token = strings.TrimSpace(strings.Split(value, " ")[0]) + return true } } } - *token = tok - return + return false +} + +func (s *HTTPHandlers) clearTokenFromHeaders(req *http.Request) { + req.Header.Del("X-Consul-Token") + req.Header.Del("Authorization") } // parseTokenWithDefault passes through to parseTokenInternal and optionally resolves proxy tokens to real ACL tokens. diff --git a/agent/ui_endpoint.go b/agent/ui_endpoint.go index 603d32436..f08fb9a96 100644 --- a/agent/ui_endpoint.go +++ b/agent/ui_endpoint.go @@ -9,6 +9,7 @@ import ( "sort" "strings" + "github.com/hashicorp/consul/acl" "github.com/hashicorp/consul/agent/config" "github.com/hashicorp/consul/agent/structs" "github.com/hashicorp/consul/api" @@ -566,6 +567,33 @@ func (s *HTTPHandlers) UIMetricsProxy(resp http.ResponseWriter, req *http.Reques return nil, NotFoundError{Reason: "Metrics proxy is not enabled"} } + // Fetch the ACL token, if provided, but ONLY from headers since other + // metrics proxies might use a ?token query string parameter for something. + var token string + s.parseTokenFromHeaders(req, &token) + + // Clear the token from the headers so we don't end up proxying it. + s.clearTokenFromHeaders(req) + + var entMeta structs.EnterpriseMeta + authz, err := s.agent.resolveTokenAndDefaultMeta(token, &entMeta, nil) + if err != nil { + return nil, err + } + + if authz != nil { + // This endpoint requires wildcard read on all services and all nodes. + // + // In enterprise it requires this _in all namespaces_ too. + wildMeta := structs.WildcardEnterpriseMeta() + var authzContext acl.AuthorizerContext + wildMeta.FillAuthzContext(&authzContext) + + if authz.NodeReadAll(&authzContext) != acl.Allow || authz.ServiceReadAll(&authzContext) != acl.Allow { + return nil, acl.ErrPermissionDenied + } + } + log := s.agent.logger.Named(logging.UIMetricsProxy) // Construct the new URL from the path and the base path. Note we do this here diff --git a/agent/ui_endpoint_oss_test.go b/agent/ui_endpoint_oss_test.go new file mode 100644 index 000000000..88bf3f73a --- /dev/null +++ b/agent/ui_endpoint_oss_test.go @@ -0,0 +1,159 @@ +// +build !consulent + +package agent + +import ( + "fmt" + "net/http" + "net/http/httptest" + "sync/atomic" + "testing" + + "github.com/hashicorp/consul/agent/structs" + "github.com/hashicorp/consul/testrpc" + "github.com/stretchr/testify/require" +) + +func TestUIEndpoint_MetricsProxy_ACLDeny(t *testing.T) { + t.Parallel() + + var ( + lastHeadersSent atomic.Value + backendCalled atomic.Value + ) + backend := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + backendCalled.Store(true) + lastHeadersSent.Store(r.Header) + if r.URL.Path == "/some/prefix/ok" { + w.Write([]byte("OK")) + return + } + http.Error(w, "not found on backend", http.StatusNotFound) + })) + defer backend.Close() + + backendURL := backend.URL + "/some/prefix" + + a := NewTestAgent(t, TestACLConfig()+fmt.Sprintf(` + ui_config { + enabled = true + metrics_proxy { + base_url = %q + } + } + http_config { + response_headers { + "Access-Control-Allow-Origin" = "*" + } + } + `, backendURL)) + defer a.Shutdown() + + h := a.srv.handler(true) + + testrpc.WaitForLeader(t, a.RPC, "dc1") + + const endpointPath = "/v1/internal/ui/metrics-proxy" + + // create some ACL things + for name, rules := range map[string]string{ + "one-service": `service "foo" { policy = "read" }`, + "all-services": `service_prefix "" { policy = "read" }`, + "one-node": `node "bar" { policy = "read" }`, + "all-nodes": `node_prefix "" { policy = "read" }`, + } { + req := structs.ACLPolicySetRequest{ + Policy: structs.ACLPolicy{ + Name: name, + Rules: rules, + }, + Datacenter: "dc1", + WriteRequest: structs.WriteRequest{Token: "root"}, + } + var policy structs.ACLPolicy + require.NoError(t, a.RPC("ACL.PolicySet", &req, &policy)) + } + + makeToken := func(t *testing.T, policyNames []string) string { + req := structs.ACLTokenSetRequest{ + ACLToken: structs.ACLToken{}, + Datacenter: "dc1", + WriteRequest: structs.WriteRequest{Token: "root"}, + } + for _, name := range policyNames { + req.ACLToken.Policies = append(req.ACLToken.Policies, structs.ACLTokenPolicyLink{Name: name}) + } + require.Len(t, req.ACLToken.Policies, len(policyNames)) + + var token structs.ACLToken + require.NoError(t, a.RPC("ACL.TokenSet", &req, &token)) + return token.SecretID + } + + type testcase struct { + name string + token string + policies []string + expect int + } + + for _, tc := range []testcase{ + {name: "no token", token: "", expect: http.StatusForbidden}, + {name: "root token", token: "root", expect: http.StatusOK}, + // + {name: "one node", policies: []string{"one-node"}, expect: http.StatusForbidden}, + {name: "all nodes", policies: []string{"all-nodes"}, expect: http.StatusForbidden}, + // + {name: "one service", policies: []string{"one-service"}, expect: http.StatusForbidden}, + {name: "all services", policies: []string{"all-services"}, expect: http.StatusForbidden}, + // + {name: "one service one node", policies: []string{"one-service", "one-node"}, expect: http.StatusForbidden}, + {name: "all services one node", policies: []string{"all-services", "one-node"}, expect: http.StatusForbidden}, + // + {name: "one service all nodes", policies: []string{"one-service", "one-node"}, expect: http.StatusForbidden}, + {name: "all services all nodes", policies: []string{"all-services", "all-nodes"}, expect: http.StatusOK}, + } { + tc := tc + t.Run(tc.name, func(t *testing.T) { + if tc.token == "" { + tc.token = makeToken(t, tc.policies) + } + + t.Run("via query param should not work", func(t *testing.T) { + req := httptest.NewRequest("GET", endpointPath+"/ok?token="+tc.token, nil) + rec := httptest.NewRecorder() + backendCalled.Store(false) + h.ServeHTTP(rec, req) + require.Equal(t, http.StatusForbidden, rec.Code) + + require.False(t, backendCalled.Load().(bool)) + }) + + for _, headerName := range []string{"x-consul-token", "authorization"} { + headerVal := tc.token + if headerName == "authorization" { + headerVal = "bearer " + tc.token + } + + t.Run("via header "+headerName, func(t *testing.T) { + req := httptest.NewRequest("GET", endpointPath+"/ok", nil) + req.Header.Set(headerName, headerVal) + rec := httptest.NewRecorder() + backendCalled.Store(false) + h.ServeHTTP(rec, req) + require.Equal(t, tc.expect, rec.Code) + + headersSent, _ := lastHeadersSent.Load().(http.Header) + if tc.expect == http.StatusOK { + require.True(t, backendCalled.Load().(bool)) + // Ensure we didn't accidentally ship our consul token to the proxy. + require.Empty(t, headersSent.Get("X-Consul-Token")) + require.Empty(t, headersSent.Get("Authorization")) + } else { + require.False(t, backendCalled.Load().(bool)) + } + }) + } + }) + } +}