[v18] winpki: prevent CRL common names from overflowing

lib/auth/db_test.go

@@ -158,9 +158,11 @@ func TestDBCertSigning(t *testing.T) {
 		name           string
 		requester      proto.DatabaseCertRequest_Requester
 		extensions     proto.DatabaseCertRequest_Extensions
+		crlDomain      string
 		wantCertSigner []byte
 		wantCACerts    [][]byte
 		wantKeyUsage   []x509.ExtKeyUsage
+		wantCDP        []string
 	}{
 		{
 			name:           "DB service request is signed by active db client CA and trusts db CAs",
@@ -190,6 +192,16 @@ func TestDBCertSigning(t *testing.T) {
 			wantCACerts:    [][]byte{activeDBClientCACert, newDBClientCACert},
 			wantKeyUsage:   []x509.ExtKeyUsage{x509.ExtKeyUsageServerAuth},
 		},
+		{
+			name:           "tctl request for SQL Server database with CDPs",
+			requester:      proto.DatabaseCertRequest_TCTL,
+			extensions:     proto.DatabaseCertRequest_WINDOWS_SMARTCARD,
+			crlDomain:      "example.com",
+			wantCertSigner: newDBCACert,
+			wantCACerts:    [][]byte{activeDBClientCACert, newDBClientCACert},
+			wantKeyUsage:   []x509.ExtKeyUsage{x509.ExtKeyUsageServerAuth},
+			wantCDP:        []string{"ldap:///CN=local.me,CN=TeleportDB,CN=CDP,CN=Public Key Services,CN=Services,CN=Configuration,DC=example,DC=com?certificateRevocationList?base?objectClass=cRLDistributionPoint"},
+		},
 	}
 	for _, tt := range tests {
 		tt := tt
@@ -201,25 +213,30 @@ func TestDBCertSigning(t *testing.T) {
 				TTL:                   proto.Duration(time.Hour),
 				RequesterName:         tt.requester,
 				CertificateExtensions: tt.extensions,
+				CRLDomain:             tt.crlDomain,
 			})
 			require.NoError(t, err)
 			require.Equal(t, tt.wantCACerts, certResp.CACerts)
 
 			// verify that the response cert is a DB CA cert.
-			mustVerifyCert(t, tt.wantCertSigner, certResp.Cert, tt.wantKeyUsage...)
+			mustVerifyCert(t, tt.wantCertSigner, certResp.Cert, tt.wantCDP, tt.wantKeyUsage...)
 		})
 	}
 }
 
 // mustVerifyCert is a helper func that verifies leaf cert with root cert.
