fix(evidence): key-constrain all redaction allowlist subtypes#1419
Conversation
…losed) The package documented a fully fail-closed allowlist, but the keep()-all subtypes (server, hardware, release, summary) were fail-open at the key level: a sensitive key a future collector attaches to one of them would ship in minimal bundles by default. OS.release was the concrete risk — the collector ships all of /etc/os-release verbatim, so operator-influenced keys could leak. Constrain every kept subtype to an explicit key allowlist and remove the keep-all escape hatch, so the guarantee holds at type, subtype, and key level. release is bound to standard distro-identity keys. Follow-up to PR NVIDIA#1414 review (mchmarny).
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 (3)
📝 WalkthroughWalkthroughThe minimal evidence redaction policy is tightened to be fail-closed at the individual data-key level for snapshot measurement subtypes. The Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related issues
🚥 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 |
2 participants
Summary
Make the minimal-evidence snapshot allowlist fail-closed at the key level, not just at type/subtype. Every kept subtype is now key-constrained and the keep-all escape hatch is removed, so the package's stated guarantee holds at every level.
Motivation / Context
Follow-up to a review comment on the merged #1414 (discussion, @mchmarny). The package doc claims "anything a future collector adds is dropped until explicitly allowlisted," but that held only at type and subtype granularity (plus key granularity for
K8s.node). Thekeep()-all subtypes —server,hardware,release,summary— were fail-open at the key level: a sensitive key a future collector attaches to one of them would ship by default.OS.releasewas the concrete risk: the collector ships all of/etc/os-releaseverbatim, so a non-standard distro could inject arbitrary operator-influenced keys that would publish in the "minimal" bundle.Nothing leaks today (these subtypes currently carry only version/model/count/distro-identity keys), but the asymmetry with the deliberately key-constrained
nodesubtype was exactly the trap. This change makes the implementation match the stated guarantee.Fixes: N/A
Related: #1414, #1345
Type of Change
Component(s) Affected
pkg/evidence/redact,docs/design/007-recipe-evidence.mdImplementation Notes
keep()-all variant is removed, socopySubtypealways filters by the key set (no nil = keep-all path). A key not listed in its subtype's policy is dropped, fail-closed.OS.releaseis bound to standard distro-identity keys (ID,ID_LIKE,NAME,PRETTY_NAME,VERSION,VERSION_ID,VERSION_CODENAME); other/etc/os-releasefields are dropped.server,hardware, andsummaryare pinned to their known collector keys.PolicyVersionis intentionally not bumped: v1 merged yesterday and has no consumers, so there is no signal to preserve.Testing
TestSnapshotKeyConstrainsAllKeptSubtypesproves an unknown/sensitive key on each formerly-keep-all subtype (incl. non-standardos-releasekeys) is dropped while known keys survive.pkg/evidence/redactcoverage 98.1%.-raceand the sigstore-dependent tests rely on CI (no local C compiler / sigstore network in my sandbox).Risk Assessment
Rollout notes: Minimal bundles now ship slightly fewer
os-releasekeys (standard identity only). No verification or schema impact;--fullis unaffected. N/A migration.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)