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

bug: Don't try to handle SIGINT when prompting for passwords #1498

Merged
merged 13 commits into from
May 18, 2022

Conversation

dwahler
Copy link
Contributor

@dwahler dwahler commented May 16, 2022

This PR disables the SIGINT handler in cliui.Prompt when the Secret option is set. This handler tries to run at the same time as the one installed by speakeasy.Ask, and if it wins the race condition, it causes the program to exit before speakeasy can correctly restore the terminal settings.

We keep the signal handler when prompting for non-password inputs so that we can print a newline and give the user a clean prompt after we exit.

Fixes #1378

pty/pty.go Outdated
Comment on lines 31 to 32
// The file descriptor of the underlying PTY, if available
PTYFile() *os.File
Copy link
Member

Choose a reason for hiding this comment

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

You could have methods for input and output file instead of a single method, which would make it Windows compatible. And then for the file descriptor just have another method for that as well (the windows.Handle is a file descriptor and can be cast to uintptr)

Copy link
Member

Choose a reason for hiding this comment

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

It might be cleaner to expose the readWriter interface and add a File function to it! readWriter could be renamed to pipe, since that's pretty much what it's doing.

Comment on lines 42 to 53
termios, err := unix.IoctlGetTermios(int(ptty.PTY.PTYFile().Fd()), unix.TCGETS)
require.NoError(t, err)
require.Zero(t, termios.Lflag&unix.ECHO, "echo is on while reading password")

err = cmd.Process.Signal(os.Interrupt)
require.NoError(t, err)
_, err = cmd.Process.Wait()
require.NoError(t, err)

termios, err = unix.IoctlGetTermios(int(ptty.PTY.PTYFile().Fd()), unix.TCGETS)
require.NoError(t, err)
require.NotZero(t, termios.Lflag&unix.ECHO, "echo is off after reading password")
Copy link
Member

Choose a reason for hiding this comment

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

It would be good to see a Windows test too, shouldn't be too hard to get the console mode:

var state uint32
err := windows.GetConsoleMode(handle, &state)
if err != nil {
	return err
}

@dwahler
Copy link
Contributor Author

dwahler commented May 17, 2022

I spent way too much time today trying to get this test to run on Windows and wasn't successful, so unless someone else has the time or the deep knowledge of Win32 to make it work, I think we'll have to be satisfied with testing on Linux.

As far as I've been able to determine, the problem boils down to a core difference between Unix pty devices and Windows ConPTY. On Linux, the kernel creates "master" and "slave" pty devices as a pair, and they behave as though they're connected by a bidirectional pipe. The slave device is what keeps track of the local echo state, and although the simplest thing you would do with it is let a child process inherit it as its TTY, there's nothing stopping the parent process from also keeping a reference to it and monitoring its state.

But on Windows, there are at least three distinct sets of handles:

  1. A pair of pipes created by the parent; the parent holds onto one end of each of these, and they play roughly the same role as a Unix "master" pty
  2. A "pseudoconsole handle" which the parent passes as a parameter to CreateProcess, encapsulating the other ends of the pipes (corresponding to data read/written by the child)
  3. A "console input buffer" and "console screen buffer" handle, which the child process receives as its stdio handles

Of these, 3 seems to be the only one that supports GetConsoleMode which allows you to query the echo status. (The pipes are just streams of bytes that happen to contain terminal escape codes, and the pseudoconsole handle is an opaque reference that isn't actually usable for I/O.) But there seems to be no good way for the parent to use 1 or 2 to get a reference to 3. The child's handles are created automatically at process creation time, without the parent ever being able to see them.

see also:

https://devblogs.microsoft.com/commandline/windows-command-line-introducing-the-windows-pseudo-console-conpty/

microsoft/terminal#262 (comment)

It might be that testing this on Windows would require a totally different approach.

@kylecarbs
Copy link
Member

@dwahler I went down a similar rabbit hole with ConPTY when creating the pty package. Even our PTYs on Windows in testing aren't real PTYs, because ConPTY requires attaching to a command.

@dwahler dwahler enabled auto-merge (squash) May 18, 2022 15:10
@dwahler dwahler merged commit 5f21a14 into main May 18, 2022
@dwahler dwahler deleted the david/1378-terminal-state branch May 18, 2022 15:26
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.

Bug: CLI does not properly restore terminal state if interrupted during password input
3 participants