On 20 Aug 2022, at 16:01, Gert Doering wrote:
> The existing DCO code had extra logic for "if this is not
> MR_WITH_NETBITS, set 32/128 as address length", but only for
> iroute addition. For iroute deletion, this was missing, and
> subsequently iroute deletion for IPv4 host routes failed on
> FreeBSD DCO (commit 3433577a99).
>
> Iroute handling differenciates between "primary" iroutes (coming
> from anm IP pool or ccd/ifconfig-push), and "non-primary" iroutes,
> coming from --iroute and --iroute-ipv6 statements in per-client config.
>
> "Primary" iroutes always use "-1" for their netbits, but since these
> are not installed via DCO, this is of no concern here. Whether these
> can and should be changed needs further study on internal route
> learning and cleanup.
>
> Refactor options.c and multi.c to ensure that netbits is always set
> for non-primary iroutes - and ASSERT() on this in the DCO path, so we can
> find out if there might be other code violating this.
>
> Change options.c::option_iroute() to always set netbits=32 for IPv4
> host routes (options_iroute_ipv6() never differenciated). Since
> netmask_to_netbits() also insists on "-1" for host routes, change
> to netmask_to_netbits2().
>
> Remove all the extra MR_WITH_NETBITS logic from dco.c, where it should
> have never appeared.
>
> Signed-off-by: Gert Doering <g...@greenie.muc.de>
Seems sane to me. Passes the FreeBSD automated tests.
Minor style nitpicks below.
Acked-by: Kristof Provost <kprov...@netgate.com>
> diff --git a/src/openvpn/multi.c b/src/openvpn/multi.c
> index 95414429..1f9d0201 100644
> --- a/src/openvpn/multi.c
> +++ b/src/openvpn/multi.c
> @@ -1241,6 +1241,7 @@ multi_learn_in_addr_t(struct multi_context *m,
> /* "primary" is the VPN ifconfig address of the peer and already
> * known to DCO, so only install "extra" iroutes (primary = false)
> */
> + ASSERT(netbits>=0); /* DCO requires populated netbits */
I’d put spaces around the comparison, so ‘ASSERT(netbits >= 0);’. That seems to
be what’s done in all conditionals anyway.
> diff --git a/src/openvpn/options.c b/src/openvpn/options.c
> index 2b0bb20c..21e2f69b 100644
> --- a/src/openvpn/options.c
> +++ b/src/openvpn/options.c
> @@ -1572,12 +1572,14 @@ option_iroute(struct options *o,
>
> ALLOC_OBJ_GC(ir, struct iroute, &o->gc);
> ir->network = getaddr(GETADDR_HOST_ORDER, network_str, 0, NULL, NULL);
> - ir->netbits = -1;
> + ir->netbits = 32; /* host route if no netmask given */
>
> if (netmask_str)
> {
> const in_addr_t netmask = getaddr(GETADDR_HOST_ORDER, netmask_str,
> 0, NULL, NULL);
> - if (!netmask_to_netbits(ir->network, netmask, &ir->netbits))
> + ir->netbits = netmask_to_netbits2(netmask);
> +
> + if (ir->netbits<0)
And here too. So ‘if (ir->netbits < 0)’
> {
> msg(msglevel, "in --iroute %s %s : Bad network/subnet
> specification",
> network_str,
Regards,
Kristof
_______________________________________________
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel
