Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

aws-s3: grantPut not working as advertised #13616

Closed
PhilKershaw opened this issue Mar 16, 2021 · 8 comments · Fixed by #18494
Closed

aws-s3: grantPut not working as advertised #13616

PhilKershaw opened this issue Mar 16, 2021 · 8 comments · Fixed by #18494
Labels
@aws-cdk/aws-s3 Related to Amazon S3 documentation This is a problem with documentation. effort/small Small work item – less than a day of effort p2

Comments

@PhilKershaw
Copy link

The grantPut method documentation reads:

Grants s3:PutObject* and s3:Abort* permissions for this bucket to an IAM principal.

If encryption is used, permission to use the key to encrypt the contents of written files will also be granted to the same principal.

Reproduction Steps

const function = new lambda.Function(this, "FunkyTown", {...});
const bucket = new s3.Bucket(this, "AWholeLotOfBucket");
bucket.grantPut(function);

What did you expect to happen?

To add action s3:PutObject* thus:

Action:
  - s3:PutObject*
  - s3:Abort*
Effect: Allow
Resource:
  Fn::Join:
    - ""
    - - Fn::GetAtt:
          - AWholeLotOfBucketA765543B
          - Arn
      - /*

What actually happened?

Action:
  - s3:PutObject
  - s3:Abort*
Effect: Allow
Resource:
  Fn::Join:
    - ""
    - - Fn::GetAtt:
          - AWholeLotOfBucketA765543B
          - Arn
      - /*

Environment

  • **CDK CLI Version : 1.93.0
  • **Framework Version: 1.93.0
  • **Node.js Version: v14.16.0
  • **OS : Amazon Linux release 2 (Karoo)
  • **Language (Version): 3.9.9

Other

Link to docs: https://docs.aws.amazon.com/cdk/api/latest/docs/@aws-cdk_aws-s3.Bucket.html#grantwbrputidentity-objectskeypattern
Link to original PR: #591


This is 🐛 Bug Report

@PhilKershaw PhilKershaw added bug This issue is a bug. needs-triage This issue or PR still needs to be triaged. labels Mar 16, 2021
@github-actions github-actions bot added the @aws-cdk/aws-s3 Related to Amazon S3 label Mar 16, 2021
@iliapolo
Copy link
Contributor

@PhilKershaw Thanks. Yes we recently changed this behavior. We'll fix the docs.

@iliapolo iliapolo added documentation This is a problem with documentation. p2 effort/small Small work item – less than a day of effort and removed needs-triage This issue or PR still needs to be triaged. bug This issue is a bug. labels Mar 16, 2021
@PhilKershaw
Copy link
Author

Ah, I was actually hoping for the code to be aligned to the docs. Otherwise, there's no other convenient way to grant PutObjectTagging or any other PutObject* action.

@iliapolo
Copy link
Contributor

@PhilKershaw We decided against using * because it winds up granting PubObjectAcl - which is undesirable in most cases.

You can have a look at this PR for more context.

@PhilKershaw
Copy link
Author

In which case it would be nice to have convenient methods for Tagging etc.. for PutObject like PutObjectAcl - are they on the roadmap? In the meantime, I'll stop being lazy and write the proper policies.

@iliapolo
Copy link
Contributor

We don't currently have a request for those policies. But you are welcome to be the first to make it :)

@iliapolo iliapolo removed their assignment Jun 27, 2021
@demauk
Copy link

demauk commented Oct 4, 2021

I agree that PutObject* is too permissive. On the other hand, I would expect grantPut() to allow tagging.

The same issue exists with grantWrite() and grantReadWrite().

Surely 'PutObject', 'PutObjectTagging' would be ok? May follow up with a PR when time permits.

@flavioleggio
Copy link
Contributor

I am facing with the same issue now, upgrading my cdk lib version. Wouldn't it be better to just explicitly exclude the PutObjectAcl action with a deny effect on the policy instead of removing all PutObject* related acions?

@mergify mergify bot closed this as completed in #18494 Jan 25, 2022
mergify bot pushed a commit that referenced this issue Jan 25, 2022
…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*
@github-actions
Copy link

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

LukvonStrom pushed a commit to LukvonStrom/aws-cdk that referenced this issue Jan 26, 2022
…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*
TikiTDO pushed a commit to TikiTDO/aws-cdk that referenced this issue Feb 21, 2022
…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*
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
@aws-cdk/aws-s3 Related to Amazon S3 documentation This is a problem with documentation. effort/small Small work item – less than a day of effort p2
Projects
None yet
4 participants