fix(s3): Bucket.grantWrite() no longer adds s3:PutObject* permission by skinny85 · Pull Request #12391 · aws/aws-cdk

skinny85 · 2021-01-07T02:08:00Z

Right now, Bucket.grantWrite() adds an 's3:PutObject*' permission to the passed principal.
That means it grants the 's3:PubObjectAcl' permission,
which allows changing the ACL of an individual object written to the Bucket,
potentially giving it more visibility than other objects in the Bucket.

Change that behavior to grant 's3:PutObject' instead.
Since customers might be relying on the old overly-broad permissions,
gate that change behind a feature flag,
which means only newly created CDK projects will get this behavior.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license

gitpod-io · 2021-01-07T02:08:04Z

rix0rrr · 2021-01-07T14:25:24Z

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

+ * Use a feature flag to make sure existing customers who might be relying
+ * on the overly-broad permissions are not broken.
+ */
+export const S3_BUCKET_GRANT_WRITE_ACL = '@aws-cdk/aws-s3:BucketGrantWriteAcl';


From the name this looks like it 'grants write acl', but in reality it seems to 'SKIP grants write acl'.

Can we name it something more clear like LimitGrantWrite ? (I'm willing to forego the word "Bucket" since we already scope this to S3).

Agreed. Changed to S3_GRANT_WRITE_WITHOUT_ACL / '@aws-cdk/aws-s3:grantWriteWithoutAcl', let me know if this is better!

rix0rrr · 2021-01-07T14:26:05Z

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

  [DOCKER_IGNORE_SUPPORT]: false,
  [SECRETS_MANAGER_PARSE_OWNED_SECRET_NAME]: false,
  [KMS_DEFAULT_KEY_POLICIES]: false,
+  [S3_BUCKET_GRANT_WRITE_ACL]: false,


Shouldn't the new project default and the V2 default be the same? We want this to become the standard behavior, yeah?

I thought FUTURE_FLAGS is the one that will be used for V2? And this is only used for the defaults?

Their JSDocs certainly seem to indicate that...

Yes but shouldn't the value be true then?

If you put it as true here, you will change the behavior to have s3:PutObject instead of s3:PutObject* if the feature flag is not explicitly set, kind of defeating the point of having the feature flag at all... (we risk breaking people that created their CDK project before this change was out, when they update to the newest version)

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

rix0rrr · 2021-01-07T14:37:43Z

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

   */
  public grantWrite(identity: iam.IGrantable, objectsKeyPattern: any = '*') {
-    return this.grant(identity, perms.BUCKET_WRITE_ACTIONS, perms.KEY_WRITE_ACTIONS,
+    return this.grant(identity, this.writeActions, perms.KEY_WRITE_ACTIONS,


I'm kinda expecting this change to break this:

aws-cdk/packages/@aws-cdk/aws-codepipeline-actions/lib/s3/deploy-action.ts

Line 112 in 49a9116

this.props.bucket.grantWrite(options.role);

if this field is set:

aws-cdk/packages/@aws-cdk/aws-codepipeline-actions/lib/s3/deploy-action.ts

Line 78 in 49a9116

readonly accessControl?: s3.BucketAccessControl;

I think it makes sense to add a method to explicitly opt in the old behavior, which we're going to need to use in at least the CodePipeline Action, and potentially other cases.

It's also a lot more user friendly, for everyone that intentionally needs PutObjectAcl. It's a much better advisory to say:

"Replace grantWrite with grantFullWrite, because security"

Than to say:

"Replace grantWrite with grant("s3:GetObject*", "s3:Get*", "s3:DeleteObject*", "s3:Abort*", "s3:PutObject*", ...), because security"

I don't disagree, but do you maybe want to wait for our users to let us know if it breaks, instead of doing it in this PR? What if you're wrong, and it works? 🙂

I'm proposing you test 😝

Did it for you (props to the people writing the error message here!):

Repro in case you need it:

import * as cpipe from '@aws-cdk/aws-codepipeline'; import { App, RemovalPolicy, SecretValue, Stack, StackProps } from '@aws-cdk/core'; import { Construct } from 'constructs'; import * as s3 from '@aws-cdk/aws-s3'; import * as actions from '../../lib'; class MyStack extends Stack { constructor(scope: Construct, id: string, props?: StackProps) { super(scope, id, props); const bukkit = new s3.Bucket(this, 'Bukkit', { removalPolicy: RemovalPolicy.DESTROY, }); const arto = new cpipe.Artifact(); const pipe = new cpipe.Pipeline(this, 'Pipe'); pipe.addStage({ stageName: 'Source', actions: [ new actions.GitHubSourceAction({ actionName: 'GitHub', oauthToken: SecretValue.secretsManager('github-token'), output: arto, owner: 'rix0rrr', repo: 'kessel-run', branch: 'main', }), ], }); pipe.addStage({ stageName: 'PutErThere', actions: [ new actions.S3DeployAction({ actionName: 'InS3', bucket: bukkit, input: arto, accessControl: s3.BucketAccessControl.PUBLIC_READ, }), ], }); } } const app = new App({ context: { '@aws-cdk/aws-s3:grantWriteWithoutAcl': true, }, }); new MyStack(app, 'DeployStack', { env: { region: process.env.CDK_DEFAULT_REGION, account: process.env.CDK_DEFAULT_ACCOUNT, }, }); app.synth();

Thanks Rico! I completely blanked on the fact that accessControl in S3DeployAction actually uses Bucket ACLs to achieve its functionality.

I confirmed that setting any accessControl does require s3:PutObjectAcl. Although it's possible to deploy publicly readable objects into a Bucket without using accessControl (you have to change the resource policy of the Bucket to allow everyone to read), we have to support accessControl as well.

I've added a grantPutAcl() to IBucket, and changed the S3DeployAction to call that method if the feature flag is set and accessControl was passed.

I only added s3:PutObjectAcl permissions in grantPutAcl() for now; contrary to the (I agree excellent) error message above, I was not able to get a situation where s3:PutObjectVersionAcl would be required (yes, I tried overwriting the same file multiple times, didn't cause see any issues with just s3:PutObjectAcl), so I left it out for now; we can always add it later.

Let me know if this works!

yes, I tried overwriting the same file multiple times

Did you try this with a versioned bucket?

yes, I tried overwriting the same file multiple times

Did you try this with a versioned bucket?

Yep, I did. Exactly the same result (no error even though the Action's Role did not have s3:PutObjectVersionAcl permissions, the resulting object was readable publicly, multiple versions were created without problems).

skinny85 · 2021-01-08T00:12:15Z

@rix0rrr this is ready for another round of reviews!

skinny85 · 2021-01-12T00:47:19Z

@rix0rrr if you could take another pass, I would appreciate it!

rix0rrr · 2021-01-12T09:22:09Z

packages/@aws-cdk/aws-codepipeline-actions/lib/s3/deploy-action.ts

    this.props.bucket.grantWrite(options.role);

+    if (this.props.accessControl !== undefined &&
+        FeatureFlags.of(scope).isEnabled(cxapi.S3_GRANT_WRITE_WITHOUT_ACL)) {


This feels worse because it's tying implementation details of two different classes together.

To my mind, we should be replacing

this.props.bucket.grantWrite(options.role);

With either:

this.props.bucket.grant(options.role, 's3:PutObject', 's3:PubObject*Acl', 's3:DeleteObject', 's3:...WhateverElseHere );

OR

this.props.bucket.grantWriteWithAcl(options.role);

CDK methods should be intent-based. You tell me what you need, I'll make it happen.

Not: "you tell me be a bit of what you need, and you'll predict what I'll have done based on some flags, and then you tell me to do some more"

I don't like either of those options 😕.

this.props.bucket.grant(options.role, 's3:PutObject', 's3:PubObject*Acl', ...) I think is out of the question - it's way too low-level, and completely not intent-based.

I think grantWriteWithAcl() would actually be a mistake. One of the problems that were raised with grantWrite() giving the permissions it does now is that s3:PutObjectAcl is not a write permissions - it's a permission-changing permission, and that it's surprising that it's present on a method that deals with writing. So, having grantWriteWithAcl() encourages a practice that our security team considers bad (mixing permissions in different categories).

Let's just always call bucket.grantPutAcl() if accessControl was passed, regardless of the presence of the feature flag. The code will be simple and, like you said, intent-based, and we'll just have superfluous permissions in the case the feature flag is not set.

I've changed the code to match, let me know if this works!

rix0rrr · 2021-01-12T09:24:13Z

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

   */
  public grantWrite(identity: iam.IGrantable, objectsKeyPattern: any = '*') {
-    return this.grant(identity, perms.BUCKET_WRITE_ACTIONS, perms.KEY_WRITE_ACTIONS,
+    return this.grant(identity, this.writeActions, perms.KEY_WRITE_ACTIONS,


yes, I tried overwriting the same file multiple times

Did you try this with a versioned bucket?

Right now, Bucket.grantWrite() adds an 's3:PutObject*' permission to the passed principal. That means it grants the 's3:PubObjectAcl' permission, which allows changing the ACL of an individual object written to the Bucket, potentially giving it more visibility than other objects in the Bucket. Change that behavior to grant 's3:PutObject' instead. Since customers might be relying on the old overly-broad permissions, gate that change behind a feature flag, which means only newly created CDK projects will get this behavior.

skinny85 · 2021-01-12T23:10:38Z

@rix0rrr incorporated your comments, please re-review, especially considering #12391 (comment) .

mergify · 2021-01-13T08:40:20Z

Thank you for contributing! Your pull request will be updated from master and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

aws-cdk-automation · 2021-01-13T09:16:35Z

AWS CodeBuild CI Report

CodeBuild project: AutoBuildProject6AEA49D1-qxepHUsryhcu
Commit ID: f954d70
Result: SUCCEEDED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

mergify · 2021-01-13T09:17:55Z

Thank you for contributing! Your pull request will be updated from master and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

aav · 2021-01-28T00:58:06Z

I'm using BitBucketSourceAction as a part of my pipeline. It seems that this action requires s3:PutObjectAcl to write sources to the artifact_bucket. I had to add the following line as a workaround:

pipeline.artifact_bucket.grant_put_acl(pipeline.role)

Without it, I observe an access denied error when the source action runs.

Should I open a separate issue for this problem?

skinny85 · 2021-01-28T01:00:27Z

@aav Yes please!

aav · 2021-01-28T02:00:29Z

@skinny85 after the artifact_bucket was re-created, the problem disappeared. So it's hard to say under which conditions source action requires s3:PutObjectAcl.

If this problem will re-appear, I will investigate more and open an issue if needed.

…ross-account Apparently, when removing the s3:PutObject* permissions in aws#12391, we broke the CodeCommitSourceAction when it's cross-account. Not entirely sure why is that permission required only when the action is cross-account, but I have confirmed this fixes the problem, so add an explicit call to `Bucket.grantPutAcl()` when the actions is cross-account. Fixes aws#14156

…ross-account (#14260) Apparently, when removing the s3:PutObject* permissions in #12391, we broke the CodeCommitSourceAction when it's cross-account. Not entirely sure why is that permission required only when the action is cross-account, but I have confirmed this fixes the problem, so add an explicit call to `Bucket.grantPutAcl()` when the actions is cross-account. Fixes #14156 ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*

…ross-account (aws#14260) Apparently, when removing the s3:PutObject* permissions in aws#12391, we broke the CodeCommitSourceAction when it's cross-account. Not entirely sure why is that permission required only when the action is cross-account, but I have confirmed this fixes the problem, so add an explicit call to `Bucket.grantPutAcl()` when the actions is cross-account. Fixes aws#14156 ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*

…nd `grantPut` methods (#18494) In this pull request I try to fix a big behavioral change in the bucket grant methods for write actions. This change was introduced due to security issues as stated in [this pull request](#12391) and substituted the `s3:PutObject*` action glob pattern with the simple `s3:PutObject` to exclude the dangerous `s3:PutObjectAcl` and `s3:PutObjectVersionAcl` from the equation. While I agree about the security issues, I think that other useful actions - if safe, obviously - should be added when calling the `grantWrite`, `grantReadWrite` and `grantPut` methods. This pull request adds the following actions: * s3:PutObjectLegalHold * s3:PutObjectRetention * s3:PutObjectTagging * s3:PutObjectVersionTagging I also added the `s3:PutObjectVersionAcl` action to the `grantPutAcl` method, along with the existing `s3:PutObjectAcl`. I adapted existing unit and integ tests to accept these new actions. Fixes #13616 ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*

…nd `grantPut` methods (aws#18494) In this pull request I try to fix a big behavioral change in the bucket grant methods for write actions. This change was introduced due to security issues as stated in [this pull request](aws#12391) and substituted the `s3:PutObject*` action glob pattern with the simple `s3:PutObject` to exclude the dangerous `s3:PutObjectAcl` and `s3:PutObjectVersionAcl` from the equation. While I agree about the security issues, I think that other useful actions - if safe, obviously - should be added when calling the `grantWrite`, `grantReadWrite` and `grantPut` methods. This pull request adds the following actions: * s3:PutObjectLegalHold * s3:PutObjectRetention * s3:PutObjectTagging * s3:PutObjectVersionTagging I also added the `s3:PutObjectVersionAcl` action to the `grantPutAcl` method, along with the existing `s3:PutObjectAcl`. I adapted existing unit and integ tests to accept these new actions. Fixes aws#13616 ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*

skinny85 self-assigned this Jan 7, 2021

skinny85 requested a review from rix0rrr January 7, 2021 02:08

github-actions bot assigned iliapolo Jan 7, 2021

github-actions bot added the @aws-cdk/aws-s3 Related to Amazon S3 label Jan 7, 2021

mergify bot added the contribution/core This is a PR that came from AWS. label Jan 7, 2021

skinny85 force-pushed the fix/s3-grant-write-acl branch from c5b4b09 to e01ea04 Compare January 7, 2021 02:11

rix0rrr requested changes Jan 7, 2021

View reviewed changes

skinny85 force-pushed the fix/s3-grant-write-acl branch 2 times, most recently from c5fa1e8 to 46475bd Compare January 8, 2021 00:11

skinny85 requested a review from rix0rrr January 8, 2021 00:11

skinny85 force-pushed the fix/s3-grant-write-acl branch from 46475bd to 4cf1842 Compare January 12, 2021 00:45

rix0rrr requested changes Jan 12, 2021

View reviewed changes

skinny85 force-pushed the fix/s3-grant-write-acl branch from 4cf1842 to cf0e6b2 Compare January 12, 2021 23:06

skinny85 requested a review from rix0rrr January 12, 2021 23:07

rix0rrr approved these changes Jan 13, 2021

View reviewed changes

Merge branch 'master' into fix/s3-grant-write-acl

f954d70

mergify bot merged commit cd437cf into aws:master Jan 13, 2021

skinny85 deleted the fix/s3-grant-write-acl branch January 13, 2021 17:51

Kruspe mentioned this pull request Mar 12, 2021

(codepipeline-actions): BitBucketSourceAction requires s3:PutObjectAcl permissions #13557

Closed

iliapolo mentioned this pull request Mar 16, 2021

aws-s3: grantPut not working as advertised #13616

Closed

iliapolo mentioned this pull request Apr 8, 2021

[aws-elasticloadbalancingv2] cross-account logAccessLogs() triggers SecurityHub warning #11351

Closed

2 tasks

skinny85 mentioned this pull request Apr 19, 2021

fix(codepipeline-actions): CodeCommit source action fails when it's cross-account #14260

Merged

njlynch mentioned this pull request Dec 1, 2021

(elbv2): LoadBalancer.logAccessLogs results in breached S3.6 security best practice #17726

Closed

flavioleggio mentioned this pull request Jan 18, 2022

fix(s3): add missing safe actions to grantWrite, grantReadWrite and grantPut methods #18494

Merged

peterwoodworth mentioned this pull request Jan 26, 2022

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

Closed

Conversation

skinny85 commented Jan 7, 2021

Uh oh!

gitpod-io bot commented Jan 7, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rix0rrr Jan 11, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rix0rrr Jan 11, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

skinny85 commented Jan 8, 2021

Uh oh!

skinny85 commented Jan 12, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

skinny85 Jan 12, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

skinny85 commented Jan 12, 2021

Uh oh!

mergify bot commented Jan 13, 2021

Uh oh!

aws-cdk-automation commented Jan 13, 2021

AWS CodeBuild CI Report

Uh oh!

mergify bot commented Jan 13, 2021

Uh oh!

aav commented Jan 28, 2021

Uh oh!

skinny85 commented Jan 28, 2021

Uh oh!

aav commented Jan 28, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

gitpod-io bot commented Jan 7, 2021 •

edited

Loading

rix0rrr Jan 11, 2021 •

edited

Loading

rix0rrr Jan 11, 2021 •

edited

Loading

skinny85 Jan 12, 2021 •

edited

Loading