Skip to content

Commit

Permalink
Prevent duplicate policy violations
Browse files Browse the repository at this point in the history 
Ports DependencyTrack/dependency-track#4234

The implementation differs in the following ways:

* Locks are not acquired in-memory but instead through the database (shedlock), since it must work across multiple app instances.
* Risk of duplicates during reconciliation of violations was already fixed when implementing the CEL-based policy engine.
* A UNIQUE constraint is added to prevent duplicate records from being created at the database-level. Now-redundant indexes are removed.

Signed-off-by: nscuro <[email protected]>
  • Loading branch information
@nscuro
nscuro committed Dec 5, 2024
1 parent e00ede7 commit 7af919a
Show file tree
Hide file tree
Showing 4 changed files with 69 additions and 38 deletions.
8 changes: 1 addition & 7 deletions src/main/java/org/dependencytrack/model/PolicyViolation.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -23,13 +23,12 @@
import com.fasterxml.jackson.annotation.JsonIgnoreProperties;
import com.fasterxml.jackson.annotation.JsonInclude;
import io.swagger.v3.oas.annotations.media.Schema;

import jakarta.validation.constraints.NotNull;
import jakarta.validation.constraints.Pattern;
import jakarta.validation.constraints.Size;

import javax.jdo.annotations.Column;
import javax.jdo.annotations.IdGeneratorStrategy;
import javax.jdo.annotations.Index;
import javax.jdo.annotations.PersistenceCapable;
import javax.jdo.annotations.Persistent;
import javax.jdo.annotations.PrimaryKey;
Expand All @@ -47,9 +46,6 @@
@PersistenceCapable
@JsonInclude(JsonInclude.Include.NON_NULL)
@JsonIgnoreProperties(ignoreUnknown = true)
// TODO: Add @Unique composite constraint on the fields component, policyCondition, and type.
// The legacy PolicyEngine erroneously allows for duplicates on those fields, but CelPolicyEngine
// will never produce such duplicates. Until we remove the legacy engine, we can't add this constraint.
public class PolicyViolation implements Serializable {

public enum Type {
Expand All @@ -69,12 +65,10 @@ public enum Type {

@Persistent(defaultFetchGroup = "true")
@Column(name = "PROJECT_ID", allowsNull = "false")
@Index(name = "POLICYVIOLATION_PROJECT_IDX")
private Project project;

@Persistent(defaultFetchGroup = "true")
@Column(name = "COMPONENT_ID", allowsNull = "false")
@Index(name = "POLICYVIOLATION_COMPONENT_IDX")
private Component component;

@Persistent(defaultFetchGroup = "true")
Expand Down
5 changes: 1 addition & 4 deletions src/main/java/org/dependencytrack/policy/cel/CelPolicyEngine.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -66,7 +66,6 @@
import org.dependencytrack.util.VulnerabilityUtil;
import org.projectnessie.cel.tools.ScriptCreateException;
import org.projectnessie.cel.tools.ScriptException;
import org.slf4j.MDC;

import java.math.BigDecimal;
import java.time.Duration;
Expand All @@ -86,7 +85,6 @@
import static java.util.Collections.emptyList;
import static org.apache.commons.collections4.MultiMapUtils.emptyMultiValuedMap;
import static org.apache.commons.lang3.StringUtils.trimToEmpty;
import static org.dependencytrack.common.MdcKeys.MDC_PROJECT_UUID;
import static org.dependencytrack.persistence.jdbi.JdbiFactory.withJdbiHandle;
import static org.dependencytrack.policy.cel.definition.CelPolicyTypes.TYPE_COMPONENT;
import static org.dependencytrack.policy.cel.definition.CelPolicyTypes.TYPE_LICENSE;
Expand Down Expand Up @@ -143,8 +141,7 @@ public void evaluateProject(final UUID uuid) {
final long startTimeNs = System.nanoTime();

try (final var qm = new QueryManager();
final var celQm = new CelPolicyQueryManager(qm);
var ignoredMdcProjectUuid = MDC.putCloseable(MDC_PROJECT_UUID, uuid.toString())) {
final var celQm = new CelPolicyQueryManager(qm)) {
// TODO: Should this entire procedure run in a single DB transaction?
// Would be better for atomicity, but could block DB connections for prolonged
// period of time for larger projects with many violations.
Expand Down
83 changes: 56 additions & 27 deletions src/main/java/org/dependencytrack/tasks/PolicyEvaluationTask.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -28,10 +28,16 @@
import org.dependencytrack.model.WorkflowState;
import org.dependencytrack.persistence.QueryManager;
import org.dependencytrack.policy.cel.CelPolicyEngine;
import org.dependencytrack.util.WaitingLockConfiguration;
import org.slf4j.MDC;

import java.util.UUID;
import java.time.Duration;
import java.time.Instant;

import static org.dependencytrack.common.MdcKeys.MDC_COMPONENT_UUID;
import static org.dependencytrack.common.MdcKeys.MDC_PROJECT_UUID;
import static org.dependencytrack.model.WorkflowStep.POLICY_EVALUATION;
import static org.dependencytrack.util.LockProvider.executeWithLockWaiting;

/**
* A {@link Subscriber} task that executes policy evaluations for {@link Project}s or {@link Component}s.
Expand All @@ -49,40 +55,63 @@ public PolicyEvaluationTask() {

@Override
public void inform(final Event e) {
if (e instanceof final ProjectPolicyEvaluationEvent event) {
WorkflowState projectPolicyEvaluationState;
try (final var qm = new QueryManager()) {
projectPolicyEvaluationState = qm.updateStartTimeIfWorkflowStateExists(event.getChainIdentifier(), POLICY_EVALUATION);
try {
evaluateProject(event.getUuid());
qm.updateWorkflowStateToComplete(projectPolicyEvaluationState);
} catch (Exception ex) {
qm.updateWorkflowStateToFailed(projectPolicyEvaluationState, ex.getMessage());
LOGGER.error("An unexpected error occurred while evaluating policies for project " + event.getUuid(), ex);
}
try {
if (e instanceof final ProjectPolicyEvaluationEvent event) {
executeWithLockWaiting(createLockConfiguration(event), () -> evaluateProject(event));
} else if (e instanceof final ComponentPolicyEvaluationEvent event) {
executeWithLockWaiting(createLockConfiguration(event), () -> evaluateComponent(event));
}
} else if (e instanceof final ComponentPolicyEvaluationEvent event) {
WorkflowState componentMetricsEvaluationState;
try (final var qm = new QueryManager()) {
componentMetricsEvaluationState = qm.updateStartTimeIfWorkflowStateExists(event.getChainIdentifier(), POLICY_EVALUATION);
try {
evaluateComponent(event.getUuid());
qm.updateWorkflowStateToComplete(componentMetricsEvaluationState);
} catch (Exception ex) {
qm.updateWorkflowStateToFailed(componentMetricsEvaluationState, ex.getMessage());
LOGGER.error("An unexpected error occurred while evaluating policies for component " + event.getUuid(), ex);
}
} catch (Throwable t) {
LOGGER.error("Failed to execute policy evaluation", t);
}
}

private void evaluateProject(final ProjectPolicyEvaluationEvent event) {
WorkflowState projectPolicyEvaluationState;
try (final var qm = new QueryManager()) {
projectPolicyEvaluationState = qm.updateStartTimeIfWorkflowStateExists(event.getChainIdentifier(), POLICY_EVALUATION);
try (var ignoredMdcProjectUuid = MDC.putCloseable(MDC_PROJECT_UUID, event.getUuid().toString())) {
new CelPolicyEngine().evaluateProject(event.getUuid());
qm.updateWorkflowStateToComplete(projectPolicyEvaluationState);
} catch (Exception ex) {
qm.updateWorkflowStateToFailed(projectPolicyEvaluationState, ex.getMessage());
LOGGER.error("An unexpected error occurred while evaluating policies for project " + event.getUuid(), ex);
}
}
}

private void evaluateProject(final UUID uuid) {
new CelPolicyEngine().evaluateProject(uuid);
private void evaluateComponent(final ComponentPolicyEvaluationEvent event) {
WorkflowState componentMetricsEvaluationState;
try (final var qm = new QueryManager()) {
componentMetricsEvaluationState = qm.updateStartTimeIfWorkflowStateExists(event.getChainIdentifier(), POLICY_EVALUATION);
try (var ignoredMdcComponentUuid = MDC.putCloseable(MDC_COMPONENT_UUID, event.getUuid().toString())) {
new CelPolicyEngine().evaluateComponent(event.getUuid());
qm.updateWorkflowStateToComplete(componentMetricsEvaluationState);
} catch (Exception ex) {
qm.updateWorkflowStateToFailed(componentMetricsEvaluationState, ex.getMessage());
LOGGER.error("An unexpected error occurred while evaluating policies for component " + event.getUuid(), ex);
}
}
}

private void evaluateComponent(final UUID uuid) {
new CelPolicyEngine().evaluateComponent(uuid);
private static WaitingLockConfiguration createLockConfiguration(final ProjectPolicyEvaluationEvent event) {
return new WaitingLockConfiguration(
/* createdAt */ Instant.now(),
/* name */ "%s-project-%s".formatted(PolicyEvaluationTask.class.getSimpleName(), event.getUuid()),
/* lockAtMostFor */ Duration.ofMinutes(5),
/* lockAtLeastFor */ Duration.ZERO,
/* pollInterval */ Duration.ofMillis(100),
/* waitTimeout */ Duration.ofMinutes(5));
}

private static WaitingLockConfiguration createLockConfiguration(final ComponentPolicyEvaluationEvent event) {
return new WaitingLockConfiguration(
/* createdAt */ Instant.now(),
/* name */ "%s-component-%s".formatted(PolicyEvaluationTask.class.getSimpleName(), event.getUuid()),
/* lockAtMostFor */ Duration.ofMinutes(1),
/* lockAtLeastFor */ Duration.ZERO,
/* pollInterval */ Duration.ofMillis(100),
/* waitTimeout */ Duration.ofMinutes(1));
}

}
11 changes: 11 additions & 0 deletions src/main/resources/migration/changelog-v5.6.0.xml
View file
Original file line number Diff line number Diff line change
Expand Up @@ -132,4 +132,15 @@
AND "PROPERTYNAME" = 'nvd.feeds.url';
</sql>
</changeSet>

<changeSet id="v5.6.0-9" author="nscuro">
<dropIndex tableName="POLICYVIOLATION" indexName="POLICYVIOLATION_COMPONENT_IDX"/>
<dropIndex tableName="POLICYVIOLATION" indexName="POLICYVIOLATION_POLICYCONDITION_ID_IDX"/>
<dropIndex tableName="POLICYVIOLATION" indexName="POLICYVIOLATION_PROJECT_IDX"/>
<createIndex tableName="POLICYVIOLATION" indexName="POLICYVIOLATION_IDENTITY_IDX" unique="true">
<column name="COMPONENT_ID"/>
<column name="PROJECT_ID"/>
<column name="POLICYCONDITION_ID"/>
</createIndex>
</changeSet>
</databaseChangeLog>

0 comments on commit 7af919a

Please sign in to comment.