Add CSR signing verification of service ACL, trust domain and datacenter.

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.
 //

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)
+		})
+	}
+}

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,

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)
+			}
+		})
+	}
+}