Re: [Openvpn-devel] [PATCH] [PATCH v5] Insert client connection data into PAM environment
Hi,
On Mon, Mar 30, 2020 at 8:59 AM Paolo Cerrito wrote:
> 1) so remote was set to the maxlenght of ipv6 address defined into
> arpa/inet.h + 1 for string terminator
>
> 2) I refactored the call to get_env to take first ipv6 address, then
>only if it is NULL, i make a call for ipv4
> ---
> src/plugins/auth-pam/auth-pam.c | 8 +---
> 1 file changed, 5 insertions(+), 3 deletions(-)
>
> diff --git a/src/plugins/auth-pam/auth-pam.c
> b/src/plugins/auth-pam/auth-pam.c
> index ae0d495a..cd91a33c 100644
> --- a/src/plugins/auth-pam/auth-pam.c
> +++ b/src/plugins/auth-pam/auth-pam.c
> @@ -48,7 +48,7 @@
> #include
> #include
> #include "utils.h"
> -
> +#include
#include
>
> #define DEBUG(verb) ((verb) >= 4)
> @@ -115,7 +115,7 @@ struct user_pass {
> char password[128];
> char common_name[128];
> char response[128];
> -char remote[46]; //46 as ipv6 form n:n:n:n:n:n:d.d.d.d and +
> terminator \0
> +char remote[INET6_ADDRSTRLEN+1]; //INET6_ADDRSTRLEN is the lenght of
> ipv6 + terminator \0
>
INET6_ADDRSTRLEN = 46 already includes space for nul termination
More importantly, you have to provide a single updated patch
preferably with version indicated in the subject and sent out with
--in-reply-to referring to the previous version.
Submitting incremental pieces of fixup commits doesn't help. And please
wait for review before making changes unless correcting a critical error.
Thanks,
Selva
___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel
Re: [Openvpn-devel] [PATCH 11/12] openvpnmsica: Merge FindTUNTAPAdapters into FindSystemInfo
Hi Lev,
I'm struggling with family duties now that schools are closed. This makes it
hard to find any time for computers.
Nevertheless, should find_adapters() fail for some reason, it is not critical
to bail out of FindSystemInfo() custom action.
The find_adapters() itself already displays a resumable error message (MSI also
writes it to the log) on all of the error return paths:
- tap_list_adapters() calls msg(M_NONFATAL...) on error returns
- other returns have msg(M_NONFATAL...)
Mind that msg() is using MSI error messaging
[https://github.com/rozmansi/openvpn/blob/feature/msi/src/openvpnmsica/dllmain.c#L108].
MsiProcessMessage(s->hInstall, INSTALLMESSAGE_ERROR, hRecordProg); will popup
an error dialog in interactive MSI sessions, and write error message to the log
in interactive and non-interactive sessions.
To summarize: the return value of find_adapters() call is ignored on purpose.
Regards,
Simon
-Original Message-
From: Lev Stipakov
Date: Tuesday, 24 March 2020 at 13:07
To: Simon Rozman
Cc: "openvpn-devel@lists.sourceforge.net"
Subject: Re: [Openvpn-devel] [PATCH 11/12] openvpnmsica: Merge
FindTUNTAPAdapters into FindSystemInfo
Hi,
Compiled with msvc, smoke-tested with rundll32.
One thing:
> +set_openvpnserv_state(hInstall);
> +find_adapters(
> +hInstall,
> +TEXT("root\\") TEXT(TAP_WIN_COMPONENT_ID),
> +TEXT("TAPWINDOWS6ADAPTERS"),
> +TEXT("ACTIVETAPWINDOWS6ADAPTERS"));
Both methods return error codes which we ignore.
smime.p7s
Description: S/MIME cryptographic signature
___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel
[Openvpn-devel] [PATCH v2 1/2] Move querying username/password from management to a function
From: Selva Nair
This helps the next patch. No functionality changes, only
refactoring.
Signed-off-by: Selva Nair
---
No changes from v1
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
[Openvpn-devel] [PATCH v2 2/2] When auth-user-pass file has no password, query the management
From: Selva Nair
When only username is found in the file, redirect the auth-user-pass
query to the management if management-query-passwords is enabled.
Otherwise the user is prompted on console, if available, as before.
This changes the behaviour for those who run from the command line,
with --management-query-passwords, but still expect the prompt
on the console.
Note that the management will prompt for both username and password
ignoring the username read from the file. As most GUIs can save the
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 when console is not available
(windows GUI, tunnelblick etc.) or when the log is redirected
to a file on Windows (for some reason prompt goes to the log file).
Trac # 757
Signed-off-by: Selva Nair
---
v2: Following discussions with Jonathan and Gert, removed the dependence
on stdout redirection and applied to all platforms.
src/openvpn/misc.c | 16
1 file changed, 16 insertions(+)
diff --git a/src/openvpn/misc.c b/src/openvpn/misc.c
index 0d5ac30..546cd71 100644
--- a/src/openvpn/misc.c
+++ b/src/openvpn/misc.c
@@ -261,6 +261,22 @@ get_user_pass_cr(struct user_pass *up,
{
strncpy(up->password, password_buf, USER_PASS_LEN);
}
+/* The auth-file does not have the password: get both username
+ * and password from the management if possible.
+ * Otherwise set to read password from console.
+ */
+#if defined(ENABLE_MANAGEMENT)
+else if (management
+ && (flags & GET_USER_PASS_MANAGEMENT)
+ && management_query_user_pass_enabled(management))
+{
+msg(D_LOW, "No password found in %s authfile '%s'. Querying
the management", prefix, auth_file);
+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
Re: [Openvpn-devel] [PATCH 2/2] When auth-user-pass file has no password, query the management
On Mon, Mar 30, 2020 at 12:30 PM Selva Nair 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 ___ 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
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 -- "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
Hi, On Mon, Mar 30, 2020 at 12:11 PM Jonathan K. Bullard wrote: > Hi, > > On Mon, Mar 30, 2020 at 11:12 AM Selva Nair wrote: > > Jonathan K. Bullard 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 > ___ 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
Hi, On Mon, Mar 30, 2020 at 11:12 AM Selva Nair wrote: > Jonathan K. Bullard 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 ___ 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
Hi, On Mon, Mar 30, 2020 at 2:07 AM Gert Doering 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 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 ___ Openvpn-devel mailing list Openvpn-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/openvpn-devel
[Openvpn-devel] [PATCH] [PATCH v5] Insert client connection data into PAM environment
1) so remote was set to the maxlenght of ipv6 address defined into
arpa/inet.h + 1 for string terminator
2) I refactored the call to get_env to take first ipv6 address, then
only if it is NULL, i make a call for ipv4
---
src/plugins/auth-pam/auth-pam.c | 8 +---
1 file changed, 5 insertions(+), 3 deletions(-)
diff --git a/src/plugins/auth-pam/auth-pam.c b/src/plugins/auth-pam/auth-pam.c
index ae0d495a..cd91a33c 100644
--- a/src/plugins/auth-pam/auth-pam.c
+++ b/src/plugins/auth-pam/auth-pam.c
@@ -48,7 +48,7 @@
#include
#include
#include "utils.h"
-
+#include
#include
#define DEBUG(verb) ((verb) >= 4)
@@ -115,7 +115,7 @@ struct user_pass {
char password[128];
char common_name[128];
char response[128];
-char remote[46]; //46 as ipv6 form n:n:n:n:n:n:d.d.d.d and + terminator \0
+char remote[INET6_ADDRSTRLEN+1]; //INET6_ADDRSTRLEN is the lenght of ipv6
+ terminator \0
const struct name_value_list *name_value_list;
};
@@ -518,12 +518,14 @@ openvpn_plugin_func_v1(openvpn_plugin_handle_t handle,
const int type, const cha
const char *username = get_env("username", envp);
const char *password = get_env("password", envp);
const char *common_name = get_env("common_name", envp) ?
get_env("common_name", envp) : "";
+
const char *remote = get_env("untrusted_ip6", envp);
if (remote == NULL){
- remote = get_env("untrusted_ip", envp); //try to take ipv4 if
not set ipv6
+ remote = get_env("untrusted_ip", envp); //if Null, try to take
ipv4 if not set ipv6
}
+
if (username && strlen(username) > 0 && password)
{
if (send_control(context->foreground_fd, COMMAND_VERIFY) == -1
--
2.26.0
___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel
[Openvpn-devel] [Openvpn-Devel] [PATCH v4] Insert client connection data into PAM environment
From: Paolo Cerrito
1) changed again remote lenght, we have to consider only the lenght op
ipv6 address into form :::::ddd.ddd.ddd.ddd not
the mask, so we have max lenght of 45 plus terminator.
2) refactored calls to get_env, now we make one call to take ipv6
address, the if it's NULL, we try to get ipv4 address.
---
src/plugins/auth-pam/auth-pam.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/src/plugins/auth-pam/auth-pam.c b/src/plugins/auth-pam/auth-pam.c
index b55a2ecd..ae0d495a 100644
--- a/src/plugins/auth-pam/auth-pam.c
+++ b/src/plugins/auth-pam/auth-pam.c
@@ -115,7 +115,7 @@ struct user_pass {
char password[128];
char common_name[128];
char response[128];
-char remote[51]; //51 as ipv6 form n:n:n:n:n:n:d.d.d.d/mask and
terminator+1
+char remote[46]; //46 as ipv6 form n:n:n:n:n:n:d.d.d.d and + terminator \0
const struct name_value_list *name_value_list;
};
--
2.26.0
___
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
Hi, On 30/03/2020 07:55, Gert Doering wrote: >> 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". > I totally agree with Gert here. > > 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). > IMHO we should not change the process - git repos can go, while emails remains in archives so it's possible to see the whole flow later in the future. IMHO the best course of action would be: 1. David reviewing the git branch 2. David privatelly talking to Arne and eventually saying "all patches look good! I consider them ready for merging" 3. Arne sends the rebased patches to the mailing list 4. David checks that the patches are still the same he reviewed 5. David ACKs the patches on the mailing list in a public way. I think this process would allow us to be super transparent and would allow everybody to chime in until the very last minute. The other upside is that point 1 and 2 can be repeated as much as needed until David is satisfied, without spamming the ml. my 2 cents. Cheers, -- Antonio Quartulli ___ Openvpn-devel mailing list Openvpn-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/openvpn-devel
