-
Notifications
You must be signed in to change notification settings - Fork 210
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
Make RedHat CVE import more robust #319
Conversation
vulnerabilities/importers/redhat.py
Outdated
page_no = 1 | ||
url = "https://access.redhat.com/hydra/rest/securitydata/cve.json?page={}" | ||
url_template = "https://access.redhat.com/hydra/rest/securitydata/cve.json?per_page=10000&page={}" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is way better !
impacted_package_urls=affected_purls, | ||
vuln_references=references, | ||
) | ||
|
||
|
||
def rpm_to_purl(rpm_string): | ||
# FIXME: there is code in scancode to handle RPM conversion AND this should |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tried nevra.py, but it's horrible here since, redhat has weird names with multiple -
.
The method used here is more conservative.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tried nevra.py, but it's horrible here since, redhat has weird names with multiple -.
I need to see actual real examples of real RPM package names that nevra.py cannot parse. If so this is a serious bug.
And https://github.com/nexB/vulnerablecode/blob/8efdbd190d1249e5f4bcda044f98ea50af8a86a4/vulnerabilities/tests/test_redhat_importer.py#L43 is not enough tests and foo is a not a real name: s=you should avoid to make use fake names here IMHO
We really want to have that correct and correctly done in one place only, that we can package in a library if needed.
@pombredanne FWIW, #290 also makes some changes to redhat, but your changes probably won't conflict with them. |
Tests are missing and need to be added here. |
Could you apply black on this ? |
f09a032
to
9a22a5e
Compare
All is working fine now, merge this as per your convenience |
This importer failed at times toward the end of the fetch step when reaching (and trying repeatidly) to re-fetch the last page. The fix consists in fetching larger batches at once (10K instead of 1k) which measn fewer API calls and less risk to be throttled and properly handling exceptions and HTTP response codes and breaking rather than retrying. This also adds some minimal logging. Signed-off-by: Philippe Ombredanne <[email protected]>
Signed-off-by: Philippe Ombredanne <[email protected]>
Signed-off-by: Shivam Sandbhor <[email protected]>
Signed-off-by: Shivam Sandbhor <[email protected]>
9a22a5e
to
9fbfe50
Compare
This importer failed at times toward the end of the fetch step when
reaching (and trying repeatidly) to re-fetch the last page.
The fix consists in fetching larger batches at once (10K instead of 1k)
which measn fewer API calls and less risk to be throttled and properly
handling exceptions and HTTP response codes and breaking rather than
retrying. This also adds some minimal logging.
Signed-off-by: Philippe Ombredanne [email protected]