Improve tty prompt error for keypair generation

[hslatman]

When a prompt is used, it may happen that /dev/tty is not available.
In many cases it's not clear to the user what exactly went wrong in
such a case, because that context is lost. Using STEPDEBUG=1 may
help in these cases, but the user must know about that. Arguably,
the stacktrace in the output isn't very helpful for casual users.

In this commit we opt for an explicit check when a prompt is requested
to see if the error indicates that /dev/tty is unavailable. The error
can be contextualized by the caller. This is a proposal and hasn't been
applied to all cases of a prompt potentially failing, because Joe and
I first wanted some feedback on this approach.

I think it would be nice if we could take this even further, like
including a context.Context in the call to the prompt, so that
additional information could be attached and potentially used when
creating or printing the error. Right now it's up to all callers to
handle this specific case, because only the callers know the context
of the call and what the user or system was trying to do with the
call. It's not trivial to devise a (more) elegant way to provide
this context to the password prompt without context.Context. The
alternative is to ensure we always wrap errors with the function
that called them, so that the (full) lineage of the error can be
seen. errors.As and errors.Is will still work with that approach.
The current version of urfave/cli doesn't play super nice with
context.Context; v2 seems to do a better job at that, but requires
a bigger refactor.

In this commit we don't make use of the errors.Wrap functionality,
which results in STEPDEBUG=1 not printing the stacktrace for an
error. This may be acceptable if the caller provides the context, but
we could add it back in if we want to or find a different solution.

The generalized utility function can probably be moved to
smallstep/cli-utils at a later stage.

Relates to #674

[hslatman]

@dopey: what do you think about this approach? This is what @jdoss and I came up with as a pragmatic solution. @jdoss will have a stab at extracting the logic into a utility function that can be reused and apply that to the other prompts, if this approach is OK.

We also discussed refactoring the commands to verify all inputs at the start of execution and failing early if one of the inputs wasn't set and would result in a prompt before actually invoking the application logic, preventing application logic intertwined with inputs. That would take quite some more time to refactor, but would probably be very nice to have. I think that's also one of the directions @azazeal has pointed at with the CLI refactor work.

[tashian]

Just a quick thought... The conditional pe.Op == "open" && pe.Path == "/dev/tty", which I assume checks if stdin is a tty?, might make sense to break out into a one-line function so it can have a descriptive name.

[dopey]

Of the proposed solutions, my favorite would be the one that requires refactoring to require context.Context (or some kind of a WithOption) that allowed passing through the state of the caller.

I think trying to validate against all the possible edge cases is going to get ugly and difficult to manage. For example, incorrect password isn't one where you can pre-validate.

As a stop gap (until we've designed and prioritized this work), I would propose wrapping all the errors returned by Prompt_xxx so that we can at least get the stack trace with STEDEBUG=1.

[hslatman]

@dopey I'm with you on the context.Context. It feels like a nice way to do this, but we would have to think about what info we pass in the context.Context and need to ensure it is read from the context.Context when the error is created. As an interim solution, without having to update urfave/cli to v2 and ensure the entire CLI works as expected with that, we could add a context.Context to every Prompt*** function. Or we can do what many others do and add a Prompt***Context() function. The current function could call into the one with Context with a context.Background() to reduce code duplication. At a later stage we should be able to link that up to the new urfave/cli and its context (methods).

The proposed stop gap is effectively what's already in place. The current error handling code uses errors.Wrap and so the stack trace is available using STEPDEBUG=1 (we also fall back to that behavior if it's not a TTY unavailable error). The thing we're trying to improve is the actual error message returned to the user, with or without debug info enabled, so that the potential root cause of the failure is easier to find. I think a user should not need STEPDEBUG=1 for this. We could still use errors.Wrap, but with a reworded error message that states what the prompt was required for.

@tashian absolutely! Something like IsTTYUnavailableError should do the job, I think. Trying to open /dev/tty is the surest way to see if it can be used to prompt for user input, so if that fails, we can't prompt.

[maraino]

I think if we want to add a better message should be done here:
https://github.com/smallstep/cli-utils/blob/dab2062ecd8c8176026c1fe6a60a034d69c6a87f/ui/ui.go#L279-L282

And If I'm not correct right now the error would be:

error reading password: error allocating terminal: os: blah blah blah

Perhaps the only thing that we need is to clarify the error coming to cli-utils/ui.

[hslatman]

What we would like to clarify is why the TTY was required in a specific case. For example, in this case it's for providing a password. A different use case would be providing a yes/no answer for a certain operation, like overwriting a file.

Having a slightly more informative error message would make it easier for the user to prevent it from happening (i.e. "I need to provide a password without a prompt availalble; let's use a --password-file for that"). The proposal to use context.Context would pass some information down, so that an error can be returned with the contextual info in it.

[maraino]

All or most or UI methods have and options variadic, that can easily be changed to provide custom error messages, but we can provide contest wrapping those errors here without checking for different errors, we should do that in ui if we want to:

// If we return something nicer from ui:
could not read password to encrypt the private key: cannot open /dev/tty 

// or for a different error:
could not read password to encrypt the private key: foo bar zar

[hslatman]

@azazeal pointed me to an example that I think is nice: https://github.com/superfly/flyctl/blob/ac3bf682a43ac6b0dc8586b7a13ae2a766d6893a/internal/prompt/prompt.go#L96-L106. It goes a bit deeper to check if a certain file is a TTY, and if not, this low level error is returned. It is wrapped by an error that provides a description of what the prompt was required for. Will add something like that to the ui package in cli-utils if you think this is a nice way to do it too.