Skip to content

Adding risk data model validation to integration testing #387

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

Merged
merged 7 commits into from
Apr 29, 2025

Conversation

cmcginley-splunk
Copy link
Contributor

@cmcginley-splunk cmcginley-splunk commented Mar 27, 2025

Context

  • ES 8+ now allows notables (Findings) to have risk data associated with them
  • We have TTP detections which generate both risks and notables, so we want to ensure risk data is only being associated with risks in this case to avoid risk score doubling
  • We also have Correlation detections which generate notables alone (often based on an aggregation of existing risk entities); as of now, we don't define any risk modifiers for these, so we wouldn't expect them to be associated w/ risk data
  • It's possible that ESCU's behavior in the future may change, but as of now, we do not expect ESCU detections to generate notables which appear in the risk data model

Code changes

  • Added new methods to CorrelationSearch class which retrieves events from the risk data model
  • During notable validation, risk data model is checked to ensure no notables appear in there
  • New base class added to support notable and risk events

Testing

  • TODO

TODOs

  • disable logging
  • address outstanding in-line TODOs

@cmcginley-splunk cmcginley-splunk marked this pull request as draft March 27, 2025 18:23
@cmcginley-splunk cmcginley-splunk marked this pull request as ready for review March 27, 2025 19:16
pyth0n1c
pyth0n1c previously approved these changes Apr 29, 2025
@@ -33,7 +34,7 @@
from contentctl.objects.risk_event import RiskEvent

# Suppress logging by default; enable for local testing
ENABLE_LOGGING = False
ENABLE_LOGGING = True
Copy link
Contributor

Choose a reason for hiding this comment

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

I am going to switch this to False.
But we should probably consider exposing ALL logging and verbosity as a common setting somewhere (possibly in contentctl.yml or the command line) since as we add more and more of it, it can be extremely useful

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 made two additional small changes to bump the version and disable logging.
Given those are small changes, I will approve and merge myself.

@pyth0n1c pyth0n1c merged commit df437ae into main Apr 29, 2025
16 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants