| 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 | expand |
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
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 <<a href="mailto:lstipakov@gmail.com" target="_blank">lstipakov@gmail.com</a>> 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 <<a href="mailto:lev@openvpn.net" target="_blank">lev@openvpn.net</a>><br> <br> This is the rebase of original Selva Nair's patch<br> which hasn'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'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 "offset 0" 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 "rule" 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 <<a href="mailto:lev@openvpn.net" target="_blank">lev@openvpn.net</a>><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 > :code:`-256`<br> - and < :code:`256` and which defaults to -1. If offset is positive,<br> + and < :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 "route-gateway 10.8.0.1"<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->ifconfig_pool_defined = true;<br> o->ifconfig_pool_start = o->server_network + 2;<br> - o->ifconfig_pool_end = (o->server_network | ~o->server_netmask) - 2;<br> + o->ifconfig_pool_end = (o->server_network | ~o->server_netmask) - 1;<br> ifconfig_pool_verify_range(M_USAGE, o->ifconfig_pool_start, o->ifconfig_pool_end);<br> }<br> o->ifconfig_pool_netmask = o->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->topology == TOP_SUBNET)<br> {<br> - if (tt->options.dhcp_masq_custom_offset)<br> - {<br> - ep[2] = dhcp_masq_addr(tt->local, tt->remote_netmask, tt->options.dhcp_masq_offset);<br> - }<br> - else<br> - {<br> - ep[2] = dhcp_masq_addr(tt->local, tt->remote_netmask, -1);<br> - }<br> + ep[2] = dhcp_masq_addr(tt->local, tt->remote_netmask, tt->options.dhcp_masq_custom_offset ? tt->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>
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?
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
> > 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
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
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 {