Add daily firewall container security scan - #47407
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
@copilot scan all know container images used in compiled lock files |
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
✅ Test Quality Sentinel completed test quality analysis. No test files were added or modified in this PR. Test Quality Sentinel skipped. |
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. No ADR enforcement needed: PR does not have the 'implementation' label and has ≤100 new lines of code in business logic directories (0 additions detected in default business logic directories: src/, lib/, pkg/, internal/, app/, core/, domain/, services/, api/). |
|
✅ PR Code Quality Reviewer completed the code quality review. |
There was a problem hiding this comment.
Pull request overview
Adds a daily container supply-chain scan with vulnerability, license, digest-drift, and issue reporting gates.
Changes:
- Installs checksum-verified Syft, Grype, and Grant.
- Scans immutable image digests and reports findings.
- Adds strict license enforcement.
Show a summary per file
| File | Description |
|---|---|
.grant.yaml |
Defines the license allowlist. |
.github/workflows/daily-squid-image-scan.md |
Defines scanning, reporting, and enforcement. |
.github/workflows/daily-squid-image-scan.lock.yml |
Compiled GitHub Actions workflow. |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 2/3 changed files
- Comments generated: 3
- Review effort level: Medium
| jq -s ' | ||
| [.[].containers[]?] | ||
| | unique_by(.pinned_image) | ||
| | sort_by(.image) | ||
| ' > "$output/images.json"; then |
|
|
||
| jq -r ' | ||
| .manifests[]? | ||
| | select(.platform.os == "linux") |
There was a problem hiding this comment.
Review: Add daily firewall container security scan
Good overall design — pinned immutable digests, SHA-256 verified tool downloads, fallback reporting when artifacts are absent, and a single deduplicated issue per image digest.
Three non-blocking but reliability-affecting issues flagged inline:
-
Tool versions duplicated as literal strings (lines 52-56, 370-374) — the fallback and scan summary hardcode the same version strings as the
Install pinned security toolsenv vars. A future version bump will leave stale metadata in both places. -
Grype exit-code guard is wrong (line 285) — the condition
exit_code != 0 && critical_count == 0swallows unexpected Grype errors (exit code > 1, e.g. DB failure) when criticals are present. Should distinguish Grype'sexit 1(threshold exceeded) from other non-zero exits. -
Tool install hardcoded to
linux_amd64(line 94) — theassert x86_64guard makes the install step fail entirely on arm64 runners; the scan infrastructure already iterates arm64 platform digests but would have no scanner available.
These are COMMENT-level — the core scanning logic, supply-chain controls, and issue deduplication are sound.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · sonnet46 50.4 AIC · ⌖ 4.9 AIC · ⊞ 5K
| tools: { | ||
| syft: "1.49.0", | ||
| grype: "0.116.0", | ||
| grant: "0.6.8" |
There was a problem hiding this comment.
Tool versions hardcoded as literal strings in fallback step
Lines 52-56 hardcode the tool versions (syft 1.49.0, grype 0.116.0, grant 0.6.8) in the fallback summary.json. The same versions appear again at lines 370-374. Both locations duplicate the authoritative versions declared in the Install pinned security tools step env vars (SYFT_VERSION, GRYPE_VERSION, GRANT_VERSION).
If those env vars are bumped in the future, the fallback / scan steps will silently emit stale version metadata, making scanner output look like it came from a different tool version than what actually ran.
Suggested fix: Write the versions to a shared file (e.g. $output/tool-versions.env) during installation, then source that file in both places. Or pass the env vars through to the step that builds the fallback JSON.
@copilot please address this.
| image_fixable=$((image_fixable + platform_fixable)) | ||
|
|
||
| GRYPE_DB_AUTO_UPDATE=false grype "sbom:${syft_json}" \ | ||
| --fail-on critical -o table > "$output/grype-${suffix}.txt" 2>&1 |
There was a problem hiding this comment.
Grype --fail-on critical exit code ignored; error only if critical count is zero
Lines 282-287:
GRYPE_DB_AUTO_UPDATE=false grype "sbom:${syft_json}" \
--fail-on critical -o table > "$output/grype-${suffix}.txt" 2>&1
grype_exit=$?
if [ "$grype_exit" -ne 0 ] && [ "$platform_critical" -eq 0 ]; then
record_image_error "Grype table scan failed unexpectedly..."
fiWhen platform_critical > 0, a non-zero exit is silently ignored, so a Grype crash or unexpected error during the table pass is swallowed. The exit code should also be recorded as an operational error when it's non-zero for reasons other than detected criticals (e.g. DB read failure, network issue on a partial run).
Consider:
if [ "$grype_exit" -ne 0 ] && [ "$grype_exit" -ne 1 ]; then
# exit 1 = vulnerabilities found above threshold; anything else is unexpected
record_image_error ...
fi(Grype exits 1 when vulnerabilities exceed the threshold and 0 otherwise.)
@copilot please address this.
| GRANT_SHA256: "6500f8bbf0f20fb993de8084686e199f0ba1eb494769ff75454286d5ef63f919" | ||
| run: | | ||
| set -euo pipefail | ||
| test "$(uname -m)" = "x86_64" |
There was a problem hiding this comment.
Tool install hardcoded to linux_amd64 only — fails silently on arm64 runners
Line 94 asserts uname -m == x86_64 which guards the rest, but the scan step at line 114 uses both amd64 and arm64 platforms. The install script downloads only the linux_amd64 tarball (line 104), so if this job ever runs on an arm64 runner the entire install step fails, tools_ready becomes false, and all per-platform scan data is absent from the summary with only an operational error.
Since the scan already iterates multi-arch platforms, consider installing the appropriate binary for the runner architecture, or document the amd64-only constraint explicitly in the workflow description.
@copilot please address this.
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /tdd, /codebase-design, and /grill-with-docs — requesting changes on correctness and reliability issues.
📋 Key Themes & Highlights
Key Themes
- Silent error swallowing: Missing
-eflag in the scan step and an under-guardedgrype_exitcheck can let errors pass undetected, producing a false-clean summary. - Fragile manifest integrity check: Using
sha256sumon the raw manifest bytes returned bydocker buildx imagetools inspect --rawmay not be byte-stable — consider comparing the OCI digest field directly. - License policy likely too narrow:
.grant.yamlomits common Debian/Ubuntu OS-level licenses (GPL-2.x, LGPL, MPL-2.0); the first real run will likely generate false-positive license failures. - Hardcoded tool versions in two places: The fallback
summary.jsonduplicates version strings from the env block; a version bump will hit one but miss the other. - Hard-coded x86_64 assertion: Produces an unhelpful failure if the runner architecture changes.
- Runner image inconsistency:
ubuntu-lateston the scan job vsubuntu-slimelsewhere — likely intentional (needsdocker buildx), but should be documented.
Positive Highlights
- ✅ Excellent supply-chain hygiene: all tools downloaded with pinned SHA-256 checksums.
- ✅ Immutable-digest scanning is exactly the right approach for reproducible results.
- ✅ Comprehensive error propagation via
record_error/record_image_errorthroughout the scan loop. - ✅ Deduplicated issue creation via
deduplicate-by-titleprevents issue spam on repeated runs. - ✅ Clean separation between the scan job and the agentic reporting step.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · sonnet46 68.1 AIC · ⌖ 5.32 AIC · ⊞ 6.7K
Comment /matt to run again
Comments that could not be inline-anchored
.github/workflows/daily-squid-image-scan.md:2112
[/tdd] The outer scan step uses set -uo pipefail (missing -e), while the install step uses set -euo pipefail. An unhandled error mid-loop won't abort the step and can silently produce a false-clean summary.
<details>
<summary>💡 Suggested fix</summary>
Change the scan step preamble:
set -euo pipefailIf the intent is to allow the loop to continue after per-image errors (which is handled via record_error), add a comment explaining the deliberate omission of -e so futur…
.github/workflows/daily-squid-image-scan.md:2087
[/codebase-design] test "$(uname -m)" = "x86_64" hard-codes the architecture and silently blocks arm64 runners. If CI ever switches to an arm runner, the install step fails with no useful message.
<details>
<summary>💡 Suggested fix</summary>
Replace with an explicit, informative error:
arch="$(uname -m)"
if [ "$arch" != 'x86_64' ]; then
echo "Unsupported architecture: $arch. Only x86_64 is supported." >&2
exit 1
fiOr, better yet, parameterise the download URL so arm64 i…
.github/workflows/daily-squid-image-scan.md:2276
[/tdd] Grype is run twice for each platform: once to get JSON counts and once to get a human-readable table. The second run (line 2275) discards its exit code into grype_exit but it is only checked on the condition platform_critical -eq 0. If grype exits non-zero for a reason other than critical CVEs (e.g. DB corruption), the error is silently swallowed.
<details>
<summary>💡 Suggested fix</summary>
Check grype_exit unconditionally:
if [ "$grype_exit" -ne 0 ]; then
record_im…
</details>
<details><summary>.github/workflows/daily-squid-image-scan.md:2194</summary>
**[/tdd]** The manifest integrity check (`sha256sum -c`) is performed against the raw bytes of the downloaded JSON file, not the OCI digest value. OCI content digests are computed from the serialised manifest bytes, but `docker buildx imagetools inspect --raw` may normalise whitespace or add a trailing newline, making the check fragile.
<details>
<summary>💡 Why this matters</summary>
If the manifest bytes differ even by a single byte from what was originally digested (e.g. pretty-printed vs …
</details>
<details><summary>.github/workflows/daily-squid-image-scan.md:2046</summary>
**[/grill-with-docs]** The fallback `summary.json` hardcodes tool versions (`syft: "1.49.0"`, etc.) inline at two places — here in the fallback and at line 2363 in the real summary. If the versions are ever bumped, one location is likely to be missed.
<details>
<summary>💡 Suggested fix</summary>
Extract the versions into environment variables at the top of the step (where they already appear as `SYFT_VERSION`, etc.) and reference them from both the fallback and the summary generation via `--…
</details>
<details><summary>.grant.yaml:2425</summary>
**[/grill-with-docs]** The allowlist does not include `GPL-2.0-only`, `LGPL-2.1-or-later`, or `MPL-2.0`, which appear in many system packages inside Debian/Ubuntu base images (e.g. `bash`, `libc`). Grant will reject these and trigger false-positive license failures on every scan run until the list is extended.
<details>
<summary>💡 Suggested fix</summary>
Review what licenses the current squid image actually carries (the first scan will reveal this), then extend the allowlist. Typical additio…
</details>
<details><summary>.github/workflows/daily-squid-image-scan.md:2065</summary>
**[/codebase-design]** `runs-on: ubuntu-latest` is used for the scan job, but other jobs in this repo (e.g. the `activation` job above) use `ubuntu-slim`. Using `ubuntu-latest` is slightly heavier and less consistent with the rest of the workflow.
Consider switching to `ubuntu-slim` for alignment, or document why the full image is required (e.g., `docker buildx` availability).
@copilot please address this.
</details>|
@copilot run pr-finisher skill |
There was a problem hiding this comment.
REQUEST_CHANGES — 3 high-severity blocking issues found
The workflow has three correctness issues that can suppress security alerts entirely, and two medium issues that should be fixed before this runs daily in production.
🔴 Blocking issues (high severity)
-
Fail-open in
safe_outputs(line 1557): When the threat-detection LLM fails, the entiresafe_outputsjob is skipped. Real vulnerability findings collected by the agent are never filed as issues — the workflow silently succeeds with no output. -
Drift detection returns
image_updated: falsewhen registry is unreachable (lines ~1787–1798): A registry outage causes every image to be reported as up-to-date, masking unknown drift state. Theoperational_errorsarray captures the fetch failure, but theimage_updatedfield contradicts it. -
Scan step missing
-einset -uo pipefail(line 1721): Without-e, silent intermediate failures (e.g. a failedjqwrite) allow the script to continue, accumulate zero counts, and produce a passing summary withcritical_vulnerabilities: 0.
🟡 Medium issues (should fix)
-
Agent burns AI credits when
scan_imagefails (line 380): The agent condition allows the LLM to run to completion against a placeholder summary, wasting quota and potentially creating duplicate operational-failure issues, before the post-step gate kills the job. -
Issue deduplication relies on the LLM reproducing an exact title string (prompt line ~2390): Any capitalization or digest-length variation breaks deduplication, creating duplicate issues or silently merging new findings into old closed ones.
🔎 Code quality review by PR Code Quality Reviewer · sonnet46 135.2 AIC · ⌖ 5.3 AIC · ⊞ 5.7K
Comment /review to run again
Comments that could not be inline-anchored
.github/workflows/daily-squid-image-scan.lock.yml:1557
Fail-open: safe_outputs is silently skipped when detection fails, causing all vulnerability findings to be dropped with no GitHub issues created.
<details>
<summary>💡 Details and suggested fix</summary>
The condition on line 1557:
if: (!cancelled()) && needs.agent.result != 'skipped' && needs.detection.result == 'success'When threat-detection times out, hits an API error, or exceeds its invocation cap, needs.detection.result == 'success' is false and safe_outputs is…
.github/workflows/daily-squid-image-scan.lock.yml:1797
Drift detection silently reports image_updated=false when the registry is unreachable, masking unknown drift state as "no change".
<details>
<summary>💡 Details and suggested fix</summary>
When docker buildx imagetools inspect "$image_tag" fails (registry down, auth error, firewall block), the outer else branch sets current_digest="unknown" but leaves image_updated=false. The summary written to image-results.jsonl therefore reports the image as up-to-date with `image_updated: …
.github/workflows/daily-squid-image-scan.lock.yml:1721
Scan step uses set -uo pipefail but omits -e, allowing silent failures that produce zero-finding summaries without aborting the script.
<details>
<summary>💡 Details and suggested fix</summary>
Line 1721 (and its mirror on line 2112 in the .md compiled output):
set -uo pipefailWithout -e, commands that fail but are not wrapped in if ! (e.g. intermediate jq file-writes, grype db status) continue silently. This means an empty or absent output file propagates down…
.github/workflows/daily-squid-image-scan.lock.yml:380
Agent job runs and burns AI credits when scan_image fails, then a post-step gate kills it — the LLM executes to completion first, consuming quota with no actionable output.
<details>
<summary>💡 Details and suggested fix</summary>
Line 380:
if: (always() && needs.scan_image.result != 'skipped') && (needs.activation.outputs.daily_ai_credits_exceeded != 'true')When scan_image has result failure, this condition is satisfied. The "Ensure image scan summary exists" step syn…
.github/workflows/daily-squid-image-scan.md:2390
Issue deduplication depends entirely on the LLM producing an exact title format — any variation creates a duplicate open issue or silently merges a new finding into an old closed one.
<details>
<summary>💡 Details and suggested fix</summary>
The prompt at line 2390 instructs the agent:
> Use the title Container findings for <first 12 characters of index_digest>
The deduplicate_by_title: true safe-output handler matches on the full, prefix-adjusted title. The deduplication logic is th…
|
@copilot run pr-finisher skill |
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
@copilot rename agentic workflow file to match intent |
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
🎉 This pull request is included in a new release. Release: |
Adds daily supply-chain scanning for the immutable multi-architecture
gh-aw-firewall/squidimage. Findings create a deduplicated issue labeledcookieandsecurity.Changes
Container scanning
linux/amd64andlinux/arm64image digests with Syft and Grype.License enforcement
.grant.yamlusing the license policy fromCONTRIBUTING.md.Supply-chain controls
Reporting and gates