[Openvpn-devel,2/2] Include Chacha20-Poly1305 into default --data-ciphers when available

Message ID 20210818213354.687736-2-arne@rfc2549.org
State Accepted
Headers show
Series [Openvpn-devel,1/2] Detect unusable ciphers on patched OpenSSL of RHEL/Centos | expand

Commit Message

Arne Schwabe Aug. 18, 2021, 11:33 a.m. UTC
Most TLS 1.3 libraries inlcude the Chacha20-Poly1305 based cipher suite
beside the AES-GCM based ones int he list of default ciphers suites.
Chacha20-Poly1305 is accepted as good alternative AEAD algorithm to the
AES-GCM algorithm by crypto community.

Follow this and include Chacha20-Poly1305 by default in data-ciphers
when available. This makes picking Chacha20-Poly1305 easier as it only
requires to change server (by changing priority) or client side (removing
AES-GCM from data-ciphers) to change to Chacha20-Poly1305.

Signed-off-by: Arne Schwabe <arne@rfc2549.org>
---
 Changes.rst                             |  5 +++++
 doc/man-sections/cipher-negotiation.rst |  3 ++-
 doc/man-sections/protocol-options.rst   |  3 ++-
 src/openvpn/options.c                   | 25 ++++++++++++++++++++++++-
 4 files changed, 33 insertions(+), 3 deletions(-)

Comments

Antonio Quartulli Sept. 7, 2021, 3:53 a.m. UTC | #1
Hi,

On 18/08/2021 23:33, Arne Schwabe wrote:
> Most TLS 1.3 libraries inlcude the Chacha20-Poly1305 based cipher suite
> beside the AES-GCM based ones int he list of default ciphers suites.
> Chacha20-Poly1305 is accepted as good alternative AEAD algorithm to the
> AES-GCM algorithm by crypto community.
> 
> Follow this and include Chacha20-Poly1305 by default in data-ciphers
> when available. This makes picking Chacha20-Poly1305 easier as it only
> requires to change server (by changing priority) or client side (removing
> AES-GCM from data-ciphers) to change to Chacha20-Poly1305.
> 
> Signed-off-by: Arne Schwabe <arne@rfc2549.org>

Not sure why this is 2/2 as it seems unrelated to 1/2.
Indeed I applied and reviewed this patch alone.

It does what it says and imho it is pretty clean.

Tested with a client that connects against a server having default
ciphers. The client, if willing, can specify chacha20poly1305 only and
connect with it.

This is a sensible change which IMHO will make a lot of people happy.

Acked-by: Antonio Quartulli <antonio@openvpn.net>
Gert Doering Sept. 7, 2021, 6 a.m. UTC | #2
I have not tested this "in-depth", but the code looks good, and the result
passes my t_client rig with various OpenSSL and mbedTLS versions.

