From ed8a901be72a45d28b70ea357c44e0b009a83cb0 Mon Sep 17 00:00:00 2001 From: "R.B. Boyer" <4903+rboyer@users.noreply.github.com> Date: Fri, 25 Jun 2021 16:47:47 -0500 Subject: [PATCH] connect: include optional partition prefixes in SPIFFE identifiers (#10507) NOTE: this does not include any intentions enforcement changes yet --- .changelog/10507.txt | 3 + agent/auto-config/tls.go | 1 + agent/cache-types/connect_ca_leaf.go | 6 + agent/connect/uri.go | 34 +++-- agent/connect/uri_agent.go | 12 +- agent/connect/uri_agent_oss.go | 9 ++ agent/connect/uri_agent_oss_test.go | 32 +++++ agent/connect/uri_agent_test.go | 17 --- agent/connect/uri_service.go | 21 +++- agent/connect/uri_service_oss.go | 12 +- agent/connect/uri_service_oss_test.go | 40 ++++++ agent/connect/uri_signing.go | 6 +- agent/connect/uri_signing_test.go | 8 +- agent/connect/uri_test.go | 175 +++++++++++++++----------- agent/connect_auth.go | 8 ++ agent/structs/connect.go | 4 + agent/structs/structs_oss.go | 8 ++ connect/resolver.go | 1 + 18 files changed, 279 insertions(+), 118 deletions(-) create mode 100644 .changelog/10507.txt create mode 100644 agent/connect/uri_agent_oss.go create mode 100644 agent/connect/uri_agent_oss_test.go delete mode 100644 agent/connect/uri_agent_test.go create mode 100644 agent/connect/uri_service_oss_test.go diff --git a/.changelog/10507.txt b/.changelog/10507.txt new file mode 100644 index 0000000000..aa331651fe --- /dev/null +++ b/.changelog/10507.txt @@ -0,0 +1,3 @@ +```release-note:feature +connect: include optional partition prefixes in SPIFFE identifiers +``` diff --git a/agent/auto-config/tls.go b/agent/auto-config/tls.go index 08ac6f9c7f..c152203082 100644 --- a/agent/auto-config/tls.go +++ b/agent/auto-config/tls.go @@ -216,6 +216,7 @@ func (ac *AutoConfig) generateCSR() (csr string, key string, err error) { Host: unknownTrustDomain, Datacenter: ac.config.Datacenter, Agent: ac.config.NodeName, + // TODO(rb)(partitions): populate the partition field from the agent config } caConfig, err := ac.config.ConnectCAConfiguration() diff --git a/agent/cache-types/connect_ca_leaf.go b/agent/cache-types/connect_ca_leaf.go index 95f1de5385..9a7fcd216c 100644 --- a/agent/cache-types/connect_ca_leaf.go +++ b/agent/cache-types/connect_ca_leaf.go @@ -524,6 +524,7 @@ func (c *ConnectCALeaf) generateNewLeaf(req *ConnectCALeafRequest, id = &connect.SpiffeIDService{ Host: roots.TrustDomain, Datacenter: req.Datacenter, + Partition: req.TargetPartition(), Namespace: req.TargetNamespace(), Service: req.Service, } @@ -532,6 +533,7 @@ func (c *ConnectCALeaf) generateNewLeaf(req *ConnectCALeafRequest, id = &connect.SpiffeIDAgent{ Host: roots.TrustDomain, Datacenter: req.Datacenter, + Partition: req.TargetPartition(), Agent: req.Agent, } dnsNames = append([]string{"localhost"}, req.DNSSAN...) @@ -676,6 +678,10 @@ func (r *ConnectCALeafRequest) Key() string { return "" } +func (req *ConnectCALeafRequest) TargetPartition() string { + return req.PartitionOrDefault() +} + func (r *ConnectCALeafRequest) CacheInfo() cache.RequestInfo { return cache.RequestInfo{ Token: r.Token, diff --git a/agent/connect/uri.go b/agent/connect/uri.go index 2f44546320..2ae5034b6c 100644 --- a/agent/connect/uri.go +++ b/agent/connect/uri.go @@ -21,9 +21,9 @@ type CertURI interface { var ( spiffeIDServiceRegexp = regexp.MustCompile( - `^/ns/([^/]+)/dc/([^/]+)/svc/([^/]+)$`) + `^(?:/ap/([^/]+))?/ns/([^/]+)/dc/([^/]+)/svc/([^/]+)$`) spiffeIDAgentRegexp = regexp.MustCompile( - `^/agent/client/dc/([^/]+)/id/([^/]+)$`) + `^(?:/ap/([^/]+))?/agent/client/dc/([^/]+)/id/([^/]+)$`) ) // ParseCertURIFromString attempts to parse a string representation of a @@ -56,24 +56,29 @@ func ParseCertURI(input *url.URL) (CertURI, error) { // Determine the values. We assume they're sane to save cycles, // but if the raw path is not empty that means that something is // URL encoded so we go to the slow path. - ns := v[1] - dc := v[2] - service := v[3] + ap := v[1] + ns := v[2] + dc := v[3] + service := v[4] if input.RawPath != "" { var err error - if ns, err = url.PathUnescape(v[1]); err != nil { + if ap, err = url.PathUnescape(v[1]); err != nil { + return nil, fmt.Errorf("Invalid admin partition: %s", err) + } + if ns, err = url.PathUnescape(v[2]); err != nil { return nil, fmt.Errorf("Invalid namespace: %s", err) } - if dc, err = url.PathUnescape(v[2]); err != nil { + if dc, err = url.PathUnescape(v[3]); err != nil { return nil, fmt.Errorf("Invalid datacenter: %s", err) } - if service, err = url.PathUnescape(v[3]); err != nil { + if service, err = url.PathUnescape(v[4]); err != nil { return nil, fmt.Errorf("Invalid service: %s", err) } } return &SpiffeIDService{ Host: input.Host, + Partition: ap, Namespace: ns, Datacenter: dc, Service: service, @@ -82,20 +87,25 @@ func ParseCertURI(input *url.URL) (CertURI, error) { // Determine the values. We assume they're sane to save cycles, // but if the raw path is not empty that means that something is // URL encoded so we go to the slow path. - dc := v[1] - agent := v[2] + ap := v[1] + dc := v[2] + agent := v[3] if input.RawPath != "" { var err error - if dc, err = url.PathUnescape(v[1]); err != nil { + if ap, err = url.PathUnescape(v[1]); err != nil { + return nil, fmt.Errorf("Invalid admin partition: %s", err) + } + if dc, err = url.PathUnescape(v[2]); err != nil { return nil, fmt.Errorf("Invalid datacenter: %s", err) } - if agent, err = url.PathUnescape(v[2]); err != nil { + if agent, err = url.PathUnescape(v[3]); err != nil { return nil, fmt.Errorf("Invalid node: %s", err) } } return &SpiffeIDAgent{ Host: input.Host, + Partition: ap, Datacenter: dc, Agent: agent, }, nil diff --git a/agent/connect/uri_agent.go b/agent/connect/uri_agent.go index e8e3c8f069..3d144b016a 100644 --- a/agent/connect/uri_agent.go +++ b/agent/connect/uri_agent.go @@ -1,22 +1,28 @@ package connect import ( - "fmt" "net/url" + + "github.com/hashicorp/consul/agent/structs" ) // SpiffeIDService is the structure to represent the SPIFFE ID for an agent. type SpiffeIDAgent struct { Host string + Partition string Datacenter string Agent string } +func (id SpiffeIDAgent) PartitionOrDefault() string { + return structs.PartitionOrDefault(id.Partition) +} + // URI returns the *url.URL for this SPIFFE ID. -func (id *SpiffeIDAgent) URI() *url.URL { +func (id SpiffeIDAgent) URI() *url.URL { var result url.URL result.Scheme = "spiffe" result.Host = id.Host - result.Path = fmt.Sprintf("/agent/client/dc/%s/id/%s", id.Datacenter, id.Agent) + result.Path = id.uriPath() return &result } diff --git a/agent/connect/uri_agent_oss.go b/agent/connect/uri_agent_oss.go new file mode 100644 index 0000000000..bf13697ee3 --- /dev/null +++ b/agent/connect/uri_agent_oss.go @@ -0,0 +1,9 @@ +// +build !consulent + +package connect + +import "fmt" + +func (id SpiffeIDAgent) uriPath() string { + return fmt.Sprintf("/agent/client/dc/%s/id/%s", id.Datacenter, id.Agent) +} diff --git a/agent/connect/uri_agent_oss_test.go b/agent/connect/uri_agent_oss_test.go new file mode 100644 index 0000000000..8bfc387849 --- /dev/null +++ b/agent/connect/uri_agent_oss_test.go @@ -0,0 +1,32 @@ +// +build !consulent + +package connect + +import ( + "testing" + + "github.com/stretchr/testify/require" +) + +func TestSpiffeIDAgentURI(t *testing.T) { + t.Run("default partition", func(t *testing.T) { + agent := &SpiffeIDAgent{ + Host: "1234.consul", + Datacenter: "dc1", + Agent: "123", + } + + require.Equal(t, "spiffe://1234.consul/agent/client/dc/dc1/id/123", agent.URI().String()) + }) + + t.Run("partitions are ignored", func(t *testing.T) { + agent := &SpiffeIDAgent{ + Host: "1234.consul", + Partition: "foobar", + Datacenter: "dc1", + Agent: "123", + } + + require.Equal(t, "spiffe://1234.consul/agent/client/dc/dc1/id/123", agent.URI().String()) + }) +} diff --git a/agent/connect/uri_agent_test.go b/agent/connect/uri_agent_test.go deleted file mode 100644 index 7e0cb81845..0000000000 --- a/agent/connect/uri_agent_test.go +++ /dev/null @@ -1,17 +0,0 @@ -package connect - -import ( - "testing" - - "github.com/stretchr/testify/require" -) - -func TestSpiffeIDAgentURI(t *testing.T) { - agent := &SpiffeIDAgent{ - Host: "1234.consul", - Datacenter: "dc1", - Agent: "123", - } - - require.Equal(t, "spiffe://1234.consul/agent/client/dc/dc1/id/123", agent.URI().String()) -} diff --git a/agent/connect/uri_service.go b/agent/connect/uri_service.go index d19fbe7754..82ce662f66 100644 --- a/agent/connect/uri_service.go +++ b/agent/connect/uri_service.go @@ -1,24 +1,37 @@ package connect import ( - "fmt" "net/url" + + "github.com/hashicorp/consul/agent/structs" ) // SpiffeIDService is the structure to represent the SPIFFE ID for a service. type SpiffeIDService struct { Host string + Partition string Namespace string Datacenter string Service string } +func (id SpiffeIDService) NamespaceOrDefault() string { + return structs.NamespaceOrDefault(id.Namespace) +} + +func (id SpiffeIDService) MatchesPartition(partition string) bool { + return id.PartitionOrDefault() == structs.PartitionOrDefault(partition) +} + +func (id SpiffeIDService) PartitionOrDefault() string { + return structs.PartitionOrDefault(id.Partition) +} + // URI returns the *url.URL for this SPIFFE ID. -func (id *SpiffeIDService) URI() *url.URL { +func (id SpiffeIDService) URI() *url.URL { var result url.URL result.Scheme = "spiffe" result.Host = id.Host - result.Path = fmt.Sprintf("/ns/%s/dc/%s/svc/%s", - id.Namespace, id.Datacenter, id.Service) + result.Path = id.uriPath() return &result } diff --git a/agent/connect/uri_service_oss.go b/agent/connect/uri_service_oss.go index c76fea239d..3838ef955a 100644 --- a/agent/connect/uri_service_oss.go +++ b/agent/connect/uri_service_oss.go @@ -3,11 +3,21 @@ package connect import ( + "fmt" + "github.com/hashicorp/consul/agent/structs" ) // GetEnterpriseMeta will synthesize an EnterpriseMeta struct from the SpiffeIDService. // in OSS this just returns an empty (but never nil) struct pointer -func (id *SpiffeIDService) GetEnterpriseMeta() *structs.EnterpriseMeta { +func (id SpiffeIDService) GetEnterpriseMeta() *structs.EnterpriseMeta { return &structs.EnterpriseMeta{} } + +func (id SpiffeIDService) uriPath() string { + return fmt.Sprintf("/ns/%s/dc/%s/svc/%s", + id.NamespaceOrDefault(), + id.Datacenter, + id.Service, + ) +} diff --git a/agent/connect/uri_service_oss_test.go b/agent/connect/uri_service_oss_test.go new file mode 100644 index 0000000000..a844469e59 --- /dev/null +++ b/agent/connect/uri_service_oss_test.go @@ -0,0 +1,40 @@ +// +build !consulent + +package connect + +import ( + "testing" + + "github.com/stretchr/testify/require" +) + +func TestSpiffeIDServiceURI(t *testing.T) { + t.Run("default partition; default namespace", func(t *testing.T) { + svc := &SpiffeIDService{ + Host: "1234.consul", + Datacenter: "dc1", + Service: "web", + } + require.Equal(t, "spiffe://1234.consul/ns/default/dc/dc1/svc/web", svc.URI().String()) + }) + + t.Run("partitions are ignored", func(t *testing.T) { + svc := &SpiffeIDService{ + Host: "1234.consul", + Partition: "other", + Datacenter: "dc1", + Service: "web", + } + require.Equal(t, "spiffe://1234.consul/ns/default/dc/dc1/svc/web", svc.URI().String()) + }) + + t.Run("namespaces are ignored", func(t *testing.T) { + svc := &SpiffeIDService{ + Host: "1234.consul", + Namespace: "other", + Datacenter: "dc1", + Service: "web", + } + require.Equal(t, "spiffe://1234.consul/ns/default/dc/dc1/svc/web", svc.URI().String()) + }) +} diff --git a/agent/connect/uri_signing.go b/agent/connect/uri_signing.go index ad8de307e1..671bb79fc4 100644 --- a/agent/connect/uri_signing.go +++ b/agent/connect/uri_signing.go @@ -16,7 +16,7 @@ type SpiffeIDSigning struct { } // URI returns the *url.URL for this SPIFFE ID. -func (id *SpiffeIDSigning) URI() *url.URL { +func (id SpiffeIDSigning) URI() *url.URL { var result url.URL result.Scheme = "spiffe" result.Host = id.Host() @@ -24,7 +24,7 @@ func (id *SpiffeIDSigning) URI() *url.URL { } // Host is the canonical representation as a DNS-compatible hostname. -func (id *SpiffeIDSigning) Host() string { +func (id SpiffeIDSigning) Host() string { return strings.ToLower(fmt.Sprintf("%s.%s", id.ClusterID, id.Domain)) } @@ -36,7 +36,7 @@ func (id *SpiffeIDSigning) Host() string { // method on CertURI interface since we don't intend this to be extensible // outside and it's easier to reason about the security properties when they are // all in one place with "allowlist" semantics. -func (id *SpiffeIDSigning) CanSign(cu CertURI) bool { +func (id SpiffeIDSigning) CanSign(cu CertURI) bool { switch other := cu.(type) { case *SpiffeIDSigning: // We can only sign other CA certificates for the same trust domain. Note diff --git a/agent/connect/uri_signing_test.go b/agent/connect/uri_signing_test.go index 9a62ea2448..ca4020b99d 100644 --- a/agent/connect/uri_signing_test.go +++ b/agent/connect/uri_signing_test.go @@ -79,25 +79,25 @@ func TestSpiffeIDSigning_CanSign(t *testing.T) { { name: "service - good", id: testSigning, - input: &SpiffeIDService{TestClusterID + ".consul", "default", "dc1", "web"}, + input: &SpiffeIDService{Host: TestClusterID + ".consul", Namespace: "default", Datacenter: "dc1", Service: "web"}, want: true, }, { name: "service - good midex case", id: testSigning, - input: &SpiffeIDService{strings.ToUpper(TestClusterID) + ".CONsuL", "defAUlt", "dc1", "WEB"}, + input: &SpiffeIDService{Host: strings.ToUpper(TestClusterID) + ".CONsuL", Namespace: "defAUlt", Datacenter: "dc1", Service: "WEB"}, want: true, }, { name: "service - different cluster", id: testSigning, - input: &SpiffeIDService{"55555555-4444-3333-2222-111111111111.consul", "default", "dc1", "web"}, + input: &SpiffeIDService{Host: "55555555-4444-3333-2222-111111111111.consul", Namespace: "default", Datacenter: "dc1", Service: "web"}, want: false, }, { name: "service - different TLD", id: testSigning, - input: &SpiffeIDService{TestClusterID + ".fake", "default", "dc1", "web"}, + input: &SpiffeIDService{Host: TestClusterID + ".fake", Namespace: "default", Datacenter: "dc1", Service: "web"}, want: false, }, } diff --git a/agent/connect/uri_test.go b/agent/connect/uri_test.go index 22b8a637d2..47e1e41990 100644 --- a/agent/connect/uri_test.go +++ b/agent/connect/uri_test.go @@ -3,86 +3,113 @@ package connect import ( "testing" - "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + "github.com/hashicorp/consul/sdk/testutil" ) -// testCertURICases contains the test cases for parsing and encoding -// the SPIFFE IDs. This is a global since it is used in multiple test functions. -var testCertURICases = []struct { - Name string - URI string - Struct interface{} - ParseError string -}{ - { - "invalid scheme", - "http://google.com/", - nil, - "scheme", - }, - - { - "basic service ID", - "spiffe://1234.consul/ns/default/dc/dc01/svc/web", - &SpiffeIDService{ - Host: "1234.consul", - Namespace: "default", - Datacenter: "dc01", - Service: "web", - }, - "", - }, - - { - "basic agent ID", - "spiffe://1234.consul/agent/client/dc/dc1/id/uuid", - &SpiffeIDAgent{ - Host: "1234.consul", - Datacenter: "dc1", - Agent: "uuid", - }, - "", - }, - - { - "service with URL-encoded values", - "spiffe://1234.consul/ns/foo%2Fbar/dc/bar%2Fbaz/svc/baz%2Fqux", - &SpiffeIDService{ - Host: "1234.consul", - Namespace: "foo/bar", - Datacenter: "bar/baz", - Service: "baz/qux", - }, - "", - }, - - { - "signing ID", - "spiffe://1234.consul", - &SpiffeIDSigning{ - ClusterID: "1234", - Domain: "consul", - }, - "", - }, -} - func TestParseCertURIFromString(t *testing.T) { - for _, tc := range testCertURICases { - t.Run(tc.Name, func(t *testing.T) { - assert := assert.New(t) + var cases = []struct { + Name string + URI string + Struct interface{} + ParseError string + }{ + { + "invalid scheme", + "http://google.com/", + nil, + "scheme", + }, + { + "basic service ID", + "spiffe://1234.consul/ns/default/dc/dc01/svc/web", + &SpiffeIDService{ + Host: "1234.consul", + Namespace: "default", + Datacenter: "dc01", + Service: "web", + }, + "", + }, + { + "basic service ID with partition", + "spiffe://1234.consul/ap/bizdev/ns/default/dc/dc01/svc/web", + &SpiffeIDService{ + Host: "1234.consul", + Partition: "bizdev", + Namespace: "default", + Datacenter: "dc01", + Service: "web", + }, + "", + }, + { + "basic agent ID", + "spiffe://1234.consul/agent/client/dc/dc1/id/uuid", + &SpiffeIDAgent{ + Host: "1234.consul", + Datacenter: "dc1", + Agent: "uuid", + }, + "", + }, + { + "basic agent ID with partition", + "spiffe://1234.consul/ap/bizdev/agent/client/dc/dc1/id/uuid", + &SpiffeIDAgent{ + Host: "1234.consul", + Partition: "bizdev", + Datacenter: "dc1", + Agent: "uuid", + }, + "", + }, + { + "service with URL-encoded values", + "spiffe://1234.consul/ns/foo%2Fbar/dc/bar%2Fbaz/svc/baz%2Fqux", + &SpiffeIDService{ + Host: "1234.consul", + Namespace: "foo/bar", + Datacenter: "bar/baz", + Service: "baz/qux", + }, + "", + }, + { + "service with URL-encoded values with partition", + "spiffe://1234.consul/ap/biz%2Fdev/ns/foo%2Fbar/dc/bar%2Fbaz/svc/baz%2Fqux", + &SpiffeIDService{ + Host: "1234.consul", + Partition: "biz/dev", + Namespace: "foo/bar", + Datacenter: "bar/baz", + Service: "baz/qux", + }, + "", + }, + { + "signing ID", + "spiffe://1234.consul", + &SpiffeIDSigning{ + ClusterID: "1234", + Domain: "consul", + }, + "", + }, + } - // Parse the ID and check the error/return value + for _, tc := range cases { + t.Run(tc.Name, func(t *testing.T) { actual, err := ParseCertURIFromString(tc.URI) - if err != nil { - t.Logf("parse error: %s", err.Error()) + if tc.ParseError != "" { + require.Error(t, err) + require.Contains(t, err.Error(), tc.ParseError) + testutil.RequireErrorContains(t, err, tc.ParseError) + } else { + require.NoError(t, err) + require.Equal(t, tc.Struct, actual) } - assert.Equal(tc.ParseError != "", err != nil, "error value") - if err != nil { - assert.Contains(err.Error(), tc.ParseError) - return - } - assert.Equal(tc.Struct, actual) }) } } diff --git a/agent/connect_auth.go b/agent/connect_auth.go index 71f14b3194..610b213811 100644 --- a/agent/connect_auth.go +++ b/agent/connect_auth.go @@ -3,6 +3,7 @@ package agent import ( "context" "fmt" + "github.com/hashicorp/consul/acl" "github.com/hashicorp/consul/agent/cache" cachetype "github.com/hashicorp/consul/agent/cache-types" @@ -68,6 +69,13 @@ func (a *Agent) ConnectAuthorize(token string, return returnErr(acl.ErrPermissionDenied) } + if !uriService.MatchesPartition(req.TargetPartition()) { + reason = fmt.Sprintf("Mismatched partitions: %q != %q", + uriService.PartitionOrDefault(), + structs.PartitionOrDefault(req.TargetPartition())) + return false, reason, nil, nil + } + // Note that we DON'T explicitly validate the trust-domain matches ours. See // the PR for this change for details. diff --git a/agent/structs/connect.go b/agent/structs/connect.go index 1ca18cf7aa..533b442700 100644 --- a/agent/structs/connect.go +++ b/agent/structs/connect.go @@ -18,3 +18,7 @@ type ConnectAuthorizeRequest struct { ClientCertURI string ClientCertSerial string } + +func (req *ConnectAuthorizeRequest) TargetPartition() string { + return req.PartitionOrDefault() +} diff --git a/agent/structs/structs_oss.go b/agent/structs/structs_oss.go index 64305c3065..2aeb016741 100644 --- a/agent/structs/structs_oss.go +++ b/agent/structs/structs_oss.go @@ -46,6 +46,10 @@ func (m *EnterpriseMeta) NamespaceOrDefault() string { return IntentionDefaultNamespace } +func NamespaceOrDefault(_ string) string { + return IntentionDefaultNamespace +} + func (m *EnterpriseMeta) NamespaceOrEmpty() string { return "" } @@ -54,6 +58,10 @@ func (m *EnterpriseMeta) PartitionOrDefault() string { return "" } +func PartitionOrDefault(_ string) string { + return "" +} + func (m *EnterpriseMeta) PartitionOrEmpty() string { return "" } diff --git a/connect/resolver.go b/connect/resolver.go index 054ebe70d3..73f83de586 100644 --- a/connect/resolver.go +++ b/connect/resolver.go @@ -155,6 +155,7 @@ func (cr *ConsulResolver) resolveServiceEntry(entry *api.ServiceEntry) (string, Namespace: "default", Datacenter: entry.Node.Datacenter, Service: service, + // NOTE: this only handles the default implicit partition currently. } return ipaddr.FormatAddressPort(addr, port), certURI, nil