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

Trivy vulnerability scanning #14145

Merged
merged 2 commits into from
Sep 27, 2024
Merged

Conversation

hamistao
Copy link
Contributor

@hamistao hamistao commented Sep 23, 2024

This adds two jobs to our workflows. One uses Trivy to scan LXD's dependencies and the other scans the snap.

@hamistao
Copy link
Contributor Author

hamistao commented Sep 23, 2024

image
This shows how the alerts are shown in the security page (those alerts shown are false positives and can be closed as such after this is merged).

@simondeziel @tomponline Notice how it is possible to filter the alerts by tool. Unfortunately, since GitHub does not support setting custom filters for alerts and we do not use tags nor branches to mark the versions used in each snap channel, we can't filter alerts from each snap channel scan easily.
An alternative would be to include snap scanning in lxd-pkg-snap, include each snap channel scan in its respective branch and filter by branch. I am open to suggestion on how to better handle this.

@tomponline
Copy link
Member

An alternative would be to include snap scanning in lxd-pkg-snap, include each snap channel scan in its respective branch and filter by branch. I am open to suggestion on how to better handle this.

Lets check its actually working on the snap scan first, we need to confirm its actually recognising the deps inside the snap.

@hamistao
Copy link
Contributor Author

hamistao commented Sep 23, 2024

image
Another image showing some alerts from the snap scanner (these alerts are fixed on latest channels)

@tomponline
Copy link
Member

image Another image showing some alerts from the snap scanner (these alerts are fixed on latest channels)

why dont we set it up in each one of our source branches on this repo, but targeting <series>/stable as its the stable snaps we are interested most in.

@hamistao
Copy link
Contributor Author

why dont we set it up in each one of our source branches on this repo, but targeting /stable as its the stable snaps we are interested most in.

Works for me! Another option would also be appending the channel in the beginning of the alert name. For example: 5.0/stable - PyYAML: Incomplete fix for CVE-2020-1747. That said, searching for branch should be simpler so I would still prefer the option of using the branches.

@tomponline
Copy link
Member

why dont we set it up in each one of our source branches on this repo, but targeting /stable as its the stable snaps we are interested most in.

Works for me! Another option would also be appending the channel in the beginning of the alert name. For example: 5.0/stable - PyYAML: Incomplete fix for CVE-2020-1747. That said, searching for branch should be simpler so I would still prefer the option of using the branches.

can you do that too so its clear its coming from a snap alert

@tomponline
Copy link
Member

Ready for review?

@hamistao hamistao force-pushed the trivy_vuln_scan branch 2 times, most recently from 80fb66a to 93dbcf4 Compare September 25, 2024 22:38
@hamistao
Copy link
Contributor Author

@tomponline Now it is!

Example of alerts for the snap scanner, the repo scanner alerts are the same.

image

@hamistao hamistao marked this pull request as ready for review September 25, 2024 22:54
@hamistao
Copy link
Contributor Author

image

One more image to ilustrate one particular behavior, if more than one version has the same vulnerability, the most recent is shown in the alert title, I believe this is because the alert ID is the same so the title should stay constant across branches. They are still filtered by branch and have the indicator on the side so in my view this doesn't hurt the UX.

@tomponline
Copy link
Member

Does the repo scan generate the vuln cache dir? If so can we get it to update the cache when it finishes and then have the snap task depend on it?

@hamistao
Copy link
Contributor Author

Does the repo scan generate the vuln cache dir? If so can we get it to update the cache when it finishes and then have the snap task depend on it?

Yes, we can!

@hamistao
Copy link
Contributor Author

@tomponline @simondeziel Requested changes made

image
Updated alerts above

@tomponline tomponline merged commit f1271f6 into canonical:main Sep 27, 2024
30 checks passed
uses: github/codeql-action/upload-sarif@v3
with:
sarif_file: "${{ matrix.version }}-stable.sarif"
sha: ${{ github.sha }}
Copy link
Member

Choose a reason for hiding this comment

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

do we need to provide this line at all?

Copy link
Contributor Author

@hamistao hamistao Sep 27, 2024

Choose a reason for hiding this comment

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

I also find it confusing but I get an error if I don't provide both sha and ref. The error explicitly says both are needed here.

Copy link
Member

Choose a reason for hiding this comment

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

can parse the version from the snap somehow, and use the ref of the associated tag perhaps?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I will try and see if I can get something like this working

@hamistao
Copy link
Contributor Author

The PRs for microcloud and microcluster will be ready soon.

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

Successfully merging this pull request may close these issues.

2 participants