Skip to content

Commit

Permalink
fix(sdk): bucket needs to be marked as public before adding a public …
Browse files Browse the repository at this point in the history
…policy (#3536)

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](hashicorp/terraform-provider-aws#31363 (comment)) 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)*.
  • Loading branch information
skorfmann authored Jul 20, 2023
1 parent a60b937 commit ce24a6a
Show file tree
Hide file tree
Showing 7 changed files with 50 additions and 9 deletions.
19 changes: 12 additions & 7 deletions libs/wingsdk/src/target-tf-aws/bucket.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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: [
Expand All @@ -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", {
Expand Down
18 changes: 18 additions & 0 deletions libs/wingsdk/test/target-tf-aws/__snapshots__/bucket.test.ts.snap
Original file line number Diff line number Diff line change
Expand Up @@ -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}/*\\\\\\"]}]}\\"
}
},
Expand Down Expand Up @@ -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}/*\\\\\\"]}]}\\"
}
},
Expand Down Expand Up @@ -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}/*\\\\\\"]}]}\\"
}
},
Expand Down Expand Up @@ -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}/*\\\\\\"]}]}\\"
}
},
Expand Down Expand Up @@ -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}/*\\\\\\"]}]}\\"
}
},
Expand Down Expand Up @@ -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}/*\\\\\\"]}]}\\"
}
},
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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}/*\\\\\\"]}]}\\"
}
},
Expand Down Expand Up @@ -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}/*\\\\\\"]}]}\\"
}
},
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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}/*\"]}]}"
}
},
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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",
Expand Down Expand Up @@ -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}/*\"]}]}"
}
},
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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}/*\"]}]}"
}
},
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -126,7 +126,7 @@ module.exports = function({ }) {
},
"rest_api_id": "${aws_api_gateway_rest_api.cloudApi_api_2B334D75.id}",
"triggers": {
"redeployment": "67854313abc040abe4f906782a46948864d75773"
"redeployment": "c46535d385dc86a1ba350f23ab28b58821f513fc"
}
}
},
Expand Down Expand Up @@ -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}/*\"]}]}"
}
},
Expand Down

0 comments on commit ce24a6a

Please sign in to comment.