diff mbox series

[Openvpn-devel,1/2] Add --bind-dev option.

Message ID 1593197385-30618-2-git-send-email-max@rfc2324.org
State Superseded
Headers
Series [Openvpn-devel,1/2] Add --bind-dev option. |

Commit Message

Maximilian Wilhelm June 26, 2020, 8:49 a.m. UTC 
  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(+)

Comments

Gert Doering June 28, 2020, 9:20 p.m. UTC | #1 
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
Maximilian Wilhelm June 28, 2020, 10:25 p.m. UTC | #2 
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
Maximilian Wilhelm June 29, 2020, 12:49 a.m. UTC | #3 
Hi,

once again, this time with added error handling in the setsockopt(). Sorry
for missing this before.

Best
Max
diff mbox series

Patch

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