[Feature] Vulnerability update notifications #4067

4naesthetic · 2024-08-12T18:12:21Z

Description

Adds support for sending notifications when the severity of an existing vulnerability associated with a project is updated.

Addressed Issue

Addresses: #2361
Supersedes: #2730

Additional Details

This PR builds on the previous work done in #2730 by @Ehoky (thank you for your work!).

The main difference is that the quantity of notifications has been reduced. Instead of generating a separate notification for every affected component associated with an updated vulnerability we only generate one. The previous implementation generated a notification for every instance of a component, even if they represented the same logical software package, which means that if the same vulnerable component was used across 200 projects then 200 notifications would be generated instead of one.

Other differences include:

No longer reporting on affected projects to prevent huge notification bodies, and only including a single component entry in the notification
Adding vuln source, vuln ID and aliases to the vulnerability notification subject JSON payload
Correctly emitting events from the AbstractNistMirrorTask that doesn't use the VulnerabilityQueryManager::updateVulnerability method

To support the 'Limit To Projects' functionality we still need to read all affected components (and their associated project IDs) before dispatching the notification even though they don't appear in the payload, so there is still some potential for scalability issues.

Checklist

I have read and understand the contributing guidelines
~~- [ ] This PR fixes a defect, and I have provided tests to verify that the fix is effective~~
This PR implements an enhancement, and I have provided tests to verify that it works as intended
~~- [ ] This PR introduces changes to the database model, and I have added corresponding update logic~~
This PR introduces new or alters existing behavior, and I have updated the documentation accordingly

Signed-off-by: Enora Germond <[email protected]> Signed-off-by: 4naesthetic <[email protected]>

…d NVD updates Signed-off-by: 4naesthetic <[email protected]>

codacy-production · 2024-08-13T11:06:53Z

Coverage summary from Codacy

See diff coverage on Codacy

Coverage variation	Diff coverage
✅ +0.08% (target: -1.00%)	✅ 86.67% (target: 70.00%)

Coverage variation details

	Coverable lines	Covered lines	Coverage
Common ancestor commit (`2812b3a`)	22107	16894	76.42%
Head commit (`a63290d`)	22233 (+126)	17008 (+114)	76.50% (+0.08%)

Coverage variation is the difference between the coverage for the head and common ancestor commits of the pull request branch: <coverage of head commit> - <coverage of common ancestor commit>

Diff coverage details

	Coverable lines	Covered lines	Diff coverage
