[Openvpn-devel] tun/iproute2: support 31-bit ipv4 prefix (RFC 3021)

Message ID 20191103053148.18059-1-tom.ty89@gmail.com
State New
Headers show
Series
  • [Openvpn-devel] tun/iproute2: support 31-bit ipv4 prefix (RFC 3021)
Related show

Commit Message

Tom Yan Nov. 3, 2019, 5:31 a.m.
---
 src/openvpn/tun.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

Comments

Gert Doering Nov. 3, 2019, 6:24 p.m. | #1
Hi,

On Sun, Nov 03, 2019 at 01:31:48PM +0800, Tom Yan wrote:
>  src/openvpn/tun.c | 7 ++++---
>  1 file changed, 4 insertions(+), 3 deletions(-)

I tend to NAK this, on a number of reasons - we support arbitrary
point-to-point links "since ever" if you do "topology p2p" (can be
out of the same /31, or just arbitrary addresses on both ends), so
I do not see why doing this in "topology subnet" would be beneficial.

OTOH, *if* you do it, it needs to be consistently, not just "linux/iproute"
(on linux, "sitnl" is the new gold standard), and the commit message should
explain a bit more about the "why do it this way" and "how do other 
platforms deal with it" (windows, *BSD, MacOS, etc.)

gert
Gert Doering Nov. 3, 2019, 6:33 p.m. | #2
Hi,

On Sun, Nov 03, 2019 at 07:24:43PM +0100, Gert Doering wrote:
> I tend to NAK this, on a number of reasons - we support arbitrary
> point-to-point links "since ever" if you do "topology p2p" (can be
> out of the same /31, or just arbitrary addresses on both ends), so
> I do not see why doing this in "topology subnet" would be beneficial.

I do see where you're coming from - the man page talks about depreciating
p2p, in which case you need to make sure topology subnet does /31s.

It might certainly be a useful excercise to investigate our current
ifconfig (etc) calls - in the 2.5 branch - and possibly get rid of all
the "broadcast" settings, across all platforms that do not need them.

Personally I've never understood why people are so keen on explicitly
configuring broadcast addresses everywhere (like in the network config
files, etc.) - the standard address can be computed and "just works"
(and on a tun interface, there are no link-layer broadcasts anyway, even 
if we pretend it were differently).

The code was that way when David and I inherited the project, so I can't
explain *why* it is - but this might be the opportunity to kick out a bit
of needless garbage.


Out of curiosity: does the sitnl code path handle /31s?

gert
Tom Yan Nov. 3, 2019, 7:54 p.m. | #3
While the commit message says "support" 31-bit prefix, this patch is a
bug fix by nature. Whether one can actually uses a /31 subnet for
*anything* (i.e. not just OpenVPN) pretty much depends entirely on the
platform itself. This patch is needed simply because broadcast address
does not "apply" in a /31 subnet, and having it set *prevents* it from
working. In fact, if openvpn tries to set the broadcast address with
`+` instead of an explicit address calculated by itself, `ip` can
handle it well. I do it with a prefix length check because I am not
sure if there's a reason that `broadcast +` wasn't used instead.

Yeah the removal of the p2p topology was one of the reasons. It's
actually the fact that ics-openvpn doesn't really parse point-to-point
ifconfig that made me aware of this.

I don't know anything about sitnl. Is it available only in 2.5/master?
While I have also sent the equivalent fix for that, it's merely a
"forwardport". I haven't actually used 2.5/master at all (unless
ics-openvpn counts, while I don't see broadcast being set for any case
on Android).

