diff mbox series

[Openvpn-devel,3/7] reject compression by default

Message ID 20210904095629.6273-4-a@unstable.cc
State Accepted
Headers show
Series change defaults and introduce compat-mode | expand

Commit Message

Antonio Quartulli Sept. 3, 2021, 11:56 p.m. UTC 
With this change the value of '--allow-compression- is set to 'no'.
Therefore compression is not enabled by default and cannot be enabled
by the server either.

This change is in line with the current rend of not recommending
compression over VPN tunnels for security reasons (check Voracle).

Of top of that compression is mostly useless nowadays, therefore
there is not real reason to enable it.

Signed-off-by: Arne Schwabe <arne@rfc2549.org>
Signed-off-by: Antonio Quartulli <a@unstable.cc>
---
 Changes.rst                          |  7 +++++++
 doc/man-sections/generic-options.rst |  6 ++++++
 src/openvpn/comp.h                   |  1 +
 src/openvpn/options.c                | 11 +++++++++++
 4 files changed, 25 insertions(+)

Comments

Arne Schwabe Sept. 6, 2021, 3:23 a.m. UTC | #1 
Am 04.09.21 um 11:56 schrieb Antonio Quartulli:
> With this change the value of '--allow-compression- is set to 'no'.
> Therefore compression is not enabled by default and cannot be enabled
> by the server either.
> 
> This change is in line with the current rend of not recommending

I think rend should be trend


