Skip to content

Commit 6975a7e

Browse files
laurelmaylaurelmay
authored
feat(s3): use Bucket Policy for Server Access Logging grant (under feature flag) (#23386)
Using ACLs to grant access to buckets is no longer recommended. In fact, it doesn't work if Object Ownership is set to be enforced for the bucket. According to the service documentation for [enabling server access logging][1], it is now preferred to use a bucket policy to grant permission to deliver logs to a bucket. Changing the default would result in changes to deployed resources, so the new behavior is added behind a feature flag. An alternative here may be to use the Bucket Policy either when the feature flag is enabled or when ownership is set to `BUCKET_OWNER_ENFORCED` since the latter doesn't work with the current implementation anyway. Closes: #22183 [1]: https://docs.aws.amazon.com/AmazonS3/latest/userguide/enable-server-access-logging.html ---- ### All Submissions: * [X] Have you followed the guidelines in our [Contributing guide?](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md) ### Adding new Construct Runtime Dependencies: * [ ] This PR adds new construct runtime dependencies following the process described [here](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md/#adding-construct-runtime-dependencies) ### New Features * [X] Have you added the new feature to an [integration test](https://github.com/aws/aws-cdk/blob/main/INTEGRATION_TESTS.md)? * [X] Did you use `yarn integ` to deploy the infrastructure and generate the snapshot (i.e. `yarn integ` without `--dry-run`)? *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
1 parent 7c752db commit 6975a7e

File tree

12 files changed

+345
-41
lines changed

12 files changed

+345
-41
lines changed

‎packages/@aws-cdk/aws-s3/README.md

Lines changed: 29 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -349,7 +349,25 @@ const bucket = new s3.Bucket(this, 'MyBucket', {
349349
});
350350
```
351351

352-
[S3 Server access logging]: https://docs.aws.amazon.com/AmazonS3/latest/dev/ServerLogs.html
352+
### Allowing access log delivery using a Bucket Policy (recommended)
353+
354+
When possible, it is recommended to use a bucket policy to grant access instead of
355+
using ACLs. When the `@aws-cdk/aws-s3:serverAccessLogsUseBucketPolicy` feature flag
356+
is enabled, this is done by default for server access logs. If S3 Server Access Logs
357+
are the only logs delivered to your bucket (or if all other services logging to the
358+
bucket support using bucket policy instead of ACLs), you can set object ownership
359+
to [bucket owner enforced](#bucket-owner-enforced-recommended), as is recommended.
360+
361+
```ts
362+
const accessLogsBucket = new s3.Bucket(this, 'AccessLogsBucket', {
363+
objectOwnership: s3.ObjectOwnership.BUCKET_OWNER_ENFORCED,
364+
});
365+
366+
const bucket = new s3.Bucket(this, 'MyBucket', {
367+
serverAccessLogsBucket: accessLogsBucket,
368+
serverAccessLogsPrefix: 'logs',
369+
});
370+
```
353371

354372
## S3 Inventory
355373

@@ -485,14 +503,23 @@ new s3.Bucket(this, 'MyBucket', {
485503

486504
### Bucket owner enforced (recommended)
487505

488-
ACLs are disabled, and the bucket owner automatically owns and has full control over every object in the bucket. ACLs no longer affect permissions to data in the S3 bucket. The bucket uses policies to define access control.
506+
ACLs are disabled, and the bucket owner automatically owns and has full control
507+
over every object in the bucket. ACLs no longer affect permissions to data in the
508+
S3 bucket. The bucket uses policies to define access control.
489509

490510
```ts
491511
new s3.Bucket(this, 'MyBucket', {
492512
objectOwnership: s3.ObjectOwnership.BUCKET_OWNER_ENFORCED,
493513
});
494514
```
495515

516+
Some services may not not support log delivery to buckets that have object ownership
517+
set to bucket owner enforced, such as
518+
[S3 buckets using ACLs](#allowing-access-log-delivery-using-a-bucket-policy-recommended)
519+
or [CloudFront Distributions][CloudFront S3 Bucket].
520+
521+
[CloudFront S3 Bucket]: https://docs.aws.amazon.com/AmazonCloudFront/latest/DeveloperGuide/AccessLogs.html#AccessLogsBucketAndFileOwnership
522+
496523
## Bucket deletion
497524

498525
When a bucket is removed from a stack (or the stack is deleted), the S3

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

Lines changed: 28 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -1832,7 +1832,9 @@ export class Bucket extends BucketBase {
18321832
}
18331833

18341834
if (props.serverAccessLogsBucket instanceof Bucket) {
1835-
props.serverAccessLogsBucket.allowLogDelivery();
1835+
props.serverAccessLogsBucket.allowLogDelivery(this, props.serverAccessLogsPrefix);
1836+
} else if (props.serverAccessLogsPrefix) {
1837+
this.allowLogDelivery(this, props.serverAccessLogsPrefix);
18361838
}
18371839

18381840
for (const inventory of props.inventories ?? []) {
@@ -2189,17 +2191,35 @@ export class Bucket extends BucketBase {
21892191
}
21902192

21912193
/**
2192-
* Allows the LogDelivery group to write, fails if ACL was set differently.
2194+
* Allows Log Delivery to the S3 bucket, using a Bucket Policy if the relevant feature
2195+
* flag is enabled, otherwise the canned ACL is used.
2196+
*
2197+
* If log delivery is to be allowed using the ACL and an ACL has already been set, this fails.
21932198
*
21942199
* @see
2195-
* https://docs.aws.amazon.com/AmazonS3/latest/dev/acl-overview.html#canned-acl
2196-
*/
2197-
private allowLogDelivery() {
2198-
if (this.accessControl && this.accessControl !== BucketAccessControl.LOG_DELIVERY_WRITE) {
2200+
* https://docs.aws.amazon.com/AmazonS3/latest/userguide/enable-server-access-logging.html
2201+
*/
2202+
private allowLogDelivery(from: IBucket, prefix?: string) {
2203+
if (FeatureFlags.of(this).isEnabled(cxapi.S3_SERVER_ACCESS_LOGS_USE_BUCKET_POLICY)) {
2204+
this.addToResourcePolicy(new iam.PolicyStatement({
2205+
effect: iam.Effect.ALLOW,
2206+
principals: [new iam.ServicePrincipal('logging.s3.amazonaws.com')],
2207+
actions: ['s3:PutObject'],
2208+
resources: [this.arnForObjects(prefix ? `${prefix}*`: '*')],
2209+
conditions: {
2210+
ArnLike: {
2211+
'aws:SourceArn': from.bucketArn,
2212+
},
2213+
StringEquals: {
2214+
'aws:SourceAccount': from.env.account,
2215+
},
2216+
},
2217+
}));
2218+
} else if (this.accessControl && this.accessControl !== BucketAccessControl.LOG_DELIVERY_WRITE) {
21992219
throw new Error("Cannot enable log delivery to this bucket because the bucket's ACL has been set and can't be changed");
2220+
} else {
2221+
this.accessControl = BucketAccessControl.LOG_DELIVERY_WRITE;
22002222
}
2201-
2202-
this.accessControl = BucketAccessControl.LOG_DELIVERY_WRITE;
22032223
}
22042224

22052225
private parseInventoryConfiguration(): CfnBucket.InventoryConfigurationProperty[] | undefined {

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

Lines changed: 65 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2205,6 +2205,71 @@ describe('bucket', () => {
22052205
).toThrow(/Cannot enable log delivery to this bucket because the bucket's ACL has been set and can't be changed/);
22062206
});
22072207

2208+
test('Bucket Allow Log delivery should use the recommended policy when flag enabled', () => {
2209+
// GIVEN
2210+
const stack = new cdk.Stack();
2211+
stack.node.setContext('@aws-cdk/aws-s3:serverAccessLogsUseBucketPolicy', true);
2212+
2213+
// WHEN
2214+
const bucket = new s3.Bucket(stack, 'TestBucket', { serverAccessLogsPrefix: 'test' });
2215+
2216+
// THEN
2217+
const template = Template.fromStack(stack);
2218+
template.hasResourceProperties('AWS::S3::Bucket', {
2219+
AccessControl: Match.absent(),
2220+
});
2221+
template.hasResourceProperties('AWS::S3::BucketPolicy', {
2222+
Bucket: stack.resolve(bucket.bucketName),
2223+
PolicyDocument: Match.objectLike({
2224+
Statement: Match.arrayWith([Match.objectLike({
2225+
Effect: 'Allow',
2226+
Principal: { Service: 'logging.s3.amazonaws.com' },
2227+
Action: 's3:PutObject',
2228+
Resource: stack.resolve(`${bucket.bucketArn}/test*`),
2229+
Condition: {
2230+
ArnLike: {
2231+
'aws:SourceArn': stack.resolve(bucket.bucketArn),
2232+
},
2233+
StringEquals: {
2234+
'aws:SourceAccount': { 'Ref': 'AWS::AccountId' },
2235+
},
2236+
},
2237+
})]),
2238+
}),
2239+
});
2240+
});
2241+
2242+
test('Log Delivery bucket policy should properly set source bucket ARN/Account', () => {
2243+
// GIVEN
2244+
const stack = new cdk.Stack();
2245+
stack.node.setContext('@aws-cdk/aws-s3:serverAccessLogsUseBucketPolicy', true);
2246+
2247+
// WHEN
2248+
const targetBucket = new s3.Bucket(stack, 'TargetBucket');
2249+
const sourceBucket = new s3.Bucket(stack, 'SourceBucket', { serverAccessLogsBucket: targetBucket });
2250+
2251+
// THEN
2252+
Template.fromStack(stack).hasResourceProperties('AWS::S3::BucketPolicy', {
2253+
Bucket: stack.resolve(targetBucket.bucketName),
2254+
PolicyDocument: Match.objectLike({
2255+
Statement: Match.arrayWith([Match.objectLike({
2256+
Effect: 'Allow',
2257+
Principal: { Service: 'logging.s3.amazonaws.com' },
2258+
Action: 's3:PutObject',
2259+
Resource: stack.resolve(`${targetBucket.bucketArn}/*`),
2260+
Condition: {
2261+
ArnLike: {
2262+
'aws:SourceArn': stack.resolve(sourceBucket.bucketArn),
2263+
},
2264+
StringEquals: {
2265+
'aws:SourceAccount': stack.resolve(sourceBucket.env.account),
2266+
},
2267+
},
2268+
})]),
2269+
}),
2270+
});
2271+
});
2272+
22082273
test('Defaults for an inventory bucket', () => {
22092274
// Given
22102275
const stack = new cdk.Stack();

‎packages/@aws-cdk/aws-s3/test/integ.bucket.server-access-logs.js.snapshot/aws-cdk-s3-access-logs.assets.json

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1,15 +1,15 @@
11
{
2-
"version": "20.0.0",
2+
"version": "22.0.0",
33
"files": {
4-
"8091e7796cbf83b5d8109a10ea23dfbe9ac04ad84434da5dd183b89eb89e351b": {
4+
"f58a2b25314952a1a5a6b42c6b9092caf2710430af09ba4f5d807e60f2fd3542": {
55
"source": {
66
"path": "aws-cdk-s3-access-logs.template.json",
77
"packaging": "file"
88
},
99
"destinations": {
1010
"current_account-current_region": {
1111
"bucketName": "cdk-hnb659fds-assets-${AWS::AccountId}-${AWS::Region}",
12-
"objectKey": "8091e7796cbf83b5d8109a10ea23dfbe9ac04ad84434da5dd183b89eb89e351b.json",
12+
"objectKey": "f58a2b25314952a1a5a6b42c6b9092caf2710430af09ba4f5d807e60f2fd3542.json",
1313
"assumeRoleArn": "arn:${AWS::Partition}:iam::${AWS::AccountId}:role/cdk-hnb659fds-file-publishing-role-${AWS::AccountId}-${AWS::Region}"
1414
}
1515
}

‎packages/@aws-cdk/aws-s3/test/integ.bucket.server-access-logs.js.snapshot/aws-cdk-s3-access-logs.template.json

Lines changed: 49 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -2,12 +2,58 @@
22
"Resources": {
33
"MyAccessLogsBucketF7FE6635": {
44
"Type": "AWS::S3::Bucket",
5-
"Properties": {
6-
"AccessControl": "LogDeliveryWrite"
7-
},
85
"UpdateReplacePolicy": "Delete",
96
"DeletionPolicy": "Delete"
107
},
8+
"MyAccessLogsBucketPolicyEA9AB063": {
9+
"Type": "AWS::S3::BucketPolicy",
10+
"Properties": {
11+
"Bucket": {
12+
"Ref": "MyAccessLogsBucketF7FE6635"
13+
},
14+
"PolicyDocument": {
15+
"Statement": [
16+
{
17+
"Action": "s3:PutObject",
18+
"Condition": {
19+
"ArnLike": {
20+
"aws:SourceArn": {
21+
"Fn::GetAtt": [
22+
"MyBucketF68F3FF0",
23+
"Arn"
24+
]
25+
}
26+
},
27+
"StringEquals": {
28+
"aws:SourceAccount": {
29+
"Ref": "AWS::AccountId"
30+
}
31+
}
32+
},
33+
"Effect": "Allow",
34+
"Principal": {
35+
"Service": "logging.s3.amazonaws.com"
36+
},
37+
"Resource": {
38+
"Fn::Join": [
39+
"",
40+
[
41+
{
42+
"Fn::GetAtt": [
43+
"MyAccessLogsBucketF7FE6635",
44+
"Arn"
45+
]
46+
},
47+
"/example*"
48+
]
49+
]
50+
}
51+
}
52+
],
53+
"Version": "2012-10-17"
54+
}
55+
}
56+
},
1157
"MyBucketF68F3FF0": {
1258
"Type": "AWS::S3::Bucket",
1359
"Properties": {

‎packages/@aws-cdk/aws-s3/test/integ.bucket.server-access-logs.js.snapshot/cdk.out

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1 +1 @@
1-
{"version":"20.0.0"}
1+
{"version":"22.0.0"}

‎packages/@aws-cdk/aws-s3/test/integ.bucket.server-access-logs.js.snapshot/integ.json

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
11
{
2-
"version": "20.0.0",
2+
"version": "22.0.0",
33
"testCases": {
44
"integ.bucket.server-access-logs": {
55
"stacks": [

‎packages/@aws-cdk/aws-s3/test/integ.bucket.server-access-logs.js.snapshot/manifest.json

Lines changed: 14 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -1,12 +1,6 @@
11
{
2-
"version": "20.0.0",
2+
"version": "22.0.0",
33
"artifacts": {
4-
"Tree": {
5-
"type": "cdk:tree",
6-
"properties": {
7-
"file": "tree.json"
8-
}
9-
},
104
"aws-cdk-s3-access-logs.assets": {
115
"type": "cdk:asset-manifest",
126
"properties": {
@@ -23,7 +17,7 @@
2317
"validateOnSynth": false,
2418
"assumeRoleArn": "arn:${AWS::Partition}:iam::${AWS::AccountId}:role/cdk-hnb659fds-deploy-role-${AWS::AccountId}-${AWS::Region}",
2519
"cloudFormationExecutionRoleArn": "arn:${AWS::Partition}:iam::${AWS::AccountId}:role/cdk-hnb659fds-cfn-exec-role-${AWS::AccountId}-${AWS::Region}",
26-
"stackTemplateAssetObjectUrl": "s3://cdk-hnb659fds-assets-${AWS::AccountId}-${AWS::Region}/8091e7796cbf83b5d8109a10ea23dfbe9ac04ad84434da5dd183b89eb89e351b.json",
20+
"stackTemplateAssetObjectUrl": "s3://cdk-hnb659fds-assets-${AWS::AccountId}-${AWS::Region}/f58a2b25314952a1a5a6b42c6b9092caf2710430af09ba4f5d807e60f2fd3542.json",
2721
"requiresBootstrapStackVersion": 6,
2822
"bootstrapStackVersionSsmParameter": "/cdk-bootstrap/hnb659fds/version",
2923
"additionalDependencies": [
@@ -45,6 +39,12 @@
4539
"data": "MyAccessLogsBucketF7FE6635"
4640
}
4741
],
42+
"/aws-cdk-s3-access-logs/MyAccessLogsBucket/Policy/Resource": [
43+
{
44+
"type": "aws:cdk:logicalId",
45+
"data": "MyAccessLogsBucketPolicyEA9AB063"
46+
}
47+
],
4848
"/aws-cdk-s3-access-logs/MyBucket/Resource": [
4949
{
5050
"type": "aws:cdk:logicalId",
@@ -65,6 +65,12 @@
6565
]
6666
},
6767
"displayName": "aws-cdk-s3-access-logs"
68+
},
69+
"Tree": {
70+
"type": "cdk:tree",
71+
"properties": {
72+
"file": "tree.json"
73+
}
6874
}
6975
}
7076
}

0 commit comments

Comments
 (0)