-
Notifications
You must be signed in to change notification settings - Fork 4k
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
feat(cloudfront-origins): list access level for 404 response #32059
base: main
Are you sure you want to change the base?
Conversation
if (actions.some(({ bucketArn }) => bucketArn)) { | ||
resources.push(this.bucket.bucketArn); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you explain this part please?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
s3:ListBucket action requires bucket ARN in resources
(instead of object ARN required by s3:GetObject etc.)
see https://docs.aws.amazon.com/service-authorization/latest/reference/list_amazons3.html
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When READ and LIST are specified, actions
will be [{ action: 's3:GetObject' }, { action: 's3:ListBucket', bucketArn: true }]
.
The if condition returns whether actions
has one or more elements with truthy bucketArn
prop, so the result will be true. Then the the bucketArn will be pushed to resources
.
It is the reason why the object arn (arnForObjects('*')
) is always in resources that the LIST permission should be used with READ in most cases.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
updated names more descriptive.
It is highly recommended to specify `defaultRootObject` distribution property. | ||
Without it, the root path `https://xxxx.cloudfront.net/` will return the list of the S3 object keys. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm this raises some concern to me, can we make it so that defaultRootObject
MUST be specified when user adds LIST permission?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It should not be forced to specify defaultRootObject
because origins associated to non-default behaviors do not require it. We can add a warning annotation instead.
For example:
new cloudfront.Distribution(this, 'Distribution', {
defaultBehavior: {
// the origin server will handle root path
// setting defaultRootObject will break the web application's router
origin: new origins.HttpOrigin('mywebserver.example.com'),
},
additionalBehaviors: {
// the bucket will not receive requests to the root path
'assets/*': {
origin: origins.S3BucketOrigin.withOriginAccessControl(bucket, {
originAccessLevels: [cloudfront.AccessLevel.READ, cloudfront.AccessLevel.LIST],
}),
},
},
});
@gracelu0 Thank you for your review! |
I created an example CDK project. The root path returns the list of objects in the bucket: A missing object returns 404: Regular objects return 200 and their contents: A folder created by S3 console returns 200 and empty content (content-type: application/x-directory): Without a folder, S3 returns 404: |
Pull request has been modified.
@@ -47,6 +47,10 @@ export enum AccessLevel { | |||
* Grants read permissions to CloudFront Distribution | |||
*/ | |||
READ = 'READ', | |||
/** | |||
* Grants list permissions to CloudFront Distribution |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed the previous comment in favor of the warning annotation.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #32059 +/- ##
==========================================
- Coverage 77.18% 77.17% -0.01%
==========================================
Files 105 105
Lines 7161 7169 +8
Branches 1312 1315 +3
==========================================
+ Hits 5527 5533 +6
- Misses 1454 1455 +1
- Partials 180 181 +1
Flags with carried forward coverage won't be shown. Click here to find out more.
|
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
Issue # (if applicable)
Closes #13983.
Closes #31689.
Reason for this change
When we want to receive HTTP 404 response where the requested object does not exist,
s3:ListBucket permission is needed in the S3 bucket policy.
Unlike
errorResponses
to convert 403 response to 404, This is useful to distinguish between responses blocked by WAF (403) and responses where the file does not exist (404).Description of changes
Added a new
AccessLevel.LIST
to allow s3:ListBucket.Description of how you validated changes
Unit test and integration test. The integ test also tests the response is 404.
Checklist
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license