[Openvpn-devel,2/3] Remove --ncp-disable option

Message ID 20210408140229.31824-3-arne@rfc2549.org
State Superseded
Headers show
Series
  • P2P NCP support patch set
Related show

Commit Message

Arne Schwabe April 8, 2021, 2:02 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

Jan Just Keijser April 8, 2021, 2:36 p.m. | #1
Hi,

On 08/04/21 16:02, 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.
I would Feature-NAK this :   disabling NCP is a valuable option. IMHO.

Also, is the MTU calculation part on the server side now done 
correctly?    I remember that with 2.4 the server would subtract (or 
add, depending on point of iew) far too many bytes to handle NCP

JJK

> 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(-)
>
> diff --git a/Changes.rst b/Changes.rst
> index 457dfc07e..d6ccc1c92 100644
> --- a/Changes.rst
> +++ b/Changes.rst
> @@ -47,6 +47,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 4b6928c68..fe8ca8fd1 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
> @@ -230,10 +230,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 28d183aa0..4a6b84914 100644
> --- a/src/openvpn/init.c
> +++ b/src/openvpn/init.c
> @@ -2226,18 +2226,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;
>   }
>   
> @@ -2734,8 +2730,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:
> @@ -2745,12 +2739,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);
>           }
> @@ -2847,7 +2841,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 e6eb34bfb..1dd7c8983 100644
> --- a/src/openvpn/multi.c
> +++ b/src/openvpn/multi.c
> @@ -1790,10 +1790,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 24d722fd5..e2759a1ac 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";
> @@ -1715,7 +1713,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);
> @@ -3061,7 +3058,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");
>   
> @@ -3078,18 +3074,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";
> @@ -3131,13 +3115,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)
> @@ -3879,8 +3860,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);
>           }
> @@ -7957,14 +7937,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 b80cd3d1b..17f21103d 100644
> --- a/src/openvpn/options.h
> +++ b/src/openvpn/options.h
> @@ -506,7 +506,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 5d65c3da5..068b66616 100644
> --- a/src/openvpn/ssl.c
> +++ b/src/openvpn/ssl.c
> @@ -2149,8 +2149,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 5b24623d4..3f8dd0178 100644
> --- a/src/openvpn/ssl_common.h
> +++ b/src/openvpn/ssl_common.h
> @@ -308,7 +308,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))
Arne Schwabe April 8, 2021, 2:55 p.m. | #2
Am 08.04.21 um 16:36 schrieb Jan Just Keijser:
> Hi,
> 
> On 08/04/21 16:02, 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.
> I would Feature-NAK this :   disabling NCP is a valuable option. IMHO.

I disagree here. NCP in 2.5 is mature mature enough that disabling NCP
is not necessary any more. If you have any evidence otherwise, please
explain a bit more since I personally don't see the value in it.

> Also, is the MTU calculation part on the server side now done
> correctly?    I remember that with 2.4 the server would subtract (or
> add, depending on point of iew) far too many bytes to handle NCP


Not fully yet but the case where NCP cipher == original cipher has a
workaround.

Arne
Jan Just Keijser April 8, 2021, 3:30 p.m. | #3
On 08/04/21 16:55, Arne Schwabe wrote:
> Am 08.04.21 um 16:36 schrieb Jan Just Keijser:
>> Hi,
>>
>> On 08/04/21 16:02, 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.
>> I would Feature-NAK this :   disabling NCP is a valuable option. IMHO.
> I disagree here. NCP in 2.5 is mature mature enough that disabling NCP
> is not necessary any more. If you have any evidence otherwise, please
> explain a bit more since I personally don't see the value in it.

I don't have any evidence with 2.5 right now but this is just a matter 
of use/principle to me: I can very well see that I would like to have a 
setup *without* NCP as I simply do not need it (e.g. my cipher is 
hardwired to aes-256-gcm)  and in that case I don't *want* NCP to ensure 
my setup is 100% predictable.

Disabling this option means you give me less control over the setup and 
I don't like that, thus Feature-NAK.
>> Also, is the MTU calculation part on the server side now done
>> correctly?    I remember that with 2.4 the server would subtract (or
>> add, depending on point of iew) far too many bytes to handle NCP
>
> Not fully yet but the case where NCP cipher == original cipher has a
> workaround.
>
>
I'd say that removing the ability to disable NCP  can happen *only* when 
all negative side-effects of enabling it have been mitigated fully.  On 
a slow link the NCP overhead can be quite disastrous and not just during 
connection setup, but during the *whole* session. To me, yet another 
reason for Feature-NAK

JJK
Gert Doering April 8, 2021, 3:52 p.m. | #4
Hi,

On Thu, Apr 08, 2021 at 05:30:52PM +0200, Jan Just Keijser wrote:
> I don't have any evidence with 2.5 right now but this is just a matter 
> of use/principle to me: I can very well see that I would like to have a 
> setup *without* NCP as I simply do not need it (e.g. my cipher is 
> hardwired to aes-256-gcm)  and in that case I don't *want* NCP to ensure 
> my setup is 100% predictable.

By setting "--data-ciphers AES-256-GCM" on the client side, you achieve
a 100% predictable outcome.  No other cipher will be offered or accepted.

> Disabling this option means you give me less control over the setup and 
> I don't like that, thus Feature-NAK.

Removing that option means "less confusing code variants that can lead
to 'it works with NCP but not with NCP disabled'".

