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

AWS: Support source_profile #299

Merged
merged 13 commits into from Jun 23, 2023
Merged

Conversation

AndyTitu
Copy link
Contributor

@AndyTitu AndyTitu commented Jun 19, 2023

Overview

This change makes so that having source_profile in your .aws/config file doesn't error out and instead behaves as expected.

This is the expected behaviour:

  1. Case 1: Only source_profile is specified in a profile. --> The source profile is inspected recursively and the right credentials provider is chosen to be used for the current profile.

  2. Case 2: source_profile is specified, alongside other role/mfa specific information (role_arn. mfa_serial etc) --> the source profile is inspected recursively to determine the right provider. This provider is then used as "base" credentials for executing SDK calls to the STS service for retrieving another set of temporary credentials as marked by the other role/mfa specific information present in the current profile.

Further aspects regarding recursivity:

  • Base case for recursivity is using the access keys stored in the associated 1Password item.
  • Endless loops caused by source_profile are handled when parsing the config file using was-vault's LoadFromProfile function.

Type of change

  • Improved an existing plugin

Related Issue(s)

How To Test

Use a config file that specifies source_profile and execute aws sts get-caller-identity --profile <name> to check that the right credentials were retrieved.

Example of config file:

[default]
source_profile=andy
output=json
region=us-east-1

[profile andy]
role_arn=arn:aws:iam::123456789012:role/testRole
mfa_serial=arn:aws:iam::123456789012:mfa/andi

[profile dev]
source_profile=Andy
role_arn=arn:aws:iam::123456789012:role/testRole1

[profile prod]
source_profile=dev
role_arn=arn:aws:iam::123456789012:role/testRole2

Changelog

AWS Shell Plugin now supports sourcing credentials from another profile.

@AndyTitu AndyTitu requested review from jpcoenen and accraw June 19, 2023 10:58
@AndyTitu AndyTitu changed the title AWS: Support source_credentials AWS: Support source_profile Jun 19, 2023
@jpcoenen
Copy link
Member

Thanks for adding this; looks very useful. I have two general questions, not about a specific part of the code:

  1. What's the source of the expected behaviour as described in the PR description? How do we know that is actually how this works?
  2. Could we add a test to make sure that a source_profile loop is detected? That would make sure that we still catch that even if we replace the code to load the config file.

@AndyTitu
Copy link
Contributor Author

AndyTitu commented Jun 19, 2023

What's the source of the expected behaviour as described in the PR description? How do we know that is actually how this works?

According to https://docs.aws.amazon.com/cli/latest/topic/config-vars.html#using-aws-iam-roles:

source_profile - The AWS CLI profile that contains credentials / configuration the CLI should use for the initial assume-role call. This profile may be another profile configured to use assume-role, though if static credentials are present in the profile they will take precedence.

The static credentials part is not something we need to concern ourselves about: Our 'static' credentials are the access keys stored in 1P and because now we support only 1 credential to be mapped to a plugin, users will always use that 1 access key pair, so looking for another pair of static credentials that can be associated to the called profile is out of scope and not supported by us.

@AndyTitu
Copy link
Contributor Author

AndyTitu commented Jun 19, 2023

Could we add a test to make sure that a source_profile loop is detected? That would make sure that we still catch that even if we replace the code to load the config file.

On a second check, I have realised there's a bug in aws-vault logic which makes it not detect endless loops in source_profile, whereas the AWS cli detects them. In order to not rewrite all logic for parsing config files, as we are using aws-vault's util function, I have added an additional check which is performed before continuing to parse the config file.

I have opened an issue on was-vault's repo: 99designs/aws-vault#1211

I've also found out self-loops, i.e. profiles which are referencing themselves, are accepted as they would normally work with AWS cli, and it's our tools responsibility to gracefully handle that case. aws-vault util on which we rely, handles that.

I have introduced a couple of regression tests to cover both roundtrip loop and self-loops.

@jpcoenen
Copy link
Member

What's the source of the expected behaviour as described in the PR description? How do we know that is actually how this works?

According to https://docs.aws.amazon.com/cli/latest/topic/config-vars.html#using-aws-iam-roles:

source_profile - The AWS CLI profile that contains credentials / configuration the CLI should use for the initial assume-role call. This profile may be another profile configured to use assume-role, though if static credentials are present in the profile they will take precedence.

The static credentials part is not something we need to concern ourselves about: Our 'static' credentials are the access keys stored in 1P and because now we support only 1 credential to be mapped to a plugin, users will always use that 1 access key pair, so looking for another pair of static credentials that can be associated to the called profile is out of scope and not supported by us.

I agree that we are indeed achieving the functionality that is described in there. I think it will work for some cases, but I am not sure that we are achieving 100% parity with the CLI when it comes to MFA. Just to check

If I understand it correctly, we're only supporting an MFA device on the first role in the chain (i.e. the one that the user refers to). However, it is very well possible that your setup requires the MFA device on a different link that chain.

For example, say you have role A and B. The user can assume role A and must use MFA to do so. Role A can assume role B. In a normal config that would look something like:

[profile roleA]
role_arn=arn:aws:iam::123456789012:role/RoleA
mfa_serial=arn:aws:iam::123456789012:mfa/1Password

[profile roleB]
source_profile=roleA
role_arn=arn:aws:iam::123456789012:role/RoleB

I am pretty sure this is possible with the AWS CLI, but I am not sure whether this is possible with our shell plugin. The example config in the PR description suggests you have gotten this to work. Are you sure that the MFA device was actually used when using the dev or prod roles in that example?

@AndyTitu
Copy link
Contributor Author

AndyTitu commented Jun 20, 2023

Yes yes, this is exactly what happens in our current version. Roles are chained and each profile uses all information available for retrieving their temporary credentials to pass to the next profile in the chain (so the scenario explained in your example works as you'd expect)

… if mfa serial and otp are in 1Password item they will be used for all profiles. If only OTP is in 1Password item, it will be used only in profiles that present an mfa serial in config
@AndyTitu
Copy link
Contributor Author

AndyTitu commented Jun 20, 2023

There still is another edge case matter I'm looking to address: Using mfa authentication in multiple of the chained profiles. Specifically using the same virtual MFA device in multiple profiles in the chain.

Using several different MFA devices along the chain of profiles is out of scope because we only support 1 item per plugin.

UPDATE: we have decided this use case does not need to be supported, as it's something that wouldn't really occur in practice

Copy link
Contributor

@libutcher libutcher left a comment

Choose a reason for hiding this comment

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

Took a look at the rest of the error messages and left some small questions and notes, but overall I think they look good!

plugins/aws/access_key_test.go Show resolved Hide resolved
plugins/aws/sts_provisioner.go Outdated Show resolved Hide resolved
@AndyTitu AndyTitu added the waiting-on-reviewer signals that a certain PR is waiting for a review from a 1Password team member label Jun 22, 2023
Copy link
Member

@jpcoenen jpcoenen left a comment

Choose a reason for hiding this comment

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

One tiny remark, but overall this looks good to me. Great job, Andy! 🙌

plugins/aws/sts_provisioner.go Outdated Show resolved Hide resolved
Copy link
Member

@hculea hculea left a comment

Choose a reason for hiding this comment

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

Did a code-review, looks good in broad lines, only a couple nits and a question!

plugins/aws/sts_provisioner.go Outdated Show resolved Hide resolved
plugins/aws/sts_provisioner.go Outdated Show resolved Hide resolved
plugins/aws/sts_provisioner.go Show resolved Hide resolved
plugins/aws/access_key_test.go Outdated Show resolved Hide resolved
@AndyTitu AndyTitu removed the request for review from accraw June 23, 2023 09:45
@AndyTitu AndyTitu merged commit cd89b26 into main Jun 23, 2023
5 checks passed
@AndyTitu AndyTitu deleted the andi_t/aws-source-profile-support branch June 23, 2023 11:06
@arunsathiya arunsathiya removed the waiting-on-reviewer signals that a certain PR is waiting for a review from a 1Password team member label Aug 15, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

AWS - support sourcing credentials from a different profile
5 participants