| Message ID | 20190403225657.8003-1-rosenp@gmail.com |
|---|---|
| State | Superseded |
| Delegated to: | Arne Schwabe |
| Headers | show |
| Series | [Openvpn-devel,PATCHv2] openssl: Fix compilation without deprecated OpenSSL 1.1 APIs | expand |
Am 04.04.19 um 00:56 schrieb Rosen Penev: > EVP_CIPHER_CTX_init and _cleanup were deprecated in 1.1 and both were > replaced with _reset. > > 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. > +#if !defined(HAVE_EVP_CIPHER_CTX_INIT) > +#define EVP_CIPHER_CTX_init EVP_CIPHER_CTX_reset > +#endif > + > +#if !defined(HAVE_EVP_CIPHER_CTX_CLEANUP) > +#define EVP_CIPHER_CTX_cleanup EVP_CIPHER_CTX_reset > +#endif > + Generally we tried to avoid things like implementing the old API with the new API but instead try to only use the current OpenSSL API and implement it using the older API on older OpenSSL version. Is there way for these functions to do that as well? Arne
Am 04.04.19 um 00:56 schrieb Rosen Penev: > EVP_CIPHER_CTX_init and _cleanup were deprecated in 1.1 and both were > replaced with _reset. > > 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. > This gets kind of an ACK but needs some additional changes to be really good. > > +#if !defined(HAVE_EVP_CIPHER_CTX_INIT) > +#define EVP_CIPHER_CTX_init EVP_CIPHER_CTX_reset > +#endif > + > +#if !defined(HAVE_EVP_CIPHER_CTX_CLEANUP) > +#define EVP_CIPHER_CTX_cleanup EVP_CIPHER_CTX_reset > +#endif These two keep the older API instead of switching to the new one, from OpenSSL. # if OPENSSL_API_COMPAT < 0x10100000L # define EVP_CIPHER_CTX_init(c) EVP_CIPHER_CTX_reset(c) # define EVP_CIPHER_CTX_cleanup(c) EVP_CIPHER_CTX_reset(c) # endif Since just using only the new API in this case does not really work I think in case it would be better to rather always use EVP_CIPHER_CTX_reset isntead of init and have ifdefs in the 2-3 places where we actually use EVP_CIPHER_CTX_cleanup so we can remove the old API when we bump our minimum OpenSSL version (and find this thing easy since it is an ifdef depending on the openssl version). > + > +#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 The space between # and ifndef looks wrong. Arne
Am 14.06.19 um 12:38 schrieb Arne Schwabe: > >> -#ifndef ENABLE_SMALL >> +# ifndef ENABLE_SMALL > The space between # and ifndef looks wrong. It's standard C. (Chapter 3.8 in the 1989/1990 standard, chapter 6.10 in recent editions, I checked 1999 and 2017/2018, although worded in a quite convoluted way) and I have yet to see a relevant C compiler that would reject this. The # is a token by itself can can be separated from the other preprocessing tokens on the same line by space and horizontal-tab.
Am 12.07.19 um 18:37 schrieb Matthias Andree: > Am 14.06.19 um 12:38 schrieb Arne Schwabe: >> >>> -#ifndef ENABLE_SMALL >>> +# ifndef ENABLE_SMALL >> The space between # and ifndef looks wrong. > > It's standard C. (Chapter 3.8 in the 1989/1990 standard, chapter 6.10 in > recent editions, I checked 1999 and 2017/2018, although worded in a > quite convoluted way) and I have yet to see a relevant C compiler that > would reject this. I know :). Looks "wrong" in the sense that it does not match the style we want to have in openvpn.
Hi, On Fri, Jul 12, 2019 at 06:44:06PM +0200, Arne Schwabe wrote: > Looks "wrong" in the sense that it does not match the style we want to > have in openvpn. Indeed. I have seen and used "# cpp-thing" in other projects, but OpenVPN seems to have decided to not use space-indenting for nested #if-things. So, we'll keep this (and try to get rid of #ifdefs anyway :-) ). gert
On Fri, Jun 14, 2019 at 3:38 AM Arne Schwabe <arne@rfc2549.org> wrote: > > Am 04.04.19 um 00:56 schrieb Rosen Penev: > > EVP_CIPHER_CTX_init and _cleanup were deprecated in 1.1 and both were > > replaced with _reset. > > > > 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. > > > > This gets kind of an ACK but needs some additional changes to be really > good. > > > > > > +#if !defined(HAVE_EVP_CIPHER_CTX_INIT) > > +#define EVP_CIPHER_CTX_init EVP_CIPHER_CTX_reset > > +#endif > > + > > +#if !defined(HAVE_EVP_CIPHER_CTX_CLEANUP) > > +#define EVP_CIPHER_CTX_cleanup EVP_CIPHER_CTX_reset > > +#endif > > These two keep the older API instead of switching to the new one, from > OpenSSL. Yes I know. I feel that it's a cleaner solution as _init and _cleanup both get defined to _reset. > > # if OPENSSL_API_COMPAT < 0x10100000L > # define EVP_CIPHER_CTX_init(c) EVP_CIPHER_CTX_reset(c) > # define EVP_CIPHER_CTX_cleanup(c) EVP_CIPHER_CTX_reset(c) > # endif > > Since just using only the new API in this case does not really work I > think in case it would be better to rather always use > EVP_CIPHER_CTX_reset isntead of init and have ifdefs in the 2-3 places > where we actually use EVP_CIPHER_CTX_cleanup so we can remove the old > API when we bump our minimum OpenSSL version (and find this thing easy > since it is an ifdef depending on the openssl version). OK. Will change. > > > + > > +#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 > > The space between # and ifndef looks wrong. Will eliminate. Not sure exactly why I added the space ATM. > > > Arne >
Hi all, On 14-06-19 12:38, Arne Schwabe wrote: >> +#if !defined(HAVE_EVP_CIPHER_CTX_INIT) >> +#define EVP_CIPHER_CTX_init EVP_CIPHER_CTX_reset >> +#endif >> + >> +#if !defined(HAVE_EVP_CIPHER_CTX_CLEANUP) >> +#define EVP_CIPHER_CTX_cleanup EVP_CIPHER_CTX_reset >> +#endif > > These two keep the older API instead of switching to the new one, from > OpenSSL. > > # if OPENSSL_API_COMPAT < 0x10100000L > # define EVP_CIPHER_CTX_init(c) EVP_CIPHER_CTX_reset(c) > # define EVP_CIPHER_CTX_cleanup(c) EVP_CIPHER_CTX_reset(c) > # endif > > Since just using only the new API in this case does not really work I > think in case it would be better to rather always use > EVP_CIPHER_CTX_reset isntead of init and have ifdefs in the 2-3 places > where we actually use EVP_CIPHER_CTX_cleanup so we can remove the old > API when we bump our minimum OpenSSL version (and find this thing easy > since it is an ifdef depending on the openssl version). Why wouldn't using the new API work? _reset() is basically the new name for _cleanup(), which some some actual cleanup + init. As far as I can see it would work perfectly fine to use _reset() instead of the _init/_cleanup calls everywhere. We never call _init on uninitialized memory (which is the only case where _init() would work while _cleanup() would fail). All we'd have to do than is add something like #ifndef HAVE_EVP_CIPHER_CTX_RESET #define EVP_CIPHER_CTX_reset EVP_CIPHER_CTX_cleanup #endif to openssl_compat.h. That way we would just use the new API everywhere, and can get rid of the lines in the compat file once we drop support for OpenSSL 1.0. Or am I missing something here? -Steffan
Am 24.07.19 um 12:28 schrieb Steffan Karger: > Hi all, > > On 14-06-19 12:38, Arne Schwabe wrote: >>> +#if !defined(HAVE_EVP_CIPHER_CTX_INIT) >>> +#define EVP_CIPHER_CTX_init EVP_CIPHER_CTX_reset >>> +#endif >>> + >>> +#if !defined(HAVE_EVP_CIPHER_CTX_CLEANUP) >>> +#define EVP_CIPHER_CTX_cleanup EVP_CIPHER_CTX_reset >>> +#endif >> >> These two keep the older API instead of switching to the new one, from >> OpenSSL. >> >> # if OPENSSL_API_COMPAT < 0x10100000L >> # define EVP_CIPHER_CTX_init(c) EVP_CIPHER_CTX_reset(c) >> # define EVP_CIPHER_CTX_cleanup(c) EVP_CIPHER_CTX_reset(c) >> # endif >> >> Since just using only the new API in this case does not really work I >> think in case it would be better to rather always use >> EVP_CIPHER_CTX_reset isntead of init and have ifdefs in the 2-3 places >> where we actually use EVP_CIPHER_CTX_cleanup so we can remove the old >> API when we bump our minimum OpenSSL version (and find this thing easy >> since it is an ifdef depending on the openssl version). Okay, I double checked this. The new API drops EVP_CIPHER_CTX_cleanup, so we have do either ifdef the EVP_CIPHER_CTX_cleanup calls or remove them completely. Currently we only call the EVP_CIPHER_CTX_cleanup directly before EVP_CIPHER_CTX_free. Looking at the 1.0.2 openssl code: void EVP_CIPHER_CTX_free(EVP_CIPHER_CTX *ctx) { if (ctx) { EVP_CIPHER_CTX_cleanup(ctx); OPENSSL_free(ctx); } } it looks like the EVP_CIPHER_CTX_cleanup is implicit already, so we can remove those call and drop the cipher_ctx_cleanup function and combine cipher_ctx_cleanup and _free into one for mbed TLS. Does that sound reasonble? Arne
diff --git a/configure.ac b/configure.ac index dfb268ca..891799ea 100644 --- a/configure.ac +++ b/configure.ac @@ -918,10 +918,14 @@ if test "${with_crypto_library}" = "openssl"; then EVP_MD_CTX_new \ EVP_MD_CTX_free \ EVP_MD_CTX_reset \ + EVP_CIPHER_CTX_init \ + EVP_CIPHER_CTX_cleanup \ OpenSSL_version \ 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..2453b85e 100644 --- a/src/openvpn/openssl_compat.h +++ b/src/openvpn/openssl_compat.h @@ -89,6 +89,22 @@ EVP_MD_CTX_new(void) } #endif +#if !defined(HAVE_EVP_CIPHER_CTX_INIT) +#define EVP_CIPHER_CTX_init EVP_CIPHER_CTX_reset +#endif + +#if !defined(HAVE_EVP_CIPHER_CTX_CLEANUP) +#define EVP_CIPHER_CTX_cleanup EVP_CIPHER_CTX_reset +#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;
EVP_CIPHER_CTX_init and _cleanup were deprecated in 1.1 and both were replaced with _reset. 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> --- v2: Squashed previous patches together. configure.ac | 4 ++++ src/openvpn/openssl_compat.h | 16 ++++++++++++++++ src/openvpn/ssl_openssl.c | 18 ++++++++++++------ 3 files changed, 32 insertions(+), 6 deletions(-)