diff --git a/docs/root/intro/version_history.rst b/docs/root/intro/version_history.rst index c24bf1bd675a3..bc03bce5c0d87 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.13.7 (December 7, 2020) ========================= diff --git a/source/common/http/http1/codec_impl.cc b/source/common/http/http1/codec_impl.cc index 358b2dd074762..9a43018e6ebb4 100644 --- a/source/common/http/http1/codec_impl.cc +++ b/source/common/http/http1/codec_impl.cc @@ -792,7 +792,7 @@ void ServerConnectionImpl::handlePath(HeaderMapImpl& headers, unsigned int metho } 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"); } diff --git a/source/common/http/utility.cc b/source/common/http/utility.cc index 6d160f98c1e5a..a6310a5f30b6d 100644 --- a/source/common/http/utility.cc +++ b/source/common/http/utility.cc @@ -35,9 +35,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); @@ -52,21 +75,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 + host_and_port().length()); + if (path_etc_len > 0) { + uint64_t path_beginning = authority_beginning + host_and_port().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 9b6f9ecbccc1f..8eb048404d97e 100644 --- a/source/common/http/utility.h +++ b/source/common/http/utility.h @@ -28,7 +28,7 @@ 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_; } diff --git a/source/common/router/router.cc b/source/common/router/router.cc index 721eaa0b25660..06316e1ef3457 100644 --- a/source/common/router/router.cc +++ b/source/common/router/router.cc @@ -68,7 +68,7 @@ bool convertRequestHeadersForInternalRedirect(Http::HeaderMap& downstream_header } Http::Utility::Url absolute_url; - if (!absolute_url.initialize(internal_redirect.value().getStringView())) { + if (!absolute_url.initialize(internal_redirect.value().getStringView(), false)) { return false; } diff --git a/source/extensions/filters/http/csrf/csrf_filter.cc b/source/extensions/filters/http/csrf/csrf_filter.cc index 1f113b60b3f0b..697869b6a4fa1 100644 --- a/source/extensions/filters/http/csrf/csrf_filter.cc +++ b/source/extensions/filters/http/csrf/csrf_filter.cc @@ -35,7 +35,7 @@ bool isModifyMethod(const Http::HeaderMap& 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())) { + if (absolute_url.initialize(header->value().getStringView(), false)) { return absolute_url.host_and_port(); } 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 b967bdb90b584..8d51021f7dfcc 100644 --- a/test/common/http/utility_test.cc +++ b/test/common/http/utility_test.cc @@ -1063,21 +1063,39 @@ 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, 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); } +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.path_and_query_params().empty()); + EXPECT_EQ(url.host_and_port(), 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", "/"); @@ -1110,6 +1128,14 @@ TEST(Url, ParsingTest) { 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"); @@ -1131,6 +1157,19 @@ TEST(Url, ParsingTest) { "/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 91d06ed17c4b7..afefeadf768ae 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 cee054a9291c9..a86ff9e0f3e69 100644 --- a/test/integration/integration_test.cc +++ b/test/integration/integration_test.cc @@ -711,6 +711,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. @@ -723,6 +758,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) {