Separate import and improve operations #525

Hritik14 · 2021-08-10T13:49:45Z

!! Breaking Changes !!

Entire database model is restructured.

Get rid of added_advisory and batches
- useless after a530627
Introduce improvers
See: Separate import and improve operations #525 (comment) for a basic structure overview

Note: All changes in the importers are only reflected in the nginx importer while this PR is in draft stage.

Knows defects (in current PR):

UI break (see above)
might crash in multiple imports / improves
No improver than default improver is implemented yet
normalized function of AdvisoryData has no body
nginx importer still has remains of set_api etc
Inference -> AdvisoryData encapsulation
Duplicated data in database
Drop migrations
???

Knows defects (to be solved in different PR):

Signed-off-by: Hritik Vijay hritikxx8@gmail.com

Hritik14 · 2021-08-13T00:40:52Z

Feel free to review now. It's not perfect yet, so please consider these points

Only the framework is there, the improvers/nginx.py improver doesn't actually improve anything. It needs to be written properly to fit this structure
Many many names for patched/fixed/resolved packages are still there, please replace them in your mind, I'll refactor all of these in one commit so as to not pollute current commit with irrelevant diffs
There are a couple of fixmes, I'll try to figure them out, feel free to comment on those
I have made no change in the importer logic yet (like refactoring importer_yielder, other potential improvements making it consistent with improvers), it is for the same reason that I don't want to pollute the current commit. The current commit only focuses on improvers. I'll add a new commit after refactoring importers explicitly.
tl;dr : there's only improver framework (not improver itself), least possible irrelevant code is touched (although it needs to be refactored)

pombredanne · 2021-08-13T07:21:10Z

@Hritik14 re:

but then the importers would have to
mention that whatever they import have 100% confidence which might be
susceptible to typo errors making some importers not mention their
confidence thus zeroing on confidence.

If you have a default of 100% this becomes a non-issue? I kinda like to avoid near-duplicated data structures.
In a way it makes sense to have a "100%" confidence in factual data we import... and there could be cases where we do not have 100% confidence in some imported factual data too?

pombredanne · 2021-08-13T07:22:39Z

Note that using a common data structure does not mean that we have only importers.

pombredanne

Thank you!
... see some review comments inline.

pombredanne · 2021-08-13T07:25:21Z

vulnerabilities/data_inference.py

@@ -0,0 +1,37 @@
+import dataclasses


For consistence, please add a license header. We still need to update the license everywhere per #277 , but best is to have whatever we have now consistently across all code files.

Noted. How about I add all the license in the last commit ? Might make things clearer. Let me know

vulnerabilities/data_source.py

vulnerabilities/import_runner.py

vulnerabilities/improvers/nginx.py

vulnerabilities/management/commands/improve.py

vulnerabilities/models.py

Hritik14 · 2021-08-14T02:43:53Z

@pombredanne

If you have a default of 100% this becomes a non-issue?

A typo could lead to inferences to have 100% confidence in that case...

I kinda like to avoid near-duplicated data structures

Importers and improvers look similar for now mostly because improvers don't really contain anything yet. There are various subclasess for importers like GitDataSource or OvalDataSource with corresponding helper functions to parse advisory data from an actual data source, whereas in case of improvers, they only get the entries from the existing data thus no advisory parsing would be required. On the other hand, improvers could benefit with functions enabling them to access data from the database in a better manner (than done here, I'll be removing direct database interactions in improvers). Other than that an Inference could be seen as what we actually store in our system.

In a way it makes sense to have a "100%" confidence in factual data we import... and there could be cases where we do not have 100% confidence in some imported factual data too

If it's a factual data then then by definition it's a fact thus 100% confidence, right?

Note that using a common data structure does not mean that we have only importers.

Inserting into the database (be it importer or improver) is essentially the same in our structure. Wouldn't we finally need a common data structure anyway ?

vulnerabilities/improvers/nginx.py

vulnerabilities/models.py

Hritik14 · 2021-08-28T23:51:09Z

Current commit brings breaking changes. Feel free to review but do ignore the TODO's and pointers mentioned in the latest commit message.

This is a step towards https://github.com/nexB/vulnerablecode/wiki/WeeklyMeetings#meeting-on-tuesday-2021-08-17-at-1400-utc

Hritik14 · 2021-08-29T23:16:02Z

Trying out adding a real improver other than the default one now. It is not ready yet. Suggestions are welcome for the WIP commit but please avoid it in any strict review currently.
Latest reviewable commit: 38f5872

