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

feat(ci): add step to scan built container image for vulnerabilities #32288

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

hainenber
Copy link
Contributor

feat(ci): add step to scan built container image for vulnerabilities

SUMMARY

Currently we don't have observability over container image vulnerabilities or at least not the one I'm aware of. This PR is to add a Trivy-based container scan to (1) proactively patch container image-based vulnerabilities and (2) have a holistic view over the scene.

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

TESTING INSTRUCTIONS

ADDITIONAL INFORMATION

  • Has associated issue:
  • Required feature flags:
  • Changes UI
  • Includes DB Migration (follow approval process in SIP-59)
    • Migration is atomic, supports rollback & is backwards-compatible
    • Confirm DB migration upgrade and downgrade tested
    • Runtime estimates and downtime expectations provided
  • Introduces new feature or API
  • Removes existing feature or API

Copy link

@korbit-ai korbit-ai bot left a comment

Choose a reason for hiding this comment

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

I've completed my review and didn't find any issues.

Need a new review? Comment /korbit-review on this PR and I'll review your latest changes.

Korbit Guide: Usage and Customization

Interacting with Korbit

  • You can manually ask Korbit to review your PR using the /korbit-review command in a comment at the root of your PR.
  • You can ask Korbit to generate a new PR description using the /korbit-generate-pr-description command in any comment on your PR.
  • Too many Korbit comments? I can resolve all my comment threads if you use the /korbit-resolve command in any comment on your PR.
  • Chat with Korbit on issues we post by tagging @korbit-ai in your reply.
  • Help train Korbit to improve your reviews by giving a 👍 or 👎 on the comments Korbit posts.

Customizing Korbit

  • Check out our docs on how you can make Korbit work best for you and your team.
  • Customize Korbit for your organization through the Korbit Console.

Current Korbit Configuration

General Settings
Setting Value
Review Schedule Automatic excluding drafts
Max Issue Count 10
Automatic PR Descriptions
Issue Categories
Category Enabled
Documentation
Logging
Error Handling
Readability
Design
Performance
Security
Functionality

Feedback and Support

Note

Korbit Pro is free for open source projects 🎉

Looking to add Korbit to your team? Get started with a free 2 week trial here

@github-actions github-actions bot added the github_actions Pull requests that update GitHub Actions code label Feb 17, 2025
@dosubot dosubot bot added the infra:container Infra container and K8s label Feb 17, 2025
@hainenber hainenber self-assigned this Feb 17, 2025
@hainenber hainenber force-pushed the feat/add-container-sec-scan-in-ci branch from 2b9106d to 1afaa7a Compare February 17, 2025 14:53
@github-advanced-security
Copy link

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.

@mistercrunch
Copy link
Member

I like the idea of having a vulnerability/package scanner in the open. A few things about it.

First, let's note that vulns can be discovered at any time in the lifecycle of a package, essentially guaranteeing non-deterministic results, meaning a SHA that passes the test today may not pass the test tomorrow. This also means that this new check could start failing all open PRs (including those that don't touch packages) when some package gets marked as vulnerable. I don't think it's productive to hold all PRs for unrelated issues.

For release management, it's likely that older releases, say 3.x.+1, would have numerous vulns, making it hard/harder to provide LTS if/when that's a goal.

Vulns include lots of false negative, as often we may use packages that have vulns, but not in ways that trigger the particular issue. That's often the case with devDeps and other edge case vulnerabilities. We need a clear way to mark for issues that are known to us and aren't real issues given the context of how we use the given package.

One more point, is depending on how your envrionment is set up, the level of criticality may or may not be a concern. For instance at Preset we run Superset on the open internet and take every security issue very seriously, but in other envrionments (behind VPN, local set up, specific set of configuration or feature flags, ...) the issue may or may not be a concern. Sorting that out is subtle/complicated. For that reason, different organizations have their own package/vulnerability scanning mechanisms - say at Preset we use Snyk on top of the particular set of dependencies we pin for ourselves.

In any case, there's value in knowing this, at certain points in time, mainly around release time. Given all this, seems a daily/nightly job testing a list of "supported" releases (latest, LTS, master) would make most sense.

@mistercrunch
Copy link
Member

Oh, one more thing, which tool/tech should we use here? Is Trivy the best choice these days? What are all the options?

@hainenber
Copy link
Contributor Author

hainenber commented Feb 19, 2025

First, let's note that vulns can be discovered at any time in the lifecycle of a package, essentially guaranteeing non-deterministic results, meaning a SHA that passes the test today may not pass the test tomorrow. This also means that this new check could start failing all open PRs (including those that don't touch packages) when some package gets marked as vulnerable. I don't think it's productive to hold all PRs for unrelated issues.

Unless you set the exit code to be 1 when vulns are detected, Trivy won't break the CI by default. First and foremost concern is not to fail people's PRs when enumerating vulnerabilities. I've seen too many horror stories like that in $WORKPLACE and wouldn't want it to be here.

Vulns include lots of false negative, as often we may use packages that have vulns, but not in ways that trigger the particular issue. That's often the case with devDeps and other edge case vulnerabilities. We need a clear way to mark for issues that are known to us and aren't real issues given the context of how we use the given package.

I should have made the PR clearer in description as this PR would scan for container image vulnerabilities, namely the OS type. We already have GH security alerts for package-type vulnerabilities.

One more point, is depending on how your envrionment is set up, the level of criticality may or may not be a concern. For instance at Preset we run Superset on the open internet and take every security issue very seriously, but in other envrionments (behind VPN, local set up, specific set of configuration or feature flags, ...) the issue may or may not be a concern. Sorting that out is subtle/complicated. For that reason, different organizations have their own package/vulnerability scanning mechanisms - say at Preset we use Snyk on top of the particular set of dependencies we pin for ourselves.

This PR is more of "establishing visibility in container image-kind vulnerabilities" which is lacking currently in GH (great hearing you guys having Snyk at Preset, no shilling but it's nice!) than "adding a security blocker for all changes". Some sort of inventory auditing so Security task force can work on with

Oh, one more thing, which tool/tech should we use here? Is Trivy the best choice these days? What are all the options?
I picked Trivy because it's lightweight, GitHub Action integration from the get go, open source and does not have account requirement.

  • Snyk has nice UX and large coverage in this area but it requires signup/signin.
  • Anchore is feature parity with Trivy so it's swappable between these two. I don't mind at all
  • Tenable doesn't get active contribution within last 4 year.
  • SonarQube is bulky and has all the features that I don't need just for a mere container security scan.

I haven't checked the remaining players so there could be a hidden gem somewhere that I missed.

@mistercrunch
Copy link
Member

Oh good to hear it's not blocking, though how will we know/notice when [silently] failing? I'm guessing it would rely on people looking for red mark in builds.

Wondering if [instead] we should set up a nightly job (GHA has a cron feature, so it's easy to schedule a daily job) against latest master and a matrix of "supported release" (maybe just the latest release for now) that would alert when failing. Maybe it sends an email to the dev@ mailing list when it finds an issue(?) and/or pings #committers on Slack on failure?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
github_actions Pull requests that update GitHub Actions code infra:container Infra container and K8s size/S
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants