[lambda][output][cli] modular outputs refactor #97

ryandeivert · 2017-04-06T07:56:35Z

to @airbnb/streamalert-maintainers
size: large
resolves: #6

Changes

stream_alert_cli.py can now configure new integrations for supported services
- currently supported services are: aws-s3, slack, pagerduty, phantom
OutputBase class handles all shared operations for each services
subclasses of OutputBase (such as SlackOutput) are designed to handle service-specific takes such as dispatching the actual alert
new output classes for each services dictate what is required for each integration
outputs.json file in root config is currently an empty json map, but is used to house all new output configurations added by the user through the cli
credential storage is now handled by S3 and encrypted creds will not be packaged directly
- kms is used to encrypt the creds prior to sending to S3
storing creds in encrypted_credentials directory is deprecated and will be going away in a future commit
credential storage has been switched to a json map to allow for better flexibility and ability to store arbitrary key/value pairs
credentials will be cached in tmp and loaded from there if they exist
- creds in tmp must still be decrypted using kms if they are cached (will not store decrypted creds locally)
some terraform updates to enable getting objects from the encrypted secrets bucket
will prevent user from adding same output configuration twice
Some notes on how boto3 exceptions are handled:
- when configuring a new output: if the credential info cannot be written to S3, we will log an error and raise the exception
- when sending alerts to outputs: if the credential info cannot be read from S3, we will log an error and continue. This is very much so by design, since we want to try to send alerts to the other output options even if there is a failure with one.
various typo fixes in other code not related to this refactor

Why

Creates a better user experience when adding an integration for a supported service.
Enables 1-to-many ability for outputs. Now, instead of only being able to send to one destination within a service (say, slack), users can configure as many as they want and choose which to send to for which rules.
Makes adding new services for output much easier through a common base class.
Encrypted credentials do not have to be packaged with the lambda function and are stored securely on S3.

jacknagz

good start!

jacknagz · 2017-04-06T16:20:22Z

stream_alert/alert_processor/output_base.py

+
+
+class OutputBase(object):
+    """StreamOutputBase is the base class to handle routing alters to outputs


StreamOutputBase should be OutputBase in this comment

typo: routing alerts*

I would add some more background on how to use this base class, along with going into the class variables just below this comment. Also describe the public methods and attributes available

changed base name and fixed typo. working on documentation also

jacknagz · 2017-04-06T16:24:06Z

stream_alert/alert_processor/outputs.py

+            alert [dict]: The alert relevant to the triggered rule
+        """
+        creds = self._load_creds(descriptor)
+        message = "StreamAlert Rule Triggered - {}".format(rule_name)


nit: no need for double quotes

jacknagz · 2017-04-06T16:24:45Z

stream_alert/alert_processor/outputs.py

+        """
+        creds = self._load_creds(descriptor)
+        message = "StreamAlert Rule Triggered - {}".format(rule_name)
+        values_json = json.dumps({


nit 2: double quote usage in this variable

jacknagz · 2017-04-06T16:30:29Z

stream_alert/alert_processor/output_base.py

+
+        Args:
+            cred_location [string]: tmp path on disk to to store the encrypted blob
+            descriptor [string]: service destination (ie: slack channel, pd integration)


Add a Returns: description here

jacknagz · 2017-04-06T16:32:12Z

stream_alert/alert_processor/output_base.py

+        """
+        LOGGER.error('unable to send alert for service %s', self.__service__)
+
+    def push_creds_to_s3(self, user_input):


shouldn't this be in the cli library?

jacknagz · 2017-04-06T16:42:35Z

stream_alert/alert_processor/output_base.py

+        return resp and (200 <= resp.getcode() <= 299)
+
+    @classmethod
+    def get_user_defined_properties(cls):


can you add more background on usage here?

jacknagz · 2017-04-06T16:42:52Z

stream_alert/alert_processor/output_base.py

+        pass
+
+    @classmethod
+    def get_default_properties(cls):


also add usage here, what should the orderdict contain?

jacknagz · 2017-04-06T16:43:34Z

stream_alert/alert_processor/output_base.py

+
+    def dispatch(self, descriptor, rule_name, alert):
+        """Send alerts to the given service. This base class just
+            logs an error if not implemented on the inheriting class


as mentioned in the class declaration, we should enforce it with ABC

jacknagz · 2017-04-06T16:45:03Z

stream_alert/alert_processor/outputs.py

+                            cred_requirement=True))
+        ])
+
+    def dispatch(self, descriptor, _, alert):


should we just use kwargs instead of negating certain positional arguments with _?

jacknagz · 2017-04-06T16:45:14Z

stream_alert/alert_processor/outputs.py

+        url = os.path.join(creds['url'])
+
+        slack_message = json.dumps({'text': '```{}```'.format(
+            json.dumps(alert, indent=4)


austinbyers

Great work! I love the generic base class for outputs, that's exactly what I envisioned.

Overall:

Use logging.exception when appropriate
Be careful that the docstring exactly matches the actual arguments and return types

austinbyers · 2017-04-07T21:59:31Z

stream_alert/alert_processor/main.py

-
-    def run(self, alerts):
-        """Send an Alert to its described outputs.
+            logging.error('an error occurred while decoding message to JSON: %s', err)


Use logging.exception (which is basically a wrapper around logging.error that automatically adds the exception context). Also use the LOGGER object?

yes to all this - good catches

austinbyers · 2017-04-07T22:02:05Z

stream_alert/alert_processor/main.py

-    def run(self, alerts):
-        """Send an Alert to its described outputs.
+            logging.error('an error occurred while decoding message to JSON: %s', err)
+            return


Do we not want to bubble this error all the way up so that the Lambda invocation fails?

austinbyers · 2017-04-07T22:02:21Z

stream_alert/alert_processor/main.py

-        Group alerts into a dictionary by their rule name, with
-        an array of alerts as the value.
+        if not 'default' in alert:
+            logging.info('malformed alert: %s', alert)


logging.warn

austinbyers · 2017-04-07T22:05:25Z

stream_alert/alert_processor/main.py

-            {
-                'default': [alert]
-            }
+    Args:


The args docstring (alerts) does not match the actual arguments (message, context)

austinbyers · 2017-04-07T22:06:46Z

stream_alert/alert_processor/main.py

+        try:
+            output_dispatcher.dispatch(descriptor, rule_name, alert)
+        except BaseException as err:
+            LOGGER.error('an error occurred while sending alert to %s: %s',


LOGGER.exception('an error occurred while sending alert to %s', service)
The exception context will be logged automatically

austinbyers · 2017-04-07T22:43:17Z

stream_alert_cli/outputs.py

+    if service in config and props['descriptor'].value in config[service]:
+        LOGGER_CLI.error('this descriptor is already configured for %s. '
+                         'please select a new and unique descriptor', service)
+        return


return False?

austinbyers · 2017-04-07T22:45:33Z

stream_alert_cli/package.py

@@ -19,9 +19,9 @@
 import base64
 import hashlib
 import os
+import pip


I believe pip is a non-standard package (doesn't come with Python), so it would actually go after boto3. Not sure though

austinbyers · 2017-04-07T22:46:49Z

stream_alert_cli/runner.py

+            return user_input(requested_info, mask)
+    else:
+        while not response:
+            response = getpass(prompt=prompt)


++ for hidden credential input 🔑

austinbyers · 2017-04-07T22:48:20Z

terraform/main.tf

- //   export AWS_SECRET_ACCESS_KEY="secret-key"
- //   export AWS_DEFAULT_REGION="region"
+//   export AWS_SECRET_ACCESS_KEY="secret-key"
+//   export AWS_DEFAULT_REGION="region"


Align the exports. Also, why not put the region into the provider block? Then you could make it a terraform variable. The only reason the access key isn't a variable is to discourage people from saving it on disk

austinbyers · 2017-04-07T22:49:49Z

terraform/modules/tf_stream_alert/iam.tf

+    {
+      "Action": [
+        "s3:GetObject",
+        "s3:ListBucket"


I don't think you actually need s3:ListBucket. Unless you're literally paginating over all the items in the bucket, GetObject should suffice

…n of new services and configuring integrations from the cli

…ode in the main handler

…tent

… the lambda function itself

…thod implementation enforcement

… functions

[cli] restricting colon characters along with spaces in user input for service descriptors [cli] migrating some code from the runner to the new outputs file for the cli [rules][testing] updating rules to conform to new outputs configuration style. also updating tests to go with this change [lambda][alert] remove encrypted_credentials folder [tf] fix bug in streamalert.secrets iam policy [lambda][rule] send a single alert to SNS without wrapping in a list [lambda] bug fixes: make it work * load the outputs.json config * package in the conf directory * load the conf * read s3 buckets from the conf * fix bugs in request helper * more [lambda][output] masking slack url during input and restricting the use of colons (:) in unmasked input [lambda][output][cli] fixing various nits

…the service

jacknagz suggested changes Apr 6, 2017

View reviewed changes

austinbyers reviewed Apr 7, 2017

View reviewed changes

ryandeivert force-pushed the ryandeivert-modular-outputs-refactor branch from ce59f49 to 9c913d2 Compare April 10, 2017 21:58

ryandeivert added 8 commits April 10, 2017 15:28

Fixing typos

7097047

[lambda][output][cli] refactor of outputs to allow for easier additio…

38d326b

…n of new services and configuring integrations from the cli

[cli] updating how prompting for user input occurs in the cli

a820ccd

[lambda][output] removing an unnecessary class and simplifying some c…

5f6092f

…ode in the main handler

[lambda][output] some updates to naming conventions to be more consis…

652a784

…tent

[lambda][output][cli] de-coupling the configuration of an output from…

cd4972d

… the lambda function itself

[lambda][output][cli] using abstract base class to handle subclass me…

7e36f89

…thod implementation enforcement

[lambda][output][cli] updating some docstrings and restructuring some…

f95a0b8

… functions

ryandeivert force-pushed the ryandeivert-modular-outputs-refactor branch from ac587cc to cacfd95 Compare April 10, 2017 22:32

ryandeivert force-pushed the ryandeivert-modular-outputs-refactor branch from cacfd95 to 3afa054 Compare April 10, 2017 22:35

[lambda][output][cli] credential storage on s3 will be prefixed with …

cf75786

…the service

jacknagz approved these changes Apr 11, 2017

View reviewed changes

ryandeivert merged commit 3f02f7a into master Apr 11, 2017

ryandeivert deleted the ryandeivert-modular-outputs-refactor branch April 11, 2017 22:32

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[lambda][output][cli] modular outputs refactor #97

[lambda][output][cli] modular outputs refactor #97

ryandeivert commented Apr 6, 2017 •

edited

Loading

jacknagz left a comment

jacknagz Apr 6, 2017

jacknagz Apr 6, 2017

ryandeivert Apr 6, 2017

jacknagz Apr 6, 2017

jacknagz Apr 6, 2017

jacknagz Apr 6, 2017

ryandeivert Apr 6, 2017

jacknagz Apr 6, 2017

jacknagz Apr 6, 2017

jacknagz Apr 6, 2017

jacknagz Apr 6, 2017

jacknagz Apr 6, 2017

jacknagz Apr 6, 2017

austinbyers left a comment

austinbyers Apr 7, 2017

ryandeivert Apr 7, 2017

austinbyers Apr 7, 2017

austinbyers Apr 7, 2017

austinbyers Apr 7, 2017

austinbyers Apr 7, 2017

austinbyers Apr 7, 2017

austinbyers Apr 7, 2017

austinbyers Apr 7, 2017

austinbyers Apr 7, 2017

austinbyers Apr 7, 2017



		class OutputBase(object):
		"""StreamOutputBase is the base class to handle routing alters to outputs

[lambda][output][cli] modular outputs refactor #97

[lambda][output][cli] modular outputs refactor #97

Conversation

ryandeivert commented Apr 6, 2017 • edited Loading

jacknagz left a 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

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

austinbyers left a 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ryandeivert commented Apr 6, 2017 •

edited

Loading