Skip to content

Fix the presentation URL - #616

Closed
ziadhany wants to merge 46 commits into
aboutcode-org:mainfrom
ziadhany:main
Closed

Fix the presentation URL#616
ziadhany wants to merge 46 commits into
aboutcode-org:mainfrom
ziadhany:main

Conversation

@ziadhany

@ziadhany ziadhany commented Feb 7, 2022

Copy link
Copy Markdown
Collaborator

No description provided.

ziadhany and others added 30 commits February 5, 2022 01:13
useless after a530627

Signed-off-by: Hritik Vijay <hritikxx8@gmail.com>
Signed-off-by: Ziad <ziadhany2016@gmail.com>
Improver look strikingly similar to importers with some enhancements.
(Eg: It doesn't use an importer_yielder alternative, see issue 501).

Overview:
Improvers maintain a contract (like Advisory) with improve_runner which
is named Inference. Inference class embeds an advisory and a confidence
score for that advisory. It is the job of an improver to fetch data to
improve from the database (probably using some helper functions) then
use whatever means necessary to improve that data sample and return with
Inferences. Do note, that Inferences which have already been "imported"
by importers would be totally discarded as redundant. Also, in case of
two inferences on same data point, the one with highest confidence will
be taken into the database.

Food for thought:
Pssst... Probably Inference class is useless and Advisory class can
itself have that confidence score, 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. Anyway, importer and improvers
should be different and separated. If not, then we could totally discard
the idea of improvers and embed everything in an importer with a
confidence score. Well, then, where goes the idea of modularity and
keeping things simple ? Also, data coming from an "import"er should
always be absolutely correct. This will also ensure that if downstream
doesn't want any "improved" data then they don't get our guesses. The
whole point of separating importers and improvers is that running
improvers could be totally optional and based on downstream taste.

Signed-off-by: Hritik Vijay <hritikxx8@gmail.com>
Signed-off-by: Ziad <ziadhany2016@gmail.com>
This is work in progress, there are a few bugs and a few fixmes as well.
Everything will be replaced before the final commit

Signed-off-by: Hritik Vijay <hritikxx8@gmail.com>
Signed-off-by: Ziad <ziadhany2016@gmail.com>
Importing Advisory data is now 2 step process.
1) Import Advisory
2) Create relations (default improver)

importers are now required to return AdvisoryData instead of Advisory.
The data contained inside AdvisoryData is converted into a model named
Advisory and then saved into the database. This will be useful later
for improvers to work on existing Advisories.
All the relationships for the Advisories are generated using a default
improver at improvers/default.py.

NOTE: There are errors in the UI due to models.py
    # TODO: Cannot resolve keyword 'resolved_vulnerabilities' into field
    # make vulnerabilities and resolved_vulnerabilities use the `fix` flag of PackageRelatedVulnerability

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
-[ ] ???

Knows defects (to be solved in different PR):
-[ ] inconsistent naming - will be resolved in a different PR
-[ ] unordered imports

Signed-off-by: Hritik Vijay <hritikxx8@gmail.com>
Signed-off-by: Ziad <ziadhany2016@gmail.com>
Earlier Inference used to nest Advisory class, this doesn't make sense
logically. Now, the Inference class is of it's own and contains some
attributes that could be present in an advisory but are not required.

Technical reason behind this decoupling is that the PackageURL in
AffectedPackage is NOT supposed to contain version information. This
information must be present inside an Inference (that's the whole point).
Improvers are required to read AdvisoryData, modify the relevant fields,
add inference specific fields (confidence, source) and expand the
VersionSpecifier contained inside AffectedPackage package as they deem
fit into PackageURLs.

Known 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
-[x] Inference -> AdvisoryData encapsulation
-[ ] Duplicated data in database
-[ ] ???

Knows defects (to be solved in different PR):
-[ ] inconsistent naming - will be resolved in a different PR
-[ ] unordered imports

Signed-off-by: Hritik Vijay <hritikxx8@gmail.com>
Signed-off-by: Ziad <ziadhany2016@gmail.com>
Recent model changes break the UI as now the PackageRelatedVulnerability
contains a ``fix`` flag to mark the relationship as a fix.
This is leveraged to eliminate multiple columns like patched_package or
vulnerable_package.

Known defects (in current PR):
-[x] UI break
-[ ] 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
-[x] Inference -> AdvisoryData encapsulation
-[ ] Duplicated data in database
-[ ] ???

Knows defects (to be solved in different PR):
-[ ] inconsistent naming - will be resolved in a different PR
-[ ] unordered imports

Signed-off-by: Hritik Vijay <hritikxx8@gmail.com>
Signed-off-by: Ziad <ziadhany2016@gmail.com>
The refactors are based on #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: Ziad <ziadhany2016@gmail.com>
AffectedPackges now contains all affected versions and one fix version.
This is inspired from the design documented at https://docs.google.com/document/d/1sylBGNooKtf220RHQn1I8pZRmqXZQADDQ_TOABrKTpA
Under "Format Overview", along the lines of:

    "affects": {
            "ranges": [ {
                "type": string,
                "repo": string,
                "introduced": string,
                "fixed": string
            } ],
            "versions": [ string ]
        },

Signed-off-by: Hritik Vijay <hritikxx8@gmail.com>
Signed-off-by: Ziad <ziadhany2016@gmail.com>
This is still not perfect because univers is not stable yet. The
uncertainty about the structure of version_specifier needs to be
resolved. As of now, there are many redundant AffectedPackage objects
which would be gone after aboutcode-org/univers#8 is
fixed.

Signed-off-by: Hritik Vijay <hritikxx8@gmail.com>
Signed-off-by: Ziad <ziadhany2016@gmail.com>
There must be an affected or fixed purls if there's no vulnerability id.
Otherwise, when vulnerability id is present, any (except summery for now)
could be present. It does not make sense to neither have vulnerability id
nor affected or fixed purls, such relation would be meaningless.

Signed-off-by: Hritik Vijay <hritikxx8@gmail.com>
Signed-off-by: Ziad <ziadhany2016@gmail.com>
Updated way:

- Each Improver has a method to process a single Advisory model instance
  such as Improver.get_inferences(self, advisory): -> Inference

- The framework then iterates on an Improver-provided QuerySet such as
  Improver.interesting_advisories 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

This avoids failing the entire improver when only a single inference is
erroneous. Also, the atomic transaction for every advisory and its
inferences makes sure that improved_on date of advisory is consistent.

Signed-off-by: Hritik Vijay <hritikxx8@gmail.com>
Signed-off-by: Ziad <ziadhany2016@gmail.com>
Git rid of extraneous methods with empty body and commented code.
Now as AdvisoryData has all the parameters optional, make sure there's
a __post_init__ to assert the constraints.

Signed-off-by: Hritik Vijay <hritikxx8@gmail.com>
Signed-off-by: Ziad <ziadhany2016@gmail.com>
aboutcode-org/univers#10
- parse_version() should accept None and empty strings

Signed-off-by: Hritik Vijay <hritikxx8@gmail.com>
Signed-off-by: Ziad <ziadhany2016@gmail.com>
This is based on PR review done at
#525 (review)
Majorly removes commented code, adds some doctest

Signed-off-by: Hritik Vijay <hritikxx8@gmail.com>
Signed-off-by: Ziad <ziadhany2016@gmail.com>
This adds univer from the current branch that support "vers".
See aboutcode-org/univers#12

Signed-off-by: Philippe Ombredanne <pombredanne@nexb.com>
Signed-off-by: Ziad <ziadhany2016@gmail.com>
All of this comes from the review done at
#525 (review)

Co-authored-by: Philippe Ombredanne <pombredanne@gmail.com>
Signed-off-by: Hritik Vijay <hritikxx8@gmail.com>
Signed-off-by: Ziad <ziadhany2016@gmail.com>
This commit adopts the new univers, this also requires to reset all the
migrations as migration 0003 was using a univers method which is no
longer available.
There are also a few refactors based on #525 (review)

Signed-off-by: Hritik Vijay <hritikxx8@gmail.com>
Signed-off-by: Ziad <ziadhany2016@gmail.com>
because why not ?

Signed-off-by: Hritik Vijay <hritikxx8@gmail.com>
Signed-off-by: Ziad <ziadhany2016@gmail.com>
Signed-off-by: Hritik Vijay <hritikxx8@gmail.com>
Signed-off-by: Ziad <ziadhany2016@gmail.com>
The following suggestions were acception plus some code refactor

https://github.com/nexB/vulnerablecode/blob/main/vulnerabilities/migrations/0003_populate_patched_package.py
Is this hand written migration ? Why ? I'm resetting migrations, this is breaks on changes in univers
- > move to init migration, provide data dump

https://github.com/Hritik14/vulnerablecode/blob/ee0dba45f1d5b6680e121d91ce59b050325a5e67/vulnerabilities/import_runner.py#L73
- [x] name should be inside the importer class
https://github.com/Hritik14/vulnerablecode/blob/ee0dba45f1d5b6680e121d91ce59b050325a5e67/vulnerabilities/import_runner.py#L108
- [ ] Advisory. get or create in the loop itself
https://github.com/Hritik14/vulnerablecode/blob/ee0dba45f1d5b6680e121d91ce59b050325a5e67/vulnerabilities/import_runner.py#L95
- [x] Make it a list (or iterable is better), not set

class AdvisoryData:
"""
This data class expresses the contract between data sources and the import runner.
"""

vulnerability_id: Optional[str] = None
summary: str = None
affected_packages: List[AffectedPackage] = dataclasses.field(default_factory=list)
references: List[Reference] = dataclasses.field(default_factory=list)
date_published: Optional[datetime.datetime] = None

- [x] Use this to create an Advisory model and store List objects as json

https://github.com/Hritik14/vulnerablecode/blob/ee0dba45f1d5b6680e121d91ce59b050325a5e67/vulnerabilities/data_source.py#L99
- [x] affected_version_range

- [x] https://github.com/Hritik14/vulnerablecode/blob/ee0dba45f1d5b6680e121d91ce59b050325a5e67/vulnerabilities/data_source.py#L120
VersionRange.version_class to get the Version subclass

https://github.com/Hritik14/vulnerablecode/blob/ee0dba45f1d5b6680e121d91ce59b050325a5e67/vulnerabilities/importers/nginx.py#L71
- [ ] docify this

- [x] advisory_data should return an iterable as a contract

https://github.com/Hritik14/vulnerablecode/blob/ee0dba45f1d5b6680e121d91ce59b050325a5e67/vulnerabilities/importers/nginx.py#L77
- [x] yield better

https://github.com/Hritik14/vulnerablecode/blob/ee0dba45f1d5b6680e121d91ce59b050325a5e67/vulnerabilities/importers/nginx.py#L158
- [ ] use getattr

https://github.com/Hritik14/vulnerablecode/blob/ee0dba45f1d5b6680e121d91ce59b050325a5e67/vulnerabilities/importers/nginx.py#L162
- [x] only for nginx advisory

https://github.com/Hritik14/vulnerablecode/blob/ee0dba45f1d5b6680e121d91ce59b050325a5e67/vulnerabilities/importers/nginx.py#L188
- [x] return a dict and use ** on 77
https://github.com/Hritik14/vulnerablecode/blob/ee0dba45f1d5b6680e121d91ce59b050325a5e67/vulnerabilities/importers/nginx.py#L99
- [x] _,_,fixed_versions =

https://github.com/Hritik14/vulnerablecode/blob/ee0dba45f1d5b6680e121d91ce59b050325a5e67/vulnerabilities/importers/nginx.py#L86
- [x] how does a paragraph look

https://github.com/Hritik14/vulnerablecode/blob/ee0dba45f1d5b6680e121d91ce59b050325a5e67/vulnerabilities/importers/nginx.py#L116
- [x] remove branch qualifier

Signed-off-by: Hritik Vijay <hritikxx8@gmail.com>
Signed-off-by: Ziad <ziadhany2016@gmail.com>
Rewrite following improvers:
	DefaultImprover
Plus adjust the improver framework

Signed-off-by: Hritik Vijay <hritikxx8@gmail.com>
Signed-off-by: Ziad <ziadhany2016@gmail.com>
Signed-off-by: Hritik Vijay <hritikxx8@gmail.com>
Signed-off-by: Ziad <ziadhany2016@gmail.com>
Signed-off-by: Hritik Vijay <hritikxx8@gmail.com>
Signed-off-by: Ziad <ziadhany2016@gmail.com>
Signed-off-by: Hritik Vijay <hritikxx8@gmail.com>
Signed-off-by: Ziad <ziadhany2016@gmail.com>
VersionRange is always a flat list of constraints

Signed-off-by: Hritik Vijay <hritikxx8@gmail.com>
Signed-off-by: Ziad <ziadhany2016@gmail.com>
Signed-off-by: Hritik Vijay <hritikxx8@gmail.com>
Signed-off-by: Ziad <ziadhany2016@gmail.com>
An alias is a unique vulnerability identifier in some database, such as
the NVD, PYSEC, CVE or similar. These databases guarantee that these
identifiers are unique within their namespace.
An alias may also be used as a Reference. But in contrast with some
Reference may not be an identifier for a single vulnerability, for instance,
security advisories such as Debian security advisory reference various
vulnerabilities.

Signed-off-by: Hritik Vijay <hritikxx8@gmail.com>
Signed-off-by: Ziad <ziadhany2016@gmail.com>
Signed-off-by: Hritik Vijay <hritikxx8@gmail.com>
Signed-off-by: Ziad <ziadhany2016@gmail.com>
Signed-off-by: Hritik Vijay <hritikxx8@gmail.com>
Signed-off-by: Ziad <ziadhany2016@gmail.com>
Hritik14 and others added 16 commits February 7, 2022 21:18
Signed-off-by: Hritik Vijay <hritikxx8@gmail.com>
Signed-off-by: Ziad <ziadhany2016@gmail.com>
Signed-off-by: Hritik Vijay <hritikxx8@gmail.com>
Signed-off-by: Ziad <ziadhany2016@gmail.com>
Signed-off-by: Hritik Vijay <hritikxx8@gmail.com>
Signed-off-by: Ziad <ziadhany2016@gmail.com>
Signed-off-by: Hritik Vijay <hritikxx8@gmail.com>
Signed-off-by: Ziad <ziadhany2016@gmail.com>
Signed-off-by: Hritik Vijay <hritikxx8@gmail.com>
Signed-off-by: Ziad <ziadhany2016@gmail.com>
Signed-off-by: Hritik Vijay <hritikxx8@gmail.com>
Signed-off-by: Ziad <ziadhany2016@gmail.com>
from_dict is a factory, should be a classmethod

Signed-off-by: Hritik Vijay <hritikxx8@gmail.com>
Signed-off-by: Ziad <ziadhany2016@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>
Signed-off-by: Ziad <ziadhany2016@gmail.com>
Fixes are in accordance with the final review for this PR done at :
#525 (review)

Signed-off-by: Hritik Vijay <hritikxx8@gmail.com>
Co-authored-by: Philippe Ombredanne <pombredanne@gmail.com>
Signed-off-by: Ziad <ziadhany2016@gmail.com>
Signed-off-by: Philippe Ombredanne <pombredanne@nexb.com>
Signed-off-by: Ziad <ziadhany2016@gmail.com>
Signed-off-by: Philippe Ombredanne <pombredanne@nexb.com>
Signed-off-by: Ziad <ziadhany2016@gmail.com>
Fixes: #550

Signed-off-by: keshav-space <keshav6240@gmail.com>
Signed-off-by: Ziad <ziadhany2016@gmail.com>
Signed-off-by: Philippe Ombredanne <pombredanne@nexb.com>
Signed-off-by: Ziad <ziadhany2016@gmail.com>
Signed-off-by: Ziad <ziadhany2016@gmail.com>
Signed-off-by: Ziad <ziadhany2016@gmail.com>
Signed-off-by: Ziad <ziadhany2016@gmail.com>
@ziadhany ziadhany closed this Feb 7, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants