| Message ID | 20220628082024.19059-1-a@unstable.cc |
|---|---|
| State | Accepted |
| Headers | show |
| Series | [Openvpn-devel,v2] do not push route-ipv6 entries that are also in the iroute-ipv6 list | expand |
On Dienstag, 28. Juni 2022 10:20:24 CEST Antonio Quartulli wrote: > A server should push a route to a client only if there is no matching > iroute for the same client. > > While this logic works fine for IPv4, there is no IPv6 counterpart. > > Implement the same check for IPv6 routes and discard matching ones > from the push list. > > Trac: #354 > Cc: Gert Doering <gert@greenie.muc.de> > Signed-off-by: Antonio Quartulli <a@unstable.cc> ACK
Stared-at-code for a bit...
The remove_iroutes_from_push_route_list() function would benefit from
our new "early exit" style - but that's outside the scope of this patch
(and actually not changing both at the same time helps readability).
The way the change is done is symmetric v4/v6, so whatever quality
problems the code might have, it's not introduced by this patch.
If I had written, this, I might have used "ir6" as iterator variable
for "struct iroute_ipv6 *ir6", but this is really minor - the code
is clear enough.
As a matter of optimization, I'd compare "netbits" first, and only then
run the memcmp()) - but for the normal volumes of ipv6 iroutes that
OpenVPN instances handle, this is inconsequential, and for Really Huge
Installations with 10.000s of iroutes, the "walk a linked list" approach
stinks anyway, one might want to change this to an iroute* hash lookup...
Tested on the FreeBSD 14 DCO instance, which excercises "push route-ipv6"
and "iroute-ipv6" quite heavily, with various netmasks, to see if
client-to-client routing works properly:
Client #1 "has no iroutes"
Oct 6 10:12:10 fbsd14 tun-udp-p2mp[43951]: SENT CONTROL [cron2-ubuntu-2004-amd64]: 'PUSH_REPLY,route 10.114.0.0 255.255.0.0,route-ipv6 fd00:abcd:114::/48,route-ipv6 fd00:abcd:114:200::74/128,route 10.114.201.0 255.255.255.0,route 10.114.201.0 255.255.255.128,route-ipv6 fd00:abcd:114:201::/64,route-ipv6 fd00:abcd:114:201::/65,route 10.114.202.74 255.255.255.255,route-ipv6 fd00:abcd:114:202::74/128,...,key-derivation tls-ekm' (status=1)
Client #2 "has all these more-specific iroutes, v4+v6"
Oct 6 10:12:52 fbsd14 tun-udp-p2mp[43951]: freebsd-74-amd64/2001:608:0:814::f000:3 SENT CONTROL [freebsd-74-amd64]: 'PUSH_REPLY,route 10.114.0.0 255.255.0.0,route-ipv6 fd00:abcd:114::/48,...,cipher AES-256-GCM' (status=1)
-> so, it works as it says on the tin, all matching iroute/iroute-ipv6
are removed from the to-be-pushed route/route-ipv6 entries, and it
says so in the log:
Oct 6 10:12:51 ... REMOVE PUSH ROUTE: 'route-ipv6 fd00:abcd:114:200::74/128'
Oct 6 10:12:51 ... REMOVE PUSH ROUTE: 'route 10.114.201.0 255.255.255.0'
Oct 6 10:12:51 ... REMOVE PUSH ROUTE: 'route 10.114.201.0 255.255.255.128'
Oct 6 10:12:51 ... REMOVE PUSH ROUTE: 'route-ipv6 fd00:abcd:114:201::/64'
Oct 6 10:12:51 ... REMOVE PUSH ROUTE: 'route-ipv6 fd00:abcd:114:201::/65'
Oct 6 10:12:51 ... REMOVE PUSH ROUTE: 'route 10.114.202.74 255.255.255.255'
Oct 6 10:12:51 ... REMOVE PUSH ROUTE: 'route-ipv6 fd00:abcd:114:202::74/128'
(the funny ordering comes from "this is the ordering of the push statements
in the server.conf", so, yes, this is as expected)
I discussed inclusion in release/2.5 with Antonio, and we agree that
this is more a "missing feature" than a bug, so I've not backported
it for now (it should apply trivially, so if someone comes screaming,
we can always do so).
Your patch has been applied to the master branch.
commit 437812d4eac9ac892fbfd2fd97c50384b2d2ec07
Author: Antonio Quartulli
Date: Tue Jun 28 10:20:24 2022 +0200
do not push route-ipv6 entries that are also in the iroute-ipv6 list
Signed-off-by: Antonio Quartulli <a@unstable.cc>
Acked-by: Heiko Hund <heiko@ist.eigentlich.net>
Message-Id: <20220628082024.19059-1-a@unstable.cc>
URL: https://www.mail-archive.com/openvpn-devel@lists.sourceforge.net/msg24577.html
Signed-off-by: Gert Doering <gert@greenie.muc.de>
--
kind regards,
Gert Doering
diff --git a/src/openvpn/push.c b/src/openvpn/push.c index 63257348..e5b588bb 100644 --- a/src/openvpn/push.c +++ b/src/openvpn/push.c @@ -1002,7 +1002,7 @@ process_incoming_push_msg(struct context *c, void remove_iroutes_from_push_route_list(struct options *o) { - if (o && o->push_list.head && o->iroutes) + if (o && o->push_list.head && (o->iroutes || o->iroutes_ipv6)) { struct gc_arena gc = gc_new(); struct push_entry *e = o->push_list.head; @@ -1019,7 +1019,7 @@ remove_iroutes_from_push_route_list(struct options *o) && parse_line(e->option, p, SIZE(p), "[PUSH_ROUTE_REMOVE]", 1, D_ROUTE_DEBUG, &gc)) { /* is the push item a route directive? */ - if (p[0] && !strcmp(p[0], "route") && !p[3]) + if (p[0] && !strcmp(p[0], "route") && !p[3] && o->iroutes) { /* get route parameters */ bool status1, status2; @@ -1042,6 +1042,30 @@ remove_iroutes_from_push_route_list(struct options *o) } } } + else if (p[0] && !strcmp(p[0], "route-ipv6") && !p[2] + && o->iroutes_ipv6) + { + /* get route parameters */ + struct in6_addr network; + unsigned int netbits; + + /* parse route-ipv6 arguments */ + if (get_ipv6_addr(p[1], &network, &netbits, D_ROUTE_DEBUG)) + { + struct iroute_ipv6 *ir; + + /* does this route-ipv6 match an iroute-ipv6? */ + for (ir = o->iroutes_ipv6; ir != NULL; ir = ir->next) + { + if (!memcmp(&network, &ir->network, sizeof(network)) + && netbits == ir->netbits) + { + enable = false; + break; + } + } + } + } /* should we copy the push item? */ e->enable = enable;
A server should push a route to a client only if there is no matching iroute for the same client. While this logic works fine for IPv4, there is no IPv6 counterpart. Implement the same check for IPv6 routes and discard matching ones from the push list. Trac: #354 Cc: Gert Doering <gert@greenie.muc.de> Signed-off-by: Antonio Quartulli <a@unstable.cc> --- Changes from v1: * add "&& o->iroutes{,_ipv6}" check before attempting to traverse iroutes list. This way we avoid executing getaddr or get_ipv6_addr if we already know that we have no iroutes to compare to. src/openvpn/push.c | 28 ++++++++++++++++++++++++++-- 1 file changed, 26 insertions(+), 2 deletions(-)