Skip to content

Disable moto S3 bucket policy enforcement to unblock CDK bootstraps#7907

Merged
thrau merged 4 commits intomasterfrom
s3-disable-iam
Mar 21, 2023
Merged

Disable moto S3 bucket policy enforcement to unblock CDK bootstraps#7907
thrau merged 4 commits intomasterfrom
s3-disable-iam

Conversation

@whummer
Copy link
Member

@whummer whummer commented Mar 19, 2023 •
edited by viren-nadkarni
Loading

Disable moto's S3 bucket policy enforcement to unblock CDK bootstraps and bump moto-ext to 4.1.5.post1

Moto recently added more strict bucket policy enforcement: getmoto/moto#5883

This can break cdklocal bootstrap commands, as CDK is creating a bucket policy that denies access to bucket objects over non-SSL connections (by enforcing the aws:SecureTransport attribute in the policy): https://github.com/aws/aws-cdk/blob/e8158af34eb6402c79edbc171746fb5501775c68/packages/aws-cdk/lib/api/bootstrap/bootstrap-template.yaml#L217-L233

After the bootstrap, cdklocal deploy can then result in errors in the CloudFormation deployment:

  ...
  File ".../localstack/services/cloudformation/provider.py", line 415, in create_change_set
    api_utils.prepare_template_body(
  File ".../localstack/services/cloudformation/api_utils.py", line 23, in prepare_template_body
    modified_template_body = get_template_body(req_data)
  File ".../localstack/services/cloudformation/api_utils.py", line 54, in get_template_body
    result = client.get_object(Bucket=parts[0], Key=parts[2])
  File ".../.venv/lib/python3.10/site-packages/botocore/client.py", line 530, in _api_call
    return self._make_api_call(operation_name, kwargs)
  File ".../.venv/lib/python3.10/site-packages/botocore/client.py", line 960, in _make_api_call
    raise error_class(parsed_response, operation_name)
botocore.exceptions.ClientError: An error occurred (403) when calling the GetObject operation: Forbidden

This was discovered when deploying some CDK stacks recently, among others this Developer Hub sample here: localstack-samples/sample-microservices-apigateway-lambda-dynamodb-sqs#1

Seems like a reasonable approach to disable the bucket enforcement entirely (without making it configurable), to unblock CDK users, and also given the recent enhancements in our own IAM enforcement - but open to suggestions! /cc @bentsku @dfangl @thrau

@whummer whummer requested review from dfangl and thrau March 19, 2023 14:11
@whummer whummer temporarily deployed to localstack-ext-tests March 19, 2023 14:11 — with GitHub Actions Inactive
@thrau
Copy link
Member

thrau commented Mar 19, 2023 •
edited
Loading

definitely think this is a bit too restrictive on moto's side. @whummer i saw that @viren-nadkarni reverted this commit in moto-ext: localstack/moto@ffca152

how do we plan to consolidate the changes in this PR and the commit revert? can we remove the reverted commit and just use the patch? is there some dependence between the two?

@github-actions
Copy link

github-actions bot commented Mar 19, 2023 •
edited
Loading

LocalStack integration with Pro

       3 files  ±  0         3 suites  ±0   1h 35m 2s ⏱️ + 2m 31s
1 831 tests +17  1 448 ✔️ +19  383 💤  - 2  0 ❌ ±0 
2 555 runs  +23  1 815 ✔️ +20  740 💤 +3  0 ❌ ±0 

Results for commit ea47b81. ± Comparison against base commit 5949082.

♻️ This comment has been updated with latest results.

@whummer
Copy link
Member Author

whummer commented Mar 19, 2023 •
edited
Loading

@thrau Good catch, thanks! (looks like I didn't have that moto change locally yet 🙈 ). To me, either way is fine - undoing the commit revert maybe could make sense (to reduce the number of potential rebase conflicts, going forward). 👍 In any case, this PR would only work/build if the commit revert is undone (otherwise it could also be closed.. 🤷 ). Thoughts @viren-nadkarni ?

@bentsku
Copy link
Contributor

bentsku commented Mar 19, 2023 •
edited
Loading

Before doing a review I'll wait for the decision between the revert of the commit or patching, but I have opinions on the patching.

To make a clear difference between the legacy and the new ASF provider, patches have been duplicated.
Some are in the s3_listener/starter, and other are in the new ASF provider provider.py.
I think we should not have a patch in s3_utils, as we're trying to sunset it in favour of the new s3/utils.py. The goal would be to move "legacy" things into a subfolder s3/legacy/<files>, and see what breaks (lots of imports I believe).

The best would be to add this patch both in s3_listener and in provider if possible. I'll review once the choice have been made 😄

@dfangl
Copy link
Member

dfangl commented Mar 19, 2023

Viren and I decided to revert the patch for now, as it would have unforseen consequences and does not integrate with our IAM enforcement at all.
Plan was to check the PR in the future and make it interoperable with our IAM enforcement, but this has not happened so far, which is why the revert is still there.
I don't have a specific opinion on patching vs revert though, just to give some context.

@viren-nadkarni
Copy link
Member

viren-nadkarni commented Mar 20, 2023

I think this patch makes sense over maintaining the revert commit in moto-ext. There were already quite a few conflicts during the last moto-ext bump (4.1.4).

I will re-release moto-ext with latest 4.1.5 changes and remove the revert commit. We can merge this PR after the bump 👍

@bentsku bentsku temporarily deployed to localstack-ext-tests March 20, 2023 19:37 — with GitHub Actions Inactive
@viren-nadkarni viren-nadkarni temporarily deployed to localstack-ext-tests March 21, 2023 08:21 — with GitHub Actions Inactive
@viren-nadkarni
Remove policy serialisation
ea47b81 
With getmoto/moto#6049, policies are saved as str
@viren-nadkarni viren-nadkarni temporarily deployed to localstack-ext-tests March 21, 2023 12:17 — with GitHub Actions Inactive
@coveralls
Copy link

coveralls commented Mar 21, 2023

Coverage Status

Coverage: 85.089% (-0.03%) from 85.121% when pulling ea47b81 on s3-disable-iam into 5949082 on master.

@viren-nadkarni
Copy link
Member

viren-nadkarni commented Mar 21, 2023

moto-ext is now bumped to 4.1.5.post1

@viren-nadkarni viren-nadkarni mentioned this pull request Mar 21, 2023
Closed
whummer
whummer commented Mar 21, 2023
View reviewed changes
Copy link
Member Author

@whummer whummer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM ✅ Thanks @viren-nadkarni for pushing this one over the line! 🙌

@bentsku
Copy link
Contributor

bentsku commented Mar 21, 2023

LGTM too for the code I haven't written 😄 that's one collaborative PR!

dfangl
dfangl approved these changes Mar 21, 2023
View reviewed changes
Copy link
Member

@dfangl dfangl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@thrau thrau merged commit 5ec57f0 into master Mar 21, 2023
@thrau thrau deleted the s3-disable-iam branch March 21, 2023 17:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@dfangl dfangl dfangl approved these changes

@thrau thrau Awaiting requested review from thrau

@bentsku bentsku Awaiting requested review from bentsku

@macnev2013 macnev2013 Awaiting requested review from macnev2013

@dominikschubert dominikschubert Awaiting requested review from dominikschubert

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@whummer @thrau @bentsku @dfangl @viren-nadkarni @coveralls