-
Notifications
You must be signed in to change notification settings - Fork 3.9k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
s3.Bucket: CDK Deploy: s3:PutBucketPolicy Access Denied #25983
Comments
Yes this makes sense to me. We should improve the user experience in this use case. I am making it a feature request and any pull request would be highly welcome and appreciated. |
Not saying I will have capacity to look at implementing this, but a few initial code references/exploration for future me, or anyone who feels like improving this DX. From #25358
Looking at the CHANGELOG v2
Some relevant references/snippets from those issues/PRs:
In #25303, changes were made to:
In #25298, changes were made to:
Based on the above exploration, I think looking at
Without thinking about it too deeply, I think it might be relevant to add another check within this function (similar to I haven't looked too deeply to see if there are other cases throughout the CDK where there are other features that would cause this to need to be configured. I also feel like we could potentially do something useful with the TypeScript types to pre-emptively inform the user that they are trying to do an invalid thing if they try and explicitly set
|
I don't think there is a great way to do this, usually we do this type of mutual exclusivity by manually detecting conflicting values and then throwing an error. That being said.
Should we detect of |
@MrArnoldPalmer I think handling it for the user if they only specified BUT, with the caveat that if they explicitly try to set the other 1-2 related properties to an invalid value (thus overriding the defaults), I would expect it to have a runtime error (which is in line with the existing patterns (Ref)), but also to give that guidance to me at 'dev time' through the TypeScript types. |
|
Following up my above comment, I actually tested all of the various
|
For anybody who finds this issue, here what worked for us with cdk 2.83.1:
Example bucket.grantWrite(workerNodejsFunction);
bucket.grantPutAcl(workerNodejsFunction);
Example this.publicAssetsBucket = new s3.Bucket(
this,
"myBucket",
{
blockPublicAccess: new s3.BlockPublicAccess({
blockPublicAcls: false,
blockPublicPolicy: false,
ignorePublicAcls: false,
restrictPublicBuckets: false,
}),
objectOwnership: s3.ObjectOwnership.OBJECT_WRITER,
...
}
); |
Screen.Recording.2023-07-28.at.04.21.28.movI've also encountered this issue. I believe we should alert the user to set |
So we used to create a bucket like this: s3.Bucket(
self, "Bucket",
public_read_access=True,
enforce_ssl=True,
removal_policy=RemovalPolicy.DESTROY,
) After discussing this with AWS support, the correct way to create a bucket with the same permissions now, is: s3.Bucket(
self, "Bucket",
public_read_access=True,
block_public_access=s3.BlockPublicAccess(
block_public_acls=False,
block_public_policy=False,
ignore_public_acls=False,
restrict_public_buckets=False,
),
object_ownership=s3.ObjectOwnership.OBJECT_WRITER,
enforce_ssl=True,
removal_policy=RemovalPolicy.DESTROY,
) |
Thanks @rectalogic ! But this is ugly! the whole point of using CDk is that there are sensible defaults for all of these permissions. Ether make this stuff default when |
I am still unable to get this to work unless I disable account-level public access block, and I'm not sure I really want to do that. I've tried various of the above permutations, including trying to append a bucket policy after bucket creation in the code, but I keep getting access denied errors at the bucket policy creation point. I would be interested to know if those that got it to work had disabled the account-level block on S3 buckets or not. |
jesus. I just wanted to make a static web page hosted on an s3 bucket. Thank you everyone for your help. |
in case someone else wastes a full day due to this change: adding const websiteBucket = new s3.Bucket(this, "WebsiteBucket", {
websiteIndexDocument: "index.html",
publicReadAccess: true,
blockPublicAccess: s3.BlockPublicAccess.BLOCK_ACLS,
});
would be great if someone at aws would update the docs: |
Describe the bug
When creating a new S3 bucket with
publicReadAccess
set totrue
:Since the new AWS defaults on public buckets:
Attempting to deploy the stack will fail with an error like this:
Expected Behavior
Given this seems to be incompatible with the new defaults on AWS, I would expect that this should do some measure of:
Current Behavior
See above
Reproduction Steps
Deploy a stack with a new S3 Bucket with
publicReadAccess
set totrue
.Possible Solution
See above + the solutions given on the following:
Additional Information/Context
Both of these are the same issue, but those users closed them prematurely as 'user issues', when in fact the CDK should more explicitly and correctly handle this situation:
This Reddit thread also contains a number of people who hit this same issue:
This is also the same root issue I believe, yet it mistakenly claims that it should be fixed from
aws-cdk-lib
>=2.77.0
, but it seems to miss the edge case of thepublicReadAccess
key being set totrue
creating aAWS::S3::BucketPolicy
:You can also see my full original notes on this comment:
These sections of the docs may also be relevant:
CDK CLI Version
2.84.0
Framework Version
2.84.0
Node.js Version
v16.15.1
OS
macOS Ventura 13.3.1
Language
Typescript
Language Version
No response
Other information
N/A
The text was updated successfully, but these errors were encountered: