feat(verify): offline / air-gapped verification (--insecure-ignore-tlog) - #1798
Conversation
lockwobr
added
the
theme/supply-chain
|
🌿 Preview your docs: https://nvidia-preview-feat-1154-offline-verify.docs.buildwithfern.com/aicr |
Recipe evidence checkNo leaf overlays affected by this PR. This gate is warning-only and never blocks merge. |
|
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 key-based offline bundle verification that skips transparency-log and observer-timestamp requirements. The verifier rejects Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related issues
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://cdn.statically.io/img/avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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`:
- Line 2637: Correct the `--insecure-ignore-tlog` documentation and its offline
example so the claimed no-network verification uses a local exported PEM file,
not `awskms://alias/my-key`. Update the example and surrounding text to
reference the PEM path recommended by the existing note, and clarify that KMS
URI keys still require network access to resolve the public key.
🪄 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: 69d2d944-9e54-40d9-b0d9-6bb77583da93
📒 Files selected for processing (7)
docs/user/cli-reference.mdpkg/bundler/attestation/verifying_test.gopkg/bundler/attestation/verifytransparency.gopkg/bundler/verifier/verifier.gopkg/bundler/verifier/verifier_test.gopkg/cli/bundle_verify.gopkg/cli/bundle_verify_test.go
Coverage Report ✅
Coverage BadgeMerging this branch changes the coverage (1 decrease, 2 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. |
49ef3ba to
7e57f3d
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 `@pkg/bundler/verifier/verifier.go`:
- Around line 286-293: Move the IgnoreTLog and Key validation from its current
location to immediately after the context check in the verifier entry point,
before os.Stat(bundleDir) or any bundle-path inspection. Normalize the options
first, then return InvalidRequest for IgnoreTLog with an empty Key so this
validation consistently takes precedence over NotFound errors.
🪄 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: cba3f1a1-e737-408c-bc27-10973ee2fa08
📒 Files selected for processing (7)
docs/user/cli-reference.mdpkg/bundler/attestation/verifying_test.gopkg/bundler/attestation/verifytransparency.gopkg/bundler/verifier/verifier.gopkg/bundler/verifier/verifier_test.gopkg/cli/bundle_verify.gopkg/cli/bundle_verify_test.go
7e57f3d to
5e9f409
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 `@pkg/bundler/verifier/verifier.go`:
- Around line 158-166: The IgnoreTLog contract must not promise fully offline
verification when KMS key resolution remains remote. In
pkg/bundler/verifier/verifier.go lines 158-166 and 726-730, clarify that it
suppresses transparency-log network calls only and that fully offline
verification requires a local PEM key; in pkg/cli/bundle_verify.go lines 68-70,
replace the KMS URI example with ./bundle-signer.pub, and in lines 105-108 state
the local-PEM requirement.
🪄 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: acc4d30f-4b45-42e1-b120-4dbdc4f4c6ec
📒 Files selected for processing (7)
docs/user/cli-reference.mdpkg/bundler/attestation/verifying_test.gopkg/bundler/attestation/verifytransparency.gopkg/bundler/verifier/verifier.gopkg/bundler/verifier/verifier_test.gopkg/cli/bundle_verify.gopkg/cli/bundle_verify_test.go
5e9f409 to
9efe67c
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/attestation/verifytransparency.go`:
- Around line 42-59: Update the noTLogVerifyPolicy comments and
NewNoTLogVerifyPolicy documentation to promise only no transparency-log network
calls, not zero network calls overall. Explain that fully offline verification
additionally requires offline verification identity and trusted-material
sources, while preserving the policy’s requirement for no transparency-log proof
or observer timestamp.
In `@pkg/cli/bundle_verify_test.go`:
- Around line 297-343: The “ignore-tlog with key” case does not verify
successful offline verification or that the no-tlog policy is selected. Update
TestBundleVerifyCmd_IgnoreTLogRequiresKey to use a tlog-less signed fixture and
its local PEM, then assert verification succeeds with --insecure-ignore-tlog and
fails without that flag while retaining the existing missing-key validation.
🪄 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: 0a0d2762-2824-4238-b17e-2b0cd5c012ed
📒 Files selected for processing (7)
docs/user/cli-reference.mdpkg/bundler/attestation/verifying_test.gopkg/bundler/attestation/verifytransparency.gopkg/bundler/verifier/verifier.gopkg/bundler/verifier/verifier_test.gopkg/cli/bundle_verify.gopkg/cli/bundle_verify_test.go
Signed-off-by: Brian Lockwood <lockwobr@gmail.com>
9efe67c to
d3bf424
Compare
|
🌿 Preview your docs: https://nvidia-preview-feat-1154-offline-verify.docs.buildwithfern.com/aicr |
|
@coderabbitai full review |
✅ Action performedFull review finished. |
njhensley
left a comment
njhensley
left a comment
There was a problem hiding this comment.
Overall assessment — ✅ Approve
Multi-persona review (supply-chain-security, correctness, test-coverage), each finding independently confirmed or refuted by a senior meta-reviewer against the resolved code; the security boundary was additionally re-derived by hand at head d3bf424.
Implements offline key-based verification (#1154) by adding noTLogVerifyPolicy as the clean dual of the signing-side noTLogPolicy, threading IgnoreTLog through VerifyOptions → verifyStagedSnapshot → verifyKeySignedBundle, and fail-closing on --key at both the CLI and library layers. The claimed security boundary holds: the relaxation reaches only verifyKeySignedBundle (gated on Key != ""); the keyless path (verifySigstoreBundle) and the NVIDIA-CI binary attestation (VerifyBinaryAttestation, hardcoded NewRequireTLogPolicy()) cannot be relaxed. Content binding (artifact digest) is preserved.
✅ Confirmed non-issues (examined and cleared)
- Trust-downgrade / path confinement —
IgnoreTLogcannot reach keyless or binary attestation (re-derived at head SHA). - Fail-closed guard —
IgnoreTLog && Key==""rejected withErrCodeInvalidRequestinverifier.Verifyand the CLI; the guard correctly precedesos.Statso it isn't masked byNotFound. - sigstore-go usage —
WithNoObserverTimestamps()used exclusively per the vendoredVerifierConfig.Validate()constraint; valid only for key-based verify, which is exactly this path. - Docs —
cli-reference.md+ flag help updated in-PR; "insecure" naming is accurate.
Summary
| Tier | Count |
|---|---|
| 🔴 Blocker | 0 |
| 🟠 Major | 0 |
| 🟡 Minor | 0 |
| 🔵 Nitpick | 1 |
Recommendation: Approve. The single nitpick is optional test hardening.
| } | ||
| signer, err := attestation.VerifyStatementWith(ctx, data, id, attestation.NewRequireTLogPolicy(), artifactDigest) | ||
| tlogPolicy := attestation.NewRequireTLogPolicy() | ||
| if ignoreTLog { |
There was a problem hiding this comment.
🔵 Nitpick — Offline-verify passing path proven only at the attestation layer, not the verifier layer
This if ignoreTLog { tlogPolicy = NewNoTLogVerifyPolicy() } branch and its threading from VerifyOptions are exercised in the verifier package only by the reject guard; the one positive verifier-package case uses an awskms://test/key URI that fails remote key resolution before reaching the policy selection (the test intentionally tolerates that downstream failure). So no verifier-package test drives Verify() end-to-end with IgnoreTLog=true to a successful offline verification. The relaxation itself IS fully proven one layer down by TestNoTLogVerifyPolicy_RoundTrip (sign offline -> verify with the no-tlog policy succeeds; the same bundle under NewRequireTLogPolicy() fails), so a policy regression is caught — only this trivial 3-line verifier delegation lacks an in-package end-to-end test.
Blast radius: Coverage only; the underlying behavior is covered at the attestation layer. Optional hardening.
Fix: Add a verifier-level test: sign with a local ECDSA PEM, then assert Verify(ctx, dir, &VerifyOptions{Key: pemPath, IgnoreTLog: true}) succeeds and a companion with IgnoreTLog:false fails — proving the threading, not just the guard.
2 participants
Summary
Add an offline / air-gapped verification mode to
aicr verifyvia a--insecure-ignore-tlogflag (bool, defaultfalse). When set (and only with--key), verification skips the Rekor transparency-log and observer-timestamp requirement and verifies the signature against the public key with no network calls. This is the verification counterpart to #409'saicr bundle --signing-key ... --tlog-upload=false.Motivation / Context
Air-gapped signing (#409) produces a bundle with no Rekor entry. The default verify path requires a transparency-log inclusion proof, so such a bundle cannot be verified in a disconnected environment. This flag drops that requirement for key-based verification, mirroring
cosign verify --insecure-ignore-tlog.Fixes: #1154
Related: #409 (air-gapped signing, the sign-side counterpart), #407 (KMS public-key verification)
Type of Change
Component(s) Affected
cmd/aicr,pkg/cli)pkg/bundler,pkg/component/*)docs/,examples/)Implementation Notes
noTLogVerifyPolicy/NewNoTLogVerifyPolicy()inverifytransparency.go, the verify-side dual of feat(bundle): air-gapped signing with --tlog-upload=false for restricted networks #409'snoTLogPolicy.verify.WithNoObserverTimestamps()(confirmed in vendoredpkg/verify/signed_entity.go), used exclusively — the vendoredVerifierConfig.Validate()forbids combining it withWithTransparencyLog/WithObserverTimestamps, and it is only valid for key-based (not certificate) verification, exactly the air-gapped path.IgnoreTLogreaches onlyverifyKeySignedBundle. The keyless path (verifySigstoreBundle) and the NVIDIA-CI binary attestation (VerifyBinaryAttestation) stay hardcoded toNewRequireTLogPolicy()and cannot be relaxed. Defense-in-depth:WithKeysetsrequireSigningKey, so a Fulcio-cert bundle fed to the key path is rejected.IgnoreTLog && Key==""is rejected withErrCodeInvalidRequestin both the library (verifier.Verify) and the CLI.VerifyStatementWithstill appliesWithArtifactDigestand refuses an empty digest, so offline mode cannot accept a signature over unrelated content.Testing
go test -race ./pkg/cli/... ./pkg/bundler/verifier/... ./pkg/bundler/attestation/... golangci-lint run -c .golangci.yaml ./pkg/cli/... ./pkg/bundler/verifier/... ./pkg/bundler/attestation/...TestNoTLogVerifyPolicy_VerifierOptions(exactly one option),TestNoTLogVerifyPolicy_RoundTrip(sign offline withNewNoTLogPolicy(), verify withNewNoTLogVerifyPolicy()→ success; same bundle withNewRequireTLogPolicy()→ failure, proving the relaxation is load-bearing; runs hermetically via an injected trust source),TestVerify_IgnoreTLogRequiresKey, and the CLI--insecure-ignore-tlog-without---keyguard.golangci-lint: 0 issues on all changed packages.Risk Assessment
falsepreserves current behavior. The relaxation is scoped to the key-signed bundle attestation and gated on--key; the keyless and binary-attestation trust paths are unchanged.Rollout notes: Backwards compatible. "insecure" is intentional in the name: with no transparency log there is no trusted timestamp of when the signature was made, so it is only for air-gapped bundles you signed yourself.
Checklist
make testwith-race)make lint)git commit -S)