From 81be53940835555f63a9e1e221b8f0c4b90b8279 Mon Sep 17 00:00:00 2001 From: Nic Klaassen Date: Wed, 30 Oct 2024 11:27:43 -0700 Subject: [PATCH] fix: remove KeyEncipherment usage for non-RSA certificates --- lib/tlsca/ca.go | 8 ++++- lib/tlsca/ca_test.go | 57 ++++++++++++++++++++++++++++++++++++ lib/tlsca/parsegen.go | 23 ++++++++++----- lib/utils/cert/selfsigned.go | 2 +- 4 files changed, 80 insertions(+), 10 deletions(-) diff --git a/lib/tlsca/ca.go b/lib/tlsca/ca.go index 19e04dc386d45..ce7d3cad32f0a 100644 --- 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 index 91b4db336e173..d565a9f00d3bc 100644 --- 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 index 7d667ebaa19df..055670870466b 100644 --- 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 index cde88823f1af0..ea1f4adeda36a 100644 --- 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,