Hi,
On 20/05/2021 17:11, Arne Schwabe wrote:
> NCP has proven to be stable and apart from the one VPN Provider doing
> hacky things with homebrewed NCP we have not had any reports about
> ncp-disable being required. Remove ncp-disable to simplify code paths.
>
> Note: This patch breaks client without --pull. The follow up patch
> for P2P NCP will restore that. But to avoid all the NCP/non-NCP special
> cases to be implemented in P2P. P2P will directly switch from always
> non-NCP to always NCP.
>
> Signed-off-by: Arne Schwabe <a...@rfc2549.org>
I tested this patch on top of master and against master, 2.5 and 2.4
(2.3 did not compile and I did not want to check why) and I alternated
server and client.
Everything works also when the server is running 2.4 with --ncp-disabled
and the client is master+this-patch.
Change also looks good and it is fairly contained.
Acked-by: Antonio Quartulli <anto...@openvpn.net>
> ---
> Changes.rst | 4 +++
> doc/man-sections/protocol-options.rst | 8 ++----
> src/openvpn/init.c | 17 ++++---------
> src/openvpn/multi.c | 4 ---
> src/openvpn/options.c | 36 +++------------------------
> src/openvpn/options.h | 1 -
> src/openvpn/ssl.c | 3 +--
> src/openvpn/ssl_common.h | 1 -
> src/openvpn/ssl_ncp.c | 4 ---
> 9 files changed, 16 insertions(+), 62 deletions(-)
>
> diff --git a/Changes.rst b/Changes.rst
> index 9185b55f7..e7ae6abed 100644
> --- a/Changes.rst
> +++ b/Changes.rst
> @@ -57,6 +57,10 @@ Deprecated features
> is considered "too complicated", using ``--peer-fingerprint`` makes
> TLS mode about as easy as using ``--secret``.
>
> +``ncp-disable`` has been removed
> + This option mainly served a role as debug option when NCP was first
> + introduced. It should now no longer be necessary.
> +
> Overview of changes in 2.5
> ==========================
>
> diff --git a/doc/man-sections/protocol-options.rst
> b/doc/man-sections/protocol-options.rst
> index 34d4255ee..5ae780e1f 100644
> --- a/doc/man-sections/protocol-options.rst
> +++ b/doc/man-sections/protocol-options.rst
> @@ -65,8 +65,8 @@ configured in a compatible way between both the local and
> remote side.
> The default is :code:`BF-CBC`, an abbreviation for Blowfish in Cipher
> Block Chaining mode. When cipher negotiation (NCP) is allowed,
> OpenVPN 2.4 and newer on both client and server side will automatically
> - upgrade to :code:`AES-256-GCM`. See ``--data-ciphers`` and
> - ``--ncp-disable`` for more details on NCP.
> + upgrade to :code:`AES-256-GCM`. See ``--data-ciphers`` for more details
> + on NCP.
>
> Using :code:`BF-CBC` is no longer recommended, because of its 64-bit
> block size. This small block size allows attacks based on collisions, as
> @@ -235,10 +235,6 @@ configured in a compatible way between both the local
> and remote side.
> have been configured with `--enable-small`
> (typically used on routers or other embedded devices).
>
> ---ncp-disable
> - **DEPRECATED** Disable "Negotiable Crypto Parameters". This completely
> - disables cipher negotiation.
> -
> --secret args
> **DEPRECATED** Enable Static Key encryption mode (non-TLS). Use pre-shared
> secret
> ``file`` which was generated with ``--genkey``.
> diff --git a/src/openvpn/init.c b/src/openvpn/init.c
> index 4335d4870..38abf9f3a 100644
> --- a/src/openvpn/init.c
> +++ b/src/openvpn/init.c
> @@ -2227,18 +2227,14 @@ pull_permission_mask(const struct context *c)
> | OPT_P_EXPLICIT_NOTIFY
> | OPT_P_ECHO
> | OPT_P_PULL_MODE
> - | OPT_P_PEER_ID;
> + | OPT_P_PEER_ID
> + | OPT_P_NCP;
>
> if (!c->options.route_nopull)
> {
> flags |= (OPT_P_ROUTE | OPT_P_IPWIN32);
> }
>
> - if (c->options.ncp_enabled)
> - {
> - flags |= OPT_P_NCP;
> - }
> -
> return flags;
> }
>
> @@ -2720,8 +2716,6 @@ do_init_crypto_tls_c1(struct context *c)
> *
> * Therefore, the key structure has to be initialized when:
> * - any non-BF-CBC cipher was selected; or
> - * - BF-CBC is selected and NCP is disabled (explicit request to
> - * use the BF-CBC cipher); or
> * - BF-CBC is selected, NCP is enabled and fallback is enabled
> * (BF-CBC will be the fallback).
> * - BF-CBC is in data-ciphers and we negotiate to use BF-CBC:
> @@ -2731,12 +2725,12 @@ do_init_crypto_tls_c1(struct context *c)
> * Note that BF-CBC will still be part of the OCC string to retain
> * backwards compatibility with older clients.
> */
> - if (!streq(options->ciphername, "BF-CBC") || !options->ncp_enabled
> - || (options->ncp_enabled && tls_item_in_cipher_list("BF-CBC",
> options->ncp_ciphers))
> + if (!streq(options->ciphername, "BF-CBC")
> + || tls_item_in_cipher_list("BF-CBC", options->ncp_ciphers)
> || options->enable_ncp_fallback)
> {
> /* Do not warn if the if the cipher is used only in OCC */
> - bool warn = !options->ncp_enabled ||
> options->enable_ncp_fallback;
> + bool warn = options->enable_ncp_fallback;
> init_key_type(&c->c1.ks.key_type, options->ciphername,
> options->authname,
> true, warn);
> }
> @@ -2838,7 +2832,6 @@ do_init_crypto_tls(struct context *c, const unsigned
> int flags)
> to.tcp_mode = link_socket_proto_connection_oriented(options->ce.proto);
> to.config_ciphername = c->options.ciphername;
> to.config_ncp_ciphers = c->options.ncp_ciphers;
> - to.ncp_enabled = options->ncp_enabled;
> to.transition_window = options->transition_window;
> to.handshake_window = options->handshake_window;
> to.packet_timeout = options->tls_timeout;
> diff --git a/src/openvpn/multi.c b/src/openvpn/multi.c
> index 7cb9e86aa..2507108e2 100644
> --- a/src/openvpn/multi.c
> +++ b/src/openvpn/multi.c
> @@ -1791,10 +1791,6 @@ multi_client_set_protocol_options(struct context *c)
> #endif
>
> /* Select cipher if client supports Negotiable Crypto Parameters */
> - if (!o->ncp_enabled)
> - {
> - return true;
> - }
>
> /* if we have already created our key, we cannot *change* our own
> * cipher -> so log the fact and push the "what we have now" cipher
> diff --git a/src/openvpn/options.c b/src/openvpn/options.c
> index fa3ee50d6..2f4ccaa09 100644
> --- a/src/openvpn/options.c
> +++ b/src/openvpn/options.c
> @@ -526,7 +526,6 @@ static const char usage_message[] =
> " (default=%s).\n"
> " Set alg=none to disable encryption.\n"
> "--data-ciphers list : List of ciphers that are allowed to be
> negotiated.\n"
> - "--ncp-disable : (DEPRECATED) Disable cipher negotiation.\n"
> "--prng alg [nsl] : For PRNG, use digest algorithm alg, and\n"
> " nonce_secret_len=nsl. Set alg=none to disable
> PRNG.\n"
> #ifndef ENABLE_CRYPTO_MBEDTLS
> @@ -843,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_enabled = true;
> o->ncp_ciphers = "AES-256-GCM:AES-128-GCM";
> o->authname = "SHA1";
> o->prng_hash = "SHA1";
> @@ -1719,7 +1717,6 @@ show_settings(const struct options *o)
> SHOW_STR_INLINE(shared_secret_file);
> SHOW_PARM(key_direction, keydirection2ascii(o->key_direction, false,
> true), "%s");
> SHOW_STR(ciphername);
> - SHOW_BOOL(ncp_enabled);
> SHOW_STR(ncp_ciphers);
> SHOW_STR(authname);
> SHOW_STR(prng_hash);
> @@ -3082,7 +3079,6 @@ options_postprocess_cipher(struct options *o)
> if (!o->pull && !(o->mode == MODE_SERVER))
> {
> /* we are in the classic P2P mode */
> - o->ncp_enabled = false;
> msg( M_WARN, "Cipher negotiation is disabled since neither "
> "P2MP client nor server mode is enabled");
>
> @@ -3099,18 +3095,6 @@ options_postprocess_cipher(struct options *o)
> /* pull or P2MP mode */
> if (!o->ciphername)
> {
> - if (!o->ncp_enabled)
> - {
> - msg(M_USAGE, "--ncp-disable needs an explicit --cipher or "
> - "--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";
> @@ -3152,13 +3136,10 @@ options_postprocess_mutate(struct options *o)
> options_postprocess_cipher(o);
> options_postprocess_mutate_invariant(o);
>
> - if (o->ncp_enabled)
> + o->ncp_ciphers = mutate_ncp_cipher_list(o->ncp_ciphers, &o->gc);
> + if (o->ncp_ciphers == NULL)
> {
> - o->ncp_ciphers = mutate_ncp_cipher_list(o->ncp_ciphers, &o->gc);
> - if (o->ncp_ciphers == NULL)
> - {
> - msg(M_USAGE, "NCP cipher list contains unsupported ciphers or is
> too long.");
> - }
> + msg(M_USAGE, "NCP cipher list contains unsupported ciphers or is too
> long.");
> }
>
> if (o->remote_list && !o->connection_list)
> @@ -3908,8 +3889,7 @@ options_string(const struct options *o,
> }
> /* Only announce the cipher to our peer if we are willing to
> * support it */
> - if (p2p_nopull || !o->ncp_enabled
> - || tls_item_in_cipher_list(ciphername, o->ncp_ciphers))
> + if (p2p_nopull || tls_item_in_cipher_list(ciphername,
> o->ncp_ciphers))
> {
> buf_printf(&out, ",cipher %s", ciphername);
> }
> @@ -7994,14 +7974,6 @@ add_option(struct options *options,
> msg(msglevel, "Unknown key-derivation method %s", p[1]);
> }
> }
> - else if (streq(p[0], "ncp-disable") && !p[1])
> - {
> - VERIFY_PERMISSION(OPT_P_GENERAL|OPT_P_INSTANCE);
> - options->ncp_enabled = false;
> - msg(M_WARN, "DEPRECATED OPTION: ncp-disable. Disabling "
> - "cipher negotiation is a deprecated debug feature that "
> - "will be removed in OpenVPN 2.6");
> - }
> else if (streq(p[0], "prng") && p[1] && !p[3])
> {
> VERIFY_PERMISSION(OPT_P_GENERAL);
> diff --git a/src/openvpn/options.h b/src/openvpn/options.h
> index 41e84f7e1..69897c5a7 100644
> --- a/src/openvpn/options.h
> +++ b/src/openvpn/options.h
> @@ -512,7 +512,6 @@ struct options
> const char *ciphername;
> bool enable_ncp_fallback; /**< If defined fall back to
> * ciphername if NCP fails */
> - bool ncp_enabled;
> const char *ncp_ciphers;
> const char *authname;
> const char *prng_hash;
> diff --git a/src/openvpn/ssl.c b/src/openvpn/ssl.c
> index b28d8edf8..dd6e462d0 100644
> --- a/src/openvpn/ssl.c
> +++ b/src/openvpn/ssl.c
> @@ -2187,8 +2187,7 @@ push_peer_info(struct buffer *buf, struct tls_session
> *session)
> }
>
> /* support for Negotiable Crypto Parameters */
> - if (session->opt->ncp_enabled
> - && (session->opt->mode == MODE_SERVER || session->opt->pull))
> + if (session->opt->mode == MODE_SERVER || session->opt->pull)
> {
> if (tls_item_in_cipher_list("AES-128-GCM",
> session->opt->config_ncp_ciphers)
> && tls_item_in_cipher_list("AES-256-GCM",
> session->opt->config_ncp_ciphers))
> diff --git a/src/openvpn/ssl_common.h b/src/openvpn/ssl_common.h
> index 928e80929..43af51ee9 100644
> --- a/src/openvpn/ssl_common.h
> +++ b/src/openvpn/ssl_common.h
> @@ -324,7 +324,6 @@ struct tls_options
>
> const char *config_ciphername;
> const char *config_ncp_ciphers;
> - bool ncp_enabled;
>
> bool tls_crypt_v2;
> const char *tls_crypt_v2_verify_script;
> diff --git a/src/openvpn/ssl_ncp.c b/src/openvpn/ssl_ncp.c
> index f02a3103c..722256b42 100644
> --- a/src/openvpn/ssl_ncp.c
> +++ b/src/openvpn/ssl_ncp.c
> @@ -289,10 +289,6 @@ check_pull_client_ncp(struct context *c, const int found)
> return true;
> }
>
> - if (!c->options.ncp_enabled)
> - {
> - return true;
> - }
> /* If the server did not push a --cipher, we will switch to the
> * remote cipher if it is in our ncp-ciphers list */
> if(tls_poor_mans_ncp(&c->options, c->c2.tls_multi->remote_ciphername))
>
--
Antonio Quartulli
_______________________________________________
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel
