Add restore handler for password input #1312

grisu48 · 2023-02-06T14:03:16Z

This commit restores the terminal state in case the program is interrupted while being in password read mode. This ensures the terminal remains usable, also if the password input is being cancelled

This PR provides a solution for containers/podman#17372

Signed-off-by: Felix Niederwanger [email protected]

grisu48 · 2023-02-06T14:09:45Z

I'm not sure why the DCO fails. The commit and PR message have both the required Signed-off-by footer appended 🤔

Luap99 · 2023-02-06T14:18:28Z

The DCO is failing because you did not set your actual name for the git commit (git config user.name). But I don't think this is required for us, we can set the check manually to pass.

grisu48 · 2023-02-06T14:23:44Z

The DCO is failing because you did not set your actual name for the git commit (git config user.name). But I don't think this is required for us, we can set the check manually to pass.

Ahhhhhhh, thanks. Fixed and force-pushed.

Luap99

@vrothberg @mtrmac PTAL

pkg/auth/auth.go

Luap99 · 2023-02-06T14:33:39Z

pkg/auth/auth.go

+				if oldState != nil {
+					terminal.Restore(stdin, oldState)
+				}
+				os.Exit(1)


I don't think we want to call exit so far down in the stack. At least in podman we have handlers that depend on proper function, see libpod/shutdown.

Also I have no idea where this code is used besides podman, I think it might be better to return an error so that the caller can decide what to do with it.

I'm not sure how to handle this - In the podman repository I couldn't find any signal handlers for os.Interrupt except for machine.go, which also uses os.Exit(1).

I can certainly modify it such that getUserAndPass returns a fmt.Errorf("interrupted") but I'm not sure if this is the desired behaviour. What do you think?

I concur that we should return an error here.

vrothberg

Thanks for contributing!

It's probably worth moving the logic around terminal.ReadPassword into a separate function and update all callers (incl. pkg/ssh).

vrothberg · 2023-02-06T15:12:21Z

pkg/auth/auth.go

+				if oldState != nil {
+					terminal.Restore(stdin, oldState)
+				}
+				os.Exit(1)


I concur that we should return an error here.

grisu48 · 2023-02-06T15:49:49Z

I've updated the PR where I refactored a ReadPassword function to utils/utils.go. There are some places that would need to be updated later on as well, as @vrothberg stated.
They are located in other repositories and require this PR first, but it can be easily done afterwards.

mtrmac

AFAICS this is just impossible to do with term.ReadPassword: the original sample only works because of os.Exit, which we don’t want to do.

Looking around a bit, I just can’t see any plausible solution (x/term does have some code that attempts to handle Ctrl-C, but 1) in ICANON mode, it would only work after pressing Enter, and 2) we would have to signal.Ignore(os.Interrupt), which is a disruptive operation that can’t be undone).

In the extreme, we could fork / spawn a subprocess. Ugh… I’m not even sure that is safe to do, and it would be so very ugly.

pkg/util/util.go

mtrmac · 2023-02-06T20:37:46Z

In the extreme, we could fork / spawn a subprocess. Ugh…

That wouldn’t, alone, be sufficient either, because Ctrl-C causes SIGINT to be sent to the whole foreground process group, including bot the child and the parent, so Podman would still need to arrange to ignore SIGINT in some reversible way. And Podman creating a separate process group is another can of worms… that would interfere with Ctrl-Z / fg / bg.

I wouldn’t entirely rule out the possibility of making all of that work, but the complexity (and dependency one of the most obscure aspects of POSIX — with Windows and other systems a whole another can of worms) makes it very attractive to me to suggest to just give up on this. Convincing users to use a more modern / featured shell that resets the terminal state before printing a prompt, or providing such patches to major shells, is starting to look easier than Podman handling this…

One, sort of, alternative is to try to make the os.Exit approach more viable, by explicitly introducing some kind of ”CLI parsing phase” to Podman, where Podman is not yet allowed to really do anything substantial (but what about user namespaces and reexec??), and providing only a ReadPassphraseInCLIParsingPhase that makes a lot of assumptions about the environment (it’s safe to call os.Exit; this is the only package so far to invoke os/signal; and the like).

But, again, ugh.

grisu48 · 2023-02-07T08:27:15Z

Thanks for the review and the input! I see two paths on how this could be addresses, both have their own caveats:

