From 622a475eb19ab54bb635ef465aee45fc808f9b23 Mon Sep 17 00:00:00 2001 From: Paul Banks Date: Wed, 9 May 2018 14:25:48 +0100 Subject: [PATCH] Add CSR signing verification of service ACL, trust domain and datacenter. --- agent/connect/uri_signing.go | 35 ++++++- agent/connect/uri_signing_test.go | 97 ++++++++++++++++++++ agent/consul/connect_ca_endpoint.go | 52 ++++++++--- agent/consul/connect_ca_endpoint_test.go | 112 ++++++++++++++++++++++- 4 files changed, 277 insertions(+), 19 deletions(-) diff --git a/agent/connect/uri_signing.go b/agent/connect/uri_signing.go index b43971ed7d..843f955965 100644 --- a/agent/connect/uri_signing.go +++ b/agent/connect/uri_signing.go @@ -3,6 +3,7 @@ package connect import ( "fmt" "net/url" + "strings" "github.com/hashicorp/consul/agent/structs" ) @@ -18,16 +19,48 @@ type SpiffeIDSigning struct { func (id *SpiffeIDSigning) URI() *url.URL { var result url.URL result.Scheme = "spiffe" - result.Host = fmt.Sprintf("%s.%s", id.ClusterID, id.Domain) + result.Host = id.Host() return &result } +// Host is the canonical representation as a DNS-compatible hostname. +func (id *SpiffeIDSigning) Host() string { + return strings.ToLower(fmt.Sprintf("%s.%s", id.ClusterID, id.Domain)) +} + // CertURI impl. func (id *SpiffeIDSigning) Authorize(ixn *structs.Intention) (bool, bool) { // Never authorize as a client. return false, true } +// CanSign takes any CertURI and returns whether or not this signing entity is +// allowed to sign CSRs for that entity (i.e. represents the trust domain for +// that entity). +// +// I choose to make this a fixed centralised method here for now rather than a +// 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 "whitelist" semantics. +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 + // that we could open this up later for example to support external + // federation of roots and cross-signing external roots that have different + // URI structure but it's simpler to start off restrictive. + return id == other + case *SpiffeIDService: + // The host component of the service must be an exact match for now under + // ascii case folding (since hostnames are case-insensitive). Later we might + // worry about Unicode domains if we start allowing customisation beyond the + // built-in cluster ids. + return strings.ToLower(other.Host) == id.Host() + default: + return false + } +} + // SpiffeIDSigningForCluster returns the SPIFFE signing identifier (trust // domain) representation of the given CA config. // diff --git a/agent/connect/uri_signing_test.go b/agent/connect/uri_signing_test.go index 98babbc2d9..2d8975858c 100644 --- a/agent/connect/uri_signing_test.go +++ b/agent/connect/uri_signing_test.go @@ -1,6 +1,8 @@ package connect import ( + "net/url" + "strings" "testing" "github.com/hashicorp/consul/agent/structs" @@ -24,3 +26,98 @@ func TestSpiffeIDSigningForCluster(t *testing.T) { id := SpiffeIDSigningForCluster(config) assert.Equal(t, id.URI().String(), "spiffe://"+testClusterID+".consul") } + +// fakeCertURI is a CertURI implementation that our implementation doesn't know +// about +type fakeCertURI string + +func (f fakeCertURI) Authorize(*structs.Intention) (auth bool, match bool) { + return false, false +} + +func (f fakeCertURI) URI() *url.URL { + u, _ := url.Parse(string(f)) + return u +} +func TestSpiffeIDSigning_CanSign(t *testing.T) { + + testSigning := &SpiffeIDSigning{ + ClusterID: testClusterID, + Domain: "consul", + } + + tests := []struct { + name string + id *SpiffeIDSigning + input CertURI + want bool + }{ + { + name: "same signing ID", + id: testSigning, + input: testSigning, + want: true, + }, + { + name: "other signing ID", + id: testSigning, + input: &SpiffeIDSigning{ + ClusterID: "fakedomain", + Domain: "consul", + }, + want: false, + }, + { + name: "different TLD signing ID", + id: testSigning, + input: &SpiffeIDSigning{ + ClusterID: testClusterID, + Domain: "evil", + }, + want: false, + }, + { + name: "nil", + id: testSigning, + input: nil, + want: false, + }, + { + name: "unrecognised CertURI implementation", + id: testSigning, + input: fakeCertURI("spiffe://foo.bar/baz"), + want: false, + }, + { + name: "service - good", + id: testSigning, + input: &SpiffeIDService{testClusterID + ".consul", "default", "dc1", "web"}, + want: true, + }, + { + name: "service - good midex case", + id: testSigning, + input: &SpiffeIDService{strings.ToUpper(testClusterID) + ".CONsuL", "defAUlt", "dc1", "WEB"}, + want: true, + }, + { + name: "service - different cluster", + id: testSigning, + input: &SpiffeIDService{"55555555-4444-3333-2222-111111111111.consul", "default", "dc1", "web"}, + want: false, + }, + { + name: "service - different TLD", + id: testSigning, + input: &SpiffeIDService{testClusterID + ".fake", "default", "dc1", "web"}, + want: false, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got := tt.id.CanSign(tt.input) + assert.Equal(t, tt.want, got) + }) + } +} diff --git a/agent/consul/connect_ca_endpoint.go b/agent/consul/connect_ca_endpoint.go index f70c0d3a3e..a72a4998b4 100644 --- a/agent/consul/connect_ca_endpoint.go +++ b/agent/consul/connect_ca_endpoint.go @@ -225,13 +225,8 @@ func (s *ConnectCA) Roots( return err } // Build TrustDomain based on the ClusterID stored. - spiffeID := connect.SpiffeIDSigningForCluster(config) - uri := spiffeID.URI() - if uri == nil { - // Impossible(tm) but let's not panic - return errors.New("no trust domain found") - } - reply.TrustDomain = uri.Host + signingID := connect.SpiffeIDSigningForCluster(config) + reply.TrustDomain = signingID.Host() } return s.srv.blockingQuery( @@ -297,11 +292,11 @@ func (s *ConnectCA) Sign( } // Parse the SPIFFE ID - spiffeId, err := connect.ParseCertURI(csr.URIs[0]) + spiffeID, err := connect.ParseCertURI(csr.URIs[0]) if err != nil { return err } - serviceId, ok := spiffeId.(*connect.SpiffeIDService) + serviceID, ok := spiffeID.(*connect.SpiffeIDService) if !ok { return fmt.Errorf("SPIFFE ID in CSR must be a service ID") } @@ -311,7 +306,35 @@ func (s *ConnectCA) Sign( return fmt.Errorf("internal error: CA provider is nil") } - // todo(kyhavlov): more validation on the CSR before signing + // Verify that the CSR entity is in the cluster's trust domain + state := s.srv.fsm.State() + _, config, err := state.CAConfig() + if err != nil { + return err + } + signingID := connect.SpiffeIDSigningForCluster(config) + if !signingID.CanSign(serviceID) { + return fmt.Errorf("SPIFFE ID in CSR from a different trust domain: %s, "+ + "we are %s", serviceID.Host, signingID.Host()) + } + + // Verify that the ACL token provided has permission to act as this service + rule, err := s.srv.resolveToken(args.Token) + if err != nil { + return err + } + if rule != nil && !rule.ServiceWrite(serviceID.Service, nil) { + return acl.ErrPermissionDenied + } + + // Verify that the DC in the service URI matches us. We might relax this + // requirement later but being restrictive for now is safer. + if serviceID.Datacenter != s.srv.config.Datacenter { + return fmt.Errorf("SPIFFE ID in CSR from a different datacenter: %s, "+ + "we are %s", serviceID.Datacenter, s.srv.config.Datacenter) + } + + // All seems to be in order, actually sign it. pem, err := provider.Sign(csr) if err != nil { return err @@ -322,9 +345,10 @@ func (s *ConnectCA) Sign( // the built-in provider being supported and the implementation detail that we // have to write a SerialIndex update to the provider config table for every // cert issued so in all cases this index will be higher than any previous - // sign response. This has to happen after the provider.Sign call to observe - // the index update. - modIdx, _, err := s.srv.fsm.State().CAConfig() + // sign response. This has to be reloaded after the provider.Sign call to + // observe the index update. + state = s.srv.fsm.State() + modIdx, _, err := state.CAConfig() if err != nil { return err } @@ -338,7 +362,7 @@ func (s *ConnectCA) Sign( *reply = structs.IssuedCert{ SerialNumber: connect.HexString(cert.SerialNumber.Bytes()), CertPEM: pem, - Service: serviceId.Service, + Service: serviceID.Service, ServiceURI: cert.URIs[0].String(), ValidAfter: cert.NotBefore, ValidBefore: cert.NotAfter, diff --git a/agent/consul/connect_ca_endpoint_test.go b/agent/consul/connect_ca_endpoint_test.go index 655b1d7f4f..f209358777 100644 --- a/agent/consul/connect_ca_endpoint_test.go +++ b/agent/consul/connect_ca_endpoint_test.go @@ -241,6 +241,7 @@ func TestConnectCASign(t *testing.T) { t.Parallel() assert := assert.New(t) + require := require.New(t) dir1, s1 := testServer(t) defer os.RemoveAll(dir1) defer s1.Shutdown() @@ -251,30 +252,133 @@ func TestConnectCASign(t *testing.T) { // Generate a CSR and request signing spiffeId := connect.TestSpiffeIDService(t, "web") + spiffeId.Host = testGetClusterTrustDomain(t, s1) csr, _ := connect.TestCSR(t, spiffeId) args := &structs.CASignRequest{ Datacenter: "dc1", CSR: csr, } var reply structs.IssuedCert - assert.NoError(msgpackrpc.CallWithCodec(codec, "ConnectCA.Sign", args, &reply)) + require.NoError(msgpackrpc.CallWithCodec(codec, "ConnectCA.Sign", args, &reply)) // Get the current CA state := s1.fsm.State() _, ca, err := state.CARootActive(nil) - assert.NoError(err) + require.NoError(err) // Verify that the cert is signed by the CA roots := x509.NewCertPool() assert.True(roots.AppendCertsFromPEM([]byte(ca.RootCert))) leaf, err := connect.ParseCert(reply.CertPEM) - assert.NoError(err) + require.NoError(err) _, err = leaf.Verify(x509.VerifyOptions{ Roots: roots, }) - assert.NoError(err) + require.NoError(err) // Verify other fields assert.Equal("web", reply.Service) assert.Equal(spiffeId.URI().String(), reply.ServiceURI) } + +func testGetClusterTrustDomain(t *testing.T, s *Server) string { + t.Helper() + state := s.fsm.State() + _, config, err := state.CAConfig() + require.NoError(t, err) + // Build TrustDomain based on the ClusterID stored. + signingID := connect.SpiffeIDSigningForCluster(config) + return signingID.Host() +} + +func TestConnectCASignValidation(t *testing.T) { + t.Parallel() + + dir1, s1 := testServerWithConfig(t, func(c *Config) { + c.ACLDatacenter = "dc1" + c.ACLMasterToken = "root" + c.ACLDefaultPolicy = "deny" + }) + defer os.RemoveAll(dir1) + defer s1.Shutdown() + codec := rpcClient(t, s1) + defer codec.Close() + + testrpc.WaitForLeader(t, s1.RPC, "dc1") + + // Create an ACL token with service:write for web* + var webToken string + { + arg := structs.ACLRequest{ + Datacenter: "dc1", + Op: structs.ACLSet, + ACL: structs.ACL{ + Name: "User token", + Type: structs.ACLTypeClient, + Rules: ` + service "web" { + policy = "write" + }`, + }, + WriteRequest: structs.WriteRequest{Token: "root"}, + } + require.NoError(t, msgpackrpc.CallWithCodec(codec, "ACL.Apply", &arg, &webToken)) + } + + trustDomain := testGetClusterTrustDomain(t, s1) + + tests := []struct { + name string + id connect.CertURI + wantErr string + }{ + { + name: "different cluster", + id: &connect.SpiffeIDService{ + "55555555-4444-3333-2222-111111111111.consul", + "default", "dc1", "web"}, + wantErr: "different trust domain", + }, + { + name: "same cluster should validate", + id: &connect.SpiffeIDService{ + trustDomain, + "default", "dc1", "web"}, + wantErr: "", + }, + { + name: "same cluster, CSR for a different DC should NOT validate", + id: &connect.SpiffeIDService{ + trustDomain, + "default", "dc2", "web"}, + wantErr: "different datacenter", + }, + { + name: "same cluster and DC, different service should not have perms", + id: &connect.SpiffeIDService{ + trustDomain, + "default", "dc1", "db"}, + wantErr: "Permission denied", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + csr, _ := connect.TestCSR(t, tt.id) + args := &structs.CASignRequest{ + Datacenter: "dc1", + CSR: csr, + WriteRequest: structs.WriteRequest{Token: webToken}, + } + var reply structs.IssuedCert + err := msgpackrpc.CallWithCodec(codec, "ConnectCA.Sign", args, &reply) + if tt.wantErr == "" { + require.NoError(t, err) + // No other validation that is handled in different tests + } else { + require.Error(t, err) + require.Contains(t, err.Error(), tt.wantErr) + } + }) + } +}