pombredanne

Consider using to_dict rather than directly to_json ... a to_json becomes then a thin wrapper on the serialization with to_dict
See also my other comments inline.

pombredanne · 2021-09-02T15:06:54Z

vulnerabilities/data_inference.py

@@ -0,0 +1,37 @@
+import dataclasses


vulnerabilities/data_inference.py

vulnerabilities/data_source.py

vulnerabilities/data_inference.py

vulnerabilities/views.py

vulnerabilities/data_source.py

vulnerabilities/importers/nginx.py

The refactors are based on aboutcode-org#525 (review) - class Inference: order should be the logic order from most important to least important fields - class Improver: docs - data_source.py: Use to_dict() than json() - models: Advisory: docs - improvers/nginx.py: Removed, relevant improvers will now reside inside importer module. In this case, importers/nginx.py - black formatting Signed-off-by: Hritik Vijay <hritikxx8@gmail.com>

Hritik14 · 2021-09-12T22:42:03Z

Using the OSV design has some caveats. Given data like:

Not vulnerable: 1.21.0+, 1.20.1+
Vulnerable: 0.6.18-1.20.0

It is impossible to directly get one fixed version for a range of affected versions.

pombredanne · 2021-09-13T12:46:15Z

@Hritik14 re:

Using the OSV design has some caveats. Given data like:
Not vulnerable: 1.21.0+, 1.20.1+
Vulnerable: 0.6.18-1.20.0
It is impossible to directly get one fixed version for a range of affected versions.

Is this a real case rather than some made up example?
I cannot fathom how you could get the two versions fixing the same bug.
In the example you gave, 1.21.0+ is impossible and 1.20.1+ can only be the correct one as this is the latest one.

Now things would be different if there were more than one range of course... but then that would be something entirely different?

pombredanne · 2021-09-14T08:07:50Z

Here is the gist of our discussion:

nginx advisories are weird at best and likely not meant to be strictly parsable.
the corresponding CVEs CPE ranges seem to be also problematic in some cases
we should reach out to nginx to clarify
in the meantime and in general, where there are ambiguities like this we
4.1. make a safe judgment call in this case use the latest versions of multiple "fixed" version that apply to the same range. We may be overly cautious and there could be some configs that are not vulnerable that we may report as vulnerable in some cases
4.2. log this and flag these for manual review

Hritik14 · 2021-09-16T13:34:16Z

Turns out we can parse nginx versions smoothly. Here's more on it #553

vulnerabilities/improve_runner.py

vulnerabilities/improvers/default.py

pombredanne · 2021-09-17T11:28:55Z

I do not like the batch processing of multiple Advisory at once in a single transaction... this may be an optimization we will need in the future, but not now.

Here is a suggested updated way:

each Improver has a method to process a single Advisory model instance such as Improver.get_inferences(self, advisory): -> Inference or something like that.
the framework then iterates on an Improver-provided query set such as Improver.get_interesting_advisories(self): -> QuerySet that has the Advisories it is interested in.
in the framework, there is an atomic transaction that updates both the Advisory (e.g. date and later a log of improvements with select for update) and whatever is updated or create from the Inference

The refactors are based on aboutcode-org#525 (review) - class Inference: order should be the logic order from most important to least important fields - class Improver: docs - data_source.py: Use to_dict() than json() - models: Advisory: docs - improvers/nginx.py: Removed, relevant improvers will now reside inside importer module. In this case, importers/nginx.py - black formatting Signed-off-by: Hritik Vijay <hritikxx8@gmail.com>

Signed-off-by: Hritik Vijay <hritikxx8@gmail.com>

pombredanne

Ready to merge! ... see a few nits for your review.

vulnerabilities/data_source.py

pombredanne · 2022-01-25T10:35:11Z

vulnerabilities/data_source.py

+        affected_version_range = VersionRange.from_string(affected_pkg["affected_version_range"])
+        fixed_version = affected_pkg["fixed_version"]
+        if fixed_version:
+            # TODO: revisit after https://github.com/nexB/univers/issues/10


This is fixed now.

H: No..... aboutcode-org/univers#10 (comment)

vulnerabilities/importers/nginx.py

vulnerabilities/views.py

vulnerabilities/models.py

Signed-off-by: Hritik Vijay <hritikxx8@gmail.com>

Fixes are in accordance with the final review for this PR done at : aboutcode-org#525 (review) Signed-off-by: Hritik Vijay <hritikxx8@gmail.com> Co-authored-by: Philippe Ombredanne <pombredanne@gmail.com>

