| Message ID | 20210904095629.6273-5-a@unstable.cc |
|---|---|
| State | Accepted |
| Headers | show |
| Series |
|
| Related | show |
Am 04.09.21 um 11:56 schrieb Antonio Quartulli: > The --cipher option has been there since a while, but it became more and > more confusing since the introduction of NCP (data cipher negotiation). > > The fallback cipher can now be specified via --data-cipher-fallback, > while the list of accepted ciphers is specified via --data-ciphers. > > --cipher can still be used for compatibility reasons, but won't affect > the cipher negotiation. > Acked-By: Arne Schwabe <arne@rfc2549.org>
Hi,
we discussed on IRC how to improve the Changes.rst and the manpage part
about --cipher. Here is the result:
Changes.rst:
``--cipher`` argument is no longer appended to ``--data-ciphers``
by default. Data cipher negotiation has been introduced in 2.4.0
and been significantly improved in 2.5.0. The implicit fallback
to the cipher specified in ``--cipher`` has been removed.
Effectively, ``--cipher`` is a no-op in TLS mode now, and will
only have an effect in pre-shared-key mode (``--secret``).
From now on ``--cipher`` should not be used in new configurations
for TLS mode.
Should backwards compatibility with older OpenVPN peers be
required, please see the ``--compat-mode`` instead.
manpage:
--cipher alg
This option should not be used any longer in TLS mode and still
exists for two reasons:
* compatibility with old configurations still carrying it
around;
* allow users connecting to OpenVPN peers older than 2.6.0
to have ``--cipher`` configured the same way as the remote
counterpart. This can avoid MTU/frame size warnings.
Before 2.4.0, this option was used to select the cipher to be
configured on the data channel, however, later versions usually
ignored this directive in favour of a negotiated cipher.
Starting with 2.6.0, this option is always ignored in TLS mode
when it comes to configuring the cipher and will only control the
cipher for ``--secret`` pre-shared-key mode (note: this mode is
deprecated strictly not recommended).
If you wish to specify the cipher to use on the data channel,
please see ``--data-ciphers`` (for regular negotiation) and
``--data-ciphers-fallback`` (for a fallback option when the
negotiation cannot take place because the other peer is old or
has negotiation disabled).
I hope the formatting will not be messed up.
Gert offered to add this text to the patch while committing.
Regards,
Your patch has been applied to the master branch.
As discussed on IRC, the Changes.rst entry was extended, and the
--cipher entry in the manpage (doc/man-sections/protocol-options.rst)
was replaced with the new text. I'm not sure if it is clear enough
now, so I susggest we revisit the whole "document ciphers and NCP"
topic in the "pre-release" phase for 2.6 - Richard's help would
certainly be most welcome there.
The code itself looks reasonable. Some basic tests with my current
t_client rig confirms that it broke all the "--cipher BF-CBC" tests
(that had this explicitly added because NCP broke the frame stuff,
thus "big packets"). Adding "--data-ciphers BF-CBC" fixed those
tests, and so did "--compat-mode 2.4.0".
commit 65f6da8eeb84fbcea357765e13fa73d0169a143c
Author: Antonio Quartulli
Date: Sat Sep 4 11:56:26 2021 +0200
do not include --cipher value in data-ciphers
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-5-a@unstable.cc>
URL: https://www.mail-archive.com/openvpn-devel@lists.sourceforge.net/msg22799.html
Signed-off-by: Gert Doering <gert@greenie.muc.de>
--
kind regards,
Gert Doering
diff --git a/Changes.rst b/Changes.rst index 65b838b9..f803b760 100644 --- a/Changes.rst +++ b/Changes.rst @@ -71,6 +71,11 @@ Deprecated features This option mainly served a role as debug option when NCP was first introduced. It should now no longer be necessary. +``--cipher`` argument is no longer included in ``--data-ciphers`` by default + Data cipher negotiation has been introduced in 2.4.0 and been significantly + improved in 2.5.0. The implicit fallback to the cipher specified in + ``--cipher`` has been removed. + 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. diff --git a/doc/man-sections/generic-options.rst b/doc/man-sections/generic-options.rst index a8d24572..8b26cd1a 100644 --- a/doc/man-sections/generic-options.rst +++ b/doc/man-sections/generic-options.rst @@ -66,6 +66,8 @@ which mode OpenVPN is configured as. - 2.5.x or lower: ``--allow-compression asym`` is automatically added to the configuration if no other compression options are present. + - 2.4.x or lower: The cipher in ``--cipher`` is appended to + ``--data-ciphers`` --config file Load additional config options from ``file`` where each line corresponds diff --git a/src/openvpn/options.c b/src/openvpn/options.c index 21c76a69..88ac5bed 100644 --- a/src/openvpn/options.c +++ b/src/openvpn/options.c @@ -3102,26 +3102,20 @@ options_postprocess_cipher(struct options *o) /* 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"; + + msg(M_INFO, "Note: --cipher is not set. OpenVPN versions before 2.6 " + "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."); } 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, "DEPRECATED OPTION: --cipher set to '%s' but missing in " + "--data-ciphers (%s). OpenVPN ignores --cipher for cipher " + "negotiations. ", + o->ciphername, o->ncp_ciphers); } } @@ -3146,6 +3140,18 @@ need_compatibility_before(const struct options *o, int version) static void options_set_backwards_compatible_options(struct options *o) { + /* Versions < 2.5.0 do need --cipher in the list of accepted ciphers. + * Version 2.4 might probably does not need it but NCP was not so + * good with 2.4 and ncp-disable might be more common on 2.4 peers. + * Only do this iif --cipher is not explicitly (BF-CBC). This is not + * 100% correct backwards compatible behaviour but 2.5 already behaved like + * this */ + if (o->ciphername && need_compatibility_before(o, 20500) + && !tls_item_in_cipher_list(o->ciphername, o->ncp_ciphers)) + { + append_cipher_to_ncp_list(o, o->ciphername); + } + /* Compression is deprecated and we do not want to announce support for it * by default anymore, additionally DCO breaks with compression. * diff --git a/src/openvpn/ssl_ncp.c b/src/openvpn/ssl_ncp.c index 6967e2bb..022a9dc3 100644 --- a/src/openvpn/ssl_ncp.c +++ b/src/openvpn/ssl_ncp.c @@ -172,6 +172,19 @@ mutate_ncp_cipher_list(const char *list, struct gc_arena *gc) return ret; } + +void +append_cipher_to_ncp_list(struct options *o, const char *ciphername) +{ + /* Append the --cipher to ncp_ciphers to allow it in NCP */ + size_t newlen = strlen(o->ncp_ciphers) + 1 + strlen(ciphername) + 1; + char *ncp_ciphers = gc_malloc(newlen, false, &o->gc); + + ASSERT(openvpn_snprintf(ncp_ciphers, newlen, "%s:%s", o->ncp_ciphers, + ciphername)); + o->ncp_ciphers = ncp_ciphers; +} + bool tls_item_in_cipher_list(const char *item, const char *list) { diff --git a/src/openvpn/ssl_ncp.h b/src/openvpn/ssl_ncp.h index 4a2601a2..09ddeb28 100644 --- a/src/openvpn/ssl_ncp.h +++ b/src/openvpn/ssl_ncp.h @@ -102,6 +102,14 @@ tls_peer_ncp_list(const char *peer_info, struct gc_arena *gc); char * mutate_ncp_cipher_list(const char *list, struct gc_arena *gc); +/** + * Appends the cipher specified by the ciphernamer parameter to to + * the o->ncp_ciphers list. + * @param o options struct to modify. Its gc is also used + * @param ciphername the ciphername to add + */ +void append_cipher_to_ncp_list(struct options *o, const char *ciphername); + /** * Return true iff item is present in the colon-separated zero-terminated * cipher list.