From 3acc7797ce3e2088979997fb649c41f6a9edd413 Mon Sep 17 00:00:00 2001 From: nscuro Date: Fri, 4 Oct 2024 00:09:38 +0200 Subject: [PATCH] Prevent duplicate policy violations Introduces project-level locking for BOM upload processing, policy evaluation, and vulnerability analysis. This prevents duplicate records from being created during any of the mentioned activities. The locking happens in-memory. Refactors policy violation reconciliation to be more deterministic and able to remove duplicates. In a later release, a `UNIQUE` constraint should be added to the `POLICYVIOLATION` table to prevent duplicate records on the database-level (already done in Hyades). Fixes #4215 Signed-off-by: nscuro --- .../persistence/PolicyQueryManager.java | 94 ++++++++++++++----- .../dependencytrack/policy/PolicyEngine.java | 9 +- .../tasks/BomUploadProcessingTask.java | 8 +- .../tasks/PolicyEvaluationTask.java | 38 ++++++-- .../tasks/VulnerabilityAnalysisTask.java | 65 +++++++++---- .../org/dependencytrack/util/LockUtil.java | 78 +++++++++++++++ .../resources/v1/PolicyResourceTest.java | 1 - 7 files changed, 233 insertions(+), 60 deletions(-) create mode 100644 src/main/java/org/dependencytrack/util/LockUtil.java diff --git a/src/main/java/org/dependencytrack/persistence/PolicyQueryManager.java b/src/main/java/org/dependencytrack/persistence/PolicyQueryManager.java index 0f0ded45c9..2f2637aa2a 100644 --- a/src/main/java/org/dependencytrack/persistence/PolicyQueryManager.java +++ b/src/main/java/org/dependencytrack/persistence/PolicyQueryManager.java @@ -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; @@ -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 policyViolations) { - // Removes violations as dependencies to the project for all - // components not included in the list provided - List 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 reportedViolations) { + assertPersistent(component, "component must be persistent"); + assertNonPersistentAll(reportedViolations, "reportedViolations must not be persistent"); + + runInTransaction(() -> { + final List existingViolations = getAllPolicyViolations(component); + final var violationsToCreate = new ArrayList(); + final var violationsToDelete = new ArrayList(); + + final var existingViolationByIdentity = new HashMap(); + 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(); + for (final PolicyViolation violation : reportedViolations) { + reportedViolationsByIdentity.put(new ViolationIdentity(violation), violation); } - } - if (!markedForDeletion.isEmpty()) { - for (final PolicyViolation violation : markedForDeletion) { - deleteViolationAnalysisTrail(violation); + + final Set 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); + } + }); } /** @@ -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; @@ -465,7 +509,7 @@ public List cloneViolationAnalysisComments(PolicyViola comments.add(comment); } } - + return comments; } diff --git a/src/main/java/org/dependencytrack/policy/PolicyEngine.java b/src/main/java/org/dependencytrack/policy/PolicyEngine.java index 67db3db1a9..b6ee0a2d20 100644 --- a/src/main/java/org/dependencytrack/policy/PolicyEngine.java +++ b/src/main/java/org/dependencytrack/policy/PolicyEngine.java @@ -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; @@ -102,10 +103,10 @@ private List evaluate(final QueryManager qm, final List } 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)); } } } @@ -125,7 +126,7 @@ 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 createPolicyViolations(final QueryManager qm, final List pcvList) { + private List createPolicyViolations(final List pcvList) { final List policyViolations = new ArrayList<>(); for (PolicyConditionViolation pcv : pcvList) { final PolicyViolation pv = new PolicyViolation(); @@ -133,7 +134,7 @@ private List createPolicyViolations(final QueryManager qm, fina pv.setPolicyCondition(pcv.getPolicyCondition()); pv.setType(determineViolationType(pcv.getPolicyCondition().getSubject())); pv.setTimestamp(new Date()); - policyViolations.add(qm.addPolicyViolationIfNotExist(pv)); + policyViolations.add(pv); } return policyViolations; } diff --git a/src/main/java/org/dependencytrack/tasks/BomUploadProcessingTask.java b/src/main/java/org/dependencytrack/tasks/BomUploadProcessingTask.java index cc234968af..f21b52c88c 100644 --- a/src/main/java/org/dependencytrack/tasks/BomUploadProcessingTask.java +++ b/src/main/java/org/dependencytrack/tasks/BomUploadProcessingTask.java @@ -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; @@ -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; @@ -169,10 +171,12 @@ 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())); @@ -180,6 +184,8 @@ private void processEvent(final Context ctx, final BomUploadEvent event) { } catch (RuntimeException e) { LOGGER.error("Failed to process BOM", e); dispatchBomProcessingFailedNotification(ctx, e); + } finally { + lock.unlock(); } } @@ -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); diff --git a/src/main/java/org/dependencytrack/tasks/PolicyEvaluationTask.java b/src/main/java/org/dependencytrack/tasks/PolicyEvaluationTask.java index 93100fcc69..7be4477124 100644 --- a/src/main/java/org/dependencytrack/tasks/PolicyEvaluationTask.java +++ b/src/main/java/org/dependencytrack/tasks/PolicyEvaluationTask.java @@ -18,7 +18,6 @@ */ package org.dependencytrack.tasks; -import alpine.common.logging.Logger; import alpine.event.framework.Event; import alpine.event.framework.Subscriber; import org.dependencytrack.event.PolicyEvaluationEvent; @@ -26,26 +25,45 @@ 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(); } } diff --git a/src/main/java/org/dependencytrack/tasks/VulnerabilityAnalysisTask.java b/src/main/java/org/dependencytrack/tasks/VulnerabilityAnalysisTask.java index b351bc99e1..06747afa59 100644 --- a/src/main/java/org/dependencytrack/tasks/VulnerabilityAnalysisTask.java +++ b/src/main/java/org/dependencytrack/tasks/VulnerabilityAnalysisTask.java @@ -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 { @@ -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 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 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 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 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 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"); diff --git a/src/main/java/org/dependencytrack/util/LockUtil.java b/src/main/java/org/dependencytrack/util/LockUtil.java new file mode 100644 index 0000000000..407a5582c4 --- /dev/null +++ b/src/main/java/org/dependencytrack/util/LockUtil.java @@ -0,0 +1,78 @@ +/* + * 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.util; + +import alpine.Config; +import alpine.common.metrics.Metrics; +import com.github.benmanes.caffeine.cache.Caffeine; +import com.github.benmanes.caffeine.cache.LoadingCache; +import io.micrometer.core.instrument.binder.cache.CaffeineCacheMetrics; +import org.dependencytrack.model.Project; + +import java.time.Duration; +import java.util.Collections; +import java.util.concurrent.locks.ReentrantLock; + +import static alpine.Config.AlpineKey.METRICS_ENABLED; +import static java.util.Objects.requireNonNull; + +/** + * @since 4.13.0 + */ +public final class LockUtil { + + private static final LoadingCache CACHE = buildCache(); + + private LockUtil() { + } + + public static ReentrantLock getLockForName(final String name) { + requireNonNull(name, "name must not be null"); + return CACHE.get(name); + } + + public static ReentrantLock getLockForProjectAndNamespace(final Project project, final String namespace) { + requireNonNull(namespace, "namespace must not be null"); + requireNonNull(project, "project must not be null"); + requireNonNull(project.getUuid(), "project UUID must not be null"); + return getLockForName(namespace + ":" + project.getUuid()); + } + + private static LoadingCache buildCache() { + final boolean metricsEnabled = Config.getInstance() + .getPropertyAsBoolean(METRICS_ENABLED); + + final Caffeine cacheBuilder = Caffeine.newBuilder() + .expireAfterAccess(Duration.ofMinutes(1)); + if (metricsEnabled) { + cacheBuilder.recordStats(); + } + + final LoadingCache cache = cacheBuilder + .build(key -> new ReentrantLock()); + + if (metricsEnabled) { + new CaffeineCacheMetrics<>(cache, "dtrack_locks", Collections.emptyList()) + .bindTo(Metrics.getRegistry()); + } + + return cache; + } + +} diff --git a/src/test/java/org/dependencytrack/resources/v1/PolicyResourceTest.java b/src/test/java/org/dependencytrack/resources/v1/PolicyResourceTest.java index f2a1d6d402..0e7b5f2f06 100644 --- a/src/test/java/org/dependencytrack/resources/v1/PolicyResourceTest.java +++ b/src/test/java/org/dependencytrack/resources/v1/PolicyResourceTest.java @@ -211,7 +211,6 @@ public void deletePolicyCascadingTest() { violation.setPolicyCondition(condition); violation.setType(PolicyViolation.Type.OPERATIONAL); violation.setTimestamp(new Date()); - violation = qm.addPolicyViolationIfNotExist(violation); qm.reconcilePolicyViolations(component, singletonList(violation));