fix: ctx cancellation on login prompt

[Benehiko]

- What I did

Implemented a new Prompt function to capture user input called PromptForInput. This function works virtually similar to PromptForConfirmation and handles context cancellation.

- How I did it

Replicate PromptForConfirmation.

- How to verify it
Run docker login and cancel it midway with a SIGINT or SIGTERM or ctrl+c.

docker login

Username:^C

PromptForInput also allows for hiding user input - for example when the user enters their password.

The original behavior of docker login should still be in-tact with trimming spaces and not showing user input on the password prompt.

- Description for the changelog

- A picture of a cute animal (not mandatory but encouraged)

[laurazard]

Before this, when receiving a SIGINT during the login prompt, the CLI would exit with code 140

 ⭑ docker login      
Log in with your Docker ID or email address to push and pull images from Docker Hub. If you don't have a Docker ID, head over to https://hub.docker.com/ to create one.
You can log in with your password or a Personal Access Token (PAT). Using a limited-scope PAT grants better security and is required for organizations using SSO. Learn more at https://docs.docker.com/go/access-tokens/

Username (laura.brehm@docker.com): ^C
☆｡･:*:･ﾟ⭑ laurabrehm ✿ cli [e06bee6999] ⭑ echo $?     
130

However, now it's exiting with 0:

⭑ ./build/docker login                 
Log in with your Docker ID or email address to push and pull images from Docker Hub. If you don't have a Docker ID, head over to https://hub.docker.com/ to create one.
You can log in with your password or a Personal Access Token (PAT). Using a limited-scope PAT grants better security and is required for organizations using SSO. Learn more at https://docs.docker.com/go/access-tokens/

Username (laura.brehm@docker.com): ^C
☆｡･:*:･ﾟ⭑ laurabrehm ✿ cli [fix-cli-login] ⭑ echo $?                              
0

I don't think this is necessarily wrong (on one hand, the user is cancelling the operation) but not sure.

[Benehiko]

Before this, when receiving a SIGINT during the login prompt, the CLI would exit with code 140

 ⭑ docker login      
Log in with your Docker ID or email address to push and pull images from Docker Hub. If you don't have a Docker ID, head over to https://hub.docker.com/ to create one.
You can log in with your password or a Personal Access Token (PAT). Using a limited-scope PAT grants better security and is required for organizations using SSO. Learn more at https://docs.docker.com/go/access-tokens/

Username (laura.brehm@docker.com): ^C
☆｡･:*:･ﾟ⭑ laurabrehm ✿ cli [e06bee6999] ⭑ echo $?     
130

However, now it's exiting with 0:

⭑ ./build/docker login                 
Log in with your Docker ID or email address to push and pull images from Docker Hub. If you don't have a Docker ID, head over to https://hub.docker.com/ to create one.
You can log in with your password or a Personal Access Token (PAT). Using a limited-scope PAT grants better security and is required for organizations using SSO. Learn more at https://docs.docker.com/go/access-tokens/

Username (laura.brehm@docker.com): ^C
☆｡･:*:･ﾟ⭑ laurabrehm ✿ cli [fix-cli-login] ⭑ echo $?                              
0

I don't think this is necessarily wrong (on one hand, the user is cancelling the operation) but not sure.

Error code 130 used to occur on the previous Prompt confirmations that were cancelled as well. I can't remember if this was an error that Go decides or that we did with os.Exit. But see here #4850

[codecov-commenter]

Codecov Report

Attention: Patch coverage is 43.75000% with 18 lines in your changes missing coverage. Please review.

Project coverage is 61.47%. Comparing base (01dd6ab) to head (c15ade0).
Report is 11 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #5168      +/-   ##
==========================================
+ Coverage   61.43%   61.47%   +0.04%     
==========================================
  Files         298      298              
  Lines       20799    20808       +9     
==========================================
+ Hits        12777    12791      +14     
+ Misses       7109     7105       -4     
+ Partials      913      912       -1     

[Benehiko]

Please see this PR which would also be required in the case of other commands/features not respecting context cancellation #5171

[vvoland]

Why we ignore the passed ins on Windows?

[Benehiko]

I'm not too sure tbh - might be a legacy bug which has been resolved. I can maybe check out if we still need to do this fallback on a windows install.

It's possible we don't need it, but I guess keep it for good measure for the time being? 🤷‍♂️
See this comment: https://github.com/docker/cli/blob/master/cli/command/registry.go#L94-L104

[vvoland]

cli/cli/command/registry.go

