AWS Sec Hub compliance.py not importing FindingId correctly - bug

[testaccount90009]

Not sure if this is a Bug or Enhancement, I'd consider a Bug but please label as necessary.

For AWS Security Hub parser and the compliance.py result = Finding(), the unique_id_from_tool=finding_id is not making it into the Finding data in DefectDojo, so as a (bad) workaround I've suggested adding it into the Description field. If finding_id can be correctly added to the Finding another way, please let me know and I can delete this (like a working unique_id_from_tool field) as that would be ideal, since I understand Description is used for deduping as well.

⚠️ Note on feature completeness ⚠️

We are narrowing the scope of acceptable enhancements to DefectDojo in preparation for v3. Learn more here:
https://github.com/DefectDojo/django-DefectDojo/blob/master/readme-docs/CONTRIBUTING.md

Description

Finding ID is crucial to have as a data piece for a Finding from AWS Security Hub into DefectDojo, but it is not being applied correctly. If the Finding ID can correctly be attributed from AWS to DefectDojo correctly in a better way (like unique_id_from_tool, even though that's what's being tried and for some reason the FindingId does not make it to DefectDojo).

Test results

Ideally you extend the test suite in tests/ and dojo/unittests to cover the changed in this PR.
Alternatively, describe what you have and haven't tested.

I don't think this has a test, or needs a new one? Assuming it had a test associated, it could catch the result = Finding for unique_id_from_tool=finding_id not being added correctly.

Documentation

Please update any documentation when needed in the documentation folder)

Checklist

This checklist is for your information.

• Make sure to rebase your PR against the very latest dev.
• Features/Changes should be submitted against the dev.
• [?] Bugfixes should be submitted against the bugfix branch.
• Give a meaningful name to your PR, as it may end up being used in the release notes.
• Your code is flake8 compliant.
• Your code is python 3.11 compliant.
• If this is a new feature and not a bug fix, you've included the proper documentation in the docs at https://github.com/DefectDojo/django-DefectDojo/tree/dev/docs as part of this PR.
• Model changes must include the necessary migrations in the dojo/db_migrations folder.
• Add applicable tests to the unit tests.
• Add the proper label to categorize your PR.

Extra information

Please clear everything below when submitting your pull request, it's here purely for your information.

Moderators: Labels currently accepted for PRs:

• Import Scans (for new scanners/importers)
• enhancement
• performance
• feature
• bugfix
• maintenance (a.k.a chores)
• dependencies
• New Migration (when the PR introduces a DB migration)
• settings_changes (when the PR introduces changes or new settings in settings.dist.py)

[dryrunsecurity]

Hi there 👋, @DryRunSecurity here, below is a summary of our analysis and findings.

DryRun Security	Status	Findings
Configured Codepaths Analyzer	✅	0 findings
Sensitive Files Analyzer	✅	0 findings
AppSec Analyzer	✅	0 findings
Authn/Authz Analyzer	✅	0 findings
Secrets Analyzer	✅	0 findings

Note

🟢 Risk threshold not exceeded.

Powered by DryRun Security

[mtesauro]

Note: Those Ruff linter issues are handled in #9903

[cneill]

I don't think putting the ID in the description is the right approach here. Do you have an example file for which unique_id_from_tool isn't captured properly? The code looks fine. Id is a required field in ASFF so it should never be empty, and it should always contain a unique ARN. Since the Compliance parser is the default for all Security Hub findings that don't match a known product name, I'm wondering if these are 3rd-party findings that don't properly populate the Id field.

[testaccount90009]

@cneill I can provide an in-depth example on Monday when I'm back. I definitely agree with you that it should be getting captured somewhere in the Findings and I can't suggest any better code because I think like you said - it should be fine. But in the Findings data in DefectDojo - there is no mention of the ARN or Id that is supposed to be captured. A colleague thought it could be due to the length or format of the ARN being returned. I understand the compliance.py parser is the default and these are matching the related fields for AWS Security Hubin the Product Related Fields. I will provide specifics on Monday to hopefully further help provide some insight into what we're experiencing.

[manuel-sommer]

I guess if Id would be empty, then the findings except one would not appear inside DefectDojo due to AWS Security Hub Scan': DEDUPE_ALGO_UNIQUE_ID_FROM_TOOL.
Yeah, a sample file would help in this case.

[testaccount90009]

