Skip to content

Conversation

@sicarul
Copy link
Contributor

@sicarul sicarul commented Oct 21, 2025

This PR sets up the S3 bucket as described in https://docs.aws.amazon.com/AmazonCloudFront/latest/DeveloperGuide/private-content-restricting-access-to-s3.html

With this merged non-existent paths will return 404 instead of 403, e.g.:

Current behavior:

image

After this change:

image

@claude
Copy link
Contributor

claude bot commented Oct 21, 2025

Code Review

Summary

This PR correctly modernizes the CloudFront-to-S3 access control by migrating from the legacy Origin Access Identity (OAI) to the recommended Origin Access Control (OAC) approach. The changes follow AWS best practices as documented.

Issues Found

1. Condition Key Capitalization (Line 820)

"AWS:SourceArn": distributionArn,

AWS IAM condition keys are case-sensitive and typically use lowercase. This should likely be:

"aws:SourceArn": distributionArn,

Please verify against AWS CloudFront OAC documentation to confirm the correct capitalization.

2. Policy Version Change (Line 789)

The IAM policy version changed from "2008-10-17" to "2012-10-17". While 2012-10-17 is the current version and generally recommended, confirm this change is intentional and doesn't affect any existing policy behavior.

3. Missing Deployment Guidance

This is a significant infrastructure change that affects how CloudFront accesses S3. Consider documenting:

  • Whether existing deployments require manual intervention
  • If there's a deployment strategy to avoid service interruption
  • Any rollback procedures if issues arise

Suggestions

Add explanatory comment (Line 808):
Consider adding a brief comment explaining the OAC choice for future maintainers:

// Configure bucket policy to allow CloudFront OAC access and deny direct access
// OAC (Origin Access Control) is the modern replacement for OAI (Origin Access Identity)
// and provides improved security with support for all S3 bucket configurations
// This must be created after the distribution so we can reference its ARN

Positive Aspects

  • ✅ Correct use of OAC, following current AWS best practices
  • ✅ Proper bucket policy with CloudFront service principal
  • ✅ Correct switch from websiteEndpoint to bucketRegionalDomainName
  • ✅ Proper resource ordering (policy created after distribution)
  • ✅ Maintains existing DenyDirectListBucket security rule

Mention @claude if you'd like me to review again after making changes or if you have questions about these findings.

@pulumi-bot
Copy link
Collaborator

@pulumi-bot
Copy link
Collaborator

@pulumi-bot
Copy link
Collaborator

@pulumi-bot
Copy link
Collaborator

@pulumi-bot
Copy link
Collaborator

Copy link
Contributor

@CamSoper CamSoper left a comment

Choose a reason for hiding this comment

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

I don't have any feedback of note. Thank you for doing this!

@pulumi-bot
Copy link
Collaborator

@sicarul sicarul enabled auto-merge (squash) October 22, 2025 19:44
@pulumi-bot
Copy link
Collaborator

@sicarul sicarul merged commit b02f772 into master Oct 22, 2025
8 checks passed
@sicarul sicarul deleted the setup-oac-for-cloudfront branch October 22, 2025 19:53
sicarul added a commit that referenced this pull request Oct 22, 2025
sicarul added a commit that referenced this pull request Oct 22, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants