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.

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

aws-s3: blockPublicAccess has a counterintuitive behaviour #32811

Open
1 task
garysassano opened this issue Jan 9, 2025 · 4 comments · May be fixed by #33668
Open
1 task

aws-s3: blockPublicAccess has a counterintuitive behaviour #32811

garysassano opened this issue Jan 9, 2025 · 4 comments · May be fixed by #33668
Labels
@aws-cdk/aws-s3 Related to Amazon S3 bug This issue is a bug. effort/small Small work item – less than a day of effort p2

Comments

@garysassano
Copy link
Contributor

Describe the bug

When a bucket is created without specifying the blockPublicAccess property:

const myBucket = new Bucket(this, "MyBucket");

It is equivalent to explicitly setting all BlockPublicAccess options to true:

const myBucket = new Bucket(this, "MyBucket", {
  blockPublicAccess: new BlockPublicAccess({
    blockPublicAcls: true,
    ignorePublicAcls: true,
    blockPublicPolicy: true,
    restrictPublicBuckets: true,
  }),
});

This might lead you to assume that all BlockPublicAccess options default to true. However, that's not the case. For example, if you deploy a bucket like this:

const myBucket = new Bucket(this, "MyBucket", {
  blockPublicAccess: new BlockPublicAccess({
    blockPublicPolicy: false,
    restrictPublicBuckets: false,
  }),
});

You would get this configuration:

image

This happens because all options within BlockPublicAccess are undefined by default, which is equivalent to false.

This behavior is counterintuitive. If you do not define blockPublicAccess, all options default to true. However, if you define a BlockPublicAccess, any unspecified options default to false.

This seemingly paradoxical situation stems from a change introduced a couple of years ago.

Regression Issue

  • Select this option if this issue appears to be a regression.

Last Known Working CDK Version

No response

Expected Behavior

see above.

Current Behavior

see above.

Reproduction Steps

see above.

Possible Solution

No response

Additional Information/Context

No response

CDK CLI Version

2.174.1

Framework Version

No response

Node.js Version

22.12.0

OS

Ubuntu 24.04.1

Language

TypeScript

Language Version

No response

Other information

No response

@garysassano garysassano added bug This issue is a bug. needs-triage This issue or PR still needs to be triaged. labels Jan 9, 2025
@github-actions github-actions bot added the @aws-cdk/aws-s3 Related to Amazon S3 label Jan 9, 2025
@khushail khushail added needs-reproduction This issue needs reproduction. p2 and removed needs-triage This issue or PR still needs to be triaged. labels Jan 9, 2025
@khushail khushail self-assigned this Jan 9, 2025
@khushail khushail added investigating This issue is being investigated and/or work is in progress to resolve the issue. and removed needs-reproduction This issue needs reproduction. labels Jan 10, 2025
@khushail
Copy link
Contributor

khushail commented Jan 13, 2025

Hi @garysassano , thanks for reporting this. I tried to repro with 3 different scenario and here is my observation -

export class BucketAccessv2Stack extends cdk.Stack {
  constructor(scope: Construct, id: string, props?: cdk.StackProps) {
    super(scope, id, props);

    const bucketWithDefaultAccess = new s3.Bucket(this, 'bucketdefaulted', {}); //means by default, it should have  blockallPublicaccess :true which is true when seen in console

    const bucketwithsetaccess = new s3.Bucket(this, 'bucketwithsetaccess', {
      blockPublicAccess: new s3.BlockPublicAccess({
        blockPublicAcls: true,
        ignorePublicAcls: true,
        blockPublicPolicy: true,
        restrictPublicBuckets: true,  // means  it should have blockallPublicaccess :true
      }),
    });  

    const bukcetwithfalseaccess = new s3.Bucket(this, "bucketwithfalseaccess", {
      blockPublicAccess: new s3.BlockPublicAccess({
        blockPublicPolicy: false,
        restrictPublicBuckets: false,   //means it should have blockPublicaccess: false for defined policy and rest 2 which are undefined should be defaulted to true
      }),  
    });
  }
}

In console , the setting for resppective buckets are -

  1. Bucket -bucketWithDefaultAccess ( no access policy defined, so default is true)
Image
  1. Bucket - bucketwithsetaccess (access policy is declared, so it is as expected)
