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

IAM/Groups/S3: Default policy replaced when group imported with from_group_arn, then bucket.grant_read(group) #29762

Closed
sklibanov312 opened this issue Apr 8, 2024 · 8 comments
Assignees
Labels
@aws-cdk/aws-iam Related to AWS Identity and Access Management bug This issue is a bug. closed-for-staleness This issue was automatically closed because it hadn't received any attention in a while. effort/medium Medium work item – several days of effort p2 response-requested Waiting on additional info and feedback. Will move to "closing-soon" in 7 days.

Comments

@sklibanov312
Copy link

Describe the bug

Consider 3 CDK stacks, communicating with SSM string parameters.

GroupStack - creates an IAM Group, and writes its ARN to an SSM String parameter
FirstBucketStack - Gets the group ARN from SSM parameter, creates the first bucket, grants the group read permission to that bucket.
SecondBucketStack - Gets the group ARN from SSM parameter, creates the second bucket, grants the group read permission to that bucket.

Deploy them in that order.

Expected Behavior

Expected behavior is that the group's policy now allows reading from both the first and second buckets, after all 3 stacks are deployed.

Current Behavior

Current behavior is that the group's policy contains read permission for only one bucket. The other bucket is totally absent.

Reproduction Steps

Unzip the attached file stacks.zip

.

  1. cd group
  2. cdk deploy GroupStack
  3. In the AWS Management Console in the IAM page, find the group. Note that it has no policies and no users. Fine.
  4. cd ../firstbucket
  5. cdk deploy FirstBucketStack
  6. Observe that there's now a readergroupDefaultPolicyNNNNN which grants GetBucket, GetObject, st:List* to the Resources arn:aws:s3:::firstbucketstack-firstbucketNNNNN . So far so good
  7. cd ../secondbucket
  8. cdk deploy SecondBucketStack
  9. Observe that the readergroupDefaultPolicyNNNNN has changed to only mention the arn:aws:s3:::secondbucketstack-secondbucketNNNNN and any mention of the first bucket is now gone.

If you go on to delete the SecondBucketStack, you'll see that the policy is gone completely. You have to destroy and re-create the FirstBucketStack, then, to get permissions added.

Possible Solution

Should the CDK create a new policy document in these cases, instead of editing the default?

Additional Information/Context

If you change the situation to add some kind of default policy to the group in the group stack (e.g. s3:ListAllMyBuckets) then it will also get replaced.

Decoupling stacks from each other completely and communicating resources from one stack to another with the SSM Parameter Store is important to our wider use case.

CDK CLI Version

2.135.0 (build d46c474)

Framework Version

No response

Node.js Version

v18.19.1

OS

Debian 12

Language

Python

Language Version

Python (3.11.2)

Other information

No response

@sklibanov312 sklibanov312 added bug This issue is a bug. needs-triage This issue or PR still needs to be triaged. labels Apr 8, 2024
@github-actions github-actions bot added the @aws-cdk/aws-iam Related to AWS Identity and Access Management label Apr 8, 2024
@sklibanov312
Copy link
Author

sklibanov312 commented Apr 8, 2024

Just for convenience, here's the most important parts of all 3 stacks:

group stack:

        group = iam.Group(self, 'reader-group')

        ssm.StringParameter(self, "reader-group-arn-parameter",
                            parameter_name='/group-arn',
                            string_value=group.group_arn)

first bucket stack:

        group_arn = ssm.StringParameter.value_from_lookup(self, '/group-arn')
...
        group = iam.Group.from_group_arn(self, 'reader-group', group_arn)
        first_bucket = s3.Bucket(...)
        first_bucket.grant_read(group)

Second bucket stack looks pretty much the same as the first. Full contents are in the attached stacks.zip.

@ashishdhingra ashishdhingra self-assigned this Apr 8, 2024
@ashishdhingra ashishdhingra added needs-reproduction This issue needs reproduction. and removed needs-triage This issue or PR still needs to be triaged. labels Apr 8, 2024
@ashishdhingra
Copy link
Contributor

