[Openvpn-devel,M] Change in openvpn[master]: Automatically enable --compress migrate on the server

Message ID e808d4206024cd233bea4d4f4ddb0ee220f374df-HTML@gerrit.openvpn.net
State New
Headers show
Series [Openvpn-devel,M] Change in openvpn[master]: Automatically enable --compress migrate on the server | expand

Commit Message

flichtenheld (Code Review) Sept. 22, 2024, 2:15 p.m. UTC
Attention is currently required from: plaisthos.

Hello plaisthos,

I'd like you to do a code review.
Please visit

    http://gerrit.openvpn.net/c/openvpn/+/756?usp=email

to review the following change.


Change subject: Automatically enable --compress migrate on the server
......................................................................

Automatically enable --compress migrate on the server

If we enable LZO compression, automatically switch to
migrate mode.

Change-Id: I00209b880cfcedd93e28f97fc3941d8b85e095f3
Signed-off-by: Frank Lichtenheld <frank@lichtenheld.com>
---
M doc/man-sections/protocol-options.rst
M src/openvpn/comp.h
M src/openvpn/options.c
3 files changed, 56 insertions(+), 62 deletions(-)



  git pull ssh://gerrit.openvpn.net:29418/openvpn refs/changes/56/756/1

Patch

diff --git a/doc/man-sections/protocol-options.rst b/doc/man-sections/protocol-options.rst
index 8b061d2..b2a23fa 100644
--- a/doc/man-sections/protocol-options.rst
+++ b/doc/man-sections/protocol-options.rst
@@ -30,7 +30,9 @@ 
       framing (stub).
 
   :code:`yes`
-      OpenVPN will send and receive compressed packets.
+      **DEPRECATED** This option is an alias for :code:`asym`. Previously
+      it did enable compression for uplink packets, but OpenVPN never
+      compresses uplink packets now.
 
 --auth alg
   Authenticate data channel packets and (if enabled) ``tls-auth`` control
@@ -125,6 +127,12 @@ 
   configuration if supported by the client and otherwise switch to ``comp-lzo no``
   and add ``--push comp-lzo`` to the client specific configuration.
 
+  If used in a server configuration :code:`lzo` is an alias for :code:`migrate` in
+  current versions of OpenVPN. Compression will only be enabled if there is no
+  other choice. Note that these versions of OpenVPN also never actually compress
+  any packets. But they still will decompress packets received from the other side
+  of the connection if required.
+
   ***Security Considerations***
 
   Compression and encryption is a tricky combination. If an attacker knows
@@ -135,48 +143,31 @@ 
   entirely sure that the above does not apply to your traffic, you are
   advised to *not* enable compression.
 
+  For this reason compression support was removed from current versions
+  of OpenVPN. It will still decompress compressed packets removed via
+  a VPN connection but it will never compress any outgoing packets.
+
 --comp-lzo mode
   **DEPRECATED** Enable LZO compression algorithm.  Compression is
   generally not recommended.  VPN tunnels which uses compression are
   suspectible to the VORALCE attack vector.
 
-  Use LZO compression -- may add up to 1 byte per packet for incompressible
-  data. ``mode`` may be :code:`yes`, :code:`no`, or :code:`adaptive`
-  (default).
+  Allows the other side of the connection to use LZO compression. Due
+  to difference in packet format this may adds 1 additional byte per packet.
+  With current versions of OpenVPN no actual compression will happen.
 
-  In a server mode setup, it is possible to selectively turn compression
-  on or off for individual clients.
+  ``mode`` may be :code:`yes`, :code:`no`, or :code:`adaptive`
+  but there is no actual change in behavior anymore.
 
-  First, make sure the client-side config file enables selective
-  compression by having at least one ``--comp-lzo`` directive, such as
-  ``--comp-lzo no``. This will turn off compression by default, but allow
-  a future directive push from the server to dynamically change the
-  :code:`on`/:code:`off`/:code:`adaptive` setting.
-
-  Next in a ``--client-config-dir`` file, specify the compression setting
-  for the client, for example:
-  ::
-
-    comp-lzo yes
-    push "comp-lzo yes"
-
-  The first line sets the ``comp-lzo`` setting for the server side of the
-  link, the second sets the client side.
+  In server mode we convert this setting to ``--compress migrate`` to
+  automatically disable it when the client doesn't need it. If you want
+  to remove this setting from your server config you might need to add
+  an explicit ``--compress migrate`` instead if some clients still have
+  any variant of ``--comp-lzo`` in their config.
 
 --comp-noadapt
-  **DEPRECATED** When used in conjunction with ``--comp-lzo``, this option
-  will disable OpenVPN's adaptive compression algorithm. Normally, adaptive
-  compression is enabled with ``--comp-lzo``.
-
-  Adaptive compression tries to optimize the case where you have
-  compression enabled, but you are sending predominantly incompressible
-  (or pre-compressed) packets over the tunnel, such as an FTP or rsync
-  transfer of a large, compressed file. With adaptive compression, OpenVPN
-  will periodically sample the compression process to measure its
-  efficiency. If the data being sent over the tunnel is already
-  compressed, the compression efficiency will be very low, triggering
-  openvpn to disable compression for a period of time until the next
-  re-sample test.
+  **DEPRECATED** This option does not have any effect anymore since current
+  versions of OpenVPN never compress outgoing packets.
 
 --key-direction
   Alternative way of specifying the optional direction parameter for the
