| Message ID | 20190327215626.23502-1-rosenp@gmail.com |
|---|---|
| State | Changes Requested |
| Delegated to: | Arne Schwabe |
| Headers | show |
| Series | [Openvpn-devel] openssl: Replace not[Before/After] functions with get0 variants | expand |
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
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 + #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 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;
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(-)