[Openvpn-devel] openssl: Replace not[Before/After] functions with get0 variants
Commit Message
Also removed initialization with OpenSSL 1.1 as it is no longer needed and
causes compilation errors when disabling deprecated APIs.
Same with SSL_CTX_set_ecdh_auto as it got removed.
Signed-off-by: Rosen Penev <rosenp@gmail.com>
---
configure.ac | 2 ++
src/openvpn/openssl_compat.h | 8 ++++++++
src/openvpn/ssl_openssl.c | 18 ++++++++++++------
3 files changed, 22 insertions(+), 6 deletions(-)
Comments
Hi,
On Wed, Mar 27, 2019 at 02:56:26PM -0700, Rosen Penev wrote:
> Also removed initialization with OpenSSL 1.1 as it is no longer needed and
> causes compilation errors when disabling deprecated APIs.
>
> Same with SSL_CTX_set_ecdh_auto as it got removed.
The Subject: line is a bit misleading - shouldn't this be more something
like "Avoid calling deprecated-API calls of OpenSSL 1.1" or similar?
(People *do* look at commit logs, and the current subject does not cover
making the SSL_library_init() call conditional, etc.)
On the patch itself, I have no strong opinion and would welcome a review
from Steffan or Arne :-) - the Subject: line I can fix at commit time.
gert
> diff --git a/configure.ac b/configure.ac
> index dfb268ca..2617f344 100644
> --- a/configure.ac
> +++ b/configure.ac
> @@ -922,6 +922,8 @@ if test "${with_crypto_library}" = "openssl"; then
> SSL_CTX_get_default_passwd_cb \
> SSL_CTX_get_default_passwd_cb_userdata \
> SSL_CTX_set_security_level \
> + X509_get0_notBefore \
> + X509_get0_notAfter \
> X509_get0_pubkey \
> X509_STORE_get0_objects \
> X509_OBJECT_free \
> diff --git a/src/openvpn/openssl_compat.h b/src/openvpn/openssl_compat.h
> index a4072b9a..788843a2 100644
> --- a/src/openvpn/openssl_compat.h
> +++ b/src/openvpn/openssl_compat.h
> @@ -89,6 +89,14 @@ EVP_MD_CTX_new(void)
> }
> #endif
>
> +#if !defined(HAVE_X509_GET0_NOTBEFORE)
> +#define X509_get0_notBefore X509_get_notBefore
> +#endif
> +
> +#if !defined(HAVE_X509_GET0_NOTAFTER)
> +#define X509_get0_notAfter X509_get_notAfter
> +#endif
> +
This is fine.
> #if !defined(HAVE_HMAC_CTX_RESET)
> /**
> * Reset a HMAC context
> diff --git a/src/openvpn/ssl_openssl.c b/src/openvpn/ssl_openssl.c
> index 8bcebac4..e41cafa5 100644
> --- a/src/openvpn/ssl_openssl.c
> +++ b/src/openvpn/ssl_openssl.c
> @@ -76,12 +76,13 @@ int mydata_index; /* GLOBAL */
> void
> tls_init_lib(void)
> {
> +#if (OPENSSL_VERSION_NUMBER < 0x10100000L && !defined(LIBRESSL_VERSION_NUMBER))
> SSL_library_init();
> -#ifndef ENABLE_SMALL
> +# ifndef ENABLE_SMALL
> SSL_load_error_strings();
> -#endif
> +# endif
> OpenSSL_add_all_algorithms();
> -
> +#endif
Please add a comment like
/* On OpenSSL 1.1.0 or above, then the library will initialize itself
automatically. */
Otherwise people will be very confused why this code is that way.
> mydata_index = SSL_get_ex_new_index(0, "struct session *", NULL, NULL, NULL);
> ASSERT(mydata_index >= 0);
> }
> @@ -89,9 +90,11 @@ tls_init_lib(void)
> void
> tls_free_lib(void)
> {
> +#if (OPENSSL_VERSION_NUMBER < 0x10100000L && !defined(LIBRESSL_VERSION_NUMBER))
> EVP_cleanup();
> -#ifndef ENABLE_SMALL
> +# ifndef ENABLE_SMALL
> ERR_free_strings();
> +# endif
> #endif
> }
Same as above.
>
> @@ -541,7 +544,7 @@ tls_ctx_check_cert_time(const struct tls_root_ctx *ctx)
> goto cleanup; /* Nothing to check if there is no certificate */
> }
>
> - ret = X509_cmp_time(X509_get_notBefore(cert), NULL);
> + ret = X509_cmp_time(X509_get0_notBefore(cert), NULL);
> if (ret == 0)
> {
> msg(D_TLS_DEBUG_MED, "Failed to read certificate notBefore field.");
> @@ -551,7 +554,7 @@ tls_ctx_check_cert_time(const struct tls_root_ctx *ctx)
> msg(M_WARN, "WARNING: Your certificate is not yet valid!");
> }
>
> - ret = X509_cmp_time(X509_get_notAfter(cert), NULL);
> + ret = X509_cmp_time(X509_get0_notAfter(cert), NULL);
> if (ret == 0)
> {
> msg(D_TLS_DEBUG_MED, "Failed to read certificate notAfter field.");
> @@ -634,10 +637,13 @@ tls_ctx_load_ecdh_params(struct tls_root_ctx *ctx, const char *curve_name
> else
> {
> #if OPENSSL_VERSION_NUMBER >= 0x10002000L
> +#if (OPENSSL_VERSION_NUMBER < 0x10100000L && !defined(LIBRESSL_VERSION_NUMBER))
> +
> /* OpenSSL 1.0.2 and newer can automatically handle ECDH parameter
> * loading */
> SSL_CTX_set_ecdh_auto(ctx->ctx, 1);
> return;
> +#endif
> #else
> /* For older OpenSSL we have to extract the curve from key on our own */
> EC_KEY *eckey = NULL;
>
In general it be better split this patch into two: renaming the get/set
methods and removing the initialisation from OpenSSL >=1.1.0
Arne
On Thu, Mar 28, 2019 at 12:51 AM Gert Doering <gert@greenie.muc.de> wrote:
>
> Hi,
>
> On Wed, Mar 27, 2019 at 02:56:26PM -0700, Rosen Penev wrote:
> > Also removed initialization with OpenSSL 1.1 as it is no longer needed and
> > causes compilation errors when disabling deprecated APIs.
> >
> > Same with SSL_CTX_set_ecdh_auto as it got removed.
>
> The Subject: line is a bit misleading - shouldn't this be more something
> like "Avoid calling deprecated-API calls of OpenSSL 1.1" or similar?
Yeah. The patch doesn't fully fix it though. I ended up with a
somewhat workable solution though.
https://github.com/neheb/openvpn/commit/945f190a1bfbde3d6bf11f5b576f5c9e5ec1b0f3
I can squash both of these so that they get the same Subject line.
>
> (People *do* look at commit logs, and the current subject does not cover
> making the SSL_library_init() call conditional, etc.)
>
> On the patch itself, I have no strong opinion and would welcome a review
> from Steffan or Arne :-) - the Subject: line I can fix at commit time.
>
> gert
> --
> "If was one thing all people took for granted, was conviction that if you
> feed honest figures into a computer, honest figures come out. Never doubted
> it myself till I met a computer with a sense of humor."
> Robert A. Heinlein, The Moon is a Harsh Mistress
>
> Gert Doering - Munich, Germany gert@greenie.muc.de
@@ -922,6 +922,8 @@ if test "${with_crypto_library}" = "openssl"; then
SSL_CTX_get_default_passwd_cb \
SSL_CTX_get_default_passwd_cb_userdata \
SSL_CTX_set_security_level \
+ X509_get0_notBefore \
+ X509_get0_notAfter \
X509_get0_pubkey \
X509_STORE_get0_objects \
X509_OBJECT_free \
@@ -89,6 +89,14 @@ EVP_MD_CTX_new(void)
}
#endif
+#if !defined(HAVE_X509_GET0_NOTBEFORE)
+#define X509_get0_notBefore X509_get_notBefore
+#endif
+
+#if !defined(HAVE_X509_GET0_NOTAFTER)
+#define X509_get0_notAfter X509_get_notAfter
+#endif
+
#if !defined(HAVE_HMAC_CTX_RESET)
/**
* Reset a HMAC context
@@ -76,12 +76,13 @@ int mydata_index; /* GLOBAL */
void
tls_init_lib(void)
{
+#if (OPENSSL_VERSION_NUMBER < 0x10100000L && !defined(LIBRESSL_VERSION_NUMBER))
SSL_library_init();
-#ifndef ENABLE_SMALL
+# ifndef ENABLE_SMALL
SSL_load_error_strings();
-#endif
+# endif
OpenSSL_add_all_algorithms();
-
+#endif
mydata_index = SSL_get_ex_new_index(0, "struct session *", NULL, NULL, NULL);
ASSERT(mydata_index >= 0);
}
@@ -89,9 +90,11 @@ tls_init_lib(void)
void
tls_free_lib(void)
{
+#if (OPENSSL_VERSION_NUMBER < 0x10100000L && !defined(LIBRESSL_VERSION_NUMBER))
EVP_cleanup();
-#ifndef ENABLE_SMALL
+# ifndef ENABLE_SMALL
ERR_free_strings();
+# endif
#endif
}
@@ -541,7 +544,7 @@ tls_ctx_check_cert_time(const struct tls_root_ctx *ctx)
goto cleanup; /* Nothing to check if there is no certificate */
}
- ret = X509_cmp_time(X509_get_notBefore(cert), NULL);
+ ret = X509_cmp_time(X509_get0_notBefore(cert), NULL);
if (ret == 0)
{
msg(D_TLS_DEBUG_MED, "Failed to read certificate notBefore field.");
@@ -551,7 +554,7 @@ tls_ctx_check_cert_time(const struct tls_root_ctx *ctx)
msg(M_WARN, "WARNING: Your certificate is not yet valid!");
}
- ret = X509_cmp_time(X509_get_notAfter(cert), NULL);
+ ret = X509_cmp_time(X509_get0_notAfter(cert), NULL);
if (ret == 0)
{
msg(D_TLS_DEBUG_MED, "Failed to read certificate notAfter field.");
@@ -634,10 +637,13 @@ tls_ctx_load_ecdh_params(struct tls_root_ctx *ctx, const char *curve_name
else
{
#if OPENSSL_VERSION_NUMBER >= 0x10002000L
+#if (OPENSSL_VERSION_NUMBER < 0x10100000L && !defined(LIBRESSL_VERSION_NUMBER))
+
/* OpenSSL 1.0.2 and newer can automatically handle ECDH parameter
* loading */
SSL_CTX_set_ecdh_auto(ctx->ctx, 1);
return;
+#endif
#else
/* For older OpenSSL we have to extract the curve from key on our own */
EC_KEY *eckey = NULL;