agent: leaf endpoint accepts name, not service ID

This change is important so that requests can made representing a
service that may not be registered with the same local agent.

agent/agent.go

@@ -2177,21 +2177,22 @@ func (a *Agent) verifyProxyToken(token, targetService, targetProxy string) (stri
 	// that the service name of the matching proxy matches our target
 	// service.
 	if proxy != nil {
-		if proxy.Proxy.TargetServiceID != targetService {
+		// Get the target service since we only have the name. The nil
+		// check below should never be true since a proxy token always
+		// represents the existence of a local service.
+		target := a.State.Service(proxy.Proxy.TargetServiceID)
+		if target == nil {
+			return "", fmt.Errorf("proxy target service not found: %q",
+				proxy.Proxy.TargetServiceID)
+		}
+
+		if target.Service != targetService {
 			return "", acl.ErrPermissionDenied
 		}
 
 		// Resolve the actual ACL token used to register the proxy/service and
 		// return that for use in RPC calls.
-		return a.State.ServiceToken(targetService), nil
+		return a.State.ServiceToken(proxy.Proxy.TargetServiceID), nil
-	}
-
-	// Retrieve the service specified. This should always exist because
-	// we only call this function for proxies and leaf certs and both can
-	// only be called for local services.
-	service := a.State.Service(targetService)
-	if service == nil {
-		return "", fmt.Errorf("unknown service ID: %s", targetService)
 	}
 
 	// Doesn't match, we have to do a full token resolution. The required
@@ -2202,7 +2203,7 @@ func (a *Agent) verifyProxyToken(token, targetService, targetProxy string) (stri
 	if err != nil {
 		return "", err
 	}
-	if rule != nil && !rule.ServiceWrite(service.Service, nil) {
+	if rule != nil && !rule.ServiceWrite(targetService, nil) {
 		return "", acl.ErrPermissionDenied
 	}
 

agent/agent_endpoint.go

@@ -908,16 +908,10 @@ func (s *HTTPServer) AgentConnectCARoots(resp http.ResponseWriter, req *http.Req
 // instance. This supports blocking queries to update the returned bundle.
 func (s *HTTPServer) AgentConnectCALeafCert(resp http.ResponseWriter, req *http.Request) (interface{}, error) {
 	// Get the service ID. Note that this is the ID of a service instance.
-	id := strings.TrimPrefix(req.URL.Path, "/v1/agent/connect/ca/leaf/")
+	serviceName := strings.TrimPrefix(req.URL.Path, "/v1/agent/connect/ca/leaf/")
-
-	// Retrieve the service specified
-	service := s.agent.State.Service(id)
-	if service == nil {
-		return nil, fmt.Errorf("unknown service ID: %s", id)
-	}
 
 	args := cachetype.ConnectCALeafRequest{
-		Service: service.Service, // Need name not ID
+		Service: serviceName, // Need name not ID
 	}
 	var qOpts structs.QueryOptions
 	// Store DC in the ConnectCALeafRequest but query opts separately
@@ -928,7 +922,7 @@ func (s *HTTPServer) AgentConnectCALeafCert(resp http.ResponseWriter, req *http.
 
 	// Verify the proxy token. This will check both the local proxy token
 	// as well as the ACL if the token isn't local.
-	effectiveToken, err := s.agent.verifyProxyToken(qOpts.Token, id, "")
+	effectiveToken, err := s.agent.verifyProxyToken(qOpts.Token, serviceName, "")
 	if err != nil {
 		return nil, err
 	}
@@ -983,12 +977,6 @@ func (s *HTTPServer) AgentConnectProxyConfig(resp http.ResponseWriter, req *http
 				return "", nil, nil
 			}
 
-			// Validate the ACL token
-			_, err := s.agent.verifyProxyToken(token, proxy.Proxy.TargetServiceID, id)
-			if err != nil {
-				return "", nil, err
-			}
-
 			// Lookup the target service as a convenience
 			target := s.agent.State.Service(proxy.Proxy.TargetServiceID)
 			if target == nil {
@@ -999,6 +987,12 @@ func (s *HTTPServer) AgentConnectProxyConfig(resp http.ResponseWriter, req *http
 				return "", nil, nil
 			}
 
+			// Validate the ACL token
+			_, err := s.agent.verifyProxyToken(token, target.Service, id)
+			if err != nil {
+				return "", nil, err
+			}
+
 			// Watch the proxy for changes
 			ws.Add(proxy.WatchCh)
 

agent/agent_endpoint_test.go

@@ -2238,7 +2238,7 @@ func TestAgentConnectCALeafCert_aclDefaultDeny(t *testing.T) {
 		require.Equal(200, resp.Code, "body: %s", resp.Body.String())
 	}
 
-	req, _ := http.NewRequest("GET", "/v1/agent/connect/ca/leaf/test-id", nil)
+	req, _ := http.NewRequest("GET", "/v1/agent/connect/ca/leaf/test", nil)
 	resp := httptest.NewRecorder()
 	_, err := a.srv.AgentConnectCALeafCert(resp, req)
 	require.Error(err)
@@ -2284,7 +2284,7 @@ func TestAgentConnectCALeafCert_aclProxyToken(t *testing.T) {
 	token := proxy.ProxyToken
 	require.NotEmpty(token)
 
-	req, _ := http.NewRequest("GET", "/v1/agent/connect/ca/leaf/test-id?token="+token, nil)
+	req, _ := http.NewRequest("GET", "/v1/agent/connect/ca/leaf/test?token="+token, nil)
 	resp := httptest.NewRecorder()
 	obj, err := a.srv.AgentConnectCALeafCert(resp, req)
 	require.NoError(err)
@@ -2355,7 +2355,7 @@ func TestAgentConnectCALeafCert_aclProxyTokenOther(t *testing.T) {
 	token := proxy.ProxyToken
 	require.NotEmpty(token)
 
-	req, _ := http.NewRequest("GET", "/v1/agent/connect/ca/leaf/test-id?token="+token, nil)
+	req, _ := http.NewRequest("GET", "/v1/agent/connect/ca/leaf/test?token="+token, nil)
 	resp := httptest.NewRecorder()
 	_, err := a.srv.AgentConnectCALeafCert(resp, req)
 	require.Error(err)
@@ -2413,7 +2413,7 @@ func TestAgentConnectCALeafCert_aclServiceWrite(t *testing.T) {
 		token = aclResp.ID
 	}
 
-	req, _ := http.NewRequest("GET", "/v1/agent/connect/ca/leaf/test-id?token="+token, nil)
+	req, _ := http.NewRequest("GET", "/v1/agent/connect/ca/leaf/test?token="+token, nil)
 	resp := httptest.NewRecorder()
 	obj, err := a.srv.AgentConnectCALeafCert(resp, req)
 	require.NoError(err)
@@ -2474,7 +2474,7 @@ func TestAgentConnectCALeafCert_aclServiceReadDeny(t *testing.T) {
 		token = aclResp.ID
 	}
 
-	req, _ := http.NewRequest("GET", "/v1/agent/connect/ca/leaf/test-id?token="+token, nil)
+	req, _ := http.NewRequest("GET", "/v1/agent/connect/ca/leaf/test?token="+token, nil)
 	resp := httptest.NewRecorder()
 	_, err := a.srv.AgentConnectCALeafCert(resp, req)
 	require.Error(err)
@@ -2517,7 +2517,113 @@ func TestAgentConnectCALeafCert_good(t *testing.T) {
 	}
 
 	// List
-	req, _ := http.NewRequest("GET", "/v1/agent/connect/ca/leaf/foo", nil)
+	req, _ := http.NewRequest("GET", "/v1/agent/connect/ca/leaf/test", nil)
+	resp := httptest.NewRecorder()
+	obj, err := a.srv.AgentConnectCALeafCert(resp, req)
+	require.NoError(err)
+
+	// Get the issued cert
+	issued, ok := obj.(*structs.IssuedCert)
+	assert.True(ok)
+
+	// Verify that the cert is signed by the CA
+	requireLeafValidUnderCA(t, issued, ca1)
+
+	// Verify blocking index
+	assert.True(issued.ModifyIndex > 0)
+	assert.Equal(fmt.Sprintf("%d", issued.ModifyIndex),
+		resp.Header().Get("X-Consul-Index"))
+
+	// That should've been a cache miss, so no hit change
+	require.Equal(cacheHits, a.cache.Hits())
+
+	// Test caching
+	{
+		// Fetch it again
+		obj2, err := a.srv.AgentConnectCALeafCert(httptest.NewRecorder(), req)
+		require.NoError(err)
+		require.Equal(obj, obj2)
+
+		// Should cache hit this time and not make request
+		require.Equal(cacheHits+1, a.cache.Hits())
+		cacheHits++
+	}
+
+	// Test that caching is updated in the background
+	{
+		// Set a new CA
+		ca := connect.TestCAConfigSet(t, a, nil)
+
+		retry.Run(t, func(r *retry.R) {
+			// Try and sign again (note no index/wait arg since cache should update in
+			// background even if we aren't actively blocking)
+			obj, err := a.srv.AgentConnectCALeafCert(httptest.NewRecorder(), req)
+			r.Check(err)
+
+			issued2 := obj.(*structs.IssuedCert)
+			if issued.CertPEM == issued2.CertPEM {
+				r.Fatalf("leaf has not updated")
+			}
+
+			// Got a new leaf. Sanity check it's a whole new key as well as differnt
+			// cert.
+			if issued.PrivateKeyPEM == issued2.PrivateKeyPEM {
+				r.Fatalf("new leaf has same private key as before")
+			}
+
+			// Verify that the cert is signed by the new CA
+			requireLeafValidUnderCA(t, issued2, ca)
+		})
+
+		// Should be a cache hit! The data should've updated in the cache
+		// in the background so this should've been fetched directly from
+		// the cache.
+		if v := a.cache.Hits(); v < cacheHits+1 {
+			t.Fatalf("expected at least one more cache hit, still at %d", v)
+		}
+		cacheHits = a.cache.Hits()
+	}
+}
+
+// Test we can request a leaf cert for a service we have permission for
+// but is not local to this agent.
+func TestAgentConnectCALeafCert_goodNotLocal(t *testing.T) {
+	t.Parallel()
+
+	assert := assert.New(t)
+	require := require.New(t)
+	a := NewTestAgent(t.Name(), "")
+	defer a.Shutdown()
+
+	// CA already setup by default by NewTestAgent but force a new one so we can
+	// verify it was signed easily.
+	ca1 := connect.TestCAConfigSet(t, a, nil)
+
+	// Grab the initial cache hit count
+	cacheHits := a.cache.Hits()
+
+	{
+		// Register a non-local service (central catalog)
+		args := &structs.RegisterRequest{
+			Node:    "foo",
+			Address: "127.0.0.1",
+			Service: &structs.NodeService{
+				Service: "test",
+				Address: "127.0.0.1",
+				Port:    8080,
+			},
+		}
+		req, _ := http.NewRequest("PUT", "/v1/catalog/register", jsonReader(args))
+		resp := httptest.NewRecorder()
+		_, err := a.srv.CatalogRegister(resp, req)
+		require.NoError(err)
+		if !assert.Equal(200, resp.Code) {
+			t.Log("Body: ", resp.Body.String())
+		}
+	}
+
+	// List
+	req, _ := http.NewRequest("GET", "/v1/agent/connect/ca/leaf/test", nil)
 	resp := httptest.NewRecorder()
 	obj, err := a.srv.AgentConnectCALeafCert(resp, req)
 	require.NoError(err)