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.

Sign up for GitHub

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

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from
Draft

Improve tty prompt error for keypair generation #688

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 9 additions & 0 deletions command/crypto/keypair.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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"
Expand Down Expand Up @@ -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)
}
}
Comment on lines +189 to +194
Copy link
Collaborator

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.

Copy link
Member Author

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.

Copy link
Collaborator

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

Copy link
Member Author

@hslatman hslatman Jun 16, 2022 •
edited
Loading

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.

if err != nil {
return errors.Wrap(err, "error reading password")
}
Expand Down