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

credential_process should connect subprocess stderr to caller stderr #1348

Open
bwalding opened this issue Dec 23, 2017 · 16 comments
Open

credential_process should connect subprocess stderr to caller stderr #1348

bwalding opened this issue Dec 23, 2017 · 16 comments
Labels
cross-sdk enhancement This issue requests an improvement to a current feature. feature-request This issue requests a feature. p2 This is a standard priority issue

Comments

@bwalding
Copy link

When the botocore is configured to use a credential_process to acquire AWS credentials, the sub-process stderr is never sent to the user.

(In our case, the stderr is prompting for the MFA details to be entered)

stdin is mapped correctly - and if I know that I'm waiting on MFA I can enter it and the process completes successfully.

What I'd hope for - stderr is passed from the credential_process to the stderr on the aws CLI process.

bwalding added a commit to bwalding/botocore that referenced this issue Dec 23, 2017
@JordonPhillips JordonPhillips added enhancement This issue requests an improvement to a current feature. needs-discussion labels Dec 26, 2017
@JordonPhillips
Copy link
Contributor

Off the top of my head I can't think of anything that this behavior change would break. I was initially unsure about using stderr as a prompting source, but when stdout is being used entirely then there's not much other choice. The library we use in our sample implementation uses getpass, which has stderr as a fallback.

Incidentally, getpass is a possible workaround since it tries to write/read from the tty directly first. Most cases that should work, though there is the potential downside of a non-echoing input.

@JordonPhillips JordonPhillips self-assigned this Dec 26, 2017
@pcolmer
Copy link

pcolmer commented Jan 9, 2018

Part of the drawback to using getpass to handle this requirement is where the credential process needs to output something more complicated than a single prompt.

For example, in the code I've submitted to enhance awsprocesscreds to support MFA under Okta, the code lists out all of the MFA options available to the user. That could be built up as a single string prompt but it isn't nice for the user to then parse it visually.

@pcolmer
Copy link

pcolmer commented Jan 9, 2018

There is also the scenario where the credential process needs to provide the user with information about the progress of the authentication process. For example, if the user chooses a push notification operation, my code currently emits some text to let the user know that the code is waiting for the result of the push notification. Granted, I could remove that but I think it is more user-friendly if the user is kept informed about what is happening ...

@ghost
Copy link

ghost commented May 12, 2019

This thread has been stale for a while but it would be nice to know if this is gaining any traction. We have federated logins for AWS and a company policy that requires MFA at login. This means that we need more interaction with the user than just prompting for a password. Not being able to use stdout for anything but the JSON payload, and not being able to use stderr at all limits what you can do with this integration.

@edulop91
Copy link

FWIW AWS libraries in other languages already do this:
https://github.com/aws/aws-sdk-go/blob/master/aws/credentials/processcreds/provider.go#L347

@ryan-gerstenkorn-sp
Copy link

ryan-gerstenkorn-sp commented Jan 27, 2020

Wanted to link this PR here as well as add that this would be useful for aws-vault as it prompts to decrypt the credentials when accessed.

#1835

@ryan-gerstenkorn-sp
Copy link

ryan-gerstenkorn-sp commented Jan 28, 2020

Hi all, wanted to post a workaround that can be done through the ~/.aws/config fairly easily. This should work with any executable, not just aws-vault.

[profile develop]
credential_process = sh -c 'aws-vault exec develop --json 2> /dev/tty'
region = us-east-1

I'm not sure why 'sh -c' is required here. I would think it should work without another subprocess but that doesn't seem to be the case.

@hryamzik
Copy link

@ryan-gerstenkorn-sp sh is required as otherwise 2> and /dev/tty are just another args for aws-vault

@kdaily kdaily added feature-request This issue requests a feature. needs-review This issue or pull request needs review from a core team member. and removed needs-discussion labels Aug 25, 2021
@jonathanmorley
Copy link

cmd /C "foo 2>CON" sounds like it would be an analagous construct for windows, but it does not appear to support overwriting the output (I use this to create an interactive prompt for users to choose their MFA method)

@benkehoe
Copy link

What I really want, though this is a tall order, is for the credential process spec to allow for interactivity. Let me send something like this to stdout:

{
  "Version": "2",
  "Prompt": "Enter password:",
  "NoEcho": true
}

and have the SDK responsible for prompting the user, and sending that to stdin, after which I send another JSON document containing credentials.

@jonathanmorley
Copy link

Why is that preferable to attaching stderr?

@kdaily
Copy link
Member

kdaily commented Oct 13, 2021

Overall, this seems like a sensible feature. I'm researching how our other SDKs handle this to provide a consistent experience - as previously noted, the Go SDK does not capture stderr.

@benkehoe, agree on the "tall order". It would be pretty powerful, but would definitely require a major overhaul to differentiate between events that are the credential JSON payloads and other messages.

@kdaily kdaily assigned kdaily and unassigned JordonPhillips Oct 13, 2021
@kdaily kdaily removed the needs-review This issue or pull request needs review from a core team member. label Oct 13, 2021
@benkehoe
Copy link

Why is that preferable to attaching stderr?
@jonathanmorley because then the SDK could provide hooks into the interaction, so an application could potentially include the authentication as part of its own interface (e.g., a GUI program)

agree on the "tall order"
@kdaily for sure, just Thinking Big and providing a theoretical ideal to work backward from when we're working out how to address the problem practically

@jonathanmorley
Copy link

jonathanmorley commented Feb 4, 2022

Would there be a downside to supporting both? Both being:

  1. stderr passthrough
  2. Interactivity via something like the Prompt JSON output to stdout proposed by @benkehoe above

The stderr approach seems like a much smaller ask, while still allowing for basic (non-gui) interactivity

@Nevon
Copy link

Nevon commented Sep 27, 2022

There are two open pull requests addressing this issue since 3 years now - #2091 and #1835. At the moment it's not possible to use credential_process together with the aws-cli if you are using MFA, since it uses botocore (see aws/aws-cli#3057).

Who has the power to move this forward or is there anything preventing this from being fixed?

@RyanFitzSimmonsAK
Copy link
Contributor

This issue is being tracked in the cross-SDK repository now, since the change would need to be implemented across SDKs to maintain consistency. I'm going to close this issue; if you want to further indicate your support for this issue, please do so in the issue in the other repository.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cross-sdk enhancement This issue requests an improvement to a current feature. feature-request This issue requests a feature. p2 This is a standard priority issue
Projects
None yet
Development

No branches or pull requests