Message ID | 20220914185937.31423-2-a@unstable.cc |
---|---|
State | Accepted |
Headers | show |
Series | [Openvpn-devel,1/2] auth-user-pass: add support for inline credentials | expand |
On Wed, Sep 14, 2022 at 3:02 PM Antonio Quartulli <a@unstable.cc> wrote: > Until now, when HTTP proxy user and password were specified inline, > it was assumed that both creds were specified. A missing password would > result in an empty password being stored. > > This behaviour is not ideal, as we want to allow the user to store the > username, but let the password be entered via stdin. > > This affects both http proxy and authentication inline'd creds. > > Signed-off-by: Antonio Quartulli <a@unstable.cc> > --- > Changes.rst | 4 +++- > src/openvpn/misc.c | 5 +++++ > 2 files changed, 8 insertions(+), 1 deletion(-) > > diff --git a/Changes.rst b/Changes.rst > index 2967533a..2daa97fb 100644 > --- a/Changes.rst > +++ b/Changes.rst > @@ -89,7 +89,9 @@ Data channel offloading with ovpn-dco > > Inline auth username and password > Username and password can now be specified inline in the > configuration file > - within the <auth-user-pass></auth-user-pass> tags. > + within the <auth-user-pass></auth-user-pass> tags. If the password is > + missing OpenVPN will prompt for input via stdin. This applies to > inline'd > + http-proxy-user-pass too. > > > Deprecated features > diff --git a/src/openvpn/misc.c b/src/openvpn/misc.c > index 07f6e202..50f7f975 100644 > --- a/src/openvpn/misc.c > +++ b/src/openvpn/misc.c > @@ -197,6 +197,11 @@ get_user_pass_cr(struct user_pass *up, > buf_parse(&buf, '\n', up->username, USER_PASS_LEN); > } > buf_parse(&buf, '\n', up->password, USER_PASS_LEN); > + > + if (strlen(up->password) == 0) > + { > + password_from_stdin = 1; > This works when stdin is available, but reading username from file and password from the management interface is still not possible. Currently, if --management-query-passwords and --auth-user-pass are used, the file must contain username and password (management i/f not queried). This patch allows username only files, but only if reading from stdin is possible. It may be a bit tricky to prompt the management interface in such cases, but if we are improving the UX, this is a good opportunity to do a better job. Selva > + } > } > /* > * Read from auth file unless this is a dynamic challenge request. > -- > 2.35.1 > > > > _______________________________________________ > Openvpn-devel mailing list > Openvpn-devel@lists.sourceforge.net > https://lists.sourceforge.net/lists/listinfo/openvpn-devel >
Hi, On 14/09/2022 21:26, Selva Nair wrote: > diff --git a/src/openvpn/misc.c b/src/openvpn/misc.c > index 07f6e202..50f7f975 100644 > --- a/src/openvpn/misc.c > +++ b/src/openvpn/misc.c > @@ -197,6 +197,11 @@ get_user_pass_cr(struct user_pass *up, > buf_parse(&buf, '\n', up->username, USER_PASS_LEN); > } > buf_parse(&buf, '\n', up->password, USER_PASS_LEN); > + > + if (strlen(up->password) == 0) > + { > + password_from_stdin = 1; > > > This works when stdin is available, but reading username from file and > password from the management interface is still not possible. Currently, > if --management-query-passwords and --auth-user-pass are used, the file > must contain username and password (management i/f not queried). This > patch allows username only files, but only if reading from stdin is > possible. Just to make sure I got your comment right: this patch is not allowing files to have no password (this is allowed already without this patch). This patch is only allowing having no password when doing inline credentials. Does your comment still apply? If the mgmt interface has troubles with querying the password, then it means we already have this problem without the patch, right?
On Wed, Sep 14, 2022 at 3:30 PM Antonio Quartulli <a@unstable.cc> wrote: > Hi, > > On 14/09/2022 21:26, Selva Nair wrote: > > diff --git a/src/openvpn/misc.c b/src/openvpn/misc.c > > index 07f6e202..50f7f975 100644 > > --- a/src/openvpn/misc.c > > +++ b/src/openvpn/misc.c > > @@ -197,6 +197,11 @@ get_user_pass_cr(struct user_pass *up, > > buf_parse(&buf, '\n', up->username, USER_PASS_LEN); > > } > > buf_parse(&buf, '\n', up->password, USER_PASS_LEN); > > + > > + if (strlen(up->password) == 0) > > + { > > + password_from_stdin = 1; > > > > > > This works when stdin is available, but reading username from file and > > password from the management interface is still not possible. Currently, > > if --management-query-passwords and --auth-user-pass are used, the file > > must contain username and password (management i/f not queried). This > > patch allows username only files, but only if reading from stdin is > > possible. > > Just to make sure I got your comment right: this patch is not allowing > files to have no password (this is allowed already without this patch). > This patch is only allowing having no password when doing inline > credentials. > > Does your comment still apply? > > If the mgmt interface has troubles with querying the password, then it > means we already have this problem without the patch, right? > Yes, on re-reading the patches, my comment was not totally accurate.. The issue is long-standing one with username/password in file --- if password is missing it's not prompted from the management interface even if management-query-passwords is in effect. Instead it's prompted from stdin which is generally not available when run from a GUI. So, yes, its not a fault of this patch, except that its committing the same "mistake" of not using the management interface instead of stdin as the fallback when "--management-query-passwords" is in effect. Selva
Hi, On 14/09/2022 21:40, Selva Nair wrote: > > On Wed, Sep 14, 2022 at 3:30 PM Antonio Quartulli <a@unstable.cc > <mailto:a@unstable.cc>> wrote: > > Hi, > > On 14/09/2022 21:26, Selva Nair wrote: > > diff --git a/src/openvpn/misc.c b/src/openvpn/misc.c > > index 07f6e202..50f7f975 100644 > > --- a/src/openvpn/misc.c > > +++ b/src/openvpn/misc.c > > @@ -197,6 +197,11 @@ get_user_pass_cr(struct user_pass *up, > > buf_parse(&buf, '\n', up->username, > USER_PASS_LEN); > > } > > buf_parse(&buf, '\n', up->password, USER_PASS_LEN); > > + > > + if (strlen(up->password) == 0) > > + { > > + password_from_stdin = 1; > > > > > > This works when stdin is available, but reading username from > file and > > password from the management interface is still not possible. > Currently, > > if --management-query-passwords and --auth-user-pass are used, > the file > > must contain username and password (management i/f not queried). > This > > patch allows username only files, but only if reading from stdin is > > possible. > > Just to make sure I got your comment right: this patch is not allowing > files to have no password (this is allowed already without this patch). > This patch is only allowing having no password when doing inline > credentials. > > Does your comment still apply? > > If the mgmt interface has troubles with querying the password, then it > means we already have this problem without the patch, right? > > > Yes, on re-reading the patches, my comment was not totally accurate.. > > The issue is long-standing one with username/password in file --- if > password is missing it's not prompted from the management interface even > if management-query-passwords is in effect. Instead it's prompted from > stdin which is generally not available when run from a GUI. > > So, yes, its not a fault of this patch, except that its committing the > same "mistake" of not using the management interface instead of stdin as > the fallback when "--management-query-passwords" is in effect. > Ok, thanks for clarifying. Now it all fits together. I'd say this patch is basically adding another case for which we need password_from_stdin. How password_from_stdin is consumed is something happened down below in misc.c. IMHO that would require a separate patch to fix that exact (mis)behaviour. Especially because it requires some extra shuffling of this function (that's my guess at first glance). But thanks for raising this point - I was not aware of this issue. Cheers, > Selva
Acked-by: Gert Doering <gert@greenie.muc.de> Amazingly trivial, as soon as one understands the intricacies of get_user_pass_cr() ;-) - of course I have tested this. Without the patch, <auth-user-pass> with no password will send an empty password, with the patch, it will query on stdin and things will succeed (hard to test in an automated way, though). I have not tested if this would query via systemd, as I don't have any systemd systems - but since this is using already-existing paths to query for password, I'd expect so. Now, to management interface - as Selva correctly remarked, all these "we have a username and now need to query for a password" cases should really work via management interface as well, otherwise the user experience (especially on windows) will be fairly poor. Inside auth_user_pass_cr() this should be doable with some reshuffling, and from the documentation I see that the managment interface already knows how to query "only for password". So maybe something like >PASSWORD:Need 'Auth' password user=$username (extend Need 'Auth' with a username) and the GUI could then present a user/password dialogue with a non-editable "user" field, or so... Your patch has been applied to the master branch. commit 39619b7fab213e9cadaa4a8b50b795ad63d9d91f Author: Antonio Quartulli Date: Wed Sep 14 20:59:37 2022 +0200 get_user_pass_cr: get password from stdin if missing inline Signed-off-by: Antonio Quartulli <a@unstable.cc> Acked-by: Gert Doering <gert@greenie.muc.de> Message-Id: <20220914185937.31423-2-a@unstable.cc> URL: https://www.mail-archive.com/openvpn-devel@lists.sourceforge.net/msg25215.html Signed-off-by: Gert Doering <gert@greenie.muc.de> -- kind regards, Gert Doering
diff --git a/Changes.rst b/Changes.rst index 2967533a..2daa97fb 100644 --- a/Changes.rst +++ b/Changes.rst @@ -89,7 +89,9 @@ Data channel offloading with ovpn-dco Inline auth username and password Username and password can now be specified inline in the configuration file - within the <auth-user-pass></auth-user-pass> tags. + within the <auth-user-pass></auth-user-pass> tags. If the password is + missing OpenVPN will prompt for input via stdin. This applies to inline'd + http-proxy-user-pass too. Deprecated features diff --git a/src/openvpn/misc.c b/src/openvpn/misc.c index 07f6e202..50f7f975 100644 --- a/src/openvpn/misc.c +++ b/src/openvpn/misc.c @@ -197,6 +197,11 @@ get_user_pass_cr(struct user_pass *up, buf_parse(&buf, '\n', up->username, USER_PASS_LEN); } buf_parse(&buf, '\n', up->password, USER_PASS_LEN); + + if (strlen(up->password) == 0) + { + password_from_stdin = 1; + } } /* * Read from auth file unless this is a dynamic challenge request.
Until now, when HTTP proxy user and password were specified inline, it was assumed that both creds were specified. A missing password would result in an empty password being stored. This behaviour is not ideal, as we want to allow the user to store the username, but let the password be entered via stdin. This affects both http proxy and authentication inline'd creds. Signed-off-by: Antonio Quartulli <a@unstable.cc> --- Changes.rst | 4 +++- src/openvpn/misc.c | 5 +++++ 2 files changed, 8 insertions(+), 1 deletion(-)