fix: record Gemara source provenance in pack artifacts - #225
fix: record Gemara source provenance in pack artifacts#225trevor-vaughan wants to merge 2 commits into
Conversation
trevor-vaughan
added
enhancement
- 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>
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
|
trevor-vaughan
changed the title
jpower432
left a comment
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
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)
| // distributable blob (CWE-200): embedded credentials, query strings, and | ||
| // fragments are stripped, and local (file://) paths are omitted entirely. | ||
| // The reference entry is always emitted even when its URI is omitted. | ||
| func BuildProvenance(resolved map[string]*requirement.ResolvedPolicy, policySources map[string]string) []complypack.Provenance { |
There was a problem hiding this comment.
HIGH
BuildProvenance uses make([]complypack.Provenance, 0, len(resolved)), returning a non-nil empty slice when no entries are produced. Go's json:",omitempty" only omits nil slices, so the config blob emits "source":[] instead of omitting the key. Contradicts README line 279. Fix: return nil when len(provenance) == 0.
| } | ||
| prov, err := resolveProvenance(ctx, cfg, t.TempDir()) | ||
| require.NoError(t, err) | ||
| assert.Empty(t, prov) |
There was a problem hiding this comment.
Medium
assert.Empty(t, prov) passes for both nil and []Provenance{}, masking finding #1. Should use assert.Nil(t, prov). No test covers the sources-declared-but-zero-policies path through to JSON serialization
| loadErrs = append(loadErrs, fmt.Errorf("source %s: %w", name, err)) | ||
| continue | ||
| } | ||
| for id := range src.Policies { |
There was a problem hiding this comment.
Low
policySources[id] = entry.Source silently overwrites when multiple sources provide the same policy ID. Last-declared source wins. Deterministic but undocumented; consider a godoc note.
| return ref | ||
| } | ||
|
|
||
| // RedactCredentials removes any userinfo (user:password@) embedded in an OCI |
There was a problem hiding this comment.
RedactCredentials doc claims generic scheme handling, but splitScheme rejects digit-leading prefixes (e.g. 1abc://user:secret@host), passing credentials through unredacted. Limitation is tested but not documented in the function's doc comment.
4 participants
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.