RedHatProductSecurity / cvelib

A Python library and command line interface for CVE Services.
MIT License
52 stars 24 forks source link

cvelib cve_api ping method return is counterintuitive #59

Closed sei-vsarvepalli closed 1 year ago

sei-vsarvepalli commented 1 year ago

Hello RedHat,

It looks like at some point the cve_api's ping() method no longer return a (True,None) tuple on success but returns a None or a null object when successful.

https://github.com/RedHatProductSecurity/cvelib/blob/e807083fed19871b2a07d049858e370e15d430b5/cvelib/cve_api.py#L275-L284

The earlier return seems more sensible. This has also impacted our project VINCE in giving erronous "CVE service not responding" error. See related https://github.com/CERTCC/VINCE/issues/71

    def ping(self):
        """Check the CVE API status.

        Returns a tuple containing a boolean value of whether the request succeeded and any
        error message that was emitted if it did not succeed.
        """
        try:
            self.get("health-check")
        except CveApiError as exc:
            return False, str(exc)
        return True, None

If there is a reason for this change, we can pursue fixing our code.

mprpic commented 1 year ago

The change happened between 1.x.0 to 2.x.0 (edit: correction, the major version bump was from 0.x.0 to 1.x.0), so yes it broke backwards compatibility. The 1.x.0 version simply duplicated data that is apparent from the second item in the tuple it used to return. You can make the same assertion today: None -> API is up -> previously True; exception is returned -> API is down -> previously False.

Looking at e.g. https://github.com/CERTCC/VINCE/blob/main/vince/views.py#L14650, your usage of ping was incorrect anyway because it was checking the boolean value of the tuple that ping returned instead of just its first element, so it was always returning True no matter if the API was up or down:

In [1]: bool((False, 'w/e'))
Out[1]: True

If you wanted to make it work with the current version of ping(), you can update your IFs to if cve_lib.ping() is None: .... Maybe the doc string for the method could be improved to note "Return an exception if the CVE API is not accessible" instead of the more ambiguous "Check the API" wording.

sei-vsarvepalli commented 1 year ago

So in the past we were happily accepting CVE services is always up! Now it is always down. Thanks for explaining the thinking behind this routine.

Vijay