Add patched package #436

sbs2001 · 2021-04-09T06:17:41Z

Fixes #375

I've done a terrible job at naming things here(see review comments), need help there.

We would want to move all the intermediate data structures to one place. That will be done in another PR.

See high level overview at #436 (comment)

sbs2001 · 2021-04-19T10:42:35Z

vulnerabilities/tests/test_models.py

+
+# FIXME: The fixture code is duplicated. setUpClass is not working with the pytest mark.
+@pytest.mark.django_db
+class TestPackageRelatedVulnerablity(TestCase):


@tdruez these are the tests

sbs2001 · 2021-04-19T10:49:48Z

vulnerabilities/data_source.py

@@ -87,17 +88,16 @@ class Advisory:

    summary: str
    vulnerability_id: Optional[str] = None
-    impacted_package_urls: Iterable[PackageURL] = dataclasses.field(default_factory=list)
-    resolved_package_urls: Iterable[PackageURL] = dataclasses.field(default_factory=list)
+    affected_packages_with_patched_package: List[


Name problem 1

Could you survey the words use to depict affected/patched in other tools?

kaybee/vulas

dependencycheck and dependency track (and OWASP)

in general all the data source we use
...
with these listed here we can make a good pick together!

The general renaming should be tracked in a separate ticket IMHO, not here.

Beyond this, what about affected_packages or just impacted_packages?

sbs2001 · 2021-04-19T10:50:30Z

vulnerabilities/helpers.py

+
+
+@dataclasses.dataclass(order=True, frozen=True)
+class AffectedPackageWithPatchedPackage:


This will be moved somewhere else in another PR

Use AffectedPackageWithPatch

Or just AffectedPackage ?

sbs2001 · 2021-04-19T10:51:20Z

vulnerabilities/helpers.py

-    """
-    Return True if the input 'string' contains any alphabet
-    """
+def nearest_patched_package(vulnerable_packages, resolved_packages):


This is where the magic happens, again it has poor naming. Help.

we will fix that in a separate ticket.

pombredanne

Some commits message is just "WIP"... may be there is a better message to reword? 👼

pombredanne

Thanks!
We need to find better names alright and settle on this once and for all IMHO.
vulnerable/affected/resolved/patched are used and I feel we are not consistent.

Also can you explain the high level design here?

pombredanne · 2021-04-20T08:15:51Z

vulnerabilities/admin.py

@@ -51,7 +51,7 @@ class PackageAdmin(admin.ModelAdmin):

 @admin.register(PackageRelatedVulnerability)
 class PackageRelatedVulnerabilityAdmin(admin.ModelAdmin):
-    list_filter = ("is_vulnerable", "package__type", "package__namespace")
+    list_filter = ("package__type", "package__namespace")


Is there anything that could replace is_vulnerable?

that should be patched_package adding it.

pombredanne · 2021-04-20T08:21:29Z

vulnerabilities/data_source.py

@@ -87,17 +88,16 @@ class Advisory:

    summary: str
    vulnerability_id: Optional[str] = None
-    impacted_package_urls: Iterable[PackageURL] = dataclasses.field(default_factory=list)
-    resolved_package_urls: Iterable[PackageURL] = dataclasses.field(default_factory=list)
+    affected_packages_with_patched_package: List[


Could you survey the words use to depict affected/patched in other tools?

kaybee/vulas

dependencycheck and dependency track (and OWASP)

in general all the data source we use
...
with these listed here we can make a good pick together!

pombredanne · 2021-04-20T08:27:23Z

vulnerabilities/helpers.py

-is_cve = re.compile(r"CVE-\d{4}-\d{4,7}", re.IGNORECASE).match
+def contains_alpha(string):
+    """
+    Return True if the input 'string' contains any alphabet


Suggested change

Return True if the input 'string' contains any alphabet

Return True if the input 'string' contains any alphanumeric character```

pombredanne · 2021-04-20T08:28:04Z

vulnerabilities/helpers.py

-    Return True if the input 'string' contains any alphabet
-    """
+def nearest_patched_package(vulnerable_packages, resolved_packages):
+    # This class is used to  get around bisect module's lack of supplying custom


This should be a docstring

pombredanne · 2021-04-20T08:29:39Z

vulnerabilities/helpers.py

+    )
+
+    resolved_package_count = len(resolved_packages)
+    affected_package_with_patched_package_objects = []


Why the _objects suffix?

Suggested change

affected_package_with_patched_package_objects = []

affected_package_with_patched_packages = []

I'm aware of the principle of not specifying the type. The problem here is this list contains instances of AffectedPackageWithPatchedPackage . I don't know how to pluralise that,

affected_package_with_patched_packages -> is it one one affected package with multiple patched packages
affected_packages_with_patched_package -> vice versa

This is a mess :(

Based on other review, affected_package would be a better name for now.

pombredanne · 2021-04-20T08:31:47Z

vulnerabilities/importer_yielder.py

-        "data_source": "SUSEBackportsDataSource",
-        "data_source_cfg": {"url": "http://ftp.suse.com/pub/projects/security/yaml/", "etags": {}},
-    },
+    # {


Why is this disabled?

We probably want to get rid of that importer, since it only contains (vulnerability, fixed/backported package)

sbs2001 · 2021-04-20T09:42:49Z

@pombredanne here's a high level overview:

This PR removes the notion of resolved_package essentially only storing packages which are vulnerable. It also links these vulnerable packages with nearest(version-wise) package which is safe.

On a lower level, this required :

Removing PackageRelatedVulnerability's is_vulnerable flag, since now we only store vulnerable packages primarily.
Adding a field called patched_package to PackageRelatedVulnerability . This is a foreign key to the nearest(wrt PackageRelatedVulnerability.package version wise) safe package .
Implementing a method called nearest_patched_package which accepts 2 lists of vulnerable packages and safe packages (wrt to some same vulnerablity) respectively. This method does the following:

a. Sort both the lists , ie the packages are now in ascending order wrt their versions. univers is used here.
b. Iterate over each vulnerable package, let's call the package being iterated pkg. For each pkg we find a package
which is just greater(version-wise ) than itself in the safe packages list. See the code here

c. If there exists such a package in (b) we bind this safe package with pkg by creating a AffectedPackageWithPatchedPackage instance

pombredanne

I added a few more comments for your consideration and please merge.
Let's have a separate ticket for the naming issue.

pombredanne · 2021-04-21T13:06:29Z

vulnerabilities/data_source.py

@@ -87,17 +88,16 @@ class Advisory:

    summary: str
    vulnerability_id: Optional[str] = None
-    impacted_package_urls: Iterable[PackageURL] = dataclasses.field(default_factory=list)
-    resolved_package_urls: Iterable[PackageURL] = dataclasses.field(default_factory=list)
+    affected_packages_with_patched_package: List[


The general renaming should be tracked in a separate ticket IMHO, not here.

Beyond this, what about affected_packages or just impacted_packages?

pombredanne · 2021-04-21T13:09:05Z

vulnerabilities/helpers.py

+
+
+@dataclasses.dataclass(order=True, frozen=True)
+class AffectedPackageWithPatchedPackage:


Or just AffectedPackage ?

pombredanne · 2021-04-21T13:11:44Z

vulnerabilities/helpers.py

+@dataclasses.dataclass(order=True, frozen=True)
+class AffectedPackageWithPatchedPackage:
+    vulnerable_package: PackageURL
+    patched_package: Optional[PackageURL] = None


What about:

Suggested change

patched_package: Optional[PackageURL] = None

patched_version: Optional[str] = None

pombredanne · 2021-04-21T13:23:35Z

vulnerabilities/helpers.py

-    """
-    Return True if the input 'string' contains any alphabet
-    """
+def nearest_patched_package(vulnerable_packages, resolved_packages):


we will fix that in a separate ticket.

pombredanne · 2021-04-21T13:30:04Z

vulnerabilities/helpers.py

+    # This class is used to  get around bisect module's lack of supplying custom
+    # compartor. Get rid of this once we use python 3.10 which supports this.
+    # See https://github.com/python/cpython/pull/20556
+    class PackageURLWithVersionComparator:


Are you missing a @total_ordering there?

Also you should mention this is only to compare packages of the same type OR you could implement comparison and sorting across types.

You may want to check that the type/ns/name are the same too

pombredanne · 2021-04-21T13:32:26Z

vulnerabilities/helpers.py

+    )
+
+    resolved_package_count = len(resolved_packages)
+    affected_package_with_patched_package_objects = []


Based on other review, affected_package would be a better name for now.

Notes about migrations : - Remove the is_vulnerable flag from PackageRelatedVulnerability model - Add the patched_package fk in PackageRelatedVulnerability model - Add data migrations to populate the patched_package Signed-off-by: Shivam Sandbhor <[email protected]>

Signed-off-by: Shivam Sandbhor <[email protected]>

…stances Signed-off-by: Shivam Sandbhor <[email protected]>

Signed-off-by: Shivam Sandbhor <[email protected]>

sbs2001 force-pushed the add-patched-pkg branch 4 times, most recently from c342565 to 25c6972 Compare April 12, 2021 06:22

sbs2001 changed the title ~~[WIP] Add patched package~~ Add patched package Apr 19, 2021

sbs2001 requested review from pombredanne and tdruez April 19, 2021 10:41

sbs2001 commented Apr 19, 2021

View reviewed changes

pombredanne requested changes Apr 20, 2021

View reviewed changes

pombredanne reviewed Apr 20, 2021

View reviewed changes

sbs2001 force-pushed the add-patched-pkg branch 2 times, most recently from 39fe25b to efeea4f Compare April 21, 2021 06:27

pombredanne approved these changes Apr 21, 2021

View reviewed changes

sbs2001 added 13 commits April 22, 2021 15:55

Refactor the import process to implement patched_package

7021526

Signed-off-by: Shivam Sandbhor <[email protected]>

Refactor alpine importer and it's tests fwrt new models

d684871

Signed-off-by: Shivam Sandbhor <[email protected]>

Refactor debian importer and it's tests wrt to new models

8bc91ff

Signed-off-by: Shivam Sandbhor <[email protected]>

Refactor apache kafka and it's tests wrt new models

cea3738

Signed-off-by: Shivam Sandbhor <[email protected]>

Refactor apache tomcata and it's importers for new models

b0ccf83

Signed-off-by: Shivam Sandbhor <[email protected]>

Refactor archlinux importer wrt new models

5344e95

Signed-off-by: Shivam Sandbhor <[email protected]>

Refactor elixr security importer wrt new models

6607c29

Signed-off-by: Shivam Sandbhor <[email protected]>

Refactor gentoo importer wrt new models and update univers

d430a21

Signed-off-by: Shivam Sandbhor <[email protected]>

Use AffectedPackageWithPatched dataclass and refactor the Advisory in…

46fd0a0

…stances Signed-off-by: Shivam Sandbhor <[email protected]>

Refactor github importer wrt new models

1508274

Signed-off-by: Shivam Sandbhor <[email protected]>

Refactor istio importer wrt new models

2b3334f

Signed-off-by: Shivam Sandbhor <[email protected]>

Refactor kaybee importer wrt new models

1cae542

Signed-off-by: Shivam Sandbhor <[email protected]>

sbs2001 added 20 commits April 22, 2021 15:57

Refactor nginx importer wrt new models

f73fbc9

Signed-off-by: Shivam Sandbhor <[email protected]>

Refactor npm importer wrt new models

e97c415

Signed-off-by: Shivam Sandbhor <[email protected]>

Refactor openssl wrt new models

c1c0d14

Signed-off-by: Shivam Sandbhor <[email protected]>

Refactor postgresql importer

e9cd094

Signed-off-by: Shivam Sandbhor <[email protected]>

Refactor msr2019 importer

db17f09

Signed-off-by: Shivam Sandbhor <[email protected]>

Refactor redhat importer

9f6bb60

Signed-off-by: Shivam Sandbhor <[email protected]>

Refactor retire dot net

92fd1ad

Signed-off-by: Shivam Sandbhor <[email protected]>

Refactor ruby importer

30b0b49

Signed-off-by: Shivam Sandbhor <[email protected]>

Refactor rust importer and simplify it's tests

c852afc

Signed-off-by: Shivam Sandbhor <[email protected]>

Refactor safetydb wrt new models and simplify tests

d1f5485

Signed-off-by: Shivam Sandbhor <[email protected]>

Disable suse backport importer and refactor suse score importer

527324f

Signed-off-by: Shivam Sandbhor <[email protected]>

Refactor ubuntu usn importer and tests

29f1185

Signed-off-by: Shivam Sandbhor <[email protected]>

Refactor ubuntu

c12e985

Signed-off-by: Shivam Sandbhor <[email protected]>

Fix more tests and refactor importer_runner

cbd6c73

Signed-off-by: Shivam Sandbhor <[email protected]>

Update fixtures

9246553

Signed-off-by: Shivam Sandbhor <[email protected]>

Fix model relations and patch inference in importers

36f4d1d

Signed-off-by: Shivam Sandbhor <[email protected]>

Simplify patch inference, fix arch importer bug and remove useless code

5999a8a

Signed-off-by: Shivam Sandbhor <[email protected]>

Fix codestyle

d70a931

Signed-off-by: Shivam Sandbhor <[email protected]>

Add docstrings for model properties

5973bbc

Signed-off-by: Shivam Sandbhor <[email protected]>

Improve naming and docs

cc5bbb4

Signed-off-by: Shivam Sandbhor <[email protected]>

sbs2001 force-pushed the add-patched-pkg branch from ff833e4 to cc5bbb4 Compare April 22, 2021 10:28

sbs2001 merged commit fd15724 into aboutcode-org:main Apr 22, 2021

This was referenced Apr 22, 2021

Improve naming and be consistent about it #447

Open

Collect Mozilla #393

Merged

Make distinction between patched and unaffected packages #144

Closed

Hritik14 mentioned this pull request Apr 26, 2021

Importers without a vulnerable package list #449

Closed

pombredanne mentioned this pull request May 24, 2021

Store version ranges #140

Closed

Hritik14 mentioned this pull request Jul 5, 2021

Advisories created by alpine importer contain no version information #496

Open

Hritik14 mentioned this pull request Jul 15, 2021

Refactor alpine importer #499

Open

6 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add patched package #436

Add patched package #436

sbs2001 commented Apr 9, 2021 •

edited

Loading

sbs2001 Apr 19, 2021

sbs2001 Apr 19, 2021

pombredanne Apr 20, 2021

pombredanne Apr 21, 2021

sbs2001 Apr 19, 2021

sbs2001 Apr 21, 2021

pombredanne Apr 21, 2021

sbs2001 Apr 19, 2021

pombredanne Apr 21, 2021

pombredanne left a comment

pombredanne left a comment

pombredanne Apr 20, 2021

sbs2001 Apr 20, 2021

pombredanne Apr 20, 2021

pombredanne Apr 20, 2021

pombredanne Apr 20, 2021

pombredanne Apr 20, 2021

sbs2001 Apr 20, 2021

pombredanne Apr 21, 2021

pombredanne Apr 20, 2021

sbs2001 Apr 20, 2021

sbs2001 commented Apr 20, 2021 •

edited

Loading

pombredanne left a comment

pombredanne Apr 21, 2021

pombredanne Apr 21, 2021

pombredanne Apr 21, 2021

pombredanne Apr 21, 2021

pombredanne Apr 21, 2021

pombredanne Apr 21, 2021

pombredanne Apr 21, 2021



		@dataclasses.dataclass(order=True, frozen=True)
		class AffectedPackageWithPatchedPackage:

	Return True if the input 'string' contains any alphabet
	Return True if the input 'string' contains any alphanumeric character```

	affected_package_with_patched_package_objects = []
	affected_package_with_patched_packages = []

	patched_package: Optional[PackageURL] = None
	patched_version: Optional[str] = None

Add patched package #436

Add patched package #436

Conversation

sbs2001 commented Apr 9, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pombredanne left a comment

Choose a reason for hiding this comment

pombredanne left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sbs2001 commented Apr 20, 2021 • edited Loading

pombredanne left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sbs2001 commented Apr 9, 2021 •

edited

Loading

sbs2001 commented Apr 20, 2021 •

edited

Loading