S3 Bucket Policy Changes Not Recognized As A Change on CDK Deploy

[bensoer]

When making changes to a bucket policy from a pre-existing bucket, applying changes to its Policy are not applied. The CDK seems to act as if no changes are needed

Reproduction Steps

Note that I have changed the names of things in this example to simplify and avoid disclosing

Adding the following code to my application to edit a pre-existing bucket's bucket policy so that other resources may get to it which may or may not have been created with the CDK

const myPreExistingBucket = s3.Bucket.fromBucketName(this, 'MyPreExistingBucket-Lookup-ID', "mypreexistingbucket");
myPreExistingBucket.addToResourcePolicy(new iam.PolicyStatement({
            actions:[
                "s3:*"
            ],
            resources:[
                "arn:aws:s3:::mypreexistingbucket",
                "arn:aws:s3:::mypreexistingbucket/*"
            ],
            principals:[
                new iam.AccountPrincipal("arn:aws:iam::XXXXXXXXXXXX:root")
            ]
        }));

Then deploy with the CDK:
cdk -i --region us-east-1 --app 'npx --quiet ts-node app.ts' deploy --profile datascience

Error Log

Error message is not an error but a false positive in that there are no changes needing to be applied, when there are. Checking the account as well shows no updates in the cloud formation templates and the Bucket Policy not being applied to the Bucket

Environment

• CLI Version : Attempted with v1.26.0 and v1.18.0
• Framework Version: Nodejs - v12.16.1, NPM - v6.13.4
• OS : MAC OS Mojave
• Language : Typescript - v3.7.4

Other

---

This is 🐛 Bug Report

[iliapolo]

Hi @bensoer

This is actually the desired behavior. Imported/Existing resources are not strictly speaking a part of the CloudFormation stack. Notice that if you simply do:

s3.Bucket.fromBucketName(this, 'MyPreExistingBucket-Lookup-ID', 
  "mypreexistingbucket");

you will get an empty stack. Existing resources can be used for referencing by other stack resources, but they cannot be mutated. Since this might break its previous, unmanaged consumers. Having said that, we acknowledge the use case for this, and actually CloudFormation have added support for explicitly importing existing resources, but this isn't yet supported in the CDK.

There are ways around this, like using the AwsCustomResource construct.

We encourage you to open a feature request that details your use case :)

[BenoitDel]

If i am not mistaken, the ressource mutation fails silently. IMHO this should trigger en error message

[iliapolo]

@BenoitDel You're right. Actually, the correct behavior would be to not even expose the addToResourcePolicy method in this case.

However, i want to make a small correction to a statement i previously made:

Existing resources can be used for referencing by other stack resources, but they cannot be mutated.

Thats not always the case, it actually depends on CloudFormation capabilities, but in principal, CDK does not enforce this (i.e, some imported resources can be updated).

So in reality, we should either properly support this scenario (in which case, this is just a bug), or, if this scenario is not supported, we should hide the addToResourcePolicy method altogether.

Thanks for reporting this, we will take a look.

[skinny85]

So in reality, we should either properly support this scenario (in which case, this is just a bug), or, if this scenario is not supported, we should hide the addToResourcePolicy method altogether.

Remember that a Bucket is also an IBucket, which means just having an IBucket, we don't know whether it's mutable or not. In many cases, it makes sense to have a method like addToResourcePolicy on IBucket, even if an imported bucket doesn't support it. One of the patterns we use is to return a boolean value indicating whether a change was actually made.

This will become even more important, as we move to our improved CfnInclude experience (#7304), and we will want to have an L1-to-L2 transition, which will result in mutable IBuckets.

[iliapolo]

@skinny85 Do you think it makes sense to specifically have addToResourcePolicy on IBucket at the moment? even though always returns statementAdded: false?

[skinny85]

@skinny85 Do you think it makes sense to specifically have addToResourcePolicy on IBucket at the moment? even though always returns statementAdded: false?

Yes. Not sure what you mean by "it always returns statementAdded: false"...? It doesn't:

aws-cdk/packages/@aws-cdk/aws-s3/lib/bucket.ts

Line 459 in f5dbed6

return { statementAdded: true, policyDependable: this.policy };

[iliapolo]

@skinny85 Notice that the full code reads:

aws-cdk/packages/@aws-cdk/aws-s3/lib/bucket.ts

Lines 452 to 462 in 745922a

	public addToResourcePolicy(permission: iam.PolicyStatement): iam.AddToResourcePolicyResult {
	if (!this.policy && this.autoCreatePolicy) {
	this.policy = new BucketPolicy(this, 'Policy', { bucket: this });
	}
	if (this.policy) {
	this.policy.document.addStatements(permission);
	return { statementAdded: true, policyDependable: this.policy };
	}
	return { statementAdded: false };

When the bucket is an Import, policy is undefined, and autoCreatePolicy is always false:

aws-cdk/packages/@aws-cdk/aws-s3/lib/bucket.ts

Lines 1155 to 1156 in 745922a

	public policy?: BucketPolicy = undefined;
	protected autoCreatePolicy = false;

So we end up here:

aws-cdk/packages/@aws-cdk/aws-s3/lib/bucket.ts

Line 462 in 745922a

return { statementAdded: false };

[skinny85]

That is true that today, but might not be in the future. For example, we might have a a fromCfnBucket(): IBucket in the future (#9719) that returns a mutable implementation of IBucket.

[philhorrocks]

just experienced this issue. An error would be nice as cdk fails silently :(

[ajhool]

The silent failure made this issue difficult to identify, so +1 on that.

There does seem to be a workaround for this specific goal assuming you want to CREATE a new bucket policy on an existing bucket and don't mind overwriting a bucket policy that already exists.

1. The bucket policy on the target s3 bucket was empty for me.
2. The "BucketPolicy" resource in cloudformation is separate from the Bucket resource, so I hoped that it could be managed independently (eg. the "bucket.grantRead" policy could have an overrideExistingBucketPolicy flag). That doesn't seem to be the case with the aws-s3.BucketPolicy resource.

However, there seems to be a different resource type named CfnBucketPolicy. I was able to create a CfnBucketPolicy that puts the BucketPolicy on an existing bucket. My goal was to attach a cloudfront origin access identity, but you can use a different PolicyStatement to grant access to whatever resource you want.

   // hastily copied the imports over, so some might be missing, sorry!
  import { Bucket, CfnBucketPolicy, IBucket } from '@aws-cdk/aws-s3';
  import { OriginAccessIdentity } from '@aws-cdk/aws-cloudfront';
  import { S3Origin } from '@aws-cdk/aws-cloudfront-origins'
  import { PolicyDocument, PolicyStatement } from '@aws-cdk/aws-iam';
  ...

  const existingBucketName = 'asfhaosfhasofhaowhfoawfhawf'

    const existingBucket = Bucket.fromBucketAttributes(this, 'ExistingBucket', {
      bucketArn: `arn:aws:s3:::${existingBucketName}`,
    });

    const originAccessIdentity = new OriginAccessIdentity(this, 'originAccessIdentity', {
      comment: `CloudFront access to ${existingBucketName}`,
    });

    // See: https://github.com/aws/aws-cdk/issues/941
    const policyStatement = new PolicyStatement();
    policyStatement.addActions('s3:GetBucket*');
    policyStatement.addActions('s3:GetObject*');
    policyStatement.addActions('s3:List*');
    policyStatement.addResources(existingBucket.bucketArn);
    policyStatement.addResources(`${existingBucket.bucketArn}/*`);
    policyStatement.addCanonicalUserPrincipal(originAccessIdentity.cloudFrontOriginAccessIdentityS3CanonicalUserId);

    const bucketPolicy = new CfnBucketPolicy(this, 'cloudfrontAccessBucketPolicy', {
      bucket: existingBucket.bucketName,
      policyDocument: new PolicyDocument({
        statements: [
          policyStatement
        ]
      })
    });

Disclaimer, I'm not sure how CfnBucketPolicy is different from BucketPolicy, so I'm a little suspicious of this approach... but it appears to be working. It would be nice to have a sanity check from the cdk team on this one

[iliapolo]

CfnBucketPolicy is the underlying resource implementation of BucketPolicy. See Using L1 constructs.

It's perfectly valid and safe to use. Though in this case BucketPolicy could also be used:

const bucketPolicy = new BucketPolicy(this, 'cloudfrontAccessBucketPolicy', {
  bucket: existingBucket,
})
bucketPolicy.document.addStatements(...)

[alex9311]

would love it if anyone can share a workaround that doesn't overwrite the bucket policy of the imported bucket. @ajhool's snippet works great apart from overwriting the existing policy (as they point out).

Is there any way to get the existing policies on an imported bucket? .policy on an IBucket is undefined, as @iliapolo points out above

[JamieMcKernanKaizen]

Yup, this also applies to imported KMS keys. I'm trying to grand a lambda function in Stack B permissions to decrypt a key on a bucket (both in Stack A) by importing the key from A into B and adding to the resource policy. But when printing a response I always get statementAdded: false.

Changing the allow/deny on the resource policy has no effect (CDK reports no changes) so yes, this silent failing is quite hard to notice and rather annoying.

[codeedog]

I was in what can best be described as psychic pain trying to get this working: having problems with CloudFront OAI to S3 restricted. The silently failing bit tripped me up for a while. Finally, noticed the failure to apply the policy response. When I found this thread.

I was all prepared to implement the @ajhool recommendation when I scrolled a bit further and saw the @iliapolo comment. It worked like a charm. Here's some code if anyone needs it:

const oai = new cloudfront.OriginAccessIdentity(this, "cf-oai", {
  comment: `Cloudfront to reach ${bucketName}`,
});

const policyStatement = new iam.PolicyStatement({
  actions:    [ 's3:GetObject' ],
  resources:  [ this.bucket.arnForObjects("*") ],
  principals: [ oai.grantPrincipal ],
});

const bucketPolicy = new s3.BucketPolicy(this, 'cloudfrontAccessBucketPolicy', {
  bucket: bucket,
})
bucketPolicy.document.addStatements(policyStatement);

[github-actions]

⚠️COMMENT VISIBILITY WARNING⚠️

Comments on closed issues are hard for our team to see.
If you need more assistance, please either tag a team member or open a new issue that references this one.
If you wish to keep having a conversation with other community members under this issue feel free to do so.

[omishagupta]

I'm kinda facing similar issue. Is there a way to append the statements to existing policy in the existing bucket? The solutions discussed here just overwrites the whole bucket policy.

[David-Tamrazov]

To anyone running into an Access Denied permissions error trying to hit their s3 bucket via Cloudfront using this fix, make sure pass in the origin access identity to which you granted bucket permissions to your S3 origin:

const oai = new cloudfront.OriginAccessIdentity(this, "cf-oai", {
  comment: `Cloudfront to reach ${bucketName}`,
});
...
const myS3Origin = new origins.S3Origin(myBucket, {
  originAccessIdentity: oai,
});

[asri6725]

@iliapolo - I was having a new issue, the stack was failing saying a policy already exists - new workarounds..?

const bucketPolicy = new BucketPolicy(this, 'cloudfrontAccessBucketPolicy', {
  bucket: existingBucket,
})
bucketPolicy.document.addStatements(...)

[ostrowr]

@otaviomacedo Thanks for the docs! But I'm not sure the linked issue should count to close this issue – having to check for a success value isn't very ergonomic and isn't obvious to an end user. (Obviously we should RTFM but as far as I know, checking for success isn't a pattern that is generally accepted or useful in CDK; we want things to yell at us if they aren't applied.)

[renatoargh]

As @asri6725 said a year ago, it is still throwing "policy already exists" error. Any new workarounds? I am about to attempt to use a custom resource to update the policy... But it's quite frustrating having to use a lambda & custom code just to do it 🤡 🤷‍♂️

[oappicgi]

Does this still work? I tried codeedog's example and I get no error, but bucketpolicy has not been applied to bucket defined in frombucketname import and otherwise set similarly to what codeedog has. Different policies, but it should throw error if policy is incorrect rather than silently not do anything?

I am also wondering if there is reason why addStatements is in plural form if it only accepts one item?