Re: [Openvpn-devel] (no subject)
El mar., 19 de noviembre de 2019 3:38 p. m., Fernando Mendez < fernando.brenda.34...@gmail.com> escribió: > ___ > Openvpn-devel mailing list > Openvpn-devel@lists.sourceforge.net > https://lists.sourceforge.net/lists/listinfo/openvpn-devel > ___ Openvpn-devel mailing list Openvpn-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/openvpn-devel
[Openvpn-devel] (no subject)
___ Openvpn-devel mailing list Openvpn-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/openvpn-devel
Re: [Openvpn-devel] [PATCH v2] Fix ACL_CHECK_ADD_COMPILE_FLAGS to work with clang
Typo below,
On 19/11/2019 17:03, selva.n...@gmail.com wrote:
From: Selva Nair
Some compilers (e.g., clang) only issue a warning for
unsupported options unless an additional flag such
as -Werror is used to convert the warning to an error.
The behaviour is unchanged when using gcc as it either
errors or ignores unknown options whether or not -Werror
is present.
Signed-off-by: Selva Nair
---
v2: Do not extend the macro to take an extra argument.
Instead, add -Werror while doing the test.
The commit message and comment changed to match this.
configure.ac | 9 ++---
1 file changed, 6 insertions(+), 3 deletions(-)
diff --git a/configure.ac b/configure.ac
index 807804e5..f4a947d2 100644
--- a/configure.ac
+++ b/configure.ac
@@ -1275,11 +1275,14 @@ if test "${enable_pkcs11}" = "yes"; then
)
fi
+# When testing a compiler option, we add -Werror to force
+# an error when the option is unsupported. This is not
+# required for gcc, but come compilers such as clang needs it.
come compilers --> some compilers
AC_DEFUN([ACL_CHECK_ADD_COMPILE_FLAGS], [
old_cflags="$CFLAGS"
-CFLAGS="$1 $CFLAGS"
-AC_MSG_CHECKING([whether the compiler acceppts $1])
-AC_COMPILE_IFELSE([AC_LANG_PROGRAM()], [AC_MSG_RESULT([yes])],
+CFLAGS="$1 -Werror $CFLAGS"
+AC_MSG_CHECKING([whether the compiler accepts $1])
+AC_COMPILE_IFELSE([AC_LANG_PROGRAM()], [AC_MSG_RESULT([yes])]; CFLAGS="$1
$old_cflags",
[AC_MSG_RESULT([no]); CFLAGS="$old_cflags"])]
)
___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel
[Openvpn-devel] [PATCH 8/7] wintun: fix ringbuffer registration call
From: Lev Stipakov
As MS documentation says:
> If lpOverlapped is NULL, lpBytesReturned cannot be NULL
While on Windows 10 passing NULL works by accident,
on Windows 7 it crashes.
Reported-by: kitsune1
Signed-off-by: Lev Stipakov
---
src/openvpn/ring_buffer.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/src/openvpn/ring_buffer.c b/src/openvpn/ring_buffer.c
index 482e3336..d384f497 100644
--- a/src/openvpn/ring_buffer.c
+++ b/src/openvpn/ring_buffer.c
@@ -35,6 +35,7 @@ register_ring_buffers(HANDLE device,
{
struct tun_register_rings rr;
BOOL res;
+DWORD bytes_returned;
ZeroMemory(&rr, sizeof(rr));
@@ -46,7 +47,8 @@ register_ring_buffers(HANDLE device,
rr.receive.ring_size = sizeof(receive_ring->data);
rr.receive.tail_moved = receive_tail_moved;
-res = DeviceIoControl(device, TUN_IOCTL_REGISTER_RINGS, &rr, sizeof(rr),
NULL, 0, NULL, NULL);
+res = DeviceIoControl(device, TUN_IOCTL_REGISTER_RINGS, &rr, sizeof(rr),
+NULL, 0, &bytes_returned, NULL);
return res == TRUE;
}
--
2.17.1
___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel
Re: [Openvpn-devel] [PATCH v2] Fix ACL_CHECK_ADD_COMPILE_FLAGS to work with clang
On 19/11/2019 18:03, selva.n...@gmail.com wrote: > From: Selva Nair > > Some compilers (e.g., clang) only issue a warning for > unsupported options unless an additional flag such > as -Werror is used to convert the warning to an error. > > The behaviour is unchanged when using gcc as it either > errors or ignores unknown options whether or not -Werror > is present. > > Signed-off-by: Selva Nair > --- > v2: Do not extend the macro to take an extra argument. > Instead, add -Werror while doing the test. > > The commit message and comment changed to match this. > > configure.ac | 9 ++--- > 1 file changed, 6 insertions(+), 3 deletions(-) > Tested running ./configure and 'make' using gcc-4.3.8, gcc-8.3.1 and clang-5.0.1. With patch applied, everything works as expected and no unexpected halts during these two steps. Without this patch, clang-5.0.1 spits out lots of warnings related to -Wno-stringop-truncation. Acked-By: David Sommerseth -- kind regards, David Sommerseth OpenVPN Inc signature.asc Description: OpenPGP digital 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 Lev, On Tue, Nov 19, 2019 at 12:23 PM Lev Stipakov 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 ___ 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
[Openvpn-devel] [PATCH v2] Fix ACL_CHECK_ADD_COMPILE_FLAGS to work with clang
From: Selva Nair
Some compilers (e.g., clang) only issue a warning for
unsupported options unless an additional flag such
as -Werror is used to convert the warning to an error.
The behaviour is unchanged when using gcc as it either
errors or ignores unknown options whether or not -Werror
is present.
Signed-off-by: Selva Nair
---
v2: Do not extend the macro to take an extra argument.
Instead, add -Werror while doing the test.
The commit message and comment changed to match this.
configure.ac | 9 ++---
1 file changed, 6 insertions(+), 3 deletions(-)
diff --git a/configure.ac b/configure.ac
index 807804e5..f4a947d2 100644
--- a/configure.ac
+++ b/configure.ac
@@ -1275,11 +1275,14 @@ if test "${enable_pkcs11}" = "yes"; then
)
fi
+# When testing a compiler option, we add -Werror to force
+# an error when the option is unsupported. This is not
+# required for gcc, but come compilers such as clang needs it.
AC_DEFUN([ACL_CHECK_ADD_COMPILE_FLAGS], [
old_cflags="$CFLAGS"
-CFLAGS="$1 $CFLAGS"
-AC_MSG_CHECKING([whether the compiler acceppts $1])
-AC_COMPILE_IFELSE([AC_LANG_PROGRAM()], [AC_MSG_RESULT([yes])],
+CFLAGS="$1 -Werror $CFLAGS"
+AC_MSG_CHECKING([whether the compiler accepts $1])
+AC_COMPILE_IFELSE([AC_LANG_PROGRAM()], [AC_MSG_RESULT([yes])]; CFLAGS="$1
$old_cflags",
[AC_MSG_RESULT([no]); CFLAGS="$old_cflags"])]
)
--
2.20.1
___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel
Re: [Openvpn-devel] [PATCH] Fix ACL_CHECK_ADD_COMPILE_FLAGS to work with clang
Hi, On Tue, Nov 19, 2019 at 9:09 AM David Sommerseth < open...@sf.lists.topphemmelig.net> wrote: > On 14/11/2019 22:58, Selva Nair wrote: > > Hi David > > > > Thanks for the comments > > > > My idea was just to add -Werror right in the line above, and not > extend the > > ACL_CHECK_ADD_COMPILE_FLAGS macro with another argument. > > > > > > I'm fine with that approach as well. Let me know if you want a v2. > > Sorry for the delay responding. I've done some pondering, and I have come > to > the same conclusion as Arne, so this is a reasonable approach to add > -Werror. > > That said, is there any specific reasons why adding the [-Werror] to > essentially all ACL_CHECK_ADD_COMPILE_FLAGS() calls? Our ./configure > script > is the only one implementing and supporting and using this macro, and it is > defined in configure.ac. > > If there are no foreseeable reason for that now, I think it's just fine to > change the AC_COMPILE_IFELSE() call to use CFLAGS="$1 -Werror $old_cflags". > Just add a comment in that area why we add the -Werror explicitly. This > makes > the whole change smaller and clearer. If we need the flexibility later > on, we > can adjust this when that time arrives. > Agreed. v2 follows. Selva ___ Openvpn-devel mailing list Openvpn-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/openvpn-devel
Re: [Openvpn-devel] [PATCH v3] wintun: add --windows-driver config option
Hi,
On Tue, Nov 19, 2019 at 3:29 AM Lev Stipakov wrote:
> Hello,
>
> ti 19. marrask. 2019 klo 9.37 Gert Doering (g...@greenie.muc.de)
> kirjoitti:
>
> > Looks like this will most likely break any dhcp-related options
>> > in the client config.. Say "dhcp-option DNS xxx".
>
>
> Oops, indeed. When I add those to client config, I got:
>
> > Options error: --dhcp-options requires --ip-win32 dynamic or adaptive
>
>
>> We currently require ip_win32_type to be adaptive or dynamic when such
>> options are specified.
>>
>
> That limitation has been presented since the very first commit in github:
>
> if (options->tuntap_options.dhcp_options
> && options->tuntap_options.ip_win32_type != IPW32_SET_DHCP_MASQ
> && options->tuntap_options.ip_win32_type != IPW32_SET_ADAPTIVE)
> {
> msg(M_USAGE, "--dhcp-options requires --ip-win32 dynamic or
> adaptive");
> }
>
> I commented it out and client-side --dhcp-options worked for me. Any ideas
> why we have this code?
>
>
>> > I think we should set this to adaptive here, and then handle it in tun.c
>>
>
> Actually, in follow-up commit I have changed IPAPI to NETSH - if I
> remember correctly
> I could not set up routing using former, although it works when done by
> iserv.
>
> If we don't know why we have that check and we could apply dhcp options
> from client config
> without it, shouldn't we just get rid of it?
>
>
>> >
>> > Although there is no dhcp when wintun is used, we'll still support
>> > dhcp-options such as DNS etc using netsh or service, right?
>>
>
> That's correct. At the moment we use:
>
> - netsh when running openvpn without interactive service
> - iserv, which itself uses both IPAPI (for setting up IP address and
> routing) and netsh (for setting up DNS)
>
The ip-win32 mess does need clenup but at least it would be nice to use the
same API/method in iservice and the core. Can we figure out why IP helper
API is not working for setting IP? Unfortunately there is no API for
setting DNS (not that I know of) so there the use of netsh is unavoidable.
Anyway, wintun + dhcp-option in client config is broken right now. Looks
like it may work when dhcp-option is pushed (not tested) but there are
cases where we do use it directly in the config.
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, 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] Fix ACL_CHECK_ADD_COMPILE_FLAGS to work with clang
On 14/11/2019 22:58, Selva Nair wrote: > Hi David > > Thanks for the comments > > My idea was just to add -Werror right in the line above, and not extend > the > ACL_CHECK_ADD_COMPILE_FLAGS macro with another argument. > > > I'm fine with that approach as well. Let me know if you want a v2. Sorry for the delay responding. I've done some pondering, and I have come to the same conclusion as Arne, so this is a reasonable approach to add -Werror. That said, is there any specific reasons why adding the [-Werror] to essentially all ACL_CHECK_ADD_COMPILE_FLAGS() calls? Our ./configure script is the only one implementing and supporting and using this macro, and it is defined in configure.ac. If there are no foreseeable reason for that now, I think it's just fine to change the AC_COMPILE_IFELSE() call to use CFLAGS="$1 -Werror $old_cflags". Just add a comment in that area why we add the -Werror explicitly. This makes the whole change smaller and clearer. If we need the flexibility later on, we can adjust this when that time arrives. -- kind regards, David Sommerseth OpenVPN Inc signature.asc Description: OpenPGP digital 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, 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 v3] wintun: add --windows-driver config option
Hello,
ti 19. marrask. 2019 klo 9.37 Gert Doering (g...@greenie.muc.de) kirjoitti:
> Looks like this will most likely break any dhcp-related options
> > in the client config.. Say "dhcp-option DNS xxx".
Oops, indeed. When I add those to client config, I got:
> Options error: --dhcp-options requires --ip-win32 dynamic or adaptive
> We currently require ip_win32_type to be adaptive or dynamic when such
> options are specified.
>
That limitation has been presented since the very first commit in github:
if (options->tuntap_options.dhcp_options
&& options->tuntap_options.ip_win32_type != IPW32_SET_DHCP_MASQ
&& options->tuntap_options.ip_win32_type != IPW32_SET_ADAPTIVE)
{
msg(M_USAGE, "--dhcp-options requires --ip-win32 dynamic or
adaptive");
}
I commented it out and client-side --dhcp-options worked for me. Any ideas
why we have this code?
> > I think we should set this to adaptive here, and then handle it in tun.c
>
Actually, in follow-up commit I have changed IPAPI to NETSH - if I remember
correctly
I could not set up routing using former, although it works when done by
iserv.
If we don't know why we have that check and we could apply dhcp options
from client config
without it, shouldn't we just get rid of it?
> >
> > Although there is no dhcp when wintun is used, we'll still support
> > dhcp-options such as DNS etc using netsh or service, right?
>
That's correct. At the moment we use:
- netsh when running openvpn without interactive service
- iserv, which itself uses both IPAPI (for setting up IP address and
routing) and netsh (for setting up DNS)
I had the same concerns, and if I remember right (I haven't looked just now)
> one of the later patches adds the missing bits to the iservice.
>
Yep, patch 6/7 https://patchwork.openvpn.net/patch/883/
--
-Lev
___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel
