Change from fail-on-result to fail-on-cvss

coyotesqrl commented 3 weeks ago

The dependency-check Maven plugin has deprecated the failBuildOnAnyVulnerability in favor of setting failBuildOnCVSS to 0 for all vulnerabilities.

This allows for greater flexibility and granularity, making CI adoption more attractive. For example, a team could opt to fail builds with any vulnerabilities with a CVSS > 6. This can help reduce churn a little, as lower-criticality issues can be addressed at more leisure.

coyotesqrl commented 3 weeks ago

@seancorfield I had opened the issue and figured I would submit a PR with my proposed solution. I'm happy to discuss alternatives here.

One obvious path would be to leave the current flag with its current all-or-nothing meaning and add the proposed new flag to allow for more granular failure conditions.

seancorfield commented 3 weeks ago

I think we maybe need to look at #112 first? @lread or is that issue completely orthogonal to this one?

I definitely don't want to break anyone's workflow that uses -f / --fail-on-result today (even tho' that is an unclear name), so I would want to see a new option provided for --fail-on-cvss (suggestions for a short name?).

It does seem like if we had --fail-on-cvss, then -f / --fail-on-result could become a shortcut for --fail-on-cvss 0 yes? (in terms of implementation), and we'd need a check that you couldn't provide both options.

We could tell folks that this new option was the recommended approach and deprecate the older fail option (similar to our treatment of -d in 6.0: remove it from the docs but still support it).

coyotesqrl commented 3 weeks ago

Could possibly name the new flag --cvss-fail-threshold or something like that, with -c for the short name.

Then could add a validation check that both flags are not present and treat the current --fail-on-result as --cvss-fail-threshold 0.

Whether #112 is orthogonal to this or not probably depends on how that one is solved. If the structure of the vulnerabilities remains the same and the different (CVSS2, CVSS3, CVSS4) values are somehow combined into the current shape, it's independent; if the response structure changes, it'd be best to wait to implement this change.

seancorfield commented 3 weeks ago

Could possibly name the new flag --cvss-fail-threshold or something like that, with -c for the short name.

That works, yes.

We'll dig a bit more into #112 before finalizing an approach here.

lread commented 3 weeks ago

I think we maybe need to look at #112 first? @lread or is that issue completely orthogonal to this one?

Yes, we should deal with #112 first.

lread commented 3 weeks ago

I had a peek at what DependencyCheck does here.

They don't look at CVSS4 yet (they maybe should?) but if any base score from CVSS2 or CVSS3 is >= threshold, the "build" is failed. They also seem to compensate for missing scores... I had a peek at the h2 db and didn't see a need to do that, but I should take a 2nd look.

An option would be to see if we can just call DependencyCheck's function here and let it decide. But... maybe best to do our own thing and include CVSS4 scores as well?

lread commented 3 weeks ago

You raising this issue is helpful, @coyotesqrl. Not only is it a good idea, but it is also helping me to better think about #112. So, thanks for that!

coyotesqrl commented 3 weeks ago

No problem, @lread . I'm looking at incorporating clj-watson into our CI workflow and this is one of the two changes we'd need. I was just a little too quick to put up my initial PR. 😀

I'd like to give the final version of it a go when the dust has settled on the changes stemming from #112.

seancorfield commented 3 weeks ago

Now I'm curious what the other change you would need is?

coyotesqrl commented 3 weeks ago

I want to think of a couple proposed solutions before writing the issue, but so far have only come up with one. Basically, I want the DependencyCheck vulnerability scan to support using a clj-watson-config.edn for CVE whitelisting by date as well. Unlike the GH Advisory Database, it would probably have to be post-processing of the vulnerabilities. I was thinking in the scan* fn.

seancorfield commented 3 weeks ago

Is the regular false positives XML file approach not sufficient for whitelisting CVEs? That's what we've used at work.

coyotesqrl commented 3 weeks ago

Ooh. I think I just missed that completely. 🤦

That will probably work; I'll look into it tomorrow.

coyotesqrl commented 3 weeks ago

AH! Now I know why I missed it. Maybe I'll write up some docs for #55

seancorfield commented 3 weeks ago

We have a couple of open issues around it. Firstly, to actually document how to do it(!) #55 and also encourage users to submit corrections for false positives upstream #101

lread commented 3 weeks ago

This issue was raised from the perspective of dependency-check, but we also have our github-advisory strategy. The data returned by github advisory is sometimes incomplete. Dependency-check seems pretty complete today, but we don't know about tomorrow.

For github-advisory, I've seen cases where we have the severity but no score (or a score of 0.0 that clearly doesn't match the severity).

For missing data:

If we have severity and no score, we can derive the score, it won't be perfect because CVSS2 and CVSS3 mappings are different. We'll err on the side of caution when converting. Perhaps both Critical and High will translate to 10.0.
If both severity and score are missing, we'll treat that as the topmost critical score with regard to threshold checking. We just don't know in this case.

When clj-watson exits with 1 due to --cvss-fail-threshold being exceeded, I think it should report why. Maybe something like:

Score threshold of 5.2 met for:

Dependency Version CVSS Score 
foo/bar    1.2.3   5.2 (version 3.1)
foo/bar1   7.7a    6.9 (score missing - derived from Medium severity)
foo/bar2   2.0     10.0 (score missing - derived from High severity)
foo/bar3   24.2    10.0 (score missing - derived from Critical severity)
foo/bar4   8.2     10.0 (score missing and severity Foo unrecognized)
foo/bar5   1.12    10.0 (score and severity missing)

Exiting with code 1.

I was thinking that these derived scores would only be shown here. But I suppose an alternative would be to include these derived scores in reports (with clear indicators of being derived) and then we would not have to repeat a summary here. I'm a bit leary of enriching CVE data in clj-watson reports, though.

Watcha think?

coyotesqrl commented 3 weeks ago

I personally think it's sufficient to include this as just a threshold report on cvss failure. It's extra data that doesn't seem to have a ton of value in the normal output report, but that would be very valuable in deciding whether something needed to be whitelisted, resolved, or removed.

seancorfield commented 2 weeks ago

If no further discussion/analysis is needed on this issue, I'd be happy to move to a PR and get it into 6.1 -- since it sounds like this would allow @coyotesqrl to start using clj-watson in CI?

lread commented 2 weeks ago

So, to recap:

always use highest base score from vulnerability (regardless of CVSS version)
always derive a score when missing, only show derived scores in threshold score summary (see above - summary to be refined as needed during implementation)
show the threshold score summary when exiting with non-zero due to the threshold being met
new: my feeling is that --fail-on-result is simpler to understand, arguably safer (less going on), and therefore still valid, and should not be deprecated.

seancorfield commented 2 weeks ago

Agree on all of the above. Thank you @lread !

coyotesqrl commented 2 weeks ago

One more point...might want to warn if both flags are passed.

lread commented 2 weeks ago

Ok, I'll take a crack at a PR.

clj-holmes / clj-watson

Change from fail-on-result to fail-on-cvss #114