diff --git a/pkg/operator/certrotation/cabundle_test.go b/pkg/operator/certrotation/cabundle_test.go index cf15bafff1..253fcf3c2b 100644 --- a/pkg/operator/certrotation/cabundle_test.go +++ b/pkg/operator/certrotation/cabundle_test.go @@ -1,7 +1,6 @@ package certrotation import ( - "context" gcrypto "crypto" "crypto/rand" "crypto/x509" @@ -216,7 +215,7 @@ func TestEnsureConfigMapCABundle(t *testing.T) { if err != nil { t.Fatal(err) } - _, err = c.EnsureConfigMapCABundle(context.TODO(), newCA, "signer-secret") + _, err = c.EnsureConfigMapCABundle(t.Context(), newCA, "signer-secret") switch { case err != nil && len(test.expectedError) == 0: t.Error(err) diff --git a/pkg/operator/certrotation/client_cert_rotation_controller_test.go b/pkg/operator/certrotation/client_cert_rotation_controller_test.go index efc6ab22e9..c16ff047cc 100644 --- a/pkg/operator/certrotation/client_cert_rotation_controller_test.go +++ b/pkg/operator/certrotation/client_cert_rotation_controller_test.go @@ -300,7 +300,7 @@ func TestCertRotationController_SyncWorker(t *testing.T) { } // Run SyncWorker - ctx := context.Background() + ctx := t.Context() err := controller.SyncWorker(ctx) // Check error diff --git a/pkg/operator/certrotation/signer_test.go b/pkg/operator/certrotation/signer_test.go index c19117c565..c26e2379d7 100644 --- a/pkg/operator/certrotation/signer_test.go +++ b/pkg/operator/certrotation/signer_test.go @@ -1,7 +1,6 @@ package certrotation import ( - "context" "crypto/x509" "encoding/pem" "strings" @@ -271,7 +270,7 @@ func TestEnsureSigningCertKeyPair(t *testing.T) { RefreshOnlyWhenExpired: test.RefreshOnlyWhenExpired, } - _, updated, err := c.EnsureSigningCertKeyPair(context.TODO()) + _, updated, err := c.EnsureSigningCertKeyPair(t.Context()) switch { case err != nil && len(test.expectedError) == 0: t.Error(err) @@ -422,7 +421,7 @@ func TestEnsureSigningCertKeyPair_WithKeyConfig(t *testing.T) { }, } - ca, updated, err := c.EnsureSigningCertKeyPair(context.TODO()) + ca, updated, err := c.EnsureSigningCertKeyPair(t.Context()) if err != nil { t.Fatalf("EnsureSigningCertKeyPair() error = %v", err) } diff --git a/pkg/operator/certrotation/target.go b/pkg/operator/certrotation/target.go index b76c494402..d1c0a59ee8 100644 --- a/pkg/operator/certrotation/target.go +++ b/pkg/operator/certrotation/target.go @@ -327,10 +327,10 @@ func (r *ClientRotation) CertificateType() pki.CertificateType { } func (r *ClientRotation) NewCertificate(signer *crypto.CA, validity time.Duration, keyGen crypto.KeyPairGenerator) (*crypto.TLSCertificateConfig, error) { - if keyGen != nil { - return signer.NewClientCertificate(r.UserInfo, keyGen, crypto.WithLifetime(validity)) + if keyGen == nil { + return signer.MakeClientCertificateForDuration(r.UserInfo, validity) } - return signer.MakeClientCertificateForDuration(r.UserInfo, validity) + return signer.NewClientCertificate(r.UserInfo, keyGen, crypto.WithLifetime(validity)) } func (r *ClientRotation) NeedNewTargetCertKeyPair(currentCertSecret *corev1.Secret, signer *crypto.CA, caBundleCerts []*x509.Certificate, refresh time.Duration, refreshOnlyWhenExpired, exists bool) string { @@ -356,14 +356,13 @@ func (r *ServingRotation) NewCertificate(signer *crypto.CA, validity time.Durati if len(hostnames) == 0 { return nil, fmt.Errorf("no hostnames set") } - if keyGen != nil { - return signer.NewServerCertificate( - sets.New(hostnames...), keyGen, - crypto.WithLifetime(validity), - crypto.WithExtensions(r.CertificateExtensionFn...), - ) + if keyGen == nil { + return signer.MakeServerCertForDuration(sets.New(hostnames...), validity, r.CertificateExtensionFn...) } - return signer.MakeServerCertForDuration(sets.New(hostnames...), validity, r.CertificateExtensionFn...) + return signer.NewServerCertificate(sets.New(hostnames...), keyGen, + crypto.WithLifetime(validity), + crypto.WithExtensions(r.CertificateExtensionFn...), + ) } func (r *ServingRotation) RecheckChannel() <-chan struct{} { @@ -423,23 +422,25 @@ func (r *SignerRotation) CertificateType() pki.CertificateType { func (r *SignerRotation) NewCertificate(signer *crypto.CA, validity time.Duration, keyGen crypto.KeyPairGenerator) (*crypto.TLSCertificateConfig, error) { signerName := fmt.Sprintf("%s_@%d", r.SignerName, time.Now().Unix()) - if keyGen != nil { - return crypto.NewSigningCertificate(signerName, keyGen, - crypto.WithSigner(signer), - crypto.WithLifetime(validity), - ) + if keyGen == nil { + return crypto.MakeCAConfigForDuration(signerName, validity, signer) } - return crypto.MakeCAConfigForDuration(signerName, validity, signer) + return crypto.NewSigningCertificate(signerName, keyGen, + crypto.WithSigner(signer), + crypto.WithLifetime(validity), + ) } // PeerRotation creates certificates used for both server and client authentication // (e.g., etcd peer certificates). It uses CertificateTypePeer for PKI profile // resolution, which selects the stronger of the serving and client key configurations. // -// When keyGen is non-nil (ConfigurablePKI enabled), it calls NewPeerCertificate -// which requires UserInfo for the client identity. When keyGen is nil (legacy), -// it falls back to MakeServerCertForDuration with an extension function that -// sets both ClientAuth and ServerAuth ExtKeyUsages. +// UserInfo is always required. When keyGen is non-nil (ConfigurablePKI enabled), +// NewPeerCertificate encodes UserInfo as the client identity in the certificate +// subject. When keyGen is nil (legacy), UserInfo.Name must match the sorted first +// hostname (used as CN by MakeServerCertForDuration), and the certificate falls +// back to MakeServerCertForDuration with an extension function that sets both +// ClientAuth and ServerAuth ExtKeyUsages. type PeerRotation struct { Hostnames ServingHostnameFunc UserInfo user.Info @@ -456,24 +457,28 @@ func (r *PeerRotation) NewCertificate(signer *crypto.CA, validity time.Duration, if len(hostnames) == 0 { return nil, fmt.Errorf("no hostnames set") } - if keyGen != nil { - if r.UserInfo == nil { - return nil, fmt.Errorf("PeerRotation requires UserInfo for configurable PKI certificates") + if r.UserInfo == nil { + return nil, fmt.Errorf("PeerRotation requires UserInfo") + } + if keyGen == nil { + // Legacy path: use server cert template with extension fn to add both ExtKeyUsages. + // MakeServerCertForDuration sorts hostnames and uses the first sorted value as CN. + sortedHostnames := sets.List(sets.New(hostnames...)) + if cn := sortedHostnames[0]; r.UserInfo.GetName() != cn { + return nil, fmt.Errorf("PeerRotation legacy path uses sorted first hostname %q as CN; UserInfo.Name %q conflicts — set UserInfo.Name to match or enable ConfigurablePKI", cn, r.UserInfo.GetName()) + } + peerExtFn := func(cert *x509.Certificate) error { + cert.ExtKeyUsage = []x509.ExtKeyUsage{x509.ExtKeyUsageClientAuth, x509.ExtKeyUsageServerAuth} + return nil } - return signer.NewPeerCertificate( - sets.New(hostnames...), r.UserInfo, keyGen, - crypto.WithLifetime(validity), - crypto.WithExtensions(r.CertificateExtensionFn...), - ) - } - // Legacy path: use server cert template with extension fn to add both ExtKeyUsages. - // The subject CN comes from the first hostname (preserves current behavior). - peerExtFn := func(cert *x509.Certificate) error { - cert.ExtKeyUsage = []x509.ExtKeyUsage{x509.ExtKeyUsageClientAuth, x509.ExtKeyUsageServerAuth} - return nil - } - extensions := append(append([]crypto.CertificateExtensionFunc{}, r.CertificateExtensionFn...), peerExtFn) - return signer.MakeServerCertForDuration(sets.New(hostnames...), validity, extensions...) + extensions := append(append([]crypto.CertificateExtensionFunc{}, r.CertificateExtensionFn...), peerExtFn) + return signer.MakeServerCertForDuration(sets.New(hostnames...), validity, extensions...) + } + return signer.NewPeerCertificate( + sets.New(hostnames...), r.UserInfo, keyGen, + crypto.WithLifetime(validity), + crypto.WithExtensions(r.CertificateExtensionFn...), + ) } func (r *PeerRotation) RecheckChannel() <-chan struct{} { diff --git a/pkg/operator/certrotation/target_test.go b/pkg/operator/certrotation/target_test.go index c4736b1b98..b63beaef45 100644 --- a/pkg/operator/certrotation/target_test.go +++ b/pkg/operator/certrotation/target_test.go @@ -1,9 +1,9 @@ package certrotation import ( - "context" "crypto/x509" "crypto/x509/pkix" + "slices" "strings" "testing" "time" @@ -302,7 +302,7 @@ func TestEnsureTargetCertKeyPair(t *testing.T) { if err != nil { t.Fatal(err) } - _, err = c.EnsureTargetCertKeyPair(context.TODO(), newCA, newCA.Config.Certs) + _, err = c.EnsureTargetCertKeyPair(t.Context(), newCA, newCA.Config.Certs) switch { case err != nil && len(test.expectedError) == 0: t.Error(err) @@ -493,7 +493,7 @@ func TestEnsureTargetSignerCertKeyPair(t *testing.T) { if err != nil { t.Fatal(err) } - _, err = c.EnsureTargetCertKeyPair(context.TODO(), newCA, newCA.Config.Certs) + _, err = c.EnsureTargetCertKeyPair(t.Context(), newCA, newCA.Config.Certs) switch { case err != nil && len(test.expectedError) == 0: t.Error(err) @@ -727,6 +727,11 @@ func TestClientRotation_NewCertificate_WithKeyPairGenerator(t *testing.T) { keyGen: crypto.ECDSAKeyPairGenerator{Curve: crypto.P256}, wantAlg: x509.ECDSA, }, + { + name: "ECDSA-P384", + keyGen: crypto.ECDSAKeyPairGenerator{Curve: crypto.P384}, + wantAlg: x509.ECDSA, + }, } for _, tc := range testCases { t.Run(tc.name, func(t *testing.T) { @@ -765,11 +770,21 @@ func TestServingRotation_NewCertificate_WithKeyPairGenerator(t *testing.T) { keyGen: nil, wantAlg: x509.RSA, }, + { + name: "RSA-4096", + keyGen: crypto.RSAKeyPairGenerator{Bits: 4096}, + wantAlg: x509.RSA, + }, { name: "ECDSA-P256", keyGen: crypto.ECDSAKeyPairGenerator{Curve: crypto.P256}, wantAlg: x509.ECDSA, }, + { + name: "ECDSA-P384", + keyGen: crypto.ECDSAKeyPairGenerator{Curve: crypto.P384}, + wantAlg: x509.ECDSA, + }, } for _, tc := range testCases { t.Run(tc.name, func(t *testing.T) { @@ -797,6 +812,113 @@ func TestServingRotation_NewCertificate_WithKeyPairGenerator(t *testing.T) { } } +func TestPeerRotation_NewCertificate_WithKeyPairGenerator(t *testing.T) { + testCases := []struct { + name string + keyGen crypto.KeyPairGenerator + userInfo user.Info + hostnames []string + wantAlg x509.PublicKeyAlgorithm + wantErr string + }{ + { + name: "nil keyGen (legacy) uses RSA with dual ExtKeyUsage", + keyGen: nil, + userInfo: &user.DefaultInfo{Name: "127.0.0.1"}, + hostnames: []string{"localhost", "127.0.0.1"}, + wantAlg: x509.RSA, + }, + { + name: "ECDSA keyGen with UserInfo", + keyGen: crypto.ECDSAKeyPairGenerator{Curve: crypto.P256}, + userInfo: &user.DefaultInfo{Name: "peer-user", Groups: []string{"peer-group"}}, + hostnames: []string{"localhost", "127.0.0.1"}, + wantAlg: x509.ECDSA, + }, + { + name: "nil UserInfo errors", + keyGen: nil, + userInfo: nil, + hostnames: []string{"localhost"}, + wantErr: "requires UserInfo", + }, + { + name: "nil UserInfo with keyGen errors", + keyGen: crypto.ECDSAKeyPairGenerator{Curve: crypto.P256}, + userInfo: nil, + hostnames: []string{"localhost"}, + wantErr: "requires UserInfo", + }, + { + name: "mismatched UserInfo name on legacy path", + keyGen: nil, + userInfo: &user.DefaultInfo{Name: "wrong-name"}, + hostnames: []string{"localhost", "127.0.0.1"}, + wantErr: "conflicts", + }, + } + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + ca, err := newTestCACertificate(pkix.Name{CommonName: "test-ca"}, int64(1), metav1.Duration{Duration: time.Hour * 24}, time.Now) + if err != nil { + t.Fatal(err) + } + + r := &PeerRotation{ + Hostnames: func() []string { return tc.hostnames }, + UserInfo: tc.userInfo, + } + certConfig, err := r.NewCertificate(ca, time.Hour, tc.keyGen) + if tc.wantErr != "" { + if err == nil { + t.Fatalf("expected error containing %q, got nil", tc.wantErr) + } + if !strings.Contains(err.Error(), tc.wantErr) { + t.Fatalf("expected error containing %q, got %q", tc.wantErr, err.Error()) + } + return + } + if err != nil { + t.Fatalf("NewCertificate() error = %v", err) + } + + cert := certConfig.Certs[0] + if cert.PublicKeyAlgorithm != tc.wantAlg { + t.Errorf("PublicKeyAlgorithm = %v, want %v", cert.PublicKeyAlgorithm, tc.wantAlg) + } + + // Verify both ExtKeyUsages are present + if !slices.Contains(cert.ExtKeyUsage, x509.ExtKeyUsageClientAuth) { + t.Error("missing ExtKeyUsageClientAuth") + } + if !slices.Contains(cert.ExtKeyUsage, x509.ExtKeyUsageServerAuth) { + t.Error("missing ExtKeyUsageServerAuth") + } + + // Verify hostnames in SANs + if len(cert.DNSNames) == 0 { + t.Error("expected DNS SANs") + } + + // Verify Subject based on path + if tc.keyGen != nil { + // ConfigurablePKI path: UserInfo encoded in Subject + if cert.Subject.CommonName != tc.userInfo.GetName() { + t.Errorf("CN = %q, want %q", cert.Subject.CommonName, tc.userInfo.GetName()) + } + if len(tc.userInfo.GetGroups()) > 0 && len(cert.Subject.Organization) == 0 { + t.Error("expected Organization from UserInfo.Groups") + } + } else { + // Legacy path: CN = sorted first hostname (MakeServerCertForDuration sorts) + if cert.Subject.CommonName != tc.userInfo.GetName() { + t.Errorf("CN = %q, want %q (must match UserInfo.Name which must match sorted first hostname)", cert.Subject.CommonName, tc.userInfo.GetName()) + } + } + }) + } +} + func TestSignerRotation_NewCertificate_WithKeyPairGenerator(t *testing.T) { testCases := []struct { name string @@ -808,6 +930,16 @@ func TestSignerRotation_NewCertificate_WithKeyPairGenerator(t *testing.T) { keyGen: nil, wantAlg: x509.RSA, }, + { + name: "RSA-4096", + keyGen: crypto.RSAKeyPairGenerator{Bits: 4096}, + wantAlg: x509.RSA, + }, + { + name: "ECDSA-P256", + keyGen: crypto.ECDSAKeyPairGenerator{Curve: crypto.P256}, + wantAlg: x509.ECDSA, + }, { name: "ECDSA-P384", keyGen: crypto.ECDSAKeyPairGenerator{Curve: crypto.P384},