aws-samples / aws-samples/sample-aiml-security-assessment
CI: two ASH security-scan steps cannot fail the build, plus unpinned actions and linters
- Dominant language
- HTML
- Stars
- 43
- Forks
- 20
- Avg merge
- 2d 23h
- Merged PRs (30d)
- 4
Description
## Summary
While working on #54 I found that **two of the ASH security-scan steps cannot fail the build**,
so a failing scan reports green. Alongside that there are several supply-chain and
reproducibility gaps in the workflows.
I originally raised these as a PR (#55) but have closed it. All the fixes require editing
`.github/workflows/`, and a rebrand contribution isn't the right vehicle for changing this
repo's CI risk posture — that's a maintainer call. Filing here instead so the findings are
recorded without asking anyone to accept a workflow diff.
A working patch exists on `ci/harden-release-pipeline` in
https://github.com/mehtadman87/sample-aiml-security-assessment if any of it is wanted. It was
verified green (6/6 checks). I'm happy to re-open a PR for any subset.
---
## 1. Two security gates cannot fail (highest priority)
### `pipefail` is missing on both ASH scan steps
`.github/workflows/ash-full-repository-scan.yml` and `.github/workflows/ash-security-scan.yml`
both pipe the scan into `tee`:
```yaml
run: ash --mode container 2>&1 | tee ash-output.log
```
In a pipeline the shell reports the exit status of the **last** command — `tee`, which
essentially always succeeds. ASH's non-zero exit on actionable findings is discarded.
This is not covered by GitHub's defaults. `set -eo pipefail` is applied only when
`shell: bash` is *explicitly* specified; it is **not** on by default for `run:` steps
([reference](https://stackoverflow.com/questions/75419587/does-a-github-action-step-use-set-e-semantics-by-default)).
No workflow in the repo sets `pipefail` or an explicit shell.
Fix is either `set -o pipefail` at the top of the `run:` block, or adding `shell: bash` to the
step.
### The full-repository scan also has `continue-on-error: true`
`.github/workflows/ash-full-repository-scan.yml`:
```yaml
run: ash --mode container 2>&1 | tee ash-output.log
continue-on-error: true
```
So even with `pipefail` fixed, that job could not block. Between the two issues, past green
runs of these scans prove less than they appear to.
**If advisory-only reporting is the intent here, that's a legitimate choice** — in which case
the ask is just a comment saying so, since right now it reads as a gate. This is the one change
most likely to newly-fail builds, so it deserves an explicit decision rather than a silent fix.
---
## 2. Supply chain
### `tj-actions/changed-files` is referenced by mutable tag
Four workflows use `tj-actions/changed-files@v46`. A tag on this action was compromised in
March 2025 (CVE-2025-30066) to exfiltrate CI secrets. Mutable tags on third-party actions are
re-pointable by definition, so the reference should be an immutable commit SHA.
Sixteen action references across the six workflows are on moving tags in total:
| Action | Current | Suggested pin | Version |
|---|---|---|---|
| `tj-actions/changed-files` | `v46` | `9426d40962ed5378910ee2e21d5f8c6fcbf2dd96` | v47.0.6 |
| `actions/checkout` | `v4` | `11d5960a326750d5838078e36cf38b85af677262` | v4.4.0 |
| `actions/setup-python` | `v5` | `a26af69be951a213d495a4c3e4e4022e16d87065` | v5.6.0 |
| `actions/upload-artifact` | `v4` | `ea165f8d65b6e75b540449e92b4886f43607fa02` | v4.6.2 |
| `astral-sh/setup-uv` | `v3` | `caf0cab7a618c569241d31dcd442f54681755d39` | v3.2.4 |
| `aws-actions/setup-sam` | `v2` | `f84ec7d548307efafe33230528756de3c5841a17` | v2 |
All SHAs resolved through the GitHub API rather than copied from a third party.
`aws-actions/setup-sam` publishes only major tags, so `v2` is the most specific label available.
Pinning does not freeze upgrades: a trailing `# v4.4.0` comment is the convention Dependabot
reads, so it can still propose bumps and update the SHA and comment together.
Worth noting this is self-reinforcing: ASH's own changed-files scan flags
`github-actions-mutable-action-tag` on these files, so any PR that touches a workflow currently
inherits 16 findings (32 rows — `opengrep` and `semgrep` both carry the rule).
### Changed filenames are interpolated into a shell
`.github/workflows/python-lint.yml` passes filenames straight into `run:`:
```yaml
uv tool run ruff check --output-format=github ${{ steps.changed-files.outputs.all_changed_files }}
```
A fork PR controls its own filenames, which makes this a command-injection sink. Passing the
value through `env:` and referencing `${CHANGED_FILES}` avoids the expansion.
### `python-tests.yml` has no `permissions:` block
Five of the seven workflows declare `permissions: contents: read`; `python-tests.yml` does not,
so the job receives an implicit write-all `GITHUB_TOKEN` (checkov `CKV2_GHA_1`). The workflow
only reads the repo and uploads an artifact.
This one is also fixable **without editing any workflow**, via
Settings → Actions → General → Workflow permissions → *Read repository contents*, which would
set a safer default repo-wide.
---
## 3. A dependency pin that silently does nothing
`.github/workflows/python-tests.yml`:
```yaml
pip install pydantic>=2.0.0
```
Unquoted, `>=` is a shell redirect. This writes an empty file named `=2.0.0` into the working
directory and installs pydantic **completely unconstrained** — the opposite of the intent.
Quoting it fixes it.
(The matching `tests/requirements.txt` bound is a repository file rather than a workflow, so
that half is included in #54.)
---
## 4. Reproducibility and coverage
- **`ruff` and `cfn-lint` are installed unpinned** (`uv tool install ruff`,
`pip install cfn-lint`), so an upstream release turns into a red build on an unrelated PR.
- **Both SAM templates are validated but only one is built.** `sam-validate.yml` builds
`template.yaml` only, so a break affecting `template-multi-account.yaml` could ship
undetected.
- **`python-tests.yml`'s path filter omits files the suite asserts against** — both templates,
the state machine, `deployment/`, `buildspec.yml`, `docs/`. A change to any of them can break
tests without running them. The `push` and `pull_request` lists are also duplicated and can
drift.
### Related: the lint gate is already broken on `main`
Separate from pinning. The repo has no ruff configuration, and `python-lint.yml` installs ruff
unpinned. Ruff's default rule set has widened between releases — `UP006`, `RUF010`, `BLE001`,
`G201`, `I001` and others are now default-on — and a current release reports **935 findings
repo-wide** against `main`. Any PR touching a `.py` file fails on rules unrelated to its change.
#54 adds a `ruff.toml` with `select = ["E4", "E7", "E9", "F"]`, restoring the rule set this
codebase was written to. On the twelve Python files that PR changes:
```
ruff check --isolated <12 files> -> Found 232 errors.
ruff check <12 files> -> All checks passed!
```
So this part is already addressed there and needs nothing here. Flagging it because pinning the
ruff *binary* without that config would lock in the broad default rule set — the two changes
want to land in that order.
---
## 5. A drift gap that let a stale contract through
Not a workflow issue, but found by the same review and worth recording.
`app.py` names `docs/SECURITY_CHECKS_FINSERV_SEVERITY_REGISTER.md` as the source of truth its
`SEVERITY_REGISTER` derives from. `test_severity_register.py` guards code↔code parity — every
finding name the registry emits exists in `SEVERITY_REGISTER` with a matching severity — but it
never parses that markdown file. So the doc and the code can diverge invisibly, and had: three
FS-08 finding names in the register were stale and a fourth row was missing entirely (fixed in
#54).
A parity test that reads the register markdown would close this.
Related and broader: comparing each check's documented `Detection` row against the AWS APIs its
implementation actually reaches finds **16 checks naming an API that appears nowhere in the code
path**. FS-08 and FS-67 are fixed in #54; the rest are listed in
https://github.com/aws-samples/sample-aiml-security-assessment/pull/54#issuecomment-5268463550
and are probably worth their own issue once someone decides, per check, whether the doc should
narrow or the check should widen.
Contributor guide
Research direction
Start by reviewing the six workflows under .github/workflows/, especially ash-full-repository-scan.yml, ash-security-scan.yml, python-lint.yml, python-tests.yml, and sam-validate.yml. Compare the reported findings with the existing checks and the patch on ci/harden-release-pipeline. Done means the maintainer-approved subset is applied, scans fail when intended, workflow inputs and dependencies are safer, and the relevant checks remain green.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- aws, github-actions, python, shell
- Domain
- ci-cd, devops, security
- Issue type
- Bug
- Difficulty
- 4/5
- Estimated time
- 3-5 days
- Activity status
- Stale
- Clarity
- Mostly clear
- Newbie friendliness
- 35/100