Separates CredentialProvider from OutputDispatcher #875

Ryxias · 2019-01-14T22:54:11Z

to: @ryandeivert
cc: @airbnb/streamalert-maintainers

Background

Implementing a new OutputDispatcher requires a lot of in-depth understanding of how the base class is implemented, and testing it has dependencies on AWS S3, AWS KMS, os, and the filesystem. To write a good test, you will need to know how to mock all three of these out which is lot of boilerplate.

Example: TestPagerDutyOutput.setup/teardown

Changes

I refactored OutputDispatcher and decoupled the credentials logic out of the base class and into a new class, OutputCredentialsProvider. This class now becomes solely responsible for providing _load_creds() functionality to the parent class.

Benefit?

Now, when writing unit tests, instead of having to mock out many different things as boilerplate, we can simply use mock to patch the OutputCredentialsProvider class to always return stub credentials.

Example: Commit showing before/after

This reduces the amount of code slightly, and makes it slightly easier to understand.

Additionally, this PR contains all changes in #878, with the new notion of Drivers. This opens up an easier path in the future to build an integration with SSM (#240).

Other (Unrelated?) Changes

I DRY'd out some code here and there, and removed some private (?) methods from OutputDispatcher that I didn't see being used in production code, in order to simplify the class's interface and its inheritance model.

Testing

I'm not quite done yet but I'm adding new unit tests which are passing. Will paste results here shortly.

tests/scripts/unit_tests.sh

stream_alert/threat_intel_downloader/__init__.py          0      0   100%
stream_alert/threat_intel_downloader/exceptions.py        4      0   100%
stream_alert/threat_intel_downloader/main.py            134      1    99%   348
-----------------------------------------------------------------------------------
TOTAL                                                  4414     94    98%
[success] 1.76% tests.unit.stream_alert_alert_merger.test_main.TestAlertMerger.test_dispatch: 0.3028s
[success] 1.45% tests.unit.streamalert.classifier.clients.test_firehose.TestFirehoseClient.test_send_batch: 0.2494s
[success] 1.42% tests.unit.stream_alert_shared.test_rule.RuleImportTest.test_path_to_module: 0.2436s
[success] 1.05% tests.unit.stream_alert_apps.test_apps.test_aliyun.TestAliyunApp.test_date_formatter: 0.1794s
[success] 0.99% tests.unit.stream_alert_alert_processor.test_outputs.test_pagerduty.TestPagerDutyIncidentOutput.test_dispatch_success_no_context: 0.1701s
[success] 0.99% tests.unit.stream_alert_shared.test_config.TestConfigLoading.test_load_invalid_file: 0.1696s
[success] 0.85% tests.unit.stream_alert_alert_processor.test_outputs.test_github.TestGithubOutput.test_dispatch_failure: 0.1454s
[success] 0.84% tests.unit.stream_alert_shared.test_rule.RuleImportTest.test_import_rules: 0.1447s
[success] 0.81% tests.unit.stream_alert_shared.test_config.TestConfigLoading.test_load_include: 0.1398s
[success] 0.81% tests.unit.stream_alert_shared.test_alert_table.TestAlertTable.test_get_alert_records: 0.1383s
----------------------------------------------------------------------
Ran 882 tests in 17.284s

OK

tests/scripts/rule_test.sh

Summary:

Total Tests: 77
Pass: 77
Fail: 0

[INFO 2019-01-14 15:48:57,377 (stream_alert_cli.runner:85)]: Completed

Ryxias · 2019-01-14T23:03:23Z

Per @ryandeivert 's comment worth keeping #240 in mind

coveralls · 2019-01-15T00:04:38Z

Coverage increased (+0.02%) to 97.131% when pulling cde8b20 on dw--output-credentials-provider into 6334ede on master.

stream_alert/alert_processor/outputs/output_base.py

ryandeivert

thank you for the work on this @Ryxias !! looking good so far, a handful of comments :)

ryandeivert · 2019-01-18T18:16:45Z

stream_alert/alert_processor/outputs/output_base.py

