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

Update credential precedence to match AWS CLI #621

Merged
merged 24 commits into from
May 11, 2023
Merged

Update credential precedence to match AWS CLI #621

merged 24 commits into from
May 11, 2023

Conversation

omus
Copy link
Member

@omus omus commented May 10, 2023

I noticed there were some credential precedence ordering differences between AWS.jl and AWS CLI. I ended up doing some experimentation with pairing different AWS CLI settings to determine the precedence ordering used by AWS CLI. Here are the results of those tests:

  • aws --profile used over env AWS_ACCESS_KEY_ID/AWS_SECRET_ACCESS_KEY
  • aws --profile used over env AWS_PROFILE
  • env AWS_ACCESS_KEY_ID/AWS_SECRET_ACCESS_KEY used over env AWS_PROFILE
  • env AWS_ACCESS_KEY_ID/AWS_SECRET_ACCESS_KEY used over config file sso_*
  • config file sso_* used over ~/.aws/credentials (if exists)
  • ~/.aws/credentials (if exists) used over config file credential_process
  • config file credential_process used over config file aws_access_key_id/aws_secret_access_key
  • config file aws_access_key_id/aws_secret_access_key used over EC2 instance metadata
  • config file aws_access_key_id/aws_secret_access_key used over AWS_CONTAINER_CREDENTIALS_FULL_URI

Using aws-cli/2.11.13 Python/3.11.3 Darwin/22.4.0 source/arm64 prompt/off

Notes:

  • Defining sso_account_id or sso_role_name in a profile without other sso_* keys results in an error about missing required configuration. Defining sso_start_url and sso_region by themselves doesn't produce this error.
  • Specifying the AWS credential file with AWS_SHARED_CREDENTIALS_FILE just replaces ~/.aws/credentials
  • Tested this by specifying bad credentials in one source and valid ones in the other. As I didn't have an SSO setup to test against I could only force these to fail.
  • Some additional testing was done to verify that the credential preference ordering is linear. I didn't find any examples of non-linear ordering.

Comment on lines 438 to 440
elseif !isnothing(sso_start_url)
access_key, secret_key, token, expiry = _aws_get_sso_credential_details(p, ini)
return AWSCredentials(access_key, secret_key, token; expiry=expiry)
Copy link
Member Author

@omus omus May 10, 2023

Choose a reason for hiding this comment

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

Potential breakage if someone was using dot_aws_config directly to get credentials for SSO support. If this is a concern I can leave this logic in place.

Copy link
Member

Choose a reason for hiding this comment

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

I think it's probably safe to remove, but I feel like we should keep it in place. Having the rug pulled under you would be quite annoying.

Copy link
Member Author

Choose a reason for hiding this comment

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

I think it's probably safe to remove, but I feel like we should keep it in place. Having the rug pulled under you would be quite annoying.
#621 (comment)

Copy link
Member Author

Choose a reason for hiding this comment

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

I'll add this back in with either a deprecation or just a comment

Comment on lines +424 to +425
if isfile(config_file)
ini = read(Inifile(), config_file)
Copy link
Member Author

Choose a reason for hiding this comment

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

We may want to refactor this credential code in the future to avoid reloading the config_file on each call. As this function and others are called in _aws_get_role we could see some improved performance with this change.

Comment on lines -551 to -566
# The AWS CLI uses the config file `credential_process` setting over
# specifying the config file `aws_access_key_id`/`aws_secret_access_key`.
@testset "precedence" begin
open(config_file, "w") do io
write(
io,
"""
[profile $(test_values["Test-Config-Profile"])]
aws_access_key_id = invalid
aws_secret_access_key = invalid
credential_process = $(abspath(credential_process_file))
""",
)
end

result = dot_aws_config(test_values["Test-Config-Profile"])
Copy link
Member Author

Choose a reason for hiding this comment

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

Diff is pretty messy but this was refactored and moved into the "Credential Precedence" testset

@omus omus force-pushed the cv/precedence branch from 27e8b99 to e13fca8 Compare May 10, 2023 15:43
@omus
Copy link
Member Author

omus commented May 10, 2023

bors try

bors bot added a commit that referenced this pull request May 10, 2023
@omus omus self-assigned this May 10, 2023
@bors
Copy link
Contributor

bors bot commented May 10, 2023

try

Build failed:

test/AWSCredentials.jl Outdated Show resolved Hide resolved
test/AWSCredentials.jl Outdated Show resolved Hide resolved
test/AWSCredentials.jl Outdated Show resolved Hide resolved
@omus omus force-pushed the cv/precedence branch from 018f312 to 9483fb8 Compare May 10, 2023 18:34
@omus
Copy link
Member Author

omus commented May 10, 2023

bors try

bors bot added a commit that referenced this pull request May 10, 2023
@bors
Copy link
Contributor

bors bot commented May 10, 2023

try

Build failed:

@omus
Copy link
Member Author

omus commented May 10, 2023

bors try

bors bot added a commit that referenced this pull request May 10, 2023
@bors
Copy link
Contributor

bors bot commented May 10, 2023

try

Build succeeded!

The publicly hosted instance of bors-ng is deprecated and will go away soon.

If you want to self-host your own instance, instructions are here.
For more help, visit the forum.

If you want to switch to GitHub's built-in merge queue, visit their help page.

@omus omus requested a review from mattBrzezinski May 10, 2023 19:08
src/AWSCredentials.jl Show resolved Hide resolved
src/AWSCredentials.jl Show resolved Hide resolved
src/AWSCredentials.jl Show resolved Hide resolved
Comment on lines 438 to 440
elseif !isnothing(sso_start_url)
access_key, secret_key, token, expiry = _aws_get_sso_credential_details(p, ini)
return AWSCredentials(access_key, secret_key, token; expiry=expiry)
Copy link
Member

Choose a reason for hiding this comment

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

I think it's probably safe to remove, but I feel like we should keep it in place. Having the rug pulled under you would be quite annoying.

@omus
Copy link
Member Author

omus commented May 11, 2023

bors try

bors bot added a commit that referenced this pull request May 11, 2023
@bors
Copy link
Contributor

bors bot commented May 11, 2023

try

Build failed:

@omus
Copy link
Member Author

omus commented May 11, 2023

bors try

bors bot added a commit that referenced this pull request May 11, 2023
@bors
Copy link
Contributor

bors bot commented May 11, 2023

try

Build failed:

@omus omus force-pushed the cv/precedence branch 2 times, most recently from bcb9319 to 380f7ba Compare May 11, 2023 14:16
@omus omus force-pushed the cv/precedence branch from 380f7ba to 996821f Compare May 11, 2023 14:52
@omus
Copy link
Member Author

omus commented May 11, 2023

bors try

bors bot added a commit that referenced this pull request May 11, 2023
@bors
Copy link
Contributor

bors bot commented May 11, 2023

try

Build succeeded!

The publicly hosted instance of bors-ng is deprecated and will go away soon.

If you want to self-host your own instance, instructions are here.
For more help, visit the forum.

If you want to switch to GitHub's built-in merge queue, visit their help page.

@omus
Copy link
Member Author

omus commented May 11, 2023

bors try

bors bot added a commit that referenced this pull request May 11, 2023
@omus
Copy link
Member Author

omus commented May 11, 2023

Alright, should be done with making any more changes to this PR. Summary of changes since the last review:

  • Update AWS.jl documentation on credential precedence
  • Document deviation of having ECS credentials over EC2 credentials
  • Clean up docstrings
  • When attempting to connect to the ECS credential provider timeout quickly (service should be provided on the localhost)
  • Restore SSO support to dot_aws_config but with a deprecation warning. In practise the deprecation will only be seen if calling dot_aws_config directly.
  • Increase precedence of web token identity credentials (matches documentation) and add precedence test

@bors
Copy link
Contributor

bors bot commented May 11, 2023

try

Build succeeded!

The publicly hosted instance of bors-ng is deprecated and will go away soon.

If you want to self-host your own instance, instructions are here.
For more help, visit the forum.

If you want to switch to GitHub's built-in merge queue, visit their help page.

@omus
Copy link
Member Author

omus commented May 11, 2023

bors r+

@bors
Copy link
Contributor

bors bot commented May 11, 2023

Build succeeded!

The publicly hosted instance of bors-ng is deprecated and will go away soon.

If you want to self-host your own instance, instructions are here.
For more help, visit the forum.

If you want to switch to GitHub's built-in merge queue, visit their help page.

@bors bors bot merged commit 9a4322a into master May 11, 2023
@bors bors bot deleted the cv/precedence branch May 11, 2023 18:55
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.

2 participants