Image
  1. Bucket - bukcetwithfalseaccess ( access policy is set false for 2 properties and rest 2 are undefined, which means by default, undefined should have true value as per Amazon policy which is contradictive
Image

As its seen in the code, default value for these variables is set to undefined which is causing the inconsistency wrt to Amazon policy -

Code commit - 299fb6a#diff-97a6344bb43117c16441333d96eede412c2c7f7df034f88bbebb90b151eca42d

https://github.com/aws/aws-cdk/blob/e33ebb4788a1b169a3f83b99d522debe752b396d/packages/aws-cdk-lib/aws-s3/lib/bucket.ts#L1020C1-L1044C1

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


  public static readonly BLOCK_ACLS = 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;
  }
}

One can edit the settings by going through the console or setting the policies explicitly to true in the code But this should be addressed so I am marking this as P2 which means it won't be immediately addressed by the team but would be on their radar.

@khushail khushail added p1 effort/small Small work item – less than a day of effort and removed p2 investigating This issue is being investigated and/or work is in progress to resolve the issue. labels Jan 13, 2025
@khushail khushail removed their assignment Jan 13, 2025
@khushail khushail added p2 and removed p1 labels Jan 13, 2025
@IkeNefcy
Copy link
Contributor

IkeNefcy commented Feb 26, 2025

hmm I'm questioning if this is a bug or not.
@khushail when I read the CFN docs
https://docs.aws.amazon.com/cdk/api/v2/docs/aws-cdk-lib.aws_s3.CfnBucket.PublicAccessBlockConfigurationProperty.html
the options are optional, there is no default for true. I understand your test, but this relies on the author assumption;

--

When a bucket is created without specifying the blockPublicAccess property:

const myBucket = new Bucket(this, "MyBucket");

It is equivalent to explicitly setting all BlockPublicAccess options to true:

const myBucket = new Bucket(this, "MyBucket", {
blockPublicAccess: new BlockPublicAccess({
blockPublicAcls: true,
ignorePublicAcls: true,
blockPublicPolicy: true,
restrictPublicBuckets: true,
}),
});

This might lead you to assume that all BlockPublicAccess options default to true

--

But in reality, if we leave out BlockPublicAccess from the CDK / it's not in the template, all 4 are being turned on yes, but NOT because this is a CFN default, but because this is the S3 default state when creating buckets.
I think if we were to change this behavior from the CDK side of things it would only lead to more confusion.
I think what would be appropriate here would be a warning, stating that if you set only some of the available bucket security settings, block all public access will be disabled.
This would encourage users to simply set all 4 options.
Because at the end of the day the solution to your 3rd test;

    const bukcetwithfalseaccess = new s3.Bucket(this, "bucketwithfalseaccess", {
      blockPublicAccess: new s3.BlockPublicAccess({
        blockPublicPolicy: false,
        restrictPublicBuckets: false,
      }),  
    });

is

    const bukcetwithfalseaccess = new s3.Bucket(this, "bucketwithfalseaccess", {
      blockPublicAccess: new s3.BlockPublicAccess({
        blockPublicAcls: true,
        ignorePublicAcls: true,
        blockPublicPolicy: false,
        restrictPublicBuckets: false,
      }),  
    });

But if we force true to appear when the user has not explicitly set it, then we would be doing something that is different from how both CFN and console behave and could just blur the line totally on this function.
Let me know your thoughts. If you think a warning would work I can whip one up

@garysassano
Copy link
Contributor Author

But in reality, if we leave out BlockPublicAccess from the CDK / it's not in the template, all 4 are being turned on yes, but NOT because this is a CFN default, but because this is the S3 default state when creating buckets.
I think if we were to change this behavior from the CDK side of things it would only lead to more confusion.

As I mentioned in the original post, AWS updated the default security settings for S3 buckets a couple of years ago. We aren't planning to override those defaults. When a new S3 bucket is created without any additional configuration, all blockPublicAccess options will still default to true.

But if we force true to appear when the user has not explicitly set it, then we would be doing something that is different from how both CFN and console behave and could just blur the line totally on this function.

CDK has always taken a different approach from CloudFormation by applying sensible defaults to improve the developer experience. And no, this would't be inconsistent with the AWS Console. As you pointed out, in the AWS Console, all options are automatically enabled by default, and users only specify which ones they want to disable. This aligns with the behavior CDK would enforce—assuming everything is enabled by default and only specifying exceptions.

That said, I'm okay with requiring users to explicitly set all four options. While this could introduce some friction in the developer experience, it would ensure users make deliberate choices and avoid unexpected configurations.

@IkeNefcy
Copy link
Contributor

hmm color me convinced, will work on a PR later today

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
@aws-cdk/aws-s3 Related to Amazon S3 bug This issue is a bug. effort/small Small work item – less than a day of effort p2
Projects
None yet
3 participants