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 <a...@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
_______________________________________________
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel
