10000 ExpectedBucketOwner for S3 bucket policy operations (#11827) · localstack/localstack@b17f4a1 · GitHub
[go: up one dir, main page]
Skip to content

Commit b17f4a1

Browse files
nikosmichasnikosmichas
authored
ExpectedBucketOwner for S3 bucket policy operations (#11827)
1 parent 3660784 commit b17f4a1

File tree

5 files changed

+488
-50
lines changed

5 files changed

+488
-50
lines changed

‎localstack-core/localstack/services/s3/exceptions.py

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -41,3 +41,8 @@ def __init__(self, message=None):
4141
class MalformedPolicy(CommonServiceException):
4242
def __init__(self, message=None):
4343
super().__init__("MalformedPolicy", status_code=400, message=message)
44+
45+
46+
class InvalidBucketOwnerAWSAccountID(CommonServiceException):
47+
def __init__(self, message=None) -> None:
48+
super().__init__("InvalidBucketOwnerAWSAccountID", status_code=400, message=message)

‎localstack-core/localstack/services/s3/provider.py

Lines changed: 24 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,7 @@
33
import datetime
44
import json
55
import logging
6+
import re
67
from collections import defaultdict
78
from io import BytesIO
89
from operator import itemgetter
@@ -223,6 +224,7 @@
223224
)
224225
from localstack.services.s3.cors import S3CorsHandler, s3_cors_request_handler
225226
from localstack.services.s3.exceptions import (
227+
InvalidBucketOwnerAWSAccountID,
226228
InvalidBucketState,
227229
InvalidRequest,
228230
MalformedPolicy,
@@ -412,8 +414,17 @@ def _get_expiration_header(
412414
return expiration_header
413415

414416
def _get_cross_account_bucket(
415-
self, context: RequestContext, bucket_name: BucketName
417+
self,
418+
context: RequestContext,
419+
bucket_name: BucketName,
420+
*,
421+
expected_bucket_owner: AccountId = None,
416422
) -> tuple[S3Store, S3Bucket]:
423+
if expected_bucket_owner and not re.fullmatch(r"\w{12}", expected_bucket_owner):
424+
raise InvalidBucketOwnerAWSAccountID(
425+
f"The value of the expected bucket owner parameter must be an AWS Account ID... [{expected_bucket_owner}]",
426+
)
427+
417428
store = self.get_store(context.account_id, context.region)
418429
if not (s3_bucket := store.buckets.get(bucket_name)):
419430
if not (account_id := store.global_bucket_map.get(bucket_name)):
@@ -423,6 +434,9 @@ def _get_cross_account_bucket(
423434
if not (s3_bucket := store.buckets.get(bucket_name)):
424435
raise NoSuchBucket("The specified bucket does not exist", BucketName=bucket_name)
425436

437+
if expected_bucket_owner and s3_bucket.bucket_account_id != expected_bucket_owner:
438+
raise AccessDenied("Access Denied")
439+
426440
return store, s3_bucket
427441

428442
@staticmethod
@@ -3646,7 +3660,9 @@ def get_bucket_policy(
36463660
expected_bucket_owner: AccountId = None,
36473661
**kwargs,
36483662
) -> GetBucketPolicyOutput:
3649-
store, s3_bucket = self._get_cross_account_bucket(context, bucket)
3663+
store, s3_bucket = self._get_cross_account_bucket(
3664+
context, bucket, expected_bucket_owner=expected_bucket_owner
3665+
)
36503666
if not s3_bucket.policy:
36513667
raise NoSuchBucketPolicy(
36523668
"The bucket policy does not exist",
@@ -3665,7 +3681,9 @@ def put_bucket_policy(
36653681
expected_bucket_owner: AccountId = None,
36663682
**kwargs,
36673683
) -> None:
3668-
store, s3_bucket = self._get_cross_account_bucket(context, bucket)
3684+
store, s3_bucket = self._get_cross_account_bucket(
3685+
context, bucket, expected_bucket_owner=expected_bucket_owner
3686+
)
36693687

36703688
if not policy or policy[0] != "{":
36713689
raise MalformedPolicy("Policies must be valid JSON and the first byte must be '{'")
@@ -3686,7 +3704,9 @@ def delete_bucket_policy(
36863704
expected_bucket_owner: AccountId = None,
36873705
**kwargs,
36883706
) -> None:
3689-
store, s3_bucket = self._get_cross_account_bucket(context, bucket)
3707+
store, s3_bucket = self._get_cross_account_bucket(
3708+
context, bucket, expected_bucket_owner=expected_bucket_owner
3709+
)
36903710

36913711
s3_bucket.policy = None
36923712

‎tests/aws/services/s3/test_s3.py

Lines changed: 132 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -267,6 +267,20 @@ def _filter_header(param: dict) -> dict:
267267
return {k: v for k, v in param.items() if k.startswith("x-amz") or k in ["content-type"]}
268268

269269

270+
def _simple_bucket_policy(s3_bucket: str) -> dict:
271+
return {
272+
"Version": "2012-10-17",
273+
"Statement": [
274+
{
275+
"Action": "s3:GetObject",
276+
"Effect": "Allow",
277+
"Resource": f"arn:aws:s3:::{s3_bucket}/*",
278+
"Principal": {"AWS": "*"},
279+
}
280+
],
281+
}
282+
283+
270284
class TestS3:
271285
@pytest.mark.skipif(condition=TEST_S3_IMAGE, reason="KMS not enabled in S3 image")
272286
@markers.aws.validated
@@ -964,31 +978,136 @@ def test_create_bucket_via_host_name(self, s3_vhost_client, aws_client, region_n
964978
s3_vhost_client.delete_bucket(Bucket=bucket_name)
965979

966980
@markers.aws.validated
967-
def test_put_and_get_bucket_policy(self, s3_bucket, snapshot, aws_client, allow_bucket_acl):
981+
def test_get_bucket_policy(self, s3_bucket, snapshot, aws_client, allow_bucket_acl, account_id):
982+
snapshot.add_transformer(snapshot.transform.key_value("Resource"))
983+
snapshot.add_transformer(snapshot.transform.key_value("BucketName"))
984+
985+
with pytest.raises(ClientError) as e:
986+
aws_client.s3.get_bucket_policy(Bucket=s3_bucket)
987+
snapshot.match("get-bucket-policy-no-such-bucket-policy", e.value.response)
988+
989+
policy = _simple_bucket_policy(s3_bucket)
990+
aws_client.s3.put_bucket_policy(Bucket=s3_bucket, Policy=json.dumps(policy))
991+
992+
# retrieve and check policy config
993+
response = aws_client.s3.get_bucket_policy(Bucket=s3_bucket)
994+
snapshot.match("get-bucket-policy", response)
995+
assert policy == json.loads(response["Policy"])
996+
997+
response = aws_client.s3.get_bucket_policy(Bucket=s3_bucket, ExpectedBucketOwner=account_id)
998+
snapshot.match("get-bucket-policy-with-expected-bucket-owner", response)
999+
assert policy == json.loads(response["Policy"])
1000+
1001+
with pytest.raises(ClientError) as e:
1002+
aws_client.s3.get_bucket_policy(Bucket=s3_bucket, ExpectedBucketOwner="000000000002")
1003+
snapshot.match("get-bucket-policy-with-expected-bucket-owner-error", e.value.response)
1004+
1005+
@pytest.mark.parametrize(
1006+
"invalid_account_id", ["0000", "0000000000020", "abcd", "aa000000000$"]
1007+
)
1008+
@markers.aws.validated
1009+
def test_get_bucket_policy_invalid_account_id(
1010+
self, s3_bucket, snapshot, aws_client, invalid_account_id
1011+
):
1012+
with pytest.raises(ClientError) as e:
1013+
aws_client.s3.get_bucket_policy(
1014+
Bucket=s3_bucket, ExpectedBucketOwner=invalid_account_id
1015+
)
1016+
1017+
snapshot.match("get-bucket-policy-invalid-bucket-owner", e.value.response)
1018+
1019+
@markers.aws.validated
1020+
def test_put_bucket_policy(self, s3_bucket, snapshot, aws_client, allow_bucket_acl):
9681021
# just for the joke: Response syntax HTTP/1.1 200
9691022
# sample response: HTTP/1.1 204 No Content
9701023
# https://docs.aws.amazon.com/AmazonS3/latest/API/API_PutBucketPolicy.html
9711024
snapshot.add_transformer(snapshot.transform.key_value("Resource"))
9721025
# put bucket policy
973-
policy = {
974-
"Version": "2012-10-17",
975-
"Statement": [
976-
{
977-
"Action": "s3:GetObject",
978-
"Effect": "Allow",
979-
"Resource": f"arn:aws:s3:::{s3_bucket}/*",
980-
"Principal": {"AWS": "*"},
981-
}
982-
],
983-
}
1026+
policy = _simple_bucket_policy(s3_bucket)
9841027
response = aws_client.s3.put_bucket_policy(Bucket=s3_bucket, Policy=json.dumps(policy))
9851028
snapshot.match("put-bucket-policy", response)
9861029

987-
# retrieve and check policy config
9881030
response = aws_client.s3.get_bucket_policy(Bucket=s3_bucket)
9891031
snapshot.match("get-bucket-policy", response)
9901032
assert policy == json.loads(response["Policy"])
9911033

1034+
@markers.aws.validated
1035+
def test_put_bucket_policy_expected_bucket_owner(
1036+
self, s3_bucket, snapshot, aws_client, allow_bucket_acl, account_id, secondary_account_id
1037+
):
1038+
snapshot.add_transformer(snapshot.transform.key_value("Resource"))
1039+
policy = _simple_bucket_policy(s3_bucket)
1040+
1041+
with pytest.raises(ClientError) as e:
1042+
aws_client.s3.put_bucket_policy(
1043+
Bucket=s3_bucket,
1044+
Policy=json.dumps(policy),
1045+
ExpectedBucketOwner=secondary_account_id,
1046+
)
1047+
snapshot.match("put-bucket-policy-with-expected-bucket-owner-error", e.value.response)
1048+
1049+
response = aws_client.s3.put_bucket_policy(
1050+
Bucket=s3_bucket, Policy=json.dumps(policy), ExpectedBucketOwner=account_id
1051+
)
1052+
snapshot.match("put-bucket-policy-with-expected-bucket-owner", response)
1053+
1054+
@pytest.mark.parametrize(
1055+
"invalid_account_id", ["0000", "0000000000020", "abcd", "aa000000000$"]
1056+
)
1057+
@markers.aws.validated
1058+
def test_put_bucket_policy_invalid_account_id(
1059+
self, s3_bucket, snapshot, aws_client, invalid_account_id
1060+
):
1061+
snapshot.add_transformer(snapshot.transform.key_value("Resource"))
1062+
policy = _simple_bucket_policy(s3_bucket)
1063+
1064+
with pytest.raises(ClientError) as e:
1065+
aws_client.s3.put_bucket_policy(
1066+
Bucket=s3_bucket, Policy=json.dumps(policy), ExpectedBucketOwner=invalid_account_id
1067+
)
1068+
1069+
snapshot.match("put-bucket-policy-invalid-bucket-owner", e.value.response)
1070+
1071+
@markers.aws.validated
1072+
def test_delete_bucket_policy(self, s3_bucket, snapshot, aws_client, allow_bucket_acl):
1073+
snapshot.add_transformer(snapshot.transform.key_value("Resource"))
1074+
snapshot.add_transformer(snapshot.transform.key_value("BucketName"))
1075+
1076+
policy = _simple_bucket_policy(s3_bucket)
1077+
aws_client.s3.put_bucket_policy(Bucket=s3_bucket, Policy=json.dumps(policy))
1078+
1079+
response = aws_client.s3.delete_bucket_policy(Bucket=s3_bucket)
1080+
snapshot.match("delete-bucket-policy", response)
1081+
1082+
with pytest.raises(ClientError) as e:
1083+
aws_client.s3.get_bucket_policy(Bucket=s3_bucket)
1084+
snapshot.match("get-bucket-policy-no-such-bucket-policy", e.value.response)
1085+
1086+
@markers.aws.validated
1087+
def test_delete_bucket_policy_expected_bucket_owner(
1088+
self, s3_bucket, snapshot, aws_client, allow_bucket_acl, account_id, secondary_account_id
1089+
):
1090+
snapshot.add_transformer(snapshot.transform.key_value("Resource"))
1091+
snapshot.add_transformer(snapshot.transform.key_value("BucketName"))
1092+
1093+
policy = _simple_bucket_policy(s3_bucket)
1094+
aws_client.s3.put_bucket_policy(Bucket=s3_bucket, Policy=json.dumps(policy))
1095+
1096+
with pytest.raises(ClientError) as e:
1097+
aws_client.s3.delete_bucket_policy(
1098+
Bucket=s3_bucket, ExpectedBucketOwner=secondary_account_id
1099+
)
1100+
snapshot.match("delete-bucket-policy-with-expected-bucket-owner-error", e.value.response)
1101+
1102+
with pytest.raises(ClientError) as e:
1103+
aws_client.s3.delete_bucket_policy(Bucket=s3_bucket, ExpectedBucketOwner="invalid")
1104+
snapshot.match("delete-bucket-policy-invalid-bucket-owner", e.value.response)
1105+
1106+
response = aws_client.s3.delete_bucket_policy(
1107+
Bucket=s3_bucket, ExpectedBucketOwner=account_id
1108+
)
1109+
snapshot.match("delete-bucket-policy-with-expected-bucket-owner", response)
1110+
9921111
@markers.aws.validated
9931112
def test_put_object_tagging_empty_list(self, s3_bucket, snapshot, aws_client):
9941113
key = "my-key"

0 commit comments

Comments
 (0)
0