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

Snyk vulnerability scan #3387

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open

Snyk vulnerability scan #3387

wants to merge 4 commits into from

Conversation

DaMandal0rian
Copy link
Member

@DaMandal0rian DaMandal0rian commented Feb 14, 2025

This PR introduces a GitHub Actions workflow to automate Docker image vulnerability scanning using Snyk, replacing the existing Trivy scanner due to recent issues with Trivy stability. The workflow enhances our security pipeline by identifying and reporting high/critical vulnerabilities in our Docker images.

Key Changes:

  1. Snyk Integration:

    • Scans Docker images (farmer, node, bootstrap-node, and gateway) for high-severity vulnerabilities.
    • Uses the snyk/actions/docker action for scanning, with a severity threshold set to high.
  2. SARIF Output Processing:

    • Post-processes SARIF output to replace undefined or null security severity values with 0, ensuring compatibility with GitHub Code Scanning.
    • Addresses issues where NVD CVSS scores are unavailable.
  3. GitHub Code Scanning Integration:

    • Uploads processed SARIF results to GitHub Code Scanning for centralized vulnerability tracking.
    • Results are categorized by image type for easy identification.
  4. Triggers:

    • Repository Dispatch: Triggered by a snyk-scan-dispatch event.
    • Manual Trigger: Can be manually initiated via the GitHub Actions UI.
  5. Replaces Trivy:

    • This workflow replaces the Trivy container scanner, which has been experiencing reliability and performance issues lately.

Why This Matters:

  • Improved Security: Ensures Docker images are free from high-severity vulnerabilities before deployment.
  • Automation: Reduces manual effort by automating vulnerability scanning and reporting.
  • Better Visibility: Integrates with GitHub Code Scanning for a centralized view of security issues.
  • Reliability: Snyk provides a more stable and reliable alternative to Trivy.

Testing:

  • All steps, including image building, Snyk scanning, SARIF post-processing, and result uploading, have been verified. Reference

This PR is a critical step toward improving the security and reliability of our Docker image scanning process.

Code contributor checklist:

Copy link
Member

@teor2345 teor2345 left a comment

Choose a reason for hiding this comment

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

It seems like we’re re-building the Docker images just for the scan. Is there a way to scan the images we’re distributing instead?

That would catch issues in the images we’re distributing to users (in most cases they’re similar, but we’re not doing reproducible builds, so there could be differences). It would also save us a bit of CI time.

Or is there a specific reason we’re rebuilding the images just for this scan?

@DaMandal0rian
Copy link
Member Author

DaMandal0rian commented Feb 17, 2025

It seems like we’re re-building the Docker images just for the scan. Is there a way to scan the images we’re distributing instead?

That would catch issues in the images we’re distributing to users (in most cases they’re similar, but we’re not doing reproducible builds, so there could be differences). It would also save us a bit of CI time.

Or is there a specific reason we’re rebuilding the images just for this scan?

Thanks @teor2345 yes, we can just scan the images, but the reason i went this direction is that building the image and scanning from the Dockerfile can be more accurate and find more obscured vulnerabilities than simply scanning the pre-built image. See https://docs.snyk.io/scan-with-snyk/snyk-container/use-snyk-container/detect-the-container-base-image#how-snyk-container-identifies-base-images

The CI builds are also done on GH runners and not self-hosted runners. When a new snapshot is built for distributing, the scan is triggered by our snapshot workflow through the dispatch event.

Copy link
Member

@nazar-pc nazar-pc left a comment

Choose a reason for hiding this comment

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

more accurate and find more obscured vulnerabilities than simply scanning the pre-built image

This once again goes back to the goals of this endeavor. If we build an image and it is fine, but the image our users are downloading is actually vulnerable, is it okay? I'd argue it is still an problem.

We either scan something for the purpose of scanning something, in which case any tool that produces any result is fine, or we want to achieve something specific, in which case it requires analysis of the approach and whether outcomes match the requirements.

@DaMandal0rian
Copy link
Member Author

DaMandal0rian commented Feb 17, 2025

more accurate and find more obscured vulnerabilities than simply scanning the pre-built image

This once again goes back to the goals of this endeavor. If we build an image and it is fine, but the image our users are downloading is actually vulnerable, is it okay? I'd argue it is still an problem.

We either scan something for the purpose of scanning something, in which case any tool that produces any result is fine, or we want to achieve something specific, in which case it requires analysis of the approach and whether outcomes match the requirements.

In that case we could run the test on push to main and capture the issues before shipping to the end users. The goal is to detect and fix critical issues beforehand, resolve if possible, and replace Trivy. An example we use ubuntu:22.04 image, which is fine, nothing critical or high, if we updated to next LTS release, it would eliminate many of them as shown in this image attached. Now without this tool, did we know that? Then we can make the decision depending on severity if we need to upgrade the base image or patch it, depending on severity and exploitation to end-users.

Screenshot 2025-02-17 at 4 28 08 PM

@teor2345
Copy link
Member

Or is there a specific reason we’re rebuilding the images just for this scan?

Thanks @teor2345 yes, we can just scan the images, but the reason i went this direction is that building the image and scanning from the Dockerfile can be more accurate and find more obscured vulnerabilities than simply scanning the pre-built image. See https://docs.snyk.io/scan-with-snyk/snyk-container/use-snyk-container/detect-the-container-base-image#how-snyk-container-identifies-base-images

That makes sense, would you mind adding a short comment about this, so someone doesn’t accidentally remove it later as part of making builds more “efficient”?

This once again goes back to the goals of this endeavor. If we build an image and it is fine, but the image our users are downloading is actually vulnerable, is it okay? I'd argue it is still a problem.

It seems like we might want to scan for three different reasons:

  1. avoid introducing new vulnerable dependencies
  2. discover new vulnerabilities in existing dependencies, and fix them
  3. detect new vulnerabilities while building and releasing images

I’m happy with any steps that move us towards those goals, as long as we make sure we eventually satisfy them all.

This PR seems to satisfy 1 & 2, and I think a high threshold is a reasonable start. Similarly, I think high would be reasonable for 3.

If they don’t detect enough issues, I’d be open to changing either of them to medium, but let’s see how they go first?

@nazar-pc
Copy link
Member

  1. discover new vulnerabilities in already published images

teor2345
teor2345 previously approved these changes Feb 19, 2025
Copy link
Member

@teor2345 teor2345 left a comment

Choose a reason for hiding this comment

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

Let's try this and see how it goes.

I opened ticket #3396 for the remaining reasons we want to scan images. (This PR only implements scanning of snapshot builds.)

@DaMandal0rian
Copy link
Member Author

DaMandal0rian commented Feb 20, 2025

That makes sense, would you mind adding a short comment about this, so someone doesn’t accidentally remove it later as part of making builds more “efficient”?

@teor2345 added the comments in a6963d2

Do we want to enable push event for main branch?

@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.

@teor2345
Copy link
Member

That makes sense, would you mind adding a short comment about this, so someone doesn’t accidentally remove it later as part of making builds more “efficient”?

@teor2345 added the comments in a6963d2

Do we want to enable push event for main branch?

I don’t think that’s a blocker for this PR, so I’m happy to follow it up in a separate PR.

Let’s see how the scanning goes first, there are a lot of potential vulnerabilities we need to work through here:
https://github.com/autonomys/subspace/security/code-scanning?query=pr%3A3387+is%3Aopen

If we add a new vulnerable dependency, I’d prefer to catch it before it merges, because backing code out is complicated. But if the scans are too noisy, or too expensive, maybe we just want to do it on main.

What do you think?

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.

3 participants