openvpn-devel — OpenVPN developers list

[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 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 v2] t_net.sh: drop hard dependency on t_client.rc

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

Right now t_net.sh depends on t_client.rc in order to source the
RUN_SUDO variable only.
However, t_client.rc is something that a few people only have configured
and thus this would result in t_net.sh almost never executed even if it
just could.

Drop dependency on t_client.rc by falling back to RUN_SUDO=sudo when the
file is missing and no RUN_SUDO is passed via env.

While at it, reword the error message to better match the current logic
flow.

Signed-off-by: Antonio Quartulli <a...@un...>
---

Changes from v1:
* default to sudo when no RUN_SUDO is set externally
* change warning message

 tests/t_net.sh | 26 +++++++++++---------------
 1 file changed, 11 insertions(+), 15 deletions(-)

diff --git a/tests/t_net.sh b/tests/t_net.sh
index c67c3df2..eef3c5d1 100755
--- a/tests/t_net.sh
+++ b/tests/t_net.sh
@@ -76,10 +76,6 @@ if [ -r "${top_builddir}"/t_client.rc ]; then
     . "${top_builddir}"/t_client.rc
 elif [ -r "${srcdir}"/t_client.rc ]; then
     . "${srcdir}"/t_client.rc
-else
-    echo "$0: cannot find 't_client.rc' in build dir ('${top_builddir}')" >&2
-    echo "$0: or source directory ('${srcdir}'). SKIPPING TEST." >&2
-    exit 77
 fi
 
 if [ ! -x "$openvpn" ]; then
@@ -117,18 +113,18 @@ else
 
     if [ -z "$RUN_SUDO" ]
     then
-        echo "$0: this test must run be as root, or RUN_SUDO=... " >&2
-        echo "      must be set correctly in 't_client.rc'. SKIP." >&2
-        exit 77
+        echo "$0: no RUN_SUDO=... in t_client.rc or environment, defaulting to 'sudo'." >&2
+        echo "      if that does not work, set RUN_SUDO= correctly for your system." >&2
+        RUN_SUDO="sudo"
+    fi
+
+    # check that we can run the unit-test binary with sudo
+	if $RUN_SUDO $UNIT_TEST test
+    then
+	    echo "$0: $RUN_SUDO $UNIT_TEST succeeded, good."
     else
-        # check that we can run the unit-test binary with sudo
-	    if $RUN_SUDO $UNIT_TEST test
-        then
-	        echo "$0: $RUN_SUDO $UNIT_TEST succeeded, good."
-        else
-	        echo "$0: $RUN_SUDO $UNIT_TEST failed, cannot go on. SKIP." >&2
-	        exit 77
-        fi
+	    echo "$0: $RUN_SUDO $UNIT_TEST failed, cannot go on. SKIP." >&2
+	    exit 77
     fi
 fi
 
-- 
2.27.0

[Openvpn-devel] [PATCH applied] Re: Remove ENABLE_OCC #define

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

NOW I can finally merge this, since key-method v1 is gone
and this compiles without unresolveds \o/

Stared-at-code, test compiled, ship.

Your patch has been applied to the master branch.

commit ba66faad5608233f792c3679ebade09ff324a4b3
Author: Arne Schwabe
Date:   Fri Jul 17 15:47:36 2020 +0200

     Remove ENABLE_OCC #define

     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.../msg20442.html
     Signed-off-by: Gert Doering <ge...@gr...>


--
kind regards,

Gert Doering

Re: [Openvpn-devel] [PATCH 1/9 v3] Indicate that a client is in pull mode in IV_PROTO

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

Hi,

On 21/07/2020 18:38, Arne Schwabe wrote:
> This allows us to skip waiting for the first PUSH_REQUEST message from
> the client to send the response.
> 
> This changes the interpretation of IV_PROTO from a scalar to a bitfield
> Since we only have IV_PROTO=2 defined so far and will support DATA_V2
> this should not make any problem. This avoid adding another IV_xxx variable
> that takes valuable space in the protocol frame.
> 
> Signed-off-by: Arne Schwabe <ar...@rf...>
> 
> Patch V2: Use bitmask for IV_PROTO_DATA_V2 and add more documentation.
> 
> Patch V3: Rewrite IV_PROTO paragraph in man page, incoperate spelling fixes
>           by tincanteksup.
> 
> Signed-off-by: Arne Schwabe <ar...@rf...>


Thanks a lot for all the fixes. Looks good to me now.

-- 
Antonio Quartulli

[Openvpn-devel] [PATCH applied] Re: Remove key-method 1

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

Your patch has been applied to the master branch.

I have run a t_client test on FreeBSD/OpenSSL and Linux/mbedTLS, and
a full server side test.  Just to be sure.  This is surprisingly large 
changes in crypto code...  the changes look good, but...!

All tests pass :-)

Test sets succeeded: 1 1a 1b 1c 1d 1e 2 2a 2b 2c 2d 2e 3 4 5 5a 5b 5c 5v1 5v2 5v3 5w1 5w2 5w3 5w4 5x1 5x2 5x3 5x4 6 7 7x 8 8a 9 2f 4b.
Test sets failed: none.


I have ignored that hunk from the patch:

@@ -2312,7 +2246,7 @@ push_peer_info(struct buffer *buf, struct tls_session  
+*session)
          * push request, also signal that the client wants
          * to get push-reply messages without without requiring a round
          * trip for a push request message*/
-        if(session->opt->pull)
+        if (session->opt->pull)
         {
             iv_proto |= IV_PROTO_REQUEST_PUSH;
         }

because it fixes whitespace in code that is not there yet - so I'll
fix the whitespace on the fly when applying *that* patch (if we're
not at v8 by then, with the whitespace fix included).

commit 36bef1b52b49ebbc3790635be230e2f30f0532a7
Author: Arne Schwabe
Date:   Tue Jul 21 12:01:28 2020 +0200

     Remove key-method 1

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


--
kind regards,

Gert Doering

Re: [Openvpn-devel] [PATCH 1/9 v3] Indicate that a client is in pull mode in IV_PROTO

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

1x spelling 1x grammar

On 21/07/2020 17:38, Arne Schwabe wrote:
> This allows us to skip waiting for the first PUSH_REQUEST message from
> the client to send the response.
> 
> This changes the interpretation of IV_PROTO from a scalar to a bitfield
> Since we only have IV_PROTO=2 defined so far and will support DATA_V2
> this should not make any problem. This avoid adding another IV_xxx variable
> that takes valuable space in the protocol frame.
> 
> Signed-off-by: Arne Schwabe <ar...@rf...>
> 
> Patch V2: Use bitmask for IV_PROTO_DATA_V2 and add more documentation.
> 
> Patch V3: Rewrite IV_PROTO paragraph in man page, incoperate spelling fixes
>            by tincanteksup.

incoperate -> incorporate =]

Please feel free use my full details:
Richard Bonhomme <tin...@gm...>

> 
> Signed-off-by: Arne Schwabe <ar...@rf...>
> ---
>   doc/man-sections/server-options.rst | 10 ++++++++--
>   src/openvpn/multi.c                 | 12 ++++++++++--
>   src/openvpn/ssl.c                   | 15 +++++++++++++--
>   src/openvpn/ssl.h                   | 16 ++++++++++++++++
>   4 files changed, 47 insertions(+), 6 deletions(-)
> 
> diff --git a/doc/man-sections/server-options.rst b/doc/man-sections/server-options.rst
> index c8e9fc61..babf33d3 100644
> --- a/doc/man-sections/server-options.rst
> +++ b/doc/man-sections/server-options.rst
> @@ -464,8 +464,14 @@ fast hardware. SSL/TLS authentication must be used in this mode.
>     :code:`IV_LZ4=1`
>           If the client supports LZ4 compressions.
>   
> -  :code:`IV_PROTO=2`
> -        If the client supports peer-id floating mechanism
> +  :code:`IV_PROTO`
> +    Details about protocol extensions that the peer supports. The
> +    variable is a bitfield and the bits are defined as follows
> +    (starting a bit 0 for the first (unused) bit:
> +
> +    - bit 1: The peer supports peer-id floating mechanism
> +    - bit 2: The client expects a push-reply and the server may
> +      send this reply without waiting for a push-request first.
>   
>     :code:`IV_NCP=2`
>           Negotiable ciphers, client supports ``--cipher`` pushed by
> diff --git a/src/openvpn/multi.c b/src/openvpn/multi.c
> index 2ae9c03e..b83a0f06 100644
> --- a/src/openvpn/multi.c
> +++ b/src/openvpn/multi.c
> @@ -1792,10 +1792,18 @@ multi_client_set_protocol_options(struct context *c)
>       {
>           int proto = 0;
>           int r = sscanf(optstr, "IV_PROTO=%d", &proto);
> -        if ((r == 1) && (proto >= 2))
> +        if (r == 1)
>           {
> -            tls_multi->use_peer_id = true;
> +            if (proto & IV_PROTO_DATA_V2)
> +            {
> +                tls_multi->use_peer_id = true;
> +            }
> +            if (proto & IV_PROTO_REQUEST_PUSH)
> +            {
> +                c->c2.push_request_received = true;
> +            }
>           }
> +
>       }
>   
>       /* Select cipher if client supports Negotiable Crypto Parameters */
> diff --git a/src/openvpn/ssl.c b/src/openvpn/ssl.c
> index 54a23011..04d78a81 100644
> --- a/src/openvpn/ssl.c
> +++ b/src/openvpn/ssl.c
> @@ -2299,8 +2299,19 @@ push_peer_info(struct buffer *buf, struct tls_session *session)
>           buf_printf(&out, "IV_PLAT=win\n");
>   #endif
>   
> -        /* support for P_DATA_V2 */
> -        buf_printf(&out, "IV_PROTO=2\n");
> +        /* support for P_DATA_V2*/
> +        int iv_proto = IV_PROTO_DATA_V2;
> +
> +        /* support for receiving push_reply before sending
> +         * push request, also signal that the client wants
> +         * to get push-reply messages without without requiring a round
> +         * trip for a push request message*/
> +        if(session->opt->pull)
> +        {
> +            iv_proto |= IV_PROTO_REQUEST_PUSH;
> +        }
> +
> +        buf_printf(&out, "IV_PROTO=%d\n", iv_proto);
>   
>           /* support for Negotiable Crypto Parameters */
>           if (session->opt->ncp_enabled
> diff --git a/src/openvpn/ssl.h b/src/openvpn/ssl.h
> index 58a9b0d4..0da16974 100644
> --- a/src/openvpn/ssl.h
> +++ b/src/openvpn/ssl.h
> @@ -99,6 +99,22 @@
>   /* Maximum length of OCC options string passed as part of auth handshake */
>   #define TLS_OPTIONS_LEN 512
>   
> +/* Definitions of the bits in the IV_PROTO bitfield
> + *
> + * In older OpenVPN versions this used in a comparison

In older OpenVPN versions this used in a comparison ->
In older OpenVPN versions this is used in a comparison


> + * IV_PROTO >= 2 to determine if DATA_V2 is supported.
> + * Therefore any client announcing any of the flags must
> + * also announce IV_PROTO_DATA_V2. We also treat bit 0
> + * as reserved for this reason */
> +
> +/** Support P_DATA_V2 */
> +#define IV_PROTO_DATA_V2        (1<<1)
> +
> +/** Assume client will send a push request and server does not need
> + * to wait for a push-request to send a push-reply */
> +#define IV_PROTO_REQUEST_PUSH   (1<<2)
> +
> +
>   /* Default field in X509 to be username */
>   #define X509_USERNAME_FIELD_DEFAULT "CN"
>   
> 

Re: [Openvpn-devel] [PATCH] t_net.sh: drop hard dependency on t_client.rc

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

Hi,

On 21/07/2020 18:39, Gert Doering wrote:
> In here, print & set
> 
>       if [ -z "$RUN_SUDO" ]
>       then
>  +        echo "$0: no RUN_SUDO=... in t_client.rc or environment, defaulting to 'sudo'." >&2
>  +        echo "      if that does not work, set RUN_SUDO= correctly for your system." >&2
>  +        RUN_SUDO=sudo"
>       fi
> 
> done - less code, message conveyed if needed.
>   


hmhmhmh makes sense. v2 incoming!

-- 
Antonio Quartulli

Re: [Openvpn-devel] [PATCH v3 5/9] Remove key-method 1

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

On 21/07/2020 12:01, Arne Schwabe wrote:
> Key-method 1 is only needed to talk to pre OpenVPN 2.0 clients.
> 
> Patch V2: Fix style. Make V1 op codes illegal, remove all code handling
>           v1 op codes and give a good warning message if we encounter
>           them in the legal op codes pre-check.
> 
> Patch V3: Add a bit more comments in the existing methods.
> 
> Signed-off-by: Arne Schwabe <ar...@rf...>
> ---
>  doc/doxygen/doc_control_processor.h |   6 +-
>  doc/doxygen/doc_key_generation.h    |   6 +-
>  doc/doxygen/doc_protocol_overview.h |   2 +-
>  src/openvpn/forward.c               |   2 +-
>  src/openvpn/helper.c                |   5 -
>  src/openvpn/init.c                  |   1 -
>  src/openvpn/options.c               |  35 +---
>  src/openvpn/options.h               |   4 -
>  src/openvpn/ssl.c                   | 240 +++++-----------------------
>  src/openvpn/ssl.h                   |  19 +--
>  src/openvpn/ssl_common.h            |   1 -
>  11 files changed, 53 insertions(+), 268 deletions(-)
> 

This LGTM now.  Thanks for the updates and adjustments!

I've done light local build testing (applying just this patch on git master
commit 08469ca1eccc).  Builds fine, 'make check' looks good.

Acked-By: David Sommerseth <da...@op...>


-- 
kind regards,

David Sommerseth
OpenVPN Inc

Re: [Openvpn-devel] [PATCH] t_net.sh: drop hard dependency on t_client.rc

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

Hi,

On Fri, Jul 17, 2020 at 06:02:31PM +0200, Antonio Quartulli wrote:
> Right now t_net.sh depends on t_client.rc in order to source the
> RUN_SUDO variable only.

I was about to merge this ("nice and easy") but I think it's just
complicated.

> diff --git a/tests/t_net.sh b/tests/t_net.sh
> index c67c3df2..63732db9 100755
> --- a/tests/t_net.sh
> +++ b/tests/t_net.sh
> @@ -77,9 +77,7 @@ if [ -r "${top_builddir}"/t_client.rc ]; then
>  elif [ -r "${srcdir}"/t_client.rc ]; then
>      . "${srcdir}"/t_client.rc
>  else
> -    echo "$0: cannot find 't_client.rc' in build dir ('${top_builddir}')" >&2
> -    echo "$0: or source directory ('${srcdir}'). SKIPPING TEST." >&2
> -    exit 77
> +    RUN_SUDO="${RUN_SUDO:-sudo}"
>  fi

Leave off the whole else clause.

>  if [ ! -x "$openvpn" ]; then
> @@ -117,8 +115,7 @@ else
>  
>      if [ -z "$RUN_SUDO" ]
>      then
> -        echo "$0: this test must run be as root, or RUN_SUDO=... " >&2
> -        echo "      must be set correctly in 't_client.rc'. SKIP." >&2
> +        echo "$0: using t_client.rc, but RUN_SUDO=... is not defined correctly. SKIP. " >&2
>          exit 77

In here, print & set

      if [ -z "$RUN_SUDO" ]
      then
 +        echo "$0: no RUN_SUDO=... in t_client.rc or environment, defaulting to 'sudo'." >&2
 +        echo "      if that does not work, set RUN_SUDO= correctly for your system." >&2
 +        RUN_SUDO=sudo"
      fi

done - less code, message conveyed if needed.
  
gert
-- 
"If was one thing all people took for granted, was conviction that if you 
 feed honest figures into a computer, honest figures come out. Never doubted 
 it myself till I met a computer with a sense of humor."
                             Robert A. Heinlein, The Moon is a Harsh Mistress

Gert Doering - Munich, Germany                             ge...@gr...

[Openvpn-devel] [PATCH 1/9 v3] Indicate that a client is in pull mode in IV_PROTO

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

This allows us to skip waiting for the first PUSH_REQUEST message from
the client to send the response.

This changes the interpretation of IV_PROTO from a scalar to a bitfield
Since we only have IV_PROTO=2 defined so far and will support DATA_V2
this should not make any problem. This avoid adding another IV_xxx variable
that takes valuable space in the protocol frame.

Signed-off-by: Arne Schwabe <ar...@rf...>

Patch V2: Use bitmask for IV_PROTO_DATA_V2 and add more documentation.

Patch V3: Rewrite IV_PROTO paragraph in man page, incoperate spelling fixes
          by tincanteksup.

Signed-off-by: Arne Schwabe <ar...@rf...>
---
 doc/man-sections/server-options.rst | 10 ++++++++--
 src/openvpn/multi.c                 | 12 ++++++++++--
 src/openvpn/ssl.c                   | 15 +++++++++++++--
 src/openvpn/ssl.h                   | 16 ++++++++++++++++
 4 files changed, 47 insertions(+), 6 deletions(-)

diff --git a/doc/man-sections/server-options.rst b/doc/man-sections/server-options.rst
index c8e9fc61..babf33d3 100644
--- a/doc/man-sections/server-options.rst
+++ b/doc/man-sections/server-options.rst
@@ -464,8 +464,14 @@ fast hardware. SSL/TLS authentication must be used in this mode.
   :code:`IV_LZ4=1`
         If the client supports LZ4 compressions.
 
-  :code:`IV_PROTO=2`
-        If the client supports peer-id floating mechanism
+  :code:`IV_PROTO`
+    Details about protocol extensions that the peer supports. The
+    variable is a bitfield and the bits are defined as follows
+    (starting a bit 0 for the first (unused) bit:
+
+    - bit 1: The peer supports peer-id floating mechanism
+    - bit 2: The client expects a push-reply and the server may
+      send this reply without waiting for a push-request first.
 
   :code:`IV_NCP=2`
         Negotiable ciphers, client supports ``--cipher`` pushed by
diff --git a/src/openvpn/multi.c b/src/openvpn/multi.c
index 2ae9c03e..b83a0f06 100644
--- a/src/openvpn/multi.c
+++ b/src/openvpn/multi.c
@@ -1792,10 +1792,18 @@ multi_client_set_protocol_options(struct context *c)
     {
         int proto = 0;
         int r = sscanf(optstr, "IV_PROTO=%d", &proto);
-        if ((r == 1) && (proto >= 2))
+        if (r == 1)
         {
-            tls_multi->use_peer_id = true;
+            if (proto & IV_PROTO_DATA_V2)
+            {
+                tls_multi->use_peer_id = true;
+            }
+            if (proto & IV_PROTO_REQUEST_PUSH)
+            {
+                c->c2.push_request_received = true;
+            }
         }
+
     }
 
     /* Select cipher if client supports Negotiable Crypto Parameters */
diff --git a/src/openvpn/ssl.c b/src/openvpn/ssl.c
index 54a23011..04d78a81 100644
--- a/src/openvpn/ssl.c
+++ b/src/openvpn/ssl.c
@@ -2299,8 +2299,19 @@ push_peer_info(struct buffer *buf, struct tls_session *session)
         buf_printf(&out, "IV_PLAT=win\n");
 #endif
 
-        /* support for P_DATA_V2 */
-        buf_printf(&out, "IV_PROTO=2\n");
+        /* support for P_DATA_V2*/
+        int iv_proto = IV_PROTO_DATA_V2;
+
+        /* support for receiving push_reply before sending
+         * push request, also signal that the client wants
+         * to get push-reply messages without without requiring a round
+         * trip for a push request message*/
+        if(session->opt->pull)
+        {
+            iv_proto |= IV_PROTO_REQUEST_PUSH;
+        }
+
+        buf_printf(&out, "IV_PROTO=%d\n", iv_proto);
 
         /* support for Negotiable Crypto Parameters */
         if (session->opt->ncp_enabled
diff --git a/src/openvpn/ssl.h b/src/openvpn/ssl.h
index 58a9b0d4..0da16974 100644
--- a/src/openvpn/ssl.h
+++ b/src/openvpn/ssl.h
@@ -99,6 +99,22 @@
 /* Maximum length of OCC options string passed as part of auth handshake */
 #define TLS_OPTIONS_LEN 512
 
+/* Definitions of the bits in the IV_PROTO bitfield
+ *
+ * In older OpenVPN versions this used in a comparison
+ * IV_PROTO >= 2 to determine if DATA_V2 is supported.
+ * Therefore any client announcing any of the flags must
+ * also announce IV_PROTO_DATA_V2. We also treat bit 0
+ * as reserved for this reason */
+
+/** Support P_DATA_V2 */
+#define IV_PROTO_DATA_V2        (1<<1)
+
+/** Assume client will send a push request and server does not need
+ * to wait for a push-request to send a push-reply */
+#define IV_PROTO_REQUEST_PUSH   (1<<2)
+
+
 /* Default field in X509 to be username */
 #define X509_USERNAME_FIELD_DEFAULT "CN"
 
-- 
2.26.2

[Openvpn-devel] [PATCH applied] Re: Remove --client-cert-not-required

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

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

Another easy one for me to grab :-)

Adjusted Changes.rst context to apply, looked at patch, did a test
build.  Agree to the M_FATAL, this is important here to see what is
wrong right away, before having client connects fail.

People will scream at you anyway.

Fixed one grammar error

     msg(M_USAGE, "--verify-client-cert requires --mode server");
                                               ^^

Your patch has been applied to the master branch.

commit 08469ca1eccc5f0ba68edf5166497ac2efcb72c5
Author: David Sommerseth
Date:   Mon Jul 20 13:30:10 2020 +0200

     Remove --client-cert-not-required

     Signed-off-by: David Sommerseth <da...@op...>
     Acked-by: Gert Doering <ge...@gr...>
     Message-Id: <202...@op...>
     URL: https://www.mail-archive.com/ope...@li.../msg20502.html
     Signed-off-by: Gert Doering <ge...@gr...>


--
kind regards,

Gert Doering

[Openvpn-devel] [PATCH applied] Re: Remove --ifconfig-pool-linear

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

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

I take the easy ones today :-)

Looked at the diff (looks good), fixed Changes.rst (if I apply
in non-sent-order, context is wrong), test compiled (just to be
sure).

Your patch has been applied to the master branch.

commit 2d5facaa5f6e6ee3dd2f15c2e7f5510939dd445b
Author: David Sommerseth
Date:   Mon Jul 20 13:51:56 2020 +0200

     Remove --ifconfig-pool-linear

     Signed-off-by: David Sommerseth <da...@op...>
     Acked-by: Gert Doering <ge...@gr...>
     Message-Id: <202...@op...>
     URL: https://www.mail-archive.com/ope...@li.../msg20504.html
     Signed-off-by: Gert Doering <ge...@gr...>


--
kind regards,

Gert Doering

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

[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 v4] Allow changing cipher from a ccd file

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

Hi,

On Sun, Aug 12, 2018 at 10:51:04AM +0200, Steffan Karger wrote:
> As described in msg  <374...@be...> on
> ope...@li..., clients that are compiled with
> --disable-occ (included in --enable-small) won't send an options string.
> Without the options string, the 2.4 server doesn't know which cipher to
> use for poor man's NCP.

Just for the record: we do not need this patch anymore, as Arne's
magic refactoring work has made adding this functionality very trivial
("just permit cipher in ccd/ files") - and that one has been merged
already.

I just found this one in patchwork and thought a bit of commenting
would help transparency :-)

gert
-- 
"If was one thing all people took for granted, was conviction that if you 
 feed honest figures into a computer, honest figures come out. Never doubted 
 it myself till I met a computer with a sense of humor."
                             Robert A. Heinlein, The Moon is a Harsh Mistress

Gert Doering - Munich, Germany                             ge...@gr...

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

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 v3 5/9] Remove key-method 1

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

1x typo

On 21/07/2020 11:01, Arne Schwabe wrote:
> Key-method 1 is only needed to talk to pre OpenVPN 2.0 clients.
> 
> Patch V2: Fix style. Make V1 op codes illegal, remove all code handling
>            v1 op codes and give a good warning message if we encounter
>            them in the legal op codes pre-check.
> 
> Patch V3: Add a bit more comments in the existing methods.
> 
> Signed-off-by: Arne Schwabe <ar...@rf...>
> ---
>   doc/doxygen/doc_control_processor.h |   6 +-
>   doc/doxygen/doc_key_generation.h    |   6 +-
>   doc/doxygen/doc_protocol_overview.h |   2 +-
>   src/openvpn/forward.c               |   2 +-
>   src/openvpn/helper.c                |   5 -
>   src/openvpn/init.c                  |   1 -
>   src/openvpn/options.c               |  35 +---
>   src/openvpn/options.h               |   4 -
>   src/openvpn/ssl.c                   | 240 +++++-----------------------
>   src/openvpn/ssl.h                   |  19 +--
>   src/openvpn/ssl_common.h            |   1 -
>   11 files changed, 53 insertions(+), 268 deletions(-)
> 
> diff --git a/doc/doxygen/doc_control_processor.h b/doc/doxygen/doc_control_processor.h
> index f87324cc..1bbf2d2d 100644
> --- a/doc/doxygen/doc_control_processor.h
> +++ b/doc/doxygen/doc_control_processor.h
> @@ -175,11 +175,7 @@
>    *    appropriate messages to be sent.
>    *
>    * @par Functions which control data channel key generation
> - *  - Key method 1 key exchange functions:
> - *     - \c key_method_1_write(), generates and processes key material to
> - *       be sent to the remote OpenVPN peer.
> - *     - \c key_method_1_read(), processes key material received from the
> - *       remote OpenVPN peer.
> + *  - Key method 1 key exchange functions were removed from OpenVPN 2.5
>    *  - Key method 2 key exchange functions:
>    *     - \c key_method_2_write(), generates and processes key material to
>    *       be sent to the remote OpenVPN peer.
> diff --git a/doc/doxygen/doc_key_generation.h b/doc/doxygen/doc_key_generation.h
> index efe61155..4bb9c708 100644
> --- a/doc/doxygen/doc_key_generation.h
> +++ b/doc/doxygen/doc_key_generation.h
> @@ -131,11 +131,7 @@ S_ACTIVE                                                            S_ACTIVE
>    * control_processor Control Channel Processor module's\endlink \c
>    * tls_process() function and control the %key generation and exchange
>    * process as follows:
> - * - %Key method 1:
> - *   - \c key_method_1_write(): generate random material locally, and load
> - *     as "sending" keys.
> - *   - \c key_method_1_read(): read random material received from remote
> - *     peer, and load as "receiving" keys.
> + * - %Key method 1 has been removed in OpenVPN 2.5
>    * - %Key method 2:
>    *   - \c key_method_2_write(): generate random material locally, and if
>    *     in server mode generate %key expansion.
> diff --git a/doc/doxygen/doc_protocol_overview.h b/doc/doxygen/doc_protocol_overview.h
> index 3f48b18a..08212223 100644
> --- a/doc/doxygen/doc_protocol_overview.h
> +++ b/doc/doxygen/doc_protocol_overview.h
> @@ -150,7 +150,7 @@
>    *
>    * @subsection network_protocol_control_plaintext Structure of plaintext control channel messages
>    *
> - *  - %Key method 1:
> + *  - %Key method 1 (support removed in OpenVPN 2.5):
>    *     - Cipher %key length in bytes (1 byte).
>    *     - Cipher %key (n bytes).
>    *     - HMAC %key length in bytes (1 byte).
> diff --git a/src/openvpn/forward.c b/src/openvpn/forward.c
> index 5c4370a8..698451d1 100644
> --- a/src/openvpn/forward.c
> +++ b/src/openvpn/forward.c
> @@ -1100,7 +1100,7 @@ process_incoming_link_part1(struct context *c, struct link_socket_info *lsi, boo
>                                   floated, &ad_start))
>               {
>                   /* Restore pre-NCP frame parameters */
> -                if (is_hard_reset(opcode, c->options.key_method))
> +                if (is_hard_reset_method2(opcode))
>                   {
>                       c->c2.frame = c->c2.frame_initial;
>   #ifdef ENABLE_FRAGMENT
> diff --git a/src/openvpn/helper.c b/src/openvpn/helper.c
> index 6e9cc63c..a1d03070 100644
> --- a/src/openvpn/helper.c
> +++ b/src/openvpn/helper.c
> @@ -490,11 +490,6 @@ helper_client_server(struct options *o)
>        */
>       if (o->client)
>       {
> -        if (o->key_method != 2)
> -        {
> -            msg(M_USAGE, "--client requires --key-method 2");
> -        }
> -
>           o->pull = true;
>           o->tls_client = true;
>       }
> diff --git a/src/openvpn/init.c b/src/openvpn/init.c
> index e9c01629..b96d1471 100644
> --- a/src/openvpn/init.c
> +++ b/src/openvpn/init.c
> @@ -2852,7 +2852,6 @@ do_init_crypto_tls(struct context *c, const unsigned int flags)
>       to.ssl_ctx = c->c1.ks.ssl_ctx;
>       to.key_type = c->c1.ks.key_type;
>       to.server = options->tls_server;
> -    to.key_method = options->key_method;
>       to.replay = options->replay;
>       to.replay_window = options->replay_window;
>       to.replay_time = options->replay_time;
> diff --git a/src/openvpn/options.c b/src/openvpn/options.c
> index 7da04b6f..14d4c911 100644
> --- a/src/openvpn/options.c
> +++ b/src/openvpn/options.c
> @@ -881,7 +881,6 @@ init_options(struct options *o, const bool init_gc)
>   #ifdef ENABLE_PREDICTION_RESISTANCE
>       o->use_prediction_resistance = false;
>   #endif
> -    o->key_method = 2;
>       o->tls_timeout = 2;
>       o->renegotiate_bytes = -1;
>       o->renegotiate_seconds = 3600;
> @@ -1719,7 +1718,6 @@ show_settings(const struct options *o)
>   
>       SHOW_BOOL(tls_server);
>       SHOW_BOOL(tls_client);
> -    SHOW_INT(key_method);
>       SHOW_STR_INLINE(ca_file);
>       SHOW_STR(ca_path);
>       SHOW_STR(dh_file);
> @@ -2380,10 +2378,6 @@ options_postprocess_verify_ce(const struct options *options, const struct connec
>           {
>               msg(M_USAGE, "--ccd-exclusive must be used with --client-config-dir");
>           }
> -        if (options->key_method != 2)
> -        {
> -            msg(M_USAGE, "--mode server requires --key-method 2");
> -        }
>           if (options->auth_token_generate && !options->renegotiate_seconds)
>           {
>               msg(M_USAGE, "--auth-gen-token needs a non-infinite "
> @@ -2550,13 +2544,6 @@ options_postprocess_verify_ce(const struct options *options, const struct connec
>               "may accept clients which do not present a certificate");
>       }
>   
> -    if (options->key_method == 1)
> -    {
> -        msg(M_WARN, "WARNING: --key-method 1 is deprecated and will be removed "
> -            "in OpenVPN 2.5.  By default --key-method 2 will be used if not set "
> -            "in the configuration file, which is the recommended approach.");
> -    }
> -
>       const int tls_version_max =
>           (options->ssl_flags >> SSLF_TLS_VERSION_MAX_SHIFT)
>           & SSLF_TLS_VERSION_MAX_MASK;
> @@ -2798,7 +2785,6 @@ options_postprocess_verify_ce(const struct options *options, const struct connec
>           MUST_BE_UNDEF(push_peer_info);
>           MUST_BE_UNDEF(tls_exit);
>           MUST_BE_UNDEF(crl_file);
> -        MUST_BE_UNDEF(key_method);
>           MUST_BE_UNDEF(ns_cert_type);
>           MUST_BE_UNDEF(remote_cert_ku[0]);
>           MUST_BE_UNDEF(remote_cert_eku);
> @@ -3827,10 +3813,7 @@ options_string(const struct options *o,
>                * tls-auth/tls-crypt does not match.  Removing tls-auth here would
>                * break stuff, so leaving that in place. */
>   
> -            if (o->key_method > 1)
> -            {
> -                buf_printf(&out, ",key-method %d", o->key_method);
> -            }
> +            buf_printf(&out, ",key-method %d", KEY_METHOD_2);
>           }
>   
>           if (remote)
> @@ -8476,22 +8459,6 @@ add_option(struct options *options,
>           VERIFY_PERMISSION(OPT_P_GENERAL);
>           options->tls_crypt_v2_verify_script = p[1];
>       }
> -    else if (streq(p[0], "key-method") && p[1] && !p[2])
> -    {
> -        int key_method;
> -
> -        VERIFY_PERMISSION(OPT_P_GENERAL);
> -        key_method = atoi(p[1]);
> -        if (key_method < KEY_METHOD_MIN || key_method > KEY_METHOD_MAX)
> -        {
> -            msg(msglevel, "key_method parameter (%d) must be >= %d and <= %d",
> -                key_method,
> -                KEY_METHOD_MIN,
> -                KEY_METHOD_MAX);
> -            goto err;
> -        }
> -        options->key_method = key_method;
> -    }
>       else if (streq(p[0], "x509-track") && p[1] && !p[2])
>       {
>           VERIFY_PERMISSION(OPT_P_GENERAL);
> diff --git a/src/openvpn/options.h b/src/openvpn/options.h
> index 1b038c91..3546bab3 100644
> --- a/src/openvpn/options.h
> +++ b/src/openvpn/options.h
> @@ -572,10 +572,6 @@ struct options
>   #ifdef ENABLE_CRYPTOAPI
>       const char *cryptoapi_cert;
>   #endif
> -
> -    /* data channel key exchange method */
> -    int key_method;
> -
>       /* Per-packet timeout on control channel */
>       int tls_timeout;
>   
> diff --git a/src/openvpn/ssl.c b/src/openvpn/ssl.c
> index 00b97352..ed35f792 100644
> --- a/src/openvpn/ssl.c
> +++ b/src/openvpn/ssl.c
> @@ -861,23 +861,12 @@ print_key_id(struct tls_multi *multi, struct gc_arena *gc)
>   }
>   
>   bool
> -is_hard_reset(int op, int key_method)
> +is_hard_reset_method2(int op)
>   {
> -    if (!key_method || key_method == 1)
> +    if (op == P_CONTROL_HARD_RESET_CLIENT_V2 || op == P_CONTROL_HARD_RESET_SERVER_V2
> +        || op == P_CONTROL_HARD_RESET_CLIENT_V3)
>       {
> -        if (op == P_CONTROL_HARD_RESET_CLIENT_V1 || op == P_CONTROL_HARD_RESET_SERVER_V1)
> -        {
> -            return true;
> -        }
> -    }
> -
> -    if (!key_method || key_method >= 2)
> -    {
> -        if (op == P_CONTROL_HARD_RESET_CLIENT_V2 || op == P_CONTROL_HARD_RESET_SERVER_V2
> -            || op == P_CONTROL_HARD_RESET_CLIENT_V3)
> -        {
> -            return true;
> -        }
> +        return true;
>       }
>   
>       return false;
> @@ -1097,23 +1086,14 @@ tls_session_init(struct tls_multi *multi, struct tls_session *session)
>       }
>   
>       /* Are we a TLS server or client? */
> -    ASSERT(session->opt->key_method >= 1);
> -    if (session->opt->key_method == 1)
> +    if (session->opt->server)
>       {
> -        session->initial_opcode = session->opt->server ?
> -                                  P_CONTROL_HARD_RESET_SERVER_V1 : P_CONTROL_HARD_RESET_CLIENT_V1;
> +        session->initial_opcode = P_CONTROL_HARD_RESET_SERVER_V2;
>       }
> -    else /* session->opt->key_method >= 2 */
> +    else
>       {
> -        if (session->opt->server)
> -        {
> -            session->initial_opcode = P_CONTROL_HARD_RESET_SERVER_V2;
> -        }
> -        else
> -        {
> -            session->initial_opcode = session->opt->tls_crypt_v2 ?
> -                                      P_CONTROL_HARD_RESET_CLIENT_V3 : P_CONTROL_HARD_RESET_CLIENT_V2;
> -        }
> +        session->initial_opcode = session->opt->tls_crypt_v2 ?
> +                                  P_CONTROL_HARD_RESET_CLIENT_V3 : P_CONTROL_HARD_RESET_CLIENT_V2;
>       }
>   
>       /* Initialize control channel authentication parameters */
> @@ -2225,52 +2205,6 @@ read_string_alloc(struct buffer *buf)
>       return str;
>   }
>   
> -/*
> - * Handle the reading and writing of key data to and from
> - * the TLS control channel (cleartext).
> - */
> -
> -static bool
> -key_method_1_write(struct buffer *buf, struct tls_session *session)
> -{
> -    struct key key;
> -    struct key_state *ks = &session->key[KS_PRIMARY];      /* primary key */
> -
> -    ASSERT(session->opt->key_method == 1);
> -    ASSERT(buf_init(buf, 0));
> -
> -    generate_key_random(&key, &session->opt->key_type);
> -    if (!check_key(&key, &session->opt->key_type))
> -    {
> -        msg(D_TLS_ERRORS, "TLS Error: Bad encrypting key generated");
> -        return false;
> -    }
> -
> -    if (!write_key(&key, &session->opt->key_type, buf))
> -    {
> -        msg(D_TLS_ERRORS, "TLS Error: write_key failed");
> -        return false;
> -    }
> -
> -    init_key_ctx(&ks->crypto_options.key_ctx_bi.encrypt, &key,
> -                 &session->opt->key_type, OPENVPN_OP_ENCRYPT,
> -                 "Data Channel Encrypt");
> -    secure_memzero(&key, sizeof(key));
> -
> -    /* send local options string */
> -    {
> -        const char *local_options = local_options_string(session);
> -        const int optlen = strlen(local_options) + 1;
> -        if (!buf_write(buf, local_options, optlen))
> -        {
> -            msg(D_TLS_ERRORS, "TLS Error: KM1 write options failed");
> -            return false;
> -        }
> -    }
> -
> -    return true;
> -}
> -
>   static bool
>   push_peer_info(struct buffer *buf, struct tls_session *session)
>   {
> @@ -2312,7 +2246,7 @@ push_peer_info(struct buffer *buf, struct tls_session *session)
>            * push request, also signal that the client wants
>            * to get push-reply messages without without requiring a round
>            * trip for a push request message*/
> -        if(session->opt->pull)
> +        if (session->opt->pull)
>           {
>               iv_proto |= IV_PROTO_REQUEST_PUSH;
>           }
> @@ -2389,12 +2323,15 @@ error:
>       return ret;
>   }
>   
> +/**
> + * Handle the writing of key data, peer-info, username/password, OCC
> + * to the TLS control channel (cleartext).
> + */
>   static bool
>   key_method_2_write(struct buffer *buf, struct tls_session *session)
>   {
>       struct key_state *ks = &session->key[KS_PRIMARY];      /* primary key */
>   
> -    ASSERT(session->opt->key_method == 2);
>       ASSERT(buf_init(buf, 0));
>   
>       /* write a uint32 0 */
> @@ -2404,7 +2341,7 @@ key_method_2_write(struct buffer *buf, struct tls_session *session)
>       }
>   
>       /* write key_method + flags */
> -    if (!buf_write_u8(buf, (session->opt->key_method & KEY_METHOD_MASK)))
> +    if (!buf_write_u8(buf, KEY_METHOD_2))
>       {
>           goto error;
>       }
> @@ -2506,73 +2443,15 @@ error:
>       return false;
>   }
>   
> -static bool
> -key_method_1_read(struct buffer *buf, struct tls_session *session)
> -{
> -    int status;
> -    struct key key;
> -    struct key_state *ks = &session->key[KS_PRIMARY];      /* primary key */
> -
> -    ASSERT(session->opt->key_method == 1);
> -
> -    if (!session->verified)
> -    {
> -        msg(D_TLS_ERRORS,
> -            "TLS Error: Certificate verification failed (key-method 1)");
> -        goto error;
> -    }
> -
> -    status = read_key(&key, &session->opt->key_type, buf);
> -    if (status != 1)
> -    {
> -        msg(D_TLS_ERRORS,
> -            "TLS Error: Error reading data channel key from plaintext buffer");
> -        goto error;
> -    }
> -
> -    if (!check_key(&key, &session->opt->key_type))
> -    {
> -        msg(D_TLS_ERRORS, "TLS Error: Bad decrypting key received from peer");
> -        goto error;
> -    }
> -
> -    if (buf->len < 1)
> -    {
> -        msg(D_TLS_ERRORS, "TLS Error: Missing options string");
> -        goto error;
> -    }
> -
> -#ifdef ENABLE_OCC
> -    /* compare received remote options string
> -     * with our locally computed options string */
> -    if (!session->opt->disable_occ
> -        && !options_cmp_equal_safe((char *) BPTR(buf), session->opt->remote_options, buf->len))
> -    {
> -        options_warning_safe((char *) BPTR(buf), session->opt->remote_options, buf->len);
> -    }
> -#endif
> -
> -    buf_clear(buf);
> -
> -    init_key_ctx(&ks->crypto_options.key_ctx_bi.decrypt, &key,
> -                 &session->opt->key_type, OPENVPN_OP_DECRYPT,
> -                 "Data Channel Decrypt");
> -    secure_memzero(&key, sizeof(key));
> -    ks->authenticated = KS_AUTH_TRUE;
> -    return true;
> -
> -error:
> -    buf_clear(buf);
> -    secure_memzero(&key, sizeof(key));
> -    return false;
> -}
> -
> +/**
> + * Handle reading key data, peer-info, username/password, OCC
> + * from the TLS control channel (cleartext).
> + */
>   static bool
>   key_method_2_read(struct buffer *buf, struct tls_multi *multi, struct tls_session *session)
>   {
>       struct key_state *ks = &session->key[KS_PRIMARY];      /* primary key */
>   
> -    int key_method_flags;
>       bool username_status, password_status;
>   
>       struct gc_arena gc = gc_new();
> @@ -2582,8 +2461,6 @@ key_method_2_read(struct buffer *buf, struct tls_multi *multi, struct tls_sessio
>       /* allocate temporary objects */
>       ALLOC_ARRAY_CLEAR_GC(options, char, TLS_OPTIONS_LEN, &gc);
>   
> -    ASSERT(session->opt->key_method == 2);
> -
>       /* discard leading uint32 */
>       if (!buf_advance(buf, 4))
>       {
> @@ -2593,7 +2470,7 @@ key_method_2_read(struct buffer *buf, struct tls_multi *multi, struct tls_sessio
>       }
>   
>       /* get key method */
> -    key_method_flags = buf_read_u8(buf);
> +    int key_method_flags = buf_read_u8(buf);
>       if ((key_method_flags & KEY_METHOD_MASK) != 2)
>       {
>           msg(D_TLS_ERRORS,
> @@ -3003,23 +2880,9 @@ tls_process(struct tls_multi *multi,
>           if (!buf->len && ((ks->state == S_START && !session->opt->server)
>                             || (ks->state == S_GOT_KEY && session->opt->server)))
>           {
> -            if (session->opt->key_method == 1)
> -            {
> -                if (!key_method_1_write(buf, session))
> -                {
> -                    goto error;
> -                }
> -            }
> -            else if (session->opt->key_method == 2)
> -            {
> -                if (!key_method_2_write(buf, session))
> -                {
> -                    goto error;
> -                }
> -            }
> -            else
> +            if (!key_method_2_write(buf, session))
>               {
> -                ASSERT(0);
> +                goto error;
>               }
>   
>               state_change = true;
> @@ -3033,23 +2896,9 @@ tls_process(struct tls_multi *multi,
>               && ((ks->state == S_SENT_KEY && !session->opt->server)
>                   || (ks->state == S_START && session->opt->server)))
>           {
> -            if (session->opt->key_method == 1)
> -            {
> -                if (!key_method_1_read(buf, session))
> -                {
> -                    goto error;
> -                }
> -            }
> -            else if (session->opt->key_method == 2)
> -            {
> -                if (!key_method_2_read(buf, multi, session))
> -                {
> -                    goto error;
> -                }
> -            }
> -            else
> +            if (!key_method_2_read(buf, multi, session))
>               {
> -                ASSERT(0);
> +                goto error;
>               }
>   
>               state_change = true;
> @@ -3463,6 +3312,11 @@ tls_pre_decrypt(struct tls_multi *multi,
>               /* verify legal opcode */
>               if (op < P_FIRST_OPCODE || op > P_LAST_OPCODE)
>               {
> +                if (op == P_CONTROL_HARD_RESET_CLIENT_V1
> +                    || op == P_CONTROL_HARD_RESET_SERVER_V1)
> +                {
> +                    msg(D_TLS_ERRORS, "Peer tried unsupported key-method 1");
> +                }
>                   msg(D_TLS_ERRORS,
>                       "TLS Error: unknown opcode received from %s op=%d",
>                       print_link_socket_actual(from, &gc), op);
> @@ -3470,14 +3324,12 @@ tls_pre_decrypt(struct tls_multi *multi,
>               }
>   
>               /* hard reset ? */
> -            if (is_hard_reset(op, 0))
> +            if (is_hard_reset_method2(op))
>               {
>                   /* verify client -> server or server -> client connection */
> -                if (((op == P_CONTROL_HARD_RESET_CLIENT_V1
> -                      || op == P_CONTROL_HARD_RESET_CLIENT_V2
> +                if (((op == P_CONTROL_HARD_RESET_CLIENT_V2
>                         || op == P_CONTROL_HARD_RESET_CLIENT_V3) && !multi->opt.server)
> -                    || ((op == P_CONTROL_HARD_RESET_SERVER_V1
> -                         || op == P_CONTROL_HARD_RESET_SERVER_V2) && multi->opt.server))
> +                    || ((op == P_CONTROL_HARD_RESET_SERVER_V2) && multi->opt.server))
>                   {
>                       msg(D_TLS_ERRORS,
>                           "TLS Error: client->client or server->server connection attempted from %s",
> @@ -3539,22 +3391,14 @@ tls_pre_decrypt(struct tls_multi *multi,
>               }
>   
>               /*
> -             * Initial packet received.
> +             * Hard reset and session id does not match any session in
> +             * multi->session: Possible initial packet
>                */
> -
> -            if (i == TM_SIZE && is_hard_reset(op, 0))
> +            if (i == TM_SIZE && is_hard_reset_method2(op))
>               {
>                   struct tls_session *session = &multi->session[TM_ACTIVE];
>                   struct key_state *ks = &session->key[KS_PRIMARY];
>   
> -                if (!is_hard_reset(op, multi->opt.key_method))
> -                {
> -                    msg(D_TLS_ERRORS, "TLS ERROR: initial packet local/remote key_method mismatch, local key_method=%d, op=%s",
> -                        multi->opt.key_method,
> -                        packet_opcode_name(op));
> -                    goto error;
> -                }
> -
>                   /*
>                    * If we have no session currently in progress, the initial packet will
>                    * open a new session in TM_ACTIVE rather than TM_UNTRUSTED.
> @@ -3594,7 +3438,11 @@ tls_pre_decrypt(struct tls_multi *multi,
>                   }
>               }
>   
> -            if (i == TM_SIZE && is_hard_reset(op, 0))
> +            /*
> +             * If we detected new session in the last if block, i has

block, i has ->
block, it has


> +             * changed to TM_ACTIVE, so check the condition again.
> +             */
> +            if (i == TM_SIZE && is_hard_reset_method2(op))
>               {
>                   /*
>                    * No match with existing sessions,
> @@ -3614,14 +3462,6 @@ tls_pre_decrypt(struct tls_multi *multi,
>                       goto error;
>                   }
>   
> -                if (!is_hard_reset(op, multi->opt.key_method))
> -                {
> -                    msg(D_TLS_ERRORS, "TLS ERROR: new session local/remote key_method mismatch, local key_method=%d, op=%s",
> -                        multi->opt.key_method,
> -                        packet_opcode_name(op));
> -                    goto error;
> -                }
> -
>                   if (!read_control_auth(buf, &session->tls_wrap, from,
>                                          session->opt))
>                   {
> diff --git a/src/openvpn/ssl.h b/src/openvpn/ssl.h
> index 81f8810e..4f4f4bd5 100644
> --- a/src/openvpn/ssl.h
> +++ b/src/openvpn/ssl.h
> @@ -66,8 +66,10 @@
>   /* indicates key_method >= 2 and client-specific tls-crypt key */
>   #define P_CONTROL_HARD_RESET_CLIENT_V3 10    /* initial key from client, forget previous state */
>   
> -/* define the range of legal opcodes */
> -#define P_FIRST_OPCODE                 1
> +/* define the range of legal opcodes
> + * Since we do no longer support key-method 1 we consider
> + * the v1 op codes invalid */
> +#define P_FIRST_OPCODE                 3
>   #define P_LAST_OPCODE                  10
>   
>   /*
> @@ -118,11 +120,7 @@
>   /* Default field in X509 to be username */
>   #define X509_USERNAME_FIELD_DEFAULT "CN"
>   
> -/*
> - * Range of key exchange methods
> - */
> -#define KEY_METHOD_MIN 1
> -#define KEY_METHOD_MAX 2
> +#define KEY_METHOD_2  2
>   
>   /* key method taken from lower 4 bits */
>   #define KEY_METHOD_MASK 0x0F
> @@ -594,12 +592,11 @@ void show_tls_performance_stats(void);
>   void extract_x509_field_test(void);
>   
>   /**
> - * Given a key_method, return true if opcode represents the required form of
> - * hard_reset.
> + * Given a key_method, return true if opcode represents the one of the
> + * hard_reset op codes for key-method 2
>    *
> - * If key_method == 0, return true if any form of hard reset is used.
>    */
> -bool is_hard_reset(int op, int key_method);
> +bool is_hard_reset_method2(int op);
>   
>   void delayed_auth_pass_purge(void);
>   
> diff --git a/src/openvpn/ssl_common.h b/src/openvpn/ssl_common.h
> index e0b3ee56..d904c31f 100644
> --- a/src/openvpn/ssl_common.h
> +++ b/src/openvpn/ssl_common.h
> @@ -262,7 +262,6 @@ struct tls_options
>   #endif
>   
>       /* from command line */
> -    int key_method;
>       bool replay;
>       bool single_session;
>   #ifdef ENABLE_OCC
> 

[Openvpn-devel] [PATCH v3 5/9] Remove key-method 1

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

Key-method 1 is only needed to talk to pre OpenVPN 2.0 clients.

Patch V2: Fix style. Make V1 op codes illegal, remove all code handling
          v1 op codes and give a good warning message if we encounter
          them in the legal op codes pre-check.

Patch V3: Add a bit more comments in the existing methods.

Signed-off-by: Arne Schwabe <ar...@rf...>
---
 doc/doxygen/doc_control_processor.h |   6 +-
 doc/doxygen/doc_key_generation.h    |   6 +-
 doc/doxygen/doc_protocol_overview.h |   2 +-
 src/openvpn/forward.c               |   2 +-
 src/openvpn/helper.c                |   5 -
 src/openvpn/init.c                  |   1 -
 src/openvpn/options.c               |  35 +---
 src/openvpn/options.h               |   4 -
 src/openvpn/ssl.c                   | 240 +++++-----------------------
 src/openvpn/ssl.h                   |  19 +--
 src/openvpn/ssl_common.h            |   1 -
 11 files changed, 53 insertions(+), 268 deletions(-)

diff --git a/doc/doxygen/doc_control_processor.h b/doc/doxygen/doc_control_processor.h
index f87324cc..1bbf2d2d 100644
--- a/doc/doxygen/doc_control_processor.h
+++ b/doc/doxygen/doc_control_processor.h
@@ -175,11 +175,7 @@
  *    appropriate messages to be sent.
  *
  * @par Functions which control data channel key generation
- *  - Key method 1 key exchange functions:
- *     - \c key_method_1_write(), generates and processes key material to
- *       be sent to the remote OpenVPN peer.
- *     - \c key_method_1_read(), processes key material received from the
- *       remote OpenVPN peer.
+ *  - Key method 1 key exchange functions were removed from OpenVPN 2.5
  *  - Key method 2 key exchange functions:
  *     - \c key_method_2_write(), generates and processes key material to
  *       be sent to the remote OpenVPN peer.
diff --git a/doc/doxygen/doc_key_generation.h b/doc/doxygen/doc_key_generation.h
index efe61155..4bb9c708 100644
--- a/doc/doxygen/doc_key_generation.h
+++ b/doc/doxygen/doc_key_generation.h
@@ -131,11 +131,7 @@ S_ACTIVE                                                            S_ACTIVE
  * control_processor Control Channel Processor module's\endlink \c
  * tls_process() function and control the %key generation and exchange
  * process as follows:
- * - %Key method 1:
- *   - \c key_method_1_write(): generate random material locally, and load
- *     as "sending" keys.
- *   - \c key_method_1_read(): read random material received from remote
- *     peer, and load as "receiving" keys.
+ * - %Key method 1 has been removed in OpenVPN 2.5
  * - %Key method 2:
  *   - \c key_method_2_write(): generate random material locally, and if
  *     in server mode generate %key expansion.
diff --git a/doc/doxygen/doc_protocol_overview.h b/doc/doxygen/doc_protocol_overview.h
index 3f48b18a..08212223 100644
--- a/doc/doxygen/doc_protocol_overview.h
+++ b/doc/doxygen/doc_protocol_overview.h
@@ -150,7 +150,7 @@
  *
  * @subsection network_protocol_control_plaintext Structure of plaintext control channel messages
  *
- *  - %Key method 1:
+ *  - %Key method 1 (support removed in OpenVPN 2.5):
  *     - Cipher %key length in bytes (1 byte).
  *     - Cipher %key (n bytes).
  *     - HMAC %key length in bytes (1 byte).
diff --git a/src/openvpn/forward.c b/src/openvpn/forward.c
index 5c4370a8..698451d1 100644
--- a/src/openvpn/forward.c
+++ b/src/openvpn/forward.c
@@ -1100,7 +1100,7 @@ process_incoming_link_part1(struct context *c, struct link_socket_info *lsi, boo
                                 floated, &ad_start))
             {
                 /* Restore pre-NCP frame parameters */
-                if (is_hard_reset(opcode, c->options.key_method))
+                if (is_hard_reset_method2(opcode))
                 {
                     c->c2.frame = c->c2.frame_initial;
 #ifdef ENABLE_FRAGMENT
diff --git a/src/openvpn/helper.c b/src/openvpn/helper.c
index 6e9cc63c..a1d03070 100644
--- a/src/openvpn/helper.c
+++ b/src/openvpn/helper.c
@@ -490,11 +490,6 @@ helper_client_server(struct options *o)
      */
     if (o->client)
     {
-        if (o->key_method != 2)
-        {
-            msg(M_USAGE, "--client requires --key-method 2");
-        }
-
         o->pull = true;
         o->tls_client = true;
     }
diff --git a/src/openvpn/init.c b/src/openvpn/init.c
index e9c01629..b96d1471 100644
--- a/src/openvpn/init.c
+++ b/src/openvpn/init.c
@@ -2852,7 +2852,6 @@ do_init_crypto_tls(struct context *c, const unsigned int flags)
     to.ssl_ctx = c->c1.ks.ssl_ctx;
     to.key_type = c->c1.ks.key_type;
     to.server = options->tls_server;
-    to.key_method = options->key_method;
     to.replay = options->replay;
     to.replay_window = options->replay_window;
     to.replay_time = options->replay_time;
diff --git a/src/openvpn/options.c b/src/openvpn/options.c
index 7da04b6f..14d4c911 100644
--- a/src/openvpn/options.c
+++ b/src/openvpn/options.c
@@ -881,7 +881,6 @@ init_options(struct options *o, const bool init_gc)
 #ifdef ENABLE_PREDICTION_RESISTANCE
     o->use_prediction_resistance = false;
 #endif
-    o->key_method = 2;
     o->tls_timeout = 2;
     o->renegotiate_bytes = -1;
     o->renegotiate_seconds = 3600;
@@ -1719,7 +1718,6 @@ show_settings(const struct options *o)
 
     SHOW_BOOL(tls_server);
     SHOW_BOOL(tls_client);
-    SHOW_INT(key_method);
     SHOW_STR_INLINE(ca_file);
     SHOW_STR(ca_path);
     SHOW_STR(dh_file);
@@ -2380,10 +2378,6 @@ options_postprocess_verify_ce(const struct options *options, const struct connec
         {
             msg(M_USAGE, "--ccd-exclusive must be used with --client-config-dir");
         }
-        if (options->key_method != 2)
-        {
-            msg(M_USAGE, "--mode server requires --key-method 2");
-        }
         if (options->auth_token_generate && !options->renegotiate_seconds)
         {
             msg(M_USAGE, "--auth-gen-token needs a non-infinite "
@@ -2550,13 +2544,6 @@ options_postprocess_verify_ce(const struct options *options, const struct connec
             "may accept clients which do not present a certificate");
     }
 
-    if (options->key_method == 1)
-    {
-        msg(M_WARN, "WARNING: --key-method 1 is deprecated and will be removed "
-            "in OpenVPN 2.5.  By default --key-method 2 will be used if not set "
-            "in the configuration file, which is the recommended approach.");
-    }
-
     const int tls_version_max =
         (options->ssl_flags >> SSLF_TLS_VERSION_MAX_SHIFT)
         & SSLF_TLS_VERSION_MAX_MASK;
@@ -2798,7 +2785,6 @@ options_postprocess_verify_ce(const struct options *options, const struct connec
         MUST_BE_UNDEF(push_peer_info);
         MUST_BE_UNDEF(tls_exit);
         MUST_BE_UNDEF(crl_file);
-        MUST_BE_UNDEF(key_method);
         MUST_BE_UNDEF(ns_cert_type);
         MUST_BE_UNDEF(remote_cert_ku[0]);
         MUST_BE_UNDEF(remote_cert_eku);
@@ -3827,10 +3813,7 @@ options_string(const struct options *o,
              * tls-auth/tls-crypt does not match.  Removing tls-auth here would
              * break stuff, so leaving that in place. */
 
-            if (o->key_method > 1)
-            {
-                buf_printf(&out, ",key-method %d", o->key_method);
-            }
+            buf_printf(&out, ",key-method %d", KEY_METHOD_2);
         }
 
         if (remote)
@@ -8476,22 +8459,6 @@ add_option(struct options *options,
         VERIFY_PERMISSION(OPT_P_GENERAL);
         options->tls_crypt_v2_verify_script = p[1];
     }
-    else if (streq(p[0], "key-method") && p[1] && !p[2])
-    {
-        int key_method;
-
-        VERIFY_PERMISSION(OPT_P_GENERAL);
-        key_method = atoi(p[1]);
-        if (key_method < KEY_METHOD_MIN || key_method > KEY_METHOD_MAX)
-        {
-            msg(msglevel, "key_method parameter (%d) must be >= %d and <= %d",
-                key_method,
-                KEY_METHOD_MIN,
-                KEY_METHOD_MAX);
-            goto err;
-        }
-        options->key_method = key_method;
-    }
     else if (streq(p[0], "x509-track") && p[1] && !p[2])
     {
         VERIFY_PERMISSION(OPT_P_GENERAL);
diff --git a/src/openvpn/options.h b/src/openvpn/options.h
index 1b038c91..3546bab3 100644
--- a/src/openvpn/options.h
+++ b/src/openvpn/options.h
@@ -572,10 +572,6 @@ struct options
 #ifdef ENABLE_CRYPTOAPI
     const char *cryptoapi_cert;
 #endif
-
-    /* data channel key exchange method */
-    int key_method;
-
     /* Per-packet timeout on control channel */
     int tls_timeout;
 
diff --git a/src/openvpn/ssl.c b/src/openvpn/ssl.c
index 00b97352..ed35f792 100644
--- a/src/openvpn/ssl.c
+++ b/src/openvpn/ssl.c
@@ -861,23 +861,12 @@ print_key_id(struct tls_multi *multi, struct gc_arena *gc)
 }
 
 bool
-is_hard_reset(int op, int key_method)
+is_hard_reset_method2(int op)
 {
-    if (!key_method || key_method == 1)
+    if (op == P_CONTROL_HARD_RESET_CLIENT_V2 || op == P_CONTROL_HARD_RESET_SERVER_V2
+        || op == P_CONTROL_HARD_RESET_CLIENT_V3)
     {
-        if (op == P_CONTROL_HARD_RESET_CLIENT_V1 || op == P_CONTROL_HARD_RESET_SERVER_V1)
-        {
-            return true;
-        }
-    }
-
-    if (!key_method || key_method >= 2)
-    {
-        if (op == P_CONTROL_HARD_RESET_CLIENT_V2 || op == P_CONTROL_HARD_RESET_SERVER_V2
-            || op == P_CONTROL_HARD_RESET_CLIENT_V3)
-        {
-            return true;
-        }
+        return true;
     }
 
     return false;
@@ -1097,23 +1086,14 @@ tls_session_init(struct tls_multi *multi, struct tls_session *session)
     }
 
     /* Are we a TLS server or client? */
-    ASSERT(session->opt->key_method >= 1);
-    if (session->opt->key_method == 1)
+    if (session->opt->server)
     {
-        session->initial_opcode = session->opt->server ?
-                                  P_CONTROL_HARD_RESET_SERVER_V1 : P_CONTROL_HARD_RESET_CLIENT_V1;
+        session->initial_opcode = P_CONTROL_HARD_RESET_SERVER_V2;
     }
-    else /* session->opt->key_method >= 2 */
+    else
     {
-        if (session->opt->server)
-        {
-            session->initial_opcode = P_CONTROL_HARD_RESET_SERVER_V2;
-        }
-        else
-        {
-            session->initial_opcode = session->opt->tls_crypt_v2 ?
-                                      P_CONTROL_HARD_RESET_CLIENT_V3 : P_CONTROL_HARD_RESET_CLIENT_V2;
-        }
+        session->initial_opcode = session->opt->tls_crypt_v2 ?
+                                  P_CONTROL_HARD_RESET_CLIENT_V3 : P_CONTROL_HARD_RESET_CLIENT_V2;
     }
 
     /* Initialize control channel authentication parameters */
@@ -2225,52 +2205,6 @@ read_string_alloc(struct buffer *buf)
     return str;
 }
 
-/*
- * Handle the reading and writing of key data to and from
- * the TLS control channel (cleartext).
- */
-
-static bool
-key_method_1_write(struct buffer *buf, struct tls_session *session)
-{
-    struct key key;
-    struct key_state *ks = &session->key[KS_PRIMARY];      /* primary key */
-
-    ASSERT(session->opt->key_method == 1);
-    ASSERT(buf_init(buf, 0));
-
-    generate_key_random(&key, &session->opt->key_type);
-    if (!check_key(&key, &session->opt->key_type))
-    {
-        msg(D_TLS_ERRORS, "TLS Error: Bad encrypting key generated");
-        return false;
-    }
-
-    if (!write_key(&key, &session->opt->key_type, buf))
-    {
-        msg(D_TLS_ERRORS, "TLS Error: write_key failed");
-        return false;
-    }
-
-    init_key_ctx(&ks->crypto_options.key_ctx_bi.encrypt, &key,
-                 &session->opt->key_type, OPENVPN_OP_ENCRYPT,
-                 "Data Channel Encrypt");
-    secure_memzero(&key, sizeof(key));
-
-    /* send local options string */
-    {
-        const char *local_options = local_options_string(session);
-        const int optlen = strlen(local_options) + 1;
-        if (!buf_write(buf, local_options, optlen))
-        {
-            msg(D_TLS_ERRORS, "TLS Error: KM1 write options failed");
-            return false;
-        }
-    }
-
-    return true;
-}
-
 static bool
 push_peer_info(struct buffer *buf, struct tls_session *session)
 {
@@ -2312,7 +2246,7 @@ push_peer_info(struct buffer *buf, struct tls_session *session)
          * push request, also signal that the client wants
          * to get push-reply messages without without requiring a round
          * trip for a push request message*/
-        if(session->opt->pull)
+        if (session->opt->pull)
         {
             iv_proto |= IV_PROTO_REQUEST_PUSH;
         }
@@ -2389,12 +2323,15 @@ error:
     return ret;
 }
 
+/**
+ * Handle the writing of key data, peer-info, username/password, OCC
+ * to the TLS control channel (cleartext).
+ */
 static bool
 key_method_2_write(struct buffer *buf, struct tls_session *session)
 {
     struct key_state *ks = &session->key[KS_PRIMARY];      /* primary key */
 
-    ASSERT(session->opt->key_method == 2);
     ASSERT(buf_init(buf, 0));
 
     /* write a uint32 0 */
@@ -2404,7 +2341,7 @@ key_method_2_write(struct buffer *buf, struct tls_session *session)
     }
 
     /* write key_method + flags */
-    if (!buf_write_u8(buf, (session->opt->key_method & KEY_METHOD_MASK)))
+    if (!buf_write_u8(buf, KEY_METHOD_2))
     {
         goto error;
     }
@@ -2506,73 +2443,15 @@ error:
     return false;
 }
 
-static bool
-key_method_1_read(struct buffer *buf, struct tls_session *session)
-{
-    int status;
-    struct key key;
-    struct key_state *ks = &session->key[KS_PRIMARY];      /* primary key */
-
-    ASSERT(session->opt->key_method == 1);
-
-    if (!session->verified)
-    {
-        msg(D_TLS_ERRORS,
-            "TLS Error: Certificate verification failed (key-method 1)");
-        goto error;
-    }
-
-    status = read_key(&key, &session->opt->key_type, buf);
-    if (status != 1)
-    {
-        msg(D_TLS_ERRORS,
-            "TLS Error: Error reading data channel key from plaintext buffer");
-        goto error;
-    }
-
-    if (!check_key(&key, &session->opt->key_type))
-    {
-        msg(D_TLS_ERRORS, "TLS Error: Bad decrypting key received from peer");
-        goto error;
-    }
-
-    if (buf->len < 1)
-    {
-        msg(D_TLS_ERRORS, "TLS Error: Missing options string");
-        goto error;
-    }
-
-#ifdef ENABLE_OCC
-    /* compare received remote options string
-     * with our locally computed options string */
-    if (!session->opt->disable_occ
-        && !options_cmp_equal_safe((char *) BPTR(buf), session->opt->remote_options, buf->len))
-    {
-        options_warning_safe((char *) BPTR(buf), session->opt->remote_options, buf->len);
-    }
-#endif
-
-    buf_clear(buf);
-
-    init_key_ctx(&ks->crypto_options.key_ctx_bi.decrypt, &key,
-                 &session->opt->key_type, OPENVPN_OP_DECRYPT,
-                 "Data Channel Decrypt");
-    secure_memzero(&key, sizeof(key));
-    ks->authenticated = KS_AUTH_TRUE;
-    return true;
-
-error:
-    buf_clear(buf);
-    secure_memzero(&key, sizeof(key));
-    return false;
-}
-
+/**
+ * Handle reading key data, peer-info, username/password, OCC
+ * from the TLS control channel (cleartext).
+ */
 static bool
 key_method_2_read(struct buffer *buf, struct tls_multi *multi, struct tls_session *session)
 {
     struct key_state *ks = &session->key[KS_PRIMARY];      /* primary key */
 
-    int key_method_flags;
     bool username_status, password_status;
 
     struct gc_arena gc = gc_new();
@@ -2582,8 +2461,6 @@ key_method_2_read(struct buffer *buf, struct tls_multi *multi, struct tls_sessio
     /* allocate temporary objects */
     ALLOC_ARRAY_CLEAR_GC(options, char, TLS_OPTIONS_LEN, &gc);
 
-    ASSERT(session->opt->key_method == 2);
-
     /* discard leading uint32 */
     if (!buf_advance(buf, 4))
     {
@@ -2593,7 +2470,7 @@ key_method_2_read(struct buffer *buf, struct tls_multi *multi, struct tls_sessio
     }
 
     /* get key method */
-    key_method_flags = buf_read_u8(buf);
+    int key_method_flags = buf_read_u8(buf);
     if ((key_method_flags & KEY_METHOD_MASK) != 2)
     {
         msg(D_TLS_ERRORS,
@@ -3003,23 +2880,9 @@ tls_process(struct tls_multi *multi,
         if (!buf->len && ((ks->state == S_START && !session->opt->server)
                           || (ks->state == S_GOT_KEY && session->opt->server)))
         {
-            if (session->opt->key_method == 1)
-            {
-                if (!key_method_1_write(buf, session))
-                {
-                    goto error;
-                }
-            }
-            else if (session->opt->key_method == 2)
-            {
-                if (!key_method_2_write(buf, session))
-                {
-                    goto error;
-                }
-            }
-            else
+            if (!key_method_2_write(buf, session))
             {
-                ASSERT(0);
+                goto error;
             }
 
             state_change = true;
@@ -3033,23 +2896,9 @@ tls_process(struct tls_multi *multi,
             && ((ks->state == S_SENT_KEY && !session->opt->server)
                 || (ks->state == S_START && session->opt->server)))
         {
-            if (session->opt->key_method == 1)
-            {
-                if (!key_method_1_read(buf, session))
-                {
-                    goto error;
-                }
-            }
-            else if (session->opt->key_method == 2)
-            {
-                if (!key_method_2_read(buf, multi, session))
-                {
-                    goto error;
-                }
-            }
-            else
+            if (!key_method_2_read(buf, multi, session))
             {
-                ASSERT(0);
+                goto error;
             }
 
             state_change = true;
@@ -3463,6 +3312,11 @@ tls_pre_decrypt(struct tls_multi *multi,
             /* verify legal opcode */
             if (op < P_FIRST_OPCODE || op > P_LAST_OPCODE)
             {
+                if (op == P_CONTROL_HARD_RESET_CLIENT_V1
+                    || op == P_CONTROL_HARD_RESET_SERVER_V1)
+                {
+                    msg(D_TLS_ERRORS, "Peer tried unsupported key-method 1");
+                }
                 msg(D_TLS_ERRORS,
                     "TLS Error: unknown opcode received from %s op=%d",
                     print_link_socket_actual(from, &gc), op);
@@ -3470,14 +3324,12 @@ tls_pre_decrypt(struct tls_multi *multi,
             }
 
             /* hard reset ? */
-            if (is_hard_reset(op, 0))
+            if (is_hard_reset_method2(op))
             {
                 /* verify client -> server or server -> client connection */
-                if (((op == P_CONTROL_HARD_RESET_CLIENT_V1
-                      || op == P_CONTROL_HARD_RESET_CLIENT_V2
+                if (((op == P_CONTROL_HARD_RESET_CLIENT_V2
                       || op == P_CONTROL_HARD_RESET_CLIENT_V3) && !multi->opt.server)
-                    || ((op == P_CONTROL_HARD_RESET_SERVER_V1
-                         || op == P_CONTROL_HARD_RESET_SERVER_V2) && multi->opt.server))
+                    || ((op == P_CONTROL_HARD_RESET_SERVER_V2) && multi->opt.server))
                 {
                     msg(D_TLS_ERRORS,
                         "TLS Error: client->client or server->server connection attempted from %s",
@@ -3539,22 +3391,14 @@ tls_pre_decrypt(struct tls_multi *multi,
             }
 
             /*
-             * Initial packet received.
+             * Hard reset and session id does not match any session in
+             * multi->session: Possible initial packet
              */
-
-            if (i == TM_SIZE && is_hard_reset(op, 0))
+            if (i == TM_SIZE && is_hard_reset_method2(op))
             {
                 struct tls_session *session = &multi->session[TM_ACTIVE];
                 struct key_state *ks = &session->key[KS_PRIMARY];
 
-                if (!is_hard_reset(op, multi->opt.key_method))
-                {
-                    msg(D_TLS_ERRORS, "TLS ERROR: initial packet local/remote key_method mismatch, local key_method=%d, op=%s",
-                        multi->opt.key_method,
-                        packet_opcode_name(op));
-                    goto error;
-                }
-
                 /*
                  * If we have no session currently in progress, the initial packet will
                  * open a new session in TM_ACTIVE rather than TM_UNTRUSTED.
@@ -3594,7 +3438,11 @@ tls_pre_decrypt(struct tls_multi *multi,
                 }
             }
 
-            if (i == TM_SIZE && is_hard_reset(op, 0))
+            /*
+             * If we detected new session in the last if block, i has
+             * changed to TM_ACTIVE, so check the condition again.
+             */
+            if (i == TM_SIZE && is_hard_reset_method2(op))
             {
                 /*
                  * No match with existing sessions,
@@ -3614,14 +3462,6 @@ tls_pre_decrypt(struct tls_multi *multi,
                     goto error;
                 }
 
-                if (!is_hard_reset(op, multi->opt.key_method))
-                {
-                    msg(D_TLS_ERRORS, "TLS ERROR: new session local/remote key_method mismatch, local key_method=%d, op=%s",
-                        multi->opt.key_method,
-                        packet_opcode_name(op));
-                    goto error;
-                }
-
                 if (!read_control_auth(buf, &session->tls_wrap, from,
                                        session->opt))
                 {
diff --git a/src/openvpn/ssl.h b/src/openvpn/ssl.h
index 81f8810e..4f4f4bd5 100644
--- a/src/openvpn/ssl.h
+++ b/src/openvpn/ssl.h
@@ -66,8 +66,10 @@
 /* indicates key_method >= 2 and client-specific tls-crypt key */
 #define P_CONTROL_HARD_RESET_CLIENT_V3 10    /* initial key from client, forget previous state */
 
-/* define the range of legal opcodes */
-#define P_FIRST_OPCODE                 1
+/* define the range of legal opcodes
+ * Since we do no longer support key-method 1 we consider
+ * the v1 op codes invalid */
+#define P_FIRST_OPCODE                 3
 #define P_LAST_OPCODE                  10
 
 /*
@@ -118,11 +120,7 @@
 /* Default field in X509 to be username */
 #define X509_USERNAME_FIELD_DEFAULT "CN"
 
-/*
- * Range of key exchange methods
- */
-#define KEY_METHOD_MIN 1
-#define KEY_METHOD_MAX 2
+#define KEY_METHOD_2  2
 
 /* key method taken from lower 4 bits */
 #define KEY_METHOD_MASK 0x0F
@@ -594,12 +592,11 @@ void show_tls_performance_stats(void);
 void extract_x509_field_test(void);
 
 /**
- * Given a key_method, return true if opcode represents the required form of
- * hard_reset.
+ * Given a key_method, return true if opcode represents the one of the
+ * hard_reset op codes for key-method 2
  *
- * If key_method == 0, return true if any form of hard reset is used.
  */
-bool is_hard_reset(int op, int key_method);
+bool is_hard_reset_method2(int op);
 
 void delayed_auth_pass_purge(void);
 
diff --git a/src/openvpn/ssl_common.h b/src/openvpn/ssl_common.h
index e0b3ee56..d904c31f 100644
--- a/src/openvpn/ssl_common.h
+++ b/src/openvpn/ssl_common.h
@@ -262,7 +262,6 @@ struct tls_options
 #endif
 
     /* from command line */
-    int key_method;
     bool replay;
     bool single_session;
 #ifdef ENABLE_OCC
-- 
2.26.2

[Openvpn-devel] [PATCH applied] Re: Require AEAD support in the crypto library

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

Quick sanity check on FreeBSD/1.0.2s, Linux/1.1.1g, Linux/mbedTLS
(client side only).  All good.

Your patch has been applied to the master branch.

commit 94edc7c5dd3cf8988df15fe4d7bd6cba9486b2a6
Author: Arne Schwabe
Date:   Mon Jul 20 14:17:04 2020 +0200

     Require AEAD support in the crypto library

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


--
kind regards,

Gert Doering

[Openvpn-devel] [PATCH applied] Re: Drop support for OpenSSL 1.0.1

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

Your patch has been applied to the master branch.

Whitespace fixed on-the-go.

Sanity-tested on Linux / 1.1.1g and FreeBSD / 1.0.2s (client only).

commit ec7d0e8e0f8cd8f1c5fab58c795a59828eba6ae7
Author: Arne Schwabe
Date:   Fri Jul 17 15:47:32 2020 +0200

     Drop support for OpenSSL 1.0.1

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


--
kind regards,

Gert Doering

Re: [Openvpn-devel] [PATCH v2 5/9] Remove key-method 1

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

On 20/07/2020 15:22, Arne Schwabe wrote:
> Am 20.07.20 um 15:16 schrieb David Sommerseth:
>> On 17/07/2020 15:47, Arne Schwabe wrote:
>>> Key-method 1 is only needed to talk to pre OpenVPN 2.0 clients.
>>>
>>> Patch V2: Fix style. Make V1 op codes illegal, remove all code handling
>>>           v1 op codes and give a good warning message if we encounter
>>>           them in the legal op codes pre-check.
>>>
>>> Signed-off-by: Arne Schwabe <ar...@rf...>
>>> ---
>>>  doc/doxygen/doc_control_processor.h |   6 +-
>>>  doc/doxygen/doc_key_generation.h    |   6 +-
>>>  doc/doxygen/doc_protocol_overview.h |   2 +-
>>>  src/openvpn/forward.c               |   2 +-
>>>  src/openvpn/helper.c                |   5 -
>>>  src/openvpn/init.c                  |   1 -
>>>  src/openvpn/options.c               |  35 +----
>>>  src/openvpn/options.h               |   4 -
>>>  src/openvpn/ssl.c                   | 230 ++++------------------------
>>>  src/openvpn/ssl.h                   |  19 +--
>>>  src/openvpn/ssl_common.h            |   1 -
>>>  11 files changed, 42 insertions(+), 269 deletions(-)
>>
>> [...snip...]
>>
>>> diff --git a/src/openvpn/ssl.c b/src/openvpn/ssl.c
>>> index 00b97352..4144217d 100644
>>> --- a/src/openvpn/ssl.c
>>> +++ b/src/openvpn/ssl.c
>>
>> [...snip...]
>>
>>> @@ -2225,52 +2205,6 @@ read_string_alloc(struct buffer *buf)
>>>      return str;
>>>  }
>>>  
>>> -/*
>>> - * Handle the reading and writing of key data to and from
>>> - * the TLS control channel (cleartext).
>>> - */
>>
>> Is it needed to remove this comment?  Or move it after the push_peer_info()
>> function?
> 
> Yeah, we can move the comment.

Yes, please ... Since it's not a bad comment, I don't like loosing helpful
pointers in comments :)

>>
>>> -static bool
>>> -key_method_1_write(struct buffer *buf, struct tls_session *session)
>>> -{
>> [...snip...]
>>> -}
>>> -
>>>  static bool
>>>  push_peer_info(struct buffer *buf, struct tls_session *session)
>>>  {
>>> @@ -2312,7 +2246,7 @@ push_peer_info(struct buffer *buf, struct tls_session *session)
>>>           * push request, also signal that the client wants
>>>           * to get push-reply messages without without requiring a round
>>>           * trip for a push request message*/
>>> -        if(session->opt->pull)
>>> +        if (session->opt->pull)
>>>          {
>>>              iv_proto |= IV_PROTO_REQUEST_PUSH;
>>>          }
>>> @@ -2394,7 +2328,6 @@ key_method_2_write(struct buffer *buf, struct tls_session *session)
>>
>> [...snip...]
>>
>>> @@ -3470,14 +3316,12 @@ tls_pre_decrypt(struct tls_multi *multi,
>>>              }
>>>  
>>>              /* hard reset ? */
>>> -            if (is_hard_reset(op, 0))
>>> +            if (is_hard_reset_method2(op))
>>>              {
>>>                  /* verify client -> server or server -> client connection */
>>> -                if (((op == P_CONTROL_HARD_RESET_CLIENT_V1
>>> -                      || op == P_CONTROL_HARD_RESET_CLIENT_V2
>>> +                if (((op == P_CONTROL_HARD_RESET_CLIENT_V2
>>>                        || op == P_CONTROL_HARD_RESET_CLIENT_V3) && !multi->opt.server)
>>> -                    || ((op == P_CONTROL_HARD_RESET_SERVER_V1
>>> -                         || op == P_CONTROL_HARD_RESET_SERVER_V2) && multi->opt.server))
>>> +                    || ((op == P_CONTROL_HARD_RESET_SERVER_V2) && multi->opt.server))
>>>                  {
>>>                      msg(D_TLS_ERRORS,
>>>                          "TLS Error: client->client or server->server connection attempted from %s",
>>> @@ -3542,19 +3386,11 @@ tls_pre_decrypt(struct tls_multi *multi,
>>>               * Initial packet received.
>>>               */
>>>  
>>> -            if (i == TM_SIZE && is_hard_reset(op, 0))
>>> +            if (i == TM_SIZE && is_hard_reset_method2(op))
>>>              {
>>>                  struct tls_session *session = &multi->session[TM_ACTIVE];
>>>                  struct key_state *ks = &session->key[KS_PRIMARY];
>>>  
>>> -                if (!is_hard_reset(op, multi->opt.key_method))
>>> -                {
>>> -                    msg(D_TLS_ERRORS, "TLS ERROR: initial packet local/remote key_method mismatch, local key_method=%d, op=%s",
>>> -                        multi->opt.key_method,
>>> -                        packet_opcode_name(op));
>>> -                    goto error;
>>> -                }
>>> -
>>>                  /*
>>>                   * If we have no session currently in progress, the initial packet will
>>>                   * open a new session in TM_ACTIVE rather than TM_UNTRUSTED.
>>> @@ -3594,7 +3430,7 @@ tls_pre_decrypt(struct tls_multi *multi,
>>>                  }
>>>              }
>>>  
>>> -            if (i == TM_SIZE && is_hard_reset(op, 0))
>>> +            if (i == TM_SIZE && is_hard_reset_method2(op))
>>
>> Do we need this if() block?  Considering it is exactly the same if() statement
>> as the previous if() block.  I would suggest merging them, but you might hit a
>> challenge with reuse of struct tls_session *session, where one is referring to
>> TM_ACTIVE vs TM_UNTRUSTED.
> 
> That is something that might be possible but I would consider outside of
> patch. The first block actually modifies i (i = TM_ACTIVE;), so that
> needs a separate patch with an explaination why/how we simplify the
> logic here.

Alright, I can see the argument of not touching it now.

Could we then just add a comment to these two sections for now, indicating why
they have been kept separate despite the if() statement being identical?
Something like "This section processes {TM_ACTIVE, TM_UNTRUSTED} sessions" and
elaborate a little bit around that, which may help refactoring this code later on.


-- 
kind regards,

David Sommerseth
OpenVPN Inc

Re: [Openvpn-devel] [PATCH v7] client-connect: Add documentation for the deferred client connect feature

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

1x typo, 3x suggestions

On 20/07/2020 15:27, Arne Schwabe wrote:
> Signed-off-by: David Sommerseth <da...@op...>
> Signed-off-by: Arne Schwabe <ar...@rf...>
> 
> Patch V5: Fix typos, clarify man page section about deferred client-connect
>            script. Add section to Changes.rst
> 
> Patch V6: Convert manpage to rst
> 
>            It also incoroporates suggested changes from Richard Bonhomme

incoroporates -> incorporates


> <tin...@gm...> [0]
> 
> [0] Message-ID: <82c...@gm...>
>      URL: https://www.mail-archive.com/ope...@li.../msg20331.html
> 
> Patch V7: Reinclude the changes of Changes.rst and openvpn-plugin.h

Reinclude -> Re-include (or maybe "Restore the lost changes of..")

>            Clarify some parts of the documentation.
> ---
>   Changes.rst                          |  5 +++++
>   doc/man-sections/generic-options.rst | 11 ++++++----
>   doc/man-sections/script-options.rst  | 33 ++++++++++++++++++++++++++++
>   include/openvpn-plugin.h.in          | 21 +++++++++++++-----
>   4 files changed, 60 insertions(+), 10 deletions(-)
> 
> diff --git a/Changes.rst b/Changes.rst
> index 34abcd97..78a66650 100644
> --- a/Changes.rst
> +++ b/Changes.rst
> @@ -25,6 +25,11 @@ Improved Data channel cipher negotiation
>   Asynchronous (deferred) authentication support for auth-pam plugin.
>       See src/plugins/auth-pam/README.auth-pam for details.
>   
> +Deferred client-connect
> +    The ``--client-connect`` option and the connect plugin API allow
> +    asynchronous/deferred return of the configuration file in the same way
> +    as the auth-plugin.
> +
>   Deprecated features
>   -------------------
>   For an up-to-date list of all deprecated options, see this wiki page:
> diff --git a/doc/man-sections/generic-options.rst b/doc/man-sections/generic-options.rst
> index d44dc05f..a07fe7e7 100644
> --- a/doc/man-sections/generic-options.rst
> +++ b/doc/man-sections/generic-options.rst
> @@ -394,11 +394,14 @@ which mode OpenVPN is configured as.
>   
>     This directory will be used by in the following cases:
>   
> -  * ``--client-connect`` scripts to dynamically generate client-specific
> -    configuration files.
> +  * ``--client-connect`` scripts and :code:`OPENVPN_PLUGIN_CLIENT_CONNECT`
> +    plug-in hook to dynamically generate client-specific configuration
> +    :code:`client_connect_config_file` and return success/failure via
> +    :code:`client_connect_deferred_file` when using deferred client connect
> +    method
>   
> -  * :code:`OPENVPN_PLUGIN_AUTH_USER_PASS_VERIFY` plugin hook to return
> -    success/failure via ``auth_control_file`` when using deferred auth
> +  * :code:`OPENVPN_PLUGIN_AUTH_USER_PASS_VERIFY` plug-in hooks returns
> +    success/failure via :code:`auth_control_file` when using deferred auth
>       method
>   
>     * :code:`OPENVPN_PLUGIN_ENABLE_PF` plugin hook to pass filtering rules
> diff --git a/doc/man-sections/script-options.rst b/doc/man-sections/script-options.rst
> index ddc1363c..a1d489b8 100644
> --- a/doc/man-sections/script-options.rst
> +++ b/doc/man-sections/script-options.rst
> @@ -137,6 +137,13 @@ SCRIPT HOOKS
>     returns a non-zero error status, it will cause the client to be
>     disconnected.
>   
> +  If a ``--client-connect`` wants to defer the generating of the
> +  configuration then the script should use the
> +  :code:`client_connect_deferred_file` and
> +  :code:`client_connect_config_file` environment variables. And write
> +  status accordingly into these files.  See the `Environmental Variables`_
> +  section for more details.
> +
>   --client-disconnect cmd
>     Like ``--client-connect`` but called on client instance shutdown. Will
>     not be called unless the ``--client-connect`` script and plugins (if
> @@ -512,6 +519,32 @@ instances.
>       Total number of bytes sent to client during VPN session. Set prior to
>       execution of the ``--client-disconnect`` script.
>   
> +:code:`client_connect_config_file`
> +    The path to the configuration file that should be written by the

suggest:
should be written by the ->
should be written to by the


> +    ``--client-connect`` script.  The content of this environment variable
> +    is identical to the file as an argument of the called
> +    ``--client-connect`` script.
> +
> +:code:`client_connect_deferred_file`
> +    This file can be optionally written to to communicate a status code of

suggest:
written to to communicate
written to in order to communicate


> +    the ``--client-connect`` script.  The first character in the file must
> +    be either :code:`1` to indicate normal script execution, :code:`0`
> +    indicates an error (in the same way that a non zero exit status does)
> +    or :code:`2` to indicate that the script deferred returning the config
> +    file. When the script defers returning the configuration, it must also
> +    write :code:`2` to the file to indicate the deferral.
> +
> +    A background process or similar must then take care of writing the
> +    configuration to the file indicated by the
> +    :code:`client_connect_config_file` environment variable and when
> +    finished, write the a :code:`1` to this file (or :code:`0` in case of
> +    an error).
> +
> +    The absence of any character in the file when the script finishes
> +    executing is interpreted the same as :code:`1`. This allows scripts
> +    that are not written to support the defer mechanism to be used
> +    unmodified.
> +
>   :code:`common_name`
>       The X509 common name of an authenticated client. Set prior to execution
>       of ``--client-connect``, ``--client-disconnect`` and
> diff --git a/include/openvpn-plugin.h.in b/include/openvpn-plugin.h.in
> index 38fbe097..64b20886 100644
> --- a/include/openvpn-plugin.h.in
> +++ b/include/openvpn-plugin.h.in
> @@ -557,12 +557,21 @@ OPENVPN_PLUGIN_DEF openvpn_plugin_handle_t OPENVPN_PLUGIN_FUNC(openvpn_plugin_op
>    * OPENVPN_PLUGIN_FUNC_SUCCESS on success, OPENVPN_PLUGIN_FUNC_ERROR on failure
>    *
>    * In addition, OPENVPN_PLUGIN_FUNC_DEFERRED may be returned by
> - * OPENVPN_PLUGIN_AUTH_USER_PASS_VERIFY.  This enables asynchronous
> - * authentication where the plugin (or one of its agents) may indicate
> - * authentication success/failure some number of seconds after the return
> - * of the OPENVPN_PLUGIN_AUTH_USER_PASS_VERIFY handler by writing a single
> - * char to the file named by auth_control_file in the environmental variable
> - * list (envp).
> + * OPENVPN_PLUGIN_AUTH_USER_PASS_VERIFY, OPENVPN_PLUGIN_CLIENT_CONNECT and
> + * OPENVPN_PLUGIN_CLIENT_CONNECT_V2. This enables asynchronous
> + * authentication or client connect  where the plugin (or one of its agents)
> + * may indicate authentication success/failure or client configuration some
> + * number of seconds after the return of the function handler.
> + * For OPENVPN_PLUGIN_AUTH_USER_PASS_VERIFY and OPENVPN_PLUGIN_CLIENT_CONNECT
> + * this is done by writing a single char to the file named by
> + * auth_control_file/client_connect_deferred_file
> + * in the environmental variable list (envp).
> + *
> + * In addition the OPENVPN_PLUGIN_CLIENT_CONNECT_DEFER and
> + * OPENVPN_PLUGIN_CLIENT_CONNECT_DEFER_V2 are called when OpenVPN tries to
> + * get the deferred result. For a V2 call implementing this function is
> + * required as information is not passed by files. For the normal version
> + * the call is optional.
>    *
>    * first char of auth_control_file:
>    * '0' -- indicates auth failure
> 

[Openvpn-devel] [PATCH v7] client-connect: Add documentation for the deferred client connect feature

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

Signed-off-by: David Sommerseth <da...@op...>
Signed-off-by: Arne Schwabe <ar...@rf...>

Patch V5: Fix typos, clarify man page section about deferred client-connect
          script. Add section to Changes.rst

Patch V6: Convert manpage to rst

          It also incoroporates suggested changes from Richard Bonhomme
<tin...@gm...> [0]

[0] Message-ID: <82c...@gm...>
    URL: https://www.mail-archive.com/ope...@li.../msg20331.html

Patch V7: Reinclude the changes of Changes.rst and openvpn-plugin.h
          Clarify some parts of the documentation.
---
 Changes.rst                          |  5 +++++
 doc/man-sections/generic-options.rst | 11 ++++++----
 doc/man-sections/script-options.rst  | 33 ++++++++++++++++++++++++++++
 include/openvpn-plugin.h.in          | 21 +++++++++++++-----
 4 files changed, 60 insertions(+), 10 deletions(-)

diff --git a/Changes.rst b/Changes.rst
index 34abcd97..78a66650 100644
--- a/Changes.rst
+++ b/Changes.rst
@@ -25,6 +25,11 @@ Improved Data channel cipher negotiation
 Asynchronous (deferred) authentication support for auth-pam plugin.
     See src/plugins/auth-pam/README.auth-pam for details.
 
+Deferred client-connect
+    The ``--client-connect`` option and the connect plugin API allow
+    asynchronous/deferred return of the configuration file in the same way
+    as the auth-plugin.
+
 Deprecated features
 -------------------
 For an up-to-date list of all deprecated options, see this wiki page:
diff --git a/doc/man-sections/generic-options.rst b/doc/man-sections/generic-options.rst
index d44dc05f..a07fe7e7 100644
--- a/doc/man-sections/generic-options.rst
+++ b/doc/man-sections/generic-options.rst
@@ -394,11 +394,14 @@ which mode OpenVPN is configured as.
 
   This directory will be used by in the following cases:
 
-  * ``--client-connect`` scripts to dynamically generate client-specific
-    configuration files.
+  * ``--client-connect`` scripts and :code:`OPENVPN_PLUGIN_CLIENT_CONNECT`
+    plug-in hook to dynamically generate client-specific configuration
+    :code:`client_connect_config_file` and return success/failure via
+    :code:`client_connect_deferred_file` when using deferred client connect
+    method
 
-  * :code:`OPENVPN_PLUGIN_AUTH_USER_PASS_VERIFY` plugin hook to return
-    success/failure via ``auth_control_file`` when using deferred auth
+  * :code:`OPENVPN_PLUGIN_AUTH_USER_PASS_VERIFY` plug-in hooks returns
+    success/failure via :code:`auth_control_file` when using deferred auth
     method
 
   * :code:`OPENVPN_PLUGIN_ENABLE_PF` plugin hook to pass filtering rules
diff --git a/doc/man-sections/script-options.rst b/doc/man-sections/script-options.rst
index ddc1363c..a1d489b8 100644
--- a/doc/man-sections/script-options.rst
+++ b/doc/man-sections/script-options.rst
@@ -137,6 +137,13 @@ SCRIPT HOOKS
   returns a non-zero error status, it will cause the client to be
   disconnected.
 
+  If a ``--client-connect`` wants to defer the generating of the
+  configuration then the script should use the
+  :code:`client_connect_deferred_file` and
+  :code:`client_connect_config_file` environment variables. And write
+  status accordingly into these files.  See the `Environmental Variables`_
+  section for more details.
+
 --client-disconnect cmd
   Like ``--client-connect`` but called on client instance shutdown. Will
   not be called unless the ``--client-connect`` script and plugins (if
@@ -512,6 +519,32 @@ instances.
     Total number of bytes sent to client during VPN session. Set prior to
     execution of the ``--client-disconnect`` script.
 
+:code:`client_connect_config_file`
+    The path to the configuration file that should be written by the
+    ``--client-connect`` script.  The content of this environment variable
+    is identical to the file as an argument of the called
+    ``--client-connect`` script.
+
+:code:`client_connect_deferred_file`
+    This file can be optionally written to to communicate a status code of
+    the ``--client-connect`` script.  The first character in the file must
+    be either :code:`1` to indicate normal script execution, :code:`0`
+    indicates an error (in the same way that a non zero exit status does)
+    or :code:`2` to indicate that the script deferred returning the config
+    file. When the script defers returning the configuration, it must also
+    write :code:`2` to the file to indicate the deferral.
+
+    A background process or similar must then take care of writing the
+    configuration to the file indicated by the
+    :code:`client_connect_config_file` environment variable and when
+    finished, write the a :code:`1` to this file (or :code:`0` in case of
+    an error).
+
+    The absence of any character in the file when the script finishes
+    executing is interpreted the same as :code:`1`. This allows scripts
+    that are not written to support the defer mechanism to be used
+    unmodified.
+
 :code:`common_name`
     The X509 common name of an authenticated client. Set prior to execution
     of ``--client-connect``, ``--client-disconnect`` and
diff --git a/include/openvpn-plugin.h.in b/include/openvpn-plugin.h.in
index 38fbe097..64b20886 100644
--- a/include/openvpn-plugin.h.in
+++ b/include/openvpn-plugin.h.in
@@ -557,12 +557,21 @@ OPENVPN_PLUGIN_DEF openvpn_plugin_handle_t OPENVPN_PLUGIN_FUNC(openvpn_plugin_op
  * OPENVPN_PLUGIN_FUNC_SUCCESS on success, OPENVPN_PLUGIN_FUNC_ERROR on failure
  *
  * In addition, OPENVPN_PLUGIN_FUNC_DEFERRED may be returned by
- * OPENVPN_PLUGIN_AUTH_USER_PASS_VERIFY.  This enables asynchronous
- * authentication where the plugin (or one of its agents) may indicate
- * authentication success/failure some number of seconds after the return
- * of the OPENVPN_PLUGIN_AUTH_USER_PASS_VERIFY handler by writing a single
- * char to the file named by auth_control_file in the environmental variable
- * list (envp).
+ * OPENVPN_PLUGIN_AUTH_USER_PASS_VERIFY, OPENVPN_PLUGIN_CLIENT_CONNECT and
+ * OPENVPN_PLUGIN_CLIENT_CONNECT_V2. This enables asynchronous
+ * authentication or client connect  where the plugin (or one of its agents)
+ * may indicate authentication success/failure or client configuration some
+ * number of seconds after the return of the function handler.
+ * For OPENVPN_PLUGIN_AUTH_USER_PASS_VERIFY and OPENVPN_PLUGIN_CLIENT_CONNECT
+ * this is done by writing a single char to the file named by
+ * auth_control_file/client_connect_deferred_file
+ * in the environmental variable list (envp).
+ *
+ * In addition the OPENVPN_PLUGIN_CLIENT_CONNECT_DEFER and
+ * OPENVPN_PLUGIN_CLIENT_CONNECT_DEFER_V2 are called when OpenVPN tries to
+ * get the deferred result. For a V2 call implementing this function is
+ * required as information is not passed by files. For the normal version
+ * the call is optional.
  *
  * first char of auth_control_file:
  * '0' -- indicates auth failure
-- 
2.26.2