Skip to content
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

✨ Adds a verify script that run trivy scanner on container images #7604

Conversation

fabriziopandini
Copy link
Member

@fabriziopandini fabriziopandini commented Nov 23, 2022

What this PR does / why we need it:
Adding a verify script that runs trivy scanner on all the container images; the script is very basic for now, and it assumes someone will run it manually.
In follow-up iterations, we can think about how to automate this (when to run it, for changes on go.mod files, periodically), if to limit validation only to the image we release on production or not, and how to get a signal from the command output.
Also added a GitHub workflow running the same check periodically

For now, it will be great if we can have the release team running it periodically on main / before cutting a patch

@ykakarap @sbueringer @CecileRobertMichon opionis?

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Nov 23, 2022
@k8s-ci-robot k8s-ci-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Nov 23, 2022
@fabriziopandini
Copy link
Member Author

fabriziopandini commented Nov 23, 2022

oh, just found out there is also a GitHub action we can use https://github.com/aquasecurity/trivy-action for follow up iteractions

@oscr
Copy link
Contributor

oscr commented Nov 23, 2022

In the spirit of exploration I added the following line in the end to see if grype would find anything:

docker run \
  -v /var/run/docker.sock:/var/run/docker.sock \
  anchore/grype:latest --add-cpes-if-none gcr.io/k8s-staging-cluster-api/clusterctl-${GO_ARCH}:dev

output

NAME                        INSTALLED  FIXED-IN  TYPE       VULNERABILITY   SEVERITY 
google.golang.org/protobuf  v1.28.1              go-module  CVE-2015-5237   High      
google.golang.org/protobuf  v1.28.1              go-module  CVE-2021-22570  Medium    

@fabriziopandini
Copy link
Member Author

/retest

Copy link
Contributor

@oscr oscr left a comment

Choose a reason for hiding this comment

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

Script tested locally without issues on x86_64 GNU/Linux.

Great work!

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 23, 2022
@k8s-ci-robot k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed lgtm "Looks good to me", indicates that a PR is ready to be merged. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Dec 5, 2022
@furkatgofurov7
Copy link
Member

furkatgofurov7 commented Dec 5, 2022

In follow-up iterations, we can think about how to automate this (when to run it, for changes on go.mod files, periodically), if to limit validation only to the image we release on production or not, and how to get a signal from the command output.

GA is added as part of this PR which runs periodically, maybe we need to update the PR description unless I am misunderstanding it? Otherwise, I tried it running locally and works fine on my x86_64 Linux machine.

Copy link
Contributor

@alexander-demicev alexander-demicev left a comment

Choose a reason for hiding this comment

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

I run the script locally on my Mac, all seems to work fine!

@CecileRobertMichon
Copy link
Contributor

For now, it will be great if we can have the release team running it periodically on main / before cutting a patch

should this be documented in release tasks ?

@fabriziopandini
Copy link
Member Author

should this be documented in release tasks ?

done
@ykakarap PTAL

Copy link
Contributor

@ykakarap ykakarap left a comment

Choose a reason for hiding this comment

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

Overall LGTM. Only one comment.

Since we generally cut a release on a Tuesday(at least for the v1.4 release cycle) how about changing the cron job to run every Monday or Tuesday morning (instead of Wednesday as defined right now). This way we will have a much newer signal before cutting the release.

Running the trivy scan works locally on ARM64 mac as well.
The additions to the release-tasks.md also look good.

@fabriziopandini
Copy link
Member Author

@ykakarap done!

@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Dec 30, 2022
@fabriziopandini fabriziopandini force-pushed the add-verify-container-images branch 2 times, most recently from 6fb07b9 to beb61bf Compare December 30, 2022 12:03
@k8s-ci-robot k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Dec 30, 2022
@fabriziopandini
Copy link
Member Author

@sbueringer now the github action calls the make target, and all the scans are run first, and then if at least one fails the job fails, otherwise it passes

@sbueringer
Copy link
Member

Thank you very much!!

/lgtm

(giving other folks some time to review as well)

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jan 2, 2023
@k8s-ci-robot
Copy link
Contributor

LGTM label has been added.

Git tree hash: 3de2e2c0baba34ad4f163204d40df1434e7e906c

