Add --tls-cert-profile option for mbedtls builds

This allows the user to specify what certificate crypto algorithms to support. The supported profiles are 'preferred' (default), 'legacy' and 'suiteb', as discussed in <[email protected]....>
(https://www.mail-archive.com/[email protected]/msg14214.html).

This only implements the feature for mbed TLS builds, because for mbed it is both more easy to implement and the most relevant because mbed TLS 2+ is by default somewhat restrictive by requiring 2048-bit+ for RSA keys.

Signed-off-by Steffan Karger <[email protected]...>

---
 src/openvpn/options.c     |  8 ++++++++
 src/openvpn/options.h     |  1 +
 src/openvpn/ssl.c         |  3 +++
 src/openvpn/ssl_backend.h | 10 ++++++++++
 src/openvpn/ssl_mbedtls.c | 51 +++++++++++++++++++++++++++++++++++++++++++++++
 src/openvpn/ssl_mbedtls.h |  1 +
 src/openvpn/ssl_openssl.c |  6 ++++++
 7 files changed, 80 insertions(+)

src/openvpn/ssl_mbedtls.c +0 -0

index ba8dadf..1fcee72 100644

src/openvpn/ssl_mbedtls.c +16 -0

+    {
+        ctx->cert_profile = openvpn_x509_crt_profile_preferred;
+    }
+    else if (0 == strcmp(profile, "legacy"))
+    {
+        ctx->cert_profile = openvpn_x509_crt_profile_legacy;
+    }
+    else if (0 == strcmp(profile, "suiteb"))
+    {
+        ctx->cert_profile = openvpn_x509_crt_profile_suiteb;
+    }
+    else
+    {
+        msg (M_FATAL, "ERROR: Invalid cert profile: %s", profile);
+    }
+}

Version 2.4.1
=============
- - ``--remote-cert-ku`` now only requires the certificate to have at least the

Changes.rst +6 -11

-   bits set of one of the values in the supplied list, instead of requiring an
-   exact match to one of the values in the list.
- - ``--remote-cert-tls`` now only requires that a keyUsage is present in the
-   certificate, and leaves the verification of the value up to the crypto
-   library, which has more information (i.e. the key exchange method in use)
-   to verify that the keyUsage is correct.
- - ``--ns-cert-type`` is deprecated.  Use ``--remote-cert-tls`` instead.
-   The nsCertType x509 extension is very old, and barely used.
-   ``--remote-cert-tls`` uses the far more common keyUsage and extendedKeyUsage
-   extension instead.  Make sure your certificates carry these to be able to
-   use ``--remote-cert-tls``.
+- ``--remote-cert-ku`` now only requires the certificate to have at least the
+  bits set of one of the values in the supplied list, instead of requiring an
+  exact match to one of the values in the list.
+- ``--remote-cert-tls`` now only requires that a keyUsage is present in the
+  certificate, and leaves the verification of the value up to the crypto
+  library, which has more information (i.e. the key exchange method in use)

Quoted 20 lines · Steffan Karger

Version 2.4.1
=============
- - ``--remote-cert-ku`` now only requires the certificate to have at least the

Changes.rst +6 -11

-   bits set of one of the values in the supplied list, instead of requiring an
-   exact match to one of the values in the list.
- - ``--remote-cert-tls`` now only requires that a keyUsage is present in the
-   certificate, and leaves the verification of the value up to the crypto
-   library, which has more information (i.e. the key exchange method in use)
-   to verify that the keyUsage is correct.
- - ``--ns-cert-type`` is deprecated.  Use ``--remote-cert-tls`` instead.
-   The nsCertType x509 extension is very old, and barely used.
-   ``--remote-cert-tls`` uses the far more common keyUsage and extendedKeyUsage
-   extension instead.  Make sure your certificates carry these to be able to
-   use ``--remote-cert-tls``.
+- ``--remote-cert-ku`` now only requires the certificate to have at least the
+  bits set of one of the values in the supplied list, instead of requiring an
+  exact match to one of the values in the list.
+- ``--remote-cert-tls`` now only requires that a keyUsage is present in the
+  certificate, and leaves the verification of the value up to the crypto
+  library, which has more information (i.e. the key exchange method in use)

Changes.rst Maintainer-visible changes · line 324

Quoted 10 lines

Changes.rst +10 -0

+  to verify that the keyUsage is correct.
+- ``--ns-cert-type`` is deprecated.  Use ``--remote-cert-tls`` instead.
+  The nsCertType x509 extension is very old, and barely used.
+  ``--remote-cert-tls`` uses the far more common keyUsage and extendedKeyUsage
+  extension instead.  Make sure your certificates carry these to be able to
+  use ``--remote-cert-tls``.
+- The new option ``--tls-cert-profile`` can be used to restrict the set of
+  allowed crypto algorithms in TLS certificates in mbed TLS builds.  The
+  'legacy' profile can be used to re-enable support for SHA1 and 1024-bit RSA
+  keys.

Hrmpf, this should of course get a new section '2.4.2'... Let me know if you want a v3, or whether this can be fixed on-the-fly. Apologies!

I'm however a bit puzzled of the "non-changes" (well, the indenting is changing, unless I'm blind to other changes) to --remote-cert-cu, --remote-cert-tls and --ns-cert-type. If we want to change the indenting, I think that should be kept in a separate patch, and keep --tls-cert-profile as a patch of its own.

On a more generic note to this patch. I wonder if we should keep "legacy" the default in the v2.4 branch. In the Fedora 26 (and Rawhide/27) builds I had to do something similar [1] to keep users happy. As OpenVPN isn't ready for OpenSSL v1.1, I had to switch to mbed TLS. Unfortunately that haven't been as successful as I really hoped it would be, but that's an entirely different story (and mail thread). As long as the Fedora builds need to be built with mbed TLS, I will need to ensure 'legacy' is the default there for a while. For the coming Fedora Rawhide (which will be F28), I can make some announcements preparing users to move to stricter defaults.

[1]
<http://pkgs.fedoraproject.org/cgit/rpms/openvpn.git/tree/0001-workaround-Allow-weaker-RSA-keys-and-MD-algorithms-i.patch?h=f26>

Quoted 5 lines · David Sommerseth

I'm however a bit puzzled of the "non-changes" (well, the indenting is changing, unless I'm blind to other changes) to --remote-cert-cu, --remote-cert-tls and --ns-cert-type. If we want to change the indenting, I think that should be kept in a separate patch, and keep --tls-cert-profile as a patch of its own.

Ah, indeed. The indenting change is needed to make Github parse the rst correctly. Feel free to remove the indenting changes and I'll send a follow-up patch doing just that.

Quoted 11 lines

On a more generic note to this patch. I wonder if we should keep "legacy" the default in the v2.4 branch. In the Fedora 26 (and Rawhide/27) builds I had to do something similar [1] to keep users happy. As OpenVPN isn't ready for OpenSSL v1.1, I had to switch to mbed TLS. Unfortunately that haven't been as successful as I really hoped it would be, but that's an entirely different story (and mail thread). As long as the Fedora builds need to be built with mbed TLS, I will need to ensure 'legacy' is the default there for a while. For the coming Fedora Rawhide (which will be F28), I can make some announcements preparing users to move to stricter defaults.

Quoted 2 lines

[1]
<http://pkgs.fedoraproject.org/cgit/rpms/openvpn.git/tree/0001-workaround-Allow-weaker-RSA-keys-and-MD-algorithms-i.patch?h=f26>

The current mbed TLS builds already reject legacy crypto (except the fedora packaged build, apparently). With this patch users have the ability to use legacy stuff again, but I would prefer to not go any further than that. I think we should encourage people to drop the legacy. Just like the browser vendors are doing.

And for Fedora, they chose to experience intense pain when they chose to go for OpenSSL 1.1 this fast, that's their problem I guess... If they want to be that cutting edge, they should also stop using legacy crypto.
And otherwise, it will be a simple patch in the fedora packaging.

Quoted 12 lines · Steffan Karger

Quoted 11 lines · David Sommerseth

On a more generic note to this patch. I wonder if we should keep "legacy" the default in the v2.4 branch. In the Fedora 26 (and Rawhide/27) builds I had to do something similar [1] to keep users happy. As OpenVPN isn't ready for OpenSSL v1.1, I had to switch to mbed TLS. Unfortunately that haven't been as successful as I really hoped it would be, but that's an entirely different story (and mail thread). As long as the Fedora builds need to be built with mbed TLS, I will need to ensure 'legacy' is the default there for a while. For the coming Fedora Rawhide (which will be F28), I can make some announcements preparing users to move to stricter defaults.

Quoted 8 lines

Quoted 2 lines

[1]
<http://pkgs.fedoraproject.org/cgit/rpms/openvpn.git/tree/0001-workaround-Allow-weaker-RSA-keys-and-MD-algorithms-i.patch?h=f26>

The current mbed TLS builds already reject legacy crypto (except the fedora packaged build, apparently). With this patch users have the ability to use legacy stuff again, but I would prefer to not go any further than that. I think we should encourage people to drop the legacy. Just like the browser vendors are doing.

I do agree. But the pain as a package maintainer is quite noticeable when you're getting complaints about configurations this breaks. In fact, I got quite some dissatisfied remarks even for moving from 2.3 to 2.4 on the OpenSSL builds (F25 + EPEL) - as that also broke some setups
- mostly due to our option parser now doesn't ignore configuration errors any more or we actually implement the dual stack IPv4/IPv6 properly on the transport side.

[..snip..]

Quoted 4 lines

And for Fedora, they chose to experience intense pain when they chose to go for OpenSSL 1.1 this fast, that's their problem I guess... If they want to be that cutting edge, they should also stop using legacy crypto.
And otherwise, it will be a simple patch in the fedora packaging.

Well, they do make things move forward fast. And they do a lot of work to get upstream projects moving towards OpenSSL 1.1. As there is a major Fedora release roughly every 6 months, it is a fast moving target.

But Fedora users as side, they know each major release is a huge step forward in many areas. For most OpenVPN users, regardless of OS or distro, they are used to the more sloppy legacy crypto support of OpenSSL. So enforcing a stricter set of crypto parameters in OpenVPN v2.4 only on mbed TLS seems a bit odd to me.

Hi,

Quoted 41 lines · David Sommerseth

Quoted 12 lines · Steffan Karger

Quoted 11 lines · David Sommerseth

On a more generic note to this patch. I wonder if we should keep "legacy" the default in the v2.4 branch. In the Fedora 26 (and Rawhide/27) builds I had to do something similar [1] to keep users happy. As OpenVPN isn't ready for OpenSSL v1.1, I had to switch to mbed TLS. Unfortunately that haven't been as successful as I really hoped it would be, but that's an entirely different story (and mail thread). As long as the Fedora builds need to be built with mbed TLS, I will need to ensure 'legacy' is the default there for a while. For the coming Fedora Rawhide (which will be F28), I can make some announcements preparing users to move to stricter defaults.

Quoted 8 lines

Quoted 2 lines

[1]
<http://pkgs.fedoraproject.org/cgit/rpms/openvpn.git/tree/0001-workaround-Allow-weaker-RSA-keys-and-MD-algorithms-i.patch?h=f26>

The current mbed TLS builds already reject legacy crypto (except the fedora packaged build, apparently). With this patch users have the ability to use legacy stuff again, but I would prefer to not go any further than that. I think we should encourage people to drop the legacy. Just like the browser vendors are doing.

I do agree. But the pain as a package maintainer is quite noticeable when you're getting complaints about configurations this breaks. In fact, I got quite some dissatisfied remarks even for moving from 2.3 to 2.4 on the OpenSSL builds (F25 + EPEL) - as that also broke some setups
- mostly due to our option parser now doesn't ignore configuration errors any more or we actually implement the dual stack IPv4/IPv6 properly on the transport side.

[..snip..]

Quoted 4 lines

And for Fedora, they chose to experience intense pain when they chose to go for OpenSSL 1.1 this fast, that's their problem I guess... If they want to be that cutting edge, they should also stop using legacy crypto.
And otherwise, it will be a simple patch in the fedora packaging.

Well, they do make things move forward fast. And they do a lot of work to get upstream projects moving towards OpenSSL 1.1. As there is a major Fedora release roughly every 6 months, it is a fast moving target.

Quoted 5 lines

But Fedora users as side, they know each major release is a huge step forward in many areas. For most OpenVPN users, regardless of OS or distro, they are used to the more sloppy legacy crypto support of OpenSSL. So enforcing a stricter set of crypto parameters in OpenVPN v2.4 only on mbed TLS seems a bit odd to me.

I agree - which is why I plan to add similar behaviour to the OpenSSL builds. I'm convinced by the argument to better aligning OpenSSL and mbed TLS behaviour, so I agree that we should default to 'legacy' as long as the OpenSSL counterpart is missing.

Do you prefer a v3?

The thing I still don't buy is that we have to jump through hoops because Fedora pushes openssl 1.1 (even though 1.0.2 is fine and supported and all), but we have to keep enabling deprecated crypto *by default* that people should not be using anymore, and can easily re-enable with a --enable-old-cruft flag if they really want to.

This allows the user to specify what certificate crypto algorithms to support. The supported profiles are 'preferred' (default), 'legacy' and 'suiteb', as discussed in <[email protected]...>
(https://www.mail-archive.com/[email protected]/msg14214.html).

This only implements the feature for mbed TLS builds, because for mbed it is both more easy to implement and the most relevant because mbed TLS 2+ is by default somewhat restrictive by requiring 2048-bit+ for RSA keys.

Am 14.04.17 um 17:40 schrieb Steffan Karger:

Quoted 5 lines

This allows the user to specify what certificate crypto algorithms to support. The supported profiles are 'preferred' (default), 'legacy' and 'suiteb', as discussed in <[email protected]...>
(https://www.mail-archive.com/[email protected]/msg14214.html).

Quoted 4 lines

This only implements the feature for mbed TLS builds, because for mbed it is both more easy to implement and the most relevant because mbed TLS 2+ is by default somewhat restrictive by requiring 2048-bit+ for RSA keys.

ACK so far as the code goes. As for the whole MD5 stuff is at the moment blowing up with OSSL 1.1, we need a md5 allowing option (with a fat warning probably).

And the other thing is that OpenSSL has a similar feature in 1.1:
Security levels. Which can be specified as part of tls-cipher or set independently by
https://www.openssl.org/docs/manmaster/man3/SSL_CTX_set_security_level.html

The levels are similar to the proposed level here but not identical. Should we somehow align these two features? Configuring it for one library in tls-cipher and for the other in tls-cert-profile is bit strange.

Also shouldn't be tls-min-version included in the preferred/legacy options?

v2:
- add documentation (manpage, Changes.rst and --help)
- no longer print a warning message on each startup for OpenSSL builds
v3:
- remove format changes in unrelated items (introduced in v2)
- Update Changes.rst text to reflect that the default in 2.4.2 is 'legacy' and the default in 2.5 will be 'preferred' (as discussed on the ML).
- This patch is for the master branch only now (due to the default).

From: Steffan Karger <[email protected]...>

This allows the user to specify what certificate crypto algorithms to support. The supported profiles are 'preferred', 'legacy' (default) and 'suiteb', as discussed in <[email protected]....>
(https://www.mail-archive.com/[email protected]/msg14214.html).

This only implements the feature for mbed TLS builds, because for mbed it is both more easy to implement and the most relevant because mbed TLS 2+ is by default somewhat restrictive by requiring 2048-bit+ for RSA keys.

This patch uses 'legacy' as the default profile following discussion on the openvpn-devel mailing list. This way this patch can be applied to both the release/2.4 and master branches. I'll send a follow-up patch for the master branch to change the default to 'preferred' later.

Signed-off-by Steffan Karger <[email protected]...>

---
v2:
- add documentation (manpage, Changes.rst and --help)
- no longer print a warning message on each startup for OpenSSL builds
v3:
- remove format changes in unrelated items (introduced in v2)
- Update Changes.rst text to reflect that the default in 2.4.2 is 'legacy' and the default in 2.5 will be 'preferred' (as discussed on the ML).
- This patch is for the master branch only now (due to the default).
v4:
- Ignore the option for OpenSSL builds, instead of rejecting the option and refusing to start.
- Patch is for master and release/2.4 again. A follow-up patch will change the default for master later on.

 Changes.rst               | 12 +++++++++++
 doc/openvpn.8             | 22 ++++++++++++++++++++
 src/openvpn/options.c     | 16 ++++++++++++++-
 src/openvpn/options.h     |  1 +
 src/openvpn/ssl.c         |  3 +++
 src/openvpn/ssl_backend.h | 10 ++++++++++
 src/openvpn/ssl_mbedtls.c | 51 +++++++++++++++++++++++++++++++++++++++++++++++
 src/openvpn/ssl_mbedtls.h |  1 +
 src/openvpn/ssl_openssl.c |  6 ++++++
 9 files changed, 121 insertions(+), 1 deletion(-)

Changes.rst +12 -0

index db9b18b9..a6090cf5 100644
@@ -321,6 +321,18 @@ Maintainer-visible changes
   i386/i686 builds on RHEL5.
 
 
+Version 2.4.5
+=============
+
+New features
+------------
+- The new option ``--tls-cert-profile`` can be used to restrict the set of
+  allowed crypto algorithms in TLS certificates in mbed TLS builds.  The
+  default profile is 'legacy' for now, which allows SHA1+, RSA-1024+ and any
+  elliptic curve certificates.  The default will be changed to the 'preferred'
+  profile in the future, which requires SHA2+, RSA-2048+ and any curve.
+
+
 Version 2.4.3
 =============
 

doc/openvpn.8 +22 -0

index a4189ac2..6059c81d 100644
@@ -4917,6 +4917,28 @@ when using mbed TLS or
 OpenSSL.
 .\"*********************************************************
 .TP
+.B \-\-tls\-cert\-profile profile
+Set the allowed cryptographic algorithms for certificates according to
+.B profile\fN.
+
+The following profiles are supported:
+
+.B preferred
+: SHA2 and newer, RSA 2048-bit+, any elliptic curve.
+
+.B legacy
+(default): SHA1 and newer, RSA 2048-bit+, any elliptic curve.
+
+.B suiteb
+: SHA256/SHA384, ECDSA with P-256 or P-384.
+
+This option is only supported for mbed TLS builds.  OpenSSL builds accept any
+certificate that OpenSSL accepts.
+
+OpenVPN will migrate to 'preferred' as default in the future.  Please ensure
+that your keys already comply.
+.\"*********************************************************
+.TP
 .B \-\-tls\-timeout n
 Packet retransmit timeout on TLS control channel
 if no acknowledgment from remote within

src/openvpn/options.c +15 -1

index 49ed004b..23c2c02a 100644
@@ -599,6 +599,8 @@ static const char usage_message[] =
 #endif
     "--tls-cipher l  : A list l of allowable TLS ciphers separated by : (optional).\n"
     "                : Use --show-tls to see a list of supported TLS ciphers.\n"
+    "--tls-cert-profile p : Set the allowed certificate crypto algorithm profile\n"
+    "                  (default=%s).\n"
     "--tls-timeout n : Packet retransmit timeout on TLS control channel\n"
     "                  if no ACK from remote within n seconds (default=%d).\n"
     "--reneg-bytes n : Renegotiate data chan. key after n bytes sent and recvd.\n"

@@ -872,6 +874,7 @@ init_options(struct options *o, const bool init_gc)
     o->renegotiate_seconds = 3600;
     o->handshake_window = 60;
     o->transition_window = 3600;
+    o->tls_cert_profile = "legacy";
     o->ecdh_curve = NULL;
 #ifdef ENABLE_X509ALTUSERNAME
     o->x509_username_field = X509_USERNAME_FIELD_DEFAULT;

@@ -1750,6 +1753,7 @@ show_settings(const struct options *o)
     SHOW_STR(cryptoapi_cert);
 #endif
     SHOW_STR(cipher_list);
+    SHOW_STR(tls_cert_profile);
     SHOW_STR(tls_verify);
     SHOW_STR(tls_export_cert);
     SHOW_INT(verify_x509_type);

@@ -2729,6 +2733,7 @@ options_postprocess_verify_ce(const struct options *options, const struct connec
         MUST_BE_UNDEF(pkcs12_file);
 #endif
         MUST_BE_UNDEF(cipher_list);
+        MUST_BE_UNDEF(tls_cert_profile);
         MUST_BE_UNDEF(tls_verify);
         MUST_BE_UNDEF(tls_export_cert);
         MUST_BE_UNDEF(verify_x509_name);

@@ -4086,7 +4091,7 @@ usage(void)
             o.verbosity,
             o.authname, o.ciphername,
             o.replay_window, o.replay_time,
-            o.tls_timeout, o.renegotiate_seconds,
+            o.tls_cert_profile, o.tls_timeout, o.renegotiate_seconds,
             o.handshake_window, o.transition_window);
 #else  /* ifdef ENABLE_CRYPTO */
     fprintf(fp, usage_message,

@@ -7831,6 +7836,15 @@ add_option(struct options *options,
         VERIFY_PERMISSION(OPT_P_GENERAL);
         options->cipher_list = p[1];
     }
+    else if (streq(p[0], "tls-cert-profile") && p[1] && !p[2])
+    {
+        VERIFY_PERMISSION(OPT_P_GENERAL);
+#ifdef ENABLE_CRYPTO_MBEDTLS
+        options->tls_cert_profile = p[1];
+#else
+        msg(M_WARN, "WARNING: --tls-cert-profile is not yet implemented for OpenSSL, ignoring option.");
+#endif
+    }
     else if (streq(p[0], "crl-verify") && p[1] && ((p[2] && streq(p[2], "dir"))
                                                    || (p[2] && streq(p[1], INLINE_FILE_TAG) ) || !p[2]) && !p[3])
     {

src/openvpn/options.h +1 -0

index 496c1143..85ff200b 100644
@@ -502,6 +502,7 @@ struct options
     const char *priv_key_file;
     const char *pkcs12_file;
     const char *cipher_list;
+    const char *tls_cert_profile;
     const char *ecdh_curve;
     const char *tls_verify;
     int verify_x509_type;

src/openvpn/ssl.c +3 -0

index cb94229a..48c1b477 100644
@@ -621,6 +621,9 @@ init_ssl(const struct options *options, struct tls_root_ctx *new_ctx)
      * cipher restrictions before loading certificates */
     tls_ctx_restrict_ciphers(new_ctx, options->cipher_list);
 
+    /* Restrict allowed certificate crypto algorithms */
+    tls_ctx_set_cert_profile(new_ctx, options->tls_cert_profile);
+
     tls_ctx_set_options(new_ctx, options->ssl_flags);
 
     if (options->pkcs12_file)

src/openvpn/ssl_backend.h +10 -0

index aba5a4de..ddc76a97 100644
@@ -176,6 +176,16 @@ void tls_ctx_set_options(struct tls_root_ctx *ctx, unsigned int ssl_flags);
  */
 void tls_ctx_restrict_ciphers(struct tls_root_ctx *ctx, const char *ciphers);
 
+/**
+ * Set the TLS certificate profile.  The profile defines which crypto
+ * algorithms may be used in the supplied certificate.
+ *
+ * @param ctx           TLS context to restrict, must be valid.
+ * @param profile       The profile name ('preferred', 'legacy' or 'suiteb').
+ *                      Defaults to 'preferred' if NULL.
+ */
+void tls_ctx_set_cert_profile(struct tls_root_ctx *ctx, const char *profile);
+
 /**
  * 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,

src/openvpn/ssl_mbedtls.c +51 -0

index 861d936d..6950abfe 100644
@@ -62,6 +62,34 @@
 #include <mbedtls/pem.h>
 #include <mbedtls/sha256.h>
 
+static const mbedtls_x509_crt_profile openvpn_x509_crt_profile_legacy =
+{
+    /* Hashes from SHA-1 and above */
+    MBEDTLS_X509_ID_FLAG( MBEDTLS_MD_SHA1 ) |
+    MBEDTLS_X509_ID_FLAG( MBEDTLS_MD_RIPEMD160 ) |
+    MBEDTLS_X509_ID_FLAG( MBEDTLS_MD_SHA224 ) |
+    MBEDTLS_X509_ID_FLAG( MBEDTLS_MD_SHA256 ) |
+    MBEDTLS_X509_ID_FLAG( MBEDTLS_MD_SHA384 ) |
+    MBEDTLS_X509_ID_FLAG( MBEDTLS_MD_SHA512 ),
+    0xFFFFFFF, /* Any PK alg    */
+    0xFFFFFFF, /* Any curve     */
+    1024,      /* RSA-1024 and larger */
+};
+
+static const mbedtls_x509_crt_profile openvpn_x509_crt_profile_preferred =
+{
+    /* SHA-2 and above */
+    MBEDTLS_X509_ID_FLAG( MBEDTLS_MD_SHA224 ) |
+    MBEDTLS_X509_ID_FLAG( MBEDTLS_MD_SHA256 ) |
+    MBEDTLS_X509_ID_FLAG( MBEDTLS_MD_SHA384 ) |
+    MBEDTLS_X509_ID_FLAG( MBEDTLS_MD_SHA512 ),
+    0xFFFFFFF, /* Any PK alg    */
+    0xFFFFFFF, /* Any curve     */
+    2048,      /* RSA-2048 and larger */
+};
+
+#define openvpn_x509_crt_profile_suiteb mbedtls_x509_crt_profile_suiteb;
+
 void
 tls_init_lib(void)
 {

@@ -250,6 +278,27 @@ tls_ctx_restrict_ciphers(struct tls_root_ctx *ctx, const char *ciphers)
     free(tmp_ciphers_orig);
 }
 
+void
+tls_ctx_set_cert_profile(struct tls_root_ctx *ctx, const char *profile)
+{
+    if (!profile || 0 == strcmp(profile, "preferred"))
+    {
+        ctx->cert_profile = openvpn_x509_crt_profile_preferred;
+    }
+    else if (0 == strcmp(profile, "legacy"))
+    {
+        ctx->cert_profile = openvpn_x509_crt_profile_legacy;
+    }
+    else if (0 == strcmp(profile, "suiteb"))
+    {
+        ctx->cert_profile = openvpn_x509_crt_profile_suiteb;
+    }
+    else
+    {
+        msg (M_FATAL, "ERROR: Invalid cert profile: %s", profile);
+    }
+}
+
 void
 tls_ctx_check_cert_time(const struct tls_root_ctx *ctx)
 {

@@ -917,6 +966,8 @@ key_state_ssl_init(struct key_state_ssl *ks_ssl,
     mbedtls_ssl_conf_rng(&ks_ssl->ssl_config, mbedtls_ctr_drbg_random,
                          rand_ctx_get());
 
+    mbedtls_ssl_conf_cert_profile(&ks_ssl->ssl_config, &ssl_ctx->cert_profile);
+
     if (ssl_ctx->allowed_ciphers)
     {
         mbedtls_ssl_conf_ciphersuites(&ks_ssl->ssl_config, ssl_ctx->allowed_ciphers);

src/openvpn/ssl_mbedtls.h +1 -0

index f69b6100..341da7d4 100644
@@ -82,6 +82,7 @@ struct tls_root_ctx {
     struct external_context *external_key; /**< Management external key */
 #endif
     int *allowed_ciphers;       /**< List of allowed ciphers for this connection */
+    mbedtls_x509_crt_profile cert_profile; /**< Allowed certificate types */
 };
 
 struct key_state_ssl {

src/openvpn/ssl_openssl.c +6 -0

index 0bfb6939..aed36a26 100644
@@ -383,6 +383,12 @@ tls_ctx_restrict_ciphers(struct tls_root_ctx *ctx, const char *ciphers)
     }
 }
 
+void
+tls_ctx_set_cert_profile(struct tls_root_ctx *ctx, const char *profile)
+{
+    /* TODO --tls-cert-profile not supported for OpenSSL builds */
+}
+
 void
 tls_ctx_check_cert_time(const struct tls_root_ctx *ctx)
 {

From: Steffan Karger <[email protected]...>

This allows the user to specify what certificate crypto algorithms to support. The supported profiles are 'preferred', 'legacy' (default) and 'suiteb', as discussed in <[email protected]....>
(https://www.mail-archive.com/[email protected]/msg14214.html).

This only implements the feature for mbed TLS builds, because for mbed it is both more easy to implement and the most relevant because mbed TLS 2+ is by default somewhat restrictive by requiring 2048-bit+ for RSA keys.

This patch uses 'legacy' as the default profile following discussion on the openvpn-devel mailing list. This way this patch can be applied to both the release/2.4 and master branches. I'll send a follow-up patch for the master branch to change the default to 'preferred' later.

Signed-off-by Steffan Karger <[email protected]...>

---
v2:
- add documentation (manpage, Changes.rst and --help)
- no longer print a warning message on each startup for OpenSSL builds
v3:
- remove format changes in unrelated items (introduced in v2)
- Update Changes.rst text to reflect that the default in 2.4.2 is 'legacy' and the default in 2.5 will be 'preferred' (as discussed on the ML).
- This patch is for the master branch only now (due to the default).
v4:
- Ignore the option for OpenSSL builds, instead of rejecting the option and refusing to start.
- Patch is for master and release/2.4 again. A follow-up patch will change the default for master later on.

 Changes.rst               | 12 +++++++++++
 doc/openvpn.8             | 22 ++++++++++++++++++++
 src/openvpn/options.c     | 16 ++++++++++++++-
 src/openvpn/options.h     |  1 +
 src/openvpn/ssl.c         |  3 +++
 src/openvpn/ssl_backend.h | 10 ++++++++++
 src/openvpn/ssl_mbedtls.c | 51 +++++++++++++++++++++++++++++++++++++++++++++++
 src/openvpn/ssl_mbedtls.h |  1 +
 src/openvpn/ssl_openssl.c |  6 ++++++
 9 files changed, 121 insertions(+), 1 deletion(-)

Changes.rst +12 -0

index db9b18b9..a6090cf5 100644
@@ -321,6 +321,18 @@ Maintainer-visible changes
   i386/i686 builds on RHEL5.
 
 
+Version 2.4.5
+=============
+
+New features
+------------
+- The new option ``--tls-cert-profile`` can be used to restrict the set of
+  allowed crypto algorithms in TLS certificates in mbed TLS builds.  The
+  default profile is 'legacy' for now, which allows SHA1+, RSA-1024+ and any
+  elliptic curve certificates.  The default will be changed to the 'preferred'
+  profile in the future, which requires SHA2+, RSA-2048+ and any curve.
+
+
 Version 2.4.3
 =============
 

doc/openvpn.8 +2 -0

index a4189ac2..6059c81d 100644
@@ -4917,6 +4917,28 @@ when using mbed TLS or
 OpenSSL.
 .\"*********************************************************
 .TP
+.B \-\-tls\-cert\-profile profile
+Set the allowed cryptographic algorithms for certificates according to

doc/openvpn.8 +10 -0

+.B suiteb
+: SHA256/SHA384, ECDSA with P-256 or P-384.
+
+This option is only supported for mbed TLS builds.  OpenSSL builds accept any
+certificate that OpenSSL accepts.
+
+OpenVPN will migrate to 'preferred' as default in the future.  Please ensure
+that your keys already comply.
+.\"*********************************************************
+.TP
 .B \-\-tls\-timeout n
 Packet retransmit timeout on TLS control channel
 if no acknowledgment from remote within

src/openvpn/options.c +6 -1

index 49ed004b..23c2c02a 100644
@@ -599,6 +599,8 @@ static const char usage_message[] =
 #endif
     "--tls-cipher l  : A list l of allowable TLS ciphers separated by : (optional).\n"
     "                : Use --show-tls to see a list of supported TLS ciphers.\n"
+    "--tls-cert-profile p : Set the allowed certificate crypto algorithm profile\n"
+    "                  (default=%s).\n"
     "--tls-timeout n : Packet retransmit timeout on TLS control channel\n"
     "                  if no ACK from remote within n seconds (default=%d).\n"
     "--reneg-bytes n : Renegotiate data chan. key after n bytes sent and recvd.\n"

@@ -872,6 +874,7 @@ init_options(struct options *o, const bool init_gc)
     o->renegotiate_seconds = 3600;
     o->handshake_window = 60;
     o->transition_window = 3600;
+    o->tls_cert_profile = "legacy";
     o->ecdh_curve = NULL;
 #ifdef ENABLE_X509ALTUSERNAME
     o->x509_username_field = X509_USERNAME_FIELD_DEFAULT;

@@ -1750,6 +1753,7 @@ show_settings(const struct options *o)
     SHOW_STR(cryptoapi_cert);
 #endif
     SHOW_STR(cipher_list);
+    SHOW_STR(tls_cert_profile);
     SHOW_STR(tls_verify);
     SHOW_STR(tls_export_cert);
     SHOW_INT(verify_x509_type);

@@ -2729,6 +2733,7 @@ options_postprocess_verify_ce(const struct options *options, const struct connec
         MUST_BE_UNDEF(pkcs12_file);
 #endif
         MUST_BE_UNDEF(cipher_list);
+        MUST_BE_UNDEF(tls_cert_profile);
         MUST_BE_UNDEF(tls_verify);
         MUST_BE_UNDEF(tls_export_cert);
         MUST_BE_UNDEF(verify_x509_name);

@@ -4086,7 +4091,7 @@ usage(void)
             o.verbosity,
             o.authname, o.ciphername,
             o.replay_window, o.replay_time,
-            o.tls_timeout, o.renegotiate_seconds,
+            o.tls_cert_profile, o.tls_timeout, o.renegotiate_seconds,
             o.handshake_window, o.transition_window);
 #else  /* ifdef ENABLE_CRYPTO */
     fprintf(fp, usage_message,

@@ -7831,6 +7836,15 @@ add_option(struct options *options,
         VERIFY_PERMISSION(OPT_P_GENERAL);

src/openvpn/options.c +1 -0

+    }
     else if (streq(p[0], "crl-verify") && p[1] && ((p[2] && streq(p[2], "dir"))
                                                    || (p[2] && streq(p[1], INLINE_FILE_TAG) ) || !p[2]) && !p[3])
     {

src/openvpn/options.h +1 -0

index 496c1143..85ff200b 100644
@@ -502,6 +502,7 @@ struct options
     const char *priv_key_file;
     const char *pkcs12_file;
     const char *cipher_list;
+    const char *tls_cert_profile;
     const char *ecdh_curve;
     const char *tls_verify;
     int verify_x509_type;

src/openvpn/ssl.c +3 -0

index cb94229a..48c1b477 100644
@@ -621,6 +621,9 @@ init_ssl(const struct options *options, struct tls_root_ctx *new_ctx)
      * cipher restrictions before loading certificates */
     tls_ctx_restrict_ciphers(new_ctx, options->cipher_list);
 
+    /* Restrict allowed certificate crypto algorithms */
+    tls_ctx_set_cert_profile(new_ctx, options->tls_cert_profile);
+
     tls_ctx_set_options(new_ctx, options->ssl_flags);
 
     if (options->pkcs12_file)

src/openvpn/ssl_backend.h +10 -0

index aba5a4de..ddc76a97 100644
@@ -176,6 +176,16 @@ void tls_ctx_set_options(struct tls_root_ctx *ctx, unsigned int ssl_flags);
  */
 void tls_ctx_restrict_ciphers(struct tls_root_ctx *ctx, const char *ciphers);
 
+/**
+ * Set the TLS certificate profile.  The profile defines which crypto
+ * algorithms may be used in the supplied certificate.
+ *
+ * @param ctx           TLS context to restrict, must be valid.
+ * @param profile       The profile name ('preferred', 'legacy' or 'suiteb').
+ *                      Defaults to 'preferred' if NULL.
+ */
+void tls_ctx_set_cert_profile(struct tls_root_ctx *ctx, const char *profile);
+
 /**
  * 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,

src/openvpn/ssl_mbedtls.c +28 -0

index 861d936d..6950abfe 100644
@@ -62,6 +62,34 @@
 #include <mbedtls/pem.h>
 #include <mbedtls/sha256.h>
 
+static const mbedtls_x509_crt_profile openvpn_x509_crt_profile_legacy =
+{
+    /* Hashes from SHA-1 and above */
+    MBEDTLS_X509_ID_FLAG( MBEDTLS_MD_SHA1 ) |
+    MBEDTLS_X509_ID_FLAG( MBEDTLS_MD_RIPEMD160 ) |
+    MBEDTLS_X509_ID_FLAG( MBEDTLS_MD_SHA224 ) |
+    MBEDTLS_X509_ID_FLAG( MBEDTLS_MD_SHA256 ) |
+    MBEDTLS_X509_ID_FLAG( MBEDTLS_MD_SHA384 ) |
+    MBEDTLS_X509_ID_FLAG( MBEDTLS_MD_SHA512 ),
+    0xFFFFFFF, /* Any PK alg    */
+    0xFFFFFFF, /* Any curve     */
+    1024,      /* RSA-1024 and larger */
+};
+
+static const mbedtls_x509_crt_profile openvpn_x509_crt_profile_preferred =
+{
+    /* SHA-2 and above */
+    MBEDTLS_X509_ID_FLAG( MBEDTLS_MD_SHA224 ) |
+    MBEDTLS_X509_ID_FLAG( MBEDTLS_MD_SHA256 ) |
+    MBEDTLS_X509_ID_FLAG( MBEDTLS_MD_SHA384 ) |
+    MBEDTLS_X509_ID_FLAG( MBEDTLS_MD_SHA512 ),
+    0xFFFFFFF, /* Any PK alg    */
+    0xFFFFFFF, /* Any curve     */
+    2048,      /* RSA-2048 and larger */
+};
+
+#define openvpn_x509_crt_profile_suiteb mbedtls_x509_crt_profile_suiteb;
+
 void
 tls_init_lib(void)
 {

src/openvpn/ssl_mbedtls.c +19 -0

+    {
+        ctx->cert_profile = openvpn_x509_crt_profile_preferred;
+    }
+    else if (0 == strcmp(profile, "legacy"))
+    {
+        ctx->cert_profile = openvpn_x509_crt_profile_legacy;
+    }
+    else if (0 == strcmp(profile, "suiteb"))
+    {
+        ctx->cert_profile = openvpn_x509_crt_profile_suiteb;
+    }
+    else
+    {
+        msg (M_FATAL, "ERROR: Invalid cert profile: %s", profile);
+    }
+}
+
 void
 tls_ctx_check_cert_time(const struct tls_root_ctx *ctx)
 {

@@ -917,6 +966,8 @@ key_state_ssl_init(struct key_state_ssl *ks_ssl,
     mbedtls_ssl_conf_rng(&ks_ssl->ssl_config, mbedtls_ctr_drbg_random,
                          rand_ctx_get());
 
+    mbedtls_ssl_conf_cert_profile(&ks_ssl->ssl_config, &ssl_ctx->cert_profile);
+
     if (ssl_ctx->allowed_ciphers)
     {
         mbedtls_ssl_conf_ciphersuites(&ks_ssl->ssl_config, ssl_ctx->allowed_ciphers);

src/openvpn/ssl_mbedtls.h +1 -0

index f69b6100..341da7d4 100644
@@ -82,6 +82,7 @@ struct tls_root_ctx {
     struct external_context *external_key; /**< Management external key */
 #endif
     int *allowed_ciphers;       /**< List of allowed ciphers for this connection */
+    mbedtls_x509_crt_profile cert_profile; /**< Allowed certificate types */
 };
 
 struct key_state_ssl {

src/openvpn/ssl_openssl.c +6 -0

index 0bfb6939..aed36a26 100644
@@ -383,6 +383,12 @@ tls_ctx_restrict_ciphers(struct tls_root_ctx *ctx, const char *ciphers)
     }
 }
 
+void
+tls_ctx_set_cert_profile(struct tls_root_ctx *ctx, const char *profile)
+{
+    /* TODO --tls-cert-profile not supported for OpenSSL builds */
+}
+
 void
 tls_ctx_check_cert_time(const struct tls_root_ctx *ctx)
 {

Quoted 81 lines · Steffan Karger

From: Steffan Karger <[email protected]...>

This allows the user to specify what certificate crypto algorithms to support. The supported profiles are 'preferred', 'legacy' (default) and 'suiteb', as discussed in <[email protected]...>
(https://www.mail-archive.com/[email protected]/msg14214.html).

This only implements the feature for mbed TLS builds, because for mbed it is both more easy to implement and the most relevant because mbed TLS 2+ is by default somewhat restrictive by requiring 2048-bit+ for RSA keys.

This patch uses 'legacy' as the default profile following discussion on the openvpn-devel mailing list. This way this patch can be applied to both the release/2.4 and master branches. I'll send a follow-up patch for the master branch to change the default to 'preferred' later.

Signed-off-by Steffan Karger <[email protected]...>

---
v2:
- add documentation (manpage, Changes.rst and --help)
- no longer print a warning message on each startup for OpenSSL builds
v3:
- remove format changes in unrelated items (introduced in v2)
- Update Changes.rst text to reflect that the default in 2.4.2 is 'legacy' and the default in 2.5 will be 'preferred' (as discussed on the ML).
- This patch is for the master branch only now (due to the default).
v4:
- Ignore the option for OpenSSL builds, instead of rejecting the option and refusing to start.
- Patch is for master and release/2.4 again. A follow-up patch will change the default for master later on.

 Changes.rst               | 12 +++++++++++
 doc/openvpn.8             | 22 ++++++++++++++++++++
 src/openvpn/options.c     | 16 ++++++++++++++-
 src/openvpn/options.h     |  1 +
 src/openvpn/ssl.c         |  3 +++
 src/openvpn/ssl_backend.h | 10 ++++++++++
 src/openvpn/ssl_mbedtls.c | 51 +++++++++++++++++++++++++++++++++++++++++++++++
 src/openvpn/ssl_mbedtls.h |  1 +
 src/openvpn/ssl_openssl.c |  6 ++++++
 9 files changed, 121 insertions(+), 1 deletion(-)

Changes.rst +12 -0

index db9b18b9..a6090cf5 100644
@@ -321,6 +321,18 @@ Maintainer-visible changes
   i386/i686 builds on RHEL5.
 
 
+Version 2.4.5
+=============
+
+New features
+------------
+- The new option ``--tls-cert-profile`` can be used to restrict the set of
+  allowed crypto algorithms in TLS certificates in mbed TLS builds.  The
+  default profile is 'legacy' for now, which allows SHA1+, RSA-1024+ and any
+  elliptic curve certificates.  The default will be changed to the 'preferred'
+  profile in the future, which requires SHA2+, RSA-2048+ and any curve.
+
+
 Version 2.4.3
 =============
 

doc/openvpn.8 +7 -0

index a4189ac2..6059c81d 100644
@@ -4917,6 +4917,28 @@ when using mbed TLS or
 OpenSSL.
 .\"*********************************************************
 .TP
+.B \-\-tls\-cert\-profile profile
+Set the allowed cryptographic algorithms for certificates according to
+.B profile\fN.
+
+The following profiles are supported:
+
+.B preferred

doc/openvpn.8 when using mbed TLS or · line 4931

Quoted 5 lines

doc/openvpn.8 +5 -0

+: SHA2 and newer, RSA 2048-bit+, any elliptic curve.
+
+.B legacy
+(default): SHA1 and newer, RSA 2048-bit+, any elliptic curve.
+

don't you think it would be better to put legacy as first profile? That would sort the list from the weakest to the strongest profile.

Quoted 61 lines

doc/openvpn.8 +10 -0

+.B suiteb
+: SHA256/SHA384, ECDSA with P-256 or P-384.
+
+This option is only supported for mbed TLS builds.  OpenSSL builds accept any
+certificate that OpenSSL accepts.
+
+OpenVPN will migrate to 'preferred' as default in the future.  Please ensure
+that your keys already comply.
+.\"*********************************************************
+.TP
 .B \-\-tls\-timeout n
 Packet retransmit timeout on TLS control channel
 if no acknowledgment from remote within

src/openvpn/options.c +6 -1

index 49ed004b..23c2c02a 100644
@@ -599,6 +599,8 @@ static const char usage_message[] =
 #endif
     "--tls-cipher l  : A list l of allowable TLS ciphers separated by : (optional).\n"
     "                : Use --show-tls to see a list of supported TLS ciphers.\n"
+    "--tls-cert-profile p : Set the allowed certificate crypto algorithm profile\n"
+    "                  (default=%s).\n"
     "--tls-timeout n : Packet retransmit timeout on TLS control channel\n"
     "                  if no ACK from remote within n seconds (default=%d).\n"
     "--reneg-bytes n : Renegotiate data chan. key after n bytes sent and recvd.\n"

@@ -872,6 +874,7 @@ init_options(struct options *o, const bool init_gc)
     o->renegotiate_seconds = 3600;
     o->handshake_window = 60;
     o->transition_window = 3600;
+    o->tls_cert_profile = "legacy";
     o->ecdh_curve = NULL;
 #ifdef ENABLE_X509ALTUSERNAME
     o->x509_username_field = X509_USERNAME_FIELD_DEFAULT;

@@ -1750,6 +1753,7 @@ show_settings(const struct options *o)
     SHOW_STR(cryptoapi_cert);
 #endif
     SHOW_STR(cipher_list);
+    SHOW_STR(tls_cert_profile);
     SHOW_STR(tls_verify);
     SHOW_STR(tls_export_cert);
     SHOW_INT(verify_x509_type);

@@ -2729,6 +2733,7 @@ options_postprocess_verify_ce(const struct options *options, const struct connec
         MUST_BE_UNDEF(pkcs12_file);
 #endif
         MUST_BE_UNDEF(cipher_list);
+        MUST_BE_UNDEF(tls_cert_profile);
         MUST_BE_UNDEF(tls_verify);
         MUST_BE_UNDEF(tls_export_cert);
         MUST_BE_UNDEF(verify_x509_name);

@@ -4086,7 +4091,7 @@ usage(void)
             o.verbosity,
             o.authname, o.ciphername,
             o.replay_window, o.replay_time,
-            o.tls_timeout, o.renegotiate_seconds,
+            o.tls_cert_profile, o.tls_timeout, o.renegotiate_seconds,
             o.handshake_window, o.transition_window);
 #else  /* ifdef ENABLE_CRYPTO */
     fprintf(fp, usage_message,

@@ -7831,6 +7836,15 @@ add_option(struct options *options,
         VERIFY_PERMISSION(OPT_P_GENERAL);

src/openvpn/options.c add_option(struct options *options, · line 7846

Quoted 10 lines

src/openvpn/options.c +8 -0

         options->cipher_list = p[1];
     }
+    else if (streq(p[0], "tls-cert-profile") && p[1] && !p[2])
+    {
+        VERIFY_PERMISSION(OPT_P_GENERAL);
+#ifdef ENABLE_CRYPTO_MBEDTLS
+        options->tls_cert_profile = p[1];
+#else
+        msg(M_WARN, "WARNING: --tls-cert-profile is not yet implemented for OpenSSL, ignoring option.");
+#endif

rather than polluting this code with a new ifdef, wouldn't it be meaningful to print this message in the openssl routine doing the SSL context setup?

This way we get rid of the ifdef and the "openssl" specific code (which is just the msg()) would reside in the openssl code.

after all this is not an exception on the parsing of the option, but it's an exception on how the value is used.

Quoted 94 lines

src/openvpn/options.c +1 -0

+    }
     else if (streq(p[0], "crl-verify") && p[1] && ((p[2] && streq(p[2], "dir"))
                                                    || (p[2] && streq(p[1], INLINE_FILE_TAG) ) || !p[2]) && !p[3])
     {

src/openvpn/options.h +1 -0

index 496c1143..85ff200b 100644
@@ -502,6 +502,7 @@ struct options
     const char *priv_key_file;
     const char *pkcs12_file;
     const char *cipher_list;
+    const char *tls_cert_profile;
     const char *ecdh_curve;
     const char *tls_verify;
     int verify_x509_type;

src/openvpn/ssl.c +3 -0

index cb94229a..48c1b477 100644
@@ -621,6 +621,9 @@ init_ssl(const struct options *options, struct tls_root_ctx *new_ctx)
      * cipher restrictions before loading certificates */
     tls_ctx_restrict_ciphers(new_ctx, options->cipher_list);
 
+    /* Restrict allowed certificate crypto algorithms */
+    tls_ctx_set_cert_profile(new_ctx, options->tls_cert_profile);
+
     tls_ctx_set_options(new_ctx, options->ssl_flags);
 
     if (options->pkcs12_file)

src/openvpn/ssl_backend.h +10 -0

index aba5a4de..ddc76a97 100644
@@ -176,6 +176,16 @@ void tls_ctx_set_options(struct tls_root_ctx *ctx, unsigned int ssl_flags);
  */
 void tls_ctx_restrict_ciphers(struct tls_root_ctx *ctx, const char *ciphers);
 
+/**
+ * Set the TLS certificate profile.  The profile defines which crypto
+ * algorithms may be used in the supplied certificate.
+ *
+ * @param ctx           TLS context to restrict, must be valid.
+ * @param profile       The profile name ('preferred', 'legacy' or 'suiteb').
+ *                      Defaults to 'preferred' if NULL.
+ */
+void tls_ctx_set_cert_profile(struct tls_root_ctx *ctx, const char *profile);
+
 /**
  * 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,

src/openvpn/ssl_mbedtls.c +28 -0

index 861d936d..6950abfe 100644
@@ -62,6 +62,34 @@
 #include <mbedtls/pem.h>
 #include <mbedtls/sha256.h>
 
+static const mbedtls_x509_crt_profile openvpn_x509_crt_profile_legacy =
+{
+    /* Hashes from SHA-1 and above */
+    MBEDTLS_X509_ID_FLAG( MBEDTLS_MD_SHA1 ) |
+    MBEDTLS_X509_ID_FLAG( MBEDTLS_MD_RIPEMD160 ) |
+    MBEDTLS_X509_ID_FLAG( MBEDTLS_MD_SHA224 ) |
+    MBEDTLS_X509_ID_FLAG( MBEDTLS_MD_SHA256 ) |
+    MBEDTLS_X509_ID_FLAG( MBEDTLS_MD_SHA384 ) |
+    MBEDTLS_X509_ID_FLAG( MBEDTLS_MD_SHA512 ),
+    0xFFFFFFF, /* Any PK alg    */
+    0xFFFFFFF, /* Any curve     */
+    1024,      /* RSA-1024 and larger */
+};
+
+static const mbedtls_x509_crt_profile openvpn_x509_crt_profile_preferred =
+{
+    /* SHA-2 and above */
+    MBEDTLS_X509_ID_FLAG( MBEDTLS_MD_SHA224 ) |
+    MBEDTLS_X509_ID_FLAG( MBEDTLS_MD_SHA256 ) |
+    MBEDTLS_X509_ID_FLAG( MBEDTLS_MD_SHA384 ) |
+    MBEDTLS_X509_ID_FLAG( MBEDTLS_MD_SHA512 ),
+    0xFFFFFFF, /* Any PK alg    */
+    0xFFFFFFF, /* Any curve     */
+    2048,      /* RSA-2048 and larger */
+};
+
+#define openvpn_x509_crt_profile_suiteb mbedtls_x509_crt_profile_suiteb;
+
 void
 tls_init_lib(void)
 {

@@ -250,6 +278,27 @@ tls_ctx_restrict_ciphers(struct tls_root_ctx *ctx, const char *ciphers)
     free(tmp_ciphers_orig);
 }
 

src/openvpn/ssl_mbedtls.c tls_ctx_restrict_ciphers(struct tls_root_ctx *ctx, const char *ciphers) · line 284

Quoted 4 lines

src/openvpn/ssl_mbedtls.c +4 -0

+void
+tls_ctx_set_cert_profile(struct tls_root_ctx *ctx, const char *profile)
+{
+    if (!profile || 0 == strcmp(profile, "preferred"))

can profile really be null? Given that we are assigning it a default value in init_options() I think we could rather use an assert() here? so we make sure the developer will always set an explicit default?

Quoted 49 lines

src/openvpn/ssl_mbedtls.c +19 -0

+    {
+        ctx->cert_profile = openvpn_x509_crt_profile_preferred;
+    }
+    else if (0 == strcmp(profile, "legacy"))
+    {
+        ctx->cert_profile = openvpn_x509_crt_profile_legacy;
+    }
+    else if (0 == strcmp(profile, "suiteb"))
+    {
+        ctx->cert_profile = openvpn_x509_crt_profile_suiteb;
+    }
+    else
+    {
+        msg (M_FATAL, "ERROR: Invalid cert profile: %s", profile);
+    }
+}
+
 void
 tls_ctx_check_cert_time(const struct tls_root_ctx *ctx)
 {

@@ -917,6 +966,8 @@ key_state_ssl_init(struct key_state_ssl *ks_ssl,
     mbedtls_ssl_conf_rng(&ks_ssl->ssl_config, mbedtls_ctr_drbg_random,
                          rand_ctx_get());
 
+    mbedtls_ssl_conf_cert_profile(&ks_ssl->ssl_config, &ssl_ctx->cert_profile);
+
     if (ssl_ctx->allowed_ciphers)
     {
         mbedtls_ssl_conf_ciphersuites(&ks_ssl->ssl_config, ssl_ctx->allowed_ciphers);

src/openvpn/ssl_mbedtls.h +1 -0

index f69b6100..341da7d4 100644
@@ -82,6 +82,7 @@ struct tls_root_ctx {
     struct external_context *external_key; /**< Management external key */
 #endif
     int *allowed_ciphers;       /**< List of allowed ciphers for this connection */
+    mbedtls_x509_crt_profile cert_profile; /**< Allowed certificate types */
 };
 
 struct key_state_ssl {

src/openvpn/ssl_openssl.c +0 -0

index 0bfb6939..aed36a26 100644
@@ -383,6 +383,12 @@ tls_ctx_restrict_ciphers(struct tls_root_ctx *ctx, const char *ciphers)
     }
 }
 

src/openvpn/ssl_openssl.c tls_ctx_restrict_ciphers(struct tls_root_ctx *ctx, const char *ciphers) · line 394

Quoted 10 lines

src/openvpn/ssl_openssl.c +6 -0

+void
+tls_ctx_set_cert_profile(struct tls_root_ctx *ctx, const char *profile)
+{
+    /* TODO --tls-cert-profile not supported for OpenSSL builds */
+}
+
 void
 tls_ctx_check_cert_time(const struct tls_root_ctx *ctx)
 {

From: Steffan Karger <[email protected]...>

This allows the user to specify what certificate crypto algorithms to support. The supported profiles are 'preferred', 'legacy' (default) and 'suiteb', as discussed in <[email protected]....>
(https://www.mail-archive.com/[email protected]/msg14214.html).

This fully implements the feature for mbed TLS builds, because for mbed it is both more easy to implement and the most relevant because mbed TLS 2+ is by default somewhat restrictive by requiring 2048-bit+ for RSA keys.

For OpenSSL, this implements an approximation based on security levels, as discussed at the hackathon in Karlsruhe.

This patch uses 'legacy' as the default profile following discussion on the openvpn-devel mailing list. This way this patch can be applied to both the release/2.4 and master branches. I'll send a follow-up patch for the master branch to change the default to 'preferred' later.

Signed-off-by Steffan Karger <[email protected]...>

.. of course this conflicts with o->renegotiate_seconds_min...

Nevertheless, thanks for the patch :-) - it makes my FreeBSD 10.3 (mbedTLS 2.6) buildslave now happy again (on the default settings - with --tls-cert-profile preferred, it refuses the old-hash cert, as it should).

Also tested with openssl 1.0.1, where it warns and does nothing, as expected. Good :-)

Hi,

Quoted 8 lines · Gert Doering

.. of course this conflicts with o->renegotiate_seconds_min...

Nevertheless, thanks for the patch :-) - it makes my FreeBSD 10.3 (mbedTLS 2.6) buildslave now happy again (on the default settings - with --tls-cert-profile preferred, it refuses the old-hash cert, as it should).

Also tested with openssl 1.0.1, where it warns and does nothing, as expected. Good :-)

I *should* have tested with LibreSSL as well...

ssl_openssl.o: In function `tls_ctx_set_cert_profile':
/home/buildbot/build-openvpn/build-cron2-openbsd-60-amd64-stable-master--disable -lzo--disable-management/build/src/openvpn/ssl_openssl.c:404:
undefined reference to SSL_CTX_set_security_level' /home/buildbot/build-openvpn/build-cron2-openbsd-60-amd64-stable-master--disable-lzo--disable-management/build/src/openvpn/ssl_openssl.c:400: undefined reference to SSL_CTX_set_security_level'

... *roll eyes*

(Not sure, though, why it only complains about two out of three, but still annoyance... if LibreSSL claims OPENSSL_VERSION_NUMBER >= 0x10100000 it better should provide everything needed)

This is on OpenBSD 6.0, which happens to be something Samuli's vagrant setup can provide nicely if anyone wants to have a look :-)

Quoted 19 lines · Gert Doering

Hi,

Quoted 8 lines · Gert Doering

.. of course this conflicts with o->renegotiate_seconds_min...

Nevertheless, thanks for the patch :-) - it makes my FreeBSD 10.3 (mbedTLS 2.6) buildslave now happy again (on the default settings - with --tls-cert-profile preferred, it refuses the old-hash cert, as it should).

Also tested with openssl 1.0.1, where it warns and does nothing, as expected. Good :-)

I *should* have tested with LibreSSL as well...

ssl_openssl.o: In function `tls_ctx_set_cert_profile':
/home/buildbot/build-openvpn/build-cron2-openbsd-60-amd64-stable-master--disable -lzo--disable-management/build/src/openvpn/ssl_openssl.c:404:
undefined reference to SSL_CTX_set_security_level' /home/buildbot/build-openvpn/build-cron2-openbsd-60-amd64-stable-master--disable-lzo--disable-management/build/src/openvpn/ssl_openssl.c:400: undefined reference to SSL_CTX_set_security_level'

Quoted 5 lines

... *roll eyes*

(Not sure, though, why it only complains about two out of three, but still annoyance... if LibreSSL claims OPENSSL_VERSION_NUMBER >= 0x10100000 it better should provide everything needed)

LibreSSL defines:

  #define OPENSSL_VERSION_NUMBER        0x20000000L

breaking #ifdef checks based on it. Sadly, people tend to prefer adding

&& !defined(LIBRESSL_VERSION_NUMBER)

to fix the build, rather than doing features detection using autoconf or similar. openvpn can easily solve this.

Quoted 2 lines

This is on OpenBSD 6.0, which happens to be something Samuli's vagrant setup can provide nicely if anyone wants to have a look :-)

Here's a diff, master builds and seems to run fine as a client on OpenBSD-current.

I can cook a similar diff for the remaining OPENSSL / LIBRESSL_VERSION_NUMBER #ifdef.

Here's another diff to detect SSL_CTX_get0_certificate().

Tested against LibreSSL only; adding

  #define HAVE_SSL_CTX_GET0_CERTIFICATE 1

to config.h lets ssl_openssl.c build (with a warning), the link fails as expected.

From 1abd6089a45260e4ce7adfae3fa619f9055edcaf Mon Sep 17 00:00:00 2001
From: Jeremie Courreges-Anglas <[email protected]...>
Date: Sun, 19 Nov 2017 23:12:30 +0100
Subject: [PATCH] Detect if SSL_CTX_get0_certificate is available

Don't rely on #ifdef OPENSSL/LIBRESSL_VERSION_NUMBER checks.

Signed-off-by Jeremie Courreges-Anglas <[email protected]...>

---
 configure.ac              | 1 +
 src/openvpn/ssl_openssl.c | 4 ++--
 2 files changed, 3 insertions(+), 2 deletions(-)

configure.ac +1 -0

index acfddb22..ac6e7a76 100644
@@ -925,6 +925,7 @@ if test "${enable_crypto}" = "yes" -a "${with_crypto_library}" = "openssl"; then
 			EVP_MD_CTX_new \
 			EVP_MD_CTX_free \
 			EVP_MD_CTX_reset \
+			SSL_CTX_get0_certificate \
 			SSL_CTX_get_default_passwd_cb \
 			SSL_CTX_get_default_passwd_cb_userdata \
 			SSL_CTX_set_security_level \

src/openvpn/ssl_openssl.c +2 -2

index b782946e..3df70166 100644
@@ -425,7 +425,7 @@ tls_ctx_check_cert_time(const struct tls_root_ctx *ctx)
 
     ASSERT(ctx);
 
-#if OPENSSL_VERSION_NUMBER >= 0x10002000L && !defined(LIBRESSL_VERSION_NUMBER)
+#ifdef HAVE_SSL_CTX_GET0_CERTIFICATE
     /* OpenSSL 1.0.2 and up */
     cert = SSL_CTX_get0_certificate(ctx->ctx);
 #else

@@ -460,7 +460,7 @@ tls_ctx_check_cert_time(const struct tls_root_ctx *ctx)
     }
 
 cleanup:
-#if OPENSSL_VERSION_NUMBER < 0x10002000L || defined(LIBRESSL_VERSION_NUMBER)
+#ifndef HAVE_SSL_CTX_GET0_CERTIFICATE
     SSL_free(ssl);
 #endif
     return;

Signature · 1 line

2.15.0

NAK.

Looking at this patch again I realize I have misunderstood the intentions when first looking at it. I thought LibreSSL *did* have an SSL_CTX_get0_certificate() and this patch would make us use it (instead of the workaround in the #else). But this is just about replacing the version check with a configure check.

I oppose that change because it
hides information I want to have: "what code can be purged when we drop support for openssl 1.0 and libressl?".

On Thu, Dec 14 2017, Steffan Karger <[email protected]...> wrote:

[...]

Quoted 2 lines

NAK.

Quoted 5 lines

Looking at this patch again I realize I have misunderstood the intentions when first looking at it. I thought LibreSSL *did* have an SSL_CTX_get0_certificate() and this patch would make us use it (instead of the workaround in the #else). But this is just about replacing the version check with a configure check.

Are you still opposed to such a diff (updated version attached), now that LibreSSL HEAD provides SSL_CTX_get0_certificate?

x.y.z ?

Quoted 3 lines

I oppose that change because it
hides information I want to have: "what code can be purged when we drop support for openssl 1.0 and libressl?".

Maybe there's another way to encode that information? Like, consistently formatted comments describing the first OpenSSL (and LibreSSL) releases that provided a function?

Yes, I'd rather not use the workaround if not needed.

Quoted 3 lines

(Not sure, though, why it only complains about two out of three, but still annoyance... if LibreSSL claims OPENSSL_VERSION_NUMBER >= 0x10100000 it better should provide everything needed)

LibreSSL defines:

  #define OPENSSL_VERSION_NUMBER        0x20000000L

breaking #ifdef checks based on it.

Sadly, people tend to prefer adding

&& !defined(LIBRESSL_VERSION_NUMBER)

to fix the build, rather than doing features detection using autoconf or similar. openvpn can easily solve this.

Quoted 2 lines

This is on OpenBSD 6.0, which happens to be something Samuli's vagrant setup can provide nicely if anyone wants to have a look :-)

Here's a diff, master builds and seems to run fine as a client on OpenBSD-current.

I can cook a similar diff for the remaining OPENSSL / LIBRESSL_VERSION_NUMBER #ifdef.

Quoted 7 lines · wrote:

Quoted 4 lines

(Not sure, though, why it only complains about two out of three, but still annoyance... if LibreSSL claims OPENSSL_VERSION_NUMBER >= 0x10100000 it better should provide everything needed)

LibreSSL defines:

Quoted 3 lines

#define OPENSSL_VERSION_NUMBER 0x20000000L

breaking #ifdef checks based on it.

Indeed. I find this a curious and not useful setting - "if it's not compatible with OPENSSL, why define such a version number"? But that's slightly out of scope here...

Quoted 6 lines

Sadly, people tend to prefer adding

&& !defined(LIBRESSL_VERSION_NUMBER)

to fix the build, rather than doing features detection using autoconf or similar. openvpn can easily solve this.

... and I'm thankful for your patch, because this is exactly what I
considered doing here. We already check for all the 1.0/1.1
openssl differences (accessor functions), so adding this one is logical.

Quoted 6 lines

Quoted 3 lines

This is on OpenBSD 6.0, which happens to be something Samuli's vagrant setup can provide nicely if anyone wants to have a look :-)

Here's a diff, master builds and seems to run fine as a client on OpenBSD-current.

Thanks. Patch looks good to me, but I leave the final word to Steffan (maybe he wants to amend the non-support message to include LibreSSL, or whatever)

Quoted 2 lines

I can cook a similar diff for the remaining OPENSSL / LIBRESSL_VERSION_NUMBER #ifdef.

This would be appreciated.

Here's a diff, master builds and seems to run fine as a client on OpenBSD-current.

From: Jeremie Courreges-Anglas <[email protected]...>
Date: Sun, 19 Nov 2017 22:57:56 +0100
Subject: [PATCH] Fix build with LibreSSL

Detect the presence of SSL_CTX_set_security_level(), don't check OPENSSL_VERSION_NUMBER.

Signed-off-by Jeremie Courreges-Anglas <[email protected]...>

---
 configure.ac              | 1 +
 src/openvpn/ssl_openssl.c | 2 +-
 2 files changed, 2 insertions(+), 1 deletion(-)

configure.ac +1 -0

index 7f2e34f2..acfddb22 100644
@@ -927,6 +927,7 @@ if test "${enable_crypto}" = "yes" -a "${with_crypto_library}" = "openssl"; then
 			EVP_MD_CTX_reset \
 			SSL_CTX_get_default_passwd_cb \
 			SSL_CTX_get_default_passwd_cb_userdata \
+			SSL_CTX_set_security_level \
 			X509_get0_pubkey \
 			X509_STORE_get0_objects \
 			X509_OBJECT_free \

src/openvpn/ssl_openssl.c +1 -1

index de89cb13..b782946e 100644
@@ -386,7 +386,7 @@ tls_ctx_restrict_ciphers(struct tls_root_ctx *ctx, const char *ciphers)
 void
 tls_ctx_set_cert_profile(struct tls_root_ctx *ctx, const char *profile)
 {
-#if (OPENSSL_VERSION_NUMBER >= 0x10100000)
+#ifdef HAVE_SSL_CTX_SET_SECURITY_LEVEL
     /* OpenSSL does not have certificate profiles, but a complex set of
      * callbacks that we could try to implement to achieve something similar.
      * For now, use OpenSSL's security levels to achieve similar (but not equal)

Signature · 1 line

2.15.0