On Mon, 4 Nov 2019 at 02:33, Gert Doering <gert@greenie.muc.de> wrote:
>
> Hi,
>
> On Sun, Nov 03, 2019 at 07:24:43PM +0100, Gert Doering wrote:
> > I tend to NAK this, on a number of reasons - we support arbitrary
> > point-to-point links "since ever" if you do "topology p2p" (can be
> > out of the same /31, or just arbitrary addresses on both ends), so
> > I do not see why doing this in "topology subnet" would be beneficial.
>
> I do see where you're coming from - the man page talks about depreciating
> p2p, in which case you need to make sure topology subnet does /31s.
>
> It might certainly be a useful excercise to investigate our current
> ifconfig (etc) calls - in the 2.5 branch - and possibly get rid of all
> the "broadcast" settings, across all platforms that do not need them.
>
> Personally I've never understood why people are so keen on explicitly
> configuring broadcast addresses everywhere (like in the network config
> files, etc.) - the standard address can be computed and "just works"
> (and on a tun interface, there are no link-layer broadcasts anyway, even
> if we pretend it were differently).
>
> The code was that way when David and I inherited the project, so I can't
> explain *why* it is - but this might be the opportunity to kick out a bit
> of needless garbage.
>
>
> Out of curiosity: does the sitnl code path handle /31s?
>
> 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                             gert@greenie.muc.de
Gert Doering Nov. 3, 2019, 8:46 p.m. | #4
Hi,

On Mon, Nov 04, 2019 at 03:54:49AM +0800, Tom Yan wrote:
> While the commit message says "support" 31-bit prefix, this patch is a
> bug fix by nature. Whether one can actually uses a /31 subnet for
> *anything* (i.e. not just OpenVPN) pretty much depends entirely on the
> platform itself. This patch is needed simply because broadcast address
> does not "apply" in a /31 subnet, and having it set *prevents* it from
> working. 

Understood (thought it would be more helpful if the commit message
said so :-) ).

> In fact, if openvpn tries to set the broadcast address with
> `+` instead of an explicit address calculated by itself, `ip` can
> handle it well. I do it with a prefix length check because I am not
> sure if there's a reason that `broadcast +` wasn't used instead.

I wasn't aware that there is a "broadcast +" setting, but I'm way
more tempted to just get rid of setting broadcast at all.  This is a
computer, it can do the math itself.

> Yeah the removal of the p2p topology was one of the reasons. It's
> actually the fact that ics-openvpn doesn't really parse point-to-point
> ifconfig that made me aware of this.

I wasn't aware of that, but since Arne is reading here, maybe he can
comment on it.  It's possible that the Android VPN API just does not
permit p2p mode, but wants a subnet.

Arne, any idea if /31 works on Android?


> I don't know anything about sitnl. Is it available only in 2.5/master?
> While I have also sent the equivalent fix for that, it's merely a
> "forwardport". I haven't actually used 2.5/master at all (unless
> ics-openvpn counts, while I don't see broadcast being set for any case
> on Android).

Yeah, master.  New functionality always has to go into master, and
2.4 backporting is only done under certain conditions (undisputed
bugfixes, long-term compatibility changes).

Adding conditionals is not a bugfix.  Removing the "broadcast" part
from "ip add" could be considered as such... it needs testing on 
somewhat ancient Linux systems (I think we aim to support all 
RHEL/CentOS releases that are still supported, which will definitely
cover "OLD!!").  But seriously I expect this to be totally unnecesary
historic cruft unless told otherwise.

gert
Tom Yan Nov. 3, 2019, 9:10 p.m. | #5
So in master, when net_addr_v4_add is called in tun.c, which
net_addr_v4_add (networking_iproute2.c or networking_sitnl.c) is
actually used depends on how openvpn is built, right?

I think the `if (broadcast)` condition in the one of
networking_sitnl.c needs to be changed to `if (broadcast && prefixlen
< 31)` as well. Also it seems that net_addr_v4_add is called wrongly
in tun.c with `&tt->remote_netmask` instead of `&tt->broadcast` as the
last argument.

I don't really mind it being fixed with any of the approaches: no
broadcast, `broadcast +`, or prefix length check. Don't know if it
counts as a reference to you but I can even see in the code of
systemd-networkd similar prefix length check. As I have no idea
whether under any circumstances the broadcast address matters, the
prefix length check seems to be a more conservative approach to me and
that's why I chose it. (Also, the "conditional" added merely causes
"remove-broadcast-the-bugfix" to apply only on where it is strictly
necessary...) As for `broadcast +`, I am not sure if there's an
equivalence for sitnl, and I wonder if it raises version requirement
on iproute2.

For the record, /31 subnet works fine on Linux and Android as long as
the patch is applied (for Linux). (And again, from OpenVPN point of
view, we don't need to care if /31 subnet works on any platform, as we
never prevented users from setting that up. Well, other than the silly
code in the server directive, which should be fixed in a follow-up;
but it's not like users have to use that anyway.)

On Mon, 4 Nov 2019 at 04:46, Gert Doering <gert@greenie.muc.de> wrote:
>
> Hi,
>
> On Mon, Nov 04, 2019 at 03:54:49AM +0800, Tom Yan wrote:
> > While the commit message says "support" 31-bit prefix, this patch is a
> > bug fix by nature. Whether one can actually uses a /31 subnet for
> > *anything* (i.e. not just OpenVPN) pretty much depends entirely on the
> > platform itself. This patch is needed simply because broadcast address
> > does not "apply" in a /31 subnet, and having it set *prevents* it from
> > working.
>
> Understood (thought it would be more helpful if the commit message
> said so :-) ).
>
> > In fact, if openvpn tries to set the broadcast address with
> > `+` instead of an explicit address calculated by itself, `ip` can
> > handle it well. I do it with a prefix length check because I am not
> > sure if there's a reason that `broadcast +` wasn't used instead.
>
> I wasn't aware that there is a "broadcast +" setting, but I'm way
> more tempted to just get rid of setting broadcast at all.  This is a
> computer, it can do the math itself.
>
> > Yeah the removal of the p2p topology was one of the reasons. It's
> > actually the fact that ics-openvpn doesn't really parse point-to-point
> > ifconfig that made me aware of this.
>
> I wasn't aware of that, but since Arne is reading here, maybe he can
> comment on it.  It's possible that the Android VPN API just does not
> permit p2p mode, but wants a subnet.
>
> Arne, any idea if /31 works on Android?
>
>
> > I don't know anything about sitnl. Is it available only in 2.5/master?
> > While I have also sent the equivalent fix for that, it's merely a
> > "forwardport". I haven't actually used 2.5/master at all (unless
> > ics-openvpn counts, while I don't see broadcast being set for any case
> > on Android).
>
> Yeah, master.  New functionality always has to go into master, and
> 2.4 backporting is only done under certain conditions (undisputed
> bugfixes, long-term compatibility changes).
>
> Adding conditionals is not a bugfix.  Removing the "broadcast" part
> from "ip add" could be considered as such... it needs testing on
> somewhat ancient Linux systems (I think we aim to support all
> RHEL/CentOS releases that are still supported, which will definitely
> cover "OLD!!").  But seriously I expect this to be totally unnecesary
> historic cruft unless told otherwise.
>
> 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                             gert@greenie.muc.de
Arne Schwabe Nov. 4, 2019, 4:28 p.m. | #6
>> Yeah the removal of the p2p topology was one of the reasons. It's
>> actually the fact that ics-openvpn doesn't really parse point-to-point
>> ifconfig that made me aware of this.
> 
> I wasn't aware of that, but since Arne is reading here, maybe he can
> comment on it.  It's possible that the Android VPN API just does not
> permit p2p mode, but wants a subnet.
> 
> Arne, any idea if /31 works on Android?

Yes. But Android is special here anyway. It always wants a subnet mask
but the mask can be also be 255.255.255.255. And you also don't have a
gateway address and Android also does not set a route to the tun
interface (On Android 4.4+). So basically you always operate in p2p mode
and emulate other modes by adding routes to the tun interface.

Arne
Tom Yan Nov. 5, 2019, 3:08 a.m. | #7
Yeah I was merely referring to the trivial warning of the ifconfig mismatch. It's not exactly a problem but it just sort of made me aware of such thing as /31 subnet and noticed the bug on Linux. That's why I never opened an issue on ics-openvpn.
Tom Yan Nov. 5, 2019, 3:41 a.m. | #8
Hmm btw, not sure you are talking about the same thing, but at least
my device the "proto kernel scope link" subnet route is added for the
tun. While I can ping the "peer" out-of-the-box with /31 subnet and
the subnet topology, with p2p topology and 255.255.255.255 mask, I
need to add route in ics-openvpn for its peer before I can ping it.
(The server is configured with topology and ifconfig corresponds to
the client's in both tests.)

On Tue, 5 Nov 2019 at 01:01, Arne Schwabe <arne@rfc2549.org> wrote:
>
>  >> Yeah the removal of the p2p topology was one of the reasons. It's
> >> actually the fact that ics-openvpn doesn't really parse point-to-point
> >> ifconfig that made me aware of this.
> >
> > I wasn't aware of that, but since Arne is reading here, maybe he can
> > comment on it.  It's possible that the Android VPN API just does not
> > permit p2p mode, but wants a subnet.
> >
> > Arne, any idea if /31 works on Android?
>
> Yes. But Android is special here anyway. It always wants a subnet mask
> but the mask can be also be 255.255.255.255. And you also don't have a
> gateway address and Android also does not set a route to the tun
> interface (On Android 4.4+). So basically you always operate in p2p mode
> and emulate other modes by adding routes to the tun interface.
>
> Arne
>
> _______________________________________________
> Openvpn-devel mailing list
> Openvpn-devel@lists.sourceforge.net
> https://lists.sourceforge.net/lists/listinfo/openvpn-devel
Gert Doering Nov. 6, 2019, 2:28 p.m. | #9
Hi,

On Mon, Nov 04, 2019 at 05:10:12AM +0800, Tom Yan wrote:
> So in master, when net_addr_v4_add is called in tun.c, which
> net_addr_v4_add (networking_iproute2.c or networking_sitnl.c) is
> actually used depends on how openvpn is built, right?

Right.  It's either "default options" (-> sitnl.c) or "--with-iproute2"
(->networking_iproute2.c).

2.4 has "ifconfig or iproute2"
2.5 has "direct interface to netlink" (sitnl) or "iproute2"

> I think the `if (broadcast)` condition in the one of
> networking_sitnl.c needs to be changed to `if (broadcast && prefixlen
> < 31)` as well. 

I think this is more something the caller needs to decide "do I want to
set a broadcast address or not" (so, if /31, set broadcast = NULL).

OTOH, I'm still waiting for someone to tell me why we'd want to set
a broadcast address in the first place :-) - Antonio, you know the
kernel side better.  Why would anyone explicitly configure the standard
broadcast address ("all-ones in the host part of the relevant subnet")?

> Also it seems that net_addr_v4_add is called wrongly
> in tun.c with `&tt->remote_netmask` instead of `&tt->broadcast` as the
> last argument.

I tend to agree with you here.

Antonio, yours :-)  (though I ACKed it) - tun.c, line 1092.

We do mix up "remote" and "netmask" in the shared "remote_netmask"
variable, but "broadcast" is independent of that...  and OpenVPN
actually tells us so, in the "topology subnet" case...

   net_addr_v4_add: 10.194.6.2/24 brd 255.255.255.0 dev tun1

Linux seems to accept this, but it has no further consequences ("all
my tests pass")...

$ ip addr show dev tun1
520: tun1: <POINTOPOINT,MULTICAST,NOARP,UP,LOWER_UP> mtu 1500 qdisc pfifo_fast state UNKNOWN group default qlen 100
    link/none 
    inet 10.194.6.2/24 brd 255.255.255.0 scope global tun1
       valid_lft forever preferred_lft forever

... but it's still not correct.

gert
Gert Doering Nov. 10, 2019, 6:07 p.m. | #10
Hi,

On Mon, Nov 04, 2019 at 05:10:12AM +0800, Tom Yan wrote:
> So in master, when net_addr_v4_add is called in tun.c, which
> net_addr_v4_add (networking_iproute2.c or networking_sitnl.c) is
> actually used depends on how openvpn is built, right?

Can you have a look at the current "master" branch?  Antonio has ripped
out *all* broadcast setting - so this should now do /31s just fine?

More radical than your approach, but "less old code" is better than
"yet another special condition added" :-)

gert
Tom Yan Nov. 11, 2019, 3:12 p.m. | #11
Yes both iproute2 and sitnl works perfectly as expected. Will the fix
be backported to the 2.4 branch?

P.S. For the record, if the client is ics-openvpn with openvpn3,
somehow you need push route (either host or subnet will do) anyway.
This is not necessary with ics-openvpn with openvpn2 or OpenVPN
Connect (AFAIK its core is openvpn3 as well, albeit different
snapshot). The problem/behaviour is not specific to /31 subnet though,
apparently. I guess the main route table is ignored/masked.

