feat(evidence): minimize sensitive content in evidence bundles#1414
Conversation
…ault Evidence bundles shipped the full snapshot and per-phase CTRF stdout, publishing operational detail that is sensitive to expose: node names, provider instance IDs, the node label/taint set, kernel/sysctl tuning, loaded modules, systemd config, and raw container logs. The verifiable signal is digest-based — the signed predicate commits to artifacts by hash and carries the derived fingerprint, criteria-match, and per-phase counts. The snapshot and CTRF logs are backing content those digests point at, not the signal itself, so they can be minimized without weakening the binding. Bundles are now minimized by default; --full restores the raw payloads: - snapshot.yaml is reduced to a fail-closed allowlist (unlisted measurement types/subtypes/keys drop by default) - per-test CTRF stdout/message are omitted - the predicate records the applied policy in a new `redaction` block The predicate's fingerprint/criteriaMatch are still computed from the raw snapshot, so the conformance signal is unchanged; only the shipped bytes shrink. Digests cover whatever ships, so a minimal bundle self-verifies with `aicr evidence verify`. A --full predicate is byte-identical to prior output (redaction omitted). Adds pkg/evidence/redact, the --full flag, verifier output for the policy, and updates ADR-007 and CLI/verification docs. Closes NVIDIA#1345
njhensley
added
the
theme/supply-chain
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Enterprise Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThis PR implements minimal-by-default evidence bundle redaction. A new Estimated code review effort🎯 4 (Complex) | ⏱️ ~70 minutes 🚥 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: 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/evidence/redact/redact.go`:
- Around line 211-223: The copyHeaderDroppingSourceNode function currently uses
a blacklist approach that only drops the source-node key and passes through all
other metadata keys, which is a fail-open posture that risks exposing future
metadata keys without explicit approval. Switch to an allowlist approach where
you define a set of allowed metadata keys that are safe to include, and only
copy those specific keys from the input header's Metadata map to the output
Metadata map. This ensures the minimal-policy fail-closed posture where any new
metadata keys are excluded by default until explicitly approved.
🪄 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: 75f87e60-74e9-4c27-bc4d-79f784f2d2c8
📒 Files selected for processing (19)
docs/design/007-recipe-evidence.mddocs/user/artifact-verification.mddocs/user/cli-reference.mddocs/user/validation.mdpkg/cli/consts.gopkg/cli/validate.gopkg/cli/validate_evidence.gopkg/cli/validate_evidence_test.gopkg/evidence/attestation/builder.gopkg/evidence/attestation/emit.gopkg/evidence/attestation/emit_test.gopkg/evidence/attestation/predicate.gopkg/evidence/attestation/predicate_test.gopkg/evidence/attestation/types.gopkg/evidence/redact/redact.gopkg/evidence/redact/redact_test.gopkg/evidence/verifier/report.gopkg/evidence/verifier/report_redaction_test.gopkg/evidence/verifier/verify_minimal_test.go
Header metadata redaction dropped only source-node and forwarded every
other key, a fail-open posture inconsistent with the allowlist policy
used elsewhere. Restrict to a {timestamp, version} allowlist so future
metadata keys are excluded by default. Addresses CodeRabbit review.
There was a problem hiding this comment.
LGTM. The core insight — that the trust signal is digest-based, so the snapshot/CTRF backing content can be minimized without weakening the binding, and the fingerprint must therefore be derived from the raw snapshot rather than the shipped bytes — is exactly right, and the implementation matches it cleanly. --full byte-identity, the omitempty redaction block, and self-verification of minimal bundles are all handled well.
One medium, non-blocking note inline: the "fail-closed" guarantee is only partial — the unconstrained keep()-all subtypes are fail-open at the key level, which the package doc overclaims. Worth reconciling doc and implementation, but nothing leaks today and it doesn't gate this merge.
Nice work — the ADR-007 extension and the docs are thorough.
| // when the allowlist changes. | ||
| var snapshotAllowlist = map[measurement.Type]map[string]subtypePolicy{ | ||
| measurement.TypeK8s: { | ||
| "server": keep(), // version etc. — not sensitive |
There was a problem hiding this comment.
Medium, non-blocking — fail-closed claim is only partial. The package doc (lines 27-31) says "anything a future collector adds is dropped until explicitly allowlisted," but that holds only at type and subtype granularity, plus key granularity for key-constrained subtypes like node. The keep()-all subtypes here (server, hardware, release, summary) are fail-open at the key level: a sensitive key a future collector attaches to one of these ships by default.
Nothing leaks today — verified these four currently carry only version/model/count/distro-identity keys. But the asymmetry with node (which you correctly key-constrained because it has sensitive keys) is exactly the trap. Two options: key-constrain these four as well, or tighten the doc and add an invariant note that server/hardware/release/summary must never accrue node-identifying keys (and bump PolicyVersion if they do). Either way the stated guarantee and the implementation should match.
2 participants
Summary
Evidence bundles are now minimized by default:
snapshot.yamlis reduced to a fail-closed allowlist and per-test CTRFstdout/messageare omitted, keeping sensitive operational detail out of the published artifact. A new--fullflag restores the raw payloads.Motivation / Context
An evidence bundle shipped the full validate-time snapshot and per-phase CTRF stdout, publishing detail that is sensitive to expose: node names, cloud provider instance IDs, the full node label/taint set, kernel/sysctl tuning, loaded modules, systemd service config, and raw container logs.
The verifiable trust signal is digest-based — the signed in-toto predicate commits to artifacts by hash and carries the derived fingerprint, criteria-match, and per-phase counts. The snapshot and CTRF logs are the backing content those digests point at, not the signal itself, so they can be minimized without weakening the cryptographic binding. This extends the same reasoning ADR-007 already applied to logs.
Fixes: #1345
Related: N/A
Type of Change
Component(s) Affected
cmd/aicr,pkg/cli)docs/,examples/)pkg/evidence/{redact,attestation,verifier}Implementation Notes
pkg/evidence/redact— pure, deterministic, non-mutatingSnapshot()andCTRF(). The snapshot allowlist is fail-closed: only enumerated measurement subtypes/keys survive, so a field a future collector adds is dropped until explicitly allowlisted. v1 keepsK8s.server, an allowlistedK8s.nodesubset (dropssource-node,provider-id,container-runtime-id),GPU.hardware,OS.release,NodeTopology.summary; dropsOS.{grub,sysctl,kmod}, all ofSystemD,NodeTopology.{label,taint}, subtypeContext, and headermetadata.source-node.fingerprint.FromMeasurementsreads fields the allowlist drops (e.g.NodeTopology.label), so deriving the predicate's fingerprint/criteriaMatch from the full snapshot keeps the conformance signal identical whether or not the shipped bytes were minimized. The verifier trusts the fingerprint via the signature; it never recomputes it fromsnapshot.yaml.aicr evidence verify. The applied policy is recorded in a newpredicate.redactionblock (omitempty), so a--fullpredicate is byte-identical to prior output.aicr validate --emit-attestationnow writes a redacted bundle. No documented downstream consumer (recipe-health, CNCF submission) reads the bundle'ssnapshot.yamlback.--fullis the opt-out.redactionis an additiveomitemptypredicate field, consistent with ADR-007's additive-field policy;PredicateSchemaVersionstays1.0.0.Testing
pkg/evidence/redactcoverage 98.4%; new round-trip test confirms a minimal bundle self-verifies (TestVerify_MinimalBundleSelfVerifies);--fullbyte-identity and fingerprint-from-raw decoupling are guarded by tests.-race;sigstore.devTUF/Fulcio network blocked) — the-racerun and thepkg/bundler/attestation/pkg/trustsigstore tests rely on CI. Those packages do not import the changed code.Risk Assessment
Rollout notes: Default
--emit-attestationoutput is now redacted; operators who want the prior full payloads pass--full. Verification is unaffected (minimal bundles self-verify;--fullpredicates are byte-identical to prior output). No migration required.Checklist
make testwith-race) — non-race suite passes locally;-racerequires CI (no local C compiler)make lint) — pinnedgolangci-lint v2.12.2, 0 issuesgit commit -S)