Skip to content

Commit c7d4322

Browse files
openshift-merge-bot[bot]openshift-merge-bot[bot]
authored
Merge pull request #2157 from sanchezl/peerrotation-userinfo-followup
NO-JIRA: certrotation: require UserInfo on PeerRotation, add tests
2 parents 3ad832f + 27f907d commit c7d4322

5 files changed

Lines changed: 181 additions & 46 deletions

File tree

‎pkg/operator/certrotation/cabundle_test.go‎

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,6 @@
11
package certrotation
22

33
import (
4-
"context"
54
gcrypto "crypto"
65
"crypto/rand"
76
"crypto/x509"
@@ -216,7 +215,7 @@ func TestEnsureConfigMapCABundle(t *testing.T) {
216215
if err != nil {
217216
t.Fatal(err)
218217
}
219-
_, err = c.EnsureConfigMapCABundle(context.TODO(), newCA, "signer-secret")
218+
_, err = c.EnsureConfigMapCABundle(t.Context(), newCA, "signer-secret")
220219
switch {
221220
case err != nil && len(test.expectedError) == 0:
222221
t.Error(err)

‎pkg/operator/certrotation/client_cert_rotation_controller_test.go‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -300,7 +300,7 @@ func TestCertRotationController_SyncWorker(t *testing.T) {
300300
}
301301

302302
// Run SyncWorker
303-
ctx := context.Background()
303+
ctx := t.Context()
304304
err := controller.SyncWorker(ctx)
305305

306306
// Check error

‎pkg/operator/certrotation/signer_test.go‎

Lines changed: 2 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,6 @@
11
package certrotation
22

33
import (
4-
"context"
54
"crypto/x509"
65
"encoding/pem"
76
"strings"
@@ -271,7 +270,7 @@ func TestEnsureSigningCertKeyPair(t *testing.T) {
271270
RefreshOnlyWhenExpired: test.RefreshOnlyWhenExpired,
272271
}
273272

274-
_, updated, err := c.EnsureSigningCertKeyPair(context.TODO())
273+
_, updated, err := c.EnsureSigningCertKeyPair(t.Context())
275274
switch {
276275
case err != nil && len(test.expectedError) == 0:
277276
t.Error(err)
@@ -422,7 +421,7 @@ func TestEnsureSigningCertKeyPair_WithKeyConfig(t *testing.T) {
422421
},
423422
}
424423

425-
ca, updated, err := c.EnsureSigningCertKeyPair(context.TODO())
424+
ca, updated, err := c.EnsureSigningCertKeyPair(t.Context())
426425
if err != nil {
427426
t.Fatalf("EnsureSigningCertKeyPair() error = %v", err)
428427
}

‎pkg/operator/certrotation/target.go‎

Lines changed: 42 additions & 37 deletions
Original file line numberDiff line numberDiff line change
@@ -327,10 +327,10 @@ func (r *ClientRotation) CertificateType() pki.CertificateType {
327327
}
328328

329329
func (r *ClientRotation) NewCertificate(signer *crypto.CA, validity time.Duration, keyGen crypto.KeyPairGenerator) (*crypto.TLSCertificateConfig, error) {
330-
if keyGen != nil {
331-
return signer.NewClientCertificate(r.UserInfo, keyGen, crypto.WithLifetime(validity))
330+
if keyGen == nil {
331+
return signer.MakeClientCertificateForDuration(r.UserInfo, validity)
332332
}
333-
return signer.MakeClientCertificateForDuration(r.UserInfo, validity)
333+
return signer.NewClientCertificate(r.UserInfo, keyGen, crypto.WithLifetime(validity))
334334
}
335335

336336
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
356356
if len(hostnames) == 0 {
357357
return nil, fmt.Errorf("no hostnames set")
358358
}
359-
if keyGen != nil {
360-
return signer.NewServerCertificate(
361-
sets.New(hostnames...), keyGen,
362-
crypto.WithLifetime(validity),
363-
crypto.WithExtensions(r.CertificateExtensionFn...),
364-
)
359+
if keyGen == nil {
360+
return signer.MakeServerCertForDuration(sets.New(hostnames...), validity, r.CertificateExtensionFn...)
365361
}
366-
return signer.MakeServerCertForDuration(sets.New(hostnames...), validity, r.CertificateExtensionFn...)
362+
return signer.NewServerCertificate(sets.New(hostnames...), keyGen,
363+
crypto.WithLifetime(validity),
364+
crypto.WithExtensions(r.CertificateExtensionFn...),
365+
)
367366
}
368367

369368
func (r *ServingRotation) RecheckChannel() <-chan struct{} {
@@ -423,23 +422,25 @@ func (r *SignerRotation) CertificateType() pki.CertificateType {
423422

424423
func (r *SignerRotation) NewCertificate(signer *crypto.CA, validity time.Duration, keyGen crypto.KeyPairGenerator) (*crypto.TLSCertificateConfig, error) {
425424
signerName := fmt.Sprintf("%s_@%d", r.SignerName, time.Now().Unix())
426-
if keyGen != nil {
427-
return crypto.NewSigningCertificate(signerName, keyGen,
428-
crypto.WithSigner(signer),
429-
crypto.WithLifetime(validity),
430-
)
425+
if keyGen == nil {
426+
return crypto.MakeCAConfigForDuration(signerName, validity, signer)
431427
}
432-
return crypto.MakeCAConfigForDuration(signerName, validity, signer)
428+
return crypto.NewSigningCertificate(signerName, keyGen,
429+
crypto.WithSigner(signer),
430+
crypto.WithLifetime(validity),
431+
)
433432
}
434433

435434
// PeerRotation creates certificates used for both server and client authentication
436435
// (e.g., etcd peer certificates). It uses CertificateTypePeer for PKI profile
437436
// resolution, which selects the stronger of the serving and client key configurations.
438437
//
439-
// When keyGen is non-nil (ConfigurablePKI enabled), it calls NewPeerCertificate
440-
// which requires UserInfo for the client identity. When keyGen is nil (legacy),
441-
// it falls back to MakeServerCertForDuration with an extension function that
442-
// sets both ClientAuth and ServerAuth ExtKeyUsages.
438+
// UserInfo is always required. When keyGen is non-nil (ConfigurablePKI enabled),
439+
// NewPeerCertificate encodes UserInfo as the client identity in the certificate
440+
// subject. When keyGen is nil (legacy), UserInfo.Name must match the sorted first
441+
// hostname (used as CN by MakeServerCertForDuration), and the certificate falls
442+
// back to MakeServerCertForDuration with an extension function that sets both
443+
// ClientAuth and ServerAuth ExtKeyUsages.
443444
type PeerRotation struct {
444445
Hostnames ServingHostnameFunc
445446
UserInfo user.Info
@@ -456,24 +457,28 @@ func (r *PeerRotation) NewCertificate(signer *crypto.CA, validity time.Duration,
456457
if len(hostnames) == 0 {
457458
return nil, fmt.Errorf("no hostnames set")
458459
}
459-
if keyGen != nil {
460-
if r.UserInfo == nil {
461-
return nil, fmt.Errorf("PeerRotation requires UserInfo for configurable PKI certificates")
460+
if r.UserInfo == nil {
461+
return nil, fmt.Errorf("PeerRotation requires UserInfo")
462+
}
463+
if keyGen == nil {
464+
// Legacy path: use server cert template with extension fn to add both ExtKeyUsages.
465+
// MakeServerCertForDuration sorts hostnames and uses the first sorted value as CN.
466+
sortedHostnames := sets.List(sets.New(hostnames...))
467+
if cn := sortedHostnames[0]; r.UserInfo.GetName() != cn {
468+
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())
469+
}
470+
peerExtFn := func(cert *x509.Certificate) error {
471+
cert.ExtKeyUsage = []x509.ExtKeyUsage{x509.ExtKeyUsageClientAuth, x509.ExtKeyUsageServerAuth}
472+
return nil
462473
}
463-
return signer.NewPeerCertificate(
464-
sets.New(hostnames...), r.UserInfo, keyGen,
465-
crypto.WithLifetime(validity),
466-
crypto.WithExtensions(r.CertificateExtensionFn...),
467-
)
468-
}
469-
// Legacy path: use server cert template with extension fn to add both ExtKeyUsages.
470-
// The subject CN comes from the first hostname (preserves current behavior).
471-
peerExtFn := func(cert *x509.Certificate) error {
472-
cert.ExtKeyUsage = []x509.ExtKeyUsage{x509.ExtKeyUsageClientAuth, x509.ExtKeyUsageServerAuth}
473-
return nil
474-
}
475-
extensions := append(append([]crypto.CertificateExtensionFunc{}, r.CertificateExtensionFn...), peerExtFn)
476-
return signer.MakeServerCertForDuration(sets.New(hostnames...), validity, extensions...)
474+
extensions := append(append([]crypto.CertificateExtensionFunc{}, r.CertificateExtensionFn...), peerExtFn)
475+
return signer.MakeServerCertForDuration(sets.New(hostnames...), validity, extensions...)
476+
}
477+
return signer.NewPeerCertificate(
478+
sets.New(hostnames...), r.UserInfo, keyGen,
479+
crypto.WithLifetime(validity),
480+
crypto.WithExtensions(r.CertificateExtensionFn...),
481+
)
477482
}
478483

479484
func (r *PeerRotation) RecheckChannel() <-chan struct{} {

‎pkg/operator/certrotation/target_test.go‎

Lines changed: 135 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1,9 +1,9 @@
11
package certrotation
22

33
import (
4-
"context"
54
"crypto/x509"
65
"crypto/x509/pkix"
6+
"slices"
77
"strings"
88
"testing"
99
"time"
@@ -302,7 +302,7 @@ func TestEnsureTargetCertKeyPair(t *testing.T) {
302302
if err != nil {
303303
t.Fatal(err)
304304
}
305-
_, err = c.EnsureTargetCertKeyPair(context.TODO(), newCA, newCA.Config.Certs)
305+
_, err = c.EnsureTargetCertKeyPair(t.Context(), newCA, newCA.Config.Certs)
306306
switch {
307307
case err != nil && len(test.expectedError) == 0:
308308
t.Error(err)
@@ -493,7 +493,7 @@ func TestEnsureTargetSignerCertKeyPair(t *testing.T) {
493493
if err != nil {
494494
t.Fatal(err)
495495
}
496-
_, err = c.EnsureTargetCertKeyPair(context.TODO(), newCA, newCA.Config.Certs)
496+
_, err = c.EnsureTargetCertKeyPair(t.Context(), newCA, newCA.Config.Certs)
497497
switch {
498498
case err != nil && len(test.expectedError) == 0:
499499
t.Error(err)
@@ -727,6 +727,11 @@ func TestClientRotation_NewCertificate_WithKeyPairGenerator(t *testing.T) {
727727
keyGen: crypto.ECDSAKeyPairGenerator{Curve: crypto.P256},
728728
wantAlg: x509.ECDSA,
729729
},
730+
{
731+
name: "ECDSA-P384",
732+
keyGen: crypto.ECDSAKeyPairGenerator{Curve: crypto.P384},
733+
wantAlg: x509.ECDSA,
734+
},
730735
}
731736
for _, tc := range testCases {
732737
t.Run(tc.name, func(t *testing.T) {
@@ -765,11 +770,21 @@ func TestServingRotation_NewCertificate_WithKeyPairGenerator(t *testing.T) {
765770
keyGen: nil,
766771
wantAlg: x509.RSA,
767772
},
773+
{
774+
name: "RSA-4096",
775+
keyGen: crypto.RSAKeyPairGenerator{Bits: 4096},
776+
wantAlg: x509.RSA,
777+
},
768778
{
769779
name: "ECDSA-P256",
770780
keyGen: crypto.ECDSAKeyPairGenerator{Curve: crypto.P256},
771781
wantAlg: x509.ECDSA,
772782
},
783+
{
784+
name: "ECDSA-P384",
785+
keyGen: crypto.ECDSAKeyPairGenerator{Curve: crypto.P384},
786+
wantAlg: x509.ECDSA,
787+
},
773788
}
774789
for _, tc := range testCases {
775790
t.Run(tc.name, func(t *testing.T) {
@@ -797,6 +812,113 @@ func TestServingRotation_NewCertificate_WithKeyPairGenerator(t *testing.T) {
797812
}
798813
}
799814

815+
func TestPeerRotation_NewCertificate_WithKeyPairGenerator(t *testing.T) {
816+
testCases := []struct {
817+
name string
818+
keyGen crypto.KeyPairGenerator
819+
userInfo user.Info
820+
hostnames []string
821+
wantAlg x509.PublicKeyAlgorithm
822+
wantErr string
823+
}{
824+
{
825+
name: "nil keyGen (legacy) uses RSA with dual ExtKeyUsage",
826+
keyGen: nil,
827+
userInfo: &user.DefaultInfo{Name: "127.0.0.1"},
828+
hostnames: []string{"localhost", "127.0.0.1"},
829+
wantAlg: x509.RSA,
830+
},
831+
{
832+
name: "ECDSA keyGen with UserInfo",
833+
keyGen: crypto.ECDSAKeyPairGenerator{Curve: crypto.P256},
834+
userInfo: &user.DefaultInfo{Name: "peer-user", Groups: []string{"peer-group"}},
835+
hostnames: []string{"localhost", "127.0.0.1"},
836+
wantAlg: x509.ECDSA,
837+
},
838+
{
839+
name: "nil UserInfo errors",
840+
keyGen: nil,
841+
userInfo: nil,
842+
hostnames: []string{"localhost"},
843+
wantErr: "requires UserInfo",
844+
},
845+
{
846+
name: "nil UserInfo with keyGen errors",
847+
keyGen: crypto.ECDSAKeyPairGenerator{Curve: crypto.P256},
848+
userInfo: nil,
849+
hostnames: []string{"localhost"},
850+
wantErr: "requires UserInfo",
851+
},
852+
{
853+
name: "mismatched UserInfo name on legacy path",
854+
keyGen: nil,
855+
userInfo: &user.DefaultInfo{Name: "wrong-name"},
856+
hostnames: []string{"localhost", "127.0.0.1"},
857+
wantErr: "conflicts",
858+
},
859+
}
860+
for _, tc := range testCases {
861+
t.Run(tc.name, func(t *testing.T) {
862+
ca, err := newTestCACertificate(pkix.Name{CommonName: "test-ca"}, int64(1), metav1.Duration{Duration: time.Hour * 24}, time.Now)
863+
if err != nil {
864+
t.Fatal(err)
865+
}
866+
867+
r := &PeerRotation{
868+
Hostnames: func() []string { return tc.hostnames },
869+
UserInfo: tc.userInfo,
870+
}
871+
certConfig, err := r.NewCertificate(ca, time.Hour, tc.keyGen)
872+
if tc.wantErr != "" {
873+
if err == nil {
874+
t.Fatalf("expected error containing %q, got nil", tc.wantErr)
875+
}
876+
if !strings.Contains(err.Error(), tc.wantErr) {
877+
t.Fatalf("expected error containing %q, got %q", tc.wantErr, err.Error())
878+
}
879+
return
880+
}
881+
if err != nil {
882+
t.Fatalf("NewCertificate() error = %v", err)
883+
}
884+
885+
cert := certConfig.Certs[0]
886+
if cert.PublicKeyAlgorithm != tc.wantAlg {
887+
t.Errorf("PublicKeyAlgorithm = %v, want %v", cert.PublicKeyAlgorithm, tc.wantAlg)
888+
}
889+
890+
// Verify both ExtKeyUsages are present
891+
if !slices.Contains(cert.ExtKeyUsage, x509.ExtKeyUsageClientAuth) {
892+
t.Error("missing ExtKeyUsageClientAuth")
893+
}
894+
if !slices.Contains(cert.ExtKeyUsage, x509.ExtKeyUsageServerAuth) {
895+
t.Error("missing ExtKeyUsageServerAuth")
896+
}
897+
898+
// Verify hostnames in SANs
899+
if len(cert.DNSNames) == 0 {
900+
t.Error("expected DNS SANs")
901+
}
902+
903+
// Verify Subject based on path
904+
if tc.keyGen != nil {
905+
// ConfigurablePKI path: UserInfo encoded in Subject
906+
if cert.Subject.CommonName != tc.userInfo.GetName() {
907+
t.Errorf("CN = %q, want %q", cert.Subject.CommonName, tc.userInfo.GetName())
908+
}
909+
if len(tc.userInfo.GetGroups()) > 0 && len(cert.Subject.Organization) == 0 {
910+
t.Error("expected Organization from UserInfo.Groups")
911+
}
912+
} else {
913+
// Legacy path: CN = sorted first hostname (MakeServerCertForDuration sorts)
914+
if cert.Subject.CommonName != tc.userInfo.GetName() {
915+
t.Errorf("CN = %q, want %q (must match UserInfo.Name which must match sorted first hostname)", cert.Subject.CommonName, tc.userInfo.GetName())
916+
}
917+
}
918+
})
919+
}
920+
}
921+
800922
func TestSignerRotation_NewCertificate_WithKeyPairGenerator(t *testing.T) {
801923
testCases := []struct {
802924
name string
@@ -808,6 +930,16 @@ func TestSignerRotation_NewCertificate_WithKeyPairGenerator(t *testing.T) {
808930
keyGen: nil,
809931
wantAlg: x509.RSA,
810932
},
933+
{
934+
name: "RSA-4096",
935+
keyGen: crypto.RSAKeyPairGenerator{Bits: 4096},
936+
wantAlg: x509.RSA,
937+
},
938+
{
939+
name: "ECDSA-P256",
940+
keyGen: crypto.ECDSAKeyPairGenerator{Curve: crypto.P256},
941+
wantAlg: x509.ECDSA,
942+
},
811943
{
812944
name: "ECDSA-P384",
813945
keyGen: crypto.ECDSAKeyPairGenerator{Curve: crypto.P384},

0 commit comments

Comments
 (0)