API caller for AquaSec full repository scan #89
Conversation
tmikula-dev
requested review from
ABMC831,
Zejnilovic,
oto-macenauer-absa and
petr-pokorny-absa
as code owners
tmikula-dev
added
enhancement
WalkthroughReplaces repository Trivy scanning with a new AquaSec API-based full-repository scan workflow, removes two Trivy workflows and related Trivy config/rules, and pins several GitHub Actions to exact commit SHAs in existing CI workflows. Changes
Sequence Diagram(s)sequenceDiagram
participant GHA as GitHub Actions
participant Aqua as AquaSec API
participant Security as GitHub Security
participant PR as Pull Request
rect rgb(230,240,255)
Note over GHA,Aqua: Auth & Retrieval
GHA->>GHA: Validate secrets & repo UUID
GHA->>Aqua: POST HMAC-signed auth request
Aqua-->>GHA: Bearer token
loop paginate
GHA->>Aqua: GET scan results (page N)
Aqua-->>GHA: Results page
GHA->>GHA: Append to aquasec_scan_results.json
end
end
rect rgb(230,255,230)
Note over GHA,Security: Convert & Upload
GHA->>GHA: Map severities & build SARIF rules
GHA->>GHA: Emit aquasec_scan.sarif
GHA->>Security: Upload SARIF
Security-->>GHA: Upload response
end
rect rgb(255,245,230)
Note over GHA,PR: Summarize & Notify
GHA->>GHA: Generate Markdown summary, set GITHUB_OUTPUT
alt event is pull_request
GHA->>PR: Post or update PR comment with summary & overview link
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (2)
.github/workflows/aquasec_repository_scan.yml (2)
33-33: Consider parameterizing hardcoded API endpoints.The AquaSec API endpoints are hardcoded with specific regions (
eu-1andeu-central-1). This limits flexibility if infrastructure changes or different regions are needed. Consider making these configurable via workflow inputs or environment variables, or document the region strategy.Also applies to: 68-68
25-80: Add retry logic for network resilience.The workflow makes multiple HTTP requests (AquaSec auth, GitHub API, AquaSec scan results) without retry logic. Transient network failures will cause the entire workflow to fail. Consider adding exponential backoff retry logic to improve resilience.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
.github/workflows/aquasec_repository_scan.yml(1 hunks).github/workflows/trivy_repository_scan.yml(0 hunks)
💤 Files with no reviewable changes (1)
- .github/workflows/trivy_repository_scan.yml
🔇 Additional comments (1)
.github/workflows/aquasec_repository_scan.yml (1)
8-12: Clarify unusedsecurity-events: writepermission.The workflow declares
security-events: writepermission but doesn't upload SARIF results or use GitHub security events API. Given the PR is marked as WIP, clarify whether:
- SARIF upload to GitHub Security tab is planned?
- GitHub security event integration is part of the roadmap?
- This permission should be removed if not needed?
Once the workflow is complete, align permissions with actual usage.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (3)
.github/workflows/aquasec_repository_scan.yml (3)
66-77: Add HTTP status validation and timeout to AquaSec scan results request.The curl request (lines 69–72) lacks HTTP status validation and a timeout. An error response from AquaSec (e.g., 4xx/5xx) could be parsed as valid results, silently masking failures. Add
--max-time, capture HTTP status, and validate it before processing results.Apply this diff:
echo "=== Receiving AquaSec Scan Results ===" SCAN_RESULTS_ENDPOINT="https://eu-central-1.edge.cloud.aquasec.com/codesec/api/v1/scans/results" - SCAN_RESULTS=$(curl -s -X GET \ + SCAN_RESPONSE=$(curl -s -w "\n%{http_code}" --max-time 10 -X GET \ "$SCAN_RESULTS_ENDPOINT?repositoryIds=$REPO_ID" \ -H "Authorization: Bearer $BEARER_TOKEN" \ -H "Accept: application/json") - if [ -z "$SCAN_RESULTS" ]; then + SCAN_RESULTS=$(echo "$SCAN_RESPONSE" | head -n -1) + HTTP_STATUS=$(echo "$SCAN_RESPONSE" | tail -n 1) + + if [ "$HTTP_STATUS" != "200" ]; then + echo "AquaSec API returned HTTP status $HTTP_STATUS" echo "Failed to retrieve scan results" exit 1 + fi + + if [ -z "$SCAN_RESULTS" ]; then + echo "Scan results body is empty" exit 1 fi
30-55: Mask AQUA_SECRET before using it in shell commands to prevent log exposure.The
AQUA_SECRETis used directly in the OpenSSL HMAC calculation (line 37) without masking. If the step fails, debug logging is enabled, or the openssl command outputs diagnostic information, the secret could be leaked into workflow logs. AlthoughBEARER_TOKENis correctly masked on line 51,AQUA_SECRETshould also be masked immediately upon use.Apply this diff to mask the secret:
echo "=== Authenticating with AquaSec ===" TIMESTAMP=$(date -u +%s) AUTH_ENDPOINT="https://eu-1.api.cloudsploit.com" METHOD="POST" POST_BODY='{"validity":240,"allowed_endpoints":["GET","POST"]}' STRING_TO_SIGN="${TIMESTAMP}${METHOD}/v2/tokens${POST_BODY}" + echo "::add-mask::$AQUA_SECRET" SIGNATURE=$(echo -n "$STRING_TO_SIGN" | openssl dgst -sha256 -hmac "$AQUA_SECRET" -hex | sed 's/.*= //g')
57-64: Add explicit error handling and timeout to GitHub API call.The curl request on line 59 lacks error handling and a timeout. If the request hangs or fails, the subsequent check for empty/null
REPO_IDmasks the root cause. Additionally, silent curl failures are not detected. Add--max-time, explicit error handling, and curl exit code validation.Apply this diff:
echo "=== Getting Repository ID from GitHub ===" - REPO_ID=$(curl -s "https://api.github.com/repos/${{ github.repository }}" | jq -r '.id') + REPO_ID=$(curl -s --max-time 10 "https://api.github.com/repos/${{ github.repository }}" 2>/dev/null | jq -r '.id' 2>/dev/null) + if [ $? -ne 0 ]; then + echo "Failed to retrieve repository ID from GitHub API" + exit 1 + fi if [ -z "$REPO_ID" ] || [ "$REPO_ID" = "null" ]; then echo "Failed to get repository ID from GitHub" exit 1 fi
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (4)
.github/workflows/aquasec_repository_scan.yml (4)
44-45: Mask AQUA_SECRET before use in HMAC computation.
AQUA_SECRETis used directly in the openssl command without masking. If the step fails or debug logging is enabled, the secret could be exposed in logs.Apply this diff to mask the secret immediately:
STRING_TO_SIGN="${TIMESTAMP}${METHOD}/v2/tokens${POST_BODY}" + echo "::add-mask::$AQUA_SECRET" SIGNATURE=$(echo -n "$STRING_TO_SIGN" | openssl dgst -sha256 -hmac "$AQUA_SECRET" -hex | sed 's/.*= //g')
55-64: Validate jq exit code after parsing auth response.Line 55 assumes jq successfully parsed the response. If the response is malformed or jq fails,
RESPONSE_STATUSmay be empty or contain an error, and the subsequent check on line 57 will silently fail.Apply this diff:
RESPONSE_STATUS=$(echo "$AUTH_RESPONSE" | jq -r '.status' 2>/dev/null) + if [ $? -ne 0 ]; then + echo "Failed to parse AquaSec authentication response" + exit 1 + fi if [ "$RESPONSE_STATUS" = "200" ]; then
79-80: Export scan results to GITHUB_OUTPUT for downstream job consumption.The workflow outputs results to stdout but does not emit them as a GitHub Actions workflow output. To allow downstream jobs to consume the scan results, append them to
GITHUB_OUTPUTusing a heredoc pattern to preserve newlines and special characters.Apply this diff:
echo "=== Scan Results ===" echo "$SCAN_RESULTS" | jq '.' + echo "scan_results<<EOF" >> $GITHUB_OUTPUT + echo "$SCAN_RESULTS" >> $GITHUB_OUTPUT + echo "EOF" >> $GITHUB_OUTPUT
69-72: Fix malformed curl URL syntax; close quote before line continuation.Line 70 has a syntax error: the URL string is not properly terminated before the backslash continuation. The shell will fail to parse this command.
Additionally, no HTTP status code validation is performed, so 4xx/5xx error responses from AquaSec will be treated as valid results.
Apply this diff to fix the syntax and add HTTP status validation:
- SCAN_RESULTS=$(curl -s -X GET \ + SCAN_RESPONSE=$(curl -s -w "\n%{http_code}" --max-time 10 -X GET \ - "$SCAN_RESULTS_ENDPOINT?repositoryIds=${{ github.repository_id }} \ + "$SCAN_RESULTS_ENDPOINT?repositoryIds=${{ github.repository_id }}" \ -H "Authorization: Bearer $BEARER_TOKEN" \ -H "Accept: application/json") + + SCAN_RESULTS=$(echo "$SCAN_RESPONSE" | head -n -1) + HTTP_STATUS=$(echo "$SCAN_RESPONSE" | tail -n 1) + + if [ "$HTTP_STATUS" != "200" ]; then + echo "AquaSec API returned status $HTTP_STATUS" + echo "Failed to retrieve scan results" + exit 1 + fi
🧹 Nitpick comments (1)
.github/workflows/aquasec_repository_scan.yml (1)
74-77: Add explicit check for empty scan results after HTTP validation.Once HTTP status validation is added (see previous comment), add an explicit empty-content check as a secondary safety net.
Apply this diff after HTTP validation:
+ if [ -z "$SCAN_RESULTS" ]; then + echo "Scan results are empty" + exit 1 + fi
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (4)
.github/workflows/aquasec_repository_scan.yml (4)
47-52: Add timeout to AquaSec authentication curl request.The curl call lacks a timeout. If the AquaSec endpoint is unresponsive, the step will block indefinitely. Add
--max-time 10to ensure the request fails after 10 seconds.Apply this diff:
- AUTH_RESPONSE=$(curl -s -X "$METHOD" "$AUTH_ENDPOINT" \ + AUTH_RESPONSE=$(curl -s --max-time 10 -X "$METHOD" "$AUTH_ENDPOINT" \ -H "Content-Type: application/json" \ -H "X-API-Key: $AQUA_KEY" \ -H "X-Signature: $SIGNATURE" \ -H "X-Timestamp: $TIMESTAMP" \ -d "$POST_BODY")
78-79: Export scan results via GITHUB_OUTPUT for downstream job access.The workflow outputs scan results to stdout but does not export them via
GITHUB_OUTPUT. As described in the PR objectives, results should be emitted so downstream jobs can consume them. Add a multi-line output block.Apply this diff:
echo "=== Scan Results ===" echo "$SCAN_RESULTS" | jq '.' + echo "scan_results<<EOF" >> $GITHUB_OUTPUT + echo "$SCAN_RESULTS" >> $GITHUB_OUTPUT + echo "EOF" >> $GITHUB_OUTPUT
32-44: Mask AQUA_SECRET before use in HMAC calculation.The
AQUA_SECRETis used directly in the openssl command (line 44) without masking. If the step fails or debug logging is enabled, the secret could be exposed in workflow logs. Mask it immediately before the HMAC calculation.Apply this diff to mask the secret:
STRING_TO_SIGN="${TIMESTAMP}${METHOD}/v2/tokens${POST_BODY}" + echo "::add-mask::$AQUA_SECRET" SIGNATURE=$(echo -n "$STRING_TO_SIGN" | openssl dgst -sha256 -hmac "$AQUA_SECRET" -hex | sed 's/.*= //g')
68-71: Fix syntax error in curl URL and add timeout and HTTP status validation.Syntax Error: Line 69 is missing a closing quote after the URL parameter. The URL will be malformed, causing curl to fail.
Reliability Issues: The curl request lacks timeout and HTTP status validation. If AquaSec returns an error (4xx/5xx), it will be treated as valid results, potentially masking failures.
Apply this diff to fix the syntax error and add timeout/status validation:
SCAN_RESULTS_ENDPOINT="https://eu-central-1.edge.cloud.aquasec.com/codesec/api/v1/scans/results" - SCAN_RESULTS=$(curl -s -X GET \ - "$SCAN_RESULTS_ENDPOINT?repositoryIds=${{ github.repository_id }} \ + SCAN_RESPONSE=$(curl -s -w "\n%{http_code}" --max-time 10 -X GET \ + "$SCAN_RESULTS_ENDPOINT?repositoryIds=${{ github.repository_id }}" \ -H "Authorization: Bearer $BEARER_TOKEN" \ -H "Accept: application/json") + + SCAN_RESULTS=$(echo "$SCAN_RESPONSE" | head -n -1) + HTTP_STATUS=$(echo "$SCAN_RESPONSE" | tail -n 1) + + if [ "$HTTP_STATUS" != "200" ]; then + echo "AquaSec API returned HTTP status $HTTP_STATUS" + echo "Failed to retrieve scan results" + exit 1 + fi
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.github/workflows/aquasec_repository_scan.yml(1 hunks)
🔇 Additional comments (3)
.github/workflows/aquasec_repository_scan.yml (3)
20-23: LGTM — checkout is properly configured.The upgrade to
actions/checkout@v6addresses the prior review feedback, and disablingpersist-credentialsfollows security best practices.
8-12: Permissions are appropriately scoped.The permissions block correctly enables the actions needed (contents: read for checkout, write permissions for security events, PR comments, etc.) while maintaining least-privilege principles.
1-17: Workflow trigger and job configuration are appropriate.The workflow correctly triggers on both manual dispatch and pull requests, with a clear job name and appropriate runner selection.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (5)
.github/workflows/aquasec_repository_scan.yml (5)
78-79: Export scan results viaGITHUB_OUTPUTfor downstream job access.The workflow echoes scan results to stdout but does not emit them as a GitHub Actions output. Per the PR description, the workflow should "emit the scan JSON via GITHUB_OUTPUT" so downstream jobs can consume it.
Apply this diff to export the results:
echo "=== Scan Results ===" echo "$SCAN_RESULTS" | jq '.' + + # Export as multi-line output to preserve JSON formatting + { + echo "scan_results<<EOF" + echo "$SCAN_RESULTS" + echo "EOF" + } >> $GITHUB_OUTPUT
54-63: Add explicit error handling for jq parsing of authentication response.The jq calls assume the response is valid JSON. If jq fails or fields are missing, errors are silent. Validate that jq parsing succeeds and that extracted values are non-empty before use.
Apply this diff:
RESPONSE_STATUS=$(echo "$AUTH_RESPONSE" | jq -r '.status') + if [ $? -ne 0 ] || [ -z "$RESPONSE_STATUS" ]; then + echo "Failed to parse AquaSec authentication response" + exit 1 + fi if [ "$RESPONSE_STATUS" = "200" ]; then echo "Login successful." BEARER_TOKEN=$(echo "$AUTH_RESPONSE" | jq -r '.data') + if [ $? -ne 0 ] || [ -z "$BEARER_TOKEN" ]; then + echo "Failed to extract bearer token from response" + exit 1 + fi echo "::add-mask::$BEARER_TOKEN" else echo "Login failed" exit 1 fi
44-44: MaskAQUA_SECRETbefore use to prevent exposure in logs.The
AQUA_SECRETis used directly in the HMAC calculation without masking. If the step fails or debug logging is enabled, the secret could be exposed in workflow logs. Mask it immediately before the shell command that references it.Apply this diff:
STRING_TO_SIGN="${TIMESTAMP}${METHOD}/v2/tokens${POST_BODY}" + echo "::add-mask::$AQUA_SECRET" SIGNATURE=$(echo -n "$STRING_TO_SIGN" | openssl dgst -sha256 -hmac "$AQUA_SECRET" -hex | sed 's/.*= //g')
47-52: Add timeout and error handling to AquaSec authentication curl call.The curl request lacks both a timeout (risking indefinite hangs) and explicit error handling. If curl fails or the endpoint is unreachable, the failure is not caught until a downstream check on the token.
Apply this diff to add timeout and error handling:
- AUTH_RESPONSE=$(curl -s -X "$METHOD" "$AUTH_ENDPOINT" \ + AUTH_RESPONSE=$(curl -s --max-time 10 -w "\n%{http_code}" -X "$METHOD" "$AUTH_ENDPOINT" \ -H "Content-Type: application/json" \ -H "X-API-Key: $AQUA_KEY" \ -H "X-Signature: $SIGNATURE" \ -H "X-Timestamp: $TIMESTAMP" \ -d "$POST_BODY") + + HTTP_STATUS=$(echo "$AUTH_RESPONSE" | tail -n 1) + AUTH_RESPONSE=$(echo "$AUTH_RESPONSE" | head -n -1) + + if [ "$HTTP_STATUS" != "200" ]; then + echo "AquaSec authentication failed with HTTP status $HTTP_STATUS" + exit 1 + fi
68-76: Add timeout and HTTP status validation to scan results retrieval.The curl request (after fixing the syntax error above) lacks a timeout and HTTP status validation. If AquaSec returns a 4xx/5xx error, it will be treated as valid results, masking the failure. Additionally, no timeout is configured, risking indefinite waits.
Apply this diff to add timeout and HTTP status validation:
SCAN_RESULTS=$(curl -s -w "\n%{http_code}" --max-time 10 -X GET \ "$SCAN_RESULTS_ENDPOINT?repositoryIds=${{ github.repository_id }}" \ -H "Authorization: Bearer $BEARER_TOKEN" \ -H "Accept: application/json") + SCAN_HTTP_STATUS=$(echo "$SCAN_RESULTS" | tail -n 1) + SCAN_RESULTS=$(echo "$SCAN_RESULTS" | head -n -1) + + if [ "$SCAN_HTTP_STATUS" != "200" ]; then + echo "AquaSec API returned HTTP status $SCAN_HTTP_STATUS" + exit 1 + fi + if [ -z "$SCAN_RESULTS" ]; then echo "Failed to retrieve scan results" exit 1 fi
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (4)
.github/workflows/aquasec_repo_scan.yml (4)
43-43: Secret may be exposed in process listing.Using
${AQUA_SECRET}directly in theopensslcommand line argument could expose the secret in process listings (ps aux). Consider using stdin or environment variable approach.🔎 Proposed fix using stdin
- SIGNATURE=$(echo -n "$STRING_TO_SIGN" | openssl dgst -sha256 -hmac "${AQUA_SECRET}" -hex | sed 's/.*= //g') + SIGNATURE=$(echo -n "$STRING_TO_SIGN" | openssl dgst -sha256 -hmac "$(cat <<< "$AQUA_SECRET")" -hex | sed 's/.*= //g')Alternatively, you could use a file descriptor or heredoc approach to avoid the secret appearing in command arguments.
134-135: Add error handling for file operations.The file read operation lacks error handling. If
aquasec_scan_results.jsondoesn't exist or is malformed, the script will fail with an unclear error message.🔎 Proposed fix
+ import sys + + try: - with open("aquasec_scan_results.json", "r") as f: - data = json.load(f) + with open("aquasec_scan_results.json", "r") as f: + data = json.load(f) + except FileNotFoundError: + print("Error: aquasec_scan_results.json not found", file=sys.stderr) + sys.exit(1) + except json.JSONDecodeError as e: + print(f"Error: Invalid JSON in aquasec_scan_results.json: {e}", file=sys.stderr) + sys.exit(1)
248-248: Consider externalizing the categories list.The hardcoded
CATEGORIESlist must stay in sync with what AquaSec returns. If AquaSec adds new categories, they won't appear in the summary. Consider dynamically discovering categories from the SARIF data, or document this dependency.
54-60: Mask the bearer token to prevent log exposure.The
BEARER_TOKENshould be masked using::add-mask::to prevent it from appearing in workflow logs if any subsequent command inadvertently echoes it.🔎 Proposed fix
if [ "$RESPONSE_STATUS" = "200" ]; then echo "Login successful." BEARER_TOKEN=$(echo "$AUTH_RESPONSE" | jq -r '.data') + echo "::add-mask::$BEARER_TOKEN" else echo "Login failed with error message: $(echo "$AUTH_RESPONSE" | jq -r '.errors')" exit 1 fi
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
.github/workflows/aquasec_repo_scan.yml.gitignore
✅ Files skipped from review due to trivial changes (1)
- .gitignore
🔇 Additional comments (5)
.github/workflows/aquasec_repo_scan.yml (5)
1-16: LGTM!Workflow triggers, permissions, and concurrency configuration are well-defined. The minimal required permissions follow the principle of least privilege.
142-207: LGTM!The SARIF conversion logic is well-implemented:
- Proper severity mapping with fallback
- Rule deduplication using
avd_id- Line number sanitization to meet SARIF schema requirements
- Fingerprints for cross-scan deduplication
231-235: LGTM!Standard usage of the CodeQL SARIF upload action with an appropriate category identifier.
251-264: Good error handling pattern.The try-except block properly handles file I/O errors and JSON parsing issues with clear error messages to stderr.
23-27: [No action needed] —actions/checkout@v6is the current latest version and is correctly specified in the workflow.Likely an incorrect or invalid review comment.
|
This pull request sets up GitHub code scanning for this repository. Once the scans have completed and the checks have passed, the analysis results for this pull request branch will appear on this overview. Once you merge this pull request, the 'Security' tab will show more code scanning analysis results (for example, for the default branch). Depending on your configuration and choice of analysis tool, future pull requests will be annotated with code scanning analysis results. For more information about GitHub code scanning, check out the documentation. |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
.github/workflows/aquasec_repo_scan.yml (4)
53-68: Consider adding--fail-with-bodyfor better HTTP error handling.The
curlcommand with--max-time 30doesn't fail on HTTP error responses (4xx/5xx). While the script checks.statusfrom the JSON response, if AquaSec returns a non-JSON error page,jqmay fail or return unexpected results.🔎 Proposed fix
- AUTH_RESPONSE=$(curl -s --max-time 30 -X $METHOD "$AUTH_ENDPOINT" \ + AUTH_RESPONSE=$(curl -s --fail-with-body --max-time 30 -X $METHOD "$AUTH_ENDPOINT" \ -H "Content-Type: application/json" \ -H "X-API-Key: $AQUA_KEY" \ -H "X-Timestamp: $TIMESTAMP" \ -H "X-Signature: $SIGNATURE" \ - -d "$POST_BODY") + -d "$POST_BODY") || { + echo "Error: Authentication request failed" + exit 1 + }
83-90: Add error handling for scan results API call.Similar to the authentication call, the pagination API call should handle HTTP errors and validate the response format before parsing.
🔎 Proposed fix
- PAGE_RESPONSE=$(curl -s --max-time 30 -X GET "$REQUEST_URL" \ + PAGE_RESPONSE=$(curl -s --fail-with-body --max-time 30 -X GET "$REQUEST_URL" \ -H "Authorization: Bearer $BEARER_TOKEN" \ - -H "Accept: application/json") + -H "Accept: application/json") || { + echo "Error: Failed to fetch page $PAGE_NUM" + exit 1 + } if [ -z "$PAGE_RESPONSE" ]; then echo "Failed to retrieve scan results on page $PAGE_NUM" exit 1 fi + + if ! echo "$PAGE_RESPONSE" | jq -e . >/dev/null 2>&1; then + echo "Error: Invalid JSON response on page $PAGE_NUM" + exit 1 + fi
142-148: Add error handling for JSON file operations.The file read operation lacks exception handling. If
aquasec_scan_results.jsonis missing or contains invalid JSON, the script will fail with an unclear error message.🔎 Proposed fix
+ import sys + + try: - with open("aquasec_scan_results.json", "r") as f: - data = json.load(f) + with open("aquasec_scan_results.json", "r", encoding="utf-8") as f: + data = json.load(f) + except (IOError, json.JSONDecodeError) as e: + print(f"Error reading scan results: {e}", file=sys.stderr) + sys.exit(1)
256-256: Note: Categories list is hardcoded.The
CATEGORIESlist is hardcoded. If AquaSec introduces new finding categories in the future, they won't appear in the summary table. Consider whether dynamically discovering categories from the SARIF data would be more maintainable.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.github/workflows/aquasec_repo_scan.yml
🔇 Additional comments (6)
.github/workflows/aquasec_repo_scan.yml (6)
1-17: LGTM!The workflow configuration is well-structured with appropriate permissions scoped to what's needed (security-events for SARIF upload, issues/pull-requests for commenting). The concurrency configuration correctly prevents duplicate runs on the same branch.
217-237: LGTM!The SARIF output structure is well-formed and compliant with the 2.1.0 schema. The tool driver includes all required fields, and findings are correctly mapped with rule indices and physical locations.
239-243: LGTM!The SARIF upload step correctly uses the CodeQL action with appropriate parameters. The
category: aquasecensures these results are properly labeled in the Security tab.
260-324: LGTM!The summary table generation is well-implemented with proper error handling for file operations, bounds checking on rule indices, and correct use of the GITHUB_OUTPUT heredoc pattern for multi-line values.
326-343: LGTM!The PR comment step is well-implemented. The previous review concerns have been addressed:
awaitis now correctly used for the API call- The summary table is safely passed via environment variable, preventing template injection issues
23-27: No issues found —actions/checkout@v6is a valid, released version.
|
AquaSec has completed a full security repository scan ✅ You can find the analysis results for this PR branch on this overview.
|
tmikula-dev
removed
the
work in progress
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
.github/workflows/aquasec_repo_scan.yml (1)
27-27: Consider removing full history fetch.The
fetch-depth: 0fetches the entire repository history, which can significantly slow down the workflow for large repositories. Security scans typically only need the current code state.🔎 Proposed optimization
- fetch-depth: 0 + fetch-depth: 1
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.github/workflows/aquasec_repo_scan.yml
🔇 Additional comments (5)
.github/workflows/aquasec_repo_scan.yml (5)
101-116: LGTM - Variable expansion issues resolved.The shell variable references now correctly use the
$prefix and proper quoting. The pagination logic correctly aggregates findings and constructs the final JSON output.
190-195: LGTM - Good defensive line number sanitization.The line number validation ensures SARIF schema compliance by enforcing
start_line >= 1andend_line >= start_line, preventing invalid region data.
234-238: LGTM - Standard SARIF upload configuration.The upload-sarif action is correctly configured with the generated SARIF file and a category identifier for filtering in the GitHub Security tab.
240-319: LGTM - Well-structured summary generation with proper error handling.The script correctly processes the SARIF file, aggregates findings by category and severity, and writes the markdown table to GITHUB_OUTPUT using the EOF delimiter for multiline content. Error handling for file I/O and JSON parsing is comprehensive.
321-359: LGTM - Previous review issues have been resolved.The
awaitkeywords are now present on all GitHub API calls (lines 336, 346, 353), and the summary table is safely accessed viaprocess.env.SUMMARY_TABLErather than direct template interpolation, preventing any injection vulnerabilities. The marker-based logic correctly handles updating existing comments or creating new ones.
miroslavpojer
left a comment
miroslavpojer
left a comment
There was a problem hiding this comment.
I like this solution.
I preffer to create a GH action to host it and deliver short workflow using it in this repository.
| runs-on: ubuntu-latest | ||
| steps: | ||
| - name: Checkout repository | ||
| uses: actions/checkout@v6 |
There was a problem hiding this comment.
You should use GH sha instead of tags to avoid error report - AVD-PIPELINE-0008.
There was a problem hiding this comment.
See the solution for all workflows in the commit: 7a580f0.
|
.gitignore
Outdated
|
|
||
| # Terraform Plan output files | ||
| *.sarif | ||
|
|
There was a problem hiding this comment.
use .git/info/exclude for dev files
There was a problem hiding this comment.
I found out that .git/ is never tracked, so I am not sure, if I understand the suggestion.
There was a problem hiding this comment.
I did delete the .gitignore file that was only for my debugging purposes. See the change in: bf05a69
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
.github/workflows/aquasec_repo_scan.yml.github/workflows/check_docker.yml.github/workflows/check_python.yml.github/workflows/check_terraform.yml.trivyignoretrivy-secret.yaml
💤 Files with no reviewable changes (3)
- trivy-secret.yaml
- .github/workflows/check_docker.yml
- .trivyignore
🔇 Additional comments (7)
.github/workflows/aquasec_repo_scan.yml (5)
29-120: The AquaSec API integration logic looks solid.The authentication flow with HMAC signature and paginated result retrieval is well-implemented. The UUID validation regex correctly validates the repository ID format, and the pagination logic properly aggregates results across multiple pages.
121-233: Well-structured SARIF conversion with proper schema compliance.The conversion logic correctly:
- Maps AquaSec severities to SARIF levels and security-severity scores
- Generates unique rules with deduplication via
rule_index_lookup- Sanitizes line numbers to ensure valid SARIF (lines 190-195)
- Produces valid SARIF 2.1.0 output with proper schema structure
240-320: Excellent summary table generation with proper error handling.The Python script correctly:
- Uses proper multiline output syntax with EOF delimiters (lines 312-314)
- Implements comprehensive error handling for file I/O and JSON parsing (lines 255-268, 309-319)
- Aggregates findings by category and severity using Counter
- Generates a well-formatted markdown table with totals
234-238: The codeql-action SHA corresponds to v4.31.0, a legitimate but outdated release.The pinned SHA
4e94bd11f71e507f7f87df81788dff88d1dacbfbis valid and matches the stable v4.31.0 release tag. However, the action is several minor versions behind; consider updating to a more recent stable version (currently v4.31.9 is available) to benefit from security patches and bug fixes.
321-359: The github-script action SHA is legitimate and corresponds to v8.0.0.The PR comment logic correctly uses
awaiton all async operations and reads the summary table from environment variables to prevent template injection. The github-script action SHAed597411d8f924073f98dfc5c65a23a2325f34cdis a verified merge commit that introduced v8 of actions/github-script—a stable, legitimate release..github/workflows/check_python.yml (1)
26-26: Action SHAs are pinned to legitimate stable versions.The pinned SHAs correspond to
actions/checkout@v6andactions/[email protected], which are legitimate stable releases. SHA pinning here is a valid security practice that locks workflow dependencies to specific versions..github/workflows/check_terraform.yml (1)
26-26: No action required—all pinned action SHAs are legitimate.The pinned SHAs for actions/checkout (v6), terraform-linters/setup-tflint (v6.2.1), and github/codeql-action/upload-sarif (v3.29.5) all correspond to current, supported releases. These pins are secure and do not require updates.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In @.github/workflows/aquasec_repo_scan.yml:
- Around line 340-343: The find call can throw if a comment's body is
null/undefined; update the search for existingComment (the expression using
comments.find and c.body.includes(marker)) to defensively check the body
first—e.g., use optional chaining or a truthy check on c.body before calling
includes (or coerce to string) so the predicate never invokes includes on
null/undefined; keep the rest of the logic unchanged and still match marker.
- Around line 150-160: The code currently uses avd_id directly as the rule
identifier, which allows empty strings to become an invalid shared rule; detect
when avd_id is falsy and synthesize a stable fallback rule ID (e.g., prefix +
category or source + a short hash of title/message or an incremental
counter/UUID) before using it in checks against rule_index_lookup and when
adding rules; ensure the generated fallback is deterministic per finding group
so distinct findings do not collapse into the same empty-ID rule and use that
fallback wherever avd_id was previously used (references: avd_id,
rule_index_lookup, aquasec_findings).
🧹 Nitpick comments (1)
.github/workflows/aquasec_repo_scan.yml (1)
46-51: Consider parameterizing the hardcoded API configuration.The
group_id:1228and EU-1 endpoint are hardcoded. If this workflow needs to support multiple AquaSec tenants or regions, consider extracting these as secrets or repository variables.+ # Configuration - consider moving to secrets/vars for multi-tenant support METHOD="POST" - AUTH_ENDPOINT="https://eu-1.api.cloudsploit.com/v2/tokens" + AUTH_ENDPOINT="${AQUA_AUTH_ENDPOINT:-https://eu-1.api.cloudsploit.com/v2/tokens}" TIMESTAMP=$(date -u +%s) - POST_BODY='{"group_id":1228,"allowed_endpoints":["GET"],"validity":240}' + GROUP_ID="${AQUA_GROUP_ID:-1228}" + POST_BODY="{\"group_id\":${GROUP_ID},\"allowed_endpoints\":[\"GET\"],\"validity\":240}"
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.github/workflows/aquasec_repo_scan.yml
🔇 Additional comments (7)
.github/workflows/aquasec_repo_scan.yml (7)
1-17: LGTM!Workflow metadata is well-structured: triggers are appropriate, permissions follow least-privilege, and concurrency settings prevent redundant runs.
78-119: LGTM!Pagination logic is sound with proper break conditions and rate-limit mitigation via sleep.
212-227: LGTM!SARIF 2.1.0 structure is correctly implemented with proper schema reference and required fields.
240-319: LGTM!Summary table generation is well-implemented with proper error handling, defensive checks for rule indices, and correct use of
GITHUB_OUTPUTheredoc syntax for multi-line content.
234-238: The SHA4e94bd11f71e507f7f87df81788dff88d1dacbfbis valid and corresponds to release v4.31.0 of github/codeql-action. SHA pinning is a security best practice and this usage is appropriate.
23-27: SHA correctly pinned to actions/checkout v6.0.0. This SHA-pinned approach is secure and best practice. No action needed.
321-327: The pinned SHA ed597411d8f924073f98dfc5c65a23a2325f34cd is valid and corresponds to actions/github-script v8.0.0, the latest release. The pinning strategy is sound.
|
I do agree, that the best way how to distribute this POC solution across multiple teams and projects is to put it into a GH Action that accepts: AQUA_KEY, AQUA_SECRET and REPOSITORY_ID as input values. In the following GH action solution we can fine tune this POC scan. @miroslavpojer please can you do the review of the last 4-5 commits? |
miroslavpojer
left a comment
miroslavpojer
left a comment
There was a problem hiding this comment.
All review comments from my side were addressed.
Background
AquaSec scanning workflow to fetch, convert and fill the Security GitHub tab with corresponding GitHub PR comment.
Release Notes
Closes #78
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.