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

Add support for process-based credential providers #1316

Merged
merged 1 commit into from
Nov 22, 2017

Conversation

JordonPhillips
Copy link
Contributor

@JordonPhillips JordonPhillips commented Nov 21, 2017

This allows users to configure an external process to provide
credentials using the credential_process config variable.

Note that this is built on top of the assume role provider since
there is rebasing necessary.

@codecov-io
Copy link

codecov-io commented Nov 21, 2017

Codecov Report

Merging #1316 into develop will increase coverage by <.01%.
The diff coverage is 100%.

Impacted file tree graph

@@             Coverage Diff             @@
##           develop    #1316      +/-   ##
===========================================
+ Coverage     98.1%   98.11%   +<.01%     
===========================================
  Files           46       46              
  Lines         7608     7644      +36     
===========================================
+ Hits          7464     7500      +36     
  Misses         144      144
Impacted Files Coverage Δ
botocore/credentials.py 98.31% <100%> (+0.09%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e32e818...c67182f. Read the comment docs.

Copy link
Contributor

@kyleknap kyleknap left a comment

Choose a reason for hiding this comment

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

Nice! 🚢

@@ -474,8 +705,79 @@ def _extract_creds_from_mapping(self, mapping, *key_names):
return found


class ProcessProvider(CredentialProvider):
Copy link
Member

Choose a reason for hiding this comment

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

We should probably work to get a canonical name added for this provider (though I don't think it should block the merge).

This allows credentials to be sourced by running an external
process.
@@ -82,6 +84,8 @@ def create_credential_resolver(session):
creds_filename=credential_file,
profile_name=profile_name
),
ProcessProvider(profile_name=profile_name,

Choose a reason for hiding this comment

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

Can the ProcessProvider take precedence over the SharedCredentialProvider or would this cause issues?

I can submit a PR with the change if there's no obvious reasons not to do it. It'll allow me to re-use a saml provider CLI tool more efficiently.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Existing providers always take precedence over new providers (except for EC2 metadata, which is always last). This is to make sure that people's config files continue to work the same as they had before if they don't change them.

Regardless, all you will need to do to make sure you're using this provider is to make a new profile where it's the only one set and use that profile explicitly.

Copy link

@stell-aelsabbahy stell-aelsabbahy Dec 1, 2017

Choose a reason for hiding this comment

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

Guess where it feels a bit inconsistent to me is source_profile/role_arn win over hardcoded credentials:

[profile1]
source_profile = rd
role_arn       = arn:blah:blah
# These will be ignored and it will use assumeRole provider
aws_access_key_id = ...
aws_secret_access_key = ...
aws_session_token = ...

But when using the credential_process it's the other way around, the hardcoded credentials win over the config option.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The ability to have hard-coded credentials in a profile along with static credentials was never intended behavior. We won't break that (because people rely on it now), but we also don't want to continue doing that for other providers.

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.

5 participants