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.