@sbueringer
Copy link
Member

@fabriziopandini Let's cherry-pick into release-1.3 and release-1.2? (but maybe we want to wait for a first successful run after merge)

@sbueringer
Copy link
Member

/approve

@sbueringer
Copy link
Member

/cherry-pick release-1.3

@k8s-infra-cherrypick-robot

@sbueringer: once the present PR merges, I will cherry-pick it on top of release-1.3 in a new PR and assign it to you.

In response to this:

/cherry-pick release-1.3

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@sbueringer
Copy link
Member

/cherry-pick release-1.2

@k8s-infra-cherrypick-robot

@sbueringer: once the present PR merges, I will cherry-pick it on top of release-1.2 in a new PR and assign it to you.

In response to this:

/cherry-pick release-1.2

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: sbueringer

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jan 2, 2023
@k8s-ci-robot k8s-ci-robot merged commit 6257be4 into kubernetes-sigs:main Jan 2, 2023
@k8s-ci-robot k8s-ci-robot added this to the v1.4 milestone Jan 2, 2023
@k8s-infra-cherrypick-robot

@sbueringer: new pull request created: #7820

In response to this:

/cherry-pick release-1.3

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@k8s-infra-cherrypick-robot

@sbueringer: #7604 failed to apply on top of branch "release-1.2":

Applying: Add verify script and weekly scan workflow
Using index info to reconstruct a base tree...
M	Makefile
M	docs/book/src/reference/jobs.md
A	docs/release/release-tasks.md
Falling back to patching base and 3-way merge...
CONFLICT (modify/delete): docs/release/release-tasks.md deleted in HEAD and modified in Add verify script and weekly scan workflow. Version Add verify script and weekly scan workflow of docs/release/release-tasks.md left in tree.
Auto-merging docs/book/src/reference/jobs.md
Auto-merging Makefile
error: Failed to merge in the changes.
hint: Use 'git am --show-current-patch=diff' to see the failed patch
Patch failed at 0001 Add verify script and weekly scan workflow
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".

In response to this:

/cherry-pick release-1.2

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.


# Scan the images
${TOOL_BIN}/trivy image -q --exit-code 1 --ignore-unfixed --severity MEDIUM,HIGH,CRITICAL gcr.io/k8s-staging-cluster-api/clusterctl-"${GO_ARCH}":dev && R1=$? || R1=$?
${TOOL_BIN}/trivy image -q --exit-code 1 --ignore-unfixed --severity MEDIUM,HIGH,CRITICAL gcr.io/k8s-staging-cluster-api/test-extension-"${GO_ARCH}":dev && R2=$? || R2=$?
Copy link

Choose a reason for hiding this comment

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

Hi @fabriziopandini , so if the scanned vulnerability hasn't got a fixed version, we just ignore it and won't report it; we only report the ones having a fix version but we haven't update. Right?

I am curious what should we do for the ones haven't got a fixed version? If ignored, it means we won't pay attention to them, won't document them when cutting branch etc.?

Copy link
Member

@sbueringer sbueringer Jan 3, 2023

Choose a reason for hiding this comment

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

Hi @fabriziopandini , so if the scanned vulnerability hasn't got a fixed version, we just ignore it and won't report it; we only report the ones having a fix version but we haven't update. Right?

Yes

I am curious what should we do for the ones haven't got a fixed version? If ignored, it means we won't pay attention to them, won't document them when cutting branch etc.?

Good question. We could add a second job which runs the same target with a parameter specifying that it should even fail if there is no fixed version (the parameter would control if "--ignore-unfixed" is used or not)

This way we would still have a signal for the dependencies we can bump and a successful job after it's done. But we also would have a signal for the "unfixable" dependencies.

Copy link

Choose a reason for hiding this comment

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

I see. Thanks!

Copy link
Member Author

@fabriziopandini fabriziopandini Jan 3, 2023

Choose a reason for hiding this comment

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

I think that the scan job should have a goal, which is to give a signal someone has to take action.
In this light, we care about vulnerabilities that have a fix.

Also, please consider we are starting now with this process, and until we figure it out how this signal works and how we can handle it I personally would prefer to not add more noise or nuances on top of it, especially if the additional info is not actionable

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet