[Openvpn-devel] Use network address for emulated DHCP server as a default

Message ID 20211109015927.311-1-lstipakov@gmail.com
State Accepted
Headers show
Series
  • [Openvpn-devel] Use network address for emulated DHCP server as a default
Related show

Commit Message

Lev Stipakov Nov. 9, 2021, 1:59 a.m.
From: Lev Stipakov <lev@openvpn.net>

This is the rebase of original Selva Nair's patch
which hasn't been merged:

  https://sourceforge.net/p/openvpn/mailman/message/34674818/

and documentation change to reflect code changes, which
is basically a revert of another Selva's patch (which got merged):

  https://www.mail-archive.com/openvpn-devel@lists.sourceforge.net/msg13387.html

For subnet topology use "offset 0" as default for
calculating DHCP server address, which makes it equal
to the network address.

There is no know reason why non-zero default offset
is needed. Besides, offset -1 breaks subnet /30 case,
which in some cases is pushed by OpenVPN Cloud product.

Signed-off-by: Lev Stipakov <lev@openvpn.net>
---
 doc/man-sections/windows-options.rst | 2 +-
 src/openvpn/helper.c                 | 4 ++--
 src/openvpn/tun.c                    | 9 +--------
 3 files changed, 4 insertions(+), 11 deletions(-)

Comments

Gert Doering Nov. 11, 2021, 4:39 p.m. | #1
Hi,

On Tue, Nov 09, 2021 at 03:59:27AM +0200, Lev Stipakov wrote:
> For subnet topology use "offset 0" as default for
> calculating DHCP server address, which makes it equal
> to the network address.

I assume this has been tested across all windows platforms, including
Windows 7?

On the patch itself, the code change looks good, I just need confirmation
about actual testing (with /30 and larger netmasks) across the variety
of platforms we support.

gert
Selva Nair Nov. 11, 2021, 5:03 p.m. | #2
Hi

On Thu, Nov 11, 2021 at 4:41 AM Lev Stipakov <lstipakov@gmail.com> wrote:

> From: Lev Stipakov <lev@openvpn.net>
>
> This is the rebase of original Selva Nair's patch
> which hasn't been merged:
>
>   https://sourceforge.net/p/openvpn/mailman/message/34674818/


Yes, something I wanted 5 years ago


>
> and documentation change to reflect code changes, which
> is basically a revert of another Selva's patch (which got merged):
>
>
> https://www.mail-archive.com/openvpn-devel@lists.sourceforge.net/msg13387.html


That was a compromise as the offset patch got no traction..


>
>
> For subnet topology use "offset 0" as default for
> calculating DHCP server address, which makes it equal
> to the network address.
>
> There is no know reason why non-zero default offset
> is needed. Besides, offset -1 breaks subnet /30 case,
> which in some cases is pushed by OpenVPN Cloud product.
>

I could not sell this as a bugfix that time as /30 in subnet mode was not
possible with our server mode as the pool would have been empty in that
case without the patch. Also /30 in subnet mode was considered somewhat
silly.

The reason for the original -1 is probably historical -- the general "rule"
not to use network address as a source address.


>
> Signed-off-by: Lev Stipakov <lev@openvpn.net>
> ---
>  doc/man-sections/windows-options.rst | 2 +-
>  src/openvpn/helper.c                 | 4 ++--
>  src/openvpn/tun.c                    | 9 +--------
>  3 files changed, 4 insertions(+), 11 deletions(-)
>
> diff --git a/doc/man-sections/windows-options.rst
> b/doc/man-sections/windows-options.rst
> index eacb9af8..c389fbc4 100644
> --- a/doc/man-sections/windows-options.rst
> +++ b/doc/man-sections/windows-options.rst
> @@ -93,7 +93,7 @@ Windows-Specific Options
>          server to masquerade as if it were coming from the remote
> endpoint.
>
>          The optional offset parameter is an integer which is >
> :code:`-256`
> -        and < :code:`256` and which defaults to -1. If offset is positive,
> +        and < :code:`256` and which defaults to 0. If offset is positive,
>          the DHCP server will masquerade as the IP address at network
>          address + offset. If offset is negative, the DHCP server will
>          masquerade as the IP address at broadcast address + offset.
> diff --git a/src/openvpn/helper.c b/src/openvpn/helper.c
> index 032a71e8..4ac1cf8e 100644
> --- a/src/openvpn/helper.c
> +++ b/src/openvpn/helper.c
> @@ -237,7 +237,7 @@ helper_client_server(struct options *o)
>       * if tap OR (tun AND topology == subnet):
>       *   ifconfig 10.8.0.1 255.255.255.0
>       *   if !nopool:
> -     *     ifconfig-pool 10.8.0.2 10.8.0.253 255.255.255.0
> +     *     ifconfig-pool 10.8.0.2 10.8.0.254 255.255.255.0
>       *   push "route-gateway 10.8.0.1"
>       *   if route-gateway unset:
>       *     route-gateway 10.8.0.2
> @@ -340,7 +340,7 @@ helper_client_server(struct options *o)
>                  {
>                      o->ifconfig_pool_defined = true;
>                      o->ifconfig_pool_start = o->server_network + 2;
> -                    o->ifconfig_pool_end = (o->server_network |
> ~o->server_netmask) - 2;
> +                    o->ifconfig_pool_end = (o->server_network |
> ~o->server_netmask) - 1;
>                      ifconfig_pool_verify_range(M_USAGE,
> o->ifconfig_pool_start, o->ifconfig_pool_end);
>                  }
>                  o->ifconfig_pool_netmask = o->server_netmask;
> diff --git a/src/openvpn/tun.c b/src/openvpn/tun.c
> index 28f803ec..75d5eaf7 100644
> --- a/src/openvpn/tun.c
> +++ b/src/openvpn/tun.c
> @@ -6357,14 +6357,7 @@ tuntap_dhcp_mask(const struct tuntap *tt, const
> char *device_guid)
>      {
>          if (tt->topology == TOP_SUBNET)
>          {
> -            if (tt->options.dhcp_masq_custom_offset)
> -            {
> -                ep[2] = dhcp_masq_addr(tt->local, tt->remote_netmask,
> tt->options.dhcp_masq_offset);
> -            }
> -            else
> -            {
> -                ep[2] = dhcp_masq_addr(tt->local, tt->remote_netmask, -1);
> -            }
> +            ep[2] = dhcp_masq_addr(tt->local, tt->remote_netmask,
> tt->options.dhcp_masq_custom_offset ? tt->options.dhcp_masq_offset : 0);
>          }
>          else
>          {
> --
> 2.23.0.windows.1
>

Looks good to me based on my recollection of testing this a long time ago
-- I have not re-tested this version of the patch.

(An ACK from me may look like an attempt to get in via the backdoor, so
resisting that..).

Selva
<div dir="ltr"><div>Hi</div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Thu, Nov 11, 2021 at 4:41 AM Lev Stipakov &lt;<a href="mailto:lstipakov@gmail.com" target="_blank">lstipakov@gmail.com</a>&gt; wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">From: Lev Stipakov &lt;<a href="mailto:lev@openvpn.net" target="_blank">lev@openvpn.net</a>&gt;<br>
<br>
This is the rebase of original Selva Nair&#39;s patch<br>
which hasn&#39;t been merged:<br>
<br>
  <a href="https://sourceforge.net/p/openvpn/mailman/message/34674818/" rel="noreferrer" target="_blank">https://sourceforge.net/p/openvpn/mailman/message/34674818/</a></blockquote><div><br></div><div>Yes, something I wanted 5 years ago</div><div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><br>
<br>
and documentation change to reflect code changes, which<br>
is basically a revert of another Selva&#39;s patch (which got merged):<br>
<br>
  <a href="https://www.mail-archive.com/openvpn-devel@lists.sourceforge.net/msg13387.html" rel="noreferrer" target="_blank">https://www.mail-archive.com/openvpn-devel@lists.sourceforge.net/msg13387.html</a></blockquote><div><br></div><div>That was a compromise as the offset patch got no traction..</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><br>
<br>
For subnet topology use &quot;offset 0&quot; as default for<br>
calculating DHCP server address, which makes it equal<br>
to the network address.<br>
<br>
There is no know reason why non-zero default offset<br>
is needed. Besides, offset -1 breaks subnet /30 case,<br>
which in some cases is pushed by OpenVPN Cloud product.<br></blockquote><div><br></div><div>I could not sell this as a bugfix that time as /30 in subnet mode was not possible with our server mode as the pool would have been empty in that case without the patch. Also /30 in subnet mode was considered somewhat silly.</div><div><br></div><div>The reason for the original -1 is probably historical -- the general &quot;rule&quot; not to use network address as a source address.</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
Signed-off-by: Lev Stipakov &lt;<a href="mailto:lev@openvpn.net" target="_blank">lev@openvpn.net</a>&gt;<br>
---<br>
 doc/man-sections/windows-options.rst | 2 +-<br>
 src/openvpn/helper.c                 | 4 ++--<br>
 src/openvpn/tun.c                    | 9 +--------<br>
 3 files changed, 4 insertions(+), 11 deletions(-)<br>
<br>
diff --git a/doc/man-sections/windows-options.rst b/doc/man-sections/windows-options.rst<br>
index eacb9af8..c389fbc4 100644<br>
--- a/doc/man-sections/windows-options.rst<br>
+++ b/doc/man-sections/windows-options.rst<br>
@@ -93,7 +93,7 @@ Windows-Specific Options<br>
         server to masquerade as if it were coming from the remote endpoint.<br>
<br>
         The optional offset parameter is an integer which is &gt; :code:`-256`<br>
-        and &lt; :code:`256` and which defaults to -1. If offset is positive,<br>
+        and &lt; :code:`256` and which defaults to 0. If offset is positive,<br>
         the DHCP server will masquerade as the IP address at network<br>
         address + offset. If offset is negative, the DHCP server will<br>
         masquerade as the IP address at broadcast address + offset.<br>
diff --git a/src/openvpn/helper.c b/src/openvpn/helper.c<br>
index 032a71e8..4ac1cf8e 100644<br>
--- a/src/openvpn/helper.c<br>
+++ b/src/openvpn/helper.c<br>
@@ -237,7 +237,7 @@ helper_client_server(struct options *o)<br>
      * if tap OR (tun AND topology == subnet):<br>
      *   ifconfig 10.8.0.1 255.255.255.0<br>
      *   if !nopool:<br>
-     *     ifconfig-pool 10.8.0.2 10.8.0.253 255.255.255.0<br>
+     *     ifconfig-pool 10.8.0.2 10.8.0.254 255.255.255.0<br>
      *   push &quot;route-gateway 10.8.0.1&quot;<br>
      *   if route-gateway unset:<br>
      *     route-gateway 10.8.0.2<br>
@@ -340,7 +340,7 @@ helper_client_server(struct options *o)<br>
                 {<br>
                     o-&gt;ifconfig_pool_defined = true;<br>
                     o-&gt;ifconfig_pool_start = o-&gt;server_network + 2;<br>
-                    o-&gt;ifconfig_pool_end = (o-&gt;server_network | ~o-&gt;server_netmask) - 2;<br>
+                    o-&gt;ifconfig_pool_end = (o-&gt;server_network | ~o-&gt;server_netmask) - 1;<br>
                     ifconfig_pool_verify_range(M_USAGE, o-&gt;ifconfig_pool_start, o-&gt;ifconfig_pool_end);<br>
                 }<br>
                 o-&gt;ifconfig_pool_netmask = o-&gt;server_netmask;<br>
diff --git a/src/openvpn/tun.c b/src/openvpn/tun.c<br>
index 28f803ec..75d5eaf7 100644<br>
--- a/src/openvpn/tun.c<br>
+++ b/src/openvpn/tun.c<br>
@@ -6357,14 +6357,7 @@ tuntap_dhcp_mask(const struct tuntap *tt, const char *device_guid)<br>
     {<br>
         if (tt-&gt;topology == TOP_SUBNET)<br>
         {<br>
-            if (tt-&gt;options.dhcp_masq_custom_offset)<br>
-            {<br>
-                ep[2] = dhcp_masq_addr(tt-&gt;local, tt-&gt;remote_netmask, tt-&gt;options.dhcp_masq_offset);<br>
-            }<br>
-            else<br>
-            {<br>
-                ep[2] = dhcp_masq_addr(tt-&gt;local, tt-&gt;remote_netmask, -1);<br>
-            }<br>
+            ep[2] = dhcp_masq_addr(tt-&gt;local, tt-&gt;remote_netmask, tt-&gt;options.dhcp_masq_custom_offset ? tt-&gt;options.dhcp_masq_offset : 0);<br>
         }<br>
         else<br>
         {<br>
-- <br>
2.23.0.windows.1<br></blockquote><div><br></div><div>Looks good to me based on my recollection of testing this a long time ago -- I have not re-tested this version of the patch.</div><div><br></div><div>(An ACK from me may look like an attempt to get in via the backdoor, so resisting that..).</div><div><br></div><div>Selva </div></div></div>
Lev Stipakov Nov. 12, 2021, 12:33 p.m. | #3
Hi,

I tested yesterday on Windows 10 and today also on Windows 7 and 8.1.

 - /30 subnet (OpenVPN Cloud as server)
 - /24 subnet (openvpn2) with redirect-gateway def1. Tested that ping
works and traffic goes through the tunnel.

Also our QA tested this (not exactly this one, but my original patch
with offset 0 for /28 and -1 for everything else) for
/30 subnet against OpenVPN Cloud. He has checked the connection,
access to the internet (e.g. ping 8.8.8.8), access to OpenVPN Cloud
(ping by internal IP).

I asked James what was the reason to have "-1" in the first place. He
said that he doesn't recall details, but thinks that the safest
approach would probably be to retain the old logic.

How about we apply my original patch (0 for /28 and -1 for the rest)
to 2.5 and this one to master?
Gert Doering Nov. 12, 2021, 12:56 p.m. | #4
Hi,

On Fri, Nov 12, 2021 at 02:33:31PM +0200, Lev Stipakov wrote:
> How about we apply my original patch (0 for /28 and -1 for the rest)
> to 2.5 and this one to master?

I tend to go for "make it simple and consistent", which would make this
"0 everywhere, if it breaks, the option is still available to set it to -1".

But with your testing, it should be fine on all platforms we build for
(so, yes, it might break XP or Vista, or 3.1 :-))

gert
Selva Nair Nov. 12, 2021, 1:22 p.m. | #5
>
> How about we apply my original patch (0 for /28 and -1 for the rest)
> to 2.5 and this one to master?

I do not see the logic behind that. If there are any platforms where 0
(network address) is not acceptable, it's unlikely to work for /30 as
well. /31 is special, /30 is not. And, based on your tests 0 does work
on all platforms we support. Same was my conclusion the last time I
checked.

So I would also vote for the current patch as is.

Selva
Gert Doering Nov. 12, 2021, 4:08 p.m. | #6
Acked-by: Gert Doering <gert@greenie.muc.de>

Not sure, really, why this did not get enough traction 5 years ago... so
thanks for pushing it :-) - and I understand that Selva did not want to
ACK it.

I have not tested it, just stared a bit at the code (fine) and test
compiled on MinGW (also fine).

Getting rid of the if() statement in tun.c is okay, though the 
resulting line is violating our line length... this is a big ugly.
(Also, just passing in "tt->options.dhcp_masq_offset" always (as it 
defaults to "0") would do the job fine now...)

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

commit 7df6583d76fc2ff485186ede75f00c9b7dc3e42c (master)
commit 93e4f286b906391a21697e44a02a00b871b977ba (release/2.5)
Author: Lev Stipakov
Date:   Tue Nov 9 03:59:27 2021 +0200

     Use network address for emulated DHCP server as a default

     Signed-off-by: Lev Stipakov <lev@openvpn.net>
     Acked-by: Gert Doering <gert@greenie.muc.de>
     Message-Id: <20211109015927.311-1-lstipakov@gmail.com>
     URL: https://www.mail-archive.com/openvpn-devel@lists.sourceforge.net/msg23156.html
     Signed-off-by: Gert Doering <gert@greenie.muc.de>


--
kind regards,

Gert Doering

Patch

diff --git a/doc/man-sections/windows-options.rst b/doc/man-sections/windows-options.rst
index eacb9af8..c389fbc4 100644
--- a/doc/man-sections/windows-options.rst
+++ b/doc/man-sections/windows-options.rst
@@ -93,7 +93,7 @@  Windows-Specific Options
         server to masquerade as if it were coming from the remote endpoint.
 
         The optional offset parameter is an integer which is > :code:`-256`
-        and < :code:`256` and which defaults to -1. If offset is positive,
+        and < :code:`256` and which defaults to 0. If offset is positive,
         the DHCP server will masquerade as the IP address at network
         address + offset. If offset is negative, the DHCP server will
         masquerade as the IP address at broadcast address + offset.
diff --git a/src/openvpn/helper.c b/src/openvpn/helper.c
index 032a71e8..4ac1cf8e 100644
--- a/src/openvpn/helper.c
+++ b/src/openvpn/helper.c
@@ -237,7 +237,7 @@  helper_client_server(struct options *o)
      * if tap OR (tun AND topology == subnet):
      *   ifconfig 10.8.0.1 255.255.255.0
      *   if !nopool:
-     *     ifconfig-pool 10.8.0.2 10.8.0.253 255.255.255.0
+     *     ifconfig-pool 10.8.0.2 10.8.0.254 255.255.255.0
      *   push "route-gateway 10.8.0.1"
      *   if route-gateway unset:
      *     route-gateway 10.8.0.2
@@ -340,7 +340,7 @@  helper_client_server(struct options *o)
                 {
                     o->ifconfig_pool_defined = true;
                     o->ifconfig_pool_start = o->server_network + 2;
-                    o->ifconfig_pool_end = (o->server_network | ~o->server_netmask) - 2;
+                    o->ifconfig_pool_end = (o->server_network | ~o->server_netmask) - 1;
                     ifconfig_pool_verify_range(M_USAGE, o->ifconfig_pool_start, o->ifconfig_pool_end);
                 }
                 o->ifconfig_pool_netmask = o->server_netmask;
diff --git a/src/openvpn/tun.c b/src/openvpn/tun.c
index 28f803ec..75d5eaf7 100644
--- a/src/openvpn/tun.c
+++ b/src/openvpn/tun.c
@@ -6357,14 +6357,7 @@  tuntap_dhcp_mask(const struct tuntap *tt, const char *device_guid)
     {
         if (tt->topology == TOP_SUBNET)
         {
-            if (tt->options.dhcp_masq_custom_offset)
-            {
-                ep[2] = dhcp_masq_addr(tt->local, tt->remote_netmask, tt->options.dhcp_masq_offset);
-            }
-            else
-            {
-                ep[2] = dhcp_masq_addr(tt->local, tt->remote_netmask, -1);
-            }
+            ep[2] = dhcp_masq_addr(tt->local, tt->remote_netmask, tt->options.dhcp_masq_custom_offset ? tt->options.dhcp_masq_offset : 0);
         }
         else
         {