feat(aws-s3): add option to specify block public access settings #1664

jogold · 2019-02-04T11:29:41Z

Adds option to enable all block public access settings on the bucket (blockPublicAccess). Also throws an error when trying to use grantPublicAccess if blockPublicAccess is set to true.

https://docs.aws.amazon.com/AmazonS3/latest/dev/access-control-block-public-access.html

Pull Request Checklist

Testing
- Unit test added
- CLI change?: coordinate update of integration tests with team
- cdk-init template change?: coordinated update of integration tests with team
Docs
- jsdocs: All public APIs documented
- README: README and/or documentation topic updated
Title and Description
- Change type: title prefixed with fix, feat will appear in changelog
- Title: use lower-case and doesn't end with a period
- Breaking?: last paragraph: "BREAKING CHANGE: <describe what changed + link for details>"
- Issues: Indicate issues fixed via: "Fixes #xxx" or "Closes #xxx"
Sensitive Modules (requires 2 PR approvers)
- IAM Policy Document (in @aws-cdk/aws-iam)
- EC2 Security Groups and ACLs (in @aws-cdk/aws-ec2)
- Grant APIs (only if not based on official documentation with a reference)

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license.

eladb

Add a README entry on this

eladb · 2019-02-04T11:34:34Z

packages/@aws-cdk/aws-s3/lib/bucket.ts

@@ -514,6 +519,10 @@ export abstract class BucketBase extends cdk.Construct implements IBucket {
   * @returns The `iam.PolicyStatement` object, which can be used to apply e.g. conditions.
   */
  public grantPublicAccess(keyPrefix = '*', ...allowedActions: string[]): iam.PolicyStatement {
+    if (this.publicAccessBlocked) {
+      throw new Error('Cannot grant public access when block public access settings are enabled');


Change "block public access" to "blockPublicAccess"

eladb · 2019-02-04T11:35:42Z

packages/@aws-cdk/aws-s3/lib/bucket.ts

+
+  /**
+   * Enables all block public access settings on the bucket.
+   *


Add a "@see" with a link

eladb · 2019-02-04T11:41:35Z

packages/@aws-cdk/aws-s3/lib/bucket.ts

@@ -666,7 +683,15 @@ export class Bucket extends BucketBase {
      bucketEncryption,
      versioningConfiguration: props.versioned ? { status: 'Enabled' } : undefined,
      lifecycleConfiguration: new cdk.Token(() => this.parseLifecycleConfiguration()),
-      websiteConfiguration: this.renderWebsiteConfiguration(props)
+      websiteConfiguration: this.renderWebsiteConfiguration(props),
+      publicAccessBlockConfiguration: props.blockPublicAccess


We should support the entire surface area of the feature, but also find a way to provide a more ergonomic API.

How about using an "enum-like class":

export interface BlockPublicAccessOptions { blockPublicAcls?: boolean; ignorePublicAcls?: boolean; blockPublicPolicy?: boolean; restrictPublicBuckets?: boolean; } export class BlockPublicAccess { public static blockAll = new BlockPublicAccess({ blockPublicAcls: true, ...true, true, true... }); public static block... // maybe there are other canned settings we can provide constructor(options: BlockPublicAccessOptions) { ... } }

@jogold have you seen this comment?

Yes, I understand your point. I initially thought that a propertyOverrides would be sufficient here.

What kind of api/naming would like to see on the Bucket construct? props.blockPublicAccess becomes an instance of BlockPublicAccess? Do I need to add an interface BlockPublicAccessOptions as suggested or can I reuse the generated one (CfnBucket.PublicAccessBlockConfigurationProperty)

We try not to "leak" the CFN layer, eventually L2 should cover the entire surface area. We don't want people to need to traverse these layers to use prop overrides. Please define another props interface. In most cases it's not a 1:1 mapping (although it might be here). We also want to able to evolve those independently.

As for naming, blockPublicAccess of type BlockPublicAccess, so the usage would be:

blockPublicAccess: BlockPublicAccess.blockAll

(a bit of a mouthfull but not too bad)

See latest commit:

used BlockAll (capitalized)

added BlockAcls

grantPublicRead now only throws when blockPublicPolicy is enabled because it's only in this case that S3 rejects calls to PUT Bucket policy.

eladb · 2019-02-04T11:43:50Z

packages/@aws-cdk/aws-s3/lib/bucket.ts

+  /**
+   * Whether all block public access settings are enabled
+   */
+  protected abstract publicAccessBlocked?: boolean;


Since this is not really accurate for the case of an imported bucket, rename this to disallowPublicAccess. It will semantically reflect what this is actually used for instead falsely representing this configuration for imported buckets.

Is indeed better, done.

eladb · 2019-02-04T11:44:08Z

packages/@aws-cdk/aws-s3/test/test.bucket.ts

+            "PublicAccessBlockConfiguration": {
+              "BlockPublicAcls": true,
+              "BlockPublicPolicy": true,
+              "IgnorePublicAcls": true,


Verify behavior for imported buckets

What do you want me to check for imported buckets in the generated CF output? I don't get it.

Specifically, I think the code not covered is grantPublicAccess should throw an error, no?

eladb

#1664 (comment)

eladb · 2019-02-04T23:07:46Z

packages/@aws-cdk/aws-s3/lib/bucket.ts

+  /**
+   * The block public access configuration of this bucket.
+   */
+  protected abstract blockPublicAccess?: BlockPublicAccess;


I actually think this should have remained disallowPublicAccess because it reflects the intent to disallow grantPublicAccess and not the specific type of public access blocked during configuration. Otherwise, you go back to faking it with the imported resource...

eladb · 2019-02-04T23:59:57Z

❤️ thanks!

…#1664) Adds option to enable all block public access settings on the bucket (`blockPublicAccess`). Also throws an error when trying to use grantPublicAccess if blockPublicAccess is set to true. https://docs.aws.amazon.com/AmazonS3/latest/dev/access-control-block-public-access.html

feat(aws-s3): add option to enable all bock public access settings

716f615

jogold requested a review from a team as a code owner February 4, 2019 11:29

eladb reviewed Feb 4, 2019

View reviewed changes

jogold added 2 commits February 4, 2019 15:57

PR review

b48f9ee

Add test

ceb6393

eladb approved these changes Feb 4, 2019

View reviewed changes

eladb changed the title ~~feat(aws-s3): add option to enable all bock public access settings~~ feat(aws-s3): add option to enable all block public access settings Feb 4, 2019

eladb suggested changes Feb 4, 2019

View reviewed changes

Add BlockPublicAccess class

20b9581

jogold changed the title ~~feat(aws-s3): add option to enable all block public access settings~~ feat(aws-s3): add option to specify block public access settings Feb 4, 2019

Fix typo in README

93ac3e4

eladb reviewed Feb 4, 2019

View reviewed changes

Revert to disallowPublicAccess

3ab6e3b

eladb approved these changes Feb 4, 2019

View reviewed changes

eladb merged commit 299fb6a into aws:master Feb 4, 2019

jogold deleted the s3-block-public-access branch March 12, 2019 11:23

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(aws-s3): add option to specify block public access settings #1664

feat(aws-s3): add option to specify block public access settings #1664

jogold commented Feb 4, 2019

eladb left a comment

eladb Feb 4, 2019

jogold Feb 4, 2019

eladb Feb 4, 2019

jogold Feb 4, 2019

eladb Feb 4, 2019

eladb Feb 4, 2019

jogold Feb 4, 2019

eladb Feb 4, 2019

jogold Feb 4, 2019

eladb Feb 4, 2019

jogold Feb 4, 2019

eladb Feb 4, 2019

jogold Feb 4, 2019

eladb Feb 4, 2019

eladb left a comment

eladb Feb 4, 2019

eladb commented Feb 4, 2019

feat(aws-s3): add option to specify block public access settings #1664

feat(aws-s3): add option to specify block public access settings #1664

Conversation

jogold commented Feb 4, 2019

Pull Request Checklist

eladb left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eladb left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eladb commented Feb 4, 2019