Add export-creds command to the CLI

[jamesls]

This PR builds on the interface proposed in #6808 and implements the additional features proposed in #7388.

From the original PRs, the additional features are:

• Added support for an explicit --format args to control the output format.
• Add support for env vars, powershell/windows vars, and a JSON format that's enables this command to be used as a credential_process.
• Detect, and prevent infinite recursion when the credential process resolution results in the CLI calling itself with the same command.

Closes #7388
Closes #5261

[codecov-commenter]

Codecov Report

Merging #7398 (3ce9534) into v2 (ecbb197) will increase coverage by 0.01%.
The diff coverage is 96.12%.

❗ Current head 3ce9534 differs from pull request most recent head 59ebc9c. Consider uploading reports for the commit 59ebc9c to get more accurate results

@@            Coverage Diff             @@
##               v2    #7398      +/-   ##
==========================================
+ Coverage   93.70%   93.72%   +0.01%     
==========================================
  Files         351      352       +1     
  Lines       36175    36306     +131     
  Branches     5202     5221      +19     
==========================================
+ Hits        33899    34028     +129     
  Misses       1652     1652              
- Partials      624      626       +2     

Impacted Files	Coverage Δ
awscli/customizations/configure/exportcreds.py	96.09% <96.09%> (ø)
awscli/customizations/configure/configure.py	100.00% <100.00%> (ø)
awscli/s3transfer/compat.py	86.84% <0.00%> (-5.27%)	⬇️
awscli/botocore/credentials.py	97.39% <0.00%> (-0.11%)	⬇️
awscli/help.py	96.03% <0.00%> (+0.03%)	⬆️
awscli/customizations/commands.py	98.77% <0.00%> (+0.81%)	⬆️
awscli/s3transfer/tasks.py	95.00% <0.00%> (+3.33%)	⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

[jamesls]

Pushed an udpate to add more lenient recursion detection

There's a reasonable scenario where at least two levels of recursion
is reasonable. Specifically if a user explicitly runs the
aws configure export-creds command with a profile that uses
a credential_process with a value of
aws configure export-creds --profile other-profile.

To handle this I've modified the env var to now track the stack of
profiles seen and ensure there's no cycles given the current profile.

I also added a final safeguard of a general recursion limit of 4.
I figured that 2 levels has valid real world scenarios, we'll give one
more level as a buffer, so fail at 4.

I don't feel super strongly about the hard limit, my rationale being
if a user wants to configure some long export-creds chain of 10 profiles,
it's their configuration so let them, but I can't come up with a valid
scenario where you'd want more than 4. However, I'd be cool with
removing the hard limit if we don't think it's necessary.

[benkehoe]

A couple of comments:

As mentioned by @kyleknap over on the issue, --env should output the basic KEY=VALUE env format, and people can use export $(aws configure export-creds --format env) to export them. A --env-export could be provided for convenience.

The CLI tends towards verbosity, aws configure export-credentials feels more consistent than export-creds.

I think having --format json might be more obvious for people looking for JSON, maybe it could be an option in addition to --format process even though they act the same.

I don't fully understand the recursive profile detection; is it specific to this command or should it be in the credential resolver or something instead?

[kyleknap]

Building on this comment:

I think having --format json might be more obvious for people looking for JSON, maybe it could be an option in addition to --format process even though they act the same.

I'm wondering if it makes sense to just have a --format default that outputs the credentials as if they were a normal CLI output that can be mutated by the --output and --query parameters. So if you were to run this command with all of the defaults, it would be a JSON payload:

aws configure export-creds
{
    "AccessKeyId": "access_key",
    "SecretAccessKey": "secret_key",
    "SessionToken": "token",
    "Expiration": "2023-01-01T00:00:00Z"
}

but then you can then mutate this response to be YAML/text and use --query if you want to pull out a specific value. Then for the --format arg we would just ignore applying --output and --query if the non-default --format value was provided. It seems that could be useful and make the command more reusable.

[benkehoe]

I'm wondering if it makes sense to just have a --format default that outputs the credentials as if they were a normal CLI output

I think that makes sense.

[benkehoe]

I called this AWS_CREDENTIALS_EXPIRATION with an S in aws-export-credentials, could we make it the same here? https://github.com/benkehoe/aws-export-credentials/blob/0c53fa6afd4123d7a28550919cea85163a067605/aws_export_credentials/aws_export_credentials.py#L228

[jamesls]

I was basing it off of what's already supported in botocore (boto/botocore#1187). Looks like it's also supported in javascript.

[benkehoe]

Oh dang, I missed that when I was implementing it! I'll change aws-export-credentials to conform to it, then.

[benkehoe]

Might be worth documenting in here as well: https://docs.aws.amazon.com/cli/latest/userguide/cli-configure-envvars.html

[benkehoe]

Is there a commensurate aws_credential_expiration value allowed in the profile config? If not, should there be?

[kyleknap]

@benkehoe Thanks for pointing out that the environment variable is not documented there. We will follow up on this to get it documented.

For the a profile config version of AWS_CREDENTIAL_EXPIRATION, the CLI/botocore does not have one. I don't think the other SDKs have one either. It could potentially make sense. We have had internal discussions about it before I believe and I think the biggest concern is around potentially reloading the shared config file and handling any edge cases in reloading the shared config file.

If this is something that you'd like to see, I'd say recommend opening a feature request in the shared SDK GitHub repository to get a discussion going around it: https://github.com/aws/aws-sdk

[jamesls]

I don't fully understand the recursive profile detection; is it specific to this command or should it be in the credential resolver or something instead?

It happens if you're using the AWS CLI as a credential process (this command) and also running AWS CLI commands. In the base case you could have just this in your ~/.aws/config file:

[default]
credential_process = aws configure export-credentials

So running any CLI command, e.g. aws s3 ls, would result in asking botocore to resolve credentials, in which it invokes aws configure export-credentials, which in turn asks botocore to resolve credentials, which then invokes aws configure export-credentials, etc. until you run out of system resources to spawn more processes.

I initially had it so you could never have the export-credentials call itself, but there is a valid case if you have the CLI configured as a credential process while also specifically running the aws configure export-credentials command, you just can't have any cycles in terms of profiles visited. There's also a cap to the number of recursive calls we'll allow. It does mean that there are certain cases like the config below that won't work, but it doesn't seem like a realistic scenario we'd want to encourage in the first place.

Running: aws configure export-creds --profile cycle-a with the following config will error out:

[profile cycle-base]
aws_access_key_id = access_key
aws_secret_access_key = secret_key

[profile cycle-a]
credential_process = aws configure export-creds --profile cycle-b

[profile cycle-b]
credential_process = aws configure export-creds --profile cycle-c

[profile cycle-c]
credential_process = aws configure export-creds --profile cycle-d

[profile cycle-d]
credential_process = aws configure export-creds --profile cycle-base

As mentioned by @kyleknap #7388 (comment), --env should output the basic KEY=VALUE env format, and people can use export $(aws configure export-creds --format env) to export them. A --env-export could be provided for convenience.

I can see supporting both, the only question I have is if it's env and env-export or env and env-no-export (or whatever name). Things to consider:

1. Matching the expectations in the various consoles that give you temp creds (sso, etc.) their default env vars are exported.
2. We're also adding windows formats such as powershell, and $Env:AWS_ACCESS_KEY_ID behaves like the export AWS_ACCESS_KEY_ID variant in bash so we might consider consistent behavior/naming across OSes.

The CLI tends towards verbosity, aws configure export-credentials feels more consistent than export-creds.

Yeah makes sense, I'll update.

[benkehoe]

I can see supporting both, the only question I have is if it's env and env-export or env and env-no-export (or whatever name).

Could go explicit for both, something like env-raw and env-export

[kyleknap]

Looks good! Just left some comments. I also tried it out in powershell and Windows CMD and it was working well there too with regards to consuming their respective formats.

[kyleknap]

We should also probably clarify that the global arguments --output and --query do not have an effect on the format to avoid confusion.

[kyleknap]

For this case, I'm wondering if we should match with some of the more granular return codes that we defined for v2: https://awscli.amazonaws.com/v2/documentation/api/latest/topic/return-codes.html. Specifically, this seems like it should be a 253 for configuration error, which we can propagate by raising a ConfigurationError.

For the other error cases, they seem like 253 configuration errors as well based on the documentation? I don't think I have a strong opinion on the ones that are returning 1, but I'm not sure if we should be using 2 as a return code as it is only isolated to S3 right now.

[jamesls]

Ended up using 253 for both. If you have no credentials configured at all and try to run a CLI command you get 253, so it made sense to be consistent that if get_credentials() returns None, we have the same RC with this command.

[kyleknap]

With regards to the error message, I'm wondering if we should include a note that the profile recursion cannot be deeper than X maximum? I'm mainly suggesting it as the error message is misleading if you do happen to have a non-cyclical credential process chain that is greater than or equal to four levels of recursion.

[jamesls]

Yeah ended up having separate error messages for each case, now you get:

$ aws configure export-credentials --profile cycle-a

Unable to retrieve credentials: Error when retrieving credentials from custom-process:
Unable to retrieve credentials: Error when retrieving credentials from custom-process:
Unable to retrieve credentials: Error when retrieving credentials from custom-process:
Unable to retrieve credentials: Error when retrieving credentials from custom-process:
Maximum recursive credential process resolution reached (4).
Profiles seen: cycle-a -> cycle-b -> cycle-c -> cycle-d

[kyleknap]

Could we limit the starting environment variable value to just one profile to start? Mainly given the maximum recursion depth is four, it seems ambiguous on whether it is failing due to encountering a previously visited profile or we are hitting the recursion maximum (if we assume that we are not properly escaping commas) especially since the error message does not discriminate between the two different cycle detection cases.

[jamesls]

I ended up having separate error messages for each case so I updated the test to check that this is because of a cycle and not max recursion. The cycle error message now looks like this:

$ aws configure export-credentials --profile cycle-a

Unable to retrieve credentials: Error when retrieving credentials from custom-process:
Unable to retrieve credentials: Error when retrieving credentials from custom-process:
Unable to retrieve credentials: Error when retrieving credentials from custom-process:
Credential process resolution detected an infinite loop, profile cycle: cycle-a -> cycle-b -> cycle-c -> cycle-a

[jamesls]

@kyleknap I believe I got everything, ready for another look.

[kyleknap]

Looks good! 🚢

[kyleknap]

For reference related to this comment: #7398 (comment), I talked to @jamesls about it. We decided to pass on implementing this format for the initial implementation for the following reasons:

1. It adds complexity to the command in that there are three different args to control output for the command (e.g. --format, --output, and --query) and the application of some of the args is dependent on the format value.
2. The default format is already in a JSON format that can be parsed out by other tools (e.g. jq, jp).
3. We can always add the format in the future in a backwards compatible way.