>  /*
> diff --git a/src/openvpn/options.c b/src/openvpn/options.c
> index 4d971a56..21c76a69 100644
> --- a/src/openvpn/options.c
> +++ b/src/openvpn/options.c
> @@ -3146,6 +3146,16 @@ need_compatibility_before(const struct options *o, int version)
>  static void
>  options_set_backwards_compatible_options(struct options *o)
>  {
> +    /* Compression is deprecated and we do not want to announce support for it
> +     * by default anymore, additionally DCO breaks with compression.
> +     *
> +     * Disable compression by default starting with 2.6.0 if no other
> +     * compression related option has been explicitly set */
> +    if (!comp_non_stub_enabled(&o->comp) && !need_compatibility_before(o, 20600)
> +        && (o->comp.flags == 0))
> +    {
> +        o->comp.flags = COMP_F_ALLOW_STUB_ONLY|COMP_F_ADVERTISE_STUBS_ONLY;
> +    }
>  }
>  
>  static void
> @@ -7732,6 +7742,7 @@ add_option(struct options *options,
>          else if (streq(p[1], "asym"))
>          {
>              options->comp.flags &= ~COMP_F_ALLOW_COMPRESS;
> +            options->comp.flags |= COMP_F_ALLOW_ASYM;
>          }
>          else if (streq(p[1], "yes"))
>          {
> 

Acked-By: Arne Schwabe <arne@rfc2549.org>
Gert Doering Sept. 7, 2021, 11:51 p.m. UTC | #2 
This needed a bit of massaging for the Changes.rst hunk.  The rest
was straightforward.

The interaction of this with existing configs is interesting.

 - if the local config has "comp-lzo" or "compress <something>" in it,
   this will still work(!) - because the "allow-compress no" changed 
   default will only become active after the config is parsed
   (and the client will announce IV_LZO=1 to the server)

 - if the local config has *no* "comp-lzo" or "compress" statements,
   "allow-compress no" becomes active, and no IV_LZO/IV_LZ4 is sent
   to the server

 - if the local config has no compression setting but --compat-mode 2.3.0
   is set, we also announce IV_LZO/IV_LZ4 to the server

So the compat bit and changed default works, it is only "not active yet"
when reading the local config.  OTOH, users are free to put stuff into
their local config, including "allow-compression yes", to override the
new defaults - so I think this is okay, if properly understood.

Making the (possibly) changed "allow-compression" setting active already
when reading the config is a more intrusive change, as one would need to
recalculate defaults right upon hitting the "compat-mode" statement
(and the "compat-mode" statement would need to be *before* the "compress"
statement otherwise disallowed) - we could do that, but I'm not sure it's
worth it.  Definitely worth discussing.


One interesting side effect of the current code is that none of the 
t_client buildslaves break - because they all have "comp-lzo" in the
configs (for historic reasons, sorry...) and thus everything keeps
working :-) - need to modify these cases a add variants.


Your patch has been applied to the master branch.

commit 79367a3fde433d0660cc7122aa21c3c76ee6b2da
Author: Antonio Quartulli
Date:   Sat Sep 4 11:56:25 2021 +0200

     reject compression by default

     Signed-off-by: Arne Schwabe <arne@rfc2549.org>
     Signed-off-by: Antonio Quartulli <a@unstable.cc>
     Acked-by: Arne Schwabe <arne@rfc2549.org>
     Message-Id: <20210904095629.6273-4-a@unstable.cc>
     URL: https://www.mail-archive.com/openvpn-devel@lists.sourceforge.net/msg22797.html
     Signed-off-by: Gert Doering <gert@greenie.muc.de>


--
kind regards,

Gert Doering
diff mbox series

Patch

diff --git a/Changes.rst b/Changes.rst
index f55b0e3e..65b838b9 100644
--- a/Changes.rst
+++ b/Changes.rst
@@ -71,6 +71,13 @@  Deprecated features
     This option mainly served a role as debug option when NCP was first
     introduced. It should now no longer be necessary.
 
+Compression no longer enabled by default
+    Unless an explicit compression option is specified in the configuration,
+    ``--allow-compression`` defaults to ``no`` in OpeNVPN 2.6.0.
+    By default, OpenVPN 2.5 still allowed a server to enable compression by
+    pushing compression related options.
+
+
 Overview of changes in 2.5
 ==========================
 
diff --git a/doc/man-sections/generic-options.rst b/doc/man-sections/generic-options.rst
index 63c6227c..a8d24572 100644
--- a/doc/man-sections/generic-options.rst
+++ b/doc/man-sections/generic-options.rst
@@ -61,6 +61,12 @@  which mode OpenVPN is configured as.
   Note: Using this option reverts defaults to no longer recommended
   values and should be avoided if possible.
 
+  The following table details what defaults are changed depending on the
+  version specified.
+
+  - 2.5.x or lower: ``--allow-compression asym`` is automatically added
+    to the configuration if no other compression options are present.
+
 --config file
   Load additional config options from ``file`` where each line corresponds
   to one command line option, but with the leading '--' removed.
diff --git a/src/openvpn/comp.h b/src/openvpn/comp.h
index cd4f0e1a..619a574e 100644
--- a/src/openvpn/comp.h
+++ b/src/openvpn/comp.h
@@ -59,6 +59,7 @@ 
 #define COMP_F_ALLOW_STUB_ONLY      (1<<4) /* Only accept stub compression, even with COMP_F_ADVERTISE_STUBS_ONLY
                                             * we still accept other compressions to be pushed */
 #define COMP_F_MIGRATE              (1<<5) /* push stub-v2 or comp-lzo no when we see a client with comp-lzo in occ */
+#define COMP_F_ALLOW_ASYM           (1<<6) /* Compression was explicitly set to allow assymetric compression */
 
 
 /*
diff --git a/src/openvpn/options.c b/src/openvpn/options.c
index 4d971a56..21c76a69 100644
--- a/src/openvpn/options.c
+++ b/src/openvpn/options.c
@@ -3146,6 +3146,16 @@  need_compatibility_before(const struct options *o, int version)
 static void
 options_set_backwards_compatible_options(struct options *o)
 {
+    /* Compression is deprecated and we do not want to announce support for it
+     * by default anymore, additionally DCO breaks with compression.
+     *
+     * Disable compression by default starting with 2.6.0 if no other
+     * compression related option has been explicitly set */
+    if (!comp_non_stub_enabled(&o->comp) && !need_compatibility_before(o, 20600)
+        && (o->comp.flags == 0))
+    {
+        o->comp.flags = COMP_F_ALLOW_STUB_ONLY|COMP_F_ADVERTISE_STUBS_ONLY;
+    }
 }
 
 static void
@@ -7732,6 +7742,7 @@  add_option(struct options *options,
         else if (streq(p[1], "asym"))
         {
             options->comp.flags &= ~COMP_F_ALLOW_COMPRESS;
+            options->comp.flags |= COMP_F_ALLOW_ASYM;
         }
         else if (streq(p[1], "yes"))
         {