Re: [Openvpn-devel] [PATCH 2/2] When auth-user-pass file has no password, query the management

[Gert Doering]

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
-- 
"If was one thing all people took for granted, was conviction that if you 
 feed honest figures into a computer, honest figures come out. Never doubted 
 it myself till I met a computer with a sense of humor."
 Robert A. Heinlein, The Moon is a Harsh Mistress

Gert Doering - Munich, Germany g...@greenie.muc.de


signature.asc
Description: PGP signature
___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel

Re: [Openvpn-devel] [PATCH] Reformat all source files

[Gert Doering]

Hi,

On Sun, Mar 29, 2020 at 11:54:36PM +0200, David Sommerseth wrote:
> > Applying: Reformat all source files
> > error: patch failed: src/openvpn/tun.c:3418
> > error: src/openvpn/tun.c: patch does not apply
> > Patch failed at 0001 Reformat all source files
> 
> Ahh, sorry ... I did the reviews based on Arne's github tree [1], so I could
> compile test them.  And I trust him enough to not do any stupid stunts in his
> tree.  His icsopenvpn branch has all of the stuff I've reviewed; that branch
> rebased against our latest master without any hickups.
> 
> Unfortunately git apply is (rightfully) super picky about conflicts.
> 
> Not sure now what would be the best approach forward. Picking the commit
> contents from a rebased icsopenvpn branch would be one way (I can provide
> commitish references I reviewed, if needed).  Another approach is for Arne to
> resend rebased patches to ML.

Well, our current defined process is "we review, test and merge *exactly*
what is on the list".

So, ACKing list patches based on "some other tree" is doubtful at best
(for initial review and discussion, fine, but for the final ACK?), and
"have something on the list and then merge something else" is also
clearly violating the "everything must be transparent, and no code changes
compared to what is archived in a public archive".


We can change this, of course, but even then it needs to be fairly 
transparent what was exactly was ACKed and merged (and what, if anything,
was changed between ACK and merge).

It would make my life easier to be able to do minor code changes on
the fly, or do larger adjustments like for the argv<->tun.c adjustments,
but we need to be very clear that we change the process.

gert
-- 
"If was one thing all people took for granted, was conviction that if you 
 feed honest figures into a computer, honest figures come out. Never doubted 
 it myself till I met a computer with a sense of humor."
 Robert A. Heinlein, The Moon is a Harsh Mistress

Gert Doering - Munich, Germany g...@greenie.muc.de


signature.asc
Description: PGP signature
___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel

Re: [Openvpn-devel] [PATCH 2/2] When auth-user-pass file has no password, query the management

[Jonathan K. Bullard]

Hi,

On Sun, Mar 29, 2020 at 7:58 PM Selva Nair  wrote:
>
> Hi,
>
> On Sun, Mar 29, 2020 at 7:13 PM Jonathan K. Bullard  
> wrote:

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


___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel

Re: [Openvpn-devel] [PATCH 2/2] When auth-user-pass file has no password, query the management

[Selva Nair]

Hi,

On Sun, Mar 29, 2020 at 7:13 PM Jonathan K. Bullard  wrote:
>
> Hi,
>
> On Sun, Mar 29, 2020 at 4:34 PM  wrote:
> >
> > From: Selva Nair 
> >
> > 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


___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel

Re: [Openvpn-devel] [PATCH 2/2] When auth-user-pass file has no password, query the management

[Jonathan K. Bullard]

Hi,

On Sun, Mar 29, 2020 at 4:34 PM  wrote:
>
> From: Selva Nair 
>
> 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


___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel

Re: [Openvpn-devel] [Openvpn-users] Removing --disable-server option from OpenVPN

[David Sommerseth]

On 28/02/2020 20:18, Radu Hociung wrote:
> I would recommend that rather than removing the useful bit of
> documentation that is P2MP_SERVER, the developers consider:
> 
> 1. Restructure the source tree to split the src/openvpn code into
>src/{common,client,server}
> 
> 2. Remove the configure --disable-server option, but add Makefile rules
>to build separate client, server and combined binaries, while
>also enabling distro maintainers to package openvpn-server and
>openvpn-client separately, from the same tarball. The combined
>binary can be used to build drop-in replacement packages.
> 
> 3. Improve the test suite to verify the inter-operability of the
>3 binaries.
> 
> 4. Separate the manpage into a server and a client edition.

I've not had the capacity to go through your detailed mail too closely.  But I
consider these 4 points to be a reasonable TL;DR version.

Yes, this all sounds lovely.  But I wonder who will have time to work on such
work?  This work will require quite some efforts to avoid duplicating too much
of what already exists in options.c and openvpn.c; which would be the starting
point for such a task.

In addition, such a change has a big possibility to break a lot of existing
configurations via package upgrades - as in this case the unified openvpn will
no longer exists but be split into two different binaries.

To be honest, I think it makes more sense to put efforts into the OpenVPN 3
code base, which already is ready for such a split.  On top of getting such a
split in place, you would get a higher performance as the OpenVPN 3 code is
more efficient than OpenVPN 2.x.  But OpenVPN 3 does not support all the use
cases OpenVPN 2.x does.

I hate to be so pessimistic, but I doubt any of the current core OpeVPN
community developers will have capacity to get such a job done in the near
future.  We already struggle to get the current open patches for review
processed, in addition to get OpenVPN 2.5 ready.  More people helping out
reviewing patches thoroughly would certainly help us move forward.

Btw, in regards to the man page.  On my plate, in time for the 2.5 release, I
will redo the formatting of the man page from ?roff format to some .rst files
(and parse that to ?roff format during packaging).  This is just the first
step to get the man page in an easier editable format than what it is today.


-- 
kind regards,

David Sommerseth
OpenVPN Inc




signature.asc
Description: OpenPGP digital signature
___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel

Re: [Openvpn-devel] [PATCH] Reformat all source files

[David Sommerseth]

On 28/03/2020 12:33, Gert Doering wrote:
> Hi,
> 
> On Fri, Mar 27, 2020 at 04:24:00PM +0100, David Sommerseth wrote:
>> On 16/11/2019 11:28, Arne Schwabe wrote:
>>> Over time some patches slipped in that were not 100% complient to uncrustify
>>> This rerun fixes those issues
> [..]
>> Only done quick code review and RHEL-7 build.  Changes looks reasonable and 
>> is
>> by far closer to what I would expect our coding style to look like.
>>
>> Acked-By: David Sommerseth 
> 
> ... but it does not apply to current git master...?
> 
> Applying: Reformat all source files
> error: patch failed: src/openvpn/tun.c:3418
> error: src/openvpn/tun.c: patch does not apply
> Patch failed at 0001 Reformat all source files

Ahh, sorry ... I did the reviews based on Arne's github tree [1], so I could
compile test them.  And I trust him enough to not do any stupid stunts in his
tree.  His icsopenvpn branch has all of the stuff I've reviewed; that branch
rebased against our latest master without any hickups.

Unfortunately git apply is (rightfully) super picky about conflicts.

Not sure now what would be the best approach forward. Picking the commit
contents from a rebased icsopenvpn branch would be one way (I can provide
commitish references I reviewed, if needed).  Another approach is for Arne to
resend rebased patches to ML.

As we have several patch series lingering in our mail queue, we should look at
alternatives pulling in patches which would go smoother and allow a better
flexibility for all of us (submitter, reviewer and committer) while keeping
the review and commit process decentralized.  But that's a longer discussion
we need to take in a different place than this thread.


[1] 


-- 
kind regards,

David Sommerseth
OpenVPN Inc




signature.asc
Description: OpenPGP digital signature
___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel

Re: [Openvpn-devel] [PATCH] Document some limitations of --auth-user-pass

[Selva Nair]

Hi,

On Tue, Mar 17, 2020 at 6:25 AM Gert Doering  wrote:
>
> Hi,
>
> On Tue, Mar 17, 2020 at 11:06:53AM +0100, David Sommerseth wrote:
> > On 16/03/2020 14:48, Selva Nair wrote:
> > [...snip...]
> > >> I would just rephrase it to say:
> > >>
> > >>   OpenVPN GUI v11 and newer uses its own internal username/password 
> > >> storage
> > >>   independent of the --auth-user-pass file provided.  The file argument 
> > >> is
> > >>   ignored on such installations.
> > >
> > > I wish it behaved  like that. Unfortunately the file argument is not
> > > ignored in such cases. If the file has only username, openvpn.exe
> > > reads it from the file and then fails to prompt for password as there
> > > is no console available.
> >
> > Ouch ... that is a pointless misbehavior.  Lets try to fix that.
>
> Have you recovered from your latest adventures in "password query code
> in OpenVPN" already? :-)
>
> Not sure if the management commands permit the "we have a username but
> no password" flow today... Arne, Selva?
>
> But yes, this needs to be either a clear error, or "work correctly"
>
> > > I propose to change this behaviour to: if --management-query-passwords
> > > is set (which the GUI does), ignore the file given in auth-user-pass
> > > and prompt both username and password from management. I think its
> > > only logical for a later option (in this case the one set by the GUI)
> > > to override a previous one. Anyway we do already ignore it if the file
> > > is "stdin".
> >
> > Agreed!
>
> No, as this will break working configs *if* both username + password
> are in the file (did we ever merge the "inline auth-user-pass" patch?).

See the patch in mail for what looks like an acceptable solution to me.


Selva


___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel

[Openvpn-devel] [PATCH 2/2] When auth-user-pass file has no password, query the management

[selva . nair]

From: Selva Nair 

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.

Trac # 757

Signed-off-by: Selva Nair 
---
 src/openvpn/error.c |  9 +
 src/openvpn/error.h |  3 +++
 src/openvpn/misc.c  | 17 +
 3 files changed, 29 insertions(+)

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;
-- 
2.1.4



___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel

[Openvpn-devel] [PATCH 1/2] Move querying username/password from management to a function

[selva . nair]

From: Selva Nair 

This helps the next patch. No functionality changes, only
refactoring.

Signed-off-by: Selva Nair 
---
 src/openvpn/misc.c | 54 ++
 1 file changed, 34 insertions(+), 20 deletions(-)

diff --git a/src/openvpn/misc.c b/src/openvpn/misc.c
index 1931149..0d5ac30 100644
--- a/src/openvpn/misc.c
+++ b/src/openvpn/misc.c
@@ -116,6 +116,38 @@ hostname_randomize(const char *hostname, struct gc_arena 
*gc)
 #undef n_rnd_bytes
 }
 
+#ifdef ENABLE_MANAGEMENT
+/* Get username/password from the management interface */
+static bool
+auth_user_pass_mgmt(struct user_pass *up, const char *prefix, const unsigned 
int flags,
+const char *auth_challenge)
+{
+const char *sc = NULL;
+
+if (flags & GET_USER_PASS_PREVIOUS_CREDS_FAILED)
+{
+management_auth_failure(management, prefix, "previous auth credentials 
failed");
+}
+
+if (auth_challenge && (flags & GET_USER_PASS_STATIC_CHALLENGE))
+{
+sc = auth_challenge;
+}
+if (!management_query_user_pass(management, up, prefix, flags, sc))
+{
+if ((flags & GET_USER_PASS_NOFATAL) != 0)
+{
+return false;
+}
+else
+{
+msg(M_FATAL, "ERROR: could not read %s username/password/ok/string 
from management interface", prefix);
+}
+}
+return true;
+}
+#endif
+
 /*
  * Get and store a username/password
  */
@@ -149,28 +181,10 @@ get_user_pass_cr(struct user_pass *up,
 && (!from_authfile && (flags & GET_USER_PASS_MANAGEMENT))
 && management_query_user_pass_enabled(management))
 {
-const char *sc = NULL;
 response_from_stdin = false;
-
-if (flags & GET_USER_PASS_PREVIOUS_CREDS_FAILED)
+if (!auth_user_pass_mgmt(up, prefix, flags, auth_challenge))
 {
-management_auth_failure(management, prefix, "previous auth 
credentials failed");
-}
-
-if (auth_challenge && (flags & GET_USER_PASS_STATIC_CHALLENGE))
-{
-sc = auth_challenge;
-}
-if (!management_query_user_pass(management, up, prefix, flags, sc))
-{
-if ((flags & GET_USER_PASS_NOFATAL) != 0)
-{
-return false;
-}
-else
-{
-msg(M_FATAL, "ERROR: could not read %s 
username/password/ok/string from management interface", prefix);
-}
+return false;
 }
 }
 else
-- 
2.1.4



___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel