[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:

*) OpenSSL now uses a new threading API. It is no longer necessary to
set locking callbacks to use OpenSSL in a multi-threaded environment. There
are two supported threading models: pthreads and windows threads. It is
also possible to configure OpenSSL at compile time for "no-threads". The
old threading API should no longer be used. The functions have been
replaced with "no-op" compatibility macros.

It may mean that our zbx_openssl_locking_cb() may no longer be needed, although it does not hurt to have one.

*) All instances of the string "ssleay" in the public API were replaced
with OpenSSL (case-matching; e.g., OPENSSL_VERSION for #define's)
Some error codes related to internal RSA_eay API's were renamed.

We use SSLeay_version(SSLEAY_VERSION) currently.

*) Version negotiation has been rewritten. In particular SSLv23_method(),
SSLv23_client_method() and SSLv23_server_method() have been deprecated,
and turned into macros which simply call the new preferred function names
TLS_method(), TLS_client_method() and TLS_server_method(). All new code
should use the new names instead. Also as part of this change the ssl23.h
header file has been removed.

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.
CLOSED

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!
CLOSED

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.

The SSL_library_init() and OpenSSL_add_ssl_algorithms() functions were deprecated in OpenSSL 1.1.0 by OPENSSL_init_ssl().

glebs.ivanovskis Here I'll try to list stuff we use which is deprecated in 1.1.0:

  • SSL_library_init()
  • SSL_load_error_strings()
  • ERR_free_strings()
  • RAND_cleanup()

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.
Several comments:

  1. I don't like the idea to use conditional compilation in m4 scripts. Why can't we simply choose a function that retains its name across API versions (SSL_accept() or SSL_connect for example)? Or here is described another approach using AC_SEARCH_LIBS() instead of AC_TRY_LINK().
  2. There is OPENSSL_cleanup() function in 1.1.0 which may be handy for us:

    Typically there should be no need to call this function directly as it is initiated automatically on application exit. This is done via the standard C library atexit(3) function. In the event that the application will close in a manner that will not call the registered atexit() handlers then the application should call OPENSSL_cleanup() directly.

  3. I've moved pre-1.1.0 initialization and deinitialization into legacy macro section in r63104. We will now have a couple of -Wunused-value warnings but zbx_tls_library_init/deinit() look nicer. What's your opinion?

Partially REOPENED, partially still RESOLVED

andris

  1. conditional compilation in m4/libopenssl.m4 replaced with SSL_connect() for all versions in r63323.
  2. r63104 does not compile with OpenSSL 1.0.2 (with 1.1.0 it is ok).

glebs.ivanovskis

  1. Nice! This part CLOSED
  2. I'm sure it was working at some point for me. Probably I forgot to check it after final refactoring. Hopefully RESOLVED in r63332.

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))
  1. Is there really no better way to determine connection type with OpenSSL?
  2. Why do we compare to "NONE" when OpenSSL documentation implies that SSL_get_cipher() returns "(NONE)"?
  3. Why do we call SSL_get_verify_result() twice?

andris RESOLVED in 63033.

  1. I could not find a better way to distinguish between accepted certificate-based and PSK-based connection than examining the ciphersuite name.
  2. Seems like a mistake. RESOLVED in 63033.
  3. SSL_get_verify_result() is called only once:
  • if SSL_connect() or SSL_accept() fail - to find out why TLS handshake failed or
  • if SSL_connect() or SSL_accept() succeed - to find out about other possible certificate errors which were accepted during handshake but might not be acceptable for Zabbix.

glebs.ivanovskis Thank you for the fix and for explanations!
CLOSED

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!
CLOSED

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 ZBXNEXT-3047). However, do you think it would be possible to automatically choose between 1.0.x and 1.1.0 names in Makefile_tls.inc?

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:

  • pre-3.0.6rc1 r63565,
  • pre-3.2.2rc1 r63566,
  • pre-3.3.0 (trunk) r63567.
Comment by Andris Mednis [ 2016 Nov 07 ]

Documented in:
https://www.zabbix.com/documentation/3.0/manual/encryption (added OpenSSL 1.1.0 ciphersuites)
https://www.zabbix.com/documentation/3.2/manual/encryption
https://www.zabbix.com/documentation/3.4/manual/encryption
https://www.zabbix.com/documentation/3.0/manual/introduction/whatsnew306#daemon_changes
https://www.zabbix.com/documentation/3.2/manual/introduction/whatsnew322#daemon_changes

glebs.ivanovskis Looks good to me, everything seems to be covered.
APPROVED

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:

  • pre-3.0.6rc1 r63666,
  • pre-3.2.2rc1 r63667,
  • pre-3.3.0 (trunk) r63668.
Generated at Fri Apr 19 09:47:37 EEST 2024 using Jira 9.12.4#9120004-sha1:625303b708afdb767e17cb2838290c41888e9ff0.