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.

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

-f or --no-input option does not work with piv-tool set-pin #2296

Open
trondat opened this issue Sep 30, 2022 · 2 comments
Open

-f or --no-input option does not work with piv-tool set-pin #2296

trondat opened this issue Sep 30, 2022 · 2 comments
Assignees
Labels
bug Something isn't working good first issue Good for newcomers

Comments

@trondat
Copy link

trondat commented Sep 30, 2022

Description

When setting new pin on the PIV device the command cosign piv-tool set-pin --no-input --new-pin <new pin> --old-pin <old pin> will prompt for confirmation even though --no-input is specfied
? Setting new pin. This will overwrite the previous pin.? [y/N] █

Version

GitVersion: devel
GitCommit: unknown
GitTreeState: unknown
BuildDate: unknown
GoVersion: go1.19.1
Compiler: gc
Platform: linux/amd64

@trondat trondat added the bug Something isn't working label Sep 30, 2022
@cpanato cpanato self-assigned this Sep 30, 2022
@cpanato
Copy link
Member

cpanato commented Sep 30, 2022

thanks, i will try to reproduce and if confirms will see how to fix it

@znewman01
Copy link
Contributor

Problem is Confirm in pivcli/commands.go:

var Confirm = func(p string) bool {

The solution would be to use ConfirmPrompt instead. This would be a fast fix.

It has an added benefit of getting rid of a dependency on promptui!

While we're in there, we should replace any term.ReadPassword calls with calls to GetPassFromTerm.

Long term fix

Nail down semantics

In past discussions (see #844 #1909 #2039) it's come up that there are two distinct use cases for a --no-input-like flag:

  1. "I'm running in a script, fail fast instead of waiting for input" ("non-interactive")
  2. "shut up and do what I say" ("force")

There are also two kinds of types of prompt:

  1. simple confirmation ("FYI this information is going into a transparency log")
  2. destructive operations

The following is a proposed matrix of how they'd interact:

|              | interactive | non-interactive | force |
|--------------+-------------+-----------------+-------|
| notification | `[Yn]`      | print, proceed  | do it |
| destructive  | `[yN]`      | fail fast       | do it |

We should codify this in some documentation.

Enforce those semantics

Ban reader.ReadString and term.ReadPassword (using forbidigo in golanglint-ci) so that we always go through the Cosign libraries.

@znewman01 znewman01 added the good first issue Good for newcomers label Oct 17, 2022
znewman01 added a commit to znewman01/cosign that referenced this issue Dec 22, 2022
This library:

- Is testable.
- Is consistent across the CLI.

Related to (does not fix) sigstore#2296 and sigstore#2204.

Signed-off-by: Zachary Newman <zjn@chainguard.dev>
znewman01 added a commit to znewman01/cosign that referenced this issue Dec 22, 2022
This library:

- Is testable.
- Is consistent across the CLI.

Related to (does not fix) sigstore#2296 and sigstore#2204.

Signed-off-by: Zachary Newman <zjn@chainguard.dev>
znewman01 added a commit to znewman01/cosign that referenced this issue Dec 23, 2022
This library:

- Is testable.
- Is consistent across the CLI.

Related to (does not fix) sigstore#2296 and sigstore#2204.

Signed-off-by: Zachary Newman <zjn@chainguard.dev>
znewman01 added a commit to znewman01/cosign that referenced this issue Dec 23, 2022
This library:

- Is testable.
- Is consistent across the CLI.

Related to (does not fix) sigstore#2296 and sigstore#2204.

Signed-off-by: Zachary Newman <zjn@chainguard.dev>
znewman01 added a commit to znewman01/cosign that referenced this issue Jan 2, 2023
This library:

- Is testable.
- Is consistent across the CLI.

Related to (does not fix) sigstore#2296 and sigstore#2204.

Signed-off-by: Zachary Newman <zjn@chainguard.dev>
znewman01 added a commit that referenced this issue Jan 2, 2023
This library:

- Is testable.
- Is consistent across the CLI.

Related to (does not fix) #2296 and #2204.

Signed-off-by: Zachary Newman <zjn@chainguard.dev>

Signed-off-by: Zachary Newman <zjn@chainguard.dev>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working good first issue Good for newcomers
Projects
None yet
Development

No branches or pull requests

3 participants