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

Add SSO Role suffix support #416

Merged
merged 9 commits into from
Jun 16, 2022

Conversation

logandavies181
Copy link
Contributor

@logandavies181 logandavies181 commented Dec 23, 2021

This PR adds the ability to match AWS SSO roles without needing to know the random suffix.

This solves the issue of teams that use AWS SSO roles and aws-iam-authenticator (including EKS users) from needing to keep their configurations up-to-date with the random suffixes that AWS SSO applies to roles it creates. This also has the side-effect of making SSO roles more intuitive to work with.

This PR solves this issue:
#333

In earlier commits, this PR also solved the below roadmap issue, as it allowed for full wildcard support, constrained via ArnLike condition checks.
aws/containers-roadmap#474

A new nested type has been added to the mapRoles elements to specify a configuration for matching SSO created roles.
This looks like:

  mapRoles: |
  - sso:
      permissionSetName: MyPermissionSet
      accountID: "000000000000"
    username: foo
    groups:
    - bar
    - baz

Changes:

  • add.go has a new option for creating entries in the configmap for the new mapping subtype
  • a new feature gate has been added to control whether this new feature is active or not
  • a new package has been added to provide a robust way to perform ArnLike matching of ARNs
  • config.UserMapping and config.RoleMapping have had methods added and their usage in filemapper and configmapper has been refactored to reduce duplication of logic
  • greater unit test coverage 😎
  • new type configmap.SSOARNMatcher for containing the new configuration and specifying new yaml keys

@k8s-ci-robot
Copy link
Contributor

Thanks for your pull request. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please follow instructions at https://git.k8s.io/community/CLA.md#the-contributor-license-agreement to sign the CLA.

It may take a couple minutes for the CLA signature to be fully registered; after that, please reply here with a new comment and we'll verify. Thanks.


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. I understand the commands that are listed here.

@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented Dec 23, 2021

CLA Signed

The committers are authorized under a signed CLA.

@k8s-ci-robot k8s-ci-robot added the cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. label Dec 23, 2021
@k8s-ci-robot
Copy link
Contributor

Welcome @logandavies181!

It looks like this is your first PR to kubernetes-sigs/aws-iam-authenticator 🎉. Please refer to our pull request process documentation to help your PR have a smooth ride to approval.

You will be prompted by a bot to use commands during the review process. Do not be afraid to follow the prompts! It is okay to experiment. Here is the bot commands documentation.

You can also check if kubernetes-sigs/aws-iam-authenticator has its own contribution guidelines.

You may want to refer to our testing guide if you run into trouble with your tests not passing.

If you are having difficulty getting your pull request seen, please follow the recommended escalation practices. Also, for tips and tricks in the contribution process you may want to read the Kubernetes contributor cheat sheet. We want to make sure your contribution gets all the attention it needs!

Thank you, and welcome to Kubernetes. 😃

@k8s-ci-robot
Copy link
Contributor

Hi @logandavies181. Thanks for your PR.

I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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.

@k8s-ci-robot k8s-ci-robot added needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Dec 23, 2021
@logandavies181
Copy link
Contributor Author

I signed it

@logandavies181
Copy link
Contributor Author

/check-cla

@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. and removed cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. labels Dec 23, 2021
@moretea
Copy link

moretea commented Jan 10, 2022

@logandavies181 is there any way that I can assist you with e.g. testing? I'd prefer that above writing a cronjob that would sync the list of roles to the CRD's every few minutes.

@logandavies181
Copy link
Contributor Author

@moretea Thanks for the offer. I'm all good RE: assistance and I've got plenty of free time to complete this, but I'm waiting for a reviewer to indicate that this contribution would actually be considered before putting the effort into implementing the last pieces.

Just giving a bit more grace period before I start making noise as it's just been the Christmas/New Year period

@logandavies181
Copy link
Contributor Author

/assign @jaypipes

@logandavies181
Copy link
Contributor Author

@jaypipes @nckturner would be really nice to get some feedback on this, please. I believe it would solve aws/containers-roadmap#474 which has a lot of interest

Copy link

@jaypipes jaypipes left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@logandavies181 thank you very much for this excellent contribution! I like the idea (pun intended) but have a number of suggestions inline for you to ponder.

cmd/aws-iam-authenticator/root.go Outdated Show resolved Hide resolved
pkg/config/types.go Outdated Show resolved Hide resolved
pkg/mapper/configmap/configmap.go Outdated Show resolved Hide resolved
pkg/mapper/configmap/configmap.go Outdated Show resolved Hide resolved
pkg/config/types.go Show resolved Hide resolved
pkg/mapper/configmap/configmap_test.go Outdated Show resolved Hide resolved
pkg/mapper/crd/apis/iamauthenticator/v1alpha1/types.go Outdated Show resolved Hide resolved
pkg/mapper/file/mapper.go Outdated Show resolved Hide resolved
vendor/modules.txt Outdated Show resolved Hide resolved
@k8s-ci-robot k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Feb 11, 2022
Copy link

@jaypipes jaypipes left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @logandavies181 thanks for your latest push! Getting closer but I still think we can improve the simplicity and readability of the solution. Please check my inline comments.

go.sum Outdated Show resolved Hide resolved
pkg/mapper/configmap/configmap.go Outdated Show resolved Hide resolved
pkg/mapper/configmap/mapper.go Outdated Show resolved Hide resolved
pkg/mapper/configmap/configmap.go Show resolved Hide resolved
Copy link

@jaypipes jaypipes left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@logandavies181 you did a great job on this, thank you for hanging in there through the revisions.

There's one remaining typo/copypasta to address and then I'd like to discuss this patch with provider-aws at the Friday SIG meeting.

Note that we (AWS) need to do an internal review for security purposes of this new functionality. While I like the feature very much (pun intended), I do think that it would be wise to gate this feature behind an opt-in boolean feature flag in the webhook configuration so that folks who want to use the feature can enable it by setting something like an enableARNLikeMatch option in their config file. I think doing that, you will find less resistance to the feature and allow a security review to proceed apace.

/cc @nckturner @jqmichael @wongma7

Best,
-jay

cmd/aws-iam-authenticator/add.go Outdated Show resolved Hide resolved
go.sum Outdated Show resolved Hide resolved
pkg/config/mapper.go Show resolved Hide resolved
pkg/mapper/configmap/client/client.go Show resolved Hide resolved
nnmin-aws added a commit to nnmin-aws/aws-iam-authenticator that referenced this pull request Nov 1, 2022
nnmin-aws added a commit to nnmin-aws/aws-iam-authenticator that referenced this pull request Nov 3, 2022
k8s-ci-robot added a commit that referenced this pull request Nov 3, 2022
nnmin-aws added a commit to nnmin-aws/aws-iam-authenticator that referenced this pull request Nov 5, 2022
nnmin-aws added a commit to nnmin-aws/aws-iam-authenticator that referenced this pull request Nov 8, 2022
@nnmin-aws nnmin-aws mentioned this pull request Nov 8, 2022
nnmin-aws added a commit to nnmin-aws/aws-iam-authenticator that referenced this pull request Nov 8, 2022
k8s-ci-robot added a commit that referenced this pull request Nov 9, 2022
Revert "Add SSO Role suffix support (#416)"
@tommy31
Copy link

tommy31 commented Nov 16, 2022

Hey, any update on when we could use this on EKS ?

@zelch
Copy link

zelch commented Jan 23, 2023

Am I correct in reading that this was reverted by #509 ?

@nnmin-aws

Was there discussion about this somewhere? And/or a plan going forward?

@nnmin-aws
Copy link
Contributor

thanks @zelch for your query. apology if any inconvenience.

#416 has never been picked up in any release. Then it was reverted by #509 in branch release-0.6.

It is still available in master branch. You can build image with master branch if you want to use this feature.

Thank you

@zelch
Copy link

zelch commented Jan 23, 2023

What are the release plans for this change at this point?

@justinas-b
Copy link

Hey @nnmin-aws , could you elaborate more on why this has been reverted? Our organisation is also experiencing this issue and are looking forward for release of this PR.

@OverStruck
Copy link

@justinas-b Looks to have been removed due #504 (reasons why are explain there)

@johnkeates
Copy link

Well, that's a bummer. But I can see how even undocumented usage would still preferably not be broken by a new feature. It does seem to be in need of some pushing/tickling to get this back on track considering not much happened after the bug fix (unless it's waiting for that SSO e2e test mentioned in #506 ?).

@logandavies181
Copy link
Contributor Author

Well, that's a bummer. But I can see how even undocumented usage would still preferably not be broken by a new feature. It does seem to be in need of some pushing/tickling to get this back on track considering not much happened after the bug fix (unless it's waiting for that SSO e2e test mentioned in #506 ?).

It looked like a bug, so I squashed it ¯\_(ツ)_/¯. There was nothing to indicate that it wasn't a bug or that it was expected.

Seeing as there has been this interest, I'll have a go at the E2E test - maybe they'll even review it

@johnkeates
Copy link

Looked like a bug to me as well 🤷 Oh well...

@jku8 jku8 mentioned this pull request Feb 1, 2023
@nnmin-aws
Copy link
Contributor

nnmin-aws commented Feb 1, 2023

Apology for late reply, was out the last a few days.

The background:

(1) #416 was merged into master but it was not picked in any official release.

(2) in v0.6.0 #416 was picked but then issue #504 was found. Hence it was removed

(3) author contributed a fix #506. But there is a new SSO feature design under review. The new design will replace #416. Hence #416 is not re-picked.

We appreciate your interest in SSO feature and can organize some discussion with the new SSO design in kubernetes slack channel if you are interested. @jku8 can help arrange the discussion.

@nckturner
Copy link
Contributor

Let me add some context -- the "new SSO feature" is an integration between Identity Center and EKS. EKS uses the Authenticator, so there is some overlap. However, #416 can exist in the Authenticator code base without being enabled in EKS, so we could choose to support it in the meantime. I think the question is how many users want this feature who are not using EKS, because we will not be enabling it in EKS.

@zelch
Copy link

zelch commented Feb 1, 2023

We are currently running kubernetes in AWS, not using EKS, so this continues to be of extreme interest.

@logandavies181
Copy link
Contributor Author

because we will not be enabling it in EKS.

#416 also refactors some duplicated code and improves uinit test coverage. Can you please just review #506? It does not look possible to sensibly test SSO E2E in CI as getting SSO credentials requires user interaction

@dm3ch
Copy link

dm3ch commented Feb 16, 2023

@nckturner Is there any link where I can track the progress of new IAM Identity Center SSO auth in EKS feature?

@logandavies181
Copy link
Contributor Author

logandavies181 commented Mar 2, 2023

We are currently running kubernetes in AWS, not using EKS, so this continues to be of extreme interest.

@nckturner seeing as there is interest in this but it will not be enabled in EKS, would there be any possibility of the original wildcard feature getting merged in?

Originally this PR allowed for full wildcard support, and there's a great deal of interest in the feature: aws/containers-roadmap#474

It would be great to contribute to the openness of Kubernetes recognize the non-EKS users that are interested in this feature by allowing it to be merged (with a feature flag of course) without going down the fork route

cc. @zelch - would that be useful to non-EKS users such as yourself? I'd certainly like it, but my team is committed to EKS

@dm3ch
Copy link

dm3ch commented Mar 2, 2023

But what are the possible options for EKS users, if this is not going to be enabled in EKS?

@zelch
Copy link

zelch commented Mar 2, 2023

@logandavies181 Absolutely, it would be very useful.

@dm3ch My understanding is that they are working on an EKS specific direct integration between Identity Center and EKS,

@dm3ch
Copy link

dm3ch commented Mar 2, 2023

But is there any links to announces or issues that would allows to track AWS implementation?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. 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. lgtm "Looks good to me", indicates that a PR is ready to be merged. ok-to-test Indicates a non-member PR verified by an org member that is safe to test. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet