Skip to content

feat: support running detectors#2021

Merged
another-rex merged 8 commits into
google:mainfrom
ackama:support-detectors
Jul 21, 2025
Merged

feat: support running detectors#2021
another-rex merged 8 commits into
google:mainfrom
ackama:support-detectors

Conversation

@G-Rath
Copy link
Copy Markdown
Collaborator

@G-Rath G-Rath commented Jul 7, 2025

This adds experimental support for having the scanner run detectors which are plugins aimed at identifying vulnerabilities through more interactive means than extractors such as by reading config files or making HTTP requests with crafted payloads.

Currently this does not have a lot of tests as only a handful of detectors exist and most are based on very specific situations which cannot be changed without shipping support for custom configuration (which is its own whole thing), but I have managed to at least get one with a positive finding using the weakcredentials/etcshadow detector scanning a custom image.

Also this does not remove the requirement of at least one extractor being enabled and a package found since that too would be a much larger change and with the current pool of detectors available being so limited I don't think it's appropriate to drop that requirement just yet.

Resolves #1969

@G-Rath G-Rath force-pushed the support-detectors branch 4 times, most recently from 6935cce to c1cf931 Compare July 7, 2025 03:44
hogo6002 pushed a commit that referenced this pull request Jul 7, 2025
Only one of these tests actually uses `--all-packages` but they all
output in JSON format so I think this is a better name and that also
means its reasonable for us to add more cases such as in #2021
@G-Rath G-Rath force-pushed the support-detectors branch 3 times, most recently from c6483e0 to 0ff98a9 Compare July 10, 2025 00:45
@G-Rath G-Rath force-pushed the support-detectors branch 2 times, most recently from bf0d582 to 97ca687 Compare July 10, 2025 01:28
@codecov-commenter
Copy link
Copy Markdown

codecov-commenter commented Jul 10, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 67.61%. Comparing base (95d840a) to head (40f4490).
⚠️ Report is 455 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2021      +/-   ##
==========================================
+ Coverage   67.50%   67.61%   +0.11%     
==========================================
  Files         172      173       +1     
  Lines       16242    16293      +51     
==========================================
+ Hits        10964    11017      +53     
+ Misses       4603     4601       -2     
  Partials      675      675              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@G-Rath
Copy link
Copy Markdown
Collaborator Author

G-Rath commented Jul 10, 2025

@another-rex turns out I just completely forgot about configuring the snapshot normalizers for JSON 🤦

I think this is good to go, and probably best to land as-is so it can be used, and get things improved in follow up, specifically:

  • supporting it in more output formats
  • reusing the detector building, in particular trying to leverage list.DetectorsFromName()
    • (I've started looking into this but frankly InitMap has been giving me a headache and I'm thinking it might be easier to tackle once we've refactored extractors in a similar manner per our convo the other day)

@G-Rath
Copy link
Copy Markdown
Collaborator Author

G-Rath commented Jul 10, 2025

hmm so the failures on Windows are from detectors such as weakcreds/filesystem in the container tests, which is interesting as that detector should only run on a running Linux system which we're not in - I think this is because we are not actually doing any filtering and scalibr only applies its filtering in some situations? But I think it would be good to get a review and check-in from you either way so I'll wait to hear from you before digging deeper

@G-Rath G-Rath requested a review from another-rex July 10, 2025 01:51
@G-Rath G-Rath force-pushed the support-detectors branch 3 times, most recently from 38458e8 to 62afcdf Compare July 16, 2025 04:26
Copy link
Copy Markdown
Collaborator

@another-rex another-rex left a comment

Choose a reason for hiding this comment

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

To make the detector presets more useful, I think we need to do the capabilities filtering ourselves on OSV-Scanner's side, and auto-disable detectors that have requirements that doesn't match.

I'm a bit confused on the windows specific errors, since for container scanning we should be scanning a linux system, even on windows, so I'm not sure why there are those errors appearing.


// todo: rethink this life choice...
for name, enabled := range detectors {
if name != "" && enabled {
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

We can probably invert some of these if statements

Name: "scanning_insecure_alpine_image_with_detector_preset",
Args: []string{
"", "image", "--format=json",
"--experimental-detectors", "weakcreds",
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I'm surprised this is running at all, since in ValidatePluginRequirements in scalibr it should fail if we are running some of these extractors on containers.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Oh, ScanContainer is not checking for capabilities... that's not intended I believe.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

*and we are also not passing in any capabilities

@G-Rath G-Rath force-pushed the support-detectors branch 3 times, most recently from ef6c913 to f2713b1 Compare July 17, 2025 00:23
@G-Rath G-Rath force-pushed the support-detectors branch from f2713b1 to 51f787c Compare July 17, 2025 00:42
@G-Rath G-Rath force-pushed the support-detectors branch from 51f787c to 4479370 Compare July 17, 2025 00:52
@G-Rath G-Rath force-pushed the support-detectors branch 3 times, most recently from 1cc9446 to 7d5a5e7 Compare July 17, 2025 03:11
@G-Rath G-Rath force-pushed the support-detectors branch 2 times, most recently from ed70c09 to 78ffc51 Compare July 17, 2025 03:55
@G-Rath G-Rath force-pushed the support-detectors branch from 78ffc51 to 40f4490 Compare July 17, 2025 04:11
@G-Rath G-Rath marked this pull request as ready for review July 20, 2025 23:30
@another-rex another-rex merged commit 641d7a9 into google:main Jul 21, 2025
15 checks passed
@another-rex another-rex deleted the support-detectors branch July 21, 2025 00:23
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.

Allow detectors to be specified as plugins that can be enabled in osv-scanner

3 participants