Are security indicators used ?

[VannTen]

Do anyone know if we're using the security indicators at all ?
Looking at the code in thoth/storages/security_indicators.py, it seems broken

Bug description

All security indicators use the type (security_indicator_type, "bandit", "cloc")
as the document_id when accessing document. So they can only ever access one
document named by the type.

Actual behavior

class _SecurityIndicatorBase:
    """A base class for security-indicators analyzers results."""

    ...

    def retrieve_document(self) -> Dict[str, Any]:
        """Retrieve security indicator document."""
        return self.ceph.retrieve_document(self.security_indicator_type)

    def document_exists(self) -> bool:
        """Check if the there is an object with the given key in bucket."""
        return self.ceph.document_exists(self.security_indicator_type)

the ceph methods are implemented like this

    def retrieve_document(self, document_id: str) -> dict:
        """Retrieve a dictionary stored as JSON from S3."""
        return json.loads(self.retrieve_blob(document_id).decode())

    def document_exists(self, document_id: str) -> bool:
        """Check if the there is an object with the given key in bucket.

        This check does only HEAD request.
        """
        try:
            self._s3.Object(self.bucket, f"{self.prefix}{document_id}").load()
        (...)

So can't possibly works.

Additional context

That could be fixed (That stuff should probably inherit from result_base) but
I'd first wanted to check if we actually use that; it seems like if we were, it
would cause pretty serious breakage.

@mayaCostantini : would you know if we do use it ?

If we can just get rid of the code entirely, that would also help with #2673

/kind bug
/triage needs-information
/sig stack-guidance
/priority important-soon

[goern]

I think the concept of Security Indicators has been superseded by prescriptions, right?! @mayaCostantini @harshad16

[mayaCostantini]

In adviser, security indicator steps are still part of the resolution logic for the security recommendation type, as they are run with other steps such as CVE etc.

However, scoring of a State based on security indicators for a package rely on the information retrieved from the database via

storages/thoth/storages/graph/postgres.py

Line 1968 in b606074

def get_si_aggregated_python_package_version(

but after inspection of a database dump from 22-07-08, it seems like the only security indicator document aggregated in the si_aggregated_run table is from 22-05-13 (which I did not find in the S3 bucket).

After verification, it seems like the security-indicators workflow has not been updated for 8 months, so my guess is this feature has been abandoned.

(cc @harshad16 for verification as I am still unsure if we are still aggregating those indicators).

[mayaCostantini]

As for if we should remove the code entirely, I don't think that would be a good idea for the moment as we could decide to enable those indicators later in the future. However, I am lacking some context on why this feature ceased to be used.

[VannTen]

As for if we should remove the code entirely, I don't think that would be a good idea for the moment as we could decide to enable those indicators later in the future. However, I am lacking some context on why this feature ceased to be used.

By enabling, do you mean: - It's currently unused because it's soft-disabled -> aka, a settings switch could be toggled which would make current released version to use that code Or - Enabling would be writing new code depending on this In the second case, we can drop it, if we want to get it back later, we'll just need to revert the commit, it's all there in git history. -> the benefits of reduced maintenance outweighs the (hypothetical) inconvenient of having to revert the commit / pr in the future, in my opinion. For the first case, I'll be a bit worried, since I really can't see how we can produce meaningful results with the current state of the code.

[mayaCostantini]

You are right about the fact that the implementation of the retrieve_document and document_exists methods in the base class is wrong and could definitely introduce a bug if security indicators documents happen to be present again in the database.
A solution could be to fix those methods and to leave the security indicators-related code as is, so that it can be easily reused just in case, wdyt?

[VannTen]

A solution could be to fix those methods and to leave the security indicators-related code as is, so that it can be easily reused just in case, wdyt?

I think the costs/benefits is not worth it compared to cutting it out, for the following reasons: - We don't know if we're going to use it or not in the future, so it might become wasted work. - Unused code is untested code, and will bitrot. I'm not sure if we have units tests on those particular class, but even if that's the case, integration/e2e tests will not cover library code that's not used by the applications. - It's very easy to retrieve the code sometimes in the futures should we need it. (git revert) - We our workload for broad changes (stuff like #2673, which is how I discovered this in the first place)

[harshad16]

The security indicator is still in use, and the ingestion with respect to the security indicator is still taking a place.
The issue stated seems like a broken piece that we should fix.
The security indicator is not abandoned, just got lesser in priority as we were trying to get the other things in place.
Let's definitely pull this issue into some work cycle and work on it.

[VannTen]

(maya)

but after inspection of a database dump from 22-07-08, it seems like the only security indicator document aggregated in the `si_aggregated_run` table is from 22-05-13 (which I did not find in the S3 bucket).

This might just be the result of the broken code.

The security indicator is still in use, and the ingestion with respect to the security indicator is still taking a place. The issue stated seems like a broken piece that we should fix. The security indicator is not abandoned, just got lesser in priority as we were trying to get the other things in place. Let's definitely pull this issue into some work cycle and work on it.

👍 In that case I think we can make this a subclass of ResultBase stuff right ? Is there any reason not to ?

[VannTen]

/close

[sesheta]

@VannTen: Closing this issue.

In response to this:

/close

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.