diff mbox series

[Openvpn-devel] Ignore --cipher for cipher negotiation in server client mode

Message ID 20200907161849.20508-1-arne@rfc2549.org
State Changes Requested
Headers
Series [Openvpn-devel] Ignore --cipher for cipher negotiation in server client mode |

Commit Message

Arne Schwabe Sept. 7, 2020, 6:18 a.m. UTC 
  OpenVPN will ignore --cipher in lieu of the replacement data-ciphers
for cipher negioation.

Signed-off-by: Arne Schwabe <arne@rfc2549.org>
---
 doc/man-sections/protocol-options.rst |  6 ++++--
 src/openvpn/options.c                 | 26 ++++----------------------
 2 files changed, 8 insertions(+), 24 deletions(-)

Comments

David Sommerseth March 4, 2021, 10:55 a.m. UTC | #1 
On 07/09/2020 18:18, Arne Schwabe wrote:
> OpenVPN will ignore --cipher in lieu of the replacement data-ciphers
> for cipher negioation.
> 
> Signed-off-by: Arne Schwabe <arne@rfc2549.org>
> ---
>   doc/man-sections/protocol-options.rst |  6 ++++--
>   src/openvpn/options.c                 | 26 ++++----------------------
>   2 files changed, 8 insertions(+), 24 deletions(-)

I'm not at all convinced about this one.  Because this essentially 
touches the same arguments we had a discussion about on --udp-mtu. 
Removing an outdated alias was too much hassle for no gain, despite 
being an alias not been seen in use for ages (it was deprecated before 
the v1.5 release, and no configs or bug reports turns up in various 
google/bing/yahoo/ddg searches using --udp-mtu).  Dropping this outdated 
option would with high probability *not* break any configuration in the 
wild.

However, one of the argument raised for keeping it as is was that it 
*could* break a configuration in the wild.  The behaviour between
--udp-mtu and --link-mtu are identical.


This change on --ciphers adds this warning if --cipher is used in 2.6+:

> +········msg(M_WARN,·"Note:·--cipher·set·to·'%s'·but·missing·in"
> +············"·--data-ciphers·(%s).·OpenVPN·2.6+·ignores·--cipher·for·"
> +············"cipher·negiotiation.",

Ignoring --cipher in a future release will have quite a higher 
probability of breaking existing configurations.  Now, this is set in 
context of --data-ciphers, which is very different code wise.  But the 
code for --ciphers is essentially the same as --data-ciphers-fallback. 
I am therefore of the opinion, based on the prior --udp-mtu discussion, 
that --ciphers should be an alias to --data-ciphers-fallback.

In addition, since adding a warning about using the deprecated --udp-mtu 
option and put up a plan for removing it was also considered too much, I 
don't see why that argument would be much different with --ciphers.
Arne Schwabe March 4, 2021, 12:14 p.m. UTC | #2 
> 
> Ignoring --cipher in a future release will have quite a higher
> probability of breaking existing configurations.  Now, this is set in
> context of --data-ciphers, which is very different code wise.  But the
> code for --ciphers is essentially the same as --data-ciphers-fallback.

It is not the same. After this change the following will happen:

--cipher sets *ONLY* the OCC cipher that we announce during the
handshake and give a cipher that it is being ignored.

--data-cipher-fallback sets the OCC cipher that we announcer *AND*
determines what cipher to use if and only the other side does not
announce any cipher.

> I am therefore of the opinion, based on the prior --udp-mtu discussion,
> that --ciphers should be an alias to --data-ciphers-fallback.
> 
> In addition, since adding a warning about using the deprecated --udp-mtu
> option and put up a plan for removing it was also considered too much, I
> don't see why that argument would be much different with --ciphers.

I think again the difference here is that we want to change what the
cipher directives and I think we can agree on:

- data-ciphers-fallback behaviour that is only useful for 2.3 and
earlier clients with --enable-small does not need to be turned on by
default anymore in 2.6. It is already turned off by default in 2.5 if
you have no '--cipher' in your config.

The --data-ciphers-fallback option is an option that allows you to
workaround these situations where the fallback is needed. We should keep
it as is.

Going forward we need:

A way that sets the cipher that will be announced in OCC to keep older
(everything 2.4 and below) happy, especially servers/clients that have
occ strict enabled.

Making --cipher only setting the OCC cipher allows us to be compatible
in this as long server/client are 2.4+ as these will negotiate to
AES-256-GCM.

One thing that we could discuss is if --cipher should add itself to
data-ciphers. But if it does we need another switch/option that disables
that behaviour again. Otherwise there is no way of migrating (in the
sense of not allowing) the cipher used by --cipher.

So in summary the options do a combination from:

a) set the occ cipher
b) set the fallback cipher when the peer is a non-OCC openvpn version
c) add themselves to data-ciphers

