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

chore(s3): use bucket policy instead of ACL for logging access control #20898

Closed
wants to merge 3 commits into from
Closed

chore(s3): use bucket policy instead of ACL for logging access control #20898

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
c00859a
chore(s3): use bucket policy instead of ACL for logging access control
otaviomacedo Jun 28, 2022
436f648
Added source account condition
otaviomacedo Jun 28, 2022
fab78e2
Removed bucket owner enforced control
otaviomacedo Jul 4, 2022
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
38 changes: 26 additions & 12 deletions 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 @@ -2,6 +2,7 @@ import { EOL } from 'os';
import * as path from 'path';
import * as events from '@aws-cdk/aws-events';
import * as iam from '@aws-cdk/aws-iam';
import { PolicyStatement, ServicePrincipal } from '@aws-cdk/aws-iam';
import * as kms from '@aws-cdk/aws-kms';
import {
Fn, IResource, Lazy, RemovalPolicy, Resource, ResourceProps, Stack, Token,
Expand Down Expand Up @@ -1658,6 +1659,7 @@ export class Bucket extends BucketBase {
private readonly cors: CorsRule[] = [];
private readonly inventories: Inventory[] = [];
private readonly _resource: CfnBucket;
private objectOwnership?: ObjectOwnership;

constructor(scope: Construct, id: string, props: BucketProps = {}) {
super(scope, id, {
Expand Down Expand Up @@ -1685,7 +1687,7 @@ export class Bucket extends BucketBase {
accessControl: Lazy.string({ produce: () => this.accessControl }),
loggingConfiguration: this.parseServerAccessLogs(props),
inventoryConfigurations: Lazy.any({ produce: () => this.parseInventoryConfiguration() }),
ownershipControls: this.parseOwnershipControls(props),
ownershipControls: Lazy.any({ produce: () => this.parseOwnershipControls() }),
accelerateConfiguration: props.transferAcceleration ? { accelerationStatus: 'Enabled' } : undefined,
intelligentTieringConfigurations: this.parseTieringConfig(props),
});
Expand Down Expand Up @@ -1713,14 +1715,15 @@ export class Bucket extends BucketBase {

this.disallowPublicAccess = props.blockPublicAccess && props.blockPublicAccess.blockPublicPolicy;
this.accessControl = props.accessControl;
this.objectOwnership = props.objectOwnership;

// Enforce AWS Foundational Security Best Practice
if (props.enforceSSL) {
this.enforceSSLStatement();
}

if (props.serverAccessLogsBucket instanceof Bucket) {
props.serverAccessLogsBucket.allowLogDelivery();
props.serverAccessLogsBucket.allowLogDelivery(this, Stack.of(this).account, props.serverAccessLogsPrefix);
Copy link
Contributor

Choose a reason for hiding this comment

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

This is where we setup the log delivery policy/acl, but it only gets done if the user passes in a bucket to serverAccessLogsBucket. If a user provides serverAccessLogsPrefix but does not provide serverAccessLogsBucket, then access logs are sent to the source bucket. Does it need to be something like

if (props.serverAccessLogsBucket instanceof Bucket) {
  ...
} else if (props.serverAcessLogsPrefix && !props.serverAccessLogsBucket) {
  this.allowLogDelivery(this, Stack.of(this).account, props.serverAccessLogsPrefix);
}

It's not being done today so it could be a bug or it could be that you don't need to grant access to allow access logs to be written to the same bucket?

}

for (const inventory of props.inventories ?? []) {
Expand Down Expand Up @@ -1990,13 +1993,13 @@ export class Bucket extends BucketBase {
}));
}

private parseOwnershipControls({ objectOwnership }: BucketProps): CfnBucket.OwnershipControlsProperty | undefined {
if (!objectOwnership) {
private parseOwnershipControls(): CfnBucket.OwnershipControlsProperty | undefined {
if (!this.objectOwnership) {
return undefined;
}
return {
rules: [{
objectOwnership,
objectOwnership: this.objectOwnership,
}],
};
}
Expand Down Expand Up @@ -2069,17 +2072,28 @@ export class Bucket extends BucketBase {
}

/**
* Allows the LogDelivery group to write, fails if ACL was set differently.
* Allows the 'logging.s3.amazonaws.com' service principal to write log events
* to this bucket.
*
* @see
* https://docs.aws.amazon.com/AmazonS3/latest/dev/acl-overview.html#canned-acl
* https://docs.aws.amazon.com/AmazonS3/latest/userguide/enable-server-access-logging.html
*/
private allowLogDelivery() {
if (this.accessControl && this.accessControl !== BucketAccessControl.LOG_DELIVERY_WRITE) {
throw new Error("Cannot enable log delivery to this bucket because the bucket's ACL has been set and can't be changed");
}
private allowLogDelivery(sourceBucket: IBucket, accountId: string, logFilePrefix?: string) {
const prefix = logFilePrefix ?? '';

this.accessControl = BucketAccessControl.LOG_DELIVERY_WRITE;
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this need to be added back then?

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 think so, which means that this change adds no value. Closing the PR until CloudFront (and possibly other services) catches up.

this.addToResourcePolicy(new PolicyStatement({
principals: [new ServicePrincipal('logging.s3.amazonaws.com')],
actions: ['s3:PutObject'],
conditions: {
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this also include the condition:

"StringEquals": {
                    "aws:SourceAccount": "SOURCE-ACCOUNT-ID"
                }

https://docs.aws.amazon.com/AmazonS3/latest/userguide/enable-server-access-logging.html

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added.

Does the policy need to be added to the source bucket if the access logs are configured to be sent to the source bucket?

I think so. Otherwise, the service principal wouldn't have permission to write the logs.

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think this is being done today.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not sure if I understand... today the LOG_DELIVERY_WRITE ACL is applied to the target bucket. If it happens that the source and target are the same, then the ACL is applied to the source bucket. By the same logic, this change applies the policy to the source bucket if source and target are the same.

Copy link
Contributor

Choose a reason for hiding this comment

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

I added a comment that tries to clarify.

ArnLike: {
'aws:SourceArn': sourceBucket.bucketArn,
},
StringEquals: {
'aws:SourceAccount': accountId,
},
},
resources: [`${this.bucketArn}/${prefix}*`],
}));
}

private parseInventoryConfiguration(): CfnBucket.InventoryConfigurationProperty[] | undefined {
Expand Down
52 changes: 49 additions & 3 deletions ...aws-s3/test/bucket.server-access-logs.integ.snapshot/aws-cdk-s3-access-logs.template.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,12 +2,58 @@
"Resources": {
"MyAccessLogsBucketF7FE6635": {
"Type": "AWS::S3::Bucket",
"Properties": {
"AccessControl": "LogDeliveryWrite"
},
"UpdateReplacePolicy": "Delete",
"DeletionPolicy": "Delete"
},
"MyAccessLogsBucketPolicyEA9AB063": {
"Type": "AWS::S3::BucketPolicy",
"Properties": {
"Bucket": {
"Ref": "MyAccessLogsBucketF7FE6635"
},
"PolicyDocument": {
"Statement": [
{
"Action": "s3:PutObject",
"Condition": {
"ArnLike": {
"aws:SourceArn": {
"Fn::GetAtt": [
"MyBucketF68F3FF0",
"Arn"
]
}
},
"StringEquals": {
"aws:SourceAccount": {
"Ref": "AWS::AccountId"
}
}
},
"Effect": "Allow",
"Principal": {
"Service": "logging.s3.amazonaws.com"
},
"Resource": {
"Fn::Join": [
"",
[
{
"Fn::GetAtt": [
"MyAccessLogsBucketF7FE6635",
"Arn"
]
},
"/example*"
]
]
}
}
],
"Version": "2012-10-17"
}
}
},
"MyBucketF68F3FF0": {
"Type": "AWS::S3::Bucket",
"Properties": {
Expand Down
2 changes: 1 addition & 1 deletion packages/@aws-cdk/aws-s3/test/bucket.server-access-logs.integ.snapshot/cdk.out
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1 +1 @@
{"version":"17.0.0"}
{"version":"20.0.0"}
4 changes: 2 additions & 2 deletions packages/@aws-cdk/aws-s3/test/bucket.server-access-logs.integ.snapshot/integ.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
{
"version": "18.0.0",
"version": "20.0.0",
"testCases": {
"aws-s3/test/integ.bucket.server-access-logs": {
"integ.bucket.server-access-logs": {
"stacks": [
"aws-cdk-s3-access-logs"
],
Expand Down
8 changes: 7 additions & 1 deletion packages/@aws-cdk/aws-s3/test/bucket.server-access-logs.integ.snapshot/manifest.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
{
"version": "17.0.0",
"version": "20.0.0",
"artifacts": {
"Tree": {
"type": "cdk:tree",
Expand All @@ -21,6 +21,12 @@
"data": "MyAccessLogsBucketF7FE6635"
}
],
"/aws-cdk-s3-access-logs/MyAccessLogsBucket/Policy/Resource": [
{
"type": "aws:cdk:logicalId",
"data": "MyAccessLogsBucketPolicyEA9AB063"
}
],
"/aws-cdk-s3-access-logs/MyBucket/Resource": [
{
"type": "aws:cdk:logicalId",
Expand Down
71 changes: 68 additions & 3 deletions packages/@aws-cdk/aws-s3/test/bucket.server-access-logs.integ.snapshot/tree.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,14 +25,79 @@
"path": "aws-cdk-s3-access-logs/MyAccessLogsBucket/Resource",
"attributes": {
"aws:cdk:cloudformation:type": "AWS::S3::Bucket",
"aws:cdk:cloudformation:props": {
"accessControl": "LogDeliveryWrite"
}
"aws:cdk:cloudformation:props": {}
},
"constructInfo": {
"fqn": "@aws-cdk/aws-s3.CfnBucket",
"version": "0.0.0"
}
},
"Policy": {
"id": "Policy",
"path": "aws-cdk-s3-access-logs/MyAccessLogsBucket/Policy",
"children": {
"Resource": {
"id": "Resource",
"path": "aws-cdk-s3-access-logs/MyAccessLogsBucket/Policy/Resource",
"attributes": {
"aws:cdk:cloudformation:type": "AWS::S3::BucketPolicy",
"aws:cdk:cloudformation:props": {
"bucket": {
"Ref": "MyAccessLogsBucketF7FE6635"
},
"policyDocument": {
"Statement": [
{
"Action": "s3:PutObject",
"Condition": {
"ArnLike": {
"aws:SourceArn": {
"Fn::GetAtt": [
"MyBucketF68F3FF0",
"Arn"
]
}
},
"StringEquals": {
"aws:SourceAccount": {
"Ref": "AWS::AccountId"
}
}
},
"Effect": "Allow",
"Principal": {
"Service": "logging.s3.amazonaws.com"
},
"Resource": {
"Fn::Join": [
"",
[
{
"Fn::GetAtt": [
"MyAccessLogsBucketF7FE6635",
"Arn"
]
},
"/example*"
]
]
}
}
],
"Version": "2012-10-17"
}
}
},
"constructInfo": {
"fqn": "@aws-cdk/aws-s3.CfnBucketPolicy",
"version": "0.0.0"
}
}
},
"constructInfo": {
"fqn": "@aws-cdk/aws-s3.BucketPolicy",
"version": "0.0.0"
}
}
},
"constructInfo": {
Expand Down
17 changes: 0 additions & 17 deletions packages/@aws-cdk/aws-s3/test/bucket.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2181,23 +2181,6 @@ describe('bucket', () => {
});
});

test('Bucket Allow Log delivery changes bucket Access Control should fail', () => {
// GIVEN
const stack = new cdk.Stack();

// WHEN
const accessLogBucket = new s3.Bucket(stack, 'AccessLogs', {
accessControl: s3.BucketAccessControl.AUTHENTICATED_READ,
});
expect(() =>
new s3.Bucket(stack, 'MyBucket', {
serverAccessLogsBucket: accessLogBucket,
serverAccessLogsPrefix: 'hello',
accessControl: s3.BucketAccessControl.AUTHENTICATED_READ,
}),
).toThrow(/Cannot enable log delivery to this bucket because the bucket's ACL has been set and can't be changed/);
});

test('Defaults for an inventory bucket', () => {
// Given
const stack = new cdk.Stack();
Expand Down