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

.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

@@ -406,9 +406,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
-	if len(x509CSR.URIs) < 1 {
-		return nil, nil, fmt.Errorf("CSR didn't include any URI SANs")
+	// ensure that exactly one URI SAN is present
+	if len(x509CSR.URIs) != 1 {
+		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"
@@ -876,6 +881,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",