From cd39f096934970de0eef75a4cbabf52225a01c91 Mon Sep 17 00:00:00 2001 From: Mitchell Hashimoto Date: Fri, 18 May 2018 23:27:02 -0700 Subject: [PATCH] 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 | 23 +++---- agent/agent_endpoint.go | 24 +++---- agent/agent_endpoint_test.go | 118 +++++++++++++++++++++++++++++++++-- 3 files changed, 133 insertions(+), 32 deletions(-) diff --git a/agent/agent.go b/agent/agent.go index 622a105e82..6f909e5c71 100644 --- a/agent/agent.go +++ b/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 - } - - // 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) + return a.State.ServiceToken(proxy.Proxy.TargetServiceID), nil } // 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 } diff --git a/agent/agent_endpoint.go b/agent/agent_endpoint.go index 5b9a5f8ece..239f962a1a 100644 --- a/agent/agent_endpoint.go +++ b/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/") - - // Retrieve the service specified - service := s.agent.State.Service(id) - if service == nil { - return nil, fmt.Errorf("unknown service ID: %s", id) - } + serviceName := strings.TrimPrefix(req.URL.Path, "/v1/agent/connect/ca/leaf/") 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) diff --git a/agent/agent_endpoint_test.go b/agent/agent_endpoint_test.go index 9c10a61ff3..62d095fbae 100644 --- a/agent/agent_endpoint_test.go +++ b/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)