| Message ID | 20211201180727.2496903-1-arne@rfc2549.org |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | [Openvpn-devel,1/9] Implement optional cipher in --data-ciphers prefixed with ? | expand |
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
Hi, following up on this one: On Mon, Dec 06, 2021 at 04:41:17PM +0100, Gert Doering wrote: > 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 After some discussion on IRC, Arne and I decided that this is useful for OpenSSL 3.0 compat in 2.5.9+ as well ("...:?BF-CBC") - when the patch was originally done, all the "optional BF-CBC" stuff was targeted at 2.6, and OpenSSL 3.0 support was also targeted at 2.6 only. With the Linux distributions shipping OpenSSL 3.0 with OpenVPN 2.5 left and right, this causes compat issues, so "?BF-CBC" is a nice-to-have compat hack (for those that *REALLY REALLY* want to enable this in their configs). The change is small, well-contained, and has unit tests, so "okay for 2.5". Thus: commit abe0bb1a7f727a24e2d7cb7215cb309aea5fcffc Author: Arne Schwabe <arne@rfc2549.org> Date: Wed Dec 1 19:07:19 2021 +0100 Implement optional cipher in --data-ciphers prefixed with ? (cherry picked from commit 766044507497c41f0319159c37992788ecb681e6) It needs a second patch, though, which I also pulled up: t b43a9b9f3324ccd7dffde3048c616aa5becc2b13 (HEAD -> release/2.5) Author: Arne Schwabe <arne@rfc2549.org> Date: Mon Dec 6 16:08:52 2021 +0100 Fix handling an optional invalid cipher at the end of data-ciphers (cherry picked from commit 868433857fbf8d71515ac0ffecb98eae893515dc) 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(-)