Disable TLS renegotiation and fix compile error on OpenBSD

[Al2Klimov]

@sthen Ok?

[sthen]

ok with me!

[yhabteab]

I have absolutely no clue about OpenBSD, but how do you turn off tls renegotiation then?

[Al2Klimov]

That's the neat part, you don't! It's already the default. https://github.com/libressl/portable/blob/68ad61fd6d199607af327188c2dad0779f98fa46/ChangeLog#L2316-L2318

[yhabteab]

🤷

[Al2Klimov]

Actually, despite what I thought, v2.14.0 from ports allows renegotiation by default:

    Verify return code: 19 (self signed certificate in certificate chain)
---
R
RENEGOTIATING
depth=1 CN = aklimov-openbsd.my.domain
verify error:num=19:self signed certificate in certificate chain
verify return:0
R
RENEGOTIATING
depth=1 CN = aklimov-openbsd.my.domain
verify error:num=19:self signed certificate in certificate chain
verify return:0
closed
aklimov-openbsd#

However with

--- lib/base/tlsutility.cpp
+++ lib/base/tlsutility.cpp
@@ -90,5 +90,5 @@ static void InitSslContext(const Shared<boost::asio::ssl::context>::Ptr& context
 	long flags = SSL_CTX_get_options(sslContext);

-	flags |= SSL_OP_CIPHER_SERVER_PREFERENCE;
+	flags |= SSL_OP_CIPHER_SERVER_PREFERENCE | SSL_OP_NO_CLIENT_RENEGOTIATION;

 	SSL_CTX_set_options(sslContext, flags);

under patches/:

    Verify return code: 19 (self signed certificate in certificate chain)
---
R
RENEGOTIATING
14177350148600:error:1400444C:SSL routines:CONNECT_CR_SRVR_HELLO:tlsv1 alert no renegotiation:/usr/src/lib/libssl/ssl_pkt.c:753:SSL alert number 100
14177350148600:error:140040E5:SSL routines:CONNECT_CR_SRVR_HELLO:ssl handshake failure:/usr/src/lib/libssl/ssl_pkt.c:495:
aklimov-openbsd#

[oxzi]

I have successfully built Icing 2 on OpenBSD through ports via replacing patches/patch-lib_base_tlsutility_cpp by an adequately formatted version of this PR.

Index: lib/base/tlsutility.cpp
--- lib/base/tlsutility.cpp.orig
+++ lib/base/tlsutility.cpp
@@ -93,7 +93,9 @@ static void InitSslContext(const Shared<boost::asio::s

        flags |= SSL_OP_CIPHER_SERVER_PREFERENCE;

-#if OPENSSL_VERSION_NUMBER < 0x10100000L
+#ifdef LIBRESSL_VERSION_NUMBER
+       flags |= SSL_OP_NO_CLIENT_RENEGOTIATION;
+#elif OPENSSL_VERSION_NUMBER < 0x10100000L
        SSL_CTX_set_info_callback(sslContext, [](const SSL* ssl, int where, int) {
                if (where & SSL_CB_HANDSHAKE_DONE) {
                        ssl->s3->flags |= SSL3_FLAGS_NO_RENEGOTIATE_CIPHERS;

So this PR should work and for future Icinga 2 releases this patch can be removed from the OpenBSD ports. The other diff in the original patch was already merged in #9882. (CC: @sthen)

For the future, we should find a Linux distribution that allows us to build Icinga 2 with LibreSSL via the CI. I will to create an issue.

[Al2Klimov]

• CC GHA: also build on Alpine to test LibreSSL which is used on OpenBSD #9949

[sthen]

Why not just guard it behind #ifdef SSL_OP_NO_CLIENT_RENEGOTIATION like postgresql and nginx do?

Cc @botovq, do you have opinions on this?

[Al2Klimov]

Rather SSL_OP_NO_RENEGOTIATION and fall back to SSL_OP_NO_CLIENT_RENEGOTIATION, but yes – you have a point.

[julianbrost]

This would add the assumption that OpenSSL will never ever introduce a macro with the same name (and if they did, there would be the question if in a compatible way or in a way that might break the code silently). So I'm not immediately convinced that this is actually a good idea.

[oxzi]

As both OpenSSL's SSL_OP_NO_RENEGOTIATION and LibreSSL's SSL_OP_NO_CLIENT_RENEGOTIATION disallow renegotiation in TLS versions <=1.2 and renegotiation was removed in TLS 1.3, I won't expect a SSL_OP_NO_CLIENT_RENEGOTIATION to emerge in OpenSSL.

However, @julianbrost has a valid point and OpenSSL is our main target¹. Thus, I would keep the PR as it is with a comparison against LIBRESSL_VERSION_NUMBER.

Footnotes

1. Especially since OpenBSD seems to be the only target using LibreSSL since Alpine and Void have dropped LibreSSL in favor of OpenSSL. ↩

[julianbrost]

Anyone who wants to make an argument for using #ifdef SSL_OP_* instead? One reason I can see would be to maximize the chance that the code keeps compiling in the future. However, if we say disabling renegotiation is important and OpenSSL or LibreSSL change something regarding these constants, I'd rather have it fail at compile time so that someone can look into it, instead of it doing something unknown.

If not, @Al2Klimov can you please rebase the PR so that it runs the latest actions? It changes code that does a version distinction on OpenSSL and there's a wide range of versions, so also running it on the newer distros sounds like a good idea to me.