Skip to content

Commit

Permalink
Merge pull request #4216 from nscuro/issue-4215
Browse files Browse the repository at this point in the history 
Prevent duplicate policy violations
  • Loading branch information
@nscuro
nscuro authored Oct 4, 2024
2 parents 6f7c49c + 3acc779 commit 2eea86f
Show file tree
Hide file tree
Showing 7 changed files with 233 additions and 60 deletions.
94 changes: 69 additions & 25 deletions src/main/java/org/dependencytrack/persistence/PolicyQueryManager.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -43,10 +43,13 @@
import java.util.Collection;
import java.util.Date;
import java.util.HashMap;
import java.util.HashSet;
import java.util.List;
import java.util.Map;
import java.util.Set;
import java.util.UUID;

import static org.dependencytrack.util.PersistenceUtil.assertNonPersistentAll;
import static org.dependencytrack.util.PersistenceUtil.assertPersistent;
import static org.dependencytrack.util.PersistenceUtil.assertPersistentAll;

Expand Down Expand Up @@ -153,38 +156,79 @@ public PolicyCondition updatePolicyCondition(final PolicyCondition policyConditi
return persist(pc);
}

private record ViolationIdentity(
long componentId,
long conditionId,
PolicyViolation.Type type) {

private ViolationIdentity(final PolicyViolation violation) {
this(violation.getComponent().getId(), violation.getPolicyCondition().getId(), violation.getType());
}

}

/**
* Intelligently adds dependencies for components that are not already a dependency
* of the specified project and removes the dependency relationship for components
* that are not in the list of specified components.
* @param component the project to bind components to
* @param policyViolations the complete list of existing dependent components
*/
public synchronized void reconcilePolicyViolations(final Component component, final List<PolicyViolation> policyViolations) {
// Removes violations as dependencies to the project for all
// components not included in the list provided
List<PolicyViolation> markedForDeletion = new ArrayList<>();
for (final PolicyViolation existingViolation: getAllPolicyViolations(component)) {
boolean keep = false;
for (final PolicyViolation violation: policyViolations) {
if (violation.getType() == existingViolation.getType()
&& violation.getPolicyCondition().getId() == existingViolation.getPolicyCondition().getId()
&& violation.getComponent().getId() == existingViolation.getComponent().getId())
{
keep = true;
break;
* @param reportedViolations the complete list of existing dependent components
*/
public synchronized void reconcilePolicyViolations(
final Component component,
final List<PolicyViolation> reportedViolations) {
assertPersistent(component, "component must be persistent");
assertNonPersistentAll(reportedViolations, "reportedViolations must not be persistent");

runInTransaction(() -> {
final List<PolicyViolation> existingViolations = getAllPolicyViolations(component);
final var violationsToCreate = new ArrayList<PolicyViolation>();
final var violationsToDelete = new ArrayList<PolicyViolation>();

final var existingViolationByIdentity = new HashMap<ViolationIdentity, PolicyViolation>();
for (final PolicyViolation violation : existingViolations) {
// Previous reconciliation logic allowed for duplicate violations
// to exist. Take that into consideration and ensure their deletion.
final boolean isDuplicate = existingViolationByIdentity.putIfAbsent(
new ViolationIdentity(violation), violation) != null;
if (isDuplicate) {
violationsToDelete.add(violation);
}
}
if (!keep) {
markedForDeletion.add(existingViolation);

final var reportedViolationsByIdentity = new HashMap<ViolationIdentity, PolicyViolation>();
for (final PolicyViolation violation : reportedViolations) {
reportedViolationsByIdentity.put(new ViolationIdentity(violation), violation);
}
}
if (!markedForDeletion.isEmpty()) {
for (final PolicyViolation violation : markedForDeletion) {
deleteViolationAnalysisTrail(violation);

final Set<ViolationIdentity> violationIdentities = new HashSet<>(
existingViolationByIdentity.size() + reportedViolationsByIdentity.size());
violationIdentities.addAll(existingViolationByIdentity.keySet());
violationIdentities.addAll(reportedViolationsByIdentity.keySet());

for (final ViolationIdentity identity : violationIdentities) {
final PolicyViolation existingViolation = existingViolationByIdentity.get(identity);
final PolicyViolation reportedViolation = reportedViolationsByIdentity.get(identity);

if (existingViolation == null) {
violationsToCreate.add(reportedViolation);
} else if (reportedViolation == null) {
violationsToDelete.add(existingViolation);
}
}
delete(markedForDeletion);
}

if (!violationsToCreate.isEmpty()) {
persist(violationsToCreate);
}

if (!violationsToDelete.isEmpty()) {
for (final PolicyViolation violation : violationsToDelete) {
deleteViolationAnalysisTrail(violation);
}

delete(violationsToDelete);
}
});
}

/**
Expand Down Expand Up @@ -222,7 +266,7 @@ public PolicyViolation clonePolicyViolation(PolicyViolation sourcePolicyViolatio
if(comments != null){
violationAnalysis.setAnalysisComments(comments);
}
policyViolation.setAnalysis(violationAnalysis);
policyViolation.setAnalysis(violationAnalysis);
policyViolation.getAnalysis().setPolicyViolation(policyViolation);
policyViolation.setUuid(sourcePolicyViolation.getUuid());
return policyViolation;
Expand Down Expand Up @@ -465,7 +509,7 @@ public List<ViolationAnalysisComment> cloneViolationAnalysisComments(PolicyViola
comments.add(comment);
}
}

return comments;
}

Expand Down
9 changes: 5 additions & 4 deletions src/main/java/org/dependencytrack/policy/PolicyEngine.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@
import org.dependencytrack.model.Tag;
import org.dependencytrack.persistence.QueryManager;
import org.dependencytrack.util.NotificationUtil;

import java.util.ArrayList;
import java.util.Date;
import java.util.List;
Expand Down Expand Up @@ -102,10 +103,10 @@ private List<PolicyViolation> evaluate(final QueryManager qm, final List<Policy>
}
if (Policy.Operator.ANY == policy.getOperator()) {
if (policyConditionsViolated > 0) {
policyViolations.addAll(createPolicyViolations(qm, policyConditionViolations));
policyViolations.addAll(createPolicyViolations(policyConditionViolations));
}
} else if (Policy.Operator.ALL == policy.getOperator() && policyConditionsViolated == policy.getPolicyConditions().size()) {
policyViolations.addAll(createPolicyViolations(qm, policyConditionViolations));
policyViolations.addAll(createPolicyViolations(policyConditionViolations));
}
}
}
Expand All @@ -125,15 +126,15 @@ private boolean isPolicyAssignedToProject(Policy policy, Project project) {
return (policy.getProjects().stream().anyMatch(p -> p.getId() == project.getId()) || (Boolean.TRUE.equals(policy.isIncludeChildren()) && isPolicyAssignedToParentProject(policy, project)));
}

private List<PolicyViolation> createPolicyViolations(final QueryManager qm, final List<PolicyConditionViolation> pcvList) {
private List<PolicyViolation> createPolicyViolations(final List<PolicyConditionViolation> pcvList) {
final List<PolicyViolation> policyViolations = new ArrayList<>();
for (PolicyConditionViolation pcv : pcvList) {
final PolicyViolation pv = new PolicyViolation();
pv.setComponent(pcv.getComponent());
pv.setPolicyCondition(pcv.getPolicyCondition());
pv.setType(determineViolationType(pcv.getPolicyCondition().getSubject()));
pv.setTimestamp(new Date());
policyViolations.add(qm.addPolicyViolationIfNotExist(pv));
policyViolations.add(pv);
}
return policyViolations;
}
Expand Down
8 changes: 6 additions & 2 deletions src/main/java/org/dependencytrack/tasks/BomUploadProcessingTask.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -77,6 +77,7 @@
import java.util.Set;
import java.util.UUID;
import java.util.concurrent.TimeUnit;
import java.util.concurrent.locks.ReentrantLock;
import java.util.function.Predicate;

import static org.apache.commons.lang3.StringUtils.isNotBlank;
Expand All @@ -99,6 +100,7 @@
import static org.dependencytrack.parser.cyclonedx.util.ModelConverter.convertToProject;
import static org.dependencytrack.parser.cyclonedx.util.ModelConverter.convertToProjectMetadata;
import static org.dependencytrack.parser.cyclonedx.util.ModelConverter.flatten;
import static org.dependencytrack.util.LockUtil.getLockForProjectAndNamespace;
import static org.dependencytrack.util.PersistenceUtil.applyIfChanged;
import static org.dependencytrack.util.PersistenceUtil.assertPersistent;

Expand Down Expand Up @@ -169,17 +171,21 @@ private void processEvent(final Context ctx, final BomUploadEvent event) {
ctx.bomSerialNumber = cdxBom.getSerialNumber().replaceFirst("^urn:uuid:", "");
}

final ReentrantLock lock = getLockForProjectAndNamespace(ctx.project, getClass().getSimpleName());
try (var ignoredMdcBomFormat = MDC.putCloseable(MDC_BOM_FORMAT, ctx.bomFormat.getFormatShortName());
var ignoredMdcBomSpecVersion = MDC.putCloseable(MDC_BOM_SPEC_VERSION, ctx.bomSpecVersion);
var ignoredMdcBomSerialNumber = MDC.putCloseable(MDC_BOM_SERIAL_NUMBER, ctx.bomSerialNumber);
var ignoredMdcBomVersion = MDC.putCloseable(MDC_BOM_VERSION, String.valueOf(ctx.bomVersion))) {
lock.lock();
processBom(ctx, cdxBom);

LOGGER.debug("Dispatching %d events".formatted(eventsToDispatch.size()));
eventsToDispatch.forEach(Event::dispatch);
} catch (RuntimeException e) {
LOGGER.error("Failed to process BOM", e);
dispatchBomProcessingFailedNotification(ctx, e);
} finally {
lock.unlock();
}
}

Expand Down Expand Up @@ -272,8 +278,6 @@ private void processBom(final Context ctx, final org.cyclonedx.model.Bom cdxBom)
// by project; This is not the case for vulnerabilities. We don't want the entire TRX to fail,
// just because another TRX created or modified the same vulnerability record.

// TODO: Introduce locking by project ID / UUID to avoid processing BOMs for the same project concurrently.

qm.runInTransaction(() -> {
final Project persistentProject = processProject(ctx, qm, project, projectMetadata);

Expand Down
38 changes: 28 additions & 10 deletions src/main/java/org/dependencytrack/tasks/PolicyEvaluationTask.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -18,34 +18,52 @@
*/
package org.dependencytrack.tasks;

import alpine.common.logging.Logger;
import alpine.event.framework.Event;
import alpine.event.framework.Subscriber;
import org.dependencytrack.event.PolicyEvaluationEvent;
import org.dependencytrack.event.ProjectMetricsUpdateEvent;
import org.dependencytrack.model.Component;
import org.dependencytrack.model.Project;
import org.dependencytrack.policy.PolicyEngine;
import org.slf4j.MDC;

import java.util.ArrayList;
import java.util.List;
import java.util.concurrent.locks.ReentrantLock;

public class PolicyEvaluationTask implements Subscriber {
import static org.dependencytrack.common.MdcKeys.MDC_EVENT_TOKEN;
import static org.dependencytrack.common.MdcKeys.MDC_PROJECT_NAME;
import static org.dependencytrack.common.MdcKeys.MDC_PROJECT_UUID;
import static org.dependencytrack.common.MdcKeys.MDC_PROJECT_VERSION;
import static org.dependencytrack.util.LockUtil.getLockForProjectAndNamespace;

private static final Logger LOGGER = Logger.getLogger(PolicyEvaluationTask.class);
public class PolicyEvaluationTask implements Subscriber {

/**
* {@inheritDoc}
*/
@Override
public void inform(final Event e) {
if (e instanceof PolicyEvaluationEvent event) {
if (event.getProject() != null) {
if (event.getComponents() != null && !event.getComponents().isEmpty()) {
performPolicyEvaluation(event.getProject(), event.getComponents());
} else {
performPolicyEvaluation(event.getProject(), new ArrayList<>());
}
if (!(e instanceof final PolicyEvaluationEvent event)) {
return;
}
if (event.getProject() == null) {
return;
}

final ReentrantLock lock = getLockForProjectAndNamespace(event.getProject(), getClass().getSimpleName());
try (var ignoredMdcProjectUuid = MDC.putCloseable(MDC_PROJECT_UUID, event.getProject().getUuid().toString());
var ignoredMdcProjectName = MDC.putCloseable(MDC_PROJECT_NAME, event.getProject().getName());
var ignoredMdcProjectVersion = MDC.putCloseable(MDC_PROJECT_VERSION, event.getProject().getVersion());
var ignoredMdcEventToken = MDC.putCloseable(MDC_EVENT_TOKEN, event.getChainIdentifier().toString())) {
lock.lock();
if (event.getComponents() != null && !event.getComponents().isEmpty()) {
performPolicyEvaluation(event.getProject(), event.getComponents());
} else {
performPolicyEvaluation(event.getProject(), new ArrayList<>());
}
} finally {
lock.unlock();
}
}

Expand Down
65 changes: 47 additions & 18 deletions src/main/java/org/dependencytrack/tasks/VulnerabilityAnalysisTask.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -43,12 +43,15 @@
import org.dependencytrack.tasks.scanners.SnykAnalysisTask;
import org.dependencytrack.tasks.scanners.TrivyAnalysisTask;
import org.dependencytrack.tasks.scanners.VulnDbAnalysisTask;

import java.time.Duration;
import java.time.Instant;
import java.util.ArrayList;
import java.util.List;
import java.util.UUID;
import java.util.stream.Collectors;
import java.util.concurrent.locks.ReentrantLock;

import static org.dependencytrack.util.LockUtil.getLockForProjectAndNamespace;

public class VulnerabilityAnalysisTask implements Subscriber {

Expand All @@ -60,33 +63,59 @@ public class VulnerabilityAnalysisTask implements Subscriber {
@Override
public void inform(final Event e) {
if (e instanceof VulnerabilityAnalysisEvent event) {
if (event.getComponents() != null && event.getComponents().size() > 0) {
final List<Component> components = new ArrayList<>();
try (final QueryManager qm = new QueryManager()) {
for (final Component c : event.getComponents()) {
// Ensures the current component (and related objects such as Project) are attached to the
// current persistence manager. This may cause duplicate projects to be created and other
// unexpected behavior.
components.add(qm.getObjectByUuid(Component.class, c.getUuid()));
final ReentrantLock projectLock;
if (event.getProject() != null) {
projectLock = getLockForProjectAndNamespace(event.getProject(), getClass().getSimpleName());
} else {
projectLock = null;
}

try {
if (projectLock != null) {
projectLock.lock();
}

if (event.getComponents() != null && !event.getComponents().isEmpty()) {
final List<Component> components = new ArrayList<>();
try (final QueryManager qm = new QueryManager()) {
for (final Component c : event.getComponents()) {
// Ensures the current component (and related objects such as Project) are attached to the
// current persistence manager. This may cause duplicate projects to be created and other
// unexpected behavior.
components.add(qm.getObjectByUuid(Component.class, c.getUuid()));
}
analyzeComponents(qm, components, e);
}
analyzeComponents(qm, components, e);
}
} finally {
if (projectLock != null) {
projectLock.unlock();
}
}
} else if (e instanceof PortfolioVulnerabilityAnalysisEvent event) {
} else if (e instanceof PortfolioVulnerabilityAnalysisEvent) {
LOGGER.info("Analyzing portfolio");
try (final QueryManager qm = new QueryManager()) {
final List<UUID> projectUuids = qm.getAllProjects(true)
.stream()
.map(Project::getUuid)
.collect(Collectors.toList());
.toList();
for (final UUID projectUuid : projectUuids) {
final Project project = qm.getObjectByUuid(Project.class, projectUuid);
if (project == null) continue;
final List<Component> components = qm.getAllComponents(project);
LOGGER.info("Analyzing " + components.size() + " components in project: " + project.getUuid());
analyzeComponents(qm, components, e);
performPolicyEvaluation(project, components);
LOGGER.info("Completed scheduled analysis of " + components.size() + " components in project: " + project.getUuid());
if (project == null) {
continue;
}

final ReentrantLock projectLock = getLockForProjectAndNamespace(project, getClass().getSimpleName());
try {
projectLock.lock();
final List<Component> components = qm.getAllComponents(project);
LOGGER.info("Analyzing " + components.size() + " components in project: " + project.getUuid());
analyzeComponents(qm, components, e);
performPolicyEvaluation(project, components);
LOGGER.info("Completed scheduled analysis of " + components.size() + " components in project: " + project.getUuid());
} finally {
projectLock.unlock();
}
}
}
LOGGER.info("Portfolio analysis complete");
Expand Down
Loading

0 comments on commit 2eea86f

Please sign in to comment.