-
Notifications
You must be signed in to change notification settings - Fork 27
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
Feature: validation of detections against cms_main #303
Conversation
…ing should be enforced
…com/splunk/contentctl into feature/validation_against_cms_main
This reverts commit b3c6948.
contentctl/actions/detection_testing/infrastructures/DetectionTestingInfrastructure.py
Show resolved
Hide resolved
contentctl/actions/detection_testing/infrastructures/DetectionTestingInfrastructure.py
Show resolved
Hide resolved
contentctl/actions/detection_testing/infrastructures/DetectionTestingInfrastructure.py
Outdated
Show resolved
Hide resolved
f"[{self.infrastructure.instance_name}] {offset}: Matching cms_main entry " | ||
f"'{cms_entry_name}' against detections" | ||
) | ||
ptrn = re.compile(r"^" + self.global_config.app.label + r" - (?P<stripped_cms_entry_name>.+) - Rule$") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if it's better to go in reverse now that we have the following function to generate a search name:
contentctl/contentctl/objects/abstract_security_content_objects/detection_abstract.py
Line 101 in 98573b0
def get_action_dot_correlationsearch_dot_label(self, app:CustomApp, max_stanza_length:int=ES_MAX_STANZA_LENGTH)->str: |
This also means if we change the template used in the link above to generate the detection name, we don't have to change it again in this function.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ahhhhh I don't think I knew about this function, I'll take a look!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This one is FAIRLY new. In the future, it might be a nice idea to actually associate the app config with a detection so that we don't have to carry/pass it around, making action_dot_correlationsearch_dot_label
a cached_property instead of a function that takes an argument.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated this block to use this function for matching instead of regex
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Left some comments, but out of curiosity not because changes are required.
I will do a bit more local testing of these changes then get them merged and do a new contentctl release!
) | ||
self.logger.error(msg) | ||
return Exception(msg) | ||
elif cms_event["version"] != f"{detection.version}.1": |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are there any other fields we can/should be validating as well? For example, the search field itself, given that we have seen some consistency issues there in the past? Although we can argue that things which actually impact the search at runtime are caught during a later phase of the integration test.
Or is this an issue of "yes, we could probably check this, but then should we check EVERY field?"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So, yes! Hahah I have an open ticket PEX-509 (see comment on line 473) to look into this. I think there's a lot of room to validate more here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Re-approving after additional runtime testing. Nice, descriptive errors, and I like that testing bails as SOON as we detect this error - it means we don't waste time on a full integration test (which is costly in terms of time AND resource costs) when we know, early on, that there has been a CMS issue.
These errors were stimulated by running a test (with the latest security_content) and all apps, then rerunning the test after making some changes to YML. Since the changes were made in YML, they should not be reflected in cms_main and SHOULD show as error:
create new detection with new name - NOT FOUND IN CMS MAIN
create new detection with new name - CMS MAIN CONTAINS 1 LESS OBJECT THAN EXPECTED
change the version of an existing detection - WRONG VERSION IN CMS MAIN
change the uuid of existing detection - UUID MISMATCH WARNING
These errors are exactly what we would expect to find. Great work!
Thanks for the thorough testing Eric!! |
Context
Code change
ContentVersioningService
cms_main
and detections based on YAMLs matchcms_event
cms_event
can be matched to a detectioncms_event
is repeatedDetectionTestingInfrastructure
raise
any exceptions that crop up during setup, instead of just logging themDetectionTestingInfrastructure
to query installed ES version, so we can determine when it is appropriate to run content versioning validationAll
CorrelationSearch
(useful for debugging) tohelper.utils.Utils
so it could be re-used inContentVersioningService
Testing
savedsearches.conf
file and all seems to work wellescu-production
and all results passed (see attached truncated log)content_versioning_service.log
Caveats
cms_main
index should happen in aggregate, a test of the whole package, not individual detections. We do not have an easy way to insert testing at that level presently.TODOs