| Message ID | 20230124142316.441-1-lstipakov@gmail.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series | [Openvpn-devel] openvpnmsica: fix adapters discovery logic for DCO | expand |
> > The ultimate solution to this would be moving adapter creation to MSM, > a shared component which adds/removes the DCO driver. However this change > is not trivial and requires a lot of work. For the time being we apply > this band-aid by excluding Connect-created adapters from enumerations in > "FindSystemInfo" custom action. This makes sure that OpenVPN-GUI won't > rely on adapter created by Connnect (which is deleted on Connect uninstall) > and ensures that additional DCO adapters won't be created on upgrade > if user decides to rename adapter. > Isn't allowing a user to add/remove adapters from UI a better approach? Arne
Hi, On Tue, Jan 24, 2023 at 9:25 AM Lev Stipakov <lstipakov@gmail.com> wrote: > From: Lev Stipakov <lev@openvpn.net> > > Custom action "FindSystemInfo" finds adapters with certain hwid and > assigns found adapters' guids to a certain property. Later another custom > action "EvaluateTUNTAPAdapters" schedules adapter creation if the > abovementioned property is not set - which means no adapters exist > with given hwid. > > I think this logic is needed to prevent duplicate adapter creation > if adapter was renamed and then new version is installed. > Is the driver still always updated (i.e., when update required) even if the adapter is not created because of existing ones found? Can we add an adapter creation to iservice so that openvp.exe can create adapters on the fly? An alternative is for the GUI to catch the "no adapters" error and offer to create one. But that would work only for users who can elevate and is less opaque to the user. As we support multiple connections and default to DCO, auto-creating at least DCO adapters will be an improvement. If we just fix a typo, OpenVPN-GUI won't create a adapter on step 2 and > after Connect removal on step 3 there won't be DCO adapters anymore > for OpenVPN-GUI to use. > Can the "Connect team" be convinced to use a different hwid for the adapter? Using a name to distinguish between adapters installed by different packages doesn't look very robust. That said, the quick-fix looks good to me.. I did not test it though. + /* exclude adapters created by OpenVPN Connect, since they're > removed on Connect uninstallation */ > + if (_tcsstr(pAdapter->szName, OPENVPN_CONNECT_ADAPTER_SUBSTR)) + { > + msg(M_WARN, "%s: skip OpenVPN Connect adapter '%ls'", > __FUNCTION__, pAdapter->szName); > M_INFO may be enough here? > + continue; > + } > + > /* Convert adapter GUID to UTF-16 string. (LPOLESTR defaults to > LPWSTR) */ > LPOLESTR szAdapterId = NULL; > StringFromIID((REFIID)&pAdapter->guid, &szAdapterId); > @@ -316,7 +325,7 @@ FindSystemInfo(_In_ MSIHANDLE hInstall) > find_adapters( > hInstall, > TEXT("ovpn-dco") TEXT("\0"), > - TEXT("OVPNDCOAPTERS"), > + TEXT("OVPNDCOADAPTERS"), > TEXT("ACTIVEOVPNDCOADAPTERS")); > if (bIsCoInitialized) Acked-by Selva Nair <selva.nair@gmail.com> Selva
Hi, >> I think this logic is needed to prevent duplicate adapter creation >> if adapter was renamed and then new version is installed. > > Is the driver still always updated (i.e., when update required) even if the adapter is not created because of existing ones found? At the moment yes. Moreover, the installer will always install the driver whether better drivers already exist. https://github.com/OpenVPN/ovpn-dco-win/blob/master/msm/msi.c#L186 Probably we should remove that flag and hope that UpdateDriverForPlugAndPlayDevices will do the right thing. > Can we add an adapter creation to iservice so that openvp.exe can create adapters on the fly? An alternative is for the GUI to catch the "no adapters" error and offer to create one. But that would work only for users who can elevate and is less opaque to the user. > As we support multiple connections and default to DCO, auto-creating at least DCO adapters will be an improvement. Yes, iservice can probably just run tapctl.exe --hwid ovpn-dco. Let's do it for 2.6.1 > Can the "Connect team" be convinced to use a different hwid for the adapter? Using a name to distinguish between adapters installed by different packages doesn't look very robust. That's what they were planning to do before I asked them not to - I don't think that is a good idea because - it increases maintenance cost - we would need to build/sign two versions of the driver - we are thinking about pushing driver to Windows Update, which requires passing HLK tests, which we do From adapter usage perspective, adapter name doesn't matter - it could happen so that when both Connect and OpenVPN-GUI are installed, Connect will use GUI adapter and vice versa. If we move adapter creation to MSM, we could have one adapter which is created once and removed when there are no clients. Also creating adapter on demand (we would need to implement that in openvpn3 too) would make this name dependency obsolete. >> + msg(M_WARN, "%s: skip OpenVPN Connect adapter '%ls'", __FUNCTION__, pAdapter->szName); > M_INFO may be enough here? In openvpnmsica we use error.h from ..\tapctl, which doesn't have M_INFO. I could probably add it in V2 if you think we need it. > Acked-by Selva Nair <selva.nair@gmail.com> Or in some follow-up patch :)
And here we have the winning patch for "very last minute before 2.6.0
release" - I hope it won't break Windows installation, as I couldn't
actually test it. It does not break Windows compilation :-) (GH is
broken, as is most of Microsoft today - local mingw build succeeds)
Your patch has been applied to the master and release/2.6 branch.
commit 7a23a7dda27349f7c7fd7af2ed1380ede1becf06 (master)
commit f4904e2ef4a61665561272fead0c4393947a27d1 (release/2.6)
Author: Lev Stipakov
Date: Tue Jan 24 16:23:16 2023 +0200
openvpnmsica: fix adapters discovery logic for DCO
Signed-off-by: Lev Stipakov <lev@openvpn.net>
Acked-by: Selva Nair <selva.nair@gmail.com>
Message-Id: <20230124142316.441-1-lstipakov@gmail.com>
URL: https://www.mail-archive.com/openvpn-devel@lists.sourceforge.net/msg26072.html
Signed-off-by: Gert Doering <gert@greenie.muc.de>
--
kind regards,
Gert Doering
diff --git a/src/openvpnmsica/openvpnmsica.c b/src/openvpnmsica/openvpnmsica.c index af12b2c4..06c9789d 100644 --- a/src/openvpnmsica/openvpnmsica.c +++ b/src/openvpnmsica/openvpnmsica.c @@ -65,6 +65,8 @@ #define FILE_NEED_REBOOT L".ovpn_need_reboot" +#define OPENVPN_CONNECT_ADAPTER_SUBSTR L"OpenVPN Connect" + /** * Joins an argument sequence and sets it to the MSI property. * @@ -224,6 +226,13 @@ find_adapters( for (struct tap_adapter_node *pAdapter = pAdapterList; pAdapter; pAdapter = pAdapter->pNext) { + /* exclude adapters created by OpenVPN Connect, since they're removed on Connect uninstallation */ + if (_tcsstr(pAdapter->szName, OPENVPN_CONNECT_ADAPTER_SUBSTR)) + { + msg(M_WARN, "%s: skip OpenVPN Connect adapter '%ls'", __FUNCTION__, pAdapter->szName); + continue; + } + /* Convert adapter GUID to UTF-16 string. (LPOLESTR defaults to LPWSTR) */ LPOLESTR szAdapterId = NULL; StringFromIID((REFIID)&pAdapter->guid, &szAdapterId); @@ -316,7 +325,7 @@ FindSystemInfo(_In_ MSIHANDLE hInstall) find_adapters( hInstall, TEXT("ovpn-dco") TEXT("\0"), - TEXT("OVPNDCOAPTERS"), + TEXT("OVPNDCOADAPTERS"), TEXT("ACTIVEOVPNDCOADAPTERS")); if (bIsCoInitialized)