-
Notifications
You must be signed in to change notification settings - Fork 245
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
Improve tty prompt error for keypair generation #688
base: master
Are you sure you want to change the base?
Conversation
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
@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. |
Just a quick thought... The conditional |
Of the proposed solutions, my favorite would be the one that requires refactoring to require context.Context (or some kind of a 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 |
@dopey I'm with you on the The proposed stop gap is effectively what's already in place. The current error handling code uses @tashian absolutely! Something like |
var pe *os.PathError | ||
if errors.As(err, &pe) { | ||
if pe.Op == "open" && pe.Path == "/dev/tty" { | ||
return fmt.Errorf("could not read password to encrypt the private key: %w", err) | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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.
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 errorcan 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 thatadditional 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
. Thealternative 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
anderrors.Is
will still work with that approach.The current version of
urfave/cli
doesn't play super nice withcontext.Context
; v2 seems to do a better job at that, but requiresa 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 anerror. 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