@manuel-sommer @cneill as promised, here is the scrubbed sample json from AWS Security Hub. When I check in on the corresponding finding in DefectDojo - there is no mention of the Id / ARN for the specific resource. Ideally the ARN/Id of the resource would be in the Finding in DefectDojo somewhere. The full resource arn would be best in my opinion, since at large scale if a developer is looking at a Finding in DefectDojo (or the corresponding Jira) then it should contain all of the necessary data on where that Finding is. (Account#, Region, ResourceId, etc.)

I can see some Findings get the Endpoint / Host added with the IP correctly - but again, without the full ARN/Id information my corresponding teams would have no way of identifying where that Finding is coming from (Account#, Region, ResourceId, etc.).

Adding to the Description was the only way I could see passing back this information easily, since I assume things like CVE, CWE, or unique_id_from_tool will be wanting a traditionally structured "CVE-ID-1234" or "CWE-83" or "WS-2019-9321" (Whitesource ID), etc. and I imagine the full resource Id/ARN does not match that structure. Creating a custom field to attach the ARN for an AWS Finding may be best instead of trying to use unique_id_from_tool - but this is just my speculation at a high level.

I appreciate any input on this, as our teams are really loving using DefectDojo and I think solving this AWS Finding ARN association would be a huge benefit.
scrubbed-aws-findings-for-dojo.json

[cneill]

I think I misunderstood your original message here. Given that the AWS Security Hub parser uses "unique ID from tool" to de-duplicate Findings, I interpreted "is not making it into the Finding data in DefectDojo" as saying the unique ID wasn't captured at all and that this was causing problems. Below is a screenshot from importing the attached issue to demonstrate that it is in fact present in the Finding data.

Given that it is captured, just not displayed on the Finding view page, I think I now understand what you're after here. I don't have a problem with including this ID in the Finding description, however I would make it clear what that ID actually is. I'd revert the string-formatting on line 53 and add something like this on line 17:

description += f"\n**AWS Finding ARN:** {finding_id}\n"

For context: the unique ID from the tool is (exclusively?) used for de-duplication. Its intent is to distinguish individual instances of Findings in the same tool from one another that might otherwise look very similar, like if you had 2 separate cases of XSS in the same file. You're probably looking for something like the "vulnerability_ids" field, which is meant to capture global identifiers like CVEs. That field wouldn't be appropriate for AWS ARNs, though, because those identifiers are specific to a particular DefectDojo user's environment. I would be open to displaying the unique ID from the tool on the view Finding page, but I think in many cases that identifier won't be meaningful to an end-user the way it is with the AWS ARN used by Security Hub.

[testaccount90009]

@cneill thank you very much for the in-depth explanation and apologies for my poorly phrased original message. You are correct, the ARN/ResourceId is not displayed in the Finding and I think adding it into a visible area in the Finding would be extremely beneficial for a user attempting to view that Finding and follow up on the data presented.

I can go ahead and revert my changes and add your suggestion on Line 17. The context is very much appreciated!

[testaccount90009]

@cneill I've gone ahead and followed your suggestion - thank you again for the input.

[testaccount90009]

I think this unit test should work with these proposed changes. @manuel-sommer @cneill thank you very much for your guidance throughout this. I will try to contribute additional PRs in the future with some enhancements to some parsers (Mend related another topic another time). Please let me know if there's anything else I should check or add.

[testaccount90009]

@manuel-sommer @cneill I've attempted to add to the unit test but unfortunately I am not successful. Is there any guidance you could provide when you have some time? I am trying to understand what value to assert to the finding (or endpoint.host) in DefectDojo since my changes are mainly to the description, but I did not see a call in any other unit test example to the description field. I am probably doing something fundamentally incorrect and I am hoping to learn what that is.

[manuel-sommer]

@testaccount90009: you can test the unittests locally if you have a running DefectDojo instance locally.

./dc-unittest.sh --profile postgres-rabbitmq --test-case unittests.tools.<scannertest>.<scannerclass>
e.g. I am making a Progpilot PR and running the unittests through:
./dc-unittest.sh --profile postgres-rabbitmq --test-case unittests.tools.test_progpilot_parser.TestProgpilotParser

See this PR: #10052

Documentation:
https://defectdojo.github.io/django-DefectDojo/contributing/how-to-write-a-parser/#unit-tests

[manuel-sommer]

I commented already (see above)

[testaccount90009]

I removed my failing unit test for now and can re add it later.

[manuel-sommer]

@testaccount90009 you need a unittest to finish this PR. I would like to take over here if this is ok for you as I also need this fix to get live.

[testaccount90009]

@manuel-sommer sure no worries at all. Sorry I am unable to get a correctly functioning unit test in time. If you would like to commit to this PR, or open a new one (hopefully containing these proposed changes as well) I can close this one, no problem. If there is a new one and not too much trouble, I would very much appreciate being tagged on it to learn. I am curious to see and hope to continue learning.

[testaccount90009]

Closing this PR since this one here (#10072) is taking its place.

Thank you again for the opportunity to learn here.

Cheers!