Skip to content

Commit

Permalink
fix(s3): Bucket.grantWrite() no longer adds s3:PutObject* permission (#…
Browse files Browse the repository at this point in the history 
…12391)

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*
  • Loading branch information
@skinny85
skinny85 authored Jan 13, 2021
1 parent d0305f3 commit cd437cf
Show file tree
Hide file tree
Showing 8 changed files with 352 additions and 358 deletions.
6 changes: 6 additions & 0 deletions packages/@aws-cdk/aws-codepipeline-actions/lib/s3/deploy-action.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -111,6 +111,12 @@ export class S3DeployAction extends Action {
// pipeline needs permissions to write to the S3 bucket
this.props.bucket.grantWrite(options.role);

if (this.props.accessControl !== undefined) {
// we need to modify the ACL settings of objects within the Bucket,
// so grant the Action's Role permissions to do that
this.props.bucket.grantPutAcl(options.role);
}

// the Action Role also needs to read from the Pipeline's bucket
options.bucket.grantRead(options.role);

Expand Down
231 changes: 25 additions & 206 deletions packages/@aws-cdk/aws-codepipeline-actions/test/integ.pipeline-s3-deploy.expected.json
View file
Original file line number Diff line number Diff line change
Expand Up @@ -12,155 +12,9 @@
},
"DeployBucket67E2C076": {
"Type": "AWS::S3::Bucket",
"UpdateReplacePolicy": "Retain",
"DeletionPolicy": "Retain"
},
"PipelineArtifactsBucketEncryptionKey01D58D69": {
"Type": "AWS::KMS::Key",
"Properties": {
"KeyPolicy": {
"Statement": [
{
"Action": [
"kms:Create*",
"kms:Describe*",
"kms:Enable*",
"kms:List*",
"kms:Put*",
"kms:Update*",
"kms:Revoke*",
"kms:Disable*",
"kms:Get*",
"kms:Delete*",
"kms:ScheduleKeyDeletion",
"kms:CancelKeyDeletion",
"kms:GenerateDataKey",
"kms:TagResource",
"kms:UntagResource"
],
"Effect": "Allow",
"Principal": {
"AWS": {
"Fn::Join": [
"",
[
"arn:",
{
"Ref": "AWS::Partition"
},
":iam::",
{
"Ref": "AWS::AccountId"
},
":root"
]
]
}
},
"Resource": "*"
},
{
"Action": [
"kms:Decrypt",
"kms:DescribeKey",
"kms:Encrypt",
"kms:ReEncrypt*",
"kms:GenerateDataKey*"
],
"Effect": "Allow",
"Principal": {
"AWS": {
"Fn::GetAtt": [
"PipelineRoleD68726F7",
"Arn"
]
}
},
"Resource": "*"
},
{
"Action": [
"kms:Encrypt",
"kms:ReEncrypt*",
"kms:GenerateDataKey*",
"kms:Decrypt"
],
"Effect": "Allow",
"Principal": {
"AWS": {
"Fn::GetAtt": [
"PipelineSourceCodePipelineActionRoleC6F9E7F5",
"Arn"
]
}
},
"Resource": "*"
},
{
"Action": [
"kms:Decrypt",
"kms:DescribeKey"
],
"Effect": "Allow",
"Principal": {
"AWS": {
"Fn::GetAtt": [
"PipelineDeployDeployActionCodePipelineActionRole1C288A60",
"Arn"
]
}
},
"Resource": "*"
}
],
"Version": "2012-10-17"
}
},
"UpdateReplacePolicy": "Delete",
"DeletionPolicy": "Delete"
},
"PipelineArtifactsBucketEncryptionKeyAlias5C510EEE": {
"Type": "AWS::KMS::Alias",
"Properties": {
"AliasName": "alias/codepipeline-awscdkcodepipelines3deploypipeline907bf1e7",
"TargetKeyId": {
"Fn::GetAtt": [
"PipelineArtifactsBucketEncryptionKey01D58D69",
"Arn"
]
}
},
"UpdateReplacePolicy": "Delete",
"DeletionPolicy": "Delete"
},
"PipelineArtifactsBucket22248F97": {
"Type": "AWS::S3::Bucket",
"Properties": {
"BucketEncryption": {
"ServerSideEncryptionConfiguration": [
{
"ServerSideEncryptionByDefault": {
"KMSMasterKeyID": {
"Fn::GetAtt": [
"PipelineArtifactsBucketEncryptionKey01D58D69",
"Arn"
]
},
"SSEAlgorithm": "aws:kms"
}
}
]
},
"PublicAccessBlockConfiguration": {
"BlockPublicAcls": true,
"BlockPublicPolicy": true,
"IgnorePublicAcls": true,
"RestrictPublicBuckets": true
}
},
"UpdateReplacePolicy": "Retain",
"DeletionPolicy": "Retain"
},
"PipelineRoleD68726F7": {
"Type": "AWS::IAM::Role",
"Properties": {
Expand Down Expand Up @@ -196,7 +50,7 @@
"Resource": [
{
"Fn::GetAtt": [
"PipelineArtifactsBucket22248F97",
"PipelineBucketB967BD35",
"Arn"
]
},
Expand All @@ -206,7 +60,7 @@
[
{
"Fn::GetAtt": [
"PipelineArtifactsBucket22248F97",
"PipelineBucketB967BD35",
"Arn"
]
},
Expand All @@ -216,22 +70,6 @@
}
]
},
{
"Action": [
"kms:Decrypt",
"kms:DescribeKey",
"kms:Encrypt",
"kms:ReEncrypt*",
"kms:GenerateDataKey*"
],
"Effect": "Allow",
"Resource": {
"Fn::GetAtt": [
"PipelineArtifactsBucketEncryptionKey01D58D69",
"Arn"
]
}
},
{
"Action": "sts:AssumeRole",
"Effect": "Allow",
Expand Down Expand Up @@ -341,17 +179,8 @@
}
],
"ArtifactStore": {
"EncryptionKey": {
"Id": {
"Fn::GetAtt": [
"PipelineArtifactsBucketEncryptionKey01D58D69",
"Arn"
]
},
"Type": "KMS"
},
"Location": {
"Ref": "PipelineArtifactsBucket22248F97"
"Ref": "PipelineBucketB967BD35"
},
"Type": "S3"
}
Expand Down Expand Up @@ -438,7 +267,7 @@
"Resource": [
{
"Fn::GetAtt": [
"PipelineArtifactsBucket22248F97",
"PipelineBucketB967BD35",
"Arn"
]
},
Expand All @@ -448,7 +277,7 @@
[
{
"Fn::GetAtt": [
"PipelineArtifactsBucket22248F97",
"PipelineBucketB967BD35",
"Arn"
]
},
Expand All @@ -457,21 +286,6 @@
]
}
]
},
{
"Action": [
"kms:Encrypt",
"kms:ReEncrypt*",
"kms:GenerateDataKey*",
"kms:Decrypt"
],
"Effect": "Allow",
"Resource": {
"Fn::GetAtt": [
"PipelineArtifactsBucketEncryptionKey01D58D69",
"Arn"
]
}
}
],
"Version": "2012-10-17"
Expand Down Expand Up @@ -551,6 +365,24 @@
}
]
},
{
"Action": "s3:PutObjectAcl",
"Effect": "Allow",
"Resource": {
"Fn::Join": [
"",
[
{
"Fn::GetAtt": [
"DeployBucket67E2C076",
"Arn"
]
},
"/*"
]
]
}
},
{
"Action": [
"s3:GetObject*",
Expand All @@ -561,7 +393,7 @@
"Resource": [
{
"Fn::GetAtt": [
"PipelineArtifactsBucket22248F97",
"PipelineBucketB967BD35",
"Arn"
]
},
Expand All @@ -571,7 +403,7 @@
[
{
"Fn::GetAtt": [
"PipelineArtifactsBucket22248F97",
"PipelineBucketB967BD35",
"Arn"
]
},
Expand All @@ -580,19 +412,6 @@
]
}
]
},
{
"Action": [
"kms:Decrypt",
"kms:DescribeKey"
],
"Effect": "Allow",
"Resource": {
"Fn::GetAtt": [
"PipelineArtifactsBucketEncryptionKey01D58D69",
"Arn"
]
}
}
],
"Version": "2012-10-17"
Expand Down
5 changes: 4 additions & 1 deletion packages/@aws-cdk/aws-codepipeline-actions/test/integ.pipeline-s3-deploy.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -19,9 +19,12 @@ const sourceAction = new cpactions.S3SourceAction({
bucketKey: 'key',
});

const deployBucket = new s3.Bucket(stack, 'DeployBucket', {});
const deployBucket = new s3.Bucket(stack, 'DeployBucket', {
removalPolicy: cdk.RemovalPolicy.DESTROY,
});

new codepipeline.Pipeline(stack, 'Pipeline', {
artifactBucket: bucket,
stages: [
{
stageName: 'Source',
Expand Down
Loading

0 comments on commit cd437cf

Please sign in to comment.