Adding support for vulnerability aliases

[stevespringett]

Implements #1642

[sahibamittal]

Hey @stevespringett - This is good, I tested it as well. But, its missing code to add aliases in OSVdownloadTask, like below code snippet needs to be added in method 'mapAdvisoryToVulnerability'

if (advisory.getAliases() != null) {
            for (int i=0; i<advisory.getAliases().size(); i++) {
                String alias = advisory.getAliases().get(i);
                final VulnerabilityAlias vulnerabilityAlias = new VulnerabilityAlias();
                vulnerabilityAlias.setOsvId(advisory.getId());
                if(alias.startsWith("CVE")) {
                    vulnerabilityAlias.setCveId(alias);
                } else if (alias.startsWith("GHSA")) {
                    vulnerabilityAlias.setGhsaId(alias);
                }
                qm.synchronizeVulnerabilityAlias(vulnerabilityAlias);
            }
}

[nscuro]

Should we offer some sort of server-side deduping for the /finding API? Right now it'd be the client's responsibility of deduping findings based on the aliases. But maybe we should offer a basic solution as well to improve usability.

It should be an explicit opt-in, or potentially an entirely different endpoint like /finding/<PROJECT_UUID>/unique. The format could be such that finding[].vulnerability refers to the vulnerability as defined by the authoritative source (e.g. CVE -> NVD), with an aliases or references field populated with either:

• entire vulnerability objects of the aliases
• UUID or source-vulnId pairs of the aliases (this is how it's done right now)

In the later case, user's can then just fetch the additional info if desired.

I can throw something together in a separate issue / PR if desired.

[stevespringett]

Should we offer some sort of server-side deduping for the /finding API?

Yes, I'd like to. Currently, the findings api is being augmented with aliases for each finding so the client can de-dup.

If we choose to do this, we need to take into consideration the algorithm:

• The most desired approach would be to favor CVE's over any of the alternative identifiers. This assumption should not be hard-coded.
• There will be occasions where a CVE does not exist, yet there are aliases between say GHSA and OSSINDEX. Need to figure out how to handle this case.
• There will be occasions where a CVE does not exist initially, but a OSSINDEX finding does. That OSSINDEX finding could be audited. At a later time, a CVE may be created and now there's a mapping. This happened with log4j and is likely the norm for high-profile vulnerabilities. I don't think we want to de-dup any finding that has an existing audit.

There may be other cases as well. These are the ones that I could immediately think of. But I do think this should be implemented in a different PR as some thought will need to be made into the design and configuration of the approach.

[nscuro]

There will be occasions where a CVE does not exist initially, but a OSSINDEX finding does. That OSSINDEX finding could be audited.

True. Just brainstorming here, how about we rewire analyses to be associated with a VulnerabilityAlias instead of a Vulnerability? Would require a more complex migration, but would drastically simplify this case.

[stevespringett]

how about we rewire analyses to be associated with a VulnerabilityAlias instead of a Vulnerability?

VulnerabilityAliases do not exist for most vulnerabilities, only the ones that have aliases. This approach would need to take into consideration a future requirement we'll have to correct aliases in the event OSSI (or another provider) originally says a CVE is X and later come correct their own data and say its Y. This automated correction is not currently in place, but will need to be.

[stevespringett]

@nscuro @sahibamittal thoughts on merge? Is it ready?

[sahibamittal]

It's a bit confusing that GSD and Snyk are mentioned in a few places, although those sources aren't yet supported and we don't have an entry in the Vulnerability.Source enum for them yet.

I'd prefer it if we left those out for now.

I agree we can remove these for now, I'll add Snyk mapping in Snyk PR later.

[sahibamittal]

@stevespringett - For endpoint to getAllVulnerabilities for a project ("/project/{uuid}"), we're missing this explicit setting of aliases to vulnerability object in VulnerabilityQueryManager. Need to do same in method 'getVulnerabilities' being called by this endpoint.

[nscuro]

We may also want to include the aliases in the notification subjects, which currently have their own mapping logic:

https://github.com/DependencyTrack/dependency-track/blob/a3b42a6fe4d19fc91122887dba0841f26fa7c7c4/src/main/java/org/dependencytrack/util/NotificationUtil.java#L260-L292

[nscuro]

@stevespringett I reckon you are super busy ATM, would you like me to take over and work on the remaining outstanding issues?

[sonatype-lift[bot]]

:warning: 14 God Classes were detected by Lift in this project. Visit the Lift web console for more details.

[stevespringett]

@sahibamittal I cannot find a place where aliases are missing.

[stevespringett]

We may also want to include the aliases in the notification subjects, which currently have their own mapping logic:

https://github.com/DependencyTrack/dependency-track/blob/a3b42a6fe4d19fc91122887dba0841f26fa7c7c4/src/main/java/org/dependencytrack/util/NotificationUtil.java#L260-L292

This should likely be an additional enhancement as it will have to involve changes to all of the notification templates as well.

[nscuro]

This should likely be an additional enhancement as it will have to involve changes to all of the notification templates as well.

Aye. I raised https://github.com/DependencyTrack/dependency-track/issues/1992 for it.

FYI: I created more tickets based on the discussions in this PR. They're tracked under the vuln-aliases tag.

[nscuro]

@stevespringett Is this ready to go from your side?