backport 1.13: http: fixing a bug with IPv6 hosts

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)
 =========================

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");
   }

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;

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_; }

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;
   }
 

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();

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

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);

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
+IP.2 = ::1

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();

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) {