[outputs] adding support for required alerting outputs #643

ryandeivert · 2018-03-20T20:31:09Z

to: @austinbyers
cc: @airbnb/streamalert-maintainers
size: medium

Background

A previous change was made to enable athena as part of the default deployment. Due to this change, StreamAlert must enforce sending alerts to required outputs even if they are not defined in the actual rule.

Changes

Required alerting outputs will be hard-coded in the the stream_alert/shared/resource.py module.
- Currently, this only includes the default aws-firehose output for alerts
The rule processor will combine the default output names with the user-defined outputs for a rule.
The alert processor config will now be passed to the resources function to merge in required outputs.
Updating rules as necessary.
Adding suppression for noisy log statement during tests.

Testing

Updating unit tests that broke because of this change. Adding tests for the shared functions that return the required outputs.

ryandeivert · 2018-03-20T20:52:22Z

@austinbyers just fixed the integration testing mocks, PTAL

coveralls · 2018-03-20T20:55:24Z

Coverage increased (+0.08%) to 95.727% when pulling da8192a on ryandeivert-default-output into a97fb60 on master.

austinbyers

I like how rules don't need explicit outputs now! A few comments

austinbyers · 2018-03-20T21:00:12Z

stream_alert/rule_processor/rules_engine.py

@@ -19,7 +19,7 @@

 from stream_alert.rule_processor import LOGGER
 from stream_alert.rule_processor.threat_intel import StreamThreatIntel
-from stream_alert.shared import NORMALIZATION_KEY
+from stream_alert.shared import resources, NORMALIZATION_KEY


In the alert processor, these are imported in the other order. Not sure which way is the "proper" alphabetization, but let's make it the same either way

good catch - this was a lazy find/replace since I used to have resources named common but switch the naming convention

austinbyers · 2018-03-20T21:06:51Z

stream_alert/rule_processor/rules_engine.py

@@ -53,6 +53,7 @@ class StreamRules(object):
    def __init__(self, config):
        """Initialize a StreamRules instance to cache a StreamThreatIntel instance."""
        self._threat_intel = StreamThreatIntel.load_from_config(config)
+        self._required_outputs = set(resources.get_required_outputs())


get_required_outputs() already returns a set, it doesn't need to be recreated as such. However, for clarity, you could rename self._required_outputs to self._required_output_set if you wanted

good catch - the initial implementation of get_required_outputs actually returned a dict, which needed to be cast to set. updating now

austinbyers · 2018-03-20T21:09:57Z

stream_alert/shared/resources.py

+    outputs = set()
+    for service, value in REQUIRED_OUTPUTS.iteritems():
+        if not isinstance(value, dict):
+            continue


Since REQUIRED_OUTPUTS is hardcoded, I don't think we need any error-checking logic. I would rather fail with an exception here than just silently skip what is likely a problem with the REQUIRED_OUTPUTS definition

austinbyers · 2018-03-20T21:10:16Z

stream_alert/shared/resources.py

+    """
+    for service, value in REQUIRED_OUTPUTS.iteritems():
+        if not isinstance(value, dict):
+            continue


Ditto - I don't think we need this

austinbyers · 2018-03-20T21:13:03Z

stream_alert/shared/resources.py

+    """Iterates through the required outputs and merges them with the user outputs
+
+    Args:
+        user_config (dict): Loaded user outputs dictionary from conf/outputs.json


Can you add a line explaining that the incoming config is modified? Just reading the docstring I was under the impression a new dictionary was returned

I will actually change this to return a copy, not modify in place

austinbyers · 2018-03-20T21:28:48Z

stream_alert/rule_processor/rules_engine.py

+            # Combine the required alert outputs with the ones for this rule
+            all_outputs = self._required_outputs
+            if rule.outputs:
+                all_outputs = all_outputs.union(set(rule.outputs))


When I saw this, I thought it was a bug because it looks like modifying all_outputs should modify self._required_outputs (they are initially the same reference). But this line actually rebinds all_outputs to reference a new set created by the union.

For clarity, and to avoid possible future bugs with this, can we just do

all_outputs = self._required_outputs | set(rule.outputs)

Ideally, rule.outputs is always a list or set, and that will work fine. But if not, you can do

all_outputs = self._required_outputs | set(rule.outputs or [])

ah thank you! I was actually trying to figure out the best way to ensure rule outputs wasn't None. going to use set.union vs | for clarity

austinbyers · 2018-03-20T21:31:04Z

