| Message ID | 1585513970-32658-2-git-send-email-selva.nair@gmail.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [Openvpn-devel,1/2] Move querying username/password from management to a function | expand |
Hi, On Sun, Mar 29, 2020 at 4:34 PM <selva.nair@gmail.com> wrote: > > From: Selva Nair <selva.nair@gmail.com> > > If only username is found in the file, redirect the auth-user-pass > query to the management on Windows if (i) management-query-passwords > is enabled and (ii) stdout is redirected to a log file. These > restrictions avoid regressive behaviour: those running from the > command line will continue to get the prompt on the console > and if both username and password are in the file those will > continue to get used. > > Note that the management will prompt for both username and password > ignoring the username read from the file. As the GUI saves the > username, this is a one-time inconvenience. > > Currently, the password is queried on the console (or systemd) > in such cases. This is not sensible on windows if log file is > redirected (prompt goes to the log file), or the console > is not available as happens when the GUI is in use. Why only Windows? I'd like this for macOS, too! On a Mac using Tunnelblick (which uses the management interface with management-query-passwords enabled), if the auth-user-pass file contains only the password (and a LF), then the following occurs: neither stdin nor stderr are a tty device and you have neither a controlling tty nor systemd - can't ask for 'Enter Auth Password:'. If you used --daemon, you need to use --askpass to make passphrase-protected keys work, and you can not use --auth-nocache. Exiting due to fatal error Note: Tunnelblick uses the "--log" option to redirect output to a file. I am assuming that's what is meant by "stdout is redirected to a log file". It would be nice to have the same behavior on both Windows and Mac. For Tunnelblick, too, the username from the file would be lost but, as with the Windows GUI, the user can opt to save it so it isn't asked for again. Best regards, Jon Bullard
Hi, On Sun, Mar 29, 2020 at 7:13 PM Jonathan K. Bullard <jkbullard@gmail.com> wrote: > > Hi, > > On Sun, Mar 29, 2020 at 4:34 PM <selva.nair@gmail.com> wrote: > > > > From: Selva Nair <selva.nair@gmail.com> > > > > If only username is found in the file, redirect the auth-user-pass > > query to the management on Windows if (i) management-query-passwords > > is enabled and (ii) stdout is redirected to a log file. These > > restrictions avoid regressive behaviour: those running from the > > command line will continue to get the prompt on the console > > and if both username and password are in the file those will > > continue to get used. > > > > Note that the management will prompt for both username and password > > ignoring the username read from the file. As the GUI saves the > > username, this is a one-time inconvenience. > > > > Currently, the password is queried on the console (or systemd) > > in such cases. This is not sensible on windows if log file is > > redirected (prompt goes to the log file), or the console > > is not available as happens when the GUI is in use. > > Why only Windows? I'd like this for macOS, too! I did not know what other platforms were affected and, in particular, how to handle them. We can't make this the default as some have systemd. Also on unix-like OSes, we can and we do prompt on the controlling tty even if stdout and stderr are redirected to files or /dev/null. > > On a Mac using Tunnelblick (which uses the management interface with > management-query-passwords enabled), if the auth-user-pass file > contains only the password (and a LF), then the following occurs: > > neither stdin nor stderr are a tty device and you have neither a > controlling tty nor systemd - can't ask for 'Enter Auth Password:'. > If you used --daemon, you need to use --askpass to make > passphrase-protected keys work, and you can not use --auth-nocache. > Exiting due to fatal error In those cases it looks obviously wrong to use auth-file with username only, and I would consider that a user error. The purpose of my patch was to handle only some naive usages where the user expects the console prompt to get automatically directed to the GUI. Indeed, that does happen (from user's POV) for all cases except user-pass with only username in a file. But I agree, we should do something like this for other GUIs such as tunnelblick too. > > Note: Tunnelblick uses the "--log" option to redirect output to a > file. I am assuming that's what is meant by "stdout is redirected to a > log file". Yes, that's right. However, that logic wont be proper on OS-X, would it? Command line users who use --log can still see password prompt on /dev/tty. We'll be breaking that behaviour. I considered checking for env vars like IV_UI_VER set by the UI client, but that's not readily accessible from auth_user_pass_cr() call. Alternatives like checking whether /dev/tty can be opened and/or systemd is available didn't appeal to me. If at all, that would have to be a separate patch. Any suggestions? Selva
Hi, On Sun, Mar 29, 2020 at 7:58 PM Selva Nair <selva.nair@gmail.com> wrote: > > Hi, > > On Sun, Mar 29, 2020 at 7:13 PM Jonathan K. Bullard <jkbullard@gmail.com> wrote: <snip> > > On a Mac using Tunnelblick (which uses the management interface with > > management-query-passwords enabled), if the auth-user-pass file > > contains only the password (and a LF), then the following occurs: > > > > neither stdin nor stderr are a tty device and you have neither a > > controlling tty nor systemd - can't ask for 'Enter Auth Password:'. > > If you used --daemon, you need to use --askpass to make > > passphrase-protected keys work, and you can not use --auth-nocache. > > Exiting due to fatal error > > In those cases it looks obviously wrong to use auth-file with username > only, and I would consider that a user error. The purpose of > my patch was to handle only some naive usages where the user > expects the console prompt to get automatically directed to the GUI. > Indeed, that does happen (from user's POV) for all cases except user-pass > with only username in a file. > > But I agree, we should do something like this for other GUIs such as > tunnelblick too. > > > > > Note: Tunnelblick uses the "--log" option to redirect output to a > > file. I am assuming that's what is meant by "stdout is redirected to a > > log file". > > Yes, that's right. However, that logic wont be proper on OS-X, would it? > Command line users who use --log can still see password > prompt on /dev/tty. We'll be breaking that behaviour. If the OS X command line user was using --management-query-passwords (as Tunnelblick does), they wouldn't see the password prompt on /dev/tty, would they? Your patch checks for that, so wouldn't you only need to change && defined(_WIN32) to something like && (defined(_WIN32) || TARGET_OSX) (and add OS X to the comment at the start of the patch).
Hi, On Sun, Mar 29, 2020 at 07:58:15PM -0400, Selva Nair wrote: > Yes, that's right. However, that logic wont be proper on OS-X, would it? > Command line users who use --log can still see password > prompt on /dev/tty. We'll be breaking that behaviour. > > I considered checking for env vars like IV_UI_VER set by the UI > client, but that's not readily accessible from auth_user_pass_cr() > call. Alternatives like checking whether /dev/tty can be opened and/or > systemd is available didn't appeal to me. If at all, that would have > to be a separate patch. Not sure if the case "there is an active management client, and --management-query-passwords is set, but we *could* ask on /dev/tty" is really worth considering. (There might be cases where the management interface is not used for password prompting, in which case /dev/tty is the way to go). Not sure I'd worry too much about systemd here - as far as I understand, this is somewhat orthogonal to "management interface". So if run from systemd, and querying via systemd, you have no management client connected. Am I making sense? It's monday morning, halfway through my first cup of tea :-) gert
Hi, On Mon, Mar 30, 2020 at 2:07 AM Gert Doering <gert@greenie.muc.de> wrote: > > Hi, > > On Sun, Mar 29, 2020 at 07:58:15PM -0400, Selva Nair wrote: > > Yes, that's right. However, that logic wont be proper on OS-X, would it? > > Command line users who use --log can still see password > > prompt on /dev/tty. We'll be breaking that behaviour. > > > > I considered checking for env vars like IV_UI_VER set by the UI > > client, but that's not readily accessible from auth_user_pass_cr() > > call. Alternatives like checking whether /dev/tty can be opened and/or > > systemd is available didn't appeal to me. If at all, that would have > > to be a separate patch. > > Not sure if the case "there is an active management client, and > --management-query-passwords is set, but we *could* ask on /dev/tty" > is really worth considering. I agree, but the problem is that we currently do that when auth-file has username but no password. Current behaviour: if auth-file is set always read from it else query management if management-query-passwords is set etc. else ask on /dev/tty or windows console with a quirk that if auth-file has username but no password, ask on /dev/tty or console, not the management ever.. (ignoring systemd which just replaces the query on console). I'm all for changing the latter behaviour to query the management if possible, /dev/tty otherwise. But that's a regression and some may complain. Jonathan K. Bullard <jkbullard@gmail.com> wrote: > > If the OS X command line user was using --management-query-passwords > (as Tunnelblick does), they wouldn't see the password prompt on > /dev/tty, would they? In case of auth-file missing password, they would see it on /dev/tty on linux, and I would guess on OSX as well, but I've not checked. Selva <div dir="ltr">Hi,<br><br>On Mon, Mar 30, 2020 at 2:07 AM Gert Doering <<a href="mailto:gert@greenie.muc.de">gert@greenie.muc.de</a>> wrote:<br>><br>> Hi,<br>><br>> On Sun, Mar 29, 2020 at 07:58:15PM -0400, Selva Nair wrote:<br>> > Yes, that's right. However, that logic wont be proper on OS-X, would it?<br>> > Command line users who use --log can still see password<br>> > prompt on /dev/tty. We'll be breaking that behaviour.<br>> ><br>> > I considered checking for env vars like IV_UI_VER set by the UI<br>> > client, but that's not readily accessible from auth_user_pass_cr()<br>> > call. Alternatives like checking whether /dev/tty can be opened and/or<br>> > systemd is available didn't appeal to me. If at all, that would have<br>> > to be a separate patch.<br>><br>> Not sure if the case "there is an active management client, and<br>> --management-query-passwords is set, but we *could* ask on /dev/tty"<br>> is really worth considering.<br><br>I agree, but the problem is that we currently do that when auth-file has<div>username but no password.<br><br>Current behaviour:<br> if auth-file is set always read from it<br> else query management if management-query-passwords is set etc.<br> else ask on /dev/tty or windows console<br>with a quirk that if auth-file has username but no password, ask on /dev/tty<br>or console, not the management ever..</div><div><br>(ignoring systemd which just replaces the query on console).<br><br>I'm all for changing the latter behaviour to query the management if possible,<br>/dev/tty otherwise. But that's a regression and some may complain.<br><br>Jonathan K. Bullard <<a href="mailto:jkbullard@gmail.com">jkbullard@gmail.com</a>> wrote:<br>><br>> If the OS X command line user was using --management-query-passwords<br>> (as Tunnelblick does), they wouldn't see the password prompt on<br>> /dev/tty, would they?<br><div><div><br></div><div>In case of auth-file missing password, they would see it on /dev/tty</div><div>on linux, and I would guess on OSX as well, but I've not checked.</div><div> <br></div><blockquote style="margin:0 0 0 40px;border:none;padding:0px"></blockquote><blockquote style="margin:0 0 0 40px;border:none;padding:0px"></blockquote><blockquote style="margin:0 0 0 40px;border:none;padding:0px"></blockquote><blockquote style="margin:0 0 0 40px;border:none;padding:0px"></blockquote><blockquote style="margin:0 0 0 40px;border:none;padding:0px"></blockquote><blockquote style="margin:0 0 0 40px;border:none;padding:0px"></blockquote>Selva<br></div></div></div>
Hi, On Mon, Mar 30, 2020 at 11:12 AM Selva Nair <selva.nair@gmail.com> wrote: > Jonathan K. Bullard <jkbullard@gmail.com> wrote: > > > > If the OS X command line user was using --management-query-passwords > > (as Tunnelblick does), they wouldn't see the password prompt on > > /dev/tty, would they? > > In case of auth-file missing password, they would see it on /dev/tty > on linux, and I would guess on OSX as well, but I've not checked. The password prompt appears on /dev/tty on OS X only if --daemon is not used. If --daemon and --management-query-passwords are used but --askpass is not (whether or not --auth-nocache is also used), which is typical for a Tunnelblick configuration on OS X, the following appears in the log: neither stdin nor stderr are a tty device and you have neither a controlling tty nor systemd - can't ask for 'Enter Auth Password:'. If you used --daemon, you need to use --askpass to make passphrase-protected keys work, and you can not use --auth-nocache. Exiting due to fatal error if --daemon, --management-query-passwords, and --askpass are all used (whether or not --auth-nocache is used), you get: Need password(s) from management interface, waiting... If Windows GUI uses --daemon, that could be an additional requirement that would work for Tunnelblick and OS X, which would mean one less incompatibility between Windows and OS X. Or it could test for Windows || (OS X && --daemon). Best regards, Jon Bullard
Hi, On Mon, Mar 30, 2020 at 12:11 PM Jonathan K. Bullard <jkbullard@gmail.com> wrote: > Hi, > > On Mon, Mar 30, 2020 at 11:12 AM Selva Nair <selva.nair@gmail.com> wrote: > > Jonathan K. Bullard <jkbullard@gmail.com> wrote: > > > > > > If the OS X command line user was using --management-query-passwords > > > (as Tunnelblick does), they wouldn't see the password prompt on > > > /dev/tty, would they? > > > > In case of auth-file missing password, they would see it on /dev/tty > > on linux, and I would guess on OSX as well, but I've not checked. > > The password prompt appears on /dev/tty on OS X only if --daemon is not > used. > > If --daemon and --management-query-passwords are used but --askpass is > not (whether or not --auth-nocache is also used), which is typical for > a Tunnelblick configuration on OS X, the following appears in the log: > > neither stdin nor stderr are a tty device and you have neither a > controlling tty nor systemd - can't ask for 'Enter Auth > Password:'. > If you used --daemon, you need to use --askpass to make > passphrase-protected keys work, and you can not use > --auth-nocache. > Exiting due to fatal error > > if --daemon, --management-query-passwords, and --askpass are all used > (whether or not --auth-nocache is used), you get: > > Need password(s) from management interface, waiting... > > If Windows GUI uses --daemon, that could be an additional requirement > that would work for Tunnelblick and OS X, which would mean one less > incompatibility between Windows and OS X. > --daemon is a unix/linux option (not supported on Windows) and after deamonizing there is no controlling tty leading to the behaviour you mention above. I think that's documented. > Or it could test for Windows || (OS X && --daemon). > Personally I would prefer to enable this code for all platforms although its a minor regression. That is, if management-query-passwords is enabled and auth file is missing password, query the management, not on console irrespective of other options and OS. If that's acceptable, I'll submit a v2. Selva > Best regards, > > Jon Bullard > <div dir="ltr"><div>Hi,</div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Mon, Mar 30, 2020 at 12:11 PM Jonathan K. Bullard <<a href="mailto:jkbullard@gmail.com">jkbullard@gmail.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">Hi,<br> <br> On Mon, Mar 30, 2020 at 11:12 AM Selva Nair <<a href="mailto:selva.nair@gmail.com" target="_blank">selva.nair@gmail.com</a>> wrote:<br> > Jonathan K. Bullard <<a href="mailto:jkbullard@gmail.com" target="_blank">jkbullard@gmail.com</a>> wrote:<br> > ><br> > > If the OS X command line user was using --management-query-passwords<br> > > (as Tunnelblick does), they wouldn't see the password prompt on<br> > > /dev/tty, would they?<br> ><br> > In case of auth-file missing password, they would see it on /dev/tty<br> > on linux, and I would guess on OSX as well, but I've not checked.<br> <br> The password prompt appears on /dev/tty on OS X only if --daemon is not used.<br> <br> If --daemon and --management-query-passwords are used but --askpass is<br> not (whether or not --auth-nocache is also used), which is typical for<br> a Tunnelblick configuration on OS X, the following appears in the log:<br> <br> neither stdin nor stderr are a tty device and you have neither a<br> controlling tty nor systemd - can't ask for 'Enter Auth Password:'.<br> If you used --daemon, you need to use --askpass to make<br> passphrase-protected keys work, and you can not use<br> --auth-nocache.<br> Exiting due to fatal error<br> <br> if --daemon, --management-query-passwords, and --askpass are all used<br> (whether or not --auth-nocache is used), you get:<br> <br> Need password(s) from management interface, waiting...<br> <br> If Windows GUI uses --daemon, that could be an additional requirement<br> that would work for Tunnelblick and OS X, which would mean one less<br> incompatibility between Windows and OS X.<br></blockquote><div><br></div><div>--daemon is a unix/linux option (not supported on Windows) and after</div><div>deamonizing there is no controlling tty leading to the behaviour you mention</div><div>above. I think that's documented.</div><div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"> <br> Or it could test for Windows || (OS X && --daemon).<br></blockquote><div><br></div><div>Personally I would prefer to enable this code for all platforms although</div><div>its a minor regression.</div><div><br></div><div>That is, if management-query-passwords is enabled and auth file is</div><div>missing password, query the management, not on console irrespective</div><div>of other options and OS. If that's acceptable, I'll submit a v2.</div><div><br></div><div>Selva</div><div><br></div><div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"> <br> Best regards,<br> <br> Jon Bullard<br> </blockquote></div></div>
Hi, On Mon, Mar 30, 2020 at 12:29:44PM -0400, Selva Nair wrote: > Personally I would prefer to enable this code for all platforms although > its a minor regression. > > That is, if management-query-passwords is enabled and auth file is > missing password, query the management, not on console irrespective > of other options and OS. If that's acceptable, I'll submit a v2. I find this reasonable, and can't really think of a situation where the current behaviour might be desirable. So, yes, changing user-visible behaviour, but in a way that avoids one particular hard-to-explain (or even document) corner case. For master = 2.5, I'd definitely do this. For 2.4, I'm not totally sure, but this is something we can discuss in the community meeting if people have a strong opposing opinion (*I* consider the current behaviour to be a bug, and the change a bugfix). gert
On Mon, Mar 30, 2020 at 12:30 PM Selva Nair <selva.nair@gmail.com> wrote: > That is, if management-query-passwords is enabled and auth file is > missing password, query the management, not on console irrespective > of other options and OS. If that's acceptable, I'll submit a v2. That's fine with me (and Tunnelblick), thanks. Best regards, Jon Bullard
diff --git a/src/openvpn/error.c b/src/openvpn/error.c index ad4f0ef..8ce6873 100644 --- a/src/openvpn/error.c +++ b/src/openvpn/error.c @@ -190,6 +190,15 @@ errors_to_stderr(void) } /* + * Return true if stdout is redirected to log file + */ +bool +is_stdout_redirected(void) +{ + return std_redir; +} + +/* * Return a file to print messages to before syslog is opened. */ FILE * diff --git a/src/openvpn/error.h b/src/openvpn/error.h index eaedf17..5078f6a 100644 --- a/src/openvpn/error.h +++ b/src/openvpn/error.h @@ -398,6 +398,9 @@ nonfatal(const unsigned int err) return err & M_FATAL ? (err ^ M_FATAL) | M_NONFATAL : err; } +/** Return true if stdout is redirected to log file */ +bool is_stdout_redirected(void); + #include "errlevel.h" #endif /* ifndef ERROR_H */ diff --git a/src/openvpn/misc.c b/src/openvpn/misc.c index 0d5ac30..02afd98 100644 --- a/src/openvpn/misc.c +++ b/src/openvpn/misc.c @@ -261,6 +261,23 @@ get_user_pass_cr(struct user_pass *up, { strncpy(up->password, password_buf, USER_PASS_LEN); } + /* The auth-file does not have the password: if we are on Windows + * and stdout has been redirected to log file, try to get both username + * and password from the management. + * Otherwise set to read password from console. + */ +#if defined(ENABLE_MANAGEMENT) && defined(_WIN32) + else if (is_stdout_redirected() + && management + && (flags & GET_USER_PASS_MANAGEMENT) + && management_query_user_pass_enabled(management)) + { + if (!auth_user_pass_mgmt(up, prefix, flags, auth_challenge)) + { + return false; + } + } +#endif else { password_from_stdin = 1;