Don't stop the INTERRUPTED signal, just restore the terminal

Please see suggestion 1 in the PR. I'm catching SIGINT, restore the terminal and then re-emit the signal. This is IMHO the least invasive change, as it should not alter the existing behaviour much: It would only restore the terminal as intermittent step.

Handle SIGINT gracefully, return an error from ReadPassword but leave the terminal reader active, and thus stdin blocked

This is ugly, but has the advantage, that podman can run it's clean-up routines, as suggested by @Luap99 and @vrothberg. Successive access to stdin would result in unintended behaviour though, so this might be better from the perspective of ReadPassword but introduces a tripping hazard.

Please see the current commit, where I added a suggestion for both methods.

How should we continue? I have a preference for solution 1, but would leave it to someone with far better insight to suggest on how to proceed.

Thank you!

mtrmac

I really think we can’t reasonably solve this locally in c/common.

90% of the problem is that golang.org/x/term.ReadPassword needs to be interruptible (maybe using a context.Context). That would make its existing implementation automatically take care of restoring the TTY configuration.

Then we would need to “only” care about turning a signal into a cancellation + error, and possibly turning the error back into a signal. That’s still not trivial — but having the cancellation feature would allow us to turn the signal into a cancellation+error using a very non-invasive signal.Notify which doesn’t interfere with anything else in the process, and to turn the error into a signal at the very top of the Podman call stack, where there is nothing else to interfere with.

So, would anyone care to propose this to …/x/term maintainers?

pkg/util/util.go

mtrmac · 2023-02-07T17:31:24Z

pkg/util/util.go

+					term.Restore(fd, oldState)
+				}
+				signal.Stop(interruptChannel)
+				syscall.Kill(syscall.Getpid(), syscall.SIGINT)


I would expect the goroutine to exit here instead of waiting on interruptChannel (and hoping that the SIGINT actually terminates the parent function, which is not guaranteed in presence of other code).

mtrmac · 2023-02-07T18:09:46Z

pkg/util/util.go

+					term.Restore(fd, oldState)
+				}
+				signal.Stop(interruptChannel)
+				syscall.Kill(syscall.Getpid(), syscall.SIGINT)


Suggestion 1:

This works in a top-level application; it’s fairly suspect in library code. E.g. if there is anything else registered via signal.Notify, it will now see the signal twice.

Also, even with this approach, I think we need to interrupt term.ReadPassword, because there could be some other party with a signal handler; in that case even this syscall.Kill might never terminate the term.ReadPassword.

The good thing about this approach is that the exit code of the process actually reflects the signal. (BTW that doesn’t actually happen directly here, there is still a Go signal handler installed, but it does ~the same thing, uninstalling the handler and re-sending the signal.)

My main concern is that there is no way of determining if there are other signal handlers for SIGINT present that attempt to gracefully terminate the application. This signal handler re-emits SIGINT and kills them. I could not find anything in os/signal that allows us to check, if there are other handlers present.

This makes this solution undesirable, as it would kill possible other signal handlers, apart from your points.

mtrmac · 2023-02-07T18:20:01Z

pkg/util/util.go

+			buf.buf, buf.err = term.ReadPassword(fd)
+			inputChannel <- buf


Suggestion 2:

With this, term.ReadPassword continues running, consuming resources (meh), but potentially interfering: it continues to consume data from stdin, and it can eventually return and change the terminal state at any future time.

I can’t see how we can terminate the current implementation: fd must be a terminal (so that ioctls work); and I can’t see that a pending read(3) would be interrupted by a close(3).

A downside of turning the signal into an error is that the exit status of the process will not be traditionally correct. (In principle, we could turn it into a recognizable error, and turn that error into a signal at the very top of Podman.)

Yes. The only way of solving this would be to change term.ReadPassword to handle interrupts itself gracefully.

mtrmac · 2023-02-07T18:30:46Z

