Skip to content

Commit 8f6fb4f

Browse files
hashi-derekhashi-derek
committed
Backport #14579 to 1.13
1 parent 02ebc1d commit 8f6fb4f

5 files changed

+281
-7
lines changed

‎.changelog/14579.txt

+3
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,3 @@
1+
```release-note:security
2+
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.
3+
```

‎agent/consul/auto_config_endpoint.go

+6-3
Original file line numberDiff line numberDiff line change
@@ -394,9 +394,12 @@ func parseAutoConfigCSR(csr string) (*x509.CertificateRequest, *connect.SpiffeID
394394
return nil, nil, fmt.Errorf("Failed to parse CSR: %w", err)
395395
}
396396

397-
// ensure that a URI SAN is present
398-
if len(x509CSR.URIs) < 1 {
399-
return nil, nil, fmt.Errorf("CSR didn't include any URI SANs")
397+
// ensure that exactly one URI SAN is present
398+
if len(x509CSR.URIs) != 1 {
399+
return nil, nil, fmt.Errorf("CSR SAN contains an invalid number of URIs: %v", len(x509CSR.URIs))
400+
}
401+
if len(x509CSR.EmailAddresses) > 0 {
402+
return nil, nil, fmt.Errorf("CSR SAN does not allow specifying email addresses")
400403
}
401404

402405
// Parse the SPIFFE ID

‎agent/consul/auto_config_endpoint_test.go

+125
Original file line numberDiff line numberDiff line change
@@ -1,12 +1,17 @@
11
package consul
22

33
import (
4+
"bytes"
5+
"crypto"
6+
crand "crypto/rand"
47
"crypto/x509"
58
"encoding/base64"
9+
"encoding/pem"
610
"fmt"
711
"io/ioutil"
812
"math/rand"
913
"net"
14+
"net/url"
1015
"path"
1116
"testing"
1217
"time"
@@ -874,6 +879,126 @@ func TestAutoConfig_updateJoinAddressesInConfig(t *testing.T) {
874879
backend.AssertExpectations(t)
875880
}
876881

882+
func TestAutoConfig_parseAutoConfigCSR(t *testing.T) {
883+
// createCSR copies the behavior of connect.CreateCSR with some
884+
// customizations to allow for better unit testing.
885+
createCSR := func(tmpl *x509.CertificateRequest, privateKey crypto.Signer) (string, error) {
886+
connect.HackSANExtensionForCSR(tmpl)
887+
bs, err := x509.CreateCertificateRequest(crand.Reader, tmpl, privateKey)
888+
require.NoError(t, err)
889+
var csrBuf bytes.Buffer
890+
err = pem.Encode(&csrBuf, &pem.Block{Type: "CERTIFICATE REQUEST", Bytes: bs})
891+
require.NoError(t, err)
892+
return csrBuf.String(), nil
893+
}
894+
pk, _, err := connect.GeneratePrivateKey()
895+
require.NoError(t, err)
896+
897+
agentURI := connect.SpiffeIDAgent{
898+
Host: "test-host",
899+
Datacenter: "tdc1",
900+
Agent: "test-agent",
901+
}.URI()
902+
903+
tests := []struct {
904+
name string
905+
setup func() string
906+
expectErr string
907+
}{
908+
{
909+
name: "err_garbage_data",
910+
expectErr: "Failed to parse CSR",
911+
setup: func() string { return "garbage" },
912+
},
913+
{
914+
name: "err_not_one_uri",
915+
expectErr: "CSR SAN contains an invalid number of URIs",
916+
setup: func() string {
917+
tmpl := &x509.CertificateRequest{
918+
URIs: []*url.URL{agentURI, agentURI},
919+
SignatureAlgorithm: connect.SigAlgoForKey(pk),
920+
}
921+
csr, err := createCSR(tmpl, pk)
922+
require.NoError(t, err)
923+
return csr
924+
},
925+
},
926+
{
927+
name: "err_email",
928+
expectErr: "CSR SAN does not allow specifying email addresses",
929+
setup: func() string {
930+
tmpl := &x509.CertificateRequest{
931+
URIs: []*url.URL{agentURI},
932+
EmailAddresses: []string{"test@example.com"},
933+
SignatureAlgorithm: connect.SigAlgoForKey(pk),
934+
}
935+
csr, err := createCSR(tmpl, pk)
936+
require.NoError(t, err)
937+
return csr
938+
},
939+
},
940+
{
941+
name: "err_spiffe_parse_uri",
942+
expectErr: "Failed to parse the SPIFFE URI",
943+
setup: func() string {
944+
tmpl := &x509.CertificateRequest{
945+
URIs: []*url.URL{connect.SpiffeIDAgent{}.URI()},
946+
SignatureAlgorithm: connect.SigAlgoForKey(pk),
947+
}
948+
csr, err := createCSR(tmpl, pk)
949+
require.NoError(t, err)
950+
return csr
951+
},
952+
},
953+
{
954+
name: "err_not_agent",
955+
expectErr: "SPIFFE ID is not an Agent ID",
956+
setup: func() string {
957+
spiffe := connect.SpiffeIDService{
958+
Namespace: "tns",
959+
Datacenter: "tdc1",
960+
Service: "test-service",
961+
}
962+
tmpl := &x509.CertificateRequest{
963+
URIs: []*url.URL{spiffe.URI()},
964+
SignatureAlgorithm: connect.SigAlgoForKey(pk),
965+
}
966+
csr, err := createCSR(tmpl, pk)
967+
require.NoError(t, err)
968+
return csr
969+
},
970+
},
971+
{
972+
name: "success",
973+
setup: func() string {
974+
tmpl := &x509.CertificateRequest{
975+
URIs: []*url.URL{agentURI},
976+
SignatureAlgorithm: connect.SigAlgoForKey(pk),
977+
}
978+
csr, err := createCSR(tmpl, pk)
979+
require.NoError(t, err)
980+
return csr
981+
},
982+
},
983+
}
984+
985+
for _, tc := range tests {
986+
t.Run(tc.name, func(t *testing.T) {
987+
req, spif, err := parseAutoConfigCSR(tc.setup())
988+
if tc.expectErr != "" {
989+
require.Error(t, err)
990+
require.Contains(t, err.Error(), tc.expectErr)
991+
} else {
992+
require.NoError(t, err)
993+
// TODO better verification of these
994+
require.NotNil(t, req)
995+
require.NotNil(t, spif)
996+
}
997+
998+
})
999+
}
1000+
}
1001+
8771002
func TestAutoConfig_invalidSegmentName(t *testing.T) {
8781003
invalid := []string{
8791004
"\n",

‎agent/consul/leader_connect_ca.go

+9-4
Original file line numberDiff line numberDiff line change
@@ -1406,10 +1406,15 @@ func (l *connectSignRateLimiter) getCSRRateLimiterWithLimit(limit rate.Limit) *r
14061406
// identified by the SPIFFE ID in the given CSR's SAN. It performs authorization
14071407
// using the given acl.Authorizer.
14081408
func (c *CAManager) AuthorizeAndSignCertificate(csr *x509.CertificateRequest, authz acl.Authorizer) (*structs.IssuedCert, error) {
1409-
// Parse the SPIFFE ID from the CSR SAN.
1410-
if len(csr.URIs) == 0 {
1411-
return nil, connect.InvalidCSRError("CSR SAN does not contain a SPIFFE ID")
1409+
// Note that only one spiffe id is allowed currently. If more than one is desired
1410+
// in future implmentations, then each ID should have authorization checks.
1411+
if len(csr.URIs) != 1 {
1412+
return nil, connect.InvalidCSRError("CSR SAN contains an invalid number of URIs: %v", len(csr.URIs))
1413+
}
1414+
if len(csr.EmailAddresses) > 0 {
1415+
return nil, connect.InvalidCSRError("CSR SAN does not allow specifying email addresses")
14121416
}
1417+
// Parse the SPIFFE ID from the CSR SAN.
14131418
spiffeID, err := connect.ParseCertURI(csr.URIs[0])
14141419
if err != nil {
14151420
return nil, err
@@ -1452,7 +1457,7 @@ func (c *CAManager) AuthorizeAndSignCertificate(csr *x509.CertificateRequest, au
14521457
"we are %s", v.Datacenter, dc)
14531458
}
14541459
default:
1455-
return nil, connect.InvalidCSRError("SPIFFE ID in CSR must be a service or agent ID")
1460+
return nil, connect.InvalidCSRError("SPIFFE ID in CSR must be a service, mesh-gateway, or agent ID")
14561461
}
14571462

14581463
return c.SignCertificate(csr, spiffeID)

‎agent/consul/leader_connect_ca_test.go

+138
Original file line numberDiff line numberDiff line change
@@ -24,6 +24,7 @@ import (
2424
msgpackrpc "github.com/hashicorp/consul-net-rpc/net-rpc-msgpackrpc"
2525
"github.com/hashicorp/consul-net-rpc/net/rpc"
2626

27+
"github.com/hashicorp/consul/acl"
2728
"github.com/hashicorp/consul/agent/connect"
2829
ca "github.com/hashicorp/consul/agent/connect/ca"
2930
"github.com/hashicorp/consul/agent/consul/fsm"
@@ -1042,3 +1043,140 @@ func setupPrimaryCA(t *testing.T, client *vaultapi.Client, path string, rootPEM
10421043
require.NoError(t, err, "failed to set signed intermediate")
10431044
return lib.EnsureTrailingNewline(buf.String())
10441045
}
1046+
1047+
func TestCAManager_AuthorizeAndSignCertificate(t *testing.T) {
1048+
conf := DefaultConfig()
1049+
conf.PrimaryDatacenter = "dc1"
1050+
conf.Datacenter = "dc2"
1051+
manager := NewCAManager(nil, nil, testutil.Logger(t), conf)
1052+
1053+
agentURL := connect.SpiffeIDAgent{
1054+
Agent: "test-agent",
1055+
Datacenter: conf.PrimaryDatacenter,
1056+
Host: "test-host",
1057+
}.URI()
1058+
serviceURL := connect.SpiffeIDService{
1059+
Datacenter: conf.PrimaryDatacenter,
1060+
Namespace: "ns1",
1061+
Service: "test-service",
1062+
}.URI()
1063+
meshURL := connect.SpiffeIDMeshGateway{
1064+
Datacenter: conf.PrimaryDatacenter,
1065+
Host: "test-host",
1066+
Partition: "test-partition",
1067+
}.URI()
1068+
1069+
tests := []struct {
1070+
name string
1071+
expectErr string
1072+
getCSR func() *x509.CertificateRequest
1073+
authAllow bool
1074+
}{
1075+
{
1076+
name: "err_not_one_uri",
1077+
expectErr: "CSR SAN contains an invalid number of URIs",
1078+
getCSR: func() *x509.CertificateRequest {
1079+
return &x509.CertificateRequest{
1080+
URIs: []*url.URL{agentURL, agentURL},
1081+
}
1082+
},
1083+
},
1084+
{
1085+
name: "err_email",
1086+
expectErr: "CSR SAN does not allow specifying email addresses",
1087+
getCSR: func() *x509.CertificateRequest {
1088+
return &x509.CertificateRequest{
1089+
URIs: []*url.URL{agentURL},
1090+
EmailAddresses: []string{"test@example.com"},
1091+
}
1092+
},
1093+
},
1094+
{
1095+
name: "err_invalid_spiffe_id",
1096+
expectErr: "SPIFFE ID is not in the expected format",
1097+
getCSR: func() *x509.CertificateRequest {
1098+
return &x509.CertificateRequest{
1099+
URIs: []*url.URL{connect.SpiffeIDAgent{}.URI()},
1100+
}
1101+
},
1102+
},
1103+
{
1104+
name: "err_service_write_not_allowed",
1105+
expectErr: "Permission denied",
1106+
getCSR: func() *x509.CertificateRequest {
1107+
return &x509.CertificateRequest{
1108+
URIs: []*url.URL{serviceURL},
1109+
}
1110+
},
1111+
},
1112+
{
1113+
name: "err_service_different_dc",
1114+
expectErr: "SPIFFE ID in CSR from a different datacenter",
1115+
authAllow: true,
1116+
getCSR: func() *x509.CertificateRequest {
1117+
return &x509.CertificateRequest{
1118+
URIs: []*url.URL{serviceURL},
1119+
}
1120+
},
1121+
},
1122+
{
1123+
name: "err_agent_write_not_allowed",
1124+
expectErr: "Permission denied",
1125+
getCSR: func() *x509.CertificateRequest {
1126+
return &x509.CertificateRequest{
1127+
URIs: []*url.URL{agentURL},
1128+
}
1129+
},
1130+
},
1131+
{
1132+
name: "err_meshgw_write_not_allowed",
1133+
expectErr: "Permission denied",
1134+
getCSR: func() *x509.CertificateRequest {
1135+
return &x509.CertificateRequest{
1136+
URIs: []*url.URL{meshURL},
1137+
}
1138+
},
1139+
},
1140+
{
1141+
name: "err_meshgw_different_dc",
1142+
expectErr: "SPIFFE ID in CSR from a different datacenter",
1143+
authAllow: true,
1144+
getCSR: func() *x509.CertificateRequest {
1145+
return &x509.CertificateRequest{
1146+
URIs: []*url.URL{meshURL},
1147+
}
1148+
},
1149+
},
1150+
{
1151+
name: "err_invalid_spiffe_type",
1152+
expectErr: "SPIFFE ID in CSR must be a service, mesh-gateway, or agent ID",
1153+
getCSR: func() *x509.CertificateRequest {
1154+
u := connect.SpiffeIDSigning{
1155+
ClusterID: "test-cluster-id",
1156+
Domain: "test-domain",
1157+
}.URI()
1158+
return &x509.CertificateRequest{
1159+
URIs: []*url.URL{u},
1160+
}
1161+
},
1162+
},
1163+
}
1164+
1165+
for _, tc := range tests {
1166+
t.Run(tc.name, func(t *testing.T) {
1167+
authz := acl.DenyAll()
1168+
if tc.authAllow {
1169+
authz = acl.AllowAll()
1170+
}
1171+
1172+
cert, err := manager.AuthorizeAndSignCertificate(tc.getCSR(), authz)
1173+
if tc.expectErr != "" {
1174+
require.Error(t, err)
1175+
require.Contains(t, err.Error(), tc.expectErr)
1176+
} else {
1177+
require.NoError(t, err)
1178+
require.NotNil(t, cert)
1179+
}
1180+
})
1181+
}
1182+
}

0 commit comments

Comments
 (0)