fix(s3): Bucket.grantWrite() no longer adds s3:PutObject* permission

[skinny85]

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]

[rix0rrr]

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

[skinny85]

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

[rix0rrr]

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

[skinny85]

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

[rix0rrr]

Yes but shouldn't the value be true then?

[skinny85]

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)

[rix0rrr]

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"

[skinny85]

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? 🙂

[rix0rrr]

I'm proposing you test 😝

[rix0rrr]

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();

[skinny85]

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!

[rix0rrr]

yes, I tried overwriting the same file multiple times

Did you try this with a versioned bucket?

[skinny85]

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]

@rix0rrr this is ready for another round of reviews!

[skinny85]

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

[rix0rrr]

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"

[skinny85]

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]

yes, I tried overwriting the same file multiple times

Did you try this with a versioned bucket?

[skinny85]

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

[mergify]

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]

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]

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]

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]

@aav Yes please!

[aav]

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