botocore icon indicating copy to clipboard operation
botocore copied to clipboard

credential_process should connect subprocess stderr to caller stderr

Open bwalding opened this issue 6 years ago • 15 comments

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 avatar Dec 23 '17 04:12 bwalding

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 avatar Dec 26 '17 23:12 JordonPhillips

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 avatar Jan 09 '18 10:01 pcolmer

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 ...

pcolmer avatar Jan 09 '18 10:01 pcolmer

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.

ghost avatar May 12 '19 14:05 ghost

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

edulop91 avatar Dec 11 '19 18:12 edulop91

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.

https://github.com/boto/botocore/pull/1835

ryan-gerstenkorn-sp avatar Jan 27 '20 20:01 ryan-gerstenkorn-sp

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.

ryan-gerstenkorn-sp avatar Jan 28 '20 15:01 ryan-gerstenkorn-sp

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

hryamzik avatar Feb 24 '20 15:02 hryamzik

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)

jonathanmorley avatar Oct 11 '21 13:10 jonathanmorley

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.

benkehoe avatar Oct 12 '21 01:10 benkehoe

Why is that preferable to attaching stderr?

jonathanmorley avatar Oct 12 '21 16:10 jonathanmorley

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 avatar Oct 13 '21 21:10 kdaily

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

benkehoe avatar Oct 13 '21 22:10 benkehoe

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

jonathanmorley avatar Feb 04 '22 16:02 jonathanmorley

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 https://github.com/aws/aws-cli/issues/3057).

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

Nevon avatar Sep 27 '22 12:09 Nevon

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.

RyanFitzSimmonsAK avatar Nov 10 '22 18:11 RyanFitzSimmonsAK