| Message ID | 1593197385-30618-2-git-send-email-max@rfc2324.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [Openvpn-devel,1/2] Add --bind-dev option. | expand |
Hi, reading this more closely at merging/testing time, I do have a change request... On Fri, Jun 26, 2020 at 08:49:44PM +0200, Maximilian Wilhelm wrote: > +#ifdef TARGET_LINUX > + else if (streq (p[0], "bind-dev") && p[1]) > + { > + VERIFY_PERMISSION (OPT_P_SOCKFLAGS); > + options->bind_dev = p[1]; > + } > +#endif One could argue whether the argument should be changed for IFNAMSIZ here (so we can error-out right away if it's too long). But this is just something to consider. > --- a/src/openvpn/socket.c > +++ b/src/openvpn/socket.c > @@ -1138,6 +1138,14 @@ create_socket(struct link_socket *sock, struct addrinfo *addr) > /* set socket to --mark packets with given value */ > socket_set_mark(sock->sd, sock->mark); > > +#if defined(TARGET_LINUX) > + if (sock->bind_dev) > + { > + msg (M_INFO, "Using bind-dev %s", sock->bind_dev); > + setsockopt (sock->sd, SOL_SOCKET, SO_BINDTODEVICE, sock->bind_dev, strlen (sock->bind_dev) + 1); > + } > +#endif Here, we *must* have a return code check, and logging of an error message if setsocktopt() fails. Imagine someone calling "openvpn --bind-dev eht0" (because he has fat fingers). The current code will silently fail the setsockopt() - because there is no such interface name - but nothing in the logs will show a hint *why* openvpn is just not doing what requested. Sorry for not spotting this in the first review. gert
Anno domini 2020 Gert Doering scripsit: Hi, > reading this more closely at merging/testing time, I do have a change > request... > > On Fri, Jun 26, 2020 at 08:49:44PM +0200, Maximilian Wilhelm wrote: > > +#ifdef TARGET_LINUX > > + else if (streq (p[0], "bind-dev") && p[1]) > > + { > > + VERIFY_PERMISSION (OPT_P_SOCKFLAGS); > > + options->bind_dev = p[1]; > > + } > > +#endif > > One could argue whether the argument should be changed for IFNAMSIZ here > (so we can error-out right away if it's too long). But this is just > something to consider. That might be a bad idea as of upcoming altnames, see https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=7a56493f0620cc1b4cffc9bc59289fdefe76b5f3 > > --- a/src/openvpn/socket.c > > +++ b/src/openvpn/socket.c > > @@ -1138,6 +1138,14 @@ create_socket(struct link_socket *sock, struct addrinfo *addr) > > /* set socket to --mark packets with given value */ > > socket_set_mark(sock->sd, sock->mark); > > > > +#if defined(TARGET_LINUX) > > + if (sock->bind_dev) > > + { > > + msg (M_INFO, "Using bind-dev %s", sock->bind_dev); > > + setsockopt (sock->sd, SOL_SOCKET, SO_BINDTODEVICE, sock->bind_dev, strlen (sock->bind_dev) + 1); > > + } > > +#endif > > Here, we *must* have a return code check, and logging of an error message > if setsocktopt() fails. > > Imagine someone calling "openvpn --bind-dev eht0" (because he has fat > fingers). The current code will silently fail the setsockopt() - because > there is no such interface name - but nothing in the logs will show a hint > *why* openvpn is just not doing what requested. I'll look into that and add a patch for that. Best Max
Hi, once again, this time with added error handling in the setsockopt(). Sorry for missing this before. Best Max
diff --git a/src/openvpn/init.c b/src/openvpn/init.c index 2af25c1..dd1747f 100644 --- a/src/openvpn/init.c +++ b/src/openvpn/init.c @@ -3445,6 +3445,7 @@ do_init_socket_1(struct context *c, const int mode) c->options.rcvbuf, c->options.sndbuf, c->options.mark, + c->options.bind_dev, &c->c2.server_poll_interval, sockflags); } diff --git a/src/openvpn/options.c b/src/openvpn/options.c index 3484f7d..6b36269 100644 --- a/src/openvpn/options.c +++ b/src/openvpn/options.c @@ -294,6 +294,9 @@ static const char usage_message[] = #if defined(TARGET_LINUX) && HAVE_DECL_SO_MARK "--mark value : Mark encrypted packets being sent with value. The mark value\n" " can be matched in policy routing and packetfilter rules.\n" + "--bind-dev dev : Bind to the given device when making connection to a peer or\n" + " listening for connections. This allows sending encrypted packets\n" + " via a VRF present on the system.\n" #endif "--txqueuelen n : Set the tun/tap TX queue length to n (Linux only).\n" #ifdef ENABLE_MEMSTATS @@ -6064,6 +6067,13 @@ add_option(struct options *options, } } } +#ifdef TARGET_LINUX + else if (streq (p[0], "bind-dev") && p[1]) + { + VERIFY_PERMISSION (OPT_P_SOCKFLAGS); + options->bind_dev = p[1]; + } +#endif else if (streq(p[0], "txqueuelen") && p[1] && !p[2]) { VERIFY_PERMISSION(OPT_P_GENERAL); diff --git a/src/openvpn/options.h b/src/openvpn/options.h index 375a4fc..c83a46a 100644 --- a/src/openvpn/options.h +++ b/src/openvpn/options.h @@ -352,6 +352,7 @@ struct options /* mark value */ int mark; + char *bind_dev; /* socket flags */ unsigned int sockflags; diff --git a/src/openvpn/socket.c b/src/openvpn/socket.c index f2fb54d..fd92d52 100644 --- a/src/openvpn/socket.c +++ b/src/openvpn/socket.c @@ -1138,6 +1138,14 @@ create_socket(struct link_socket *sock, struct addrinfo *addr) /* set socket to --mark packets with given value */ socket_set_mark(sock->sd, sock->mark); +#if defined(TARGET_LINUX) + if (sock->bind_dev) + { + msg (M_INFO, "Using bind-dev %s", sock->bind_dev); + setsockopt (sock->sd, SOL_SOCKET, SO_BINDTODEVICE, sock->bind_dev, strlen (sock->bind_dev) + 1); + } +#endif + bind_local(sock, addr->ai_family); } @@ -1880,6 +1888,7 @@ link_socket_init_phase1(struct link_socket *sock, int rcvbuf, int sndbuf, int mark, + const char *bind_dev, struct event_timeout *server_poll_timeout, unsigned int sockflags) { @@ -1906,6 +1915,7 @@ link_socket_init_phase1(struct link_socket *sock, sock->sockflags = sockflags; sock->mark = mark; + sock->bind_dev = bind_dev; sock->info.proto = proto; sock->info.af = af; diff --git a/src/openvpn/socket.h b/src/openvpn/socket.h index 38e5138..8db9e8b 100644 --- a/src/openvpn/socket.h +++ b/src/openvpn/socket.h @@ -212,6 +212,7 @@ struct link_socket #define SF_GETADDRINFO_DGRAM (1<<4) unsigned int sockflags; int mark; + const char *bind_dev; /* for stream sockets */ struct stream_buf stream_buf; @@ -326,6 +327,7 @@ link_socket_init_phase1(struct link_socket *sock, int rcvbuf, int sndbuf, int mark, + const char *bind_dev, struct event_timeout *server_poll_timeout, unsigned int sockflags); /* Reenable uncrustify *INDENT-ON* */
This options allows the user to specify a network device the OpenVPN process should use when making a connection or binding to an address. This translates in setting the SO_BINDTODEVICE option to the corresponding socket (on Linux). When for example using VRFs on Linux [0] this allows making connections using the non-default VRF and having the tun/tap interface in the default VRF. Thanks to David Ahern (Cumulus Networks) for insights on this. [0] https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/Documentation/networking/vrf.txt Signed-off-by: Maximilian Wilhelm <max@sdn.clinic> --- src/openvpn/init.c | 1 + src/openvpn/options.c | 10 ++++++++++ src/openvpn/options.h | 1 + src/openvpn/socket.c | 10 ++++++++++ src/openvpn/socket.h | 2 ++ 5 files changed, 24 insertions(+)