Skip to content

AWS: Support source_profile#299

Merged
AndyTitu merged 13 commits into
mainfrom
andi_t/aws-source-profile-support
Jun 23, 2023
Merged

AWS: Support source_profile#299
AndyTitu merged 13 commits into
mainfrom
andi_t/aws-source-profile-support

Conversation

@AndyTitu

@AndyTitu AndyTitu commented Jun 19, 2023

Copy link
Copy Markdown
Contributor

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 accraw and jpcoenen June 19, 2023 10:58
@AndyTitu AndyTitu changed the title AWS: Support source_credentials AWS: Support source_profile Jun 19, 2023
@jpcoenen

Copy link
Copy Markdown
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

AndyTitu commented Jun 19, 2023

Copy link
Copy Markdown
Contributor Author

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

AndyTitu commented Jun 19, 2023

Copy link
Copy Markdown
Contributor Author

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
Copy Markdown
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

AndyTitu commented Jun 20, 2023

Copy link
Copy Markdown
Contributor Author

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

AndyTitu commented Jun 20, 2023

Copy link
Copy Markdown
Contributor Author

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

Comment thread plugins/aws/sts_provisioner.go Outdated

@libutcher libutcher left a comment

Copy link
Copy Markdown
Contributor

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!

Comment thread plugins/aws/access_key_test.go
Comment thread plugins/aws/sts_provisioner.go Outdated
@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

@jpcoenen jpcoenen left a comment

Copy link
Copy Markdown
Member

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! 🙌

Comment thread plugins/aws/sts_provisioner.go Outdated

@hculea hculea left a comment

Copy link
Copy Markdown
Member

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!

Comment thread plugins/aws/sts_provisioner.go Outdated
Comment thread plugins/aws/sts_provisioner.go Outdated
Comment thread plugins/aws/sts_provisioner.go
Comment thread plugins/aws/access_key_test.go Outdated
@AndyTitu AndyTitu removed the request for review from accraw June 23, 2023 09:45
@AndyTitu AndyTitu merged commit cd89b26 into main Jun 23, 2023
@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