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

Resolve AWS account aliases (if possible) and print them to the users #39

Merged
merged 4 commits into from
Jan 29, 2018

Conversation

s-maj
Copy link

@s-maj s-maj commented Jan 11, 2018

This change allows to print AWS account alias and role ARN when -a option is used. It's easier to select correct account when alias is printed, especially when we have set of roles called "Administrator" across accounts.

@coveralls
Copy link

coveralls commented Jan 11, 2018

Coverage Status

Coverage decreased (-0.6%) to 31.818% when pulling 807a1df on s-maj:master into 50bcd11 on cevoaustralia:master.

@mide
Copy link
Contributor

mide commented Jan 11, 2018

I just had a discussion the other day about this, and how useful it would be. I'm completely in favor of this! I would prefer PR #38 gets in first, and then we can rebase this (I will gladly help).

@s-maj
Copy link
Author

s-maj commented Jan 11, 2018

Sure thing. I've subscribed #38 and I'll rebase once it's merged.

@coveralls
Copy link

Coverage Status

Coverage decreased (-1.1%) to 31.323% when pulling 0d3356c on s-maj:master into 50bcd11 on cevoaustralia:master.

@coveralls
Copy link

coveralls commented Jan 12, 2018

Coverage Status

Coverage decreased (-2.9%) to 40.074% when pulling 0ce92ce on s-maj:master into 75d0ba5 on cevoaustralia:master.

@coveralls
Copy link

Coverage Status

Coverage decreased (-1.1%) to 31.323% when pulling 0d3356c on s-maj:master into 50bcd11 on cevoaustralia:master.

@nonspecialist
Copy link
Contributor

Hmmm ... I have to say I like the idea, but I'm a bit uncertain about this implementation. Every time someone authenticates, there's going to be a flood of sts:AssumeRoleWithSAML calls, whether that account is actually used or not in the end. That's going to pollute CloudTrail, at the very least.

A couple of alternatives that I can think of quickly:

  • keep an alias of each account in some file under ~/.aws; load it if it's there, create/update it every time you assume a role in an account so that it's there for next time (means the first time you use the tool for each account, it's confusing; and if account names change, you don't see that until afterwards)
  • allow user-assigned aliases for accounts (implementation thoughts based on hand-waving)

Let's keep kicking this around a bit ...

@s-maj
Copy link
Author

s-maj commented Jan 19, 2018 via email

@nonspecialist
Copy link
Contributor

aha ... well, as it turns out, that landing page is only presented if you're a browser. For AssumeRoleWithSAML it never shows up. I suppose we could pretend to be a browser ... again.

@s-maj
Copy link
Author

s-maj commented Jan 28, 2018

@mide this should work with 0.18.
@nonspecialist like you said, it might create mess in CloudTrail but now it's disabled by default.

@mide
Copy link
Contributor

mide commented Jan 28, 2018

Awesome! I know I personally would love this feature!

@nonspecialist
Copy link
Contributor

It works, and it doesn't appreciably slow things down. I like the usability piece, and I like that it's not on-by-default (we can change that later once it's had a bit of time to soak).

The one thing I think we can improve is the output; for one example use case, I see:

$ aws-google-auth -a --resolve-aliases
Failed to import U2F libraries, U2F login unavailable. Other methods can still continue.
Google Password: 
Open the Google App, and tap 'Yes' on the prompt to sign in ...
[  1] demo arn:aws:iam::123456789012:role/Demo-Administrator
[  2] dev arn:aws:iam::987654321098:role/Dev-Administrator
[  3] prod arn:aws:iam::314159265359:role/Prod-Administrator
Type the number (1 - 3) of the role to assume: 2

and I find that the account alias gets lost between the choice number and the ARN. Perhaps using a separator, or more whitespace (or a subset of the ARN, like just the role name without the rest) would make it more readable.

How about this as a mock-up:

$ aws-google-auth -a --resolve-aliases
Failed to import U2F libraries, U2F login unavailable. Other methods can still continue.
Google Password: 
Open the Google App, and tap 'Yes' on the prompt to sign in ...
[  1] demo      (Demo-Administrator)
[  2] dev       (Dev-Administrator)
[  3] prod      (Prod-Administrator)
Type the number (1 - 3) of the role to assume: 2

All I've done is trim the "noise" of the ARN and standardised the whitespace with tabs so that the account aliases are separated from the role names.

Other thoughts:

Using the @ syntax to indicate Role@account-alias:

$ aws-google-auth -a --resolve-aliases
Failed to import U2F libraries, U2F login unavailable. Other methods can still continue.
Google Password: 
Open the Google App, and tap 'Yes' on the prompt to sign in ...
[  1] Demo-Administrator@demo
[  2] Dev-Administrator@dev
[  3] Prod-Administrator@prod
Type the number (1 - 3) of the role to assume: 2

Using the term Account 123456789012 when no account alias is set (instead of the current AliasNotAvailable):

$ aws-google-auth -a --resolve-aliases
Failed to import U2F libraries, U2F login unavailable. Other methods can still continue.
Google Password: 
Open the Google App, and tap 'Yes' on the prompt to sign in ...
[  1] Demo-Administrator@demo
[  2] Dev-Administrator@(Account 123456789012)
[  3] Prod-Administrator@prod
Type the number (1 - 3) of the role to assume: 2

I'm happy with the technical implementation, but I'd like to make sure we improve the usability as much as we reasonably can, since that's the purpose of this improvement.

@s-maj
Copy link
Author

s-maj commented Jan 28, 2018

I really like "@" design but I think that account@role_name would be better (eg my-prod-account@administrator) than role_name@account.

@nonspecialist
Copy link
Contributor

That's interesting ... I wonder if this is something that (gasp) we should start being aware of from a localisation perspective (is this a cultural difference for precedence in organisational hierarchy?).

In my thinking, it's like an email address: user@domain (and so role@account) but I'd be keen to hear whether I'm making a broad assumption about how other languages tend to organise these things (from your perspective, Polish ... right?)

@s-maj
Copy link
Author

s-maj commented Jan 28, 2018

Right, I don't think this is culturally or lingually related :) In my org, one person might have multiple roles assigned so I thought that might be better to output like this:

  • My-Account-A@Administrator
  • My-Account-A@BI-Developer
  • My-Account-A@Developer
  • My-Account-A@FE-Developer
  • My-Account-B@Administrator
  • My-Account-B@BI-Developer
  • My-Account-B@Developer

rather than this:

  • Administrator@My-Account-A
  • Administrator@My-Account-B
  • BI-Developer@My-Account-A
  • BI-Developer@My-Account-B
  • Developer@My-Account-A
  • Developer@My-Account-B
  • FE-Developer@My-Account-A

but honestly both options look good. I'll test sorting and flip output ot role@account.

@nonspecialist
Copy link
Contributor

Yeah I see your point -- that is visually a lot cleaner for your use case. So in that case, because I have a gut feeling (not backed up by any research) that use of the @ symbol in this case will cause confusion where people only have one role in 2 or more accounts, how about we go for Account::Rolename -- so you'd get:

[  1] My-Account-A::Administrator
[  2] My-Account-A::BI-Developer
[  3] My-Account-A::Developer
[  4] My-Account-A::FE-Developer
[  5] My-Account-B::Administrator
[  6] My-Account-B::BI-Developer
[  7] My-Account-B::Developer

and I'd see:

[  1] demo::Demo-Administrator
[  2] Account 123456789012::Dev-Administrator
[  3] prod::Prod-Administrator

@s-maj
Copy link
Author

s-maj commented Jan 28, 2018

Ok, I've been experimenting with tabulate library. How about something like this:

Failed to import U2F libraries, U2F login unavailable. Other methods can still continue.
Google Password:
Open the Google App, and tap 'Yes' on the prompt to sign in ...
  No  AWS account       Role
----  ----------------  -------------
   1  ddddd-ppppp-dev   Administrator
   2  ddddd-ppppp-dev   Developer
   3  ddddd-ppppp-live  Administrator
   4  ddddd-ppppp-live  Developer
   5  123456789012      Administrator
Type the number (1 - 5) of the role to assume:

@nonspecialist
Copy link
Contributor

that looks very nice. 👍

@s-maj
Copy link
Author

s-maj commented Jan 29, 2018

Alright, here you go.

Copy link
Contributor

@nonspecialist nonspecialist left a comment

Choose a reason for hiding this comment

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

I like it. I'll allow the drop in coverage for this usability piece, because most of it comes from needing to mock out boto3 calls (which we have a good way to do, but isn't really relevant for this PR)

@nonspecialist nonspecialist merged commit 39d57c4 into cevoaustralia:master Jan 29, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants