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

STEP CLI assume human-in-the-loop (ignoring STDIN), making it difficult to script #502

Open
eengstrom opened this issue Jul 9, 2021 · 9 comments

Comments

@eengstrom
Copy link

The STEP CLI is not easily used in a non-interactive (i.e. scripted) fashion, since it seems to assume a human-in-the-loop.

What would you like to be added

It's not clear what the "best" solution is to the problem, fully described below. I see several possible options:

  • CLI should provide command line option or environment variable equivalents for all TTY prompts,
  • CLI should have a flag, say --non-interactive, never prompting the user and accepting all 'defaults',
  • CLI should accept input on STDIN for all prompts (admittedly brittle).

Why this is needed

As currently implemented, step-cli looks to be coded to open the TTY directly to ask questions of the user, assuming there is a human running the script. Moreover, there is no option to say --non-interactive (i.e. --don't-prompt-me--I'm-not-human), nor is there a way to provide answers to the all the questions that may be asked. This is a problem when trying to build automation around the cli, using Ansible, for example.

One very (POSIX) traditional, albeit brittle, way of providing input to a CLI would be to pipe the answers in via STDIN, in a pipeline fashion. Looking briefly through the code base, it looks like the use of the ui.Prompt() is pervasive, which is great from a consistency perspective, but that function doesn't pay any attention to STDIN, and attempts to directly interact with the underlying TTY.

Another way would be to ensure that all questions have corresponding command line options or environment variable equivalents that can be used if there is no TTY from which to read. Perhaps the most uniform way would be to uniquely "TAG" all the questions being asked. Then a generic -o TAG=value command line option could be added. This would also be amenable to allowing STEPCLI_TAG=value step-cli ... environment variable equivalents. Most of the functionality for this would be implemented directly in ui.go, thus minimizing pervasive changes.

The final approach I can envision, and probably the easiest to implement, would be to add a --non-interactive option to the CLI, which would be used in the ui/ui.go function ui.Prompt() to avoid even the attempt to open the TTY and ask a question, instead returning the default response directly as the answer.

Honestly, that last approach could probably be assumed if there is no TTY available, thus no additional command-line option (e.g. --non-interactive) is needed at all.

Example Use Case

My goal is to automate use of the step-cli. This most recently came up because I am deploying a new CA, replacing an existing CA, and therefore my current Ansible playbooks (i.e. scripts) are trying to issue a command like this:

step-cli ca bootstrap --ca-url=https://HOSTNAME:443 --fingerprint=HASH

If I issue that command by hand, I get a nice prompt:

✗ Would you like to overwrite /root/.step/certs/root_ca.crt [y/n]: █

However, in the case of the automation, this fails with the above no /dev/tty error. Attempts to close stdin or do yes no | step-cli ca ... have no effect, obviously, since the cli doesn't even look at STDIN.

I know I can manually remove the config file or directory before running the command, or use the --force option, but in this case, I want to get an error (return code != 0) if the host thinks it's already got a CA configured, and then have the automation report that to the user, or do something smarter. Ideally, the answer no could be provided to the CLI, in which case I would get exactly what I want, just like as if I run the command by hand:

root@steptest # step-cli ca bootstrap --ca-url=https://HOST:443 --fingerprint=HASH
✔ Would you like to overwrite /root/.step/certs/root_ca.crt [y/n]: n
unexpected error on /root/.step/certs/root_ca.crt: file exists

root@steptest # echo $?
1

However, as it is currently, when running that command via my scripts, this fails with an unhelpful error:

open /dev/tty: no such device or address

Note that the fundamental problem is much bigger than my example here. IMO, anywhere the cli would issue a prompt to a presumed human user should be avoidable by some means employable by scripts/ CI / automation.

Related Issues

I believe versions of this problem have been discussed before, in one or more of these issues/PRs:

@eengstrom eengstrom added enhancement needs triage Waiting for discussion / prioritization by team labels Jul 9, 2021
eengstrom added a commit to eengstrom/ansible-collection-smallstep that referenced this issue Jul 9, 2021
This fix captures a different or updated error message from
`step-cli` when it attempts to open `/dev/tty`:

    open /dev/tty: no such device or address

Ideally, `step-cli` would provide a way to avoid this entirely; c.f:
  - smallstep/cli#502
@maxhoesel
Copy link

maxhoesel commented Jul 10, 2021

For some more perspective, I'm the main author of the linked/mentioned Ansible collection for automating both step-ca and step-cli. This "always-prompt-for-input" behavior hast been problematic because it makes handling missing parameters pretty rough:

Right now, if the user doesn't supply all the required parameters to a command, we just hope that step-cli throws out a message indicating that it couldn't open a terminal and assume that this is due to missing user input. Not only is this pretty indirect, but it would also fail iif the step-cli error message is different or if step-cli does somehow manage to open a terminal - in which case it would probably wait for user input that can never arrive.

This could be avoided with a --non-interactive/--no-prompt option that causes step-cli to fallback to defaults/exits and display a nice error message on missing input. The implicit behavior option described by @eengstrom would work well for that too.

eengstrom added a commit to eengstrom/ansible-collection-smallstep that referenced this issue Jul 12, 2021
This workaround captures a different or updated error message
from `step-cli` when it attempts to open `/dev/tty`:

    open /dev/tty: no such device or address

Ideally, `step-cli` would provide a way to avoid this entirely; c.f:
  - smallstep/cli#502
@dopey
Copy link
Contributor

dopey commented Jul 12, 2021

Hey @eengstrom, @maxhoesel thanks for the detailed issue (and context)! We triage all new issues (that aren't "drop everything" bugs) once a week (on wednesdays generally), so I'll follow up after that meeting.

eengstrom added a commit to eengstrom/ansible-collection-smallstep that referenced this issue Jul 12, 2021
This workaround captures a different or updated error message
from `step-cli` when it attempts to open `/dev/tty`:

    open /dev/tty: no such device or address

Ideally, `step-cli` would provide a way to avoid this entirely; c.f:
  - smallstep/cli#502
maxhoesel pushed a commit to maxhoesel-ansible/ansible-collection-smallstep that referenced this issue Jul 13, 2021
This workaround captures a different or updated error message
from `step-cli` when it attempts to open `/dev/tty`:

    open /dev/tty: no such device or address

Ideally, `step-cli` would provide a way to avoid this entirely; c.f:
  - smallstep/cli#502
@maraino
Copy link
Collaborator

maraino commented Jul 13, 2021

For errors like:

✗ Would you like to overwrite /root/.step/certs/root_ca.crt [y/n]: █

Have you try to use the flag --force?

@maxhoesel
Copy link

That works for confirmations like the above, but it unfortunately doesn't work when there is missing input. One example I can think of is step prompting the user to select a provisioner when running step ca certificate like here:

❯ sudo step-cli ca certificate test out.crt out.key --force
Use the arrow keys to navigate: ↓ ↑ → ← 
What provisioner key do you want to use?
  ▸ sshpop (SSHPOP)
    apps-acme (ACME)

Ideally, with the --no-prompt flag proposed above, step-cli would just realize that there are missing parameters and print out a helpful error for the user. Again, this would be really helpful for automating step-cli.

@eengstrom
Copy link
Author

what Max said, plus in the exact use case, I don't want to use --force and instead want an error from step_cli. To use --force would hide the fact that the user is, inadvertently, overwriting existing configuration.

In my brief survey of the code base, it appears that most, perhaps all, of the prompts to the user have default values. Assuming that the defaults are "safe" (can't have unexpected side-effects, such as overwriting configuration), then assuming the defaults when given --no-prompt might be appropriate.

@dopey
Copy link
Contributor

dopey commented Jul 21, 2021

Hey, sorry for the radio silence. We are definitely open to adding --no-prompt or --non-interactive (whatever is common among other CLI tools) and forcing failure in those scenarios where a required value has not been set.

If anyone is interested in getting their hands dirty here, we'd be happy to point folks in the right direction in the code base.

The tricky part might be outputting a useful error message that has the name of the missing flag since we may not have access to that at the moment we go to prompt.

@dopey dopey removed the needs triage Waiting for discussion / prioritization by team label Jul 21, 2021
@maxhoesel
Copy link

maxhoesel commented Jul 22, 2021

I don't have much of any experience with go, but I'd be down to give it a shot. From what I can tell, the simplest way to do this would be to check for --no-prompt before each call to Prompt(Password/YesNo)() in the individual command files. So for example, in certificate/create:

if passFile := ctx.String("password-file"); passFile != "" {
        pass, err = utils.ReadPasswordFromFile(passFile)
	if err != nil {
	        return errors.Wrap(err, "error reading encrypting password from file")
	}
} else if noPrompt := ctx.Bool("no-prompt"); noPrompt {
         // Could also be a new error type (NoPromptWithMissingFlag or similar)
        return errors.RequiredWithFlag(ctx, "no-prompt", "password-file")
} else {
	pass, err = ui.PromptPassword("Please enter the password to encrypt the private key",
	        ui.WithValidateNotEmpty())
	if err != nil {
	        return errors.Wrap(err, "error reading password")
	}
}

That said, this approach would add an extra check before every single Prompt() call in the entire codebase. A quick check with grep spits out about 50 calls, so it might be better to move the check into the Prompt() functions themselves. Not quite sure how we'd pass the required data in that case though

The tricky part might be outputting a useful error message that has the name of the missing flag since we may not have access to that at the moment we go to prompt.

Do you have an example of where this might be the case? I'm afraid I'm not familiar enough with some of the more advanced CLI features to think of such a case.

@eengstrom
Copy link
Author

When I looked at the code, I thought an easy solution would be to bake in the error and/or defaults into the ui.Prompt*() functions themselves. A very brief look led me to believe there was sufficient information in the calls to do what we would want there.

Explicit caveat: I did NOT look at all the uses, so there may be rework around some of them.

A current dev with better understanding of the UI library might have some useful insight.

@dopey
Copy link
Contributor

dopey commented Jul 22, 2021

Yep, I think you've both kind of nailed what I was thinking -- basically adding this stuff to the Prompt method (and all the other prompt alternatives e.g., promptpassword). We've got a few options:

  1. Change Prompt(label string, opts ...Option) to something like `Prompt(ctx *cli.Context, flag string, label string, opts ...Option). Main issue here is that we'll be changing the API so it will be a breaking change for anyone outside our cli package using ui.Prompt (I doubt anyone is using it, but something to think about).

  2. Make a new method like PromptNonInteractive(ctx *cli.Context, flag string, label string, opts ...Option). And then update every use of Prompt to use the new method.

  3. Pass ctx.Bool("no-prompt") and the name of the flag as options to the Prompt method. In the ...Option list. The problem with this is that it puts the onus on the developer to remember to do this in future invocations. It would be nice if this were the default behavior (in my opinion) -- which leaves us with 1 or 2.

I'm partial to 1). @maraino what do you think?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants