Message ID | 20171112163636.17434-1-steffan@karger.me |
---|---|
State | Accepted |
Headers | show |
Series | [Openvpn-devel,v6] Add --tls-cert-profile option for mbedtls builds | expand |
Hi, On 13/11/17 00:36, Steffan Karger wrote: > From: Steffan Karger <steffan.karger@fox-it.com> > > This allows the user to specify what certificate crypto algorithms to > support. The supported profiles are 'preferred', 'legacy' (default) and > 'suiteb', as discussed in <84590a17-1c48-9df2-c48e-4160750b2e33@fox-it.com> > (https://www.mail-archive.com/openvpn-devel@lists.sourceforge.net/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 <steffan.karger@fox-it.com> Code looks good, but the commit subject is now wrong, because this patch is actually implementing cert profiles for both mbedTLS and OpenSSL. So I ACK it, but the committer should fix the subject for sake of clarity. I have tested the patch with mbedTLS and with OpenSSL 1.0, but not with OpenSSL 1.1. Acked-by: Antonio Quartulli <a@unstable.cc>
.. 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 :-) Commit subject amended according to Antonio's comment. Your patch has been applied to the master and release/2.4 branch. commit aba758740d26224b7b3957df221def7ab80c5802 (master) commit 8bcabf0a1621e6ccc7a44465a73de29fd2d541b3 (release/2.4) Author: Steffan Karger Date: Sun Nov 12 17:36:36 2017 +0100 Add --tls-cert-profile option. Signed-off-by: Steffan Karger <steffan.karger@fox-it.com> Acked-by: Antonio Quartulli <antonio@openvpn.net> Message-Id: <20171112163636.17434-1-steffan@karger.me> URL: https://www.mail-archive.com/openvpn-devel@lists.sourceforge.net/msg15848.html Signed-off-by: Gert Doering <gert@greenie.muc.de> -- kind regards, Gert Doering ------------------------------------------------------------------------------ Check out the vibrant tech community on one of the world's most engaging tech sites, Slashdot.org! http://sdm.link/slashdot
Hi, On Sun, Nov 19, 2017 at 09:37:56PM +0100, Gert Doering wrote: > .. 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 :-) gert
On Sun, Nov 19 2017, Gert Doering <gert@greenie.muc.de> wrote: > Hi, > > On Sun, Nov 19, 2017 at 09:37:56PM +0100, Gert Doering wrote: >> .. 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) 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. > 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. From 15315d3c3b25814a426bfc8184c4dfd262f28768 Mon Sep 17 00:00:00 2001 From: Jeremie Courreges-Anglas <jca@wxcvbn.org> 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 <jca@wxcvbn.org> --- configure.ac | 1 + src/openvpn/ssl_openssl.c | 2 +- 2 files changed, 2 insertions(+), 1 deletion(-) diff --git a/configure.ac b/configure.ac index 7f2e34f2..acfddb22 100644 --- a/configure.ac +++ b/configure.ac @@ -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 \ diff --git a/src/openvpn/ssl_openssl.c b/src/openvpn/ssl_openssl.c index de89cb13..b782946e 100644 --- a/src/openvpn/ssl_openssl.c +++ b/src/openvpn/ssl_openssl.c @@ -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)
On Sun, Nov 19 2017, Jeremie Courreges-Anglas <jca@wxcvbn.org> wrote: > On Sun, Nov 19 2017, Gert Doering <gert@greenie.muc.de> wrote: >> Hi, >> >> On Sun, Nov 19, 2017 at 09:37:56PM +0100, Gert Doering wrote: >>> .. 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) > > 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. > >> 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 <jca@wxcvbn.org> 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 <jca@wxcvbn.org> --- configure.ac | 1 + src/openvpn/ssl_openssl.c | 4 ++-- 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/configure.ac b/configure.ac index acfddb22..ac6e7a76 100644 --- a/configure.ac +++ b/configure.ac @@ -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 \ diff --git a/src/openvpn/ssl_openssl.c b/src/openvpn/ssl_openssl.c index b782946e..3df70166 100644 --- a/src/openvpn/ssl_openssl.c +++ b/src/openvpn/ssl_openssl.c @@ -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;
Hi, On Sun, Nov 19, 2017 at 11:01:39PM +0100, Jeremie Courreges-Anglas wrote: > > (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. 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... > 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. > > 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) > I can cook a similar diff for the remaining OPENSSL / > LIBRESSL_VERSION_NUMBER #ifdef. This would be appreciated. gert
Hi, On 20-11-17 09:06, Gert Doering wrote:> On Sun, Nov 19, 2017 at 11:01:39PM +0100, Jeremie Courreges-Anglas > wrote: >>> (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. > > 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... +1, highly frustrating. LibreSSL should really just make up their mind whether they want to be OpenSSL-compatible or not, and act accordingly. >> 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. *If* we want to keep LibreSSL working, I agree this is the way to go. But I'm kind of annoyed that we are including more and more #ifdefs to keep LibreSSL happy. The version checks are much simpler and make it easy to see what code can be purged when we drop support for e.g openssl 1.0.1. I don't want to keep these 'backwards compatibility' ifdefs forever. At some point we'll have to decide to either completely stop supporting LibreSSL, or add it as a true abstraction (which I will *not* maintain). We're getting closer and closer to that point. >>> 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) They look good at first sight, but I'll check these properly later this week - when I have some spare cycles available. >> I can cook a similar diff for the remaining OPENSSL / >> LIBRESSL_VERSION_NUMBER #ifdef. > > This would be appreciated. Same reservations as above. To reiterate: our policy towards LibreSSL is currently that we do *not* support it, but we won't break it on purpose and accept trivial patches to keep it working. Where 'trivial' is - of course - fuzzy. -Steffan ------------------------------------------------------------------------------ Check out the vibrant tech community on one of the world's most engaging tech sites, Slashdot.org! http://sdm.link/slashdot
Hi, On 19-11-17 23:01, Jeremie Courreges-Anglas wrote: > Here's a diff, master builds and seems to run fine as a client on > OpenBSD-current. > > > From: Jeremie Courreges-Anglas <jca@wxcvbn.org> > 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 <jca@wxcvbn.org> > --- > configure.ac | 1 + > src/openvpn/ssl_openssl.c | 2 +- > 2 files changed, 2 insertions(+), 1 deletion(-) > > diff --git a/configure.ac b/configure.ac > index 7f2e34f2..acfddb22 100644 > --- a/configure.ac > +++ b/configure.ac > @@ -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 \ > diff --git a/src/openvpn/ssl_openssl.c b/src/openvpn/ssl_openssl.c > index de89cb13..b782946e 100644 > --- a/src/openvpn/ssl_openssl.c > +++ b/src/openvpn/ssl_openssl.c > @@ -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) > -- > 2.15.0 Patch looks good and clean enough to restore compatibilty with libressl. Tested that this doesn't break --tls-cert-profile with OpenSSL 1.1, and doesn't break builds with OpenSSL 1.0. Acked-by: Steffan Karger <steffan@karger.me> Tested-by: Steffan Karger <steffan@karger.me> -Steffan ------------------------------------------------------------------------------ Check out the vibrant tech community on one of the world's most engaging tech sites, Slashdot.org! http://sdm.link/slashdot
Thanks. Tested on OpenBSD 6.0, fixes compilation again. Your patch has been applied to the master and release/2.4 branch. commit 88a827f25cb4a79f06597ca438f8f04d37a03d4e (master) commit eac1e089239999a161d81e0222d461c8b580a776 (release/2.4) Author: Jeremie Courreges-Anglas Date: Sun Nov 19 22:57:56 2017 +0100 Fix build with LibreSSL Signed-off-by: Jeremie Courreges-Anglas <jca@wxcvbn.org> Acked-by: Steffan Karger <steffan.karger@fox-it.com> Message-Id: <8760a6kjwc.fsf@ritchie.wxcvbn.org> URL: https://www.mail-archive.com/openvpn-devel@lists.sourceforge.net/msg15902.html Signed-off-by: Gert Doering <gert@greenie.muc.de> -- kind regards, Gert Doering ------------------------------------------------------------------------------ Check out the vibrant tech community on one of the world's most engaging tech sites, Slashdot.org! http://sdm.link/slashdot
Hi, On 19-11-17 23:18, Jeremie Courreges-Anglas wrote: > 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 <jca@wxcvbn.org> > 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 <jca@wxcvbn.org> > --- > configure.ac | 1 + > src/openvpn/ssl_openssl.c | 4 ++-- > 2 files changed, 3 insertions(+), 2 deletions(-) > > diff --git a/configure.ac b/configure.ac > index acfddb22..ac6e7a76 100644 > --- a/configure.ac > +++ b/configure.ac > @@ -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 \ > diff --git a/src/openvpn/ssl_openssl.c b/src/openvpn/ssl_openssl.c > index b782946e..3df70166 100644 > --- a/src/openvpn/ssl_openssl.c > +++ b/src/openvpn/ssl_openssl.c > @@ -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; > -- > 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?". -Steffan ------------------------------------------------------------------------------ Check out the vibrant tech community on one of the world's most engaging tech sites, Slashdot.org! http://sdm.link/slashdot
On Thu, Dec 14 2017, Steffan Karger <steffan@karger.me> wrote: [...] > 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. Are you still opposed to such a diff (updated version attached), now that LibreSSL HEAD provides SSL_CTX_get0_certificate? > 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?
Hi, On 04-03-18 19:59, Jeremie Courreges-Anglas wrote: > On Thu, Dec 14 2017, Steffan Karger <steffan@karger.me> wrote: > > [...] > >> 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. > > Are you still opposed to such a diff (updated version attached), now > that LibreSSL HEAD provides SSL_CTX_get0_certificate? Yes, I'd rather not use the workaround if not needed. Still not very happy about the approach though. Why not simply add || LIBRESSL_VERSION > x.y.z ? >> 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, we could do that. But if we're going to put that info into the code anyway, why not just use the define? -Steffan ------------------------------------------------------------------------------ Check out the vibrant tech community on one of the world's most engaging tech sites, Slashdot.org! http://sdm.link/slashdot
On 05-03-18 00:26, Steffan Karger wrote:
> Yes, I'd rather not use the workaround if not needed.
Bad wording. Read that as "I'm no longer opposed to a patch".
-Steffan
------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot
diff --git a/Changes.rst b/Changes.rst index db9b18b9..a6090cf5 100644 --- a/Changes.rst +++ b/Changes.rst @@ -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 ============= diff --git a/doc/openvpn.8 b/doc/openvpn.8 index a4189ac2..549d96cb 100644 --- a/doc/openvpn.8 +++ b/doc/openvpn.8 @@ -4917,6 +4917,37 @@ 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 legacy +(default): SHA1 and newer, RSA 2048-bit+, any elliptic curve. + +.B preferred +: SHA2 and newer, RSA 2048-bit+, any elliptic curve. + +.B suiteb +: SHA256/SHA384, ECDSA with P-256 or P-384. + +This option is only fully supported for mbed TLS builds. OpenSSL builds use +the following approximation: + +.B legacy +(default): sets "security level 1" + +.B preferred +: sets "security level 2" + +.B suiteb +: sets "security level 3" and \-\-tls\-cipher "SUITEB128". + +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 diff --git a/src/openvpn/init.c b/src/openvpn/init.c index 1ed2c55e..cccc63b2 100644 --- a/src/openvpn/init.c +++ b/src/openvpn/init.c @@ -1016,7 +1016,8 @@ print_openssl_info(const struct options *options) } if (options->show_tls_ciphers) { - show_available_tls_ciphers(options->cipher_list); + show_available_tls_ciphers(options->cipher_list, + options->tls_cert_profile); } if (options->show_curves) { diff --git a/src/openvpn/options.c b/src/openvpn/options.c index 49ed004b..9d6e279a 100644 --- a/src/openvpn/options.c +++ b/src/openvpn/options.c @@ -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=legacy).\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 = NULL; 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); @@ -7831,6 +7836,11 @@ 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); + options->tls_cert_profile = p[1]; + } 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]) { diff --git a/src/openvpn/options.h b/src/openvpn/options.h index 496c1143..85ff200b 100644 --- a/src/openvpn/options.h +++ b/src/openvpn/options.h @@ -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; diff --git a/src/openvpn/ssl.c b/src/openvpn/ssl.c index cb94229a..359ef635 100644 --- a/src/openvpn/ssl.c +++ b/src/openvpn/ssl.c @@ -616,6 +616,9 @@ init_ssl(const struct options *options, struct tls_root_ctx *new_ctx) tls_ctx_client_new(new_ctx); } + /* Restrict allowed certificate crypto algorithms */ + tls_ctx_set_cert_profile(new_ctx, options->tls_cert_profile); + /* Allowable ciphers */ /* Since @SECLEVEL also influces loading of certificates, set the * cipher restrictions before loading certificates */ diff --git a/src/openvpn/ssl_backend.h b/src/openvpn/ssl_backend.h index aba5a4de..f588110c 100644 --- a/src/openvpn/ssl_backend.h +++ b/src/openvpn/ssl_backend.h @@ -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, @@ -505,9 +515,12 @@ void print_details(struct key_state_ssl *ks_ssl, const char *prefix); * Show the TLS ciphers that are available for us to use in the OpenSSL * library. * - * @param - list of allowed TLS cipher, or NULL. + * @param cipher_list list of allowed TLS cipher, or NULL. + * @param tls_cert_profile TLS certificate crypto profile name. */ -void show_available_tls_ciphers(const char *tls_ciphers); +void +show_available_tls_ciphers(const char *cipher_list, + const char *tls_cert_profile); /* * Show the available elliptic curves in the crypto library diff --git a/src/openvpn/ssl_mbedtls.c b/src/openvpn/ssl_mbedtls.c index 861d936d..09829ebb 100644 --- a/src/openvpn/ssl_mbedtls.c +++ b/src/openvpn/ssl_mbedtls.c @@ -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, "legacy")) + { + ctx->cert_profile = openvpn_x509_crt_profile_legacy; + } + else if (0 == strcmp(profile, "preferred")) + { + ctx->cert_profile = openvpn_x509_crt_profile_preferred; + } + 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); @@ -1271,12 +1322,14 @@ print_details(struct key_state_ssl *ks_ssl, const char *prefix) } void -show_available_tls_ciphers(const char *cipher_list) +show_available_tls_ciphers(const char *cipher_list, + const char *tls_cert_profile) { struct tls_root_ctx tls_ctx; const int *ciphers = mbedtls_ssl_list_ciphersuites(); tls_ctx_server_new(&tls_ctx); + tls_ctx_set_cert_profile(&tls_ctx, tls_cert_profile); tls_ctx_restrict_ciphers(&tls_ctx, cipher_list); if (tls_ctx.allowed_ciphers) diff --git a/src/openvpn/ssl_mbedtls.h b/src/openvpn/ssl_mbedtls.h index f69b6100..341da7d4 100644 --- a/src/openvpn/ssl_mbedtls.h +++ b/src/openvpn/ssl_mbedtls.h @@ -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 { diff --git a/src/openvpn/ssl_openssl.c b/src/openvpn/ssl_openssl.c index 0bfb6939..de89cb13 100644 --- a/src/openvpn/ssl_openssl.c +++ b/src/openvpn/ssl_openssl.c @@ -383,6 +383,40 @@ 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) + /* 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) + * behaviour. */ + if (!profile || 0 == strcmp(profile, "legacy")) + { + SSL_CTX_set_security_level(ctx->ctx, 1); + } + else if (0 == strcmp(profile, "preferred")) + { + SSL_CTX_set_security_level(ctx->ctx, 2); + } + else if (0 == strcmp(profile, "suiteb")) + { + SSL_CTX_set_security_level(ctx->ctx, 3); + SSL_CTX_set_cipher_list(ctx->ctx, "SUITEB128"); + } + else + { + msg(M_FATAL, "ERROR: Invalid cert profile: %s", profile); + } +#else + if (profile) + { + msg(M_WARN, "WARNING: OpenSSL 1.0.1 does not support --tls-cert-profile" + ", ignoring user-set profile: '%s'", profile); + } +#endif +} + void tls_ctx_check_cert_time(const struct tls_root_ctx *ctx) { @@ -1722,7 +1756,8 @@ print_details(struct key_state_ssl *ks_ssl, const char *prefix) } void -show_available_tls_ciphers(const char *cipher_list) +show_available_tls_ciphers(const char *cipher_list, + const char *tls_cert_profile) { struct tls_root_ctx tls_ctx; SSL *ssl; @@ -1742,6 +1777,7 @@ show_available_tls_ciphers(const char *cipher_list) crypto_msg(M_FATAL, "Cannot create SSL object"); } + tls_ctx_set_cert_profile(&tls_ctx, tls_cert_profile); tls_ctx_restrict_ciphers(&tls_ctx, cipher_list); printf("Available TLS Ciphers,\n");