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-iam]: Should it warn or fail if using grantAssumeRole on imported role? #23090

Closed
asgerjensen opened this issue Nov 25, 2022 · 4 comments
Closed
Assignees
Labels
@aws-cdk/aws-iam Related to AWS Identity and Access Management bug This issue is a bug. effort/small Small work item – less than a day of effort p2

Comments

@asgerjensen
Copy link

Describe the bug

After playing around with an imported eks cluster, trying to get the kubeprovider lambda to be able to assume the role of the kubecontroller, i finally realized that the issue was, that the kubecontroller role was imported, and the code tried to grant the lambda-role permission to run as the kubecontroller role, via lambdarole.grant(kubecontroller, 'sts::AssumeRole'.

But since, in my case, the kubecontroller role was also an import (in order to preregister it in aws-auth), this was rendered as

  "HandlerServiceRoleDefaultPolicyCBD0CC91": {
   "Type": "AWS::IAM::Policy",
   "Properties": {
    "PolicyDocument": {
     "Statement": [
      {
       "Action": "eks:DescribeCluster",
       "Effect": "Allow",
       "Resource": "arn:aws:eks:eu-north-1:xxxx:cluster/MyCluster"
      },
      {
       "Action": "sts:AssumeRole",
       "Effect": "Allow",
       "Resource": "arn:aws:iam::xxxx:role/KubernetesController"
      }
     ],
     "Version": "2012-10-17"
    },
    "PolicyName": "HandlerServiceRoleDefaultPolicyCBD0CC91",
    "Roles": [
     {
      "Ref": "HandlerServiceRoleFCDC14AE"
     }
    ]
   },

Ie a resource policy, and not as part of the KubernetesControllers trust-relationship policy.

I get that it cant just (at least i dont think it can), update the trust-relationship policy of the imported role, but it would have saved me a few days if it had just bailed early, with a "This is not going to do what you think it will do".

Does it make sense to add a check to Grant, that checks if the principal is non-owned, and give a warning during the validation step?

Expected Behavior

Either grantAssumeRole or grant(..., 'sts:AssumeRole') would work on any kind of principal, owned or not, or it would give a warning that the operation will not end up being functional.

Current Behavior

Generates resource policy that does not seem to have the desired effect.

Reproduction Steps

const roleA = Role.fromRoleArn('.....');
const roleB = new Role(this, 'newrole', {});


roleA.grantAssumeRole(roleB);

Try to run something with roleB, that assumes roleA.

Possible Solution

Warning or Error in validation step if granter is not owned.

Additional Information/Context

No response

CDK CLI Version

2.51.1

Framework Version

No response

Node.js Version

16

OS

mac

Language

Typescript

Language Version

3.9.7

Other information

No response

@asgerjensen asgerjensen added bug This issue is a bug. needs-triage This issue or PR still needs to be triaged. labels Nov 25, 2022
@github-actions github-actions bot added the @aws-cdk/aws-iam Related to AWS Identity and Access Management label Nov 25, 2022
@peterwoodworth
Copy link
Contributor

Your reproduction code should work. That line of code generates a new Policy to attach to RoleB

  "RoleBDefaultPolicyB06DAA81": {
   "Type": "AWS::IAM::Policy",
   "Properties": {
    "PolicyDocument": {
     "Statement": [
      {
       "Action": "sts:AssumeRole",
       "Effect": "Allow",
       "Resource": "arn:aws:iam::676158502875:role/RoleA"
      }
     ],
     "Version": "2012-10-17"
    },
    "PolicyName": "RoleBDefaultPolicyB06DAA81",
    "Roles": [
     {
      "Ref": "RoleB318292C8"
     }
    ]
   },
   "Metadata": {
    "aws:cdk:path": "MyStack/RoleB/DefaultPolicy/Resource"
   }
  },

Can you provide reproduction code which encounters the same error you were experiencing?

@peterwoodworth peterwoodworth added response-requested Waiting on additional info and feedback. Will move to "closing-soon" in 7 days. p2 effort/small Small work item – less than a day of effort and removed needs-triage This issue or PR still needs to be triaged. labels Nov 25, 2022
@asgerjensen
Copy link
Author

Yes, its not that its not creating the policy. Its that the policy has no effect. As I understand it, AssumeRole has to involve the principal.

Ie, ala

{
  "Version": "2012-10-17",
  "Statement": {
    "Sid": "AssumeRolePolicy",
    "Effect": "Allow",
    "Principal": { "AWS": "arn:aws:iam::<AccountId>:role/RoleB" },
    "Action": "sts:AssumeRole"
  }
}

But i dont see any way to make that happen, as it would usually be listed like

   "Type": "AWS::IAM::Role",
   "Properties": {
    "AssumeRolePolicyDocument": {
     "Statement": [
      {
       "Action": "sts:AssumeRole",
       "Effect": "Allow",
       "Principal": {
        "AWS": "arn:aws:iam::<AccountId>:role/RoleB"
       }
      }
     ],
     "Version": "2012-10-17"
    },

in RoleA's definition, which, in my example isnt present in CDK as its an imported role.

@github-actions github-actions bot removed the response-requested Waiting on additional info and feedback. Will move to "closing-soon" in 7 days. label Nov 26, 2022
@peterwoodworth
Copy link
Contributor

peterwoodworth commented Nov 28, 2022

@asgerjensen I see the issue now,

The issue is exactly what I describe in this comment here. The grantAssumeRole method isn't actually going to modify the trust policy - that is expected to be handled independently of the method you're calling. For your case, importing the role or not doesn't actually matter. It won't work either way without explicitly setting the trust policy yourself (addToPrincipalPolicy)
I'm going to be closing this issue as a duplicate of the issue I commented on previously. This is something we need to document to reduce confusion and errors

@github-actions
Copy link

⚠️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.

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 bug This issue is a bug. effort/small Small work item – less than a day of effort p2
Projects
None yet
Development

No branches or pull requests

3 participants