| Message ID | 20210408140229.31824-3-arne@rfc2549.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | P2P NCP support patch set | expand |
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))
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
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
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
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 -> mtu = 1431 <br> openvpn 2.4.6 -> 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 -> mtu = 1431 <br> openvpn 2.4.6 -> 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 <--- 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>
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,
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
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
>>> 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
-----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-----
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
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,
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))
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(-)