From ce24a6a4df52f1e4bff80c3363ab00105abbefb1 Mon Sep 17 00:00:00 2001 From: Sebastian Korfmann Date: Thu, 20 Jul 2023 15:17:48 +0200 Subject: [PATCH] fix(sdk): bucket needs to be marked as public before adding a public policy (#3536) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This was a super nasty bug to track down since it's apparently working on MacOS but randomly failing on Linux (i.e. Github Actions). Due to this, I went down a few other rabbit holes but eventually was able to isolate it. There are also a few issues in the AWS Provider repo about this, where the explicit dependency is also [mentioned](https://github.com/hashicorp/terraform-provider-aws/issues/31363#issuecomment-1588091658) as the way forward here. ### Minimal case With this minimal case, I'm able to reproduce this on `ghcr.io/winglang/wing-github-action:v0.1.0`. The unscientific test with the minimal example and applying / destroying 10 times in a row: 1. Linux (Docker) 3/10 failed 2. Mac 0/10 failed (might fail if run a 100 times, don't know) With more Terraform resources, the failure rate on Linux seems to increase. #### Failed run ``` aws_s3_bucket.cloudWebsite_WebsiteBucket_EB03D355: Creating... aws_s3_bucket.cloudWebsite_WebsiteBucket_EB03D355: Creation complete after 2s [id=cloud-website-c8e58765-20230719204937669500000001] aws_s3_bucket_policy.cloudWebsite_PublicPolicy_44BB71F3: Creating... aws_s3_bucket_public_access_block.cloudWebsite_PublicAccessBlock_18A70311: Creating... aws_s3_bucket_public_access_block.cloudWebsite_PublicAccessBlock_18A70311: Creation complete after 1s [id=cloud-website-c8e58765-20230719204937669500000001] ╷ │ Error: Error putting S3 policy: AccessDenied: Access Denied ``` #### Successful run ``` aws_s3_bucket.cloudWebsite_WebsiteBucket_EB03D355: Creating... aws_s3_bucket.cloudWebsite_WebsiteBucket_EB03D355: Creation complete after 2s [id=cloud-website-c8e58765-20230719205053652900000001] aws_s3_bucket_public_access_block.cloudWebsite_PublicAccessBlock_18A70311: Creating... aws_s3_bucket_policy.cloudWebsite_PublicPolicy_44BB71F3: Creating... aws_s3_bucket_policy.cloudWebsite_PublicPolicy_44BB71F3: Creation complete after 1s [id=cloud-website-c8e58765-20230719205053652900000001] aws_s3_bucket_public_access_block.cloudWebsite_PublicAccessBlock_18A70311: Creation complete after 1s [id=cloud-website-c8e58765-20230719205053652900000001] Apply complete! Resources: 3 added, 0 changed, 0 destroyed. ``` Notice the different order of resource creation. #### Code ``` { "provider": { "aws": [ { } ] }, "resource": { "aws_s3_bucket": { "cloudWebsite_WebsiteBucket_EB03D355": { "//": { "metadata": { "path": "root/Default/Default/cloud.Website/WebsiteBucket", "uniqueId": "cloudWebsite_WebsiteBucket_EB03D355" } }, "bucket_prefix": "cloud-website-c8e58765-", "force_destroy": false } }, "aws_s3_bucket_policy": { "cloudWebsite_PublicPolicy_44BB71F3": { "//": { "metadata": { "path": "root/Default/Default/cloud.Website/PublicPolicy", "uniqueId": "cloudWebsite_PublicPolicy_44BB71F3" } }, "bucket": "${aws_s3_bucket.cloudWebsite_WebsiteBucket_EB03D355.bucket}", "policy": "{\"Version\":\"2012-10-17\",\"Statement\":[{\"Effect\":\"Allow\",\"Principal\":\"*\",\"Action\":[\"s3:GetObject\"],\"Resource\":[\"${aws_s3_bucket.cloudWebsite_WebsiteBucket_EB03D355.arn}/*\"]}]}" } }, "aws_s3_bucket_public_access_block": { "cloudWebsite_PublicAccessBlock_18A70311": { "//": { "metadata": { "path": "root/Default/Default/cloud.Website/PublicAccessBlock", "uniqueId": "cloudWebsite_PublicAccessBlock_18A70311" } }, "block_public_acls": false, "block_public_policy": false, "bucket": "${aws_s3_bucket.cloudWebsite_WebsiteBucket_EB03D355.bucket}", "ignore_public_acls": false, "restrict_public_buckets": false } } }, "terraform": { "backend": { "local": { "path": "./terraform.tfstate" } }, "required_providers": { "aws": { "source": "aws", "version": "4.65.0" } } } } ``` #### Terraform versions Seems to apply to a broad range of versions according to the issues in the AWS Provider Github repo. But that's what I used ``` Terraform v1.5.0 on linux_amd64 + provider registry.terraform.io/hashicorp/aws v4.65.0 Terraform v1.5.2 on darwin_amd64 + provider registry.terraform.io/hashicorp/aws v4.65.0 ``` ## Changes This PR adds an explicit dependency between `aws_s3_bucket_policy` and `aws_s3_bucket_public_access_block` for public buckets. Without this, there might be race conditions between both resources and with the recent [AWS Changes](https://aws.amazon.com/about-aws/whats-new/2023/04/amazon-s3-two-security-best-practices-buckets-default/), public access is blocked by default for all new buckets now. Which means, that the bucket policy - which grants public access (principal `*`) - will be rejected in case `aws_s3_bucket_public_access_block` hasn't been applied yet. The dependency graph of Terraform: ### Before ![graphviz](https://github.com/winglang/wing/assets/136789/f65726f9-d04a-4de6-a581-d1d037257f62) ### After ![graphviz (1)](https://github.com/winglang/wing/assets/136789/6cd37a5c-52f5-4384-a636-9079124b6d75) ## Summary This is one of these issues which makes cloud development (on AWS) sometimes a huge pain and really frustrating. It's easily wasting a day by providing unclear errors messages `Error putting S3 policy: AccessDenied: Access Denied` in combination with different behaviour depending on the operating system being used ("works on my machine, it has to be something related to CI"). Which sets one up to step into several trap doors troubleshooting permissions ("maybe it's a subtle misconfiguration in the OIDC setup") or some weird trace logs from Terraform, which seem related but probably are not (more like a symptom) ``` [WARN] Provider "provider[\"registry.terraform.io/hashicorp/aws\"]" produced an unexpected new value for aws_s3_bucket_public_access_block.env0_cloudWebsite_PublicAccessBlock_E7BC7F4B, but we are tolerating it because it is using the legacy plugin SDK. The following problems may be the cause of any confusing errors from downstream operations: - .block_public_acls: was cty.False, but now cty.True - .block_public_policy: was cty.False, but now cty.True - .ignore_public_acls: was cty.False, but now cty.True - .restrict_public_buckets: was cty.False, but now cty.True ``` to eventually dig really deep to come up with a two line change 🎉 ## Checklist - [x] Title matches [Winglang's style guide](https://www.winglang.io/contributing/start-here/pull_requests#how-are-pull-request-titles-formatted) - [x] Description explains motivation and solution - [x] Tests added (always) - [x] Docs updated (only required for features) - [x] Added `pr/e2e-full` label if this feature requires end-to-end testing *By submitting this pull request, I confirm that my contribution is made under the terms of the [Wing Cloud Contribution License](https://github.com/winglang/wing/blob/main/CONTRIBUTION_LICENSE.md)*. --- libs/wingsdk/src/target-tf-aws/bucket.ts | 19 ++++++++++++------- .../__snapshots__/bucket.test.ts.snap | 18 ++++++++++++++++++ .../__snapshots__/website.test.ts.snap | 6 ++++++ .../bucket/public_url.w_compile_tf-aws.md | 3 +++ .../website/website.w_compile_tf-aws.md | 5 ++++- .../valid/captures.w_compile_tf-aws.md | 3 +++ .../website_with_api.w_compile_tf-aws.md | 5 ++++- 7 files changed, 50 insertions(+), 9 deletions(-) diff --git a/libs/wingsdk/src/target-tf-aws/bucket.ts b/libs/wingsdk/src/target-tf-aws/bucket.ts index ce2bc934b7a..d92f864acb7 100644 --- a/libs/wingsdk/src/target-tf-aws/bucket.ts +++ b/libs/wingsdk/src/target-tf-aws/bucket.ts @@ -180,13 +180,17 @@ export function createEncryptedBucket( }); if (isPublic) { - new S3BucketPublicAccessBlock(scope, "PublicAccessBlock", { - bucket: bucket.bucket, - blockPublicAcls: false, - blockPublicPolicy: false, - ignorePublicAcls: false, - restrictPublicBuckets: false, - }); + const publicAccessBlock = new S3BucketPublicAccessBlock( + scope, + "PublicAccessBlock", + { + bucket: bucket.bucket, + blockPublicAcls: false, + blockPublicPolicy: false, + ignorePublicAcls: false, + restrictPublicBuckets: false, + } + ); const policy = { Version: "2012-10-17", Statement: [ @@ -201,6 +205,7 @@ export function createEncryptedBucket( new S3BucketPolicy(scope, "PublicPolicy", { bucket: bucket.bucket, policy: JSON.stringify(policy), + dependsOn: [publicAccessBlock], }); } else { new S3BucketPublicAccessBlock(scope, "PublicAccessBlock", { diff --git a/libs/wingsdk/test/target-tf-aws/__snapshots__/bucket.test.ts.snap b/libs/wingsdk/test/target-tf-aws/__snapshots__/bucket.test.ts.snap index 99c3127b7c2..1f88d6681b4 100644 --- a/libs/wingsdk/test/target-tf-aws/__snapshots__/bucket.test.ts.snap +++ b/libs/wingsdk/test/target-tf-aws/__snapshots__/bucket.test.ts.snap @@ -17,6 +17,9 @@ exports[`bucket is public 1`] = ` \\"aws_s3_bucket_policy\\": { \\"my_bucket_PublicPolicy_AF351571\\": { \\"bucket\\": \\"\${aws_s3_bucket.my_bucket.bucket}\\", + \\"depends_on\\": [ + \\"aws_s3_bucket_public_access_block.my_bucket_PublicAccessBlock_538547C0\\" + ], \\"policy\\": \\"{\\\\\\"Version\\\\\\":\\\\\\"2012-10-17\\\\\\",\\\\\\"Statement\\\\\\":[{\\\\\\"Effect\\\\\\":\\\\\\"Allow\\\\\\",\\\\\\"Principal\\\\\\":\\\\\\"*\\\\\\",\\\\\\"Action\\\\\\":[\\\\\\"s3:GetObject\\\\\\"],\\\\\\"Resource\\\\\\":[\\\\\\"\${aws_s3_bucket.my_bucket.arn}/*\\\\\\"]}]}\\" } }, @@ -571,6 +574,9 @@ exports[`bucket with onCreate method 1`] = ` \\"aws_s3_bucket_policy\\": { \\"my_bucket_PublicPolicy_AF351571\\": { \\"bucket\\": \\"\${aws_s3_bucket.my_bucket.bucket}\\", + \\"depends_on\\": [ + \\"aws_s3_bucket_public_access_block.my_bucket_PublicAccessBlock_538547C0\\" + ], \\"policy\\": \\"{\\\\\\"Version\\\\\\":\\\\\\"2012-10-17\\\\\\",\\\\\\"Statement\\\\\\":[{\\\\\\"Effect\\\\\\":\\\\\\"Allow\\\\\\",\\\\\\"Principal\\\\\\":\\\\\\"*\\\\\\",\\\\\\"Action\\\\\\":[\\\\\\"s3:GetObject\\\\\\"],\\\\\\"Resource\\\\\\":[\\\\\\"\${aws_s3_bucket.my_bucket.arn}/*\\\\\\"]}]}\\" } }, @@ -1085,6 +1091,9 @@ exports[`bucket with onDelete method 1`] = ` \\"aws_s3_bucket_policy\\": { \\"my_bucket_PublicPolicy_AF351571\\": { \\"bucket\\": \\"\${aws_s3_bucket.my_bucket.bucket}\\", + \\"depends_on\\": [ + \\"aws_s3_bucket_public_access_block.my_bucket_PublicAccessBlock_538547C0\\" + ], \\"policy\\": \\"{\\\\\\"Version\\\\\\":\\\\\\"2012-10-17\\\\\\",\\\\\\"Statement\\\\\\":[{\\\\\\"Effect\\\\\\":\\\\\\"Allow\\\\\\",\\\\\\"Principal\\\\\\":\\\\\\"*\\\\\\",\\\\\\"Action\\\\\\":[\\\\\\"s3:GetObject\\\\\\"],\\\\\\"Resource\\\\\\":[\\\\\\"\${aws_s3_bucket.my_bucket.arn}/*\\\\\\"]}]}\\" } }, @@ -1687,6 +1696,9 @@ exports[`bucket with onEvent method 1`] = ` \\"aws_s3_bucket_policy\\": { \\"my_bucket_PublicPolicy_AF351571\\": { \\"bucket\\": \\"\${aws_s3_bucket.my_bucket.bucket}\\", + \\"depends_on\\": [ + \\"aws_s3_bucket_public_access_block.my_bucket_PublicAccessBlock_538547C0\\" + ], \\"policy\\": \\"{\\\\\\"Version\\\\\\":\\\\\\"2012-10-17\\\\\\",\\\\\\"Statement\\\\\\":[{\\\\\\"Effect\\\\\\":\\\\\\"Allow\\\\\\",\\\\\\"Principal\\\\\\":\\\\\\"*\\\\\\",\\\\\\"Action\\\\\\":[\\\\\\"s3:GetObject\\\\\\"],\\\\\\"Resource\\\\\\":[\\\\\\"\${aws_s3_bucket.my_bucket.arn}/*\\\\\\"]}]}\\" } }, @@ -2669,6 +2681,9 @@ exports[`bucket with onUpdate method 1`] = ` \\"aws_s3_bucket_policy\\": { \\"my_bucket_PublicPolicy_AF351571\\": { \\"bucket\\": \\"\${aws_s3_bucket.my_bucket.bucket}\\", + \\"depends_on\\": [ + \\"aws_s3_bucket_public_access_block.my_bucket_PublicAccessBlock_538547C0\\" + ], \\"policy\\": \\"{\\\\\\"Version\\\\\\":\\\\\\"2012-10-17\\\\\\",\\\\\\"Statement\\\\\\":[{\\\\\\"Effect\\\\\\":\\\\\\"Allow\\\\\\",\\\\\\"Principal\\\\\\":\\\\\\"*\\\\\\",\\\\\\"Action\\\\\\":[\\\\\\"s3:GetObject\\\\\\"],\\\\\\"Resource\\\\\\":[\\\\\\"\${aws_s3_bucket.my_bucket.arn}/*\\\\\\"]}]}\\" } }, @@ -3117,6 +3132,9 @@ exports[`bucket with two preflight objects 1`] = ` \\"aws_s3_bucket_policy\\": { \\"my_bucket_PublicPolicy_AF351571\\": { \\"bucket\\": \\"\${aws_s3_bucket.my_bucket.bucket}\\", + \\"depends_on\\": [ + \\"aws_s3_bucket_public_access_block.my_bucket_PublicAccessBlock_538547C0\\" + ], \\"policy\\": \\"{\\\\\\"Version\\\\\\":\\\\\\"2012-10-17\\\\\\",\\\\\\"Statement\\\\\\":[{\\\\\\"Effect\\\\\\":\\\\\\"Allow\\\\\\",\\\\\\"Principal\\\\\\":\\\\\\"*\\\\\\",\\\\\\"Action\\\\\\":[\\\\\\"s3:GetObject\\\\\\"],\\\\\\"Resource\\\\\\":[\\\\\\"\${aws_s3_bucket.my_bucket.arn}/*\\\\\\"]}]}\\" } }, diff --git a/libs/wingsdk/test/target-tf-aws/__snapshots__/website.test.ts.snap b/libs/wingsdk/test/target-tf-aws/__snapshots__/website.test.ts.snap index 3cac02e999d..b0bbe0192b0 100644 --- a/libs/wingsdk/test/target-tf-aws/__snapshots__/website.test.ts.snap +++ b/libs/wingsdk/test/target-tf-aws/__snapshots__/website.test.ts.snap @@ -61,6 +61,9 @@ exports[`default website behavior 1`] = ` \\"aws_s3_bucket_policy\\": { \\"Website_PublicPolicy_FF8AB959\\": { \\"bucket\\": \\"\${aws_s3_bucket.Website_WebsiteBucket_3C0321F0.bucket}\\", + \\"depends_on\\": [ + \\"aws_s3_bucket_public_access_block.Website_PublicAccessBlock_C196C11D\\" + ], \\"policy\\": \\"{\\\\\\"Version\\\\\\":\\\\\\"2012-10-17\\\\\\",\\\\\\"Statement\\\\\\":[{\\\\\\"Effect\\\\\\":\\\\\\"Allow\\\\\\",\\\\\\"Principal\\\\\\":\\\\\\"*\\\\\\",\\\\\\"Action\\\\\\":[\\\\\\"s3:GetObject\\\\\\"],\\\\\\"Resource\\\\\\":[\\\\\\"\${aws_s3_bucket.Website_WebsiteBucket_3C0321F0.arn}/*\\\\\\"]}]}\\" } }, @@ -337,6 +340,9 @@ exports[`website with add_json 1`] = ` \\"aws_s3_bucket_policy\\": { \\"Website_PublicPolicy_FF8AB959\\": { \\"bucket\\": \\"\${aws_s3_bucket.Website_WebsiteBucket_3C0321F0.bucket}\\", + \\"depends_on\\": [ + \\"aws_s3_bucket_public_access_block.Website_PublicAccessBlock_C196C11D\\" + ], \\"policy\\": \\"{\\\\\\"Version\\\\\\":\\\\\\"2012-10-17\\\\\\",\\\\\\"Statement\\\\\\":[{\\\\\\"Effect\\\\\\":\\\\\\"Allow\\\\\\",\\\\\\"Principal\\\\\\":\\\\\\"*\\\\\\",\\\\\\"Action\\\\\\":[\\\\\\"s3:GetObject\\\\\\"],\\\\\\"Resource\\\\\\":[\\\\\\"\${aws_s3_bucket.Website_WebsiteBucket_3C0321F0.arn}/*\\\\\\"]}]}\\" } }, diff --git a/tools/hangar/__snapshots__/test_corpus/sdk_tests/bucket/public_url.w_compile_tf-aws.md b/tools/hangar/__snapshots__/test_corpus/sdk_tests/bucket/public_url.w_compile_tf-aws.md index 160b2a08b37..e7d7857d6c0 100644 --- a/tools/hangar/__snapshots__/test_corpus/sdk_tests/bucket/public_url.w_compile_tf-aws.md +++ b/tools/hangar/__snapshots__/test_corpus/sdk_tests/bucket/public_url.w_compile_tf-aws.md @@ -168,6 +168,9 @@ module.exports = function({ $http_Util, $privateBucket, $publicBucket, $util_Uti } }, "bucket": "${aws_s3_bucket.publicBucket.bucket}", + "depends_on": [ + "aws_s3_bucket_public_access_block.publicBucket_PublicAccessBlock_54D9EFBA" + ], "policy": "{\"Version\":\"2012-10-17\",\"Statement\":[{\"Effect\":\"Allow\",\"Principal\":\"*\",\"Action\":[\"s3:GetObject\"],\"Resource\":[\"${aws_s3_bucket.publicBucket.arn}/*\"]}]}" } }, diff --git a/tools/hangar/__snapshots__/test_corpus/sdk_tests/website/website.w_compile_tf-aws.md b/tools/hangar/__snapshots__/test_corpus/sdk_tests/website/website.w_compile_tf-aws.md index 9c30166d769..ad6fe59164e 100644 --- a/tools/hangar/__snapshots__/test_corpus/sdk_tests/website/website.w_compile_tf-aws.md +++ b/tools/hangar/__snapshots__/test_corpus/sdk_tests/website/website.w_compile_tf-aws.md @@ -163,7 +163,7 @@ module.exports = function({ }) { "variables": { "WING_FUNCTION_NAME": "Handler-c867c4e0", "WING_TARGET": "tf-aws", - "WING_TOKEN_TFTOKEN_TOKEN_11": "${jsonencode(aws_cloudfront_distribution.cloudWebsite_Distribution_083B5AF9.domain_name)}" + "WING_TOKEN_TFTOKEN_TOKEN_12": "${jsonencode(aws_cloudfront_distribution.cloudWebsite_Distribution_083B5AF9.domain_name)}" } }, "function_name": "Handler-c867c4e0", @@ -210,6 +210,9 @@ module.exports = function({ }) { } }, "bucket": "${aws_s3_bucket.cloudWebsite_WebsiteBucket_EB03D355.bucket}", + "depends_on": [ + "aws_s3_bucket_public_access_block.cloudWebsite_PublicAccessBlock_18A70311" + ], "policy": "{\"Version\":\"2012-10-17\",\"Statement\":[{\"Effect\":\"Allow\",\"Principal\":\"*\",\"Action\":[\"s3:GetObject\"],\"Resource\":[\"${aws_s3_bucket.cloudWebsite_WebsiteBucket_EB03D355.arn}/*\"]}]}" } }, diff --git a/tools/hangar/__snapshots__/test_corpus/valid/captures.w_compile_tf-aws.md b/tools/hangar/__snapshots__/test_corpus/valid/captures.w_compile_tf-aws.md index f6aacc645c8..318cc8a2b27 100644 --- a/tools/hangar/__snapshots__/test_corpus/valid/captures.w_compile_tf-aws.md +++ b/tools/hangar/__snapshots__/test_corpus/valid/captures.w_compile_tf-aws.md @@ -307,6 +307,9 @@ module.exports = function({ $bucket1, $bucket2, $bucket3 }) { } }, "bucket": "${aws_s3_bucket.PublicBucket.bucket}", + "depends_on": [ + "aws_s3_bucket_public_access_block.PublicBucket_PublicAccessBlock_4FE1A1A3" + ], "policy": "{\"Version\":\"2012-10-17\",\"Statement\":[{\"Effect\":\"Allow\",\"Principal\":\"*\",\"Action\":[\"s3:GetObject\"],\"Resource\":[\"${aws_s3_bucket.PublicBucket.arn}/*\"]}]}" } }, diff --git a/tools/hangar/__snapshots__/test_corpus/valid/website_with_api.w_compile_tf-aws.md b/tools/hangar/__snapshots__/test_corpus/valid/website_with_api.w_compile_tf-aws.md index 04ec4ac3320..1ef05ff0cd8 100644 --- a/tools/hangar/__snapshots__/test_corpus/valid/website_with_api.w_compile_tf-aws.md +++ b/tools/hangar/__snapshots__/test_corpus/valid/website_with_api.w_compile_tf-aws.md @@ -126,7 +126,7 @@ module.exports = function({ }) { }, "rest_api_id": "${aws_api_gateway_rest_api.cloudApi_api_2B334D75.id}", "triggers": { - "redeployment": "67854313abc040abe4f906782a46948864d75773" + "redeployment": "c46535d385dc86a1ba350f23ab28b58821f513fc" } } }, @@ -474,6 +474,9 @@ module.exports = function({ }) { } }, "bucket": "${aws_s3_bucket.cloudWebsite_WebsiteBucket_EB03D355.bucket}", + "depends_on": [ + "aws_s3_bucket_public_access_block.cloudWebsite_PublicAccessBlock_18A70311" + ], "policy": "{\"Version\":\"2012-10-17\",\"Statement\":[{\"Effect\":\"Allow\",\"Principal\":\"*\",\"Action\":[\"s3:GetObject\"],\"Resource\":[\"${aws_s3_bucket.cloudWebsite_WebsiteBucket_EB03D355.arn}/*\"]}]}" } },