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 <fr...@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

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"))
         {

--
To view, visit http://gerrit.openvpn.net/c/openvpn/+/756?usp=email
To unsubscribe, or for help writing mail filters, visit 
http://gerrit.openvpn.net/settings

Gerrit-Project: openvpn
Gerrit-Branch: master
Gerrit-Change-Id: I00209b880cfcedd93e28f97fc3941d8b85e095f3
Gerrit-Change-Number: 756
Gerrit-PatchSet: 1
Gerrit-Owner: flichtenheld <fr...@lichtenheld.com>
Gerrit-Reviewer: plaisthos <arne-open...@rfc2549.org>
Gerrit-CC: openvpn-devel <openvpn-devel@lists.sourceforge.net>
Gerrit-Attention: plaisthos <arne-open...@rfc2549.org>
Gerrit-MessageType: newchange
_______________________________________________
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel

Reply via email to