Currently in 2.5:

no '--cipher' in config: a) with BF-CBC

--cipher xyz in config a), b) and c) if not in --data-ciphers
                       a) but not b) when in --data-ciphers

--data-ciphers-fallback a), b) but not c)

Basically the idea was that when you ensured that data-ciphers is
correct or data-ciphers-fallback was used, you already configured the
config with the new NCP in mind.

My proposal for 2.6:

--cipher just does a)
--data-ciphers-fallback does a) and b)
diff mbox series

Patch

diff --git a/doc/man-sections/protocol-options.rst b/doc/man-sections/protocol-options.rst
index e9d5d63d..ca1407b9 100644
--- a/doc/man-sections/protocol-options.rst
+++ b/doc/man-sections/protocol-options.rst
@@ -57,8 +57,10 @@  configured in a compatible way between both the local and remote side.
   http://www.cs.ucsd.edu/users/mihir/papers/hmac.html
 
 --cipher alg
-  This option is deprecated for server-client mode. ``--data-ciphers``
-  or possibly `--data-ciphers-fallback`` should be used instead.
+  This option is ignored for server-client mode cipher selection.
+  ``--data-ciphers`` or possibly ``--data-ciphers-fallback`` must be used
+  instead.  It only determines which cipher is send in the
+  OCC string (see ``opt-verify``) for compatbility with old peers.
 
   Encrypt data channel packets with cipher algorithm ``alg``.
 
diff --git a/src/openvpn/options.c b/src/openvpn/options.c
index 01da88ad..7dc3e3eb 100644
--- a/src/openvpn/options.c
+++ b/src/openvpn/options.c
@@ -3074,12 +3074,6 @@  options_postprocess_cipher(struct options *o)
                          "--data-ciphers-fallback config option");
         }
 
-        msg(M_WARN, "--cipher is not set. Previous OpenVPN version defaulted to "
-            "BF-CBC as fallback when cipher negotiation failed in this case. "
-            "If you need this fallback please add '--data-ciphers-fallback "
-            "BF-CBC' to your configuration and/or add BF-CBC to "
-            "--data-ciphers.");
-
         /* We still need to set the ciphername to BF-CBC since various other
          * parts of OpenVPN assert that the ciphername is set */
         o->ciphername = "BF-CBC";
@@ -3087,22 +3081,10 @@  options_postprocess_cipher(struct options *o)
     else if (!o->enable_ncp_fallback
              && !tls_item_in_cipher_list(o->ciphername, o->ncp_ciphers))
     {
-        msg(M_WARN, "DEPRECATED OPTION: --cipher set to '%s' but missing in"
-            " --data-ciphers (%s). Future OpenVPN version will "
-            "ignore --cipher for cipher negotiations. "
-            "Add '%s' to --data-ciphers or change --cipher '%s' to "
-            "--data-ciphers-fallback '%s' to silence this warning.",
-            o->ciphername, o->ncp_ciphers, o->ciphername,
-            o->ciphername, o->ciphername);
-        o->enable_ncp_fallback = true;
-
-        /* Append the --cipher to ncp_ciphers to allow it in NCP */
-        size_t newlen = strlen(o->ncp_ciphers) + 1 + strlen(o->ciphername) + 1;
-        char *ncp_ciphers = gc_malloc(newlen, false, &o->gc);
-
-        ASSERT(openvpn_snprintf(ncp_ciphers, newlen, "%s:%s", o->ncp_ciphers,
-                                o->ciphername));
-        o->ncp_ciphers = ncp_ciphers;
+        msg(M_WARN, "Note: --cipher set to '%s' but missing in"
+            " --data-ciphers (%s). OpenVPN 2.6+ ignores --cipher for "
+            "cipher negiotiation.",
+            o->ciphername, o->ncp_ciphers);
     }
 }