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): Warn/error when changes to imported principals are dropped #12188

Open
stephenwiebe opened this issue Dec 21, 2020 · 8 comments
Open

(iam): Warn/error when changes to imported principals are dropped #12188

stephenwiebe opened this issue Dec 21, 2020 · 8 comments
Labels
@aws-cdk/aws-iam Related to AWS Identity and Access Management effort/medium Medium work item – several days of effort feature-request A feature should be added or improved. p1

Comments

@stephenwiebe
Copy link
Contributor

When I import an IRole with Role.fromRoleArn, if I try to add a managed policy with addManagedPolicy, nothing happens. However, addToPrincipalPolicy with the desired policy statements does work. Is this the expected behavior?

Reproduction Steps

const codebuildRole = Role.fromRoleArn(
      this,
      'CodebuildRole',
      StringParameter.valueForStringParameter(this, props.commonCodebuildRoleParamName)
    );

codebuildRole.addManagedPolicy(new iam.ManagedPolicy(...));  // doesn't do anything, although the ManagedPolicy is created

Environment

  • CDK CLI Version : 1.79.0
  • Framework Version: 1.79.0
  • Node.js Version: 14.4.0
  • OS : Linux
  • Language (Version): Typescript (3.7.2)

This is 🐛 Bug Report

@stephenwiebe stephenwiebe added bug This issue is a bug. needs-triage This issue or PR still needs to be triaged. labels Dec 21, 2020
@github-actions github-actions bot added the @aws-cdk/aws-iam Related to AWS Identity and Access Management label Dec 21, 2020
@redbaron
Copy link
Contributor

Did you try passing mutable flag in FromRoleArnOptions optional argument? https://docs.aws.amazon.com/cdk/api/latest/docs/@aws-cdk_aws-iam.FromRoleArnOptions.html

@jumi-dev
Copy link
Contributor

addToPrincipalPolicy in Role.fromRoleArn is not supported, see:

public addManagedPolicy(_policy: IManagedPolicy): void {
// FIXME: Add warning that we're ignoring this
}

Is this technically not possible? Or is there any other reason why this doesn't make sense? We should explain it in the warning.

@redbaron
Copy link
Contributor

I'd say it should not warn, but throw.

@rix0rrr
Copy link
Contributor

rix0rrr commented Jan 5, 2021

Depending on the situation, it is technically not possible. CloudFormation only allows us to attached managed policies upon role creation or upon managed policy creation (there is no AWS::IAM::ManagedPolicyAttachment resource).

We don't have provisions for the second case at all at the moment.

@rix0rrr rix0rrr changed the title (iam): Imported IRole.addManagedPolicy silently does nothing (iam): Warn/error when changes to imported principals are dropped Jan 5, 2021
@rix0rrr rix0rrr added effort/medium Medium work item – several days of effort p1 and removed bug This issue is a bug. labels Jan 5, 2021
@ryparker ryparker removed the needs-triage This issue or PR still needs to be triaged. label Jun 2, 2021
@brentryan
Copy link
Contributor

Why couldn't the CDK code still allow addManagedPolicy() to work properly when it's imported? I understand that cloudformation has this limitation but that shouldn't stop CDK from allowing you to generate the right cloudformation for certain cases. For example, if you're deploying a CDK application that consists of 2 stacks, why couldn't this addManagedPolicy() method mutate the Role in stackA if being imported to stackB? CDK in this case has full control over all the objects and should be able to do this before synth. In the case of importing a role from an external role then it should throw an error and say this isn't supported ....

@github-actions
Copy link

This issue has not received any attention in 1 year. 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 Nov 23, 2022
@stephenwiebe
Copy link
Contributor Author

I'm wondering about the answer to @brentryan 's question as well, but if it's not feasible than a warning should be added at least, right?

@github-actions github-actions bot removed the closing-soon This issue will automatically close in 4 days unless further comments are made. label Nov 27, 2022
@peterwoodworth
Copy link
Contributor

I understand that cloudformation has this limitation but that shouldn't stop CDK from allowing you to generate the right cloudformation for certain cases

You cannot generate cloudformation for a resource that is not managed by the same cloudformation stack

2 stacks in same app example

This should already work because you're not interacting with an IRole but a Role

I agree with this issue that a warning should throw when adding the policy is a no-op, this frequently confuses customers in other issues as well

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
@aws-cdk/aws-iam Related to AWS Identity and Access Management effort/medium Medium work item – several days of effort feature-request A feature should be added or improved. p1
Projects
None yet
Development

No branches or pull requests

7 participants