[Openvpn-devel] [PATCH v3] Swap the order of checks for validating interactive service user
From: Selva Nair
Check the config file location and command line options first
and membership in OpenVPNAdministrators group after that as
the latter could be a slow process for active directory users.
When connection to domain controllers is poor or unavailable, checking
the group membership is slow and causes timeouts in the GUI (Trac
1051). However, in cases where the config is in the global directory,
no group membership check should be required. The re-ordering here
avoids the redundant check in such cases.
In addition to this, its also proposed to improve the timeout handling
in the GUI, but this change is still useful as it should completely
eliminate the timeout issue for many users.
v3: Do not send error message to the client pipe from ValidateOptions().
Instead save the error and send it on only if user authorization also
fails. The error buffer size is increased to 512 wide chars as these
messages could get long in some cases and may get truncated otherwise.
Also see: https://github.com/OpenVPN/openvpn-gui/issues/332
Signed-off-by: Selva Nair
---
v2: Add missing closing parenthesis and improve the comment
above the edited chunk.
src/openvpnserv/interactive.c | 33 +++--
1 file changed, 19 insertions(+), 14 deletions(-)
diff --git a/src/openvpnserv/interactive.c b/src/openvpnserv/interactive.c
index 6e72a14..710f9c7 100644
--- a/src/openvpnserv/interactive.c
+++ b/src/openvpnserv/interactive.c
@@ -389,14 +389,13 @@ ReturnOpenvpnOutput(HANDLE pipe, HANDLE ovpn_output,
DWORD count, LPHANDLE event
/*
* Validate options against a white list. Also check the config_file is
* inside the config_dir. The white list is defined in validate.c
- * Returns true on success
+ * Returns true on success, false on error with reason set in errmsg.
*/
static BOOL
-ValidateOptions(HANDLE pipe, const WCHAR *workdir, const WCHAR *options)
+ValidateOptions(HANDLE pipe, const WCHAR *workdir, const WCHAR *options, WCHAR
*errmsg, DWORD capacity)
{
WCHAR **argv;
int argc;
-WCHAR buf[256];
BOOL ret = FALSE;
int i;
const WCHAR *msg1 = L"You have specified a config file location (%s
relative to %s)"
@@ -411,8 +410,9 @@ ValidateOptions(HANDLE pipe, const WCHAR *workdir, const
WCHAR *options)
if (!argv)
{
-ReturnLastError(pipe, L"CommandLineToArgvW");
-ReturnError(pipe, ERROR_STARTUP_DATA, L"Cannot validate options", 1,
&exit_event);
+openvpn_swprintf(errmsg, capacity,
+L"Cannot validate options: CommandLineToArgvW failed
with error = 0x%08x",
+GetLastError());
goto out;
}
@@ -432,9 +432,8 @@ ValidateOptions(HANDLE pipe, const WCHAR *workdir, const
WCHAR *options)
if (!CheckOption(workdir, 2, argv_tmp, &settings))
{
-openvpn_swprintf(buf, _countof(buf), msg1, argv[0], workdir,
+openvpn_swprintf(errmsg, capacity, msg1, argv[0], workdir,
settings.ovpn_admin_group);
-ReturnError(pipe, ERROR_STARTUP_DATA, buf, 1, &exit_event);
}
goto out;
}
@@ -450,15 +449,13 @@ ValidateOptions(HANDLE pipe, const WCHAR *workdir, const
WCHAR *options)
{
if (wcscmp(L"--config", argv[i]) == 0 && argc-i > 1)
{
-openvpn_swprintf(buf, _countof(buf), msg1, argv[i+1], workdir,
+openvpn_swprintf(errmsg, capacity, msg1, argv[i+1], workdir,
settings.ovpn_admin_group);
-ReturnError(pipe, ERROR_STARTUP_DATA, buf, 1, &exit_event);
}
else
{
-openvpn_swprintf(buf, _countof(buf), msg2, argv[i],
+openvpn_swprintf(errmsg, capacity, msg2, argv[i],
settings.ovpn_admin_group);
-ReturnError(pipe, ERROR_STARTUP_DATA, buf, 1, &exit_event);
}
goto out;
}
@@ -1487,6 +1484,7 @@ RunOpenvpn(LPVOID p)
size_t cmdline_size;
undo_lists_t undo_lists;
ring_buffer_handles_t ring_buffer_handles;
+WCHAR errmsg[512] = L"";
SECURITY_ATTRIBUTES inheritable = {
.nLength = sizeof(inheritable),
@@ -1580,10 +1578,17 @@ RunOpenvpn(LPVOID p)
goto out;
}
-/* Check user is authorized or options are white-listed */
-if (!IsAuthorizedUser(ovpn_user->User.Sid, imp_token,
settings.ovpn_admin_group)
-&& !ValidateOptions(pipe, sud.directory, sud.options))
+/*
+ * Only authorized users are allowed to use any command line options or
+ * have the config file in locations other than the global config
directory.
+ *
+ * Check options are white-listed and config is in the global directory
+ * OR user is authorized to run any config.
+ */
+if (!ValidateOptions(pipe, sud.directory, sud.options, errmsg,
_countof(errmsg))
+&& !IsAuthorizedUser(ovpn_us
Re: [Openvpn-devel] [PATCH] Support for wolfSSL with OpenVPN master branch
Hi Antonio,Gert is correct, our compatibility layer is a set of functions in wolfSSL which emulate the OpenSSL API. These functions are then macro defined to have the same names as the OpenSSL functions. The configure script needs to know where the wolfSSL headers are and that it should link against the wolfSSL binary, not OpenSSL. This is the reason for the configure script changes.Sincerely Juliusz Original message From: Antonio Quartulli Date: 09/02/2020 10:52 (GMT+01:00) To: Gert Doering Cc: Juliusz Sosinowicz , openvpn-devel@lists.sourceforge.net Subject: Re: [Openvpn-devel] [PATCH] Support for wolfSSL with OpenVPN master branch Hi,On 09/02/2020 10:50, Gert Doering wrote:> Hi,> > On Sun, Feb 09, 2020 at 10:44:48AM +0100, Antonio Quartulli wrote:>> if wolfssl support is being introduced by means of the openssl>> compatibility layer, why do people need to configure OpenVPN with>> "./configure --with-crypto-library=wolfssl", rather than just using>> openssl and specifying a different path for headers/libraries? Isn't the compat layer in wolfssl operating as a drop-in replacement for>> openssl?> > This question has been asked before and answered :-) - most of the> compat functions seem to be implemented as macros, which our configure> will not find. So, configure needs to explicitely define what is there> and what not.> > I do not like the extra include very much, but that seems to be hard > to avoid with the current WolfSSL header file setup.> ouch, thanks for the reminder :-)-- Antonio Quartulli___ Openvpn-devel mailing list Openvpn-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/openvpn-devel
Re: [Openvpn-devel] [PATCH] Support for wolfSSL with OpenVPN master branch
Hi Juliusz, if wolfssl support is being introduced by means of the openssl compatibility layer, why do people need to configure OpenVPN with "./configure --with-crypto-library=wolfssl", rather than just using openssl and specifying a different path for headers/libraries? Isn't the compat layer in wolfssl operating as a drop-in replacement for openssl? Regards, On 09/02/2020 10:18, Juliusz Sosinowicz wrote: > Hi Gert, > > thank you for your comments. My intention was not to add a second cipher > line in the sample config file. I added "cipher AES-256-CBC" to an > earlier version of OpenVPN when there was no cipher specified in the > loopback-client and loopback-server files. After rebasing my commit onto > master I didn't notice the double cipher lines in the config files. I > will remove this in my next patch as wolfSSL does support GCM mode but > not yet in the compatibility layer. > > I will add GCM support to our compatibility layer and send an updated > signed-off patch with a better commit message explaining what is > happening in the patch. > > Thanks > Juliusz > > On 08/02/2020 09:45, Gert Doering wrote: >> Hi Juliusz, >> >> please send patches out of a git tree, coming from a git commit with >> "git commit -s", and having a somewhat relevant commit message. >> >> Besides this, please do not >> >>> --- a/sample/sample-config-files/loopback-client >>> +++ b/sample/sample-config-files/loopback-client >>> @@ -25,3 +25,4 @@ tls-auth sample-keys/ta.key 1 >>> cipher AES-256-GCM >>> ping 1 >>> inactive 120 1000 >>> +cipher AES-256-CBC >> ... modify the sample config files (and *if* you do, do not just add >> a second cipher line, which will confuse users quite a bit). >> >> If WolfSSL does not support GCM, this needs to be documented, but our >> sample config files contain the recommended cipher for the existing >> crypto systems, and this is (and will continue to be for the time) >> GCM - faster, and lower overhead. >> >> gert > > > ___ > Openvpn-devel mailing list > Openvpn-devel@lists.sourceforge.net > https://lists.sourceforge.net/lists/listinfo/openvpn-devel > -- Antonio Quartulli 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] Support for wolfSSL with OpenVPN master branch
Hi, On 09/02/2020 10:50, Gert Doering wrote: > Hi, > > On Sun, Feb 09, 2020 at 10:44:48AM +0100, Antonio Quartulli wrote: >> if wolfssl support is being introduced by means of the openssl >> compatibility layer, why do people need to configure OpenVPN with >> "./configure --with-crypto-library=wolfssl", rather than just using >> openssl and specifying a different path for headers/libraries? >> >> Isn't the compat layer in wolfssl operating as a drop-in replacement for >> openssl? > > This question has been asked before and answered :-) - most of the > compat functions seem to be implemented as macros, which our configure > will not find. So, configure needs to explicitely define what is there > and what not. > > I do not like the extra include very much, but that seems to be hard > to avoid with the current WolfSSL header file setup. > ouch, thanks for the reminder :-) -- Antonio Quartulli ___ Openvpn-devel mailing list Openvpn-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/openvpn-devel
Re: [Openvpn-devel] [PATCH] Support for wolfSSL with OpenVPN master branch
Hi, On Sun, Feb 09, 2020 at 10:44:48AM +0100, Antonio Quartulli wrote: > if wolfssl support is being introduced by means of the openssl > compatibility layer, why do people need to configure OpenVPN with > "./configure --with-crypto-library=wolfssl", rather than just using > openssl and specifying a different path for headers/libraries? > > Isn't the compat layer in wolfssl operating as a drop-in replacement for > openssl? This question has been asked before and answered :-) - most of the compat functions seem to be implemented as macros, which our configure will not find. So, configure needs to explicitely define what is there and what not. I do not like the extra include very much, but that seems to be hard to avoid with the current WolfSSL header file setup. 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] Support for wolfSSL with OpenVPN master branch
Hi Gert, thank you for your comments. My intention was not to add a second cipher line in the sample config file. I added "cipher AES-256-CBC" to an earlier version of OpenVPN when there was no cipher specified in the loopback-client and loopback-server files. After rebasing my commit onto master I didn't notice the double cipher lines in the config files. I will remove this in my next patch as wolfSSL does support GCM mode but not yet in the compatibility layer. I will add GCM support to our compatibility layer and send an updated signed-off patch with a better commit message explaining what is happening in the patch. Thanks Juliusz On 08/02/2020 09:45, Gert Doering wrote: Hi Juliusz, please send patches out of a git tree, coming from a git commit with "git commit -s", and having a somewhat relevant commit message. Besides this, please do not --- a/sample/sample-config-files/loopback-client +++ b/sample/sample-config-files/loopback-client @@ -25,3 +25,4 @@ tls-auth sample-keys/ta.key 1 cipher AES-256-GCM ping 1 inactive 120 1000 +cipher AES-256-CBC ... modify the sample config files (and *if* you do, do not just add a second cipher line, which will confuse users quite a bit). If WolfSSL does not support GCM, this needs to be documented, but our sample config files contain the recommended cipher for the existing crypto systems, and this is (and will continue to be for the time) GCM - faster, and lower overhead. gert ___ Openvpn-devel mailing list Openvpn-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/openvpn-devel