(x/term does have some code that attempts to handle Ctrl-C,

My mistake: term.ReadPassword and term.Terminal.ReadPassword are completely independent implementations, and the quoted note refers to the latter, which we don’t use. (Using it would not help us anyway...)

grisu48 · 2023-02-08T16:28:42Z

The issue is filed in golang/go#31180 and so far there has not been much activity or any visible attempts to solve this.

If we would get an interruptible term.ReadPassword implementation, then suggestion 2 could be changed to a clean library function.

Looks like we're hitting a wall here.

mtrmac · 2023-02-08T17:27:17Z

My suggestion for what to try would be to file a PR directly at https://github.com/golang/term , to add something like ReadPasswordWithContext. But I have no idea whether it would be likely to be accepted, and it’s also not work I can prioritize doing myself.

grisu48 · 2023-02-09T07:31:42Z

Thanks. I'll have a look and see what we can do. I leave this PR open and update it accordingly, so we keep track of the progress.

grisu48 · 2023-02-14T15:12:15Z

I'm working on a prototype for ReadPasswordWithContext but am busy with other tasks as well, so this might take some time.

Also: This requires a Unix/Windows implementation on a rather low level, so this is a nontrivial task.

mtrmac · 2023-02-14T17:32:02Z

Also: This requires a Unix/Windows implementation on a rather low level, so this is a nontrivial task.

From a very quick look it seems that it could be possible to wrap the Reader passed to readPasswordLine with one that checks whether a context was canceled, but I didn’t actually try implementing that so I might be very off-base.

grisu48 · 2023-02-15T08:06:21Z

Also: This requires a Unix/Windows implementation on a rather low level, so this is a nontrivial task.

From a very quick look it seems that it could be possible to wrap the Reader passed to readPasswordLine with one that checks whether a context was canceled, but I didn’t actually try implementing that so I might be very off-base.

The problem with this approach is that a ongoing Read from the reader would block until the next character, even if the context is cancelled. I try to avoid this issue by implementing a nonblocking polling approach for this, which should in principle not have this limitation. Implementing this for all supported platforms, including Windows, is a however not as easy as I was hoping for AFAICS 🙂

grisu48 · 2023-03-06T15:06:35Z

golang/term#12 as a first attempt to implement ReadPasswordWithContext. I would not expect this to fly soon, as the approach is not yet elegant enough for a library function. Still, the issue is not forgotten 🙂

grisu48 · 2023-03-24T07:58:50Z

No reply for two weeks on golang/term#12. I also believe this is not going to fly because there two main issues that can not be resolved elegantly within the library function:

Readers can't be interrupted. I solve this by non-blocking IO, but that's ugly and it does not work on Windows (hmpf!)
Non-blocking IO requires polling, which or hogs the CPU or adds a (hopefully not noticeable) latency

I believe we're hitting a dead end there as well. Has anyone an alternative idea how we would approach this issue?
I would still argue, that having a signal handler which restores the terminal and then calls os.Exit might be the best of the bad options, given that this is a) AFAICS the current behaviour and b) missing alternatives.

What do you think?

mtrmac · 2023-03-24T19:28:05Z

I think that leaves the ReadPassphraseInCLIParsingPhase approach… Aesthetically I find that quite unattractive, but I’ll leave it to Podman maintainers to decide whether it is is worth it.

grisu48 · 2023-08-03T08:30:47Z

Reviving this PR.

Please review my current suggestion. After multiple attempts we concluded that there is no perfect solution, as ReadPassword from x/term leaves the terminal in non-echo mode (See golang/go#31180).

My suggestion based on #1312 (comment) is to implement a special ReadPassphraseInCLIParsingPhase function, which is allowed to terminate podman after restoring the terminal state. Since this function is only used in the authentication, where the current behaviour of podman is to terminate due to SIGINT, this would not alter the current program flow.

Other attempts to do busy waiting on a non-blocking terminal might work as well but have their own caveats (increased system load and latency due to busy polling). Also they don't work on Windows.

Asking @vrothberg @Luap99 on their opinion on this topic or if they have a better idea how to approach this issue.

vrothberg · 2023-08-03T08:52:12Z

pkg/util/util_supported.go

+				terminal.Restore(fd, oldState)
+			}
+			signal.Reset(os.Interrupt)
+			os.Exit(1)


Why not just return an error and give Podman a chance to properly exit? There is usually some cleanup do be done before Podman exists.

Because there is no way to return from terminal.ReadPassword, so this would mean the terminal remains blocked.

We could return from the function, but that still leaves the terminal blocked until the return key is pressed, as we discussed in #1312 (comment). This is indeed the main issue where we have been hitting a wall.

So if you returned the error, users would need to hit or if you exit here users have to hit correct?

Not sure if I got your question right - If we return an error, the stdin would still be blocked by terminal.ReadPassword until the user hits return.

