diff mbox series

[Openvpn-devel] tun: improve DHCP server address calculation for small subnets

Message ID 20211108135314.148-1-lstipakov@gmail.com
State Superseded
Headers
Series [Openvpn-devel] tun: improve DHCP server address calculation for small subnets |

Commit Message

Lev Stipakov Nov. 8, 2021, 2:53 a.m. UTC 
  From: Lev Stipakov <lev@openvpn.net>

When /30 subnet is pushed (like in the case of OpenVPN Cloud),
DHCP server address is calculated to be the same as local address,
which causes collision and therefore connection is not established.

To fix that, use openvpn3 approach, which sets DHCP server
address to a network address for small subnets

Signed-off-by: Lev Stipakov <lev@openvpn.net>
---
 src/openvpn/tun.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

Comments

Lev Stipakov Nov. 10, 2021, 12:02 a.m. UTC | #1 
This can be considered as a bugfix ("Cannot use openvpn2 as OpenVPN
Cloud connector") so could go into release/2.5 too.

ke 10. marrask. 2021 klo 12.59 Lev Stipakov (lstipakov@gmail.com) kirjoitti:
>
> From: Lev Stipakov <lev@openvpn.net>
>
> When /30 subnet is pushed (like in the case of OpenVPN Cloud),
> DHCP server address is calculated to be the same as local address,
> which causes collision and therefore connection is not established.
>
> To fix that, use openvpn3 approach, which sets DHCP server
> address to a network address for small subnets
>
> Signed-off-by: Lev Stipakov <lev@openvpn.net>
> ---
>  src/openvpn/tun.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/src/openvpn/tun.c b/src/openvpn/tun.c
> index 28f803ec..994e3751 100644
> --- a/src/openvpn/tun.c
> +++ b/src/openvpn/tun.c
> @@ -6363,7 +6363,9 @@ tuntap_dhcp_mask(const struct tuntap *tt, const char *device_guid)
>              }
>              else
>              {
> -                ep[2] = dhcp_masq_addr(tt->local, tt->remote_netmask, -1);
> +                int prefix_len = netmask_to_netbits2(tt->adapter_netmask);
> +                /* use network address as DHCP server for small subnets, otherwise last address before broadcast */
> +                ep[2] = dhcp_masq_addr(tt->local, tt->remote_netmask, prefix_len < 28 ? -1 : 0);
>              }
>          }
>          else
> --
> 2.23.0.windows.1
>
Selva Nair Nov. 10, 2021, 7:46 a.m. UTC | #2 
Hi,

On Wed, Nov 10, 2021 at 6:01 AM Lev Stipakov <lstipakov@gmail.com> wrote:

> From: Lev Stipakov <lev@openvpn.net>
>
> When /30 subnet is pushed (like in the case of OpenVPN Cloud),
> DHCP server address is calculated to be the same as local address,
> which causes collision and therefore connection is not established.
>
> To fix that, use openvpn3 approach, which sets DHCP server
> address to a network address for small subnets
>
> Signed-off-by: Lev Stipakov <lev@openvpn.net>
> ---
>  src/openvpn/tun.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/src/openvpn/tun.c b/src/openvpn/tun.c
> index 28f803ec..994e3751 100644
> --- a/src/openvpn/tun.c
> +++ b/src/openvpn/tun.c
> @@ -6363,7 +6363,9 @@ tuntap_dhcp_mask(const struct tuntap *tt, const char
> *device_guid)
>              }
>              else
>              {
> -                ep[2] = dhcp_masq_addr(tt->local, tt->remote_netmask, -1);
> +                int prefix_len = netmask_to_netbits2(tt->adapter_netmask);
> +                /* use network address as DHCP server for small subnets,
> otherwise last address before broadcast */
> +                ep[2] = dhcp_masq_addr(tt->local, tt->remote_netmask,
> prefix_len < 28 ? -1 : 0);
>              }
>          }
>          else
> --


Why not just use 0 offset always? Perpetuating this dance of 0 offset in
some cases, -1 otherwise is not a way forward. Also see my patch from 2015
that never got any traction. I have lost touch with the context, so, I'm
not sure whether this addresses the same (apart from code changes) or
something related.

https://sourceforge.net/p/openvpn/mailman/openvpn-devel/thread/1449454660-20050-1-git-send-email-selva.nair%40gmail.com/#msg34674818

Also:
https://www.mail-archive.com/openvpn-devel@lists.sourceforge.net/msg13413.html
and other messages in that thread.

Selva
<div dir="ltr"><div dir="ltr">Hi,</div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Wed, Nov 10, 2021 at 6:01 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>
When /30 subnet is pushed (like in the case of OpenVPN Cloud),<br>
DHCP server address is calculated to be the same as local address,<br>
which causes collision and therefore connection is not established.<br>
<br>
To fix that, use openvpn3 approach, which sets DHCP server<br>
address to a network address for small subnets<br>
<br>
Signed-off-by: Lev Stipakov &lt;<a href="mailto:lev@openvpn.net" target="_blank">lev@openvpn.net</a>&gt;<br>
---<br>
 src/openvpn/tun.c | 4 +++-<br>
 1 file changed, 3 insertions(+), 1 deletion(-)<br>
<br>
diff --git a/src/openvpn/tun.c b/src/openvpn/tun.c<br>
index 28f803ec..994e3751 100644<br>
--- a/src/openvpn/tun.c<br>
+++ b/src/openvpn/tun.c<br>
@@ -6363,7 +6363,9 @@ tuntap_dhcp_mask(const struct tuntap *tt, const char *device_guid)<br>
             }<br>
             else<br>
             {<br>
-                ep[2] = dhcp_masq_addr(tt-&gt;local, tt-&gt;remote_netmask, -1);<br>
+                int prefix_len = netmask_to_netbits2(tt-&gt;adapter_netmask);<br>
+                /* use network address as DHCP server for small subnets, otherwise last address before broadcast */<br>
+                ep[2] = dhcp_masq_addr(tt-&gt;local, tt-&gt;remote_netmask, prefix_len &lt; 28 ? -1 : 0);<br>
             }<br>
         }<br>
         else<br>
--</blockquote><div><br></div><div>Why not just use 0 offset always? Perpetuating this dance of 0 offset in some cases, -1 otherwise is not a way forward. Also see my patch from 2015 that never got any traction. I have lost touch with the context, so, I&#39;m not sure whether this addresses the same (apart from code changes) or something related.</div><div><br></div><div><a href="https://sourceforge.net/p/openvpn/mailman/openvpn-devel/thread/1449454660-20050-1-git-send-email-selva.nair%40gmail.com/#msg34674818">https://sourceforge.net/p/openvpn/mailman/openvpn-devel/thread/1449454660-20050-1-git-send-email-selva.nair%40gmail.com/#msg34674818</a><br></div><div><br></div><div>Also: <a href="https://www.mail-archive.com/openvpn-devel@lists.sourceforge.net/msg13413.html">https://www.mail-archive.com/openvpn-devel@lists.sourceforge.net/msg13413.html</a> and other messages in that thread.</div><div><br></div><div>Selva</div></div></div>
Lev Stipakov Nov. 10, 2021, 9:38 p.m. UTC | #3 
Hi,

> Why not just use 0 offset always? Perpetuating this dance of 0 offset in some cases, -1 otherwise is not a way forward. Also see my patch from 2015 that never got any traction. I have lost touch with the context, so, I'm not sure whether this addresses the same (apart from code changes) or something related.
>
> https://sourceforge.net/p/openvpn/mailman/openvpn-devel/thread/1449454660-20050-1-git-send-email-selva.nair%40gmail.com/#msg34674818
>
> Also: https://www.mail-archive.com/openvpn-devel@lists.sourceforge.net/msg13413.html and other messages in that thread.

Thanks for pointing me out to this, obviously I missed your original
patch and that discussion.

I don't know why there is an offset in the first place for both
openvpn2 and openvpn3, there is probably some reason for it? I wanted
to be on the safe side and use a network address for some cases only
(small subnet). If you think it is safe to always use 0, let's do that
- I can send V2.

I tried to apply your patch but it doesn't apply anymore - tun.c has
changed a lot since 2015.

What comes to discussion about getting rid of the offset option -
since the syntax is like this:

  --ip-win32 dynamic [offset] [lease-time]

we cannot just remove [offset] since we might break configurations
which set [lease-time].
diff mbox series

Patch

diff --git a/src/openvpn/tun.c b/src/openvpn/tun.c
index 28f803ec..994e3751 100644
--- a/src/openvpn/tun.c
+++ b/src/openvpn/tun.c
@@ -6363,7 +6363,9 @@  tuntap_dhcp_mask(const struct tuntap *tt, const char *device_guid)
             }
             else
             {
-                ep[2] = dhcp_masq_addr(tt->local, tt->remote_netmask, -1);
+                int prefix_len = netmask_to_netbits2(tt->adapter_netmask);
+                /* use network address as DHCP server for small subnets, otherwise last address before broadcast */
+                ep[2] = dhcp_masq_addr(tt->local, tt->remote_netmask, prefix_len < 28 ? -1 : 0);
             }
         }
         else