Add CSR check for number of URIs. (#14579)

Add CSR check for number of URIs.

.changelog/14579.txt

@@ -0,0 +1,3 @@
+```release-note:security
+connect: Added URI length checks to ConnectCA CSR requests. Prior to this change, it was possible for a malicious actor to designate multiple SAN URI values in a call to the `ConnectCA.Sign` endpoint. The endpoint now only allows for exactly one SAN URI to be specified.
+```

agent/consul/auto_config_endpoint.go

@@ -394,9 +394,12 @@ func parseAutoConfigCSR(csr string) (*x509.CertificateRequest, *connect.SpiffeID
 		return nil, nil, fmt.Errorf("Failed to parse CSR: %w", err)
 	}
 
-	// ensure that a URI SAN is present
+	// ensure that exactly one URI SAN is present
-	if len(x509CSR.URIs) < 1 {
+	if len(x509CSR.URIs) != 1 {
-		return nil, nil, fmt.Errorf("CSR didn't include any URI SANs")
+		return nil, nil, fmt.Errorf("CSR SAN contains an invalid number of URIs: %v", len(x509CSR.URIs))
+	}
+	if len(x509CSR.EmailAddresses) > 0 {
+		return nil, nil, fmt.Errorf("CSR SAN does not allow specifying email addresses")
 	}
 
 	// Parse the SPIFFE ID

agent/consul/auto_config_endpoint_test.go

@@ -1,12 +1,17 @@
 package consul
 
 import (
+	"bytes"
+	"crypto"
+	crand "crypto/rand"
 	"crypto/x509"
 	"encoding/base64"
+	"encoding/pem"
 	"fmt"
 	"io/ioutil"
 	"math/rand"
 	"net"
+	"net/url"
 	"path"
 	"testing"
 	"time"
@@ -874,6 +879,126 @@ func TestAutoConfig_updateJoinAddressesInConfig(t *testing.T) {
 	backend.AssertExpectations(t)
 }
 
+func TestAutoConfig_parseAutoConfigCSR(t *testing.T) {
+	// createCSR copies the behavior of connect.CreateCSR with some
+	// customizations to allow for better unit testing.
+	createCSR := func(tmpl *x509.CertificateRequest, privateKey crypto.Signer) (string, error) {
+		connect.HackSANExtensionForCSR(tmpl)
+		bs, err := x509.CreateCertificateRequest(crand.Reader, tmpl, privateKey)
+		require.NoError(t, err)
+		var csrBuf bytes.Buffer
+		err = pem.Encode(&csrBuf, &pem.Block{Type: "CERTIFICATE REQUEST", Bytes: bs})
+		require.NoError(t, err)
+		return csrBuf.String(), nil
+	}
+	pk, _, err := connect.GeneratePrivateKey()
+	require.NoError(t, err)
+
+	agentURI := connect.SpiffeIDAgent{
+		Host:       "test-host",
+		Datacenter: "tdc1",
+		Agent:      "test-agent",
+	}.URI()
+
+	tests := []struct {
+		name      string
+		setup     func() string
+		expectErr string
+	}{
+		{
+			name:      "err_garbage_data",
+			expectErr: "Failed to parse CSR",
+			setup:     func() string { return "garbage" },
+		},
+		{
+			name:      "err_not_one_uri",
+			expectErr: "CSR SAN contains an invalid number of URIs",
+			setup: func() string {
+				tmpl := &x509.CertificateRequest{
+					URIs:               []*url.URL{agentURI, agentURI},
+					SignatureAlgorithm: connect.SigAlgoForKey(pk),
+				}
+				csr, err := createCSR(tmpl, pk)
+				require.NoError(t, err)
+				return csr
+			},
+		},
+		{
+			name:      "err_email",
+			expectErr: "CSR SAN does not allow specifying email addresses",
+			setup: func() string {
+				tmpl := &x509.CertificateRequest{
+					URIs:               []*url.URL{agentURI},
+					EmailAddresses:     []string{"test@example.com"},
+					SignatureAlgorithm: connect.SigAlgoForKey(pk),
+				}
+				csr, err := createCSR(tmpl, pk)
+				require.NoError(t, err)
+				return csr
+			},
+		},
+		{
+			name:      "err_spiffe_parse_uri",
+			expectErr: "Failed to parse the SPIFFE URI",
+			setup: func() string {
+				tmpl := &x509.CertificateRequest{
+					URIs:               []*url.URL{connect.SpiffeIDAgent{}.URI()},
+					SignatureAlgorithm: connect.SigAlgoForKey(pk),
+				}
+				csr, err := createCSR(tmpl, pk)
+				require.NoError(t, err)
+				return csr
+			},
+		},
+		{
+			name:      "err_not_agent",
+			expectErr: "SPIFFE ID is not an Agent ID",
+			setup: func() string {
+				spiffe := connect.SpiffeIDService{
+					Namespace:  "tns",
+					Datacenter: "tdc1",
+					Service:    "test-service",
+				}
+				tmpl := &x509.CertificateRequest{
+					URIs:               []*url.URL{spiffe.URI()},
+					SignatureAlgorithm: connect.SigAlgoForKey(pk),
+				}
+				csr, err := createCSR(tmpl, pk)
+				require.NoError(t, err)
+				return csr
+			},
+		},
+		{
+			name: "success",
+			setup: func() string {
+				tmpl := &x509.CertificateRequest{
+					URIs:               []*url.URL{agentURI},
+					SignatureAlgorithm: connect.SigAlgoForKey(pk),
+				}
+				csr, err := createCSR(tmpl, pk)
+				require.NoError(t, err)
+				return csr
+			},
+		},
+	}
+
+	for _, tc := range tests {
+		t.Run(tc.name, func(t *testing.T) {
+			req, spif, err := parseAutoConfigCSR(tc.setup())
+			if tc.expectErr != "" {
+				require.Error(t, err)
+				require.Contains(t, err.Error(), tc.expectErr)
+			} else {
+				require.NoError(t, err)
+				// TODO better verification of these
+				require.NotNil(t, req)
+				require.NotNil(t, spif)
+			}
+
+		})
+	}
+}
+
 func TestAutoConfig_invalidSegmentName(t *testing.T) {
 	invalid := []string{
 		"\n",

agent/consul/leader_connect_ca.go

@@ -1406,10 +1406,15 @@ func (l *connectSignRateLimiter) getCSRRateLimiterWithLimit(limit rate.Limit) *r
 // identified by the SPIFFE ID in the given CSR's SAN. It performs authorization
 // using the given acl.Authorizer.
 func (c *CAManager) AuthorizeAndSignCertificate(csr *x509.CertificateRequest, authz acl.Authorizer) (*structs.IssuedCert, error) {
-	// Parse the SPIFFE ID from the CSR SAN.
+	// Note that only one spiffe id is allowed currently. If more than one is desired
-	if len(csr.URIs) == 0 {
+	// in future implmentations, then each ID should have authorization checks.
-		return nil, connect.InvalidCSRError("CSR SAN does not contain a SPIFFE ID")
+	if len(csr.URIs) != 1 {
+		return nil, connect.InvalidCSRError("CSR SAN contains an invalid number of URIs: %v", len(csr.URIs))
 	}
+	if len(csr.EmailAddresses) > 0 {
+		return nil, connect.InvalidCSRError("CSR SAN does not allow specifying email addresses")
+	}
+	// Parse the SPIFFE ID from the CSR SAN.
 	spiffeID, err := connect.ParseCertURI(csr.URIs[0])
 	if err != nil {
 		return nil, err
@@ -1465,7 +1470,7 @@ func (c *CAManager) AuthorizeAndSignCertificate(csr *x509.CertificateRequest, au
 				"we are %s", v.Datacenter, dc)
 		}
 	default:
-		return nil, connect.InvalidCSRError("SPIFFE ID in CSR must be a service or agent ID")
+		return nil, connect.InvalidCSRError("SPIFFE ID in CSR must be a service, mesh-gateway, or agent ID")
 	}
 
 	return c.SignCertificate(csr, spiffeID)

agent/consul/leader_connect_ca_test.go

@@ -24,6 +24,7 @@ import (
 	msgpackrpc "github.com/hashicorp/consul-net-rpc/net-rpc-msgpackrpc"
 	"github.com/hashicorp/consul-net-rpc/net/rpc"
 
+	"github.com/hashicorp/consul/acl"
 	"github.com/hashicorp/consul/agent/connect"
 	ca "github.com/hashicorp/consul/agent/connect/ca"
 	"github.com/hashicorp/consul/agent/consul/fsm"
@@ -1082,3 +1083,140 @@ func TestCAManager_Sign_SpiffeIDServer(t *testing.T) {
 	// Verify the Server's URI.
 	require.Equal(t, fmt.Sprintf("spiffe://%s/agent/server/dc/dc1", roots.TrustDomain), cert.ServerURI)
 }
+
+func TestCAManager_AuthorizeAndSignCertificate(t *testing.T) {
+	conf := DefaultConfig()
+	conf.PrimaryDatacenter = "dc1"
+	conf.Datacenter = "dc2"
+	manager := NewCAManager(nil, nil, testutil.Logger(t), conf)
+
+	agentURL := connect.SpiffeIDAgent{
+		Agent:      "test-agent",
+		Datacenter: conf.PrimaryDatacenter,
+		Host:       "test-host",
+	}.URI()
+	serviceURL := connect.SpiffeIDService{
+		Datacenter: conf.PrimaryDatacenter,
+		Namespace:  "ns1",
+		Service:    "test-service",
+	}.URI()
+	meshURL := connect.SpiffeIDMeshGateway{
+		Datacenter: conf.PrimaryDatacenter,
+		Host:       "test-host",
+		Partition:  "test-partition",
+	}.URI()
+
+	tests := []struct {
+		name      string
+		expectErr string
+		getCSR    func() *x509.CertificateRequest
+		authAllow bool
+	}{
+		{
+			name:      "err_not_one_uri",
+			expectErr: "CSR SAN contains an invalid number of URIs",
+			getCSR: func() *x509.CertificateRequest {
+				return &x509.CertificateRequest{
+					URIs: []*url.URL{agentURL, agentURL},
+				}
+			},
+		},
+		{
+			name:      "err_email",
+			expectErr: "CSR SAN does not allow specifying email addresses",
+			getCSR: func() *x509.CertificateRequest {
+				return &x509.CertificateRequest{
+					URIs:           []*url.URL{agentURL},
+					EmailAddresses: []string{"test@example.com"},
+				}
+			},
+		},
+		{
+			name:      "err_invalid_spiffe_id",
+			expectErr: "SPIFFE ID is not in the expected format",
+			getCSR: func() *x509.CertificateRequest {
+				return &x509.CertificateRequest{
+					URIs: []*url.URL{connect.SpiffeIDAgent{}.URI()},
+				}
+			},
+		},
+		{
+			name:      "err_service_write_not_allowed",
+			expectErr: "Permission denied",
+			getCSR: func() *x509.CertificateRequest {
+				return &x509.CertificateRequest{
+					URIs: []*url.URL{serviceURL},
+				}
+			},
+		},
+		{
+			name:      "err_service_different_dc",
+			expectErr: "SPIFFE ID in CSR from a different datacenter",
+			authAllow: true,
+			getCSR: func() *x509.CertificateRequest {
+				return &x509.CertificateRequest{
+					URIs: []*url.URL{serviceURL},
+				}
+			},
+		},
+		{
+			name:      "err_agent_write_not_allowed",
+			expectErr: "Permission denied",
+			getCSR: func() *x509.CertificateRequest {
+				return &x509.CertificateRequest{
+					URIs: []*url.URL{agentURL},
+				}
+			},
+		},
+		{
+			name:      "err_meshgw_write_not_allowed",
+			expectErr: "Permission denied",
+			getCSR: func() *x509.CertificateRequest {
+				return &x509.CertificateRequest{
+					URIs: []*url.URL{meshURL},
+				}
+			},
+		},
+		{
+			name:      "err_meshgw_different_dc",
+			expectErr: "SPIFFE ID in CSR from a different datacenter",
+			authAllow: true,
+			getCSR: func() *x509.CertificateRequest {
+				return &x509.CertificateRequest{
+					URIs: []*url.URL{meshURL},
+				}
+			},
+		},
+		{
+			name:      "err_invalid_spiffe_type",
+			expectErr: "SPIFFE ID in CSR must be a service, mesh-gateway, or agent ID",
+			getCSR: func() *x509.CertificateRequest {
+				u := connect.SpiffeIDSigning{
+					ClusterID: "test-cluster-id",
+					Domain:    "test-domain",
+				}.URI()
+				return &x509.CertificateRequest{
+					URIs: []*url.URL{u},
+				}
+			},
+		},
+	}
+
+	for _, tc := range tests {
+		t.Run(tc.name, func(t *testing.T) {
+			authz := acl.DenyAll()
+			if tc.authAllow {
+				authz = acl.AllowAll()
+			}
+
+			cert, err := manager.AuthorizeAndSignCertificate(tc.getCSR(), authz)
+			if tc.expectErr != "" {
+				require.Error(t, err)
+				require.Contains(t, err.Error(), tc.expectErr)
+			} else {
+				require.NoError(t, err)
+				require.NotNil(t, cert)
+			}
+		})
+	}
+}