[apps] Aliyun ActionTrail app #792

GarretReece · 2018-07-25T17:00:23Z

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

Background

SteamAlert doesn't have an app that collects ActionTrail events from Aliyun

Changes

Adds an app that collects ActionTrail events from Aliyun
Schema definition for the new log type is included
Unit tests for the app as well as schema validation tests are included
Documentation for authorizing the app is included in the README in the apps directory as well as in a section of the app-configuration.rst in the docs directory

Testing

Barebones StreamAlert session created, the Aliyun app configured and deployed, and verifying that events are listed as captured in the log

…yment

coveralls · 2018-07-25T17:05:35Z

Coverage increased (+0.03%) to 97.418% when pulling 32a1757 on trailofbits:garret/actiontrail_app into fae89fb on airbnb:master.

ryandeivert

this is nice work!! added a few quick first round comments :)

ryandeivert · 2018-07-25T21:39:38Z

app_integrations/apps/aliyun.py

+
+from aliyunsdkcore.client import AcsClient
+from aliyunsdkcore.acs_exception.exceptions import ClientException
+from aliyunsdkcore.acs_exception.exceptions import ServerException


can you group this exception with the import above pls

ryandeivert · 2018-07-25T21:39:38Z

app_integrations/apps/aliyun.py

+
+from app_integrations import LOGGER
+from app_integrations.apps.app_base import StreamAlertApp, AppIntegration
+


add an extra new line here please (2 lines between imports and class declarations)

ryandeivert · 2018-07-25T21:39:38Z

app_integrations/apps/aliyun.py

+
+    def _gather_logs(self):
+        auth = self._config.auth
+        client = AcsClient(auth['access_key_id'], auth['access_key_secret'], auth['region_id'])


the _gather_logs method could be called a bunch of times in an execution - therefore, would it make sense to have the AcsClient created upon __init__ and cached as an instance property?

ryandeivert · 2018-07-25T21:39:38Z

app_integrations/apps/aliyun.py

+
+            return json_response['Events']
+
+        except ServerException as e:


These two except blocks could be consolidated into one with:

except (ServerException, ClientException) as err: LOGGER.exception("%s exception occurred", err.get_error_type()) return False

also, there's no need to include the actual content of the exception (with str(e)) when using LOGGER.exception - this call handles all of that for you.

ryandeivert · 2018-07-25T21:39:38Z

app_integrations/apps/aliyun.py

