| Message ID | 20211201180727.2496903-1-arne@rfc2549.org |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series |
|
| Related | show |
Hi, On Wed, Dec 01, 2021 at 07:07:19PM +0100, Arne Schwabe wrote: > This allows to use the same configuration multiple platforms/ssl libraries > and include optional algorithms that are not available on all platforms > > For example "AES-256-GCM:AES-128-GCM:?CHACHA20-POLY1305" can be used to > emulate the default behaviour of OpenVPN 2.6. NAK on this, for two small and one big reason... > diff --git a/Changes.rst b/Changes.rst > index 7cceffcdb..c1a04deed 100644 > --- a/Changes.rst > +++ b/Changes.rst > @@ -58,6 +58,10 @@ OpenSSL 3.0 support > (and other deprecated) algorithm by default and the new option ``--providers`` > allows loading the legacy provider to renable these algorithms. > > +Optional ciphers in ``--data-ciphers`` > + Ciphers in ``--data-ciphers`` can now be prefixes with a ``?`` to mark > + those as optional and only use them if the SSL library supports them. Typo, "prefixed" > + > Deprecated features > ------------------- > ``inetd`` has been removed > diff --git a/doc/man-sections/protocol-options.rst b/doc/man-sections/protocol-options.rst > index c7aa6b0e3..7095b6f4d 100644 > diff --git a/src/openvpn/ssl_ncp.c b/src/openvpn/ssl_ncp.c > index 022a9dc3b..b0b248aae 100644 > --- a/src/openvpn/ssl_ncp.c > +++ b/src/openvpn/ssl_ncp.c > @@ -109,7 +109,18 @@ mutate_ncp_cipher_list(const char *list, struct gc_arena *gc) > * (and translate_cipher_name_from_openvpn/ > * translate_cipher_name_to_openvpn) also normalises the cipher name, > * e.g. replacing AeS-128-gCm with AES-128-GCM > + * > + * ciphers that have ? in front of them are considered optional and > + * OpenVPN will only warn if they are not found (and remove them from > + * the list) > */ > + > + bool optional = false; > + if (token[0] == '?') > + { > + token= token + 1; The whitespace dragon says this should be "token++;" (These are small, and I could fix that on the fly). The problem is here: > @@ -121,8 +132,9 @@ mutate_ncp_cipher_list(const char *list, struct gc_arena *gc) > } > if (!ktc && strcmp(token, "none") != 0) > { > - msg(M_WARN, "Unsupported cipher in --data-ciphers: %s", token); > - error_found = true; > + const char* optstr = optional ? "optional ": ""; > + msg(M_WARN, "Unsupported %scipher in --data-ciphers: %s", optstr, token); > + error_found = !optional; > } This only works if the last cipher in the list is "borken or optional". If you do openvpn --data-ciphers Vollbit:Littlebit:AES-256-CBC:BF-CBC:?nixbit it will log 2021-12-05 18:09:27 Unsupported cipher in --data-ciphers: Vollbit 2021-12-05 18:09:27 Unsupported cipher in --data-ciphers: Littlebit 2021-12-05 18:09:27 Unsupported optional cipher in --data-ciphers: nixbit ... and go ahead... Dec 5 18:09:28 gentoo tap-tcp-p2p[26765]: peer info: IV_CIPHERS=AES-256-CBC:BF-CBC without flagging "Vollbit" and "Littlebit" as hard errors. If the "nixbit" cipher is removed openvpn --data-ciphers Vollbit:Littlebit:AES-256-CBC:BF-CBC will lead to 2021-12-05 18:10:29 Unsupported cipher in --data-ciphers: Vollbit 2021-12-05 18:10:29 Unsupported cipher in --data-ciphers: Littlebit Options error: --data-ciphers list contains unsupported ciphers or is too long. Sorry for excercising my special ability of breaking everything :-) gert
Hi, On Sun, Dec 05, 2021 at 06:11:14PM +0100, Gert Doering wrote: > On Wed, Dec 01, 2021 at 07:07:19PM +0100, Arne Schwabe wrote: > > This allows to use the same configuration multiple platforms/ssl libraries > > and include optional algorithms that are not available on all platforms > > > > For example "AES-256-GCM:AES-128-GCM:?CHACHA20-POLY1305" can be used to > > emulate the default behaviour of OpenVPN 2.6. > > NAK on this, for two small and one big reason... Turns out that this is an ACK after all. It was not *that* late yesterday, but it seems my brain already went south... I merged this, tested it, figured I did not like the result, and forgot to "git reset" the tree... ... and today it slipped out with the next patch. Thus: commit 766044507497c41f0319159c37992788ecb681e6 Author: Arne Schwabe <arne@rfc2549.org> Date: Wed Dec 1 19:07:19 2021 +0100 Implement optional cipher in --data-ciphers prefixed with ? Signed-off-by: Arne Schwabe <arne@rfc2549.org> Acked-by: Gert Doering <gert@greenie.muc.de> Message-Id: <20211201180727.2496903-1-arne@rfc2549.org> URL: https://www.mail-archive.com/openvpn-devel@lists.sourceforge.net/msg23279.html (the patch does not *break* anything, does what it says on the lid, just is not strict enough about erroring out - and Arne already sent a final one for that) Sorry for the confusion. gert
diff --git a/Changes.rst b/Changes.rst index 7cceffcdb..c1a04deed 100644 --- a/Changes.rst +++ b/Changes.rst @@ -58,6 +58,10 @@ OpenSSL 3.0 support (and other deprecated) algorithm by default and the new option ``--providers`` allows loading the legacy provider to renable these algorithms. +Optional ciphers in ``--data-ciphers`` + Ciphers in ``--data-ciphers`` can now be prefixes with a ``?`` to mark + those as optional and only use them if the SSL library supports them. + Deprecated features ------------------- ``inetd`` has been removed diff --git a/doc/man-sections/protocol-options.rst b/doc/man-sections/protocol-options.rst index c7aa6b0e3..7095b6f4d 100644 --- a/doc/man-sections/protocol-options.rst +++ b/doc/man-sections/protocol-options.rst @@ -204,6 +204,13 @@ configured in a compatible way between both the local and remote side. supported by the client will be pushed to clients that support cipher negotiation. + Starting with OpenVPN 2.6 a cipher can be prefixed with a :code:`?` to mark + it as optional. This allows including ciphers in the list that may not be + available on all platforms. + E.g. :code:`AES-256-GCM:AES-128-GCM:?CHACHA20-POLY1305` would only enable + Chacha20-Poly1305 if the underlying SSL library (and its configuration) + supports it. + Cipher negotiation is enabled in client-server mode only. I.e. if ``--mode`` is set to 'server' (server-side, implied by setting ``--server`` ), or if ``--pull`` is specified (client-side, implied by diff --git a/src/openvpn/ssl_ncp.c b/src/openvpn/ssl_ncp.c index 022a9dc3b..b0b248aae 100644 --- a/src/openvpn/ssl_ncp.c +++ b/src/openvpn/ssl_ncp.c @@ -109,7 +109,18 @@ mutate_ncp_cipher_list(const char *list, struct gc_arena *gc) * (and translate_cipher_name_from_openvpn/ * translate_cipher_name_to_openvpn) also normalises the cipher name, * e.g. replacing AeS-128-gCm with AES-128-GCM + * + * ciphers that have ? in front of them are considered optional and + * OpenVPN will only warn if they are not found (and remove them from + * the list) */ + + bool optional = false; + if (token[0] == '?') + { + token= token + 1; + optional = true; + } const cipher_kt_t *ktc = cipher_kt_get(token); if (strcmp(token, "none") == 0) { @@ -121,8 +132,9 @@ mutate_ncp_cipher_list(const char *list, struct gc_arena *gc) } if (!ktc && strcmp(token, "none") != 0) { - msg(M_WARN, "Unsupported cipher in --data-ciphers: %s", token); - error_found = true; + const char* optstr = optional ? "optional ": ""; + msg(M_WARN, "Unsupported %scipher in --data-ciphers: %s", optstr, token); + error_found = !optional; } else { diff --git a/tests/unit_tests/openvpn/test_ncp.c b/tests/unit_tests/openvpn/test_ncp.c index 6fb0c0e51..faf09a36c 100644 --- a/tests/unit_tests/openvpn/test_ncp.c +++ b/tests/unit_tests/openvpn/test_ncp.c @@ -84,6 +84,17 @@ test_check_ncp_ciphers_list(void **state) assert_ptr_equal(mutate_ncp_cipher_list(bf_chacha, &gc), NULL); } + /* Check that optional ciphers work */ + assert_string_equal(mutate_ncp_cipher_list("AES-256-GCM:?vollbit:AES-128-GCM", &gc), + aes_ciphers); + + /* Check that optional ciphers work */ + assert_string_equal(mutate_ncp_cipher_list("?AES-256-GCM:?AES-128-GCM", &gc), + aes_ciphers); + + /* All unsupported should still yield an empty list */ + assert_ptr_equal(mutate_ncp_cipher_list("?kugelfisch:?grasshopper", &gc), NULL); + /* For testing that with OpenSSL 1.1.0+ that also accepts ciphers in * a different spelling the normalised cipher output is the same */ bool have_chacha_mixed_case = cipher_kt_get("ChaCha20-Poly1305");
This allows to use the same configuration multiple platforms/ssl libraries and include optional algorithms that are not available on all platforms For example "AES-256-GCM:AES-128-GCM:?CHACHA20-POLY1305" can be used to emulate the default behaviour of OpenVPN 2.6. Signed-off-by: Arne Schwabe <arne@rfc2549.org> --- Changes.rst | 4 ++++ doc/man-sections/protocol-options.rst | 7 +++++++ src/openvpn/ssl_ncp.c | 16 ++++++++++++++-- tests/unit_tests/openvpn/test_ncp.c | 11 +++++++++++ 4 files changed, 36 insertions(+), 2 deletions(-)