Lines 93 to 104 in b83cf58

	func ConfigureAuth(cli Cli, flUser, flPassword string, authconfig *registrytypes.AuthConfig, isDefaultRegistry bool) error {
	// On Windows, force the use of the regular OS stdin stream.
	//
	// See:
	// - https://github.com/moby/moby/issues/14336
	// - https://github.com/moby/moby/issues/14210
	// - https://github.com/moby/moby/pull/17738
	//
	// TODO(thaJeztah): we need to confirm if this special handling is still needed, as we may not be doing this in other places.
	if runtime.GOOS == "windows" {
	cli.SetIn(streams.NewIn(os.Stdin))
	}

Hmm, so the code you linked is still there and already sets the in for the whole cli so we don't need to do that again in PromptForInput, because cli.In() will already be the overriden stdin.

[Benehiko]

Yes, although other places might call PromptForInput which might not set this. We also set this in PromptForConfirmation so keeping it consistent would be best I think.

[vvoland]

It completely breaks the assumption that the input will be read from the provided ins though.
If the ins happens to be something different than stdin, it would still read from stdin on Windows.

This will also make the unit tests from this PR fail on Windows because they provide a io.Pipe as an stdin.

[thaJeztah]

Alternative could be to pass a bare io.Reader, and always handle wrapping it into a streams.In internally; perhaps we don't even need the streams considering that we already use moby/term (which does most / all of the heavy-lifting in the streams package.

I'm curious though; if we need the special handling here; did we do something wrong with setting up the CLI and/or elsewhere; we should definitely look at that, and check if this is still needed.

I did a quick search; this code was originally introduced in the Moby repo (before it was moved to docker/cli); in this commit; moby/moby@9c76504

That's this PR, and related tickets;

• Windows: [TP4] Fix docker login moby/moby#17738
• docker login: cannot input value for username on windows + powershell moby/moby#14336
• boot2docker Windows running docker login freezes moby/moby#14210

That PR was for Windows RS4, so ... really old, and if it was a Windows bug, that side may at least be obsolete (🤞) but we should check!

[Benehiko]

I've removed the windows check in the last commit 668f874 - but still need to test a build on windows. I guess I'll need to install some older (supported) versions to ensure the majority of users won't be impacted.

[Benehiko]

I've tested on a recent version of windows 10 (19045.4529) and seems to work fine. I removed these lines

cli/cli/command/registry.go

Lines 93 to 104 in b83cf58

	func ConfigureAuth(cli Cli, flUser, flPassword string, authconfig *registrytypes.AuthConfig, isDefaultRegistry bool) error {
	// On Windows, force the use of the regular OS stdin stream.
	//
	// See:
	// - https://github.com/moby/moby/issues/14336
	// - https://github.com/moby/moby/issues/14210
	// - https://github.com/moby/moby/pull/17738
	//
	// TODO(thaJeztah): we need to confirm if this special handling is still needed, as we may not be doing this in other places.
	if runtime.GOOS == "windows" {
	cli.SetIn(streams.NewIn(os.Stdin))
	}

locally and tested the login flow. Works fine in powershell and command prompt

[thaJeztah]

• Looks like outs could be bare io.Writer (as we don't use any of the features provided by streams.Out
• nit: perhaps use singular for the ins and outs variables (in and out ?); it's more common.
• ☝️ I recalled we also had a Streams interface, which could be an alternative to use as argument, but it looks like that's in the cli/command package, so a bit on the fence on that one (see comment further below)
• Wondering though if this function should do a check if outs is a terminal, and return an error instead otherwise 🤔 ❓

Also wondering; is message always required, or could we think of situations where the message itself is already printed, and we're only interested in input provided by a user?

In that case, this could be named WaitForInput() and/or have a WithMessage() option.

Finally; as this is a new function, and cli/command is becoming a bit of a grab-bag of things; wondering if we should put this in a separate package; e.g. prompt.WaitForInput() or userinput.Prompt().

[thaJeztah]

Even considering; if it's a separate package, we could have a separate WaitforPassword (WaitforPassphrase ?) or RequestPassphrase utility that would handle the disableEcho. In that case, we could get rid of the PromptOptions type (at least for now), or keep that one internal until we have new options that could warrant introducing more options.

[Benehiko]

I'm not sure if the message parameter could have a default value since this is not a prompt like "confirm x" it's more like an open ended question "input x".

I've also gotten rid of the variadic parameter and instead opted for a utility function to disable input echo's in the terminal.

I guess one could also check inside this PromptForInput function if it's a terminal or not - but thinking about the PromptForConfirmation function we also don't check it there. I guess we would need think about a redesign of these two functions and move them into their own package.

[Benehiko]

@vvoland could we get this one merged? The windows specific code can be addressed in a follow-up. I'll be running some tests to verify if we need this on older (supported) versions of windows 10

cli/cli/command/registry.go

Lines 93 to 104 in b83cf58

	func ConfigureAuth(cli Cli, flUser, flPassword string, authconfig *registrytypes.AuthConfig, isDefaultRegistry bool) error {
	// On Windows, force the use of the regular OS stdin stream.
	//
	// See:
	// - https://github.com/moby/moby/issues/14336
	// - https://github.com/moby/moby/issues/14210
	// - https://github.com/moby/moby/pull/17738
	//
	// TODO(thaJeztah): we need to confirm if this special handling is still needed, as we may not be doing this in other places.
	if runtime.GOOS == "windows" {
	cli.SetIn(streams.NewIn(os.Stdin))
	}

[vvoland]

LGTM; needs a squash though

[laurazard]

LGTM