diff --git a/src/openvpn/comp.h b/src/openvpn/comp.h
index decf0d9..1bc4648 100644
--- a/src/openvpn/comp.h
+++ b/src/openvpn/comp.h
@@ -32,8 +32,8 @@ 
  * outside of the USE_COMP define */
 
 /* Compression flags */
-#define COMP_F_ADAPTIVE             (1<<0) /* COMP_ALG_LZO only */
 /*Removed */
+/*#define COMP_F_ADAPTIVE             (1<<0) / * COMP_ALG_LZO only */
 /*#define COMP_F_ALLOW_COMPRESS       (1<<1) / * not only downlink is compressed but also uplink * / */
 #define COMP_F_SWAP                 (1<<2) /* initial command byte is swapped with last byte in buffer to preserve payload alignment */
 #define COMP_F_ADVERTISE_STUBS_ONLY (1<<3) /* tell server that we only support compression stubs */
@@ -49,6 +49,7 @@ 
 #define COMP_ALG_LZO    2 /* LZO algorithm */
 #define COMP_ALG_SNAPPY 3 /* Snappy algorithm (no longer supported) */
 #define COMP_ALG_LZ4    4 /* LZ4 algorithm */
+#define COMP_ALG_LZO_NO 5 /* --comp-lzo no which is similar to COMP_ALG_STUB, but no SWAP */
 
 
 /* algorithm v2 */
diff --git a/src/openvpn/options.c b/src/openvpn/options.c
index 4745ddf..f8688d4 100644
--- a/src/openvpn/options.c
+++ b/src/openvpn/options.c
@@ -3448,6 +3448,24 @@ 
     }
 #endif /* ifdef _WIN32 */
 
+    if (options->mode == MODE_SERVER)
+    {
+        /*
+         * Enable comp migrate automatically on server
+         */
+        if (options->comp.alg == COMP_ALG_LZO || options->comp.alg == COMP_ALG_LZO_NO)
+        {
+            msg(M_INFO, "DEPRECATED OPTION: LZO compression enabled on the server side. "
+                "We will enable --compress migrate instead.");
+            options->comp.alg = COMP_ALG_UNDEF;
+            options->comp.flags = COMP_F_MIGRATE;
+        }
+    }
+    else if (options->comp.alg == COMP_ALG_LZO_NO)
+    {
+        options->comp.alg = COMP_ALG_STUB;
+    }
+
 #ifdef DEFAULT_PKCS11_MODULE
     /* If p11-kit is present on the system then load its p11-kit-proxy.so
      * by default if the user asks for PKCS#11 without otherwise specifying
@@ -8450,45 +8468,29 @@ 
 
         /* All lzo variants do not use swap */
         options->comp.flags &= ~COMP_F_SWAP;
+        options->comp.alg = COMP_ALG_LZO;
 
-        if (p[1] && streq(p[1], "no"))
+        if (p[1])
         {
-            options->comp.alg = COMP_ALG_STUB;
-            options->comp.flags &= ~COMP_F_ADAPTIVE;
-        }
-        else if (p[1])
-        {
-            if (streq(p[1], "yes"))
+            if (streq(p[1], "no"))
             {
-                options->comp.alg = COMP_ALG_LZO;
-                options->comp.flags &= ~COMP_F_ADAPTIVE;
+                options->comp.alg = COMP_ALG_LZO_NO;
             }
-            else if (streq(p[1], "adaptive"))
-            {
-                options->comp.alg = COMP_ALG_LZO;
-                options->comp.flags |= COMP_F_ADAPTIVE;
-            }
-            else
+            /* There is no actual difference anymore between these variants.
+             * We never compress. On the server side we replace this with
+             * --compress migrate later anyway.
+             */
+            else if (!(streq(p[1], "yes") || streq(p[1], "adaptive")))
             {
                 msg(msglevel, "bad comp-lzo option: %s -- must be 'yes', 'no', or 'adaptive'", p[1]);
                 goto err;
             }
         }
-        else
-        {
-            options->comp.alg = COMP_ALG_LZO;
-            options->comp.flags |= COMP_F_ADAPTIVE;
-        }
         show_compression_warning(&options->comp);
     }
     else if (streq(p[0], "comp-noadapt") && !p[1])
     {
-        /*
-         * We do not need to check here if we allow compression since
-         * it only modifies a flag if compression is enabled
-         */
-        VERIFY_PERMISSION(OPT_P_COMP);
-        options->comp.flags &= ~COMP_F_ADAPTIVE;
+        /* NO-OP since we never compress anymore */
     }
     else if (streq(p[0], "compress") && !p[2])
     {
@@ -8517,7 +8519,7 @@ 
         else if (streq(alg, "lzo"))
         {
             options->comp.alg = COMP_ALG_LZO;
-            options->comp.flags &= ~(COMP_F_ADAPTIVE | COMP_F_SWAP);
+            options->comp.flags &= ~COMP_F_SWAP;
         }
         else if (streq(alg, "lz4"))
         {