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

Restore compatibility with aws-cli #438

Closed
mtibben opened this issue Oct 28, 2019 · 20 comments · Fixed by #440 or #445
Closed

Restore compatibility with aws-cli #438

mtibben opened this issue Oct 28, 2019 · 20 comments · Fixed by #440 or #445

Comments

@mtibben
Copy link
Member

mtibben commented Oct 28, 2019

#369 allowed aws-vault to re-use credentials between profiles, which came at the expense of aws-cli compatibility. This went against the approach in #164 and #301 where compatibility with aws-cli was the first priority.

So aws-vault now recognises mfa_serial in a source_profile while aws-cli does not.

This has been the cause of a number of subtle bugs I've experienced, originating from a mis-understanding of how aws-cli uses credentials and config. Many of our profiles use the same credentials but individual roles may specify a MFA device. This means that if a non-MFA role is used first the session is cached and trying to use roles with MFA will subsequently fail.

While I could now work around this by creating different source profiles, this is not an issue with aws-cli and I feel this divergence is more confusing than it's worth, and we should restore compatibility with aws-cli.

I'm pretty sure we can have both compatibility with the aws-cli and keep the session chaining. This can be achieved by changing our session caching strategy to use the mfa metadata, and look up sessions based on the mfa device being used.

Thoughts @lox @jstewmon @rossmckelvie @FernandoMiguel

@mtibben mtibben changed the title mfa_serial Restore compatibility with aws-cli Oct 28, 2019
@FernandoMiguel
Copy link
Collaborator

I personally prefer the easiness of a single input of MFA rather than once per role.
Also, security wise it would be preferred for account managers to secure their accounts with MFA everywhere.

Bottom line, if someone is using a less secure pattern then they should be using a different source profile, like you said.

@FernandoMiguel
Copy link
Collaborator

A bit off topic here but,
Does anyone using aws-vault actually uses aws cli outside of aws-vault?
Sounds like an extremely rare occurrence for us to bother in keeping compability.
But if we want to make that obvious, why not use a different filename for the config?
If that exists, we ignore the standard config file

@frezbo
Copy link
Contributor

frezbo commented Oct 28, 2019

Agree with @mtibben here, I use more than 10 profiles and almost all profiles are always used alongside aws-vault. There are two or three profiles that I use where the credential_source=Ec2InstanceMetadata, this works inside a --server session. This is because I need to switch between aws accounts.

@FernandoMiguel
Copy link
Collaborator

@frezbo using STS and having your app assume a new role would be ideal, if possible to implement

@frezbo
Copy link
Contributor

frezbo commented Oct 28, 2019

@FernandoMiguel yes that what our app does, it directly uses the aws sdks. Others things are just for local testing.

@jstewmon
Copy link
Collaborator

@mtibben I do consider it a bug that the session cache key does not have the mfa_serial encoded somehow - users shouldn't have to consider nuances like the combination of a source_profile with and without an mfa_serial or what happens if they change their mfa_serial in their config.

Can you recall any of the other subtle bugs you've observed? I'm willing to invest some time reproducing and fixing.

At my current job, we have around 130 team members who use aws-vault with the session caching features I contributed to ease friction in their daily workflow. I'm not sure if you meant that the specific behavior you mentioned or the session caching in general is "more confusing than it's worth", but if you meant the latter, I (and my colleagues) respectfully disagree, since we are deriving substantial value from it.

FWIW, my perspective on aws-vault compatibility with AWS CLI / SDKs is that aws-vault is fundamentally incompatible with the AWS CLI and SDKs, since it stores credentials in the keychain, which the CLI and SDKs do not support. i.e. users cannot use the same config file with aws-vault and AWS CLI / SDKs without making special entries to create compatibility.

That said, I think aws-vault should be intuitive for AWS CLI and SDK users, and unexpected behaviors like the one you've raise in this issue should be considered bugs, or documented if the correct behavior cannot be inferred based on configuration or usage.

@mtibben
Copy link
Member Author

mtibben commented Oct 28, 2019

Yes I do completely agree, the session caching/chaining is certainly a valuable feature that we should absolutely retain. Our team has also appreciated it, it's just always been an annoyance that we've need to clear sessions when we run into this bug pretty regularly.

FWIW, my perspective on aws-vault compatibility with AWS CLI / SDKs is that aws-vault is fundamentally incompatible with the AWS CLI and SDKs

Yes, but it's nice to be able to say "config is the exactly the same as aws-cli". Do we really get value from the one change of mfa_serial being allowed in source_profile? What's the actual use-case behind this divergence - is it when you have a lot of profiles to define? At 99 we distribute a .aws/config template and updating the MFA is just a find-and-replace

@jstewmon
Copy link
Collaborator

Do we really get value from the one change of mfa_serial being allowed in source_profile? What's the actual use-case behind this divergence - is it when you have a lot of profiles to define? At 99 we distribute a .aws/config template and updating the MFA is just a find-and-replace

We also distribute a .aws/config, but the user only has to change their mfa_serial in the master creds entry.

As it stands today, there is no difference in aws-vault whether you put your mfa_serial on the master profile or on every profile if every profile when using assume role...

In the olden days, the latter required you to enter your MFA token for every profile on which you defined an mfa_serial.

The only way to let users assume multiple roles without entering a token per profile is to reduce the role chain to a pair of master creds and an mfa serial, get a session token, and cache that token for future assume role calls that use the same combination of master creds and mfa serial. With this behavior, it doesn't really matter if the mfa_serial is allowed to be defined on the master profile or not.

But, there's more - we also require mfa to rotate access keys, so supporting mfa_serial on the master profile enabled that feature for us.

@mtibben
Copy link
Member Author

mtibben commented Oct 29, 2019

But, there's more - we also require mfa to rotate access keys, so supporting mfa_serial on the master profile enabled that feature for us.

That is a good point

@rossmckelvie
Copy link
Contributor

Happy to help resolve any bugs we might have caused, though I'm in favor of keeping the MFA session improvements. Reducing the number of requests for MFA tokens struck the perfect balance between security and ergonomics for us, to the point that I consider aws-vault today a key component of our workflow & aws organization design. The aws-cli is only used in the context of aws-vault exec for all of our staff.

In total I have about 50 roles I can assume as an administrator, though most developers in my organization have ~5-10 profiles that they use. The roles that we assume have max session lengths ranging from 15-60min, and we allow for a longer session to be created using the master creds; as a result we are only prompting users for MFA a couple of times a day. Prior to jstewmon's changes, I was entering a MFA token every half hour, multiple times when I was testing resource sharing & networking across accounts.

@mtibben
Copy link
Member Author

mtibben commented Oct 29, 2019

But, there's more - we also require mfa to rotate access keys, so supporting mfa_serial on the master profile enabled that feature for us.

@jstewmon so thinking about this more, there are still edge-cases. Consider the following scenario

[profile default]
region=us-east-1
mfa_serial=arn:aws:iam::yyyyyyyyyyy:mfa/user.name

[profile read-only]
region=us-east-1
role_arn=arn:aws:iam::xxxxxxxxxxx:role/ReadOnly
source_profile=default

[profile admin]
region=us-east-1
role_arn=arn:aws:iam::xxxxxxxxxxx:role/Administrator
mfa_serial=arn:aws:iam::yyyyyyyyyyy:mfa/user.name
source_profile=default
  • default requires MFA for key rotations
  • read-only does not require MFA
  • admin requires MFA

So right now aws-vault does not allow this config to work. read-only will inherit from default and MFA will be required. If we remove mfa_serial from default, key rotation will not work.

@rossmckelvie
Copy link
Contributor

So right now aws-vault does not allow this config to work. read-only will inherit from default and MFA will be required.

This config still works. you can still assume the role. aws-vault currently does prompt for MFA to create the session for the default profile, even though it's not needed for read-only, though I'd argue that any role you have iam user principals assuming should require MFA.

The only way I was able to break the config in your example was by explicitly denying assume role with MFA present:

{
    "Version": "2012-10-17",
    "Statement": [
        {
            "Sid": "AssumeReadOnlyRoleWithoutMFA",
            "Effect": "Allow",
            "Action": "sts:AssumeRole",
            "Resource": [
                "arn:aws:iam::123456789012:role/ReadOnly"
            ]
        },
        {
            "Sid": "DenyAssumeReadOnlyRoleWithMFA",
            "Effect": "Deny",
            "Action": "sts:AssumeRole",
            "Resource": [
                "arn:aws:iam::123456789012:role/ReadOnly"
            ],
            "Condition": {
                "Bool": {
                    "aws:MultiFactorAuthPresent": "true"
                }
            }
        }
    ]
}

With @jstewmon's changes, you don't need to redefine the mfa_serial in the admin profile, even though that role's trust policy will require it be present. That is one of the added benefits of the changes imo.

@mtibben
Copy link
Member Author

mtibben commented Oct 29, 2019

@rossmckelvie the whole point of the read-only role is that MFA is not required. So this is not behaving the way I'd expect.

With @jstewmon's changes, you don't need to redefine the mfa_serial in the admin profile, even though that role's trust policy will require it be present. That is one of the added benefits of the changes imo.

So the benefit I'm hearing is that it allows us to be a bit more DRY, and saves some typing. Is that correct? If this comes at the cost of compatibility and functionality, then I'm not convinced this stacks up.

Note I'm not talking about the session caching/chaining. That stays. I'm just talking about the cascading mfa_serial config

@jstewmon
Copy link
Collaborator

So the benefit I'm hearing is that it allows us to be a bit more DRY, and saves some typing. Is that correct? If this comes at the cost of compatibility and functionality, then I'm not convinced this stacks up.

IIUC, what you'd suggest is that given the config below, you get the following behaviors:

  • aws-vault cmd default will prompt for MFA on session miss, allowing e.g. aws-vault login default, aws-vault rotate default to satisfy MFA conditions
  • aws-vault cmd read-only will not prompt for MFA
  • aws-vault cmd admin will prompt for MFA on session miss
[profile default]
region=us-east-1
mfa_serial=arn:aws:iam::yyyyyyyyyyy:mfa/user.name

[profile read-only]
region=us-east-1
role_arn=arn:aws:iam::xxxxxxxxxxx:role/ReadOnly
source_profile=default

[profile admin]
region=us-east-1
role_arn=arn:aws:iam::xxxxxxxxxxx:role/Administrator
mfa_serial=arn:aws:iam::yyyyyyyyyyy:mfa/user.name
source_profile=default

I can see why you'd expect those, but I also think it's harder for the user to reason about the expected behavior when referencing the default profile because its mfa_serial is only used with the profile is referenced directly, so the user now has to be familiar with the logic of aws-vault. I think this is also reflected in how we would have to implement the behavior, since when searching a profile chain for an mfa serial, we'd have to exclude the master credentials profile from the search.

I think what @rossmckelvie is suggesting is that it's a better tradeoff for the user to possibly encounter an MFA prompt when using the read-only role, since it is highly unlikely that presenting an MFA token would be problematic.

Please correct me if I've misunderstood the suggested change in behavior. At this point, I think either way is acceptable, but I think the current behavior is easier to reason about.

In any case, the buggy behavior around caching can be resolved in #440 - we can continue discussing the mechanics here as long as is needed.

@FernandoMiguel
Copy link
Collaborator

@rossmckelvie the whole point of the read-only role is that MFA is not required. So this is not behaving the way I'd expect.

With @jstewmon's changes, you don't need to redefine the mfa_serial in the admin profile, even though that role's trust policy will require it be present. That is one of the added benefits of the changes imo.

So the benefit I'm hearing is that it allows us to be a bit more DRY, and saves some typing. Is that correct? If this comes at the cost of compatibility and functionality, then I'm not convinced this stacks up.

Note I'm not talking about the session caching/chaining. That stays. I'm just talking about the cascading mfa_serial config

But, there's more - we also require mfa to rotate access keys, so supporting mfa_serial on the master profile enabled that feature for us.

@jstewmon so thinking about this more, there are still edge-cases. Consider the following scenario

[profile default]
region=us-east-1
mfa_serial=arn:aws:iam::yyyyyyyyyyy:mfa/user.name

[profile read-only]
region=us-east-1
role_arn=arn:aws:iam::xxxxxxxxxxx:role/ReadOnly
source_profile=default

[profile admin]
region=us-east-1
role_arn=arn:aws:iam::xxxxxxxxxxx:role/Administrator
mfa_serial=arn:aws:iam::yyyyyyyyyyy:mfa/user.name
source_profile=default
  • default requires MFA for key rotations
  • read-only does not require MFA
  • admin requires MFA

So right now aws-vault does not allow this config to work. read-only will inherit from default and MFA will be required. If we remove mfa_serial from default, key rotation will not work.

taking a different approach here, that might not be the most consensual:
why would you even want a role with MFA?
even if it is read only, it scopes into your accounts, and is a single factor protection

@mtibben mtibben reopened this Oct 30, 2019
@reegnz
Copy link
Contributor

reegnz commented Nov 5, 2019

Just my 2 cents:

So I use the AWS_MFA_SERIAL environmental variable instead, to keep the config portable across users.
The reason is because everyone has a different value for mfa_serial, and the config is not easy to share. This way I can put the aws config into version control and have it be used by AWS_CONFIG_FILE environment variable.

That also enables combining profile + MFA requirements in the environment and keeps user-specific configuration out of the aws config itself.

I do believe having the mfa_serial in the config is an anti-pattern in itself, at least with profile credentials it was put in a separate file to decouple user-specific config from a generic config.

The same goes to the initial mission of aws-vault: decouple user-specific access keys from the config and put it into the environment 12 factor app style.

For me I think it would be best not to cram this mfa_serial logic into the aws config itself, but give a similarly decoupled solution for pairing profiles with mfa keys, maybe in a global aws-vault config file. In my use-case the idea is to keep user-specific config out of the aws config file.

Right now I am solving this issue with a direnv-based solution, but it would be nice for the tooling to support it out of the box. Either aws-sdk or aws-vault (but I do feel that the problem in this case is with aws-sdk itself).

@FernandoMiguel
Copy link
Collaborator

@reegnz how are you doing the AWS_MFA_SERIAL as env var?
I tried that before (even had a ticket here) but got nothing :/

@reegnz
Copy link
Contributor

reegnz commented Nov 5, 2019

@FernandoMiguel let me show you an example config for my setup:

aws-config

[profile root]
region = us-east-1
credential_process = aws-vault exec root --json

[profile account-1-admin]
region = us-east-1
source_profile = root
role_arn = arn:aws:iam::222222222222:role/AdminRole

With .envrc (direnv config) I set the environment as follows:

export AWS_CONFIG_FILE=$PWD/aws-config
export AWS_PROFILE=account-1-admin
export AWS_SDK_LOAD_CONFIG=1

In my .zshrc (or .bashrc) I put the config specific to my own environment:

export AWS_VAULT_KEYCHAIN_NAME=login
export AWS_VAULT_PROMPT=osascript
AWS_SESSION_TTL=12h
AWS_FEDERATION_TOKEN_TTL=12h
AWS_ASSUME_ROLE_TTL=1h
AWS_MFA_SERIAL=arn:aws:iam::222222222222:mfa/reegnz

When I cd into the directory, everything is set up, the profile is selected that I need in that directory and I just start using the tool I want to use, no direct interaction with aws-vault at all. It needs to be fully transparent for developers not to feel it difficult working with multiple accounts.

The only difficulty they encounter is having to run direnv allow on first checkout of the repo and enter their MFA token every day the first time they start working on AWS.

So you see, defining mfa_serial within the config isn't necessarily the only MFA use-case, I'd argue that using environment variables for configuring the AWS_SDK is the less common, but still widely used approach.

@FernandoMiguel
Copy link
Collaborator

I dont need it now (AzureAD sadly) but good to know.
would have been great 2 years ago!
would have simplified my devs life a lot

@mtibben
Copy link
Member Author

mtibben commented Nov 6, 2019

OK excellent, now that we have a way to specify a MFA serial with AWS_MFA_SERIAL I believe this has removed the remaining blocking issue for @jstewmon @rossmckelvie @FernandoMiguel.

I've created a PR at #445

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
6 participants