diff --git a/packages/@aws-cdk/aws-s3/lib/bucket.ts b/packages/@aws-cdk/aws-s3/lib/bucket.ts index 51f4a30800cad..419d5077d2be3 100644 --- a/packages/@aws-cdk/aws-s3/lib/bucket.ts +++ b/packages/@aws-cdk/aws-s3/lib/bucket.ts @@ -5,8 +5,9 @@ import * as iam from '@aws-cdk/aws-iam'; import * as kms from '@aws-cdk/aws-kms'; import { Fn, IResource, Lazy, RemovalPolicy, Resource, Stack, Token, - CustomResource, CustomResourceProvider, CustomResourceProviderRuntime, + CustomResource, CustomResourceProvider, CustomResourceProviderRuntime, FeatureFlags, } from '@aws-cdk/core'; +import * as cxapi from '@aws-cdk/cx-api'; import { Construct } from 'constructs'; import { BucketPolicy } from './bucket-policy'; import { IBucketNotificationDestination } from './destination'; @@ -584,7 +585,7 @@ abstract class BucketBase extends Resource implements IBucket { * @param objectsKeyPattern Restrict the permission to a certain key pattern (default '*') */ 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, this.bucketArn, this.arnForObjects(objectsKeyPattern)); } @@ -598,7 +599,7 @@ abstract class BucketBase extends Resource implements IBucket { * @param objectsKeyPattern Restrict the permission to a certain key pattern (default '*') */ public grantPut(identity: iam.IGrantable, objectsKeyPattern: any = '*') { - return this.grant(identity, perms.BUCKET_PUT_ACTIONS, perms.KEY_WRITE_ACTIONS, + return this.grant(identity, this.putActions, perms.KEY_WRITE_ACTIONS, this.arnForObjects(objectsKeyPattern)); } @@ -625,7 +626,7 @@ abstract class BucketBase extends Resource implements IBucket { * @param objectsKeyPattern Restrict the permission to a certain key pattern (default '*') */ public grantReadWrite(identity: iam.IGrantable, objectsKeyPattern: any = '*') { - const bucketActions = perms.BUCKET_READ_ACTIONS.concat(perms.BUCKET_WRITE_ACTIONS); + const bucketActions = perms.BUCKET_READ_ACTIONS.concat(this.writeActions); // we need unique permissions because some permissions are common between read and write key actions const keyActions = [...new Set([...perms.KEY_READ_ACTIONS, ...perms.KEY_WRITE_ACTIONS])]; @@ -673,6 +674,19 @@ abstract class BucketBase extends Resource implements IBucket { }); } + private get writeActions(): string[] { + return [ + ...perms.BUCKET_DELETE_ACTIONS, + ...this.putActions, + ]; + } + + private get putActions(): string[] { + return FeatureFlags.of(this).isEnabled(cxapi.S3_GRANT_WRITE_WITHOUT_ACL) + ? perms.BUCKET_PUT_ACTIONS + : perms.LEGACY_BUCKET_PUT_ACTIONS; + } + private urlJoin(...components: string[]): string { return components.reduce((result, component) => { if (result.endsWith('/')) { diff --git a/packages/@aws-cdk/aws-s3/lib/perms.ts b/packages/@aws-cdk/aws-s3/lib/perms.ts index 544bdda936da9..9ea8b92eea362 100644 --- a/packages/@aws-cdk/aws-s3/lib/perms.ts +++ b/packages/@aws-cdk/aws-s3/lib/perms.ts @@ -4,18 +4,18 @@ export const BUCKET_READ_ACTIONS = [ 's3:List*', ]; -export const BUCKET_PUT_ACTIONS = [ +export const LEGACY_BUCKET_PUT_ACTIONS = [ 's3:PutObject*', 's3:Abort*', ]; -export const BUCKET_DELETE_ACTIONS = [ - 's3:DeleteObject*', +export const BUCKET_PUT_ACTIONS = [ + 's3:PutObject', + 's3:Abort*', ]; -export const BUCKET_WRITE_ACTIONS = [ - ...BUCKET_DELETE_ACTIONS, - ...BUCKET_PUT_ACTIONS, +export const BUCKET_DELETE_ACTIONS = [ + 's3:DeleteObject*', ]; export const KEY_READ_ACTIONS = [ diff --git a/packages/@aws-cdk/aws-s3/package.json b/packages/@aws-cdk/aws-s3/package.json index 02da965202f64..8adbd67cf204b 100644 --- a/packages/@aws-cdk/aws-s3/package.json +++ b/packages/@aws-cdk/aws-s3/package.json @@ -85,6 +85,7 @@ "@aws-cdk/aws-iam": "0.0.0", "@aws-cdk/aws-kms": "0.0.0", "@aws-cdk/core": "0.0.0", + "@aws-cdk/cx-api": "0.0.0", "constructs": "^3.2.0" }, "homepage": "https://github.com/aws/aws-cdk", @@ -93,6 +94,7 @@ "@aws-cdk/aws-iam": "0.0.0", "@aws-cdk/aws-kms": "0.0.0", "@aws-cdk/core": "0.0.0", + "@aws-cdk/cx-api": "0.0.0", "constructs": "^3.2.0" }, "engines": { diff --git a/packages/@aws-cdk/aws-s3/test/bucket.test.ts b/packages/@aws-cdk/aws-s3/test/bucket.test.ts index 997a3abd4e56f..cb9202b28cfd7 100644 --- a/packages/@aws-cdk/aws-s3/test/bucket.test.ts +++ b/packages/@aws-cdk/aws-s3/test/bucket.test.ts @@ -3,6 +3,7 @@ import { countResources, expect, haveResource, haveResourceLike, ResourcePart, S import * as iam from '@aws-cdk/aws-iam'; import * as kms from '@aws-cdk/aws-kms'; import * as cdk from '@aws-cdk/core'; +import * as cxapi from '@aws-cdk/cx-api'; import { nodeunitShim, Test } from 'nodeunit-shim'; import * as s3 from '../lib'; @@ -1085,176 +1086,293 @@ nodeunitShim({ test.done(); }, - }, - 'grantWrite with KMS key has appropriate permissions for multipart uploads'(test: Test) { - const stack = new cdk.Stack(); - const bucket = new s3.Bucket(stack, 'MyBucket', { encryption: s3.BucketEncryption.KMS }); - const user = new iam.User(stack, 'MyUser'); - bucket.grantWrite(user); + 'does not grant PutObjectAcl when the S3_GRANT_WRITE_WITHOUT_ACL feature is enabled'(test: Test) { + const app = new cdk.App({ + context: { + [cxapi.S3_GRANT_WRITE_WITHOUT_ACL]: true, + }, + }); + const stack = new cdk.Stack(app, 'Stack'); + const bucket = new s3.Bucket(stack, 'MyBucket'); + const user = new iam.User(stack, 'MyUser'); - expect(stack).toMatch({ - 'Resources': { - 'MyBucketKeyC17130CF': { - 'Type': 'AWS::KMS::Key', - 'Properties': { - 'KeyPolicy': { - 'Statement': [ + bucket.grantReadWrite(user); + + expect(stack).to(haveResourceLike('AWS::IAM::Policy', { + 'PolicyDocument': { + 'Statement': [ + { + 'Action': [ + 's3:GetObject*', + 's3:GetBucket*', + 's3:List*', + 's3:DeleteObject*', + 's3:PutObject', + 's3:Abort*', + ], + 'Resource': [ + { 'Fn::GetAtt': ['MyBucketF68F3FF0', 'Arn'] }, { - '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', + 'Fn::Join': ['', [ + { 'Fn::GetAtt': ['MyBucketF68F3FF0', 'Arn'] }, + '/*', + ]], + }, + ], + }, + ], + }, + })); + + test.done(); + }, + }, + + 'grantWrite': { + 'with KMS key has appropriate permissions for multipart uploads'(test: Test) { + const stack = new cdk.Stack(); + const bucket = new s3.Bucket(stack, 'MyBucket', { encryption: s3.BucketEncryption.KMS }); + const user = new iam.User(stack, 'MyUser'); + bucket.grantWrite(user); + + expect(stack).toMatch({ + 'Resources': { + 'MyBucketKeyC17130CF': { + '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': '*', }, - 'Resource': '*', - }, - { - 'Action': [ - 'kms:Encrypt', - 'kms:ReEncrypt*', - 'kms:GenerateDataKey*', - 'kms:Decrypt', - ], - 'Effect': 'Allow', - 'Principal': { - 'AWS': { - 'Fn::GetAtt': [ - 'MyUserDC45028B', - 'Arn', - ], + { + 'Action': [ + 'kms:Encrypt', + 'kms:ReEncrypt*', + 'kms:GenerateDataKey*', + 'kms:Decrypt', + ], + 'Effect': 'Allow', + 'Principal': { + 'AWS': { + 'Fn::GetAtt': [ + 'MyUserDC45028B', + 'Arn', + ], + }, }, + 'Resource': '*', }, - 'Resource': '*', - }, - ], - 'Version': '2012-10-17', + ], + 'Version': '2012-10-17', + }, + 'Description': 'Created by Default/MyBucket', }, - 'Description': 'Created by Default/MyBucket', + 'UpdateReplacePolicy': 'Retain', + 'DeletionPolicy': 'Retain', }, - 'UpdateReplacePolicy': 'Retain', - 'DeletionPolicy': 'Retain', - }, - 'MyBucketF68F3FF0': { - 'Type': 'AWS::S3::Bucket', - 'Properties': { - 'BucketEncryption': { - 'ServerSideEncryptionConfiguration': [ - { - 'ServerSideEncryptionByDefault': { - 'KMSMasterKeyID': { + 'MyBucketF68F3FF0': { + 'Type': 'AWS::S3::Bucket', + 'Properties': { + 'BucketEncryption': { + 'ServerSideEncryptionConfiguration': [ + { + 'ServerSideEncryptionByDefault': { + 'KMSMasterKeyID': { + 'Fn::GetAtt': [ + 'MyBucketKeyC17130CF', + 'Arn', + ], + }, + 'SSEAlgorithm': 'aws:kms', + }, + }, + ], + }, + }, + 'UpdateReplacePolicy': 'Retain', + 'DeletionPolicy': 'Retain', + }, + 'MyUserDC45028B': { + 'Type': 'AWS::IAM::User', + }, + 'MyUserDefaultPolicy7B897426': { + 'Type': 'AWS::IAM::Policy', + 'Properties': { + 'PolicyDocument': { + 'Statement': [ + { + 'Action': [ + 's3:DeleteObject*', + 's3:PutObject*', + 's3:Abort*', + ], + 'Effect': 'Allow', + 'Resource': [ + { + 'Fn::GetAtt': [ + 'MyBucketF68F3FF0', + 'Arn', + ], + }, + { + 'Fn::Join': [ + '', + [ + { + 'Fn::GetAtt': [ + 'MyBucketF68F3FF0', + 'Arn', + ], + }, + '/*', + ], + ], + }, + ], + }, + { + 'Action': [ + 'kms:Encrypt', + 'kms:ReEncrypt*', + 'kms:GenerateDataKey*', + 'kms:Decrypt', + ], + 'Effect': 'Allow', + 'Resource': { 'Fn::GetAtt': [ 'MyBucketKeyC17130CF', 'Arn', ], }, - 'SSEAlgorithm': 'aws:kms', }, + ], + 'Version': '2012-10-17', + }, + 'PolicyName': 'MyUserDefaultPolicy7B897426', + 'Users': [ + { + 'Ref': 'MyUserDC45028B', }, ], }, }, - 'UpdateReplacePolicy': 'Retain', - 'DeletionPolicy': 'Retain', }, - 'MyUserDC45028B': { - 'Type': 'AWS::IAM::User', + }); + + test.done(); + }, + + 'does not grant PutObjectAcl when the S3_GRANT_WRITE_WITHOUT_ACL feature is enabled'(test: Test) { + const app = new cdk.App({ + context: { + [cxapi.S3_GRANT_WRITE_WITHOUT_ACL]: true, }, - 'MyUserDefaultPolicy7B897426': { - 'Type': 'AWS::IAM::Policy', - 'Properties': { - 'PolicyDocument': { - 'Statement': [ - { - 'Action': [ - 's3:DeleteObject*', - 's3:PutObject*', - 's3:Abort*', - ], - 'Effect': 'Allow', - 'Resource': [ - { - 'Fn::GetAtt': [ - 'MyBucketF68F3FF0', - 'Arn', - ], - }, - { - 'Fn::Join': [ - '', - [ - { - 'Fn::GetAtt': [ - 'MyBucketF68F3FF0', - 'Arn', - ], - }, - '/*', - ], - ], - }, - ], - }, + }); + const stack = new cdk.Stack(app, 'Stack'); + const bucket = new s3.Bucket(stack, 'MyBucket'); + const user = new iam.User(stack, 'MyUser'); + + bucket.grantWrite(user); + + expect(stack).to(haveResourceLike('AWS::IAM::Policy', { + 'PolicyDocument': { + 'Statement': [ + { + 'Action': [ + 's3:DeleteObject*', + 's3:PutObject', + 's3:Abort*', + ], + 'Resource': [ + { 'Fn::GetAtt': ['MyBucketF68F3FF0', 'Arn'] }, { - 'Action': [ - 'kms:Encrypt', - 'kms:ReEncrypt*', - 'kms:GenerateDataKey*', - 'kms:Decrypt', - ], - 'Effect': 'Allow', - 'Resource': { - 'Fn::GetAtt': [ - 'MyBucketKeyC17130CF', - 'Arn', - ], - }, + 'Fn::Join': ['', [ + { 'Fn::GetAtt': ['MyBucketF68F3FF0', 'Arn'] }, + '/*', + ]], }, ], - 'Version': '2012-10-17', }, - 'PolicyName': 'MyUserDefaultPolicy7B897426', - 'Users': [ - { - 'Ref': 'MyUserDC45028B', + ], + }, + })); + + test.done(); + }, + }, + + 'grantPut': { + 'does not grant PutObjectAcl when the S3_GRANT_WRITE_WITHOUT_ACL feature is enabled'(test: Test) { + const app = new cdk.App({ + context: { + [cxapi.S3_GRANT_WRITE_WITHOUT_ACL]: true, + }, + }); + const stack = new cdk.Stack(app, 'Stack'); + const bucket = new s3.Bucket(stack, 'MyBucket'); + const user = new iam.User(stack, 'MyUser'); + + bucket.grantPut(user); + + expect(stack).to(haveResourceLike('AWS::IAM::Policy', { + 'PolicyDocument': { + 'Statement': [ + { + 'Action': [ + 's3:PutObject', + 's3:Abort*', + ], + 'Resource': { + 'Fn::Join': ['', [ + { 'Fn::GetAtt': ['MyBucketF68F3FF0', 'Arn'] }, + '/*', + ]], }, - ], - }, + }, + ], }, - }, - }); + })); - test.done(); + test.done(); + }, }, 'more grants'(test: Test) { diff --git a/packages/@aws-cdk/cx-api/lib/features.ts b/packages/@aws-cdk/cx-api/lib/features.ts index 499c8e3438fe8..58b77595d2f36 100644 --- a/packages/@aws-cdk/cx-api/lib/features.ts +++ b/packages/@aws-cdk/cx-api/lib/features.ts @@ -80,6 +80,15 @@ export const SECRETS_MANAGER_PARSE_OWNED_SECRET_NAME = '@aws-cdk/aws-secretsmana */ export const KMS_DEFAULT_KEY_POLICIES = '@aws-cdk/aws-kms:defaultKeyPolicies'; +/** + * Change the old 's3:PutObject*' permission to 's3:PutObject', + * as the former includes 's3:PutObjectAcl', + * which allows changing the visibility of an object written to the Bucket. + * Use a feature flag to make sure existing customers who might be relying + * on the overly-broad permissions are not broken. + */ +export const S3_GRANT_WRITE_WITHOUT_ACL = '@aws-cdk/aws-s3:grantWriteWithoutAcl'; + /** * This map includes context keys and values for feature flags that enable * capabilities "from the future", which we could not introduce as the default @@ -100,6 +109,7 @@ export const FUTURE_FLAGS = { [DOCKER_IGNORE_SUPPORT]: true, [SECRETS_MANAGER_PARSE_OWNED_SECRET_NAME]: true, [KMS_DEFAULT_KEY_POLICIES]: true, + [S3_GRANT_WRITE_WITHOUT_ACL]: true, // We will advertise this flag when the feature is complete // [NEW_STYLE_STACK_SYNTHESIS_CONTEXT]: 'true', @@ -117,6 +127,7 @@ const FUTURE_FLAGS_DEFAULTS: { [key: string]: boolean } = { [DOCKER_IGNORE_SUPPORT]: false, [SECRETS_MANAGER_PARSE_OWNED_SECRET_NAME]: false, [KMS_DEFAULT_KEY_POLICIES]: false, + [S3_GRANT_WRITE_WITHOUT_ACL]: false, }; export function futureFlagDefault(flag: string): boolean {