Since I test all these variants, and find the confusing corner cases, it
would be good to have less code paths throughout OpenVPN, especially 
in the server option negotiation and key setup phases.


> I'd say that removing the ability to disable NCP  can happen *only* when 
> all negative side-effects of enabling it have been mitigated fully.  On 
> a slow link the NCP overhead can be quite disastrous and not just during 
> connection setup, but during the *whole* session. To me, yet another 
> reason for Feature-NAK

The overhead of NCP is roughly "some 100 bytes sent extra from client to
server in the TLS handshake phase" (to announce the acceptable ciphers)
and "cipher xxx" in the PUSH_REPLY.

There is no overhead in the data phase.

Please explain how this can be "quite disastrous"?


(Of course, if NCP negotiates a cipher with more overhead than you'd
like to use, that would be "more overhead" - but this is fully under 
the client's control with --data-ciphers.  It even works with "none",
provided both client and server permit this)

gert
Jan Just Keijser April 9, 2021, 9:24 a.m. | #5
Hi,

On 08/04/21 17:52, Gert Doering wrote:
> Hi,
>
> On Thu, Apr 08, 2021 at 05:30:52PM +0200, Jan Just Keijser wrote:
>> I don't have any evidence with 2.5 right now but this is just a matter
>> of use/principle to me: I can very well see that I would like to have a
>> setup *without* NCP as I simply do not need it (e.g. my cipher is
>> hardwired to aes-256-gcm)  and in that case I don't *want* NCP to ensure
>> my setup is 100% predictable.
> By setting "--data-ciphers AES-256-GCM" on the client side, you achieve
> a 100% predictable outcome.  No other cipher will be offered or accepted.
Ah a new 2.5 option - I assume the same thing is true for adding this on 
the server side?

And I see that --data-ciphers works only in combination with NCP, that 
is, if you do
   --data-ciphers aes-256-gcm --ncp-disable
then --data-ciphers is effectively ignored.  Nice  ;)

>> Disabling this option means you give me less control over the setup and
>> I don't like that, thus Feature-NAK.
> Removing that option means "less confusing code variants that can lead
> to 'it works with NCP but not with NCP disabled'".
>
> Since I test all these variants, and find the confusing corner cases, it
> would be good to have less code paths throughout OpenVPN, especially
> in the server option negotiation and key setup phases.
>
>
>> I'd say that removing the ability to disable NCP  can happen *only* when
>> all negative side-effects of enabling it have been mitigated fully.  On
>> a slow link the NCP overhead can be quite disastrous and not just during
>> connection setup, but during the *whole* session. To me, yet another
>> reason for Feature-NAK
> The overhead of NCP is roughly "some 100 bytes sent extra from client to
> server in the TLS handshake phase" (to announce the acceptable ciphers)
> and "cipher xxx" in the PUSH_REPLY.
>
> There is no overhead in the data phase.
>
> Please explain how this can be "quite disastrous"?
>
>
> (Of course, if NCP negotiates a cipher with more overhead than you'd
> like to use, that would be "more overhead" - but this is fully under
> the client's control with --data-ciphers.  It even works with "none",
> provided both client and server permit this)
I was thinking about this email discussion we had in October 2018:

> On 29/10/18 18:08, Gert Doering wrote:
>> On Mon, Oct 29, 2018 at 05:40:04PM +0100, Jan Just Keijser wrote:
>>> So, the '32' is easily explained. However, the rest of the MTU
>>> calculation baffles me.
>> Part of this is "peer-id" (+3 bytes) and "the theoretical maximum
>> crypto + hmac overhead" which 2.3 calculates "for the cipher chosen"
>> and 2.4 needs to calculale for the worst-case cipher+auth, since it does
>> not know what the server will push.
>>
>> In other words, you do not want to know - and the whole "match
>> configured client/server tun-mtu/link-mtu OCC thingie" is a real 
>> nuisance.
>
> so I now understand the client MTU:
>   openvpn 2.3.18 -> mtu = 1431
>   openvpn 2.4.6 -> mtu = 1428 which accounts for peer-id (+3)
>
> but the *server* mtu?!?!?!  I would have expected that with 
> --ncp-disable added I would end up with more or less the same MTU as 
> with the 2.3 code. Instead I see
>   openvpn 2.3.18 -> mtu = 1431
>   openvpn 2.4.6 -> mtu = 1379
> which is 62 bytes LESS , so even with peer-id subtracted (does it 
> apply to the server MTU?)  I end up with 59 bytes unaccounted for *in 
> tun mode*.
>
and I was hoping that this would be resolved before removing something 
like --ncp-disable. Having said that, I now see that with openvpn 2.5, 
the server mtu is still 1379 in my setup, regardless of whether I use 
--ncp-disable or not  - seems to me that is still too low.

On the client side I now see even more confusing fun:

Server has --ncp-disable set:
- with  --cipher aes-256-cbc  the client side MTU is 1428
- with  --cipher aes-256-gcm  the client side MTU is 1376  <--- huh?

Server does not have --ncp-disable set:
- with  --cipher aes-256-cbc  the client side MTU is 1428
- with  --cipher aes-256-gcm  the client side MTU is 1448
(ok I understand the latter).

What I'd really like to see is a way to get the server-side MTU to also 
be 1428/1448 using the right magic options - any idea if that's possible?
The lower the MTU on either side, the lower the maximum bandwidth - 
which will hurt especially over low-bandwidth links.

Also, when this option is dropped, does that mean that a 2.4/2.5 client 
with the option set can no longer connect to a 2.6 server?

Conclusion: a further overhaul of the ncp/cipher/data-ciphers logic 
might be in order...
Which is one of my gut reasons for saying : don't remove --ncp-disable 
until this mess is thoroughly sorted out.


JM2CW,

JJK


=============
Client config:

client
proto udp
remote $SERVER
port 1194
dev tun
nobind
remote-cert-tls server
ca       ca.crt
cert     client1.crt
key      client1.key
cipher aes-256-cbc
auth   sha256
link-mtu 1500
fragment 0
mssfix 0

=============
Server config:

proto udp
port 1194
dev tun
server 10.200.0.0 255.255.255.0
dh       dh2048.pem
ca       ca.crt
cert     server.crt
key      server.key
persist-key
persist-tun
keepalive 10 60
topology subnet
cipher aes-256-cbc
auth   sha256
link-mtu 1500
fragment 0
mssfix 0
<html>
  <head>
    <meta http-equiv="Content-Type" content="text/html;
      charset=windows-1252">
  </head>
  <body text="#000000" bgcolor="#FFFFFF">
    <div class="moz-cite-prefix">Hi,<br>
      <br>
      On 08/04/21 17:52, Gert Doering wrote:<br>
    </div>
    <blockquote type="cite"
      cite="mid:20210408155251.GX976@greenie.muc.de">
      <pre wrap="">Hi,

On Thu, Apr 08, 2021 at 05:30:52PM +0200, Jan Just Keijser wrote:
</pre>
      <blockquote type="cite">
        <pre wrap="">I don't have any evidence with 2.5 right now but this is just a matter 
of use/principle to me: I can very well see that I would like to have a 
setup *without* NCP as I simply do not need it (e.g. my cipher is 
hardwired to aes-256-gcm)  and in that case I don't *want* NCP to ensure 
my setup is 100% predictable.
</pre>
      </blockquote>
      <pre wrap="">
By setting "--data-ciphers AES-256-GCM" on the client side, you achieve
a 100% predictable outcome.  No other cipher will be offered or accepted.
</pre>
    </blockquote>
    Ah a new 2.5 option - I assume the same thing is true for adding
    this on the server side?<br>
    <br>
    And I see that --data-ciphers works only in combination with NCP,
    that is, if you do<br>
      --data-ciphers aes-256-gcm --ncp-disable<br>
    then --data-ciphers is effectively ignored.  Nice  ;)<br>
    <br>
    <blockquote type="cite"
      cite="mid:20210408155251.GX976@greenie.muc.de">
      <blockquote type="cite">
        <pre wrap="">Disabling this option means you give me less control over the setup and 
I don't like that, thus Feature-NAK.
</pre>
      </blockquote>
      <pre wrap="">
Removing that option means "less confusing code variants that can lead
to 'it works with NCP but not with NCP disabled'".

Since I test all these variants, and find the confusing corner cases, it
would be good to have less code paths throughout OpenVPN, especially 
in the server option negotiation and key setup phases.


</pre>
      <blockquote type="cite">
        <pre wrap="">I'd say that removing the ability to disable NCP  can happen *only* when 
all negative side-effects of enabling it have been mitigated fully.  On 
a slow link the NCP overhead can be quite disastrous and not just during 
connection setup, but during the *whole* session. To me, yet another 
reason for Feature-NAK
</pre>
      </blockquote>
      <pre wrap="">
The overhead of NCP is roughly "some 100 bytes sent extra from client to
server in the TLS handshake phase" (to announce the acceptable ciphers)
and "cipher xxx" in the PUSH_REPLY.

There is no overhead in the data phase.

Please explain how this can be "quite disastrous"?


(Of course, if NCP negotiates a cipher with more overhead than you'd
like to use, that would be "more overhead" - but this is fully under 
the client's control with --data-ciphers.  It even works with "none",
provided both client and server permit this)
</pre>
    </blockquote>
    I was thinking about this email discussion we had in October 2018:<br>
    <br>
    <blockquote type="cite">On 29/10/18 18:08, Gert Doering wrote:
      <br>
      <blockquote type="cite" style="color: #000000;">On Mon, Oct 29,
        2018 at 05:40:04PM +0100, Jan Just Keijser wrote:
        <br>
        <blockquote type="cite" style="color: #000000;">So, the '32' is
          easily explained. However, the rest of the MTU
          <br>
          calculation baffles me.
          <br>
        </blockquote>
        Part of this is "peer-id" (+3 bytes) and "the theoretical
        maximum
        <br>
        crypto + hmac overhead" which 2.3 calculates "for the cipher
        chosen"
        <br>
        and 2.4 needs to calculale for the worst-case cipher+auth, since
        it does
        <br>
        not know what the server will push.
        <br>
        <br>
        In other words, you do not want to know <span
          class="moz-smiley-s1" title=":-)"></span> - and the whole
        "match
        <br>
        configured client/server tun-mtu/link-mtu OCC thingie" is a real
        nuisance.
        <br>
      </blockquote>
      <br>
      so I now understand the client MTU:
      <br>
        openvpn 2.3.18 -&gt; mtu = 1431
      <br>
        openvpn 2.4.6 -&gt; mtu = 1428 which accounts for peer-id (+3)
      <br>
      <br>
      but the <b class="moz-txt-star"><span class="moz-txt-tag">*</span>server<span
          class="moz-txt-tag">*</span></b> mtu?!?!?!  I would have
      expected that with --ncp-disable added I would end up with more or
      less the same MTU as with the 2.3 code. Instead I see
      <br>
        openvpn 2.3.18 -&gt; mtu = 1431
      <br>
        openvpn 2.4.6 -&gt; mtu = 1379
      <br>
      which is 62 bytes LESS , so even with peer-id subtracted (does it
      apply to the server MTU?)  I end up with 59 bytes unaccounted for
      <b class="moz-txt-star"><span class="moz-txt-tag">*</span>in tun
        mode<span class="moz-txt-tag">*</span></b>.
      <br>
      <br>
    </blockquote>
    and I was hoping that this would be resolved before removing
    something like --ncp-disable. Having said that, I now see that with
    openvpn 2.5, the server mtu is still 1379 in my setup, regardless of
    whether I use --ncp-disable or not  - seems to me that is still too
    low.<br>
    <br>
    On the client side I now see even more confusing fun:<br>
    <br>
    Server has --ncp-disable set:<br>
    - with  --cipher aes-256-cbc  the client side MTU is 1428<br>
    - with  --cipher aes-256-gcm  the client side MTU is 1376  &lt;---
    huh?<br>
    <br>
    Server does not have --ncp-disable set:<br>
    - with  --cipher aes-256-cbc  the client side MTU is 1428<br>
    - with  --cipher aes-256-gcm  the client side MTU is 1448<br>
    (ok I understand the latter).<br>
    <br>
    What I'd really like to see is a way to get the server-side MTU to
    also be 1428/1448 using the right magic options - any idea if that's
    possible?<br>
    The lower the MTU on either side, the lower the maximum bandwidth -
    which will hurt especially over low-bandwidth links.<br>
    <br>
    Also, when this option is dropped, does that mean that a 2.4/2.5
    client with the option set can no longer connect to a 2.6 server? <br>
    <br>
    Conclusion: a further overhaul of the ncp/cipher/data-ciphers logic
    might be in order...<br>
    Which is one of my gut reasons for saying : don't remove
    --ncp-disable until this mess is thoroughly sorted out.<br>
    <br>
    <br>
    JM2CW,<br>
    <br>
    JJK<br>
    <br>
    <br>
    =============<br>
    Client config:<br>
    <br>
    client<br>
    proto udp<br>
    remote $SERVER<br>
    port 1194<br>
    dev tun<br>
    nobind<br>
    remote-cert-tls server<br>
    ca       ca.crt<br>
    cert     client1.crt<br>
    key      client1.key<br>
    cipher aes-256-cbc<br>
    auth   sha256<br>
    link-mtu 1500<br>
    fragment 0<br>
    mssfix 0<br>
    <br>
    =============<br>
    Server config:<br>
    <br>
    proto udp<br>
    port 1194<br>
    dev tun<br>
    server 10.200.0.0 255.255.255.0<br>
    dh       dh2048.pem<br>
    ca       ca.crt<br>
    cert     server.crt<br>
    key      server.key<br>
    persist-key<br>
    persist-tun<br>
    keepalive 10 60<br>
    topology subnet<br>
    cipher aes-256-cbc<br>
    auth   sha256<br>
    link-mtu 1500<br>
    fragment 0<br>
    mssfix 0<br>
    <br>
    <br>
    <br>
  </body>
</html>
Antonio Quartulli April 9, 2021, 9:52 a.m. | #6
Hi Jan Just,

On 09/04/2021 11:24, Jan Just Keijser wrote:
> Hi,
> 
> On 08/04/21 17:52, Gert Doering wrote:
>> Hi,
>>
>> On Thu, Apr 08, 2021 at 05:30:52PM +0200, Jan Just Keijser wrote:
>>> I don't have any evidence with 2.5 right now but this is just a matter 
>>> of use/principle to me: I can very well see that I would like to have a 
>>> setup *without* NCP as I simply do not need it (e.g. my cipher is 
>>> hardwired to aes-256-gcm)  and in that case I don't *want* NCP to ensure 
>>> my setup is 100% predictable.
>> By setting "--data-ciphers AES-256-GCM" on the client side, you achieve
>> a 100% predictable outcome.  No other cipher will be offered or accepted.
> Ah a new 2.5 option - I assume the same thing is true for adding this on
> the server side?
> 
> And I see that --data-ciphers works only in combination with NCP, that
> is, if you do
>   --data-ciphers aes-256-gcm --ncp-disable
> then --data-ciphers is effectively ignored.  Nice  ;)

Yeah, so the NCP behaviour is now customizable enough to give you total
control over what should happen.

> 
>>> Disabling this option means you give me less control over the setup and 
>>> I don't like that, thus Feature-NAK.
>> Removing that option means "less confusing code variants that can lead
>> to 'it works with NCP but not with NCP disabled'".
>>
>> Since I test all these variants, and find the confusing corner cases, it
>> would be good to have less code paths throughout OpenVPN, especially 
>> in the server option negotiation and key setup phases.
>>
>>
>>> I'd say that removing the ability to disable NCP  can happen *only* when 
>>> all negative side-effects of enabling it have been mitigated fully.  On 
>>> a slow link the NCP overhead can be quite disastrous and not just during 
>>> connection setup, but during the *whole* session. To me, yet another 
>>> reason for Feature-NAK
>> The overhead of NCP is roughly "some 100 bytes sent extra from client to
>> server in the TLS handshake phase" (to announce the acceptable ciphers)
>> and "cipher xxx" in the PUSH_REPLY.
>>
>> There is no overhead in the data phase.
>>
>> Please explain how this can be "quite disastrous"?
>>
>>
>> (Of course, if NCP negotiates a cipher with more overhead than you'd
>> like to use, that would be "more overhead" - but this is fully under 
>> the client's control with --data-ciphers.  It even works with "none",
>> provided both client and server permit this)
> I was thinking about this email discussion we had in October 2018:
> 
>> On 29/10/18 18:08, Gert Doering wrote:
>>> On Mon, Oct 29, 2018 at 05:40:04PM +0100, Jan Just Keijser wrote:
>>>> So, the '32' is easily explained. However, the rest of the MTU
>>>> calculation baffles me.
>>> Part of this is "peer-id" (+3 bytes) and "the theoretical maximum
>>> crypto + hmac overhead" which 2.3 calculates "for the cipher chosen"
>>> and 2.4 needs to calculale for the worst-case cipher+auth, since it does
>>> not know what the server will push.
>>>
>>> In other words, you do not want to know - and the whole "match
>>> configured client/server tun-mtu/link-mtu OCC thingie" is a real
>>> nuisance.
>>
>> so I now understand the client MTU:
>>   openvpn 2.3.18 -> mtu = 1431
>>   openvpn 2.4.6 -> mtu = 1428 which accounts for peer-id (+3)
>>
>> but the *server* mtu?!?!?!  I would have expected that with
>> --ncp-disable added I would end up with more or less the same MTU as
>> with the 2.3 code. Instead I see
>>   openvpn 2.3.18 -> mtu = 1431
>>   openvpn 2.4.6 -> mtu = 1379
>> which is 62 bytes LESS , so even with peer-id subtracted (does it
>> apply to the server MTU?)  I end up with 59 bytes unaccounted for *in
>> tun mode*.
>>
> and I was hoping that this would be resolved before removing something
> like --ncp-disable. Having said that, I now see that with openvpn 2.5,
> the server mtu is still 1379 in my setup, regardless of whether I use
> --ncp-disable or not  - seems to me that is still too low.
> 

If I understand your point correctly, you basically confirmed that this
"MTU issue" you are seeing is unrelated to having --ncp-disable or not.

Therefore I think this topic should not be an argument for *not*
removing --ncp-disable.

We have been busy discussing the "MTU mess" in the past weeks, so this
is also something that we are all committed to solve.
However, for the sake of this discussion, I'd keep this problem
separated (worth discussing in another thread, of course!).

> On the client side I now see even more confusing fun:
> 
> Server has --ncp-disable set:
> - with  --cipher aes-256-cbc  the client side MTU is 1428
> - with  --cipher aes-256-gcm  the client side MTU is 1376  <--- huh?
> 
> Server does not have --ncp-disable set:
> - with  --cipher aes-256-cbc  the client side MTU is 1428
> - with  --cipher aes-256-gcm  the client side MTU is 1448
> (ok I understand the latter).
> 
> What I'd really like to see is a way to get the server-side MTU to also
> be 1428/1448 using the right magic options - any idea if that's possible?
> The lower the MTU on either side, the lower the maximum bandwidth -
> which will hurt especially over low-bandwidth links.
> 
> Also, when this option is dropped, does that mean that a 2.4/2.5 client
> with the option set can no longer connect to a 2.6 server?

No. *All* clients will still be able to connect.

The NCP logic has a case for "the client is not NCP-capable".

In this case the server checks OCC string to see what cipher the client
is going to use. If this cipher is part of the data-ciphers list on the
server, then that cipher will be used.
If there is no OCC string, I think the server will use the configured
fallback-cipher.

So compatibility is preserved.

As you can see NCP is much more comprehensive that what it was before,
hence the request to drop --ncp-disable.

> 
> Conclusion: a further overhaul of the ncp/cipher/data-ciphers logic
> might be in order...

This is exactly what has been going on, I'd say, with the addition of
the new options and new logic to keep compatibility with non-NCP clients.

> Which is one of my gut reasons for saying : don't remove --ncp-disable
> until this mess is thoroughly sorted out.

I hope this patch makes more sense now.

Regards,
Arne Schwabe April 9, 2021, 9:53 a.m. | #7
Am 09.04.21 um 11:24 schrieb Jan Just Keijser:
> Hi,
> 
> On 08/04/21 17:52, Gert Doering wrote:
>> Hi,
>>
>> On Thu, Apr 08, 2021 at 05:30:52PM +0200, Jan Just Keijser wrote:
>>> I don't have any evidence with 2.5 right now but this is just a matter 
>>> of use/principle to me: I can very well see that I would like to have a 
>>> setup *without* NCP as I simply do not need it (e.g. my cipher is 
>>> hardwired to aes-256-gcm)  and in that case I don't *want* NCP to ensure 
>>> my setup is 100% predictable.
>> By setting "--data-ciphers AES-256-GCM" on the client side, you achieve
>> a 100% predictable outcome.  No other cipher will be offered or accepted.
> Ah a new 2.5 option - I assume the same thing is true for adding this on
> the server side?
> 
> And I see that --data-ciphers works only in combination with NCP, that
> is, if you do
>   --data-ciphers aes-256-gcm --ncp-disable
> then --data-ciphers is effectively ignored.  Nice  ;)