from_dict is a factory, should be a classmethod Signed-off-by: Hritik Vijay <hritikxx8@gmail.com>

The entire view is messed up. Will probably need a rewrite Co-authored-by: Philippe Ombredanne <pombredanne@gmail.com> Signed-off-by: Hritik Vijay <hritikxx8@gmail.com>

Fixes are in accordance with the final review for this PR done at : aboutcode-org#525 (review) Signed-off-by: Hritik Vijay <hritikxx8@gmail.com> Co-authored-by: Philippe Ombredanne <pombredanne@gmail.com>

Signed-off-by: Philippe Ombredanne <pombredanne@nexb.com>

Hritik14 force-pushed the importer-refactor branch 2 times, most recently from 331f70c to 925d0ef Compare August 13, 2021 00:33

pombredanne requested changes Aug 13, 2021

View reviewed changes

sbs2001 requested changes Aug 16, 2021

View reviewed changes

pombredanne changed the title ~~Seperate import and improve operations~~ Separate import and improve operations Aug 17, 2021

Hritik14 force-pushed the importer-refactor branch from 6b706c0 to 1834ef0 Compare August 29, 2021 11:01

Hritik14 mentioned this pull request Aug 29, 2021

Report newly added CVEs/vulnerabilities #536

Open

Hritik14 linked an issue Aug 29, 2021 that may be closed by this pull request

Importers without a vulnerable package list #449

Closed

Hritik14 force-pushed the importer-refactor branch from 1834ef0 to 7f9fbb4 Compare August 29, 2021 23:14

pombredanne requested changes Sep 7, 2021

View reviewed changes

Hritik14 force-pushed the importer-refactor branch from 7f9fbb4 to fbe993b Compare September 12, 2021 22:40

pombredanne reviewed Sep 17, 2021

View reviewed changes

vulnerabilities/improve_runner.py Outdated Show resolved Hide resolved

pombredanne reviewed Sep 17, 2021

View reviewed changes

vulnerabilities/improvers/default.py Outdated Show resolved Hide resolved

Hritik14 force-pushed the importer-refactor branch from fbe993b to 8897af9 Compare September 24, 2021 06:20

Hritik14 force-pushed the importer-refactor branch 2 times, most recently from d2a2167 to 56866f7 Compare October 12, 2021 21:11

Hritik14 added 6 commits January 25, 2022 15:40

Change __repr__ to qualified_name fn

b9ba26a

Signed-off-by: Hritik Vijay <hritikxx8@gmail.com>

Update .gitignore for junk files

e3a1ea5

Signed-off-by: Hritik Vijay <hritikxx8@gmail.com>

Fix few tests to recent structure, dump ugettext_lazy

c3e71f4

Signed-off-by: Hritik Vijay <hritikxx8@gmail.com>

Ignore outdated tests

e822c41

Signed-off-by: Hritik Vijay <hritikxx8@gmail.com>

Disable outdated importers

e9f940d

Signed-off-by: Hritik Vijay <hritikxx8@gmail.com>

Reset migrations

318663e

Signed-off-by: Hritik Vijay <hritikxx8@gmail.com>

Hritik14 force-pushed the importer-refactor branch from e25b437 to 318663e Compare January 25, 2022 10:10

pombredanne approved these changes Jan 25, 2022

View reviewed changes

Hritik14 force-pushed the importer-refactor branch from 761fdab to 61fea9e Compare January 25, 2022 12:39

Partition without numerical index

86d175e

Signed-off-by: Hritik Vijay <hritikxx8@gmail.com>

Hritik14 force-pushed the importer-refactor branch from 61fea9e to 06c793c Compare January 25, 2022 18:03

This was referenced Jan 25, 2022

Create and use an nginx type now that we have a specific versioning scheme for it #594

Open

The query for affected and fixed packages should be abstracted as methods on the models (and/or on a model manager) #595

Closed

Hritik14 force-pushed the importer-refactor branch from 06c793c to ce87783 Compare January 25, 2022 20:51

Hritik14 and others added 3 commits January 26, 2022 03:13

Add get_fixed_purl in AffectedPackage, fix from_dict

bd390d4

from_dict is a factory, should be a classmethod Signed-off-by: Hritik Vijay <hritikxx8@gmail.com>

Add FIXME about wrong filters in PackageSearchView

6ab6f06

