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

System version more plugins #1317

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

Conversation

jstucke
Copy link
Collaborator

@jstucke jstucke commented Dec 12, 2024

@jstucke jstucke self-assigned this Dec 12, 2024
@codecov-commenter
Copy link

codecov-commenter commented Dec 12, 2024

Codecov Report

Attention: Patch coverage is 79.48718% with 8 lines in your changes missing coverage. Please review.

Project coverage is 91.84%. Comparing base (ab95ac5) to head (181850b).
Report is 1 commits behind head on master.

Files with missing lines Patch % Lines
...ugins/analysis/cve_lookup/internal/data_parsing.py 52.94% 8 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1317      +/-   ##
==========================================
- Coverage   92.48%   91.84%   -0.64%     
==========================================
  Files         379      378       -1     
  Lines       24115    21157    -2958     
==========================================
- Hits        22302    19432    -2870     
+ Misses       1813     1725      -88     

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

@maringuu
Copy link
Collaborator

What problem does this solve?
I do not quite understand how the system version is useful.

@jstucke
Copy link
Collaborator Author

jstucke commented Dec 18, 2024

What problem does this solve? I do not quite understand how the system version is useful.

It is useful for two things:

  • track the internal version of the data that the analysis is based on (since the outcome of the analysis may change based on this data)
  • if you run the analysis again with the same version of the plugin, it depends on the system version whether or not the analysis is run again or skipped (and since the outcome may change, it makes sense to update it)

@jstucke jstucke force-pushed the system-version-more-plugins branch from 1ff0d2c to 181850b Compare January 9, 2025 10:16
@maringuu
Copy link
Collaborator

To me the system version still seems like a design flaw.
Imo it would make more sense to fix the version of whatever tool a plugin needs and bump the plugin version if we update the tool.

Let's consider the file_type plugin for example.
If I'm on ubuntu and happen to update my system (via apt upgrade) and the file package is updated, this results in a new system version for the file_type plugin.
When I then upload a new firmware image to FACT, the results are different than what I would have gotten when I uploaded the same firmware before upgrading file.
To me this does not make sense. The analysis results should not change, if I didn't touch the fact install.

@jstucke
Copy link
Collaborator Author

jstucke commented Jan 16, 2025

I tend to agree with you. That is one of the reasons why we don't install some tools through apt: To get reproducible results. In some cases it is hard to address this, e.g. in case of the cve_lookup plugin it wouldn't make a lot of sense to pin the commit of the CVE sources (because you would need to update the plugin all the time to get the latest entries). But it adds some meta information which makes it easier to track why we see certain results. Maybe it would make sense to remove the "system version" from scheduling.

And I think that we should try to eliminate as many sources of indeterminism/uncertainty as possible (like the version of file/libmagic). In the case of file/libmagic I'm not sure what the best way to do that would look like, though: compiling it adds unnecessary installation overhead and running it inside a docker container adds runtime overhead during analysis (which is even worse). Maybe downloading a Debian/Ubuntu package with the pre-compiled magic file and extracting it could work 🙈

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