From 872646788feb9b72bd5977331dc17c492b63a3f7 Mon Sep 17 00:00:00 2001 From: crashGoBoom Date: Mon, 1 Feb 2021 09:23:25 -0500 Subject: [PATCH 1/3] feat(aws-s3): adds s3 bucket AWS FSBP option This adds an option to enforce ssl for s3 buckets. Closes #10969 Signed-off-by: crashGoBoom --- packages/@aws-cdk/aws-s3/lib/bucket.ts | 35 ++++++++++++- packages/@aws-cdk/aws-s3/test/bucket.test.ts | 52 ++++++++++++++++++++ 2 files changed, 86 insertions(+), 1 deletion(-) diff --git a/packages/@aws-cdk/aws-s3/lib/bucket.ts b/packages/@aws-cdk/aws-s3/lib/bucket.ts index 5c5abfbfec559..c92de5dba4ca5 100644 --- a/packages/@aws-cdk/aws-s3/lib/bucket.ts +++ b/packages/@aws-cdk/aws-s3/lib/bucket.ts @@ -1064,6 +1064,14 @@ export interface BucketProps { */ readonly encryptionKey?: kms.IKey; + /** + * Enforces SSL for requests. S3.5 of the AWS Foundational Security Best Practices Regarding S3. + * @see https://docs.aws.amazon.com/config/latest/developerguide/s3-bucket-ssl-requests-only.html + * + * @default false + */ + readonly enforceSSL?: boolean; + /** * Specifies whether Amazon S3 should use an S3 Bucket Key with server-side * encryption using KMS (SSE-KMS) for new objects in the bucket. @@ -1310,6 +1318,8 @@ export class Bucket extends BucketBase { private readonly metrics: BucketMetrics[] = []; private readonly cors: CorsRule[] = []; private readonly inventories: Inventory[] = []; + private readonly enforceSSL?: boolean; + private readonly blockPublicAccess: BlockPublicAccess | undefined; constructor(scope: Construct, id: string, props: BucketProps = {}) { super(scope, id, { @@ -1319,6 +1329,8 @@ export class Bucket extends BucketBase { const { bucketEncryption, encryptionKey } = this.parseEncryption(props); this.validateBucketName(this.physicalName); + this.blockPublicAccess = props.blockPublicAccess; + this.enforceSSL = props.enforceSSL; const websiteConfiguration = this.renderWebsiteConfiguration(props); this.isWebsite = (websiteConfiguration !== undefined); @@ -1357,9 +1369,14 @@ export class Bucket extends BucketBase { this.bucketDualStackDomainName = resource.attrDualStackDomainName; this.bucketRegionalDomainName = resource.attrRegionalDomainName; - this.disallowPublicAccess = props.blockPublicAccess && props.blockPublicAccess.blockPublicPolicy; + this.disallowPublicAccess = this.blockPublicAccess && this.blockPublicAccess.blockPublicPolicy; this.accessControl = props.accessControl; + // Enforce AWS Foundational Security Best Practice + if (this.enforceSSL) { + this.enforceSSLStatement(); + } + if (props.serverAccessLogsBucket instanceof Bucket) { props.serverAccessLogsBucket.allowLogDelivery(); } @@ -1482,6 +1499,22 @@ export class Bucket extends BucketBase { this.inventories.push(inventory); } + /** + * Adds an iam statement to enforce SSL requests only. + */ + private enforceSSLStatement() { + const statement = new iam.PolicyStatement({ + actions: ['s3:*'], + conditions: { + Bool: { 'aws:SecureTransport': 'false' }, + }, + effect: iam.Effect.DENY, + resources: [this.arnForObjects('*')], + principals: [new iam.AnyPrincipal()], + }); + this.addToResourcePolicy(statement); + } + private validateBucketName(physicalName: string): void { const bucketName = physicalName; if (!bucketName || Token.isUnresolved(bucketName)) { diff --git a/packages/@aws-cdk/aws-s3/test/bucket.test.ts b/packages/@aws-cdk/aws-s3/test/bucket.test.ts index adf4858bae8c8..aa2174383b9f0 100644 --- a/packages/@aws-cdk/aws-s3/test/bucket.test.ts +++ b/packages/@aws-cdk/aws-s3/test/bucket.test.ts @@ -335,6 +335,58 @@ describe('bucket', () => { }); + test('enforceSsl can be enabled', () => { + const stack = new cdk.Stack(); + new s3.Bucket(stack, 'MyBucket', { enforceSSL: true }); + + expect(stack).toMatchTemplate({ + 'Resources': { + 'MyBucketF68F3FF0': { + 'Type': 'AWS::S3::Bucket', + 'UpdateReplacePolicy': 'Retain', + 'DeletionPolicy': 'Retain', + }, + 'MyBucketPolicyE7FBAC7B': { + 'Type': 'AWS::S3::BucketPolicy', + 'Properties': { + 'Bucket': { + 'Ref': 'MyBucketF68F3FF0', + }, + 'PolicyDocument': { + 'Statement': [ + { + 'Action': 's3:*', + 'Condition': { + 'Bool': { + 'aws:SecureTransport': 'false', + }, + }, + 'Effect': 'Deny', + 'Principal': '*', + 'Resource': { + 'Fn::Join': [ + '', + [ + { + 'Fn::GetAtt': [ + 'MyBucketF68F3FF0', + 'Arn', + ], + }, + '/*', + ], + ], + }, + }, + ], + 'Version': '2012-10-17', + }, + }, + }, + }, + }); + }); + test('bucketKeyEnabled can be enabled', () => { const stack = new cdk.Stack(); From 68431f13e27580afab5d4ff1b352c651de7108ec Mon Sep 17 00:00:00 2001 From: crashGoBoom Date: Mon, 1 Feb 2021 09:56:08 -0500 Subject: [PATCH 2/3] Fixes lint error Signed-off-by: crashGoBoom --- packages/@aws-cdk/aws-s3/lib/bucket.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/@aws-cdk/aws-s3/lib/bucket.ts b/packages/@aws-cdk/aws-s3/lib/bucket.ts index c92de5dba4ca5..f1eb024426189 100644 --- a/packages/@aws-cdk/aws-s3/lib/bucket.ts +++ b/packages/@aws-cdk/aws-s3/lib/bucket.ts @@ -1070,7 +1070,7 @@ export interface BucketProps { * * @default false */ - readonly enforceSSL?: boolean; + readonly enforceSSL?: boolean; /** * Specifies whether Amazon S3 should use an S3 Bucket Key with server-side From ee75effe34a15d1e1e3f7e92770c0d016c311292 Mon Sep 17 00:00:00 2001 From: crashGoBoom Date: Sun, 21 Feb 2021 17:30:18 -0500 Subject: [PATCH 3/3] Add details to readme and refactor for suggestions Signed-off-by: crashGoBoom --- packages/@aws-cdk/aws-s3/README.md | 12 ++++++++++++ packages/@aws-cdk/aws-s3/lib/bucket.ts | 8 ++------ 2 files changed, 14 insertions(+), 6 deletions(-) diff --git a/packages/@aws-cdk/aws-s3/README.md b/packages/@aws-cdk/aws-s3/README.md index 1ce00f56ba0a9..a6c1c1b607c23 100644 --- a/packages/@aws-cdk/aws-s3/README.md +++ b/packages/@aws-cdk/aws-s3/README.md @@ -122,6 +122,18 @@ bucket.grantReadWrite(lambda); Will give the Lambda's execution role permissions to read and write from the bucket. +## AWS Foundational Security Best Practices + +### Enforcing SSL + +To require all requests use Secure Socket Layer (SSL): + +```ts +const bucket = new Bucket(this, 'Bucket', { + enforceSSL: true +}); +``` + ## Sharing buckets between stacks To use a bucket in a different stack in the same CDK application, pass the object to the other stack: diff --git a/packages/@aws-cdk/aws-s3/lib/bucket.ts b/packages/@aws-cdk/aws-s3/lib/bucket.ts index f1eb024426189..01c9fa3cfad14 100644 --- a/packages/@aws-cdk/aws-s3/lib/bucket.ts +++ b/packages/@aws-cdk/aws-s3/lib/bucket.ts @@ -1318,8 +1318,6 @@ export class Bucket extends BucketBase { private readonly metrics: BucketMetrics[] = []; private readonly cors: CorsRule[] = []; private readonly inventories: Inventory[] = []; - private readonly enforceSSL?: boolean; - private readonly blockPublicAccess: BlockPublicAccess | undefined; constructor(scope: Construct, id: string, props: BucketProps = {}) { super(scope, id, { @@ -1329,8 +1327,6 @@ export class Bucket extends BucketBase { const { bucketEncryption, encryptionKey } = this.parseEncryption(props); this.validateBucketName(this.physicalName); - this.blockPublicAccess = props.blockPublicAccess; - this.enforceSSL = props.enforceSSL; const websiteConfiguration = this.renderWebsiteConfiguration(props); this.isWebsite = (websiteConfiguration !== undefined); @@ -1369,11 +1365,11 @@ export class Bucket extends BucketBase { this.bucketDualStackDomainName = resource.attrDualStackDomainName; this.bucketRegionalDomainName = resource.attrRegionalDomainName; - this.disallowPublicAccess = this.blockPublicAccess && this.blockPublicAccess.blockPublicPolicy; + this.disallowPublicAccess = props.blockPublicAccess && props.blockPublicAccess.blockPublicPolicy; this.accessControl = props.accessControl; // Enforce AWS Foundational Security Best Practice - if (this.enforceSSL) { + if (props.enforceSSL) { this.enforceSSLStatement(); }