Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

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

Merged
merged 6 commits into from
Feb 4, 2019
Merged

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

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
716f615
feat(aws-s3): add option to enable all bock public access settings
jogold Feb 4, 2019
b48f9ee
PR review
jogold Feb 4, 2019
ceb6393
Add test
jogold Feb 4, 2019
20b9581
Add BlockPublicAccess class
jogold Feb 4, 2019
93ac3e4
Fix typo in README
Feb 4, 2019
3ab6e3b
Revert to disallowPublicAccess
jogold Feb 4, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
30 changes: 29 additions & 1 deletion packages/@aws-cdk/aws-s3/lib/bucket.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -293,6 +293,11 @@ export abstract class BucketBase extends cdk.Construct implements IBucket {
*/
protected abstract autoCreatePolicy = false;

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is indeed better, done.


/**
* Exports this bucket from the stack.
*/
Expand Down Expand Up @@ -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');
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Change "block public access" to "blockPublicAccess"

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

}

allowedActions = allowedActions.length > 0 ? allowedActions : [ 's3:GetObject' ];

const statement = new iam.PolicyStatement()
Expand Down Expand Up @@ -623,6 +632,13 @@ export interface BucketProps {
* Similar to calling `bucket.grantPublicAccess()`
*/
publicReadAccess?: boolean;

/**
* Enables all block public access settings on the bucket.
*
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add a "@see" with a link

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

* @default false
*/
blockPublicAccess?: boolean;
}

/**
Expand Down Expand Up @@ -652,6 +668,7 @@ export class Bucket extends BucketBase {
public readonly encryptionKey?: kms.IEncryptionKey;
public policy?: BucketPolicy;
protected autoCreatePolicy = true;
protected publicAccessBlocked?: boolean;
private readonly lifecycleRules: LifecycleRule[] = [];
private readonly versioned?: boolean;
private readonly notifications: BucketNotifications;
Expand All @@ -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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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) { ... }
}

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jogold have you seen this comment?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

? {
blockPublicAcls: true,
blockPublicPolicy: true,
ignorePublicAcls: true,
restrictPublicBuckets: true
}
: undefined
});

cdk.applyRemovalPolicy(resource, props.removalPolicy !== undefined ? props.removalPolicy : cdk.RemovalPolicy.Orphan);
Expand Down Expand Up @@ -1042,6 +1067,8 @@ class ImportedBucket extends BucketBase {
public policy?: BucketPolicy;
protected autoCreatePolicy: boolean;

protected publicAccessBlocked: boolean;

constructor(scope: cdk.Construct, id: string, private readonly props: BucketImportProps) {
super(scope, id);

Expand All @@ -1059,6 +1086,7 @@ class ImportedBucket extends BucketBase {
? false
: props.bucketWebsiteNewUrlFormat;
this.policy = undefined;
this.publicAccessBlocked = false;
}

/**
Expand Down
25 changes: 25 additions & 0 deletions packages/@aws-cdk/aws-s3/test/test.bucket.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -201,6 +201,31 @@ export = {
test.done();
},

'bucket with block public access set to true'(test: Test) {
const stack = new cdk.Stack();
new s3.Bucket(stack, 'MyBucket', {
blockPublicAccess: true,
});

expect(stack).toMatch({
"Resources": {
"MyBucketF68F3FF0": {
"Type": "AWS::S3::Bucket",
"Properties": {
"PublicAccessBlockConfiguration": {
"BlockPublicAcls": true,
"BlockPublicPolicy": true,
"IgnorePublicAcls": true,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Verify behavior for imported buckets

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

"RestrictPublicBuckets": true,
}
},
"DeletionPolicy": "Retain",
}
}
});
test.done();
},

'permissions': {

'addPermission creates a bucket policy'(test: Test) {
Expand Down