Handle resolving proxy tokens when parsing HTTP requests (#4453)

Fixes: #4441

This fixes the issue with Connect Managed Proxies + ACLs being broken.

The underlying problem was that the token parsed for most http endpoints was sent untouched to the servers via the RPC request. These changes make it so that at the HTTP endpoint when parsing the token we additionally attempt to convert potential proxy tokens into regular tokens before sending to the RPC endpoint. Proxy tokens are only valid on the agent with the managed proxy so the resolution has to happen before it gets forwarded anywhere.

agent/agent_endpoint.go

@@ -944,14 +944,22 @@ func (s *HTTPServer) AgentConnectCALeafCert(resp http.ResponseWriter, req *http.
 		Service: serviceName, // Need name not ID
 	}
 	var qOpts structs.QueryOptions
+
 	// Store DC in the ConnectCALeafRequest but query opts separately
-	if done := s.parse(resp, req, &args.Datacenter, &qOpts); done {
+	// Don't resolve a proxy token to a real token that will be
+	// done with a call to verifyProxyToken later along with
+	// other security relevant checks.
+	if done := s.parseWithoutResolvingProxyToken(resp, req, &args.Datacenter, &qOpts); done {
 		return nil, nil
 	}
 	args.MinQueryIndex = qOpts.MinQueryIndex
 
 	// Verify the proxy token. This will check both the local proxy token
-	// as well as the ACL if the token isn't local.
+	// as well as the ACL if the token isn't local. The checks done in
+	// verifyProxyToken are still relevant because a leaf cert can be cached
+	// verifying the proxy token matches the service id or that a real
+	// acl token still is valid and has ServiceWrite is necessary or
+	// that cached cert is potentially unprotected.
 	effectiveToken, _, err := s.agent.verifyProxyToken(qOpts.Token, serviceName, "")
 	if err != nil {
 		return nil, err
@@ -989,9 +997,11 @@ func (s *HTTPServer) AgentConnectProxyConfig(resp http.ResponseWriter, req *http
 		return nil, nil
 	}
 
-	// Parse the token
+	// Parse the token - don't resolve a proxy token to a real token
+	// that will be done with a call to verifyProxyToken later along with
+	// other security relevant checks.
 	var token string
-	s.parseToken(req, &token)
+	s.parseTokenWithoutResolvingProxyToken(req, &token)
 
 	// Parse hash specially since it's only this endpoint that uses it currently.
 	// Eventually this should happen in parseWait and end up in QueryOptions but I
@@ -1018,7 +1028,12 @@ func (s *HTTPServer) AgentConnectProxyConfig(resp http.ResponseWriter, req *http
 				return "", nil, nil
 			}
 
-			// Validate the ACL token
+			// Validate the ACL token - because this endpoint uses data local to a single
+			// agent, this function is responsible for all enforcement regarding
+			// protection of the configuration. verifyProxyToken will match the proxies
+			// token to the correct service or in the case of being provide a real ACL
+			// token it will ensure that the requester has ServiceWrite privileges
+			// for this service.
 			_, isProxyToken, err := s.agent.verifyProxyToken(token, target.Service, id)
 			if err != nil {
 				return "", nil, err

agent/http.go

@@ -563,22 +563,43 @@ func (s *HTTPServer) parseDC(req *http.Request, dc *string) {
 	}
 }
 
-// parseToken is used to parse the ?token query param or the X-Consul-Token header
-func (s *HTTPServer) parseToken(req *http.Request, token *string) {
+// parseTokenInternal is used to parse the ?token query param or the X-Consul-Token header and
+// optionally resolve proxy tokens to real ACL tokens. If no token is specified it will populate
+// the token with the agents UserToken (acl_token in the consul configuration)
+func (s *HTTPServer) parseTokenInternal(req *http.Request, token *string, resolveProxyToken bool) {
+	tok := ""
 	if other := req.URL.Query().Get("token"); other != "" {
-		*token = other
-		return
+		tok = other
+	} else if other := req.Header.Get("X-Consul-Token"); other != "" {
+		tok = other
 	}
 
-	if other := req.Header.Get("X-Consul-Token"); other != "" {
-		*token = other
+	if tok != "" {
+		if resolveProxyToken {
+			if p := s.agent.resolveProxyToken(tok); p != nil {
+				*token = s.agent.State.ServiceToken(p.Proxy.TargetServiceID)
+				return
+			}
+		}
+
+		*token = tok
 		return
 	}
 
-	// Set the default ACLToken
 	*token = s.agent.tokens.UserToken()
 }
 
+// parseToken is used to parse the ?token query param or the X-Consul-Token header and
+// resolve proxy tokens to real ACL tokens
+func (s *HTTPServer) parseToken(req *http.Request, token *string) {
+	s.parseTokenInternal(req, token, true)
+}
+
+// parseTokenWithoutResolvingProxyToken is used to parse the ?token query param or the X-Consul-Token header
+func (s *HTTPServer) parseTokenWithoutResolvingProxyToken(req *http.Request, token *string) {
+	s.parseTokenInternal(req, token, false)
+}
+
 func sourceAddrFromRequest(req *http.Request) string {
 	xff := req.Header.Get("X-Forwarded-For")
 	forwardHosts := strings.Split(xff, ",")
@@ -631,13 +652,24 @@ func (s *HTTPServer) parseMetaFilter(req *http.Request) map[string]string {
 	return nil
 }
 
-// parse is a convenience method for endpoints that need
+// parseInternal is a convenience method for endpoints that need
 // to use both parseWait and parseDC.
-func (s *HTTPServer) parse(resp http.ResponseWriter, req *http.Request, dc *string, b *structs.QueryOptions) bool {
+func (s *HTTPServer) parseInternal(resp http.ResponseWriter, req *http.Request, dc *string, b *structs.QueryOptions, resolveProxyToken bool) bool {
 	s.parseDC(req, dc)
-	s.parseToken(req, &b.Token)
+	s.parseTokenInternal(req, &b.Token, resolveProxyToken)
 	if s.parseConsistency(resp, req, b) {
 		return true
 	}
 	return parseWait(resp, req, b)
 }
+
+// parse is a convenience method for endpoints that need
+// to use both parseWait and parseDC.
+func (s *HTTPServer) parse(resp http.ResponseWriter, req *http.Request, dc *string, b *structs.QueryOptions) bool {
+	return s.parseInternal(resp, req, dc, b, true)
+}
+
+// parseWithoutResolvingProxyToken is a convenience method similar to parse except that it disables resolving proxy tokens
+func (s *HTTPServer) parseWithoutResolvingProxyToken(resp http.ResponseWriter, req *http.Request, dc *string, b *structs.QueryOptions) bool {
+	return s.parseInternal(resp, req, dc, b, false)
+}

agent/http_test.go

@@ -22,6 +22,7 @@ import (
 	"github.com/hashicorp/consul/api"
 	"github.com/hashicorp/consul/testutil"
 	"github.com/hashicorp/go-cleanhttp"
+	"github.com/stretchr/testify/require"
 	"golang.org/x/net/http2"
 )
 
@@ -786,6 +787,97 @@ func TestEnableWebUI(t *testing.T) {
 	}
 }
 
+func TestParseToken_ProxyTokenResolve(t *testing.T) {
+	t.Parallel()
+
+	type endpointCheck struct {
+		endpoint string
+		handler  func(s *HTTPServer, resp http.ResponseWriter, req *http.Request) (interface{}, error)
+	}
+
+	// This is not an exhaustive list of all of our endpoints and is only testing GET endpoints
+	// right now. However it provides decent coverage that the proxy token resolution
+	// is happening properly
+	tests := []endpointCheck{
+		{"/v1/acl/info/root", (*HTTPServer).ACLGet},
+		{"/v1/agent/self", (*HTTPServer).AgentSelf},
+		{"/v1/agent/metrics", (*HTTPServer).AgentMetrics},
+		{"/v1/agent/services", (*HTTPServer).AgentServices},
+		{"/v1/agent/checks", (*HTTPServer).AgentChecks},
+		{"/v1/agent/members", (*HTTPServer).AgentMembers},
+		{"/v1/agent/connect/ca/roots", (*HTTPServer).AgentConnectCARoots},
+		{"/v1/agent/connect/ca/leaf/test", (*HTTPServer).AgentConnectCALeafCert},
+		{"/v1/agent/connect/ca/proxy/test", (*HTTPServer).AgentConnectProxyConfig},
+		{"/v1/catalog/connect", (*HTTPServer).CatalogConnectServiceNodes},
+		{"/v1/catalog/datacenters", (*HTTPServer).CatalogDatacenters},
+		{"/v1/catalog/nodes", (*HTTPServer).CatalogNodes},
+		{"/v1/catalog/node/" + t.Name(), (*HTTPServer).CatalogNodeServices},
+		{"/v1/catalog/services", (*HTTPServer).CatalogServices},
+		{"/v1/catalog/service/test", (*HTTPServer).CatalogServiceNodes},
+		{"/v1/connect/ca/configuration", (*HTTPServer).ConnectCAConfiguration},
+		{"/v1/connect/ca/roots", (*HTTPServer).ConnectCARoots},
+		{"/v1/connect/intentions", (*HTTPServer).IntentionEndpoint},
+		{"/v1/coordinate/datacenters", (*HTTPServer).CoordinateDatacenters},
+		{"/v1/coordinate/nodes", (*HTTPServer).CoordinateNodes},
+		{"/v1/coordinate/node/" + t.Name(), (*HTTPServer).CoordinateNode},
+		{"/v1/event/list", (*HTTPServer).EventList},
+		{"/v1/health/node/" + t.Name(), (*HTTPServer).HealthNodeChecks},
+		{"/v1/health/checks/test", (*HTTPServer).HealthNodeChecks},
+		{"/v1/health/state/passing", (*HTTPServer).HealthChecksInState},
+		{"/v1/health/service/test", (*HTTPServer).HealthServiceNodes},
+		{"/v1/health/connect/test", (*HTTPServer).HealthConnectServiceNodes},
+		{"/v1/operator/raft/configuration", (*HTTPServer).OperatorRaftConfiguration},
+		// keyring endpoint has issues with returning errors if you haven't enabled encryption
+		// {"/v1/operator/keyring", (*HTTPServer).OperatorKeyringEndpoint},
+		{"/v1/operator/autopilot/configuration", (*HTTPServer).OperatorAutopilotConfiguration},
+		{"/v1/operator/autopilot/health", (*HTTPServer).OperatorServerHealth},
+		{"/v1/query", (*HTTPServer).PreparedQueryGeneral},
+		{"/v1/session/list", (*HTTPServer).SessionList},
+		{"/v1/status/leader", (*HTTPServer).StatusLeader},
+		{"/v1/status/peers", (*HTTPServer).StatusPeers},
+	}
+
+	a := NewTestAgent(t.Name(), TestACLConfig()+testAllowProxyConfig())
+	defer a.Shutdown()
+
+	// Register a service with a managed proxy
+	{
+		reg := &structs.ServiceDefinition{
+			ID:      "test-id",
+			Name:    "test",
+			Address: "127.0.0.1",
+			Port:    8000,
+			Check: structs.CheckType{
+				TTL: 15 * time.Second,
+			},
+			Connect: &structs.ServiceConnect{
+				Proxy: &structs.ServiceDefinitionConnectProxy{},
+			},
+		}
+
+		req, _ := http.NewRequest("PUT", "/v1/agent/service/register?token=root", jsonReader(reg))
+		resp := httptest.NewRecorder()
+		_, err := a.srv.AgentRegisterService(resp, req)
+		require.NoError(t, err)
+		require.Equal(t, 200, resp.Code, "body: %s", resp.Body.String())
+	}
+
+	// Get the proxy token from the agent directly, since there is no API.
+	proxy := a.State.Proxy("test-id-proxy")
+	require.NotNil(t, proxy)
+	token := proxy.ProxyToken
+	require.NotEmpty(t, token)
+
+	for _, check := range tests {
+		t.Run(fmt.Sprintf("GET(%s)", check.endpoint), func(t *testing.T) {
+			req, _ := http.NewRequest("GET", fmt.Sprintf("%s?token=%s", check.endpoint, token), nil)
+			resp := httptest.NewRecorder()
+			_, err := check.handler(a.srv, resp, req)
+			require.NoError(t, err)
+		})
+	}
+}
+
 // assertIndex tests that X-Consul-Index is set and non-zero
 func assertIndex(t *testing.T, resp *httptest.ResponseRecorder) {
 	header := resp.Header().Get("X-Consul-Index")