We even tried to close the fd but not even that unblocks the read call from terminal.ReadPassword . So returning an error here on interrupt is possible, but it would mean that the terminal (stdin) remains in a weird state, which is likely not usable on subsequent calls. 🙁

I can easily rewrite it such that the function returns an error, but then stdin remains blocked. This would allow podman to do all the cleanup routines with the caveat that stdin cannot be used in the error case.

I decided to go the path of terminating within the function, because that's how podman currently handles the situation. From the functional standpoint this is the least intrusive change - the only difference is that the terminal is restored before podman is being terminated anyways - either via os.Exit(1) as in this PR or via SIGINT as it is of now.

How should we proceed?

man podman ... 125 The error is with podman itself $ podman run --foo busybox; echo $? Error: unknown flag: --foo 125

I guess it could be argued that killing Podman at this state is an error in Podman, so 125 is the correct error.
Other error codes usually come from the container or the lack of ability to execute a command within the container.

@mheon WDYT?

The SIGINT handler in question should be https://github.com/containers/podman/blob/main/libpod/shutdown/handler.go but that doesn't help us much - that's nestled deep into Libpod and as such we can't touch it from here in c/common.

Generally speaking we should avoid os.Exit() whenever possible (for the exact reasons we wrote the shutdown handler in Libpod - there are situations where we need to keep Podman alive long enough to finish essential cleanup before obeying user-initiated shutdown), so I'm in favor of returning an error, but I have no easy way of disabling the interrupt handler from within c/common to give us consistent error codes.

I see that almost everyone in this threads favours an approach of returning an error. This doesn't remove the race condition with libpod/shutdown/handler.go, but it works in most of the cases I tested (1/19), so it would improve the situation already substantially.

I will update the PR accordingly.

@rhatdan @vrothberg @mheon Please review. The discussed race condition with libpod/shutdown/handler.go remains, but there is little what we can do within this function as signal handlers cannot be disabled an re-enabled. See also #1312 (comment)

On my laptop, only in 1/19 cases the race condition prevented the terminal from being restored, in the other 18 cases it works as expected. It's not ideal, but given a missing viable alternative I'd argue that it still is an improvement and the ReadPassword function behaves as desired.

Thanks for the patience!

So I think the answer here is to take a step back, I understand what this PR is trying to do and that looks reasonable.

If we look at the code path exactly podman login bypasses pretty much all of libpod and calls directly in to pkg/auth here. So shouldn't be the answer disable the libpod setup for podman login/logout in which case there cannot be a race between the signal handlers.

rhatdan · 2023-08-14T16:13:19Z

/approve
LGTM
@vrothberg @Luap99 @mtrmac PTAL

pkg/util/util_supported.go

rhatdan · 2023-08-16T12:42:25Z

@giuseppe @Luap99 @vrothberg @mtrmac PTAL

This commit restores the terminal state in case the program is interrupted while being in password read mode. This ensures the terminal remains usable, also if the password input is being cancelled. Signed-off-by: phoenix <[email protected]>

grisu48 · 2023-08-17T12:54:37Z

Rebased and applied gofumpt.

grisu48 · 2023-09-01T09:01:32Z

@rhatdan ping?

rhatdan · 2023-09-02T10:47:42Z

LGTM
@giuseppe @Luap99 @vrothberg @mtrmac PTAL

vrothberg

/lgtm

openshift-ci · 2023-09-04T07:51:02Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: grisu48, rhatdan, vrothberg

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [rhatdan,vrothberg]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

grisu48 force-pushed the password branch from 23586ba to e518e72 Compare February 6, 2023 14:23

grisu48 force-pushed the password branch 2 times, most recently from 110399a to cad3633 Compare February 6, 2023 14:29

Luap99 reviewed Feb 6, 2023

View reviewed changes

grisu48 force-pushed the password branch from cad3633 to 2084565 Compare February 6, 2023 14:44

vrothberg reviewed Feb 6, 2023

View reviewed changes

grisu48 force-pushed the password branch from 2084565 to 1637e40 Compare February 6, 2023 15:47

mtrmac requested changes Feb 6, 2023

View reviewed changes

grisu48 force-pushed the password branch from 1637e40 to 4e05db3 Compare February 7, 2023 08:17

grisu48 changed the title ~~Add workaround for no password echo~~ Add restore handler for password input Feb 7, 2023

