From 17227343134813376fb8b954b720024ceaffa572 Mon Sep 17 00:00:00 2001 From: Paul Banks Date: Wed, 9 May 2018 20:30:43 +0100 Subject: [PATCH] Verify trust domain on /authorize calls --- agent/agent_endpoint.go | 28 ++++++++- agent/agent_endpoint_test.go | 106 +++++++++++++++++++++++++++----- agent/cache/cache.go | 2 +- agent/connect/testing_spiffe.go | 8 ++- 4 files changed, 123 insertions(+), 21 deletions(-) diff --git a/agent/agent_endpoint.go b/agent/agent_endpoint.go index 32b326867b..c9afa55db7 100644 --- a/agent/agent_endpoint.go +++ b/agent/agent_endpoint.go @@ -1159,8 +1159,30 @@ func (s *HTTPServer) AgentConnectAuthorize(resp http.ResponseWriter, req *http.R return nil, acl.ErrPermissionDenied } - // TODO(mitchellh): we need to verify more things here, such as the - // trust domain, blacklist lookup of the serial, etc. + // Validate the trust domain matches ours. Later we will support explicit + // external federation but not built yet. + rootArgs := &structs.DCSpecificRequest{Datacenter: s.agent.config.Datacenter} + raw, err := s.agent.cache.Get(cachetype.ConnectCARootName, rootArgs) + if err != nil { + return nil, err + } + + roots, ok := raw.(*structs.IndexedCARoots) + if !ok { + return nil, fmt.Errorf("internal error: roots response type not correct") + } + if roots.TrustDomain == "" { + return nil, fmt.Errorf("connect CA not bootstrapped yet") + } + if roots.TrustDomain != strings.ToLower(uriService.Host) { + return &connectAuthorizeResp{ + Authorized: false, + Reason: fmt.Sprintf("Identity from an external trust domain: %s", + uriService.Host), + }, nil + } + + // TODO(banks): Implement revocation list checking here. // Get the intentions for this target service. args := &structs.IntentionQueryRequest{ @@ -1177,7 +1199,7 @@ func (s *HTTPServer) AgentConnectAuthorize(resp http.ResponseWriter, req *http.R } args.Token = token - raw, err := s.agent.cache.Get(cachetype.IntentionMatchName, args) + raw, err = s.agent.cache.Get(cachetype.IntentionMatchName, args) if err != nil { return nil, err } diff --git a/agent/agent_endpoint_test.go b/agent/agent_endpoint_test.go index 4749148e1a..c1fd16eb9d 100644 --- a/agent/agent_endpoint_test.go +++ b/agent/agent_endpoint_test.go @@ -3286,6 +3286,17 @@ func TestAgentConnectAuthorize_idNotService(t *testing.T) { assert.Contains(obj.Reason, "must be a valid") } +func testFetchTrustDomain(t *testing.T, a *TestAgent) string { + req, _ := http.NewRequest("GET", "/v1/agent/connect/ca/roots", nil) + resp := httptest.NewRecorder() + obj, err := a.srv.AgentConnectCARoots(resp, req) + require.NoError(t, err) + + value := obj.(structs.IndexedCARoots) + require.NotEmpty(t, value.TrustDomain) + return value.TrustDomain +} + // Test when there is an intention allowing the connection func TestAgentConnectAuthorize_allow(t *testing.T) { t.Parallel() @@ -3296,6 +3307,8 @@ func TestAgentConnectAuthorize_allow(t *testing.T) { target := "db" + trustDomain := testFetchTrustDomain(t, a) + // Create some intentions var ixnId string { @@ -3317,8 +3330,9 @@ func TestAgentConnectAuthorize_allow(t *testing.T) { cacheHits := a.cache.Hits() args := &structs.ConnectAuthorizeRequest{ - Target: target, - ClientCertURI: connect.TestSpiffeIDService(t, "web").URI().String(), + Target: target, + ClientCertURI: connect.TestSpiffeIDServiceWithHost(t, "web", trustDomain). + URI().String(), } req, _ := http.NewRequest("POST", "/v1/agent/connect/authorize", jsonReader(args)) resp := httptest.NewRecorder() @@ -3330,8 +3344,13 @@ func TestAgentConnectAuthorize_allow(t *testing.T) { require.True(obj.Authorized) require.Contains(obj.Reason, "Matched") - // That should've been a cache miss, so not hit change - require.Equal(cacheHits, a.cache.Hits()) + // That should've been a cache miss, so no hit change, however since + // testFetchTrustDomain already called Roots and caused it to be in cache, the + // authorize call above will also call it and see a cache hit for the Roots + // RPC. In other words, there are 2 cached calls in /authorize and we always + // expect one of them to be a hit. So asserting only 1 happened is as close as + // we can get to verifying that the intention match RPC was a hit. + require.Equal(cacheHits+1, a.cache.Hits()) // Make the request again { @@ -3346,9 +3365,10 @@ func TestAgentConnectAuthorize_allow(t *testing.T) { require.Contains(obj.Reason, "Matched") } - // That should've been a cache hit - require.Equal(cacheHits+1, a.cache.Hits()) - cacheHits++ + // That should've been a cache hit. We add the one hit from Roots from first + // call as well as the 2 from this call (Roots + Intentions). + require.Equal(cacheHits+1+2, a.cache.Hits()) + cacheHits = a.cache.Hits() // Change the intention { @@ -3384,9 +3404,9 @@ func TestAgentConnectAuthorize_allow(t *testing.T) { } // That should've been a cache hit, too, since it updated in the - // background. - require.Equal(cacheHits+1, a.cache.Hits()) - cacheHits++ + // background. (again 2 hits for Roots + Intentions) + require.Equal(cacheHits+2, a.cache.Hits()) + cacheHits += 2 } // Test when there is an intention denying the connection @@ -3399,6 +3419,8 @@ func TestAgentConnectAuthorize_deny(t *testing.T) { target := "db" + trustDomain := testFetchTrustDomain(t, a) + // Create some intentions { req := structs.IntentionRequest{ @@ -3417,8 +3439,9 @@ func TestAgentConnectAuthorize_deny(t *testing.T) { } args := &structs.ConnectAuthorizeRequest{ - Target: target, - ClientCertURI: connect.TestSpiffeIDService(t, "web").URI().String(), + Target: target, + ClientCertURI: connect.TestSpiffeIDServiceWithHost(t, "web", trustDomain). + URI().String(), } req, _ := http.NewRequest("POST", "/v1/agent/connect/authorize", jsonReader(args)) resp := httptest.NewRecorder() @@ -3431,6 +3454,53 @@ func TestAgentConnectAuthorize_deny(t *testing.T) { assert.Contains(obj.Reason, "Matched") } +// Test when there is an intention allowing service but for a different trust +// domain. +func TestAgentConnectAuthorize_denyTrustDomain(t *testing.T) { + t.Parallel() + + assert := assert.New(t) + a := NewTestAgent(t.Name(), "") + defer a.Shutdown() + + target := "db" + + // Create some intentions + { + req := structs.IntentionRequest{ + Datacenter: "dc1", + Op: structs.IntentionOpCreate, + Intention: structs.TestIntention(t), + } + req.Intention.SourceNS = structs.IntentionDefaultNamespace + req.Intention.SourceName = "web" + req.Intention.DestinationNS = structs.IntentionDefaultNamespace + req.Intention.DestinationName = target + req.Intention.Action = structs.IntentionActionAllow + + var reply string + assert.Nil(a.RPC("Intention.Apply", &req, &reply)) + } + + { + args := &structs.ConnectAuthorizeRequest{ + Target: target, + // Rely on the test trust domain this will choose to not match the random + // one picked on agent startup. + ClientCertURI: connect.TestSpiffeIDService(t, "web").URI().String(), + } + req, _ := http.NewRequest("POST", "/v1/agent/connect/authorize", jsonReader(args)) + resp := httptest.NewRecorder() + respRaw, err := a.srv.AgentConnectAuthorize(resp, req) + assert.Nil(err) + assert.Equal(200, resp.Code) + + obj := respRaw.(*connectAuthorizeResp) + assert.False(obj.Authorized) + assert.Contains(obj.Reason, "Identity from an external trust domain") + } +} + func TestAgentConnectAuthorize_denyWildcard(t *testing.T) { t.Parallel() @@ -3440,6 +3510,8 @@ func TestAgentConnectAuthorize_denyWildcard(t *testing.T) { target := "db" + trustDomain := testFetchTrustDomain(t, a) + // Create some intentions { // Deny wildcard to DB @@ -3477,8 +3549,9 @@ func TestAgentConnectAuthorize_denyWildcard(t *testing.T) { // Web should be allowed { args := &structs.ConnectAuthorizeRequest{ - Target: target, - ClientCertURI: connect.TestSpiffeIDService(t, "web").URI().String(), + Target: target, + ClientCertURI: connect.TestSpiffeIDServiceWithHost(t, "web", trustDomain). + URI().String(), } req, _ := http.NewRequest("POST", "/v1/agent/connect/authorize", jsonReader(args)) resp := httptest.NewRecorder() @@ -3494,8 +3567,9 @@ func TestAgentConnectAuthorize_denyWildcard(t *testing.T) { // API should be denied { args := &structs.ConnectAuthorizeRequest{ - Target: target, - ClientCertURI: connect.TestSpiffeIDService(t, "api").URI().String(), + Target: target, + ClientCertURI: connect.TestSpiffeIDServiceWithHost(t, "api", trustDomain). + URI().String(), } req, _ := http.NewRequest("POST", "/v1/agent/connect/authorize", jsonReader(args)) resp := httptest.NewRecorder() diff --git a/agent/cache/cache.go b/agent/cache/cache.go index 1b4653cb49..e2eee03d85 100644 --- a/agent/cache/cache.go +++ b/agent/cache/cache.go @@ -72,7 +72,7 @@ type Cache struct { // of "//" in order to properly partition // requests to different datacenters and ACL tokens. This format has some // big drawbacks: we can't evict by datacenter, ACL token, etc. For an - // initial implementaiton this works and the tests are agnostic to the + // initial implementation this works and the tests are agnostic to the // internal storage format so changing this should be possible safely. entriesLock sync.RWMutex entries map[string]cacheEntry diff --git a/agent/connect/testing_spiffe.go b/agent/connect/testing_spiffe.go index d6a70cb81e..42db76495d 100644 --- a/agent/connect/testing_spiffe.go +++ b/agent/connect/testing_spiffe.go @@ -6,8 +6,14 @@ import ( // TestSpiffeIDService returns a SPIFFE ID representing a service. func TestSpiffeIDService(t testing.T, service string) *SpiffeIDService { + return TestSpiffeIDServiceWithHost(t, service, testClusterID+".consul") +} + +// TestSpiffeIDServiceWithHost returns a SPIFFE ID representing a service with +// the specified trust domain. +func TestSpiffeIDServiceWithHost(t testing.T, service, host string) *SpiffeIDService { return &SpiffeIDService{ - Host: testClusterID + ".consul", + Host: host, Namespace: "default", Datacenter: "dc1", Service: service,