fix: ctx cancellation on login prompt #5168

Benehiko · 2024-06-18T12:26:58Z

- 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)

cli/command/utils.go

laurazard · 2024-06-18T12:40:46Z

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 ([email protected]): ^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 ([email protected]): ^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 · 2024-06-18T12:52:36Z

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 ([email protected]): ^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 ([email protected]): ^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 · 2024-06-18T13:39:25Z

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 · 2024-06-18T13:47:02Z

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

vvoland · 2024-06-21T09:04:11Z

cli/command/utils.go

+	// On Windows, force the use of the regular OS stdin stream.
+	if runtime.GOOS == "windows" {
+		ins = streams.NewIn(os.Stdin)
+	}


Why we ignore the passed ins on Windows?

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

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.

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.

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.

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!

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.

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 · 2024-06-21T11:48:38Z

cli/command/utils.go

+// When the prompt returns an error, the caller should propagate the error up
+// the stack and close the io.Reader used for the prompt which will prevent the
+// background goroutine from blocking indefinitely.
+func PromptForInput(ctx context.Context, ins *streams.In, outs *streams.Out, message string, opts ...PromptOptions) (string, error) {


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().

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.

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.

cli/command/utils.go

cli/command/registry/login_test.go

cli/command/utils.go

Benehiko · 2024-07-01T16:31:13Z

@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

Signed-off-by: Alano Terblanche <[email protected]>

laurazard

LGTM

Benehiko force-pushed the fix-cli-login branch from 76cbd4f to 623f59b Compare June 18, 2024 12:27

Benehiko requested a review from a team June 18, 2024 12:28

vvoland reviewed Jun 18, 2024

View reviewed changes

cli/command/utils.go Outdated Show resolved Hide resolved

thaJeztah added kind/bug status/2-code-review labels Jun 18, 2024

thaJeztah added this to the 27.0.0 milestone Jun 18, 2024

vvoland added kind/bugfix PR's that fix bugs and removed kind/bug labels Jun 18, 2024

Benehiko force-pushed the fix-cli-login branch from 7ed5d31 to f63f68e Compare June 21, 2024 06:50

Benehiko requested a review from vvoland June 21, 2024 07:17

Benehiko self-assigned this Jun 21, 2024

vvoland reviewed Jun 21, 2024

View reviewed changes

thaJeztah reviewed Jun 21, 2024

View reviewed changes

Benehiko force-pushed the fix-cli-login branch from 668f874 to 3aed38a Compare June 26, 2024 08:43

vvoland modified the milestones: 27.0.0, 28.0.0 Jun 26, 2024

vvoland reviewed Jun 26, 2024

View reviewed changes

cli/command/utils.go Outdated Show resolved Hide resolved

vvoland reviewed Jun 26, 2024

View reviewed changes

cli/command/registry/login_test.go Outdated Show resolved Hide resolved

vvoland reviewed Jun 26, 2024

View reviewed changes

cli/command/utils.go Outdated Show resolved Hide resolved

Benehiko force-pushed the fix-cli-login branch from 72033de to cc2b612 Compare June 28, 2024 09:39

Benehiko requested review from thaJeztah and vvoland July 1, 2024 16:31

vvoland approved these changes Jul 2, 2024

View reviewed changes

fix: ctx cancellation on login prompt

c15ade0

Signed-off-by: Alano Terblanche <[email protected]>

Benehiko force-pushed the fix-cli-login branch from cc2b612 to c15ade0 Compare July 2, 2024 10:08

vvoland requested a review from laurazard July 3, 2024 09:15

laurazard approved these changes Jul 3, 2024

View reviewed changes

krissetto approved these changes Jul 3, 2024

View reviewed changes

vvoland merged commit bab48eb into docker:master Jul 3, 2024
88 checks passed

thaJeztah added the area/authentication label Jul 18, 2024

thaJeztah mentioned this pull request Jul 18, 2024

[27.1 backport] fix: ctx cancellation on login prompt #5260

Merged

thaJeztah added the process/cherry-picked label Jul 18, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: ctx cancellation on login prompt #5168

fix: ctx cancellation on login prompt #5168

Benehiko commented Jun 18, 2024

laurazard commented Jun 18, 2024

Benehiko commented Jun 18, 2024

codecov-commenter commented Jun 18, 2024 •

edited

Loading

Benehiko commented Jun 18, 2024

vvoland Jun 21, 2024

Benehiko Jun 21, 2024

vvoland Jun 21, 2024

Benehiko Jun 21, 2024

vvoland Jun 21, 2024

thaJeztah Jun 21, 2024

Benehiko Jun 21, 2024

Benehiko Jul 1, 2024

thaJeztah Jun 21, 2024

thaJeztah Jun 21, 2024

Benehiko Jun 21, 2024

Benehiko commented Jul 1, 2024

vvoland left a comment

laurazard left a comment

	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))
	}

fix: ctx cancellation on login prompt #5168

fix: ctx cancellation on login prompt #5168

Conversation

Benehiko commented Jun 18, 2024

laurazard commented Jun 18, 2024

Benehiko commented Jun 18, 2024

codecov-commenter commented Jun 18, 2024 • edited Loading

Codecov Report

Benehiko commented Jun 18, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Benehiko commented Jul 1, 2024

vvoland left a comment

Choose a reason for hiding this comment

laurazard left a comment

Choose a reason for hiding this comment

codecov-commenter commented Jun 18, 2024 •

edited

Loading