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

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 5 commits
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: 30 additions & 0 deletions packages/@aws-cdk/aws-s3/README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -208,3 +208,33 @@ bucket.onEvent(s3.EventType.ObjectRemoved, myQueue, { prefix: 'foo/', suffix: '.
```

[S3 Bucket Notifications]: https://docs.aws.amazon.com/AmazonS3/latest/dev/NotificationHowTo.html


### Block Public Access

Use `blockPublicAccess` to specify [block public access settings] on the bucket.

Enable all block public access settings:
```ts
const bucket = new Bucket(this, 'MyBlockedBucket', {
blockPublicAccess: BlockPublicAccess.BlockAll
});
```

Block and ignore public ACLs:
```ts
const bucket = new Bucket(this, 'MyBlockedBucket', {
blockPublicAccess: BlockPublicAccess.BlockAcls
});
```

Alternatively, specify the settings manually:
```ts
const bucket = new Bucket(this, 'MyBlockedBucket', {
blockPublicAccess: new BlockPublicAccess({ blockPublicPolicy: true })
});
```

When `blockPublicPolicy` is set to `true`, `grantPublicRead()` throws an error.

[block public access settings]: https://docs.aws.amazon.com/AmazonS3/latest/dev/access-control-block-public-access.html
80 changes: 79 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;

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

Choose a reason for hiding this comment

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

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...


/**
* 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.blockPublicAccess && this.blockPublicAccess.blockPublicPolicy) {
throw new Error("Cannot grant public access when 'blockPublicPolicy' is enabled");
}

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

const statement = new iam.PolicyStatement()
Expand Down Expand Up @@ -555,6 +564,62 @@ export abstract class BucketBase extends cdk.Construct implements IBucket {
}
}

export interface BlockPublicAccessOptions {
/**
* Whether to block public ACLs
*
* @see https://docs.aws.amazon.com/AmazonS3/latest/dev/access-control-block-public-access.html#access-control-block-public-access-options
*/
blockPublicAcls?: boolean;

/**
* Whether to block public policy
*
* @see https://docs.aws.amazon.com/AmazonS3/latest/dev/access-control-block-public-access.html#access-control-block-public-access-options
*/
blockPublicPolicy?: boolean;

/**
* Whether to ignore public ACLs
*
* @see https://docs.aws.amazon.com/AmazonS3/latest/dev/access-control-block-public-access.html#access-control-block-public-access-options
*/
ignorePublicAcls?: boolean;

/**
* Whether to restrict public access
*
* @see https://docs.aws.amazon.com/AmazonS3/latest/dev/access-control-block-public-access.html#access-control-block-public-access-options
*/
restrictPublicBuckets?: boolean;
}

export class BlockPublicAccess {
public static readonly BlockAll = new BlockPublicAccess({
blockPublicAcls: true,
blockPublicPolicy: true,
ignorePublicAcls: true,
restrictPublicBuckets: true
});

public static readonly BlockAcls = new BlockPublicAccess({
blockPublicAcls: true,
ignorePublicAcls: true
});

public blockPublicAcls: boolean | undefined;
public blockPublicPolicy: boolean | undefined;
public ignorePublicAcls: boolean | undefined;
public restrictPublicBuckets: boolean | undefined;

constructor(options: BlockPublicAccessOptions) {
this.blockPublicAcls = options.blockPublicAcls;
this.blockPublicPolicy = options.blockPublicPolicy;
this.ignorePublicAcls = options.ignorePublicAcls;
this.restrictPublicBuckets = options.restrictPublicBuckets;
}
}

export interface BucketProps {
/**
* The kind of server-side encryption to apply to this bucket.
Expand Down Expand Up @@ -623,6 +688,13 @@ export interface BucketProps {
* Similar to calling `bucket.grantPublicAccess()`
*/
publicReadAccess?: boolean;

/**
* The block public access configuration of this 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

* @see https://docs.aws.amazon.com/AmazonS3/latest/dev/access-control-block-public-access.html
*/
blockPublicAccess?: BlockPublicAccess;
}

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

});

cdk.applyRemovalPolicy(resource, props.removalPolicy !== undefined ? props.removalPolicy : cdk.RemovalPolicy.Orphan);
Expand All @@ -678,6 +752,7 @@ export class Bucket extends BucketBase {
this.domainName = resource.bucketDomainName;
this.bucketWebsiteUrl = resource.bucketWebsiteUrl;
this.dualstackDomainName = resource.bucketDualStackDomainName;
this.blockPublicAccess = props.blockPublicAccess;

// Add all lifecycle rules
(props.lifecycleRules || []).forEach(this.addLifecycleRule.bind(this));
Expand Down Expand Up @@ -1042,6 +1117,8 @@ class ImportedBucket extends BucketBase {
public policy?: BucketPolicy;
protected autoCreatePolicy: boolean;

protected blockPublicAccess?: BlockPublicAccess;

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

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

/**
Expand Down
83 changes: 83 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,76 @@ export = {
test.done();
},

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

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();
},

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

expect(stack).toMatch({
"Resources": {
"MyBucketF68F3FF0": {
"Type": "AWS::S3::Bucket",
"Properties": {
"PublicAccessBlockConfiguration": {
"BlockPublicAcls": true,
"IgnorePublicAcls": true,
}
},
"DeletionPolicy": "Retain",
}
}
});
test.done();
},

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

expect(stack).toMatch({
"Resources": {
"MyBucketF68F3FF0": {
"Type": "AWS::S3::Bucket",
"Properties": {
"PublicAccessBlockConfiguration": {
"RestrictPublicBuckets": true,
}
},
"DeletionPolicy": "Retain",
}
}
});
test.done();
},

'permissions': {

'addPermission creates a bucket policy'(test: Test) {
Expand Down Expand Up @@ -1175,6 +1245,19 @@ export = {
"Version": "2012-10-17"
}
}));
test.done();
},

'throws when blockPublicPolicy is set to true'(test: Test) {
// GIVEN
const stack = new cdk.Stack();
const bucket = new s3.Bucket(stack, 'MyBucket', {
blockPublicAccess: new s3.BlockPublicAccess({ blockPublicPolicy: true })
});

// THEN
test.throws(() => bucket.grantPublicAccess(), /blockPublicPolicy/);

test.done();
}
},
Expand Down