Support un-canonicalized ARNs in filemapper

[logandavies181]

• Fixes got "*access denied*" when pick up authenticator v0.6.0 #504

Re-adds undocumented behaviour of allowing non-canonicalized role ARNs to be specified in the MountedFile config file

[k8s-ci-robot]

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.

[logandavies181]

/cc @nnmin-aws

[k8s-ci-robot]

@logandavies181: GitHub didn't allow me to request PR reviews from the following users: nnmin-aws.

Note that only kubernetes-sigs members and repo collaborators can review this PR, and authors cannot review their own PRs.

In response to this:

/cc @nnmin-aws

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.

[nnmin-aws]

thanks @logandavies181 for the swift code update! Could you please kindly run make e2e RUNNER=kind to test this fix? @nckturner

[logandavies181]

thanks @logandavies181 for the swift code update! Could you please kindly run make e2e RUNNER=kind to test this fix? @nckturner

My development environment isn't set up for the e2e tests. How about a reviewer marks this as ok-to-test?

[nckturner]

/ok-to-test

[nnmin-aws]

@logandavies181: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name Commit Details Required Rerun command
pull-aws-iam-authenticator-e2e-kind 236a7b8 link true /test pull-aws-iam-authenticator-e2e-kind
Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

apology @logandavies181 for your inconvenience. we have noticed this and working on it #513. it will be fixed soon.

[nnmin-aws]

@logandavies181: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:
Test name Commit Details Required Rerun command
pull-aws-iam-authenticator-e2e-kind 236a7b8 link true /test pull-aws-iam-authenticator-e2e-kind
Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

apology @logandavies181 for your inconvenience. we have noticed this and working on it #513. it will be fixed soon.

Hi @logandavies181 #513 is merged. Could you please kindly gave another try? Thank you

[logandavies181]

/test pull-aws-iam-authenticator-e2e-kind

[logandavies181]

Thanks @nnmin-aws the test passes.

Over to you @nckturner @jyotimahapatra

[logandavies181]

So is #504 instead being fixed on the EKS side?

[nnmin-aws]

So is #504 instead being fixed on the EKS side?

apology @logandavies181 for late reply. This fix passed the test, thank you for the fix!

Could you please kindly add an end-to-end test for this SSO feature?

[logandavies181]

@nnmin-aws No sorry, given that this change is only related to how the fileMapper parses uncanonicalized ARNs.

There were no e2e tests when I wrote the feature so really the maintainers should have added a test for that when the e2e tests were added.

I'd otherwise be happy to write a test but given the glacial pace of reviews here I'm feeling far too discouraged to spend my free time doing so

[nnmin-aws]

I am so sorry for it. Apology for the delay on this PR because we were upgrading the prow jobs at the moment. The upgrade will benefit all the coming PR.

We'll continue to improve the review process. Your contribution is valuable to us.

[logandavies181]

In this PR I'd be happy to respond to comments on the supplied code and the problem that it is trying to fix.

If adding the tests in a separate PR would mean the feature being bumped to beta and activated by default and available in EKS then I'd gladly take the time to do so

[zelch]

Hello @nnmin-aws .

Given that this appears to be a blocker for getting a release with #416 , what is the current status on this?

Is this change good to go? Is adding more test cases a hard requirement before it is merged? If the latter, can you give some specifics on the test cases that need to be made?

[zelch]

/test pull-aws-iam-authenticator-e2e-kind
/test pull-aws-iam-authenticator-e2e

[logandavies181]

So I've been looking at this in earnest, however I'm a bit stumped as to how I would even get valid SSO creds in CI as it seems to only support browser-based login flows. Anyone have any ideas on this? @nnmin-aws?

[zelch]

@logandavies181

Looking at the AWS documentation, they explicitly do not support any kind of browserless authentication method.

Digging deeper, they are using the RFC 8628 OAUTH Device Authentication flow.

Now, it does look like some people have tried a few different routes for this, like https://mziyabo.co/2022/aws-sso-headless-login/, however those still depend on user interaction, and seem to require a full browser somewhere as well.

I have some thoughts on how it might be possible, eventually, to provide some automation around this, however at the moment my only real suggestion is to setup AWS Identity Center, login, and capture a bunch about the role that it gives you, and then build a similar role for the CI environment.

As far as the feature goes, the biggest thing that is needed is the wildcard support, so simply doing a static role with a wildcard as part of the match would probably test the bulk of the functionality.

And if we can ever figure out a good solution for a fully headless login, we can extend the test cases.

(Short version for automation: You can sign into some supported identity providers, such as Okta, via an API. If it is at all possible to obtain credentials usable as a browser cookie from that login, then the problem 'just' becomes convincing the remaining login flow that our automation is a browser. Visit the URL provided by aws sso login --no-browser, step through things to the IdP stage, for the IdP stage use the browser cookie so that we are already logged into the IdP, and then once we are redirected back to AWS find the link for approving the CLI application and visit it. In concept, doable. In practice, probably a lot harder. I have no idea if I'll have the time to even try anytime in the next several months. Any level of active support from engineering at either AWS or a supported IdP would be amazingly helpful though. And if that turned into something that allowed a user to enter the username, password, and use a WebAuthn token all from the CLI cleanly, it would be a significant accessibility gain for many users, including myself.)

[nnmin-aws]

thanks @zelch @logandavies181 for your idea on sso feature! we are working on a new SSO feature design and your ideas are valuable to us.

We would like to have some discussion in slack channel about SSO feature as there is a lot of interest on it.
I will post the slack url later on.
thank you

[zelch]

Hi @nnmin-aws, I am definitely willing and interested.

Schedule wise, I will be away from the 6th until the 21st, so if we could at least start the discussion this week that would be great. :)

[nckturner]

I don't personally think slack is the best place for such a discussion as its not discoverable, however we were chatting in a thread in the #provider-aws channel to try to find a good place for a discussion. I propose we have a recorded conversation in the provider-aws biweekly meeting this Friday at 9am PST. I'll add it to the agenda here: https://docs.google.com/document/d/1-i0xQidlXnFEP9fXHWkBxqySkXwJnrGJP9OGyP2_P14. If the time doesn't work for anyone, we can schedule a stand-alone meeting using the provider-aws zoom.

[logandavies181]

Coming back to this PR: it's looking like it's not possible to sensibly test this in CI, and you're not planning on supporting this in EKS; so can you please just review this PR? It's not very big and it solves the issue #504

@nckturner @jyotimahapatra

[nckturner]

/lgtm
/approve

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: logandavies181, nckturner

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:

• OWNERS [nckturner]

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