Skip to content

POC: Harden OLM serving Secret validation - #3928

Open
sebrandon1 wants to merge 1 commit into
operator-framework:masterfrom
sebrandon1:harden-olm-serving-secret-validation
Open

sebrandon1 wants to merge 1 commit into
operator-framework:masterfrom
sebrandon1:harden-olm-serving-secret-validation

Conversation

@sebrandon1

@sebrandon1 sebrandon1 commented Sep 22, 2026

Copy link
Copy Markdown
Contributor

Summary

This PR is a proof of concept for hardening validation of OLM-managed serving Secrets.

  • Demonstrates storing SHA-256 fingerprints for the CA, serving certificate, and private key.
  • Demonstrates detecting malformed, expired, mismatched, modified, or legacy serving Secret data.
  • Regenerates serving credentials when validation fails and updates dependent deployment/webhook configuration.
  • Limited to the OLM serving Secret installer path; no CRD, status, or public API changes.

POC Behavior

The serving Secret is treated as OLM-controlled after creation. OLM reuses it only when:

  • CA, certificate, and private key are present and parseable.
  • The private key matches the certificate.
  • The certificate is valid for the expected hostname and signed by the expected CA.
  • All three stored fingerprints match the Secret data.

Otherwise, OLM generates a new serving Secret. Legacy Secrets without fingerprints rotate once to establish them.

Unit Test Coverage

Test case Expected result
Valid material and fingerprints Reuse Secret
Missing CA, certificate, or private key Rotate
Malformed CA, certificate, or private key Rotate
Expired certificate Rotate
Not-yet-valid certificate Rotate
Wrong hostname or CA Rotate
Mismatched private key and certificate Rotate
Changed CA fingerprint Rotate
Changed certificate fingerprint Rotate
Changed private-key fingerprint Rotate
Legacy Secret without fingerprints Rotate once
Valid installer-level Secret No UpdateSecret call
Rotated installer-level Secret Regenerated data and fingerprints

E2E Validation

  • Replaces a serving Secret with valid material and stale fingerprints.
  • Verifies OLM regenerates the Secret and updates the deployment CA hash and webhook CA bundle.
  • Removes fingerprints from a legacy serving Secret.
  • Verifies OLM regenerates all certificate material, restores fingerprints, and preserves olm.managed=true.
  • Verifies a valid rotated Secret remains stable without repeated reconciliation.

Test Plan

  • GOTOOLCHAIN=go1.26.3 make unit
  • make vendor && make diff
  • GOTOOLCHAIN=go1.26.3 make lint vet
  • GOTOOLCHAIN=go1.26.3 make verify
  • make test-split
  • make e2e-build
  • Focused serving-Secret e2e tests pass
  • Full CI e2e suite passes

Local full e2e execution was limited by an amd64 bundle test image crashing under ARM64 emulation on macOS; the relevant serving-Secret scenarios passed locally.

Summary by CodeRabbit

  • Bug Fixes

    • Improved serving certificate rotation by detecting missing, invalid, expired, or mismatched certificates and private keys.
    • Certificate rotation now responds to changes in CA, certificate, or private-key fingerprints.
    • Existing managed secrets without fingerprint metadata are refreshed and updated with the required fingerprints.
    • Certificate and private-key data are now consistently recognized during secret and deployment updates.
  • Tests

    • Added coverage for certificate rotation scenarios, legacy secrets, fingerprint changes, and stable post-rotation behavior.

@openshift-ci

openshift-ci Bot commented Sep 22, 2026

Copy link
Copy Markdown

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please assign joelanford for approval. For more information see the Code Review Process.

The full list of commands accepted by this bot can be found here.

Details Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-ci openshift-ci Bot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Sep 22, 2026
@openshift-ci

openshift-ci Bot commented Sep 22, 2026

Copy link
Copy Markdown

Hi @sebrandon1. Thanks for your PR.

I'm waiting for a operator-framework member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work.

Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Details

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.

@coderabbitai

coderabbitai Bot commented Sep 22, 2026

Copy link
Copy Markdown

Review in Change Stack →

Navigate logical layers of code changes, visualize relationships, and explore their blast radius.

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Advanced

Run ID: defaf593-5fba-4abc-89c8-b902c2fab70b

📥 Commits

Reviewing files that changed from the base of the PR and between 580b74d and f60016d.

📒 Files selected for processing (3)
  • pkg/controller/install/certresources.go
  • pkg/controller/install/certresources_test.go
  • test/e2e/webhook_e2e_test.go

Included review availability: Your plan provides up to 4 included reviews per hour; 3 remain after this review.


📝 Walkthrough

Walkthrough

The controller now validates serving certificate and private-key pairs, stores fingerprints for all TLS material, and rotates invalid or legacy Secrets. Unit and end-to-end tests cover fingerprint changes, Secret reuse, deployment updates, and webhook CA bundle updates.

Changes

Serving certificate rotation

Layer / File(s) Summary
Rotation validation and Secret generation
pkg/controller/install/certresources.go
The controller validates EC private keys against serving certificates, compares CA, certificate, and key fingerprints, stores all three fingerprints, and uses Kubernetes TLS key constants.
Controller rotation and reuse tests
pkg/controller/install/certresources_test.go
Unit tests cover invalid or mismatched material, changed or missing fingerprints, legacy Secret handling, Secret reuse, and TLS volume mappings.
Webhook Secret rotation validation
test/e2e/webhook_e2e_test.go
End-to-end tests verify rotation for stale and missing fingerprints, deployment CA hash updates, webhook CA bundle updates, and post-rotation stability.

Priority: ⬇️ Low

Estimated code review effort: 3 (Moderate) | ~30 minutes

Change: Feature

Sequence Diagram(s)

sequenceDiagram
  participant WebhookTest
  participant ServingSecret
  participant OLM
  participant Deployment
  participant WebhookConfiguration
  WebhookTest->>ServingSecret: Replace material or remove fingerprints
  OLM->>ServingSecret: Detect invalid or legacy fingerprints
  OLM->>ServingSecret: Generate and store new certificate material
  OLM->>Deployment: Update CA hash annotation
  OLM->>WebhookConfiguration: Update CA bundle
  WebhookTest->>ServingSecret: Verify stable fingerprints
Loading

Merge Risk: ⚪ Minimal · up to f6001

The change hardens serving Secret validation and rotates invalid or legacy TLS material while updating dependent resources. No concrete merge-blocking risk remains identified.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 12.50% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 16 functions across 3 files. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly summarizes the main change: hardening validation for OLM-managed serving Secrets in a proof-of-concept implementation.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
  • Fix all pre-merge checks with AI
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create a new PR

Warning

Some tools did not complete. Review the errors below.

🔧 golangci-lint (2.13.2)

Error: can't load config: can't unmarshal config by viper (flags, file): 1 error(s) decoding:

  • 'output.formats' expected a map, got 'string'
    The command is terminated due to an error: can't load config: can't unmarshal config by viper (flags, file): 1 error(s) decoding:

  • 'output.formats' expected a map, got 'string'


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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@tmshort

tmshort commented Sep 23, 2026

Copy link
Copy Markdown
Member

/ok-to-test
Even though all CI ran...

@openshift-ci openshift-ci Bot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Sep 23, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ok-to-test Indicates a non-member PR verified by an org member that is safe to test.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants