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

Global Audit View: Vulnerabilities #2472

Merged

Conversation

rbt-mm
Copy link
Contributor

@rbt-mm rbt-mm commented Feb 8, 2023

Description

This PR introduces introduces new API endpoints in the FindingResource of the backend. These two new endpoints filter every finding by ACLs and other optional filters and then returns them , either by occurrence of vulnerability or grouped by vulnerability.

Those endpoints are used in a new view, which is introduced in the Frontend PR, and grant a simple way of gathering all findings in one place and filtering/sorting them by different criteria.

Addressed Issue

#1770

Additional Details

  • Requires the VIEW_VULNERABILITY permission
  • Tested on internal H2 Database, MSSQL and PostgreSQL

localhost_8081_vulnerabilityAudit (3)

localhost_8081_vulnerabilityAudit (4)

A PR for a policy violations audit will soon follow!

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

@valentijnscholten
Copy link
Contributor

I tried it, but whatever I put in the filters on the left, or Clear All, I always get No Matching records found.

@rkg-mm
Copy link
Contributor

rkg-mm commented Apr 15, 2023

I tried it, but whatever I put in the filters on the left, or Clear All, I always get No Matching records found.

Thats strange since I tested it too.. you have projects with vulnerabilities and your account also has permissions to audit vulnerabilities on these projects?

@valentijnscholten
Copy link
Contributor

valentijnscholten commented Apr 15, 2023

I am logged in as admin with all permissions assigned. And I am seeing the menu entry in the left menu bar.

My main usecase for this view is to see the most recently found vulnerabilities and the projects that are affected. So even without permissions to audit vulnerabilities, it would be useful to be able to use this view.

EDIT: Hmm all projects have disappeared, let me check what's going on.

@rkg-mm
Copy link
Contributor

rkg-mm commented Apr 15, 2023

I am logged in as admin with all permissions assigned. And I am seeing the menu entry in the left menu bar.

My main usecase for this view is to see the most recently found vulnerabilities and the projects that are affected. So even without permissions to audit vulnerabilities, it would be useful to be able to use this view.

Ah yes, my mistake, as Richard mentioned in the PR description: VIEW_VULNERABILITY is enough and should fit your use case.

EDIT: Hmm all projects have disappeared, let me check what's going on.

That sounds like the reason then

@valentijnscholten
Copy link
Contributor

Yeah, projects are back and I can see vulnerabilities. But the initial page load takes a very long time. The download size is over 14MB for that xhr call.

@rkg-mm
Copy link
Contributor

rkg-mm commented Apr 15, 2023

Damn. That sounds a bit too much. We don't by chance have some huge example BOMs prepared that could be shared for contributors to test changes on larger databases? Our test setup is too small it seems :D

@valentijnscholten
Copy link
Contributor

I have ~200 projects and ~6000 vulnerabilities. Might be that that big join without pagina is slow, or that DataNucleus performs a couple of extra queries on each finding to get missing data.

Here's a link a 22MB BOM with 9k components: #1905 (comment)

@rkg-mm
Copy link
Contributor

rkg-mm commented Apr 15, 2023

That big JOIN might take some time - but the result is still only 25 (or whatever you configured) entries, which should even for 100 rows not be 14MB

@melba-lopez melba-lopez added the enhancement New feature or request label Jul 28, 2023
@melba-lopez
Copy link
Contributor

@rkg-mm are you able to fix/address the sonatype bugs that were identified?

@rkg-mm
Copy link
Contributor

rkg-mm commented Aug 8, 2023

@melba-lopez

@rkg-mm are you able to fix/address the sonatype bugs that were identified?

@rbt-mm will provide a reworked version soon, that fixes the performance issues and will also address sonatype issues. Please don't merge for now.

@rbt-mm
Copy link
Contributor Author

rbt-mm commented Aug 8, 2023

This PR is now updated to include the following changes:

  • Changed pagination from client side to server side, resulting in faster load times
  • Changed hierarchical ACL processing to the default ACL processing that is currently used in Dependency-Track (database specific code is now removed)

Please check this updated feature out again and leave your feedback!

Copy link
Member

@nscuro nscuro left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the PR, and addressing all comments so far @rbt-mm and @rkg-mm!

A few additional comments from my side regarding the implementation.

As for the feature itself, it certainly makes sense to me. However, especially for reporting, requirements and desired features can vary quite a bit between organizations. Before we ship this feature, I'd really love more community input. I posted a request for it in our Slack channel: https://owasp.slack.com/archives/C6R3R32H4/p1692563653439049

We discussed this feature in our last DT maintainers meeting, and we decided to not include it in the upcoming 4.9 release. For that, we're mostly waiting for a few small blockers to unblock (e.g. #2850), but otherwise we want to get that release shipped ASAP.

As mentioned earlier, we believe that this feature needs more eyes on it, and also more testing. The aim would be to get it merged as soon as 4.9 is out of the door. We're aware of the resource constraints on your side, but given the current state and quality of the code in this PR (props to @rbt-mm!), we're optimistic that we can finish the work ourselves if required.

@rkg-mm
Copy link
Contributor

rkg-mm commented Aug 21, 2023

Thanks for the PR, and addressing all comments so far @rbt-mm and @rkg-mm!

A few additional comments from my side regarding the implementation.

As for the feature itself, it certainly makes sense to me. However, especially for reporting, requirements and desired features can vary quite a bit between organizations. Before we ship this feature, I'd really love more community input. I posted a request for it in our Slack channel: https://owasp.slack.com/archives/C6R3R32H4/p1692563653439049

We discussed this feature in our last DT maintainers meeting, and we decided to not include it in the upcoming 4.9 release. For that, we're mostly waiting for a few small blockers to unblock (e.g. #2850), but otherwise we want to get that release shipped ASAP.

As mentioned earlier, we believe that this feature needs more eyes on it, and also more testing. The aim would be to get it merged as soon as 4.9 is out of the door. We're aware of the resource constraints on your side, but given the current state and quality of the code in this PR (props to @rbt-mm!), we're optimistic that we can finish the work ourselves if required.

Thanks for the Feedback, OK let's wait for community input here

@nscuro
Copy link
Member

nscuro commented Aug 21, 2023

For folks willing to test this, I pushed container images to Docker Hub:

  • dependencytrack/apiserver:1770-global-audit-view
  • dependencytrack/frontend:1770-global-audit-view

@melba-lopez
Copy link
Contributor

melba-lopez commented Aug 23, 2023

thanks for this feature @rbt-mm and @rkg-mm !! I 💟 where this is going and would see this being extremely useful! Most of my questions below i made as a "code" formatting for ease of finding.

I see the business need especially for PSIRT and CISO organizations. One thing that dawned on me as I was experimenting with the new dashboard is i wish i had a "roll up" view. For smaller organizations or maybe even a few projects, I feel the way it is could suffice. However, as the number of projects get bigger or number of users/personas get bigger, we would want some sort of summary. Then being able to click on that summary to drill down into the areas of interest. I'm not sure how this would be presented to the user visually or the type of data. However, I did not see an item specifically pertaining to this, so not sure if this should be in a new PR or not.

My DT is still syncing with nvd etc, so i think it may still be processing in the background to populate the dashboard/metrics. If after its done, I still see broken summaries we may want to revisit the code to see where it might need to be tweaked.

image
image
image
image
image
image

When i go to the new dashboard, i'm a bit confused by the occurances vs groups. Could we add a mini description in the ui (maybe a hover) to explain to the user the purpose of these two views? Instead of 2 views, should it be an option for the user? Also, doing a spot check with moment, I see 8 distinct packages with 8 critical vulnerabilities (see screenshot above), but in the occurances dashboard i only see 2. Similarly the same output happens with "grouped" dashboard. I filtered on high/critical so was surprised i didn't get a minimum of 8 critical (in at least one of the dashboards). Should this be expected?

image
image

Thanks @nscuro for publishing a snapshot of this feature! Was fun playing with it and excited for its future release 😄

@rkg-mm
Copy link
Contributor

rkg-mm commented Aug 23, 2023

@melba-lopez I think the missing vulnerabilties are due to the issue nscuro pointed out already, basically this: #2474 we probably need to change that behaviour in parallel to make the filtering work correct, but that seems to be a smaller change to me.
Can you check if you see all of them without severity filter?

Regarding the rollup:
Yes I agree, this would be helpful, but we lacked time and wanted to have a first version working asap. My idea is to extend this later to show the same/a similar rollup like in the vulnerabilities view. But this could be a future improvement in my opinion.

Am explanation for the views could surely be added @rbt-mm. Maybe we can put something like an info icon the tab-header, or as a mouseover to it? Otherwise an info icon somewhere in the top of the tab.
We made it 2 distinct views because technically its easier to handle the different tables as different views, since they need different definitions.

@rkg-mm
Copy link
Contributor

rkg-mm commented Sep 19, 2023

@nscuro apart from an additional change for #2474, anything else you would need for this PR? I'll try to get someone to work on #2474 next weeks.

@rkg-mm
Copy link
Contributor

rkg-mm commented Oct 27, 2023

We just created a PR for #2474 -> #3151
After this PR I think this feature should be mergeable :)

@nscuro
Copy link
Member

nscuro commented Oct 27, 2023

Thanks @rkg-mm, I will have a look shortly.

@rkg-mm
Copy link
Contributor

rkg-mm commented Dec 18, 2023

@nscuro Can you add this to 4.11 milestone?

@nscuro
Copy link
Member

nscuro commented Feb 8, 2024

@rkg-mm / @rbt-mm Could you rebase this PR on top of the current master please, to resolve the merge conflict? Also as of #3408, a solution for the last blocker for this PR has been merged.

Adds two new API methods to the FindingResource, which return a
filtered list (ACL and optional other filters) of every finding, either
by occurrence or grouped by vulnerability, to allow users to quickly
get every finding for all of their projects.

Signed-off-by: RBickert <[email protected]>
Adds test for the new class `GroupedFinding` and for the new methods in
the `FindingResource`.

Signed-off-by: RBickert <[email protected]>
Calculate severity if NULL in database

Adjust tests

Signed-off-by: RBickert <[email protected]>
Integrates server side pagination and ordering in
FindingsSearchQueryManager to reduce the Frontend traffic by only
sending the necessary data

Signed-off-by: RBickert <[email protected]>
Signed-off-by: RBickert <[email protected]>
Adds filters and sorting for CVSSv2 to the FindingsSearchQueryManager
to use it in the Vulnerability Audit in the Frontend

Signed-off-by: RBickert <[email protected]>
Fixes duplicate entries of the same finding appearing for
every team membership of the user

Signed-off-by: RBickert <[email protected]>
@rbt-mm rbt-mm force-pushed the master-global-audit-view-vulnerabilities branch from 7c8792a to 506b0dc Compare February 8, 2024 12:49
@rkg-mm
Copy link
Contributor

rkg-mm commented Feb 8, 2024

@nscuro should be fine now

Copy link
Member

@nscuro nscuro left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks and behaves good.

Thanks @rkg-mm and @rbt-mm, and deep apologies for taking this long to get this over the line. Really appreciate your contributions.

@nscuro nscuro merged commit aa3474c into DependencyTrack:master Feb 21, 2024
9 checks passed
@rbt-mm rbt-mm mentioned this pull request Mar 12, 2024
2 tasks
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Mar 23, 2024
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.

6 participants