connect/ca: cease including the common name field in generated certs (#10424)

As part of this change, we ensure that the SAN extensions are marked as
critical when the subject is empty so that AWS PCA tolerates the loss of
common names well and continues to function as a Connect CA provider.

Parts of this currently hack around a bug in crypto/x509 and can be
removed after https://go-review.googlesource.com/c/go/+/329129 lands in
a Go release.

Note: the AWS PCA tests do not run automatically, but the following
passed locally for me:

    ENABLE_AWS_PCA_TESTS=1 go test ./agent/connect/ca -run TestAWS
This commit is contained in:
R.B. Boyer 2021-06-25 13:00:00 -05:00 committed by GitHub
parent f24ee5d842
commit a2876453a5
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
20 changed files with 303 additions and 331 deletions

3
.changelog/10424.txt Normal file
View File

@ -0,0 +1,3 @@
```release-note:improvement
connect/ca: cease including the common name field in generated x509 non-CA certificates
```

View File

@ -5118,10 +5118,9 @@ func TestAgent_AutoEncrypt(t *testing.T) {
Datacenter: "dc1",
Agent: "test-client",
}
expectedCN := connect.AgentCN("test-client", connect.TestClusterID)
x509Cert, err := x509.ParseCertificate(aeCert.Certificate[0])
require.NoError(t, err)
require.Equal(t, expectedCN, x509Cert.Subject.CommonName)
require.Empty(t, x509Cert.Subject.CommonName)
require.Len(t, x509Cert.URIs, 1)
require.Equal(t, id.URI(), x509Cert.URIs[0])
}

View File

@ -4,7 +4,6 @@ import (
"context"
"crypto/x509"
"crypto/x509/pkix"
"encoding/asn1"
"fmt"
"net"
"net/url"
@ -45,16 +44,7 @@ func TestAutoEncrypt_generateCSR(t *testing.T) {
AutoEncryptTLS: true,
AutoEncryptIPSAN: []net.IP{net.IPv4(198, 18, 0, 1), net.IPv4(198, 18, 0, 2)},
},
expectedSubject: pkix.Name{
CommonName: connect.AgentCN("test-node", unknownTrustDomain),
Names: []pkix.AttributeTypeAndValue{
{
// 2,5,4,3 is the CommonName type ASN1 identifier
Type: asn1.ObjectIdentifier{2, 5, 4, 3},
Value: "testnode.agnt.unknown.consul",
},
},
},
expectedSubject: pkix.Name{},
expectedSigAlg: x509.ECDSAWithSHA256,
expectedPubAlg: x509.ECDSA,
expectedDNSNames: defaultDNSSANs,
@ -77,16 +67,7 @@ func TestAutoEncrypt_generateCSR(t *testing.T) {
AutoEncryptTLS: true,
AutoEncryptDNSSAN: []string{"foo.local", "bar.local"},
},
expectedSubject: pkix.Name{
CommonName: connect.AgentCN("test-node", unknownTrustDomain),
Names: []pkix.AttributeTypeAndValue{
{
// 2,5,4,3 is the CommonName type ASN1 identifier
Type: asn1.ObjectIdentifier{2, 5, 4, 3},
Value: "testnode.agnt.unknown.consul",
},
},
},
expectedSubject: pkix.Name{},
expectedSigAlg: x509.ECDSAWithSHA256,
expectedPubAlg: x509.ECDSA,
expectedDNSNames: append(defaultDNSSANs, "foo.local", "bar.local"),

View File

@ -245,11 +245,7 @@ func (ac *AutoConfig) generateCSR() (csr string, key string, err error) {
ipAddresses := ac.getIPSANs()
// Create a CSR.
//
// The Common Name includes the dummy trust domain for now but Server will
// override this when it is signed anyway so it's OK.
cn := connect.AgentCN(ac.config.NodeName, unknownTrustDomain)
csr, err = connect.CreateCSR(id, cn, pk, dnsNames, ipAddresses)
csr, err = connect.CreateCSR(id, pk, dnsNames, ipAddresses)
if err != nil {
return "", "", err
}

View File

@ -518,7 +518,6 @@ func (c *ConnectCALeaf) generateNewLeaf(req *ConnectCALeafRequest,
// Build the cert uri
var id connect.CertURI
var commonName string
var dnsNames []string
var ipAddresses []net.IP
if req.Service != "" {
@ -528,7 +527,6 @@ func (c *ConnectCALeaf) generateNewLeaf(req *ConnectCALeafRequest,
Namespace: req.TargetNamespace(),
Service: req.Service,
}
commonName = connect.ServiceCN(req.Service, req.TargetNamespace(), roots.TrustDomain)
dnsNames = append(dnsNames, req.DNSSAN...)
} else if req.Agent != "" {
id = &connect.SpiffeIDAgent{
@ -536,7 +534,6 @@ func (c *ConnectCALeaf) generateNewLeaf(req *ConnectCALeafRequest,
Datacenter: req.Datacenter,
Agent: req.Agent,
}
commonName = connect.AgentCN(req.Agent, roots.TrustDomain)
dnsNames = append([]string{"localhost"}, req.DNSSAN...)
ipAddresses = append([]net.IP{net.ParseIP("127.0.0.1"), net.ParseIP("::1")}, req.IPSAN...)
} else {
@ -561,7 +558,7 @@ func (c *ConnectCALeaf) generateNewLeaf(req *ConnectCALeafRequest,
}
// Create a CSR.
csr, err := connect.CreateCSR(id, commonName, pk, dnsNames, ipAddresses)
csr, err := connect.CreateCSR(id, pk, dnsNames, ipAddresses)
if err != nil {
return result, err
}

View File

@ -14,10 +14,11 @@ import (
"github.com/aws/aws-sdk-go/service/acmpca"
"github.com/mitchellh/mapstructure"
"github.com/hashicorp/go-hclog"
"github.com/hashicorp/consul/agent/connect"
"github.com/hashicorp/consul/agent/structs"
"github.com/hashicorp/consul/logging"
"github.com/hashicorp/go-hclog"
)
const (
@ -594,6 +595,8 @@ func (a *AWSProvider) GenerateIntermediate() (string, error) {
// Sign implements Provider
func (a *AWSProvider) Sign(csr *x509.CertificateRequest) (string, error) {
connect.HackSANExtensionForCSR(csr)
if a.rootPEM == "" {
return "", fmt.Errorf("AWS CA provider not fully Initialized")
}

View File

@ -14,11 +14,12 @@ import (
"sync"
"time"
"github.com/hashicorp/go-hclog"
"github.com/hashicorp/consul/agent/connect"
"github.com/hashicorp/consul/agent/consul/state"
"github.com/hashicorp/consul/agent/structs"
"github.com/hashicorp/consul/logging"
"github.com/hashicorp/go-hclog"
)
var (
@ -227,13 +228,7 @@ func (c *ConsulProvider) GenerateIntermediateCSR() (string, error) {
return "", err
}
uid, err := connect.CompactUID()
if err != nil {
return "", err
}
cn := connect.CACN("consul", uid, c.clusterID, c.isPrimary)
csr, err := connect.CreateCACSR(c.spiffeID, cn, signer)
csr, err := connect.CreateCACSR(c.spiffeID, signer)
if err != nil {
return "", err
}
@ -329,6 +324,8 @@ func (c *ConsulProvider) Cleanup(_ bool, _ map[string]interface{}) error {
// Sign returns a new certificate valid for the given SpiffeIDService
// using the current CA.
func (c *ConsulProvider) Sign(csr *x509.CertificateRequest) (string, error) {
connect.HackSANExtensionForCSR(csr)
// Lock during the signing so we don't use the same index twice
// for different cert serial numbers.
c.Lock()
@ -362,20 +359,6 @@ func (c *ConsulProvider) Sign(csr *x509.CertificateRequest) (string, error) {
return "", err
}
// Parse the SPIFFE ID
spiffeId, err := connect.ParseCertURI(csr.URIs[0])
if err != nil {
return "", err
}
// Even though leafs should be from our own CSRs which should have the same CN
// logic as here, override anyway to account for older version clients that
// didn't include the Common Name in the CSR.
subject, err := connect.CNForCertURI(spiffeId)
if err != nil {
return "", err
}
// Parse the CA cert
certPEM, err := c.ActiveIntermediate()
if err != nil {
@ -400,7 +383,6 @@ func (c *ConsulProvider) Sign(csr *x509.CertificateRequest) (string, error) {
effectiveNow := time.Now().Add(-1 * time.Minute)
template := x509.Certificate{
SerialNumber: sn,
Subject: pkix.Name{CommonName: subject},
URIs: csr.URIs,
Signature: csr.Signature,
// We use the correct signature algorithm for the CA key we are signing with
@ -487,8 +469,12 @@ func (c *ConsulProvider) SignIntermediate(csr *x509.CertificateRequest) (string,
effectiveNow := time.Now().Add(-1 * CertificateTimeDriftBuffer)
template := x509.Certificate{
SerialNumber: sn,
Subject: csr.Subject,
DNSNames: csr.DNSNames,
EmailAddresses: csr.EmailAddresses,
IPAddresses: csr.IPAddresses,
URIs: csr.URIs,
ExtraExtensions: csr.ExtraExtensions,
Subject: csr.Subject,
Signature: csr.Signature,
SignatureAlgorithm: connect.SigAlgoForKey(signer),
PublicKeyAlgorithm: csr.PublicKeyAlgorithm,

View File

@ -153,7 +153,7 @@ func TestConsulCAProvider_SignLeaf(t *testing.T) {
parsed, err := connect.ParseCert(cert)
require.NoError(err)
require.Equal(spiffeService.URI(), parsed.URIs[0])
require.Equal(connect.ServiceCN("foo", "default", connect.TestClusterID), parsed.Subject.CommonName)
require.Empty(parsed.Subject.CommonName)
require.Equal(uint64(2), parsed.SerialNumber.Uint64())
subjectKeyID, err := connect.KeyId(csr.PublicKey)
require.NoError(err)
@ -182,7 +182,7 @@ func TestConsulCAProvider_SignLeaf(t *testing.T) {
parsed, err := connect.ParseCert(cert)
require.NoError(err)
require.Equal(spiffeService.URI(), parsed.URIs[0])
require.Equal(connect.ServiceCN("bar", "default", connect.TestClusterID), parsed.Subject.CommonName)
require.Empty(parsed.Subject.CommonName)
require.Equal(parsed.SerialNumber.Uint64(), uint64(2))
requireNotEncoded(t, parsed.SubjectKeyId)
requireNotEncoded(t, parsed.AuthorityKeyId)
@ -210,7 +210,7 @@ func TestConsulCAProvider_SignLeaf(t *testing.T) {
parsed, err := connect.ParseCert(cert)
require.NoError(err)
require.Equal(spiffeAgent.URI(), parsed.URIs[0])
require.Equal(connect.AgentCN("uuid", connect.TestClusterID), parsed.Subject.CommonName)
require.Empty(parsed.Subject.CommonName)
require.Equal(uint64(2), parsed.SerialNumber.Uint64())
requireNotEncoded(t, parsed.SubjectKeyId)
requireNotEncoded(t, parsed.AuthorityKeyId)

View File

@ -412,6 +412,8 @@ func (v *VaultProvider) GenerateIntermediate() (string, error) {
// a new leaf certificate based on the provided CSR, with the issuing
// intermediate CA cert attached.
func (v *VaultProvider) Sign(csr *x509.CertificateRequest) (string, error) {
connect.HackSANExtensionForCSR(csr)
var pemBuf bytes.Buffer
if err := pem.Encode(&pemBuf, &pem.Block{Type: "CERTIFICATE REQUEST", Bytes: csr.Raw}); err != nil {
return "", err

View File

@ -11,102 +11,6 @@ import (
var invalidDNSNameChars = regexp.MustCompile(`[^a-z0-9]`)
const (
// 64 = max length of a certificate common name
// 21 = 7 bytes for ".consul", 9 bytes for .<trust domain> and 5 bytes for ".svc."
// This ends up being 43 bytes
maxServiceAndNamespaceLen = 64 - 21
minServiceNameLen = 15
minNamespaceNameLen = 15
)
// trucateServiceAndNamespace will take a service name and namespace name and truncate
// them appropriately so that they would fit within the space alloted for them in the
// Common Name field of the x509 certificate. That field is capped at 64 characters
// in length and there is other data that must be a part of the name too. This function
// takes all of that into account.
func truncateServiceAndNamespace(serviceName, namespace string) (string, string) {
svcLen := len(serviceName)
nsLen := len(namespace)
totalLen := svcLen + nsLen
// quick exit when the entirety of both can fit
if totalLen <= maxServiceAndNamespaceLen {
return serviceName, namespace
}
toRemove := totalLen - maxServiceAndNamespaceLen
// now we must figure out how to truncate each one, we need to ensure we don't remove all of either one.
if svcLen <= minServiceNameLen {
// only remove bytes from the namespace
return serviceName, truncateTo(namespace, nsLen-toRemove)
} else if nsLen <= minNamespaceNameLen {
// only remove bytes from the service name
return truncateTo(serviceName, svcLen-toRemove), namespace
} else {
// we can remove an "equal" amount from each. If the number of bytes to remove is odd we give it to the namespace
svcTruncate := svcLen - (toRemove / 2) - (toRemove % 2)
nsTruncate := nsLen - (toRemove / 2)
// checks to ensure we don't reduce one side too much when they are not roughly balanced in length.
if svcTruncate <= minServiceNameLen {
svcTruncate = minServiceNameLen
nsTruncate = maxServiceAndNamespaceLen - minServiceNameLen
} else if nsTruncate <= minNamespaceNameLen {
svcTruncate = maxServiceAndNamespaceLen - minNamespaceNameLen
nsTruncate = minNamespaceNameLen
}
return truncateTo(serviceName, svcTruncate), truncateTo(namespace, nsTruncate)
}
}
// ServiceCN returns the common name for a service's certificate. We can't use
// SPIFFE URIs because some CAs require valid FQDN format. We can't use SNI
// values because they are often too long than the 64 bytes allowed by
// CommonNames. We could attempt to encode more information into this to make
// identifying which instance/node it was issued to in a management tool easier
// but that just introduces more complications around length. It's also strange
// that the Common Name would encode more information than the actual
// identifying URI we use to assert anything does and my lead to bad assumptions
// that the common name is in some way "secure" or verified - there is nothing
// inherently provable here except that the requestor had ACLs for that service
// name in that DC.
//
// Format is:
// <sanitized_service_name>.svc.<trust_domain_first_8>.consul
//
// service name is sanitized by removing any chars that are not legal in a DNS
// name and lower casing. It is truncated to the first X chars to keep the
// total at 64.
//
// trust domain is truncated to keep the whole name short
func ServiceCN(serviceName, namespace, trustDomain string) string {
svc := invalidDNSNameChars.ReplaceAllString(strings.ToLower(serviceName), "")
svc, namespace = truncateServiceAndNamespace(svc, namespace)
return fmt.Sprintf("%s.svc.%s.%s.consul",
svc, namespace, truncateTo(trustDomain, 8))
}
// AgentCN returns the common name for an agent certificate. See ServiceCN for
// more details on rationale.
//
// Format is:
// <sanitized_node_name>.agnt.<trust_domain_first_8>.consul
//
// node name is sanitized by removing any chars that are not legal in a DNS
// name and lower casing. It is truncated to the first X chars to keep the
// total at 64.
//
// trust domain is truncated to keep the whole name short
func AgentCN(node, trustDomain string) string {
nodeSan := invalidDNSNameChars.ReplaceAllString(strings.ToLower(node), "")
// 21 = 7 bytes for ".consul", 8 bytes for trust domain, 6 bytes for ".agnt."
return fmt.Sprintf("%s.agnt.%s.consul",
truncateTo(nodeSan, 64-21), truncateTo(trustDomain, 8))
}
// CompactUID returns a crypto random Unique Identifier string consiting of 8
// characters of base36 encoded random value. This has roughly 41 bits of
// entropy so is suitable for infrequently occuring events with low probability
@ -129,8 +33,8 @@ func CompactUID() (string, error) {
return truncateTo(strconv.FormatInt(int64(i), 36), 8), nil
}
// CACN returns the common name for a CA certificate. See ServiceCN for more
// details on rationale. A uniqueID is requires because some providers (e.g.
// CACN returns the common name for a CA certificate.
// A uniqueID is requires because some providers (e.g.
// Vault) cache by subject and so produce incorrect results - for example they
// won't cross-sign an older CA certificate with the same common name since they
// think they already have a valid cert for that CN and just return the current
@ -163,22 +67,3 @@ func truncateTo(s string, n int) string {
}
return s
}
// CNForCertURI returns the correct common name for a given cert URI type. It
// doesn't work for CA Signing IDs since more context is needed and CA Providers
// always know their CN from their own context.
func CNForCertURI(uri CertURI) (string, error) {
// Even though leafs should be from our own CSRs which should have the same CN
// logic as here, override anyway to account for older version clients that
// didn't include the Common Name in the CSR.
switch id := uri.(type) {
case *SpiffeIDService:
return ServiceCN(id.Service, id.Namespace, id.Host), nil
case *SpiffeIDAgent:
return AgentCN(id.Agent, id.Host), nil
case *SpiffeIDSigning:
return "", fmt.Errorf("CertURI is a SpiffeIDSigning, not enough context to generate Common Name")
default:
return "", fmt.Errorf("CertURI type not recognized")
}
}

View File

@ -1,115 +0,0 @@
package connect
import (
"strings"
"testing"
"github.com/stretchr/testify/require"
)
func TestServiceAndNamespaceTruncation(t *testing.T) {
type tcase struct {
service string
namespace string
// if left as "" then its expected to not be truncated
expectedService string
// if left as "" then its expected to not be truncated
expectedNamespace string
}
cases := map[string]tcase{
"short-no-truncation": {
service: "foo",
namespace: "bar",
},
"long-service-no-truncation": {
// -3 because thats the length of the namespace
service: strings.Repeat("a", maxServiceAndNamespaceLen-3),
namespace: "bar",
},
"long-namespace-no-truncation": {
service: "foo",
// -3 because thats the length of the service name
namespace: strings.Repeat("b", maxServiceAndNamespaceLen-3),
},
"truncate-service-only": {
// this should force the service name to be truncated
service: strings.Repeat("a", maxServiceAndNamespaceLen-minNamespaceNameLen+5),
expectedService: strings.Repeat("a", maxServiceAndNamespaceLen-minNamespaceNameLen),
// this is the maximum length that will never be truncated for a namespace
namespace: strings.Repeat("b", minNamespaceNameLen),
},
"truncate-namespace-only": {
// this is the maximum length that will never be truncated for a service name
service: strings.Repeat("a", minServiceNameLen),
// this should force the namespace name to be truncated
namespace: strings.Repeat("b", maxServiceAndNamespaceLen-minServiceNameLen+5),
expectedNamespace: strings.Repeat("b", maxServiceAndNamespaceLen-minServiceNameLen),
},
"truncate-both-even": {
// this test would need to be update if the maxServiceAndNamespaceLen variable is updated
// I could put some more complex logic into here to prevent that but it would be mostly
// duplicating the logic in the function itself and thus not really be testing anything
//
// The original lengths of 50 / 51 were chose when maxServiceAndNamespaceLen would be 43
// Therefore a total of 58 characters must be removed. This was chose so that the value
// could be evenly split between the two strings.
service: strings.Repeat("a", 50),
expectedService: strings.Repeat("a", 21),
namespace: strings.Repeat("b", 51),
expectedNamespace: strings.Repeat("b", 22),
},
"truncate-both-odd": {
// this test would need to be update if the maxServiceAndNamespaceLen variable is updated
// I could put some more complex logic into here to prevent that but it would be mostly
// duplicating the logic in the function itself and thus not really be testing anything
//
// The original lengths of 50 / 57 were chose when maxServiceAndNamespaceLen would be 43
// Therefore a total of 57 characters must be removed. This was chose so that the value
// could not be evenly removed from both so the namespace should be truncated to a length
// 1 character more than the service.
service: strings.Repeat("a", 50),
expectedService: strings.Repeat("a", 21),
namespace: strings.Repeat("b", 50),
expectedNamespace: strings.Repeat("b", 22),
},
"truncate-both-min-svc": {
service: strings.Repeat("a", minServiceNameLen+1),
expectedService: strings.Repeat("a", minServiceNameLen),
namespace: strings.Repeat("b", maxServiceAndNamespaceLen),
expectedNamespace: strings.Repeat("b", maxServiceAndNamespaceLen-minServiceNameLen),
},
"truncate-both-min-ns": {
service: strings.Repeat("a", maxServiceAndNamespaceLen),
expectedService: strings.Repeat("a", maxServiceAndNamespaceLen-minNamespaceNameLen),
namespace: strings.Repeat("b", minNamespaceNameLen+1),
expectedNamespace: strings.Repeat("b", minNamespaceNameLen),
},
}
for name, tc := range cases {
t.Run(name, func(t *testing.T) {
actualSvc, actualNamespace := truncateServiceAndNamespace(tc.service, tc.namespace)
expectedLen := len(tc.service) + len(tc.namespace)
if tc.expectedService != "" || tc.expectedNamespace != "" {
expectedLen = maxServiceAndNamespaceLen
}
actualLen := len(actualSvc) + len(actualNamespace)
require.Equal(t, expectedLen, actualLen, "Combined length of %d (svc: %d, ns: %d) does not match expected value of %d", actualLen, len(actualSvc), len(actualNamespace), expectedLen)
if tc.expectedService != "" {
require.Equal(t, tc.expectedService, actualSvc)
} else {
require.Equal(t, tc.service, actualSvc)
}
if tc.expectedNamespace != "" {
require.Equal(t, tc.expectedNamespace, actualNamespace)
} else {
require.Equal(t, tc.namespace, actualNamespace)
}
})
}
}

View File

@ -44,16 +44,16 @@ func SigAlgoForKeyType(keyType string) x509.SignatureAlgorithm {
// CreateCSR returns a CSR to sign the given service with SAN entries
// along with the PEM-encoded private key for this certificate.
func CreateCSR(uri CertURI, commonName string, privateKey crypto.Signer,
func CreateCSR(uri CertURI, privateKey crypto.Signer,
dnsNames []string, ipAddresses []net.IP, extensions ...pkix.Extension) (string, error) {
template := &x509.CertificateRequest{
URIs: []*url.URL{uri.URI()},
SignatureAlgorithm: SigAlgoForKey(privateKey),
ExtraExtensions: extensions,
Subject: pkix.Name{CommonName: commonName},
DNSNames: dnsNames,
IPAddresses: ipAddresses,
}
HackSANExtensionForCSR(template)
// Create the CSR itself
var csrBuf bytes.Buffer
@ -72,13 +72,13 @@ func CreateCSR(uri CertURI, commonName string, privateKey crypto.Signer,
// CreateCSR returns a CA CSR to sign the given service along with the PEM-encoded
// private key for this certificate.
func CreateCACSR(uri CertURI, commonName string, privateKey crypto.Signer) (string, error) {
func CreateCACSR(uri CertURI, privateKey crypto.Signer) (string, error) {
ext, err := CreateCAExtension()
if err != nil {
return "", err
}
return CreateCSR(uri, commonName, privateKey, nil, nil, ext)
return CreateCSR(uri, privateKey, nil, nil, ext)
}
// CreateCAExtension creates a pkix.Extension for the x509 Basic Constraints

View File

@ -13,9 +13,10 @@ import (
"sync/atomic"
"time"
"github.com/hashicorp/consul/agent/structs"
"github.com/hashicorp/go-uuid"
"github.com/mitchellh/go-testing-interface"
"github.com/hashicorp/consul/agent/structs"
)
// TestClusterID is the Consul cluster ID for testing.
@ -182,16 +183,7 @@ func TestCAWithKeyType(t testing.T, xc *structs.CARoot, keyType string, keyBits
return testCA(t, xc, keyType, keyBits, 0)
}
// testCertID is an interface to be implemented the various spiffe ID / CertURI types
// It adds an addition CommonName method to the CertURI interface to prevent the need
// for any type switching on the actual CertURI's concrete type in order to figure
// out its common name
type testCertID interface {
CommonName() string
CertURI
}
func testLeafWithID(t testing.T, spiffeId testCertID, root *structs.CARoot, keyType string, keyBits int, expiration time.Duration) (string, string, error) {
func testLeafWithID(t testing.T, spiffeId CertURI, root *structs.CARoot, keyType string, keyBits int, expiration time.Duration) (string, string, error) {
if expiration == 0 {
// this is 10 years
@ -231,7 +223,6 @@ func testLeafWithID(t testing.T, spiffeId testCertID, root *structs.CARoot, keyT
// Cert template for generation
template := x509.Certificate{
SerialNumber: sn,
Subject: pkix.Name{CommonName: spiffeId.CommonName()},
URIs: []*url.URL{spiffeId.URI()},
SignatureAlgorithm: SigAlgoForKeyType(rootKeyType),
BasicConstraintsValid: true,
@ -309,16 +300,13 @@ func TestLeafWithNamespace(t testing.T, service, namespace string, root *structs
// TestCSR returns a CSR to sign the given service along with the PEM-encoded
// private key for this certificate.
func TestCSR(t testing.T, uri CertURI) (string, string) {
cn, err := CNForCertURI(uri)
if err != nil {
t.Fatalf("TestCSR failed to get Common Name: %s", err)
}
template := &x509.CertificateRequest{
Subject: pkix.Name{CommonName: cn},
URIs: []*url.URL{uri.URI()},
SignatureAlgorithm: x509.ECDSAWithSHA256,
}
HackSANExtensionForCSR(template)
// Create the private key we'll use
signer, pkPEM := testPrivateKey(t, DefaultPrivateKeyType, DefaultPrivateKeyBits)

View File

@ -20,7 +20,3 @@ func (id *SpiffeIDAgent) URI() *url.URL {
result.Path = fmt.Sprintf("/agent/client/dc/%s/id/%s", id.Datacenter, id.Agent)
return &result
}
func (id *SpiffeIDAgent) CommonName() string {
return AgentCN(id.Agent, id.Host)
}

View File

@ -22,7 +22,3 @@ func (id *SpiffeIDService) URI() *url.URL {
id.Namespace, id.Datacenter, id.Service)
return &result
}
func (id *SpiffeIDService) CommonName() string {
return ServiceCN(id.Service, id.Namespace, id.Host)
}

126
agent/connect/x509_patch.go Normal file
View File

@ -0,0 +1,126 @@
package connect
import (
"crypto/x509"
"crypto/x509/pkix"
"encoding/asn1"
"fmt"
"net"
"net/url"
"unicode"
)
// NOTE: the contents of this file were lifted from
// $GOROOT/src/crypto/x509/x509.go from a Go 1.16.5 checkout.
//
//
// After https://go-review.googlesource.com/c/go/+/329129 lands in a Go release
// we are compiling against we can safely remove all of this code.
var (
x509_oidExtensionSubjectAltName = []int{2, 5, 29, 17}
)
const (
x509_nameTypeEmail = 1
x509_nameTypeDNS = 2
x509_nameTypeURI = 6
x509_nameTypeIP = 7
)
// HackSANExtensionForCSR will create a SAN extension on the CSR off of the
// convenience fields (DNSNames, EmailAddresses, IPAddresses, URIs) and
// appropriately marks that SAN extension as critical if the CSR has an empty
// subject.
//
// This is basically attempting to repeat this blob of code from the stdlib
// ourselves:
//
// https://github.com/golang/go/blob/0e67ce3d28320e816dd8e7cf7d701c1804fb977e/src/crypto/x509/x509.go#L1088
func HackSANExtensionForCSR(template *x509.CertificateRequest) {
switch {
case len(template.DNSNames) > 0:
case len(template.EmailAddresses) > 0:
case len(template.IPAddresses) > 0:
case len(template.URIs) > 0:
default:
return
}
if x509_oidInExtensions(x509_oidExtensionSubjectAltName, template.ExtraExtensions) {
return
}
value, err := x509_marshalSANs(template.DNSNames, template.EmailAddresses, template.IPAddresses, template.URIs)
if err != nil {
return
}
ext := pkix.Extension{
Id: x509_oidExtensionSubjectAltName,
// From RFC 5280, Section 4.2.1.6:
// “If the subject field contains an empty sequence ... then
// subjectAltName extension ... is marked as critical”
//
// Since we just cleared the subject above, it's critical.
Critical: true,
Value: value,
}
template.ExtraExtensions = append(template.ExtraExtensions, ext)
}
// x509_oidInExtensions reports whether an extension with the given oid exists in
// extensions.
func x509_oidInExtensions(oid asn1.ObjectIdentifier, extensions []pkix.Extension) bool {
for _, e := range extensions {
if e.Id.Equal(oid) {
return true
}
}
return false
}
// x509_marshalSANs marshals a list of addresses into a the contents of an X.509
// SubjectAlternativeName extension.
func x509_marshalSANs(dnsNames, emailAddresses []string, ipAddresses []net.IP, uris []*url.URL) (derBytes []byte, err error) {
var rawValues []asn1.RawValue
for _, name := range dnsNames {
if err := x509_isIA5String(name); err != nil {
return nil, err
}
rawValues = append(rawValues, asn1.RawValue{Tag: x509_nameTypeDNS, Class: 2, Bytes: []byte(name)})
}
for _, email := range emailAddresses {
if err := x509_isIA5String(email); err != nil {
return nil, err
}
rawValues = append(rawValues, asn1.RawValue{Tag: x509_nameTypeEmail, Class: 2, Bytes: []byte(email)})
}
for _, rawIP := range ipAddresses {
// If possible, we always want to encode IPv4 addresses in 4 bytes.
ip := rawIP.To4()
if ip == nil {
ip = rawIP
}
rawValues = append(rawValues, asn1.RawValue{Tag: x509_nameTypeIP, Class: 2, Bytes: ip})
}
for _, uri := range uris {
uriStr := uri.String()
if err := x509_isIA5String(uriStr); err != nil {
return nil, err
}
rawValues = append(rawValues, asn1.RawValue{Tag: x509_nameTypeURI, Class: 2, Bytes: []byte(uriStr)})
}
return asn1.Marshal(rawValues)
}
func x509_isIA5String(s string) error {
for _, r := range s {
// Per RFC5280 "IA5String is limited to the set of ASCII characters"
if r > unicode.MaxASCII {
return fmt.Errorf("x509: %q cannot be encoded as an IA5String", s)
}
}
return nil
}

View File

@ -0,0 +1,126 @@
package connect
import (
"crypto/rand"
"crypto/rsa"
"crypto/x509"
"encoding/pem"
"math/big"
"testing"
)
var testPrivateKey_x509 *rsa.PrivateKey
func TestX509_EmptySubject(t *testing.T) {
// NOTE: this test is lifted straight out of the stdlib with no changes. to
// show that the cert-only workflow is fine.
template := x509.Certificate{
SerialNumber: big.NewInt(1),
DNSNames: []string{"example.com"},
}
derBytes, err := x509.CreateCertificate(rand.Reader, &template, &template, &testPrivateKey_x509.PublicKey, testPrivateKey_x509)
if err != nil {
t.Fatalf("failed to create certificate: %s", err)
}
cert, err := x509.ParseCertificate(derBytes)
if err != nil {
t.Fatalf("failed to parse certificate: %s", err)
}
for _, ext := range cert.Extensions {
if ext.Id.Equal(x509_oidExtensionSubjectAltName) {
if !ext.Critical {
t.Fatal("SAN extension is not critical")
}
return
}
}
t.Fatal("SAN extension is missing")
}
func TestX509_EmptySubjectInCSR(t *testing.T) {
// NOTE: the CSR-only workflow is flawed so we hack around it
for _, tc := range []struct {
name string
hack bool
expectCritical bool
}{
{name: "unmodified stdlib",
hack: false,
expectCritical: false,
},
{name: "hacked stdlib",
hack: true,
expectCritical: true,
},
} {
t.Run(tc.name, func(t *testing.T) {
template := x509.CertificateRequest{
DNSNames: []string{"example.com"},
}
if tc.hack {
HackSANExtensionForCSR(&template)
}
derBytes, err := x509.CreateCertificateRequest(rand.Reader, &template, testPrivateKey_x509)
if err != nil {
t.Fatalf("failed to create certificate request: %s", err)
}
csr, err := x509.ParseCertificateRequest(derBytes)
if err != nil {
t.Fatalf("failed to parse certificate request: %s", err)
}
for _, ext := range csr.Extensions {
if ext.Id.Equal(x509_oidExtensionSubjectAltName) {
if tc.expectCritical {
if !ext.Critical {
t.Fatal("SAN extension is not critical")
}
} else {
if ext.Critical {
t.Fatal("SAN extension is critical now; maybe we don't need the hack anymore with this version of Go?")
}
}
return
}
}
t.Fatal("SAN extension is missing")
})
}
}
func init() {
block, _ := pem.Decode([]byte(pemPrivateKey_x509))
var err error
testPrivateKey_x509, err = x509.ParsePKCS1PrivateKey(block.Bytes)
if err != nil {
panic(err)
}
}
var pemPrivateKey_x509 = `
-----BEGIN RSA PRIVATE KEY-----
MIICXAIBAAKBgQCxoeCUW5KJxNPxMp+KmCxKLc1Zv9Ny+4CFqcUXVUYH69L3mQ7v
IWrJ9GBfcaA7BPQqUlWxWM+OCEQZH1EZNIuqRMNQVuIGCbz5UQ8w6tS0gcgdeGX7
J7jgCQ4RK3F/PuCM38QBLaHx988qG8NMc6VKErBjctCXFHQt14lerd5KpQIDAQAB
AoGAYrf6Hbk+mT5AI33k2Jt1kcweodBP7UkExkPxeuQzRVe0KVJw0EkcFhywKpr1
V5eLMrILWcJnpyHE5slWwtFHBG6a5fLaNtsBBtcAIfqTQ0Vfj5c6SzVaJv0Z5rOd
7gQF6isy3t3w9IF3We9wXQKzT6q5ypPGdm6fciKQ8RnzREkCQQDZwppKATqQ41/R
vhSj90fFifrGE6aVKC1hgSpxGQa4oIdsYYHwMzyhBmWW9Xv/R+fPyr8ZwPxp2c12
33QwOLPLAkEA0NNUb+z4ebVVHyvSwF5jhfJxigim+s49KuzJ1+A2RaSApGyBZiwS
rWvWkB471POAKUYt5ykIWVZ83zcceQiNTwJBAMJUFQZX5GDqWFc/zwGoKkeR49Yi
MTXIvf7Wmv6E++eFcnT461FlGAUHRV+bQQXGsItR/opIG7mGogIkVXa3E1MCQARX
AAA7eoZ9AEHflUeuLn9QJI/r0hyQQLEtrpwv6rDT1GCWaLII5HJ6NUFVf4TTcqxo
6vdM4QGKTJoO+SaCyP0CQFdpcxSAuzpFcKv0IlJ8XzS/cy+mweCMwyJ1PFEc4FX6
wg/HcAJWY60xZTJDFN+Qfx8ZQvBEin6c2/h+zZi5IVY=
-----END RSA PRIVATE KEY-----
`

View File

@ -8,13 +8,14 @@ import (
"strings"
"testing"
msgpackrpc "github.com/hashicorp/net-rpc-msgpackrpc"
"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/require"
"github.com/hashicorp/consul/agent/connect"
"github.com/hashicorp/consul/agent/structs"
"github.com/hashicorp/consul/testrpc"
"github.com/hashicorp/consul/tlsutil"
msgpackrpc "github.com/hashicorp/net-rpc-msgpackrpc"
"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/require"
)
func TestAutoEncryptSign(t *testing.T) {
@ -80,11 +81,9 @@ func TestAutoEncryptSign(t *testing.T) {
require.NoError(t, err, info)
// Create a CSR.
cn, err := connect.CNForCertURI(id)
require.NoError(t, err)
dnsNames := []string{"localhost"}
ipAddresses := []net.IP{net.ParseIP("127.0.0.1")}
csr, err := connect.CreateCSR(id, cn, pk, dnsNames, ipAddresses)
csr, err := connect.CreateCSR(id, pk, dnsNames, ipAddresses)
require.NoError(t, err, info)
require.NotEmpty(t, csr, info)
args := &structs.CASignRequest{

View File

@ -8,13 +8,14 @@ import (
"strings"
"sync"
memdb "github.com/hashicorp/go-memdb"
"golang.org/x/time/rate"
"github.com/hashicorp/consul/agent/connect"
"github.com/hashicorp/consul/agent/connect/ca"
"github.com/hashicorp/consul/agent/consul/state"
"github.com/hashicorp/consul/agent/structs"
"github.com/hashicorp/consul/lib/semaphore"
memdb "github.com/hashicorp/go-memdb"
"golang.org/x/time/rate"
)
type connectSignRateLimiter struct {
@ -169,7 +170,6 @@ func (s *Server) SignCertificate(csr *x509.CertificateRequest, spiffeID connect.
originalURI := agentID.URI()
agentID.Host = trustDomain
csr.Subject.CommonName = connect.AgentCN(agentID.Agent, trustDomain)
// recreate the URIs list
uris := make([]*url.URL, len(csr.URIs))
@ -208,7 +208,10 @@ func (s *Server) SignCertificate(csr *x509.CertificateRequest, spiffeID connect.
defer s.caLeafLimiter.csrConcurrencyLimiter.Release()
}
connect.HackSANExtensionForCSR(csr)
// All seems to be in order, actually sign it.
pem, err := provider.Sign(csr)
if err == ca.ErrRateLimited {
return nil, ErrRateLimited

View File

@ -15,8 +15,9 @@ import (
"net"
"time"
"github.com/hashicorp/consul/agent/connect"
"net/url"
"github.com/hashicorp/consul/agent/connect"
)
// GenerateSerialNumber returns random bigint generated with crypto/rand