Merge pull request #1167 from aws/cloudtrail-regional-policy

Use CloudTrail's regionalized policy templates

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 <https://github.com/aws/aws-cli/pull/1167>`__)
+
 1.7.12
 ======
 

awscli/customizations/cloudtrail.py

@@ -20,8 +20,12 @@
 
 
 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'
+
+
+class CloudTrailError(Exception):
+    pass
 
 
 def initialize(cli):
@@ -100,6 +104,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:
@@ -184,6 +190,17 @@ 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)
+            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):
         """
         Creates a new S3 bucket with an appropriate policy to let CloudTrail
@@ -204,9 +221,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',
-                                     key=S3_POLICY_TEMPLATE)
-            policy = data['Body'].read().decode('utf-8')
+            policy = self._get_policy(S3_POLICY_TEMPLATE)
 
         policy = policy.replace('<BucketName>', bucket)\
                        .replace('<CustomerAccountID>', account_id)
@@ -233,10 +248,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,9 +296,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',
-                                     key=SNS_POLICY_TEMPLATE)
-            policy = data['Body'].read().decode('utf-8')
+            policy = self._get_policy(SNS_POLICY_TEMPLATE)
 
         policy = policy.replace('<Region>', region)\
                        .replace('<SNSTopicOwnerAccountId>', account_id)\

tests/unit/customizations/test_cloudtrail.py

@@ -16,9 +16,10 @@
 import os
 from awscli.compat import six
 
-from awscli.customizations.cloudtrail import CloudTrailSubscribe
+from awscli.customizations.cloudtrail import CloudTrailError, \
+                                             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 +29,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 +95,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 +117,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')
 
@@ -144,15 +158,33 @@ 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('Foo!'))
+
+        with self.assertRaises(CloudTrailError) as cm:
+            self.subscribe.setup_new_bucket('test', 'logs')
+
+        # 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 = {
+            'Body': Mock()
+        }
+        response['Body'].read.side_effect = Exception('Error!')
+        self.subscribe.s3.GetObject.return_value = response
+
+        with self.assertRaises(CloudTrailError):
+            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')
 
-        self.subscribe.s3.GetObject = orig
-
     def test_sns_create(self):
         s3 = self.subscribe.s3
         sns = self.subscribe.sns
@@ -166,6 +198,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')