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

ci: automatically scan and patch our images for known vulnerabilities #1942

Merged
merged 6 commits into from
Dec 14, 2020

Conversation

consideRatio
Copy link
Member

@consideRatio consideRatio commented Dec 12, 2020

In this PR I've created a workflow scheduled to run once daily scanning the images we manage with chartpress for known vulnerabilities using aquasecurity/trivy by using the aquasecurity/trivy-action.

Closes #1712.

In this PR also...

  • Use GitHub workflow status badges in README.md

In future PRs... (Done in 4728fa2)

  • Update Dockerfiles with a comment to trigger rebuilds by the CI system
    • Let the comment contain a hash of some kind, diffing the latest released vulns with the new vulns.
    • Update all Dockerfiles to contain such comment
  • Automate practice of forcing a rebuild to see if it resolves security concerns and if so, open a PR to trigger the rebuild.
  • Update the PRs to contain reports
  • Block this workflow from running on forks randomly disturbing anyone having forked this repo

Example workflow run

image

Copy link
Member

@minrk minrk left a comment

Choose a reason for hiding this comment

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

Nice! I left a question inline about whether splitting and recombining the image into repo:tag is necessary.

Do we also want to run this scan on PRs that change the image directories?

.github/workflows/vuln-scan.yaml Outdated Show resolved Hide resolved
.github/workflows/vuln-scan.yaml Outdated Show resolved Hide resolved
Co-authored-by: Min RK <benjaminrk@gmail.com>
id: image
run: |
IMAGE=$(
chartpress --list-images \
Copy link
Member

Choose a reason for hiding this comment

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

Suggestion for the future? Have chartpress output machine-readable JSON that can be read straight into a GitHub workflow variable. This would be more robust than using grep if more images are added, and means anyone else using chartpress could copy this action 😄.

@consideRatio consideRatio marked this pull request as draft December 14, 2020 15:21
@manics
Copy link
Member

manics commented Dec 14, 2020

Could we merge this now, and leave the additions (automatic updates) for a follow-up PR? It might make it easier to review, especially if you're going to automatically push branches.

@consideRatio consideRatio marked this pull request as ready for review December 14, 2020 15:46
@consideRatio
Copy link
Member Author

@manics ok sure!

@consideRatio consideRatio merged commit 345c926 into jupyterhub:master Dec 14, 2020
@consideRatio
Copy link
Member Author

Here is an example workflow run which i triggered manually thanks to the workflow_dispatch trigger: https://github.com/jupyterhub/zero-to-jupyterhub-k8s/runs/1551355583?check_suite_focus=true

@consideRatio
Copy link
Member Author

I successfully made the vuln-scan workflow automatically open PRs to trigger rebuilds based on if known vulnerabilities were found to be resolved by rebuilding an image. I've also verified it seem to work here in this repo like it did on my fork. See the this run for example.

Since this functionality was built on adding a comment to all our Dockerfile's, it forced a rebuild. So when merged to master some fixes were applied. This means that it may take a while before we see the automation in action in this repo. But, it can be seen in my fork from when I developed it: consideRatio#17.


I pushed 4728fa2 straight to master, which was a shortcut to ensure I got this working all the way - right away. If I had opened a PR and self-merged it, it would have provided a better UI for post-merge review/discussion, and it may have been relevant to hold back for additional review to come in. Sorry for the haste. My main worry nudging me to not wait for review of this was to be able to keep focusing to make this work all the way right now when I had the time and focus. I've found myself context switching more than I'd like recently and felt it takes a toll on my focus and productivity. I don't intend to make a habit of it.

@manics
Copy link
Member

manics commented Dec 17, 2020

Thanks, I'll take a look 😄

Is there a benefit to pushing straight to master over opening and self-merging a PR? The benefit of PRs is that it fits in with the workflow for releases where the changelog can be automatically generated, and if there is a problem in future one of the first things I do is scan through the list of PRs.

@consideRatio
Copy link
Member Author

Is there a benefit to pushing straight to master over opening and self-merging a PR?

Not worth doing it for, not in my mind! There are more benefits of having PRs as well, such as grouping related commits

@consideRatio consideRatio changed the title ci: cronjob to scan images for known vulnerabilities ci: automatically scan and patch our images for known vulnerabilities Jan 10, 2021
@meeseeksmachine
Copy link

This pull request has been mentioned on Jupyter Community Forum. There might be relevant details there:

https://discourse.jupyter.org/t/jupyterhub-helm-chart-0-11-0-released/7521/1

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

CI: Automatic hub image scanning using trivy
4 participants