aws-cdk-lib/aws_s3: bucket.grantReadWrite missing PutObject* actions

[shishkin]

What is the problem?

bucket.grantReadWrite(lambda) misses additional PutObject* actions, probably PutObjectTagging or PutObjectAcl. Issue is fixed by manually adding * after PutObject in the policy actions list.

Reproduction Steps

1. create bucket with CDK
2. create lambda with CDK
3. call bucket.grantReadWrite(lambda) with CDK
4. deploy
5. invoke lambda that does PutObject with tags

What did you expect to happen?

If I grant lambda read and write I expect it to be able to write objects into S3

What actually happened?

AccessDenied from S3

CDK CLI Version

2.8.0

Framework Version

No response

Node.js Version

14

OS

MacOS

Language

Typescript

Language Version

No response

Other information

No response

[daniel-gato]

Having the same issue.

Updated from @aws-cdk/core: 1.138.0 to aws-cdk-lib: 2.7.0 and this used to work:

new s3deploy.BucketDeployment(this, 'DeployDemoTxtFiles', {
  sources: [s3deploy.Source.asset('./src/deployments/demo/')],
  exclude: ['*'],
  include: ['*.txt'],
  destinationKeyPrefix: 'public/demo/',
  destinationBucket: bucket,
  accessControl: 'PublicRead',
  contentType: 'plain/text',
});

Now we get “access denied” during deployment. Cloudwatch logs show the Lambda function does not have PutObject rights.

[shishkin]

Apparently this is partially intentional that grantReadWrite doesn't allow all PutObject* actions: https://github.com/aws/aws-cdk/blob/master/packages/@aws-cdk/aws-s3/lib/bucket.ts#L232-L243.

Before CDK version 1.85.0, this method granted the s3:PutObject* permission that included s3:PutObjectAcl, which could be used to grant read/write object access to IAM principals in other accounts.

But there are few more PutObject* actions besides PutObjectAcl that were removed: https://docs.aws.amazon.com/AmazonS3/latest/userguide/list_amazons3.html#amazons3-actions-as-permissions.

And AWS IAM being a horrible trial-and-error mess, it's hard to figure out in advance what policy each API call needs :(

[peterwoodworth]

This merge is likely the source of the error: #12391

The change in functionality should only be seen in newly created CDK projects, since the feature is gated behind a feature flag

aws-cdk/packages/@aws-cdk/cx-api/lib/features.ts

Lines 83 to 90 in 9940952

	/**
	* Change the old 's3:PutObject*' permission to 's3:PutObject' on Bucket,
	* as the former includes 's3:PutObjectAcl',
	* which could be used to grant read/write object access to IAM principals in other accounts.
	* Use a feature flag to make sure existing customers who might be relying
	* on the overly-broad permissions are not broken.
	*/
	export const S3_GRANT_WRITE_WITHOUT_ACL = '@aws-cdk/aws-s3:grantWriteWithoutAcl';

@shishkin @daniel-gato you're likely seeing the error since you've migrated to V2 which has all feature flags enabled by default. Specifically include this feature flag to disable it 🙂

[shishkin]

@peterwoodworth Thanks for pointing to the culprit, I think you're right. Just to mention that PutObjectAcl is not the only removed action. Another relevant action is PutObjectTagging. This is my lambda code that fails with AccessDenied:

  await s3.send(
    new PutObjectCommand({
      Bucket: BUCKET,
      Key: `path/to/some/file.html`,
      Tagging: new URLSearchParams({
        foo: "bar",
      }).toString(),
      ContentType: "text/html; charset=UTF-8",
      Body: html,
    })
  );

[peterwoodworth]

PutObjectAcl was intentionally removed as it can potentially lead to granting unwanted permissions to individual objects. So the user will have to go out of their way to re-enable this

As for PutObjectTagging, I don't think there's a problem with that. Should be a pretty easy PR 🙂

[peterwoodworth]

Looks like that's already been done actually - #18494 specifically includes PutObjectTagging

Is there anything else regarding this you need help with?

[shishkin]

Very cool! Look forward for this to be released 😀

[peterwoodworth]

I'd expect the next release by the end of next week 🙂

[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.

[daniel-gato]

@peterwoodworth Ok, thank you for the clarification. I'm very new to CDK, when you say Specifically include this feature flag to disable it 🙂: I'm not sure what to add where here. Are we speaking about the lags from your link?

[daniel-gato]

I tried to add the flag: "@aws-cdk/aws-s3:grantWriteWithoutAcl": false and I get Error: Unsupported feature flag '@aws-cdk/aws-s3:grantWriteWithoutAcl'. This flag existed on CDKv1 but has been removed in CDKv2. CDK will now behave as the same as when the flag is enabled.

Edit, while digging in the the code of my node_modules I can see this:

    /**
     * Grant the given IAM identity permissions to modify the ACLs of objects in the given Bucket.
     *
     * If your application has the '@aws-cdk/aws-s3:grantWriteWithoutAcl' feature flag set,
     * calling {@link grantWrite} or {@link grantReadWrite} no longer grants permissions to modify the ACLs of the objects;
     * in this case, if you need to modify object ACLs, call this method explicitly.
     *
     * @param identity The principal.
     * @param objectsKeyPattern Restrict the permission to a certain key pattern (default '*').
     * @stability stable
     */
    grantPutAcl(identity: iam.IGrantable, objectsKeyPattern?: string): iam.Grant;

The only sure I'm not sure now is how to retrieve the principal of the BucketDeployment to pass it to this function on my bucket.

[peterwoodworth]

@daniel-gato I'm sorry, I hadn't realized that the feature flags in v2 are only the specific ones documented in this link, and no other feature flags are toggleable. My mistake!

Under the hood, the BucketDeployment construct creates both a lambda function and a custom resource. Both of which are written by the CDK team. So, this construct works fine for me normally on the current version. Maybe you're getting the error from somewhere else? What other infrastructure do you have in your code?