OCPBUGS-44842: Set not-before/not-after annotations #776
Conversation
openshift-ci-robot
added
jira/valid-reference
|
@vrutkovs: This pull request references Jira Issue OCPBUGS-44842, which is invalid:
Comment The bug has been updated to refer to the pull request using the external bug tracker. In response to this:
Instructions 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 openshift-eng/jira-lifecycle-plugin repository. |
vrutkovs
changed the title
openshift-ci-robot
added
jira/valid-bug
|
@vrutkovs: This pull request references Jira Issue OCPBUGS-57049, which is valid. 3 validation(s) were run on this bug
Requesting review from QA contact: The bug has been updated to refer to the pull request using the external bug tracker. In response to this:
Instructions 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 openshift-eng/jira-lifecycle-plugin repository. |
vrutkovs
changed the title
openshift-ci-robot
added
jira/invalid-bug
|
@vrutkovs: This pull request references Jira Issue OCPBUGS-44842, which is invalid:
Comment In response to this:
Instructions 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 openshift-eng/jira-lifecycle-plugin repository. |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: vrutkovs The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
/jira refresh |
openshift-ci-robot
added
jira/valid-bug
|
@vrutkovs: This pull request references Jira Issue OCPBUGS-44842, which is valid. 3 validation(s) were run on this bug
Requesting review from QA contact: In response to this:
Instructions 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 openshift-eng/jira-lifecycle-plugin repository. |
openshift-merge-robot
added
the
needs-rebase
vrutkovs
force-pushed
the
cert-annotations-not-before-not-after-v2
branch
from
8840643 to
e41a487
Compare
openshift-merge-robot
removed
the
needs-rebase
openshift-merge-robot
added
the
needs-rebase
vrutkovs
force-pushed
the
cert-annotations-not-before-not-after-v2
branch
from
e41a487 to
651b092
Compare
openshift-merge-robot
removed
the
needs-rebase
|
/retest-required |
|
@CodeRabbit review |
✅ Actions performedReview triggered.
|
WalkthroughAdds runtime TLS certificate/key validation and extracts certificate NotBefore/NotAfter to store as RFC3339 rotation annotations on the webhook kubeconfig Secret; existing component annotation is preserved. No public API or signature changes. (50 words) Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant C as Controller
participant TLS as tls.X509KeyPair
participant X509 as x509.ParseCertificate
participant S as K8s Secret Store
C->>TLS: Build keypair from cert/key
alt keypair valid
TLS-->>C: keypair (DER chain)
C->>X509: Parse leaf certificate (pair.Certificate[0])
alt parse success
X509-->>C: NotBefore / NotAfter
C->>S: Create/Update Secret with annotations (component, notBefore, notAfter)
S-->>C: ACK
else parse failure
X509-->>C: Error
C-->>C: Return error
end
else invalid keypair
TLS-->>C: Error
C-->>C: Return error
end
rect rgba(220,240,255,0.5)
note right of C: New logic: runtime keypair validation and\ncertificate validity annotations on Secret
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. 📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 💡 Knowledge Base configuration:
You can enable these sources in your CodeRabbit configuration. 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
✨ Finishing Touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
pkg/controllers/webhookauthenticator/webhookauthenticator_controller.go (3)
203-217: Prefer deriving the leaf cert directly from tls.X509KeyPair and wrap underlying errors.
- tls.X509KeyPair already validates the key↔cert match. You can then parse the leaf from the returned pair instead of PEM-decoding manually. This avoids brittle assumptions (e.g., rejecting PEM headers) and ensures you extract NotBefore/NotAfter from the same cert TLS will present.
- Wrap the original errors with %w for easier debugging and telemetry.
Suggested refactor:
- _, err = tls.X509KeyPair(cert, key) - if err != nil { - return nil, fmt.Errorf("private key doesn't match the certificate of authenticator secret") - } - // extract not-before/not-after timestamps valid x509 certificate - var block *pem.Block - block, _ = pem.Decode(cert) - if block == nil || block.Type != "CERTIFICATE" || len(block.Headers) != 0 { - return nil, fmt.Errorf("invalid first block found in the certificate of authenticator secret") - } - parsedCert, err := x509.ParseCertificate(block.Bytes) - if err != nil { - return nil, fmt.Errorf("failed to parse the certificate of authenticator secret") - } + pair, err := tls.X509KeyPair(cert, key) + if err != nil { + return nil, fmt.Errorf("invalid TLS keypair in authenticator secret (openshift-oauth-apiserver/openshift-authenticator-certs): %w", err) + } + // Use the same certificate bytes that TLS will treat as the leaf. + if len(pair.Certificate) == 0 { + return nil, fmt.Errorf("no certificate data found in authenticator secret (openshift-oauth-apiserver/openshift-authenticator-certs)") + } + parsedCert, err := x509.ParseCertificate(pair.Certificate[0]) + if err != nil { + return nil, fmt.Errorf("failed to parse leaf certificate from authenticator secret (openshift-oauth-apiserver/openshift-authenticator-certs): %w", err) + }If you adopt this, you can drop the now-unused "encoding/pem" import.
203-226: Add focused tests for key/cert validation and annotation values.
- Positive: matching key/cert → Secret created with CertificateNotBefore/After populated and RFC3339 UTC values.
- Negative: mismatched key/cert → sync degraded with the new error.
- Negative: malformed cert content → degraded, no Secret apply.
I can draft unit tests using a fake client and an in-memory Secret with a generated client cert/key if helpful.
223-226: Normalize annotation timestamps to UTC explicitly- certrotation.CertificateNotBeforeAnnotation: parsedCert.NotBefore.Format(time.RFC3339), - certrotation.CertificateNotAfterAnnotation: parsedCert.NotAfter.Format(time.RFC3339), + certrotation.CertificateNotBeforeAnnotation: parsedCert.NotBefore.UTC().Format(time.RFC3339), + certrotation.CertificateNotAfterAnnotation: parsedCert.NotAfter.UTC().Format(time.RFC3339),I verified that only this controller writes the
webhook-authentication-integrated-oauthSecret (tests reference it but don’t mutate it). TheApplySecrethelper (from the addon-framework) merges ObjectMeta—including annotations—when updating, so unrelated annotations are preserved (pkg.go.dev). Please apply the UTC normalization to avoid any timezone ambiguity in your RFC3339 annotation values.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
pkg/controllers/webhookauthenticator/webhookauthenticator_controller.go(3 hunks)
🔇 Additional comments (1)
pkg/controllers/webhookauthenticator/webhookauthenticator_controller.go (1)
5-9: Imports look correct for keypair validation and cert annotations.The added imports (tls, x509, pem, certrotation) are appropriate for the new behavior.
Also applies to: 34-34
webhook-authentication-integrated-oauth secret
vrutkovs
force-pushed
the
cert-annotations-not-before-not-after-v2
branch
from
651b092 to
d2ddcae
Compare
|
/test e2e-agnostic-upgrade |
|
@vrutkovs: The following tests failed, say
Full PR test history. Your PR dashboard. Instructions 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. |
5 participants
Pull in openshift/library-go#1971 to make sure most secrets created by controller would have refresh-period annotation set.
Summary by CodeRabbit
New Features
Bug Fixes