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

fix: handle unknown values in metric_finder (Fixes #4578) #4682

Merged
merged 4 commits into from
Feb 13, 2025

Conversation

vedpawar2254
Copy link
Contributor

Fixes #4578 ([CVEDB] Why does the function metric_finder returns unknown or a ### metrics_id)

Added "UNKNOWN" Metric
Implemented a method (ensure_unknown_metric) to ensure it exists
and updated the metric_finder function

I removed the commits from the other PR, you can check this one out while i'll look into why the tests for #4654 are failing

Thanks and lemme know if we need to change anything

Copy link
Contributor

@terriko terriko left a comment

Choose a reason for hiding this comment

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

Looks like you're failing a couple of lint checks because there's some whitespace on a blank line. I've added a comment where I think it's happening, but the best way to be sure it's fixed is to run black cve_bin_tool/cvedb.py then check your file back in and push it to the PR branch.

Here's some more info on our linters in case you've never run those before:
https://github.com/intel/cve-bin-tool/blob/main/CONTRIBUTING.md#running-linters

It's also complaining about the PR title because it doesn't conform to the commit message format we use, which is https://www.conventionalcommits.org/ -- I'll change the title for you now so it should pass next time but the linter won't run again until you update the branch.

@@ -416,6 +417,9 @@ def init_database(self) -> None:
for table in self.TABLE_SCHEMAS:
cursor.execute(self.TABLE_SCHEMAS[table])

# Ensure the UNKNOWN metric exists
self.ensure_unknown_metric(cursor)

Copy link
Contributor

Choose a reason for hiding this comment

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

The blank line here has some extra spaces or a tab in it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi @terriko , it should be good now, my vs code extensions are messing with me 😅, also there are some broken links in MANUAL.md as pointed out by @Molkree, i'll raise a pr to fix it, should i also create a issue.
I think I'll also checkout the other docs to see if there are any broken links or smtg

@terriko terriko changed the title Fixes #4578 ([CVEDB] Why does the function metric_finder returns unkn… fix: handle unknown values in metric_finder (Fixes #4578) Jan 10, 2025
@vedpawar2254
Copy link
Contributor Author

@terriko can you see why the test case is failing, i'd appreciate it

@terriko
Copy link
Contributor

terriko commented Jan 21, 2025

Not sure what's happening there off the top of my head. I'm going to re-run the failing test because we've had a bunch of weirdness with the cache, but I'll flag this so I come back and look at it if it doesn't pass.

Copy link

@jloehel jloehel left a comment

Choose a reason for hiding this comment

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

Just my 2¢, to move this forward. @terriko I hope it's possible to merge it soon.

"""
query = """
SELECT metrics_id FROM metrics
WHERE metrics_id=?
"""
metric = None
if cve["CVSS_version"] == "unknown":
metric = "unknown"
metric = 0
Copy link

Choose a reason for hiding this comment

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

I would use UNKNOWN_METRIC_ID just to avoid magic numbers it increases the readability.

Choose a reason for hiding this comment

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

+1

cursor = self.db_open_and_get_cursor()
try:
yield cursor
finally:
self.db_close()

def ensure_unknown_metric(self, cursor):
Copy link

Choose a reason for hiding this comment

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

Why not moving this to populate_metrics and ensure that the table metrics gets also monitored for changes like in example cve_range in get_cvelist_if_stale.

@krushndayshmookh
Copy link

Patching myself here to follow up with further conversations.

Copy link
Contributor

@terriko terriko left a comment

Choose a reason for hiding this comment

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

See jloehe's comments. I think at least the UNKNOWN_METRIC_ID change would be good here.

@vedpawar2254
Copy link
Contributor Author

@terriko, i changed the UNKNOWN_METRIC_ID but i am not getting what the other change should be, can you help me out a lil here

@terriko terriko added the awaiting maintainer Need a maintainer to respond / help out label Feb 7, 2025
@jloehel
Copy link

jloehel commented Feb 10, 2025

I mean populate_metrics ensures that the metric ids for EPSS, CVSS .. exist. Why not moving the logic of the new function ensure_unknown_metric there?

def populate_metrics(self):
"""Adding data to metric table."""
cursor = self.db_open_and_get_cursor()
# Insert a row without specifying cve_metrics_id
insert_metrics = self.INSERT_QUERIES["insert_metrics"]
data = [
(EPSS_METRIC_ID, "EPSS"),
(CVSS_2_METRIC_ID, "CVSS-2"),
(CVSS_3_METRIC_ID, "CVSS-3"),
]
# Execute the insert query for each row
for row in data:
cursor.execute(insert_metrics, row)
self.connection.commit()
self.db_close()

That means add UNKNOWN_METRIC_ID to the data.

But for this it's also necessary to change the logic for when an update (populate call) is necessary. Right now it's necessary if :

  1. the db file does not exist
  2. the db file is older than 24 hours
  3. the schemas for cve_severity, cve_range and cve_exploited have changed

A fourth condition is necessary to trigger the update if the METRIC IDs have changed.

Copy link
Contributor

@terriko terriko left a comment

Choose a reason for hiding this comment

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

I think @jloehel has the right idea here, but since this PR will at least give us a reasonable working fix right now, I'm going to go ahead and merge it as is and open a new issue with the recommended refactor.

@terriko terriko merged commit 4e1caf9 into intel:main Feb 13, 2025
23 of 24 checks passed
@vedpawar2254
Copy link
Contributor Author

Ok @terriko, thanks. I was actually working on that 🫣

22f1001635 added a commit to 22f1001635/cve-bin-tool that referenced this pull request Mar 19, 2025
22f1001635 added a commit to 22f1001635/cve-bin-tool that referenced this pull request Mar 19, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
awaiting CI awaiting maintainer Need a maintainer to respond / help out
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[CVEDB] Why does the function metric_finder returns unknown or a metrics_id
4 participants