cloudfront-origins: OAI not granted to S3 bucket.fromBucketName

rantoniuk commented 1 month ago

Describe the bug

The code below deploy correctly but OAI access is not granted for S3 bucket and when I try to access objects, I get Access Denied errors via Cloudfront.

The bucket is fully private. Account level block for S3 public access is also ON.

Just to experiment, I commented out the grantRead(cloudfrontOAI) line and the diff shows nothing.

In the console, for Cloudfront origin, I see the following was configured:

Legacy access identities -> Allow Cloudfront to access the bucket -> No, I will update the bucket policy

but it doesn't seem the grant was done anywhere.


    const anotherStackBucket = new s3.Bucket(this, 'S3AssetsBucket', {
      bucketName: `test-${cdk.Aws.ACCOUNT_ID}`,
      blockPublicAccess: s3.BlockPublicAccess.BLOCK_ALL,
    });

    const bucket = Bucket.fromBucketName(this, 'S3AssetsBucket', `test-${cdk.Aws.ACCOUNT_ID}`);

    const cloudfrontOAI = new OriginAccessIdentity(this, 'AssetsOAI');

    // this line does nothing
    bucket.grantRead(cloudfrontOAI);

    new cloudfront.Distribution(this, 'AssetsDistribution', {
      defaultBehavior: {
        origin: new origins.S3Origin(bucket, { originAccessIdentity: cloudfrontOAI }),
        viewerProtocolPolicy: cloudfront.ViewerProtocolPolicy.REDIRECT_TO_HTTPS,
        allowedMethods: cloudfront.AllowedMethods.ALLOW_GET_HEAD,
        cachedMethods: cloudfront.CachedMethods.CACHE_GET_HEAD,
      },
      priceClass: cloudfront.PriceClass.PRICE_CLASS_100,
    });

Expected Behavior

S3 OAI is granted.

Current Behavior

Not granted

Reproduction Steps

as above.

Possible Solution

No response

Additional Information/Context

No response

CDK CLI Version

2.150.0 (build 3f93027)

Framework Version

No response

Node.js Version

v20.13.1

OS

MacOS

Language

TypeScript

Language Version

No response

Other information

ref: https://github.com/aws/aws-cdk/issues/24763 ref: https://github.com/aws/aws-cdk/issues/9859

rantoniuk commented 1 month ago

As expected, this workaround works:


export class CloudfrontStack extends cdk.Stack {
  readonly cloudfrontOAI: cloudfront.IOriginAccessIdentity;

  constructor(scope: Construct, id: string, props: StackProps) {
    super(scope, id, props);

    this.cloudfrontOAI = new OriginAccessIdentity(this, 'AssetsOAI');
...

and then in the bucket stack:

    bucket.grantRead(props.assetsCfOai);

but the original version should work as well.

khushail commented 1 month ago

@rantoniuk , thanks for reporting this. When existing bucket is referenced using fromBucketName(), only 'GetObject` access is added, out of these 3-

            [-]     "Action": [
            [-]       "s3:GetBucket*",
            [-]       "s3:GetObject*",
            [-]       "s3:List*"

As mentioned in this CDK Document, ,only 'gretObject' policy access is added -

If the bucket is configured as a website, the bucket is treated as an HTTP origin, and the built-in S3 redirects and error pages can be used. Otherwise, the bucket is handled as a bucket origin and CloudFront's redirect and error handling will be used. In the latter case, the Origin will create an origin access identity and grant it access to the underlying bucket. This can be used in conjunction with a bucket that is not public to require that your users access your content using CloudFront URLs and not S3 URLs directly.

 This is general behavior in the CDK for imported buckets -- and most other imported resources; in this case, since the bucket is imported, we can't know if there is an existing bucket policy or not and whether a new policy can (or should) be created, so the grant* methods are no-ops.

Note that the OAI and bucket policy are only necessary if the bucket policy/permissions otherwise restrict access to the bucket such that CloudFront wouldn't be able to access the bucket without them. If the bucket is public (for example), then the OAI is redundant.

Code that added the policy -

https://github.com/aws/aws-cdk/blob/9752ed225c82982d8b443f7c6a47f47979458217/packages/%40aws-cdk/aws-cloudfront-origins/lib/s3-origin.ts#L81

Hope that is helpful. Please feel free to reach out if there are more questions. Thanks.

rantoniuk commented 1 month ago

I'm sorry, but I'm missing what you tried to explain with this. To reiterate:

this is a private bucket
so website hosting is of course off

I know about the CDK limitation for handling existing resources. What I do not accept is the fact that this is an issue for years that is silently hidden by CDK when deployment happens. I would add least expect a warning message saying: I don't know what policies should be added to your existing bucket, so this is a no-op.

Note that the OAI and bucket policy are only necessary if the bucket policy/permissions otherwise restrict access to the bucket such that CloudFront wouldn't be able to access the bucket without them. If the bucket is public (for example), then the OAI is redundant.

This is kinda ridiculous, since public buckets are discouraged and Cloudfront + OAI S3 Origin is the exact solution that should be used instead. I also described in a simple workaround, couldn't this approach be used by default via exports just like for other resources/dependencies?

If not, I would propose to consider creating another interface that would not allow passing an imported resource.

khushail commented 1 month ago

@rantoniuk ,I totally understand that the error message or some warning should be displayed and the bucket policies should be clearly stated. I would bring this to team's attention. thanks for having patience and reporting this.

pahud commented 1 month ago

Hi @rantoniuk

Adding a warning or annotation is a great idea to me off the top of my head and it should be a very easy PR to address.

Are you willing to submit a PR for that? The team would be happy review that and get it resolved when it's ready.

I actually have consolidated a few great resources and videos on PR contribution in community.aws(check out this article). Is this something you'd like to pick up with?

rantoniuk commented 1 month ago

Still waiting to hear why the solution I used as a workaround in here cannot be used via exports just like with other resources.

pahud commented 1 month ago

@rantoniuk

Let me explain this way.

When configuring S3 bucket with CloudFront OAI. One important requirement is the S3 Bucket should add a Bucket Policy like this:

{
    "Version": "2012-10-17",
    "Id": "PolicyForCloudFrontPrivateContent",
    "Statement": [
        {
            "Effect": "Allow",
            "Principal": {
                "AWS": "arn:aws:iam::cloudfront:user/CloudFront Origin Access Identity YOUR_OAI_ID"
            },
            "Action": "s3:GetObject",
            "Resource": "arn:aws:s3:::YOUR_BUCKET_NAME/*"
        }
    ]
}

In CDK, to add s3 bucket policy, we have two options:

Using bucket.addToResourcePolicy() as described here to add a policy statement to the bucket policy, but you need to note this may or may not add the statement to the policy depends on if you are adding that to a concrete Bucket or IBucket interface. Check this for details:

https://github.com/aws/aws-cdk/blob/88b1e1eb09cd37057295fced54229edd51ce62a7/packages/aws-cdk-lib/aws-s3/lib/bucket.ts#L98-L118

explicitly create a s3.BucketPolicy for a bucket but you need to make sure a bucket can only have one bucket policy.

Speaking of the resource.grantRead(principal) pattern, what's happening under the hood is:

The principal should add relevant read permission to its principal policies(if it's an IAM principal).
the resource should add relevant statement in its resource policies(if it has)

Now, let me explain what happens when you

bucket.grantRead(cloudfrontOAI);

It essentially invoke addToPrincipalOrResource(), which would 1)add permissions to the principal policies primarily, 2)falling back to the resource policy if necessary. The permissions must be granted somewhere. (Per described here)

Let's dive into the two steps here: 1) add permissions to the principal policies primarily - the OAI is actually not an IAM principal hence no principal policies to add. 2) falling back to the resource policy if necessary - when this happens, it tries to addToResourcePolicy per source here. As we described above, bucket.addToResourcePolicy() would not work when the bucket is a IBucket interface. It only works when bucket is a concrete s3.Bucket.

In your original example:

const bucket = Bucket.fromBucketName(this, 'S3AssetsBucket', `test-${cdk.Aws.ACCOUNT_ID}`);

fromBucketName actually returns IBucket interface, hence both two steps would not change anything.

In your 2nd example:

and then in the bucket stack:

    bucket.grantRead(props.assetsCfOai);

Looks like you create a concrete Bucket in the "bucket stack" and execute bucket.grantRead(). If that's the case, given bucket here is a concrete bucket then step 2) works! The bucket policy would be updated with required statement.

why the solution I used https://github.com/aws/aws-cdk/issues/30953#issuecomment-2251353763 cannot be used via exports just like with other resources.

cross-stack export would not give you concrete Bucket on the referencing stack but IBucket only. Hence export would not work either.

I hope you find this useful and my links help you trace all the relevant source code.

I agree we should add an annotation on the addToPrincipalOrResource() method, when 1) and 2) both don't match, it should throw some warning messages. Another point we could improvement is to add some notes in the aws-s3 README for OAI and clarify bucket.grantRead() would require the bucket to be a concret s3.Bucket rather than IBucket interface.

I would strongly recommend a pull request to help us improve either adding an annotation or just add notes in the aws-s3 OAI document.

rantoniuk commented 3 weeks ago

I agree we should add an annotation on the addToPrincipalOrResource() method, when 1) and 2) both don't match, it should throw some warning messages. Another point we could improvement is to add some notes in the aws-s3 README for OAI and clarify bucket.grantRead() would require the bucket to be a concret s3.Bucket rather than IBucket interface.

I would strongly recommend a pull request to help us improve either adding an annotation or just add notes in the aws-s3 OAI document.

How about this: If the object is an Interface only (i.e. IBucket, not Bucket), then it should not expose any operation methods. What I mean here is I want a TypeScript validation error to prevent this from syntax validation point of view, rather than seeing a warning during deployment time.

In other words, Bucket.fromBucketName should return a different type of object than new Bucket - and it does, because one returns an interface, the other a concrete object.

The problem here is that the affected operational methods, i.e. grantRead() are defined on the Interface level and as we explained above, they should be only defined on the class level. This way they would be invisible when used on IBucket objects.

aws / aws-cdk