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

Improve tests for load_aws_config #123

Open
iainelder opened this issue Jul 3, 2021 · 3 comments
Open

Improve tests for load_aws_config #123

iainelder opened this issue Jul 3, 2021 · 3 comments

Comments

@iainelder
Copy link
Contributor

The load_aws_config function is responsible for getting credentials that are used to sign the request.

It's quite complex: a mixture of custom credential parsing and calls to botocore.

The PR #116 (add support for AWS SSO profiles) caused a rergression in #122 (instance credentials are not being loaded).

The tests for the load_aws_config function don't cover all the use cases, which makes it hard to add new functionality without breaking what already works.

@okigan
Copy link
Owner

okigan commented Jul 3, 2021

i am tempted to delegate to boto to load configs (since the premise of not depending on boto is not as relevant today)

@iainelder
Copy link
Contributor Author

(Although botocore implements the credential handling, the relevant documentation describes boto3 and the AWS CLI. I'll keep referring to botocore.)

botocore has a long credential resolution chain to support all the use cases:

Boto3 will look in several locations when searching for credentials. The mechanism in which Boto3 looks for credentials is to search through a list of possible locations and stop as soon as it finds credentials. The order in which Boto3 searches for credentials is:

  1. Passing credentials as parameters in the boto.client() method
  2. Passing credentials as parameters when creating a Session object
  3. Environment variables
  4. Shared credential file (~/.aws/credentials)
  5. AWS config file (~/.aws/config)
  6. Assume Role provider
  7. Boto2 config file (/etc/boto.cfg and ~/.boto)
  8. Instance metadata service on an Amazon EC2 instance that has an IAM role configured.

(The AWS SSO credentials are not explicitly mentioned here. I'm not sure if it should be an extra step or if it's also handed by step 6. Either way it's not mentioned in that documentation!)

awscurl by itself implements steps 1 through 4. It handles step 4 more strictly than botocore because it doesn't check the AWS_SHARED_CREDENTIALS_FILE environment variable.

The botocore fallback introduced in #63 adds support for steps 5 through 8, but there are behavior differences because of how awscurl doesn't pass all the arguments to botocore. One of these differences is mentioned below.


It seems to me people are expecting that the authentication interface for awscurl be similar to that of the aws CLI.

So using botocore to load credentials makes sense to me because that's what the AWS CLI uses. It would surely solve the problems people report in #44.

as assume role just replaces env variables to the ones corresponding to the role have you considered just running aws sts assume-role ... before running awscurl?

It's okay as a workaround. I can use aws2-wrap to generate AWS SSO credentials in a format that awscurl understands. But it would be excellent to let botocore handle it automatically.

[Assume role provider] works with env variables [...] but doesn't work with CLI arg

When the profile is set as an environment variable, both awscurl and botocore can read it, so in the botocore fallback can handle step 6.

When the profile is passed as an argument to awscurl, it's not passed to the botocore fallback. (I tried to change that in my PR #116 but it clashed with the hard-coded "default" value (#112) so it got rolled back (#124)).


Another part of this is the user interface. Should the current CLI parsing need to change to depend on botocore?

I think the hard-coded "default" for profile name is unneccesary if botocore is used, and in fact may be problematic because it stops the credential resolution chain before it reaches step 8.

I would discourage people from passing the credentials as arguments because credential leakage is almost guaranteed by default settings (shell command history, CI logs, etc.). So I would be in favor of removing the option access_key, secret_key, security_token, and session_token.


What do you think?

@iainelder
Copy link
Contributor Author

I've asked for clarification of the SSO credentials resolution in boto/botocore#2433.

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

No branches or pull requests

2 participants