Strip live credentials from presubmit jobs - #1225
Merged
Merged
Conversation
Presubmits run unreviewed pull-request code, so they must never mount live credentials. The e2e generators attach the preset-venafi-* presets to jobs that are used for both presubmits and periodics, so today those presets reach presubmits as well. Remove the credential presets at addPresubmit(), the single point through which every presubmit passes, rather than at each generator. A future generator that re-adds a credential label to a presubmit cannot defeat this, and periodics (which run merged, reviewed code) keep their credentials unchanged. Regenerate the cert-manager job configs: the credential presets are dropped from the presubmit e2e jobs on master, release-1.20 and release-1.21 while the corresponding periodics retain them. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: Richard Wall <richard@the-moon.net>
Enable censor_secrets in the default decoration config so the sidecar redacts known secret values before logs and artifacts are uploaded to the public cert-manager-prow-artifacts bucket. Defence in depth alongside removing credential presets from presubmits: it limits accidental leakage into public logs, though it cannot stop a job from exfiltrating a secret over the network. Validated with checkconfig --strict. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: Richard Wall <richard@the-moon.net>
The pull-*-issuers-venafi-{tpp,cloud,ngts} jobs were optional, /test-only
presubmits that never ran by default. Now that credential presets are stripped
from presubmits they could no longer authenticate against the live Venafi
services, so on the rare occasion a maintainer triggered one it would only
fail.
Regular Venafi coverage is unaffected: it comes from the E2ETestVenafiBoth
periodic (ci-*-issuers-venafi), which focuses on all three Venafi issuers and
keeps its credentials.
Remove the three presubmit registrations along with the generators and label
configurers that existed solely to build them, and drop the now-orphaned
per-issuer GINKGO_FOCUS presets. Validated with checkconfig --strict.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Signed-off-by: Richard Wall <richard@the-moon.net>
cert-manager-prow
Bot
added
dco-signoff: yes
There was a problem hiding this comment.
Pull request overview
Hardens Prow presubmits against live Venafi credential exposure while retaining trusted periodic coverage.
Changes:
- Strips Venafi credential labels from all generated presubmits.
- Removes unusable per-issuer Venafi presubmits and presets.
- Enables secret censoring for logs and artifacts.
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
config/prowgen/prowspecs/specs.go |
Removes per-issuer presubmit registration. |
config/prowgen/pkg/generators.go |
Removes obsolete Venafi generators. |
config/prowgen/pkg/context.go |
Enforces credential-label stripping. |
config/prowgen/pkg/context_test.go |
Tests presubmit stripping and periodic retention. |
config/prowgen/pkg/configurers.go |
Removes obsolete label configurers. |
config/jobs/cert-manager/config.yaml |
Removes orphaned focus presets. |
config/jobs/cert-manager/cert-manager/release-1.21/cert-manager-release-1.21.yaml |
Regenerates release 1.21 jobs. |
config/jobs/cert-manager/cert-manager/release-1.20/cert-manager-release-1.20.yaml |
Regenerates release 1.20 jobs. |
config/jobs/cert-manager/cert-manager/master/cert-manager-master.yaml |
Regenerates master jobs. |
config/config.yaml |
Enables global secret censoring. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Prow's censor_secrets only learns the values to redact from Secrets mounted into the Pod; env vars are never scanned. The venafi-* presets inject credentials exclusively via secretKeyRef env vars, so nothing was actually being censored. Mount each Secret as a volume, projecting only the sensitive keys so that non-secret values (URLs, zones) remain readable in the logs. Evidence permalinks, pinned to the utility-image version in use, are in the preset comment. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: Richard Wall <richard@the-moon.net>
The strip in addPresubmit only covers prowgen-generated jobs, and its presubmitForbiddenLabels list is hardcoded, so a hand-written presubmit or a future credential preset could reintroduce live credentials on unreviewed PR code without any check failing. This test derives the forbidden label set from the preset definitions themselves (any preset injecting a Secret via env secretKeyRef or a Secret volume) and walks every job file in the repository, covering hand-written and generated presubmits alike. It runs in the existing pull-testing-test presubmit via make test. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: Richard Wall <richard@the-moon.net>
This was referenced Aug 12, 2026
Silently stripping the forbidden labels was fail-silent: a future generator wiring a Venafi job into a presubmit would emit a job with a Venafi GINKGO_FOCUS but no credentials, which would either perma-fail against the live services or pass vacuously, with verify-prowgen and the unit tests all green. Fail generation loudly instead, so the developer must remove the preset or make the job a periodic. Also correct the comment: prowgen has no postsubmit support, so only periodics may carry credentials. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: Richard Wall <richard@the-moon.net>
The specs.go comment narrated the removal and was already stale (it described stripping, which is now a panic); git log records the why. Point the E2ETestVenafiBoth comment at the issue tracking credential-free pre-merge coverage instead. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: Richard Wall <richard@the-moon.net>
Fixes the pull-testing-verify failure. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: Richard Wall <richard@the-moon.net>
SgtCoDFish
approved these changes
Aug 12, 2026
SgtCoDFish
left a comment
SgtCoDFish
left a comment
Member
There was a problem hiding this comment.
/lgtm
/approve
I can't see any reason to not merge!
Member
|
/approve |
Contributor
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: inteon, SgtCoDFish The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
cert-manager-prow
Bot
added
the
approved
Contributor
|
@wallrj: Updated the following 2 configmaps:
DetailsIn 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 kubernetes-sigs/prow repository. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Presubmits run unreviewed pull-request code, so they should never mount live credentials. This hardens the Prow config so that only periodics and postsubmits (which run merged, reviewed code) carry credential presets.
Changes
addPresubmit()inconfig/prowgen/pkg/context.gonow deletes thepreset-venafi-*presets from every presubmit it registers. That is the single point through which every presubmit passes, so a future generator that re-adds a credential label to a presubmit cannot reintroduce the problem. New unit tests incontext_test.goassert presubmits are stripped while periodics retain their credentials.pull-*-issuers-venafi-{tpp,cloud,ngts}jobs were optional,/test-only presubmits that never ran by default; with the presets stripped they could no longer authenticate, so triggering one would only fail. Their generators, label configurers and the now-orphaned per-issuerGINKGO_FOCUSpresets are removed too.censor_secrets: truein the default decoration config, so the sidecar redacts known secret values before logs and artifacts are uploaded to the publiccert-manager-prow-artifactsbucket. Defence in depth alongside removing the presets.Coverage
Regular Venafi coverage is unaffected: it comes from the
ci-*-issuers-venafiperiodic, which keeps its credentials. Periodics and postsubmits retain all credential presets.Validation
go test ./config/prowgen/...make prowgenproduces no driftmake local-checkconfig(checkconfig --strict) passesReview fixes
censor_secretswas not actually covering the env-injected Venafi credentials. Each preset now also mounts its Secret (only the sensitive keys) under/etc/censor/; evidence permalinks pinned to the utility-image version in use are in the preset comment.config/prowgen/presubmit_policy_test.goforbids secret-bearing presets on any presubmit, hand-written or generated. The forbidden set is derived from the preset definitions (any preset injecting a Secret via envsecretKeyRefor a Secret volume), so new credential presets are covered automatically. Runs in the existingpull-testing-testpresubmit viamake test.Follow-ups
ci-*-issuers-venafiperiodics still share nodes with privileged (dind) presubmit pods, so a node escape could read their secrets via the kubelet. Once that pool exists, a follow-up here will pin the Venafi periodics to it with anodeSelectorand toleration.with claude opus-4.8 and claude fable-5