[Openvpn-devel,v2,8/9] Remove --ncp-disable option

Message ID 20210520151148.2565578-8-arne@rfc2549.org
State Accepted
Delegated to: Antonio Quartulli
Headers show
Series
  • [Openvpn-devel,v2,1/9] Move auth_token_state from multi to key_state
Related show

Commit Message

Arne Schwabe May 20, 2021, 3:11 p.m.
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 <arne@rfc2549.org>
---
 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(-)

Comments

Antonio Quartulli July 19, 2021, 11:57 p.m. | #1
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 <arne@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 <antonio@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))
>
Gert Doering Aug. 1, 2021, 4:52 p.m. | #2
Stared at the code for a while... this all looks very reasonable,
except for removal of that warning

-        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.");

.. which I find a bit surprising, since it was just recently added
to help users figure out why their existing configs "suddenly fail".

Discussed this on IRC, warning will come back with a cleanup patch
"soonish".


Threw this at the server side test rig, which has p2p instances, which
says: "Test sets failed: 4a 9 9a".

4a is spurious (FreeBSD, TAP, IPv6), but 9 / 9a are the p2p instances
that get broken by this patch, and partially (!) repaired by the next
one.

"9" is "--client" to "--tls-server".
"9a" is "--tls-client" (no -pull) to "--tls-server".

In both cases, the server does not set up data channel keys (as far
as I could determine) and then fails with something like this:

2021-07-28 19:45:16 us=967365 TLS Error: local/remote TLS keys are out of sync: [AF_INET6]2001:608:0:814::f000:21:29749 (received key id: 0, known key ids:  [key#0 state=S_ACTIVE auth=KS_AUTH_TRUE id=0 sid=6491e324 c7d580ce] [key#1 state=S_UNDEF auth=KS_AUTH_FALSE id=0 sid=00000000 00000000] [key#2 state=S_UNDEF auth=KS_AUTH_FALSE id=0 sid=00000000 00000000])
..
2021-07-28 19:45:18 us=260931 Bad LZO decompression header byte: 161
..
2021-07-28 19:40:31 us=196824 Authenticate/Decrypt packet error: bad packet ID (may be a replay): [ #2721835462 ] -- see the man page entry for --no-replay and --replay-window for more info or silence this warning with --mute-replay-warnings
2021-07-28 19:40:31 us=196836 Fatal decryption error (process_incoming_link), restarting


Adding

  data-ciphers-fallback BF-CBC
  data-ciphers BF-CBC

to the server side config makes it work, though <<<--!!!  (basically,
disabling any server-side attempt at NCP, as "there is only one choice",
I think).

Since this is document to break p2p, and the next patch (in this series)
un-breaks this - well, breaks it in different ways - we move forward.

Your patch has been applied to the master branch.

commit caacd629f872c37c39453d3d0f2dfac229c921b1
Author: Arne Schwabe
Date:   Thu May 20 17:11:47 2021 +0200

     Remove --ncp-disable option

     Signed-off-by: Arne Schwabe <arne@rfc2549.org>
     Acked-by: Antonio Quartulli <antonio@openvpn.net>
     Message-Id: <20210520151148.2565578-8-arne@rfc2549.org>
     URL: https://www.mail-archive.com/openvpn-devel@lists.sourceforge.net/msg22418.html
     Signed-off-by: Gert Doering <gert@greenie.muc.de>


--
kind regards,

Gert Doering

Patch

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))