Add unique CVE ID field to Vulnerability by haikoschol · Pull Request #125 · aboutcode-org/vulnerablecode

haikoschol · 2019-11-04T18:38:48Z

This change adds the field cve_id to the Vulnerability model and based
on that, improves the data import for Arch Linux.

The improvements made have been discussed in issue #20:

For each CVE in a given AVG, exactly one Vulnerability is stored
For each CVE, one VulnerabilityReference to its page on
security.archlinux.org is stored
Each ASA mentioned in an AVG is stored as a VulnerabilityReference

Since there is no production deployment of vulnerablecode yet, I took
the opportunity of changing the models to remove all migrations and
create a new one that creates the whole schema.

Since the cve_id field on Vulnerability has a unique constraint set, I
needed to make some changes to the import code that belong to issue #28.
I kept them minimal however so #28 is still open and needs to be
addressed later.

closes #20

Signed-off-by: Haiko Schol hs@haikoschol.com

pombredanne

@haikoschol Thank you ++
See my comments inline ... all are mostly small nitpickings.

pombredanne · 2019-11-06T15:28:34Z

-            vulnerability=vulnerability,
-            reference_id=data.get('vulnerability_id', ''),
+        vulnerability, _ = Vulnerability.objects.get_or_create(
+            cve_id=data['cve_id'],


What if there is no cve_id in the data? will this crash? And in this case we would likely still want to create a new Vulnerability?

This is for the Debian import, where it doesn't look like there will be any entries without CVE ID. At least not in the feed we are currently consuming. The CVE ID is used as a dictionary key:

{ "librsync": { "CVE-2014-8242": { "scope": "remote", "debianbug": 776246, "description": "librsync before 1.0.0 uses a truncated MD4 checksum to match blocks", [...]

pombredanne · 2019-11-06T15:30:33Z

-            PackageReference.objects.create(
-                package=package_affected,
-                repository=f'https://security.archlinux.org/package/{package_name}',
+                version=avg['affected'],


Is affected always a single version?

It was a string in all the records I looked at so I'm pretty sure, but haven't tried to verify it any further.

In general I think it's not really a problem if the import crashes due to unexpected schemas. In fact, I'd argue we want it to crash as soon as possible instead of writing lots of empty values into the database due to overly defensive code.
We just need to make sure to be notified about any crashes that happen in cron jobs and such.

pombredanne · 2019-11-06T15:36:31Z

    """
-    Seperate list of Affected version and fixed version from all version
-    using the range specified
+    Seperate list of affected versions and fixed versions from all version


nitpicking: Separate list of affected versions and fixed versions from all versions

Also unrelated, could it make sense to adopt "affect/fixed" rather "impacted/resolved" ? is this a better wording and more explicit?

I think either is fine. We should make sure to be consistent in the data we store, API responses, documentation, etc. but I'm fine sticking with impacted/resolved. If you are in favor of affected/fixed we should change it now though, since this PR already resets migrations.

pombredanne · 2019-11-06T15:37:14Z

+    using the ranges specified
    """
-
+    # FIXME This skips unfixed vulnerabilities


Good catch... this warrants a ticket

Right. I meant to create one for it but apparently forgot. :-/ Done that now: #126

pombredanne · 2019-11-06T15:40:57Z

        vuln = result['vulnerabilities'][0]
-        self.assertEqual(1, len(vuln['references']))
-        self.assertEqual('CVE-2012-3386', vuln['references'][0]['reference_id'])
+        self.assertEqual(0, len(vuln['references']))


Note: if we decide to use pytest and its dnajo extension, we could start using plain asserts everywhere.

Right, I had that thought as well. Wrote it down in a ticket now: #127

pombredanne · 2019-11-06T15:43:20Z

-        self.assertTrue(VulnerabilityReference.objects.get(reference_id='CVE-2009-1382'))
-        self.assertTrue(VulnerabilityReference.objects.get(reference_id='CVE-2009-2459'))
-        self.assertTrue(VulnerabilityReference.objects.get(reference_id='CVE-2014-8242'))
+        self.assertEqual(0, VulnerabilityReference.objects.count())


Is this right? or needed at all? I just realize that the previous code was making assumptions about data across tests.

It is correct, although it's not ideal for the test, that for this data no vulnerability references are created any more. :)

I'm not sure what you mean with "assumptions about data across tests" though. The way the tests work now with setUpTestData() is that the data is loaded once and then used for assertions in all test methods. Since all of the tests are read-only, it should not make a difference in outcome while being a bit more efficient. Should be easy to switch to plain setUp()/tearDown() though if necessary.

This test is for the debian import and since this PR is for an issue about Arch, I wanted to keep the changes minimal. But I just checked and Debian has pages for CVEs just like Arch, so we could add those as vulnerability references.

Should be easy to switch to plain setUp()/tearDown() though if necessary.

My point there was I personally find that the use of setUp()/tearDown() and similar to be confusing and making the tests less readable and less "unit" by bringing in some global state . But that's minor and nothing to fix just now.

pombredanne · 2019-11-06T15:46:44Z

-        self.assertEqual(1, len(impacted_pkgs))
-        self.assertEqual('librsync', impacted_pkg.package.name)
-        self.assertEqual('0.9.7-10', impacted_pkg.package.version)
+        self.assertEqual(1, impacted_pkgs.count())


I am always puzzled by tests that make assertions about things done in the setup or elsewhere... this was not your making, but if we could find a way to have tests that are really unit and do not depend on each other and on global setup() that would be better IMHO... what's your take? @tdruez ping too

I might actually have changed that in an earlier PR from having the import called in every test, which was tedious to change. If we use pytest we also have the option to write tests as simple functions instead of classes. If we do that I would still prefer to reuse setup code where it makes sense, either by calling a setup function in the test or using pytest decorators. Maybe that way it is more explicit?

pombredanne · 2019-11-06T15:50:10Z

+        Check that no package references were found in the test data
        """
-        self.assertEqual(8, PackageReference.objects.count())
+        self.assertEqual(0, PackageReference.objects.count())


What is the purpose of this test?

We had a discussion on the chat a while ago about the purpose of PackageReference after we added the PURL mixin. I don't think any decisions/tickets came out of it, but until we decide what to do with it, this just checks that the ArchLinux import does not currently create any package references. For example we might eventually ask ourselves "What are we using PackageReference for?" this will answer it for Arch.

pombredanne · 2019-11-06T15:52:13Z


 from django.test import TestCase
-from vulnerabilities.scraper.npm import remove_spaces, get_all_version, extract_data
+from vulnerabilities.scraper.npm import remove_spaces, get_all_versions, extract_data


I personally prefer having each import on its own line.
Also separate imports in four sorted group with an empty line between each: future import, standard lib, thirdparty, own modules. Would this work for you too?

This change adds the field cve_id to the Vulnerability model and based on that, improves the data import for Arch Linux. The improvements made have been discussed in issue aboutcode-org#20: - For each CVE in a given AVG, exactly one Vulnerability is stored - For each CVE, one VulnerabilityReference to its page on security.archlinux.org is stored - Each ASA mentioned in an AVG is stored as a VulnerabilityReference Since there is no production deployment of vulnerablecode yet, I took the opportunity of changing the models to remove all migrations and create a new one that creates the whole schema. Since the cve_id field on Vulnerability has a unique constraint set, I needed to make some changes to the import code that belong to issue aboutcode-org#28. I kept them minimal however so aboutcode-org#28 is still open and needs to be addressed later. closes aboutcode-org#20 Signed-off-by: Haiko Schol <hs@haikoschol.com>

pombredanne

Thanks for all the replies. Merging !

haikoschol requested a review from pombredanne November 4, 2019 18:40

pombredanne requested changes Nov 6, 2019

View reviewed changes

haikoschol force-pushed the arch-import-fixes branch from 794f733 to 9312d26 Compare November 6, 2019 16:07

haikoschol force-pushed the arch-import-fixes branch from 9312d26 to 5c9493d Compare November 6, 2019 16:44

pombredanne approved these changes Nov 10, 2019

View reviewed changes

pombredanne merged commit 6f08ace into aboutcode-org:develop Nov 10, 2019

haikoschol deleted the arch-import-fixes branch April 10, 2023 00:44

Uh oh!

Uh oh!

Conversation

haikoschol commented Nov 4, 2019

Uh oh!

pombredanne left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pombredanne left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants