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

otaviomacedo · 2022-06-28T09:44:42Z

As of December 2021 AWS recommends using bucket policies instead of ACLs for logging access control.

In addition to adding the bucket policy, this change also sets the object ownership to BUCKET_OWNER_ENFORCED, effectively disabling ACLs.

Fixes #19603.

All Submissions:

Have you followed the guidelines in our Contributing guide?

Adding new Unconventional Dependencies:

This PR adds new unconventional dependencies following the process described here

New Features

Have you added the new feature to an integration test?
- 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

gitpod-io · 2022-06-28T09:44:49Z

corymhall

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

corymhall · 2022-06-28T12:46:22Z

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

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


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

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.

I don't think this is being done today.

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.

I added a comment that tries to clarify.

corymhall · 2022-06-28T18:37:11Z

@otaviomacedo another thing I just thought of is I wonder if customers might be using a centralized access log bucket. Not all access logs support the BUCKET_OWNER_ENFORCED ACL (for example CloudFront)

otaviomacedo · 2022-07-01T09:31:43Z

@otaviomacedo another thing I just thought of is I wonder if customers might be using a centralized access log bucket. Not all access logs support the BUCKET_OWNER_ENFORCED ACL (for example CloudFront)

In that case, the logging bucket is created without setting the objectOwnership or the serverAccessLogsBucket properties:

aws-cdk/packages/@aws-cdk/aws-cloudfront/lib/distribution.ts

Lines 443 to 458 in 42d1d7c

    
           private renderLogging(props: DistributionProps): CfnDistribution.LoggingProperty | undefined { 
        
             if (!props.enableLogging && !props.logBucket) { return undefined; } 
        
             if (props.enableLogging === false && props.logBucket) { 
        
               throw new Error('Explicitly disabled logging but provided a logging bucket.'); 
        
             } 
        
             const bucket = props.logBucket ?? new s3.Bucket(this, 'LoggingBucket', { 
        
               encryption: s3.BucketEncryption.S3_MANAGED, 
        
             }); 
        
             return { 
        
               bucket: bucket.bucketRegionalDomainName, 
        
               includeCookies: props.logIncludesCookies, 
        
               prefix: props.logFilePrefix, 
        
             }; 
        
           }

otaviomacedo · 2022-07-01T09:31:55Z

@otaviomacedo another thing I just thought of is I wonder if customers might be using a centralized access log bucket. Not all access logs support the BUCKET_OWNER_ENFORCED ACL (for example CloudFront)

In that case, the logging bucket is created without setting the objectOwnership or the serverAccessLogsBucket properties:

aws-cdk/packages/@aws-cdk/aws-cloudfront/lib/distribution.ts

Lines 443 to 458 in 42d1d7c

    
           private renderLogging(props: DistributionProps): CfnDistribution.LoggingProperty | undefined { 
        
             if (!props.enableLogging && !props.logBucket) { return undefined; } 
        
             if (props.enableLogging === false && props.logBucket) { 
        
               throw new Error('Explicitly disabled logging but provided a logging bucket.'); 
        
             } 
        
             const bucket = props.logBucket ?? new s3.Bucket(this, 'LoggingBucket', { 
        
               encryption: s3.BucketEncryption.S3_MANAGED, 
        
             }); 
        
             return { 
        
               bucket: bucket.bucketRegionalDomainName, 
        
               includeCookies: props.logIncludesCookies, 
        
               prefix: props.logFilePrefix, 
        
             }; 
        
           }

otaviomacedo · 2022-07-01T09:32:14Z

@otaviomacedo another thing I just thought of is I wonder if customers might be using a centralized access log bucket. Not all access logs support the BUCKET_OWNER_ENFORCED ACL (for example CloudFront)

In that case, the logging bucket is created without setting the objectOwnership or the serverAccessLogsBucket properties:

aws-cdk/packages/@aws-cdk/aws-cloudfront/lib/distribution.ts

Lines 443 to 458 in 42d1d7c

    
           private renderLogging(props: DistributionProps): CfnDistribution.LoggingProperty | undefined { 
        
             if (!props.enableLogging && !props.logBucket) { return undefined; } 
        
             if (props.enableLogging === false && props.logBucket) { 
        
               throw new Error('Explicitly disabled logging but provided a logging bucket.'); 
        
             } 
        
             const bucket = props.logBucket ?? new s3.Bucket(this, 'LoggingBucket', { 
        
               encryption: s3.BucketEncryption.S3_MANAGED, 
        
             }); 
        
             return { 
        
               bucket: bucket.bucketRegionalDomainName, 
        
               includeCookies: props.logIncludesCookies, 
        
               prefix: props.logFilePrefix, 
        
             }; 
        
           }

corymhall · 2022-07-01T12:56:18Z

In that case, the logging bucket is created without setting the objectOwnership or the serverAccessLogsBucket properties:

I'm more meaning if you did something like this

const centralLoggingBucket = new s3.Bucket(this, 'Logging');

new s3.Bucket(this, 'SomeOtherBucket', {
  serverAccessLogsBucket: centralLoggingBucket,
  serverAccessLogsPrefix: 'someotherbucket',
});

new cloudfront.Distribution(this, 'Distro', {
  logBucket: centralLoggingBucket,
  logFilePrefix: 'cloudfront',
});

otaviomacedo · 2022-07-04T09:21:14Z

@corymhall I see. Re-reading the documentation, I realised that it strongly suggests the use of BUCKET_OWNER_ENFORCED for object ownership, but it's not mandatory. Removed.

aws-cdk-automation · 2022-07-04T09:55:14Z

AWS CodeBuild CI Report

CodeBuild project: AutoBuildv2Project1C6BFA3F-wQm2hXv2jqQv
Commit ID: fab78e2
Result: SUCCEEDED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

corymhall · 2022-07-05T11:58:20Z

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


    // 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);


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?

corymhall · 2022-07-05T11:59:46Z

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


-    this.accessControl = BucketAccessControl.LOG_DELIVERY_WRITE;


Does this need to be added back then?

Yes, I think so, which means that this change adds no value. Closing the PR until CloudFront (and possibly other services) catches up.

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

c00859a

github-actions bot added the p2 label Jun 28, 2022

aws-cdk-automation requested a review from a team June 28, 2022 09:44

mergify bot added the contribution/core This is a PR that came from AWS. label Jun 28, 2022

otaviomacedo requested review from corymhall and rix0rrr June 28, 2022 09:45

corymhall reviewed Jun 28, 2022

View reviewed changes

Added source account condition

436f648

github-actions bot added effort/medium Medium work item – several days of effort feature-request A feature should be added or improved. p1 and removed p2 labels Jun 29, 2022

Removed bucket owner enforced control

fab78e2

corymhall reviewed Jul 5, 2022

View reviewed changes

otaviomacedo closed this Jul 5, 2022

laurelmay mentioned this pull request Dec 19, 2022

feat(s3): use Bucket Policy for Server Access Logging grant (under feature flag) #23386

Merged

4 tasks

laurelmay mentioned this pull request Dec 30, 2022

(aws-s3): (Buckets set up to save Access Logs to the same bucket are not saving logs) #23516

Closed

RomainMuller deleted the otaviom/s3-logging-policy branch July 4, 2023 08:46

peterwoodworth mentioned this pull request Aug 23, 2023

aws-s3: Unable to Disable ACLs for Access Log Bucket #26832

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

otaviomacedo commented Jun 28, 2022 •

edited

Loading

gitpod-io bot commented Jun 28, 2022

corymhall left a comment

corymhall Jun 28, 2022

otaviomacedo Jun 28, 2022

corymhall Jul 1, 2022

otaviomacedo Jul 4, 2022

corymhall Jul 5, 2022

corymhall commented Jun 28, 2022

otaviomacedo commented Jul 1, 2022

otaviomacedo commented Jul 1, 2022

otaviomacedo commented Jul 1, 2022

corymhall commented Jul 1, 2022

otaviomacedo commented Jul 4, 2022

aws-cdk-automation commented Jul 4, 2022

corymhall Jul 5, 2022

corymhall Jul 5, 2022

otaviomacedo Jul 5, 2022


		this.accessControl = BucketAccessControl.LOG_DELIVERY_WRITE;

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

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

Conversation

otaviomacedo commented Jun 28, 2022 • edited Loading

All Submissions:

Adding new Unconventional Dependencies:

New Features

gitpod-io bot commented Jun 28, 2022

corymhall 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

corymhall commented Jun 28, 2022

otaviomacedo commented Jul 1, 2022

otaviomacedo commented Jul 1, 2022

otaviomacedo commented Jul 1, 2022

corymhall commented Jul 1, 2022

otaviomacedo commented Jul 4, 2022

aws-cdk-automation commented Jul 4, 2022

AWS CodeBuild CI Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

otaviomacedo commented Jun 28, 2022 •

edited

Loading