diff mbox series

[Openvpn-devel] Swap the order of checks for validating interactive service user

Message ID 1580388208-26594-1-git-send-email-selva.nair@gmail.com
State Superseded
Headers show
Series [Openvpn-devel] Swap the order of checks for validating interactive service user | expand

Commit Message

Selva Nair Jan. 30, 2020, 1:43 a.m. UTC 
From: Selva Nair <selva.nair@gmail.com>

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.

Also see: https://github.com/OpenVPN/openvpn-gui/issues/332

Signed-off-by: Selva Nair <selva.nair@gmail.com>
---
 src/openvpnserv/interactive.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

Comments

Lev Stipakov Jan. 30, 2020, 11:29 p.m. UTC | #1 
Hi,

+    if (!ValidateOptions(pipe, sud.directory, sud.options)
> +        && !IsAuthorizedUser(ovpn_user->User.Sid, imp_token,
> settings.ovpn_admin_group)
>      {
>

Closing parenthesis is missing:

>C:\Users\lev\Projects\openvpn\src\openvpnserv\interactive.c(1586,5): error
C2143: syntax error: missing ')' before '{'

Also it is probably just me, but took me a while to figure out what that
code is doing and why.

Could you slightly improve the comment above lines you touch
("Check user is authorized or options are white-listed") and mention
something like
"Non-authorized users are allowed to use only white-listed options and
have config only in global openvpn config directory"?

When I started to debug it, I realized that I have to be authorized user
when config
is in my current user's directory (C:\Users\lev\OpenVPN\config) and not in
"global" config
directory ("C:\Program Files\OpenVPN\config"). Is this by design?
Selva Nair Jan. 31, 2020, 2:42 a.m. UTC | #2 
Hi,

On Fri, Jan 31, 2020 at 5:29 AM Lev Stipakov <lstipakov@gmail.com> wrote:
>
> Hi,
>
>> +    if (!ValidateOptions(pipe, sud.directory, sud.options)
>> +        && !IsAuthorizedUser(ovpn_user->User.Sid, imp_token, settings.ovpn_admin_group)
>>      {
>
>
> Closing parenthesis is missing:

That is embarrassing.. I went through the motions of compile testing it without
updating the source tree in openvpnbuild. Yikes..

>
> >C:\Users\lev\Projects\openvpn\src\openvpnserv\interactive.c(1586,5): error C2143: syntax error: missing ')' before '{'
>
> Also it is probably just me, but took me a while to figure out what that code is doing and why.
>
> Could you slightly improve the comment above lines you touch
> ("Check user is authorized or options are white-listed") and mention something like
> "Non-authorized users are allowed to use only white-listed options and
> have config only in global openvpn config directory"?

Will do.

>
> When I started to debug it, I realized that I have to be authorized user when config
> is in my current user's directory (C:\Users\lev\OpenVPN\config) and not in "global" config
> directory ("C:\Program Files\OpenVPN\config"). Is this by design?

Yes, that is by design. As iservice allows the users to do some actions that
normally require admin privilege, a limited user is only allowed to use some
whitelisted options or a config installed by an admin in the global config
directory. They are not allowed to run arbitrary configs
that they can edit. Unless an admin explicitly gives them permission to do
so --- checked by membership in "OpenVPNAdministrators" group. Users
who have admin privilege (even if not enabled in the token by UAC) are
considered authorized.

Selva
diff mbox series

Patch

diff --git a/src/openvpnserv/interactive.c b/src/openvpnserv/interactive.c
index 6e72a14..dafd5c6 100644
--- a/src/openvpnserv/interactive.c
+++ b/src/openvpnserv/interactive.c
@@ -1581,8 +1581,8 @@  RunOpenvpn(LPVOID p)
     }
 
     /* 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))
+    if (!ValidateOptions(pipe, sud.directory, sud.options)
+        && !IsAuthorizedUser(ovpn_user->User.Sid, imp_token, settings.ovpn_admin_group)
     {
         goto out;
     }