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

Lookup assumed role ARN with IAM API #144

Closed
wants to merge 2 commits into from

Conversation

jpb
Copy link

@jpb jpb commented Sep 4, 2018

Previously, arn.Canonicalize assumed that the full IAM role ARN could be extracted from an assumed-role ARN. This is not the case for IAM roles with a path, in which case only the role name is encoded in the assumed-role ARN. This change uses the IAM API to determine the correct IAM role ARN for a given assumed-role ARN.

The aws package was (initially) added to resolve circular dependencies between server and arn, but it turns out more than server needs use of the newly introduced iamProvider. ec2Provider was moved to this package to keep things symmetrical.

This is a potential proper fix for #98 / #103 (see #103 (comment))

Previously, arn.Canonicalize assumed that the full IAM role ARN could be
extracted from an assumed-role ARN. This is not the case for IAM roles with a
path, in which case only the role name is encoded in the assumed-role ARN. This
change uses the IAM API to determine the correct IAM role ARN for a given
assuemd-role ARN.
@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Sep 4, 2018
@jpb
Copy link
Author

jpb commented Sep 4, 2018

/assign @nckturner

@jpb
Copy link
Author

jpb commented Sep 19, 2018

Hey @nckturner are you able to take a look at this? Or is there someone else you would suggest to have review this PR? Thanks!

@nckturner
Copy link
Contributor

@jpb Apologies, I have been out of the office for a bit. Taking a look!

@nckturner
Copy link
Contributor

@jpb Quick update--I engaged some folks in the AWS security org to discuss the best way to handle paths in roles moving forward. I will keep this issue updated.

I totally agree that the current behavior leaves something to be desired. The role ARN does have the path in it, whereas the assumed-role ARN does not. The current behavior basically forces you to construct an ARN for the role that does not include the path to put in your map.

@mattlandis
Copy link
Contributor

Holding based on nckturner's comment.

/hold

@k8s-ci-robot k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Oct 11, 2018
@jpb
Copy link
Author

jpb commented Oct 22, 2018

Thanks for following up @nckturner.

If it helps, the documentation specifies that IAM roles include the path in their ARN (arn:${Partition}:iam::${Account}:role/${RoleNameWithPath}) but assumed roles do not (arn:${Partition}:iam::${Account}:assumed-role/${RoleName}/${RoleSessionName}). This works out because roles are uniquely identified by their name, not by their name and path. The best documentation I could find on that is that GetRole does not accept the path as a parameter when fetching a role (a path will always have a slash in it, which is not valid in the RoleName parameter, nor is there a "path" parameter that can be specified for that API endpoint).

@alex-berger
Copy link

Any progress on this PR, @nckturner any update about this from the AWS security folks?

@romilpunetha
Copy link

Can someone suggest me whats wrong with the following aws-auth configmap entry?

  - groups:
     - curefit:eks-admin
     rolearn: arn:aws:iam::000000000:role/eks-admins
     username: admin:{{SessionName}}
   - groups:
     - curefit:eks-viewer
     rolearn: arn:aws:iam::000000000:role/eks-all-cluster-viewers-role
     username: viewer:{{SessionName}}```

This role is associated with a policy, that is attached to a group, and no one from that group is able to access the cluster.

@alex-berger
Copy link

@romil-punetha I guess this is due to #114.

@romilpunetha
Copy link

@alex-berger Im using rolearn which is what the fix says.

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: jpb
To fully approve this pull request, please assign additional approvers.
We suggest the following additional approver: nckturner

If they are not already assigned, you can assign the PR to them by writing /assign @nckturner in a comment when ready.

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@fejta-bot
Copy link

Issues go stale after 90d of inactivity.
Mark the issue as fresh with /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/lifecycle stale

@k8s-ci-robot k8s-ci-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Jul 24, 2019
@fejta-bot
Copy link

Stale issues rot after 30d of inactivity.
Mark the issue as fresh with /remove-lifecycle rotten.
Rotten issues close after an additional 30d of inactivity.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/lifecycle rotten

@k8s-ci-robot k8s-ci-robot added lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed. and removed lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. labels Aug 23, 2019
@fejta-bot
Copy link

Rotten issues close after 30d of inactivity.
Reopen the issue with /reopen.
Mark the issue as fresh with /remove-lifecycle rotten.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/close

@k8s-ci-robot
Copy link
Contributor

@fejta-bot: Closed this PR.

In response to this:

Rotten issues close after 30d of inactivity.
Reopen the issue with /reopen.
Mark the issue as fresh with /remove-lifecycle rotten.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/close

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants