Unfortunately, I need to NAK this, because this will break some people's
use of OpenVPN in an IPv6 context (namely, where the OpenVPN server is
sitting in the same LAN as the client - think "EduRoam WiFi").
I did the same thing as before - built an iproute2 binary and a sitnl
binary, and tested t_client plus some manual commands.
Under some conditions, the output is *differen*, and this is very bad.
Here's the output of "openvpn --show-gateway" for my local network...
iproute2 build:
Wed Jun 5 12:04:53 2019 ROUTE6_GATEWAY fe80::250:43ff:fe01:dc37 IFACE=enp0s25
sitnl build:
Wed Jun 5 12:04:56 2019 ROUTE6_GATEWAY fe80::250:43ff:fe01:dc37 ON_LINK IFACE=enp0s25
which I find surprising, since they should both use the same backend
now...? "Older binaries" print the gateway without ON_LINK.
It gets weirder... if I ask for something which actually *is* on-link
(another address out of my on-link subnet), I get the same thing:
$ src/openvpn/openvpn --show-gateway 2001:608:4::1
iproute2 build:
Wed Jun 5 12:19:37 2019 ROUTE6_GATEWAY :: IFACE=enp0s25
sitnl build:
Wed Jun 5 12:18:20 2019 ROUTE6_GATEWAY :: ON_LINK IFACE=enp0s25 HWADDR=00:00:00:00:54:57
and *sometimes*
Wed Jun 5 12:19:56 2019 ROUTE6_GATEWAY :: IFACE=enp0s25
while my old code alwyays prints
Wed Jun 5 12:24:51 2019 ROUTE6_GATEWAY 2001:608:4::1 ON_LINK IFACE=enp0s25
("what you are looking for is on this link and it is the gateway").
This is a regression and needs to be fixed -> so, NAK.
On-Link targets always need to have ON_LINK set for both builds, off-link
targets (if there exists a default-gateway) must never have it set, and
the gateway address should.
I have no idea what is going wrong here, because both implementations
should behave exactly the same, since they use the same backend - but
this smells like "something is not correct, and it depends on build
or run-time factors". Maybe something isn't properly initialized
when rewriting this... the bug is not in code that is introduced
by this patch (it's hiding in networking_sitnl.c, I assume) but this
patch is what activates the code path,
A few comments on the actual code changes :-) - the introduction of
*ctx here cannot be avoided.
This strikes me as a bit funky, though...
+ struct in_addr gw;
+ CLEAR(gw);
.. and then "gw" is not used at all?
The introduction of a "flags" temporary variable can be certainly be
argued about (I find it less readable for a one-shot comparison), but
if you do, why not define it where it's used (C99)?
While at it, please kick out the definition of "struct rtreq" in
route.c, right above the TARGET_LINUX get_default_gateway_ipv6() - no
longer needed since we do not do the netlink stuff here anymore.
thanks,
gert
--
kind regards,
Gert Doering
@@ -1463,7 +1463,8 @@ static void
do_init_route_ipv6_list(const struct options *options,
struct route_ipv6_list *route_ipv6_list,
const struct link_socket_info *link_socket_info,
- struct env_set *es)
+ struct env_set *es,
+ openvpn_net_ctx_t *ctx)
{
const char *gw = NULL;
int metric = -1; /* no metric set */
@@ -1499,7 +1500,8 @@ do_init_route_ipv6_list(const struct options *options,
gw,
metric,
link_socket_current_remote_ipv6(link_socket_info),
- es))
+ es,
+ ctx))
{
/* copy routes to environment */
setenv_routes_ipv6(es, route_ipv6_list);
@@ -1759,7 +1761,8 @@ do_open_tun(struct context *c)
if (c->options.routes_ipv6 && c->c1.route_ipv6_list)
{
do_init_route_ipv6_list(&c->options, c->c1.route_ipv6_list,
- &c->c2.link_socket->info, c->c2.es);
+ &c->c2.link_socket->info, c->c2.es,
+ &c->net_ctx);
}
/* do ifconfig */
@@ -375,7 +375,7 @@ net_route_v4_best_gw(openvpn_net_ctx_t *ctx, const in_addr_t *dst,
/*
* The following function is not implemented in the iproute backend as it
- * already uses netlink in route.c.
+ * uses the sitnl implementation from networking_sitnl.c.
*
* int
* net_route_v6_best_gw(const struct in6_addr *dst, int prefixlen,
@@ -5014,7 +5014,7 @@ add_option(struct options *options,
}
net_ctx_init(NULL, &net_ctx);
get_default_gateway(&rgi, &net_ctx);
- get_default_gateway_ipv6(&rgi6, &remote);
+ get_default_gateway_ipv6(&rgi6, &remote, &net_ctx);
print_default_gateway(M_INFO, &rgi, &rgi6);
openvpn_exit(OPENVPN_EXIT_STATUS_GOOD); /* exit point */
}
@@ -765,7 +765,8 @@ init_route_ipv6_list(struct route_ipv6_list *rl6,
const char *remote_endpoint,
int default_metric,
const struct in6_addr *remote_host_ipv6,
- struct env_set *es)
+ struct env_set *es,
+ openvpn_net_ctx_t *ctx)
{
struct gc_arena gc = gc_new();
bool ret = true;
@@ -790,7 +791,7 @@ init_route_ipv6_list(struct route_ipv6_list *rl6,
msg(D_ROUTE, "GDG6: remote_host_ipv6=%s",
remote_host_ipv6 ? print_in6_addr(*remote_host_ipv6, 0, &gc) : "n/a" );
- get_default_gateway_ipv6(&rl6->rgi6, remote_host_ipv6);
+ get_default_gateway_ipv6(&rl6->rgi6, remote_host_ipv6, ctx);
if (rl6->rgi6.flags & RGI_ADDR_DEFINED)
{
setenv_str(es, "net_gateway_ipv6", print_in6_addr(rl6->rgi6.gateway.addr_ipv6, 0, &gc));
@@ -3323,152 +3324,30 @@ struct rtreq {
void
get_default_gateway_ipv6(struct route_ipv6_gateway_info *rgi6,
- const struct in6_addr *dest)
+ const struct in6_addr *dest, openvpn_net_ctx_t *ctx)
{
- int nls = -1;
- struct rtreq rtreq;
- struct rtattr *rta;
-
- char rtbuf[2000];
- ssize_t ssize;
-
- CLEAR(*rgi6);
-
- nls = socket( PF_NETLINK, SOCK_RAW, NETLINK_ROUTE );
- if (nls < 0)
- {
- msg(M_WARN|M_ERRNO, "GDG6: socket() failed" ); goto done;
- }
-
- /* bind() is not needed, no unsolicited msgs coming in */
-
- /* request best matching route, see netlink(7) for explanations
- */
- CLEAR(rtreq);
- rtreq.nh.nlmsg_type = RTM_GETROUTE;
- rtreq.nh.nlmsg_flags = NLM_F_REQUEST; /* best match only */
- rtreq.rtm.rtm_family = AF_INET6;
- rtreq.rtm.rtm_src_len = 0; /* not source dependent */
- rtreq.rtm.rtm_dst_len = 128; /* exact dst */
- rtreq.rtm.rtm_table = RT_TABLE_MAIN;
- rtreq.rtm.rtm_protocol = RTPROT_UNSPEC;
- rtreq.nh.nlmsg_len = NLMSG_SPACE(sizeof(rtreq.rtm));
-
- /* set RTA_DST for target IPv6 address we want */
- rta = (struct rtattr *)(((char *) &rtreq)+NLMSG_ALIGN(rtreq.nh.nlmsg_len));
- rta->rta_type = RTA_DST;
- rta->rta_len = RTA_LENGTH(16);
- rtreq.nh.nlmsg_len = NLMSG_ALIGN(rtreq.nh.nlmsg_len)
- +RTA_LENGTH(16);
-
- if (dest == NULL) /* ::, unspecified */
- {
- memset( RTA_DATA(rta), 0, 16 ); /* :: = all-zero */
- }
- else
- {
- memcpy( RTA_DATA(rta), (void *)dest, 16 );
- }
+ struct in_addr gw;
+ int flags;
- /* send and receive reply */
- if (send( nls, &rtreq, rtreq.nh.nlmsg_len, 0 ) < 0)
- {
- msg(M_WARN|M_ERRNO, "GDG6: send() failed" ); goto done;
- }
-
- ssize = recv(nls, rtbuf, sizeof(rtbuf), MSG_TRUNC);
-
- if (ssize < 0)
- {
- msg(M_WARN|M_ERRNO, "GDG6: recv() failed" ); goto done;
- }
-
- if (ssize > sizeof(rtbuf))
- {
- msg(M_WARN, "get_default_gateway_ipv6: returned message too big for buffer (%d>%d)", (int)ssize, (int)sizeof(rtbuf) );
- goto done;
- }
-
- struct nlmsghdr *nh;
+ CLEAR(gw);
- for (nh = (struct nlmsghdr *)rtbuf;
- NLMSG_OK(nh, ssize);
- nh = NLMSG_NEXT(nh, ssize))
+ if (net_route_v6_best_gw(ctx, dest, 0, &rgi6->gateway.addr_ipv6,
+ rgi6->iface) == 0)
{
- struct rtmsg *rtm;
- int attrlen;
-
- if (nh->nlmsg_type == NLMSG_DONE)
+ if (rgi6->gateway.addr_ipv6.s6_addr)
{
- break;
+ rgi6->flags |= RGI_ADDR_DEFINED;
}
- if (nh->nlmsg_type == NLMSG_ERROR)
+ if (rgi6->iface)
{
- struct nlmsgerr *ne = (struct nlmsgerr *)NLMSG_DATA(nh);
-
- /* since linux-4.11 -ENETUNREACH is returned when no route can be
- * found. Don't print any error message in this case */
- if (ne->error != -ENETUNREACH)
- {
- msg(M_WARN, "GDG6: NLMSG_ERROR: error %s\n",
- strerror(-ne->error));
- }
- break;
- }
-
- if (nh->nlmsg_type != RTM_NEWROUTE)
- {
- /* shouldn't happen */
- msg(M_WARN, "GDG6: unexpected msg_type %d", nh->nlmsg_type );
- continue;
- }
-
- rtm = (struct rtmsg *)NLMSG_DATA(nh);
- attrlen = RTM_PAYLOAD(nh);
-
- /* we're only looking for routes in the main table, as "we have
- * no IPv6" will lead to a lookup result in "Local" (::/0 reject)
- */
- if (rtm->rtm_family != AF_INET6
- || rtm->rtm_table != RT_TABLE_MAIN)
- {
- continue;
- } /* we're not interested */
-
- for (rta = RTM_RTA(rtm);
- RTA_OK(rta, attrlen);
- rta = RTA_NEXT(rta, attrlen))
- {
- if (rta->rta_type == RTA_GATEWAY)
- {
- if (RTA_PAYLOAD(rta) != sizeof(struct in6_addr) )
- {
- msg(M_WARN, "GDG6: RTA_GW size mismatch"); continue;
- }
- rgi6->gateway.addr_ipv6 = *(struct in6_addr *) RTA_DATA(rta);
- rgi6->flags |= RGI_ADDR_DEFINED;
- }
- else if (rta->rta_type == RTA_OIF)
- {
- char ifname[IF_NAMESIZE+1];
- int oif;
- if (RTA_PAYLOAD(rta) != sizeof(oif) )
- {
- msg(M_WARN, "GDG6: oif size mismatch"); continue;
- }
-
- memcpy(&oif, RTA_DATA(rta), sizeof(oif));
- if_indextoname(oif,ifname);
- strncpy( rgi6->iface, ifname, sizeof(rgi6->iface)-1 );
- rgi6->flags |= RGI_IFACE_DEFINED;
- }
+ rgi6->flags |= RGI_IFACE_DEFINED;
}
}
/* if we have an interface but no gateway, the destination is on-link */
- if ( ( rgi6->flags & (RGI_IFACE_DEFINED|RGI_ADDR_DEFINED) ) ==
- RGI_IFACE_DEFINED)
+ flags = rgi6->flags & (RGI_IFACE_DEFINED | RGI_ADDR_DEFINED);
+ if (flags == RGI_IFACE_DEFINED)
{
rgi6->flags |= (RGI_ADDR_DEFINED | RGI_ON_LINK);
if (dest)
@@ -3476,12 +3355,6 @@ get_default_gateway_ipv6(struct route_ipv6_gateway_info *rgi6,
rgi6->gateway.addr_ipv6 = *dest;
}
}
-
-done:
- if (nls >= 0)
- {
- close(nls);
- }
}
#elif defined(TARGET_DARWIN) || defined(TARGET_SOLARIS) \
@@ -3758,7 +3631,7 @@ done:
void
get_default_gateway_ipv6(struct route_ipv6_gateway_info *rgi6,
- const struct in6_addr *dest)
+ const struct in6_addr *dest, openvpn_net_ctx_t *ctx)
{
struct rtmsg m_rtmsg;
@@ -3944,7 +3817,7 @@ get_default_gateway(struct route_gateway_info *rgi, openvpn_net_ctx_t *ctx)
}
void
get_default_gateway_ipv6(struct route_ipv6_gateway_info *rgi6,
- const struct in6_addr *dest)
+ const struct in6_addr *dest, openvpn_net_ctx_t *ctx)
{
msg(D_ROUTE, "no support for get_default_gateway_ipv6() on this system");
CLEAR(*rgi6);
@@ -31,6 +31,7 @@
#include "basic.h"
#include "tun.h"
#include "misc.h"
+#include "networking.h"
#ifdef _WIN32
/*
@@ -291,7 +292,8 @@ bool init_route_ipv6_list(struct route_ipv6_list *rl6,
const char *remote_endpoint,
int default_metric,
const struct in6_addr *remote_host,
- struct env_set *es);
+ struct env_set *es,
+ openvpn_net_ctx_t *ctx);
void route_list_add_vpn_gateway(struct route_list *rl,
struct env_set *es,
@@ -323,7 +325,8 @@ void get_default_gateway(struct route_gateway_info *rgi,
openvpn_net_ctx_t *ctx);
void get_default_gateway_ipv6(struct route_ipv6_gateway_info *rgi,
- const struct in6_addr *dest);
+ const struct in6_addr *dest,
+ openvpn_net_ctx_t *ctx);
void print_default_gateway(const int msglevel,
const struct route_gateway_info *rgi,