+        def region_validator(region):
+            """Region names pulled from https://www.alibabacloud.com/help/doc-detail/40654.htm"""
+
+            if region in ["cn-qingdao", "cn-beijing", "cn-zhangjiakou", "cn-huhehaote",


please use single (') quotes on string literals and not double (") quotes

Small request, can we use set instead of list, it is not big a deal though.

ryandeivert · 2018-07-25T21:39:38Z

app_integrations/apps/aliyun.py

+            'access_key_id': {
+                'description': ('The access key id generated for a RAM user. This '
+                                'should be a string of alphanumeric characters.'),
+                'format': re.compile(r'.*')


are there no restrictions similar to AWS for their access and secret keys? ie - AWS access keys are 20 alphanumeric all-caps characters and must start with A, etc

I can't find any documentation anywhere regarding the format. The values I have are a mix of digits and upper and lower case letters, with the id 16 characters in length and the secret 30 characters in length. I'm perfectly willing to put those in as restrictions, but I don't have any reference for that being the supported format.

ryandeivert · 2018-07-25T21:39:38Z

app_integrations/apps/aliyun.py

+            'region_id': {
+                'description': ('The region for the Aliyun API. This should be '
+                                'a string like \'ap-northeast-1\'.'),
+                'format': region_validator


++ this is great

ryandeivert · 2018-07-25T21:39:38Z

app_integrations/apps/aliyun.py

+        Returns:
+            int: Number of seconds the polling function should sleep for
+        """
+        return 0


I think if this is 0 it can be left off completely. I do like the context you've added in the docstring, though, so I'm not sure if I feel strongly about removing the method

ryandeivert · 2018-07-25T21:39:38Z

stream_alert_cli/manage_lambda/package.py

@@ -203,6 +203,8 @@ class AppIntegrationPackage(LambdaPackage):
    package_name = 'stream_alert_app'
    precompiled_libs = {'boxsdk[jwt]==2.0.0a11'}
    third_party_libs = {
+        'aliyun-python-sdk-core==2.8.5',


this is great - thank you for pinning the version!

ryandeivert · 2018-07-25T21:39:38Z

app_integrations/apps/aliyun.py

+    def service(cls):
+        return 'aliyun'
+
+


Looks like there's an extra new line here

chunyong-lin

Hey @GarretReece thanks for working on this App. It looks great too me. I added some comments.

chunyong-lin · 2018-07-25T23:38:25Z

app_integrations/apps/aliyun.py

+        https://www.alibabacloud.com/help/doc-detail/28849.htm
+    """
+
+    _MAX_RESULTS = 50


Any reason we don't pull 100 results each time?

The documentation lists the range for the MaxResult parameter as 0-50; I'll admit I haven't tested with higher values to see if it worked anyhow.

Can I have one more testing request? Can you add a test case when NextToken is available in your testing environment? (I am not talking about unit test)

A while ago, I have a simple script to retrieve ActionTrail events, and set max results per request to 2. I encountered the issue that NextToken doesn't take effect, and the 2nd request returned as same events as 1st request recieved.
Can you verify this in the testing environment? Thanks!

chunyong-lin · 2018-07-25T23:38:25Z

app_integrations/apps/aliyun.py

+
+    @classmethod
+    def _type(cls):
+        return 'events'


actiontrail would be more descriptive.

chunyong-lin · 2018-07-25T23:38:25Z

requirements.txt

@@ -1,3 +1,5 @@
+aliyun-python-sdk-core==2.8.5
+aliyun-python-sdk-actiontrail==2.0.0


Can you add these two packages to requirements-top-level.txt as well?

added them in!

chunyong-lin · 2018-07-25T23:38:25Z

app_integrations/apps/aliyun.py

+        def region_validator(region):
+            """Region names pulled from https://www.alibabacloud.com/help/doc-detail/40654.htm"""
+
+            if region in ["cn-qingdao", "cn-beijing", "cn-zhangjiakou", "cn-huhehaote",


Small request, can we use set instead of list, it is not big a deal though.

chunyong-lin · 2018-07-25T23:38:25Z

app_integrations/apps/aliyun.py

+        auth = self._config.auth
+        client = AcsClient(auth['access_key_id'], auth['access_key_secret'], auth['region_id'])
+
+        request = LookupEventsRequest.LookupEventsRequest()


I think we also can cache request to __init__.

chunyong-lin · 2018-07-25T23:38:25Z

conf/logs.json

@@ -1666,5 +1666,40 @@
        "reason"
      ]
    }
+  },
+  "aliyun:events": {


Remind to update schema name if you agree with me renaming _type to actiontrail.

chunyong-lin

LGTM! 🎉

GarretReece added 4 commits Jul 23, 2018

Initial commit. Unit tests in place and passing with full coverage.

863c6f1

Schema and schema validation checks in place

d2b4388

add aliyun libraries to the third-party library list for lambda deplo…

0d17d11

…yment

Documentation, region checking, updated unit tests

Loading status checks…

69e5636

ryandeivert reviewed Jul 25, 2018

View changes

chunyong-lin reviewed Jul 25, 2018

View changes

GarretReece added 3 commits Jul 26, 2018

Addressing PR comments

456281f

bugfix, unit test to check, and final PR comments addressed

Loading status checks…

9956d78

fix for failing unit test, whoops

Loading status checks…

8d3b5b2

chunyong-lin approved these changes Jul 27, 2018

View changes

Merge branch 'master' into garret/actiontrail_app

Verified

This commit was created on GitHub.com and signed with a verified signature using GitHub’s key.

GPG key ID: 4AEE18F83AFDEB23 Learn about signing commits

Loading status checks…

32a1757

chunyong-lin merged commit 18735de into airbnb:master Jul 27, 2018
2 checks passed

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

coverage/coveralls Coverage increased (+0.03%) to 97.418%
Details

dguido deleted the trailofbits:garret/actiontrail_app branch Jul 30, 2018

airbnb / streamalert

[apps] Aliyun ActionTrail app #792

[apps] Aliyun ActionTrail app #792

GarretReece commented Jul 25, 2018

coveralls commented Jul 25, 2018 •

edited

ryandeivert left a comment

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

chunyong-lin left a comment •

edited

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

chunyong-lin left a comment


		from app_integrations import LOGGER
		from app_integrations.apps.app_base import StreamAlertApp, AppIntegration

		@@ -1,3 +1,5 @@
		aliyun-python-sdk-core==2.8.5
		aliyun-python-sdk-actiontrail==2.0.0

airbnb / streamalert

Join GitHub today

[apps] Aliyun ActionTrail app #792

[apps] Aliyun ActionTrail app #792

Conversation

GarretReece commented Jul 25, 2018

Background

Changes

Testing

coveralls commented Jul 25, 2018 • edited

ryandeivert left a comment

This comment has been minimized.

ryandeivert Jul 25, 2018 Contributor

This comment has been minimized.

ryandeivert Jul 25, 2018 Contributor

This comment has been minimized.

ryandeivert Jul 25, 2018 Contributor

This comment has been minimized.

ryandeivert Jul 25, 2018 Contributor

This comment has been minimized.

ryandeivert Jul 25, 2018 Contributor

This comment has been minimized.

chunyong-lin Jul 25, 2018 Contributor

This comment has been minimized.

ryandeivert Jul 25, 2018 Contributor

This comment has been minimized.

GarretReece Jul 26, 2018 Author Contributor

This comment has been minimized.

ryandeivert Jul 25, 2018 Contributor

This comment has been minimized.

ryandeivert Jul 25, 2018 Contributor

This comment has been minimized.

ryandeivert Jul 25, 2018 Contributor

This comment has been minimized.

ryandeivert Jul 25, 2018 Contributor

chunyong-lin left a comment • edited

This comment has been minimized.

chunyong-lin Jul 25, 2018 Contributor

This comment has been minimized.

GarretReece Jul 26, 2018 Author Contributor

This comment has been minimized.

chunyong-lin Jul 26, 2018 • edited Contributor

This comment has been minimized.

chunyong-lin Jul 25, 2018 Contributor

This comment has been minimized.

chunyong-lin Jul 25, 2018 Contributor

This comment has been minimized.

GarretReece Jul 26, 2018 Author Contributor

This comment has been minimized.

chunyong-lin Jul 25, 2018 Contributor

This comment has been minimized.

chunyong-lin Jul 25, 2018 Contributor

This comment has been minimized.

chunyong-lin Jul 25, 2018 Contributor

chunyong-lin left a comment

Essential cookies

Always active

Analytics cookies

coveralls commented Jul 25, 2018 •

edited

ryandeivert Jul 25, 2018
Contributor

ryandeivert Jul 25, 2018
Contributor

ryandeivert Jul 25, 2018
Contributor

ryandeivert Jul 25, 2018
Contributor

ryandeivert Jul 25, 2018
Contributor

chunyong-lin Jul 25, 2018
Contributor

ryandeivert Jul 25, 2018
Contributor

GarretReece Jul 26, 2018
Author Contributor

ryandeivert Jul 25, 2018
Contributor

ryandeivert Jul 25, 2018
Contributor

ryandeivert Jul 25, 2018
Contributor

ryandeivert Jul 25, 2018
Contributor

chunyong-lin left a comment •

edited

chunyong-lin Jul 25, 2018
Contributor

GarretReece Jul 26, 2018
Author Contributor

chunyong-lin Jul 26, 2018 •

edited

Contributor

chunyong-lin Jul 25, 2018
Contributor

chunyong-lin Jul 25, 2018
Contributor

GarretReece Jul 26, 2018
Author Contributor

chunyong-lin Jul 25, 2018
Contributor

chunyong-lin Jul 25, 2018
Contributor

chunyong-lin Jul 25, 2018
Contributor