Conversation
Signed-off-by: Giles Cope <gilescope@gmail.com>
|
You are seeing this message because GitHub Code Scanning has recently been set up for this repository, or this pull request contains the workflow file for the Code Scanning tool. What Enabling Code Scanning Means:
For more information about GitHub Code Scanning, check out the documentation. |
adamreynolds-io
left a comment
There was a problem hiding this comment.
Security Review
The containerisation approach is a strong security improvement over direct runner execution — good response to the KICS supply chain incident. Hash verification, digest-pinned base images, non-root execution, and secret isolation are all well done.
Verified clean:
- EarthBuild binary hash matches the v0.8.17 GitHub release asset digest exactly
- No
${{ }}injection vectors —github.action_pathis safe in this context - No secrets or tokens passed into scanner containers
- Checkov hardened with
download-external-modules: falseand--skip-download requirements.txtis pip-compile generated with 2036 lines of SHA256-hashed deps- Renovate comments present for automated updates on all pinned versions
Findings (see inline comments):
- 1 HIGH —
pip-toolsinstalled without hash verification in the requirements generation target - 3 MEDIUM — workspace capture includes
.git/;|| trueswallows all failures;continue-on-errorhides total outages - 1 LOW — GPG signature available but unused for EarthBuild binary
adamreynolds-io
left a comment
There was a problem hiding this comment.
Requesting changes on the items flagged in the inline comments — primarily the unhashed pip-tools install and the inconsistent error handling across scanner targets. See inline comments for details and suggested fixes.
Co-authored-by: Adam Reynolds <adam.reynolds@shielded.io> Signed-off-by: Sean Kwak <sean.kwak@iohk.io>
- Fix broken checkov-requirements target (restore pip-compile, hash-verify pip-tools) - Replace || true with exit code checks (exit <= 1 is findings, > 1 is error) - Add .earthlyignore to exclude .git, .env, node_modules from build context - Add scan output verification step to catch total scanner outages
|
Thanks for the thorough review Adam. Giles asked me to help out due to his busy schedule. I've pushed a commit (e37645e) addressing your findings:
|
Signed-off-by: Squirrel <giles.cope@shielded.io>
cosmir17
left a comment
There was a problem hiding this comment.
LGTM from my side.
@adamreynolds-io's security review was thorough and I've addressed his findings in my earlier commit. Deferring to Adam's re-approval before merging (if @gilescope still wants his approval)
adamreynolds-io
left a comment
There was a problem hiding this comment.
All previous review findings addressed. Security posture is a clear improvement — containerized isolation, hash-verified binaries, non-root execution. The open scorecard_checks passthrough question from @cosmir17 should be resolved before merge (follow-up PR is fine).
Signed-off-by: Giles Cope <gilescope@gmail.com>
My approval is not enough. We need an approval from mn-security or midnightntwrk/mn-sre? |
Defence in depth: containerised parallel scanning
After the KICS supply chain compromise and Trivy concerns, this PR fundamentally rearchitects how scanners run — moving from direct runner execution to isolated containers orchestrated by EarthBuild.
What changes
earth +scanSecurity improvements
GITHUB_TOKENand all env varsscanneruser--require-hashesHow it works
The
+user-sourceLOCALLY target captures the user's code from the workspace. Each scanner copies it viaCOPY +user-source/src. Config files (requirements.txt, .checkov.yml, scorecard.jq) come from the action's build context. No files are written to the user's workspace — no conflict with existing Earthfiles.Other improvements
github_tokeninput (scorecard--localmode doesn't need it)pull-requests: writepermission.gitignoreforscan_reports/download-external-modules: falseand--skip-downloadcurlcalls use--retry 3 --retry-delay 5earth +checkov-requirementstarget regenerates the hashed requirements.txtTo update checkov
earth +checkov-requirements # regenerates requirements.txt with hashesTest plan
earth +scansucceeds locally (arm64)