urlsplit compliance for https CONNECT (#360)

* Add fix to be complaint with urlparse.urlsplit semantics which expects a fully qualified URL

* Log exception stacktrace on OSError

Author: abhinavsingh

proxy/http/parser.py

@@ -105,27 +105,29 @@ def del_headers(self, headers: List[bytes]) -> None:
             self.del_header(key.lower())
 
     def set_url(self, url: bytes) -> None:
+        # Work around with urlsplit semantics.
+        #
+        # For CONNECT requests, request line contains
+        # upstream_host:upstream_port which is not complaint
+        # with urlsplit, which expects a fully qualified url.
+        if self.method == b'CONNECT':
+            url = b'https://' + url
         self.url = urlparse.urlsplit(url)
         self.set_line_attributes()
 
     def set_line_attributes(self) -> None:
         if self.type == httpParserTypes.REQUEST_PARSER:
             if self.method == httpMethods.CONNECT and self.url:
-                if self.url.scheme == b'':
-                    u = urlparse.urlsplit(b'//' + self.url.path)
-                    self.host, self.port = u.hostname, u.port
-                else:
-                    self.host = self.url.scheme
-                    self.port = 443 if self.url.path == b'' else \
-                        int(self.url.path)
+                self.host = self.url.hostname
+                self.port = 443 if self.url.port is None else self.url.port
             elif self.url:
                 self.host, self.port = self.url.hostname, self.url.port \
                     if self.url.port else DEFAULT_HTTP_PORT
             else:
                 raise KeyError(
                     'Invalid request. Method: %r, Url: %r' %
                     (self.method, self.url))
-            self.path = self.build_url()
+            self.path = self.build_path()
 
     def is_chunked_encoded(self) -> bool:
         return b'transfer-encoding' in self.headers and \
@@ -222,7 +224,7 @@ def process_header(self, raw: bytes) -> None:
         value = COLON.join(parts[1:]).strip()
         self.add_headers([(key, value)])
 
-    def build_url(self) -> bytes:
+    def build_path(self) -> bytes:
         if not self.url:
             return b'/None'
         url = self.url.path

proxy/http/proxy/server.py

@@ -278,8 +278,8 @@ def on_request_complete(self) -> Union[socket.socket, bool]:
                     logger.error(
                         'BrokenPipeError when wrapping client')
                     return True
-                except OSError:
-                    logger.error('OSError when wrapping client')
+                except OSError as e:
+                    logger.exception('OSError when wrapping client', exc_info=e)
                     return True
                 # Update all plugin connection reference
                 for plugin in self.plugins.values():
@@ -398,8 +398,8 @@ def wrap_client(self) -> None:
         self.client._conn = ssl.wrap_socket(
             self.client.connection,
             server_side=True,
-            keyfile=self.flags.ca_signing_key_file,
-            certfile=generated_cert)
+            certfile=generated_cert,
+            keyfile=self.flags.ca_signing_key_file)
         self.client.connection.setblocking(False)
         logger.debug(
             'TLS interception using %s', generated_cert)

requirements-testing.txt

@@ -10,3 +10,4 @@ codecov==2.1.4
 tox==3.15.1
 mccabe==0.6.1
 pylint==2.5.2
+rope==0.17.0

tests/http/test_http_parser.py

@@ -132,7 +132,7 @@ def test_get_full_parse(self) -> None:
                      b'example.com')
         self.parser.parse(pkt)
         self.assertEqual(self.parser.total_size, len(pkt))
-        self.assertEqual(self.parser.build_url(), b'/path/dir/?a=b&c=d#p=q')
+        self.assertEqual(self.parser.build_path(), b'/path/dir/?a=b&c=d#p=q')
         self.assertEqual(self.parser.method, b'GET')
         assert self.parser.url
         self.assertEqual(self.parser.url.hostname, b'example.com')
@@ -149,7 +149,7 @@ def test_get_full_parse(self) -> None:
             self.parser.build())
 
     def test_build_url_none(self) -> None:
-        self.assertEqual(self.parser.build_url(), b'/None')
+        self.assertEqual(self.parser.build_path(), b'/None')
 
     def test_line_rcvd_to_rcving_headers_state_change(self) -> None:
         pkt = b'GET http://localhost HTTP/1.1'