fix: record Gemara source provenance in pack artifacts - #225
Conversation
6fa364f to
fc377de
Compare
✅ CRAP Load Analysis: PASS (no baseline)No baseline file found at How to Enable Regression DetectionGenerate and commit a baseline file to track CRAP score changes over time: # 1. Install gaze
go install github.com/unbound-force/gaze/cmd/gaze@latest
# 2. Run tests and generate baseline
go test -coverprofile=coverage.out ./...
mkdir -p .gaze
gaze crap --format=json --coverprofile=coverage.out ./... > .gaze/baseline.json
# 3. Commit the baseline
git add .gaze/baseline.json
git commit -m "chore: add CRAP baseline for regression detection"For more information: Summary
|
jpower432
left a comment
There was a problem hiding this comment.
🤖 LLM-assisted
issue (blocking): BuildProvenance doesn't distinguish bundled OCI sources from other source types.
In go-gemara, looking at bundle/assemble.go: when Gemara assembles a bundle, it fetches transitive dependencies from MappingReference.Url at assembly time and packs them as layers.
By the time complypack resolves a pre-assembled OCI bundle, those URLs were already consumed -- the authoritative provenance is the bundle's OCI reference + digest, not the mapping reference URLs which describe a past assembly step.
For file sources and unbundled/live resolution, recording MappingReference.Url is reasonable.
For pre-assembled OCI bundles specifically, the provenance should record the bundle's OCI reference.
thought: For reference, this intersects with an open Gemara question about whether bundles should be resolved at pack time or fetch time.
Address PR #225 review feedback: - BuildProvenance now records the bundle's OCI reference as the authoritative URI for policies from pre-assembled OCI bundles, instead of the assembly-time MappingReference URLs which describe a past fetch step. File and unbundled sources continue to use MappingReference.Url as before. - LoadResult.PolicySources tracks which source string provided each policy ID so BuildProvenance can distinguish source types. - Remove SanitizeSourceID wrapper (single caller, zero added logic); call registry.RedactCredentials directly. TestSanitizeSourceID removed — identical coverage exists in TestRedactCredentials. - Convert log.Printf to slog.Info for the two provenance-resolution progress messages in pack.go, matching the structured logging convention used elsewhere in the file. Assisted-by: Claude (Anthropic, Claude 3.5 Sonnet 4.5) Signed-off-by: Trevor Vaughan <tvaughan@redhat.com>
yvonnedevlinrh
left a comment
There was a problem hiding this comment.
Review Summary
Overall this is a well-structured PR — the architecture follows the thin-transport/domain-logic split, security defense is layered at three levels (URI sanitization, credential redaction, error redaction), and the test suite is comprehensive with 20+ cases covering determinism, cardinality, and edge cases. CI and local pre-flight checks pass.
Requesting changes on one functional bug and one test gap that masks it. Two additional low-severity items noted inline.
Findings
| # | Severity | Location | Description |
|---|---|---|---|
| 1 | High | provenance.go:39 |
BuildProvenance returns non-nil empty slice, causing "source":[] in config blob instead of omitting the key (contradicts README) |
| 2 | Medium | pack_provenance_test.go:166 |
assert.Empty masks #1 — should be assert.Nil; missing test for sources-declared-but-zero-policies → JSON serialization path |
| 3 | Low | pipeline.go:53-55 |
policySources map silently overwrites on duplicate policy ID across sources; deterministic but undocumented |
| 4 | Low | client.go:89-100 |
RedactCredentials doc claims generic scheme handling but splitScheme rejects digit-leading prefixes, passing credentials through; limitation is tested but not documented |
What looks good
- Deterministic output: sort-by-PolicyID + sort-by-ReferenceID, verified with 20-iteration fuzz test
- CWE-200/CWE-209 defense in depth across
sanitizeURI,RedactCredentials, andredactPathError - CWE-400 timeout bound on source resolution (
packResolveTimeout) - Backward-compatible
LoadResult.PolicySourcesaddition — existing callers unaffected Config.Validate()loop correctly no-ops on nil/emptySource- Addressed prior review feedback (slog migration,
SanitizeSourceIDremoval)
- Return nil instead of empty slice from BuildProvenance when no entries are produced, so json:",omitempty" correctly omits the key (HIGH). - Use assert.Nil instead of assert.Empty in the zero-policies test to catch non-nil empty slices (Medium). - Document last-declared-wins behavior when multiple sources provide the same policy ID in PolicySources godoc (Low). - Document digit-leading scheme limitation in RedactCredentials godoc (Low). Assisted-by: Claude (Anthropic, Claude Opus 4.6) Signed-off-by: Trevor Vaughan <tvaughan@redhat.com>
515d880 to
9f006a5
Compare
yvonnedevlinrh
left a comment
There was a problem hiding this comment.
LGTM - No blockers. All change requests from both me and Jenn have been addressed
Only one medium finding that currently has zero impact today
Findings
1. policySources populated before Merge — latent stale-entry risk
Severity: Medium | Category: Correctness
File: internal/pipeline/pipeline.go:56-62
policySources[id] = src is populated inside the LoadArtifacts loop before loaded.Merge() runs. If Merge fails, policySources contains entries from the failing source, but since LoadAndResolve returns nil on any error (line 72), the stale map is never consumed. No runtime bug today, but if the error handling is ever relaxed to partial results, this ordering would silently include policies from a failed merge.
- refactor(config): redesign Config.Source from *Provenance to []Provenance
- Each entry carries a PolicyID and []GemaraRef (ReferenceID, URI, Version)
- Validation rejects empty PolicyID, empty GemaraContent, and empty
ReferenceID with indexed error messages
- feat(pipeline): add BuildProvenance to map resolved policies to provenance
- Output sorted by PolicyID then ReferenceID for byte-identical blobs
- fix(security): sanitize URIs before recording into published blobs (CWE-200)
- Strips credentials, query strings, and fragments; omits file:// paths
- fix(security): redact credentials in error messages (CWE-209)
- RedactCredentials in registry strips userinfo using last-@-before-slash
- loadFileArtifacts redacts both the wrapper and the wrapped os.PathError
- refactor(pipeline): collect all load/merge failures into one joined error
- Names every offending source instead of failing on the first
- feat(pack): add --cache-dir flag for headless/restricted environments
- feat(pack): bound source resolution with 5-minute context timeout (CWE-400)
- docs(readme): document gemara.sources config, pack flags, source provenance
- test: BuildProvenance cardinality, determinism, URI sanitization, import-less
policies, orphan refs, nil/empty input, RedactCredentials edge cases,
end-to-end credential leak through Pack, batch errors, context cancellation
BREAKING CHANGE: Config.Source changed from *Provenance to []Provenance and Provenance.GemaraContent changed from string to []GemaraRef — consumers of the OCI config blob JSON must update deserialization
Fixes: #221
Assisted-By: Claude Opus 4.8
Signed-off-by: Trevor Vaughan <tvaughan@redhat.com>
BuildProvenance now records the bundle's OCI reference as the authoritative URI for policies from pre-assembled OCI bundles. The assembly-time MappingReference URLs describe a past fetch step and misattribute provenance when the bundle is the actual source. File and unbundled sources still use MappingReference.Url. - feat(pack): track policy-to-source mapping in LoadResult.PolicySources - refactor(pack): inline SanitizeSourceID; call registry.RedactCredentials directly - Single caller with no added logic; identical coverage in TestRedactCredentials - refactor(pack): switch log.Printf to slog.Info for resolution progress - test(pack): add coverage for OCI, file, mixed, and credential-bearing sources Refs: #221 Assisted-by: Claude (Anthropic, Claude 3.5 Sonnet 4.5) Signed-off-by: Trevor Vaughan <tvaughan@redhat.com>
- Return nil instead of empty slice from BuildProvenance when no entries are produced, so json:",omitempty" correctly omits the key (HIGH). - Use assert.Nil instead of assert.Empty in the zero-policies test to catch non-nil empty slices (Medium). - Document last-declared-wins behavior when multiple sources provide the same policy ID in PolicySources godoc (Low). - Document digit-leading scheme limitation in RedactCredentials godoc (Low). Assisted-by: Claude (Anthropic, Claude Opus 4.6) Signed-off-by: Trevor Vaughan <tvaughan@redhat.com>
Stale source mappings remained when Merge() failed because
policySources was populated before the Merge check. Moving
population after the success path prevents orphaned entries.
- build(deps): bump ~25 indirect Go dependencies
- charmbracelet/bubbles v1.0.0, bubbletea v1.3.10, jwx v3.2.0,
semver v3.5.0, protobuf v1.36.12, and golang.org/x modules
- build(deps): add clipperhouse/displaywidth v0.11.0 (new transitive)
Refs: #225 (review)
Assisted-by: Claude (Anthropic, Claude 3.5 Sonnet 4.5)
Signed-off-by: Trevor Vaughan <tvaughan@redhat.com>
9f006a5 to
1c7c7bc
Compare
|
@jpower432 Rebased and your items have been addressed when you get a chance. @yvonnedevlinrh Addressed your comment since I had to rebase anyway, thanks |
Summary
Resolves declared
gemara.sourcesduringcomplypack packand writes per-policy provenance (imported catalogs and guidance references) into the OCI config blob. Packs with no declared sources are unaffected.Key changes:
Config.Sourceis now[]Provenance, each carrying aPolicyIDand[]GemaraRef(ReferenceID,URI,Version). This is a breaking change to the config blob JSON schema.BuildProvenancefunction ininternal/pipelinemaps resolved policies to deterministically ordered provenance records (sorted by PolicyID, then ReferenceID) for byte-identical config blobs.file://paths are omitted entirely.RedactCredentialsininternal/registrystrips userinfo from source references in error messages.loadFileArtifactsredacts both the wrapper message and the wrappedos.PathError.LoadAndResolvecollects all load/merge failures into one joined error naming every offending source, instead of failing on the first.--cache-dirflag — for headless or restricted environments whereHOMEis unset.gemara.sourcesconfig, pack flags, and source provenance blob format.Related Issues
complypack.Config.Sourceprovenance is never populated #221Review Hints
Start with
pkg/complypack/config.goto see the newProvenance/GemaraReftypes and validation, theninternal/pipeline/provenance.goforBuildProvenanceandsanitizeURI.internal/registry/client.gohasRedactCredentialsandsplitScheme— the credential stripping heuristic uses last-@-before-first-/to handle passwords containing@while preserving digest@sha256:references. Worth checking that the edge cases inTestRedactCredentialscover your mental model.cmd/complypack/cli/pack_provenance_test.gohas the end-to-end credential-leak test: it pushes a pack with a credentialed mapping-reference URL and asserts the published config blob contains neither the password nor the query string.The batch-error rework in
internal/pipeline/pipeline.gochanged the error format from"failed to load artifacts from ..."to"source ...: ...", which required updating assertions ininternal/mcp/server_test.goandacceptance/mcp_server_test.go.Review all commits together — they build on each other as a single logical change.