diff --git a/docs/root/intro/version_history.rst b/docs/root/intro/version_history.rst index 48d0eba45a10a..d914f0544446c 100644 --- a/docs/root/intro/version_history.rst +++ b/docs/root/intro/version_history.rst @@ -5,6 +5,7 @@ Version history ================ Changes ------- +* http: fixed URL parsing for HTTP/1.1 fully qualified URLs and connect requests containing IPv6 addresses. 1.14.6 (December 7, 2020) ========================= diff --git a/source/common/http/http1/codec_impl.cc b/source/common/http/http1/codec_impl.cc index b71b3ea5f8f5a..fd53b2034d600 100644 --- a/source/common/http/http1/codec_impl.cc +++ b/source/common/http/http1/codec_impl.cc @@ -768,7 +768,7 @@ void ServerConnectionImpl::handlePath(RequestHeaderMap& headers, unsigned int me } Utility::Url absolute_url; - if (!absolute_url.initialize(active_request.request_url_.getStringView())) { + if (!absolute_url.initialize(active_request.request_url_.getStringView(), is_connect)) { sendProtocolError(Http1ResponseCodeDetails::get().InvalidUrl); throw CodecProtocolException("http/1.1 protocol error: invalid url in request line"); } @@ -779,9 +779,9 @@ void ServerConnectionImpl::handlePath(RequestHeaderMap& headers, unsigned int me // request-target. A proxy that forwards such a request MUST generate a // new Host field-value based on the received request-target rather than // forward the received Host field-value. - headers.setHost(absolute_url.host_and_port()); + headers.setHost(absolute_url.hostAndPort()); - headers.setPath(absolute_url.path_and_query_params()); + headers.setPath(absolute_url.pathAndQueryParams()); active_request.request_url_.clear(); } diff --git a/source/common/http/utility.cc b/source/common/http/utility.cc index 4c59a4f34e0c8..f103d06e3174e 100644 --- a/source/common/http/utility.cc +++ b/source/common/http/utility.cc @@ -185,9 +185,32 @@ namespace Http { static const char kDefaultPath[] = "/"; -bool Utility::Url::initialize(absl::string_view absolute_url) { +// If http_parser encounters an IP address [address] as the host it will set the offset and +// length to point to 'address' rather than '[address]'. Fix this by adjusting the offset +// and length to include the brackets. +// @param absolute_url the absolute URL. This is usually of the form // http://host/path +// but may be host:port for CONNECT requests +// @param offset the offset for the first character of the host. For IPv6 hosts +// this will point to the first character inside the brackets and will be +// adjusted to point at the brackets +// @param len the length of the host-and-port field. For IPv6 hosts this will +// not include the brackets and will be adjusted to do so. +bool maybeAdjustForIpv6(absl::string_view absolute_url, uint64_t& offset, uint64_t& len) { + // According to https://tools.ietf.org/html/rfc3986#section-3.2.2 the only way a hostname + // may begin with '[' is if it's an ipv6 address. + if (offset == 0 || *(absolute_url.data() + offset - 1) != '[') { + return false; + } + // Start one character sooner and end one character later. + offset--; + len += 2; + // HTTP parser ensures that any [ has a closing ] + ASSERT(absolute_url.length() >= offset + len); + return true; +} + +bool Utility::Url::initialize(absl::string_view absolute_url, bool is_connect) { struct http_parser_url u; - const bool is_connect = false; http_parser_url_init(&u); const int result = http_parser_parse_url(absolute_url.data(), absolute_url.length(), is_connect, &u); @@ -202,21 +225,27 @@ bool Utility::Url::initialize(absl::string_view absolute_url) { scheme_ = absl::string_view(absolute_url.data() + u.field_data[UF_SCHEMA].off, u.field_data[UF_SCHEMA].len); - uint16_t authority_len = u.field_data[UF_HOST].len; + uint64_t authority_len = u.field_data[UF_HOST].len; if ((u.field_set & (1 << UF_PORT)) == (1 << UF_PORT)) { authority_len = authority_len + u.field_data[UF_PORT].len + 1; } - host_and_port_ = - absl::string_view(absolute_url.data() + u.field_data[UF_HOST].off, authority_len); + + uint64_t authority_beginning = u.field_data[UF_HOST].off; + const bool is_ipv6 = maybeAdjustForIpv6(absolute_url, authority_beginning, authority_len); + host_and_port_ = absl::string_view(absolute_url.data() + authority_beginning, authority_len); + if (is_ipv6 && !parseAuthority(host_and_port_).is_ip_address_) { + return false; + } // RFC allows the absolute-uri to not end in /, but the absolute path form - // must start with - uint64_t path_len = - absolute_url.length() - (u.field_data[UF_HOST].off + host_and_port().length()); - if (path_len > 0) { - uint64_t path_beginning = u.field_data[UF_HOST].off + host_and_port().length(); - path_and_query_params_ = absl::string_view(absolute_url.data() + path_beginning, path_len); - } else { + // must start with. Determine if there's a non-zero path, and if so determine + // the length of the path, query params etc. + uint64_t path_etc_len = absolute_url.length() - (authority_beginning + hostAndPort().length()); + if (path_etc_len > 0) { + uint64_t path_beginning = authority_beginning + hostAndPort().length(); + path_and_query_params_ = absl::string_view(absolute_url.data() + path_beginning, path_etc_len); + } else if (!is_connect) { + ASSERT((u.field_set & (1 << UF_PATH)) == 0); path_and_query_params_ = absl::string_view(kDefaultPath, 1); } return true; diff --git a/source/common/http/utility.h b/source/common/http/utility.h index 2daa237894d17..bf34940a4a143 100644 --- a/source/common/http/utility.h +++ b/source/common/http/utility.h @@ -101,10 +101,10 @@ namespace Utility { */ class Url { public: - bool initialize(absl::string_view absolute_url); + bool initialize(absl::string_view absolute_url, bool is_connect); absl::string_view scheme() { return scheme_; } - absl::string_view host_and_port() { return host_and_port_; } - absl::string_view path_and_query_params() { return path_and_query_params_; } + absl::string_view hostAndPort() { return host_and_port_; } + absl::string_view pathAndQueryParams() { return path_and_query_params_; } private: absl::string_view scheme_; diff --git a/source/common/router/router.cc b/source/common/router/router.cc index 4171f18440be7..f2a75ddb998f3 100644 --- a/source/common/router/router.cc +++ b/source/common/router/router.cc @@ -70,7 +70,7 @@ bool convertRequestHeadersForInternalRedirect(Http::RequestHeaderMap& downstream } Http::Utility::Url absolute_url; - if (!absolute_url.initialize(internal_redirect.value().getStringView())) { + if (!absolute_url.initialize(internal_redirect.value().getStringView(), false)) { return false; } @@ -105,8 +105,8 @@ bool convertRequestHeadersForInternalRedirect(Http::RequestHeaderMap& downstream // Replace the original host, scheme and path. downstream_headers.setScheme(absolute_url.scheme()); - downstream_headers.setHost(absolute_url.host_and_port()); - downstream_headers.setPath(absolute_url.path_and_query_params()); + downstream_headers.setHost(absolute_url.hostAndPort()); + downstream_headers.setPath(absolute_url.pathAndQueryParams()); return true; } diff --git a/source/extensions/filters/http/csrf/csrf_filter.cc b/source/extensions/filters/http/csrf/csrf_filter.cc index d18f95de35df0..396dc056c87de 100644 --- a/source/extensions/filters/http/csrf/csrf_filter.cc +++ b/source/extensions/filters/http/csrf/csrf_filter.cc @@ -35,8 +35,8 @@ bool isModifyMethod(const Http::RequestHeaderMap& headers) { absl::string_view hostAndPort(const Http::HeaderEntry* header) { Http::Utility::Url absolute_url; if (header != nullptr && !header->value().empty()) { - if (absolute_url.initialize(header->value().getStringView())) { - return absolute_url.host_and_port(); + if (absolute_url.initialize(header->value().getStringView(), false)) { + return absolute_url.hostAndPort(); } return header->value().getStringView(); } diff --git a/source/extensions/quic_listeners/quiche/platform/quic_hostname_utils_impl.cc b/source/extensions/quic_listeners/quiche/platform/quic_hostname_utils_impl.cc index 4b7b40744977a..bcbafb56639ed 100644 --- a/source/extensions/quic_listeners/quiche/platform/quic_hostname_utils_impl.cc +++ b/source/extensions/quic_listeners/quiche/platform/quic_hostname_utils_impl.cc @@ -23,7 +23,7 @@ bool QuicHostnameUtilsImpl::IsValidSNI(quiche::QuicheStringPiece sni) { // TODO(wub): Implement it on top of GoogleUrl, once it is available. return sni.find_last_of('.') != std::string::npos && - Envoy::Http::Utility::Url().initialize(absl::StrCat("http://", sni)); + Envoy::Http::Utility::Url().initialize(absl::StrCat("http://", sni), false); } // static diff --git a/test/common/http/utility_test.cc b/test/common/http/utility_test.cc index 8c5c4d55efb62..113c291ec13e7 100644 --- a/test/common/http/utility_test.cc +++ b/test/common/http/utility_test.cc @@ -1073,74 +1073,113 @@ TEST(HttpUtility, TestRejectTeHeaderTooLong) { TEST(Url, ParsingFails) { Utility::Url url; - EXPECT_FALSE(url.initialize("")); - EXPECT_FALSE(url.initialize("foo")); - EXPECT_FALSE(url.initialize("http://")); - EXPECT_FALSE(url.initialize("random_scheme://host.com/path")); + EXPECT_FALSE(url.initialize("", false)); + EXPECT_FALSE(url.initialize("foo", false)); + EXPECT_FALSE(url.initialize("http://", false)); + EXPECT_FALSE(url.initialize("random_scheme://host.com/path", false)); + EXPECT_FALSE(url.initialize("http://www.foo.com", true)); + EXPECT_FALSE(url.initialize("foo.com", true)); + EXPECT_FALSE(url.initialize("http://[notaddress]:80/?query=param", false)); + EXPECT_FALSE(url.initialize("http://[1::z::2]:80/?query=param", false)); + EXPECT_FALSE(url.initialize("http://1.2.3.4:65536/?query=param", false)); } -void ValidateUrl(absl::string_view raw_url, absl::string_view expected_scheme, +void validateUrl(absl::string_view raw_url, absl::string_view expected_scheme, absl::string_view expected_host_port, absl::string_view expected_path) { Utility::Url url; - ASSERT_TRUE(url.initialize(raw_url)) << "Failed to initialize " << raw_url; + ASSERT_TRUE(url.initialize(raw_url, false)) << "Failed to initialize " << raw_url; EXPECT_EQ(url.scheme(), expected_scheme); - EXPECT_EQ(url.host_and_port(), expected_host_port); - EXPECT_EQ(url.path_and_query_params(), expected_path); + EXPECT_EQ(url.hostAndPort(), expected_host_port); + EXPECT_EQ(url.pathAndQueryParams(), expected_path); +} + +void validateConnectUrl(absl::string_view raw_url) { + Utility::Url url; + ASSERT_TRUE(url.initialize(raw_url, true)) << "Failed to initialize " << raw_url; + EXPECT_TRUE(url.scheme().empty()); + EXPECT_TRUE(url.pathAndQueryParams().empty()); + EXPECT_EQ(url.hostAndPort(), raw_url); +} + +void invalidConnectUrl(absl::string_view raw_url) { + Utility::Url url; + ASSERT_FALSE(url.initialize(raw_url, true)) << "Unexpectedly initialized " << raw_url; } TEST(Url, ParsingTest) { // Test url with no explicit path (with and without port) - ValidateUrl("http://www.host.com", "http", "www.host.com", "/"); - ValidateUrl("http://www.host.com:80", "http", "www.host.com:80", "/"); + validateUrl("http://www.host.com", "http", "www.host.com", "/"); + validateUrl("http://www.host.com:80", "http", "www.host.com:80", "/"); // Test url with "/" path. - ValidateUrl("http://www.host.com:80/", "http", "www.host.com:80", "/"); - ValidateUrl("http://www.host.com/", "http", "www.host.com", "/"); + validateUrl("http://www.host.com:80/", "http", "www.host.com:80", "/"); + validateUrl("http://www.host.com/", "http", "www.host.com", "/"); // Test url with "?". - ValidateUrl("http://www.host.com:80/?", "http", "www.host.com:80", "/?"); - ValidateUrl("http://www.host.com/?", "http", "www.host.com", "/?"); + validateUrl("http://www.host.com:80/?", "http", "www.host.com:80", "/?"); + validateUrl("http://www.host.com/?", "http", "www.host.com", "/?"); // Test url with "?" but without slash. - ValidateUrl("http://www.host.com:80?", "http", "www.host.com:80", "?"); - ValidateUrl("http://www.host.com?", "http", "www.host.com", "?"); + validateUrl("http://www.host.com:80?", "http", "www.host.com:80", "?"); + validateUrl("http://www.host.com?", "http", "www.host.com", "?"); // Test url with multi-character path - ValidateUrl("http://www.host.com:80/path", "http", "www.host.com:80", "/path"); - ValidateUrl("http://www.host.com/path", "http", "www.host.com", "/path"); + validateUrl("http://www.host.com:80/path", "http", "www.host.com:80", "/path"); + validateUrl("http://www.host.com/path", "http", "www.host.com", "/path"); // Test url with multi-character path and ? at the end - ValidateUrl("http://www.host.com:80/path?", "http", "www.host.com:80", "/path?"); - ValidateUrl("http://www.host.com/path?", "http", "www.host.com", "/path?"); + validateUrl("http://www.host.com:80/path?", "http", "www.host.com:80", "/path?"); + validateUrl("http://www.host.com/path?", "http", "www.host.com", "/path?"); // Test https scheme - ValidateUrl("https://www.host.com", "https", "www.host.com", "/"); + validateUrl("https://www.host.com", "https", "www.host.com", "/"); // Test url with query parameter - ValidateUrl("http://www.host.com:80/?query=param", "http", "www.host.com:80", "/?query=param"); - ValidateUrl("http://www.host.com/?query=param", "http", "www.host.com", "/?query=param"); + validateUrl("http://www.host.com:80/?query=param", "http", "www.host.com:80", "/?query=param"); + validateUrl("http://www.host.com/?query=param", "http", "www.host.com", "/?query=param"); + + // Test with an ipv4 host address. + validateUrl("http://1.2.3.4/?query=param", "http", "1.2.3.4", "/?query=param"); + validateUrl("http://1.2.3.4:80/?query=param", "http", "1.2.3.4:80", "/?query=param"); + + // Test with an ipv6 address + validateUrl("http://[1::2:3]/?query=param", "http", "[1::2:3]", "/?query=param"); + validateUrl("http://[1::2:3]:80/?query=param", "http", "[1::2:3]:80", "/?query=param"); // Test url with query parameter but without slash - ValidateUrl("http://www.host.com:80?query=param", "http", "www.host.com:80", "?query=param"); - ValidateUrl("http://www.host.com?query=param", "http", "www.host.com", "?query=param"); + validateUrl("http://www.host.com:80?query=param", "http", "www.host.com:80", "?query=param"); + validateUrl("http://www.host.com?query=param", "http", "www.host.com", "?query=param"); // Test url with multi-character path and query parameter - ValidateUrl("http://www.host.com:80/path?query=param", "http", "www.host.com:80", + validateUrl("http://www.host.com:80/path?query=param", "http", "www.host.com:80", "/path?query=param"); - ValidateUrl("http://www.host.com/path?query=param", "http", "www.host.com", "/path?query=param"); + validateUrl("http://www.host.com/path?query=param", "http", "www.host.com", "/path?query=param"); // Test url with multi-character path and more than one query parameter - ValidateUrl("http://www.host.com:80/path?query=param&query2=param2", "http", "www.host.com:80", + validateUrl("http://www.host.com:80/path?query=param&query2=param2", "http", "www.host.com:80", "/path?query=param&query2=param2"); - ValidateUrl("http://www.host.com/path?query=param&query2=param2", "http", "www.host.com", + validateUrl("http://www.host.com/path?query=param&query2=param2", "http", "www.host.com", "/path?query=param&query2=param2"); // Test url with multi-character path, more than one query parameter and fragment - ValidateUrl("http://www.host.com:80/path?query=param&query2=param2#fragment", "http", + validateUrl("http://www.host.com:80/path?query=param&query2=param2#fragment", "http", "www.host.com:80", "/path?query=param&query2=param2#fragment"); - ValidateUrl("http://www.host.com/path?query=param&query2=param2#fragment", "http", "www.host.com", + validateUrl("http://www.host.com/path?query=param&query2=param2#fragment", "http", "www.host.com", "/path?query=param&query2=param2#fragment"); } +TEST(Url, ParsingForConnectTest) { + validateConnectUrl("host.com:443"); + validateConnectUrl("host.com:80"); + validateConnectUrl("1.2.3.4:80"); + validateConnectUrl("[1:2::3:4]:80"); + + invalidConnectUrl("[::12345678]:80"); + invalidConnectUrl("[1:1:1:1:1:1:1:1:1:1:1:1:1:1:1:1:1:1:1:1:1:1:1:1:1:1:1:1:1]:80"); + invalidConnectUrl("[1:1]:80"); + invalidConnectUrl("[:::]:80"); + invalidConnectUrl("[::1::]:80"); +} + void validatePercentEncodingEncodeDecode(absl::string_view source, absl::string_view expected_encoded) { EXPECT_EQ(Utility::PercentEncoding::encode(source), expected_encoded); diff --git a/test/config/integration/certs/upstreamcert.cfg b/test/config/integration/certs/upstreamcert.cfg index 4e2e760a861cb..8d10db12dbdd6 100644 --- a/test/config/integration/certs/upstreamcert.cfg +++ b/test/config/integration/certs/upstreamcert.cfg @@ -35,4 +35,4 @@ authorityKeyIdentifier = keyid:always [alt_names] DNS.1 = *.lyft.com IP.1 = 127.0.0.1 -IP.2 = ::1 \ No newline at end of file +IP.2 = ::1 diff --git a/test/extensions/filters/http/dynamic_forward_proxy/proxy_filter_integration_test.cc b/test/extensions/filters/http/dynamic_forward_proxy/proxy_filter_integration_test.cc index aba6b60a649db..cbd1470515909 100644 --- a/test/extensions/filters/http/dynamic_forward_proxy/proxy_filter_integration_test.cc +++ b/test/extensions/filters/http/dynamic_forward_proxy/proxy_filter_integration_test.cc @@ -278,8 +278,7 @@ TEST_P(ProxyFilterIntegrationTest, UpstreamTlsWithIpHost) { {":method", "POST"}, {":path", "/test/long/url"}, {":scheme", "http"}, - {":authority", fmt::format("{}:{}", Network::Test::getLoopbackAddressUrlString(GetParam()), - fake_upstreams_[0]->localAddress()->ip()->port())}}; + {":authority", fake_upstreams_[0]->localAddress()->asString()}}; auto response = codec_client_->makeHeaderOnlyRequest(request_headers); waitForNextUpstreamRequest(); diff --git a/test/integration/integration_test.cc b/test/integration/integration_test.cc index c18599476f5f5..199c8147f8567 100644 --- a/test/integration/integration_test.cc +++ b/test/integration/integration_test.cc @@ -721,6 +721,41 @@ TEST_P(IntegrationTest, AbsolutePath) { EXPECT_FALSE(response.find("HTTP/1.1 404 Not Found\r\n") == 0); } +// Make that both IPv4 and IPv6 hosts match when using relative and absolute URLs. +TEST_P(IntegrationTest, TestHostWithAddress) { + useAccessLog("%REQ(Host)%\n"); + std::string address_string; + if (GetParam() == Network::Address::IpVersion::v4) { + address_string = TestUtility::getIpv4Loopback(); + } else { + address_string = "[::1]"; + } + + auto host = config_helper_.createVirtualHost(address_string.c_str(), "/"); + host.set_require_tls(envoy::config::route::v3::VirtualHost::ALL); + config_helper_.addVirtualHost(host); + + initialize(); + std::string response; + + // Test absolute URL with ipv6. + sendRawHttpAndWaitForResponse( + lookupPort("http"), absl::StrCat("GET http://", address_string, " HTTP/1.1\r\n\r\n").c_str(), + &response, true); + EXPECT_FALSE(response.find("HTTP/1.1 404 Not Found\r\n") == 0); + EXPECT_TRUE(response.find("301") != std::string::npos); + EXPECT_THAT(waitForAccessLog(access_log_name_), HasSubstr(address_string)); + + // Test normal IPv6 request as well. + response.clear(); + sendRawHttpAndWaitForResponse( + lookupPort("http"), + absl::StrCat("GET / HTTP/1.1\r\nHost: ", address_string, "\r\n\r\n").c_str(), &response, + true); + EXPECT_FALSE(response.find("HTTP/1.1 404 Not Found\r\n") == 0); + EXPECT_TRUE(response.find("301") != std::string::npos); +} + TEST_P(IntegrationTest, AbsolutePathWithPort) { // Configure www.namewithport.com:1234 to send a redirect, and ensure the redirect is // encountered via absolute URL with a port. @@ -733,6 +768,7 @@ TEST_P(IntegrationTest, AbsolutePathWithPort) { lookupPort("http"), "GET http://www.namewithport.com:1234 HTTP/1.1\r\nHost: host\r\n\r\n", &response, true); EXPECT_FALSE(response.find("HTTP/1.1 404 Not Found\r\n") == 0); + EXPECT_TRUE(response.find("301") != std::string::npos); } TEST_P(IntegrationTest, AbsolutePathWithoutPort) {