Pull request (#4067)	120	104	86.67%

Diff coverage is the percentage of lines that are covered by tests out of the coverable lines that the pull request added or modified: <covered lines added or modified>/<coverable lines added or modified> * 100%

See your quality gate settings Change summary preferences

_{Codacy stopped sending the deprecated coverage status on June 5th, 2024. Learn more}

4naesthetic · 2024-08-13T15:37:53Z

Hmm... one of the new tests is failing in CI but passing locally which I'm having trouble replicating (NotificationUtilTest.testVulnerabilityUpdateMultipleComponents). Any ideas?

nscuro · 2024-08-13T15:52:36Z

Notifications are dispatched and processed asynchronously, so tests asserting on them can suffer from race conditions. I would recommend to use Awaitility to account for that, see here for an example:

dependency-track/src/test/java/org/dependencytrack/tasks/BomUploadProcessingTaskTest.java

Lines 1298 to 1302 in b4ac9cd

    
           await("BOM Processed Notification") 
        
                   .atMost(Duration.ofSeconds(3)) 
        
                   .untilAsserted(() -> assertThat(NOTIFICATIONS) 
        
                           .anyMatch(n -> NotificationGroup.BOM_PROCESSED.name().equals(n.getGroup()) 
        
                                          && NotificationScope.PORTFOLIO.name().equals(n.getScope())));

4naesthetic · 2024-08-13T17:37:07Z

Notifications are dispatched and processed asynchronously, so tests asserting on them can suffer from race conditions. I would recommend to use Awaitility to account for that, see here for an example:

dependency-track/src/test/java/org/dependencytrack/tasks/BomUploadProcessingTaskTest.java

Lines 1298 to 1302 in b4ac9cd

await("BOM Processed Notification")

.atMost(Duration.ofSeconds(3))

.untilAsserted(() -> assertThat(NOTIFICATIONS)

.anyMatch(n -> NotificationGroup.BOM_PROCESSED.name().equals(n.getGroup())

&& NotificationScope.PORTFOLIO.name().equals(n.getScope())));

Thanks for that. I've updated the asynchronous tests to use Awaitility so hopefully that should eliminate any race conditions.

Edit: Actually the conditions for the failing test still aren't quite correct. Will update again shortly.

Edit2: Ready to go now.

Signed-off-by: 4naesthetic <[email protected]>

4naesthetic · 2024-08-21T10:29:21Z

Frontend PR: DependencyTrack/frontend#974

I think this is ready for review. Any chance of this making it into the 4.12 release?

nscuro · 2024-08-21T10:48:28Z

I think this is ready for review. Any chance of this making it into the 4.12 release?

I'll try to get it reviewed this week. Unless there are some major issues I don't see why it wouldn't make it to v4.12.

codacy-production · 2024-08-21T11:08:14Z

Coverage summary from Codacy

See diff coverage on Codacy

Coverage variation	Diff coverage
✅ +0.40% (target: -1.00%)	✅ 86.44% (target: 70.00%)

Coverage variation details

	Coverable lines	Covered lines	Coverage
Common ancestor commit (`2812b3a`)	22107	16894	76.42%
Head commit (`1cb7273`)	22367 (+260)	17182 (+288)	76.82% (+0.40%)

Coverage variation is the difference between the coverage for the head and common ancestor commits of the pull request branch: <coverage of head commit> - <coverage of common ancestor commit>

Diff coverage details

	Coverable lines	Covered lines	Diff coverage
Pull request (#4067)	118	102	86.44%

Diff coverage is the percentage of lines that are covered by tests out of the coverable lines that the pull request added or modified: <covered lines added or modified>/<coverable lines added or modified> * 100%

See your quality gate settings Change summary preferences

_{Codacy stopped sending the deprecated coverage status on June 5th, 2024. Learn more}

nscuro · 2024-08-24T17:29:12Z

src/main/java/org/dependencytrack/tasks/AbstractNistMirrorTask.java

+                    // Check if we need to emit a vulnerability changed event
+                    if (diffs.containsKey("severity") && diffs.get("severity") != null) {
+                        final PersistenceUtil.Diff severityDiff = diffs.get("severity");
+                        if (severityDiff.before() instanceof final Severity oldSeverity && severityDiff.after() instanceof final Severity newSeverity) {
+                            Event.dispatch(new ProjectVulnerabilityUpdateEvent(persistentVuln, new VulnerabilityUpdateDiff(oldSeverity, newSeverity)));
+                        }
+                    }


Since this code executes within a transaction, it can happen that we emit an event, but committing the transaction fails, leaving us in an inconsistent state. Consider moving the event dispatch outside of the transaction block.

nscuro · 2024-08-24T17:32:51Z

src/main/java/org/dependencytrack/util/NotificationUtil.java

+            // To reduce noise we only emit a single notification for each updated vulnerability if it affects one
+            // of our components. The component details are still useful for event consumers, so we pick the first one.
+            final Component detachedComponent = qm.detach(Component.class, components.get(0).getId());


I feel like this could be a bit misleading. A vulnerability can potentially affect multiple libraries, operating systems, or hardware components. What is the benefit of including only one component?

There's definitely some nuance that I missed here.

I considered the earlier implementation where the event notification would include 1 vulnerability, 1 component and a list of affected projects (similar to a NEW_VULNERABILITY notification), which would mean generating a notification for every affected component entry in the database. As each logical component has a unique entry in the database for every project that uses it, this could mean emitting a large number of notifications where the content is identical except for the component.uuid field. So to try and reduce this redundancy and (in theory) reduce the database load, my logic was to choose just one of the "identical" component entries and emit a single event per updated vulnerability.

What I missed was that there could be separate logical components affected by the same vulnerability and picking only one of them doesn't make sense as you said.

I think what this comes down to is what useful contextual information about a vulnerability do we want to include in the notification? For my use case I actually only care about the vulnerability object itself, but others may have use cases where they need the affected component information (and potentially affected projects) as well.

I may be missing something, but at the moment there doesn't seem to be a way to fetch a list of unique logical component identities without doing a full table scan and de-duplicating. If this can't be done efficiently, perhaps it makes sense not to include component information in the notification. What do you think?

Edit: Potentially returning VulnerableSoftware or AffectedComponent entries could be the solution?

IMO we should keep the information in this notification as minimal as possible. All data we add now, users will start depending on it, and we can't easily remove it again. I think it's better to wait for users to come forward with their use cases, and then evaluating if / how that can be achieved with more data.

Transmitting affected projects is not feasible (see for example DependencyTrack/hyades#467 where we ran into this in another context).

Communicating logical components is complicated as you said, and I am hesitant to add this complexity without us knowing that users really need it.

If we only include the vulnerability itself now, receivers will have enough information to query the /api/v1/source/{source}/vuln/{vuln}/projects endpoint to fetch affected projects.

nscuro · 2024-08-24T17:33:52Z

src/main/java/org/dependencytrack/util/NotificationUtil.java

+    public static void analyzeNotificationCriteria(QueryManager qm, Vulnerability vulnerability, VulnerabilityUpdateDiff vulnerabilityUpdateDiff) {
+        // Vulnerabilities are not guaranteed to include component relationships depending on where the event was generated
+        final Vulnerability completeVulnerability = qm.getVulnerabilityByVulnId(vulnerability.getSource(), vulnerability.getVulnId());
+        final List<Component> components = completeVulnerability.getComponents();


Fetching all associated components at once can be very costly in large portfolios, especially since only one of them is actually used. Do we need to include this info (see comment below as well)?

This can absolutely be optimised if we don't need component information in the notification. It think it would be more efficient to include the affected projects instead, and use qm.getAffectedProjects(vuln) much like the REST API does in VulnerabilityResource.getAffectedProject() as we can then reuse this for filtering in NotificationRouter.

qm.getAffectedProjects(vuln) actually calls vuln.getComponents() in the query manager anyway (which makes sense now that I've thought about it properly). Even if we don't include affected projects or components in the notification subject, we still need the affected project list to be able to limit notifications by project (or tags in the future). On the other hand, if we don't filter by affected projects then we might emit a large number of irrelevant notifications whenever there is a vulnerability database update, which also has performance implications.

nscuro · 2024-08-24T17:40:54Z

src/main/java/org/dependencytrack/notification/vo/ProjectVulnerabilityUpdate.java

+import org.dependencytrack.model.Vulnerability;
+import org.dependencytrack.model.VulnerabilityUpdateDiff;
+
+public class ProjectVulnerabilityUpdate {


Consider making this a record instead.

I'd further recommend to rename this class to VulnerabilityUpdated, since it's not specific to a project, as the current name would suggest.

By using ProjectVulnerabilityUpdate instead of VulnerabilityUpdate I was trying to make it clear that the notification only represents an update to a vulnerability that affects a project in a portfolio. I thought it might be confusing to call it VulnerabilityUpdate, as that might lead someone to think that the notification would fire on any updates to a vulnerability in the vuln DB. Perhaps that's already clear enough though from the notification being scoped to the PORTFOLIO?

Ah, I see, that reasoning makes sense. IMO, VulnerabilityUpdated would still be less ambiguous than ProjectVulnerabilityUpdated. I guess there are still better ways to word the intention though, but my non-native-speaker skills are not helping.

I was thinking something like these, although they still sound a bit awkward:

AffectingVulnerabilityUpdated - i.e. a vuln affecting something was updated

FindingVulnerabilityUpdated - i.e. a vuln that is part of a finding was updated

Out of those two suggestions I think FindingVulnerabilityUpdated is better, but still might be confusing as someone might expect to receive a Finding in the event payload (which has a specific meaning and schema as an API resource).

Is ActiveVulnerabilityUpdated any better? It mirrors the idea of 'Active' and 'Inactive' projects.

nscuro · 2024-08-24T17:41:33Z

src/main/java/org/dependencytrack/model/VulnerabilityUpdateDiff.java

+ */
+package org.dependencytrack.model;
+
+public class VulnerabilityUpdateDiff {


Consider making this a record instead.

nscuro · 2024-08-24T17:45:29Z

src/main/java/org/dependencytrack/tasks/ProjectVulnerabilityUpdateTask.java

If we can get rid of the fetching of component data, this task and its corresponding event could be removed, and you could simply call NotificationUtil#analyzeNotificationCriteria in-line.

Using a task like this is really only required if generating the notification involves heavyweight I/O or computation. Otherwise, notification dispatch via Notification#dispatch is already asynchronous.

nscuro · 2024-08-24T17:49:13Z

src/main/java/org/dependencytrack/notification/publisher/PublishContext.java

+            return new VulnerabilityUpdateDiff(
+                    notificationVuln.getVulnId(),
+                    notificationVuln.getSource(),
+                    notificationVulnUpdateDiff.getOldSeverity(),
+                    notificationVulnUpdateDiff.getNewSeverity()
+            );


Do not include the actual diff values in here. Subjects are intended to give only surface-level information about the notification being dispatched, and they all appear in the logs.

Consider just mapping to SUBJECT_VULNERABILITY, and re-using Vulnerability#convert instead.

Feat: Add notification when severity of a vulnerability changes

f8402c9

Signed-off-by: Enora Germond <[email protected]> Signed-off-by: 4naesthetic <[email protected]>

4naesthetic force-pushed the feat/vulnerability-change-notifications branch 2 times, most recently from 91332a2 to 0ceb612 Compare August 13, 2024 10:28

Reduce quantity of notifications generated and emit events from misse…

66d50d7

…d NVD updates Signed-off-by: 4naesthetic <[email protected]>

4naesthetic force-pushed the feat/vulnerability-change-notifications branch from 0ceb612 to 66d50d7 Compare August 13, 2024 10:32

4naesthetic marked this pull request as draft August 13, 2024 11:23

4naesthetic force-pushed the feat/vulnerability-change-notifications branch from d138415 to 1711b9c Compare August 13, 2024 12:55

4naesthetic marked this pull request as ready for review August 13, 2024 13:01

4naesthetic force-pushed the feat/vulnerability-change-notifications branch from 1711b9c to 8404e8f Compare August 13, 2024 17:35

Additional test coverage

a63290d

Signed-off-by: 4naesthetic <[email protected]>

4naesthetic force-pushed the feat/vulnerability-change-notifications branch from 8404e8f to a63290d Compare August 13, 2024 17:55

Remove unnecessary component loop in analyzeNotificationCriteria

1cb7273

Signed-off-by: 4naesthetic <[email protected]>

4naesthetic mentioned this pull request Aug 21, 2024

[Feature] Vulnerability update notifications DependencyTrack/frontend#974

Open

2 tasks

nscuro added the enhancement New feature or request label Aug 21, 2024

nscuro requested changes Aug 24, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Feature] Vulnerability update notifications #4067

[Feature] Vulnerability update notifications #4067

4naesthetic commented Aug 12, 2024

codacy-production bot commented Aug 13, 2024 •

edited

Loading

4naesthetic commented Aug 13, 2024

nscuro commented Aug 13, 2024

4naesthetic commented Aug 13, 2024 •

edited

Loading

4naesthetic commented Aug 21, 2024

nscuro commented Aug 21, 2024

codacy-production bot commented Aug 21, 2024

nscuro Aug 24, 2024

nscuro Aug 24, 2024

4naesthetic Aug 25, 2024 •

edited

Loading

nscuro Aug 26, 2024

nscuro Aug 24, 2024

4naesthetic Aug 25, 2024

4naesthetic Aug 27, 2024

nscuro Aug 24, 2024

4naesthetic Aug 24, 2024

nscuro Aug 25, 2024

4naesthetic Aug 27, 2024

nscuro Aug 24, 2024

nscuro Aug 24, 2024

nscuro Aug 24, 2024

[Feature] Vulnerability update notifications #4067

Are you sure you want to change the base?

[Feature] Vulnerability update notifications #4067

Conversation

4naesthetic commented Aug 12, 2024

Description

Addressed Issue

Additional Details

Checklist

codacy-production bot commented Aug 13, 2024 • edited Loading

Coverage summary from Codacy

4naesthetic commented Aug 13, 2024

nscuro commented Aug 13, 2024

4naesthetic commented Aug 13, 2024 • edited Loading

4naesthetic commented Aug 21, 2024

nscuro commented Aug 21, 2024

codacy-production bot commented Aug 21, 2024

Coverage summary from Codacy

Choose a reason for hiding this comment

Choose a reason for hiding this comment

4naesthetic Aug 25, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codacy-production bot commented Aug 13, 2024 •

edited

Loading

4naesthetic commented Aug 13, 2024 •

edited

Loading

4naesthetic Aug 25, 2024 •

edited

Loading