-        """Get the local tmp directory for caching the encrypted service credentials
+       OutputDispatcher implementations may require credentials to authenticate with an external
+       gateway. All credentials for OutputDispatchers are to be stored in a single bucket on AWS S3
+       and are encrypted with AWS KMS. When the OutputDispatchers are booted, this these encrypted


typo: this these

also 'booted' might not be the right verbiage :P

stream_alert/alert_processor/outputs/output_base.py

stream_alert/alert_processor/outputs/credentials/provider.py

ryandeivert

great work so far @Ryxias - leaving a handful of comments/questions for you

stream_alert/alert_processor/outputs/credentials/provider.py

stream_alert_cli/outputs/handler.py

stream_alert/alert_processor/outputs/credentials/provider.py

Ryxias · 2019-01-28T20:12:04Z

@ryandeivert I've added a Fixup that addresses your comments, PTAL

(I also added stream_alert.shared.helps.aws_api_client.py)

Ryxias · 2019-01-28T20:20:36Z

OH no i Messed up i put it on the wrong branch... hold on..

…tests

* Working? Draft of new driver-based credentials storage * Higher quality refactor; needs tests * Kinks ironed out with good tests this time * First maybe working try * Adds lots of tests for the Drivers * Add more tests. Not final; still need to remove deprecated methods * Rename method to reduce confusion * Remove deprecated method load_encrypted_credentials_from_s3 * Removes deprecated method get_local_credentials_temp_dir * Remove deprecated method get_formatted_output_credentials_name * Removes deprecated method kms_decrypt * Removes extraneous imports * Extract globally injected REGION so the handler can be implemented properly * Pylint` * Pylint is my nemesis * Remove extraneous method * Use default_config for boto clients * Code coverage. PR feedback. * Add missing __init__.py file causing poor code coverage * Fixes the tests to get past pylint garbage

…ials provider.

Ryxias · 2019-01-28T20:25:57Z

There we go @ryandeivert I fixed it, PTAL

ryandeivert · 2019-01-29T23:43:00Z

stream_alert_cli/outputs/helpers.py

 from botocore.exceptions import ClientError

 from stream_alert.shared.logger import get_logger
+from stream_alert.shared.helpers.aws_api_client import AwsS3, AwsKms


this import should go before the previous line (h < l)

ryandeivert · 2019-01-29T23:44:06Z

stream_alert_cli/outputs/helpers.py

-        client.put_object(Body=blob_data, Bucket=bucket, Key=key)
-
-        return True
+        return AwsS3.put_object(blob_data, bucket=bucket, key=key, region=region)


nice changes - if this function (send_creds_to_s3) is in fact not being anywhere else, you can freely remove it

… code

Ryxias · 2019-01-30T00:41:57Z

Done; I removed the deprecated code and added tests to replace the old ones that are gone

ryandeivert

🚢 👍 thank you @Ryxias !

Ryxias force-pushed the dw--output-credentials-provider branch from 9340d27 to 8180c71 Compare January 15, 2019 01:00

ryandeivert reviewed Jan 15, 2019

View reviewed changes

stream_alert/alert_processor/outputs/output_base.py Outdated Show resolved Hide resolved

Ryxias force-pushed the dw--output-credentials-provider branch from 720a2db to 5ffadd5 Compare January 16, 2019 20:54

Ryxias mentioned this pull request Jan 16, 2019

Refactors Drivers for OutputCredentialsProvider #878

Merged

ryandeivert reviewed Jan 18, 2019

View reviewed changes

Ryxias force-pushed the dw--output-credentials-provider branch from 703db22 to f241094 Compare January 19, 2019 00:10

ryandeivert reviewed Jan 23, 2019

View reviewed changes

stream_alert/alert_processor/outputs/credentials/provider.py Outdated Show resolved Hide resolved

stream_alert/alert_processor/outputs/credentials/provider.py Outdated Show resolved Hide resolved

stream_alert/alert_processor/outputs/credentials/provider.py Show resolved Hide resolved

ryandeivert reviewed Jan 23, 2019

View reviewed changes

Ryxias force-pushed the dw--output-credentials-provider branch from 96cdd52 to f104123 Compare January 28, 2019 20:24

Ryxias and others added 15 commits January 28, 2019 12:24

Initial commit

1e999a6

Now with tests

909c877

Touchups

1b3b6a0

Remove extraneous static method

9306c0b

DRYs out some code related to S3 secret bucket name

c020a99

Proof-of-concept of mocking CredentialProvider

45b05f6

Fix some pylints

4bd7000

Removes output_cred_name() to consolidate functionality. Updates all …

2a899c9

…tests

pylint

771ceee

Change account_id to a property

bb24c64

Formatting

1a0fc84

(fixup) PR Feedback

9f05d07

(fixup) Disentangles streamalert cli helper with stream_alert credent…

62c9dcc

…ials provider.

(fixup) Deprecate old methods

92da1c5

Ryxias force-pushed the dw--output-credentials-provider branch from f104123 to 92da1c5 Compare January 28, 2019 20:25

pylint

cd7a959

ryandeivert reviewed Jan 29, 2019

View reviewed changes

Removes deprecated code; adds tests for aws_api_client; DRYs out test…

cde8b20

… code

ryandeivert approved these changes Jan 30, 2019

View reviewed changes

Ryxias merged commit 44fe7d0 into master Jan 30, 2019

Ryxias deleted the dw--output-credentials-provider branch January 31, 2019 22:38

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Separates CredentialProvider from OutputDispatcher #875

Separates CredentialProvider from OutputDispatcher #875

Ryxias commented Jan 14, 2019 •

edited

Loading

Ryxias commented Jan 14, 2019 •

edited

Loading

coveralls commented Jan 15, 2019 •

edited

Loading

ryandeivert left a comment

ryandeivert Jan 18, 2019

ryandeivert left a comment

Ryxias commented Jan 28, 2019

Ryxias commented Jan 28, 2019

Ryxias commented Jan 28, 2019

ryandeivert Jan 29, 2019

ryandeivert Jan 29, 2019

Ryxias commented Jan 30, 2019

ryandeivert left a comment

Separates CredentialProvider from OutputDispatcher #875

Separates CredentialProvider from OutputDispatcher #875

Conversation

Ryxias commented Jan 14, 2019 • edited Loading

Background

Changes

Benefit?

Other (Unrelated?) Changes

Testing

Ryxias commented Jan 14, 2019 • edited Loading

coveralls commented Jan 15, 2019 • edited Loading

ryandeivert left a comment

Choose a reason for hiding this comment

ryandeivert Jan 18, 2019

Choose a reason for hiding this comment

ryandeivert left a comment

Choose a reason for hiding this comment

Ryxias commented Jan 28, 2019

Ryxias commented Jan 28, 2019

Ryxias commented Jan 28, 2019

ryandeivert Jan 29, 2019

Choose a reason for hiding this comment

ryandeivert Jan 29, 2019

Choose a reason for hiding this comment

Ryxias commented Jan 30, 2019

ryandeivert left a comment

Choose a reason for hiding this comment

Ryxias commented Jan 14, 2019 •

edited

Loading

Ryxias commented Jan 14, 2019 •

edited

Loading

coveralls commented Jan 15, 2019 •

edited

Loading