Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improve logging for notification publishing #3211

Merged
merged 9 commits into from Nov 21, 2023

Conversation

nscuro
Copy link
Member

@nscuro nscuro commented Nov 17, 2023

Description

It's hard to debug missing notifications, because Dependency-Track only logs something when an error occurred during notification publishing. Further, error logs currently do not contain any information about the notification or alert rule involved, making it harder to pinpoint where something is broken.

This PR adds more information to log messages emitted during publishing. This is done in form of PublishContext, which includes:

  • The group of the notification (e.g. NEW_VULNERABILITY)
  • The level of the notification (e.g. INFORMATIONAL)
  • The scope of the notification (e.g. PORTFOLIO)
  • The timestamp of the notification
  • The subjects of the notification
    • This is limited to "affected" project(s) (UUID, name, version), component (UUID, group, name, version), and vulns (vulnId, source), as to not flood the logs
    • Aim is to provide just enough information to debug
  • The name of the applicable alert rule
  • The scope of the applicable alert rule
  • The level of the applicable alert rule

Example log message:

Destination acknowledged reception of notification with status code 200 (PublishContext{notificationGroup=GROUP_BOM_PROCESSED, notificationLevel=LEVEL_INFORMATIONAL, notificationScope=SCOPE_PORTFOLIO, notificationTimestamp=2023-11-16T12:30:58.000Z, notificationSubjects={project=Project[uuid=8ebe0e60-0647-4f11-a048-a552ee8fe3df, name=Foo, version=123]}, ruleName=Example Rule, ruleScope=PORTFOLIO, ruleLevel=INFORMATIONAL})

This is a backport from Hyades, which already has this functionality: https://github.com/DependencyTrack/hyades/blob/v0.2.0/notification-publisher/src/main/java/org/hyades/notification/publisher/PublishContext.java

Addressed Issue

https://owasp.slack.com/archives/C6R3R32H4/p1699978082979659

TODOs

  • Emit a log message when notifications have been published successfully
  • Make the above configurable on a per-rule basis (required UI change)
  • Implement mechanism to mask sensitive parts of destination URLs (e.g. for Slack Webhook URLs)
    • Idea discarded; Trying to mask all possible URLs is a whack-a-mole game

Additional Details

image

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

@nscuro nscuro added the enhancement New feature or request label Nov 17, 2023
@nscuro nscuro added this to the 4.10 milestone Nov 17, 2023
@nscuro nscuro force-pushed the improve-notification-logging branch from f0e6de6 to 3ef7fdf Compare November 17, 2023 15:23
Signed-off-by: nscuro <nscuro@protonmail.com>
Signed-off-by: nscuro <nscuro@protonmail.com>
Also add more debug logs for notification routing, and masking of destination URL for Slack.

Signed-off-by: nscuro <nscuro@protonmail.com>
Signed-off-by: nscuro <nscuro@protonmail.com>
Webhook destination URLs contain secrets with high likelihood (e.g. Slack, Mattermost, MS Teams do). We could sanitize the URLs before logging them, but we cannot foresee all the various (and potentially custom) formats. So it's better to not log it at all. Given the applicable rule's name is logged, users can simply check the rule's configuration in case the destination URL is needed.

Signed-off-by: nscuro <nscuro@protonmail.com>
Signed-off-by: nscuro <nscuro@protonmail.com>
Signed-off-by: nscuro <nscuro@protonmail.com>
Signed-off-by: nscuro <nscuro@protonmail.com>
Signed-off-by: nscuro <nscuro@protonmail.com>
@nscuro nscuro force-pushed the improve-notification-logging branch from 9d9fd29 to 1058fb3 Compare November 20, 2023 18:44
@nscuro nscuro marked this pull request as ready for review November 20, 2023 18:50
@rkg-mm
Copy link
Contributor

rkg-mm commented Nov 21, 2023

Thanks, looks good from what I can see on source code level!

@nscuro nscuro merged commit a6de47b into DependencyTrack:master Nov 21, 2023
9 checks passed
@nscuro nscuro deleted the improve-notification-logging branch November 21, 2023 09:25
@nscuro nscuro mentioned this pull request Nov 24, 2023
2 tasks
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Dec 21, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants