-
Notifications
You must be signed in to change notification settings - Fork 0
ROX-30730: add scan-image-vulnerabilities action #72
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
msugakov
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Skipped readme and some smaller details in the action.
|
@msugakov The action now:
The script has the following improvements:
You can check how this looks in the summary for https://github.com/stackrox/stackrox/actions/runs/20063752468 |
msugakov
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Made a pass. I'll likely find something else in subsequent iterations.
release/check-image-vulnerabilities/check-image-vulnerabilities.sh
Outdated
Show resolved
Hide resolved
release/check-image-vulnerabilities/check-image-vulnerabilities.sh
Outdated
Show resolved
Hide resolved
release/check-image-vulnerabilities/check-image-vulnerabilities.sh
Outdated
Show resolved
Hide resolved
release/check-image-vulnerabilities/check-image-vulnerabilities.sh
Outdated
Show resolved
Hide resolved
release/check-image-vulnerabilities/check-image-vulnerabilities.sh
Outdated
Show resolved
Hide resolved
release/check-image-vulnerabilities/check-image-vulnerabilities.sh
Outdated
Show resolved
Hide resolved
release/check-image-vulnerabilities/check-image-vulnerabilities.sh
Outdated
Show resolved
Hide resolved
This reverts commit 14030cb.
e060327 to
e6f53b0
Compare
msugakov
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, could cover only a small part today. Please expect follow-ups.
| IMAGE="${1:-}" | ||
| SUMMARY_PREFIX="${2:-}" | ||
| local result_path="scan-result.json" | ||
|
|
||
| check_not_empty \ | ||
| IMAGE \ | ||
| SUMMARY_PREFIX |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't know bash pass-by-name semantics and so the question: would check_not_empty still work if IMAGE and SUMMARY_PREFIX are both declared local?
In my tiny test, it seems to work:
$ cat a.sh
#!/usr/bin/env bash
set -euo pipefail
function main() {
local image="blah"
check image
}
function check() {
typeset -n VAR="$1"
echo "it is: $VAR"
}
main
$ ./a.sh
it is: blahThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes it works - what is your suggestion or concern here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IMAGE="${1:-}"
SUMMARY_PREFIX="${2:-}"aren't declared as local. Please declare them as local.
release/scan-image-vulnerabilities/scan-image-vulnerabilities.sh
Outdated
Show resolved
Hide resolved
msugakov
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Finished the full pass.
release/scan-image-vulnerabilities/scan-image-vulnerabilities.sh
Outdated
Show resolved
Hide resolved
release/scan-image-vulnerabilities/scan-image-vulnerabilities.sh
Outdated
Show resolved
Hide resolved
…total findings by not relying on roxctl summary
|
|
||
| Required: Yes | ||
| Prefix for the vulnerability report in the GitHub step summary. Use this to help users of the action classify images into groups when multiple matrix scans are performed in a workflow. | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(keeping a newline but deleting a blank tab)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done in 303fbdb, there was one more whitespace to fix.
| The action integrates with the [stackrox/central-login](https://github.com/stackrox/central-login) action, which uses OIDC login for authentication of the `roxctl` CLI. | ||
| The ACS Central needs to be configured to allow exchanging tokens from GitHub Actions workflow runs. | ||
|
|
||
| Additionally, an image integration for Quay.io needs to be configured in the ACS Central. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Suggestion:
| Additionally, an image integration for Quay.io needs to be configured in the ACS Central. | |
| Additionally, an image integration for Quay.io must be configured in the ACS Central so that it can pull images requested for scanning. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done in 303fbdb
| function print_vulnerability_status() { | ||
| local -n vuln_counts_ref=$1 | ||
| # Prints the vulnerability status and an overview table of the findings counts. | ||
| function print_findings_status() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about?
| function print_findings_status() { | |
| function print_summary() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done in 303fbdb
| local -n fixable_counts_ref=$2 | ||
|
|
||
| if (( fixable_counts_ref[CRITICAL] > 0 || fixable_counts_ref[IMPORTANT] > 0 )); then | ||
| if [ "$(assert_fixable_findings_present fixable_counts_ref)" = "true" ]; then |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please use [[ instead of [ in bash. There's a couple places to fix.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done in 303fbdb
| # Prints a markdown table of the findings, sorted by severity with fixable findings first. | ||
| # Each row contains left, right and column separators added by jq's join function. | ||
| function print_vulnerabilities_table() { | ||
| function print_findings_table() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Optional suggestion:
| function print_findings_table() { | |
| function print_details_table() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done in 303fbdb
| # Asserts if any fixable findings of relevant severity are present. | ||
| function assert_fixable_findings_present() { | ||
| local -n fixable_counts_map="$1" | ||
| if (( fixable_counts_map[CRITICAL] > 0 || fixable_counts_map[IMPORTANT] > 0 )); then | ||
| echo "true" | ||
| else | ||
| echo "false" | ||
| fi | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, this one's going to be a bit vague as I just throw a code suggestion:
| # Asserts if any fixable findings of relevant severity are present. | |
| function assert_fixable_findings_present() { | |
| local -n fixable_counts_map="$1" | |
| if (( fixable_counts_map[CRITICAL] > 0 || fixable_counts_map[IMPORTANT] > 0 )); then | |
| echo "true" | |
| else | |
| echo "false" | |
| fi | |
| } | |
| # Checks if any fixable findings of relevant severity are present. | |
| function are_blocking_vulns_present() { | |
| local -n fixable_counts="$1" | |
| if (( fixable_counts[CRITICAL] > 0 || fixable_counts[IMPORTANT] > 0 )); then | |
| return 0 | |
| else | |
| return 1 | |
| fi | |
| } |
0 means success or "yes", no-zero means failure, or "no". Perhaps it can be done via just return (( fixable_counts[CRITICAL] > 0 || fixable_counts[IMPORTANT] > 0 )) but I haven't tried.
After this, you can simply write
if are_blocking_vulns_present fixable_counts_ref; then
# ...
and no need to compare strings.
The logic is still invert of what it should be, 0 == "clean", no-zero == "failure", but we need a good function name and I can't come up with one at the moment. Perhaps you can?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done in 35cf37d.
I think that are_blocking_vulns_present sufficiently describes the content of the function.
The variable needs to be named fixable_counts_map if we want to keep the local -n. If it was named fixable_counts, it would create a circular dependency.
Description
Partners with stackrox/stackrox#17985
This PR mostly extracts the existing image scan logic into a composable action.
Validation
https://github.com/stackrox/stackrox/actions/runs/20063752468
Why did I not test with
test-gh-actions?The workflow there still uses quay.io ratings: https://github.com/stackrox/test-gh-actions/blob/main/.github/workflows/scripts/check-image-vulnerabilities.py and wasn't updated after stackrox/stackrox#10836.