Skip to content

Fix hang on invisible tty when using 2FA where prompt=terminal in some edge cases #1184

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

Merged
merged 2 commits into from
Mar 10, 2023

Conversation

onnos
Copy link

@onnos onnos commented Mar 9, 2023

This implements the fix suggested by @ngyuki in #1054. When using prompt=terminal (which we use to support both Linux and Mac using the same workflow and config, without osascript involvement), we used to have a workaround in .aws/config that would redirect the terminal prompt to stdout in most cases by adding 2> $(tty) to the end of the aws-vault credential_process call, like this:

credential_process=sh -c 'aws-vault --prompt terminal export base --duration 12h --format=json 2> $(tty)'

With this patch we can remove that redirect and the prompt always shows up, even when called through the SDK via aws --profile somerole s3 ls, for example. Also removes the creation of spurious files named not a tty when prompt=terminal is forced.

This expands on #1149 a bit, where instead of just preferring a non-terminal prompt without an active tty it should show up in all cases anyway when specifically requested.

@mtibben
Copy link
Member

mtibben commented Mar 10, 2023

Are there are any weird edge cases to this? Is it possible to have a terminal without a TTY these days? Does this work on Windows and other OSs?

@onnos
Copy link
Author

onnos commented Mar 10, 2023

Are there are any weird edge cases to this?

I am not able to find any, in fact this only fixes an edge case. This particular path is only prompted when prompt=terminal is forced by greybeards like myself who do not want to leave the confines of the terminal to go through 2FA. In the normal flow the "workaround" of popup GUI prompts are used. While the GUI popups are good UI for many people, I'd argue that allowing a full-terminal flow when specifically requested is a nice thing to do. With v7, this was made possible by the changes in #1149, however one edge case still remains - specifically when credential_process is called in the background and prompt-terminal is specified, the aws-vault process has no access to any foreground ttys and still swallows the prompt.

Specifically:
with ~.aws/config having the following sample configuration:

[profile base]
mfa_serial=arn:aws:iam::121212121212:mfa/username

[profile base-session]
credential_process=aws-vault7 --prompt terminal export base --duration 12h --format=json

[profile role]
role_arn=arn:aws:iam::242424242424:role/cross-account-role
source_profile=base-session

Running aws s3 ls --profile role will appear to hang. When you hit enter, you will get Error when retrieving credentials from custom-process: because the prompt was invisible. The version with this patch will show the prompt. Any other flow remains untouched.

Is it possible to have a terminal without a TTY these days?

I don't think so? I might be misunderstanding the question, but to my belief anything without a TTY cannot be considered a terminal. TTY's are essential for I/O and signalling. As The TTY demystified says though, "the TTY subsystem — while quite functional from a user's point of view — is a twisty little mess of special cases."

Does this work on Windows and other OSs?

I don't have a Windows machine to test on. That said, this works on Macos and Linux machines and I can't imagine it not working in any terminal emulator. Again though, this only targets one particular flow where prompt=terminal is forced - when that is not enabled the normal prompt flows will still trigger and work as they do currently.

I've rebased my branch and fixed the linter error that I saw show up in the Github Actions. Thanks for your quick review so far!

@mtibben mtibben merged commit 243eefc into 99designs:master Mar 10, 2023
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