[Openvpn-devel] Fix server directive for /31 subnet

As /31 subnet now works (as we stop setting broadcast address), the server directives can be fixed for it as well. Also stop repeating code for tap and tun + subnet.
---
 src/openvpn/helper.c | 90 ++++++++++++++++++--------------------------
 1 file changed, 37 insertions(+), 53 deletions(-)

For the record, as I don't see why `o->ifconfig_pool_end` was
`(o->server_network | ~o->server_netmask) - 2` for tun + subnet while
it was `(o->server_network | ~o->server_netmask) - 1` for tap, I
assume the former is a typo.

By the way, why does `o->ifconfig_pool_netmask` need to be set even
when `nopool` is set?

On Wed, 13 Nov 2019 at 16:59, Tom Yan <tom.ty89@gmail.com> wrote:
>
> As /31 subnet now works (as we stop setting broadcast address), the server directives can be fixed for it as well. Also stop repeating code for tap and tun + subnet.
> ---
>  src/openvpn/helper.c | 90 ++++++++++++++++++--------------------------
>  1 file changed, 37 insertions(+), 53 deletions(-)
>
> diff --git a/src/openvpn/helper.c b/src/openvpn/helper.c
> index ff9df506..608f886f 100644
> --- a/src/openvpn/helper.c
> +++ b/src/openvpn/helper.c
> @@ -286,13 +286,13 @@ helper_client_server(struct options *o)
>                  print_netmask(IFCONFIG_POOL_MIN_NETBITS, &gc));
>          }
>
> -        if (dev == DEV_TYPE_TUN)
> +        if (dev == DEV_TYPE_TUN && (topology == TOP_NET30 || topology == TOP_P2P))
>          {
>              int pool_end_reserve = 4;
>
>              if (netbits > 29)
>              {
> -                msg(M_USAGE, "--server directive when used with --dev tun must define a subnet of %s or lower",
> +                msg(M_USAGE, "subnet must be %s or lower",
>                      print_netmask(29, &gc));
>              }
>
> @@ -304,85 +304,69 @@ helper_client_server(struct options *o)
>              o->mode = MODE_SERVER;
>              o->tls_server = true;
>
> -            if (topology == TOP_NET30 || topology == TOP_P2P)
> +            o->ifconfig_local = print_in_addr_t(o->server_network + 1, 0, &o->gc);
> +            o->ifconfig_remote_netmask = print_in_addr_t(o->server_network + 2, 0, &o->gc);
> +
> +            if (!(o->server_flags & SF_NOPOOL))
>              {
> -                o->ifconfig_local = print_in_addr_t(o->server_network + 1, 0, &o->gc);
> -                o->ifconfig_remote_netmask = print_in_addr_t(o->server_network + 2, 0, &o->gc);
> -
> -                if (!(o->server_flags & SF_NOPOOL))
> -                {
> -                    o->ifconfig_pool_defined = true;
> -                    o->ifconfig_pool_start = o->server_network + 4;
> -                    o->ifconfig_pool_end = (o->server_network | ~o->server_netmask) - pool_end_reserve;
> -                    ifconfig_pool_verify_range(M_USAGE, o->ifconfig_pool_start, o->ifconfig_pool_end);
> -                }
> -
> -                helper_add_route(o->server_network, o->server_netmask, o);
> -                if (o->enable_c2c)
> -                {
> -                    push_option(o, print_opt_route(o->server_network, o->server_netmask, &o->gc), M_USAGE);
> -                }
> -                else if (topology == TOP_NET30)
> -                {
> -                    push_option(o, print_opt_route(o->server_network + 1, 0, &o->gc), M_USAGE);
> -                }
> +                o->ifconfig_pool_defined = true;
> +                o->ifconfig_pool_start = o->server_network + 4;
> +                o->ifconfig_pool_end = (o->server_network | ~o->server_netmask) - pool_end_reserve;
> +                ifconfig_pool_verify_range(M_USAGE, o->ifconfig_pool_start, o->ifconfig_pool_end);
>              }
> -            else if (topology == TOP_SUBNET)
> +
> +            helper_add_route(o->server_network, o->server_netmask, o);
> +            if (o->enable_c2c)
>              {
> -                o->ifconfig_local = print_in_addr_t(o->server_network + 1, 0, &o->gc);
> -                o->ifconfig_remote_netmask = print_in_addr_t(o->server_netmask, 0, &o->gc);
> -
> -                if (!(o->server_flags & SF_NOPOOL))
> -                {
> -                    o->ifconfig_pool_defined = true;
> -                    o->ifconfig_pool_start = o->server_network + 2;
> -                    o->ifconfig_pool_end = (o->server_network | ~o->server_netmask) - 2;
> -                    ifconfig_pool_verify_range(M_USAGE, o->ifconfig_pool_start, o->ifconfig_pool_end);
> -                }
> -                o->ifconfig_pool_netmask = o->server_netmask;
> -
> -                push_option(o, print_opt_route_gateway(o->server_network + 1, &o->gc), M_USAGE);
> -                if (!o->route_default_gateway)
> -                {
> -                    o->route_default_gateway = print_in_addr_t(o->server_network + 2, 0, &o->gc);
> -                }
> +                push_option(o, print_opt_route(o->server_network, o->server_netmask, &o->gc), M_USAGE);
>              }
> -            else
> +            else if (topology == TOP_NET30)
>              {
> -                ASSERT(0);
> +                push_option(o, print_opt_route(o->server_network + 1, 0, &o->gc), M_USAGE);
>              }
> -
> -            push_option(o, print_opt_topology(topology, &o->gc), M_USAGE);
>          }
> -        else if (dev == DEV_TYPE_TAP)
> +        else if (dev == DEV_TYPE_TAP || (dev == DEV_TYPE_TUN && topology == TOP_SUBNET))
>          {
> -            if (netbits > 30)
> +            int ptp = 1;
> +
> +            if (netbits > 31)
>              {
> -                msg(M_USAGE, "--server directive when used with --dev tap must define a subnet of %s or lower",
> -                    print_netmask(30, &gc));
> +                msg(M_USAGE, "subnet must be %s or lower",
> +                    print_netmask(31, &gc));
>              }
>
> +            if (netbits == 31)
> +              ptp = 0;
> +
>              o->mode = MODE_SERVER;
>              o->tls_server = true;
> -            o->ifconfig_local = print_in_addr_t(o->server_network + 1, 0, &o->gc);
> +
> +            o->ifconfig_local = print_in_addr_t(o->server_network + ptp, 0, &o->gc);
>              o->ifconfig_remote_netmask = print_in_addr_t(o->server_netmask, 0, &o->gc);
>
>              if (!(o->server_flags & SF_NOPOOL))
>              {
>                  o->ifconfig_pool_defined = true;
> -                o->ifconfig_pool_start = o->server_network + 2;
> -                o->ifconfig_pool_end = (o->server_network | ~o->server_netmask) - 1;
> +                o->ifconfig_pool_start = o->server_network + ptp + 1;
> +                o->ifconfig_pool_end = (o->server_network | ~o->server_netmask) - ptp;
>                  ifconfig_pool_verify_range(M_USAGE, o->ifconfig_pool_start, o->ifconfig_pool_end);
>              }
>              o->ifconfig_pool_netmask = o->server_netmask;
>
> -            push_option(o, print_opt_route_gateway(o->server_network + 1, &o->gc), M_USAGE);
> +            push_option(o, print_opt_route_gateway(o->server_network + ptp, &o->gc), M_USAGE);
> +            if (dev == DEV_TYPE_TUN && !o->route_default_gateway)
> +            {
> +                o->route_default_gateway = print_in_addr_t(o->server_network + ptp + 1, 0, &o->gc);
> +            }
>          }
>          else
>          {
>              ASSERT(0);
>          }
>
> +        if (dev == DEV_TYPE_TUN)
> +            push_option(o, print_opt_topology(topology, &o->gc), M_USAGE);
> +
>          /* set push-ifconfig-constraint directive */
>          if ((dev == DEV_TYPE_TAP || topology == TOP_SUBNET))
>          {
> --
> 2.24.0
>

Hi,

On Wed, Nov 13, 2019 at 05:13:09PM +0800, Tom Yan wrote:
> For the record, as I don't see why `o->ifconfig_pool_end` was
> `(o->server_network | ~o->server_netmask) - 2` for tun + subnet while
> it was `(o->server_network | ~o->server_netmask) - 1` for tap, I
> assume the former is a typo.

It might be related to the way the windows tap6 driver always needs a 
gateway address even in tun mode (and uses the last address from the
subnet for this).  There's very few typos in OpenVPN code regarding
*this* type of math, and way more "just funny platforms and old lore".

> By the way, why does `o->ifconfig_pool_netmask` need to be set even
> when `nopool` is set?

Not sure.  This code has been rewritten a number of times over the
years, so maybe the assignment slipped outside of an if() block...

Things like this can (and should) all be fixed, but please do not lump 
them together with a functional change in one big patch.

gert

Hi,

On 13/11/2019 10:18, Gert Doering wrote:
>> By the way, why does `o->ifconfig_pool_netmask` need to be set even
>> when `nopool` is set?
> 
> Not sure.  This code has been rewritten a number of times over the
> years, so maybe the assignment slipped outside of an if() block...
> 
> Things like this can (and should) all be fixed, but please do not lump 
> them together with a functional change in one big patch.

I agree with Gert here. If you are finding inconsistencies across the
code and want to fix them, please do so in small separate patches
(related fixes can and should obviously go together).

It'll make them easier to be reviewed and merged ;-)

On top of that, a fix might be worth backporting to an older release,
while a functional change would normally just go to master.

Thanks a lot for your contribution though!


Cheers,

On Wed, 13 Nov 2019 at 17:18, Gert Doering <gert@greenie.muc.de> wrote:
>
> Hi,
>
> It might be related to the way the windows tap6 driver always needs a
> gateway address even in tun mode (and uses the last address from the
> subnet for this).  There's very few typos in OpenVPN code regarding
> *this* type of math, and way more "just funny platforms and old lore".

The reason I think it is a typo is because the pool start is network
address + 2 (i.e. I think it's a copy-and-paste kind of typo). AFAIK,
the gateway address issue is addressed in commit 4cc6a25 by using the
pool start for that. (The commit message stated that using the address
of the server does not work for Windows.)

In any case, should I just keep it as-is just to be "conservative"?

> Not sure.  This code has been rewritten a number of times over the
> years, so maybe the assignment slipped outside of an if() block...

Again, should I keep it as-is, or "try it and see"?

> Things like this can (and should) all be fixed, but please do not lump
> them together with a functional change in one big patch.

No problem, I can split it and resend.


>
> 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