Improve Improvers

[Hritik14]

Improvers are constraining.

For eg: Improving reference id to reference URL, improving vuln data (not an advisory).
The problem is with both interesting_advisories and get_inferences where both of them expect AdvisoryData.
Improvers cloud be split in

• A condition which checks if this improver runs or not
• then a handling thing.

TOCTOU conditions are also present, we check at interesting_advisories and use the value at get_inferences. Could do https://docs.djangoproject.com/en/4.0/ref/models/querysets/#select-for-update to avoid TOCTOU closer where things are going to change. This would mean returning a QuerySet from interesting_advisories might not be an ideal case.

Current implementation could become a subclass which is a advisory based improver.

(via: https://github.com/nexB/vulnerablecode/wiki/WeeklyMeetings#meeting-on-tuesday-2022-04-19-at-1000-utc)

[Hritik14]

Current problem

• Inference class is not very scalable for different types of improvements
  • Eg: Improver to figure out CVE from a given set of references by their urls.
• There exists a TOCTOU condition over the entire QuerySet passed

Proposed solution

• Eliminate intermediate Inference class and allow improvers to implement their own process method which talks to the database directly.
• Improvers should be idempotent
• After getting a QuerySet, iterate over each element -> select_for_update one element -> update -> repeat
• Following is the proposed python pseudocode

class Improver:

    @classproperty
    def qualified_name(cls):
        """
        Fully qualified name prefixed with the module name of the improver used in logging.
        """
        return f"{cls.__module__}.{cls.__qualname__}"

    @property
    def interesting_queryset(self) -> QuerySet:
        """
        Return QuerySet for the advisories this improver is interested in
        """
        raise NotImplementedError

    def improve(self, args) -> isImproved?:
        """
        Generate and return Inferences for the given advisory data
        """
        raise NotImplementedError

    def process(self, args) -> isProcessed?:
        """
        Entrypoint for the improver. Consume ``interesting_queryset`` and use ``improve`` to
        process improvement.
        """
        raise NotImplementedError

class AdvisoryImprover(Improver):

    def process(record):
        """
        An atomic transaction that updates both the Advisory (e.g. date_improved)
        and processes the given inferences to create or update corresponding
        database fields.

        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 date_improved of advisory is consistent.
        """
        # This is somewhat the same method that exists currently in the framework for processing all improvers
        # Although, it takes care of TOCTOU as stated above
        ...

Now different improvers can be written by implementing the AdvisoryImprover method by providing bodies for improve and interesting_queryset

This way we have a concrete design for improvers, independent ways of handling different kinds of improvements (via the process method)

(via: https://github.com/nexB/vulnerablecode/wiki/WeeklyMeetings#meeting-on-tuesday-2022-04-26-at-1000-utc)

[Hritik14]

Related: #1068