diff mbox series

[Openvpn-devel] Interactive service: do not force a target desktop for openvpn.exe

Message ID 20230518173345.2722530-1-selva.nair@gmail.com
State Accepted
Headers
Series [Openvpn-devel] Interactive service: do not force a target desktop for openvpn.exe |

Commit Message

Selva Nair May 18, 2023, 5:33 p.m. UTC 
  From: Selva Nair <selva.nair@gmail.com>

Setting the desktop as "winsta0\default" does not always work when run
from a non-interactive session which may not have access to the
the window station "Winsta0". Leave this as NULL to let the system
automatically assign a window station and desktop.

Test runs on Win10 confirm that "Winsta0\Default" still gets selected
when run interactively (e.g., using the GUI or from task scheduler as
an interactive job). This is the same behaviour as now.

The change allows "interactive service" to be used for launching
OpenVPN from non-interactive sessions. For example, when service client
is a non-interactive task from the task scheduler, the default desktop
in a custom window station gets assigned to openvpn.exe.

Note that we already run openvpn.exe in a non-interactive window
station when directly launched by "automatic service".

Github: Fixes OpenVPN/openvpn-gui#626

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

Comments

Gert Doering May 19, 2023, 7:28 a.m. UTC | #1 
Acked-by: Gert Doering <gert@greenie.muc.de>

This is all voodoo to me, but it doesn't break compilation, doesn't
bring in unsafe pointer constructs, and people say "it makes their use
case work", so it sounds like a good fix.

Microsoft documentation on

  https://learn.microsoft.com/en-us/windows/win32/api/processthreadsapi/nf-processthreadsapi-createprocessasuserw

says interesting things...

   "The user must have full access to both the specified window
    station and desktop. If you want the process to be interactive,
    specify winsta0\default. If the lpDesktop member is NULL, the
    new process inherits the desktop and window station of its
    parent process. If this member is an empty string, "", the new
    process connects to a window station using the rules described
    in Process Connection to a Window Station."

.. so we're moving from "winsta0\default" to "NULL" (startup_info is
ZeroMemory'ed), which sounds like "now it will run in the desktop
environment of the iservice", whatever *that* might be...  but it's
certainly a valid and documented choice...  and since openvpn.exe
"as run from iservice" doesn't want any windows etc., whatever makes
it succeed sounds good to me.

Your patch has been applied to the master and release/2.6 branch.

(Not sure this is a "bugfix" or a "mini feature", but it certainly
warrants a bit more thorough testing across Windows variants before
releasing 2.6.5)

commit 244d9b7942dabf0297c8f689457eeb0f9fa0aa1e (master)
commit 9e112be5dde043cdc1f073f33ada0a1810b730a6 (release/2.6)
Author: Selva Nair
Date:   Thu May 18 13:33:45 2023 -0400

     Interactive service: do not force a target desktop for openvpn.exe

     Signed-off-by: Selva Nair <selva.nair@gmail.com>
     Acked-by: Gert Doering <gert@greenie.muc.de>
     Message-Id: <20230518173345.2722530-1-selva.nair@gmail.com>
     URL: https://www.mail-archive.com/openvpn-devel@lists.sourceforge.net/msg26705.html
     Signed-off-by: Gert Doering <gert@greenie.muc.de>


--
kind regards,

Gert Doering
diff mbox series

Patch

diff --git a/src/openvpnserv/interactive.c b/src/openvpnserv/interactive.c
index ec196274..d73cef04 100644
--- a/src/openvpnserv/interactive.c
+++ b/src/openvpnserv/interactive.c
@@ -1868,7 +1868,6 @@  RunOpenvpn(LPVOID p)
     }
 
     startup_info.cb = sizeof(startup_info);
-    startup_info.lpDesktop = L"winsta0\\default";
     startup_info.dwFlags = STARTF_USESTDHANDLES;
     startup_info.hStdInput = stdin_read;
     startup_info.hStdOutput = stdout_write;