[ZBX-11149] Zabbix does not compile with OpenSSL 1.1.0 Created: 2016 Aug 31 Updated: 2017 May 30 Resolved: 2016 Nov 14 |
|
Status: | Closed |
Project: | ZABBIX BUGS AND ISSUES |
Component/s: | Agent (G), Installation (I), Proxy (P), Server (S) |
Affects Version/s: | 3.0.4 |
Fix Version/s: | 3.0.6rc1, 3.2.2rc1, 3.4.0alpha1 |
Type: | Incident report | Priority: | Major |
Reporter: | Aleksandrs Saveljevs | Assignee: | Unassigned |
Resolution: | Fixed | Votes: | 0 |
Labels: | encryption, openssl | ||
Remaining Estimate: | Not Specified | ||
Time Spent: | Not Specified | ||
Original Estimate: | Not Specified |
Description |
A new major version of OpenSSL was recently released: see https://www.openssl.org/news/openssl-1.1.0-notes.html . Unfortunately, trying to compile Zabbix with OpenSSL 1.1.0 fails with the following errors: $ make > /dev/null tls.c:1608:34: error: incomplete definition of type 'struct ssl_ctx_st' num = sk_SSL_CIPHER_num(ciphers->cipher_list); ~~~~~~~^ /home/asaveljevs/software/openssl-1.1.0-install/include/openssl/ossl_typ.h:145:16: note: forward declaration of 'struct ssl_ctx_st' typedef struct ssl_ctx_st SSL_CTX; ^ tls.c:1613:33: error: incomplete definition of type 'struct ssl_ctx_st' sk_SSL_CIPHER_value(ciphers->cipher_list, i)->name); ~~~~~~~^ ../../../include/common.h:934:85: note: expanded from macro 'zbx_snprintf_alloc' __zbx_zbx_snprintf_alloc(str, alloc_len, offset, ZBX_CONST_STRING(fmt), ##__VA_ARGS__) ^ /home/asaveljevs/software/openssl-1.1.0-install/include/openssl/ossl_typ.h:145:16: note: forward declaration of 'struct ssl_ctx_st' typedef struct ssl_ctx_st SSL_CTX; ^ tls.c:3095:12: warning: 'TLSv1_2_client_method' is deprecated [-Wdeprecated-declarations] method = TLSv1_2_client_method(); ^ /home/asaveljevs/software/openssl-1.1.0-install/include/openssl/ssl.h:1610:45: note: 'TLSv1_2_client_method' has been explicitly marked deprecated here DEPRECATEDIN_1_1_0(__owur const SSL_METHOD *TLSv1_2_client_method(void)) /* TLSv1.2 */ ^ /home/asaveljevs/software/openssl-1.1.0-install/include/openssl/opensslconf.h:127:53: note: expanded from macro 'DEPRECATEDIN_1_1_0' # define DEPRECATEDIN_1_1_0(f) DECLARE_DEPRECATED(f) ^ /home/asaveljevs/software/openssl-1.1.0-install/include/openssl/opensslconf.h:102:35: note: expanded from macro 'DECLARE_DEPRECATED' # define DECLARE_DEPRECATED(f) f __attribute__ ((deprecated)); ^ tls.c:3097:12: warning: 'TLSv1_2_method' is deprecated [-Wdeprecated-declarations] method = TLSv1_2_method(); ^ /home/asaveljevs/software/openssl-1.1.0-install/include/openssl/ssl.h:1608:45: note: 'TLSv1_2_method' has been explicitly marked deprecated here DEPRECATEDIN_1_1_0(__owur const SSL_METHOD *TLSv1_2_method(void)) /* TLSv1.2 */ ^ /home/asaveljevs/software/openssl-1.1.0-install/include/openssl/opensslconf.h:127:53: note: expanded from macro 'DEPRECATEDIN_1_1_0' # define DEPRECATEDIN_1_1_0(f) DECLARE_DEPRECATED(f) ^ /home/asaveljevs/software/openssl-1.1.0-install/include/openssl/opensslconf.h:102:35: note: expanded from macro 'DECLARE_DEPRECATED' # define DECLARE_DEPRECATED(f) f __attribute__ ((deprecated)); ^ tls.c:5644:41: error: incomplete definition of type 'struct ssl_session_st' if (NULL != (attr->psk_identity = sess->psk_identity)) ~~~~^ /home/asaveljevs/software/openssl-1.1.0-install/include/openssl/ssl.h:228:16: note: forward declaration of 'struct ssl_session_st' typedef struct ssl_session_st SSL_SESSION; ^ 2 warnings and 3 errors generated. make[3]: *** [libzbxcrypto_a-tls.o] Error 1 make[2]: *** [all-recursive] Error 1 make[1]: *** [all-recursive] Error 1 make: *** [all-recursive] Error 1 |
Comments |
Comment by Aleksandrs Saveljevs [ 2016 Aug 31 ] |
See https://www.openssl.org/news/changelog.html for a complete ChangeLog. In particular, the following changes seem interesting:
It may mean that our zbx_openssl_locking_cb() may no longer be needed, although it does not hurt to have one.
We use SSLeay_version(SSLEAY_VERSION) currently.
These are complained about in the compilation fragment above. |
Comment by Andris Mednis [ 2016 Sep 13 ] |
Fixed in development branch svn://svn.zabbix.com/branches/dev/ZBX-11149 . |
Comment by Glebs Ivanovskis (Inactive) [ 2016 Sep 23 ] |
Just asking... Do we feel like doing some of ZBX-10645 here? |
Comment by Andris Mednis [ 2016 Sep 23 ] |
No, that was not attempted here. |
Comment by Glebs Ivanovskis (Inactive) [ 2016 Sep 23 ] |
(1) [D] Version was changed for a reason. We should document that building Zabbix with custom-built encryption libraries is a very slippery road potentially leading to crashes because cURL, NetSNMP, OpenIPMI, DB client libraries, ODBC drivers may depend on the same encryption library and library linked directly to Zabbix overshadows secondary dependencies. glebs.ivanovskis Related linker warnings: /usr/bin/ld: warning: libssl.so.1.0.0, needed by /usr/lib/x86_64-linux-gnu/libpq.so, may conflict with libssl.so.1.1 /usr/bin/ld: warning: libcrypto.so.1.0.0, needed by /usr/lib/x86_64-linux-gnu/libpq.so, may conflict with libcrypto.so.1.1 glebs.ivanovskis Seems like situation is not that bad as I imagined thanks to symbol versioning. Probing Zabbix binary with ldd and nm -u, then launching with LD_DEBUG=bindings revealed that Zabbix'es symbols are resolved to OpenSSL 1.1.0 and libpg's symbols to OpenSSL 1.0.0. |
Comment by Glebs Ivanovskis (Inactive) [ 2016 Sep 26 ] |
(2) I guess we need to check zbx_set_ecdhe_parameters(ctx_psk) before setting ciphers to "kECDHEPSK+AES128:kPSK+AES128" or "kPSK+AES128" in zbx_tls_init_child(). glebs.ivanovskis I would also try to separate list of ciphersuites from code. See my suggestions and humble attempt to fix this subissue in r62785. RESOLVED andris Thanks for thorough review! Accepted. CLOSED |
Comment by Glebs Ivanovskis (Inactive) [ 2016 Sep 26 ] |
(3) I believe our code should be aimed to compile with the latest OpenSSL API version as a mainstream. Of course, we should not give away compatibility with older versions that easy. Usually there must be a universal way of doing things. If there isn't, I believe, we should use newer functions and provide our own forward-compatibility wrappers for older library versions. Otherwise code will be cluttered with #ifdef statements and too difficult to read. Please have a look at my suggestions in r62783. andris Good idea. Minor changes in r62836. RESOLVED glebs.ivanovskis Very nice! |
Comment by Glebs Ivanovskis (Inactive) [ 2016 Sep 26 ] |
(4) Do we want to make some work for future and make Zabbix compile with -DOPENSSL_API_COMPAT=0x10100000L passed in CFLAGS? Currently configuration fails because we use deprecated SSL_library_init() in m4 script.
glebs.ivanovskis Here I'll try to list stuff we use which is deprecated in 1.1.0:
For the record, OpenSSL 1.0.2 is a long term support version and it's lifetime matches life cycle of Zabbix 3.0 nicely, but for our next LTS release we should probably abandon 1.0.2 and switch to a newer version of OpenSSL which is yet to come. So feel free to register this as a separate issue. By the way, here is a very interesting document regarding future OpenSSL plans: https://www.openssl.org/policies/roadmap.html andris Thanks for pointing to deprecated functions! RESOLVED in r62862 and r62944 glebs.ivanovskis Very good that you replaced SSLeay_version() too! I didn't spot it.
Partially REOPENED, partially still RESOLVED
andris r63332 with OpenSSL 1.0.2 compiles with confusing warnings:
tls.c: In function ‘zbx_tls_library_init’:
tls.c:74:20: warning: right-hand operand of comma expression has no effect [-Wunused-value]
SSL_library_init(), \
^
tls.c:2572:11: note: in expansion of macro ‘OPENSSL_init_ssl’
if (1 != OPENSSL_init_ssl(OPENSSL_INIT_LOAD_SSL_STRINGS | OPENSSL_INIT_LOAD_CRYPTO_STRINGS, NULL))
^~~~~~~~~~~~~~~~
...
tls.c: In function ‘zbx_tls_library_deinit’:
tls.c:67:38: warning: statement with no effect [-Wunused-value]
# define CALL_ON_WINDOWS_ONLY(func) 0
^
tls.c:84:2: note: in expansion of macro ‘CALL_ON_WINDOWS_ONLY’
CALL_ON_WINDOWS_ONLY(zbx_openssl_thread_cleanup); \
^~~~~~~~~~~~~~~~~~~~
tls.c:2605:3: note: in expansion of macro ‘OPENSSL_cleanup’
OPENSSL_cleanup();
^~~~~~~~~~~~~~~
glebs.ivanovskis I was expecting that (see 3. above). Personally, I don't see these warnings as something horrible. But anyway, they can be silenced relatively easy. Please review r63341. RESOLVED andris Thanks for r63341! Reviewed. glebs.ivanovskis So, I guess, fully CLOSED |
Comment by Glebs Ivanovskis (Inactive) [ 2016 Sep 27 ] |
(5) Seems that the usage of ZBX_CONST_STRLEN() would be more appropriate here: if (0 == strncmp("ECDHE-PSK-", cipher_name, 10) || 0 == strncmp("PSK-", cipher_name, 4))
andris RESOLVED in 63033.
glebs.ivanovskis Thank you for the fix and for explanations! |
Comment by Glebs Ivanovskis (Inactive) [ 2016 Oct 12 ] |
(6) crypto_mutexes variable is not used with OpenSSL 1.1.0. We can move its definition into "legacy OpenSSL section". andris RESOLVED in r63327. glebs.ivanovskis Same applies to inclusion of mutex.h andris Thanks for finding it! RESOLVED in r63333. glebs.ivanovskis Very good! |
Comment by Glebs Ivanovskis (Inactive) [ 2016 Oct 14 ] |
Apart from minor style/warning/m4 issues everything seems to work correctly. It took a while for me to understand why OpenSSL exports locking functions. Looks like they are not for us. Another minor issue we can consider fixing are default library names when we are building on Windows. Compilation with TLSLIBDIR does not work, one needs to specify TLSLIB and TLSLIB2, because they were finally renamed into libcrypto.lib and libssl.lib. Maybe asaveljevs can comment on whether we need it, our encryption build process is still a bit unsettled. asaveljevs Indeed, OpenSSL version for precompiled binaries still remains to be decided upon (see glebs.ivanovskis Definitely yes, let me open a new subissue for that. |
Comment by Glebs Ivanovskis (Inactive) [ 2016 Oct 25 ] |
(7) When compiling with OpenSSL 1.1.0 on Windows one needs to specify TLSLIB and TLSLIB2 even if they are installed in the same directory. If TLSLIBDIR is specified, compilation fails because names of library binaries differ between 1.0.x and 1.1.0. I think makefile should try both possibilities before reporting failure. andris Thanks for finding it! RESOLVED in r63404. glebs.ivanovskis Works great but Makefile_tls.inc looks a bit strange. First of all, it violates our 120-character-per-line rule. So instead of
!ERROR Very long error message
we can use
ERRMSG = Very\
long\
error\
message
!ERROR $(ERRMSG)
Also, I've found a way to indent macro definitions: \ MACRO = value instead of MACRO = value Downside - it bloats Makefile vertically. Let's discuss which way to go with wiper and dimir and probably engrave Makefile style guidelines somewhere here. CLOSED |
Comment by Glebs Ivanovskis (Inactive) [ 2016 Nov 04 ] |
Successfully tested! |
Comment by Andris Mednis [ 2016 Nov 07 ] |
Fixed in versions:
|
Comment by Andris Mednis [ 2016 Nov 07 ] |
Documented in: glebs.ivanovskis Looks good to me, everything seems to be covered. |
Comment by Andris Mednis [ 2016 Nov 09 ] |
(8) Coverity detected a false positive. andris RESOLVED in development branch svn://svn.zabbix.com/branches/dev/ZBX-11149 r63635. glebs.ivanovskis CLOSED andris Fixed in versions:
|