p2p tun configs break with new topology default in non-obvious ways

[cron2]

so there's a p2p tun config with

ifconfig 10.204.8.1 10.204.8.2

and with commit 32e6586687 the new default is now topology subnet. This leads to the instance no longer starting, with

Apr  3 18:17:06 gentoo tun-udp-p2p[11730]: do_ifconfig, ipv4=1, ipv6=1
Apr  3 18:17:06 gentoo tun-udp-p2p[11730]: net_addr_v4_add: 10.204.8.1/-1 dev tun7
Apr  3 18:17:06 gentoo tun-udp-p2p[11730]: sitnl_send: rtnl: generic error (-22): Invalid argument
Apr  3 18:17:06 gentoo tun-udp-p2p[11730]: Linux can't add IP to interface tun7
Apr  3 18:17:06 gentoo tun-udp-p2p[11730]: Exiting due to fatal error

so it seems "something" is trying to convert the second argument to a netmask/netbits, failing, assigning "-1" to "something" and passing that to sitnl...

This is a config with no client or server, just plain p2p udp, so it surprised me a bit that topology would be relevant here - but quite obviously it changes the interpretation of ifconfig.

So there's two questions here

• can we make the error message less obviously "the parser did not expect this and put -1 into something which didn't care"?
• should we keep the topology at net30 for point-to-point configs (no server)? Which is, of course, much more work than just changing the global default for all...

@ordex for the parser, @flichtenheld for the topology default.

[flichtenheld]

So if init_tun() is called with strict_warn == true then we warn when the second argument doesn't look like a netmask.

[flichtenheld]

But the problem with that check is that it actually uses topology to detect whether we're in P2P mode:

bool
is_tun_p2p(const struct tuntap *tt)
{
    bool tun = false;

    if (tt->type == DEV_TYPE_TAP
        || (tt->type == DEV_TYPE_TUN && tt->topology == TOP_SUBNET)
        || tt->type == DEV_TYPE_NULL)
    {
     	tun = false;
    }
    else if (tt->type == DEV_TYPE_TUN)
    {
        tun = true;
    }
    else
    {
        msg(M_FATAL, "Error: problem with tun vs. tap setting"); /* JYFIXME -- needs to be caught earlier, in ini\
t_tun? */

    }
    return tun;
}

So by changing the default we probably broke this check.

[flichtenheld]

Nevermind, in current master we do not actually use is_tun_p2p. This is only added by my patch https://gerrit.openvpn.net/c/openvpn/+/380. Master uses tt->type == DEV_TYPE_TUN which is just not correct, so the wrong checks are done probably.

[flichtenheld]

Changing the default only for --server would probably look something like this:

• Change default to TOP_UNDEF instead of TOP_SUBNET
• In helper_client_server check whether topology is still TOP_UNDEF, if yes change to TOP_SUBNET
• After helper_client_server set topology to TOP_NET30 if it is still TOP_UNDEF

[ordex]

After helper_client_server set topology to TOP_NET30 if it is still TOP_UNDEF

honestly this feels a bit hacky: i.e. using what we have in a dirty way to properly achieve what we need.

How about explicitly adding a TOP_P2P ? after all this is not NET30, but it's a truly different way of assigning the IPs (local+remote vs /30).

This way we can then explicitly check if topology == TOP_P2P and act accordingly.

[flichtenheld]

As discussed on IRC: There is already a TOP_P2P which does something slightly different again, although most of the code treats it as alias for TOP_NET30.

[cron2]

So the topology default has been taken care of, but the other aspect of passing on -1 as netbits after parsing is still not really helpful error handling.

[flichtenheld]

So the topology default has been taken care of, but the other aspect of passing on -1 as netbits after parsing is still not really helpful error handling.

@cron2 Please take a look at https://gerrit.openvpn.net/c/openvpn/+/380, my earlier comments seem to indicate that it should improve that part.