From 50b47c78b60d4a0f62b8364bc0ad9ce18a7034dc Mon Sep 17 00:00:00 2001 From: "Daniel G. Taylor" Date: Wed, 11 Mar 2015 11:23:29 -0700 Subject: [PATCH 1/3] Fix CloudTrail handling of custom policy URLs. This updates the CloudTrail customizations to stop making HTTP requests to fetch custom policies, instead relying on the updated behavior of the CLI to automatically handle file and URL parameters for custom commands. Before this fix, the CLI would fetch the data into a string, then the CloudTrail customization would try to fetch the string as if it were a URL, fail, and throw an error. --- awscli/customizations/cloudtrail.py | 17 ++++++------ tests/unit/customizations/test_cloudtrail.py | 27 ++++++++++++++++++++ 2 files changed, 35 insertions(+), 9 deletions(-) diff --git a/awscli/customizations/cloudtrail.py b/awscli/customizations/cloudtrail.py index 4cc0eb255564..17d511a6ba31 100644 --- a/awscli/customizations/cloudtrail.py +++ b/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) diff --git a/tests/unit/customizations/test_cloudtrail.py b/tests/unit/customizations/test_cloudtrail.py index 92395df8b7e5..4e6202180570 100644 --- a/tests/unit/customizations/test_cloudtrail.py +++ b/tests/unit/customizations/test_cloudtrail.py @@ -162,6 +162,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 +234,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') From 47b285ef3fb836ddcb851fb37f1e20c853b810a9 Mon Sep 17 00:00:00 2001 From: "Daniel G. Taylor" Date: Wed, 11 Mar 2015 13:24:52 -0700 Subject: [PATCH 2/3] Add a test for paramfile usage --- tests/unit/customizations/test_cloudtrail.py | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/tests/unit/customizations/test_cloudtrail.py b/tests/unit/customizations/test_cloudtrail.py index 4e6202180570..f45cf4441b19 100644 --- a/tests/unit/customizations/test_cloudtrail.py +++ b/tests/unit/customizations/test_cloudtrail.py @@ -19,7 +19,7 @@ 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 +46,14 @@ def test_create_subscription_has_zero_rc(self): # sure it says log delivery is happening. self.assertIn('Logs will be delivered to foo', stdout) + def test_policy_from_paramfile(self): + with temporary_file('w') as f: + f.write('{"Statement": []}') + command = ( + 'cloudtrail create-subscription --s3-use-bucket foo ' + '--name bar --s3-custom-policy file://{0}'.format(f.name)) + self.run_cmd(command, expected_rc=0) + class TestCloudTrailCommand(unittest.TestCase): def setUp(self): From 34fd660bb98a3ce7ebd5ef9cfc1cbac45aad88d7 Mon Sep 17 00:00:00 2001 From: "Daniel G. Taylor" Date: Fri, 13 Mar 2015 11:34:09 -0700 Subject: [PATCH 3/3] Update paramfile test --- tests/unit/customizations/test_cloudtrail.py | 27 +++++++++++++++++--- 1 file changed, 23 insertions(+), 4 deletions(-) diff --git a/tests/unit/customizations/test_cloudtrail.py b/tests/unit/customizations/test_cloudtrail.py index f45cf4441b19..5fc9bfcd62dd 100644 --- a/tests/unit/customizations/test_cloudtrail.py +++ b/tests/unit/customizations/test_cloudtrail.py @@ -12,8 +12,9 @@ # 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 @@ -46,14 +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) - def test_policy_from_paramfile(self): + @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('{"Statement": []}') + f.write(policy) + f.flush() command = ( - 'cloudtrail create-subscription --s3-use-bucket foo ' + '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):