feat(sigstore)!: add issuer/clientID to SignConfig for enterprise OIDC#2512
Merged
morri-son merged 4 commits intoMay 15, 2026
Merged
Conversation
✅ Deploy Preview for ocm-website canceled.
|
Contributor
|
Warning Rate limit exceeded
You’ve run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughAdds optional OIDC ChangesSigstore OIDC Issuer and Client ID Support
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
bindings/go/sigstore/signing/handler/handler_test.go (1)
901-937: ⚡ Quick winAdd a
clientID-only case to complete identity-attribute coverage.The table currently covers none, issuer-only, and issuer+clientID, but not clientID-only. Adding it would guard the conditional emission path independently.
Proposed test case addition
{ name: "issuer only", cfg: func() *v1alpha1.SignConfig { c := &v1alpha1.SignConfig{Issuer: "https://dex.example.com"} c.SetType(runtime.NewVersionedType(v1alpha1.SignConfigType, v1alpha1.Version)) return c }(), wantLen: 3, wantIssuer: "https://dex.example.com", }, + { + name: "clientID only", + cfg: func() *v1alpha1.SignConfig { + c := &v1alpha1.SignConfig{ClientID: "corp-sigstore"} + c.SetType(runtime.NewVersionedType(v1alpha1.SignConfigType, v1alpha1.Version)) + return c + }(), + wantLen: 3, + wantClientID: "corp-sigstore", + }, }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@bindings/go/sigstore/signing/handler/handler_test.go` around lines 901 - 937, Add a test case to the tests table in handler_test.go that exercises a clientID-only SignConfig to cover the missing identity-attribute path: create a cfg using &v1alpha1.SignConfig{ClientID: "corp-sigstore-only"} (call c.SetType(runtime.NewVersionedType(v1alpha1.SignConfigType, v1alpha1.Version))) and add expectations wantLen: 3 and wantClientID: "corp-sigstore-only" so the suite now covers none, issuer-only, clientID-only, and issuer+clientID scenarios.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@bindings/go/sigstore/integration/Taskfile.yml`:
- Around line 121-128: The sed call in the scaffolding:refresh Taskfile task
that edits {{.SIGSTORE_ENV_FILE}} uses GNU-style `sed -i` which breaks on
macOS/BSD; update the task to detect Darwin (using `uname`) and run `sed -i ''
"s|^export SIGSTORE_OIDC_TOKEN=.*|export SIGSTORE_OIDC_TOKEN='${TOKEN}'|"
{{.SIGSTORE_ENV_FILE}}` on macOS and `sed -i "s|^export
SIGSTORE_OIDC_TOKEN=.*|export SIGSTORE_OIDC_TOKEN='${TOKEN}'|"
{{.SIGSTORE_ENV_FILE}}` on other platforms (or set a SED_INPLACE var to `-i ''`
for Darwin and `-i` otherwise) so the edit works portably.
---
Nitpick comments:
In `@bindings/go/sigstore/signing/handler/handler_test.go`:
- Around line 901-937: Add a test case to the tests table in handler_test.go
that exercises a clientID-only SignConfig to cover the missing
identity-attribute path: create a cfg using &v1alpha1.SignConfig{ClientID:
"corp-sigstore-only"} (call
c.SetType(runtime.NewVersionedType(v1alpha1.SignConfigType, v1alpha1.Version)))
and add expectations wantLen: 3 and wantClientID: "corp-sigstore-only" so the
suite now covers none, issuer-only, clientID-only, and issuer+clientID
scenarios.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: e1c84e86-91aa-40ca-b8c0-b773320b4f06
📒 Files selected for processing (8)
bindings/go/sigstore/doc.gobindings/go/sigstore/integration/Taskfile.ymlbindings/go/sigstore/signing/handler/handler.gobindings/go/sigstore/signing/handler/handler_test.gobindings/go/sigstore/signing/handler/internal/credentials/credentials.gobindings/go/sigstore/signing/v1alpha1/config.gobindings/go/sigstore/signing/v1alpha1/config_test.gobindings/go/sigstore/signing/v1alpha1/schemas/SignConfig.schema.json
1885afb to
f3cfa5e
Compare
Add Issuer and ClientID fields to SigstoreSigningConfiguration so that enterprise Sigstore stacks can be configured via the signer spec. The handler emits these into the credential consumer identity, enabling .ocmconfig entries to distinguish between different Sigstore deployments. - Remove algorithm attribute from consumer identity (dispatch by config type makes it redundant; existing algorithm field in SignatureInfo is preserved for forward-compatible verification) - Fix duplicate line in doc.go and rewrite consumer identity docs - Convert handler tests to table-driven style - Regenerate SignConfig JSON schema Signed-off-by: Gerald Morrison (SAP) <gerald.morrison@sap.com>
f3cfa5e to
1775642
Compare
Skarlso
reviewed
May 14, 2026
Skarlso
reviewed
May 14, 2026
Skarlso
reviewed
May 14, 2026
Skarlso
reviewed
May 14, 2026
Skarlso
left a comment
Contributor
There was a problem hiding this comment.
Two questions really. Other than that, this is fine.
On-behalf-of: Gergely Brautigam <gergely.brautigam@sap.com> Signed-off-by: Gergely Brautigam <182850+Skarlso@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@bindings/go/sigstore/signing/v1alpha1/config.go`:
- Around line 129-133: The current validateURL check allows schemeless or
non-HTTPS issuers and URLs with query/fragment; update validateURL (used by the
Issuer validation and VerifyConfig.CertificateOIDCIssuer) to enforce OIDC rules:
require url.IsAbs() true, url.Scheme == "https", non-empty url.Host, and ensure
url.RawQuery and url.Fragment are empty; return a descriptive error if any of
these checks fail so both the Issuer and CertificateOIDCIssuer validations
reject invalid OIDC issuer formats.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 7898d1fa-fa11-456e-a77d-99bad6ac2f5d
📒 Files selected for processing (8)
bindings/go/sigstore/doc.gobindings/go/sigstore/integration/Taskfile.ymlbindings/go/sigstore/signing/handler/handler.gobindings/go/sigstore/signing/handler/handler_test.gobindings/go/sigstore/signing/handler/internal/credentials/credentials.gobindings/go/sigstore/signing/v1alpha1/config.gobindings/go/sigstore/signing/v1alpha1/config_test.gobindings/go/sigstore/signing/v1alpha1/schemas/SignConfig.schema.json
✅ Files skipped from review due to trivial changes (1)
- bindings/go/sigstore/signing/handler/internal/credentials/credentials.go
🚧 Files skipped from review as they are similar to previous changes (5)
- bindings/go/sigstore/signing/v1alpha1/schemas/SignConfig.schema.json
- bindings/go/sigstore/signing/v1alpha1/config_test.go
- bindings/go/sigstore/signing/handler/handler.go
- bindings/go/sigstore/signing/handler/handler_test.go
- bindings/go/sigstore/integration/Taskfile.yml
On-behalf-of: Gergely Brautigam <gergely.brautigam@sap.com> Signed-off-by: Gergely Brautigam <182850+Skarlso@users.noreply.github.com>
Skarlso
reviewed
May 15, 2026
Skarlso
approved these changes
May 15, 2026
8 tasks
jakobmoellerdev
approved these changes
May 15, 2026
piotrjanik
approved these changes
May 15, 2026
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
5 participants
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.
Summary
As a prerequiste for #2346 we require this PR to get merged. It contains the move of required fields from the credential into the consumer identity: issuer and clientID:
IssuerandClientIDfields toSigstoreSigningConfiguration/v1alpha1so enterprise Sigstore stacks can be configured via the signer spec.ocmconfigentries to distinguish between different Sigstore deploymentsalgorithmattribute from consumer identity (dispatch by config type makes it redundant; the algorithm field inSignatureInfois preserved for forward-compatible verification)doc.goand rewrite consumer identity documentationSignConfig.schema.jsonContext
Split from #2346 to comply with the cross-module rule — this PR contains only
bindings/go/sigstore/changes. The CLI changes in #2346 will be rebased onto this after merge and module release.Test plan
go build ./...inbindings/go/sigstore/passesgo test ./signing/...all greencli/go.modin feat(cli): integrate cosign signing handler [2/2] #2346, verify e2e