(s3): grantRead gives too wide rights when objectsKeyPattern is provided

[Hi-Fi]

Describe the bug

When adding cross-account right to bucket with account principal and objectsKeyPattern, pattern doesn't affect to policy as also bucket's plain ARN is put to policy.

Rights added with code: bucket.grantRead(new iam.AccountPrincipal('123456789123'), 'only/this/should/be/accessed/*');

Expected Behavior

When objectsKeyPattern is provided, only objects matching to that are visible for defined principal. One possible policy snippet could be (according https://docs.aws.amazon.com/AmazonS3/latest/userguide/amazon-s3-policy-keys.html#condition-key-bucket-ops-2):

{
            "Effect": "Allow",
            "Principal": {
                "AWS": [
                    "arn:aws:iam::123456789123:root"
                ]
            },
            "Action": "s3:GetObject",
            "Resource": "arn:aws:s3:::my-test-bucket/only/this/should/be/accessed/*"
        },
        {
            "Effect": "Allow",
            "Principal": {
                "AWS": [
                    "arn:aws:iam::123456789123:root"
                ]
            },
            "Action": "s3:ListBucket",
            "Resource": "arn:aws:s3:::my-test-bucket",
            "Condition": {
                "StringEquals": {
                    "s3:prefix": "only/this/should/be/accessed"
                }
            }
        }

Also expecting command to return:

aws s3api list-objects --bucket my-test-bucket | cat

An error occurred (AccessDenied) when calling the ListObjects operation: Access Denied

Current Behavior

Currently policy created is like

{
            "Effect": "Allow",
            "Principal": {
                "AWS": "arn:aws:iam::123456789123:root"
            },
            "Action": [
                "s3:GetObject*",
                "s3:GetBucket*",
                "s3:List*"
            ],
            "Resource": [
                "arn:aws:s3:::my-test-bucket",
                "arn:aws:s3:::my-test-bucket/only/this/should/be/accessed/*"
            ]
        }

Which makes command to list all content in bucket even that wasn't what was expected:

aws s3api list-objects --bucket my-test-bucket | cat
{
    "Contents": [
        {
            "Key": "test.txt",
            "LastModified": "2023-03-03T05:37:29+00:00",
            "ETag": "\"c2bf22855e39e87038f91gd8401dd23a\"",
            "Size": 6,
            "StorageClass": "STANDARD"
        }
    ]
}

Reproduction Steps

1. Create bucket and add cross account rights to it to prefix test
2. Add some objects to bucket with prefixes test and test2
3. List objects from other account (aws s3api list-objects --bucket my-test-bucket)
4. This shouldn't list things with prefix test2 like it currently does.

Possible Solution

Generate generate statement using conditions like in https://docs.aws.amazon.com/AmazonS3/latest/userguide/amazon-s3-policy-keys.html#condition-key-bucket-ops-2.

Additional Information/Context

No response

CDK CLI Version

2.66.0 (build c96c17d)

Framework Version

No response

Node.js Version

v16.19.0

OS

Linux b67dca3f6fa1 5.10.102.1-microsoft-standard-WSL2 #1 SMP Wed Mar 2 00:30:59 UTC 2022 x86_64 GNU/Linux

Language

Typescript

Language Version

Typescript 4.9.5

Other information

No response

[pahud]

Hi @Hi-Fi

Can you provide some CDK code that allows us to deploy in our account? This helps us see more details behind it. Thank you.

[Hi-Fi]

Here's the quick stack:

import { 
  aws_iam as iam,
  aws_s3 as s3,
  RemovalPolicy, 
  Stack, 
  StackProps
} from 'aws-cdk-lib';

import { Construct } from 'constructs';

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

    const bucket = new s3.Bucket(this, 'SharedBucket', {
      blockPublicAccess: s3.BlockPublicAccess.BLOCK_ALL,
      enforceSSL: true,
      autoDeleteObjects: true,
      removalPolicy: RemovalPolicy.DESTROY
    })

    if (!process.env.USER_ACCOUNT) {
      throw new Error('USER_ACCOUNT env variable needed');
    }
    bucket.grantRead(new iam.AccountPrincipal(process.env.USER_ACCOUNT), 'only/this/should/be/accessed/*')

    new CfnOutput(this, 'BucketName', {
      value: bucket.bucketName,
      description: `Name of the test bucket with prefix based sharing to ${process.env.USER_ACCOUNT}`,
    })
  }
}

USER_ACCOUNT is the account where data is tried to read from. And from there:

• aws s3api list-objects --bucket <actual bucket name> should fail
• aws s3api list-objects --bucket <actual bucket name> --prefix only/this/should/be/accessed should pass

[pahud]

Thank you for your report. And yes we probably should look into bucket.grantRead() and fix this bug.

[barreeeiroo]

I think the issue relies on this line: https://github.com/aws/aws-cdk/blob/main/packages/aws-cdk-lib/aws-s3/lib/bucket.ts#L971. It seems like both the entire bucket and the provided pattern are provided, rather than only the passed objectsKeyPattern.
And btw, the issue affects all methods (not only grantRead).

[colifran]

@barreeeiroo @Hi-Fi thanks again for reporting this. I've been in the process of investigating this and I'm leaning towards this not being a bug. The way I'm looking at the"grant" methods used on a specific S3 bucket instance is as being generic grants that cast a wide net to support the most use cases. The grantRead method specifically states in it's docstring that it is used to "grant read permissions for this bucket and it's contents to an IAM principal (Role/Group/User)". So, while it does allow a cross-account user to list the contents of the entire bucket, it will correctly limit their ability to access/read objects based on the provided objectsKeyPattern. I would also like to refer you to the following blog post which makes a nice case for having list access to all the "folders" up the tree from the prefix you're trying to allow - https://aws.amazon.com/blogs/security/writing-iam-policies-grant-access-to-user-specific-folders-in-an-amazon-s3-bucket/. My opinion is that if there is a use case that requires more controlled access, then the appropriate direction would be to add custom policy statements to the bucket and principal.

[colifran]

Closing this based on my previous comment, but happy to re-open for additional discussion.

[github-actions]

⚠️COMMENT VISIBILITY WARNING⚠️

Comments on closed issues are hard for our team to see.
If you need more assistance, please either tag a team member or open a new issue that references this one.
If you wish to keep having a conversation with other community members under this issue feel free to do so.