(aws-s3): bucket.grantRead to an organization principal grants public read access

[ehiggins0]

Describe the bug

When using bucket.grantRead(org), the generated policy allows access to the bucket for all AWS accounts without a condition.

Regression Issue

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

Last Known Working CDK Version

No response

Expected Behavior

The policy should have a condition:

  "bucketPolicy": {
   "Type": "AWS::S3::BucketPolicy",
   "Properties": {
    "Bucket": {
     "Ref": "bucket"
    },
    "PolicyDocument": {
     "Statement": [
      {
       "Action": [
        "s3:GetBucket*",
        "s3:GetObject*",
        "s3:List*"
       ],
       "Condition": {
           "StringEquals": {
               "aws:PrincipalOrgID": "o-yyyyyyyyyy"
           },
       },
       "Effect": "Allow",
       "Principal": {
        "AWS": "*"
       },
       "Resource": [
        {
         "Fn::GetAtt": [
          "bucket",
          "Arn"
         ]
        },
        {
         "Fn::Join": [
          "",
          [
           {
            "Fn::GetAtt": [
             "bucket",
             "Arn"
            ]
           },
           "/*"
          ]
         ]
        }
       ]
      }
     ],
     "Version": "2012-10-17"
    }
   },
   "Metadata": {
    "aws:cdk:path": "Stack/bucket/Policy/Resource"
   }
  }

Current Behavior

This policy gets generated:

  "bucketPolicy": {
   "Type": "AWS::S3::BucketPolicy",
   "Properties": {
    "Bucket": {
     "Ref": "bucket"
    },
    "PolicyDocument": {
     "Statement": [
      {
       "Action": [
        "s3:GetBucket*",
        "s3:GetObject*",
        "s3:List*"
       ],
       "Condition": {
        "StringEquals": {}
       },
       "Effect": "Allow",
       "Principal": {
        "AWS": "*"
       },
       "Resource": [
        {
         "Fn::GetAtt": [
          "bucket",
          "Arn"
         ]
        },
        {
         "Fn::Join": [
          "",
          [
           {
            "Fn::GetAtt": [
             "bucket",
             "Arn"
            ]
           },
           "/*"
          ]
         ]
        }
       ]
      }
     ],
     "Version": "2012-10-17"
    }
   },
   "Metadata": {
    "aws:cdk:path": "Stack/bucket/Policy/Resource"
   }
  }

Reproduction Steps

    const org = new iam.OrganizationPrincipal(orgName);
    const bucket = new s3.Bucket(this, "bucket", {...});
    bucket.grantRead(org);

Possible Solution

No response

Additional Information/Context

No response

CDK CLI Version

2.150.0

Framework Version

No response

Node.js Version

18.18.2

OS

Ubuntu 24.04

Language

TypeScript

Language Version

No response

Other information

No response

[ehiggins0]

Turns out while trying to come up with a workaround that this is not an issue with aws-s3, but is instead a silent failure when trying to add permissions to iam.OrganizationPrincipal(undefined). It still grants the "*" access, but the condition is not added due to the orgId being undefined. Not sure if this should be addressed as it could cause security issues to deployed S3 resources, but feel free to close if not.

[khushail]

@ehiggins0 , thanks for reporting this. I see that the implementation of iam.OrgranizationPrincipal -

aws-cdk/packages/aws-cdk-lib/aws-iam/lib/principals.ts

Line 607 in add7c6b

export class OrganizationPrincipal extends PrincipalBase {

and this is how grant() is implemented invoked by grantRead(..)

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

Line 770 in add7c6b

public grantRead(identity: iam.IGrantable, objectsKeyPattern: any = '*') {

aws-cdk/packages/aws-cdk-lib/aws-iam/lib/principals.ts

Line 627 in add7c6b

public dedupeString(): string | undefined {

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

Line 968 in add7c6b

private grant(

Looks like there should be a check added before granting access to * in case of organization principal.

[IkeNefcy]

I'm not able to reproduce this.
I'm using

const org = new OrganizationPrincipal('o-xxxxxxxxxxxx');
const bucket = new Bucket(this, 'bucket', {});
bucket.grantRead(org);

and it's working fine. I'm using 2.178.2.
Perhaps something changed since this issue was opened? @khushail are you able to reconfirm if you can reproduce this?
I understand your deep dive and why it may lead to an issue, but I'm not understanding how iam.OrganizationPrincipal(undefined) is possible, I get a type error if I don't include a string here, and following the steps from OrganizationPrincipal -> iam.IGrantable -> grantRead -> grant I see no where that dedupeString is being used, and this is not used in the example anywhere either.. so not sure if this is related.

[khushail]

Thanks @IkeNefcy for pointing this out.

I tried to repro the issue with this sample code -

    const org = new OrganizationPrincipal('');
    const bucket = new s3.Bucket(this, 'bucket', {});
    bucket.grantRead(org);

and got this generated template -

"bucketPolicy638F945D": {
   "Type": "AWS::S3::BucketPolicy",
   "Properties": {
    "Bucket": {
     "Ref": "bucket43879C71"
    },
    "PolicyDocument": {
     "Statement": [
      {
       "Action": [
        "s3:GetBucket*",
        "s3:GetObject*",
        "s3:List*"
       ],
       "Condition": {
        "StringEquals": {
         "aws:PrincipalOrgID": ""
        }
       },
       "Effect": "Allow",
       "Principal": {
        "AWS": "*"
       },
       "Resource": [
        {
         "Fn::GetAtt": [
          "bucket43879C71",
          "Arn"
         ]
        },

Though You are right in saying that undefined can't be given and would not be possible.

[IkeNefcy]

Copy, yeah in the OG example there is no condition key at all

      "Condition": {
        "StringEquals": {}
       },

where your current is like what mine does

       "Condition": {
        "StringEquals": {
         "aws:PrincipalOrgID": ""
        }
       },

I can add behavior to check that the organization name is not blank, and that should help with specifically the "" condition. But otherwise I think the Condition statement itself is working okay

[IkeNefcy]

@khushail
I think the best change here would simply be to add the expected regex that organization IDs are allowed to be, it should solve since there is a 10 character minimum
https://docs.aws.amazon.com/organizations/latest/APIReference/API_Organization.html#API_Organization_Contents

Pattern; ^o-[a-z0-9]{10,32}$

[khushail]

Thanks @IkeNefcy for submitting a PR. Appreciate your efforts.

[IkeNefcy]

For sure!

[github-actions]

Comments on closed issues and PRs are hard for our team to see.
If you need help, please open a new issue that references this one.