Re: [Openvpn-devel] [PATCH v2 3/7] wintun: implement opening wintun device
Hi, On Mon, Nov 25, 2019 at 09:44:19AM +, Simon Rozman wrote: > 1.if adapter "My VPN Connection" doesn't exist, create it. > 2.else enable it > 3.use it > 4.disable it > An annoyance here is, the adapters pile up over time. On multi-user > computers, OpenVPN GUI don't have a complete overview which one are > stale (no corresponding .ovpn file exist anymore) to clean them. I like the initial approach, but the "pile-up" is annoying. What about "do this for .ovpn configs that contain a --dev-node setting, and otherwise just try to enumerate 'OpenVPN Wintun 1', 'OpenVPN Wintun 2', ... and create those as-needed? This should keep the numbers down... 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 smime.p7s Description: S/MIME cryptographic signature ___ Openvpn-devel mailing list Openvpn-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/openvpn-devel
Re: [Openvpn-devel] [PATCH v2 3/7] wintun: implement opening wintun device
Hi
On Mon, Nov 25, 2019 at 4:03 AM Lev Stipakov wrote:
> Hi,
>
> (cc:ed to -devel)
>
>
>> I would vote for B and not the combination.
>>
>> With wintun there is no backwards compatibility requirements, so we could
>> use a cleaner, consistent and simpler approach (i.e B). Do not create any
>> adapter during installation and dynamically create a temporary adapter at
>> connection time.
>>
>
> My main concern with creating tun adapter on demand is that it is far from
> instant:
>
> $ time ./tapctl.exe create --hwid wintun
> {D9F56B7A-3054-4ADC-9457-61030F0B469D}
>
> real0m2,090s
>
> I don't think we want to add it to connection time.
>
No, we don't want that.
I do realize what I wrote earlier was far from ideal. I had forgotten my
travails two years back attempting to add dynamic tap adapter creation to
OpnVPN
As an easy way for supporting multiple tunnels, can we treat success of
open-device + ring-buffer-registration calls together as a successful open
of the adapter, and move on to the next one when that fails? That would be
very similar to what we do for tapwindows right now.
To assist users, when we fail with no free adapters, we could add a feature
to the GUI to add a new one using the RunAsAdmin shell-execute we already
have support for. Will need an improved devcon/tapinstall that does not
reset all adapters when a new one is added, except when unavoidable due to
driver version change.
Simon's suggestion of persistent adapters as required and kept disabled
when not in use sounds interesting. As some providers do supply 100's of
configs, I think we should refrain from creating one adapter per ovpn,
though. They should be using multiple --remote lines in a single config,
but, for that to work well, we need to improve --management-remote option
to provide a friendly UI for remote selection.
Selva
___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel
Re: [Openvpn-devel] [PATCH v2 3/7] wintun: implement opening wintun device
I know. The tap.c code needs an upgrade, not to evaluate all drivers, but just
compatible drivers when creating a new adapter. This speeds things a lot.
There's a flag that needs to be changed. Somewhere deep on my TODO lists.
I would suggest against temporary adapters on Windows. This is OK on Linux, but
Windows is not Linux. Apart from the time penalty you already discovered, there
are more annoying issues with temporary adapters: adapters always get a new
GUID on creation, making NLA think it's a new network each time. This will keep
resetting the firewall profile assigned to the adapter and piling up network
IDs in user registry at each connection attempt. Also, changing bindings on
adapters don't persist (vmWare, VirtualBox, Hyper-V bindings comes to my mind)…
I am testing an intermediate approach to adapter management on Windows,
inspired by Microsoft's own way how Windows handle dial-up adapters:
1. if adapter "My VPN Connection" doesn't exist, create it.
2. else enable it
3. use it
4. disable it
In my observation, this is the most streamlined approach on Windows. It avoids
adapter creation burden, while still (re)creates it on demand. Disabling
adapter when it is not in use releases the resources, removes the adapter from
the network stack, halts the adapter, unloads the driver when the last of the
adapters (of the same driver) is disabled… Like adapter was deleted, only its
settings persist in registry.
An annoyance here is, the adapters pile up over time. On multi-user computers,
OpenVPN GUI don't have a complete overview which one are stale (no
corresponding .ovpn file exist anymore) to clean them. Cleaning requires
elevation… An admin user may use Device manager to clean them, uninstaller
should clean them. Both are not ideal. On test computers with lots of
configuration fuss that might be a problem. On production computers this
shouldn't be a big issue. Mind that even Hyper-V doesn't clean up adapters if
you install and uninstall it on Windows.
Maybe we could save the absolute path to .ovpn file in the adapter registry to
assign it to a particular profile. Interactive service could periodically
delete orphaned adapters. I'm not worried about .ovpn-less tunnels: users
running openvpn.exe specifying all settings in the command line know what
they're doing.
Best regards,
Simon
From: Lev Stipakov
Sent: Monday, November 25, 2019 10:04 AM
To: Selva Nair
Cc: openvpn-devel
Subject: Re: [Openvpn-devel] [PATCH v2 3/7] wintun: implement opening wintun
device
Hi,
(cc:ed to -devel)
I would vote for B and not the combination.
With wintun there is no backwards compatibility requirements, so we could use a
cleaner, consistent and simpler approach (i.e B). Do not create any adapter
during installation and dynamically create a temporary adapter at connection
time.
My main concern with creating tun adapter on demand is that it is far from
instant:
$ time ./tapctl.exe create --hwid wintun
{D9F56B7A-3054-4ADC-9457-61030F0B469D}
real0m2,090s
I don't think we want to add it to connection time.
Creating one persistent adapter per profile (as viscosity does for tapwindows
and wireguard seems to do for wintun)
If I remember right, wireguard doesn't create persistent adapter, instead it
adds/removes it on demand.
If --dev-node is specified, we open the named adapter which the user is
supposed to have created as we do for tapwindows.
Yes, I plan to add support for --dev-node for wintun.
--
-Lev
smime.p7s
Description: S/MIME cryptographic signature
___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel
Re: [Openvpn-devel] [PATCH v2 3/7] wintun: implement opening wintun device
Hi,
(cc:ed to -devel)
> I would vote for B and not the combination.
>
> With wintun there is no backwards compatibility requirements, so we could
> use a cleaner, consistent and simpler approach (i.e B). Do not create any
> adapter during installation and dynamically create a temporary adapter at
> connection time.
>
My main concern with creating tun adapter on demand is that it is far from
instant:
$ time ./tapctl.exe create --hwid wintun
{D9F56B7A-3054-4ADC-9457-61030F0B469D}
real0m2,090s
I don't think we want to add it to connection time.
Creating one persistent adapter per profile (as viscosity does for
> tapwindows and wireguard seems to do for wintun)
>
If I remember right, wireguard doesn't create persistent adapter, instead
it adds/removes it on demand.
If --dev-node is specified, we open the named adapter which the user is
> supposed to have created as we do for tapwindows.
>
Yes, I plan to add support for --dev-node for wintun.
--
-Lev
___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel
Re: [Openvpn-devel] [PATCH v2 3/7] wintun: implement opening wintun device
Hi, > We have more options here: > Can we combine B and C? 1) During installation, we create one adapter, as we do now. 2) On connection we enumerate adapters and pick the first one which is not connected. 3) If we could not find free adapter (another VPN connection is already running), we create one on demand. -- -Lev ___ Openvpn-devel mailing list Openvpn-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/openvpn-devel
Re: [Openvpn-devel] [PATCH v2 3/7] wintun: implement opening wintun device
Hi, The Wintun doesn't create its own communication I/O device. Running a separate NdisRegisterDeviceEx() device came with a big can of worms, so we decided not to run our own, but rather piggy-back on the existing NDIS one from the NdisMRegisterMiniportDriver() that we tap on. Technically, we could limit it to a single client handle, but this device is primarily used by Windows to manage the adapter, therefore we must not tweak it in this direction. As Lev said: Wintun adds an IOCTL call to register ring buffers and it is this IOCTL that has a refcounting implemented to deny secondary connection attempts. Unfortunately for OpenVPN, this IOCTL is restricted to privileged processes only. You cannot use it to detect which Wintun adapters are in use by an unprivileged openvpn.exe process. WireGuard is very strict about which TUN adapter it will use – it will never ever use a lottery to pick one. We have more options here: A. Wintun adapter can be explicitly chosen by configuration only (by name or GUID). With tapctl.exe utility users can make an adapter with predefined name and put this name into .ovpn. If the Wintun adapter is not specified explicitly => configuration error. Less lottery; predictable tunnel-adapter mapping when running multiple tunnels; disturbed users because there is an inconsistency between TAP-Windows6 and Wintun adapter selection logic. B. The tapctl.exe code is in OpenVPN repo already and could be integrated in openvpn.exe and interactive service to create Wintun adapter on demand. But, you have to take responsibility to clean what you have created. Pretty much the same as A), but nicer for user. In the long term, I'd suggest this "create adapter if doesn't exist" approach even for TAP-Windows6 adapters. C. Use adapter media state to see if particular Wintun adapter is already in use or not. Mind that registering buffer also sets MediaConnectStateConnected, and stays connected until its client closes handle or dies. This allows OpenVPN to extend its "use first available adapter" approach to Wintun adapters. Best regards, Simon From: Selva Nair Sent: Tuesday, November 19, 2019 7:03 PM To: Lev Stipakov Cc: Lev Stipakov ; openvpn-devel Subject: Re: [Openvpn-devel] [PATCH v2 3/7] wintun: implement opening wintun device Hi Lev, On Tue, Nov 19, 2019 at 12:23 PM Lev Stipakov mailto:lstipa...@gmail.com> > wrote: Hi, Apart from the error message, there is a larger issue especially when we use iservice. In that case, we have to preserve privilege separation and allowing a user to open a device handle in use by another has to be avoided. Do you see it as a security issue when handle can be opened by another process? I don't know the internals of wintun to know that for sure. To read / write to tunnel one needs to register ring buffers, and this call will fail for any other process. Am I missing something here? Hopefully, Simon can confirm whether that provides a sufficient safety net. Selva smime.p7s Description: S/MIME cryptographic signature ___ Openvpn-devel mailing list Openvpn-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/openvpn-devel
Re: [Openvpn-devel] [PATCH v2 3/7] wintun: implement opening wintun device
Hi, Apart from the error message, there is a larger issue especially when we > use iservice. In that case, we have to preserve privilege separation and > allowing a user to open a device handle in use by another has to be avoided. > Do you see it as a security issue when handle can be opened by another process? To read / write to tunnel one needs to register ring buffers, and this call will fail for any other process. Am I missing something here? > Can we get the wintun folks to support exclusive open (say FILE_SHARE_READ > = 0) or expose the internal count of number of open handles? That would > also make it easier to fix this and provide better error messages. > We could maybe submit a patch for that. I agree it would be nice to have. > Hmm.. if multiple openvpn instances are not tested this is not ready for > review yet, is it? > I don't think so. I would not expect new tun driver (or code around it) to support all features of old driver right from the beginning. Wintun doesn't replace tap-windows6, so if someone requires functionality which wintun doesn't yet support, one could always switch to previous driver. Again, a quick test shows that, with multiple openvpn instances, it does > open an already opened device and then fails while registering ring buffers. > Yeah, it would be nice to eventual support multiple tunnels, but I don't think that this is "must have" in the first version. ___ Openvpn-devel mailing list Openvpn-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/openvpn-devel
Re: [Openvpn-devel] [PATCH v2 3/7] wintun: implement opening wintun device
Hi, On Tue, Nov 19, 2019 at 3:50 AM Lev Stipakov wrote: > Hi, > > Doesn't this mean that if --dev-node is specified, we'll open tapwindows >> adapter >> with that name even if "--window-driver wintun" is specified? The open >> may succeed >> but subsequent wintun-specific processing will fail. >> > > --dev-node is not (yet) supported and open will fail (just re-tested). > I did a quick test and found the open to succeed with a misleading message that Wintun device opened (though it opened the tap device) and then fails on register ring buffers. I haven't applied all patches as yet, so not sure whether this is changed in later patches. If I'm not mistaken wintun device can be opened multiple times, so we'll >> never get the >> "All wintun adapters on this system" error. >> > Well, we get it if wintun driver is not installed :) but yep, open will > succeed. > Instead, open will succeed here and something else may fail later. >> > > Right after opening we register ring buffers, and that will fail (with a > friendly message): > > ERROR: Failed to register ring buffers: 1247 > > We should probably make it even more user friendly and write something > like "make sure wintun adapter is not in use". > Apart from the error message, there is a larger issue especially when we use iservice. In that case, we have to preserve privilege separation and allowing a user to open a device handle in use by another has to be avoided. The service could keep a lock on devices it manages, but I have no idea how we are going to lock out devices opened by other means (say instances started by automatic service, or wireguard or some other third party product). Can we get the wintun folks to support exclusive open (say FILE_SHARE_READ = 0) or expose the internal count of number of open handles? That would also make it easier to fix this and provide better error messages. > Wireguard creates wintun adapter on demand (on connect) and openvpn during > installation and then > picks the first one found on connect. Maybe that logic could be more > clever to not to pick adapter which > is currently used by wireguard. > > I haven't looked yet into running wireguard and openvpn side by side (or > multiple openvpn instances > with wintun), just tested that wg and openvpn could co-exist without > problems on the same machine. > Hmm.. if multiple openvpn instances are not tested this is not ready for review yet, is it? Again, a quick test shows that, with multiple openvpn instances, it does open an already opened device and then fails while registering ring buffers. Selva ___ Openvpn-devel mailing list Openvpn-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/openvpn-devel
Re: [Openvpn-devel] [PATCH v2 3/7] wintun: implement opening wintun device
Hi, Doesn't this mean that if --dev-node is specified, we'll open tapwindows > adapter > with that name even if "--window-driver wintun" is specified? The open may > succeed > but subsequent wintun-specific processing will fail. > --dev-node is not (yet) supported and open will fail (just re-tested). > If I'm not mistaken wintun device can be opened multiple times, so we'll > never get the > "All wintun adapters on this system" error. > Well, we get it if wintun driver is not installed :) but yep, open will succeed. Instead, open will succeed here and something else may fail later. > Right after opening we register ring buffers, and that will fail (with a friendly message): ERROR: Failed to register ring buffers: 1247 We should probably make it even more user friendly and write something like "make sure wintun adapter is not in use". Wireguard creates wintun adapter on demand (on connect) and openvpn during installation and then picks the first one found on connect. Maybe that logic could be more clever to not to pick adapter which is currently used by wireguard. I haven't looked yet into running wireguard and openvpn side by side (or multiple openvpn instances with wintun), just tested that wg and openvpn could co-exist without problems on the same machine. -- -Lev ___ Openvpn-devel mailing list Openvpn-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/openvpn-devel
Re: [Openvpn-devel] [PATCH v2 3/7] wintun: implement opening wintun device
Hi,
On Thu, Nov 7, 2019 at 12:49 PM Lev Stipakov wrote:
> From: Lev Stipakov
>
> To open wintun device, we cannot use "\\.\Global\Wintun"
> path as before. To get device path which we supply to CreateFile,
> we have to use SetupAPI to:
>
> - enumerate network adapters with "wintun" as component id
> - for each adapter save its guid
> - open device information set
> - for each item in set
>- open corresponding registry key to get net_cfg_instance_id
>- get symbolic link name of device interface by instance id
> - path will be symbolic link name of device instance matched with
> adapter's guid
>
> See
> https://github.com/OpenVPN/openvpn3/blob/master/openvpn/tun/win/tunutil.hpp
> and
>
> https://github.com/WireGuard/wireguard-go/blob/master/tun/wintun/wintun_windows.go
> for
> implementation examples.
>
> Signed-off-by: Lev Stipakov
>
This also has been ACKed and merged, but two questions that may need some
attention:
@@ -5541,7 +5668,8 @@ void
> open_tun(const char *dev, const char *dev_type, const char *dev_node,
> struct tuntap *tt)
> {
> struct gc_arena gc = gc_new();
> -char device_path[256];
> +char tuntap_device_path[256];
> +char *path = NULL;
> const char *device_guid = NULL;
> DWORD len;
> bool dhcp_masq = false;
> @@ -5571,6 +5699,8 @@ open_tun(const char *dev, const char *dev_type,
> const char *dev_node, struct tun
> {
> const struct tap_reg *tap_reg = get_tap_reg();
> const struct panel_reg *panel_reg = get_panel_reg();
> +const struct device_instance_id_interface
> *device_instance_id_interface = get_device_instance_id_interface();
> +
> char actual_buffer[256];
>
> at_least_one_tap_win(tap_reg);
> @@ -5586,24 +5716,22 @@ open_tun(const char *dev, const char *dev_type,
> const char *dev_node, struct tun
> }
>
> /* Open Windows TAP-Windows adapter */
> -openvpn_snprintf(device_path, sizeof(device_path), "%s%s%s",
> +openvpn_snprintf(tuntap_device_path,
> sizeof(tuntap_device_path), "%s%s%s",
> USERMODEDEVICEDIR,
> device_guid,
> TAP_WIN_SUFFIX);
>
> -tt->hand = CreateFile(
> -device_path,
> -GENERIC_READ | GENERIC_WRITE,
> -0,/* was: FILE_SHARE_READ */
> -0,
> -OPEN_EXISTING,
> -FILE_ATTRIBUTE_SYSTEM | FILE_FLAG_OVERLAPPED,
> -0
> -);
> +tt->hand = CreateFile(tuntap_device_path,
> + GENERIC_READ | GENERIC_WRITE,
> + 0,/* was:
> FILE_SHARE_READ */
> + 0,
> + OPEN_EXISTING,
> + FILE_ATTRIBUTE_SYSTEM |
> FILE_FLAG_OVERLAPPED,
> + 0);
>
> if (tt->hand == INVALID_HANDLE_VALUE)
> {
> -msg(M_ERR, "CreateFile failed on TAP device: %s",
> device_path);
> +msg(M_ERR, "CreateFile failed on TAP device: %s",
> tuntap_device_path);
>
Doesn't this mean that if --dev-node is specified, we'll open tapwindows
adapter
with that name even if "--window-driver wintun" is specified? The open may
succeed
but subsequent wintun-specific processing will fail.
}
> }
> else
> @@ -5613,43 +5741,78 @@ open_tun(const char *dev, const char *dev_type,
> const char *dev_node, struct tun
> /* Try opening all TAP devices until we find one available */
> while (true)
> {
> +bool is_picked_device_wintun = false;
> device_guid = get_unspecified_device_guid(device_number,
>actual_buffer,
>
> sizeof(actual_buffer),
>tap_reg,
>panel_reg,
> +
> _picked_device_wintun,
>);
>
> if (!device_guid)
> {
> -msg(M_FATAL, "All TAP-Windows adapters on this system
> are currently in use.");
> +msg(M_FATAL, "All %s adapters on this system are
> currently in use.", tt->wintun ? "wintun" : "TAP - Windows");
>
If I'm not mistaken wintun device can be opened multiple times, so we'll
never get the
"All wintun adapters on this system" error. Instead, open will succeed
here and
something else may fail later. FILE_SHARE_READ = 0 will not save us when
the driver
does not enforce it.
Selva
___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel
Re: [Openvpn-devel] [PATCH v2 3/7] wintun: implement opening wintun device
Hi,
> -Original Message-
> From: Lev Stipakov [mailto:lstipa...@gmail.com]
> Sent: Thursday, November 7, 2019 6:45 PM
> To: openvpn-devel@lists.sourceforge.net
> Cc: Lev Stipakov
> Subject: [Openvpn-devel] [PATCH v2 3/7] wintun: implement opening wintun
> device
>
> +const static GUID GUID_DEVCLASS_NET = { 0x4d36e972L, 0xe325, 0x11ce, {
> +0xbf, 0xc1, 0x08, 0x00, 0x2b, 0xe1, 0x03, 0x18 } }; const static GUID
> +GUID_DEVINTERFACE_NET = { 0xcac88484, 0x7
515, 0x4c03, { 0x82, 0xe6,
> +0x71, 0xa8, 0x7a, 0xba, 0xc3, 0x61 } };
> +
GUID_DEVCLASS_NET is declared in devguid.h, GUID_DEVINTERFACE_NET in
ndisguid.h... No need to redefine them. However, while one could include
those SDK files, one needs to add the appropriate .lib files too. It's not
worth complicating for just a couple of GUIDs that will never ever change.
So, ACK.
The rest LGTM.
Acked-by: Simon Rozman
Best regards,
Simon
smime.p7s
Description: S/MIME cryptographic signature
___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel
