From 0b0c29dd51a410be48b86c72d575fa69f3c71d71 Mon Sep 17 00:00:00 2001 From: Artem Boldariev Date: Wed, 24 Nov 2021 14:03:44 +0200 Subject: [PATCH 1/4] DoH: Remove unneeded isc__nmsocket_prep_destroy() call This commit removes unneeded isc__nmsocket_prep_destroy() call on ALPN negotiation failure, which was eventually causing the TLS handle to leak. This call is not needed, as not attaching to the transport (TLS) handle should be enough. At this point it seems like a kludge from earlier days of the TLS code. --- lib/isc/netmgr/http.c | 1 - 1 file changed, 1 deletion(-) diff --git a/lib/isc/netmgr/http.c b/lib/isc/netmgr/http.c index 776f0e2b77..466ae5497a 100644 --- a/lib/isc/netmgr/http.c +++ b/lib/isc/netmgr/http.c @@ -1379,7 +1379,6 @@ transport_connect_cb(isc_nmhandle_t *handle, isc_result_t result, void *cbarg) { * client will fail if HTTP/2 cannot be * negotiated via ALPN. */ - isc__nmsocket_prep_destroy(transp_sock); result = ISC_R_HTTP2ALPNERROR; goto error; } From b211fff4cb0ccb5f34d78ae3f0acc85ddf266439 Mon Sep 17 00:00:00 2001 From: Artem Boldariev Date: Wed, 24 Nov 2021 14:09:31 +0200 Subject: [PATCH 2/4] TLS stream: disable TLS I/O debug log message by default This commit makes the TLS stream code to not issue mostly useless debug log message on error during TLS I/O. This message was cluttering logs a lot, as it can be generated on (almost) any non-clean TLS connection termination, even in the cases when the actual query completed successfully. Nor does it provide much value for end-users, yet it can occasionally be seen when using dig and quite often when running BIND over a publicly available network interface. --- lib/isc/netmgr/tlsstream.c | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/lib/isc/netmgr/tlsstream.c b/lib/isc/netmgr/tlsstream.c index f5290890cd..08725a4802 100644 --- a/lib/isc/netmgr/tlsstream.c +++ b/lib/isc/netmgr/tlsstream.c @@ -367,7 +367,9 @@ tls_do_bio(isc_nmsocket_t *sock, isc_region_t *received_data, received_data->length, &len); if (rv <= 0 || len != received_data->length) { result = ISC_R_TLSERROR; +#if defined(NETMGR_TRACE) && defined(NETMGR_TRACE_VERBOSE) saved_errno = errno; +#endif goto error; } @@ -506,6 +508,7 @@ tls_do_bio(isc_nmsocket_t *sock, isc_region_t *received_data, } error: +#if defined(NETMGR_TRACE) && defined(NETMGR_TRACE_VERBOSE) isc_log_write(isc_lctx, ISC_LOGCATEGORY_GENERAL, ISC_LOGMODULE_NETMGR, ISC_LOG_NOTICE, "SSL error in BIO: %d %s (errno: %d). Arguments: " @@ -513,6 +516,7 @@ error: "send_data: %p, finish: %s", tls_status, isc_result_totext(result), saved_errno, received_data, send_data, finish ? "true" : "false"); +#endif tls_failed_read_cb(sock, result); } From babc2749b50d7a470d5a6d31cd6c6b1e7c48d8fa Mon Sep 17 00:00:00 2001 From: Artem Boldariev Date: Wed, 24 Nov 2021 14:26:40 +0200 Subject: [PATCH 3/4] DoH: Extend 'doth' test with a check if dig can detect ALPN failure This commit extends the 'doth' system test to verify if 'dig' can detect an properly recover after ALPN negotiation failure when making a DoH query. --- bin/tests/system/doth/tests.sh | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/bin/tests/system/doth/tests.sh b/bin/tests/system/doth/tests.sh index 77acac562d..ee25102f2a 100644 --- a/bin/tests/system/doth/tests.sh +++ b/bin/tests/system/doth/tests.sh @@ -116,6 +116,17 @@ grep "$msg_xfrs_not_allowed" dig.out.test$n > /dev/null || ret=1 if [ $ret != 0 ]; then echo_i "failed"; fi status=$((status + ret)) +# Let's try to issue an HTTP/2 query over TLS port to check if dig +# will detect ALPN token negotiation problem. +n=$((n + 1)) +echo_i "checking DoH query when ALPN is expected to fail (dot, failure expected) ($n)" +ret=0 +# shellcheck disable=SC2086 +"$DIG" +https $common_dig_options -p "${TLSPORT}" "$@" @10.53.0.1 . SOA > dig.out.test$n +grep "ALPN for HTTP/2 failed." dig.out.test$n > /dev/null || ret=1 +if [ $ret != 0 ]; then echo_i "failed"; fi +status=$((status + ret)) + n=$((n + 1)) echo_i "checking DoH query (POST) ($n)" ret=0 From 44951f8cac6a20583e463cb45d80a2f398664510 Mon Sep 17 00:00:00 2001 From: Artem Boldariev Date: Wed, 24 Nov 2021 14:37:53 +0200 Subject: [PATCH 4/4] Modify CHANGES [GL #3022] Mention that [GL #3022] was resolved. --- CHANGES | 3 +++ 1 file changed, 3 insertions(+) diff --git a/CHANGES b/CHANGES index 6a4f4ddcd6..96957be818 100644 --- a/CHANGES +++ b/CHANGES @@ -1,3 +1,6 @@ +5765. [bug] Fix a bug in DoH implementation making 'dig' + abort when ALPN negotiation fails. [GL #3022] + 5764. [bug] dns_sdlz_putrr failed to process some valid resource records. [GL #3021]