From d1ce3058fcffd31496346f4575020162f6c49077 Mon Sep 17 00:00:00 2001 From: Remi Gacogne Date: Mon, 16 Jan 2023 15:28:02 +0100 Subject: [PATCH] dnsdist: Skip invalid OCSP files after issuing a warning Contrary to certificates and keys, OCSP files are never required to provide a working DoT or DoH service, so it's better to start even if would not load all, or any, OCSP files. --- pdns/dnsdistdist/doh.cc | 5 +- pdns/dolog.hh | 1 - pdns/libssl.cc | 21 ++++--- pdns/libssl.hh | 9 ++- pdns/tcpiohandler.cc | 9 ++- regression-tests.dnsdist/test_OCSP.py | 83 +++++++++++++++++++++++++++ 6 files changed, 110 insertions(+), 18 deletions(-) diff --git a/pdns/dnsdistdist/doh.cc b/pdns/dnsdistdist/doh.cc index 799462898a..6912b79fa0 100644 --- a/pdns/dnsdistdist/doh.cc +++ b/pdns/dnsdistdist/doh.cc @@ -1425,7 +1425,10 @@ static void setupTLSContext(DOHAcceptContext& acceptCtx, tlsConfig.d_ciphers = DOH_DEFAULT_CIPHERS; } - auto ctx = libssl_init_server_context(tlsConfig, acceptCtx.d_ocspResponses); + auto [ctx, warnings] = libssl_init_server_context(tlsConfig, acceptCtx.d_ocspResponses); + for (const auto& warning : warnings) { + warnlog("%s", warning); + } if (tlsConfig.d_enableTickets && tlsConfig.d_numberOfTicketsKeys > 0) { acceptCtx.d_ticketKeys = std::make_unique(tlsConfig.d_numberOfTicketsKeys); diff --git a/pdns/dolog.hh b/pdns/dolog.hh index 1767cd4c03..a28777ab81 100644 --- a/pdns/dolog.hh +++ b/pdns/dolog.hh @@ -209,4 +209,3 @@ void errlog(const char* s, Args... args) } #endif - diff --git a/pdns/libssl.cc b/pdns/libssl.cc index e9fcba6cef..2966e2b1b4 100644 --- a/pdns/libssl.cc +++ b/pdns/libssl.cc @@ -396,7 +396,7 @@ static bool libssl_validate_ocsp_response(const std::string& response) return true; } -std::map libssl_load_ocsp_responses(const std::vector& ocspFiles, std::vector keyTypes) +static std::map libssl_load_ocsp_responses(const std::vector& ocspFiles, std::vector keyTypes, std::vector& warnings) { std::map ocspResponses; @@ -408,12 +408,13 @@ std::map libssl_load_ocsp_responses(const std::vector libssl_load_ocsp_responses(const std::vector libssl_init_server_context(const TLSConfig& config, - std::map& ocspResponses) +std::pair, std::vector> libssl_init_server_context(const TLSConfig& config, + std::map& ocspResponses) { + std::vector warnings; auto ctx = std::unique_ptr(SSL_CTX_new(SSLv23_server_method()), SSL_CTX_free); if (!ctx) { @@ -881,7 +884,7 @@ std::unique_ptr libssl_init_server_context(const TLS #ifdef SSL_MODE_ASYNC mode |= SSL_MODE_ASYNC; #else - cerr<<"Warning: TLS async mode requested but not supported"< libssl_init_server_context(const TLS #ifndef DISABLE_OCSP_STAPLING if (!config.d_ocspFiles.empty()) { try { - ocspResponses = libssl_load_ocsp_responses(config.d_ocspFiles, keyTypes); + ocspResponses = libssl_load_ocsp_responses(config.d_ocspFiles, keyTypes, warnings); } catch(const std::exception& e) { throw std::runtime_error("Unable to load OCSP responses: " + std::string(e.what())); @@ -988,7 +991,7 @@ std::unique_ptr libssl_init_server_context(const TLS } #endif /* HAVE_SSL_CTX_SET_CIPHERSUITES */ - return ctx; + return std::make_pair(std::move(ctx), std::move(warnings)); } #ifdef HAVE_SSL_CTX_SET_KEYLOG_CALLBACK diff --git a/pdns/libssl.hh b/pdns/libssl.hh index d8af451112..ad6bcd8133 100644 --- a/pdns/libssl.hh +++ b/pdns/libssl.hh @@ -133,9 +133,6 @@ int libssl_ticket_key_callback(SSL* s, OpenSSLTLSTicketKeysRing& keyring, unsign #ifndef DISABLE_OCSP_STAPLING int libssl_ocsp_stapling_callback(SSL* ssl, const std::map& ocspMap); - -std::map libssl_load_ocsp_responses(const std::vector& ocspFiles, std::vector keyTypes); - #ifdef HAVE_OCSP_BASIC_SIGN bool libssl_generate_ocsp_response(const std::string& certFile, const std::string& caCert, const std::string& caKey, const std::string& outFile, int ndays, int nmin); #endif @@ -147,8 +144,10 @@ LibsslTLSVersion libssl_tls_version_from_string(const std::string& str); const std::string& libssl_tls_version_to_string(LibsslTLSVersion version); bool libssl_set_min_tls_version(std::unique_ptr& ctx, LibsslTLSVersion version); -std::unique_ptr libssl_init_server_context(const TLSConfig& config, - std::map& ocspResponses); +/* return the created context, and a list of warning messages for issues not severe enough + to trigger raising an exception, like failing to load an OCSP response file */ +std::pair, std::vector> libssl_init_server_context(const TLSConfig& config, + std::map& ocspResponses); std::unique_ptr libssl_set_key_log_file(std::unique_ptr& ctx, const std::string& logFile); diff --git a/pdns/tcpiohandler.cc b/pdns/tcpiohandler.cc index fd3a50b7c9..f2817bd902 100644 --- a/pdns/tcpiohandler.cc +++ b/pdns/tcpiohandler.cc @@ -30,7 +30,12 @@ public: { registerOpenSSLUser(); - d_tlsCtx = libssl_init_server_context(tlsConfig, d_ocspResponses); + auto [ctx, warnings] = libssl_init_server_context(tlsConfig, d_ocspResponses); + for (const auto& warning : warnings) { + warnlog("%s", warning); + } + d_tlsCtx = std::move(ctx); + if (!d_tlsCtx) { ERR_print_errors_fp(stderr); throw std::runtime_error("Error creating TLS context on " + addr.toStringWithPort()); @@ -1565,7 +1570,7 @@ public: for (const auto& file : fe.d_tlsConfig.d_ocspFiles) { rc = gnutls_certificate_set_ocsp_status_request_file(d_creds.get(), file.c_str(), count); if (rc != GNUTLS_E_SUCCESS) { - throw std::runtime_error("Error loading OCSP response from file '" + file + "' for certificate ('" + fe.d_tlsConfig.d_certKeyPairs.at(count).d_cert + "') and key ('" + fe.d_tlsConfig.d_certKeyPairs.at(count).d_key.value() + "') for TLS context on " + fe.d_addr.toStringWithPort() + ": " + gnutls_strerror(rc)); + warnlog("Error loading OCSP response from file '%s' for certificate ('%s') and key ('%s') for TLS context on %s: %s", file, fe.d_tlsConfig.d_certKeyPairs.at(count).d_cert, fe.d_tlsConfig.d_certKeyPairs.at(count).d_key.value(), fe.d_addr.toStringWithPort(), gnutls_strerror(rc)); } ++count; } diff --git a/regression-tests.dnsdist/test_OCSP.py b/regression-tests.dnsdist/test_OCSP.py index 6b29e3df55..a8577baea1 100644 --- a/regression-tests.dnsdist/test_OCSP.py +++ b/regression-tests.dnsdist/test_OCSP.py @@ -91,6 +91,33 @@ class TestOCSPStaplingDOH(DNSDistOCSPStaplingTest): self.assertTrue(serialNumber2) self.assertNotEqual(serialNumber, serialNumber2) +class TestBrokenOCSPStaplingDoH(DNSDistOCSPStaplingTest): + + _consoleKey = DNSDistTest.generateConsoleKey() + _consoleKeyB64 = base64.b64encode(_consoleKey).decode('ascii') + _serverKey = 'server.key' + _serverCert = 'server.chain' + _serverName = 'tls.tests.dnsdist.org' + _caCert = 'ca.pem' + # invalid OCSP file! + _ocspFile = '/dev/null' + _tlsServerPort = 8443 + _config_template = """ + newServer{address="127.0.0.1:%s"} + setKey("%s") + controlSocket("127.0.0.1:%s") + + addDOHLocal("127.0.0.1:%s", "%s", "%s", { "/" }, { ocspResponses={"%s"}}) + """ + _config_params = ['_testServerPort', '_consoleKeyB64', '_consolePort', '_tlsServerPort', '_serverCert', '_serverKey', '_ocspFile'] + + def testBrokenOCSPStapling(self): + """ + OCSP Stapling: Broken (DoH) + """ + output = self.checkOCSPStaplingStatus('127.0.0.1', self._tlsServerPort, self._serverName, self._caCert) + self.assertNotIn('OCSP Response Status: successful (0x0)', output) + class TestOCSPStaplingTLSGnuTLS(DNSDistOCSPStaplingTest): _consoleKey = DNSDistTest.generateConsoleKey() @@ -134,6 +161,34 @@ class TestOCSPStaplingTLSGnuTLS(DNSDistOCSPStaplingTest): self.assertTrue(serialNumber2) self.assertNotEqual(serialNumber, serialNumber2) +class TestBrokenOCSPStaplingTLSGnuTLS(DNSDistOCSPStaplingTest): + + _consoleKey = DNSDistTest.generateConsoleKey() + _consoleKeyB64 = base64.b64encode(_consoleKey).decode('ascii') + _serverKey = 'server.key' + _serverCert = 'server.chain' + _serverName = 'tls.tests.dnsdist.org' + _caCert = 'ca.pem' + # invalid OCSP file! + _ocspFile = '/dev/null' + _tlsServerPort = 8443 + _config_template = """ + newServer{address="127.0.0.1:%s"} + setKey("%s") + controlSocket("127.0.0.1:%s") + + addTLSLocal("127.0.0.1:%s", "%s", "%s", { provider="gnutls", ocspResponses={"%s"}}) + """ + _config_params = ['_testServerPort', '_consoleKeyB64', '_consolePort', '_tlsServerPort', '_serverCert', '_serverKey', '_ocspFile'] + + def testBrokenOCSPStapling(self): + """ + OCSP Stapling: Broken (GnuTLS) + """ + output = self.checkOCSPStaplingStatus('127.0.0.1', self._tlsServerPort, self._serverName, self._caCert) + self.assertNotIn('OCSP Response Status: successful (0x0)', output) + self.assertEquals(self.getTLSProvider(), "gnutls") + class TestOCSPStaplingTLSOpenSSL(DNSDistOCSPStaplingTest): _consoleKey = DNSDistTest.generateConsoleKey() @@ -176,3 +231,31 @@ class TestOCSPStaplingTLSOpenSSL(DNSDistOCSPStaplingTest): serialNumber2 = self.getOCSPSerial(output) self.assertTrue(serialNumber2) self.assertNotEqual(serialNumber, serialNumber2) + +class TestBrokenOCSPStaplingTLSOpenSSL(DNSDistOCSPStaplingTest): + + _consoleKey = DNSDistTest.generateConsoleKey() + _consoleKeyB64 = base64.b64encode(_consoleKey).decode('ascii') + _serverKey = 'server.key' + _serverCert = 'server.chain' + _serverName = 'tls.tests.dnsdist.org' + _caCert = 'ca.pem' + # invalid OCSP file! + _ocspFile = '/dev/null' + _tlsServerPort = 8443 + _config_template = """ + newServer{address="127.0.0.1:%s"} + setKey("%s") + controlSocket("127.0.0.1:%s") + + addTLSLocal("127.0.0.1:%s", "%s", "%s", { provider="openssl", ocspResponses={"%s"}}) + """ + _config_params = ['_testServerPort', '_consoleKeyB64', '_consolePort', '_tlsServerPort', '_serverCert', '_serverKey', '_ocspFile'] + + def testBrokenOCSPStapling(self): + """ + OCSP Stapling: Broken (OpenSSL) + """ + output = self.checkOCSPStaplingStatus('127.0.0.1', self._tlsServerPort, self._serverName, self._caCert) + self.assertNotIn('OCSP Response Status: successful (0x0)', output) + self.assertEquals(self.getTLSProvider(), "openssl") -- 2.47.2