From c5b26b29ca837eabfde723631950074beeae6846 Mon Sep 17 00:00:00 2001 From: Luis Sanchez Date: Thu, 9 Apr 2026 15:31:24 -0400 Subject: [PATCH 1/5] certrotation: require UserInfo on PeerRotation unconditionally Previously UserInfo was only validated when ConfigurablePKI was enabled (keyGen != nil). A caller converting ServingRotation to PeerRotation without setting UserInfo would silently work on the legacy path but fail when ConfigurablePKI is enabled on the cluster. Move the nil check before the keyGen branch so it fails early regardless of path. On the legacy path, also validate that UserInfo.Name matches the sorted first hostname, since MakeServerCertForDuration uses that as the certificate CN. --- pkg/operator/certrotation/target.go | 22 ++++++++++++++-------- 1 file changed, 14 insertions(+), 8 deletions(-) diff --git a/pkg/operator/certrotation/target.go b/pkg/operator/certrotation/target.go index b76c494402..cdafce5f5b 100644 --- a/pkg/operator/certrotation/target.go +++ b/pkg/operator/certrotation/target.go @@ -436,10 +436,12 @@ func (r *SignerRotation) NewCertificate(signer *crypto.CA, validity time.Duratio // (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,10 +458,10 @@ func (r *PeerRotation) NewCertificate(signer *crypto.CA, validity time.Duration, if len(hostnames) == 0 { return nil, fmt.Errorf("no hostnames set") } + if r.UserInfo == nil { + return nil, fmt.Errorf("PeerRotation requires UserInfo") + } if keyGen != nil { - if r.UserInfo == nil { - return nil, fmt.Errorf("PeerRotation requires UserInfo for configurable PKI certificates") - } return signer.NewPeerCertificate( sets.New(hostnames...), r.UserInfo, keyGen, crypto.WithLifetime(validity), @@ -467,7 +469,11 @@ func (r *PeerRotation) NewCertificate(signer *crypto.CA, validity time.Duration, ) } // Legacy path: use server cert template with extension fn to add both ExtKeyUsages. - // The subject CN comes from the first hostname (preserves current behavior). + // 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 From 0e503f400d340d061876e3c69f8a3923c55e5ae7 Mon Sep 17 00:00:00 2001 From: Luis Sanchez Date: Thu, 9 Apr 2026 15:31:29 -0400 Subject: [PATCH 2/5] certrotation: add PeerRotation NewCertificate tests Add TestPeerRotation_NewCertificate_WithKeyPairGenerator covering both happy paths (legacy RSA, ECDSA with ConfigurablePKI) and error paths (nil UserInfo, mismatched UserInfo name). Verifies dual ExtKeyUsage (ClientAuth + ServerAuth), Subject encoding, and hostname SANs. --- pkg/operator/certrotation/target_test.go | 117 +++++++++++++++++++++++ 1 file changed, 117 insertions(+) diff --git a/pkg/operator/certrotation/target_test.go b/pkg/operator/certrotation/target_test.go index c4736b1b98..9d28fa5502 100644 --- a/pkg/operator/certrotation/target_test.go +++ b/pkg/operator/certrotation/target_test.go @@ -797,6 +797,123 @@ 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 + hasClientAuth := false + hasServerAuth := false + for _, usage := range cert.ExtKeyUsage { + if usage == x509.ExtKeyUsageClientAuth { + hasClientAuth = true + } + if usage == x509.ExtKeyUsageServerAuth { + hasServerAuth = true + } + } + if !hasClientAuth { + t.Error("missing ExtKeyUsageClientAuth") + } + if !hasServerAuth { + 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 From 7cd1e2208c6710b6c2975a4b5d6ec5e11da3015d Mon Sep 17 00:00:00 2001 From: Luis Sanchez Date: Thu, 9 Apr 2026 15:35:19 -0400 Subject: [PATCH 3/5] certrotation: use t.Context() in tests Replace context.Background() and context.TODO() with t.Context() across all certrotation test files. Remove unused "context" imports. --- pkg/operator/certrotation/cabundle_test.go | 3 +-- .../certrotation/client_cert_rotation_controller_test.go | 2 +- pkg/operator/certrotation/signer_test.go | 5 ++--- pkg/operator/certrotation/target_test.go | 5 ++--- 4 files changed, 6 insertions(+), 9 deletions(-) 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_test.go b/pkg/operator/certrotation/target_test.go index 9d28fa5502..f4dc45b358 100644 --- a/pkg/operator/certrotation/target_test.go +++ b/pkg/operator/certrotation/target_test.go @@ -1,7 +1,6 @@ package certrotation import ( - "context" "crypto/x509" "crypto/x509/pkix" "strings" @@ -302,7 +301,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 +492,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) From 08ccb6ada52df5e3e8d46c7903305d47ace1b4a3 Mon Sep 17 00:00:00 2001 From: Luis Sanchez Date: Thu, 9 Apr 2026 20:08:26 -0400 Subject: [PATCH 4/5] certrotation: use legacy-first branching in NewCertificate methods Flip the keyGen nil check in all rotation types so the legacy path is handled first and returns early, with the configurable PKI path as the clean fall-through. --- pkg/operator/certrotation/target.go | 69 ++++++++++++++--------------- 1 file changed, 34 insertions(+), 35 deletions(-) diff --git a/pkg/operator/certrotation/target.go b/pkg/operator/certrotation/target.go index cdafce5f5b..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,13 +422,13 @@ 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 @@ -461,25 +460,25 @@ func (r *PeerRotation) NewCertificate(signer *crypto.CA, validity time.Duration, if r.UserInfo == nil { return nil, fmt.Errorf("PeerRotation requires UserInfo") } - if keyGen != 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. - // 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 - } - extensions := append(append([]crypto.CertificateExtensionFunc{}, r.CertificateExtensionFn...), peerExtFn) - return signer.MakeServerCertForDuration(sets.New(hostnames...), validity, extensions...) + 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 + } + 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{} { From 27f907d96f79fe18abe553fea19aaf8d9212be51 Mon Sep 17 00:00:00 2001 From: Luis Sanchez Date: Thu, 9 Apr 2026 20:13:17 -0400 Subject: [PATCH 5/5] certrotation: use slices.Contains and add key gen test cases Use slices.Contains for ExtKeyUsage checks in PeerRotation tests. Add RSA-4096, ECDSA-P256, and ECDSA-P384 test cases to Client, Serving, and Signer rotation tests for broader key generator coverage. Co-authored-by: Roman Feldman --- pkg/operator/certrotation/target_test.go | 40 +++++++++++++++++------- 1 file changed, 28 insertions(+), 12 deletions(-) diff --git a/pkg/operator/certrotation/target_test.go b/pkg/operator/certrotation/target_test.go index f4dc45b358..b63beaef45 100644 --- a/pkg/operator/certrotation/target_test.go +++ b/pkg/operator/certrotation/target_test.go @@ -3,6 +3,7 @@ package certrotation import ( "crypto/x509" "crypto/x509/pkix" + "slices" "strings" "testing" "time" @@ -726,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) { @@ -764,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) { @@ -872,20 +888,10 @@ func TestPeerRotation_NewCertificate_WithKeyPairGenerator(t *testing.T) { } // Verify both ExtKeyUsages are present - hasClientAuth := false - hasServerAuth := false - for _, usage := range cert.ExtKeyUsage { - if usage == x509.ExtKeyUsageClientAuth { - hasClientAuth = true - } - if usage == x509.ExtKeyUsageServerAuth { - hasServerAuth = true - } - } - if !hasClientAuth { + if !slices.Contains(cert.ExtKeyUsage, x509.ExtKeyUsageClientAuth) { t.Error("missing ExtKeyUsageClientAuth") } - if !hasServerAuth { + if !slices.Contains(cert.ExtKeyUsage, x509.ExtKeyUsageServerAuth) { t.Error("missing ExtKeyUsageServerAuth") } @@ -924,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},