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

fix(python): allowing PIN/passphrase input for Git Bash #1959

Merged
merged 1 commit into from Dec 1, 2021

Conversation

grdddj
Copy link
Contributor

@grdddj grdddj commented Nov 30, 2021

Trying to solve #1737

Allowing the input for PIN/passphrase on Windows in Git Bash, which has issues with hidden input.

Recognizing the situation (through sys.stdin.isatty()) and in negative case disallowing the hidden feature and reporting this to the user.

Screenshot from Git Bash on Windows:

image

@grdddj grdddj linked an issue Nov 30, 2021 that may be closed by this pull request
@prusnak
Copy link
Member

prusnak commented Nov 30, 2021

It seems to me that click.prompt uses getpass.getpass to obtain the input when hide_input is set.

According to the documentation getpass.getpass should be printing a warning (via GetPassWarning) if echo free input is not possible. Is that not the case for us?

@grdddj
Copy link
Contributor Author

grdddj commented Nov 30, 2021

getpass.getpass should be printing a warning if echo free input is not possible. Is that not the case for us?

It was not the case for me, neither for the reporter of the issue - I have replicated the same behavior. There was no warning, and the script literally froze and had to be killed.

@prusnak
Copy link
Member

prusnak commented Nov 30, 2021

Sorry, I now see the upstream Python bug mentioned earlier: https://bugs.python.org/issue44762

python/src/trezorlib/ui.py Outdated Show resolved Hide resolved
Copy link
Contributor

@matejcik matejcik left a comment

Choose a reason for hiding this comment

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

additional comments:

Consider looking at CAN_HANDLE_HIDDEN_INPUT in get_passphrase and in that case, only ask once. No point in retyping to confirm if the user can see typos on screen.

This solves the issue for Git bash, but I'm not sure that it solves it for usage in scripts. I was considering a separate ScriptUI implementation:

  • uses stdin instead of stderr (so click.echo and click.prompt without the err override)
  • never uses hidden inputs
  • prints prompts on single line, so instead of:
    prompt("Enter PIN")
    
    it would show
    click.echo("Enter PIN")
    pin = click.prompt()
    
  • you can select ScriptUI by something like trezorctl --script
  • it does not show PIN help text
  • it prints out button request code (and in the future also name/index), PIN request type
  • instead of auto-returning PASSPHRASE_ON_DEVICE, it asks you
    • (and of course only asks for passphrase once)
  • does not use the envvar - the script is responsible for providing it.
  • shows you something like "enter pin: code=None, max_length=50"
    • and then returns whatever you entered directly, the script is responsible for validating

In essence, this mode would turn trezorctl into an interactive tool that allows you to integrate Trezor into, say, Java, without writing a Java library.

python/src/trezorlib/ui.py Outdated Show resolved Hide resolved
python/src/trezorlib/ui.py Outdated Show resolved Hide resolved
python/src/trezorlib/ui.py Outdated Show resolved Hide resolved
python/src/trezorlib/ui.py Outdated Show resolved Hide resolved
@grdddj
Copy link
Contributor Author

grdddj commented Nov 30, 2021

Thanks for the suggestions. After c2e5955 it looks like:

image

The creation of ScriptUI sounds interesting - should we create a separate issue for it?

@matejcik
Copy link
Contributor

matejcik commented Dec 1, 2021

The creation of ScriptUI sounds interesting - should we create a separate issue for it?

I consider #1737 to be it -- if you read the issue description, it concerns script use, the git bash thing is a sub-issue.
We can accept this PR as a partial fix.

Copy link
Contributor

@matejcik matejcik left a comment

Choose a reason for hiding this comment

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

lgtm, please squash&rebase

@matejcik
Copy link
Contributor

matejcik commented Dec 1, 2021

also please add a changelog entry: "Fix PIN and passphrase entry in certain terminals on Windows"

@grdddj grdddj force-pushed the grdddj/pin_input_in_git_bash branch 3 times, most recently from 0927626 to 027f875 Compare December 1, 2021 10:28
@grdddj grdddj force-pushed the grdddj/pin_input_in_git_bash branch from 027f875 to 67bd642 Compare December 1, 2021 10:47
@grdddj
Copy link
Contributor Author

grdddj commented Dec 1, 2021

Sorry for a lot of force-pushes, I had issues with Windows line-endings and formatting

@matejcik matejcik merged commit d6b99ba into master Dec 1, 2021
@matejcik matejcik deleted the grdddj/pin_input_in_git_bash branch December 1, 2021 12:06
@grdddj grdddj mentioned this pull request Dec 16, 2021
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.

PIN/Passphrase input for trezorctl doesn't work in some environments
3 participants