[Openvpn-devel,2/2] get_user_pass_cr: get password from stdin if missing inline

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

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