(I have not actually tested "both server and client have CHACHA-POLY
active and using that works" but I trust Antonio there :-) )

Your patch has been applied to the master branch.

commit a38a377fd524d0e14a23ed17487ea3e3d3ad3fe7
Author: Arne Schwabe
Date:   Wed Aug 18 23:33:54 2021 +0200

     Include Chacha20-Poly1305 into default --data-ciphers when available

     Signed-off-by: Arne Schwabe <arne@rfc2549.org>
     Acked-by: Antonio Quartulli <antonio@openvpn.net>
     Message-Id: <20210818213354.687736-2-arne@rfc2549.org>
     URL: https://www.mail-archive.com/openvpn-devel@lists.sourceforge.net/msg22745.html
     Signed-off-by: Gert Doering <gert@greenie.muc.de>


--
kind regards,

Gert Doering

Patch

diff --git a/Changes.rst b/Changes.rst
index 0323a7f7a..637ed97a6 100644
--- a/Changes.rst
+++ b/Changes.rst
@@ -65,6 +65,11 @@  Deprecated features
     This option mainly served a role as debug option when NCP was first
     introduced. It should now no longer be necessary.
 
+
+User-visible Changes
+--------------------
+- CHACHA20-POLY1305 is included in the default of ``--data-ciphers`` when available.
+
 Overview of changes in 2.5
 ==========================
 
diff --git a/doc/man-sections/cipher-negotiation.rst b/doc/man-sections/cipher-negotiation.rst
index a2feb5f9c..423b5ab6a 100644
--- a/doc/man-sections/cipher-negotiation.rst
+++ b/doc/man-sections/cipher-negotiation.rst
@@ -18,7 +18,8 @@  with a AUTH_FAILED message (as seen in client log):
 OpenVPN 2.5 will only allow the ciphers specified in ``--data-ciphers``. To ensure
 backwards compatibility also if a cipher is specified using the ``--cipher`` option
 it is automatically added to this list. If both options are unset the default is
-:code:`AES-256-GCM:AES-128-GCM`.
+:code:`AES-256-GCM:AES-128-GCM`. In 2.6 and later the default is changed to
+:code:`AES-256-GCM:AES-128-GCM:CHACHA20-POLY1305` when Chacha20-Poly1305 is available.
 
 OpenVPN 2.4 clients
 -------------------
diff --git a/doc/man-sections/protocol-options.rst b/doc/man-sections/protocol-options.rst
index 5ae780e1f..0fef90f7b 100644
--- a/doc/man-sections/protocol-options.rst
+++ b/doc/man-sections/protocol-options.rst
@@ -192,7 +192,8 @@  configured in a compatible way between both the local and remote side.
 --data-ciphers cipher-list
   Restrict the allowed ciphers to be negotiated to the ciphers in
   ``cipher-list``. ``cipher-list`` is a colon-separated list of ciphers,
-  and defaults to :code:`AES-256-GCM:AES-128-GCM`.
+  and defaults to :code:`AES-256-GCM:AES-128-GCM:CHACHA20-POLY1305` when
+  Chacha20-Poly1305 is available and otherwise :code:`AES-256-GCM:AES-128-GCM`.
 
   For servers, the first cipher from ``cipher-list`` that is also
   supported by the client will be pushed to clients that support cipher
diff --git a/src/openvpn/options.c b/src/openvpn/options.c
index 7e146db90..9c01d6a1d 100644
--- a/src/openvpn/options.c
+++ b/src/openvpn/options.c
@@ -842,7 +842,6 @@  init_options(struct options *o, const bool init_gc)
     o->stale_routes_check_interval = 0;
     o->ifconfig_pool_persist_refresh_freq = 600;
     o->scheduled_exit_interval = 5;
-    o->ncp_ciphers = "AES-256-GCM:AES-128-GCM";
     o->authname = "SHA1";
     o->prng_hash = "SHA1";
     o->prng_nonce_secret_len = 16;
@@ -3077,6 +3076,29 @@  options_postprocess_verify(const struct options *o)
     }
 }
 
+/**
+ * Checks for availibility of Chacha20-Poly1305 and sets
+ * the ncp_cipher to either AES-256-GCM:AES-128-GCM or
+ * AES-256-GCM:AES-128-GCM:CHACHA20-POLY1305.
+ */
+static void
+options_postprocess_setdefault_ncpciphers(struct options *o)
+{
+    if (o->ncp_ciphers)
+    {
+        /* custom --data-ciphers set, keep list */
+        return;
+    }
+    else if (cipher_kt_get("CHACHA20-POLY1305"))
+    {
+        o->ncp_ciphers = "AES-256-GCM:AES-128-GCM:CHACHA20-POLY1305";
+    }
+    else
+    {
+        o->ncp_ciphers = "AES-256-GCM:AES-128-GCM";
+    }
+}
+
 static void
 options_postprocess_cipher(struct options *o)
 {
@@ -3137,6 +3159,7 @@  options_postprocess_mutate(struct options *o)
     helper_keepalive(o);
     helper_tcp_nodelay(o);
 
+    options_postprocess_setdefault_ncpciphers(o);
     options_postprocess_cipher(o);
     options_postprocess_mutate_invariant(o);