The entire view is messed up. Will probably need a rewrite Co-authored-by: Philippe Ombredanne <pombredanne@gmail.com> Signed-off-by: Hritik Vijay <hritikxx8@gmail.com>

Teeny weeny fixes

33d0c0d

Fixes are in accordance with the final review for this PR done at : aboutcode-org#525 (review) Signed-off-by: Hritik Vijay <hritikxx8@gmail.com> Co-authored-by: Philippe Ombredanne <pombredanne@gmail.com>

Hritik14 force-pushed the importer-refactor branch from d01ebc7 to 33d0c0d Compare January 25, 2022 21:43

Hritik14 mentioned this pull request Jan 25, 2022

Wrong filters in PackageSearchView #596

Closed

pombredanne added 2 commits January 25, 2022 22:46

Correct file format

21c6178

Signed-off-by: Philippe Ombredanne <pombredanne@nexb.com>

Use univers 30+. Bump lxml

0e74bea

Signed-off-by: Philippe Ombredanne <pombredanne@nexb.com>

pombredanne merged commit 67fc00e into aboutcode-org:main Jan 25, 2022

Hritik14 deleted the importer-refactor branch January 25, 2022 22:11

Hritik14 linked an issue Jan 26, 2022 that may be closed by this pull request

Missing VULCOIDS replacement implementation #552

Closed

This was referenced Jan 26, 2022

Migrate all existing importers to importer improver structure. #597

Closed

Upgrade to Python 3.9 #602

Open

Hritik14 mentioned this pull request Feb 2, 2022

Basic documentation #603

Closed

osBins mentioned this pull request Feb 15, 2022

Inconsistent naming of patched/fixed affected/vulnerable ... #593

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Separate import and improve operations #525

Separate import and improve operations #525

Hritik14 commented Aug 10, 2021 •

edited

Loading

Hritik14 commented Aug 13, 2021 •

edited

Loading

pombredanne commented Aug 13, 2021

pombredanne commented Aug 13, 2021

pombredanne left a comment

pombredanne Aug 13, 2021

Hritik14 Aug 14, 2021

pombredanne Sep 2, 2021

Hritik14 commented Aug 14, 2021 •

edited

Loading

Hritik14 commented Aug 28, 2021 •

edited

Loading

Hritik14 commented Aug 29, 2021 •

edited

Loading

pombredanne left a comment

pombredanne Sep 2, 2021

Hritik14 commented Sep 12, 2021

pombredanne commented Sep 13, 2021

pombredanne commented Sep 14, 2021

Hritik14 commented Sep 16, 2021

pombredanne commented Sep 17, 2021

pombredanne left a comment

pombredanne Jan 25, 2022 •

edited by Hritik14

Loading

Separate import and improve operations #525

Separate import and improve operations #525

Conversation

Hritik14 commented Aug 10, 2021 • edited Loading

!! Breaking Changes !!

Knows defects (in current PR):

Knows defects (to be solved in different PR):

Hritik14 commented Aug 13, 2021 • edited Loading

pombredanne commented Aug 13, 2021

pombredanne commented Aug 13, 2021

pombredanne left a comment

Choose a reason for hiding this comment

pombredanne Aug 13, 2021

Choose a reason for hiding this comment

Hritik14 Aug 14, 2021

Choose a reason for hiding this comment

pombredanne Sep 2, 2021

Choose a reason for hiding this comment

Hritik14 commented Aug 14, 2021 • edited Loading

Hritik14 commented Aug 28, 2021 • edited Loading

Hritik14 commented Aug 29, 2021 • edited Loading

pombredanne left a comment

Choose a reason for hiding this comment

pombredanne Sep 2, 2021

Choose a reason for hiding this comment

Hritik14 commented Sep 12, 2021

pombredanne commented Sep 13, 2021

pombredanne commented Sep 14, 2021

Hritik14 commented Sep 16, 2021

pombredanne commented Sep 17, 2021

pombredanne left a comment

Choose a reason for hiding this comment

pombredanne Jan 25, 2022 • edited by Hritik14 Loading

Choose a reason for hiding this comment

Hritik14 commented Aug 10, 2021 •

edited

Loading

Hritik14 commented Aug 13, 2021 •

edited

Loading

Hritik14 commented Aug 14, 2021 •

edited

Loading

Hritik14 commented Aug 28, 2021 •

edited

Loading

Hritik14 commented Aug 29, 2021 •

edited

Loading

pombredanne Jan 25, 2022 •

edited by Hritik14

Loading