ashishdhingra commented Apr 8, 2024

Findings:

  • Deploying FirstBucketStack creates default policy with Logical ID, let's say readergroupDefaultPolicy1F226D05 (with physical ID First-reade-xMHcCmPunTUu):
{
    "Version": "2012-10-17",
    "Statement": [
        {
            "Action": [
                "s3:GetBucket*",
                "s3:GetObject*",
                "s3:List*"
            ],
            "Resource": [
                "arn:aws:s3:::firstbucketstack-firstbucket918e3579-woyilreurmh2",
                "arn:aws:s3:::firstbucketstack-firstbucket918e3579-woyilreurmh2/*"
            ],
            "Effect": "Allow"
        }
    ]
}
  • Thereafter, deploying SecondBucketStack creates default policy with same Logical ID readergroupDefaultPolicy1F226D05 (with physical ID Secon-reade-EG03HblxbJMY):
{
    "Version": "2012-10-17",
    "Statement": [
        {
            "Action": [
                "s3:GetBucket*",
                "s3:GetObject*",
                "s3:List*"
            ],
            "Resource": [
                "arn:aws:s3:::secondbucketstack-secondbucketb073b450-hcg2ukqliqzo",
                "arn:aws:s3:::secondbucketstack-secondbucketb073b450-hcg2ukqliqzo/*"
            ],
            "Effect": "Allow"
        }
    ]
}

It appears that the existing policy readergroupDefaultPolicy1F226D05 is modified (instead of amended, or new policy created all together).

  • Deleting SecondBucketStack (using cdk destroy SecondBucketStack), deleted the bucket, but doesn't appear to remove policy (may be because first bucket has it's reference somehow).
  • Deleting FirstBucketStack (using cdk destroy FirstBucketStack), deleted the bucket and also now deletes the policy.

Unsure if the 2nd bucket stack should amend the existing policy since CDK stack in essence is a CloudFormation stack and it doesn't have any knowledge of existing policy. May be it should create a new policy for 2nd bucket. Needs review with the team.

EDIT:

  • Behavior is same if I use customer's provided Python code or similar code using TypeScript.
  • Using TypeScript as an example, if both buckets are created in same stack, then it correctly applies the policy:
import { RemovalPolicy, Stack, StackProps } from 'aws-cdk-lib';
import * as iam from 'aws-cdk-lib/aws-iam';
import * as ssm from 'aws-cdk-lib/aws-ssm';
import * as s3 from 'aws-cdk-lib/aws-s3';

import { Construct } from 'constructs';

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

    var groupArn = ssm.StringParameter.valueFromLookup(this, '/group-arn');
    
    if (groupArn.startsWith('dummy-value-for-')) {
      console.log('Pre-context-fetch group ARN ' + groupArn);
      groupArn = 'arn:aws:sts::123:assumed-role/some-name-here/AWSCloudFormation';
    }
    else {
      console.log('Post-context-fetch group ARN ' + groupArn)
    }

    const group = iam.Group.fromGroupArn(this, 'reader-group', groupArn);
    
    const firstBucket = new s3.Bucket(this, 'first-bucket', {
      blockPublicAccess: s3.BlockPublicAccess.BLOCK_ALL,
      encryption: s3.BucketEncryption.S3_MANAGED,
      removalPolicy: RemovalPolicy.DESTROY
    });

    const secondBucket = new s3.Bucket(this, 'second-bucket', {
      blockPublicAccess: s3.BlockPublicAccess.BLOCK_ALL,
      encryption: s3.BucketEncryption.S3_MANAGED,
      removalPolicy: RemovalPolicy.DESTROY
    });

    firstBucket.grantRead(group);
    secondBucket.grantRead(group);
  }
}

Policy applied:

{
    "Version": "2012-10-17",
    "Statement": [
        {
            "Action": [
                "s3:GetBucket*",
                "s3:GetObject*",
                "s3:List*"
            ],
            "Resource": [
                "arn:aws:s3:::twobucketsstack-firstbucket918e3579-zwywg0xedils",
                "arn:aws:s3:::twobucketsstack-secondbucketb073b450-ckrmhznuqmzb",
                "arn:aws:s3:::twobucketsstack-firstbucket918e3579-zwywg0xedils/*",
                "arn:aws:s3:::twobucketsstack-secondbucketb073b450-ckrmhznuqmzb/*"
            ],
            "Effect": "Allow"
        }
    ]
}

@ashishdhingra ashishdhingra added needs-review p1 effort/medium Medium work item – several days of effort and removed needs-reproduction This issue needs reproduction. labels Apr 8, 2024
@ashishdhingra
Copy link
Contributor

Off the top of my head, this is because when you grantRead() you are actually addToPrincipalOrResource(). And according to the description here, you can't do it twice on the same grantee.

I will provide some suggested sample for you later.

@pahud Edited my comment #29762 (comment) with more findings.

@pahud
Copy link
Contributor

pahud commented Apr 10, 2024

OK please check this working sample in TypeScript(I believe Python should work too).

stack.ts

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

    const group = new iam.Group(this, 'Group');
    
    // write to the parameter store
    new ssm.StringParameter(this, 'iam-group-arn-param', {
      parameterName: '/group-arn',
      stringValue: group.groupArn,
    })

    new CfnOutput(this, 'GroupArnOutput', { value: group.groupArn });

  }
}

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

    const groupArn = ssm.StringParameter.valueFromLookup(this, '/group-arn');
    const imported = iam.Group.fromGroupArn(this, 'ImportedRole', Lazy.string({ produce: () => groupArn }));
    const bucket = new s3.Bucket(this, 'FirstBucket', {
      removalPolicy: RemovalPolicy.DESTROY,
    });
    bucket.grantRead(imported);
  }
}

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

    const groupArn = ssm.StringParameter.valueFromLookup(this, '/group-arn');
    const imported = iam.Group.fromGroupArn(this, 'ImportedRole', Lazy.string({ produce: () => groupArn }));
    const bucket = new s3.Bucket(this, 'SecondBucket', {
      removalPolicy: RemovalPolicy.DESTROY,
    });
    bucket.grantRead(imported);
  }
}

app.ts

const app = new App();

const env = { region: process.env.CDK_DEFAULT_REGION, account: process.env.CDK_DEFAULT_ACCOUNT };

const groupStack = new GroupStack(app, 'group', { env });
const first = new FirstBucketStack(app, 'first', { env });
const second = new SecondBucketStack(app, 'second', { env });

First, you need to just deploy the group stack as it will create the ssm parameter.

$ npx cdk deploy group

You will see this in the Output:

group.GroupArnOutput = arn:aws:iam::ACCOUNT_ID:group/group-Role1ABCC5F0-gOvw4PFsgJZT

Now, deploy the first and second bucket stacks.

$ npx cdk deploy first second --require-approval=never

This SHOULD work, but IAM Group is not a valid iam principal for bucket policy(see this) so it would fail on creating the bucket policy with invalid principal error. Generally, you should use iam Role instead. So the code should be like:

stack.ts

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

    const role = new iam.Role(this, 'Role', {
      assumedBy: new iam.AccountRootPrincipal(),
    });
    
    // write to the parameter store
    new ssm.StringParameter(this, 'iam-role-arn-param', {
      parameterName: '/role-arn',
      stringValue: role.roleArn,
    })

    new CfnOutput(this, 'RoleArnOutput', { value: role.roleArn });

  }
}

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

    const roleArn = ssm.StringParameter.valueFromLookup(this, '/role-arn');
    const imported = iam.Role.fromRoleArn(this, 'ImportedRole', Lazy.string({ produce: () => roleArn }));
    const bucket = new s3.Bucket(this, 'FirstBucket', {
      removalPolicy: RemovalPolicy.DESTROY,
    });
    bucket.grantRead(imported);
  }
}

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

    const roleArn = ssm.StringParameter.valueFromLookup(this, '/role-arn');
    const imported = iam.Role.fromRoleArn(this, 'ImportedRole', Lazy.string({ produce: () => roleArn }));
    const bucket = new s3.Bucket(this, 'SecondBucket', {
      removalPolicy: RemovalPolicy.DESTROY,
    });
    bucket.grantRead(imported);
  }
}

app.ts

const app = new App();

const env = { region: process.env.CDK_DEFAULT_REGION, account: process.env.CDK_DEFAULT_ACCOUNT };

new RoleStack(app, 'role-stack', { env });
new FirstBucketStack(app, 'first-stack', { env });
new SecondBucketStack(app, 'second-stack', { env });

First, deploy the RoleStack

npx cdk deploy role

Output like this

role-stack.RoleArnOutput = arn:aws:iam::ACCOUNT_ID:role/role-stack-Role1ABCC5F0-7ks9ENYnhrzA

Now, deploy the rest 2 stacks.

npx cdk deploy first-stack second-stack --require-approval=never

And if you check the IAM policies of the role, you see two policy statements for each bucket.

image

Hope this works for you.

By the way, when you destroy all stacks, make sure to clear your cdk context cache from cdk.context.json by cdk context --reset. In my case I use this command:

cdk context --reset "ssm:account=123456789012:parameterName=/role-arn:region=us-east-1"

This will remove the cache from cdk.context.json. If you don't reset that key, StringParameter.valueFromLookup will first read from that stale cache next time when you redeploy and the value would be incorrect.

Let me know if it works for you.

@pahud pahud added the response-requested Waiting on additional info and feedback. Will move to "closing-soon" in 7 days. label Apr 10, 2024
Copy link

This issue has not received a response in a while. If you want to keep this issue open, please leave a comment below and auto-close will be canceled.

@github-actions github-actions bot added the closing-soon This issue will automatically close in 4 days unless further comments are made. label Apr 12, 2024
@sklibanov312
Copy link
Author

So, I have a different local workaround which creates a separate policy document for each of the buckets, like this:

        imported_group.attach_inline_policy(iam.Policy(context, policyName,
                                                          statements=[iam.PolicyStatement(
                                                              actions=[
                                                                  "s3:GetBucket*",
                                                                  "s3:GetObject*",
                                                                  "s3:List*"
                                                              ],
                                                              resources=[bucket.bucket_arn, bucket.bucket_arn + '/*'],
                                                              effect=iam.Effect.ALLOW)]))

instead of using greant_read. It seems to be behaving the way I want. So I have a workaround - well, two now - one using a role per your suggestion and the other using a separate policy document like I just pasted. Great.

But that doesn't actually resolve the spirit of this issue.

I think to resolve the spirit of the issue, the CDK implementation itself must be changed to prevent my original bug report from being silently buggy. It must either loudly fail, or (ideally) work according to expectations.

@github-actions github-actions bot removed closing-soon This issue will automatically close in 4 days unless further comments are made. response-requested Waiting on additional info and feedback. Will move to "closing-soon" in 7 days. labels Apr 12, 2024
@ashishdhingra ashishdhingra added the response-requested Waiting on additional info and feedback. Will move to "closing-soon" in 7 days. label Apr 16, 2024
Copy link

This issue has not received a response in a while. If you want to keep this issue open, please leave a comment below and auto-close will be canceled.

@github-actions github-actions bot added closing-soon This issue will automatically close in 4 days unless further comments are made. closed-for-staleness This issue was automatically closed because it hadn't received any attention in a while. and removed closing-soon This issue will automatically close in 4 days unless further comments are made. labels Apr 16, 2024
@aws-cdk-automation
Copy link
Collaborator

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.

@aws aws locked as resolved and limited conversation to collaborators Jul 25, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
@aws-cdk/aws-iam Related to AWS Identity and Access Management bug This issue is a bug. closed-for-staleness This issue was automatically closed because it hadn't received any attention in a while. effort/medium Medium work item – several days of effort p2 response-requested Waiting on additional info and feedback. Will move to "closing-soon" in 7 days.
Projects
None yet
Development

No branches or pull requests

4 participants