diff mbox series

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

Message ID 1580488320-10224-1-git-send-email-selva.nair@gmail.com
State Changes Requested
Headers
Series [Openvpn-devel,v2] Swap the order of checks for validating interactive service user |

Commit Message

Selva Nair Jan. 31, 2020, 5:32 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>
---

v2: Add missing closing parenthesis and improve the comment
above the edited chunk.

 src/openvpnserv/interactive.c | 12 +++++++++---
 1 file changed, 9 insertions(+), 3 deletions(-)

Comments

Lev Stipakov Feb. 2, 2020, 9:23 p.m. UTC | #1 
Built and tested, works as specified.

Acked-by: Lev Stipakov <lstipakov@gmail.com>
<div dir="ltr"><div dir="ltr">Built and tested, works as specified.<div><br></div><div>Acked-by: Lev Stipakov &lt;<a href="mailto:lstipakov@gmail.com">lstipakov@gmail.com</a>&gt;</div></div></div>
Lev Stipakov Feb. 2, 2020, 9:49 p.m. UTC | #2 
I am sorry, I have to retract my ACK.

When ValidateOptions is called first and config is non located in global
directory (Program Files),
service replies to gui via pipe with error message:

0x20000001
You have specified a config file location (client.xxx.ovpn relative to
C:\Users\lev\OpenVPN\config\client.xxx) that requires admin approval. This
error may be avoided by adding your account to the "OpenVPN Administrators"
group
(null)

This message is incorrect, since account is in admin group, we just haven't
checked that yet. When following check verifies that user indeed belongs to
the admin group, service runs openvpn and connection got established. By
some reasons, above error message is not read on the gui side. However, by
adding small delay to service code or a breakpoint, message reaches gui,
which displays an error.

While under normal circumstances it works, I believe it just works by
accident (error message is not read on gui side without delay on service
side). We need to move sending error message out of ValidateOptions.


ma 3. helmik. 2020 klo 10.23 Lev Stipakov (lstipakov@gmail.com) kirjoitti:

> Built and tested, works as specified.
>
> Acked-by: Lev Stipakov <lstipakov@gmail.com>
>
Selva Nair Feb. 3, 2020, 4:10 a.m. UTC | #3 
Hi,

On Mon, Feb 3, 2020 at 3:49 AM Lev Stipakov <lstipakov@gmail.com> wrote:
>
> I am sorry, I have to retract my ACK.
>
> When ValidateOptions is called first and config is non located in global directory (Program Files),
> service replies to gui via pipe with error message:
>
> 0x20000001
> You have specified a config file location (client.xxx.ovpn relative to C:\Users\lev\OpenVPN\config\client.xxx) that requires admin approval. This error may be avoided by adding your account to the "OpenVPN Administrators" group
> (null)
>

Nothing is as simple as it looks, is it..

After the first round of stupidity, I had run-tested version 2,  but
still did not catch this.

> This message is incorrect, since account is in admin group, we just haven't checked that yet. When following check verifies that user indeed belongs to the admin group, service runs openvpn and connection got established. By some reasons, above error message is not read on the gui side. However, by adding small delay to service code or a breakpoint, message reaches gui, which displays an error.
>

Its a timing bug in the GUI. The I/O completion routine for reading
from the service is initialized from the thread that handles the
connection. But this thread is created in a suspended state and
resumed only after sending the start request to the service from the
main thread. That could be the reason why this message is missed by
the GUI. This needs to be fixed as it could potentially affect normal
operation of the GUI.

> While under normal circumstances it works, I elieve it just works by accident (error message is not read on gui side without delay on service side). We need to move sending error message out of ValidateOptions.

This error message was originally added so that the GUI can notify the
user with a helpful message. But the GUI now checks config location
and user's group memberships and offers to spawn an elevated process
to add the user to ovpn_admin_group before calling the service.  But
still useful to return this message in case some other client decides
to use the service. I'll move it out of ValidateOptions and add code
to return it only when appropriate.


Selva
diff mbox series

Patch

diff --git a/src/openvpnserv/interactive.c b/src/openvpnserv/interactive.c
index 6e72a14..ff5b08b 100644
--- a/src/openvpnserv/interactive.c
+++ b/src/openvpnserv/interactive.c
@@ -1580,9 +1580,15 @@  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)
+        && !IsAuthorizedUser(ovpn_user->User.Sid, imp_token, settings.ovpn_admin_group))
     {
         goto out;
     }