security: remove old implementation of gitleaks and add all scanners by cgoea · Pull Request #7804 · ROCm/TheRock

cgoea · 2026-09-01T13:18:20Z

Motivation

Add single entry point for all the security scanners:
*gitleaks
*bandit
*zizmor
*trivy
*codeql

and their specific config files

JIRA ID: ROCM-28051

Technical Details

Security governance is maintained from https://github.com/ROCm/rocm-security-gh

Submission Checklist

Look over the contributing guidelines at https://github.com/ROCm/TheRock/blob/main/CONTRIBUTING.md.

…scanners *gitleaks *bandit *zizmor *trivy

therock-pr-bot · 2026-09-01T13:18:55Z

✅ All Checks Passed — Ready for Review

Check	Status	Details
📝 PR Description	✅ Pass	—
⛔ Forbidden Files	✅ Pass	—
🧪 Unit Test	✅ Pass	—
🔎 pre-commit	✅ Pass	—
🚫 Draft PR	🔜 To Be Enabled	—
🚩 Feature Flag	🔜 To Be Enabled	—
📊 Code Coverage	🔜 To Be Enabled	—
🤖 therock-pr-bot	✅ Pass	—

🎉 All checks passed! This PR is ready for review.

📖 Need help? See the Policy FAQ for details on every check and how to fix failures.

🙋 Wish to Override Policy?

therock-pr-bot · 2026-09-01T13:19:28Z

Pre-commit check failed

⛔ pre-commit failed

Please run locally:

python -m pip install pre-commit
pre-commit install
pre-commit run --all-files --show-diff-on-failure

This repo uses .pre-commit-config.yaml.

ScottTodd

Nice, this looks quite useful. Let's figure out how to get the checks to appear "passing" before we enable on all pull requests though.

ScottTodd · 2026-09-01T16:13:33Z

+on:
+  pull_request:
+
+permissions:
+  contents: read
+
+jobs:
+  security:
+    name: Security scan
+    uses: ROCm/rocm-security-gh/.github/workflows/security-baseline.yml@4b25e6bc1cb0b9bbc3c278b2f1c70d8f2f406e20 # v1.0.0
+    with:
+      report_formats: human


This has failures: https://github.com/ROCm/TheRock/actions/runs/33512746901?pr=7804

Before we enable on all pull_request runs, let's fix / allow-list / suppress preexisting failures so open PRs will only show new findings, with a path to either fix or add to those allow-lists.

Looking through the findings...

zizmor:

good findings for how to improve the security of our github actions workflows

https://docs.zizmor.sh/audits/#template-injection looks easy to implement across our workflows (replacing ${{ with env:)

https://docs.zizmor.sh/audits/#artipacked also easy, add persist-credentials: false to actions/checkout calls

https://docs.zizmor.sh/audits/#excessive-permissions case by case

https://docs.zizmor.sh/audits/#secrets-inherit case by case

bandit:

analysis of python scripts... the signal to noise ratio is particularly poor here

untrusted input with subprocess calls is very broad and flagged many cases. Needs a triage pass to prioritize. Issue: [B404:blacklist] Consider possible security implications associated with the subprocess module. is quite strict.

many false positives like
>> Issue: [B105:hardcoded_password_string] Possible hardcoded password: 'systems' Severity: Low Confidence: Medium CWE: CWE-259 (https://cwe.mitre.org/data/definitions/259.html) More Info: https://bandit.readthedocs.io/en/1.9.4/plugins/b105_hardcoded_password_string.html Location: .scan-target/build_tools/github_actions/bump_automation.py:40:8 39 "updater": "ref", 40 "token_key": "systems",

>> Issue: [B105:hardcoded_password_string] Possible hardcoded password: 'test-token-12345' Severity: Low Confidence: Medium CWE: CWE-259 (https://cwe.mitre.org/data/definitions/259.html) More Info: https://bandit.readthedocs.io/en/1.9.4/plugins/b105_hardcoded_password_string.html Location: .scan-target/build_tools/github_actions/tests/github_actions_api_test.py:110:19 109 """GITHUB_TOKEN should be used when available, even if gh CLI is present.""" 110 os.environ["GITHUB_TOKEN"] = "test-token-12345" 111

>> Issue: [B101:assert_used] Use of assert detected. The enclosed code will be removed when compiling to optimised byte code. Severity: Low Confidence: High CWE: CWE-703 (https://cwe.mitre.org/data/definitions/703.html) More Info: https://bandit.readthedocs.io/en/1.9.4/plugins/b101_assert_used.html Location: .scan-target/build_tools/github_actions/tests/workflow_summary_test.py:68:8 67 assert len(jobs) == 3 68 assert all(j.result == "success" for j in jobs) 69 assert all(not j.continue_on_error for j in jobs)

trivy:

reasonable suggestions here for how to improve our dockerfiles

Before we enable on all pull_request runs, let's fix / allow-list / suppress preexisting failures so open PRs will only show new findings, with a path to either fix or add to those allow-lists.

https://github.com/ROCm/TheRock/actions/runs/34583395292/job/103211993919?pr=7804#step:6:22

INFO Changed config (build_tools/scan_tools/trivy.yml) applies to every file, so this run scans the whole tree instead of the changed files alone

Looks like this is already going to decide to filter to just new code on most pull requests... or, at least that's what it advertises, but https://github.com/ROCm/rocm-security-gh/blob/main/security_scanners/github_actions/trivy.py says

- Derive change sets from the GitHub event for changed/all scans; in 'changed' mode short-circuit to a no-op unless a dependency manifest / IaC / container file changed under `--source-dir` (trivy's filesystem scanner needs the whole subtree to resolve transitive deps and cross-file IaC references, so -- unlike bandit/zizmor -- individual changed files are never fed to it directly: we either scan the whole subtree or nothing).

So "changed" doesn't work there, and PRs would show preexisting failures.

Could you run some tests showing what to expect for output on some typical PRs?

I've started preparing some commits/PRs for the zizmor findings, fyi.

#8275 for zizmor template-injection findings

ScottTodd · 2026-09-01T16:26:18Z

+exclude_dirs:
+  - .git
+  - .venv
+  - venv


We might want to exclude test directories, there were many false positives for tests.

ScottTodd · 2026-09-01T16:28:57Z

Does this file need to be in the repository root? Can we add to a subdirectory instead? Or use a dotfile at least? I want to limit how many files we put at the top level whenever possible.

Looks like https://bandit.readthedocs.io/en/latest/config.html supports .bandit in the root or a *.yaml file anywhere, together with the -c option (which we could forward to the security scan workflow(s))

Suggestions:

https://github.com/ROCm/TheRock/tree/main/build_tools/scan_tools (which this PR deletes)

security_tools/ new directory

.github/security_tools/ if these are limited to only running on github actions - it does look like they support running locally though

sure, I've added a parameter for each and moved them.

ScottTodd · 2026-09-01T16:29:28Z

Same comment about file location, https://trivy.dev/docs/latest/references/configuration/config-file/ supports custom locations with --config

This was my initial idea, but got some pushback regarding configs being anywhere else other than the root of the repo. I could make them configurable though.

ScottTodd · 2026-09-01T16:30:00Z

Same comment about file location, https://docs.zizmor.sh/configuration/ supports custom locations with --config

ScottTodd · 2026-09-01T16:34:33Z

+# trivy configuration for Quartz, picked up by the rocm-security-gh
+# reusable scanner (.github/workflows/security_scan_pr.yml and
+# security_scan_weekly.yml) in place of its own org-wide default.
+# Reference: https://trivy.dev/latest/docs/references/configuration/config-file/


This can be a follow-up, but let's document these security tools, including how to run them locally.

Docs which could be updated:

https://github.com/ROCm/TheRock/blob/main/SECURITY.md (what existing scanning / reporting we have, how individual reports fit into that story)

https://github.com/ROCm/TheRock/blob/main/CONTRIBUTING.md (how to run the scanners)

https://github.com/ROCm/TheRock/blob/main/TESTING.md (add details for how we test the security of our code, not just the correctness)

therock-pr-bot · 2026-09-02T06:37:05Z

🎉 All checks passed! This PR is ready for review.

…Cm/TheRock into users/cgoea/security_scanners

…scanners # Conflicts: # .github/workflows/gitleaks.yml

github-advanced-security · 2026-09-10T09:34:17Z

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:

The 'Security' tab will display more code scanning analysis results (e.g., for the default branch).
Depending on your configuration and choice of analysis tool, future pull requests will be annotated with code scanning analysis results.
You will be able to see the analysis results for the pull request's branch on this overview once the scans have completed and the checks have passed.

For more information about GitHub Code Scanning, check out the documentation.

Mirrors ROCm/TheRock#7804's docs enhancement: add an "Automated security scanning" section to SECURITY.md (what each scanner looks for, and how the PR-time vs weekly workflows differ) and a "Security scanners" section to CONTRIBUTING.md (local run commands against the same config CI uses). Answers HereThereBeDragons' review comment asking for more detail on how often the scanners run and on what.

marbre

Something is off here.

marbre · 2026-09-14T12:15:21Z

This seems unrelated. Something broken here?

I'm not seeing changes to that file in the full diff. Was that just from a merge commit in the middle?

Also not seeing this in the diff.

marbre · 2026-09-14T12:15:58Z

+      rebuild_artifacts:
+        description: "Comma-separated artifact names to build (granular reuse). Empty = build all artifacts in stage."
+        type: string
+        default: ""


Feels like you branched of the wrong or an outdated branch and this would re-introduce old changes.

Ok, I'll sync this to the head of main

@marbre let me know if you can still see those files.

haribabug

LGTM

ScottTodd

Thanks, LGTM. After merge let's monitor pull requests for false positives and and follow-up on the preexisting scanner findings as part of #8272.

marbre

Not seeing the diff race whatever that was :)

cgoea added 2 commits September 1, 2026 11:26

remove old implementation of gitleaks and add new single call to all …

c09a9e7

…scanners *gitleaks *bandit *zizmor *trivy

bump security scanners commit id and add config files

15d0231

github-project-automation Bot added this to TheRock Triage Sep 1, 2026

github-project-automation Bot moved this to TODO in TheRock Triage Sep 1, 2026

cgoea requested review from HereThereBeDragons, ScottTodd and marbre September 1, 2026 13:18

fix typo in config file

394dd68

ScottTodd reviewed Sep 1, 2026

View reviewed changes

fix precommit missing new line

8768f65

cgoea added 3 commits September 2, 2026 15:21

bandit should skip tests

0f16de1

Merge branch 'users/cgoea/security_scanners' of https://github.com/RO…

7a98e87

…Cm/TheRock into users/cgoea/security_scanners

move scanner configs from the repo root

2937855

cgoea requested a review from ScottTodd September 9, 2026 11:49

Merge remote-tracking branch 'origin/main' into users/cgoea/security_…

f488306

…scanners # Conflicts: # .github/workflows/gitleaks.yml

cgoea added the ci:skip Skip all CI builds/tests for this PR label Sep 9, 2026

cgoea added 5 commits September 9, 2026 15:34

remove local codeql configs. sec scanners enable it

fc5a8fe

bump sec scan commit id

04d4651

allign config extensions

41bffc8

add concurrency group

2198ab8

add docs for scanners and how to's

eaf11cf

bump security-baseline to latest version

abe4019

marbre requested changes Sep 14, 2026

View reviewed changes

ScottTodd mentioned this pull request Sep 14, 2026

ci(jax): serve the JAX wheel build from the EngFlow Bazel cache #7912

Open

1 task done

haribabug approved these changes Sep 16, 2026

View reviewed changes

Merge branch 'main' into users/cgoea/security_scanners

d4bb8a1

cgoea requested a review from marbre September 16, 2026 16:27

ScottTodd mentioned this pull request Sep 16, 2026

Resolve or suppress issues detected by security scanners #8272

Closed

ScottTodd approved these changes Sep 17, 2026

View reviewed changes

marbre approved these changes Sep 18, 2026

View reviewed changes

cgoea merged commit f606a02 into main Sep 18, 2026
21 of 24 checks passed

cgoea deleted the users/cgoea/security_scanners branch September 18, 2026 14:13

github-project-automation Bot moved this from TODO to Done in TheRock Triage Sep 18, 2026

Conversation

cgoea commented Sep 1, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Motivation

Technical Details

Submission Checklist

Uh oh!

therock-pr-bot Bot commented Sep 1, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

✅ All Checks Passed — Ready for Review

Uh oh!

therock-pr-bot Bot commented Sep 1, 2026

Pre-commit check failed

Uh oh!

ScottTodd left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

therock-pr-bot Bot commented Sep 2, 2026

Uh oh!

github-advanced-security AI commented Sep 10, 2026

What Enabling Code Scanning Means:

Uh oh!

marbre left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

haribabug left a comment

Choose a reason for hiding this comment

Uh oh!

ScottTodd left a comment

Choose a reason for hiding this comment

Uh oh!

marbre left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

cgoea commented Sep 1, 2026 •

edited

Loading

therock-pr-bot Bot commented Sep 1, 2026 •

edited

Loading