Skip to content

Commit

Permalink
Improve performance of findings retrieval
Browse files Browse the repository at this point in the history
The `/v1/finding/{projectUuid}` endpoint has historically been slow to respond (#3811). While the "main" query behind it is somewhat optimized SQL already, it still suffered from various performance killers:

* Filtering of suppressed findings was done in-memory, and required fetching of individual `Analysis` records *for every single finding*.
* `Clob` fields were not mapped directly from the SQL query result, but instead by re-fetching `Component` and `Vulnerability` records *for every single finding*, such that the ORM would provide properly `String`-ified field values.
* Aliases were fetched *for every single finding* individually.
* Latest component versions were fetched *for every single finding* individually.

Performance was improved via the following changes:

1. Filtering of suppressed findings is moved to the main SQL query, voiding the need to fetch individual `Analysis` records later. This also reduces the overall result set that needs to be transferred and mapped.
2. Mapping of `Clob` fields is done within the `Finding` constructor, voiding the need to re-fetch `Vulnerability` records in order to retrieve `String` values for them.
3. Aliases are loaded in bulk, and in a way that avoids redundant queries if the same `Vulnerability` appears multiple times within a list of `Finding`s.
4. Latest component versions are loaded in bulk, and in a way that avoids redundant queries if the same `Component` appears multiple times within a list of `Finding`s.

Because the modified functionality is re-used across the code base, multiple features benefit from this enhancement:

* `/v1/finding/{projectUuid}` endpoint
    * Corresponds to the *Audit Vulnerabilities* tab in the UI
* `/v1/project/{projectUuid}/export` endpoint
* CycloneDX exports for *Inventory with Vulnerabilities*, *VDR*, and *VEX*
* Fortify, Kenna, and DefectDojo integrations

Signed-off-by: nscuro <nscuro@protonmail.com>
  • Loading branch information
nscuro committed Jun 23, 2024
1 parent bc2de42 commit ba17eb2
Show file tree
Hide file tree
Showing 6 changed files with 249 additions and 34 deletions.
41 changes: 34 additions & 7 deletions src/main/java/org/dependencytrack/model/Finding.java
Original file line number Diff line number Diff line change
Expand Up @@ -19,20 +19,25 @@
package org.dependencytrack.model;

import com.fasterxml.jackson.annotation.JsonInclude;
import org.apache.commons.io.IOUtils;
import org.apache.commons.lang3.StringUtils;
import org.dependencytrack.parser.common.resolver.CweResolver;
import org.dependencytrack.util.VulnerabilityUtil;

import java.io.BufferedReader;
import java.io.IOException;
import java.io.Serializable;
import java.math.BigDecimal;
import java.sql.Clob;
import java.sql.SQLException;
import java.util.ArrayList;
import java.util.HashMap;
import java.util.HashSet;
import java.util.LinkedHashMap;
import java.util.List;
import java.util.Map;
import java.util.UUID;
import java.util.Set;
import java.util.List;
import java.util.HashSet;
import java.util.HashMap;
import java.util.UUID;


/**
Expand Down Expand Up @@ -95,7 +100,8 @@ public class Finding implements Serializable {
ON "COMPONENT"."ID" = "ANALYSIS"."COMPONENT_ID"
AND "VULNERABILITY"."ID" = "ANALYSIS"."VULNERABILITY_ID"
AND "COMPONENT"."PROJECT_ID" = "ANALYSIS"."PROJECT_ID"
WHERE "COMPONENT"."PROJECT_ID" = ?
WHERE "COMPONENT"."PROJECT_ID" = :projectId
AND (:includeSuppressed = :true OR "ANALYSIS"."SUPPRESSED" IS NULL OR "ANALYSIS"."SUPPRESSED" = :false)
""";

// language=SQL
Expand Down Expand Up @@ -175,8 +181,16 @@ public Finding(UUID project, Object... o) {
optValue(vulnerability, "vulnId", o[8]);
optValue(vulnerability, "title", o[9]);
optValue(vulnerability, "subtitle", o[10]);
//optValue(vulnerability, "description", o[11]); // CLOB - handle this in QueryManager
//optValue(vulnerability, "recommendation", o[12]); // CLOB - handle this in QueryManager
if (o[11] instanceof final Clob clob) {
optValue(vulnerability, "description", toString(clob));
} else {
optValue(vulnerability, "description", o[11]);
}
if (o[12] instanceof final Clob clob) {
optValue(vulnerability, "recommendation", toString(clob));
} else {
optValue(vulnerability, "recommendation", o[12]);
}
final Severity severity = VulnerabilityUtil.getSeverity(o[13], (BigDecimal) o[14], (BigDecimal) o[15], (BigDecimal) o[16], (BigDecimal) o[17], (BigDecimal) o[18]);
optValue(vulnerability, "cvssV2BaseScore", o[14]);
optValue(vulnerability, "cvssV3BaseScore", o[15]);
Expand Down Expand Up @@ -291,4 +305,17 @@ public void addVulnerabilityAliases(List<VulnerabilityAlias> aliases) {
}
vulnerability.put("aliases",uniqueAliases);
}

private static String toString(final Clob clob) {
if (clob == null) {
return null;
}

try (final var reader = new BufferedReader(clob.getCharacterStream())) {
return IOUtils.toString(reader);
} catch (IOException | SQLException e) {
throw new RuntimeException("Failed to read CLOB value", e);
}
}

}
32 changes: 32 additions & 0 deletions src/main/java/org/dependencytrack/model/VulnIdAndSource.java
Original file line number Diff line number Diff line change
@@ -0,0 +1,32 @@
/*
* This file is part of Dependency-Track.
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*
* SPDX-License-Identifier: Apache-2.0
* Copyright (c) OWASP Foundation. All Rights Reserved.
*/
package org.dependencytrack.model;

/**
* @param vulnId {@link Vulnerability#getVulnId()}
* @param source {@link Vulnerability#getSource()}
* @since 4.12.0
*/
public record VulnIdAndSource(String vulnId, Vulnerability.Source source) {

public VulnIdAndSource(String vulnId, String source) {
this(vulnId, Vulnerability.Source.valueOf(source));
}

}
Original file line number Diff line number Diff line change
Expand Up @@ -28,16 +28,21 @@
import org.dependencytrack.model.Component;
import org.dependencytrack.model.Finding;
import org.dependencytrack.model.Project;
import org.dependencytrack.model.RepositoryMetaComponent;
import org.dependencytrack.model.RepositoryType;
import org.dependencytrack.model.VulnIdAndSource;
import org.dependencytrack.model.Vulnerability;
import org.dependencytrack.model.VulnerabilityAlias;
import org.dependencytrack.persistence.RepositoryQueryManager.RepositoryMetaComponentSearch;
import org.dependencytrack.util.PurlUtil;

import javax.jdo.PersistenceManager;
import javax.jdo.Query;
import java.util.ArrayList;
import java.util.Collections;
import java.util.Date;
import java.util.List;
import java.util.Map;
import java.util.Objects;
import java.util.stream.Collectors;

public class FindingsQueryManager extends QueryManager implements IQueryManager {

Expand Down Expand Up @@ -265,33 +270,69 @@ public List<Finding> getFindings(Project project) {
@SuppressWarnings("unchecked")
public List<Finding> getFindings(Project project, boolean includeSuppressed) {
final Query<Object[]> query = pm.newQuery(Query.SQL, Finding.QUERY);
query.setParameters(project.getId());
final List<Object[]> list = query.executeList();
final List<Finding> findings = new ArrayList<>();
for (final Object[] o: list) {
final Finding finding = new Finding(project.getUuid(), o);
final Component component = getObjectByUuid(Component.class, (String)finding.getComponent().get("uuid"));
final Vulnerability vulnerability = getObjectByUuid(Vulnerability.class, (String)finding.getVulnerability().get("uuid"));
final Analysis analysis = getAnalysis(component, vulnerability);
final List<VulnerabilityAlias> aliases = detach(getVulnerabilityAliases(vulnerability));
finding.addVulnerabilityAliases(aliases);
if (includeSuppressed || analysis == null || !analysis.isSuppressed()) { // do not add globally suppressed findings
// These are CLOB fields. Handle these here so that database-specific deserialization doesn't need to be performed (in Finding)
finding.getVulnerability().put("description", vulnerability.getDescription());
finding.getVulnerability().put("recommendation", vulnerability.getRecommendation());
final PackageURL purl = component.getPurl();
if (purl != null) {
final RepositoryType type = RepositoryType.resolve(purl);
if (RepositoryType.UNSUPPORTED != type) {
final RepositoryMetaComponent repoMetaComponent = getRepositoryMetaComponent(type, purl.getNamespace(), purl.getName());
if (repoMetaComponent != null) {
finding.getComponent().put("latestVersion", repoMetaComponent.getLatestVersion());
}
}
}
findings.add(finding);
query.setNamedParameters(Map.ofEntries(
Map.entry("projectId", project.getId()),
Map.entry("includeSuppressed", includeSuppressed),
// NB: These are required for MSSQL, apparently it doesn't have
// a native boolean type, or DataNucleus maps booleans to a type
// that doesn't have boolean semantics. Fun!
Map.entry("false", false),
Map.entry("true", true)
));
final List<Object[]> queryResultRows = executeAndCloseList(query);

final List<Finding> findings = queryResultRows.stream()
.map(row -> new Finding(project.getUuid(), row))
.toList();

final Map<VulnIdAndSource, List<Finding>> findingsByVulnIdAndSource = findings.stream()
.collect(Collectors.groupingBy(
finding -> new VulnIdAndSource(
(String) finding.getVulnerability().get("vulnId"),
(String) finding.getVulnerability().get("source")
)
));
final Map<VulnIdAndSource, List<VulnerabilityAlias>> aliasesByVulnIdAndSource =
getVulnerabilityAliases(findingsByVulnIdAndSource.keySet());
for (final VulnIdAndSource vulnIdAndSource : findingsByVulnIdAndSource.keySet()) {
final List<Finding> affectedFindings = findingsByVulnIdAndSource.get(vulnIdAndSource);
final List<VulnerabilityAlias> aliases = aliasesByVulnIdAndSource.getOrDefault(vulnIdAndSource, Collections.emptyList());

for (final Finding finding : affectedFindings) {
finding.addVulnerabilityAliases(aliases);
}
}

final Map<RepositoryMetaComponentSearch, List<Finding>> findingsByMetaComponentSearch = findings.stream()
.filter(finding -> finding.getComponent().get("purl") != null)
.map(finding -> {
final PackageURL purl = PurlUtil.silentPurl((String) finding.getComponent().get("purl"));
if (purl == null) {
return null;
}

final var repositoryType = RepositoryType.resolve(purl);
if (repositoryType == RepositoryType.UNSUPPORTED) {
return null;
}

final var search = new RepositoryMetaComponentSearch(repositoryType, purl.getNamespace(), purl.getName());
return Map.entry(search, finding);
})
.filter(Objects::nonNull)
.collect(Collectors.groupingBy(
Map.Entry::getKey,
Collectors.mapping(Map.Entry::getValue, Collectors.toList())
));
getRepositoryMetaComponents(List.copyOf(findingsByMetaComponentSearch.keySet()))
.forEach(metaComponent -> {
final var search = new RepositoryMetaComponentSearch(metaComponent.getRepositoryType(), metaComponent.getNamespace(), metaComponent.getName());
final List<Finding> affectedFindings = findingsByMetaComponentSearch.get(search);
for (final Finding finding : affectedFindings) {
finding.getComponent().put("latestVersion", metaComponent.getLatestVersion());
}
});

return findings;
}
}
10 changes: 10 additions & 0 deletions src/main/java/org/dependencytrack/persistence/QueryManager.java
Original file line number Diff line number Diff line change
Expand Up @@ -72,6 +72,7 @@
import org.dependencytrack.model.ViolationAnalysis;
import org.dependencytrack.model.ViolationAnalysisComment;
import org.dependencytrack.model.ViolationAnalysisState;
import org.dependencytrack.model.VulnIdAndSource;
import org.dependencytrack.model.Vulnerability;
import org.dependencytrack.model.VulnerabilityAlias;
import org.dependencytrack.model.VulnerabilityMetrics;
Expand All @@ -88,6 +89,7 @@
import javax.jdo.Query;
import java.security.Principal;
import java.util.ArrayList;
import java.util.Collection;
import java.util.Date;
import java.util.HashSet;
import java.util.List;
Expand Down Expand Up @@ -1066,6 +1068,10 @@ public List<VulnerabilityAlias> getVulnerabilityAliases(Vulnerability vulnerabil
return getVulnerabilityQueryManager().getVulnerabilityAliases(vulnerability);
}

public Map<VulnIdAndSource, List<VulnerabilityAlias>> getVulnerabilityAliases(final Collection<VulnIdAndSource> vulnIdAndSources) {
return getVulnerabilityQueryManager().getVulnerabilityAliases(vulnIdAndSources);
}

List<Analysis> getAnalyses(Project project) {
return getFindingsQueryManager().getAnalyses(project);
}
Expand Down Expand Up @@ -1468,4 +1474,8 @@ public List<RepositoryMetaComponent> getRepositoryMetaComponentsBatch(final List

return results;
}

public List<RepositoryMetaComponent> getRepositoryMetaComponents(final List<RepositoryQueryManager.RepositoryMetaComponentSearch> list) {
return getRepositoryQueryManager().getRepositoryMetaComponents(list);
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@
import org.dependencytrack.model.Component;
import org.dependencytrack.model.FindingAttribution;
import org.dependencytrack.model.Project;
import org.dependencytrack.model.VulnIdAndSource;
import org.dependencytrack.model.Vulnerability;
import org.dependencytrack.model.VulnerabilityAlias;
import org.dependencytrack.model.VulnerableSoftware;
Expand All @@ -45,6 +46,7 @@
import java.util.List;
import java.util.Map;
import java.util.UUID;
import java.util.function.Function;
import java.util.stream.Collectors;

final class VulnerabilityQueryManager extends QueryManager implements IQueryManager {
Expand Down Expand Up @@ -657,6 +659,90 @@ public List<VulnerabilityAlias> getVulnerabilityAliases(Vulnerability vulnerabil
return (List<VulnerabilityAlias>)query.execute(vulnerability.getVulnId());
}



/**
* Bulk-load {@link VulnerabilityAlias}es for one or more {@link VulnIdAndSource}s.
*
* @param vulnIdAndSources The Vulnerability ID - Source pairs to load {@link VulnerabilityAlias}es for
* @return {@link VulnerabilityAlias}es, grouped by {@link VulnIdAndSource}
* @since 4.12.0
*/
public Map<VulnIdAndSource, List<VulnerabilityAlias>> getVulnerabilityAliases(final Collection<VulnIdAndSource> vulnIdAndSources) {
if (vulnIdAndSources == null || vulnIdAndSources.isEmpty()) {
return Collections.emptyMap();
}

var subQueryIndex = 0;
final var subQueries = new ArrayList<String>(vulnIdAndSources.size());
final var params = new HashMap<String, Object>(vulnIdAndSources.size());
for (final VulnIdAndSource vulnIdAndSource : vulnIdAndSources) {
subQueryIndex++;
final String vulnIdParamName = "vulnId" + subQueryIndex;

final String filter = switch (vulnIdAndSource.source()) {
case GITHUB -> "\"GHSA_ID\" = :" + vulnIdParamName;
case INTERNAL -> "\"INTERNAL_ID\" = :" + vulnIdParamName;
case NVD -> "\"CVE_ID\" = :" + vulnIdParamName;
case OSSINDEX -> "\"SONATYPE_ID\" = :" + vulnIdParamName;
case OSV -> "\"OSV_ID\" = :" + vulnIdParamName;
case SNYK -> "\"SNYK_ID\" = :" + vulnIdParamName;
case VULNDB -> "\"VULNDB_ID\" = :" + vulnIdParamName;
default -> null;
};
if (filter == null) {
continue;
}

// We'll need to correlate query results with VulnIdAndSource objects
// later, so prepend an identifier for them to the result set.
// NB: DataNucleus doesn't support usage of query parameters
// in the SELECT statement. Use hashCode of vulnIdAndSource instead
// of string literals (which could be susceptible to SQL injection).
subQueries.add("""
SELECT %d
, "GHSA_ID"
, "INTERNAL_ID"
, "CVE_ID"
, "SONATYPE_ID"
, "OSV_ID"
, "SNYK_ID"
, "VULNDB_ID"
FROM "VULNERABILITYALIAS"
WHERE %s
""".formatted(vulnIdAndSource.hashCode(), filter));
params.put(vulnIdParamName, vulnIdAndSource.vulnId());
}

final Query<Object[]> query = pm.newQuery(Query.SQL, String.join(" UNION ALL ", subQueries));
query.setNamedParameters(params);
final List<Object[]> queryResultRows = executeAndCloseList(query);
if (queryResultRows.isEmpty()) {
return Collections.emptyMap();
}

final Map<Integer, VulnIdAndSource> vulnIdAndSourceByHashCode = vulnIdAndSources.stream()
.collect(Collectors.toMap(Record::hashCode, Function.identity()));

return queryResultRows.stream()
.map(row -> {
final var vulnIdAndSource = vulnIdAndSourceByHashCode.get((Integer) row[0]);
final var alias = new VulnerabilityAlias();
alias.setGhsaId((String) row[1]);
alias.setInternalId((String) row[2]);
alias.setCveId((String) row[3]);
alias.setSonatypeId((String) row[4]);
alias.setOsvId((String) row[5]);
alias.setSnykId((String) row[6]);
alias.setVulnDbId((String) row[7]);
return Map.entry(vulnIdAndSource, alias);
})
.collect(Collectors.groupingBy(
Map.Entry::getKey,
Collectors.mapping(Map.Entry::getValue, Collectors.toList())
));
}

/**
* Reconcile {@link VulnerableSoftware} for a given {@link Vulnerability}.
* <p>
Expand Down
19 changes: 19 additions & 0 deletions src/main/java/org/dependencytrack/util/PurlUtil.java
Original file line number Diff line number Diff line change
Expand Up @@ -52,4 +52,23 @@ public static PackageURL silentPurlCoordinatesOnly(final PackageURL original) {
}
}

/**
* Attempt to parse a given package URL.
*
* @param purl The package URL to parse
* @return The parsed {@link PackageURL}, or {@code null} when parsing failed
* @since 4.12.0
*/
public static PackageURL silentPurl(final String purl) {
if (purl == null) {
return null;
}

try {
return new PackageURL(purl);
} catch (MalformedPackageURLException ignored) {
return null;
}
}

}

0 comments on commit ba17eb2

Please sign in to comment.