-func mustVerifyCert(t *testing.T, rootPEM, leafPEM []byte, keyUsages ...x509.ExtKeyUsage) {
+func mustVerifyCert(t *testing.T, rootPEM, leafPEM []byte, cdps []string, keyUsages ...x509.ExtKeyUsage) {
 	t.Helper()
+
 	leafCert, err := tlsca.ParseCertificatePEM(leafPEM)
 	require.NoError(t, err)
 
 	certPool := x509.NewCertPool()
 	ok := certPool.AppendCertsFromPEM(rootPEM)
 	require.True(t, ok)
+
+	require.Equal(t, cdps, leafCert.CRLDistributionPoints)
+
 	opts := x509.VerifyOptions{
 		Roots:     certPool,
 		KeyUsages: keyUsages,

lib/winpki/certificate_authority.go

@@ -21,7 +21,6 @@ package winpki
 import (
 	"context"
 	"crypto/tls"
-	"encoding/base32"
 	"log/slog"
 
 	"github.com/gravitational/trace"
@@ -94,9 +93,8 @@ func (c *CertificateStoreClient) Update(ctx context.Context, tc *tls.Config) err
 			if err != nil {
 				return trace.Wrap(err)
 			}
-			subjectID := base32.HexEncoding.EncodeToString(cert.SubjectKeyId)
-			issuer := subjectID + "_" + c.cfg.ClusterName
-			if err := c.updateCRL(ctx, issuer, keyPair.CRL, caType, tc); err != nil {
+
+			if err := c.updateCRL(ctx, c.cfg.ClusterName, cert.SubjectKeyId, keyPair.CRL, caType, tc); err != nil {
 				return trace.Wrap(err)
 			}
 		}
@@ -110,14 +108,14 @@ func (c *CertificateStoreClient) Update(ctx context.Context, tc *tls.Config) err
 			return trace.Wrap(err, "generating CRL")
 		}
 
-		if err := c.updateCRL(ctx, c.cfg.ClusterName, crlDER, caType, tc); err != nil {
+		if err := c.updateCRL(ctx, c.cfg.ClusterName, nil, crlDER, caType, tc); err != nil {
 			return trace.Wrap(err, "updating CRL over LDAP")
 		}
 	}
 	return nil
 }
 
-func (c *CertificateStoreClient) updateCRL(ctx context.Context, issuer string, crlDER []byte, caType types.CertAuthType, tc *tls.Config) error {
+func (c *CertificateStoreClient) updateCRL(ctx context.Context, issuerCN string, issuerSKID []byte, crlDER []byte, caType types.CertAuthType, tc *tls.Config) error {
 	// Publish the CRL for current cluster CA. For trusted clusters, their
 	// respective windows_desktop_services will publish CRLs of their CAs so we
 	// don't have to do it here.
@@ -132,7 +130,7 @@ func (c *CertificateStoreClient) updateCRL(ctx context.Context, issuer string, c
 	// CA will be placed at:
 	// ... > CDP > Teleport > prod
 	containerDN := crlContainerDN(c.cfg.Domain, caType)
-	crlDN := CRLDN(issuer, c.cfg.Domain, caType)
+	crlDN := CRLDN(issuerCN, issuerSKID, c.cfg.Domain, caType)
 
 	ldapClient, err := DialLDAP(ctx, c.cfg.LC, tc)
 	if err != nil {

lib/winpki/ldap.go

@@ -296,19 +296,34 @@ func crlContainerDN(domain string, caType types.CertAuthType) string {
 	return fmt.Sprintf("CN=%s,CN=CDP,CN=Public Key Services,CN=Services,CN=Configuration,%s", crlKeyName(caType), DomainDN(domain))
 }
 
-// CRLDN computes the distinguished name for a Teleport issuer in Windows environments.
-func CRLDN(issuerID string, activeDirectoryDomain string, caType types.CertAuthType) string {
-	return "CN=" + issuerID + "," + crlContainerDN(activeDirectoryDomain, caType)
+// CRNCN computes the common name for a Teleport CRL in Windows environments.
+// The issuer SKID is optional, but should generally be set for compatibility
+// with clusters having more than one issuer (like those using HSMs).
+func CRLCN(issuerCN string, issuerSKID []byte) string {
+	name := issuerCN
+	if len(issuerSKID) > 0 {
+		id := base32.HexEncoding.EncodeToString(issuerSKID)
+		name = id + "_" + name
+	}
+	// The limit on the CN attribute should be 64 characters, but in practice
+	// we observe that certutil.exe truncates the CN as soon as it exceeds 51 characters.
+	return name[:min(len(name), 51)]
+}
+
+// CRLDN computes the distinguished name for a Teleport CRL in Windows environments.
+// The issuer SKID is optional, but should generally be set for compatibility
+// with clusters having more than one issuer (like those using HSMs).
+func CRLDN(issuerCN string, issuerSKID []byte, activeDirectoryDomain string, caType types.CertAuthType) string {
+	return "CN=" + CRLCN(issuerCN, issuerSKID) + "," + crlContainerDN(activeDirectoryDomain, caType)
 }
 
 // CRLDistributionPoint computes the CRL distribution point for certs issued.
 func CRLDistributionPoint(activeDirectoryDomain string, caType types.CertAuthType, issuer *tlsca.CertAuthority, includeSKID bool) string {
-	name := issuer.Cert.Subject.CommonName
+	var issuerSKID []byte
 	if includeSKID {
-		id := base32.HexEncoding.EncodeToString(issuer.Cert.SubjectKeyId)
-		name = id + "_" + name
+		issuerSKID = issuer.Cert.SubjectKeyId
 	}
-	crlDN := CRLDN(name, activeDirectoryDomain, caType)
+	crlDN := CRLDN(issuer.Cert.Subject.CommonName, issuerSKID, activeDirectoryDomain, caType)
 	return fmt.Sprintf("ldap:///%s?certificateRevocationList?base?objectClass=cRLDistributionPoint", crlDN)
 }
 

lib/winpki/windows_test.go

@@ -37,6 +37,7 @@ func TestCRLDN(t *testing.T) {
 	for _, test := range []struct {
 		name        string
 		clusterName string
+		issuerSKID  []byte
 		crlDN       string
 		caType      types.CertAuthType
 	}{
@@ -62,9 +63,23 @@ func TestCRLDN(t *testing.T) {
 			caType:      types.UserCA,
 			crlDN:       "CN=cluster.goteleport.com,CN=Teleport,CN=CDP,CN=Public Key Services,CN=Services,CN=Configuration,DC=test,DC=goteleport,DC=com",
 		},
+		{
+			name:        "user CA with SKID",
+			clusterName: "example.com",
+			caType:      types.UserCA,
+			issuerSKID:  []byte{0x61, 0xbe, 0xe7, 0xf0, 0xb4, 0x88, 0x78, 0x33, 0x40, 0x7d, 0x7a, 0xc0, 0xa8, 0x2a, 0xeb, 0x3e, 0x9d, 0x9f, 0xa1, 0xba},
+			crlDN:       "CN=C6VEFS5KH1S36G3TFB0AGANB7QEPV8DQ_example.com,CN=Teleport,CN=CDP,CN=Public Key Services,CN=Services,CN=Configuration,DC=test,DC=goteleport,DC=com",
+		},
+		{
+			name:        "long CN truncated",
+			clusterName: "reallylongclustername.goteleport.com",
+			caType:      types.UserCA,
+			issuerSKID:  []byte{0x61, 0xbe, 0xe7, 0xf0, 0xb4, 0x88, 0x78, 0x33, 0x40, 0x7d, 0x7a, 0xc0, 0xa8, 0x2a, 0xeb, 0x3e, 0x9d, 0x9f, 0xa1, 0xba},
+			crlDN:       "CN=C6VEFS5KH1S36G3TFB0AGANB7QEPV8DQ_reallylongclustern,CN=Teleport,CN=CDP,CN=Public Key Services,CN=Services,CN=Configuration,DC=test,DC=goteleport,DC=com",
+		},
 	} {
 		t.Run(test.name, func(t *testing.T) {
-			require.Equal(t, test.crlDN, CRLDN(test.clusterName, "test.goteleport.com", test.caType))
+			require.Equal(t, test.crlDN, CRLDN(test.clusterName, test.issuerSKID, "test.goteleport.com", test.caType))
 		})
 	}
 }

tool/tctl/common/auth_command.go

@@ -21,7 +21,6 @@ package common
 import (
 	"context"
 	"crypto/x509"
-	"encoding/base32"
 	"encoding/pem"
 	"fmt"
 	"io"
@@ -560,7 +559,7 @@ func (a *AuthCommand) GenerateCRLForCA(ctx context.Context, clusterAPI authComma
 			return trace.Wrap(err)
 		}
 
-		cn := base32.HexEncoding.EncodeToString(cert.SubjectKeyId) + "_" + cert.Subject.CommonName
+		cn := winpki.CRLCN(cert.Subject.CommonName, cert.SubjectKeyId)
 		filename := fmt.Sprintf("%s-%v-%v.crl", a.output, certType, cn)
 		if err := os.WriteFile(filename, out.crl, os.FileMode(0644)); err != nil {
 			return trace.Wrap(err)