Merge pull request #1216 from aws/cloudtrail-policy

Fix CloudTrail handling of custom policy URLs.

awscli/customizations/cloudtrail.py

@@ -15,7 +15,6 @@
 import sys
 
 from awscli.customizations.commands import BasicCommand
-from botocore.vendored import requests
 from botocore.exceptions import ClientError
 
 
@@ -70,9 +69,9 @@ class CloudTrailSubscribe(BasicCommand):
         {'name': 'include-global-service-events',
          'help_text': 'Whether to include global service events'},
         {'name': 's3-custom-policy',
-         'help_text': 'Optional URL to a custom S3 policy template'},
+         'help_text': 'Custom S3 policy template or URL'},
         {'name': 'sns-custom-policy',
-         'help_text': 'Optional URL to a custom SNS policy template'}
+         'help_text': 'Custom SNS policy template or URL'}
     ]
 
     UPDATE = False
@@ -202,7 +201,7 @@ def _get_policy(self, key_name):
                 '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):
+    def setup_new_bucket(self, bucket, prefix, custom_policy=None):
         """
         Creates a new S3 bucket with an appropriate policy to let CloudTrail
         write to the prefix path.
@@ -219,8 +218,8 @@ def setup_new_bucket(self, bucket, prefix, policy_url=None):
             prefix += '/'
 
         # Fetch policy data from S3 or a custom URL
-        if policy_url:
-            policy = requests.get(policy_url).text
+        if custom_policy is not None:
+            policy = custom_policy
         else:
             policy = self._get_policy(S3_POLICY_TEMPLATE)
 
@@ -261,7 +260,7 @@ def setup_new_bucket(self, bucket, prefix, policy_url=None):
 
         return data
 
-    def setup_new_topic(self, topic, policy_url=None):
+    def setup_new_topic(self, topic, custom_policy=None):
         """
         Creates a new SNS topic with an appropriate policy to let CloudTrail
         post messages to the topic.
@@ -290,8 +289,8 @@ def setup_new_topic(self, topic, policy_url=None):
 
         # Get the SNS topic policy information to allow CloudTrail
         # write-access.
-        if policy_url:
-            policy = requests.get(policy_url).text
+        if custom_policy is not None:
+            policy = custom_policy
         else:
             policy = self._get_policy(SNS_POLICY_TEMPLATE)
 

tests/unit/customizations/test_cloudtrail.py

@@ -12,14 +12,15 @@
 # language governing permissions and limitations under the License.
 import json
 
-from mock import ANY, Mock, call
+from mock import ANY, Mock, call, patch
 from botocore.client import ClientError
+from botocore.session import Session
 
 from tests.unit.test_clidriver import FakeSession
 from awscli.compat import six
 from awscli.customizations import cloudtrail
 from awscli.testutils import BaseAWSCommandParamsTest
-from awscli.testutils import unittest
+from awscli.testutils import unittest, temporary_file
 
 
 class TestCloudTrailPlumbing(unittest.TestCase):
@@ -46,6 +47,32 @@ def test_create_subscription_has_zero_rc(self):
         # sure it says log delivery is happening.
         self.assertIn('Logs will be delivered to foo', stdout)
 
+    @patch.object(Session, 'create_client')
+    def test_policy_from_paramfile(self, create_client_mock):
+        client = Mock()
+        # S3 mock calls
+        client.get_user.return_value = {'User': {'Arn': ':::::'}}
+        client.head_bucket.side_effect = ClientError(
+            {'Error': {'Code': 404, 'Message': ''}}, 'HeadBucket')
+        # CloudTrail mock call
+        client.describe_trails.return_value = {}
+        create_client_mock.return_value = client
+
+        policy = '{"Statement": []}'
+
+        with temporary_file('w') as f:
+            f.write(policy)
+            f.flush()
+            command = (
+                'cloudtrail create-subscription --s3-new-bucket foo '
+                '--name bar --s3-custom-policy file://{0}'.format(f.name))
+            self.run_cmd(command, expected_rc=0)
+
+        # Ensure that the *contents* of the file are sent as the policy
+        # parameter to S3.
+        client.put_bucket_policy.assert_called_with(
+            Bucket='foo', Policy=policy)
+
 
 class TestCloudTrailCommand(unittest.TestCase):
     def setUp(self):
@@ -162,6 +189,16 @@ def test_s3_create_already_exists(self):
         with self.assertRaises(Exception):
             self.subscribe.setup_new_bucket('test2', 'logs')
 
+    def test_s3_custom_policy(self):
+        s3 = self.subscribe.s3
+        s3.head_bucket.side_effect = ClientError(
+            {'Error': {'Code': '404', 'Message': ''}}, 'HeadBucket')
+
+        self.subscribe.setup_new_bucket('test', 'logs', custom_policy='{}')
+
+        s3.get_object.assert_not_called()
+        s3.put_bucket_policy.assert_called_with(Bucket='test', Policy='{}')
+
     def test_s3_create_set_policy_fail(self):
         s3 = self.subscribe.s3
         orig = s3.put_bucket_policy
@@ -224,6 +261,23 @@ def test_sns_uses_regionalized_policy(self):
         s3.get_object.assert_called_with(
             Bucket='awscloudtrail-policy-us-east-1', Key=ANY)
 
+    def test_sns_custom_policy(self):
+        s3 = self.subscribe.s3
+        sns = self.subscribe.sns
+        sns.get_topic_attributes.return_value = {
+            'Attributes': {
+                'Policy': '{"Statement": []}'
+            }
+        }
+
+        policy = '{"Statement": []}'
+
+        self.subscribe.setup_new_topic('test', custom_policy=policy)
+
+        s3.get_object.assert_not_called()
+        sns.set_topic_attributes.assert_called_with(
+          TopicArn=ANY, AttributeName='Policy', AttributeValue=policy)
+
     def test_sns_create_already_exists(self):
         with self.assertRaises(Exception):
             self.subscribe.setup_new_topic('test2')