From e80377e19551b6c9d50ab70394f0eca076338c8f Mon Sep 17 00:00:00 2001 From: Herman Slatman Date: Mon, 13 Jun 2022 23:52:05 +0200 Subject: [PATCH] Improve tty prompt error for keypair generation 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 --- command/crypto/keypair.go | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/command/crypto/keypair.go b/command/crypto/keypair.go index 870fae2f9..e01099318 100644 --- a/command/crypto/keypair.go +++ b/command/crypto/keypair.go @@ -1,6 +1,9 @@ package crypto import ( + "fmt" + "os" + "github.com/pkg/errors" "github.com/smallstep/cli/crypto/keys" "github.com/smallstep/cli/crypto/pemutil" @@ -183,6 +186,12 @@ func createAction(ctx *cli.Context) (err error) { } else { var pass []byte pass, err = ui.PromptPassword("Please enter the password to encrypt the private key", ui.WithValue(password), ui.WithValidateNotEmpty()) + 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) + } + } if err != nil { return errors.Wrap(err, "error reading password") }