| Message ID | 20200707121615.15736-4-arne@rfc2549.org |
|---|---|
| State | Rejected |
| Headers | show |
| Series | [Openvpn-devel] Add file to ignore reformatting changes | expand |
Hi, As discusses in #openvpn-devel on IRC, this patch breaks interop with clients that don't pull, but that will be restored in a follow-up refactoring (before 2.5 rc1). I can live with that, but I think this should be mentioned in the commit message. On 07-07-2020 14:16, Arne Schwabe wrote: > Ever since the NCPv2 the ncp_get_best_cipher uses the global > options->ncp_enabled option and ignore the tls_session->ncp_enabled > option. > > The server side's poor man's NCP is implemented as seeing the list > of supported ciphers from the peer as just one cipher so this special > handling for poor man's NCP of the older NCP here is not needed anymore. > > Theoretically we can now get rid of tls_session->ncp_enabled but doing > so requires more refactoring since options is not available in the > methods that still use it. And when we remove ncp-disable the variable > will be removed anyway. > > Also document the remaining usage of tls_poor_mans_ncp better. > > Signed-off-by: Arne Schwabe <arne@rfc2549.org> > --- > src/openvpn/init.c | 2 ++ > src/openvpn/ssl.c | 15 +-------------- > 2 files changed, 3 insertions(+), 14 deletions(-) > > diff --git a/src/openvpn/init.c b/src/openvpn/init.c > index 91b919d5..e9c01629 100644 > --- a/src/openvpn/init.c > +++ b/src/openvpn/init.c > @@ -2376,6 +2376,8 @@ do_deferred_options(struct context *c, const unsigned int found) > } > else if (c->options.ncp_enabled) > { > + /* If the server did not push a --cipher, we will switch to the > + * remote cipher if it is in our ncp-ciphers list */ > tls_poor_mans_ncp(&c->options, c->c2.tls_multi->remote_ciphername); > } > struct frame *frame_fragment = NULL; > diff --git a/src/openvpn/ssl.c b/src/openvpn/ssl.c > index 9df7552d..71565dd3 100644 > --- a/src/openvpn/ssl.c > +++ b/src/openvpn/ssl.c > @@ -2463,8 +2463,7 @@ key_method_2_write(struct buffer *buf, struct tls_session *session) > * generation is postponed until after the pull/push, so we can process pushed > * cipher directives. > */ > - if (session->opt->server && !(session->opt->ncp_enabled > - && session->opt->mode == MODE_SERVER && ks->key_id <= 0)) > + if (session->opt->server && !(session->opt->mode == MODE_SERVER && ks->key_id <= 0)) > { > if (ks->authenticated != KS_AUTH_FALSE) > { > @@ -2616,18 +2615,6 @@ key_method_2_read(struct buffer *buf, struct tls_multi *multi, struct tls_sessio > multi->remote_ciphername = > options_string_extract_option(options, "cipher", NULL); > > - if (!tls_peer_supports_ncp(multi->peer_info)) > - { > - /* Peer does not support NCP, but leave NCP enabled if the local and > - * remote cipher do not match to attempt 'poor-man's NCP'. > - */ > - if (multi->remote_ciphername == NULL > - || 0 == strcmp(multi->remote_ciphername, multi->opt.config_ciphername)) > - { > - session->opt->ncp_enabled = false; > - } > - } > - This removes the last user of tls_peer_supports_ncp(), should that be removed too? Other than this I think this looks good and moves in the right direction. I haven't tested this thoroughly. Since much of the logic will be changing soon anyway, I think it's okay to move forward nevertheless. But we do need some aggressive testing when all the changes are in. -Steffan
Hi, On Tue, Jul 07, 2020 at 02:16:14PM +0200, Arne Schwabe wrote: > Ever since the NCPv2 the ncp_get_best_cipher uses the global > options->ncp_enabled option and ignore the tls_session->ncp_enabled > option. For the record, this breaks "poor man's NCP" for big packets - tested with 2.3 client and 2.4 with "--ncp-disable". Session is negotiated fine, key material is generated perfectly fine, both sides agree on ciphers, but if I do the "ping 3000 byte test" I get this on the server: 13:00 <@cron2> Jul 8 12:59:19 gentoo tun-udp-p2mp[30281]: cron2-freebsd-tc-amd64-23/2001:608:0:814::f000:21 TCP/UDP packet too large on write to [AF_INET6]2001:608:0:814::f000:21:35389 (tried=1544,max=1542) so it seems to get confused about frame size values. No --mtu-disc involved, no --anything-mtu configured on the server (= all on defaults). I do remember that this is scary stuff all intertwined... gert
Am 08.07.20 um 13:15 schrieb Gert Doering: > Hi, > > On Tue, Jul 07, 2020 at 02:16:14PM +0200, Arne Schwabe wrote: >> Ever since the NCPv2 the ncp_get_best_cipher uses the global >> options->ncp_enabled option and ignore the tls_session->ncp_enabled >> option. > > For the record, this breaks "poor man's NCP" for big packets - tested > with 2.3 client and 2.4 with "--ncp-disable". Session is negotiated > fine, key material is generated perfectly fine, both sides agree on > ciphers, but if I do the "ping 3000 byte test" I get this on the > server: > > 13:00 <@cron2> Jul 8 12:59:19 gentoo tun-udp-p2mp[30281]: cron2-freebsd-tc-amd64-23/2001:608:0:814::f000:21 TCP/UDP packet too large on write to [AF_INET6]2001:608:0:814::f000:21:35389 (tried=1544,max=1542) > > so it seems to get confused about frame size values. > > No --mtu-disc involved, no --anything-mtu configured on the server (= all > on defaults). > > I do remember that this is scary stuff all intertwined... Looks like our frame calculation for NCP is somewhat broken and this change just exposed this bug better. This "fix" might work: --- a/src/openvpn/ssl.c +++ b/src/openvpn/ssl.c @@ -1986,6 +1986,12 @@ tls_session_update_crypto_params(struct tls_session *session, options->keysize = 0; } } + else + { + /* Very hacky workaround and quick fix for our calculation + * not correct to avoid a regression */ + return tls_session_generate_data_channel_keys(session); + } init_key_type(&session->opt->key_type, options->ciphername, options->authname, options->keysize, true, true);
Am 08.07.20 um 12:10 schrieb Steffan Karger: > Hi, > > As discusses in #openvpn-devel on IRC, this patch breaks interop with > clients that don't pull, but that will be restored in a follow-up > refactoring (before 2.5 rc1). I can live with that, but I think this > should be mentioned in the commit message. I can fix that by reordering the commits but I will that this moves generation of keys for this corner case. Arne
Hi, On Wed, Jul 08, 2020 at 03:15:49PM +0200, Arne Schwabe wrote: > +++ b/src/openvpn/ssl.c > @@ -1986,6 +1986,12 @@ tls_session_update_crypto_params(struct > tls_session *session, > options->keysize = 0; > } > } > + else > + { > + /* Very hacky workaround and quick fix for our calculation > + * not correct to avoid a regression */ > + return tls_session_generate_data_channel_keys(session); > + } Just for the record: that nasty hack made the server happy again. start client jobs... 23... Test sets succeeded: 1 1a 1b 1d 2 2a 2b 2c 2d 3 4 5 6 8 8a 9. Test sets failed: none. 24... Test sets succeeded: 1 1a 1b 1c 1d 1e 2 2a 2b 2c 2d 2e 3 4 4a 5 6 8 8a 9. Test sets failed: none. master... Test sets succeeded: 1 1a 1b 1c 1d 1e 2 2a 2b 2c 2d 2e 3 4 5 6 7 7a 8 8a 9 2f 4b. Test sets failed: none. So we decided to go "with the hack for now" and clean up the mine field arounding frame size stuff afterwards. Patch set incoming tomorrow. gert
diff --git a/src/openvpn/init.c b/src/openvpn/init.c index 91b919d5..e9c01629 100644 --- a/src/openvpn/init.c +++ b/src/openvpn/init.c @@ -2376,6 +2376,8 @@ do_deferred_options(struct context *c, const unsigned int found) } else if (c->options.ncp_enabled) { + /* If the server did not push a --cipher, we will switch to the + * remote cipher if it is in our ncp-ciphers list */ tls_poor_mans_ncp(&c->options, c->c2.tls_multi->remote_ciphername); } struct frame *frame_fragment = NULL; diff --git a/src/openvpn/ssl.c b/src/openvpn/ssl.c index 9df7552d..71565dd3 100644 --- a/src/openvpn/ssl.c +++ b/src/openvpn/ssl.c @@ -2463,8 +2463,7 @@ key_method_2_write(struct buffer *buf, struct tls_session *session) * generation is postponed until after the pull/push, so we can process pushed * cipher directives. */ - if (session->opt->server && !(session->opt->ncp_enabled - && session->opt->mode == MODE_SERVER && ks->key_id <= 0)) + if (session->opt->server && !(session->opt->mode == MODE_SERVER && ks->key_id <= 0)) { if (ks->authenticated != KS_AUTH_FALSE) { @@ -2616,18 +2615,6 @@ key_method_2_read(struct buffer *buf, struct tls_multi *multi, struct tls_sessio multi->remote_ciphername = options_string_extract_option(options, "cipher", NULL); - if (!tls_peer_supports_ncp(multi->peer_info)) - { - /* Peer does not support NCP, but leave NCP enabled if the local and - * remote cipher do not match to attempt 'poor-man's NCP'. - */ - if (multi->remote_ciphername == NULL - || 0 == strcmp(multi->remote_ciphername, multi->opt.config_ciphername)) - { - session->opt->ncp_enabled = false; - } - } - if (tls_session_user_pass_enabled(session)) { /* Perform username/password authentication */
Ever since the NCPv2 the ncp_get_best_cipher uses the global options->ncp_enabled option and ignore the tls_session->ncp_enabled option. The server side's poor man's NCP is implemented as seeing the list of supported ciphers from the peer as just one cipher so this special handling for poor man's NCP of the older NCP here is not needed anymore. Theoretically we can now get rid of tls_session->ncp_enabled but doing so requires more refactoring since options is not available in the methods that still use it. And when we remove ncp-disable the variable will be removed anyway. Also document the remaining usage of tls_poor_mans_ncp better. Signed-off-by: Arne Schwabe <arne@rfc2549.org> --- src/openvpn/init.c | 2 ++ src/openvpn/ssl.c | 15 +-------------- 2 files changed, 3 insertions(+), 14 deletions(-)