On Mon, 11 Nov 2019 at 02:07, Gert Doering <gert@greenie.muc.de> wrote:
>
> Hi,
>
> On Mon, Nov 04, 2019 at 05:10:12AM +0800, Tom Yan wrote:
> > So in master, when net_addr_v4_add is called in tun.c, which
> > net_addr_v4_add (networking_iproute2.c or networking_sitnl.c) is
> > actually used depends on how openvpn is built, right?
>
> Can you have a look at the current "master" branch?  Antonio has ripped
> out *all* broadcast setting - so this should now do /31s just fine?
>
> More radical than your approach, but "less old code" is better than
> "yet another special condition added" :-)
>
> 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                             gert@greenie.muc.de
Arne Schwabe Nov. 11, 2019, 6:02 p.m. | #12
Am 11.11.19 um 16:12 schrieb Tom Yan:
> Yes both iproute2 and sitnl works perfectly as expected. Will the fix
> be backported to the 2.4 branch?
> 
> P.S. For the record, if the client is ics-openvpn with openvpn3,
> somehow you need push route (either host or subnet will do) anyway.

I am not aware of this issue. Could you go more into detail?

> This is not necessary with ics-openvpn with openvpn2 or OpenVPN
> Connect (AFAIK its core is openvpn3 as well, albeit different
> snapshot). The problem/behaviour is not specific to /31 subnet though,
> apparently. I guess the main route table is ignored/masked.

ics-openvpn is a) OpenVPN master b) uses a completely different logic to
handle network config (in OpenVPNService.java). So this patch does not
affect ics-openvpn in any way.

Arne
Tom Yan Nov. 13, 2019, 3:35 a.m. | #13
Details like...?

If you configure/push an IP with a subnet (i.e. non /32 prefix or
255.255.255.255 subnet mask) to a client, a subnet route will be added
to main route table (as show by `ip r`) by the kernel just like on
Desktop Linux (at least that's what would happen on my device). When I
use ics-openvpn without openvpn3 box checked or OpenVPN Connect, the
route would be in effect so I don't need to push/configure one
explicitly to the client for it to be able to reach the server (or
vice versa) via the tunnel. But if I use ics-openvpn with the openvpn3
box checked, apparently the route is not in effect (but still added),
and therefore for it to be able to reach the server, I would need to
push/configure a route explicitly.

On Tue, 12 Nov 2019 at 02:02, Arne Schwabe <arne@rfc2549.org> wrote:
>
> Am 11.11.19 um 16:12 schrieb Tom Yan:
> > Yes both iproute2 and sitnl works perfectly as expected. Will the fix
> > be backported to the 2.4 branch?
> >
> > P.S. For the record, if the client is ics-openvpn with openvpn3,
> > somehow you need push route (either host or subnet will do) anyway.
>
> I am not aware of this issue. Could you go more into detail?
>
> > This is not necessary with ics-openvpn with openvpn2 or OpenVPN
> > Connect (AFAIK its core is openvpn3 as well, albeit different
> > snapshot). The problem/behaviour is not specific to /31 subnet though,
> > apparently. I guess the main route table is ignored/masked.
>
> ics-openvpn is a) OpenVPN master b) uses a completely different logic to
> handle network config (in OpenVPNService.java). So this patch does not
> affect ics-openvpn in any way.
>
> Arne
>
>
>

Patch

diff --git a/src/openvpn/tun.c b/src/openvpn/tun.c
index 80eaa2c4..f5823516 100644
--- a/src/openvpn/tun.c
+++ b/src/openvpn/tun.c
@@ -965,13 +965,14 @@  do_ifconfig(struct tuntap *tt,
         else
         {
             argv_printf(&argv,
-                        "%s addr add dev %s %s/%d broadcast %s",
+                        "%s addr add dev %s %s/%d",
                         iproute_path,
                         actual,
                         ifconfig_local,
-                        netmask_to_netbits2(tt->remote_netmask),
-                        ifconfig_broadcast
+                        netmask_to_netbits2(tt->remote_netmask)
                         );
+            if (netmask_to_netbits2(tt->remote_netmask) < 31)
+                argv_printf_cat(&argv, " broadcast %s", ifconfig_broadcast);
             argv_msg(M_INFO, &argv);
             openvpn_execve_check(&argv, es, S_FATAL, "Linux ip addr add failed");
         }