From 51511bbd9e0cc459e4817b8602b08f93decf3726 Mon Sep 17 00:00:00 2001 From: "Daniel G. Taylor" Date: Thu, 19 Feb 2015 13:32:51 -0800 Subject: [PATCH 1/5] Use CloudTrail's regionalized policy templates This change switches away from always using an S3 bucket in `us-west-2` to fetch CloudTrail policy information for S3 and SNS when setting up new buckets/topics and instead uses the `awscloudtrail-policy-REGION` regionalized buckets. It also updates the policy version to use the most recent release. Tests are updated and a couple new ones make sure the regionalized buckets are used. --- awscli/customizations/cloudtrail.py | 21 +++++++++------- tests/unit/customizations/test_cloudtrail.py | 25 ++++++++++++++++++-- 2 files changed, 35 insertions(+), 11 deletions(-) diff --git a/awscli/customizations/cloudtrail.py b/awscli/customizations/cloudtrail.py index f99489301e2f..307410054e56 100644 --- a/awscli/customizations/cloudtrail.py +++ b/awscli/customizations/cloudtrail.py @@ -20,8 +20,8 @@ LOG = logging.getLogger(__name__) -S3_POLICY_TEMPLATE = 'policy/S3/AWSCloudTrail-S3BucketPolicy-2013-11-01.json' -SNS_POLICY_TEMPLATE = 'policy/SNS/AWSCloudTrail-SnsTopicPolicy-2013-11-01.json' +S3_POLICY_TEMPLATE = 'policy/S3/AWSCloudTrail-S3BucketPolicy-2014-12-17.json' +SNS_POLICY_TEMPLATE = 'policy/SNS/AWSCloudTrail-SnsTopicPolicy-2014-12-17.json' def initialize(cli): @@ -100,6 +100,8 @@ def setup_services(self, args, parsed_globals): self.sns = Service('sns', endpoint_args=endpoint_args, session=self._session) + self.region_name = self.s3.endpoint.region_name + # If the endpoint is specified, it is designated for the cloudtrail # service. Not all of the other services will use it. if 'endpoint_url' in parsed_globals: @@ -204,8 +206,9 @@ def setup_new_bucket(self, bucket, prefix, policy_url=None): if policy_url: policy = requests.get(policy_url).text else: - data = self.s3.GetObject(bucket='awscloudtrail', - key=S3_POLICY_TEMPLATE) + data = self.s3.GetObject( + bucket='awscloudtrail-policy-' + self.region_name, + key=S3_POLICY_TEMPLATE) policy = data['Body'].read().decode('utf-8') policy = policy.replace('', bucket)\ @@ -233,10 +236,9 @@ def setup_new_bucket(self, bucket, prefix, policy_url=None): # If we are not using the us-east-1 region, then we must set # a location constraint on the new bucket. - region_name = self.s3.endpoint.region_name params = {'bucket': bucket} - if region_name != 'us-east-1': - bucket_config = {'LocationConstraint': region_name} + if self.region_name != 'us-east-1': + bucket_config = {'LocationConstraint': self.region_name} params['create_bucket_configuration'] = bucket_config data = self.s3.CreateBucket(**params) @@ -282,8 +284,9 @@ def setup_new_topic(self, topic, policy_url=None): if policy_url: policy = requests.get(policy_url).text else: - data = self.s3.GetObject(bucket='awscloudtrail', - key=SNS_POLICY_TEMPLATE) + data = self.s3.GetObject( + bucket='awscloudtrail-policy-' + self.region_name, + key=SNS_POLICY_TEMPLATE) policy = data['Body'].read().decode('utf-8') policy = policy.replace('', region)\ diff --git a/tests/unit/customizations/test_cloudtrail.py b/tests/unit/customizations/test_cloudtrail.py index eb3a31600172..de29cdfdc866 100644 --- a/tests/unit/customizations/test_cloudtrail.py +++ b/tests/unit/customizations/test_cloudtrail.py @@ -18,7 +18,7 @@ from awscli.customizations.cloudtrail import CloudTrailSubscribe from awscli.customizations.service import Service -from mock import Mock, patch +from mock import ANY, Mock, patch from awscli.testutils import BaseAWSCommandParamsTest from tests.unit.test_clidriver import FakeSession from awscli.testutils import unittest @@ -28,6 +28,7 @@ class TestCloudTrail(unittest.TestCase): def setUp(self): self.session = FakeSession({'config_file': 'myconfigfile'}) self.subscribe = CloudTrailSubscribe(self.session) + self.subscribe.region_name = 'us-east-1' self.subscribe.iam = Mock() self.subscribe.iam.GetUser = Mock( @@ -93,6 +94,10 @@ def test_setup_services(self): ref_args['endpoint_url'] = parsed_globals.endpoint_url self.assertEqual(endpoint_call_args[3][1], ref_args) + # Ensure a region name was set on the command class + self.assertEqual(self.subscribe.region_name, + fake_service.get_endpoint.return_value.region_name) + def test_s3_create(self): iam = self.subscribe.iam s3 = self.subscribe.s3 @@ -111,11 +116,19 @@ def test_s3_create(self): args, kwargs = s3.CreateBucket.call_args self.assertNotIn('create_bucket_configuration', kwargs) + def test_s3_uses_regionalized_policy(self): + s3 = self.subscribe.s3 + + self.subscribe.setup_new_bucket('test', 'logs') + + s3.GetObject.assert_called_with( + bucket='awscloudtrail-policy-us-east-1', key=ANY) + def test_s3_create_non_us_east_1(self): # Because this is outside of us-east-1, it should create # a bucket configuration with a location constraint. s3 = self.subscribe.s3 - s3.endpoint.region_name = 'us-west-2' + self.subscribe.region_name = 'us-west-2' self.subscribe.setup_new_bucket('test', 'logs') @@ -166,6 +179,14 @@ def test_sns_create(self): sns.DeleteTopic.assert_not_called() + def test_sns_uses_regionalized_policy(self): + s3 = self.subscribe.s3 + + self.subscribe.setup_new_topic('test') + + s3.GetObject.assert_called_with( + bucket='awscloudtrail-policy-us-east-1', key=ANY) + def test_sns_create_already_exists(self): with self.assertRaises(Exception): self.subscribe.setup_new_topic('test2') From 4528e5ad4d50f453848c82cbdf4040b2629f28e8 Mon Sep 17 00:00:00 2001 From: "Daniel G. Taylor" Date: Mon, 23 Feb 2015 13:56:28 -0800 Subject: [PATCH 2/5] Minor refactor, log extra info on error --- awscli/customizations/cloudtrail.py | 22 ++++++++++++++-------- 1 file changed, 14 insertions(+), 8 deletions(-) diff --git a/awscli/customizations/cloudtrail.py b/awscli/customizations/cloudtrail.py index 307410054e56..ecc189506903 100644 --- a/awscli/customizations/cloudtrail.py +++ b/awscli/customizations/cloudtrail.py @@ -186,6 +186,18 @@ def _call(self, options, parsed_globals): 'Logs will be delivered to {bucket}:{prefix}\n'.format( bucket=bucket, prefix=options.s3_prefix or '')) + def _get_policy(self, key_name): + try: + data = self.s3.GetObject( + bucket='awscloudtrail-policy-' + self.region_name, + key=key_name) + except Exception: + LOG.error('Unable to get regional policy template for' + ' region %s: %s', self.region_name, key_name) + raise + + return data['Body'].read().decode('utf-8') + def setup_new_bucket(self, bucket, prefix, policy_url=None): """ Creates a new S3 bucket with an appropriate policy to let CloudTrail @@ -206,10 +218,7 @@ def setup_new_bucket(self, bucket, prefix, policy_url=None): if policy_url: policy = requests.get(policy_url).text else: - data = self.s3.GetObject( - bucket='awscloudtrail-policy-' + self.region_name, - key=S3_POLICY_TEMPLATE) - policy = data['Body'].read().decode('utf-8') + policy = self._get_policy(S3_POLICY_TEMPLATE) policy = policy.replace('', bucket)\ .replace('', account_id) @@ -284,10 +293,7 @@ def setup_new_topic(self, topic, policy_url=None): if policy_url: policy = requests.get(policy_url).text else: - data = self.s3.GetObject( - bucket='awscloudtrail-policy-' + self.region_name, - key=SNS_POLICY_TEMPLATE) - policy = data['Body'].read().decode('utf-8') + policy = self._get_policy(SNS_POLICY_TEMPLATE) policy = policy.replace('', region)\ .replace('', account_id)\ From 2bd4c56bb3d7966ddf5d92e1fa44eb43a99d0ee7 Mon Sep 17 00:00:00 2001 From: "Daniel G. Taylor" Date: Mon, 23 Feb 2015 14:44:06 -0800 Subject: [PATCH 3/5] Add some more tests --- awscli/customizations/cloudtrail.py | 3 +- tests/unit/customizations/test_cloudtrail.py | 34 ++++++++++++++++++-- 2 files changed, 33 insertions(+), 4 deletions(-) diff --git a/awscli/customizations/cloudtrail.py b/awscli/customizations/cloudtrail.py index ecc189506903..1b56db8cbd31 100644 --- a/awscli/customizations/cloudtrail.py +++ b/awscli/customizations/cloudtrail.py @@ -191,12 +191,13 @@ def _get_policy(self, key_name): data = self.s3.GetObject( bucket='awscloudtrail-policy-' + self.region_name, key=key_name) + policy = data['Body'].read().decode('utf-8') except Exception: LOG.error('Unable to get regional policy template for' ' region %s: %s', self.region_name, key_name) raise - return data['Body'].read().decode('utf-8') + return policy def setup_new_bucket(self, bucket, prefix, policy_url=None): """ diff --git a/tests/unit/customizations/test_cloudtrail.py b/tests/unit/customizations/test_cloudtrail.py index de29cdfdc866..64ace77bb14c 100644 --- a/tests/unit/customizations/test_cloudtrail.py +++ b/tests/unit/customizations/test_cloudtrail.py @@ -16,6 +16,7 @@ import os from awscli.compat import six +from awscli.customizations import cloudtrail from awscli.customizations.cloudtrail import CloudTrailSubscribe from awscli.customizations.service import Service from mock import ANY, Mock, patch @@ -157,14 +158,41 @@ def test_s3_create_set_policy_fail(self): s3.PutBucketPolicy = orig - def test_get_policy_fail(self): - orig = self.subscribe.s3.GetObject + def test_s3_get_policy_fail(self): self.subscribe.s3.GetObject = Mock(side_effect=Exception('Error!')) with self.assertRaises(Exception): self.subscribe.setup_new_bucket('test', 'logs') - self.subscribe.s3.GetObject = orig + def test_s3_get_policy_logs_messages(self): + cloudtrail.LOG = Mock() + self.subscribe.s3.GetObject = Mock(side_effect=Exception('Error!')) + + try: + self.subscribe.setup_new_bucket('test', 'logs') + except: + pass + + self.assertIn( + 'Unable to get regional policy template for region', + cloudtrail.LOG.error.call_args[0][0]) + self.assertEqual('us-east-1', cloudtrail.LOG.error.call_args[0][1]) + + def test_get_policy_read_timeout(self): + response = { + 'Body': Mock() + } + response['Body'].read.side_effect = Exception('Error!') + self.subscribe.s3.GetObject.return_value = response + + with self.assertRaises(Exception): + self.subscribe.setup_new_bucket('test', 'logs') + + def test_sns_get_policy_fail(self): + self.subscribe.s3.GetObject = Mock(side_effect=Exception('Error!')) + + with self.assertRaises(Exception): + self.subscribe.setup_new_bucket('test', 'logs') def test_sns_create(self): s3 = self.subscribe.s3 From 77a5c158d16adcd1991dc2c5ed4f802e76167346 Mon Sep 17 00:00:00 2001 From: "Daniel G. Taylor" Date: Mon, 23 Feb 2015 16:01:11 -0800 Subject: [PATCH 4/5] More code review feedback --- awscli/customizations/cloudtrail.py | 16 +++++++----- tests/unit/customizations/test_cloudtrail.py | 27 +++++++------------- 2 files changed, 18 insertions(+), 25 deletions(-) diff --git a/awscli/customizations/cloudtrail.py b/awscli/customizations/cloudtrail.py index 1b56db8cbd31..0bc4c1d557ce 100644 --- a/awscli/customizations/cloudtrail.py +++ b/awscli/customizations/cloudtrail.py @@ -24,6 +24,10 @@ SNS_POLICY_TEMPLATE = 'policy/SNS/AWSCloudTrail-SnsTopicPolicy-2014-12-17.json' +class CloudTrailError(Exception): + pass + + def initialize(cli): """ The entry point for CloudTrail high level commands. @@ -191,13 +195,11 @@ def _get_policy(self, key_name): data = self.s3.GetObject( bucket='awscloudtrail-policy-' + self.region_name, key=key_name) - policy = data['Body'].read().decode('utf-8') - except Exception: - LOG.error('Unable to get regional policy template for' - ' region %s: %s', self.region_name, key_name) - raise - - return policy + return data['Body'].read().decode('utf-8') + except Exception as e: + raise CloudTrailError( + 'Unable to get regional policy template for' + ' region %s: %s. Error: %s', self.region_name, key_name, e) def setup_new_bucket(self, bucket, prefix, policy_url=None): """ diff --git a/tests/unit/customizations/test_cloudtrail.py b/tests/unit/customizations/test_cloudtrail.py index 64ace77bb14c..8e74902d8b50 100644 --- a/tests/unit/customizations/test_cloudtrail.py +++ b/tests/unit/customizations/test_cloudtrail.py @@ -16,8 +16,8 @@ import os from awscli.compat import six -from awscli.customizations import cloudtrail -from awscli.customizations.cloudtrail import CloudTrailSubscribe +from awscli.customizations.cloudtrail import CloudTrailError, \ + CloudTrailSubscribe from awscli.customizations.service import Service from mock import ANY, Mock, patch from awscli.testutils import BaseAWSCommandParamsTest @@ -159,24 +159,15 @@ def test_s3_create_set_policy_fail(self): s3.PutBucketPolicy = orig def test_s3_get_policy_fail(self): - self.subscribe.s3.GetObject = Mock(side_effect=Exception('Error!')) + self.subscribe.s3.GetObject = Mock(side_effect=Exception('Foo!')) - with self.assertRaises(Exception): + with self.assertRaises(CloudTrailError) as cm: self.subscribe.setup_new_bucket('test', 'logs') - def test_s3_get_policy_logs_messages(self): - cloudtrail.LOG = Mock() - self.subscribe.s3.GetObject = Mock(side_effect=Exception('Error!')) - - try: - self.subscribe.setup_new_bucket('test', 'logs') - except: - pass - - self.assertIn( - 'Unable to get regional policy template for region', - cloudtrail.LOG.error.call_args[0][0]) - self.assertEqual('us-east-1', cloudtrail.LOG.error.call_args[0][1]) + # Exception should contain its custom message, the region + # where there is an issue, and the original exception message. + self.assertIn('us-east-1', str(cm.exception)) + self.assertIn('Foo!', str(cm.exception)) def test_get_policy_read_timeout(self): response = { @@ -185,7 +176,7 @@ def test_get_policy_read_timeout(self): response['Body'].read.side_effect = Exception('Error!') self.subscribe.s3.GetObject.return_value = response - with self.assertRaises(Exception): + with self.assertRaises(CloudTrailError): self.subscribe.setup_new_bucket('test', 'logs') def test_sns_get_policy_fail(self): From f549785884d6590a15f69aa9b2254760334a618c Mon Sep 17 00:00:00 2001 From: "Daniel G. Taylor" Date: Wed, 25 Feb 2015 10:12:28 -0800 Subject: [PATCH 5/5] Update changelog [ci skip] --- CHANGELOG.rst | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/CHANGELOG.rst b/CHANGELOG.rst index 602c84a1160c..048c3878cdb8 100644 --- a/CHANGELOG.rst +++ b/CHANGELOG.rst @@ -2,6 +2,13 @@ CHANGELOG ========= +Next Release (TBD) +================== + +* feature:``aws cloudtrail``: Add support for regionalized policy templates + for the ``create-subscription`` and ``update-subscription`` commands. + (`issue 1167 `__) + 1.7.12 ======