Skip to content

add image_scan operation to release-build - #867

Open
haneric00 wants to merge 2 commits into
mainfrom
artifact-verification
Open

add image_scan operation to release-build#867
haneric00 wants to merge 2 commits into
mainfrom
artifact-verification

Conversation

@haneric00

Copy link
Copy Markdown
Contributor

title

As part of adding trivy artifact verification to release builds.

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

@haneric00
haneric00 requested a review from a team as a code owner August 28, 2026 00:05
@github-actions

github-actions Bot commented Aug 28, 2026

Copy link
Copy Markdown
Contributor

Claude finished @haneric00's task in 1m 42s —— View job


Claude PR Review

  • Read prior reviews and comments (none exist)
  • Review PR diff
  • Read changed files and related context
  • Post findings

Findings

1. Scan runs after image push — vulnerable images can reach the public registry.github/workflows/release-build.yml:152-169

The new Perform image scan step (line 163) runs after the Build and push image step (line 152), which pushes with push: true to both RELEASE_PUBLIC_REPOSITORY (public.ecr.aws/...) and RELEASE_PRIVATE_REPOSITORY. If Trivy detects a vulnerability and fails, the image is already public. The scan can gate downstream steps but cannot un-publish a pushed image.

Consider one of:

  • Build locally first (push: false, load: true), scan the local image, then push in a separate step gated on scan success. Note: this requires single-arch (or a two-stage build) since docker buildx multi-platform builds cannot load: true directly.
  • Or, scan a prior candidate image (e.g., the main-build image referenced by get_main_build_run) before promoting to the release tag.

Fix this →

2. Severity threshold is very aggressive for release-gating.github/workflows/release-build.yml:167

severity: 'CRITICAL,HIGH,MEDIUM,LOW,UNKNOWN'

Because image_scan (.github/actions/image_scan/action.yml:43) sets exit-code: '1', this fails the release on any LOW or UNKNOWN CVE. Compare with .github/workflows/daily-scan.yml:93-111, which separates HIGH scans (blocking) from LOW/MEDIUM/UNKNOWN scans (informational, tracked via a separate CloudWatch metric). A release-blocking scan on LOW,UNKNOWN will likely flap frequently and pressure people to add ignores.

Recommend gating the release on CRITICAL,HIGH only (matching how the daily scan treats blocking severities), and — if desired — running a separate non-blocking LOW/MEDIUM/UNKNOWN scan for visibility.

Fix this →

3. Reusing daily-scan.trivyignore.yaml — semantics and stale entries.github/workflows/release-build.yml:169, .github/trivy/daily-scan.trivyignore.yaml

  • The trivyignore comment says "Daily scans should flag all CVEs" and the file is described as "intentionally empty" — but the release build is now consuming this same file, so the naming and comment become misleading. A pr-build.trivyignore.yaml already exists; consider adding a release-build.trivyignore.yaml (or renaming the daily one to strict.trivyignore.yaml) to make the intent explicit.
  • Both entries in daily-scan.trivyignore.yaml have expired_at: 2026-06-01, which is in the past (today: 2026-08-28). Those ignores are no longer effective, so if the release scan hits CVE-2026-44431 / CVE-2026-44432, it will fail unless the image was rebuilt. Confirm the base image has been rebuilt with a fixed urllib3, or update the expirations.

Fix this →

4. Minor: logout: 'false' without an explicit docker login public.ecr.aws

publish-sdk already logs in to public.ecr.aws (line 117-120), so logout: 'false' is intentional here to reuse that session. That is fine — flagging for confirmation that the intent is "keep the authenticated public.ecr.aws session so Trivy can pull the tagged image." If the runner ever loses that session (e.g., token expiry between long build and scan), the scan would fall back to unauthenticated pulls, which normally still work for public.ecr.aws but may be rate-limited.


Nit

  • Consider adding if: always() to the scan (as the daily-scan does) so that scan output is captured even when earlier steps fail intermittently — though for a release build blocking on failure may be preferred.

No blocking correctness bugs in the diff itself; the main concern is the ordering of scan vs. push (finding #1), which materially changes what "release gating" means here.
· Branch

@haneric00 haneric00 added the skip changelog doesn't need a CHANGELOG entry label Aug 28, 2026
@github-actions

github-actions Bot commented Aug 28, 2026

Copy link
Copy Markdown
Contributor

Claude finished @haneric00's task in 48s —— View job


Claude PR Review

  • Read prior reviews and comments
  • Review current PR diff
  • Check for new findings and dedupe
  • Post findings

No new findings on this synchronize event. The PR diff is unchanged since the prior review — this run was triggered by a merge from main (0f65482), not by changes to the added scan step.

The prior findings still apply and remain unaddressed:

  1. Scan runs after push — vulnerable images can still reach public.ecr.aws before the gate fires (.github/workflows/release-build.yml:163).
  2. Severity CRITICAL,HIGH,MEDIUM,LOW,UNKNOWN is too broad for release-gating (.github/workflows/release-build.yml:167).
  3. daily-scan.trivyignore.yaml reuse — the file's own comment says it should stay empty, and both entries have expired_at: 2026-06-01 (already past as of 2026-08-28), so those ignores are inactive.
  4. Minor: confirm logout: 'false' is intentional to reuse the public.ecr.aws login from publish-sdk.

See the previous comment for full detail and per-finding fix links.
· Branch

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

Labels

skip changelog doesn't need a CHANGELOG entry

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant