openvpn-devel

Re: [Openvpn-devel] [PATCH v5 4/9] Implement tls-groups option to specify eliptic curves/groups

[Antonio Q. <a...@un...>]

Hi,

I think a comment in my previous review was overlooked.

On 17/07/2020 15:47, Arne Schwabe wrote:
> @@ -343,6 +348,42 @@ tls_ctx_set_cert_profile(struct tls_root_ctx *ctx, const char *profile)
>      }
>  }
>  
> +void
> +tls_ctx_set_tls_groups(struct tls_root_ctx *ctx, const char *groups)
> +{
> +    ASSERT(ctx);
> +    struct gc_arena gc = gc_new();
> +
> +    /* Get number of groups and allocate an array in ctx */
> +    int groups_count = get_num_elements(groups, ':');
> +    ALLOC_ARRAY_CLEAR(ctx->groups, mbedtls_ecp_group_id, groups_count + 1)
> +
> +    /* Parse allowed ciphers, getting IDs */
> +    int i = 0;
> +    char *tmp_groups = string_alloc(groups, &gc);
> +
> +    const char *token;
> +    while ((token = strsep(&tmp_groups, ":")))
> +    {
> +        const mbedtls_ecp_curve_info *ci =
> +            mbedtls_ecp_curve_info_from_name(token);
> +        if (!ci)
> +        {
> +            msg(M_WARN, "Warning unknown curve/group specified: %s", token);
> +        }
> +        else
> +        {
> +            ctx->groups[i] = ci->grp_id;
> +            i++;
> +        }
> +        token = strsep(&tmp_groups, ":");

Aren't we calling strsep() twice in a row now?
Once in the while() condition and once at the end of the cycle?

I think Arne agreed on the issue on IRC, but maybe forgot to fix the patch?



Regards,


-- 
Antonio Quartulli

Re: [Openvpn-devel] [PATCH v5 4/9] Implement tls-groups option to specify eliptic curves/groups

[Antonio Q. <a...@un...>]

On 21/07/2020 15:46, Antonio Quartulli wrote:
> Aren't we calling strsep() twice in a row now?
> Once in the while() condition and once at the end of the cycle?
> 
> I think Arne agreed on the issue on IRC, but maybe forgot to fix the patch?
> 
> 

However, please note that now the patch compiles on my gitlab-ci test bench.

-- 
Antonio Quartulli

[Openvpn-devel] [PATCH v6 4/9] Implement tls-groups option to specify eliptic curves/groups

[Arne S. <ar...@rf...>]

By default OpenSSL 1.1+ only allows signatures and ecdh/ecdhx from the
default list of X25519:secp256r1:X448:secp521r1:secp384r1. In
TLS1.3 key exchange is independent from the signature/key of the
certificates, so allowing all groups per default is not a sensible
choice anymore and instead a shorter list is reasonable.

However, when using certificates with exotic curves that are not on
the group list, the signatures of these certificates will no longer
be accepted.

The tls-groups  option allows to modify the group list to account
for these corner cases.

Patch V2: Uses local gc_arena instead of malloc/free, reword commit
          message. Fix other typos/clarify messages

Patch V3: Style fixes, adjust code to changes from mbed tls session
          fix

Patch V5: Fix compilation with OpenSSL 1.0.2

Patch V6: Redo the 'while((token = strsep(&tmp_groups, ":"))' change
          that accidently got lost.

Signed-off-by: Arne Schwabe <ar...@rf...>
---
 README.ec                               |  7 ++--
 configure.ac                            |  1 +
 doc/man-sections/encryption-options.rst |  6 +--
 doc/man-sections/tls-options.rst        | 27 +++++++++++-
 src/openvpn/openssl_compat.h            |  6 +++
 src/openvpn/options.c                   | 10 ++++-
 src/openvpn/options.h                   |  1 +
 src/openvpn/ssl.c                       |  6 +++
 src/openvpn/ssl_backend.h               | 10 +++++
 src/openvpn/ssl_mbedtls.c               | 45 ++++++++++++++++++++
 src/openvpn/ssl_mbedtls.h               |  1 +
 src/openvpn/ssl_ncp.c                   |  5 +--
 src/openvpn/ssl_openssl.c               | 55 ++++++++++++++++++++++++-
 13 files changed, 168 insertions(+), 12 deletions(-)

diff --git a/README.ec b/README.ec
index 32938017..61f23b2e 100644
--- a/README.ec
+++ b/README.ec
@@ -12,14 +12,15 @@ OpenVPN 2.4.0 and newer automatically initialize ECDH parameters. When ECDSA is
 used for authentication, the curve used for the server certificate will be used
 for ECDH too. When autodetection fails (e.g. when using RSA certificates)
 OpenVPN lets the crypto library decide if possible, or falls back to the
-secp384r1 curve.
+secp384r1 curve. The list of groups/curves that the crypto library will choose
+from can be set with the --tls-groups <grouplist> option.
 
 An administrator can force an OpenVPN/OpenSSL server to use a specific curve
 using the --ecdh-curve <curvename> option with one of the curves listed as
-available by the --show-curves option. Clients will use the same curve as
+available by the --show-groups option. Clients will use the same curve as
 selected by the server.
 
-Note that not all curves listed by --show-curves are available for use with TLS;
+Note that not all curves listed by --show-groups are available for use with TLS;
 in that case connecting will fail with a 'no shared cipher' TLS error.
 
 Authentication (ECDSA)
diff --git a/configure.ac b/configure.ac
index 02cb0567..f8279924 100644
--- a/configure.ac
+++ b/configure.ac
@@ -929,6 +929,7 @@ if test "${with_crypto_library}" = "openssl"; then
 			OpenSSL_version \
 			SSL_CTX_get_default_passwd_cb \
 			SSL_CTX_get_default_passwd_cb_userdata \
+			SSL_CTX_set1_groups \
 			SSL_CTX_set_security_level \
 			X509_get0_notBefore \
 			X509_get0_notAfter \
diff --git a/doc/man-sections/encryption-options.rst b/doc/man-sections/encryption-options.rst
index a59f636c..ee34f14e 100644
--- a/doc/man-sections/encryption-options.rst
+++ b/doc/man-sections/encryption-options.rst
@@ -27,9 +27,9 @@ SSL Library information
   (Standalone) Show currently available hardware-based crypto acceleration
   engines supported by the OpenSSL library.
 
---show-curves
-  (Standalone) Show all available elliptic curves to use with the
-  ``--ecdh-curve`` option.
+--show-groups
+  (Standalone) Show all available elliptic curves/groups to use with the
+  ``--ecdh-curve`` and ``tls-groups`` options.
 
 Generating key material
 -----------------------
diff --git a/doc/man-sections/tls-options.rst b/doc/man-sections/tls-options.rst
index 92b832cd..ccc90ac9 100644
--- a/doc/man-sections/tls-options.rst
+++ b/doc/man-sections/tls-options.rst
@@ -338,6 +338,31 @@ certificates and keys: https://github.com/OpenVPN/easy-rsa
   Use ``--tls-crypt`` instead if you want to use the key file to not only
   authenticate, but also encrypt the TLS control channel.
 
+--tls-groups list
+    A list of allowable groups/curves in order of preference.
+
+    Set the allowed elictipic curves/groups for the TLS session.
+    These groups are  allowed to be used in signatures and key exchange.
+
+    mbed TLS currently allows all known curves per default.
+
+    OpenSSL 1.1+ restricts the list per default to
+    ::
+
+      "X25519:secp256r1:X448:secp521r1:secp384r1".
+
+    If you use certificates that use non-standard curves, you
+    might need to add them here. If you do not force the ecdh curve
+    by using ``--ecdh-curve``, the groups for ecdh will also be picked
+    from this list.
+
+    OpenVPN maps the curve name `secp256r1` to `prime256v1` to allow
+    specifying the same tls-groups option for mbed TLS and OpenSSL.
+
+    Warning: this option not only affects eliptic curve certificates
+    but also the key exchange in TLS 1.3 and using this option improperly
+    will disable TLS 1.3.
+
 --tls-cert-profile profile
   Set the allowed cryptographic algorithms for certificates according to
   ``profile``.
@@ -368,7 +393,7 @@ certificates and keys: https://github.com/OpenVPN/easy-rsa
   OpenVPN will migrate to 'preferred' as default in the future. Please
   ensure that your keys already comply.
 
-*WARNING:* ``--tls-ciphers`` and ``--tls-ciphersuites``
+*WARNING:* ``--tls-ciphers``, ``--tls-ciphersuites`` and ``tls-groups``
     These options are expert features, which - if used correctly - can
     improve the security of your VPN connection. But it is also easy to
     unwittingly use them to carefully align a gun with your foot, or just
diff --git a/src/openvpn/openssl_compat.h b/src/openvpn/openssl_compat.h
index d35251fb..eb6c9c90 100644
--- a/src/openvpn/openssl_compat.h
+++ b/src/openvpn/openssl_compat.h
@@ -183,6 +183,12 @@ SSL_CTX_get_default_passwd_cb(SSL_CTX *ctx)
 }
 #endif
 
+/* This function is implemented as macro, so the configure check for the
+ * function may fail, so we check for both variants here */
+#if !defined(HAVE_SSL_CTX_SET1_GROUPS) && !defined(SSL_CTX_set1_groups)
+#define SSL_CTX_set1_groups SSL_CTX_set1_curves
+#endif
+
 #if !defined(HAVE_X509_GET0_PUBKEY)
 /**
  * Get the public key from a X509 certificate
diff --git a/src/openvpn/options.c b/src/openvpn/options.c
index 94308a8e..7da04b6f 100644
--- a/src/openvpn/options.c
+++ b/src/openvpn/options.c
@@ -8057,7 +8057,7 @@ add_option(struct options *options,
         VERIFY_PERMISSION(OPT_P_GENERAL);
         options->show_tls_ciphers = true;
     }
-    else if (streq(p[0], "show-curves") && !p[1])
+    else if ((streq(p[0], "show-curves") || streq(p[0], "show-groups")) && !p[1])
     {
         VERIFY_PERMISSION(OPT_P_GENERAL);
         options->show_curves = true;
@@ -8065,6 +8065,9 @@ add_option(struct options *options,
     else if (streq(p[0], "ecdh-curve") && p[1] && !p[2])
     {
         VERIFY_PERMISSION(OPT_P_GENERAL);
+        msg(M_WARN, "Consider setting groups/curves preference with "
+            "tls-groups instead of forcing a specific curve with "
+            "ecdh-curve.");
         options->ecdh_curve = p[1];
     }
     else if (streq(p[0], "tls-server") && !p[1])
@@ -8235,6 +8238,11 @@ add_option(struct options *options,
         VERIFY_PERMISSION(OPT_P_GENERAL);
         options->cipher_list_tls13 = p[1];
     }
+    else if (streq(p[0], "tls-groups") && p[1] && !p[2])
+    {
+        VERIFY_PERMISSION(OPT_P_GENERAL);
+        options->tls_groups = p[1];
+    }
     else if (streq(p[0], "crl-verify") && p[1] && ((p[2] && streq(p[2], "dir"))
                                                    || !p[2]))
     {
diff --git a/src/openvpn/options.h b/src/openvpn/options.h
index c37006d3..1b038c91 100644
--- a/src/openvpn/options.h
+++ b/src/openvpn/options.h
@@ -542,6 +542,7 @@ struct options
     bool pkcs12_file_inline;
     const char *cipher_list;
     const char *cipher_list_tls13;
+    const char *tls_groups;
     const char *tls_cert_profile;
     const char *ecdh_curve;
     const char *tls_verify;
diff --git a/src/openvpn/ssl.c b/src/openvpn/ssl.c
index 04d78a81..00b97352 100644
--- a/src/openvpn/ssl.c
+++ b/src/openvpn/ssl.c
@@ -630,6 +630,12 @@ init_ssl(const struct options *options, struct tls_root_ctx *new_ctx)
     tls_ctx_restrict_ciphers(new_ctx, options->cipher_list);
     tls_ctx_restrict_ciphers_tls13(new_ctx, options->cipher_list_tls13);
 
+    /* Set the allow groups/curves for TLS if we want to override them */
+    if (options->tls_groups)
+    {
+        tls_ctx_set_tls_groups(new_ctx, options->tls_groups);
+    }
+
     if (!tls_ctx_set_options(new_ctx, options->ssl_flags))
     {
         goto err;
diff --git a/src/openvpn/ssl_backend.h b/src/openvpn/ssl_backend.h
index a1770bd4..75692797 100644
--- a/src/openvpn/ssl_backend.h
+++ b/src/openvpn/ssl_backend.h
@@ -198,6 +198,16 @@ void tls_ctx_restrict_ciphers_tls13(struct tls_root_ctx *ctx, const char *cipher
  */
 void tls_ctx_set_cert_profile(struct tls_root_ctx *ctx, const char *profile);
 
+/**
+ * Set the allowed (eliptic curve) group allowed for signatures and
+ * key exchange.
+ *
+ * @param ctx       TLS context to restrict, must be valid.
+ * @param groups    List of groups that will be allowed, in priority,
+ *                  separated by :
+ */
+void tls_ctx_set_tls_groups(struct tls_root_ctx *ctx, const char *groups);
+
 /**
  * Check our certificate notBefore and notAfter fields, and warn if the cert is
  * either not yet valid or has expired.  Note that this is a non-fatal error,
diff --git a/src/openvpn/ssl_mbedtls.c b/src/openvpn/ssl_mbedtls.c
index 977ff5c3..9c874788 100644
--- a/src/openvpn/ssl_mbedtls.c
+++ b/src/openvpn/ssl_mbedtls.c
@@ -176,6 +176,11 @@ tls_ctx_free(struct tls_root_ctx *ctx)
             free(ctx->allowed_ciphers);
         }
 
+        if (ctx->groups)
+        {
+            free(ctx->groups);
+        }
+
         CLEAR(*ctx);
 
         ctx->initialised = false;
@@ -343,6 +348,41 @@ tls_ctx_set_cert_profile(struct tls_root_ctx *ctx, const char *profile)
     }
 }
 
+void
+tls_ctx_set_tls_groups(struct tls_root_ctx *ctx, const char *groups)
+{
+    ASSERT(ctx);
+    struct gc_arena gc = gc_new();
+
+    /* Get number of groups and allocate an array in ctx */
+    int groups_count = get_num_elements(groups, ':');
+    ALLOC_ARRAY_CLEAR(ctx->groups, mbedtls_ecp_group_id, groups_count + 1)
+
+    /* Parse allowed ciphers, getting IDs */
+    int i = 0;
+    char *tmp_groups = string_alloc(groups, &gc);
+
+    const char *token;
+    while ((token = strsep(&tmp_groups, ":")))
+    {
+        const mbedtls_ecp_curve_info *ci =
+            mbedtls_ecp_curve_info_from_name(token);
+        if (!ci)
+        {
+            msg(M_WARN, "Warning unknown curve/group specified: %s", token);
+        }
+        else
+        {
+            ctx->groups[i] = ci->grp_id;
+            i++;
+        }
+    }
+    ctx->groups[i] = MBEDTLS_ECP_DP_NONE;
+
+    gc_free(&gc);
+}
+
+
 void
 tls_ctx_check_cert_time(const struct tls_root_ctx *ctx)
 {
@@ -1043,6 +1083,11 @@ key_state_ssl_init(struct key_state_ssl *ks_ssl,
         mbedtls_ssl_conf_ciphersuites(ks_ssl->ssl_config, ssl_ctx->allowed_ciphers);
     }
 
+    if (ssl_ctx->groups)
+    {
+        mbedtls_ssl_conf_curves(ks_ssl->ssl_config, ssl_ctx->groups);
+    }
+
     /* Disable record splitting (for now).  OpenVPN assumes records are sent
      * unfragmented, and changing that will require thorough review and
      * testing.  Since OpenVPN is not susceptible to BEAST, we can just
diff --git a/src/openvpn/ssl_mbedtls.h b/src/openvpn/ssl_mbedtls.h
index 92381f1a..0525134f 100644
--- a/src/openvpn/ssl_mbedtls.h
+++ b/src/openvpn/ssl_mbedtls.h
@@ -105,6 +105,7 @@ struct tls_root_ctx {
 #endif
     struct external_context external_key; /**< External key context */
     int *allowed_ciphers;       /**< List of allowed ciphers for this connection */
+    mbedtls_ecp_group_id *groups;     /**< List of allowed groups for this connection */
     mbedtls_x509_crt_profile cert_profile; /**< Allowed certificate types */
 };
 
diff --git a/src/openvpn/ssl_ncp.c b/src/openvpn/ssl_ncp.c
index e057a40b..4d10109c 100644
--- a/src/openvpn/ssl_ncp.c
+++ b/src/openvpn/ssl_ncp.c
@@ -233,15 +233,14 @@ ncp_get_best_cipher(const char *server_list, const char *server_cipher,
 
     char *tmp_ciphers = string_alloc(server_list, &gc_tmp);
 
-    const char *token = strsep(&tmp_ciphers, ":");
-    while (token)
+    const char *token;
+    while ((token = strsep(&tmp_ciphers, ":")))
     {
         if (tls_item_in_cipher_list(token, peer_ncp_list)
             || streq(token, remote_cipher))
         {
             break;
         }
-        token = strsep(&tmp_ciphers, ":");
     }
     /* We have not found a common cipher, as a last resort check if the
      * server cipher can be used
diff --git a/src/openvpn/ssl_openssl.c b/src/openvpn/ssl_openssl.c
index 14d52bfa..5ba74402 100644
--- a/src/openvpn/ssl_openssl.c
+++ b/src/openvpn/ssl_openssl.c
@@ -563,6 +563,57 @@ tls_ctx_set_cert_profile(struct tls_root_ctx *ctx, const char *profile)
 #endif /* ifdef HAVE_SSL_CTX_SET_SECURITY_LEVEL */
 }
 
+void
+tls_ctx_set_tls_groups(struct tls_root_ctx *ctx, const char *groups)
+{
+    ASSERT(ctx);
+    struct gc_arena gc = gc_new();
+    /* This method could be as easy as
+     *  SSL_CTX_set1_groups_list(ctx->ctx, groups)
+     * but OpenSSL does not like the name secp256r1 for prime256v1
+     * This is one of the important curves.
+     * To support the same name for OpenSSL and mbedTLS, we do
+     * this dance.
+     */
+
+    int groups_count = get_num_elements(groups, ':');
+
+    int *glist;
+    /* Allocate an array for them */
+    ALLOC_ARRAY_CLEAR_GC(glist, int, groups_count, &gc);
+
+    /* Parse allowed ciphers, getting IDs */
+    int glistlen = 0;
+    char *tmp_groups = string_alloc(groups, &gc);
+
+    const char *token;
+    while ((token = strsep(&tmp_groups, ":")))
+    {
+        if (streq(token, "secp256r1"))
+        {
+            token = "prime256v1";
+        }
+        int nid = OBJ_sn2nid(token);
+
+        if (nid == 0)
+        {
+            msg(M_WARN, "Warning unknown curve/group specified: %s", token);
+        }
+        else
+        {
+            glist[glistlen] = nid;
+            glistlen++;
+        }
+    }
+
+    if (!SSL_CTX_set1_groups(ctx->ctx, glist, glistlen))
+    {
+        crypto_msg(M_FATAL, "Failed to set allowed TLS group list: %s",
+                   groups);
+    }
+    gc_free(&gc);
+}
+
 void
 tls_ctx_check_cert_time(const struct tls_root_ctx *ctx)
 {
@@ -2135,6 +2186,8 @@ show_available_tls_ciphers_list(const char *cipher_list,
 void
 show_available_curves(void)
 {
+    printf("Consider using openssl 'ecparam -list_curves' as\n"
+           "alternative to running this command.\n");
 #ifndef OPENSSL_NO_EC
     EC_builtin_curve *curves = NULL;
     size_t crv_len = 0;
@@ -2144,7 +2197,7 @@ show_available_curves(void)
     ALLOC_ARRAY(curves, EC_builtin_curve, crv_len);
     if (EC_get_builtin_curves(curves, crv_len))
     {
-        printf("Available Elliptic curves:\n");
+        printf("\nAvailable Elliptic curves/groups:\n");
         for (n = 0; n < crv_len; n++)
         {
             const char *sname;
-- 
2.26.2

Re: [Openvpn-devel] [PATCH v6 4/9] Implement tls-groups option to specify eliptic curves/groups

[Antonio Q. <a...@un...>]

Hi,

On 21/07/2020 17:49, Arne Schwabe wrote:
> By default OpenSSL 1.1+ only allows signatures and ecdh/ecdhx from the
> default list of X25519:secp256r1:X448:secp521r1:secp384r1. In
> TLS1.3 key exchange is independent from the signature/key of the
> certificates, so allowing all groups per default is not a sensible
> choice anymore and instead a shorter list is reasonable.
> 
> However, when using certificates with exotic curves that are not on
> the group list, the signatures of these certificates will no longer
> be accepted.
> 
> The tls-groups  option allows to modify the group list to account
> for these corner cases.
> 
> Patch V2: Uses local gc_arena instead of malloc/free, reword commit
>           message. Fix other typos/clarify messages
> 
> Patch V3: Style fixes, adjust code to changes from mbed tls session
>           fix
> 
> Patch V5: Fix compilation with OpenSSL 1.0.2
> 
> Patch V6: Redo the 'while((token = strsep(&tmp_groups, ":"))' change
>           that accidently got lost.
> 
> Signed-off-by: Arne Schwabe <ar...@rf...>


Much better now.

Acked-by: Antonio Quartulli <a...@un...>




-- 
Antonio Quartulli

Re: [Openvpn-devel] [PATCH v6 4/9] Implement tls-groups option to specify eliptic curves/groups

[tincanteksup <tin...@gm...>]

8x fix - 2x suggestion

On 21/07/2020 16:49, Arne Schwabe wrote:
> By default OpenSSL 1.1+ only allows signatures and ecdh/ecdhx from the
> default list of X25519:secp256r1:X448:secp521r1:secp384r1. In
> TLS1.3 key exchange is independent from the signature/key of the
> certificates, so allowing all groups per default is not a sensible
> choice anymore and instead a shorter list is reasonable.
> 
> However, when using certificates with exotic curves that are not on
> the group list, the signatures of these certificates will no longer
> be accepted.
> 
> The tls-groups  option allows to modify the group list to account
> for these corner cases.

The tls-groups  option ->
The tls-groups option

> 
> Patch V2: Uses local gc_arena instead of malloc/free, reword commit
>            message. Fix other typos/clarify messages
> 
> Patch V3: Style fixes, adjust code to changes from mbed tls session
>            fix

mbed tls ->
mbedTLS

> 
> Patch V5: Fix compilation with OpenSSL 1.0.2
> 
> Patch V6: Redo the 'while((token = strsep(&tmp_groups, ":"))' change
>            that accidently got lost.

that accidently ->
which accidentally

> 
> Signed-off-by: Arne Schwabe <ar...@rf...>
> ---
>   README.ec                               |  7 ++--
>   configure.ac                            |  1 +
>   doc/man-sections/encryption-options.rst |  6 +--
>   doc/man-sections/tls-options.rst        | 27 +++++++++++-
>   src/openvpn/openssl_compat.h            |  6 +++
>   src/openvpn/options.c                   | 10 ++++-
>   src/openvpn/options.h                   |  1 +
>   src/openvpn/ssl.c                       |  6 +++
>   src/openvpn/ssl_backend.h               | 10 +++++
>   src/openvpn/ssl_mbedtls.c               | 45 ++++++++++++++++++++
>   src/openvpn/ssl_mbedtls.h               |  1 +
>   src/openvpn/ssl_ncp.c                   |  5 +--
>   src/openvpn/ssl_openssl.c               | 55 ++++++++++++++++++++++++-
>   13 files changed, 168 insertions(+), 12 deletions(-)
> 
> diff --git a/README.ec b/README.ec
> index 32938017..61f23b2e 100644
> --- a/README.ec
> +++ b/README.ec
> @@ -12,14 +12,15 @@ OpenVPN 2.4.0 and newer automatically initialize ECDH parameters. When ECDSA is
>   used for authentication, the curve used for the server certificate will be used
>   for ECDH too. When autodetection fails (e.g. when using RSA certificates)
>   OpenVPN lets the crypto library decide if possible, or falls back to the
> -secp384r1 curve.
> +secp384r1 curve. The list of groups/curves that the crypto library will choose
> +from can be set with the --tls-groups <grouplist> option.
>   
>   An administrator can force an OpenVPN/OpenSSL server to use a specific curve
>   using the --ecdh-curve <curvename> option with one of the curves listed as
> -available by the --show-curves option. Clients will use the same curve as
> +available by the --show-groups option. Clients will use the same curve as
>   selected by the server.
>   
> -Note that not all curves listed by --show-curves are available for use with TLS;
> +Note that not all curves listed by --show-groups are available for use with TLS;
>   in that case connecting will fail with a 'no shared cipher' TLS error.
>   
>   Authentication (ECDSA)
> diff --git a/configure.ac b/configure.ac
> index 02cb0567..f8279924 100644
> --- a/configure.ac
> +++ b/configure.ac
> @@ -929,6 +929,7 @@ if test "${with_crypto_library}" = "openssl"; then
>   			OpenSSL_version \
>   			SSL_CTX_get_default_passwd_cb \
>   			SSL_CTX_get_default_passwd_cb_userdata \
> +			SSL_CTX_set1_groups \
>   			SSL_CTX_set_security_level \
>   			X509_get0_notBefore \
>   			X509_get0_notAfter \
> diff --git a/doc/man-sections/encryption-options.rst b/doc/man-sections/encryption-options.rst
> index a59f636c..ee34f14e 100644
> --- a/doc/man-sections/encryption-options.rst
> +++ b/doc/man-sections/encryption-options.rst
> @@ -27,9 +27,9 @@ SSL Library information
>     (Standalone) Show currently available hardware-based crypto acceleration
>     engines supported by the OpenSSL library.
>   
> ---show-curves
> -  (Standalone) Show all available elliptic curves to use with the
> -  ``--ecdh-curve`` option.
> +--show-groups
> +  (Standalone) Show all available elliptic curves/groups to use with the
> +  ``--ecdh-curve`` and ``tls-groups`` options.
>   
>   Generating key material
>   -----------------------
> diff --git a/doc/man-sections/tls-options.rst b/doc/man-sections/tls-options.rst
> index 92b832cd..ccc90ac9 100644
> --- a/doc/man-sections/tls-options.rst
> +++ b/doc/man-sections/tls-options.rst
> @@ -338,6 +338,31 @@ certificates and keys: https://github.com/OpenVPN/easy-rsa
>     Use ``--tls-crypt`` instead if you want to use the key file to not only
>     authenticate, but also encrypt the TLS control channel.
>   
> +--tls-groups list
> +    A list of allowable groups/curves in order of preference.
> +
> +    Set the allowed elictipic curves/groups for the TLS session.

elictipic ->
elliptic

> +    These groups are  allowed to be used in signatures and key exchange.
> +
> +    mbed TLS currently allows all known curves per default.

mbedTLS

> +
> +    OpenSSL 1.1+ restricts the list per default to
> +    ::
> +
> +      "X25519:secp256r1:X448:secp521r1:secp384r1".
> +
> +    If you use certificates that use non-standard curves, you
> +    might need to add them here. If you do not force the ecdh curve
> +    by using ``--ecdh-curve``, the groups for ecdh will also be picked
> +    from this list.
> +
> +    OpenVPN maps the curve name `secp256r1` to `prime256v1` to allow
> +    specifying the same tls-groups option for mbed TLS and OpenSSL.

mbedTLS

> +
> +    Warning: this option not only affects eliptic curve certificates

elliptic

> +    but also the key exchange in TLS 1.3 and using this option improperly
> +    will disable TLS 1.3.
> +
>   --tls-cert-profile profile
>     Set the allowed cryptographic algorithms for certificates according to
>     ``profile``.
> @@ -368,7 +393,7 @@ certificates and keys: https://github.com/OpenVPN/easy-rsa
>     OpenVPN will migrate to 'preferred' as default in the future. Please
>     ensure that your keys already comply.
>   
> -*WARNING:* ``--tls-ciphers`` and ``--tls-ciphersuites``
> +*WARNING:* ``--tls-ciphers``, ``--tls-ciphersuites`` and ``tls-groups``
>       These options are expert features, which - if used correctly - can
>       improve the security of your VPN connection. But it is also easy to
>       unwittingly use them to carefully align a gun with your foot, or just
> diff --git a/src/openvpn/openssl_compat.h b/src/openvpn/openssl_compat.h
> index d35251fb..eb6c9c90 100644
> --- a/src/openvpn/openssl_compat.h
> +++ b/src/openvpn/openssl_compat.h
> @@ -183,6 +183,12 @@ SSL_CTX_get_default_passwd_cb(SSL_CTX *ctx)
>   }
>   #endif
>   
> +/* This function is implemented as macro, so the configure check for the

as a macro (not important)

> + * function may fail, so we check for both variants here */
> +#if !defined(HAVE_SSL_CTX_SET1_GROUPS) && !defined(SSL_CTX_set1_groups)
> +#define SSL_CTX_set1_groups SSL_CTX_set1_curves
> +#endif
> +
>   #if !defined(HAVE_X509_GET0_PUBKEY)
>   /**
>    * Get the public key from a X509 certificate
> diff --git a/src/openvpn/options.c b/src/openvpn/options.c
> index 94308a8e..7da04b6f 100644
> --- a/src/openvpn/options.c
> +++ b/src/openvpn/options.c
> @@ -8057,7 +8057,7 @@ add_option(struct options *options,
>           VERIFY_PERMISSION(OPT_P_GENERAL);
>           options->show_tls_ciphers = true;
>       }
> -    else if (streq(p[0], "show-curves") && !p[1])
> +    else if ((streq(p[0], "show-curves") || streq(p[0], "show-groups")) && !p[1])
>       {
>           VERIFY_PERMISSION(OPT_P_GENERAL);
>           options->show_curves = true;
> @@ -8065,6 +8065,9 @@ add_option(struct options *options,
>       else if (streq(p[0], "ecdh-curve") && p[1] && !p[2])
>       {
>           VERIFY_PERMISSION(OPT_P_GENERAL);
> +        msg(M_WARN, "Consider setting groups/curves preference with "
> +            "tls-groups instead of forcing a specific curve with "
> +            "ecdh-curve.");
>           options->ecdh_curve = p[1];
>       }
>       else if (streq(p[0], "tls-server") && !p[1])
> @@ -8235,6 +8238,11 @@ add_option(struct options *options,
>           VERIFY_PERMISSION(OPT_P_GENERAL);
>           options->cipher_list_tls13 = p[1];
>       }
> +    else if (streq(p[0], "tls-groups") && p[1] && !p[2])
> +    {
> +        VERIFY_PERMISSION(OPT_P_GENERAL);
> +        options->tls_groups = p[1];
> +    }
>       else if (streq(p[0], "crl-verify") && p[1] && ((p[2] && streq(p[2], "dir"))
>                                                      || !p[2]))
>       {
> diff --git a/src/openvpn/options.h b/src/openvpn/options.h
> index c37006d3..1b038c91 100644
> --- a/src/openvpn/options.h
> +++ b/src/openvpn/options.h
> @@ -542,6 +542,7 @@ struct options
>       bool pkcs12_file_inline;
>       const char *cipher_list;
>       const char *cipher_list_tls13;
> +    const char *tls_groups;
>       const char *tls_cert_profile;
>       const char *ecdh_curve;
>       const char *tls_verify;
> diff --git a/src/openvpn/ssl.c b/src/openvpn/ssl.c
> index 04d78a81..00b97352 100644
> --- a/src/openvpn/ssl.c
> +++ b/src/openvpn/ssl.c
> @@ -630,6 +630,12 @@ init_ssl(const struct options *options, struct tls_root_ctx *new_ctx)
>       tls_ctx_restrict_ciphers(new_ctx, options->cipher_list);
>       tls_ctx_restrict_ciphers_tls13(new_ctx, options->cipher_list_tls13);
>   
> +    /* Set the allow groups/curves for TLS if we want to override them */
> +    if (options->tls_groups)
> +    {
> +        tls_ctx_set_tls_groups(new_ctx, options->tls_groups);
> +    }
> +
>       if (!tls_ctx_set_options(new_ctx, options->ssl_flags))
>       {
>           goto err;
> diff --git a/src/openvpn/ssl_backend.h b/src/openvpn/ssl_backend.h
> index a1770bd4..75692797 100644
> --- a/src/openvpn/ssl_backend.h
> +++ b/src/openvpn/ssl_backend.h
> @@ -198,6 +198,16 @@ void tls_ctx_restrict_ciphers_tls13(struct tls_root_ctx *ctx, const char *cipher
>    */
>   void tls_ctx_set_cert_profile(struct tls_root_ctx *ctx, const char *profile);
>   
> +/**
> + * Set the allowed (eliptic curve) group allowed for signatures and

elliptic (spell checked also missed this)

Extra allowed .. not important.

> + * key exchange.
> + *
> + * @param ctx       TLS context to restrict, must be valid.
> + * @param groups    List of groups that will be allowed, in priority,
> + *                  separated by :
> + */
> +void tls_ctx_set_tls_groups(struct tls_root_ctx *ctx, const char *groups);
> +
>   /**
>    * Check our certificate notBefore and notAfter fields, and warn if the cert is
>    * either not yet valid or has expired.  Note that this is a non-fatal error,
> diff --git a/src/openvpn/ssl_mbedtls.c b/src/openvpn/ssl_mbedtls.c
> index 977ff5c3..9c874788 100644
> --- a/src/openvpn/ssl_mbedtls.c
> +++ b/src/openvpn/ssl_mbedtls.c
> @@ -176,6 +176,11 @@ tls_ctx_free(struct tls_root_ctx *ctx)
>               free(ctx->allowed_ciphers);
>           }
>   
> +        if (ctx->groups)
> +        {
> +            free(ctx->groups);
> +        }
> +
>           CLEAR(*ctx);
>   
>           ctx->initialised = false;
> @@ -343,6 +348,41 @@ tls_ctx_set_cert_profile(struct tls_root_ctx *ctx, const char *profile)
>       }
>   }
>   
> +void
> +tls_ctx_set_tls_groups(struct tls_root_ctx *ctx, const char *groups)
> +{
> +    ASSERT(ctx);
> +    struct gc_arena gc = gc_new();
> +
> +    /* Get number of groups and allocate an array in ctx */
> +    int groups_count = get_num_elements(groups, ':');
> +    ALLOC_ARRAY_CLEAR(ctx->groups, mbedtls_ecp_group_id, groups_count + 1)
> +
> +    /* Parse allowed ciphers, getting IDs */
> +    int i = 0;
> +    char *tmp_groups = string_alloc(groups, &gc);
> +
> +    const char *token;
> +    while ((token = strsep(&tmp_groups, ":")))
> +    {
> +        const mbedtls_ecp_curve_info *ci =
> +            mbedtls_ecp_curve_info_from_name(token);
> +        if (!ci)
> +        {
> +            msg(M_WARN, "Warning unknown curve/group specified: %s", token);
> +        }
> +        else
> +        {
> +            ctx->groups[i] = ci->grp_id;
> +            i++;
> +        }
> +    }
> +    ctx->groups[i] = MBEDTLS_ECP_DP_NONE;
> +
> +    gc_free(&gc);
> +}
> +
> +
>   void
>   tls_ctx_check_cert_time(const struct tls_root_ctx *ctx)
>   {
> @@ -1043,6 +1083,11 @@ key_state_ssl_init(struct key_state_ssl *ks_ssl,
>           mbedtls_ssl_conf_ciphersuites(ks_ssl->ssl_config, ssl_ctx->allowed_ciphers);
>       }
>   
> +    if (ssl_ctx->groups)
> +    {
> +        mbedtls_ssl_conf_curves(ks_ssl->ssl_config, ssl_ctx->groups);
> +    }
> +
>       /* Disable record splitting (for now).  OpenVPN assumes records are sent
>        * unfragmented, and changing that will require thorough review and
>        * testing.  Since OpenVPN is not susceptible to BEAST, we can just
> diff --git a/src/openvpn/ssl_mbedtls.h b/src/openvpn/ssl_mbedtls.h
> index 92381f1a..0525134f 100644
> --- a/src/openvpn/ssl_mbedtls.h
> +++ b/src/openvpn/ssl_mbedtls.h
> @@ -105,6 +105,7 @@ struct tls_root_ctx {
>   #endif
>       struct external_context external_key; /**< External key context */
>       int *allowed_ciphers;       /**< List of allowed ciphers for this connection */
> +    mbedtls_ecp_group_id *groups;     /**< List of allowed groups for this connection */
>       mbedtls_x509_crt_profile cert_profile; /**< Allowed certificate types */
>   };
>   
> diff --git a/src/openvpn/ssl_ncp.c b/src/openvpn/ssl_ncp.c
> index e057a40b..4d10109c 100644
> --- a/src/openvpn/ssl_ncp.c
> +++ b/src/openvpn/ssl_ncp.c
> @@ -233,15 +233,14 @@ ncp_get_best_cipher(const char *server_list, const char *server_cipher,
>   
>       char *tmp_ciphers = string_alloc(server_list, &gc_tmp);
>   
> -    const char *token = strsep(&tmp_ciphers, ":");
> -    while (token)
> +    const char *token;
> +    while ((token = strsep(&tmp_ciphers, ":")))
>       {
>           if (tls_item_in_cipher_list(token, peer_ncp_list)
>               || streq(token, remote_cipher))
>           {
>               break;
>           }
> -        token = strsep(&tmp_ciphers, ":");
>       }
>       /* We have not found a common cipher, as a last resort check if the
>        * server cipher can be used
> diff --git a/src/openvpn/ssl_openssl.c b/src/openvpn/ssl_openssl.c
> index 14d52bfa..5ba74402 100644
> --- a/src/openvpn/ssl_openssl.c
> +++ b/src/openvpn/ssl_openssl.c
> @@ -563,6 +563,57 @@ tls_ctx_set_cert_profile(struct tls_root_ctx *ctx, const char *profile)
>   #endif /* ifdef HAVE_SSL_CTX_SET_SECURITY_LEVEL */
>   }
>   
> +void
> +tls_ctx_set_tls_groups(struct tls_root_ctx *ctx, const char *groups)
> +{
> +    ASSERT(ctx);
> +    struct gc_arena gc = gc_new();
> +    /* This method could be as easy as
> +     *  SSL_CTX_set1_groups_list(ctx->ctx, groups)
> +     * but OpenSSL does not like the name secp256r1 for prime256v1
> +     * This is one of the important curves.
> +     * To support the same name for OpenSSL and mbedTLS, we do
> +     * this dance.
> +     */
> +
> +    int groups_count = get_num_elements(groups, ':');
> +
> +    int *glist;
> +    /* Allocate an array for them */
> +    ALLOC_ARRAY_CLEAR_GC(glist, int, groups_count, &gc);
> +
> +    /* Parse allowed ciphers, getting IDs */
> +    int glistlen = 0;
> +    char *tmp_groups = string_alloc(groups, &gc);
> +
> +    const char *token;
> +    while ((token = strsep(&tmp_groups, ":")))
> +    {
> +        if (streq(token, "secp256r1"))
> +        {
> +            token = "prime256v1";
> +        }
> +        int nid = OBJ_sn2nid(token);
> +
> +        if (nid == 0)
> +        {
> +            msg(M_WARN, "Warning unknown curve/group specified: %s", token);
> +        }
> +        else
> +        {
> +            glist[glistlen] = nid;
> +            glistlen++;
> +        }
> +    }
> +
> +    if (!SSL_CTX_set1_groups(ctx->ctx, glist, glistlen))
> +    {
> +        crypto_msg(M_FATAL, "Failed to set allowed TLS group list: %s",
> +                   groups);
> +    }
> +    gc_free(&gc);
> +}
> +
>   void
>   tls_ctx_check_cert_time(const struct tls_root_ctx *ctx)
>   {
> @@ -2135,6 +2186,8 @@ show_available_tls_ciphers_list(const char *cipher_list,
>   void
>   show_available_curves(void)
>   {
> +    printf("Consider using openssl 'ecparam -list_curves' as\n"
> +           "alternative to running this command.\n");
>   #ifndef OPENSSL_NO_EC
>       EC_builtin_curve *curves = NULL;
>       size_t crv_len = 0;
> @@ -2144,7 +2197,7 @@ show_available_curves(void)
>       ALLOC_ARRAY(curves, EC_builtin_curve, crv_len);
>       if (EC_get_builtin_curves(curves, crv_len))
>       {
> -        printf("Available Elliptic curves:\n");
> +        printf("\nAvailable Elliptic curves/groups:\n");
>           for (n = 0; n < crv_len; n++)
>           {
>               const char *sname;
> 

[Openvpn-devel] [PATCH applied] Re: Implement tls-groups option to specify eliptic curves/groups

[Gert D. <ge...@gr...>]

Your patch has been applied to the master branch.

I have not actually tested EC functionality in any way, just
made sure it compiles and passes basic (scripted) testing.

commit 8353ae8075fb25d1935258a2f007e024c5e2c43f
Author: Arne Schwabe
Date:   Tue Jul 21 17:49:22 2020 +0200

     Implement tls-groups option to specify eliptic curves/groups

     Signed-off-by: Arne Schwabe <ar...@rf...>
     Acked-by: Antonio Quartulli <an...@op...>
     Message-Id: <202...@rf...>
     URL: https://www.mail-archive.com/ope...@li.../msg20521.html
     Signed-off-by: Gert Doering <ge...@gr...>


--
kind regards,

Gert Doering

Re: [Openvpn-devel] [PATCH 8/9] Rename ncp-ciphers to data-ciphers

[David S. <op...@sf...>]

On 17/07/2020 15:47, Arne Schwabe wrote:
> The change in name signals that data-ciphers is the preferred way to
> configure data channel (and not --cipher). The data prefix is chosen
> to avoid ambiguity and make it distinct from tls-cipher for the TLS
> ciphers.
> 
> Signed-off-by: Arne Schwabe <ar...@rf...>
> ---
>  Changes.rst                            | 13 ++++++++++---
>  doc/man-sections/protocol-options.rst  | 11 +++++++----
>  doc/man-sections/server-options.rst    |  4 ++--
>  sample/sample-config-files/client.conf |  2 +-
>  src/openvpn/multi.c                    |  4 ++--
>  src/openvpn/options.c                  |  5 +++--
>  src/openvpn/ssl_ncp.c                  |  4 ++--
>  7 files changed, 27 insertions(+), 16 deletions(-)
> 
[...snip...]
> diff --git a/src/openvpn/options.c b/src/openvpn/options.c
> index 31e33ae3..896abcde 100644
> --- a/src/openvpn/options.c
> +++ b/src/openvpn/options.c
> @@ -536,7 +536,7 @@ static const char usage_message[] =
>      "--cipher alg    : Encrypt packets with cipher algorithm alg\n"
>      "                  (default=%s).\n"
>      "                  Set alg=none to disable encryption.\n"
> -    "--ncp-ciphers list : List of ciphers that are allowed to be negotiated.\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"
> @@ -7866,7 +7866,8 @@ add_option(struct options *options,
>          VERIFY_PERMISSION(OPT_P_NCP|OPT_P_INSTANCE);
>          options->ciphername = p[1];
>      }
> -    else if (streq(p[0], "ncp-ciphers") && p[1] && !p[2])
> +    else if ((streq(p[0], "data-ciphers") || streq(p[0], "ncp-ciphers"))
> +            && p[1] && !p[2])

I do agree to using --data-ciphers instead of --ncp-ciphers, that is far more
user-friendly naming of this feature.  NCP is a more technical
"under-the-hood" terminology which users don't really need to relate to, where
--data-ciphers better explains what it is used for.

But I do reject NOT adding a deprecation path for --ncp-ciphers.  We should
support --ncp-ciphers for 1-2 major releases, but after that it should be
removed.  We have too many options and we certainly should avoid duplicating
options with the exact same functionality.


-- 
kind regards,

David Sommerseth
OpenVPN Inc

Re: [Openvpn-devel] [PATCH 8/9] Rename ncp-ciphers to data-ciphers

[Arne S. <ar...@rf...>]

>> +++ b/src/openvpn/options.c
>> @@ -536,7 +536,7 @@ static const char usage_message[] =
>>      "--cipher alg    : Encrypt packets with cipher algorithm alg\n"
>>      "                  (default=%s).\n"
>>      "                  Set alg=none to disable encryption.\n"
>> -    "--ncp-ciphers list : List of ciphers that are allowed to be negotiated.\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"
>> @@ -7866,7 +7866,8 @@ add_option(struct options *options,
>>          VERIFY_PERMISSION(OPT_P_NCP|OPT_P_INSTANCE);
>>          options->ciphername = p[1];
>>      }
>> -    else if (streq(p[0], "ncp-ciphers") && p[1] && !p[2])
>> +    else if ((streq(p[0], "data-ciphers") || streq(p[0], "ncp-ciphers"))
>> +            && p[1] && !p[2])
> 
> I do agree to using --data-ciphers instead of --ncp-ciphers, that is far more
> user-friendly naming of this feature.  NCP is a more technical
> "under-the-hood" terminology which users don't really need to relate to, where
> --data-ciphers better explains what it is used for.
> 
> But I do reject NOT adding a deprecation path for --ncp-ciphers.  We should
> support --ncp-ciphers for 1-2 major releases, but after that it should be
> removed.  We have too many options and we certainly should avoid duplicating
> options with the exact same functionality.

This was a deliberate decision. We really want to people to move towards
ncp and putting another hurdle with having an option that works better
on but gives a warning and a option that does not work on 2.4 does not
help here. If we decide that really aliases are a no-go in OpenVPN then
I would rather drop data-ciphers and stay with ncp-ciphers forever for
this reason.

Arne

Re: [Openvpn-devel] [PATCH 8/9] Rename ncp-ciphers to data-ciphers

[David S. <op...@sf...>]

On 23/07/2020 13:36, Arne Schwabe wrote:
> 
>>> +++ b/src/openvpn/options.c
>>> @@ -536,7 +536,7 @@ static const char usage_message[] =
>>>      "--cipher alg    : Encrypt packets with cipher algorithm alg\n"
>>>      "                  (default=%s).\n"
>>>      "                  Set alg=none to disable encryption.\n"
>>> -    "--ncp-ciphers list : List of ciphers that are allowed to be negotiated.\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"
>>> @@ -7866,7 +7866,8 @@ add_option(struct options *options,
>>>          VERIFY_PERMISSION(OPT_P_NCP|OPT_P_INSTANCE);
>>>          options->ciphername = p[1];
>>>      }
>>> -    else if (streq(p[0], "ncp-ciphers") && p[1] && !p[2])
>>> +    else if ((streq(p[0], "data-ciphers") || streq(p[0], "ncp-ciphers"))
>>> +            && p[1] && !p[2])
>>
>> I do agree to using --data-ciphers instead of --ncp-ciphers, that is far more
>> user-friendly naming of this feature.  NCP is a more technical
>> "under-the-hood" terminology which users don't really need to relate to, where
>> --data-ciphers better explains what it is used for.
>>
>> But I do reject NOT adding a deprecation path for --ncp-ciphers.  We should
>> support --ncp-ciphers for 1-2 major releases, but after that it should be
>> removed.  We have too many options and we certainly should avoid duplicating
>> options with the exact same functionality.

Just let me clarify .... the "1-2 major releases" was too hasty and not well
thought numbers.  Please read my reasoning below for what I really intended to
say.

> This was a deliberate decision. We really want to people to move towards
> ncp and putting another hurdle with having an option that works better
> on but gives a warning and a option that does not work on 2.4 does not
> help here. If we decide that really aliases are a no-go in OpenVPN then
> I would rather drop data-ciphers and stay with ncp-ciphers forever for
> this reason.

Lets take a few steps back try to see a broader picture.

* --ncp-ciphers was introduced in OpenVPN 2.4 as a brand new option.

* Steffan has suggested to add --data-ciphers alias into the next v2.4
  release; to which I agree(!).

* I propose we add a warning whenever --ncp-ciphers used, recommending the
  user to switch to --data-ciphers as soon as possible.

* We keep both --ncp-ciphers *and* --data-ciphers in v2.5 and v2.6.

* When v2.5 is released v2.4 goes into "old stable support" - where both
  alternatives will work.  v2.4 is guaranteed to be supported by the community
  for at least 6 months with full support [0] after v2.5 is released.

* When 2.6 is released, v2.4 goes into "git tree only" support but will have a

  12 month "old stable support" guarantee [0].

* 12 months *after* the v2.6 release is out, we can remove --ncp-ciphers.  But
  since we will not do option changes mid-release easily, it might be natural
  to remove this in OpenVPN 2.7 at earliest.

This means --ncp-ciphers will be supported in 2.4 for the life time of that
release.  And v2.4 is supported for *at least* 18 months after v2.5 is
released.  It also guarantees --ncp-ciphers will first be removed when we
release OpenVPN 2.7.

What would be a problem with such a schedule?  Because I don't understand the
real objection you have to remove options which ends up duplicating other options.


At the same time ... it is also important for me that we try to see this at
from an even bigger perspective than just --ncp-ciphers/--data-ciphers or even
--udp-mtu alone.  Now I'm shifting the discussion to a more generic product
life cycle perspective.

If we skimp through this and just allow whatever option duplications we head
into, just because we're too concerned about various breakages with old
configurations or setups - it will not be the last time we might end up in
such discussions UNLESS we can find a proper and reasonable process for
deprecation (which we in fact already defined for feature deprecation 10 years
ago! [1]).

If we cannot remove options during the whole life time of OpenVPN, we cannot
touch compression options or possibly even deprecate any compression at all -
because we need to support both compression and decompression for the whole
lifetime of OpenVPN.  This also extends into how to ensure proper OpenVPN 3
compatibility as well.

And if we cannot remove any options during the eternal life time of OpenVPN, I
will see no other alternative to being even more critical and rejective to add
new options.  We already have pretty close to 300 options in OpenVPN.  That is
an insane amount - where a typical common OpenVPN setup might need up to 20 of
these options, the rest are for all kinds of special cases.  And we have
several competing solutions which are far simpler in many aspects which new
users might just as well prefer.

Even though I highlight the number of options we have, I do NOT say that we
should swipe them all out and reduce the amount to 50 or so; for some users
they have a _real_ value and provides really useful features.  But I want us
to save the options which do have a REAL value, not because unsupported
OpenVPN versions might break or "10 bytes extra source code" is too heavy to
carry around for an alias option.  I'm arguing for keeping options covering
_REAL_ USE CASE for users.  And no, breaking unsupported OpenVPN releases is
NOT a real use case from my point of view.

But back to the deprecated options  ... If we cannot remove options, we also
need to consider bringing back the --tls-remote option, and --compat-names -
both with the capability to break client configs (in fact, it already did for
several Fedora EPEL users [2]).  The proposal to remove --remote-nopull needs
to be reconsidered, as well as --secret, --max-routes, and possibly even
--ncp-disable.  Maybe even more of these deprecated options needs to be
reconsidered [3].


We really need a proper and sane processes to allow the development of OpenVPN
to have a chance to move on and leave things behind when appropriate, to be
able to evolve and grow with the future - without being strangled by what
existed in the far past (meaning: no longer community supported releases).
Otherwise I do fear for the future of OpenVPN 2.x.

By having a clear strategy and adhering to a process of feature/option
management in OpenVPN, we give clearly defined time-window for stability and
functionality for our users.  This predictability is, in my experience, much
more important to users than if a specifically named option is supported or not.



[0] <https://community.openvpn.net/openvpn/wiki/SupportedVersions>
[1] <https://community.openvpn.net/openvpn/wiki/FRP>
[2]
<https://src.fedoraproject.org/rpms/openvpn/c/c738486b3576df4829c9cef5ccd12c10c4fb7b84?branch=epel7>
[3] <https://community.openvpn.net/openvpn/wiki/DeprecatedOptions>


-- 
kind regards,

David Sommerseth
OpenVPN Inc

Re: [Openvpn-devel] [PATCH 8/9] Rename ncp-ciphers to data-ciphers

[Steffan K. <st...@ka...>]

Hi,

On 23-07-2020 18:09, David Sommerseth wrote:
>> This was a deliberate decision. We really want to people to move towards
>> ncp and putting another hurdle with having an option that works better
>> on but gives a warning and a option that does not work on 2.4 does not
>> help here. If we decide that really aliases are a no-go in OpenVPN then
>> I would rather drop data-ciphers and stay with ncp-ciphers forever for
>> this reason.
> 
> Lets take a few steps back try to see a broader picture.
> 
> [..snip..]
> 
> We really need a proper and sane processes to allow the development of OpenVPN
> to have a chance to move on and leave things behind when appropriate, to be
> able to evolve and grow with the future - without being strangled by what
> existed in the far past (meaning: no longer community supported releases).
> Otherwise I do fear for the future of OpenVPN 2.x.
> 
> By having a clear strategy and adhering to a process of feature/option
> management in OpenVPN, we give clearly defined time-window for stability and
> functionality for our users.  This predictability is, in my experience, much
> more important to users than if a specifically named option is supported or not.

Yes, you've made these points clear earlier on IRC. I (and with me Arne
and Gert) just don't agree with you on some of the details, resulting in
a different verdict on this patch.

None of us has trouble with deprecating options. We appreciate the work
you've put into the DeprecatedOptions page, and all of us have sent
and/or acked patched to remove dangerous or obsolete options.

This difference is in how we weigh the pros and cons per option. So I'll
leave the broader picture for now, and summarize why I'm going to ACK
Arne's patch exactly because is *doesn't* print a warning when the old
name is used.

Option name aliases add negligible code complexity and are trivial to
maintain. (Just look at --udp-mtu.) Keeping them in allows users to
write configs that work well and do not produce any warnings on both
older and newer versions. (Printing warnings for harmless things reduces
the value of the other warnings we print.)

Let's focus our time and effort on reducing actual complexity.

-Steffan

Re: [Openvpn-devel] [PATCH 8/9] Rename ncp-ciphers to data-ciphers

[Steffan K. <st...@ka...>]

Hi,

On 17-07-2020 15:47, Arne Schwabe wrote:
> The change in name signals that data-ciphers is the preferred way to
> configure data channel (and not --cipher). The data prefix is chosen
> to avoid ambiguity and make it distinct from tls-cipher for the TLS
> ciphers.
> 
> Signed-off-by: Arne Schwabe <ar...@rf...>
> ---
>  Changes.rst                            | 13 ++++++++++---
>  doc/man-sections/protocol-options.rst  | 11 +++++++----
>  doc/man-sections/server-options.rst    |  4 ++--
>  sample/sample-config-files/client.conf |  2 +-
>  src/openvpn/multi.c                    |  4 ++--
>  src/openvpn/options.c                  |  5 +++--
>  src/openvpn/ssl_ncp.c                  |  4 ++--
>  7 files changed, 27 insertions(+), 16 deletions(-)
> 
> diff --git a/Changes.rst b/Changes.rst
> index 6e283270..2158c8e7 100644
> --- a/Changes.rst
> +++ b/Changes.rst
> @@ -14,12 +14,19 @@ ChaCha20-Poly1305 cipher support
>      channel.
>  
>  Improved Data channel cipher negotiation
> +    The option ``ncp-ciphers`` has been renamed to ``data-ciphers``.
> +    The old name is still accepted. The change in name signals that
> +    ``data-ciphers`` is the preferred way to configure data channel
> +    ciphers and the data prefix is chosen to avoid the ambiguity that
> +    exists with ``--cipher`` for the data cipher and ``tls-cipher``
> +    for the TLS ciphers.
> +
>      OpenVPN clients will now signal all supported ciphers from the
> -    ``ncp-ciphers`` option to the server via ``IV_CIPHERS``. OpenVPN
> -    servers will select the first common cipher from the ``ncp-ciphers``
> +    ``data-ciphers`` option to the server via ``IV_CIPHERS``. OpenVPN
> +    servers will select the first common cipher from the ``data-ciphers``
>      list instead of blindly pushing the first cipher of the list. This
>      allows to use a configuration like
> -    ``ncp-ciphers ChaCha20-Poly1305:AES-256-GCM`` on the server that
> +    ``data-ciphers ChaCha20-Poly1305:AES-256-GCM`` on the server that
>      prefers ChaCha20-Poly1305 but uses it only if the client supports it.
>  
>  Asynchronous (deferred) authentication support for auth-pam plugin.
> diff --git a/doc/man-sections/protocol-options.rst b/doc/man-sections/protocol-options.rst
> index 923d2da0..051f1d32 100644
> --- a/doc/man-sections/protocol-options.rst
> +++ b/doc/man-sections/protocol-options.rst
> @@ -62,7 +62,7 @@ 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 ``--ncp-ciphers`` and
> +  upgrade to :code:`AES-256-GCM`.  See ``--data-ciphers`` and
>    ``--ncp-disable`` for more details on NCP.
>  
>    Using :code:`BF-CBC` is no longer recommended, because of its 64-bit
> @@ -169,7 +169,7 @@ configured in a compatible way between both the local and remote side.
>    non-standard key lengths, and a larger key may offer no real guarantee
>    of greater security, or may even reduce security.
>  
> ---ncp-ciphers cipher-list
> +--data-ciphers cipher-list
>    Restrict the allowed ciphers to be negotiated to the ciphers in
>    ``cipher-list``. ``cipher-list`` is a colon-separated list of ciphers,
>    and defaults to :code:`AES-256-GCM:AES-128-GCM`.
> @@ -189,9 +189,9 @@ configured in a compatible way between both the local and remote side.
>    Additionally, to allow for more smooth transition, if NCP is enabled,
>    OpenVPN will inherit the cipher of the peer if that cipher is different
>    from the local ``--cipher`` setting, but the peer cipher is one of the
> -  ciphers specified in ``--ncp-ciphers``. E.g. a non-NCP client (<=v2.3,
> +  ciphers specified in ``--data-ciphers``. E.g. a non-NCP client (<=v2.3,
>    or with --ncp-disabled set) connecting to a NCP server (v2.4+) with
> -  ``--cipher BF-CBC`` and ``--ncp-ciphers AES-256-GCM:AES-256-CBC`` set can
> +  ``--cipher BF-CBC`` and ``--data-ciphers AES-256-GCM:AES-256-CBC`` set can
>    either specify ``--cipher BF-CBC`` or ``--cipher AES-256-CBC`` and both
>    will work.
>  
> @@ -201,6 +201,9 @@ configured in a compatible way between both the local and remote side.
>    This list is restricted to be 127 chars long after conversion to OpenVPN
>    ciphers.
>  
> +  This option was called ``ncp-ciphers`` in OpenVPN 2.4 but has been renamed
> +  to ``data-ciphers`` in OpenVPN 2.5 to more accurately reflect its meaning.
> +
>  --ncp-disable
>    Disable "Negotiable Crypto Parameters". This completely disables cipher
>    negotiation.
> diff --git a/doc/man-sections/server-options.rst b/doc/man-sections/server-options.rst
> index c24aec0b..74ad5e18 100644
> --- a/doc/man-sections/server-options.rst
> +++ b/doc/man-sections/server-options.rst
> @@ -473,8 +473,8 @@ fast hardware. SSL/TLS authentication must be used in this mode.
>          *AES-GCM-128* and *AES-GCM-256*.
>  
>    :code:`IV_CIPHERS=<ncp-ciphers>`
> -        The client pushes the list of configured ciphers with the
> -        ``--ciphers`` option to the server.
> +        The client announces the list of supported ciphers configured with the
> +        ``--data-ciphers`` option to the server.
>  
>    :code:`IV_GUI_VER=<gui_id> <version>`
>          The UI version of a UI if one is running, for example
> diff --git a/sample/sample-config-files/client.conf b/sample/sample-config-files/client.conf
> index 7f2f30a3..47ca4099 100644
> --- a/sample/sample-config-files/client.conf
> +++ b/sample/sample-config-files/client.conf
> @@ -112,7 +112,7 @@ tls-auth ta.key 1
>  # then you must also specify it here.
>  # Note that v2.4 client/server will automatically
>  # negotiate AES-256-GCM in TLS mode.
> -# See also the ncp-cipher option in the manpage
> +# See also the data-ciphers option in the manpage
>  cipher AES-256-CBC
>  
>  # Enable compression on the VPN link.
> diff --git a/src/openvpn/multi.c b/src/openvpn/multi.c
> index 88ba9db2..d2549bca 100644
> --- a/src/openvpn/multi.c
> +++ b/src/openvpn/multi.c
> @@ -1827,7 +1827,7 @@ multi_client_set_protocol_options(struct context *c)
>          else
>          {
>              /*
> -             * Push the first cipher from --ncp-ciphers to the client that
> +             * Push the first cipher from --data-ciphers to the client that
>               * the client announces to be supporting.
>               */
>              char *push_cipher = ncp_get_best_cipher(o->ncp_ciphers, o->ciphername,
> @@ -1847,7 +1847,7 @@ multi_client_set_protocol_options(struct context *c)
>                  {
>                      msg(M_INFO, "PUSH: No common cipher between server and "
>                          "client. Expect this connection not to work. Server "
> -                        "ncp-ciphers: '%s', client supported ciphers '%s'",
> +                        "data-ciphers: '%s', client supported ciphers '%s'",
>                          o->ncp_ciphers, peer_ciphers);
>                  }
>                  else
> diff --git a/src/openvpn/options.c b/src/openvpn/options.c
> index 31e33ae3..896abcde 100644
> --- a/src/openvpn/options.c
> +++ b/src/openvpn/options.c
> @@ -536,7 +536,7 @@ static const char usage_message[] =
>      "--cipher alg    : Encrypt packets with cipher algorithm alg\n"
>      "                  (default=%s).\n"
>      "                  Set alg=none to disable encryption.\n"
> -    "--ncp-ciphers list : List of ciphers that are allowed to be negotiated.\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"
> @@ -7866,7 +7866,8 @@ add_option(struct options *options,
>          VERIFY_PERMISSION(OPT_P_NCP|OPT_P_INSTANCE);
>          options->ciphername = p[1];
>      }
> -    else if (streq(p[0], "ncp-ciphers") && p[1] && !p[2])
> +    else if ((streq(p[0], "data-ciphers") || streq(p[0], "ncp-ciphers"))
> +            && p[1] && !p[2])
>      {
>          VERIFY_PERMISSION(OPT_P_GENERAL|OPT_P_INSTANCE);
>          options->ncp_ciphers = p[1];
> diff --git a/src/openvpn/ssl_ncp.c b/src/openvpn/ssl_ncp.c
> index e057a40b..6760884e 100644
> --- a/src/openvpn/ssl_ncp.c
> +++ b/src/openvpn/ssl_ncp.c
> @@ -111,7 +111,7 @@ mutate_ncp_cipher_list(const char *list, struct gc_arena *gc)
>          const cipher_kt_t *ktc = cipher_kt_get(token);
>          if (!ktc)
>          {
> -            msg(M_WARN, "Unsupported cipher in --ncp-ciphers: %s", token);
> +            msg(M_WARN, "Unsupported cipher in --data-ciphers: %s", token);
>              error_found = true;
>          }
>          else
> @@ -130,7 +130,7 @@ mutate_ncp_cipher_list(const char *list, struct gc_arena *gc)
>              if (!(buf_forward_capacity(&new_list) >
>                    strlen(ovpn_cipher_name) + 2))
>              {
> -                msg(M_WARN, "Length of --ncp-ciphers is over the "
> +                msg(M_WARN, "Length of --data-ciphers is over the "
>                      "limit of 127 chars");
>                  error_found = true;
>              }
> 

Thanks. Patch looks good, and passes manual tests.

Acked-by: Steffan Karger <st...@ka...>

-Steffan

Re: [Openvpn-devel] [PATCH 8/9] Rename ncp-ciphers to data-ciphers

[David S. <op...@sf...>]

On 24/07/2020 10:14, Steffan Karger wrote:
> Hi,
> 
> On 17-07-2020 15:47, Arne Schwabe wrote:
>> The change in name signals that data-ciphers is the preferred way to
>> configure data channel (and not --cipher). The data prefix is chosen
>> to avoid ambiguity and make it distinct from tls-cipher for the TLS
>> ciphers.
>>
>> Signed-off-by: Arne Schwabe <ar...@rf...>
>> ---
>>  Changes.rst                            | 13 ++++++++++---
>>  doc/man-sections/protocol-options.rst  | 11 +++++++----
>>  doc/man-sections/server-options.rst    |  4 ++--
>>  sample/sample-config-files/client.conf |  2 +-
>>  src/openvpn/multi.c                    |  4 ++--
>>  src/openvpn/options.c                  |  5 +++--
>>  src/openvpn/ssl_ncp.c                  |  4 ++--
>>  7 files changed, 27 insertions(+), 16 deletions(-)
>>
>> diff --git a/Changes.rst b/Changes.rst
>> index 6e283270..2158c8e7 100644
>> --- a/Changes.rst
>> +++ b/Changes.rst
>> @@ -14,12 +14,19 @@ ChaCha20-Poly1305 cipher support
>>      channel.
>>  
>>  Improved Data channel cipher negotiation
>> +    The option ``ncp-ciphers`` has been renamed to ``data-ciphers``.
>> +    The old name is still accepted. The change in name signals that
>> +    ``data-ciphers`` is the preferred way to configure data channel
>> +    ciphers and the data prefix is chosen to avoid the ambiguity that
>> +    exists with ``--cipher`` for the data cipher and ``tls-cipher``
>> +    for the TLS ciphers.
>> +
>>      OpenVPN clients will now signal all supported ciphers from the
>> -    ``ncp-ciphers`` option to the server via ``IV_CIPHERS``. OpenVPN
>> -    servers will select the first common cipher from the ``ncp-ciphers``
>> +    ``data-ciphers`` option to the server via ``IV_CIPHERS``. OpenVPN
>> +    servers will select the first common cipher from the ``data-ciphers``
>>      list instead of blindly pushing the first cipher of the list. This
>>      allows to use a configuration like
>> -    ``ncp-ciphers ChaCha20-Poly1305:AES-256-GCM`` on the server that
>> +    ``data-ciphers ChaCha20-Poly1305:AES-256-GCM`` on the server that
>>      prefers ChaCha20-Poly1305 but uses it only if the client supports it.
>>  
>>  Asynchronous (deferred) authentication support for auth-pam plugin.
>> diff --git a/doc/man-sections/protocol-options.rst b/doc/man-sections/protocol-options.rst
>> index 923d2da0..051f1d32 100644
>> --- a/doc/man-sections/protocol-options.rst
>> +++ b/doc/man-sections/protocol-options.rst
>> @@ -62,7 +62,7 @@ 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 ``--ncp-ciphers`` and
>> +  upgrade to :code:`AES-256-GCM`.  See ``--data-ciphers`` and
>>    ``--ncp-disable`` for more details on NCP.
>>  
>>    Using :code:`BF-CBC` is no longer recommended, because of its 64-bit
>> @@ -169,7 +169,7 @@ configured in a compatible way between both the local and remote side.
>>    non-standard key lengths, and a larger key may offer no real guarantee
>>    of greater security, or may even reduce security.
>>  
>> ---ncp-ciphers cipher-list
>> +--data-ciphers cipher-list
>>    Restrict the allowed ciphers to be negotiated to the ciphers in
>>    ``cipher-list``. ``cipher-list`` is a colon-separated list of ciphers,
>>    and defaults to :code:`AES-256-GCM:AES-128-GCM`.
>> @@ -189,9 +189,9 @@ configured in a compatible way between both the local and remote side.
>>    Additionally, to allow for more smooth transition, if NCP is enabled,
>>    OpenVPN will inherit the cipher of the peer if that cipher is different
>>    from the local ``--cipher`` setting, but the peer cipher is one of the
>> -  ciphers specified in ``--ncp-ciphers``. E.g. a non-NCP client (<=v2.3,
>> +  ciphers specified in ``--data-ciphers``. E.g. a non-NCP client (<=v2.3,
>>    or with --ncp-disabled set) connecting to a NCP server (v2.4+) with
>> -  ``--cipher BF-CBC`` and ``--ncp-ciphers AES-256-GCM:AES-256-CBC`` set can
>> +  ``--cipher BF-CBC`` and ``--data-ciphers AES-256-GCM:AES-256-CBC`` set can
>>    either specify ``--cipher BF-CBC`` or ``--cipher AES-256-CBC`` and both
>>    will work.
>>  
>> @@ -201,6 +201,9 @@ configured in a compatible way between both the local and remote side.
>>    This list is restricted to be 127 chars long after conversion to OpenVPN
>>    ciphers.
>>  
>> +  This option was called ``ncp-ciphers`` in OpenVPN 2.4 but has been renamed
>> +  to ``data-ciphers`` in OpenVPN 2.5 to more accurately reflect its meaning.
>> +
>>  --ncp-disable
>>    Disable "Negotiable Crypto Parameters". This completely disables cipher
>>    negotiation.
>> diff --git a/doc/man-sections/server-options.rst b/doc/man-sections/server-options.rst
>> index c24aec0b..74ad5e18 100644
>> --- a/doc/man-sections/server-options.rst
>> +++ b/doc/man-sections/server-options.rst
>> @@ -473,8 +473,8 @@ fast hardware. SSL/TLS authentication must be used in this mode.
>>          *AES-GCM-128* and *AES-GCM-256*.
>>  
>>    :code:`IV_CIPHERS=<ncp-ciphers>`
>> -        The client pushes the list of configured ciphers with the
>> -        ``--ciphers`` option to the server.
>> +        The client announces the list of supported ciphers configured with the
>> +        ``--data-ciphers`` option to the server.
>>  
>>    :code:`IV_GUI_VER=<gui_id> <version>`
>>          The UI version of a UI if one is running, for example
>> diff --git a/sample/sample-config-files/client.conf b/sample/sample-config-files/client.conf
>> index 7f2f30a3..47ca4099 100644
>> --- a/sample/sample-config-files/client.conf
>> +++ b/sample/sample-config-files/client.conf
>> @@ -112,7 +112,7 @@ tls-auth ta.key 1
>>  # then you must also specify it here.
>>  # Note that v2.4 client/server will automatically
>>  # negotiate AES-256-GCM in TLS mode.
>> -# See also the ncp-cipher option in the manpage
>> +# See also the data-ciphers option in the manpage
>>  cipher AES-256-CBC
>>  
>>  # Enable compression on the VPN link.
>> diff --git a/src/openvpn/multi.c b/src/openvpn/multi.c
>> index 88ba9db2..d2549bca 100644
>> --- a/src/openvpn/multi.c
>> +++ b/src/openvpn/multi.c
>> @@ -1827,7 +1827,7 @@ multi_client_set_protocol_options(struct context *c)
>>          else
>>          {
>>              /*
>> -             * Push the first cipher from --ncp-ciphers to the client that
>> +             * Push the first cipher from --data-ciphers to the client that
>>               * the client announces to be supporting.
>>               */
>>              char *push_cipher = ncp_get_best_cipher(o->ncp_ciphers, o->ciphername,
>> @@ -1847,7 +1847,7 @@ multi_client_set_protocol_options(struct context *c)
>>                  {
>>                      msg(M_INFO, "PUSH: No common cipher between server and "
>>                          "client. Expect this connection not to work. Server "
>> -                        "ncp-ciphers: '%s', client supported ciphers '%s'",
>> +                        "data-ciphers: '%s', client supported ciphers '%s'",
>>                          o->ncp_ciphers, peer_ciphers);
>>                  }
>>                  else
>> diff --git a/src/openvpn/options.c b/src/openvpn/options.c
>> index 31e33ae3..896abcde 100644
>> --- a/src/openvpn/options.c
>> +++ b/src/openvpn/options.c
>> @@ -536,7 +536,7 @@ static const char usage_message[] =
>>      "--cipher alg    : Encrypt packets with cipher algorithm alg\n"
>>      "                  (default=%s).\n"
>>      "                  Set alg=none to disable encryption.\n"
>> -    "--ncp-ciphers list : List of ciphers that are allowed to be negotiated.\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"
>> @@ -7866,7 +7866,8 @@ add_option(struct options *options,
>>          VERIFY_PERMISSION(OPT_P_NCP|OPT_P_INSTANCE);
>>          options->ciphername = p[1];
>>      }
>> -    else if (streq(p[0], "ncp-ciphers") && p[1] && !p[2])
>> +    else if ((streq(p[0], "data-ciphers") || streq(p[0], "ncp-ciphers"))
>> +            && p[1] && !p[2])
>>      {
>>          VERIFY_PERMISSION(OPT_P_GENERAL|OPT_P_INSTANCE);
>>          options->ncp_ciphers = p[1];
>> diff --git a/src/openvpn/ssl_ncp.c b/src/openvpn/ssl_ncp.c
>> index e057a40b..6760884e 100644
>> --- a/src/openvpn/ssl_ncp.c
>> +++ b/src/openvpn/ssl_ncp.c
>> @@ -111,7 +111,7 @@ mutate_ncp_cipher_list(const char *list, struct gc_arena *gc)
>>          const cipher_kt_t *ktc = cipher_kt_get(token);
>>          if (!ktc)
>>          {
>> -            msg(M_WARN, "Unsupported cipher in --ncp-ciphers: %s", token);
>> +            msg(M_WARN, "Unsupported cipher in --data-ciphers: %s", token);
>>              error_found = true;
>>          }
>>          else
>> @@ -130,7 +130,7 @@ mutate_ncp_cipher_list(const char *list, struct gc_arena *gc)
>>              if (!(buf_forward_capacity(&new_list) >
>>                    strlen(ovpn_cipher_name) + 2))
>>              {
>> -                msg(M_WARN, "Length of --ncp-ciphers is over the "
>> +                msg(M_WARN, "Length of --data-ciphers is over the "
>>                      "limit of 127 chars");
>>                  error_found = true;
>>              }
>>
> 
> Thanks. Patch looks good, and passes manual tests.
> 
> Acked-by: Steffan Karger <st...@ka...>

NAK.


-- 
kind regards,

David Sommerseth
OpenVPN Inc

Re: [Openvpn-devel] [PATCH 8/9] Rename ncp-ciphers to data-ciphers

[Arne S. <ar...@rf...>]

First of all I did not want to reply to this since we had a lengthy
discussion on IRC.

> Lets take a few steps back try to see a broader picture.
> 
> * --ncp-ciphers was introduced in OpenVPN 2.4 as a brand new option.
> 
> * Steffan has suggested to add --data-ciphers alias into the next v2.4
>   release; to which I agree(!).

Yes, this sounds like a simple idea but also has the danger of creating
more confusion.

I feel it better to have ncp-cipher and data-ciphers coexist. Then you
also have the understand that ncp-cipher is an older/less capable
version of data-ciphers.

OpenVPN 2.4 has a number of oddities in NCP support:

- the client always announces AES-GCM support even if disabled in
ncp-ciphers
- the server will always push the first cipher of its ncp-ciphers list
to the client.


From a user perspective data-ciphers is a true superset of ncp-ciphers.
All existing ncp-ciphers setup will continue to work on 2.5 but the
data-ciphers option from 2.5 is likely to break on 2.5 if you don't take
into account the 2.4 oddities.

> * I propose we add a warning whenever --ncp-ciphers used, recommending the
>   user to switch to --data-ciphers as soon as possible.

As compromise maybe something like

NOTE: Rewriting old-option x y z to new new-option x y z


> 
> * We keep both --ncp-ciphers *and* --data-ciphers in v2.5 and v2.6.
> 
> * When v2.5 is released v2.4 goes into "old stable support" - where both
>   alternatives will work.  v2.4 is guaranteed to be supported by the community
>   for at least 6 months with full support [0] after v2.5 is released.
> 
> * When 2.6 is released, v2.4 goes into "git tree only" support but will have a
> 
>   12 month "old stable support" guarantee [0].
> 
> * 12 months *after* the v2.6 release is out, we can remove --ncp-ciphers.  But
>   since we will not do option changes mid-release easily, it might be natural
>   to remove this in OpenVPN 2.7 at earliest.
> 
> This means --ncp-ciphers will be supported in 2.4 for the life time of that
> release.  And v2.4 is supported for *at least* 18 months after v2.5 is
> released.  It also guarantees --ncp-ciphers will first be removed when we
> release OpenVPN 2.7.
> 
> What would be a problem with such a schedule?  Because I don't understand the
> real objection you have to remove options which ends up duplicating other options.


My general problem is that I don't see a real advantage in removing
ncp-cipher but see a lot of downsides removing it.

> 
> At the same time ... it is also important for me that we try to see this at
> from an even bigger perspective than just --ncp-ciphers/--data-ciphers or even
> --udp-mtu alone.  Now I'm shifting the discussion to a more generic product
> life cycle perspective.
> 
> If we skimp through this and just allow whatever option duplications we head
> into, just because we're too concerned about various breakages with old
> configurations or setups - it will not be the last time we might end up in
> such discussions UNLESS we can find a proper and reasonable process for
> deprecation (which we in fact already defined for feature deprecation 10 years
> ago! [1]).
> 
> If we cannot remove options during the whole life time of OpenVPN, we cannot
> touch compression options or possibly even deprecate any compression at all -
> because we need to support both compression and decompression for the whole
> lifetime of OpenVPN.  This also extends into how to ensure proper OpenVPN 3
> compatibility as well.

The amount of time we spend on testing/developing etc on compression and
the complexity it introduces is vastly different from ncp-cipher

> And if we cannot remove any options during the eternal life time of OpenVPN, I
> will see no other alternative to being even more critical and rejective to add
> new options.  We already have pretty close to 300 options in OpenVPN.  That is
> an insane amount - where a typical common OpenVPN setup might need up to 20 of
> these options, the rest are for all kinds of special cases.  And we have
> several competing solutions which are far simpler in many aspects which new
> users might just as well prefer.
> 
> Even though I highlight the number of options we have, I do NOT say that we
> should swipe them all out and reduce the amount to 50 or so; for some users
> they have a _real_ value and provides really useful features.  But I want us
> to save the options which do have a REAL value, not because unsupported
> OpenVPN versions might break or "10 bytes extra source code" is too heavy to
> carry around for an alias option.  I'm arguing for keeping options covering
> _REAL_ USE CASE for users.  And no, breaking unsupported OpenVPN releases is
> NOT a real use case from my point of view.

This is the point where we disagree. I still think that we should weigh
the pros and cons if we still want have compatibility. And also to
consider if there are alternatives/workaround for the setups/users
affected.

> But back to the deprecated options  ... If we cannot remove options, we also
> need to consider bringing back the --tls-remote option, and --compat-names -
> both with the capability to break client configs (in fact, it already did for
> several Fedora EPEL users [2]).  

Well if weighing the pros of bringing it back outwights the cons, then
the decision to compeletly remove it might have been wrong.


> The proposal to remove --remote-nopull needs
> to be reconsidered, as well as --secret, --max-routes, and possibly even
> --ncp-disable.  Maybe even more of these deprecated options needs to be
> reconsidered [3].

For ncp-disable I did the weighing of pros and cons and answer that
question. ncp-disable add complexity to the code paths and is in
conflict which the direction we want to go. And for users that currently
use the option the workable workaround is to just remove that options as
it should not be necessary in 2.5 anymore.

> We really need a proper and sane processes to allow the development of OpenVPN
> to have a chance to move on and leave things behind when appropriate, to be
> able to evolve and grow with the future - without being strangled by what
> existed in the far past (meaning: no longer community supported releases).
> Otherwise I do fear for the future of OpenVPN 2.x.

There is more a to software than the development. There is also how it
is used/etc. And we should not make it harder than it needs to be. And
even for our own company internal use case of Access server the reality
is that we even though we do not like supporting the old clients (back
to 2.2 in some cases), we still have to do it because there is demand
for it.


> By having a clear strategy and adhering to a process of feature/option
> management in OpenVPN, we give clearly defined time-window for stability and
> functionality for our users.  This predictability is, in my experience, much
> more important to users than if a specifically named option is supported or not.

Yes. If we decide to remove an option we should have this predictable
removal process. But if we remove an option/drop support for something
something that should still be a weighing of pros and cons.

For this specific option of ncp-ciphers/data-ciphers. This not just a
fringe option. This is an option that affects one of the core things of
OpenVPN, the chosen chipher. I want to make the transition to NCP as
painless as possible and  keeping ncp-cipher as alias to data-cipher is
just the better choice in my opinion.

Arne

Re: [Openvpn-devel] [PATCH 8/9] Rename ncp-ciphers to data-ciphers

[David S. <op...@sf...>]

On 24/07/2020 12:45, Arne Schwabe wrote:
> First of all I did not want to reply to this since we had a lengthy
> discussion on IRC.
> 
>> Lets take a few steps back try to see a broader picture.
>>
>> * --ncp-ciphers was introduced in OpenVPN 2.4 as a brand new option.
>>
>> * Steffan has suggested to add --data-ciphers alias into the next v2.4
>>   release; to which I agree(!).
> 
> Yes, this sounds like a simple idea but also has the danger of creating
> more confusion.
> 
> I feel it better to have ncp-cipher and data-ciphers coexist. Then you
> also have the understand that ncp-cipher is an older/less capable
> version of data-ciphers.
> 
> OpenVPN 2.4 has a number of oddities in NCP support:
> 
> - the client always announces AES-GCM support even if disabled in
> ncp-ciphers
> - the server will always push the first cipher of its ncp-ciphers list
> to the client.

These oddities, they sounds like bugs, or?  If I try to put on my
Never-used-NCP-before-hat, this would not be what I would expect.  Why haven't
we resolved this within the current 2.4 scope without changing options?



> From a user perspective data-ciphers is a true superset of ncp-ciphers.
> All existing ncp-ciphers setup will continue to work on 2.5 but the
> data-ciphers option from 2.5 is likely to break on 2.5 if you don't take
> into account the 2.4 oddities.
> 
>> * I propose we add a warning whenever --ncp-ciphers used, recommending the
>>   user to switch to --data-ciphers as soon as possible.
> 
> As compromise maybe something like
> 
> NOTE: Rewriting old-option x y z to new new-option x y z

As mentioned on IRC, yes.  I am willing to accept

msg(M_INFO, "Rewriting deprecated option X to the replacement Y" ...



[...]
>> * 12 months *after* the v2.6 release is out, we can remove --ncp-ciphers.  But
>>   since we will not do option changes mid-release easily, it might be natural
>>   to remove this in OpenVPN 2.7 at earliest.
>>
>> This means --ncp-ciphers will be supported in 2.4 for the life time of that
>> release.  And v2.4 is supported for *at least* 18 months after v2.5 is
>> released.  It also guarantees --ncp-ciphers will first be removed when we
>> release OpenVPN 2.7.
>>
>> What would be a problem with such a schedule?  Because I don't understand the
>> real objection you have to remove options which ends up duplicating other options.
> 
> 
> My general problem is that I don't see a real advantage in removing
> ncp-cipher but see a lot of downsides removing it.

I hear about you seeing downsides ... but I have not seen any argument here
convincing me otherwise.  This overall process should take care of supporting
all reasonable OpenVPN 2.x versions.



>> At the same time ... it is also important for me that we try to see this at
>> from an even bigger perspective than just --ncp-ciphers/--data-ciphers or even
>> --udp-mtu alone.  Now I'm shifting the discussion to a more generic product
>> life cycle perspective.
>>
>> If we skimp through this and just allow whatever option duplications we head
>> into, just because we're too concerned about various breakages with old
>> configurations or setups - it will not be the last time we might end up in
>> such discussions UNLESS we can find a proper and reasonable process for
>> deprecation (which we in fact already defined for feature deprecation 10 years
>> ago! [1]).
>>
>> If we cannot remove options during the whole life time of OpenVPN, we cannot
>> touch compression options or possibly even deprecate any compression at all -
>> because we need to support both compression and decompression for the whole
>> lifetime of OpenVPN.  This also extends into how to ensure proper OpenVPN 3
>> compatibility as well.
> 
> The amount of time we spend on testing/developing etc on compression and
> the complexity it introduces is vastly different from ncp-cipher

That is true.  But at the same time, if we do not have a proper deprecation
process where we draw the line in which OpenVPN versions we are willing to
support (from a functional perspective), we cannot touch compression.

Which is why I'm trying to raise the points of a bigger picture.  We need to
find a reasonable solution to which OpenVPN versions we are willing to
support, and when those versions enters the unsupported side we should be free
to cleanup code and options related to the behavior and functionality only
these now not supported releases provided.



>> And if we cannot remove any options during the eternal life time of OpenVPN, I
>> will see no other alternative to being even more critical and rejective to add
>> new options.  We already have pretty close to 300 options in OpenVPN.  That is
>> an insane amount - where a typical common OpenVPN setup might need up to 20 of
>> these options, the rest are for all kinds of special cases.  And we have
>> several competing solutions which are far simpler in many aspects which new
>> users might just as well prefer.
>>
>> Even though I highlight the number of options we have, I do NOT say that we
>> should swipe them all out and reduce the amount to 50 or so; for some users
>> they have a _real_ value and provides really useful features.  But I want us
>> to save the options which do have a REAL value, not because unsupported
>> OpenVPN versions might break or "10 bytes extra source code" is too heavy to
>> carry around for an alias option.  I'm arguing for keeping options covering
>> _REAL_ USE CASE for users.  And no, breaking unsupported OpenVPN releases is
>> NOT a real use case from my point of view.
> 
> This is the point where we disagree. I still think that we should weigh
> the pros and cons if we still want have compatibility. And also to
> consider if there are alternatives/workaround for the setups/users
> affected.

This is again the same issue, to be able to draw the line somewhere to what we
are willing to support.  What kind of life cycles do we want for OpenVPN?



>> But back to the deprecated options  ... If we cannot remove options, we also
>> need to consider bringing back the --tls-remote option, and --compat-names -
>> both with the capability to break client configs (in fact, it already did for
>> several Fedora EPEL users [2]).  
> 
> Well if weighing the pros of bringing it back outwights the cons, then
> the decision to compeletly remove it might have been wrong.

So the question is ... do we care about broken OpenVPN 2.3 (or older)
configurations today?  Which is technically supported by us another 12 months
after the v2.5 release.



>> The proposal to remove --remote-nopull needs
>> to be reconsidered, as well as --secret, --max-routes, and possibly even
>> --ncp-disable.  Maybe even more of these deprecated options needs to be
>> reconsidered [3].
> 
> For ncp-disable I did the weighing of pros and cons and answer that
> question. ncp-disable add complexity to the code paths and is in
> conflict which the direction we want to go. And for users that currently
> use the option the workable workaround is to just remove that options as
> it should not be necessary in 2.5 anymore.

But it still will break users configs without any prior warning in 2.4.  It
was added to our git master only in 15 days ago.  Including potential client
configurations.

Again, this exemplifies that we are lacking a proper product life cycle
process and even adhering to a feature deprecation process.



>> We really need a proper and sane processes to allow the development of OpenVPN
>> to have a chance to move on and leave things behind when appropriate, to be
>> able to evolve and grow with the future - without being strangled by what
>> existed in the far past (meaning: no longer community supported releases).
>> Otherwise I do fear for the future of OpenVPN 2.x.
> 
> There is more a to software than the development. There is also how it
> is used/etc. And we should not make it harder than it needs to be. And
> even for our own company internal use case of Access server the reality
> is that we even though we do not like supporting the old clients (back
> to 2.2 in some cases), we still have to do it because there is demand
> for it.

Why Access Server supports such old OpenVPN releases is a completely different
discussion not belong here in the community.  There might be valid use cases
for these customers, but in such situations additional (and often fairly
expensive) support contracts are required by the vast majority of vendor
agreements I've seen.  And these corporate special needs should not end up in
the community.

The community needs to be decoupled to be fully functional.  I say this as a
OpenVPN Inc hire - and has also said this internally to the management over
the years I've been there.  From what I experience, there is a general
understanding for the need of this separation.



>> By having a clear strategy and adhering to a process of feature/option
>> management in OpenVPN, we give clearly defined time-window for stability and
>> functionality for our users.  This predictability is, in my experience, much
>> more important to users than if a specifically named option is supported or not.
> 
> Yes. If we decide to remove an option we should have this predictable
> removal process. But if we remove an option/drop support for something
> something that should still be a weighing of pros and cons.
> 
> For this specific option of ncp-ciphers/data-ciphers. This not just a
> fringe option. This is an option that affects one of the core things of
> OpenVPN, the chosen chipher. I want to make the transition to NCP as
> painless as possible and  keeping ncp-cipher as alias to data-cipher is
> just the better choice in my opinion.

I agree that --data-cipher is a better name for it; I have not rejected that.

But that doesn't mean we should have two thoughts active at the same time: a)
NCP improvements, and b) product life cycle, what we support and how we
support OpenVPN in the long run ... in this case they do impact each other.

But when we only focus on a) without properly considering b), that is the
point where I object.


-- 
kind regards,

David Sommerseth
OpenVPN Inc

[Openvpn-devel] [PATCH 10/10] Add a note that ncp-ciphers is replaced by data-ciphers

[Arne S. <ar...@rf...>]

This patch adds a message that informs the user that the ncp-cipher
is renamed to data-ciphers. This should address the following concerns:

 - Users being confused by old options.
 - Nudge users to use the modern variant of an option

The man page already documents ncp-ciphers as an old name for
data-ciphers, so looking it up in the man page will also work.

Note that I did not add "deprecated old option" to this message
since I still think that eventually removing the option will only
break configs and we gain almost nothing from that.

Also still accepting the option even though we do not recommend usage of
it also follows the robustness principle of:
"be strict in what you send and tolerant in what you receive"

Signed-off-by: Arne Schwabe <ar...@rf...>
---
 src/openvpn/options.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/src/openvpn/options.c b/src/openvpn/options.c
index 5beaba0f..01f0ca0f 100644
--- a/src/openvpn/options.c
+++ b/src/openvpn/options.c
@@ -7939,6 +7939,11 @@ add_option(struct options *options,
             && p[1] && !p[2])
     {
         VERIFY_PERMISSION(OPT_P_GENERAL|OPT_P_INSTANCE);
+        if (streq(p[0], "ncp-ciphers"))
+        {
+            msg(M_INFO, "Note: Rewriting option '--ncp-ciphers' to "
+                        " '--data-ciphers'");
+        }
         options->ncp_ciphers = p[1];
     }
     else if (streq(p[0], "ncp-disable") && !p[1])
-- 
2.26.2

Re: [Openvpn-devel] [PATCH 10/10] Add a note that ncp-ciphers is replaced by data-ciphers

[Arne S. <ar...@rf...>]

Am 24.07.20 um 16:04 schrieb Arne Schwabe:
> This patch adds a message that informs the user that the ncp-cipher
> is renamed to data-ciphers. This should address the following concerns:
> 
>  - Users being confused by old options.
>  - Nudge users to use the modern variant of an option
> 
> The man page already documents ncp-ciphers as an old name for
> data-ciphers, so looking it up in the man page will also work.
> 
> Note that I did not add "deprecated old option" to this message
> since I still think that eventually removing the option will only
> break configs and we gain almost nothing from that.
> 
> Also still accepting the option even though we do not recommend usage of
> it also follows the robustness principle of:
> "be strict in what you send and tolerant in what you receive"
> 
> Signed-off-by: Arne Schwabe <ar...@rf...>
> ---
>  src/openvpn/options.c | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/src/openvpn/options.c b/src/openvpn/options.c
> index 5beaba0f..01f0ca0f 100644
> --- a/src/openvpn/options.c
> +++ b/src/openvpn/options.c
> @@ -7939,6 +7939,11 @@ add_option(struct options *options,
>              && p[1] && !p[2])
>      {
>          VERIFY_PERMISSION(OPT_P_GENERAL|OPT_P_INSTANCE);
> +        if (streq(p[0], "ncp-ciphers"))
> +        {
> +            msg(M_INFO, "Note: Rewriting option '--ncp-ciphers' to "
> +                        " '--data-ciphers'");
> +        }
>          options->ncp_ciphers = p[1];
>      }
>      else if (streq(p[0], "ncp-disable") && !p[1])
> 

Sorry, send out an old version. V2 incoming.

Arne

[Openvpn-devel] [PATCH v2 10/10] Add a note that ncp-ciphers is replaced by data-ciphers

[Arne S. <ar...@rf...>]

This patch adds a message that informs the user that the ncp-cipher
is renamed to data-ciphers. This should address the following concerns:

 - Users being confused by old options.
 - Nudge users to use the modern variant of an option

The man page already documents ncp-ciphers as an old name for
data-ciphers, so looking it up in the man page will also work.

Note that I did not add "deprecated old option" to this message
since I still think that eventually removing the option will only
break configs and we gain almost nothing from that.

Also still accepting the option even though we do not recommend usage of
it also follows the robustness principle of:
"be strict in what you send and tolerant in what you receive"

Signed-off-by: Arne Schwabe <ar...@rf...>
---
 src/openvpn/options.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/src/openvpn/options.c b/src/openvpn/options.c
index 5beaba0f..2cff9473 100644
--- a/src/openvpn/options.c
+++ b/src/openvpn/options.c
@@ -7939,7 +7939,11 @@ add_option(struct options *options,
             && p[1] && !p[2])
     {
         VERIFY_PERMISSION(OPT_P_GENERAL|OPT_P_INSTANCE);
+        if (streq(p[0], "ncp-ciphers"))
+        {
+            msg(M_INFO, "Note: Treating option '--ncp-ciphers' as "
+                        " '--data-ciphers' (renamed in OpenVPN 2.5).");
+        }
         options->ncp_ciphers = p[1];
     }
     else if (streq(p[0], "ncp-disable") && !p[1])
-- 
2.26.2

[Openvpn-devel] [PATCH applied] Re: Add a note that ncp-ciphers is replaced by data-ciphers

[Gert D. <ge...@gr...>]

Acked-by: Gert Doering <ge...@gr...>

>From the discussions and mails in the 8/9 thread, I understand that
this is acceptable to everyone involved.  So, move forward with 
improving our common project!

Code looks good, and compiles.  Even tested it.  Works.

Your patch has been applied to the master branch.

commit 342f9b78f107530f67581d6247d38af0792e4243
Author: Arne Schwabe
Date:   Fri Jul 24 16:25:57 2020 +0200

     Add a note that ncp-ciphers is replaced by data-ciphers

     Signed-off-by: Arne Schwabe <ar...@rf...>
     Acked-by: Gert Doering <ge...@gr...>
     Message-Id: <202...@rf...>
     URL: https://www.mail-archive.com/ope...@li.../msg20573.html
     Signed-off-by: Gert Doering <ge...@gr...>


--
kind regards,

Gert Doering

Re: [Openvpn-devel] [PATCH 9/9] Rework NCP compability logic and drop BF-CBC support by default

[Steffan K. <st...@ka...>]

Hi,

This is awesome in many ways. Better behaviour, better code and a nice
way forward to really get rid of the BF-CBC default cipher.

It's also somewhat tricky, so here goes for a review purely based on
stare-at-code:

On 17-07-2020 15:47, Arne Schwabe wrote:
> This reworks the NCP logic to be more strict about what is
> considered an acceptable result of an NCP negotiation. It also
> us to finally drop BF-CBC support by default.
> 
> All new behaviour is currently limited to server/client
> mode with pull enabled. P2p mode without pull does not change.
> 
> New Server behaviour:
> - when a client announces its supported ciphers through either
>   OCC or IV_CIPHER/IV_NCP we reject the client with a
>   AUTH_FAILED message if we have no common cipher.
> 
> - When a client does not announce any cipher in either
>   OCC or NCP we by reject it unless fallback-cipher is
>   specified in either ccd or config.
> 
> New client behaviour:
> - When no cipher is pushed (or a cipher we refused to support)
>   and we also cannot support the server's server announced in
>   OCC we fail the connection and log why
> 
> - If fallback-cipher is specified we will in the case that
>   cipher is missing from occ use the fallback cipher instead
>   of failing the connection
> 
> Both client and server behaviour:
> - We only announce --cipher xyz in occ if we are willing
>   to support that cipher.
> 
>   It means that we only announce the fallback-cipher if
>   it is also contained in --data-ciphers
> 
> Compatiblity behaviour:
> 
> In 2.5 both client and server will automatically set
> fallback-cipher xyz if --cipher xyz is in the config and
> also add append the cipher to the end of data-ciphers.
> 
> We log a warn user about this and point to --data-ciphers and
> --fallback-cipher. This also happens if the configuration
> contains an explicit --cipher BF-CBC.
> 
> If --cipher is not set, we only warn that previous versions
> allowed BF-CBC and point how to reenable BF-CBC. This might
> break very few config where someone connects a very old
> client to a 2.5 server but at some point we need to drop
> the BF-CBC and those affected use will already have a the
> scary SWEET32 warning in their logs.
> 
> In short: If --cipher is explicitly set 2.6 will work the same as
> 2.4 did. When --cipher is not set, BF-CBC support is dropped and
> we warn about it.
> 
> Examples how breaking the default BF-CBC will be logged:
> 
> Client side:
>  - Client connecting to server that does not push cipher but
>    has --cipher in OCC
> 
>     OPTIONS ERROR: failed to negotiate cipher with server.  Add the server's cipher ('BF-CBC') to --data-ciphers (currently 'AES-256-GCM:AES-128-CBC') if you want to connect to this server.
> 
>  - Client connecting to a server that does not support OCC:
> 
>    OPTIONS ERROR: failed to negotioate cipher with server. Configure --fallback-cipher if you want connect to this server.
> 
> Server Side:
> 
> - Server has a client only supporting BF-CBC connecting:> diff --git a/doc/man-sections/protocol-options.rst
b/doc/man-sections/protocol-options.rst

[ Used the output of "git diff -w" to review, so pasting that here is
"original" too. ]

> diff --git a/doc/man-sections/protocol-options.rst b/doc/man-sections/protocol-options.rst
> index 051f1d32..ab22b415 100644
> --- a/doc/man-sections/protocol-options.rst
> +++ b/doc/man-sections/protocol-options.rst
> @@ -57,6 +57,9 @@ configured in a compatible way between both the local and remote side.
>    http://www.cs.ucsd.edu/users/mihir/papers/hmac.html
>  
>  --cipher alg
> +  This option is deprecated for server-client mode and ``--data-ciphers``
> +  or rarely `--data-ciphers-fallback`` should be used instead.
> +
>    Encrypt data channel packets with cipher algorithm ``alg``.
>  
>    The default is :code:`BF-CBC`, an abbreviation for Blowfish in Cipher
> @@ -183,8 +186,9 @@ configured in a compatible way between both the local and remote side.
>    ``--server`` ), or if ``--pull`` is specified (client-side, implied by
>    setting --client).
>  
> -  If both peers support and do not disable NCP, the negotiated cipher will
> -  override the cipher specified by ``--cipher``.
> +  If no common cipher is found is found during cipher negotiation, the

Duplicate "is found".

> +  connection is terminated. To support old clients/server that do not
> +  provide any cipher support see ``data-ciphers-fallback``.
>  
>    Additionally, to allow for more smooth transition, if NCP is enabled,
>    OpenVPN will inherit the cipher of the peer if that cipher is different
> @@ -201,8 +205,18 @@ configured in a compatible way between both the local and remote side.
>    This list is restricted to be 127 chars long after conversion to OpenVPN
>    ciphers.
>  
> -  This option was called ``ncp-ciphers`` in OpenVPN 2.4 but has been renamed
> -  to ``data-ciphers`` in OpenVPN 2.5 to more accurately reflect its meaning.
> +  This option was called ``--ncp-ciphers`` in OpenVPN 2.4 but has been renamed
> +  to ``--data-ciphers`` in OpenVPN 2.5 to more accurately reflect its meaning.
> +
> +--data-ciphers-fallback alg
> +
> +    Configure a cipher that is used to fall back to if the peer does announce
> +    the cipher in OCC.

Isn't OCC to much detail here? I'd suggest something like "if we could
not determine which cipher the peer is willing to use".

> +    This option should normally not be needed.  It only exists to allow
> +    connecting to old servers or if supporting old clients as server.
> +    The are only OpenVPN 2.3 and older version that have configured with
> +    `--enable-small`.

I find this somewhat hard to read and understand, probably because it
/is/ not so easy to understand but... A sugestion (native speakers,
please help improve!):

This option should only be needed when connecting to peers which run
OpenVPN 2.3 or earlier, and are compiled with the --enable-small flag
(typically used on routers or other embedded devices).

>  --ncp-disable
>    Disable "Negotiable Crypto Parameters". This completely disables cipher
> diff --git a/src/openvpn/crypto.c b/src/openvpn/crypto.c
> index e92a0dc1..accab850 100644
> --- a/src/openvpn/crypto.c
> +++ b/src/openvpn/crypto.c
> @@ -727,7 +727,8 @@ warn_insecure_key_type(const char *ciphername, const cipher_kt_t *cipher)
>      {
>          msg(M_WARN, "WARNING: INSECURE cipher (%s) with block size less than 128"
>              " bit (%d bit).  This allows attacks like SWEET32.  Mitigate by "
> -            "using a --cipher with a larger block size (e.g. AES-256-CBC).",
> +            "using a --cipher with a larger block size (e.g. AES-256-CBC). "
> +            "Support for these insecure cipher will be removed in OpenVPN 2.6.",

These insecure cipher*s*.

> --- a/src/openvpn/init.c
> +++ b/src/openvpn/init.c
> @@ -2374,7 +2374,30 @@ do_deferred_options(struct context *c, const unsigned int found)
>          {
>              /* 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);
> +            bool useremotecipher = tls_poor_mans_ncp(&c->options,
> +                                                     c->c2.tls_multi->remote_ciphername);
> +
> +            if (!useremotecipher && !c->options.enable_ncp_fallback)
> +            {
> +                /* Give appropiate error message */
> +                if (c->c2.tls_multi->remote_ciphername)
> +                {
> +                    msg(D_TLS_ERRORS, "OPTIONS ERROR: failed to negotiate "
> +                        "cipher with server.  Add the server's "
> +                        "cipher ('%s') to --data-ciphers (currently '%s') if "
> +                        "you want to connect to this server.",
> +                        c->c2.tls_multi->remote_ciphername,
> +                        c->options.ncp_ciphers);
> +                }
> +                else
> +                {
> +                    msg(D_TLS_ERRORS, "OPTIONS ERROR: failed to negotioate "

Typo: negotiate.

> +                        "cipher with server. Configure "
> +                        "--data-ciphers-fallback if you want connect "

Typo: if you want *to* connect.

> +                        "to this server.");
> +                }
> +                return false;
> +            }

This block seems to use more indentation levels than needed. Consider
something like:

  else if (ncp_enabled) {
    if (!remote_ciphername) {
      msg("Failed to negotiate, configure --data-ciphers-fallback");
      return false;
    }
    if (!tls_poor_mans_ncp) {
      msg("Failed to negotiate, add cipher xxx to --data-ciphers");
      return false;
    }
  }

>          }
>          struct frame *frame_fragment = NULL;
>  #ifdef ENABLE_FRAGMENT
> diff --git a/src/openvpn/multi.c b/src/openvpn/multi.c
> index 9bda38b0..a1f65127 100644
> --- a/src/openvpn/multi.c
> +++ b/src/openvpn/multi.c
> @@ -1777,7 +1777,7 @@ multi_client_connect_setenv(struct multi_context *m,
>   * - choosen cipher
>   * - peer id
>   */
> -static void
> +static bool
>  multi_client_set_protocol_options(struct context *c)
>  {
>  
> @@ -1807,8 +1807,11 @@ multi_client_set_protocol_options(struct context *c)
>      }
>  
>      /* Select cipher if client supports Negotiable Crypto Parameters */
> -    if (o->ncp_enabled)
> +    if (!o->ncp_enabled)
>      {
> +        return true;
> +    }
> +

Hm, in this case I don't think this improves things. This turns

  if (foo) {
    do_foo_stuff;
  }
  if (bar) {
    do_bar_stuff;
  }

into

  if (foo) {
    do_foo_stuff;
  }
  if (!bar) {
    return;
  }
  do_bar_stuff;

I think the orignal code flow is easier to understand. If there's too
much indentation for you liking, you can split part of it into a
separate function.

(This is just about this first if, I totally agree about the flow change
below.)

>      /* 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
>       * (so the client is always told what we expect it to use)
> @@ -1820,43 +1823,69 @@ multi_client_set_protocol_options(struct context *c)
>               "server has already generated data channel keys, "
>               "re-sending previously negotiated cipher '%s'",
>               o->ciphername );
> +        return true;
>      }
> -        else
> -        {
> +
>      /*
>       * Push the first cipher from --data-ciphers to the client that
>       * the client announces to be supporting.
>       */
> -            char *push_cipher = ncp_get_best_cipher(o->ncp_ciphers, o->ciphername,
> -                                                    peer_info,
> +    char *push_cipher = ncp_get_best_cipher(o->ncp_ciphers, peer_info,
>                                              tls_multi->remote_ciphername,
>                                              &o->gc);
>  
>      if (push_cipher)
>      {
>          o->ciphername = push_cipher;
> +        return true;
>      }
> -            else
> -            {
> +
> +    /* NCP cipher negotiation failed. Try to figure out why exactly it
> +     * failed and give good error messages and potentially do a fallback
> +     * for non NCP clients */
>      struct gc_arena gc = gc_new();
> +    bool ret = false;
> +
>      const char *peer_ciphers = tls_peer_ncp_list(peer_info, &gc);
> +    /* If we are in a situation where we know the client ciphers, there is no
> +     * reason to fall back to a cipher that will not be accepted by the other
> +     * side, in this situation we fail the auth*/
>      if (strlen(peer_ciphers) > 0)
>      {
> -                    msg(M_INFO, "PUSH: No common cipher between server and "
> -                        "client. Expect this connection not to work. Server "
> -                        "data-ciphers: '%s', client supported ciphers '%s'",
> +        msg(M_INFO, "PUSH: No common cipher between server and client. "
> +            "Server data-ciphers: '%s', client supported ciphers '%s'",
>              o->ncp_ciphers, peer_ciphers);
>      }
> +    else if (tls_multi->remote_ciphername)
> +    {
> +        msg(M_INFO, "PUSH: No common cipher between server and client. "
> +            "Server data-ciphers: '%s', client supports cipher '%s'",
> +            o->ncp_ciphers, tls_multi->remote_ciphername);
> +    }
>      else
>      {
> -                    msg(M_INFO, "No NCP data received from peer, falling back "
> -                        "to --cipher '%s'. Peer reports in OCC --cipher '%s'",
> -                        o->ciphername, np(tls_multi->remote_ciphername));
> +        msg(M_INFO, "PUSH: No NCP or OCC cipher data received from peer.");
> +
> +        if (o->enable_ncp_fallback && !tls_multi->remote_ciphername)
> +        {
> +            msg(M_INFO, "Using data channel cipher '%s' since "
> +                "--data-ciphers-fallback is set.", o->ciphername);
> +            ret = true;
>          }
> -                gc_free(&gc);
> +        else
> +        {
> +            msg(M_INFO, "Use --data-ciphers-fallback with the cipher the "
> +                "client is using if you want to allow the client to connect");
>          }
>      }
> +    if (!ret)
> +    {
> +        auth_set_client_reason(tls_multi, "Data channel cipher negotiation "
> +                                          "failed (no shared cipher)");
>      }
> +
> +    gc_free(&gc);
> +    return ret;
>  }
>  
>  /**
> @@ -2394,13 +2423,17 @@ multi_client_connect_late_setup(struct multi_context *m,
>      mi->context.c2.context_auth = CAS_SUCCEEDED;
>  
>      /* authentication complete, calculate dynamic client specific options */
> -    multi_client_set_protocol_options(&mi->context);
> -
> -    /* Generate data channel keys */
> -    if (!multi_client_generate_tls_keys(&mi->context))
> +    if (!multi_client_set_protocol_options(&mi->context))
>      {
>          mi->context.c2.context_auth = CAS_FAILED;
>      }
> +    /* Generate data channel keys only if setting protocol options
> +     * has not failed */
> +    else if (!multi_client_generate_tls_keys(&mi->context))
> +    {
> +
> +        mi->context.c2.context_auth = CAS_FAILED;
> +    }
>  
>      /* send push reply if ready */
>      if (mi->context.c2.push_request_received)
> diff --git a/src/openvpn/options.c b/src/openvpn/options.c
> index 6d7dbf9f..5beaba0f 100644
> --- a/src/openvpn/options.c
> +++ b/src/openvpn/options.c
> @@ -855,7 +855,6 @@ init_options(struct options *o, const bool init_gc)
>  #if P2MP
>      o->scheduled_exit_interval = 5;
>  #endif
> -    o->ciphername = "BF-CBC";
>      o->ncp_enabled = true;
>      o->ncp_ciphers = "AES-256-GCM:AES-128-GCM";
>      o->authname = "SHA1";
> @@ -3041,6 +3040,69 @@ options_postprocess_verify(const struct options *o)
>      }
>  }
>  
> +static void
> +options_postprocess_cipher(struct options *o)
> +{
> +    if (!o->pull && !(o->mode == MODE_SERVER))
> +    {
> +        /* we are in the classic P2P mode */
> +        /* cipher negotiation (NCP) currently assumes --pull or --mode server */
> +        o->ncp_enabled = false;
> +        msg( M_WARN, "Dynamic cipher negioation is disabled since neither "

Typo: negoatiation. Also "dynamic" seems redundant, "negotiation"
already implies that.

As a side note: this warning message should be good enough for devs too,
so we probably don't need the comments above that say the same.

> +             "P2MP client nor server mode is enabled" );

Was already in the orignal code, but: no space needed after msg(.

> +        /* If the cipher is not set default to BF-CBC, we will warn that this
> +         * is deprecated on cipher initialisation, no need to warn here as
> +         * well */

I had to read this twice to understand meaning. Maybe something like "if
the cipher is not set, we set it to the old default value 'BF-CBC'." or so?

> +        if (!o->ciphername)
> +        {
> +            o->ciphername = "BF-CBC";
> +        }
> +        return;
> +    }
> +
> +    /* M2P mode */

I think this is "P2MP mode"?

> +    if (!o->ciphername)
> +    {
> +        msg(M_WARN, "--cipher is not set. Previous OpenVPN version defaulted to "
> +            "BF-CBC as fallback when dynamic 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";
> +
> +        if (!o->ncp_enabled)
> +        {
> +            msg(M_USAGE, "--ncp-disable needs an explicit --cipher or "
> +                "--data-ciphers-fallback config option");
> +        }
> +    }
> +    else if (!o->enable_ncp_fallback
> +             && !tls_item_in_cipher_list(o->ciphername, o->ncp_ciphers))
> +    {
> +        msg(M_WARN, "DEPRECATED OPTION: --cipher set to '%s' but missing in"
> +            " --ncp-ciphers (%s). Future OpenVPN version will "
> +            "ignore --cipher for dynamic cipher negotiations. "
> +            "Add '%s' to --data-ciphers or change --cipher '%s' to "
> +            "--data-ciphers-fallback '%s' to silence this warning.",
> +            o->ciphername, o->ncp_ciphers, o->ciphername,
> +            o->ciphername, o->ciphername);
> +        o->enable_ncp_fallback = true;
> +
> +        /* Append the --cipher to ncp_ciphers to allow it in NCP */
> +        char *ncp_ciphers = gc_malloc(strlen(o->ncp_ciphers)
> +                                      +strlen(o->ciphername) + 1, false, &o->gc);
> +
> +        strcpy(ncp_ciphers, o->ncp_ciphers);
> +        strcat(ncp_ciphers, ":");
> +        strcat(ncp_ciphers, o->ciphername);

Can we please avoid using strcpy and strcat? Using openvpn_snprintf()
should result in simpler code, and makes it easier to ensure that we
won't overflow.

Actually, looks like this already overflows, since there is no space for
the null-byte in ncp_ciphers.

> +        o->ncp_ciphers = ncp_ciphers;
> +    }
> +}
> +
>  static void
>  options_postprocess_mutate(struct options *o)
>  {
> @@ -3053,6 +3115,7 @@ options_postprocess_mutate(struct options *o)
>      helper_keepalive(o);
>      helper_tcp_nodelay(o);
>  
> +    options_postprocess_cipher(o);
>      options_postprocess_mutate_invariant(o);
>  
>      if (o->ncp_enabled)
> @@ -3113,16 +3176,6 @@ options_postprocess_mutate(struct options *o)
>              "include this in your server configuration");
>          o->dh_file = NULL;
>      }
> -
> -    /* cipher negotiation (NCP) currently assumes --pull or --mode server */
> -    if (o->ncp_enabled
> -        && !(o->pull || o->mode == MODE_SERVER) )
> -    {
> -        msg( M_WARN, "disabling NCP mode (--ncp-disable) because not "
> -             "in P2MP client or server mode" );
> -        o->ncp_enabled = false;
> -    }
> -
>  #if ENABLE_MANAGEMENT
>      if (o->http_proxy_override)
>      {
> @@ -3658,14 +3711,22 @@ options_string(const struct options *o,
>       */
>  
>      buf_printf(&out, ",dev-type %s", dev_type_string(o->dev, o->dev_type));
> -    buf_printf(&out, ",link-mtu %u", (unsigned int) calc_options_string_link_mtu(o, frame));
> +    if (o->ciphername)
> +    {
> +        /* the link-mtu that we send has only a meaning if have a fixed
> +         * cipher (p2p) or have a fallback cipher for older non ncp
> +         * clients. If we do have a fallback cipher, do not send it */

This confuses me. The code reads like it's dependent on --cipher, rather
than --fallbck-cipher. Also shouldn't this be "If we *don't* have a
fallback cipher"?

> +        buf_printf(&out, ",link-mtu %u",
> +                   (unsigned int) calc_options_string_link_mtu(o, frame));
> +    }
>      buf_printf(&out, ",tun-mtu %d", PAYLOAD_SIZE(frame));
>      buf_printf(&out, ",proto %s",  proto_remote(o->ce.proto, remote));
>  
> +    bool p2p_nopull = o->mode == MODE_POINT_TO_POINT && !PULL_DEFINED(o);
>      /* send tun_ipv6 only in peer2peer mode - in client/server mode, it
>       * is usually pushed by the server, triggering a non-helpful warning
>       */
> -    if (o->ifconfig_ipv6_local && o->mode == MODE_POINT_TO_POINT && !PULL_DEFINED(o))
> +    if (o->ifconfig_ipv6_local && p2p_nopull)
>      {
>          buf_printf(&out, ",tun-ipv6");
>      }
> @@ -3695,7 +3756,7 @@ options_string(const struct options *o,
>          }
>      }
>  
> -    if (tt && o->mode == MODE_POINT_TO_POINT && !PULL_DEFINED(o))
> +    if (tt && p2p_nopull)
>      {
>          const char *ios = ifconfig_options_string(tt, remote, o->ifconfig_nowarn, gc);
>          if (ios && strlen(ios))
> @@ -3751,8 +3812,15 @@ options_string(const struct options *o,
>  
>          init_key_type(&kt, o->ciphername, o->authname, o->keysize, true,
>                        false);
> -
> -        buf_printf(&out, ",cipher %s", cipher_kt_name(kt.cipher));
> +        /* Only announce the cipher to our peer if we are willing to
> +         * support it */
> +        const char *ciphername = cipher_kt_name(kt.cipher);
> +        if (p2p_nopull || !o->ncp_enabled
> +            || (o->ncp_enabled
> +                && tls_item_in_cipher_list(ciphername, o->ncp_ciphers)))

This second check for o->ncp_enabled is not needed. You already ensured
it's true before. (Saves you a line wrap.)

I wonder though, isn't it too soon to stop sending cipher? Looks like
both 2.3 and 2.4 clients will currently print options warnings if cipher
is missing from OCC. The earlier NCP versions carefully tries to send
what the peer expected here to prevent bogus warnings.

> +        {
> +            buf_printf(&out, ",cipher %s", ciphername);
> +        }
>          buf_printf(&out, ",auth %s", md_kt_name(kt.digest));
>          buf_printf(&out, ",keysize %d", kt.cipher_length * 8);
>          if (o->shared_secret_file)
> @@ -3870,7 +3938,8 @@ options_warning_safe_scan2(const int msglevel,
>          || strprefix(p1, "keydir ")
>          || strprefix(p1, "proto ")
>          || strprefix(p1, "tls-auth ")
> -        || strprefix(p1, "tun-ipv6"))
> +        || strprefix(p1, "tun-ipv6")
> +        || strprefix(p1, "cipher "))
>      {
>          return;
>      }
> @@ -7860,6 +7929,12 @@ add_option(struct options *options,
>          VERIFY_PERMISSION(OPT_P_NCP|OPT_P_INSTANCE);
>          options->ciphername = p[1];
>      }
> +    else if (streq(p[0], "data-ciphers-fallback") && p[1] && !p[2])
> +    {
> +        VERIFY_PERMISSION(OPT_P_INSTANCE);
> +        options->ciphername = p[1];
> +        options->enable_ncp_fallback = true;
> +    }
>      else if ((streq(p[0], "data-ciphers") || streq(p[0], "ncp-ciphers"))
>              && p[1] && !p[2])
>      {
> diff --git a/src/openvpn/options.h b/src/openvpn/options.h
> index c5df2d18..877e9396 100644
> --- a/src/openvpn/options.h
> +++ b/src/openvpn/options.h
> @@ -503,6 +503,8 @@ struct options
>      bool shared_secret_file_inline;
>      int key_direction;
>      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;
> diff --git a/src/openvpn/ssl.c b/src/openvpn/ssl.c
> index 91ab3bf6..06dc9f8f 100644
> --- a/src/openvpn/ssl.c
> +++ b/src/openvpn/ssl.c
> @@ -1932,13 +1932,14 @@ tls_session_update_crypto_params(struct tls_session *session,
>          return true;
>      }
>  
> -    if (!session->opt->server
> -        && 0 != strcmp(options->ciphername, session->opt->config_ciphername)
> +    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 or %s",
> -            options->ciphername, session->opt->config_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;
> diff --git a/src/openvpn/ssl_ncp.c b/src/openvpn/ssl_ncp.c
> index 8e432fb7..528b31ea 100644
> --- a/src/openvpn/ssl_ncp.c
> +++ b/src/openvpn/ssl_ncp.c
> @@ -211,9 +211,8 @@ tls_peer_ncp_list(const char *peer_info, struct gc_arena *gc)
>  }
>  
>  char *
> -ncp_get_best_cipher(const char *server_list, const char *server_cipher,
> -                    const char *peer_info,  const char *remote_cipher,
> -                    struct gc_arena *gc)
> +ncp_get_best_cipher(const char *server_list, const char *peer_info,
> +                    const char *remote_cipher, struct gc_arena *gc)
>  {
>      /*
>       * The gc of the parameter is tied to the VPN session, create a
> @@ -242,15 +241,6 @@ ncp_get_best_cipher(const char *server_list, const char *server_cipher,
>              break;
>          }
>      }
> -    /* We have not found a common cipher, as a last resort check if the
> -     * server cipher can be used
> -     */
> -    if (token == NULL
> -        && (tls_item_in_cipher_list(server_cipher, peer_ncp_list)
> -            || streq(server_cipher, remote_cipher)))
> -    {
> -        token = server_cipher;
> -    }
>  
>      char *ret = NULL;
>      if (token != NULL)
> @@ -262,16 +252,18 @@ ncp_get_best_cipher(const char *server_list, const char *server_cipher,
>      return ret;
>  }
>  
> -void
> +bool
>  tls_poor_mans_ncp(struct options *o, const char *remote_ciphername)
>  {
> -    if (o->ncp_enabled && remote_ciphername
> +    if (remote_ciphername
>          && 0 != strcmp(o->ciphername, remote_ciphername))
>      {
>          if (tls_item_in_cipher_list(remote_ciphername, o->ncp_ciphers))
>          {
>              o->ciphername = string_alloc(remote_ciphername, &o->gc);
>              msg(D_TLS_DEBUG_LOW, "Using peer cipher '%s'", o->ciphername);
> +            return true;
>          }
>      }
> +    return false;
>  }
> diff --git a/src/openvpn/ssl_ncp.h b/src/openvpn/ssl_ncp.h
> index d00c222d..98f80286 100644
> --- a/src/openvpn/ssl_ncp.h
> +++ b/src/openvpn/ssl_ncp.h
> @@ -44,10 +44,13 @@ tls_peer_supports_ncp(const char *peer_info);
>   * "Poor man's NCP": Use peer cipher if it is an allowed (NCP) cipher.
>   * Allows non-NCP peers to upgrade their cipher individually.
>   *
> + * Returns true if we switched to the peer's cipher
> + *
>   * Make sure to call tls_session_update_crypto_params() after calling this
>   * function.
>   */
> -void tls_poor_mans_ncp(struct options *o, const char *remote_ciphername);
> +bool
> +tls_poor_mans_ncp(struct options *o, const char *remote_ciphername);

I think we almost always put the return type on the same line in header
files. (Don't know why, but it seems quite consistent.)
-Steffan

Re: [Openvpn-devel] [PATCH 9/9] Rework NCP compability logic and drop BF-CBC support by default

[tincanteksup <tin...@gm...>]

10x more wee pointers

On 28/07/2020 13:27, Steffan Karger wrote:
> Hi,
> 
> This is awesome in many ways. Better behaviour, better code and a nice
> way forward to really get rid of the BF-CBC default cipher.
> 
> It's also somewhat tricky, so here goes for a review purely based on
> stare-at-code:
> 
> On 17-07-2020 15:47, Arne Schwabe wrote:
>> This reworks the NCP logic to be more strict about what is
>> considered an acceptable result of an NCP negotiation. It also
>> us to finally drop BF-CBC support by default.
>>
>> All new behaviour is currently limited to server/client
>> mode with pull enabled. P2p mode without pull does not change.
>>
>> New Server behaviour:
>> - when a client announces its supported ciphers through either
>>    OCC or IV_CIPHER/IV_NCP we reject the client with a
>>    AUTH_FAILED message if we have no common cipher.
>>
>> - When a client does not announce any cipher in either
>>    OCC or NCP we by reject it unless fallback-cipher is
>>    specified in either ccd or config.
>>
>> New client behaviour:
>> - When no cipher is pushed (or a cipher we refused to support)
>>    and we also cannot support the server's server announced in
>>    OCC we fail the connection and log why
>>
>> - If fallback-cipher is specified we will in the case that
>>    cipher is missing from occ use the fallback cipher instead
>>    of failing the connection
>>
>> Both client and server behaviour:
>> - We only announce --cipher xyz in occ if we are willing
>>    to support that cipher.
>>
>>    It means that we only announce the fallback-cipher if
>>    it is also contained in --data-ciphers
>>
>> Compatiblity behaviour:

Compatiblity -> Compatibility



>>
>> In 2.5 both client and server will automatically set
>> fallback-cipher xyz if --cipher xyz is in the config and
>> also add append the cipher to the end of data-ciphers.
>>
>> We log a warn user about this and point to --data-ciphers and
>> --fallback-cipher. This also happens if the configuration
>> contains an explicit --cipher BF-CBC.
>>
>> If --cipher is not set, we only warn that previous versions
>> allowed BF-CBC and point how to reenable BF-CBC. This might

reenable -> re-enable
(technicality only)


>> break very few config where someone connects a very old
>> client to a 2.5 server but at some point we need to drop
>> the BF-CBC and those affected use will already have a the
>> scary SWEET32 warning in their logs.
>>
>> In short: If --cipher is explicitly set 2.6 will work the same as
>> 2.4 did. When --cipher is not set, BF-CBC support is dropped and
>> we warn about it.
>>
>> Examples how breaking the default BF-CBC will be logged:
>>
>> Client side:
>>   - Client connecting to server that does not push cipher but
>>     has --cipher in OCC
>>
>>      OPTIONS ERROR: failed to negotiate cipher with server.  Add the server's cipher ('BF-CBC') to --data-ciphers (currently 'AES-256-GCM:AES-128-CBC') if you want to connect to this server.
>>
>>   - Client connecting to a server that does not support OCC:
>>
>>     OPTIONS ERROR: failed to negotioate cipher with server. Configure --fallback-cipher if you want connect to this server.

negotioate !!!


>>
>> Server Side:
>>
>> - Server has a client only supporting BF-CBC connecting:> diff --git a/doc/man-sections/protocol-options.rst
> b/doc/man-sections/protocol-options.rst
> 
> [ Used the output of "git diff -w" to review, so pasting that here is
> "original" too. ]
> 
>> diff --git a/doc/man-sections/protocol-options.rst b/doc/man-sections/protocol-options.rst
>> index 051f1d32..ab22b415 100644
>> --- a/doc/man-sections/protocol-options.rst
>> +++ b/doc/man-sections/protocol-options.rst
>> @@ -57,6 +57,9 @@ configured in a compatible way between both the local and remote side.
>>     http://www.cs.ucsd.edu/users/mihir/papers/hmac.html
>>   
>>   --cipher alg
>> +  This option is deprecated for server-client mode and ``--data-ciphers``
>> +  or rarely `--data-ciphers-fallback`` should be used instead.
>> +
>>     Encrypt data channel packets with cipher algorithm ``alg``.
>>   
>>     The default is :code:`BF-CBC`, an abbreviation for Blowfish in Cipher
>> @@ -183,8 +186,9 @@ configured in a compatible way between both the local and remote side.
>>     ``--server`` ), or if ``--pull`` is specified (client-side, implied by
>>     setting --client).
>>   
>> -  If both peers support and do not disable NCP, the negotiated cipher will
>> -  override the cipher specified by ``--cipher``.
>> +  If no common cipher is found is found during cipher negotiation, the
> 
> Duplicate "is found".
> 
>> +  connection is terminated. To support old clients/server that do not
>> +  provide any cipher support see ``data-ciphers-fallback``.
>>   
>>     Additionally, to allow for more smooth transition, if NCP is enabled,
>>     OpenVPN will inherit the cipher of the peer if that cipher is different
>> @@ -201,8 +205,18 @@ configured in a compatible way between both the local and remote side.
>>     This list is restricted to be 127 chars long after conversion to OpenVPN
>>     ciphers.
>>   
>> -  This option was called ``ncp-ciphers`` in OpenVPN 2.4 but has been renamed
>> -  to ``data-ciphers`` in OpenVPN 2.5 to more accurately reflect its meaning.
>> +  This option was called ``--ncp-ciphers`` in OpenVPN 2.4 but has been renamed
>> +  to ``--data-ciphers`` in OpenVPN 2.5 to more accurately reflect its meaning.
>> +
>> +--data-ciphers-fallback alg
>> +
>> +    Configure a cipher that is used to fall back to if the peer does announce
>> +    the cipher in OCC.
> 
> Isn't OCC to much detail here? I'd suggest something like "if we could
> not determine which cipher the peer is willing to use".
> 
>> +    This option should normally not be needed.  It only exists to allow
>> +    connecting to old servers or if supporting old clients as server.
>> +    The are only OpenVPN 2.3 and older version that have configured with
>> +    `--enable-small`.
> 
> I find this somewhat hard to read and understand, probably because it
> /is/ not so easy to understand but... A sugestion (native speakers,
> please help improve!):

sugestion ;-)
(i know, this is not in the code)

> 
> This option should only be needed when connecting to peers which run
> OpenVPN 2.3 or earlier, and are compiled with the --enable-small flag
> (typically used on routers or other embedded devices).

LGTM
(The comma is technically incorrect, commas should not be followed by 
'and' or 'but' but it is not important, if you prefer to use them here).

> 
>>   --ncp-disable
>>     Disable "Negotiable Crypto Parameters". This completely disables cipher
>> diff --git a/src/openvpn/crypto.c b/src/openvpn/crypto.c
>> index e92a0dc1..accab850 100644
>> --- a/src/openvpn/crypto.c
>> +++ b/src/openvpn/crypto.c
>> @@ -727,7 +727,8 @@ warn_insecure_key_type(const char *ciphername, const cipher_kt_t *cipher)
>>       {
>>           msg(M_WARN, "WARNING: INSECURE cipher (%s) with block size less than 128"
>>               " bit (%d bit).  This allows attacks like SWEET32.  Mitigate by "
>> -            "using a --cipher with a larger block size (e.g. AES-256-CBC).",
>> +            "using a --cipher with a larger block size (e.g. AES-256-CBC). "
>> +            "Support for these insecure cipher will be removed in OpenVPN 2.6.",
> 
> These insecure cipher*s*.
> 
>> --- a/src/openvpn/init.c
>> +++ b/src/openvpn/init.c
>> @@ -2374,7 +2374,30 @@ do_deferred_options(struct context *c, const unsigned int found)
>>           {
>>               /* 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);
>> +            bool useremotecipher = tls_poor_mans_ncp(&c->options,
>> +                                                     c->c2.tls_multi->remote_ciphername);
>> +
>> +            if (!useremotecipher && !c->options.enable_ncp_fallback)
>> +            {
>> +                /* Give appropiate error message */
>> +                if (c->c2.tls_multi->remote_ciphername)
>> +                {
>> +                    msg(D_TLS_ERRORS, "OPTIONS ERROR: failed to negotiate "
>> +                        "cipher with server.  Add the server's "
>> +                        "cipher ('%s') to --data-ciphers (currently '%s') if "
>> +                        "you want to connect to this server.",
>> +                        c->c2.tls_multi->remote_ciphername,
>> +                        c->options.ncp_ciphers);
>> +                }
>> +                else
>> +                {
>> +                    msg(D_TLS_ERRORS, "OPTIONS ERROR: failed to negotioate "
> 
> Typo: negotiate.
> 
>> +                        "cipher with server. Configure "
>> +                        "--data-ciphers-fallback if you want connect "
> 
> Typo: if you want *to* connect.
> 
>> +                        "to this server.");
>> +                }
>> +                return false;
>> +            }
> 
> This block seems to use more indentation levels than needed. Consider
> something like:
> 
>    else if (ncp_enabled) {
>      if (!remote_ciphername) {
>        msg("Failed to negotiate, configure --data-ciphers-fallback");
>        return false;
>      }
>      if (!tls_poor_mans_ncp) {
>        msg("Failed to negotiate, add cipher xxx to --data-ciphers");
>        return false;
>      }
>    }
> 
>>           }
>>           struct frame *frame_fragment = NULL;
>>   #ifdef ENABLE_FRAGMENT
>> diff --git a/src/openvpn/multi.c b/src/openvpn/multi.c
>> index 9bda38b0..a1f65127 100644
>> --- a/src/openvpn/multi.c
>> +++ b/src/openvpn/multi.c
>> @@ -1777,7 +1777,7 @@ multi_client_connect_setenv(struct multi_context *m,
>>    * - choosen cipher
>>    * - peer id
>>    */
>> -static void
>> +static bool
>>   multi_client_set_protocol_options(struct context *c)
>>   {
>>   
>> @@ -1807,8 +1807,11 @@ multi_client_set_protocol_options(struct context *c)
>>       }
>>   
>>       /* Select cipher if client supports Negotiable Crypto Parameters */
>> -    if (o->ncp_enabled)
>> +    if (!o->ncp_enabled)
>>       {
>> +        return true;
>> +    }
>> +
> 
> Hm, in this case I don't think this improves things. This turns
> 
>    if (foo) {
>      do_foo_stuff;
>    }
>    if (bar) {
>      do_bar_stuff;
>    }
> 
> into
> 
>    if (foo) {
>      do_foo_stuff;
>    }
>    if (!bar) {
>      return;
>    }
>    do_bar_stuff;
> 
> I think the orignal code flow is easier to understand. If there's too
> much indentation for you liking, you can split part of it into a
> separate function.
> 
> (This is just about this first if, I totally agree about the flow change
> below.)
> 
>>       /* 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
>>        * (so the client is always told what we expect it to use)
>> @@ -1820,43 +1823,69 @@ multi_client_set_protocol_options(struct context *c)
>>                "server has already generated data channel keys, "
>>                "re-sending previously negotiated cipher '%s'",
>>                o->ciphername );
>> +        return true;
>>       }
>> -        else
>> -        {
>> +
>>       /*
>>        * Push the first cipher from --data-ciphers to the client that
>>        * the client announces to be supporting.
>>        */
>> -            char *push_cipher = ncp_get_best_cipher(o->ncp_ciphers, o->ciphername,
>> -                                                    peer_info,
>> +    char *push_cipher = ncp_get_best_cipher(o->ncp_ciphers, peer_info,
>>                                               tls_multi->remote_ciphername,
>>                                               &o->gc);
>>   
>>       if (push_cipher)
>>       {
>>           o->ciphername = push_cipher;
>> +        return true;
>>       }
>> -            else
>> -            {
>> +
>> +    /* NCP cipher negotiation failed. Try to figure out why exactly it
>> +     * failed and give good error messages and potentially do a fallback
>> +     * for non NCP clients */
>>       struct gc_arena gc = gc_new();
>> +    bool ret = false;
>> +
>>       const char *peer_ciphers = tls_peer_ncp_list(peer_info, &gc);
>> +    /* If we are in a situation where we know the client ciphers, there is no
>> +     * reason to fall back to a cipher that will not be accepted by the other
>> +     * side, in this situation we fail the auth*/
>>       if (strlen(peer_ciphers) > 0)
>>       {
>> -                    msg(M_INFO, "PUSH: No common cipher between server and "
>> -                        "client. Expect this connection not to work. Server "
>> -                        "data-ciphers: '%s', client supported ciphers '%s'",
>> +        msg(M_INFO, "PUSH: No common cipher between server and client. "
>> +            "Server data-ciphers: '%s', client supported ciphers '%s'",
>>               o->ncp_ciphers, peer_ciphers);
>>       }
>> +    else if (tls_multi->remote_ciphername)
>> +    {
>> +        msg(M_INFO, "PUSH: No common cipher between server and client. "
>> +            "Server data-ciphers: '%s', client supports cipher '%s'",
>> +            o->ncp_ciphers, tls_multi->remote_ciphername);
>> +    }
>>       else
>>       {
>> -                    msg(M_INFO, "No NCP data received from peer, falling back "
>> -                        "to --cipher '%s'. Peer reports in OCC --cipher '%s'",
>> -                        o->ciphername, np(tls_multi->remote_ciphername));
>> +        msg(M_INFO, "PUSH: No NCP or OCC cipher data received from peer.");
>> +
>> +        if (o->enable_ncp_fallback && !tls_multi->remote_ciphername)
>> +        {
>> +            msg(M_INFO, "Using data channel cipher '%s' since "
>> +                "--data-ciphers-fallback is set.", o->ciphername);
>> +            ret = true;
>>           }
>> -                gc_free(&gc);
>> +        else
>> +        {
>> +            msg(M_INFO, "Use --data-ciphers-fallback with the cipher the "
>> +                "client is using if you want to allow the client to connect");
>>           }
>>       }
>> +    if (!ret)
>> +    {
>> +        auth_set_client_reason(tls_multi, "Data channel cipher negotiation "
>> +                                          "failed (no shared cipher)");
>>       }
>> +
>> +    gc_free(&gc);
>> +    return ret;
>>   }
>>   
>>   /**
>> @@ -2394,13 +2423,17 @@ multi_client_connect_late_setup(struct multi_context *m,
>>       mi->context.c2.context_auth = CAS_SUCCEEDED;
>>   
>>       /* authentication complete, calculate dynamic client specific options */
>> -    multi_client_set_protocol_options(&mi->context);
>> -
>> -    /* Generate data channel keys */
>> -    if (!multi_client_generate_tls_keys(&mi->context))
>> +    if (!multi_client_set_protocol_options(&mi->context))
>>       {
>>           mi->context.c2.context_auth = CAS_FAILED;
>>       }
>> +    /* Generate data channel keys only if setting protocol options
>> +     * has not failed */
>> +    else if (!multi_client_generate_tls_keys(&mi->context))
>> +    {
>> +
>> +        mi->context.c2.context_auth = CAS_FAILED;
>> +    }
>>   
>>       /* send push reply if ready */
>>       if (mi->context.c2.push_request_received)
>> diff --git a/src/openvpn/options.c b/src/openvpn/options.c
>> index 6d7dbf9f..5beaba0f 100644
>> --- a/src/openvpn/options.c
>> +++ b/src/openvpn/options.c
>> @@ -855,7 +855,6 @@ init_options(struct options *o, const bool init_gc)
>>   #if P2MP
>>       o->scheduled_exit_interval = 5;
>>   #endif
>> -    o->ciphername = "BF-CBC";
>>       o->ncp_enabled = true;
>>       o->ncp_ciphers = "AES-256-GCM:AES-128-GCM";
>>       o->authname = "SHA1";
>> @@ -3041,6 +3040,69 @@ options_postprocess_verify(const struct options *o)
>>       }
>>   }
>>   
>> +static void
>> +options_postprocess_cipher(struct options *o)
>> +{
>> +    if (!o->pull && !(o->mode == MODE_SERVER))
>> +    {
>> +        /* we are in the classic P2P mode */
>> +        /* cipher negotiation (NCP) currently assumes --pull or --mode server */
>> +        o->ncp_enabled = false;
>> +        msg( M_WARN, "Dynamic cipher negioation is disabled since neither "
> 
> Typo: negoatiation. Also "dynamic" seems redundant, "negotiation"
> already implies that.
> 
> As a side note: this warning message should be good enough for devs too,
> so we probably don't need the comments above that say the same.
> 
>> +             "P2MP client nor server mode is enabled" );
> 
> Was already in the orignal code, but: no space needed after msg(.
> 
>> +        /* If the cipher is not set default to BF-CBC, we will warn that this
>> +         * is deprecated on cipher initialisation, no need to warn here as
>> +         * well */
> 
> I had to read this twice to understand meaning. Maybe something like "if
> the cipher is not set, we set it to the old default value 'BF-CBC'." or so?
> 
>> +        if (!o->ciphername)
>> +        {
>> +            o->ciphername = "BF-CBC";
>> +        }
>> +        return;
>> +    }
>> +
>> +    /* M2P mode */
> 
> I think this is "P2MP mode"?
> 
>> +    if (!o->ciphername)
>> +    {
>> +        msg(M_WARN, "--cipher is not set. Previous OpenVPN version defaulted to "
>> +            "BF-CBC as fallback when dynamic cipher negotiation failed "

unnecessary use of dynamic again.

>> +            " 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

*cipher name*

>> +         * parts of OpenVPN assert that the ciphername is set */

*cipher name*
(just a suggestion)

>> +        o->ciphername = "BF-CBC";
>> +
>> +        if (!o->ncp_enabled)
>> +        {
>> +            msg(M_USAGE, "--ncp-disable needs an explicit --cipher or "
>> +                "--data-ciphers-fallback config option");
>> +        }
>> +    }
>> +    else if (!o->enable_ncp_fallback
>> +             && !tls_item_in_cipher_list(o->ciphername, o->ncp_ciphers))
>> +    {
>> +        msg(M_WARN, "DEPRECATED OPTION: --cipher set to '%s' but missing in"
>> +            " --ncp-ciphers (%s). Future OpenVPN version will "
>> +            "ignore --cipher for dynamic cipher negotiations. "

unnecessary use of dynamic again.


>> +            "Add '%s' to --data-ciphers or change --cipher '%s' to "
>> +            "--data-ciphers-fallback '%s' to silence this warning.",
>> +            o->ciphername, o->ncp_ciphers, o->ciphername,
>> +            o->ciphername, o->ciphername);
>> +        o->enable_ncp_fallback = true;
>> +
>> +        /* Append the --cipher to ncp_ciphers to allow it in NCP */
>> +        char *ncp_ciphers = gc_malloc(strlen(o->ncp_ciphers)
>> +                                      +strlen(o->ciphername) + 1, false, &o->gc);
>> +
>> +        strcpy(ncp_ciphers, o->ncp_ciphers);
>> +        strcat(ncp_ciphers, ":");
>> +        strcat(ncp_ciphers, o->ciphername);
> 
> Can we please avoid using strcpy and strcat? Using openvpn_snprintf()
> should result in simpler code, and makes it easier to ensure that we
> won't overflow.
> 
> Actually, looks like this already overflows, since there is no space for
> the null-byte in ncp_ciphers.
> 
>> +        o->ncp_ciphers = ncp_ciphers;
>> +    }
>> +}
>> +
>>   static void
>>   options_postprocess_mutate(struct options *o)
>>   {
>> @@ -3053,6 +3115,7 @@ options_postprocess_mutate(struct options *o)
>>       helper_keepalive(o);
>>       helper_tcp_nodelay(o);
>>   
>> +    options_postprocess_cipher(o);
>>       options_postprocess_mutate_invariant(o);
>>   
>>       if (o->ncp_enabled)
>> @@ -3113,16 +3176,6 @@ options_postprocess_mutate(struct options *o)
>>               "include this in your server configuration");
>>           o->dh_file = NULL;
>>       }
>> -
>> -    /* cipher negotiation (NCP) currently assumes --pull or --mode server */
>> -    if (o->ncp_enabled
>> -        && !(o->pull || o->mode == MODE_SERVER) )
>> -    {
>> -        msg( M_WARN, "disabling NCP mode (--ncp-disable) because not "
>> -             "in P2MP client or server mode" );
>> -        o->ncp_enabled = false;
>> -    }
>> -
>>   #if ENABLE_MANAGEMENT
>>       if (o->http_proxy_override)
>>       {
>> @@ -3658,14 +3711,22 @@ options_string(const struct options *o,
>>        */
>>   
>>       buf_printf(&out, ",dev-type %s", dev_type_string(o->dev, o->dev_type));
>> -    buf_printf(&out, ",link-mtu %u", (unsigned int) calc_options_string_link_mtu(o, frame));
>> +    if (o->ciphername)
>> +    {
>> +        /* the link-mtu that we send has only a meaning if have a fixed

*only has meaning if we have*

>> +         * cipher (p2p) or have a fallback cipher for older non ncp
>> +         * clients. If we do have a fallback cipher, do not send it */
> 
> This confuses me. The code reads like it's dependent on --cipher, rather
> than --fallbck-cipher. Also shouldn't this be "If we *don't* have a
> fallback cipher"?

I guess a rewrite then ..

> 
>> +        buf_printf(&out, ",link-mtu %u",
>> +                   (unsigned int) calc_options_string_link_mtu(o, frame));
>> +    }
>>       buf_printf(&out, ",tun-mtu %d", PAYLOAD_SIZE(frame));
>>       buf_printf(&out, ",proto %s",  proto_remote(o->ce.proto, remote));
>>   
>> +    bool p2p_nopull = o->mode == MODE_POINT_TO_POINT && !PULL_DEFINED(o);
>>       /* send tun_ipv6 only in peer2peer mode - in client/server mode, it
>>        * is usually pushed by the server, triggering a non-helpful warning
>>        */
>> -    if (o->ifconfig_ipv6_local && o->mode == MODE_POINT_TO_POINT && !PULL_DEFINED(o))
>> +    if (o->ifconfig_ipv6_local && p2p_nopull)
>>       {
>>           buf_printf(&out, ",tun-ipv6");
>>       }
>> @@ -3695,7 +3756,7 @@ options_string(const struct options *o,
>>           }
>>       }
>>   
>> -    if (tt && o->mode == MODE_POINT_TO_POINT && !PULL_DEFINED(o))
>> +    if (tt && p2p_nopull)
>>       {
>>           const char *ios = ifconfig_options_string(tt, remote, o->ifconfig_nowarn, gc);
>>           if (ios && strlen(ios))
>> @@ -3751,8 +3812,15 @@ options_string(const struct options *o,
>>   
>>           init_key_type(&kt, o->ciphername, o->authname, o->keysize, true,
>>                         false);
>> -
>> -        buf_printf(&out, ",cipher %s", cipher_kt_name(kt.cipher));
>> +        /* Only announce the cipher to our peer if we are willing to
>> +         * support it */
>> +        const char *ciphername = cipher_kt_name(kt.cipher);
>> +        if (p2p_nopull || !o->ncp_enabled
>> +            || (o->ncp_enabled
>> +                && tls_item_in_cipher_list(ciphername, o->ncp_ciphers)))
> 
> This second check for o->ncp_enabled is not needed. You already ensured
> it's true before. (Saves you a line wrap.)
> 
> I wonder though, isn't it too soon to stop sending cipher? Looks like
> both 2.3 and 2.4 clients will currently print options warnings if cipher
> is missing from OCC. The earlier NCP versions carefully tries to send
> what the peer expected here to prevent bogus warnings.
> 
>> +        {
>> +            buf_printf(&out, ",cipher %s", ciphername);
>> +        }
>>           buf_printf(&out, ",auth %s", md_kt_name(kt.digest));
>>           buf_printf(&out, ",keysize %d", kt.cipher_length * 8);
>>           if (o->shared_secret_file)
>> @@ -3870,7 +3938,8 @@ options_warning_safe_scan2(const int msglevel,
>>           || strprefix(p1, "keydir ")
>>           || strprefix(p1, "proto ")
>>           || strprefix(p1, "tls-auth ")
>> -        || strprefix(p1, "tun-ipv6"))
>> +        || strprefix(p1, "tun-ipv6")
>> +        || strprefix(p1, "cipher "))
>>       {
>>           return;
>>       }
>> @@ -7860,6 +7929,12 @@ add_option(struct options *options,
>>           VERIFY_PERMISSION(OPT_P_NCP|OPT_P_INSTANCE);
>>           options->ciphername = p[1];
>>       }
>> +    else if (streq(p[0], "data-ciphers-fallback") && p[1] && !p[2])
>> +    {
>> +        VERIFY_PERMISSION(OPT_P_INSTANCE);
>> +        options->ciphername = p[1];
>> +        options->enable_ncp_fallback = true;
>> +    }
>>       else if ((streq(p[0], "data-ciphers") || streq(p[0], "ncp-ciphers"))
>>               && p[1] && !p[2])
>>       {
>> diff --git a/src/openvpn/options.h b/src/openvpn/options.h
>> index c5df2d18..877e9396 100644
>> --- a/src/openvpn/options.h
>> +++ b/src/openvpn/options.h
>> @@ -503,6 +503,8 @@ struct options
>>       bool shared_secret_file_inline;
>>       int key_direction;
>>       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;
>> diff --git a/src/openvpn/ssl.c b/src/openvpn/ssl.c
>> index 91ab3bf6..06dc9f8f 100644
>> --- a/src/openvpn/ssl.c
>> +++ b/src/openvpn/ssl.c
>> @@ -1932,13 +1932,14 @@ tls_session_update_crypto_params(struct tls_session *session,
>>           return true;
>>       }
>>   
>> -    if (!session->opt->server
>> -        && 0 != strcmp(options->ciphername, session->opt->config_ciphername)
>> +    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 or %s",
>> -            options->ciphername, session->opt->config_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;
>> diff --git a/src/openvpn/ssl_ncp.c b/src/openvpn/ssl_ncp.c
>> index 8e432fb7..528b31ea 100644
>> --- a/src/openvpn/ssl_ncp.c
>> +++ b/src/openvpn/ssl_ncp.c
>> @@ -211,9 +211,8 @@ tls_peer_ncp_list(const char *peer_info, struct gc_arena *gc)
>>   }
>>   
>>   char *
>> -ncp_get_best_cipher(const char *server_list, const char *server_cipher,
>> -                    const char *peer_info,  const char *remote_cipher,
>> -                    struct gc_arena *gc)
>> +ncp_get_best_cipher(const char *server_list, const char *peer_info,
>> +                    const char *remote_cipher, struct gc_arena *gc)
>>   {
>>       /*
>>        * The gc of the parameter is tied to the VPN session, create a
>> @@ -242,15 +241,6 @@ ncp_get_best_cipher(const char *server_list, const char *server_cipher,
>>               break;
>>           }
>>       }
>> -    /* We have not found a common cipher, as a last resort check if the
>> -     * server cipher can be used
>> -     */
>> -    if (token == NULL
>> -        && (tls_item_in_cipher_list(server_cipher, peer_ncp_list)
>> -            || streq(server_cipher, remote_cipher)))
>> -    {
>> -        token = server_cipher;
>> -    }
>>   
>>       char *ret = NULL;
>>       if (token != NULL)
>> @@ -262,16 +252,18 @@ ncp_get_best_cipher(const char *server_list, const char *server_cipher,
>>       return ret;
>>   }
>>   
>> -void
>> +bool
>>   tls_poor_mans_ncp(struct options *o, const char *remote_ciphername)
>>   {
>> -    if (o->ncp_enabled && remote_ciphername
>> +    if (remote_ciphername
>>           && 0 != strcmp(o->ciphername, remote_ciphername))
>>       {
>>           if (tls_item_in_cipher_list(remote_ciphername, o->ncp_ciphers))
>>           {
>>               o->ciphername = string_alloc(remote_ciphername, &o->gc);
>>               msg(D_TLS_DEBUG_LOW, "Using peer cipher '%s'", o->ciphername);
>> +            return true;
>>           }
>>       }
>> +    return false;
>>   }
>> diff --git a/src/openvpn/ssl_ncp.h b/src/openvpn/ssl_ncp.h
>> index d00c222d..98f80286 100644
>> --- a/src/openvpn/ssl_ncp.h
>> +++ b/src/openvpn/ssl_ncp.h
>> @@ -44,10 +44,13 @@ tls_peer_supports_ncp(const char *peer_info);
>>    * "Poor man's NCP": Use peer cipher if it is an allowed (NCP) cipher.
>>    * Allows non-NCP peers to upgrade their cipher individually.
>>    *
>> + * Returns true if we switched to the peer's cipher
>> + *
>>    * Make sure to call tls_session_update_crypto_params() after calling this
>>    * function.
>>    */
>> -void tls_poor_mans_ncp(struct options *o, const char *remote_ciphername);
>> +bool
>> +tls_poor_mans_ncp(struct options *o, const char *remote_ciphername);
> 
> I think we almost always put the return type on the same line in header
> files. (Don't know why, but it seems quite consistent.)
> -Steffan
> 
> 


HTH
Richard

Re: [Openvpn-devel] [PATCH 9/9] Rework NCP compability logic and drop BF-CBC support by default

[Arne S. <ar...@rf...>]

Am 28.07.20 um 14:27 schrieb Steffan Karger:

>>   * - peer id
>>   */
>> -static void
>> +static bool
>>  multi_client_set_protocol_options(struct context *c)
>>  {
>>  
>> @@ -1807,8 +1807,11 @@ multi_client_set_protocol_options(struct context *c)
>>      }
>>  
>>      /* Select cipher if client supports Negotiable Crypto Parameters */
>> -    if (o->ncp_enabled)
>> +    if (!o->ncp_enabled)
>>      {
>> +        return true;
>> +    }
>> +
> 
> Hm, in this case I don't think this improves things. This turns
> 

Yes. But this code will also be removed as soon as we hit 2.6 master and
remove ncp-disable and then the flow should be good.


> 
> Can we please avoid using strcpy and strcat? Using openvpn_snprintf()
> should result in simpler code, and makes it easier to ensure that we
> won't overflow.
> 
> Actually, looks like this already overflows, since there is no space for
> the null-byte in ncp_ciphers.

Yes, I still hate string handling in C. I hope my next attempt is better...


>>  
>>      buf_printf(&out, ",dev-type %s", dev_type_string(o->dev, o->dev_type));
>> -    buf_printf(&out, ",link-mtu %u", (unsigned int) calc_options_string_link_mtu(o, frame));
>> +    if (o->ciphername)
>> +    {
>> +        /* the link-mtu that we send has only a meaning if have a fixed
>> +         * cipher (p2p) or have a fallback cipher for older non ncp
>> +         * clients. If we do have a fallback cipher, do not send it */
> 
> This confuses me. The code reads like it's dependent on --cipher, rather
> than --fallbck-cipher. Also shouldn't this be "If we *don't* have a
> fallback cipher"?

Good catch. First I wanted to set ciphername == NULL in case we don't
have a ciphername but to be to difficult for the time being. This an
oversight from that try.


>>      {
>>          const char *ios = ifconfig_options_string(tt, remote, o->ifconfig_nowarn, gc);
>>          if (ios && strlen(ios))
>> @@ -3751,8 +3812,15 @@ options_string(const struct options *o,
>>  
>>          init_key_type(&kt, o->ciphername, o->authname, o->keysize, true,
>>                        false);
>> -
>> -        buf_printf(&out, ",cipher %s", cipher_kt_name(kt.cipher));
>> +        /* Only announce the cipher to our peer if we are willing to
>> +         * support it */
>> +        const char *ciphername = cipher_kt_name(kt.cipher);
>> +        if (p2p_nopull || !o->ncp_enabled
>> +            || (o->ncp_enabled
>> +                && tls_item_in_cipher_list(ciphername, o->ncp_ciphers)))
> 
> This second check for o->ncp_enabled is not needed. You already ensured
> it's true before. (Saves you a line wrap.)

I added to make the condition a bit clearer but I will remove it.

> I wonder though, isn't it too soon to stop sending cipher? Looks like
> both 2.3 and 2.4 clients will currently print options warnings if cipher
> is missing from OCC. The earlier NCP versions carefully tries to send
> what the peer expected here to prevent bogus warnings.

The main reason that I added it that sending it would break
compatibility with 2.5 master servers. Since --cipher BF-CBC, a server
would assume that it was supported. But I think we can keep sending
and instead assume that a client that sends IV_CIPHERS will only support
those and not necessarily the one in the OCC cipher. Basically disabling
Poor man's NCP when we see a 2.5 client. I would need to fix OpenVPN3 to
also include its --cipher in the IV_CIPHERS string but that is doable.

That would bascially break support of David's currently released
openpvn3-linux clients since they use openvpn3 master that has currently
has the hardcoded list

IV_CIPHERS=CHACHA20-POLY1305:AES-256-GCM:AES-128-GCM

and we would ignore the OCC cipher. But I think that is acceptable
because you need to actively set a ncp-ciphers option on the server to
something that removes the GCM ciphers.


>>  }
>> diff --git a/src/openvpn/ssl_ncp.h b/src/openvpn/ssl_ncp.h
>> index d00c222d..98f80286 100644
>> --- a/src/openvpn/ssl_ncp.h
>> +++ b/src/openvpn/ssl_ncp.h
>> @@ -44,10 +44,13 @@ tls_peer_supports_ncp(const char *peer_info);
>>   * "Poor man's NCP": Use peer cipher if it is an allowed (NCP) cipher.
>>   * Allows non-NCP peers to upgrade their cipher individually.
>>   *
>> + * Returns true if we switched to the peer's cipher
>> + *
>>   * Make sure to call tls_session_update_crypto_params() after calling this
>>   * function.
>>   */
>> -void tls_poor_mans_ncp(struct options *o, const char *remote_ciphername);
>> +bool
>> +tls_poor_mans_ncp(struct options *o, const char *remote_ciphername);
> 
> I think we almost always put the return type on the same line in header
> files. (Don't know why, but it seems quite consistent.)
> -Steffan

Depends on the header, but you are right.

Arne