stream_alert/rule_processor/rules_engine.py

            alert = {
                'id': alert_id,
                'record': record,
                'rule_name': rule.rule_name,
                'rule_description': rule.rule_function.__doc__ or DEFAULT_RULE_DESCRIPTION,
                'log_source': str(payload.log_source),
                'log_type': payload.type,
-                'outputs': rule.outputs,
+                'outputs': list(all_outputs),


We actually are saving the outputs as a StringSet in Dynamo, so I'd say let's keep this as a set

adding a TODO to change this :)

Yeah, you still need JSON-compatibility for the alert processor (but not for much longer!)

austinbyers · 2018-03-20T21:31:55Z

stream_alert/shared/__init__.py

@@ -1,6 +1,7 @@
 """Define some shared resources."""
 import logging
 import os
+import string


I'm surprised pylint didn't complain about this! Just of curiosity, where did it come from?

oh interesting - I think pylint won't complain about imports within __init__ since it probable assumes you're doing package imports like from . import *

removed it now :)

ryandeivert · 2018-03-20T22:42:30Z

hey @austinbyers just addressed your feedback. PTAL when you get a chance

austinbyers · 2018-03-20T22:45:54Z

stream_alert/shared/resources.py

-    return outputs
+    return {'{}:{}'.format(service, output)
+            for service, value in REQUIRED_OUTPUTS.iteritems()
+            for output in value.keys()}


So Pythonic and elegant! 🐍

austinbyers

💯

ryandeivert force-pushed the ryandeivert-default-output branch 2 times, most recently from c1d7a61 to 6c7e296 Compare March 20, 2018 20:50

austinbyers reviewed Mar 20, 2018

View reviewed changes

ryandeivert added 6 commits March 20, 2018 15:41

adding preliminary support for hardcoded required outputs

0306a71

simplifying a bit and migrating code to resources file in shared module

8d84938

[test] suppressing noisy logging while running tests

8a5a616

updating test suite to support required outputs

8d62137

updates to rules to remove output that is required by default

1188ea4

addressing pr feedback

da8192a

ryandeivert force-pushed the ryandeivert-default-output branch from 3e53ef7 to da8192a Compare March 20, 2018 22:42

austinbyers reviewed Mar 20, 2018

View reviewed changes

austinbyers approved these changes Mar 20, 2018

View reviewed changes

ryandeivert merged commit e6bd483 into master Mar 20, 2018

austinbyers deleted the ryandeivert-default-output branch March 20, 2018 22:56

austinbyers mentioned this pull request Mar 21, 2018

Create alert merger which dispatches alerts from Dynamo to alert processors #642

Merged

ryandeivert added this to the 2.0.0 milestone Apr 5, 2018

ryandeivert added alerting outputs historical search labels Apr 5, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[outputs] adding support for required alerting outputs #643

[outputs] adding support for required alerting outputs #643

ryandeivert commented Mar 20, 2018

ryandeivert commented Mar 20, 2018

coveralls commented Mar 20, 2018 •

edited

Loading

austinbyers left a comment •

edited

Loading

austinbyers Mar 20, 2018

ryandeivert Mar 20, 2018

austinbyers Mar 20, 2018

ryandeivert Mar 20, 2018

austinbyers Mar 20, 2018

austinbyers Mar 20, 2018

austinbyers Mar 20, 2018

ryandeivert Mar 20, 2018

austinbyers Mar 20, 2018

ryandeivert Mar 20, 2018 •

edited

Loading

austinbyers Mar 20, 2018

ryandeivert Mar 20, 2018

austinbyers Mar 20, 2018

austinbyers Mar 20, 2018

ryandeivert Mar 20, 2018 •

edited

Loading

ryandeivert commented Mar 20, 2018

austinbyers Mar 20, 2018

austinbyers left a comment

[outputs] adding support for required alerting outputs #643

[outputs] adding support for required alerting outputs #643

Conversation

ryandeivert commented Mar 20, 2018

Background

Changes

Testing

ryandeivert commented Mar 20, 2018

coveralls commented Mar 20, 2018 • edited Loading

austinbyers left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ryandeivert Mar 20, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ryandeivert Mar 20, 2018 • edited Loading

Choose a reason for hiding this comment

ryandeivert commented Mar 20, 2018

Choose a reason for hiding this comment

austinbyers left a comment

Choose a reason for hiding this comment

coveralls commented Mar 20, 2018 •

edited

Loading

austinbyers left a comment •

edited

Loading

ryandeivert Mar 20, 2018 •

edited

Loading

ryandeivert Mar 20, 2018 •

edited

Loading