Skip to content

CI: suppress shell:S4830 false positive and scope GHA permissions to job level#1229

Merged
rapids-bot[bot] merged 2 commits into
mainfrom
fix/sonar-tls-suppress-self-hosted-test
May 18, 2026
Merged

CI: suppress shell:S4830 false positive and scope GHA permissions to job level#1229
rapids-bot[bot] merged 2 commits into
mainfrom
fix/sonar-tls-suppress-self-hosted-test

Conversation

@rgsl888prabhu
Copy link
Copy Markdown
Collaborator

@rgsl888prabhu rgsl888prabhu commented May 15, 2026

Summary

Closes 6 SonarQube findings on main — 1 CRITICAL VULN and 5 MAJOR VULNs — without changing runtime behavior.

ci/test_self_hosted_service.sh — suppress shell:S4830

The curl -k on the health probe is intentional: this same script generates a self-signed cert and starts the cuOpt server two lines earlier, so there is no real TLS endpoint to validate against (subsequent cuopt_sh calls do validate using the test CA in $CLIENT_CERT). Added a # NOSONAR marker with rationale.

.github/workflows/build_test_publish_images.yaml — job-scoped permissions

Moved the workflow-level permissions: block to per-job blocks (rule githubactions:S8264/S8233). After reading the two reusable workflows (build_images.yaml, test_images.yaml), every job only does actions/checkout + DockerHub/NGC logins via username/password secrets — no OIDC, no GHCR pull, no artifact download, no PR API. So each job is reduced to contents: read only, dropping unused actions: read, id-token: write, packages: read, pull-requests: read.

rgsl888prabhu and others added 2 commits May 15, 2026 11:29
The `curl -k` on the health-check is intentional: this same script
generates the self-signed cert and starts the server two lines earlier,
and the test CA is used to validate subsequent `cuopt_sh` calls. There
is no real TLS endpoint to trust against.

Marks the line with `# NOSONAR` and a rationale so the rule stops
firing on every nightly scan. Clears the remaining CRITICAL VULN on
`main` (rule shell:S4830).

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…4/S8233)

Move the `permissions:` block from workflow level to per-job level.
Every job in this chain (and in the two reusable workflows it calls,
build_images.yaml and test_images.yaml) only performs a checkout plus
DockerHub/NGC logins via username/password secrets — there is no OIDC,
no GHCR pull, no artifact download, no PR API usage.

So each job is reduced to `contents: read` only, dropping the unused
workflow-level grants of `actions: read`, `id-token: write`,
`packages: read`, and `pull-requests: read`.

Clears 5 MAJOR vulnerabilities on `main`:
- 4× githubactions:S8264 (read perms at workflow level)
- 1× githubactions:S8233 (write perm at workflow level)

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@rgsl888prabhu rgsl888prabhu requested a review from a team as a code owner May 15, 2026 17:23
@rgsl888prabhu rgsl888prabhu requested a review from msarahan May 15, 2026 17:23
@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented May 15, 2026

Review Change Stack

📝 Walkthrough

Walkthrough

This PR tightens GitHub Actions workflow permissions by replacing a broad top-level permissions declaration with minimal per-job contents: read grants, and adds an inline comment documenting CI certificate generation in a test script. Both changes are configuration and documentation updates with no functional logic modifications.

Changes

CI Configuration and Documentation

Layer / File(s) Summary
GitHub Actions job-level permissions hardening
.github/workflows/build_test_publish_images.yaml
Removes the broad top-level permissions block and adds explicit job-level permissions: contents: read to compute-matrix, build-images, build-cuopt-multiarch-manifest, and test-images jobs, establishing least-privilege token scopes.
Self-hosted service test documentation
ci/test_self_hosted_service.sh
Appends an inline comment to the server_status curl health-check command documenting that the self-signed certificate is generated locally for CI.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~8 minutes

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Title check ✅ Passed The title accurately and concisely summarizes the two main changes: suppressing a shell linting false positive and scoping GitHub Actions permissions to job level.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Description check ✅ Passed The pull request description is directly related to the changeset, clearly explaining the rationale for both modified files and the SonarQube findings being addressed.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix/sonar-tls-suppress-self-hosted-test

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Copy Markdown

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick comments (1)
ci/test_self_hosted_service.sh (1)

82-82: 💤 Low value

Minor inaccuracy in NOSONAR rationale.

The comment states the cert is "generated locally by this script," but lines 71-74 show the certificate files are loaded from python/cuopt_self_hosted/cuopt_sh_client/tests/utils/certs/ rather than being generated at runtime. Consider clarifying:

-server_status=$(curl -k -sL https://0.0.0.0:$CUOPT_SERVER_PORT/cuopt/health) # NOSONAR — self-signed cert generated locally by this script for CI; not a real TLS endpoint.
+server_status=$(curl -k -sL https://0.0.0.0:$CUOPT_SERVER_PORT/cuopt/health) # NOSONAR — self-signed test cert from repo's test fixtures; not a real TLS endpoint.

The suppression itself is appropriate since curl -k is intentional for CI health checks against a local server using test certificates.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@ci/test_self_hosted_service.sh` at line 82, Update the inline NOSONAR comment
on the curl invocation that sets server_status to accurately reflect that the
script uses pre-generated test certificates from
python/cuopt_self_hosted/cuopt_sh_client/tests/utils/certs/ rather than
generating them at runtime; keep the curl -k suppression and NOSONAR rationale
but change the wording around certificate origin (refer to the server_status
curl line) so it correctly documents the source of the test certs.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Nitpick comments:
In `@ci/test_self_hosted_service.sh`:
- Line 82: Update the inline NOSONAR comment on the curl invocation that sets
server_status to accurately reflect that the script uses pre-generated test
certificates from python/cuopt_self_hosted/cuopt_sh_client/tests/utils/certs/
rather than generating them at runtime; keep the curl -k suppression and NOSONAR
rationale but change the wording around certificate origin (refer to the
server_status curl line) so it correctly documents the source of the test certs.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Enterprise

Run ID: 7889d481-adde-4d59-9d66-1cabccb71220

📥 Commits

Reviewing files that changed from the base of the PR and between 72ba054 and ba4554f.

📒 Files selected for processing (2)
  • .github/workflows/build_test_publish_images.yaml
  • ci/test_self_hosted_service.sh

@rgsl888prabhu rgsl888prabhu self-assigned this May 15, 2026
@rgsl888prabhu rgsl888prabhu added non-breaking Introduces a non-breaking change improvement Improves an existing functionality labels May 15, 2026
@rgsl888prabhu
Copy link
Copy Markdown
Collaborator Author

/merge

@rapids-bot rapids-bot Bot merged commit 6f99a42 into main May 18, 2026
209 of 211 checks passed
chris-maes pushed a commit to chris-maes/cuopt that referenced this pull request May 18, 2026
…job level (NVIDIA#1229)

## Summary

Closes **6 SonarQube findings** on `main` — 1 CRITICAL VULN and 5 MAJOR VULNs — without changing runtime behavior.

### `ci/test_self_hosted_service.sh` — suppress `shell:S4830`
The `curl -k` on the health probe is intentional: this same script generates a self-signed cert and starts the cuOpt server two lines earlier, so there is no real TLS endpoint to validate against (subsequent `cuopt_sh` calls *do* validate using the test CA in `$CLIENT_CERT`). Added a `# NOSONAR` marker with rationale.

### `.github/workflows/build_test_publish_images.yaml` — job-scoped permissions
Moved the workflow-level `permissions:` block to per-job blocks (rule `githubactions:S8264`/`S8233`). After reading the two reusable workflows (`build_images.yaml`, `test_images.yaml`), every job only does `actions/checkout` + DockerHub/NGC logins via username/password secrets — no OIDC, no GHCR pull, no artifact download, no PR API. So each job is reduced to `contents: read` only, dropping unused `actions: read`, `id-token: write`, `packages: read`, `pull-requests: read`.

Authors:
  - Ramakrishnap (https://github.com/rgsl888prabhu)

Approvers:
  - Bradley Dice (https://github.com/bdice)

URL: NVIDIA#1229
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

improvement Improves an existing functionality non-breaking Introduces a non-breaking change

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants