From 5d1487e167cbe8ae93ce90a1f1927eda77dc80b7 Mon Sep 17 00:00:00 2001 From: Derek Menteer <105233703+hashi-derek@users.noreply.github.com> Date: Tue, 13 Sep 2022 14:21:47 -0500 Subject: [PATCH] Add CSR check for number of URIs. (#14579) Add CSR check for number of URIs. --- .changelog/14579.txt | 3 + agent/consul/auto_config_endpoint.go | 9 +- agent/consul/auto_config_endpoint_test.go | 125 ++++++++++++++++++++ agent/consul/leader_connect_ca.go | 13 +- agent/consul/leader_connect_ca_test.go | 138 ++++++++++++++++++++++ 5 files changed, 281 insertions(+), 7 deletions(-) create mode 100644 .changelog/14579.txt diff --git a/.changelog/14579.txt b/.changelog/14579.txt new file mode 100644 index 000000000..e03c0dc37 --- /dev/null +++ b/.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. +``` \ No newline at end of file diff --git a/agent/consul/auto_config_endpoint.go b/agent/consul/auto_config_endpoint.go index 32b2b6a53..7eda55b67 100644 --- a/agent/consul/auto_config_endpoint.go +++ b/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 - 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 diff --git a/agent/consul/auto_config_endpoint_test.go b/agent/consul/auto_config_endpoint_test.go index 782e3cdb4..1036044fa 100644 --- a/agent/consul/auto_config_endpoint_test.go +++ b/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", diff --git a/agent/consul/leader_connect_ca.go b/agent/consul/leader_connect_ca.go index 4de55ba03..8f32f6e2c 100644 --- a/agent/consul/leader_connect_ca.go +++ b/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. - if len(csr.URIs) == 0 { - return nil, connect.InvalidCSRError("CSR SAN does not contain a SPIFFE ID") + // Note that only one spiffe id is allowed currently. If more than one is desired + // in future implmentations, then each ID should have authorization checks. + 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) diff --git a/agent/consul/leader_connect_ca_test.go b/agent/consul/leader_connect_ca_test.go index 9de808bd1..d78f38d9e 100644 --- a/agent/consul/leader_connect_ca_test.go +++ b/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) + } + }) + } +}