diff mbox series

[Openvpn-devel,v102i,2/7] dco-win: check for incompatible options

Message ID 20220818092638.189632-1-a@unstable.cc
State Changes Requested
Headers show
Series None | expand

Commit Message

Antonio Quartulli Aug. 17, 2022, 11:26 p.m. UTC 
At the moment dco-win doesn't support --persist-tun and --server,
so check for these options at startup time.

Signed-off-by: Antonio Quartulli <a@unstable.cc>
Signed-off-by: Lev Stipakov <lev@openvpn.net>
---

Changes from v101:
* rebased
* remove call to dco_check_option_ from verify() and reuse invocation
  that was already implemented for linux/freebsd in mutate_ce()
* hide log level to use in case of option check failure inside
  dco_win/linux/freebsd.h

Changes from v100:
* improved commit title/message
---
 src/openvpn/dco.c         | 17 +++++++++++++++--
 src/openvpn/dco_freebsd.h |  3 +++
 src/openvpn/dco_linux.h   |  3 +++
 src/openvpn/dco_win.h     |  5 +++++
 src/openvpn/options.c     |  6 ++----
 5 files changed, 28 insertions(+), 6 deletions(-)

Comments

Gert Doering Aug. 17, 2022, 11:36 p.m. UTC | #1 
Hi,

On Thu, Aug 18, 2022 at 11:26:38AM +0200, Antonio Quartulli wrote:
> -#if defined(TARGET_LINUX) || defined(TARGET_FREEBSD)
> -    o->tuntap_options.disable_dco = !dco_check_option_conflict(D_DCO, o)
> -                                    || !dco_check_startup_option_conflict(D_DCO, o);
> -#endif
> +    o->tuntap_options.disable_dco = !dco_check_option_conflict(DCO_CHECK_OPTION_LEVEL, o)
> +                                    || !dco_check_startup_option_conflict(DCO_CHECK_OPTION_LEVEL, o);

Nah, this is not good.

This reads as "this is code which will always return, and will log with
some arbitrary log level" (how would a reader know that D_DCO and 
DCO_CHECK_OPTION_LEVEL is not both coming from errror.h?).

Make it explicit, and explain why.

gert
diff mbox series

Patch

diff --git a/src/openvpn/dco.c b/src/openvpn/dco.c
index 757ac19b..0aeecc54 100644
--- a/src/openvpn/dco.c
+++ b/src/openvpn/dco.c
@@ -225,7 +225,20 @@  dco_update_keys(dco_context_t *dco, struct tls_multi *multi)
 bool
 dco_check_startup_option_conflict(int msglevel, const struct options *o)
 {
-#if defined(TARGET_LINUX)
+#if defined(_WIN32)
+    if (o->mode == MODE_SERVER)
+    {
+        msg(msglevel, "Only client and p2p data channel offload is supported "
+            "with ovpn-dco-win.");
+        return false;
+    }
+
+    if (o->persist_tun)
+    {
+        msg(msglevel, "--persist-tun is not supported with ovpn-dco-win.");
+        return false;
+    }
+#elif defined(TARGET_LINUX)
     /* if the device name is fixed, we need to check if an interface with this
      * name already exists. IF it does, it must be a DCO interface, otherwise
      * DCO has to be disabled in order to continue.
@@ -250,7 +263,7 @@  dco_check_startup_option_conflict(int msglevel, const struct options *o)
                 strerror(-ret), ret);
         }
     }
-#endif /* if defined(TARGET_LINUX) */
+#endif /* if defined(_WIN32) */
 
 #if defined(HAVE_LIBCAPNG)
     /* DCO can't operate without CAP_NET_ADMIN. To retain it when switching user
diff --git a/src/openvpn/dco_freebsd.h b/src/openvpn/dco_freebsd.h
index 3594f229..52ba0405 100644
--- a/src/openvpn/dco_freebsd.h
+++ b/src/openvpn/dco_freebsd.h
@@ -27,6 +27,9 @@ 
 
 #include "ovpn_dco_freebsd.h"
 
+/* define to what log level we print when a conflicting option is found */
+#define DCO_CHECK_OPTION_LEVEL D_DCO
+
 typedef enum ovpn_key_slot dco_key_slot_t;
 typedef enum ovpn_key_cipher dco_cipher_t;
 
diff --git a/src/openvpn/dco_linux.h b/src/openvpn/dco_linux.h
index 416ea30a..7b7a9ca5 100644
--- a/src/openvpn/dco_linux.h
+++ b/src/openvpn/dco_linux.h
@@ -31,6 +31,9 @@ 
 #include <netlink/socket.h>
 #include <netlink/netlink.h>
 
+/* define to what log level we print when a conflicting option is found */
+#define DCO_CHECK_OPTION_LEVEL D_DCO
+
 typedef enum ovpn_key_slot dco_key_slot_t;
 typedef enum ovpn_cipher_alg dco_cipher_t;
 
diff --git a/src/openvpn/dco_win.h b/src/openvpn/dco_win.h
index 348fc568..af959cd6 100644
--- a/src/openvpn/dco_win.h
+++ b/src/openvpn/dco_win.h
@@ -27,6 +27,11 @@ 
 #include "buffer.h"
 #include "ovpn_dco_win.h"
 
+/* define to what log level we print when a conflicting option is found.
+ * On windows we can't fallback to non-DCO so we bail out with M_USAGE
+ */
+#define DCO_CHECK_OPTION_LEVEL M_USAGE
+
 typedef OVPN_KEY_SLOT dco_key_slot_t;
 typedef OVPN_CIPHER_ALG dco_cipher_t;
 
diff --git a/src/openvpn/options.c b/src/openvpn/options.c
index 2415c1a8..12aee489 100644
--- a/src/openvpn/options.c
+++ b/src/openvpn/options.c
@@ -3670,10 +3670,8 @@  options_postprocess_mutate(struct options *o, struct env_set *es)
     }
 
     /* check if any option should force disabling DCO */
-#if defined(TARGET_LINUX) || defined(TARGET_FREEBSD)
-    o->tuntap_options.disable_dco = !dco_check_option_conflict(D_DCO, o)
-                                    || !dco_check_startup_option_conflict(D_DCO, o);
-#endif
+    o->tuntap_options.disable_dco = !dco_check_option_conflict(DCO_CHECK_OPTION_LEVEL, o)
+                                    || !dco_check_startup_option_conflict(DCO_CHECK_OPTION_LEVEL, o);
 
     if (dco_enabled(o) && o->dev_node)
     {