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

Allow stderr passthrough for credential_process #1835

Closed

Conversation

twooster
Copy link

Many organizations now require MFA for AWS login, or perhaps fetch
credentials from a command-line password manager. While stdout of
the credential_process is used to acquire the credential token,
stderr is captured for raising an exception back to the user.

In order enable user-interaction with the credential provider process,
it must have access to some stream the user can see: in this case,
stderr. This commit provides that change.

In order to maintain a relatively high level of backwards
compatibility, we will continue to capture stderr for raising
exceptions unless stderr is reporting as being a TTY device, in
which case we assume that user-interaction is more favorable than
detailed exceptions.

Many organizations now require MFA for AWS login, or perhaps fetch
credentials from a command-line password manager. While `stdout` of
the `credential_process` is used to acquire the credential token,
`stderr` is captured for raising an exception back to the user.

In order enable user-interaction with the credential provider process,
it must have access to some stream the user can see: in this case,
`stderr`. This commit provides that change.

In order to maintain a relatively high level of backwards
compatibility, we will continue to capture `stderr` for raising
exceptions _unless_ `stderr` is reporting as being a TTY device, in
which case we assume that user-interaction is more favorable than
detailed exceptions.
@codecov-io
Copy link

codecov-io commented Sep 27, 2019

Codecov Report

Merging #1835 into develop will increase coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff            @@
##           develop    #1835   +/-   ##
========================================
  Coverage    92.51%   92.52%           
========================================
  Files           53       53           
  Lines         9958     9966    +8     
========================================
+ Hits          9213     9221    +8     
  Misses         745      745           
Impacted Files Coverage Δ
botocore/credentials.py 98.52% <100.00%> (+0.01%) ⬆️

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 75112ad...68c999a. Read the comment docs.

@twooster
Copy link
Author

Related PR, a while back: #1349

@ryan-gerstenkorn-sp
Copy link

Want to see if there is any plan's to merge this as well as mention this would be useful when using with aws-vault. Currently the unlock credentials prompt get's swallowed and it appears the process is stuck.

I spent some time trying to duplicate the stderr output so it can be raised in an error as well as passed through to the parent's stderr however something strange is going on and I was not able to get this to work properly. It seems like this is the next best option, was about to submit a PR for that before seeing this PR was already open.

@tim-finnigan
Copy link
Contributor

Thank you for creating this PR. This feature request is now being tracked here in our cross-SDK repository: aws/aws-sdk#358. Implementations involving credential processing need to be considered across SDKs, so further review is required here. I will link this PR in that issue to reference this as a possible approach.

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.

4 participants