certrotation: require UserInfo on PeerRotation, add tests#2157
certrotation: require UserInfo on PeerRotation, add tests#2157sanchezl wants to merge 5 commits intoopenshift:masterfrom
Conversation
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.
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.
Replace context.Background() and context.TODO() with t.Context() across all certrotation test files. Remove unused "context" imports.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughSwitched tests to use per-test contexts ( Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
/cc @rh-roman |
rh-roman
left a comment
rh-roman
left a comment
There was a problem hiding this comment.
Mostly cosmetic changes, the PR looks good!
| 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), | ||
| 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). | ||
| // 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()) | ||
| } |
There was a problem hiding this comment.
What do you think of flipping the keyGen check and putting legacy code inside. Then happy path will be the default return and removing the legacy path will be a smaller diff (eventually).
| 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 | ||
| } |
There was a problem hiding this comment.
This project imports testify, you could shorten the test to
| 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 tc.wantErr != "" { | |
| if assert.Error(t, err) { | |
| assert.ErrorContains(t, err, tc.wantErrText) | |
| } | |
| return | |
| } |
| // 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") | ||
| } |
There was a problem hiding this comment.
If you pull these out into a helper you can also add them to Client and Serving tests, which don't already have them.
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.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
pkg/operator/certrotation/target_test.go (1)
898-901: Strengthen SAN assertions to validate requested hostnames/IPs explicitly.
len(cert.DNSNames) > 0can pass even when some required SAN entries are missing (especially IP SANs). Assert eachtc.hostnamesentry exists in DNS SANs or IP SANs.Proposed test hardening
import ( "crypto/x509" "crypto/x509/pkix" + "net" "slices" "strings" "testing" "time" @@ - // Verify hostnames in SANs - if len(cert.DNSNames) == 0 { - t.Error("expected DNS SANs") - } + // Verify all requested hostnames/IPs are present in SANs + for _, h := range tc.hostnames { + if ip := net.ParseIP(h); ip != nil { + found := false + for _, certIP := range cert.IPAddresses { + if certIP.Equal(ip) { + found = true + break + } + } + if !found { + t.Errorf("missing IP SAN %q", h) + } + continue + } + if !slices.Contains(cert.DNSNames, h) { + t.Errorf("missing DNS SAN %q", h) + } + }As per coding guidelines, "Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/operator/certrotation/target_test.go` around lines 898 - 901, The test currently only asserts that cert.DNSNames is non-empty which can miss missing SAN entries; update the test to iterate over each tc.hostnames entry and assert it appears either in cert.DNSNames or in cert.IPAddresses (by comparing the string form of net.IP entries or parsing tc.hostnames with net.ParseIP), and fail with a clear t.Errorf when a specific hostname/IP from tc.hostnames is not found; reference cert.DNSNames, cert.IPAddresses and tc.hostnames in the check and use net.ParseIP to distinguish IP vs DNS entries.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@pkg/operator/certrotation/target_test.go`:
- Around line 898-901: The test currently only asserts that cert.DNSNames is
non-empty which can miss missing SAN entries; update the test to iterate over
each tc.hostnames entry and assert it appears either in cert.DNSNames or in
cert.IPAddresses (by comparing the string form of net.IP entries or parsing
tc.hostnames with net.ParseIP), and fail with a clear t.Errorf when a specific
hostname/IP from tc.hostnames is not found; reference cert.DNSNames,
cert.IPAddresses and tc.hostnames in the check and use net.ParseIP to
distinguish IP vs DNS entries.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 5813a957-0892-433a-9c50-14d563ed9a22
📒 Files selected for processing (1)
pkg/operator/certrotation/target_test.go
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 <rofeldma@redhat.com>
sanchezl
force-pushed
the
peerrotation-userinfo-followup
branch
from
9d7c515 to
27f907d
Compare
|
@sanchezl: all tests passed! Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: rh-roman, sanchezl The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
2 participants
Summary
Follow-up to #2145 addressing deferred review comments:
ServingRotation→PeerRotationwithout settingUserInfowould silently work on the legacy path but fail when ConfigurablePKI is enabled. On the legacy path, also validates thatUserInfo.Namematches the sorted first hostname (used as CN byMakeServerCertForDuration).t.Context()in certrotation tests — replacescontext.Background()/context.TODO()across all test files.Test plan
go test ./pkg/operator/certrotation/...passes