From 530133c10fb41da37cbd942aa0327732cab02d7c Mon Sep 17 00:00:00 2001 From: Artem Boldariev Date: Thu, 12 Aug 2021 10:14:30 +0300 Subject: [PATCH] Unify DoH URI making throughout the codebase This commit adds new function isc_nm_http_makeuri() which is supposed to unify DoH URI construction throughout the codebase. It handles IPv6 addresses, hostnames, and IPv6 addresses given as hostnames properly, and replaces similar ad-hoc code in the codebase. --- bin/tests/test_client.c | 33 +------- lib/isc/include/isc/netmgr.h | 15 ++++ lib/isc/netmgr/http.c | 60 +++++++++++++++ lib/isc/tests/doh_test.c | 142 +++++++++++++++++++++++++++++------ 4 files changed, 198 insertions(+), 52 deletions(-) diff --git a/bin/tests/test_client.c b/bin/tests/test_client.c index a0caead0c5..e74bc267c9 100644 --- a/bin/tests/test_client.c +++ b/bin/tests/test_client.c @@ -392,35 +392,6 @@ connect_cb(isc_nmhandle_t *handle, isc_result_t eresult, void *cbarg) { isc_nm_send(handle, &message, send_cb, NULL); } -#if HAVE_LIBNGHTTP2 -static void -sockaddr_to_url(isc_sockaddr_t *sa, const bool https, char *outbuf, - size_t outbuf_len, const char *append) { - uint16_t sa_port; - char saddr[INET6_ADDRSTRLEN] = { 0 }; - int sa_family; - - if (sa == NULL || outbuf == NULL || outbuf_len == 0) { - return; - } - - sa_family = ((struct sockaddr *)&sa->type.sa)->sa_family; - - sa_port = ntohs(sa_family == AF_INET ? sa->type.sin.sin_port - : sa->type.sin6.sin6_port); - inet_ntop(sa_family, - sa_family == AF_INET - ? (struct sockaddr *)&sa->type.sin.sin_addr - : (struct sockaddr *)&sa->type.sin6.sin6_addr, - saddr, sizeof(saddr)); - - snprintf(outbuf, outbuf_len, "%s://%s%s%s:%u%s", - https ? "https" : "http", sa_family == AF_INET ? "" : "[", - saddr, sa_family == AF_INET ? "" : "]", sa_port, - append ? append : ""); -} -#endif - static void run(void) { switch (protocol) { @@ -449,8 +420,8 @@ run(void) { bool is_post = (protocol == HTTPS_POST || protocol == HTTP_POST); char req_url[256]; - sockaddr_to_url(&sockaddr_remote, is_https, req_url, - sizeof(req_url), DEFAULT_DOH_PATH); + isc_nm_http_makeuri(is_https, &sockaddr_remote, NULL, 0, + DEFAULT_DOH_PATH, req_url, sizeof(req_url)); if (is_https) { isc_tlsctx_createclient(&tls_ctx); } diff --git a/lib/isc/include/isc/netmgr.h b/lib/isc/include/isc/netmgr.h index f9433754bd..3c5aff9e2b 100644 --- a/lib/isc/include/isc/netmgr.h +++ b/lib/isc/include/isc/netmgr.h @@ -549,6 +549,21 @@ isc_nm_is_http_handle(isc_nmhandle_t *handle); bool isc_nm_http_path_isvalid(const char *path); + +void +isc_nm_http_makeuri(const bool https, const isc_sockaddr_t *sa, + const char *hostname, const uint16_t http_port, + const char *abs_path, char *outbuf, + const size_t outbuf_len); +/*%< + * Makes a URI connection string out of na isc_sockaddr_t object 'sa' + * or the specified 'hostname' and 'http_port'. + * + * Requires: + * \li 'abs_path' is a valid absolute HTTP path string; + * \li 'outbuf' is a valid pointer to a buffer which will get the result; + * \li 'outbuf_len' is a size of the result buffer and is greater than zero. + */ #endif /* HAVE_LIBNGHTTP2 */ void diff --git a/lib/isc/netmgr/http.c b/lib/isc/netmgr/http.c index 6a8a4128c2..abca92731c 100644 --- a/lib/isc/netmgr/http.c +++ b/lib/isc/netmgr/http.c @@ -3081,6 +3081,66 @@ isc__nmhandle_http_keepalive(isc_nmhandle_t *handle, bool value) { } } +void +isc_nm_http_makeuri(const bool https, const isc_sockaddr_t *sa, + const char *hostname, const uint16_t http_port, + const char *abs_path, char *outbuf, + const size_t outbuf_len) { + char saddr[INET6_ADDRSTRLEN] = { 0 }; + int family; + bool ipv6_addr = false; + struct sockaddr_in6 sa6; + uint16_t host_port = http_port; + const char *host = NULL; + + REQUIRE(outbuf != NULL); + REQUIRE(outbuf_len != 0); + REQUIRE(isc_nm_http_path_isvalid(abs_path)); + + /* If hostname is specified, use that. */ + if (hostname != NULL && hostname[0] != '\0') { + /* + * The host name could be an IPv6 address. If so, + * wrap it between [ and ]. + */ + if (inet_pton(AF_INET6, hostname, &sa6) == 1 && + hostname[0] != '[') { + ipv6_addr = true; + } + host = hostname; + } else { + /* + * A hostname was not specified; build one from + * the given IP address. + */ + INSIST(sa != NULL); + family = ((const struct sockaddr *)&sa->type.sa)->sa_family; + host_port = ntohs(family == AF_INET ? sa->type.sin.sin_port + : sa->type.sin6.sin6_port); + ipv6_addr = family == AF_INET6; + (void)inet_ntop( + family, + family == AF_INET + ? (const struct sockaddr *)&sa->type.sin.sin_addr + : (const struct sockaddr *)&sa->type.sin6 + .sin6_addr, + saddr, sizeof(saddr)); + host = saddr; + } + + /* + * If the port number was not specified, the default + * depends on whether we're using encryption or not. + */ + if (host_port == 0) { + host_port = https ? 443 : 80; + } + + (void)snprintf(outbuf, outbuf_len, "%s://%s%s%s:%u%s", + https ? "https" : "http", ipv6_addr ? "[" : "", host, + ipv6_addr ? "]" : "", host_port, abs_path); +} + /* * DoH GET Query String Scanner-less Recursive Descent Parser/Verifier * diff --git a/lib/isc/tests/doh_test.c b/lib/isc/tests/doh_test.c index 1442610233..eb94f1cef3 100644 --- a/lib/isc/tests/doh_test.c +++ b/lib/isc/tests/doh_test.c @@ -375,27 +375,7 @@ thread_local size_t nwrites = NWRITES; static void sockaddr_to_url(isc_sockaddr_t *sa, const bool https, char *outbuf, size_t outbuf_len, const char *append) { - uint16_t port; - char saddr[INET6_ADDRSTRLEN] = { 0 }; - int family; - - if (sa == NULL || outbuf == NULL || outbuf_len == 0) { - return; - } - - family = ((struct sockaddr *)&sa->type.sa)->sa_family; - - port = ntohs(family == AF_INET ? sa->type.sin.sin_port - : sa->type.sin6.sin6_port); - inet_ntop(family, - family == AF_INET - ? (struct sockaddr *)&sa->type.sin.sin_addr - : (struct sockaddr *)&sa->type.sin6.sin6_addr, - saddr, sizeof(saddr)); - - snprintf(outbuf, outbuf_len, "%s://%s%s%s:%u%s", - https ? "https" : "http", family == AF_INET ? "" : "[", saddr, - family == AF_INET ? "" : "]", port, append ? append : ""); + isc_nm_http_makeuri(https, sa, NULL, 0, append, outbuf, outbuf_len); } static isc_quota_t * @@ -2085,6 +2065,122 @@ doh_path_validation(void **state) { assert_true(isc_nm_http_path_isvalid("/123")); } +static void +doh_connect_makeuri(void **state) { + struct in_addr localhostv4 = { ntohl(INADDR_LOOPBACK) }; + isc_sockaddr_t sa; + char uri[256]; + UNUSED(state); + + /* Firstly, test URI generation using isc_sockaddr_t */ + isc_sockaddr_fromin(&sa, &localhostv4, 0); + uri[0] = '\0'; + isc_nm_http_makeuri(true, &sa, NULL, 0, DOH_PATH, uri, sizeof(uri)); + assert_true(strcmp("https://127.0.0.1:443/dns-query", uri) == 0); + + uri[0] = '\0'; + isc_nm_http_makeuri(false, &sa, NULL, 0, DOH_PATH, uri, sizeof(uri)); + assert_true(strcmp("http://127.0.0.1:80/dns-query", uri) == 0); + + /* + * The port value should be ignored, because we can get one from + * the isc_sockaddr_t object. + */ + uri[0] = '\0'; + isc_nm_http_makeuri(true, &sa, NULL, 44343, DOH_PATH, uri, sizeof(uri)); + assert_true(strcmp("https://127.0.0.1:443/dns-query", uri) == 0); + + uri[0] = '\0'; + isc_nm_http_makeuri(false, &sa, NULL, 8080, DOH_PATH, uri, sizeof(uri)); + assert_true(strcmp("http://127.0.0.1:80/dns-query", uri) == 0); + + /* IPv6 */ + isc_sockaddr_fromin6(&sa, &in6addr_loopback, 0); + uri[0] = '\0'; + isc_nm_http_makeuri(true, &sa, NULL, 0, DOH_PATH, uri, sizeof(uri)); + assert_true(strcmp("https://[::1]:443/dns-query", uri) == 0); + + uri[0] = '\0'; + isc_nm_http_makeuri(false, &sa, NULL, 0, DOH_PATH, uri, sizeof(uri)); + assert_true(strcmp("http://[::1]:80/dns-query", uri) == 0); + + /* + * The port value should be ignored, because we can get one from + * the isc_sockaddr_t object. + */ + uri[0] = '\0'; + isc_nm_http_makeuri(true, &sa, NULL, 44343, DOH_PATH, uri, sizeof(uri)); + assert_true(strcmp("https://[::1]:443/dns-query", uri) == 0); + + uri[0] = '\0'; + isc_nm_http_makeuri(false, &sa, NULL, 8080, DOH_PATH, uri, sizeof(uri)); + assert_true(strcmp("http://[::1]:80/dns-query", uri) == 0); + + /* Try to set the port numbers. */ + isc_sockaddr_setport(&sa, 44343); + uri[0] = '\0'; + isc_nm_http_makeuri(true, &sa, NULL, 0, DOH_PATH, uri, sizeof(uri)); + assert_true(strcmp("https://[::1]:44343/dns-query", uri) == 0); + + isc_sockaddr_setport(&sa, 8080); + uri[0] = '\0'; + isc_nm_http_makeuri(false, &sa, NULL, 0, DOH_PATH, uri, sizeof(uri)); + assert_true(strcmp("http://[::1]:8080/dns-query", uri) == 0); + + /* + * Try to make a URI using a hostname and a port number. The + * isc_sockaddr_t object will be ignored. + */ + isc_sockaddr_any(&sa); + uri[0] = '\0'; + isc_nm_http_makeuri(true, &sa, "example.com", 0, DOH_PATH, uri, + sizeof(uri)); + assert_true(strcmp("https://example.com:443/dns-query", uri) == 0); + + uri[0] = '\0'; + isc_nm_http_makeuri(false, &sa, "example.com", 0, DOH_PATH, uri, + sizeof(uri)); + assert_true(strcmp("http://example.com:80/dns-query", uri) == 0); + + /* Try to set the port numbers. */ + isc_sockaddr_setport(&sa, 443); + uri[0] = '\0'; + isc_nm_http_makeuri(true, &sa, "example.com", 44343, DOH_PATH, uri, + sizeof(uri)); + assert_true(strcmp("https://example.com:44343/dns-query", uri) == 0); + + isc_sockaddr_setport(&sa, 80); + uri[0] = '\0'; + isc_nm_http_makeuri(false, &sa, "example.com", 8080, DOH_PATH, uri, + sizeof(uri)); + assert_true(strcmp("http://example.com:8080/dns-query", uri) == 0); + + /* IPv4 as the hostname - nothing fancy here */ + uri[0] = '\0'; + isc_nm_http_makeuri(false, NULL, "127.0.0.1", 8080, DOH_PATH, uri, + sizeof(uri)); + assert_true(strcmp("http://127.0.0.1:8080/dns-query", uri) == 0); + + uri[0] = '\0'; + isc_nm_http_makeuri(true, NULL, "127.0.0.1", 44343, DOH_PATH, uri, + sizeof(uri)); + assert_true(strcmp("https://127.0.0.1:44343/dns-query", uri) == 0); + + /* + * A peculiar edge case: IPv6 given as the hostname (notice + * the brackets) + */ + uri[0] = '\0'; + isc_nm_http_makeuri(false, NULL, "::1", 8080, DOH_PATH, uri, + sizeof(uri)); + assert_true(strcmp("http://[::1]:8080/dns-query", uri) == 0); + + uri[0] = '\0'; + isc_nm_http_makeuri(true, NULL, "[::1]", 44343, DOH_PATH, uri, + sizeof(uri)); + assert_true(strcmp("https://[::1]:44343/dns-query", uri) == 0); +} + int main(void) { const struct CMUnitTest tests_short[] = { @@ -2098,6 +2194,8 @@ main(void) { NULL), cmocka_unit_test_setup_teardown(doh_path_validation, NULL, NULL), + cmocka_unit_test_setup_teardown(doh_connect_makeuri, NULL, + NULL), cmocka_unit_test_setup_teardown(doh_noop_POST, nm_setup, nm_teardown), cmocka_unit_test_setup_teardown(doh_noop_GET, nm_setup, @@ -2121,6 +2219,8 @@ main(void) { NULL), cmocka_unit_test_setup_teardown(doh_path_validation, NULL, NULL), + cmocka_unit_test_setup_teardown(doh_connect_makeuri, NULL, + NULL), cmocka_unit_test_setup_teardown(doh_noop_POST, nm_setup, nm_teardown), cmocka_unit_test_setup_teardown(doh_noop_GET, nm_setup,