fix: remove KeyEncipherment usage for non-RSA certificates (#48158)

gravitational · Oct 30, 2024 · f078612 · f078612
1 parent ffa258b
commit f078612
Show file tree

Hide file tree

Showing 4 changed files with 80 additions and 10 deletions.
diff --git a/lib/tlsca/ca.go b/lib/tlsca/ca.go
@@ -21,6 +21,7 @@ package tlsca
 import (
 	"crypto"
 	"crypto/rand"
+	"crypto/rsa"
 	"crypto/tls"
 	"crypto/x509"
 	"crypto/x509/pkix"
@@ -1199,7 +1200,12 @@ func (c *CertificateRequest) CheckAndSetDefaults() error {
 		return trace.BadParameter("missing parameter NotAfter")
 	}
 	if c.KeyUsage == 0 {
-		c.KeyUsage = x509.KeyUsageKeyEncipherment | x509.KeyUsageDigitalSignature
+		c.KeyUsage = x509.KeyUsageDigitalSignature
+		if _, isRSA := c.PublicKey.(*rsa.PublicKey); isRSA {
+			// The KeyEncipherment bit is necessary for RSA key exchanges
+			// https://datatracker.ietf.org/doc/html/rfc5280#section-4.2.1.3
+			c.KeyUsage |= x509.KeyUsageKeyEncipherment
+		}
 	}
 
 	c.DNSNames = utils.Deduplicate(c.DNSNames)

diff --git a/lib/tlsca/ca_test.go b/lib/tlsca/ca_test.go
@@ -19,7 +19,9 @@
 package tlsca
 
 import (
+	"crypto"
 	"crypto/tls"
+	"crypto/x509"
 	"crypto/x509/pkix"
 	"encoding/asn1"
 	"testing"
@@ -37,6 +39,7 @@ import (
 	apievents "github.com/gravitational/teleport/api/types/events"
 	"github.com/gravitational/teleport/api/utils/keys"
 	"github.com/gravitational/teleport/lib/cryptosuites"
+	"github.com/gravitational/teleport/lib/defaults"
 	"github.com/gravitational/teleport/lib/fixtures"
 )
 
@@ -471,3 +474,57 @@ func TestIdentity_GetUserMetadata(t *testing.T) {
 		})
 	}
 }
+
+// TestKeyUsage asserts that only certs with RSA subject keys get the
+// KeyEncipherment keyUsage extension.
+func TestKeyUsage(t *testing.T) {
+	rsaKey, err := keys.ParsePrivateKey(fixtures.PEMBytes["rsa"])
+	require.NoError(t, err)
+	ecdsaKey, err := cryptosuites.GenerateKeyWithAlgorithm(cryptosuites.ECDSAP256)
+	require.NoError(t, err)
+	for _, tc := range []struct {
+		algo                  string
+		key                   crypto.Signer
+		expectCAKeyUsage      x509.KeyUsage
+		expectSubjectKeyUsage x509.KeyUsage
+	}{
+		{
+			algo:                  "RSA",
+			key:                   rsaKey,
+			expectCAKeyUsage:      x509.KeyUsageDigitalSignature | x509.KeyUsageCertSign | x509.KeyUsageCRLSign | x509.KeyUsageKeyEncipherment,
+			expectSubjectKeyUsage: x509.KeyUsageDigitalSignature | x509.KeyUsageKeyEncipherment,
+		},
+		{
+			algo:                  "ECDSA",
+			key:                   ecdsaKey,
+			expectCAKeyUsage:      x509.KeyUsageDigitalSignature | x509.KeyUsageCertSign | x509.KeyUsageCRLSign,
+			expectSubjectKeyUsage: x509.KeyUsageDigitalSignature,
+		},
+	} {
+		t.Run(tc.algo, func(t *testing.T) {
+			caPEM, err := GenerateSelfSignedCAWithSigner(tc.key, pkix.Name{
+				CommonName:   "teleport.example.com",
+				Organization: []string{"teleport.example.com"},
+			}, nil /*dnsNames*/, defaults.CATTL)
+			require.NoError(t, err)
+
+			ca, err := FromCertAndSigner(caPEM, tc.key)
+			require.NoError(t, err)
+			require.Equal(t, tc.expectCAKeyUsage, ca.Cert.KeyUsage)
+
+			subjectPEM, err := ca.GenerateCertificate(CertificateRequest{
+				PublicKey: tc.key.Public(),
+				Subject: pkix.Name{
+					CommonName:   "teleport.example.com",
+					Organization: []string{"teleport.example.com"},
+				},
+				NotAfter: time.Now().Add(time.Hour),
+			})
+			require.NoError(t, err)
+
+			subject, err := ParseCertificatePEM(subjectPEM)
+			require.NoError(t, err)
+			require.Equal(t, tc.expectSubjectKeyUsage, subject.KeyUsage)
+		})
+	}
+}
diff --git a/lib/tlsca/parsegen.go b/lib/tlsca/parsegen.go
@@ -92,15 +92,22 @@ func GenerateSelfSignedCAWithConfig(config GenerateCAConfig) (certPEM []byte, er
 	// signed by the same private key and having the same subject (happens in tests)
 	config.Entity.SerialNumber = serialNumber.String()
 
+	// Note: KeyUsageCRLSign is set only to generate empty CRLs for Desktop
+	// Access authentication with Windows.
+	keyUsage := x509.KeyUsageDigitalSignature | x509.KeyUsageCertSign | x509.KeyUsageCRLSign
+	if _, isRSA := config.Signer.Public().(*rsa.PublicKey); isRSA {
+		// The KeyEncipherment bit is necessary for RSA key exchanges
+		// https://datatracker.ietf.org/doc/html/rfc5280#section-4.2.1.3
+		keyUsage |= x509.KeyUsageKeyEncipherment
+	}
+
 	template := x509.Certificate{
-		SerialNumber: serialNumber,
-		Issuer:       config.Entity,
-		Subject:      config.Entity,
-		NotBefore:    notBefore,
-		NotAfter:     notAfter,
-		// Note: KeyUsageCRLSign is set only to generate empty CRLs for Desktop
-		// Access authentication with Windows.
-		KeyUsage:              x509.KeyUsageKeyEncipherment | x509.KeyUsageDigitalSignature | x509.KeyUsageCertSign | x509.KeyUsageCRLSign,
+		SerialNumber:          serialNumber,
+		Issuer:                config.Entity,
+		Subject:               config.Entity,
+		NotBefore:             notBefore,
+		NotAfter:              notAfter,
+		KeyUsage:              keyUsage,
 		BasicConstraintsValid: true,
 		IsCA:                  true,
 		DNSNames:              config.DNSNames,

diff --git a/lib/utils/cert/selfsigned.go b/lib/utils/cert/selfsigned.go
@@ -84,7 +84,7 @@ func GenerateSelfSignedCert(hostNames []string, ipAddresses []string, eku ...x50
 		Subject:               entity,
 		NotBefore:             notBefore,
 		NotAfter:              notAfter,
-		KeyUsage:              x509.KeyUsageKeyEncipherment | x509.KeyUsageDigitalSignature | x509.KeyUsageCertSign,
+		KeyUsage:              x509.KeyUsageDigitalSignature | x509.KeyUsageCertSign,
 		ExtKeyUsage:           eku,
 		BasicConstraintsValid: true,
 		IsCA:                  true,