From 5739db47011a1732c284c997ff8c1fed277ffc86 Mon Sep 17 00:00:00 2001 From: Bert Blommers Date: Fri, 16 Sep 2022 15:59:45 +0000 Subject: [PATCH] S3:get_object() - Simple BucketPolicy validation (#5476) --- moto/iam/access_control.py | 11 +++-- moto/moto_server/threaded_moto_server.py | 2 +- moto/s3/models.py | 10 +++- moto/s3/responses.py | 9 ++-- tests/test_s3/test_s3_bucket_policy.py | 62 ++++++++++++++++++++++++ 5 files changed, 85 insertions(+), 9 deletions(-) create mode 100644 tests/test_s3/test_s3_bucket_policy.py diff --git a/moto/iam/access_control.py b/moto/iam/access_control.py index 3f7f89026..0ad823cee 100644 --- a/moto/iam/access_control.py +++ b/moto/iam/access_control.py @@ -353,12 +353,14 @@ class IAMPolicy(object): self._policy_json = json.loads(policy_document) - def is_action_permitted(self, action): + def is_action_permitted(self, action, resource="*"): permitted = False if isinstance(self._policy_json["Statement"], list): for policy_statement in self._policy_json["Statement"]: iam_policy_statement = IAMPolicyStatement(policy_statement) - permission_result = iam_policy_statement.is_action_permitted(action) + permission_result = iam_policy_statement.is_action_permitted( + action, resource + ) if permission_result == PermissionResult.DENIED: return permission_result elif permission_result == PermissionResult.PERMITTED: @@ -377,7 +379,7 @@ class IAMPolicyStatement(object): def __init__(self, statement): self._statement = statement - def is_action_permitted(self, action): + def is_action_permitted(self, action, resource="*"): is_action_concerned = False if "NotAction" in self._statement: @@ -388,7 +390,8 @@ class IAMPolicyStatement(object): is_action_concerned = True if is_action_concerned: - if self._statement["Effect"] == "Allow": + same_resource = self._match(self._statement["Resource"], resource) + if self._statement["Effect"] == "Allow" and same_resource: return PermissionResult.PERMITTED else: # Deny return PermissionResult.DENIED diff --git a/moto/moto_server/threaded_moto_server.py b/moto/moto_server/threaded_moto_server.py index 0adf816f5..1f1e91335 100644 --- a/moto/moto_server/threaded_moto_server.py +++ b/moto/moto_server/threaded_moto_server.py @@ -31,7 +31,7 @@ class ThreadedMotoServer: self._thread = Thread(target=self._server_entry, daemon=True) self._thread.start() while not self._server_ready: - time.sleep(0.5) + time.sleep(0.1) def stop(self): self._server_ready = False diff --git a/moto/s3/models.py b/moto/s3/models.py index 3dd700757..bae01746d 100644 --- a/moto/s3/models.py +++ b/moto/s3/models.py @@ -27,6 +27,7 @@ from moto.core.utils import ( BackendDict, ) from moto.cloudwatch.models import MetricDatum +from moto.iam.access_control import IAMPolicy, PermissionResult from moto.moto_api import state_manager from moto.moto_api._internal.managed_state_model import ManagedState from moto.utilities.tagging_service import TaggingService @@ -907,6 +908,13 @@ class FakeBucket(CloudFormationModel): def is_versioned(self): return self.versioning_status == "Enabled" + def allow_action(self, action, resource): + if self.policy is None: + return False + iam_policy = IAMPolicy(self.policy.decode()) + result = iam_policy.is_action_permitted(action, resource) + return result == PermissionResult.PERMITTED + def set_lifecycle(self, rules): self.rules = [] for rule in rules: @@ -1531,7 +1539,7 @@ class S3Backend(BaseBackend, CloudWatchMetricProvider): def list_buckets(self): return self.buckets.values() - def get_bucket(self, bucket_name): + def get_bucket(self, bucket_name) -> FakeBucket: try: return self.buckets[bucket_name] except KeyError: diff --git a/moto/s3/responses.py b/moto/s3/responses.py index 4e28a9d1c..cdb9cf211 100644 --- a/moto/s3/responses.py +++ b/moto/s3/responses.py @@ -1155,10 +1155,13 @@ class S3Response(BaseResponse): signed_url = "Signature=" in request.path key = self.backend.get_object(bucket_name, key_name) - if key: - if (key.acl and not key.acl.public_read) and not signed_url: + if key and not signed_url: + bucket = self.backend.get_bucket(bucket_name) + resource = f"arn:aws:s3:::{bucket_name}/{key_name}" + bucket_policy_allows = bucket.allow_action("s3:GetObject", resource) + if not bucket_policy_allows and (key.acl and not key.acl.public_read): return 403, {}, "" - elif signed_url: + elif signed_url and not key: # coming in from requests.get(s3.generate_presigned_url()) if self._invalid_headers(request.url, dict(request.headers)): return 403, {}, S3_INVALID_PRESIGNED_PARAMETERS diff --git a/tests/test_s3/test_s3_bucket_policy.py b/tests/test_s3/test_s3_bucket_policy.py new file mode 100644 index 000000000..07944a952 --- /dev/null +++ b/tests/test_s3/test_s3_bucket_policy.py @@ -0,0 +1,62 @@ +import boto3 +import json +import requests +import pytest +import sure # noqa # pylint: disable=unused-import + +from moto.moto_server.threaded_moto_server import ThreadedMotoServer + + +class TestBucketPolicy: + @staticmethod + def setup_class(cls): + cls.server = ThreadedMotoServer(port="6000", verbose=False) + cls.server.start() + + def setup(self) -> None: + self.client = boto3.client( + "s3", region_name="us-east-1", endpoint_url="http://localhost:6000" + ) + self.client.create_bucket(Bucket="mybucket") + self.client.put_object(Bucket="mybucket", Key="test_txt", Body=b"mybytes") + self.key_name = "http://localhost:6000/mybucket/test_txt" + + def teardown(self) -> None: + self.client.delete_object(Bucket="mybucket", Key="test_txt") + self.client.delete_bucket(Bucket="mybucket") + + @staticmethod + def teardown_class(cls): + cls.server.stop() + + @pytest.mark.parametrize( + "kwargs,status", + [ + ({}, 200), + ({"resource": "arn:aws:s3:::mybucket/test_txt"}, 200), + ({"resource": "arn:aws:s3:::notmybucket/*"}, 403), + ({"resource": "arn:aws:s3:::mybucket/other*"}, 403), + ({"actions": ["s3:PutObject"]}, 403), + ({"effect": "Deny"}, 403), + ], + ) + def test_policy_allow_all(self, kwargs, status): + self._put_policy(**kwargs) + + requests.get(self.key_name).status_code.should.equal(status) + + def _put_policy( + self, resource="arn:aws:s3:::mybucket/*", effect="Allow", actions=None + ): + policy = { + "Version": "2012-10-17", + "Statement": [ + { + "Effect": effect, + "Principal": "*", + "Action": actions or ["s3:GetObject"], + "Resource": resource, + } + ], + } + self.client.put_bucket_policy(Bucket="mybucket", Policy=json.dumps(policy))