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

Message ID 20220914185937.31423-2-a@unstable.cc
State New
Headers show
Series [Openvpn-devel,1/2] auth-user-pass: add support for inline credentials | expand

Commit Message

Antonio Quartulli Sept. 14, 2022, 6:59 p.m. UTC
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(-)

Comments

Selva Nair Sept. 14, 2022, 7:26 p.m. UTC | #1
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
>
Antonio Quartulli Sept. 14, 2022, 7:30 p.m. UTC | #2
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?
Selva Nair Sept. 14, 2022, 7:40 p.m. UTC | #3
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
Antonio Quartulli Sept. 14, 2022, 7:49 p.m. UTC | #4
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

Patch

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.