feat(bundle): private Sigstore via --fulcio-url and --rekor-url#1158
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds --fulcio-url and --rekor-url support end-to-end: new defaults constants, NewKeylessAttester accepts endpoint overrides, ResolveOptions and lazy resolver propagate them, SignStatement/signAndPush use resolved URLs, config schema and Resolve validate HTTPS URLs, CLI flags parse and preflight-validate inputs, tests updated/added, and CLI docs extended. Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related issues
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pkg/config/resolve.go (1)
239-244: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick winValidate attestation URLs at the config-resolve boundary.
BundleSpec.Resolve()currently copiesspec.bundle.attestation.fulcioURL/rekorURLverbatim, so malformed config values only fail later in CLI parsing and lose thespec.bundle...attribution this resolver promises. Non-CLI callers ofResolve()also get no validation at all. Validate these fields here with spec-path labels before assigning them.♻️ Proposed fix
if b.Attestation != nil { out.Attest = b.Attestation.Enabled out.CertIDRegexp = b.Attestation.CertificateIdentityRegexp out.OIDCDeviceFlow = b.Attestation.OIDCDeviceFlow + if err := bundlercfg.ValidateHTTPSURL("spec.bundle.attestation.fulcioURL", b.Attestation.FulcioURL); err != nil { + return nil, err + } + if err := bundlercfg.ValidateHTTPSURL("spec.bundle.attestation.rekorURL", b.Attestation.RekorURL); err != nil { + return nil, err + } out.FulcioURL = b.Attestation.FulcioURL out.RekorURL = b.Attestation.RekorURL }Based on learnings: if an error is already created/wrapped with
pkg/errorsand includes the correct structured error code, return it as-is; do not double-wrap it.🤖 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 `@pkg/config/resolve.go` around lines 239 - 244, In BundleSpec.Resolve(), validate b.Attestation.FulcioURL and b.Attestation.RekorURL before assigning to out.FulcioURL/out.RekorURL: parse and ensure they are well-formed URLs and, on error, return a validation error that includes the spec-path label (e.g. "spec.bundle.attestation.fulcioURL"/"spec.bundle.attestation.rekorURL"); do this validation when b.Attestation != nil (where out.Attest and other fields are set) and avoid double-wrapping errors already created with pkg/errors — if the returned error already carries the proper structured code, return it as-is.
🤖 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 `@pkg/bundler/config/config.go`:
- Around line 117-129: ValidateHTTPSURL currently accepts URLs with embedded
credentials (u.User non-nil); update ValidateHTTPSURL to reject any URL where
u.User != nil by returning an ErrCodeInvalidRequest with a clear message (e.g.,
"credentials are not allowed in %s %q"), and add a unit test
TestValidateHTTPSURL that asserts a URL like "https://user:[email protected]/..."
produces an error. Ensure you reference and check u.User in the ValidateHTTPSURL
function and include the new test case to cover this rejection.
In `@pkg/evidence/attestation/emit.go`:
- Around line 256-260: The attestation signing currently overrides configured
Sigstore endpoints by passing defaults.SigstoreFulcioURL and
defaults.SigstoreRekorURL into bundleattest.SignStatement; update the call in
emit.go so SignOptions uses the resolved endpoints from the attestation options
(e.g., opts.OIDCResolve.FulcioURL and opts.OIDCResolve.RekorURL) instead of the
hardcoded defaults; ensure you fall back to the defaults only if
opts.OIDCResolve or its FulcioURL/RekorURL fields are empty, and keep
token/OIDCToken plumbing the same when calling bundleattest.SignStatement.
---
Outside diff comments:
In `@pkg/config/resolve.go`:
- Around line 239-244: In BundleSpec.Resolve(), validate b.Attestation.FulcioURL
and b.Attestation.RekorURL before assigning to out.FulcioURL/out.RekorURL: parse
and ensure they are well-formed URLs and, on error, return a validation error
that includes the spec-path label (e.g.
"spec.bundle.attestation.fulcioURL"/"spec.bundle.attestation.rekorURL"); do this
validation when b.Attestation != nil (where out.Attest and other fields are set)
and avoid double-wrapping errors already created with pkg/errors — if the
returned error already carries the proper structured code, return it as-is.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Enterprise
Run ID: 0f14f339-d30f-4cff-bcc6-5d7abbbe0c5e
📒 Files selected for processing (16)
docs/user/cli-reference.mdpkg/bundler/attestation/keyless.gopkg/bundler/attestation/keyless_test.gopkg/bundler/attestation/resolver.gopkg/bundler/attestation/resolver_test.gopkg/bundler/attestation/signing.gopkg/bundler/config/config.gopkg/bundler/config/config_test.gopkg/cli/bundle.gopkg/cli/bundle_test.gopkg/cli/consts.gopkg/config/config.gopkg/config/resolve.gopkg/config/resolve_test.gopkg/defaults/sigstore.gopkg/evidence/attestation/emit.go
Coverage Report ✅
Coverage BadgeMerging this branch changes the coverage (1 decrease, 3 increase)
Coverage by fileChanged files (no unit tests)
Please note that the "Total", "Covered", and "Missed" counts above refer to code statements instead of lines of code. The value in brackets refers to the test coverage of that file in the old version of the code. |
lockwobr
force-pushed
the
feat/bundle-fulcio-rekor-urls
branch
from
d64f082 to
d8c986f
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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 `@docs/user/cli-reference.md`:
- Around line 1841-1842: The doc text around the flags `--fulcio-url` and
`--rekor-url` is ambiguous about verification support; update the paragraph to
explicitly state that while signing can be redirected to private Fulcio/Rekor
with those flags, `aicr verify` does not yet support using custom root
certificates to verify privately-signed bundles (verifier support is out of
scope, tracked under issues `#1149` and `#1153`), so public Sigstore remains the
only supported verification root today; mention `aicr verify` by name and
reference the issue numbers for clarity.
In `@pkg/cli/bundle.go`:
- Around line 596-605: runBundleCmd currently calls
validateSingleValueFlags(...) but omits the new endpoint flags, so conflicting
--fulcio-url and --rekor-url values aren't rejected; update the
validateSingleValueFlags invocation inside runBundleCmd to include flagFulcioURL
and flagRekorURL alongside the other scalar bundle flags (reference
validateSingleValueFlags and the flag identifiers flagFulcioURL and
flagRekorURL) so duplicate/conflicting single-value inputs for these endpoints
are caught and fail-fast like the others.
In `@pkg/config/resolve.go`:
- Around line 140-148: The file has the GoDoc for Resolve placed above
validateAttestationEndpoints, so func (b *BundleSpec) Resolve() is left
undocumented and the helper gets the wrong comment; move the
validateAttestationEndpoints function below the BundleSpec.Resolve method (or
split the comment so each function has its own doc block) so that the comment
"Resolve converts a BundleSpec..." immediately precedes func (b *BundleSpec)
Resolve() and validateAttestationEndpoints remains documented appropriately;
update only the order or the comments around the symbols BundleSpec.Resolve and
validateAttestationEndpoints in pkg/config/resolve.go.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Enterprise
Run ID: 67f54eaf-7aaa-462d-b00b-7b6fa13d7ac7
📒 Files selected for processing (16)
docs/user/cli-reference.mdpkg/bundler/attestation/keyless.gopkg/bundler/attestation/keyless_test.gopkg/bundler/attestation/resolver.gopkg/bundler/attestation/resolver_test.gopkg/bundler/attestation/signing.gopkg/bundler/config/config.gopkg/bundler/config/config_test.gopkg/cli/bundle.gopkg/cli/bundle_test.gopkg/cli/consts.gopkg/config/config.gopkg/config/resolve.gopkg/config/resolve_test.gopkg/defaults/sigstore.gopkg/evidence/attestation/emit.go
lockwobr
force-pushed
the
feat/bundle-fulcio-rekor-urls
branch
from
d8c986f to
322c910
Compare
There was a problem hiding this comment.
CodeRabbit already owns the three open items (verifier-support wording, validateSingleValueFlags gap, GoDoc placement) plus the resolved ones, so I'll skip those.
Two additional points below. The env var gap is the substantive one; the usage-string mismatch is a nit.
lockwobr
force-pushed
the
feat/bundle-fulcio-rekor-urls
branch
from
322c910 to
a201fac
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pkg/bundler/attestation/signing.go (1)
99-118:⚠️ Potential issue | 🟠 Major | ⚡ Quick winValidate Sigstore endpoints before logging or dialing.
SignStatementnow accepts raw Fulcio/Rekor URLs from callers, but it logs them on Line 118 before any validation. A caller that passeshttps://user:pass@...will leak credentials into debug logs and only fail later in the Sigstore client. Reject non-HTTPS URLs and any URL withUserinfo here as well, not just in the CLI/config layers.🤖 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 `@pkg/bundler/attestation/signing.go` around lines 99 - 118, Validate fulcioURL and rekorURL right after resolving defaults (before slog.Debug and any network calls) in SignStatement: parse each URL (opts.FulcioURL/opts.RekorURL or their defaults) and reject anything that is not https (scheme != "https") or that contains user info (u.User != nil), returning a clear error; do not log the raw URLs until they pass validation (or log only sanitized host/port). Ensure the validation happens before the slog.Debug call that references fulcioURL and rekorURL.
🤖 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 `@docs/user/cli-reference.md`:
- Around line 1122-1126: Update the fulcioURL and rekorURL example and
surrounding text to explicitly state the same validation rules used by the
flags: URLs must be absolute https:// URLs and must not contain embedded
credentials (e.g., no user:pass@host). Modify the config snippet or its caption
to include the "no embedded credentials" constraint and, if present elsewhere
near the example, mirror the flag validation language so fulcioURL and rekorURL
reflect the exact accepted format.
In `@pkg/cli/bundle_test.go`:
- Around line 317-379: Add a subtest to TestParseBundleCmdOptions_SigstoreURLs
that verifies flag-over-env precedence: within the test, set environment
variables AICR_FULCIO_URL and AICR_REKOR_URL to known values, then call
captureBundleOpts (or tryCaptureBundleOpts if you want to assert no error)
passing explicit --fulcio-url and --rekor-url flags with different values, and
assert opts.fulcioURL and opts.rekorURL equal the flag values (not the env
values); use the existing helpers captureBundleOpts/tryCaptureBundleOpts and
keep assertions consistent with the other subtests.
---
Outside diff comments:
In `@pkg/bundler/attestation/signing.go`:
- Around line 99-118: Validate fulcioURL and rekorURL right after resolving
defaults (before slog.Debug and any network calls) in SignStatement: parse each
URL (opts.FulcioURL/opts.RekorURL or their defaults) and reject anything that is
not https (scheme != "https") or that contains user info (u.User != nil),
returning a clear error; do not log the raw URLs until they pass validation (or
log only sanitized host/port). Ensure the validation happens before the
slog.Debug call that references fulcioURL and rekorURL.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Enterprise
Run ID: e49dc28e-ac5f-44d2-8a32-f6e6b7438b86
📒 Files selected for processing (16)
docs/user/cli-reference.mdpkg/bundler/attestation/keyless.gopkg/bundler/attestation/keyless_test.gopkg/bundler/attestation/resolver.gopkg/bundler/attestation/resolver_test.gopkg/bundler/attestation/signing.gopkg/bundler/config/config.gopkg/bundler/config/config_test.gopkg/cli/bundle.gopkg/cli/bundle_test.gopkg/cli/consts.gopkg/config/config.gopkg/config/resolve.gopkg/config/resolve_test.gopkg/defaults/sigstore.gopkg/evidence/attestation/emit.go
Wire --fulcio-url / --rekor-url through `aicr bundle` so `--attest` keyless signing can target a private Sigstore instance. The library already plumbed SignOptions.FulcioURL/RekorURL; this connects the CLI and config layers to it. - attestation: ResolveOptions carries the Fulcio/Rekor URLs; NewKeylessAttester takes them with default-on-empty; both the eager and lazy resolver paths thread them through to SignStatement - config: spec.bundle.attestation.fulcioURL/rekorURL -> BundleResolved - cli: --fulcio-url / --rekor-url flags with fail-fast HTTPS validation - shared config.ValidateHTTPSURL validator (used by both flags) - move the Sigstore default endpoints into pkg/defaults as SigstoreFulcioURL / SigstoreRekorURL and update all call sites - docs: cli-reference flag table, config example, private-Sigstore note Public Sigstore remains the default when the flags are omitted; the two URLs are independent. Verification of privately-signed bundles is tracked separately under epic #1149. Fixes #408
lockwobr
force-pushed
the
feat/bundle-fulcio-rekor-urls
branch
from
a201fac to
be18ab1
Compare
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 `@docs/user/cli-reference.md`:
- Around line 1090-1091: Add AICR_FULCIO_URL and AICR_REKOR_URL to the
Environment Variables section of the CLI reference so the docs match the new
behavior: document that `--fulcio-url` reads AICR_FULCIO_URL and `--rekor-url`
reads AICR_REKOR_URL, state they must be absolute https:// URLs with no embedded
credentials, and note default behavior (public Fulcio/Rekor when omitted) and
independence of the two URLs; update cli-reference.md (and ensure the same
entries are added in the other docs listed by policy: api-reference.md,
component-catalog.md, recipe-development.md) in this PR.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Enterprise
Run ID: 9edeeac3-dcad-4f6b-ba2f-a4341035a90d
📒 Files selected for processing (16)
docs/user/cli-reference.mdpkg/bundler/attestation/keyless.gopkg/bundler/attestation/keyless_test.gopkg/bundler/attestation/resolver.gopkg/bundler/attestation/resolver_test.gopkg/bundler/attestation/signing.gopkg/bundler/config/config.gopkg/bundler/config/config_test.gopkg/cli/bundle.gopkg/cli/bundle_test.gopkg/cli/consts.gopkg/config/config.gopkg/config/resolve.gopkg/config/resolve_test.gopkg/defaults/sigstore.gopkg/evidence/attestation/emit.go
mchmarny
left a comment
mchmarny
left a comment
There was a problem hiding this comment.
Approving. Clean additive change: defaults preserved, flag + env + config + spec-path attribution all wired correctly, and both eager and lazy attester paths thread the URLs. The ValidateHTTPSURL credential-rejection check is well thought out (with a clear comment explaining why scheme+host alone isn't enough). Tests cover URL defaulting, flag-over-env precedence, HTTPS-only validation, and embedded-credential rejection. CI green.
Two low-severity notes inline plus a positive callout — nothing blocking. The doc caveat about aicr verify not yet supporting private trust roots is the right honest framing for the partial-coverage shape of this PR.
| // while leaving Scheme/Host intact, so a scheme+host-only check would | ||
| // otherwise accept "https://user:pass@host". Credentials have no place in | ||
| // a signing endpoint and would leak via config/flags/process listings. | ||
| if u.Scheme != "https" || u.Host == "" || u.User != nil { |
There was a problem hiding this comment.
Nice — the u.User != nil check (with the comment explaining that url.Parse stashes user:pass@ in u.User while leaving Scheme/Host valid) is exactly the right guard. A scheme+host check alone would have let https://user:pass@host through and leaked creds via config/flags/process listings. No action needed.
| FulcioURL: bundleattest.DefaultFulcioURL, | ||
| RekorURL: bundleattest.DefaultRekorURL, | ||
| FulcioURL: opts.OIDCResolve.FulcioURL, | ||
| RekorURL: opts.OIDCResolve.RekorURL, |
There was a problem hiding this comment.
The "validate-emit path does not yet expose --fulcio-url/--rekor-url, so these are empty today; this keeps the signer from overriding a configured endpoint once it does" framing is accurate but leaves the wire-through as an implicit TODO with no linked issue. Either reference the tracking issue (looks like #1149 / #1153 from the doc caveat) or open a follow-up and link it here so the work doesn't fall off — easy to miss in a year when someone wonders why recipe-evidence signing always hits public Sigstore.
| Category: catDeployment, | ||
| }, | ||
| &cli.StringFlag{ | ||
| Name: flagRekorURL, |
There was a problem hiding this comment.
nit: --fulcio-url / --rekor-url are accepted without --attest=true and silently no-op when signing is disabled. Matches the pattern of --certificate-identity-regexp / --identity-token, so it's internally consistent — but an operator who sets these expecting signing gets no feedback. Optional: slog.Warn in selectAttester when either URL is set but opts.attest == false. Not blocking.
2 participants
Summary
Adds
--fulcio-url/--rekor-urlflags (and matching config fields) toaicr bundleso--attestkeyless signing can target a private Sigstore instance instead of the public-good endpoints.Motivation / Context
Organizations running their own Fulcio CA and/or Rekor transparency log could not point AICR at them — all signing hit public Sigstore. The signing library already accepted custom endpoints (
SignOptions.FulcioURL/RekorURLwith default fallback); this PR connects the CLI and config layers so the capability is actually reachable.Fixes: #408
Related: #1149
Type of Change
Component(s) Affected
cmd/aicr,pkg/cli)pkg/bundler,pkg/component/*)pkg/errors,pkg/k8s) —pkg/defaultsdocs/,examples/)Implementation Notes
ResolveOptionscarriesFulcioURL/RekorURL;NewKeylessAttester(token, fulcio, rekor)applies the public-good default on empty, so existing callers are unaffected. Both the eager and lazy resolver paths thread the URLs toSignStatement.spec.bundle.attestation.fulcioURL/rekorURL→BundleResolved.https://URLs (fail fast, before bundling), via a sharedconfig.ValidateHTTPSURL(label, raw)helper that lives next to the siblingValidateAppName.pkg/bundler/attestationintopkg/defaultsasSigstoreFulcioURL/SigstoreRekorURL(matching the existingSigstoreSignTimeoutconvention) and updated all call sites, includingpkg/evidence/attestation/emit.go.Testing
make qualifypasses end-to-end: unit tests (-race), golangci-lint (0 issues), chainsaw e2e (22 passed / 0 failed), vulnerability scan (no vulnerabilities), license-header and repo checks. New table-driven tests coverNewKeylessAttesterURL defaulting, resolver URL propagation (eager + lazy), config resolution, CLI flag parsing + HTTPS-validation rejection, and the sharedValidateHTTPSURL.Risk Assessment
Rollout notes: Backwards compatible. No migration.
--fulcio-url/--rekor-urlare opt-in and only take effect with--attest.Checklist
make testwith-race)make lint)git commit -S)