[Openvpn-devel,v2] Support NCP in pure P2P VPN setups

Currently P2P mode of OpenVPN is on of the few places that cannot negotiate
modern OpenVPN features. This becomes more and more problematic since P2P and
P2MP code diverge more and more and also the lack of switching to more
advanced features like Data v2 currently blocks P2P mode from working
together with the upcoming ovpn-dco support.

This NCP support is a lot simpler and works in the following way:

- P2P peer announce an extremely limited IV_ variable set
  (IV_PROTO and IV_CIPHERS)
- Both peers check if the IV_PROTO_NCP_P2P bit is present in IV_PROTO
- if yes both sides deterministically determine according to
  IV_PROTO and IV_CIPHER what options can be used and start using these

There are no poor man's NCP or other compatibility workaround like in the
normal NCP, making this NCP leaner and more deterministic.

Patch v2: remove empty lines, add doxygen comment to push_peer_info, fix
          push_peer_info >= 2 that should be > 2

Signed-off-by: Arne Schwabe <arne@rfc2549.org>
---
 src/openvpn/init.c                  |  99 +++++++++++++++----
 src/openvpn/options.c               |   8 +-
 src/openvpn/ssl.c                   | 133 ++++++++++++++++++--------
 src/openvpn/ssl.h                   |   5 +
 src/openvpn/ssl_backend.h           |   1 +
 src/openvpn/ssl_common.h            |  10 ++
 src/openvpn/ssl_ncp.c               | 143 ++++++++++++++++++++++++++++
 src/openvpn/ssl_ncp.h               |  25 +++++
 tests/unit_tests/openvpn/test_ncp.c |  11 +++
 9 files changed, 376 insertions(+), 59 deletions(-)

Hi,

I saw a few small things that can be fixed on the fly:

On 28/07/2021 14:30, Arne Schwabe wrote:
> Currently P2P mode of OpenVPN is on of the few places that cannot negotiate
> modern OpenVPN features. This becomes more and more problematic since P2P and
> P2MP code diverge more and more and also the lack of switching to more
> advanced features like Data v2 currently blocks P2P mode from working
> together with the upcoming ovpn-dco support.
> 
> This NCP support is a lot simpler and works in the following way:
> 
> - P2P peer announce an extremely limited IV_ variable set
>   (IV_PROTO and IV_CIPHERS)
> - Both peers check if the IV_PROTO_NCP_P2P bit is present in IV_PROTO
> - if yes both sides deterministically determine according to
>   IV_PROTO and IV_CIPHER what options can be used and start using these
> 
> There are no poor man's NCP or other compatibility workaround like in the
> normal NCP, making this NCP leaner and more deterministic.
> 
> Patch v2: remove empty lines, add doxygen comment to push_peer_info, fix
>           push_peer_info >= 2 that should be > 2
> 
> Signed-off-by: Arne Schwabe <arne@rfc2549.org>
> ---
>  src/openvpn/init.c                  |  99 +++++++++++++++----
>  src/openvpn/options.c               |   8 +-
>  src/openvpn/ssl.c                   | 133 ++++++++++++++++++--------
>  src/openvpn/ssl.h                   |   5 +
>  src/openvpn/ssl_backend.h           |   1 +
>  src/openvpn/ssl_common.h            |  10 ++
>  src/openvpn/ssl_ncp.c               | 143 ++++++++++++++++++++++++++++
>  src/openvpn/ssl_ncp.h               |  25 +++++
>  tests/unit_tests/openvpn/test_ncp.c |  11 +++
>  9 files changed, 376 insertions(+), 59 deletions(-)
> 
> diff --git a/src/openvpn/init.c b/src/openvpn/init.c
> index 60471833e..386aee230 100644
> --- a/src/openvpn/init.c
> +++ b/src/openvpn/init.c
> @@ -68,6 +68,7 @@ static const char *saved_pid_file_name; /* GLOBAL */
>  #define CF_INIT_TLS_AUTH_STANDALONE (1<<2)
>  
>  static void do_init_first_time(struct context *c);
> +static bool do_deferred_p2p_ncp(struct context *c);
>  
>  void
>  context_clear(struct context *c)
> @@ -2151,6 +2152,14 @@ do_up(struct context *c, bool pulled_options, unsigned int option_types_found)
>                  return false;
>              }
>          }
> +        else if (c->mode == MODE_POINT_TO_POINT)
> +        {
> +            if (!do_deferred_p2p_ncp(c))
> +            {
> +                msg(D_TLS_ERRORS, "ERROR: Failed to apply P2P negotiated protocol options");
> +                return false;
> +            }
> +        }
>  
>          /* if --up-delay specified, open tun, do ifconfig, and run up script now */
>          if (c->options.up_delay || PULL_DEFINED(&c->options))
> @@ -2241,6 +2250,71 @@ pull_permission_mask(const struct context *c)
>      return flags;
>  }
>  
> +static
> +void adjust_mtu_peerid(struct context *c)
> +{
> +    frame_add_to_extra_frame(&c->c2.frame, 3);     /* peer-id overhead */
> +    if (!c->options.ce.link_mtu_defined)
> +    {
> +        frame_add_to_link_mtu(&c->c2.frame, 3);
> +        msg(D_PUSH, "OPTIONS IMPORT: adjusting link_mtu to %d",
> +            EXPANDED_SIZE(&c->c2.frame));
> +    }
> +    else
> +    {
> +        msg(M_WARN, "OPTIONS IMPORT: WARNING: peer-id set, but link-mtu"
> +                    " fixed by config - reducing tun-mtu to %d, expect"
> +                    " MTU problems", TUN_MTU_SIZE(&c->c2.frame));
> +    }
> +}
> +
> +static bool
> +do_deferred_p2p_ncp(struct context *c)
> +{
> +    if (!c->c2.tls_multi)
> +    {
> +        return true;
> +    }
> +
> +    if (c->c2.tls_multi->use_peer_id)
> +    {
> +        adjust_mtu_peerid(c);
> +    }
> +
> +    struct tls_session *session = &c->c2.tls_multi->session[TM_ACTIVE];
> +
> +    const char *ncp_cipher = get_p2p_ncp_cipher(session, c->c2.tls_multi->peer_info,
> +                                                &c->options.gc);
> +
> +    if (ncp_cipher)
> +    {
> +        c->options.ciphername = ncp_cipher;
> +    }
> +    else if (!c->options.enable_ncp_fallback)
> +    {
> +        msg(D_TLS_ERRORS, "ERROR: failed to negotiate cipher with peer and "
> +                          "--data-ciphers-fallback not enabled. No usable "
> +                          "data channel cipher");
> +        return false;
> +    }
> +
> +    struct frame *frame_fragment = NULL;
> +#ifdef ENABLE_FRAGMENT
> +    if (c->options.ce.fragment)
> +    {
> +        frame_fragment = &c->c2.frame_fragment;
> +    }
> +#endif
> +
> +    if (!tls_session_update_crypto_params(session, &c->options, &c->c2.frame,
> +                                         frame_fragment))
> +    {
> +        msg(D_TLS_ERRORS, "ERROR: failed to set crypto cipher");
> +        return false;
> +    }
> +    return true;
> +}
> +
>  /*
>   * Handle non-tun-related pulled options.
>   */
> @@ -2328,19 +2402,7 @@ do_deferred_options(struct context *c, const unsigned int found)
>          msg(D_PUSH, "OPTIONS IMPORT: peer-id set");
>          c->c2.tls_multi->use_peer_id = true;
>          c->c2.tls_multi->peer_id = c->options.peer_id;
> -        frame_add_to_extra_frame(&c->c2.frame, +3);     /* peer-id overhead */
> -        if (!c->options.ce.link_mtu_defined)
> -        {
> -            frame_add_to_link_mtu(&c->c2.frame, +3);
> -            msg(D_PUSH, "OPTIONS IMPORT: adjusting link_mtu to %d",
> -                EXPANDED_SIZE(&c->c2.frame));
> -        }
> -        else
> -        {
> -            msg(M_WARN, "OPTIONS IMPORT: WARNING: peer-id set, but link-mtu"
> -                " fixed by config - reducing tun-mtu to %d, expect"
> -                " MTU problems", TUN_MTU_SIZE(&c->c2.frame) );
> -        }
> +        adjust_mtu_peerid(c);
>      }
>  
>      /* process (potentially pushed) crypto options */
> @@ -2860,16 +2922,21 @@ do_init_crypto_tls(struct context *c, const unsigned int flags)
>      to.pull = options->pull;
>      if (options->push_peer_info)        /* all there is */
>      {
> -        to.push_peer_info_detail = 2;
> +        to.push_peer_info_detail = 3;
>      }
>      else if (options->pull)             /* pull clients send some details */
>      {
> -        to.push_peer_info_detail = 1;
> +        to.push_peer_info_detail = 2;
>      }
> -    else                                /* default: no peer-info at all */
> +    else if (options->mode == MODE_SERVER) /* server: no peer info at all */
>      {
>          to.push_peer_info_detail = 0;
>      }
> +    else                  /* default: minimal info to allow NCP in P2P mode */
> +    {
> +        to.push_peer_info_detail = 1;
> +    }
> +
>  
>      /* should we not xmit any packets until we get an initial
>       * response from client? */
> diff --git a/src/openvpn/options.c b/src/openvpn/options.c
> index b57e8ecc6..63cda1e86 100644
> --- a/src/openvpn/options.c
> +++ b/src/openvpn/options.c
> @@ -3081,10 +3081,6 @@ options_postprocess_cipher(struct options *o)
>  {
>      if (!o->pull && !(o->mode == MODE_SERVER))
>      {
> -        /* we are in the classic P2P mode */
> -        msg( M_WARN, "Cipher negotiation is disabled since neither "
> -             "P2MP client nor server mode is enabled");
> -
>          /* If the cipher is not set, use the old default of BF-CBC. We will
>           * warn that this is deprecated on cipher initialisation, no need
>           * to warn here as well */
> @@ -3092,6 +3088,10 @@ options_postprocess_cipher(struct options *o)
>          {
>              o->ciphername = "BF-CBC";
>          }
> +        else
> +        {
> +            o->enable_ncp_fallback = true;
> +        }
>          return;
>      }
>  
> diff --git a/src/openvpn/ssl.c b/src/openvpn/ssl.c
> index 96e04ac11..2b2d8b841 100644
> --- a/src/openvpn/ssl.c
> +++ b/src/openvpn/ssl.c
> @@ -1875,32 +1875,16 @@ cleanup:
>  }
>  
>  bool
> -tls_session_update_crypto_params(struct tls_session *session,
> -                                 struct options *options, struct frame *frame,
> +tls_session_update_crypto_params_do_work(struct tls_session *session,
> +                                 struct options* options, struct frame *frame,
>                                   struct frame *frame_fragment)
>  {
>      if (session->key[KS_PRIMARY].crypto_options.key_ctx_bi.initialized)
>      {
>          /* keys already generated, nothing to do */
>          return true;
> -    }
> -
> -    bool cipher_allowed_as_fallback = options->enable_ncp_fallback
> -                                      && streq(options->ciphername, session->opt->config_ciphername);
>  
> -    if (!session->opt->server && !cipher_allowed_as_fallback
> -        && !tls_item_in_cipher_list(options->ciphername, options->ncp_ciphers))
> -    {
> -        msg(D_TLS_ERRORS, "Error: pushed cipher not allowed - %s not in %s",
> -            options->ciphername, options->ncp_ciphers);
> -        /* undo cipher push, abort connection setup */
> -        options->ciphername = session->opt->config_ciphername;
> -        return false;
>      }
> -
> -    /* Import crypto settings that might be set by pull/push */
> -    session->opt->crypto_flags |= options->data_channel_crypto_flags;
> -
>      if (strcmp(options->ciphername, session->opt->config_ciphername))
>      {
>          msg(D_HANDSHAKE, "Data Channel: using negotiated cipher '%s'",
> @@ -1952,6 +1936,32 @@ tls_session_update_crypto_params(struct tls_session *session,
>      return tls_session_generate_data_channel_keys(session);
>  }
>  
> +bool
> +tls_session_update_crypto_params(struct tls_session *session,
> +                                 struct options *options, struct frame *frame,
> +                                 struct frame *frame_fragment)
> +{
> +
> +    bool cipher_allowed_as_fallback = options->enable_ncp_fallback
> +        && streq(options->ciphername, session->opt->config_ciphername);
> +
> +    if (!session->opt->server && !cipher_allowed_as_fallback
> +        && !tls_item_in_cipher_list(options->ciphername, options->ncp_ciphers))
> +    {
> +        msg(D_TLS_ERRORS, "Error: negotiated cipher not allowed - %s not in %s",
> +            options->ciphername, options->ncp_ciphers);
> +        /* undo cipher push, abort connection setup */
> +        options->ciphername = session->opt->config_ciphername;
> +        return false;
> +    }
> +
> +    /* Import crypto settings that might be set by pull/push */
> +    session->opt->crypto_flags |= options->data_channel_crypto_flags;
> +
> +    return tls_session_update_crypto_params_do_work(session, options, frame, frame_fragment);
> +}
> +
> +
>  static bool
>  random_bytes_to_buf(struct buffer *buf,
>                      uint8_t *out,
> @@ -2140,17 +2150,30 @@ read_string_alloc(struct buffer *buf)
>      return str;
>  }
>  
> +/**
> + * Prepares the IV_ and UV_ variables that are part of the
> + * exchange to signal the peer's capabilities. The amount
> + * of variables is determined by session->opt->push_peer_info_detail
> + *
> + *     0     nothing. Used on a TLS P2MP server side to send no information
> + *           to the client
> + *     1     minimal info needed for NCP in P2P mode
> + *     2     when --pull is enabled, the "default" set of variables
> + *     3     all information including MAC address and library versions
> + *
> + * @param buf       the buffer to write these variables to
> + * @param session   the TLS session object
> + * @return          true if no error was encountered
> + */
>  static bool
>  push_peer_info(struct buffer *buf, struct tls_session *session)
>  {
>      struct gc_arena gc = gc_new();
>      bool ret = false;
> +    struct buffer out = alloc_buf_gc(512 * 3, &gc);
>  
> -    if (session->opt->push_peer_info_detail > 0)
> +    if (session->opt->push_peer_info_detail > 1)
>      {
> -        struct env_set *es = session->opt->es;
> -        struct buffer out = alloc_buf_gc(512*3, &gc);
> -
>          /* push version */
>          buf_printf(&out, "IV_VER=%s\n", PACKAGE_VERSION);
>  
> @@ -2172,7 +2195,11 @@ push_peer_info(struct buffer *buf, struct tls_session *session)
>  #elif defined(_WIN32)
>          buf_printf(&out, "IV_PLAT=win\n");
>  #endif
> +    }
>  
> +    /* These are the IV variable that are sent to peers in p2p mode */
> +    if (session->opt->push_peer_info_detail > 0)
> +    {
>          /* support for P_DATA_V2 */
>          int iv_proto = IV_PROTO_DATA_V2;
>  
> @@ -2195,21 +2222,30 @@ push_peer_info(struct buffer *buf, struct tls_session *session)
>  
>                  buf_printf(&out, "IV_NCP=2\n");
>              }
> -            buf_printf(&out, "IV_CIPHERS=%s\n", session->opt->config_ncp_ciphers);
> +        }
> +        else
> +        {
> +            /* We are not using pull or p2mp server, instead do P2P NCP */
> +            iv_proto |= IV_PROTO_NCP_P2P;
> +        }
> +
> +        buf_printf(&out, "IV_CIPHERS=%s\n", session->opt->config_ncp_ciphers);
>  
>  #ifdef HAVE_EXPORT_KEYING_MATERIAL
> -            iv_proto |= IV_PROTO_TLS_KEY_EXPORT;
> +        iv_proto |= IV_PROTO_TLS_KEY_EXPORT;
>  #endif
> -        }
>  
>          buf_printf(&out, "IV_PROTO=%d\n", iv_proto);
>  
> -        /* push compression status */
> +        if (session->opt->push_peer_info_detail > 1)
> +        {
> +            /* push compression status */
>  #ifdef USE_COMP
> -        comp_generate_peer_info_string(&session->opt->comp_options, &out);
> +            comp_generate_peer_info_string(&session->opt->comp_options, &out);
>  #endif
> +        }
>  
> -        if (session->opt->push_peer_info_detail >= 2)
> +        if (session->opt->push_peer_info_detail > 2)
>          {
>              /* push mac addr */
>              struct route_gateway_info rgi;
> @@ -2224,20 +2260,24 @@ push_peer_info(struct buffer *buf, struct tls_session *session)
>  #endif
>          }
>  
> -        /* push env vars that begin with UV_, IV_PLAT_VER and IV_GUI_VER */
> -        for (struct env_item *e = es->list; e != NULL; e = e->next)
> +        if (session->opt->push_peer_info_detail > 1)
>          {
> -            if (e->string)
> +            struct env_set *es = session->opt->es;
> +            /* push env vars that begin with UV_, IV_PLAT_VER and IV_GUI_VER */
> +            for (struct env_item *e = es->list; e != NULL; e = e->next)
>              {
> -                if ((((strncmp(e->string, "UV_", 3)==0
> -                       || strncmp(e->string, "IV_PLAT_VER=", sizeof("IV_PLAT_VER=")-1)==0)
> -                      && session->opt->push_peer_info_detail >= 2)
> -                     || (strncmp(e->string,"IV_GUI_VER=",sizeof("IV_GUI_VER=")-1)==0)
> -                     || (strncmp(e->string,"IV_SSO=",sizeof("IV_SSO=")-1)==0)
> -                     )
> -                    && buf_safe(&out, strlen(e->string)+1))
> +                if (e->string)
>                  {
> -                    buf_printf(&out, "%s\n", e->string);
> +                    if ((((strncmp(e->string, "UV_", 3) == 0
> +                        || strncmp(e->string, "IV_PLAT_VER=", sizeof("IV_PLAT_VER=") - 1) == 0)
> +                        && session->opt->push_peer_info_detail >= 2)
> +                        || (strncmp(e->string, "IV_GUI_VER=", sizeof("IV_GUI_VER=") - 1) == 0)
> +                        || (strncmp(e->string, "IV_SSO=", sizeof("IV_SSO=") - 1) == 0)
> +                    )
> +                        && buf_safe(&out, strlen(e->string) + 1))
> +                    {
> +                        buf_printf(&out, "%s\n", e->string);
> +                    }
>                  }
>              }
>          }
> @@ -2380,6 +2420,14 @@ key_method_2_write(struct buffer *buf, struct tls_multi *multi, struct tls_sessi
>          goto error;
>      }
>  
> +    if (session->opt->server &&session->opt->mode != MODE_SERVER
> +        && ks->key_id == 0)
> +    {
> +        /* tls-server option set and not P2MP server, so we
> +         * are a P2P client running in tls-server mode */
> +        p2p_mode_ncp(multi, session);
> +    }
> +
>      return true;
>  
>  error:
> @@ -2580,6 +2628,13 @@ key_method_2_read(struct buffer *buf, struct tls_multi *multi, struct tls_sessio
>          setenv_del(session->opt->es, "exported_keying_material");
>      }
>  
> +    if (!session->opt->server && !session->opt->pull && ks->key_id == 0)
> +    {
> +        /* We are a p2p tls-client without pull, enable common
> +         * protocol options */
> +        p2p_mode_ncp(multi, session);
> +    }
> +
>      gc_free(&gc);
>      return true;
>  
> diff --git a/src/openvpn/ssl.h b/src/openvpn/ssl.h
> index 58b39466b..b14453fe2 100644
> --- a/src/openvpn/ssl.h
> +++ b/src/openvpn/ssl.h
> @@ -119,6 +119,11 @@
>  /** Supports signaling keywords with AUTH_PENDING, e.g. timeout=xy */
>  #define IV_PROTO_AUTH_PENDING_KW (1<<4)
>  
> +/** Support doing NCP in P2P mode. This mode works by both peers looking at
> + * each other's IV_ variables and deterministically deciding both on the same
> + * result. */
> +#define IV_PROTO_NCP_P2P         (1<<5)
> +
>  /* Default field in X509 to be username */
>  #define X509_USERNAME_FIELD_DEFAULT "CN"
>  
> diff --git a/src/openvpn/ssl_backend.h b/src/openvpn/ssl_backend.h
> index fe93263ca..c877e1947 100644
> --- a/src/openvpn/ssl_backend.h
> +++ b/src/openvpn/ssl_backend.h
> @@ -390,6 +390,7 @@ void backend_tls_ctx_reload_crl(struct tls_root_ctx *ssl_ctx,
>                                  const char *crl_file, bool crl_inline);
>  
>  #define EXPORT_KEY_DATA_LABEL       "EXPORTER-OpenVPN-datakeys"
> +#define EXPORT_P2P_PEERID_LABEL     "EXPORTER-OpenVPN-p2p-peerid"
>  /**
>   * Keying Material Exporters [RFC 5705] allows additional keying material to be
>   * derived from existing TLS channel. This exported keying material can then be
> diff --git a/src/openvpn/ssl_common.h b/src/openvpn/ssl_common.h
> index d7c89575a..3ab24b851 100644
> --- a/src/openvpn/ssl_common.h
> +++ b/src/openvpn/ssl_common.h
> @@ -289,6 +289,16 @@ struct tls_options
>      bool disable_occ;
>      int mode;
>      bool pull;
> +    /**
> +     * The detail of info we push in peer info
> +     *
> +     * 0 - nothing at all, P2MP server only
> +     * 1 - only the most basic information to negotiate cipher and features
> +     *     for P2P NCP
> +     * 2 - normal setting for clients
> +     * 3 - full information including "sensitive data" like IV_HWADDR
> +     *     enabled by --push-peer-info
> +     */
>      int push_peer_info_detail;
>      int transition_window;
>      int handshake_window;
> diff --git a/src/openvpn/ssl_ncp.c b/src/openvpn/ssl_ncp.c
> index 0258e6a72..b44186514 100644
> --- a/src/openvpn/ssl_ncp.c
> +++ b/src/openvpn/ssl_ncp.c
> @@ -324,3 +324,146 @@ check_pull_client_ncp(struct context *c, const int found)
>          return false;
>      }
>  }
> +
> +const char*
> +get_p2p_ncp_cipher(struct tls_session *session, const char *peer_info,
> +                   struct gc_arena *gc)
> +{
> +    /* we use a local gc arena to keep the temporary strings needed by strsep */
> +    struct gc_arena gc_local = gc_new();
> +    const char *peer_ciphers = extract_var_peer_info(peer_info, "IV_CIPHERS=", &gc_local);
> +
> +    if (!peer_ciphers)
> +    {
> +        gc_free(&gc_local);
> +        return NULL;
> +    }
> +
> +    const char* server_ciphers;
> +    const char* client_ciphers;
> +
> +    if (session->opt->server)
> +    {
> +        server_ciphers = session->opt->config_ncp_ciphers;
> +        client_ciphers = peer_ciphers;
> +    }
> +    else
> +    {
> +        client_ciphers = session->opt->config_ncp_ciphers;
> +        server_ciphers = peer_ciphers;
> +    }
> +
> +    /* Find the first common cipher from TLS server and TLS client. We
> +     * use the preference of the server here to make it deterministic */
> +    char *tmp_ciphers = string_alloc(server_ciphers, &gc_local);
> +
> +    const char *token;
> +    while ((token = strsep(&tmp_ciphers, ":")))
> +    {
> +        if (tls_item_in_cipher_list(token, client_ciphers))
> +        {
> +            break;
> +        }
> +    }
> +
> +    const char *ret = NULL;
> +    if (token != NULL)

this can just be "if (token)" like you have done for all other
conditions you introduced :)

> +    {
> +        ret = string_alloc(token, gc);
> +    }
> +    gc_free(&gc_local);
> +
> +    return ret;
> +}
> +
> +static void
> +p2p_ncp_set_options(struct tls_multi *multi, struct tls_session *session)
> +{
> +    /* will return 0 if peer_info is null */
> +    const unsigned int iv_proto_peer = extract_iv_proto(multi->peer_info);
> +
> +    /* The other peer does not support P2P NCP */
> +    if (!(iv_proto_peer & IV_PROTO_NCP_P2P))
> +    {
> +        return;
> +    }
> +
> +    if (iv_proto_peer & IV_PROTO_DATA_V2)
> +    {
> +        multi->use_peer_id = true;
> +        multi->peer_id = 0x76706e; // 'v' 'p' 'n'
> +    }
> +
> +#if defined(HAVE_EXPORT_KEYING_MATERIAL)
> +    if (iv_proto_peer & IV_PROTO_TLS_KEY_EXPORT)
> +    {
> +        session->opt->crypto_flags |= CO_USE_TLS_KEY_MATERIAL_EXPORT;
> +
> +        if (multi->use_peer_id)
> +        {
> +            /* Using a non hardcoded peer-id makes a tiny bit harder to
> +             * fingerprint packets and also gives each connection a unique
> +             * peer-id that can be useful for NAT tracking etc. */
> +
> +            uint8_t peerid[3];
> +            if (!key_state_export_keying_material(session, EXPORT_P2P_PEERID_LABEL,
> +                                                  strlen(EXPORT_P2P_PEERID_LABEL),
> +                                                  &peerid, 3))
> +            {
> +                /* Non DCO setup might still work but also this should never
> +                 * happen or very likely the TLS encryption key exporter will
> +                 * also fail */
> +                msg(M_NONFATAL, "TLS key export for P2P peer id failed. "
> +                                "Continuing anyway, expect problems");
> +            }
> +            else
> +            {
> +                multi->peer_id = (peerid[0] << 16) + (peerid[1] << 8) + peerid[2];
> +            }
> +
> +        }
> +    }
> +#endif
> +}
> +
> +void
> +p2p_mode_ncp(struct tls_multi *multi, struct tls_session *session)
> +{
> +    /* Set the common options */
> +    p2p_ncp_set_options(multi, session);
> +
> +    struct gc_arena gc = gc_new();
> +
> +    /* Query the common cipher here to log it as part of our message.
> +     * We postpone switching the cipher to do_up */
> +    const char* common_cipher = get_p2p_ncp_cipher(session, multi->peer_info, &gc);
> +
> +    if (!common_cipher)
> +    {
> +        struct buffer out = alloc_buf_gc(128, &gc);
> +        struct key_state *ks = get_key_scan(multi, KS_PRIMARY);
> +
> +        const cipher_ctx_t *ctx = ks->crypto_options.key_ctx_bi.encrypt.cipher;
> +        const cipher_kt_t *cipher = cipher_ctx_get_cipher_kt(ctx);
> +        const char *fallback_name = cipher_kt_name(cipher);
> +
> +        if (!cipher)
> +        {
> +            /* at this point we do not really know if our fallback is
> +             * not enabled or if we use 'none' cipher as fallback, so
> +             * keep this ambiguity here and print fallback-cipher: none
> +             */
> +            fallback_name = "none";
> +        }
> +
> +        buf_printf(&out, "(not negotiated, fallback-cipher: %s)", fallback_name);
> +        common_cipher = BSTR(&out);
> +    }
> +
> +    msg(D_TLS_DEBUG_LOW, "P2P mode NCP negotiation result: "
> +                         "TLS_export=%d, DATA_v2=%d, peer-id %d, cipher=%s",

the following two lines should be indented after the opening '(' because
they are not part of the message string.

> +                         (bool)(session->opt->crypto_flags & CO_USE_TLS_KEY_MATERIAL_EXPORT),
> +                         multi->use_peer_id, multi->peer_id, common_cipher);
> +
> +    gc_free(&gc);
> +}
> \ No newline at end of file
> diff --git a/src/openvpn/ssl_ncp.h b/src/openvpn/ssl_ncp.h
> index 3fa68e262..4a2601a2e 100644
> --- a/src/openvpn/ssl_ncp.h
> +++ b/src/openvpn/ssl_ncp.h
> @@ -32,6 +32,7 @@
>  
>  #include "buffer.h"
>  #include "options.h"
> +#include "ssl_common.h"
>  
>  /**
>   * Returns whether the client supports NCP either by
> @@ -115,4 +116,28 @@ bool tls_item_in_cipher_list(const char *item, const char *list);
>   */
>  #define MAX_NCP_CIPHERS_LENGTH 127
>  
> +/**
> + * Determines if there is common cipher of both peer by looking at the
> + * IV_CIPHER peer info. In contrast of the server mode NCP that tries to
> + * accomandate all kind of corner cases in P2P mode NCP only takes IV_CIPHER
> + * into account and falls back to previous behaviour if this fails.
> + */
> +void p2p_mode_ncp(struct tls_multi *multi, struct tls_session *session);
> +
> +/**
> + * Determines the best common cipher from both peers IV_CIPHER lists. The
> + * first cipher from the tls-server that is also in the tls-client IV_CIPHER
> + * list will be returned. If no common cipher can be found, both peer
> + * will continue to use whatever cipher is their default and NULL will be
> + * returned.
> + *
> + * @param session       tls_session
> + * @param peer_info     peer info of the peer
> + * @param gc            gc arena that will be used to allocate the returned cipher
> + * @return              common cipher if one exist.
> + */
> +const char *
> +get_p2p_ncp_cipher(struct tls_session *session, const char *peer_info,
> +                   struct gc_arena *gc);
> +
>  #endif /* ifndef OPENVPN_SSL_NCP_H */
> diff --git a/tests/unit_tests/openvpn/test_ncp.c b/tests/unit_tests/openvpn/test_ncp.c
> index 494a02846..293093f1a 100644
> --- a/tests/unit_tests/openvpn/test_ncp.c
> +++ b/tests/unit_tests/openvpn/test_ncp.c
> @@ -44,6 +44,17 @@
>  const char *bf_chacha = "BF-CBC:CHACHA20-POLY1305";
>  const char *aes_ciphers = "AES-256-GCM:AES-128-GCM";
>  
> +
> +/* Define this function here as dummy since including the ssl_*.c files
> + * leads to having to include even more unrelated code */
> +bool
> +key_state_export_keying_material(struct tls_session *session,
> +                                 const char* label, size_t label_size,
> +                                 void *ekm, size_t ekm_size)
> +{
> +    ASSERT(0);
> +}
> +
>  static void
>  test_check_ncp_ciphers_list(void **state)
>  {
> 


The rest looks good!
My tests and compile-tests all passed.


Acked-by: Antonio Quartulli <antonio@openvpn.net>

I have stared at the code a bit, and it generally looks good (indent
fixed as instructed).

One observation:

 - in options_postprocess_cipher(), we now set "o->enable_ncp_fallback 
   = true", but *only* if a "cipher foo" is set in the config.  If not, we 
   set the cipher to o->ciphername = "BF-CBC", but disallow ncp-fallback
   - not sure if this is part of the crusade against BF-CBC "as default",
   but it looks a bit weird (we *set* BF-CBC as default, but do not want 
   to use it).


Testing this in the server side testbed yielded interesting problems
for the two p2p test cases, when connecting from a "pre P2P NCP" client
(master, but a few months old):

** "9" is "--client" to "--tls-server" **

With a "plain" config (no data-ciphers or data-cipher-fallback) the
connection works fine, both ends negotiate AES-256-GCM (= NCP works)

   P2P mode NCP negotiation result: TLS_export=0, DATA_v2=0, peer-id 0, cipher=AES-256-GCM
   Data Channel: using negotiated cipher 'AES-256-GCM'
   Data Channel MTU parms [ L:1468 D:1450 EF:-64 EB:392 ET:32 EL:3 AF:14/8 ]
   Outgoing Data Channel: Cipher 'AES-256-GCM' initialized with 256 bit key
   SENT CONTROL [cron2-freebsd-tc-amd64]: 'PUSH_REPLY,route 10.204.0.0 255.255.0.0 10.204.9.1,route-ipv6 fd00:abcd:204::/48,cipher AES-256-GCM' (status=1)

.. but then:

   TCP/UDP packet too large on write to [AF_INET6]::ffff:194.97.140.21:36144 (tried=1504,max=1468)

so the frame calculation is wrong, and pinging with large packets fails.

If I force the server to "no AES" (data-ciphers BF-CBC), it works:

   P2P mode NCP negotiation result: TLS_export=0, DATA_v2=0, peer-id 0, cipher=BF-CBC
   Outgoing Data Channel: Cipher 'BF-CBC' initialized with 128 bit key
   SENT CONTROL [cron2-freebsd-tc-amd64]: 'PUSH_REPLY,route 10.204.0.0 255.255.0.0 10.204.9.1,route-ipv6 fd00:abcd:204::/48,cipher BF-CBC' (status=1)

Notably it's not telling me anything about "using negotiated cipher",
and also nothing about the MTU params.


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

Using the config "without --data-ciphers or --data-cipher-fallback",
incoming connections fail:

    P2P mode NCP negotiation result: TLS_export=0, DATA_v2=0, peer-id 0, cipher=(not negotiated, fallback-cipher: none)
    TLS Error: local/remote TLS keys are out of sync: [AF_INET6]2001:608:0:814::f000:21:36664 (received key id: 0, known key ids:  [key#0 state=S_ACTIVE auth=KS_AUTH_TRUE id=0 sid=9daa0f59 9d7bf877] [key#1 state=S_UNDEF auth=KS_AUTH_FALSE id=0 sid=00000000 00000000] [key#2 state=S_UNDEF auth=KS_AUTH_FALSE id=0 sid=00000000 00000000]
    ERROR: failed to negotiate cipher with peer and --data-ciphers-fallback not enabled. No usable data channel cipher
    ERROR: Failed to apply P2P negotiated protocol options
    TLS Error: local/remote TLS keys are out of sync: [AF_INET6]2001:608:0:814::f000:21:36664 (received key id: 0, known key ids:  [key#0 state=S_ACTIVE auth=KS_AUTH_TRUE id=0 sid=9daa0f59 9d7bf877] [key#1 state=S_UNDEF auth=KS_AUTH_FALSE id=0 sid=00000000 00000000] [key#2 state=S_UNDEF auth=KS_AUTH_FALSE id=0 sid=00000000 00000000])

but the server does not actively terminate the connection - so we'll
see "TLS Error" errors scroll through, masking the actual error.

Adding "data-ciphers-fallback BF-CBC" makes this test instance well-behaved
(though there still is *one* TLS error):

    P2P mode NCP negotiation result: TLS_export=0, DATA_v2=0, peer-id 0, cipher=(not negotiated, fallback-cipher: none)
    TLS Error: local/remote TLS keys are out of sync: [AF_INET6]2001:608:0:814::f000:21:47925 (received key id: 0, known key ids:  [key#0 state=S_ACTIVE auth=KS_AUTH_TRUE id=0 sid=bfa5ccf1 c5c5e901] [key#1 state=S_UNDEF auth=KS_AUTH_FALSE id=0 sid=00000000 00000000] [key#2 state=S_UNDEF auth=KS_AUTH_FALSE id=0 sid=00000000 00000000])
    Outgoing Data Channel: Cipher 'BF-CBC' initialized with 128 bit key

("Fallback-cipher: none" looks a bit bogus, though... this might be
related to BF-CBC not being initialized early, or something)


The first case definitely needs fixing (I have more failing "frame" test
cases...), the second case could need some polishing - abort the session,
so it's clear "*this* is the error".


Connecting to the same instances from a client with P2P NCP yielded
different results (unsurprisingly) :-)

** "9" is "--client" to "--tls-server" **

If the server is configured to "data-ciphers BF-CBC", this works.  If
AES is added to the cipher list, the client's IV_CIPHER= wins, and they
negotiate AES-256-GCM...

   peer info: IV_CIPHERS=AES-256-GCM:AES-128-GCM
   peer info: IV_PROTO=30
   P2P mode NCP negotiation result: TLS_export=0, DATA_v2=0, peer-id 0, cipher=AES-256-GCM
   Data Channel: using negotiated cipher 'AES-256-GCM'
   Data Channel MTU parms [ L:1504 D:1450 EF:-28 EB:398 ET:32 EL:3 ]

.. and it fails:

   TCP/UDP packet too large on write to [AF_INET6]2001:608:4::ce:c0f:51605 (tried=1536,max=1504)

forcing the client to "--data-ciphers BF-CBC" makes them "negotiate"
BF-CBC, sidestepping the frame problem.

(I assume that having "--cipher AES-256-GCM" in both client and server
configs upfront would also solve this, but "having to add a cipher on
both sides to make NCP work" sounds like the wrong way forward)


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

Given the changes to the server.conf ("data-ciphers BF-CBC") to work
around the "frame" problem, this now fails with

    peer info: IV_CIPHERS=BF-CBC
    peer info: IV_PROTO=42
    P2P mode NCP negotiation result: TLS_export=1, DATA_v2=1, peer-id 16371030, cipher=(not negotiated, fallback-cipher: none)
    ERROR: failed to negotiate cipher with peer and --data-ciphers-fallback not enabled. No usable data channel cipher
    ERROR: Failed to apply P2P negotiated protocol options

(but the client is not aborting!) - extending --data-ciphers makes it 
connect fine:

    peer info: IV_CIPHERS=BF-CBC:AES-256-GCM:AES-128-GCM
    P2P mode NCP negotiation result: TLS_export=1, DATA_v2=1, peer-id 14972877, cipher=AES-256-GCM

.. but then we are back into the "frame problem" land...

    TCP/UDP packet too large on write to [AF_INET6]2001:608:0:814::f000:11:51204 (tried=1539,max=1471)

So the only way to make this work is to restrict the client to BF-CBC,
as with the --client test case.


Your patch has been applied to the master branch.

commit 8c72d7981c32c43d1c7967a312bb439e13fc5b40
Author: Arne Schwabe
Date:   Wed Jul 28 14:30:50 2021 +0200

     Support NCP in pure P2P VPN setups

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


--
kind regards,

Gert Doering