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

Threat objects #234

Merged
merged 10 commits into from
Aug 21, 2024
Merged

Threat objects #234

merged 10 commits into from
Aug 21, 2024

Conversation

ljstella
Copy link
Contributor

@ljstella ljstella commented Aug 9, 2024

Supersedes #204

Breaks out risk objects vs threat objects for the purpose of "what needs to be in every result" vs "what needs to be have a value at least once". Instead of trying to keep track of what's missing, its much easier to keep track of what is definitely not null, adding it to a set as we go, and comparing it to the full set at the end.

@pyth0n1c feel free to test with all sorts of broken YAMLs, I think I have accounted for all of the weird edge cases of potential test failures, but its also possible I missed something between banging my head on the keyboard and wishing for magical datatypes that don't exist.

@cmcginley-splunk likewise, feel free to test. I tried to keep up with the type annotations and formatting elsewhere in this, but its quite likely I've ruined some of the stuff you've added.

@ljstella ljstella marked this pull request as ready for review August 9, 2024 22:27
@ljstella
Copy link
Contributor Author

ljstella commented Aug 9, 2024

Forgot to note, you can use splunk/security_content#2995 as an example PR that currently fails against main but passes with this- the detection detections/cloud/o365_zap_activity_detection.yml in particular exhibits the specific behavior I set out to check against- threat objects that are not present in all risk events, but are present in at least one. I tested we can still fail when something isn't present at all by adding a values(filename) as filename into the search and adjusting the observable file_name to filename- that field isn't in the underlying data so always ends up null.

Copy link
Contributor

@pyth0n1c pyth0n1c left a comment

Choose a reason for hiding this comment

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

I have left some comments with questions below, let me know your thoughts @ljstella .

I also wanted to post a few points for clarity:

  1. It would be FAR better if we had improved Pydantic Objects/typing around the "observables" section of our YMLs. Having this be a mix between Threat Objects and Risk Objects, which have different behaviors, leads to some confusion and far more complex handing in functions like this. I understand that updating those objects is a larger change and outside the scope of this PR.

  2. These changes update the check on observables to require that:
    a. If an observable is a Risk Object, EVERY result must contain a non-null value for each Risk Object.
    b. If an observable is a THREAT object, AT LEAST 1 of the results must contain a non-null value for each Threat Object. Here an is example: https://github.com/splunk/security_content/pull/2995/files#diff-93a083d42a51945f1ae116446dba1974ba182e98a18587ab3976b6054e797916R40-R51
    and a screenshot using that detection/test data:

image

Based on our discussions, we made this change because multiple different threats can create risk for one of my assets. In order to account for these different Threats, we allow one threat per event (but enforce that all possible Threat Objects must be created during a test/test dataset)

@ljstella ljstella requested a review from pyth0n1c August 13, 2024 18:41
Copy link
Contributor

@pyth0n1c pyth0n1c left a comment

Choose a reason for hiding this comment

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

Thanks Lou for feedback on my comments. After additional local testing, this looks great!
Approved!

@pyth0n1c pyth0n1c added the READY label Aug 13, 2024
@pyth0n1c pyth0n1c merged commit 6826bcc into main Aug 21, 2024
12 of 18 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants