Full_Name: Andreas Schulze Version: 2.4.45~git OS: linux URL: ftp://ftp.openldap.org/incoming/andreas-schulze-161125.patch Submission from: (NULL) (2001:a60:f0b4:e502::152) while http://www.openldap.org/its/index.cgi/Development?id=8353 end with "work with openssl-1.1" I checked out openldap from git in november 2016 and found it does not compile. So I write the patch. it solve the following challenges: - SSL_library_init no longer exist, use OPENSSL_init_ssl to detect openssl - configure use pkg-config to find the names of libssl and libcrypto - codechanges in libraries/libldap/tls_o.c to handle the API changes introduced by openssl-1.1.0
sca+openldap@andreasschulze.de wrote: > Full_Name: Andreas Schulze > Version: 2.4.45~git Patches should be relative to git master. There is no 2.4.45. > OS: linux > URL: ftp://ftp.openldap.org/incoming/andreas-schulze-161125.patch > Submission from: (NULL) (2001:a60:f0b4:e502::152) > > > while http://www.openldap.org/its/index.cgi/Development?id=8353 > end with "work with openssl-1.1" I checked out openldap from git in november > 2016 > and found it does not compile. So I write the patch. > > it solve the following challenges: > - SSL_library_init no longer exist, use OPENSSL_init_ssl to detect openssl > - configure use pkg-config to find the names of libssl and libcrypto Don't use pkg-config. Not all systems support it. > - codechanges in libraries/libldap/tls_o.c to handle the API changes introduced > by openssl-1.1.0 > > > > -- -- Howard Chu CTO, Symas Corp. http://www.symas.com Director, Highland Sun http://highlandsun.com/hyc/ Chief Architect, OpenLDAP http://www.openldap.org/project/
Howard Chu: > Patches should be relative to git master. There is no 2.4.45. right, I wrongly named the git master 2.4.45 - my fault but in fact it's git master... > Don't use pkg-config. Not all systems support it. I know only linux, sorry. Again: your right, support for openssl-1.1 don't require pkg-config. But I personally need pkg-config because my openssl library is named libssl-dv / libcrypto-dv to coexist with the distribution versions. Viktor Dukhovni told me to build openssl-1.1 that way and beside the requirement for pkg-config I'm personally fine with that advise :-) should I prepare the new patch to fit your needs? otherwise I would be fine if you modify the contribution to your needs. Anyway: after I wrote the patch for git-master it was very easy to create a patch to compile also 2.4.44 with openssl-1.1 I could publish that also next week. Do you think that makes sense? Thanks, Andreas
--On Saturday, November 26, 2016 10:50 PM +0000 sca@andreasschulze.de wrote: > But I personally need pkg-config because my openssl library is named > libssl-dv / libcrypto-dv > to coexist with the distribution versions. Viktor Dukhovni told me to > build openssl-1.1 that way > and beside the requirement for pkg-config I'm personally fine with > that advise :-) > > should I prepare the new patch to fit your needs? > otherwise I would be fine if you modify the contribution to your needs. > > Anyway: after I wrote the patch for git-master it was very easy to > create a patch to compile also 2.4.44 with openssl-1.1 > I could publish that also next week. Do you think that makes sense? Hi Andreas, Thanks for looking into this. I have a few additional comments: a) "configure.in" should be patched rather than "configure", and then I'd verify that regenerating configure works appropriately. b) A coding style issue found throughout the patch, where you are using NULL == variable rather than variable == NULL. This should be fixed. For example, you have: if ( NULL == tlso_bio_method ) { this instead should be: if ( tlso_bio_method == NULL ) { Thanks, Quanah -- Quanah Gibson-Mount Product Architect Symas Corporation Packaged, certified, and supported LDAP solutions powered by OpenLDAP: <http://www.symas.com>
Am 28.11.2016 um 17:42 schrieb Quanah Gibson-Mount: > I have a few additional comments: > ... I updated the patch as suggested: ftp://ftp.openldap.org/incoming/andreas-schulze-161204.patch Andreas
sca@andreasschulze.de wrote: > Am 28.11.2016 um 17:42 schrieb Quanah Gibson-Mount: >> I have a few additional comments: >> ... > > I updated the patch as suggested: ftp://ftp.openldap.org/incoming/andreas-schulze-161204.patch +/* REVIEW: CRYPTO_num_locks is defined as "(1)" openssl-1.1.0 */ +static ldap_pvt_thread_mutex_t tlso_mutexes[CRYPTO_num_locks()]; This is not acceptable. The fact that the macro is defined as a function macro implies that it may be turned into a genuine function at some point in the future. Either we get a commitment from the OpenSSL developers that this macro will always be a constant, or the code must change to dynamically allocate the mutexes array. @@ -882,6 +924,9 @@ static BIO_METHOD tlso_bio_method = tlso_bio_create, tlso_bio_destroy }; +#else +static BIO_METHOD *tlso_bio_method = NULL; +#endif static int tlso_sb_setup( Sockbuf_IO_Desc *sbiod, void *arg ) @@ -898,8 +943,29 @@ tlso_sb_setup( Sockbuf_IO_Desc *sbiod, void *arg ) p->session = arg; p->sbiod = sbiod; +#if OPENSSL_VERSION_NUMBER < 0x10100000L bio = BIO_new( &tlso_bio_method ); bio->ptr = (void *)p; +#else + if ( tlso_bio_method == NULL ) { + tlso_bio_method = BIO_meth_new(( 100 | 0x400 ), "sockbuf glue"); + if ( tlso_bio_method == NULL + || !BIO_meth_set_write(tlso_bio_method, tlso_bio_write) + || !BIO_meth_set_read(tlso_bio_method, tlso_bio_read) + || !BIO_meth_set_puts(tlso_bio_method, tlso_bio_puts) + || !BIO_meth_set_gets(tlso_bio_method, tlso_bio_gets) + || !BIO_meth_set_ctrl(tlso_bio_method, tlso_bio_ctrl) + || !BIO_meth_set_create(tlso_bio_method, tlso_bio_create) + || !BIO_meth_set_destroy(tlso_bio_method, tlso_bio_destroy)) { + return -1; + } + } + bio = BIO_new(tlso_bio_method); + if ( bio == NULL ) { + return -1; + } + BIO_set_data(bio, (void *)p); +#endif This is no good, the tlso_bio_method must be initialized once and only once. The structure can be referenced simultaneously from multiple threads, and initing on each use will be a performance hit as well as a waste of memory. -- -- Howard Chu CTO, Symas Corp. http://www.symas.com Director, Highland Sun http://highlandsun.com/hyc/ Chief Architect, OpenLDAP http://www.openldap.org/project/
Am 03.01.2017 um 20:12 schrieb Howard Chu: > sca@andreasschulze.de wrote: >> Am 28.11.2016 um 17:42 schrieb Quanah Gibson-Mount: >>> I have a few additional comments: >>> ... >> >> I updated the patch as suggested: ftp://ftp.openldap.org/incoming/andreas-schulze-161204.patch > > +/* REVIEW: CRYPTO_num_locks is defined as "(1)" openssl-1.1.0 */ > +static ldap_pvt_thread_mutex_t tlso_mutexes[CRYPTO_num_locks()]; > > This is not acceptable. The fact that the macro is defined as a function macro implies that it may be turned into a genuine function at some point in the future. Either we get a commitment from the OpenSSL developers that this macro will always be a constant, or the code must change to dynamically allocate the mutexes array. > > @@ -882,6 +924,9 @@ static BIO_METHOD tlso_bio_method = > tlso_bio_create, > tlso_bio_destroy > }; > +#else > +static BIO_METHOD *tlso_bio_method = NULL; > +#endif > > static int > tlso_sb_setup( Sockbuf_IO_Desc *sbiod, void *arg ) > @@ -898,8 +943,29 @@ tlso_sb_setup( Sockbuf_IO_Desc *sbiod, void *arg ) > > p->session = arg; > p->sbiod = sbiod; > +#if OPENSSL_VERSION_NUMBER < 0x10100000L > bio = BIO_new( &tlso_bio_method ); > bio->ptr = (void *)p; > +#else > + if ( tlso_bio_method == NULL ) { > + tlso_bio_method = BIO_meth_new(( 100 | 0x400 ), "sockbuf glue"); > + if ( tlso_bio_method == NULL > + || !BIO_meth_set_write(tlso_bio_method, tlso_bio_write) > + || !BIO_meth_set_read(tlso_bio_method, tlso_bio_read) > + || !BIO_meth_set_puts(tlso_bio_method, tlso_bio_puts) > + || !BIO_meth_set_gets(tlso_bio_method, tlso_bio_gets) > + || !BIO_meth_set_ctrl(tlso_bio_method, tlso_bio_ctrl) > + || !BIO_meth_set_create(tlso_bio_method, tlso_bio_create) > + || !BIO_meth_set_destroy(tlso_bio_method, tlso_bio_destroy)) { > + return -1; > + } > + } > + bio = BIO_new(tlso_bio_method); > + if ( bio == NULL ) { > + return -1; > + } > + BIO_set_data(bio, (void *)p); > +#endif > > This is no good, the tlso_bio_method must be initialized once and only once. The structure can be referenced simultaneously from multiple threads, and initing on each use will be a performance hit as well as a waste of memory. Hello Howard, looks like you're much more familiar with open[ssl|ldap] insides. My intension was to show a way to build openldap with openssl-1.1.0. I'm aware that is not ideal and you prove that now. Thanks. But I cannot contribute better code. Sorry. Andreas
changed notes changed state Open to Release moved from Incoming to Software Enhancements
changed notes moved from Software Enhancements to Development
changed notes
If openssl 1.1.0 is built with the option "no-deprecated" the build will fail, as portions of the code still use the pre 1.1 API. This needs fixing before release. --Quanah -- Quanah Gibson-Mount Product Architect Symas Corporation Packaged, certified, and supported LDAP solutions powered by OpenLDAP: <http://www.symas.com>
--On Wednesday, April 05, 2017 7:58 PM +0000 quanah@symas.com wrote: > If openssl 1.1.0 is built with the option "no-deprecated" the build will > fail, as portions of the code still use the pre 1.1 API. This needs > fixing before release. The following 5 function calls are problematic: ./.libs/libldap.so: undefined reference to `OpenSSL_add_all_digests' ./.libs/libldap.so: undefined reference to `SSL_load_error_strings' ./.libs/libldap.so: undefined reference to `ERR_free_strings' ./.libs/libldap.so: undefined reference to `EVP_cleanup' ./.libs/libldap.so: undefined reference to `SSL_library_init' Looking at the best way in which to fix. --Quanah -- Quanah Gibson-Mount Product Architect Symas Corporation Packaged, certified, and supported LDAP solutions powered by OpenLDAP: <http://www.symas.com>
--On Wednesday, April 05, 2017 10:38 PM +0000 quanah@symas.com wrote: > --On Wednesday, April 05, 2017 7:58 PM +0000 quanah@symas.com wrote: > >> If openssl 1.1.0 is built with the option "no-deprecated" the build will >> fail, as portions of the code still use the pre 1.1 API. This needs >> fixing before release. > > The following 5 function calls are problematic: > > ./.libs/libldap.so: undefined reference to `OpenSSL_add_all_digests' > ./.libs/libldap.so: undefined reference to `SSL_load_error_strings' > ./.libs/libldap.so: undefined reference to `ERR_free_strings' > ./.libs/libldap.so: undefined reference to `EVP_cleanup' > ./.libs/libldap.so: undefined reference to `SSL_library_init' > > > Looking at the best way in which to fix. These also need fixing, as they don't exist in 1.1 when the old API is disabled. CRYPTO_num_locks() CRYPTO_LOCK CRYPTO_set_locking_callback() CRYPTO_set_id_callback() --Quanah -- Quanah Gibson-Mount Product Architect Symas Corporation Packaged, certified, and supported LDAP solutions powered by OpenLDAP: <http://www.symas.com>
--On Wednesday, April 05, 2017 11:12 PM +0000 quanah@symas.com wrote: >> The following 5 function calls are problematic: >> >> ./.libs/libldap.so: undefined reference to `OpenSSL_add_all_digests' >> ./.libs/libldap.so: undefined reference to `SSL_load_error_strings' >> ./.libs/libldap.so: undefined reference to `ERR_free_strings' >> ./.libs/libldap.so: undefined reference to `EVP_cleanup' >> ./.libs/libldap.so: undefined reference to `SSL_library_init' The above can be fixed with the following patch: build@u12build:~/git/symas-packages/thirdparty/openldap/build/UBUNTU12_64/symas-openldap/libraries/libldap$ diff -u tls_o.c.orig tls_o.c --- tls_o.c.orig 2017-04-05 14:40:02.849559862 -0700 +++ tls_o.c 2017-04-05 15:13:09.371718493 -0700 @@ -154,9 +154,16 @@ (void) tlso_seed_PRNG( lo->ldo_tls_randfile ); #endif +#if OPENSSL_VERSION_NUMBER < 0x10100000 SSL_load_error_strings(); SSL_library_init(); OpenSSL_add_all_digests(); +#else + OPENSSL_init_ssl(OPENSSL_INIT_LOAD_SSL_STRINGS \ + | OPENSSL_INIT_LOAD_CRYPTO_STRINGS, NULL); + OPENSSL_init_ssl(0, NULL); + OPENSSL_init_crypto(OPENSSL_INIT_ADD_ALL_DIGESTS, NULL); +#endif /* FIXME: mod_ssl does this */ X509V3_add_standard_extensions(); @@ -172,6 +179,7 @@ { struct ldapoptions *lo = LDAP_INT_GLOBAL_OPT(); +#if OPENSSL_VERSION_NUMBER < 0x10100000 EVP_cleanup(); #if OPENSSL_VERSION_NUMBER < 0x10000000 ERR_remove_state(0); @@ -179,6 +187,9 @@ ERR_remove_thread_state(NULL); #endif ERR_free_strings(); +#else + ERR_remove_thread_state(NULL); +#endif if ( lo->ldo_tls_randfile ) { LDAP_FREE( lo->ldo_tls_randfile ); --Quanah -- Quanah Gibson-Mount Product Architect Symas Corporation Packaged, certified, and supported LDAP solutions powered by OpenLDAP: <http://www.symas.com>
quanah@symas.com wrote: > --On Wednesday, April 05, 2017 10:38 PM +0000 quanah@symas.com wrote: > >> --On Wednesday, April 05, 2017 7:58 PM +0000 quanah@symas.com wrote: >> >>> If openssl 1.1.0 is built with the option "no-deprecated" the build will >>> fail, as portions of the code still use the pre 1.1 API. This needs >>> fixing before release. >> >> The following 5 function calls are problematic: >> >> ./.libs/libldap.so: undefined reference to `OpenSSL_add_all_digests' >> ./.libs/libldap.so: undefined reference to `SSL_load_error_strings' >> ./.libs/libldap.so: undefined reference to `ERR_free_strings' >> ./.libs/libldap.so: undefined reference to `EVP_cleanup' >> ./.libs/libldap.so: undefined reference to `SSL_library_init' >> >> >> Looking at the best way in which to fix. > > These also need fixing, as they don't exist in 1.1 when the old API is > disabled. > > CRYPTO_num_locks() > CRYPTO_LOCK > CRYPTO_set_locking_callback() > CRYPTO_set_id_callback() These only exist because OpenSSL's support for threading was mostly nonexistent in the past. They should just be #ifdef'd away for 1.1. -- -- Howard Chu CTO, Symas Corp. http://www.symas.com Director, Highland Sun http://highlandsun.com/hyc/ Chief Architect, OpenLDAP http://www.openldap.org/project/
quanah@symas.com wrote: > --On Wednesday, April 05, 2017 11:12 PM +0000 quanah@symas.com wrote: > >>> The following 5 function calls are problematic: >>> >>> ./.libs/libldap.so: undefined reference to `OpenSSL_add_all_digests' >>> ./.libs/libldap.so: undefined reference to `SSL_load_error_strings' >>> ./.libs/libldap.so: undefined reference to `ERR_free_strings' >>> ./.libs/libldap.so: undefined reference to `EVP_cleanup' >>> ./.libs/libldap.so: undefined reference to `SSL_library_init' > > The above can be fixed with the following patch: > > build@u12build:~/git/symas-packages/thirdparty/openldap/build/UBUNTU12_64/symas-openldap/libraries/libldap$ > diff -u tls_o.c.orig tls_o.c > --- tls_o.c.orig 2017-04-05 14:40:02.849559862 -0700 > +++ tls_o.c 2017-04-05 15:13:09.371718493 -0700 > @@ -154,9 +154,16 @@ > (void) tlso_seed_PRNG( lo->ldo_tls_randfile ); > #endif > > +#if OPENSSL_VERSION_NUMBER < 0x10100000 Are you sure about exactly when these functions were removed? The title of this ITS is for 1.1.0c, and the earlier patches were for 1.1.0a. For version 1.1.0c we should be comparing to 0x1010003f, not 0x10100000. > SSL_load_error_strings(); > SSL_library_init(); > OpenSSL_add_all_digests(); > +#else > + OPENSSL_init_ssl(OPENSSL_INIT_LOAD_SSL_STRINGS \ > + | OPENSSL_INIT_LOAD_CRYPTO_STRINGS, NULL); > + OPENSSL_init_ssl(0, NULL); > + OPENSSL_init_crypto(OPENSSL_INIT_ADD_ALL_DIGESTS, NULL); > +#endif > > /* FIXME: mod_ssl does this */ > X509V3_add_standard_extensions(); > @@ -172,6 +179,7 @@ > { > struct ldapoptions *lo = LDAP_INT_GLOBAL_OPT(); > > +#if OPENSSL_VERSION_NUMBER < 0x10100000 > EVP_cleanup(); > #if OPENSSL_VERSION_NUMBER < 0x10000000 > ERR_remove_state(0); > @@ -179,6 +187,9 @@ > ERR_remove_thread_state(NULL); > #endif > ERR_free_strings(); > +#else > + ERR_remove_thread_state(NULL); > +#endif > > if ( lo->ldo_tls_randfile ) { > LDAP_FREE( lo->ldo_tls_randfile ); > > > --Quanah > > > -- > > Quanah Gibson-Mount > Product Architect > Symas Corporation > Packaged, certified, and supported LDAP solutions powered by OpenLDAP: > <http://www.symas.com> > > > > > -- -- Howard Chu CTO, Symas Corp. http://www.symas.com Director, Highland Sun http://highlandsun.com/hyc/ Chief Architect, OpenLDAP http://www.openldap.org/project/
--On Thursday, April 06, 2017 2:02 AM +0100 Howard Chu <hyc@symas.com> wrote: >> +#if OPENSSL_VERSION_NUMBER < 0x10100000 > > Are you sure about exactly when these functions were removed? The title > of this ITS is for 1.1.0c, and the earlier patches were for 1.1.0a. For > version 1.1.0c we should be comparing to 0x1010003f, not 0x10100000. Yes, I checked against OpenSSL 1.1.0 (released before 1.1.0a). The OpenSSL shipped headers are clear on this as well, for example: #if OPENSSL_API_COMPAT < 0x10100000L # define SSL_library_init() OPENSSL_init_ssl(0, NULL) #endif #if OPENSSL_API_COMPAT < 0x10100000L # define SSL_load_error_strings() \ OPENSSL_init_ssl(OPENSSL_INIT_LOAD_SSL_STRINGS \ | OPENSSL_INIT_LOAD_CRYPTO_STRINGS, NULL) #endif etc -- Quanah Gibson-Mount Product Architect Symas Corporation Packaged, certified, and supported LDAP solutions powered by OpenLDAP: <http://www.symas.com>
Added in master Added in RE25 Added in RE24 (2.4.45) See also ITS8353
changed notes changed state Release to Closed