Yes data-ciphers (and data-ciphers-fallback) are meant to replace all
other cipher related options eventually.



>>
> and I was hoping that this would be resolved before removing something
> like --ncp-disable. Having said that, I now see that with openvpn 2.5,
> the server mtu is still 1379 in my setup, regardless of whether I use
> --ncp-disable or not  - seems to me that is still too low.
> 


Yes DATA_V2 adds 3 bytes compared to DATA_V1.

> On the client side I now see even more confusing fun:
> 
> Server has --ncp-disable set:
> - with  --cipher aes-256-cbc  the client side MTU is 1428
> - with  --cipher aes-256-gcm  the client side MTU is 1376  <--- huh?
> 
> Server does not have --ncp-disable set:
> - with  --cipher aes-256-cbc  the client side MTU is 1428
> - with  --cipher aes-256-gcm  the client side MTU is 1448
> (ok I understand the latter).
> 
> What I'd really like to see is a way to get the server-side MTU to also
> be 1428/1448 using the right magic options - any idea if that's possible?
> The lower the MTU on either side, the lower the maximum bandwidth -
> which will hurt especially over low-bandwidth links.
> 
> Also, when this option is dropped, does that mean that a 2.4/2.5 client
> with the option set can no longer connect to a 2.6 server?

I am not sure how you came to that conclusion. I have written a fairly
comprehensible documentation how NCP in 2.5 works for our manpage:
https://github.com/OpenVPN/openvpn/blob/master/doc/man-sections/cipher-negotiation.rst

That should also answer your question.

> Conclusion: a further overhaul of the ncp/cipher/data-ciphers logic
> might be in order...
> Which is one of my gut reasons for saying : don't remove --ncp-disable
> until this mess is thoroughly sorted out.

Please don't mix two things here. Yes the MTU calculation and MTU in
general is broken but shouldn't pollute this discussion here.

And yes we will look into MTU related issues before 2.6 is released but
having a non NCP and a NCP code path for MTU is not helping to reduce
the complexity.

Arne
Jan Just Keijser April 9, 2021, 10:19 a.m. | #8
Hi Arne, Antonio,

On 09/04/21 11:53, Arne Schwabe wrote:
> Am 09.04.21 um 11:24 schrieb Jan Just Keijser:
>> On 08/04/21 17:52, Gert Doering wrote:
>>> On Thu, Apr 08, 2021 at 05:30:52PM +0200, Jan Just Keijser wrote:
>>>> I don't have any evidence with 2.5 right now but this is just a matter
>>>> of use/principle to me: I can very well see that I would like to have a
>>>> setup *without* NCP as I simply do not need it (e.g. my cipher is
>>>> hardwired to aes-256-gcm)  and in that case I don't *want* NCP to ensure
>>>> my setup is 100% predictable.
>>> By setting "--data-ciphers AES-256-GCM" on the client side, you achieve
>>> a 100% predictable outcome.  No other cipher will be offered or accepted.
>> Ah a new 2.5 option - I assume the same thing is true for adding this on
>> the server side?
>>
>> And I see that --data-ciphers works only in combination with NCP, that
>> is, if you do
>>    --data-ciphers aes-256-gcm --ncp-disable
>> then --data-ciphers is effectively ignored.  Nice  ;)
> Yes data-ciphers (and data-ciphers-fallback) are meant to replace all
> other cipher related options eventually.
>
>
>
>> and I was hoping that this would be resolved before removing something
>> like --ncp-disable. Having said that, I now see that with openvpn 2.5,
>> the server mtu is still 1379 in my setup, regardless of whether I use
>> --ncp-disable or not  - seems to me that is still too low.
>>
>
> Yes DATA_V2 adds 3 bytes compared to DATA_V1.
I understand the 3 bytes (--peerid and such) but is there any way to NOT 
have such a low server-side MTU?
Also note that having a different server-side MTU versus client-side MTU 
makes it even harder to tune the performance
>> On the client side I now see even more confusing fun:
>>
>> Server has --ncp-disable set:
>> - with  --cipher aes-256-cbc  the client side MTU is 1428
>> - with  --cipher aes-256-gcm  the client side MTU is 1376  <--- huh?
>>
>> Server does not have --ncp-disable set:
>> - with  --cipher aes-256-cbc  the client side MTU is 1428
>> - with  --cipher aes-256-gcm  the client side MTU is 1448
>> (ok I understand the latter).
>>
>> What I'd really like to see is a way to get the server-side MTU to also
>> be 1428/1448 using the right magic options - any idea if that's possible?
>> The lower the MTU on either side, the lower the maximum bandwidth -
>> which will hurt especially over low-bandwidth links.
>>
>> Also, when this option is dropped, does that mean that a 2.4/2.5 client
>> with the option set can no longer connect to a 2.6 server?
> I am not sure how you came to that conclusion. I have written a fairly
> comprehensible documentation how NCP in 2.5 works for our manpage:
> https://github.com/OpenVPN/openvpn/blob/master/doc/man-sections/cipher-negotiation.rst
>
> That should also answer your question.
>
>> Conclusion: a further overhaul of the ncp/cipher/data-ciphers logic
>> might be in order...
>> Which is one of my gut reasons for saying : don't remove --ncp-disable
>> until this mess is thoroughly sorted out.
> Please don't mix two things here. Yes the MTU calculation and MTU in
> general is broken but shouldn't pollute this discussion here.
>
> And yes we will look into MTU related issues before 2.6 is released but
> having a non NCP and a NCP code path for MTU is not helping to reduce
> the complexity.
>
>
thanks for all the clarifications - esp the cipher-negotiation.rst was 
very helpful.
And I understand the argument about "polluting the discussion" but to me 
the *order* in which things are done is important. That is, for the 2.6 
release all MTU issues should be resolved , so that noone will ever have 
a need to debug MTU issues by dis- or enabling NCP.
So I guess if things were done in the order
   1) fix the MTU issues on both client and server side
   2) drop --ncp-disable
then you would not have heard any complaints from me.

cheers,

JJK
Arne Schwabe April 9, 2021, 10:54 a.m. | #9
>>> and I was hoping that this would be resolved before removing something
>>> like --ncp-disable. Having said that, I now see that with openvpn 2.5,
>>> the server mtu is still 1379 in my setup, regardless of whether I use
>>> --ncp-disable or not  - seems to me that is still too low.
>>>
>>
>> Yes DATA_V2 adds 3 bytes compared to DATA_V1.
> I understand the 3 bytes (--peerid and such) but is there any way to NOT
> have such a low server-side MTU?
> Also note that having a different server-side MTU versus client-side MTU
> makes it even harder to tune the performance


Basically no. The Data V1 vs Data V2 discussion is basically done and V2
is here to stay. If 3 bytes really are too much (and we have a very
strong use case to save those bytes), having optionally reduce the auth
tag in AES-GCM/CachaPoly to something like 128 bit (16 bytes) or 160 bit
instead the current 256 bit (32 bytes) makes probably more sense. E.g.
Wireguard only uses 16 bytes auth tag and IPSec allows 16/12/8 bytes
according to RFC4106.

I don't think there is any real different client vs server mtu here
apart from potential bugs in the MTU code but not any fundamental

> thanks for all the clarifications - esp the cipher-negotiation.rst was
> very helpful.
> And I understand the argument about "polluting the discussion" but to me
> the *order* in which things are done is important. That is, for the 2.6
> release all MTU issues should be resolved , so that noone will ever have
> a need to debug MTU issues by dis- or enabling NCP.
> So I guess if things were done in the order
>   1) fix the MTU issues on both client and server side
>   2) drop --ncp-disable
> then you would not have heard any complaints from me.

That is an easy thing to say if you don't do the development.

But no, it doesn't work this way this time. I am simply not abanding my
30-40 outstanding patches now (that include also DCO), stop working on
them when they are nearly finished, spend all my time on MTU and then
later come back to the NCP/DCO/TLS session refactoring.

And I also don't want to invest time fixing MTU issues related to
disabled MTU if I remove that feature later. That is just wasted time.

And as Antonio said, we are committed to fixing these issues. We do not
plan to release a 2.6 version with broken MTU.

Arne
tincantech via Openvpn-devel April 9, 2021, 12:12 p.m. | #10
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA256

Hi,

‐‐‐‐‐‐‐ Original Message ‐‐‐‐‐‐‐
On Friday, 9 April 2021 10:53, Arne Schwabe <arne@rfc2549.org> wrote:

<snip>

>
> I am not sure how you came to that conclusion. I have written a fairly
> comprehensible documentation how NCP in 2.5 works for our manpage:
> https://github.com/OpenVPN/openvpn/blob/master/doc/man-sections/cipher-negotiation.rst
>
> That should also answer your question.
>


sorry for the noise but I created a quick ref. guide for cipher negotiation:

https://community.openvpn.net/openvpn/wiki/CipherNegotiation

It may be of some value to others reading this thread.

Regards
R

-----BEGIN PGP SIGNATURE-----
Version: ProtonMail

wsBzBAEBCAAGBQJgcES+ACEJEE+XnPZrkLidFiEECbw9RGejjXJ5xVVVT5ec
9muQuJ2TPAgAuSyk329uuzmDecw9kvFBa/UDQ2C8U4ZVwXZZKXk4AL5NtM9Q
Nbsi6qHMPT/WYfgVMOPbJLvWgUx2yi51rPawis5itd4Ghe7nZtBQOdjz1LZY
/5VfqgOIMtfvovL+Wlg1SpwPM5Mo/ApILcec4jfrP5XJxe/6Xo8Mx4ZcYLq7
EmjVZ3gFWSX3kmBTdtQmPRKZ6qTe3gezwduZ667eRy58kK39SRFX2tsjvFT+
2D8mtkLIQvJNDbO1KHNmW4oXxcu7YesQScAshOBpIutyU0vUyg37fp+SoTcs
Q4oS2Wp2T2HZlPMkvBopbiddk6Wu1+kaP0+jDiBllSkZRrcrwCRtsQ==
=8Qly
-----END PGP SIGNATURE-----
Gert Doering April 9, 2021, 3:19 p.m. | #11
Hi,

On Fri, Apr 09, 2021 at 11:24:01AM +0200, Jan Just Keijser wrote:
> On 08/04/21 17:52, Gert Doering wrote:
> > On Thu, Apr 08, 2021 at 05:30:52PM +0200, Jan Just Keijser wrote:
> >> I don't have any evidence with 2.5 right now but this is just a matter
> >> of use/principle to me: I can very well see that I would like to have a
> >> setup *without* NCP as I simply do not need it (e.g. my cipher is
> >> hardwired to aes-256-gcm)  and in that case I don't *want* NCP to ensure
> >> my setup is 100% predictable.
> > By setting "--data-ciphers AES-256-GCM" on the client side, you achieve
> > a 100% predictable outcome.  No other cipher will be offered or accepted.
> Ah a new 2.5 option - I assume the same thing is true for adding this on 
> the server side?
> 
> And I see that --data-ciphers works only in combination with NCP, that 
> is, if you do
>    --data-ciphers aes-256-gcm --ncp-disable
> then --data-ciphers is effectively ignored.  Nice  ;)

--data-ciphers is the option to tell the client what to announce as acceptable
ciphers to the server, and what to accept if pushed (in case a server is
not well-behaved).  

This is part of NCP, and if you turn off NCP, it is no longer active.

And because this confuses people, the idea is, for 2.6, to remove that 
knob with changes the flow "config -> operational session" in ways that
most people are not aware.


[..]
> On the client side I now see even more confusing fun:
> 
> Server has --ncp-disable set:
> - with  --cipher aes-256-cbc  the client side MTU is 1428
> - with  --cipher aes-256-gcm  the client side MTU is 1376  <--- huh?
> 
> Server does not have --ncp-disable set:
> - with  --cipher aes-256-cbc  the client side MTU is 1428
> - with  --cipher aes-256-gcm  the client side MTU is 1448
> (ok I understand the latter).

The MTU/frame overhead calculation, depending on configured and negotiated
ciphers, is a big source of pain (just grep the source for "frame") - and
yes, we are aware of it, but it's actually close to impossible to fix
without really ripping it all out and doing it anew.

Arne is currently busy with ripping out all the renegotiation / half-
authenticated keys / session stuff, and I understand that MTU is not 
far down his list.

My server test rig has a few cases where "large ping" fails due to exactly
this, MTU miscalculations.  Which is good, because it reminds me every
day that this needs fixing.

> What I'd really like to see is a way to get the server-side MTU to also 
> be 1428/1448 using the right magic options - any idea if that's possible?
> The lower the MTU on either side, the lower the maximum bandwidth - 
> which will hurt especially over low-bandwidth links.

Just set --tun-mtu 1448, done.

The "automatic" way is "take link-mtu, default 1500, and calculate tun-mtu
from it depending on cipher/auth overhead".  Which fails more often than
not, but --tun-mtu can also be set directly.

> Also, when this option is dropped, does that mean that a 2.4/2.5 client 
> with the option set can no longer connect to a 2.6 server?

No.  My test bed talks happily to 2.2 and 2.3 clients, some instances
with --ncp-disable, most with NCP enabled (= as if there were no option
to disable NCP).

You don't need that option today to make them talk, so they won't stop
talking if you take the option away.


> Conclusion: a further overhaul of the ncp/cipher/data-ciphers logic 
> might be in order...
> Which is one of my gut reasons for saying : don't remove --ncp-disable 
> until this mess is thoroughly sorted out.

ncp/cipher/data-cipher is fine, and well-documented.

frame/mtu is where the pain is.

gert
Antonio Quartulli April 21, 2021, 11:38 p.m. | #12
Hi,

On 08/04/2021 16:02, 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>

The patch looks good to me and passes my basic connectivity tests.
Also I got no compile-time complain from any platform I could build on.

However, there is a comment that still talk about NCP being
disabled/enabled. I think it should be changed too as we don't have the
two cases anymore (the if-condition below is modified by this patch):

src/openvpn/init.c:2726:        * as NCP-fallback or when NCP has been
disabled or explicitly


Cheers,

Patch

diff --git a/Changes.rst b/Changes.rst
index 457dfc07e..d6ccc1c92 100644
--- a/Changes.rst
+++ b/Changes.rst
@@ -47,6 +47,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 4b6928c68..fe8ca8fd1 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
@@ -230,10 +230,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 28d183aa0..4a6b84914 100644
--- a/src/openvpn/init.c
+++ b/src/openvpn/init.c
@@ -2226,18 +2226,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;
 }
 
@@ -2734,8 +2730,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:
@@ -2745,12 +2739,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);
         }
@@ -2847,7 +2841,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 e6eb34bfb..1dd7c8983 100644
--- a/src/openvpn/multi.c
+++ b/src/openvpn/multi.c
@@ -1790,10 +1790,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 24d722fd5..e2759a1ac 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";
@@ -1715,7 +1713,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);
@@ -3061,7 +3058,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");
 
@@ -3078,18 +3074,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";
@@ -3131,13 +3115,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)
@@ -3879,8 +3860,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);
         }
@@ -7957,14 +7937,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 b80cd3d1b..17f21103d 100644
--- a/src/openvpn/options.h
+++ b/src/openvpn/options.h
@@ -506,7 +506,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 5d65c3da5..068b66616 100644
--- a/src/openvpn/ssl.c
+++ b/src/openvpn/ssl.c
@@ -2149,8 +2149,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 5b24623d4..3f8dd0178 100644
--- a/src/openvpn/ssl_common.h
+++ b/src/openvpn/ssl_common.h
@@ -308,7 +308,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))