Skip to content

Commit

Permalink
fix(s3): add missing safe actions to grantWrite, grantReadWrite a…
Browse files Browse the repository at this point in the history 
…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*
  • Loading branch information
@flavioleggio @TikiTDO
flavioleggio authored and TikiTDO committed Feb 21, 2022
1 parent 704cd77 commit c380af0
Show file tree
Hide file tree
Showing 64 changed files with 507 additions and 19 deletions.
4 changes: 4 additions & 0 deletions packages/@aws-cdk/app-delivery/test/integ.cicd.expected.json
View file
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,10 @@
"s3:List*",
"s3:DeleteObject*",
"s3:PutObject",
"s3:PutObjectLegalHold",
"s3:PutObjectRetention",
"s3:PutObjectTagging",
"s3:PutObjectVersionTagging",
"s3:Abort*"
],
"Effect": "Allow",
Expand Down
6 changes: 5 additions & 1 deletion packages/@aws-cdk/aws-codebuild/test/integ.caching.expected.json
View file
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,10 @@
"s3:List*",
"s3:DeleteObject*",
"s3:PutObject",
"s3:PutObjectLegalHold",
"s3:PutObjectRetention",
"s3:PutObjectTagging",
"s3:PutObjectVersionTagging",
"s3:Abort*"
],
"Effect": "Allow",
Expand Down Expand Up @@ -171,4 +175,4 @@
}
}
}
}
}
6 changes: 5 additions & 1 deletion packages/@aws-cdk/aws-codebuild/test/integ.project-buildspec-artifacts.expected.json
View file
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,10 @@
"s3:List*",
"s3:DeleteObject*",
"s3:PutObject",
"s3:PutObjectLegalHold",
"s3:PutObjectRetention",
"s3:PutObjectTagging",
"s3:PutObjectVersionTagging",
"s3:Abort*"
],
"Effect": "Allow",
Expand Down Expand Up @@ -166,4 +170,4 @@
}
}
}
}
}
6 changes: 5 additions & 1 deletion packages/@aws-cdk/aws-codebuild/test/integ.project-logging.expected.json
View file
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,10 @@
"Action": [
"s3:DeleteObject*",
"s3:PutObject",
"s3:PutObjectLegalHold",
"s3:PutObjectRetention",
"s3:PutObjectTagging",
"s3:PutObjectVersionTagging",
"s3:Abort*"
],
"Effect": "Allow",
Expand Down Expand Up @@ -223,4 +227,4 @@
}
}
}
}
}
6 changes: 5 additions & 1 deletion packages/@aws-cdk/aws-codebuild/test/integ.project-secondary-sources-artifacts.expected.json
View file
Original file line number Diff line number Diff line change
Expand Up @@ -64,6 +64,10 @@
"s3:List*",
"s3:DeleteObject*",
"s3:PutObject",
"s3:PutObjectLegalHold",
"s3:PutObjectRetention",
"s3:PutObjectTagging",
"s3:PutObjectVersionTagging",
"s3:Abort*"
],
"Effect": "Allow",
Expand Down Expand Up @@ -218,4 +222,4 @@
}
}
}
}
}
5 changes: 4 additions & 1 deletion packages/@aws-cdk/aws-codepipeline-actions/test/bitbucket/bitbucket-source-action.test.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -91,7 +91,10 @@ describeDeprecated('BitBucket source Action', () => {
'PolicyDocument': {
'Statement': Match.arrayWith([
Match.objectLike({
'Action': 's3:PutObjectAcl',
'Action': [
's3:PutObjectAcl',
's3:PutObjectVersionAcl',
],
'Effect': 'Allow',
'Resource': {
'Fn::Join': [
Expand Down
5 changes: 4 additions & 1 deletion packages/@aws-cdk/aws-codepipeline-actions/test/codecommit/codecommit-source-action.test.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -554,7 +554,10 @@ describe('CodeCommit Source Action', () => {
Template.fromStack(repoStack).hasResourceProperties('AWS::IAM::Policy', {
PolicyDocument: {
Statement: Match.arrayWith([{
'Action': 's3:PutObjectAcl',
'Action': [
's3:PutObjectAcl',
's3:PutObjectVersionAcl',
],
'Effect': 'Allow',
'Resource': {
'Fn::Join': ['', [
Expand Down
5 changes: 4 additions & 1 deletion ...codepipeline-actions/test/codestar-connections/codestar-connections-source-action.test.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -93,7 +93,10 @@ describe('CodeStar Connections source Action', () => {
'PolicyDocument': {
'Statement': Match.arrayWith([
Match.objectLike({
'Action': 's3:PutObjectAcl',
'Action': [
's3:PutObjectAcl',
's3:PutObjectVersionAcl',
],
'Effect': 'Allow',
'Resource': {
'Fn::Join': ['', [
Expand Down
8 changes: 8 additions & 0 deletions ...ges/@aws-cdk/aws-codepipeline-actions/test/integ.cfn-template-from-repo.lit.expected.json
View file
Original file line number Diff line number Diff line change
Expand Up @@ -159,6 +159,10 @@
"s3:List*",
"s3:DeleteObject*",
"s3:PutObject",
"s3:PutObjectLegalHold",
"s3:PutObjectRetention",
"s3:PutObjectTagging",
"s3:PutObjectVersionTagging",
"s3:Abort*"
],
"Effect": "Allow",
Expand Down Expand Up @@ -440,6 +444,10 @@
"s3:List*",
"s3:DeleteObject*",
"s3:PutObject",
"s3:PutObjectLegalHold",
"s3:PutObjectRetention",
"s3:PutObjectTagging",
"s3:PutObjectVersionTagging",
"s3:Abort*"
],
"Effect": "Allow",
Expand Down
20 changes: 20 additions & 0 deletions ...ws-codepipeline-actions/test/integ.lambda-deployed-through-codepipeline.lit.expected.json
View file
Original file line number Diff line number Diff line change
Expand Up @@ -153,6 +153,10 @@
"s3:List*",
"s3:DeleteObject*",
"s3:PutObject",
"s3:PutObjectLegalHold",
"s3:PutObjectRetention",
"s3:PutObjectTagging",
"s3:PutObjectVersionTagging",
"s3:Abort*"
],
"Effect": "Allow",
Expand Down Expand Up @@ -508,6 +512,10 @@
"s3:List*",
"s3:DeleteObject*",
"s3:PutObject",
"s3:PutObjectLegalHold",
"s3:PutObjectRetention",
"s3:PutObjectTagging",
"s3:PutObjectVersionTagging",
"s3:Abort*"
],
"Effect": "Allow",
Expand Down Expand Up @@ -621,6 +629,10 @@
"s3:List*",
"s3:DeleteObject*",
"s3:PutObject",
"s3:PutObjectLegalHold",
"s3:PutObjectRetention",
"s3:PutObjectTagging",
"s3:PutObjectVersionTagging",
"s3:Abort*"
],
"Effect": "Allow",
Expand Down Expand Up @@ -1350,6 +1362,10 @@
"s3:List*",
"s3:DeleteObject*",
"s3:PutObject",
"s3:PutObjectLegalHold",
"s3:PutObjectRetention",
"s3:PutObjectTagging",
"s3:PutObjectVersionTagging",
"s3:Abort*"
],
"Effect": "Allow",
Expand Down Expand Up @@ -1572,6 +1588,10 @@
"s3:List*",
"s3:DeleteObject*",
"s3:PutObject",
"s3:PutObjectLegalHold",
"s3:PutObjectRetention",
"s3:PutObjectTagging",
"s3:PutObjectVersionTagging",
"s3:Abort*"
],
"Effect": "Allow",
Expand Down
8 changes: 8 additions & 0 deletions packages/@aws-cdk/aws-codepipeline-actions/test/integ.lambda-pipeline.expected.json
View file
Original file line number Diff line number Diff line change
Expand Up @@ -153,6 +153,10 @@
"s3:List*",
"s3:DeleteObject*",
"s3:PutObject",
"s3:PutObjectLegalHold",
"s3:PutObjectRetention",
"s3:PutObjectTagging",
"s3:PutObjectVersionTagging",
"s3:Abort*"
],
"Effect": "Allow",
Expand Down Expand Up @@ -388,6 +392,10 @@
"Action": [
"s3:DeleteObject*",
"s3:PutObject",
"s3:PutObjectLegalHold",
"s3:PutObjectRetention",
"s3:PutObjectTagging",
"s3:PutObjectVersionTagging",
"s3:Abort*"
],
"Effect": "Allow",
Expand Down
8 changes: 8 additions & 0 deletions packages/@aws-cdk/aws-codepipeline-actions/test/integ.pipeline-alexa-deploy.expected.json
View file
Original file line number Diff line number Diff line change
Expand Up @@ -163,6 +163,10 @@
"s3:List*",
"s3:DeleteObject*",
"s3:PutObject",
"s3:PutObjectLegalHold",
"s3:PutObjectRetention",
"s3:PutObjectTagging",
"s3:PutObjectVersionTagging",
"s3:Abort*"
],
"Effect": "Allow",
Expand Down Expand Up @@ -387,6 +391,10 @@
"Action": [
"s3:DeleteObject*",
"s3:PutObject",
"s3:PutObjectLegalHold",
"s3:PutObjectRetention",
"s3:PutObjectTagging",
"s3:PutObjectVersionTagging",
"s3:Abort*"
],
"Effect": "Allow",
Expand Down
8 changes: 8 additions & 0 deletions ...ages/@aws-cdk/aws-codepipeline-actions/test/integ.pipeline-cfn-cross-region.expected.json
View file
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,10 @@
"s3:List*",
"s3:DeleteObject*",
"s3:PutObject",
"s3:PutObjectLegalHold",
"s3:PutObjectRetention",
"s3:PutObjectTagging",
"s3:PutObjectVersionTagging",
"s3:Abort*"
],
"Effect": "Allow",
Expand Down Expand Up @@ -265,6 +269,10 @@
"Action": [
"s3:DeleteObject*",
"s3:PutObject",
"s3:PutObjectLegalHold",
"s3:PutObjectRetention",
"s3:PutObjectTagging",
"s3:PutObjectVersionTagging",
"s3:Abort*"
],
"Effect": "Allow",
Expand Down
8 changes: 8 additions & 0 deletions .../@aws-cdk/aws-codepipeline-actions/test/integ.pipeline-cfn-with-action-role.expected.json
View file
Original file line number Diff line number Diff line change
Expand Up @@ -164,6 +164,10 @@
"s3:List*",
"s3:DeleteObject*",
"s3:PutObject",
"s3:PutObjectLegalHold",
"s3:PutObjectRetention",
"s3:PutObjectTagging",
"s3:PutObjectVersionTagging",
"s3:Abort*"
],
"Effect": "Allow",
Expand Down Expand Up @@ -384,6 +388,10 @@
"Action": [
"s3:DeleteObject*",
"s3:PutObject",
"s3:PutObjectLegalHold",
"s3:PutObjectRetention",
"s3:PutObjectTagging",
"s3:PutObjectVersionTagging",
"s3:Abort*"
],
"Effect": "Allow",
Expand Down
12 changes: 12 additions & 0 deletions packages/@aws-cdk/aws-codepipeline-actions/test/integ.pipeline-cfn.expected.json
View file
Original file line number Diff line number Diff line change
Expand Up @@ -153,6 +153,10 @@
"s3:List*",
"s3:DeleteObject*",
"s3:PutObject",
"s3:PutObjectLegalHold",
"s3:PutObjectRetention",
"s3:PutObjectTagging",
"s3:PutObjectVersionTagging",
"s3:Abort*"
],
"Effect": "Allow",
Expand Down Expand Up @@ -440,6 +444,10 @@
"Action": [
"s3:DeleteObject*",
"s3:PutObject",
"s3:PutObjectLegalHold",
"s3:PutObjectRetention",
"s3:PutObjectTagging",
"s3:PutObjectVersionTagging",
"s3:Abort*"
],
"Effect": "Allow",
Expand Down Expand Up @@ -563,6 +571,10 @@
"Action": [
"s3:DeleteObject*",
"s3:PutObject",
"s3:PutObjectLegalHold",
"s3:PutObjectRetention",
"s3:PutObjectTagging",
"s3:PutObjectVersionTagging",
"s3:Abort*"
],
"Effect": "Allow",
Expand Down
4 changes: 4 additions & 0 deletions ...ages/@aws-cdk/aws-codepipeline-actions/test/integ.pipeline-code-build-batch.expected.json
View file
Original file line number Diff line number Diff line change
Expand Up @@ -110,6 +110,10 @@
"s3:List*",
"s3:DeleteObject*",
"s3:PutObject",
"s3:PutObjectLegalHold",
"s3:PutObjectRetention",
"s3:PutObjectTagging",
"s3:PutObjectVersionTagging",
"s3:Abort*"
],
"Effect": "Allow",
Expand Down
12 changes: 12 additions & 0 deletions ...codepipeline-actions/test/integ.pipeline-code-build-multiple-inputs-outputs.expected.json
View file
Original file line number Diff line number Diff line change
Expand Up @@ -110,6 +110,10 @@
"s3:List*",
"s3:DeleteObject*",
"s3:PutObject",
"s3:PutObjectLegalHold",
"s3:PutObjectRetention",
"s3:PutObjectTagging",
"s3:PutObjectVersionTagging",
"s3:Abort*"
],
"Effect": "Allow",
Expand Down Expand Up @@ -196,6 +200,10 @@
"Action": [
"s3:DeleteObject*",
"s3:PutObject",
"s3:PutObjectLegalHold",
"s3:PutObjectRetention",
"s3:PutObjectTagging",
"s3:PutObjectVersionTagging",
"s3:Abort*"
],
"Effect": "Allow",
Expand Down Expand Up @@ -557,6 +565,10 @@
"s3:List*",
"s3:DeleteObject*",
"s3:PutObject",
"s3:PutObjectLegalHold",
"s3:PutObjectRetention",
"s3:PutObjectTagging",
"s3:PutObjectVersionTagging",
"s3:Abort*"
],
"Effect": "Allow",
Expand Down
12 changes: 12 additions & 0 deletions ...ges/@aws-cdk/aws-codepipeline-actions/test/integ.pipeline-code-commit-build.expected.json
View file
Original file line number Diff line number Diff line change
Expand Up @@ -92,6 +92,10 @@
"s3:List*",
"s3:DeleteObject*",
"s3:PutObject",
"s3:PutObjectLegalHold",
"s3:PutObjectRetention",
"s3:PutObjectTagging",
"s3:PutObjectVersionTagging",
"s3:Abort*"
],
"Effect": "Allow",
Expand Down Expand Up @@ -413,6 +417,10 @@
"s3:List*",
"s3:DeleteObject*",
"s3:PutObject",
"s3:PutObjectLegalHold",
"s3:PutObjectRetention",
"s3:PutObjectTagging",
"s3:PutObjectVersionTagging",
"s3:Abort*"
],
"Effect": "Allow",
Expand Down Expand Up @@ -671,6 +679,10 @@
"s3:List*",
"s3:DeleteObject*",
"s3:PutObject",
"s3:PutObjectLegalHold",
"s3:PutObjectRetention",
"s3:PutObjectTagging",
"s3:PutObjectVersionTagging",
"s3:Abort*"
],
"Effect": "Allow",
Expand Down
Loading

0 comments on commit c380af0

Please sign in to comment.