grisu48 marked this pull request as draft February 7, 2023 08:18

openshift-ci bot added the do-not-merge/work-in-progress label Feb 7, 2023

mtrmac reviewed Feb 7, 2023

View reviewed changes

grisu48 force-pushed the password branch from 4e05db3 to 828badb Compare February 8, 2023 16:16

grisu48 mentioned this pull request Aug 1, 2023

[Bug]: Cancelling podman login breaks current console containers/podman#17372

Open

grisu48 force-pushed the password branch from 828badb to 101b41a Compare August 2, 2023 15:28

openshift-merge-robot removed the needs-rebase label Aug 2, 2023

grisu48 force-pushed the password branch 3 times, most recently from 5788140 to 0cfe322 Compare August 3, 2023 08:25

grisu48 marked this pull request as ready for review August 3, 2023 08:30

openshift-ci bot removed the do-not-merge/work-in-progress label Aug 3, 2023

grisu48 force-pushed the password branch from 0cfe322 to c906bf7 Compare August 3, 2023 08:32

vrothberg reviewed Aug 3, 2023

View reviewed changes

grisu48 force-pushed the password branch from c906bf7 to 4fa1820 Compare August 3, 2023 12:18

grisu48 force-pushed the password branch 2 times, most recently from b2c848f to 6dd9c07 Compare August 14, 2023 15:24

openshift-ci bot added the approved label Aug 14, 2023

Luap99 reviewed Aug 14, 2023

View reviewed changes

pkg/util/util_supported.go Outdated Show resolved Hide resolved

grisu48 force-pushed the password branch 2 times, most recently from 91a353f to b8fab13 Compare August 15, 2023 15:05

Add restore handler for password input

14e74aa

This commit restores the terminal state in case the program is interrupted while being in password read mode. This ensures the terminal remains usable, also if the password input is being cancelled. Signed-off-by: phoenix <[email protected]>

grisu48 force-pushed the password branch from b8fab13 to 14e74aa Compare August 17, 2023 12:54

vrothberg approved these changes Sep 4, 2023

View reviewed changes

openshift-ci bot assigned vrothberg Sep 4, 2023

openshift-ci bot added the lgtm label Sep 4, 2023

openshift-merge-robot merged commit 034496e into containers:main Sep 4, 2023

Add restore handler for password input #1312

Add restore handler for password input #1312

Conversation

grisu48 commented Feb 6, 2023 • edited Loading

grisu48 commented Feb 6, 2023

Luap99 commented Feb 6, 2023

grisu48 commented Feb 6, 2023

Luap99 left a 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

vrothberg left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

grisu48 commented Feb 6, 2023

mtrmac left a comment

Choose a reason for hiding this comment

mtrmac commented Feb 6, 2023 • edited Loading

grisu48 commented Feb 7, 2023

mtrmac left a comment • edited Loading

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

mtrmac commented Feb 7, 2023

grisu48 commented Feb 8, 2023

mtrmac commented Feb 8, 2023

grisu48 commented Feb 9, 2023

grisu48 commented Feb 14, 2023 • edited Loading

mtrmac commented Feb 14, 2023

grisu48 commented Feb 15, 2023

grisu48 commented Mar 6, 2023

grisu48 commented Mar 24, 2023

mtrmac commented Mar 24, 2023

grisu48 commented Aug 3, 2023

Choose a reason for hiding this comment

grisu48 Aug 3, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

grisu48 Aug 3, 2023 • edited Loading

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

grisu48 Aug 14, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rhatdan commented Aug 14, 2023

rhatdan commented Aug 16, 2023

grisu48 commented Aug 17, 2023

grisu48 commented Sep 1, 2023

rhatdan commented Sep 2, 2023

vrothberg left a comment

Choose a reason for hiding this comment

openshift-ci bot commented Sep 4, 2023

grisu48 commented Feb 6, 2023 •

edited

Loading

vrothberg left a comment •

edited

Loading

mtrmac commented Feb 6, 2023 •

edited

Loading

mtrmac left a comment •

edited

Loading

grisu48 commented Feb 14, 2023 •

edited

Loading

grisu48 Aug 3, 2023 •

edited

Loading

grisu48 Aug 3, 2023 •

edited

Loading

grisu48 Aug 14, 2023 •

edited

Loading