Skip to content

Add pre-cache artifact scanning hook (trivy/ClamAV/Wiz/custom) - #298

Open
aarnaud wants to merge 1 commit into
git-pkgs:mainfrom
aarnaud:feat/artifact-scanner-hook
Open

Add pre-cache artifact scanning hook (trivy/ClamAV/Wiz/custom)#298
aarnaud wants to merge 1 commit into
git-pkgs:mainfrom
aarnaud:feat/artifact-scanner-hook

Conversation

@aarnaud

@aarnaud aarnaud commented Sep 2, 2026

Copy link
Copy Markdown
Contributor

Runs fetched artifacts through pluggable external scanners after they're staged in storage but before they're committed to the cache DB, so a block verdict deletes the object instead of ever exposing it to a client. Scanners pull the staged bytes themselves via a short-lived HMAC-signed internal route rather than the proxy pushing bytes to them, keeping the mechanism storage-backend-agnostic and avoiding uploading potentially huge artifacts through the proxy's own egress.

Hardening baked in from the start: the internal scan-fetch route is gated both at router-mount time and in the handler so it's inert whenever scanning is disabled or unsigned; the signing key is mandatory whenever scanning is enabled, enforced directly in scanner.NewGroup rather than relying on callers to invoke config validation; the scan call and the delete-on-block cleanup both run on a context detached from the client's, so a client disconnecting mid-scan can't be mistaken for a scanner failure, doesn't cause a legitimate artifact to be deleted, and doesn't leave a genuinely blocked artifact's bytes orphaned in storage; and scanner infrastructure errors (connection failures, internal hostnames) are never forwarded verbatim to anonymous clients, only a generic message. The scan-error metric also correctly distinguishes a scanner's own timeout from being cancelled because a sibling scanner already decided the verdict.

Runs fetched artifacts through pluggable external scanners after they're
staged in storage but before they're committed to the cache DB, so a
block verdict deletes the object instead of ever exposing it to a
client. Scanners pull the staged bytes themselves via a short-lived
HMAC-signed internal route rather than the proxy pushing bytes to them,
keeping the mechanism storage-backend-agnostic and avoiding uploading
potentially huge artifacts through the proxy's own egress.

Hardening baked in from the start: the internal scan-fetch route is
gated both at router-mount time and in the handler so it's inert
whenever scanning is disabled or unsigned; the signing key is mandatory
whenever scanning is enabled, enforced directly in scanner.NewGroup
rather than relying on callers to invoke config validation; the scan
call and the delete-on-block cleanup both run on a context detached
from the client's, so a client disconnecting mid-scan can't be mistaken
for a scanner failure, doesn't cause a legitimate artifact to be
deleted, and doesn't leave a genuinely blocked artifact's bytes
orphaned in storage; and scanner infrastructure errors (connection
failures, internal hostnames) are never forwarded verbatim to anonymous
clients, only a generic message. The scan-error metric also correctly
distinguishes a scanner's own timeout from being cancelled because a
sibling scanner already decided the verdict.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
@aarnaud
aarnaud force-pushed the feat/artifact-scanner-hook branch from d3d1d01 to 641762f Compare September 2, 2026 03:25

@andrew andrew left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for this — the design is the right shape (matches the admission-controller framing from #155: external decision hook, fail-closed default, proxy stays out of policy), and the pull-not-push fetch route plus the fetchAndCacheFromURL de-dup into storeArtifact are both good. A few things need fixing before merge.

Blocking

  • internal/handler/npm.go:288 — response-format regression. Previously 404 and 502 on tarball fetch returned JSON via JSONError; routing through serveArtifactError now returns plain-text http.Error for both. npm expects {"error": "..."} bodies. Either keep the inline handling here and add only the ErrArtifactBlocked branch, or give serveArtifactError a JSON mode.
  • internal/config/config.go:325ParseScanningTimeout is never called. scanner.NewGroup parses cfg.Timeout itself and Group.Timeout() is what runScan uses. Delete it (and defaultScanningTimeout; keep defaultScanningTimeoutStr for Validate).
  • lint CI job is failing; all four hits are in this change:
    • internal/scanner/http.go:74 S1016 — httpScanRequest is field-identical to Request; add json tags to Request, marshal req directly, drop httpScanRequest.
    • internal/handler/scan_test.go:39 unparam — failOpen is always false; drop the parameter.
    • internal/server/server_test.go:178 gocognit — the new scan-fetch-404 assertion tipped an existing test over; move those lines to their own test func.
    • internal/server/server.go:200 maintidx — serve() was already at the limit; extract the scanGroup construction + proxy.Scan* field assignment into a small helper.

Non-blocking

  • internal/scanner/group.go:124Scan never returns a non-nil error, so the if err != nil branch in runScan is unreachable. Drop the error return.
  • internal/config/config.go:270ScanningConfig.Validate accepts enabled: true with zero scanners, which builds a Group where Enabled() is false and the feature silently no-ops despite requiring a signing key. Reject empty scanners when enabled.
  • internal/metrics/metrics.go:255RecordScanError doc lists "error", "timeout" but group.go also passes "cancelled".
  • internal/scanner/group.go:162-169 — when a sibling has already blocked and cancelled scanCtx, the remaining block-mode scanners still log "scanner call failed, blocking artifact" before finding blocked != nil. Return early on errType == "cancelled" to avoid a spurious warn per sibling.

Worth linking #155 in the description (not Fixes — the mirror/batch case there is still open).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants