From 5aefa86a1830179e7e44466b5f5db71223530aa8 Mon Sep 17 00:00:00 2001 From: Tim Burke Date: Wed, 24 Jun 2020 10:40:51 -0700 Subject: [PATCH] py3: Stop munging RAW_PATH_INFO We rely on having byte-accurate representations of the request path as-seen-on-the-wire to compute signatures in s3api; the unquoting / requoting madness to make non-ascii paths work with python's stdlib can lead to erroneous SignatureDoesNotMatch responses. Change-Id: I87fe3477d8b7ef186421ef2d08bc3b205c18a0c1 Closes-Bug: #1884991 --- swift/common/wsgi.py | 12 +++-- test/unit/common/test_wsgi.py | 99 +++++++++++++++++++++++++---------- 2 files changed, 81 insertions(+), 30 deletions(-) diff --git a/swift/common/wsgi.py b/swift/common/wsgi.py index 535b01f891..f9cfd1f2fc 100644 --- a/swift/common/wsgi.py +++ b/swift/common/wsgi.py @@ -452,17 +452,21 @@ class SwiftHttpProtocol(wsgi.HttpProtocol): return '' def parse_request(self): + # Need to track the bytes-on-the-wire for S3 signatures -- eventlet + # would do it for us, but since we rewrite the path on py3, we need to + # fix it ourselves later. + self.__raw_path_info = None + if not six.PY2: # request lines *should* be ascii per the RFC, but historically # we've allowed (and even have func tests that use) arbitrary # bytes. This breaks on py3 (see https://bugs.python.org/issue33973 # ) but the work-around is simple: munge the request line to be - # properly quoted. py2 will do the right thing without this, but it - # doesn't hurt to re-write the request line like this and it - # simplifies testing. + # properly quoted. if self.raw_requestline.count(b' ') >= 2: parts = self.raw_requestline.split(b' ', 2) path, q, query = parts[1].partition(b'?') + self.__raw_path_info = path # unquote first, so we don't over-quote something # that was *correctly* quoted path = wsgi_to_bytes(wsgi_quote(wsgi_unquote( @@ -484,6 +488,8 @@ class SwiftHttpProtocol(wsgi.HttpProtocol): if not six.PY2: def get_environ(self, *args, **kwargs): environ = wsgi.HttpProtocol.get_environ(self, *args, **kwargs) + environ['RAW_PATH_INFO'] = bytes_to_wsgi( + self.__raw_path_info) header_payload = self.headers.get_payload() if isinstance(header_payload, list) and len(header_payload) == 1: header_payload = header_payload[0].get_payload() diff --git a/test/unit/common/test_wsgi.py b/test/unit/common/test_wsgi.py index f2f71da001..da04d21f3a 100644 --- a/test/unit/common/test_wsgi.py +++ b/test/unit/common/test_wsgi.py @@ -40,7 +40,7 @@ import swift.proxy.server import swift.obj.server as obj_server import swift.container.server as container_server import swift.account.server as account_server -from swift.common.swob import Request +from swift.common.swob import Request, wsgi_to_bytes from swift.common import wsgi, utils from swift.common.storage_policy import POLICIES @@ -1086,8 +1086,8 @@ class TestSwiftHttpProtocol(unittest.TestCase): b'POST /?and+it=fixes+params&PALMTREE=%F0%9F%8C%B4 HTTP/1.1') -class TestProxyProtocol(unittest.TestCase): - def _run_bytes_through_protocol(self, bytes_from_client, protocol_class): +class ProtocolTest(unittest.TestCase): + def _run_bytes_through_protocol(self, bytes_from_client): rfile = BytesIO(bytes_from_client) wfile = BytesIO() @@ -1110,21 +1110,6 @@ class TestProxyProtocol(unittest.TestCase): def waitall(self): pass - def dinky_app(env, start_response): - start_response("200 OK", []) - body = '\r\n'.join([ - 'got addr: %s %s' % ( - env.get("REMOTE_ADDR", ""), - env.get("REMOTE_PORT", "")), - 'on addr: %s %s' % ( - env.get("SERVER_ADDR", ""), - env.get("SERVER_PORT", "")), - 'https is %s (scheme %s)' % ( - env.get("HTTPS", ""), - env.get("wsgi.url_scheme", "")), - ]) + '\r\n' - return [body.encode("utf-8")] - addr = ('127.0.0.1', 8359) fake_tcp_socket = mock.Mock( setsockopt=lambda *a: None, @@ -1145,20 +1130,81 @@ class TestProxyProtocol(unittest.TestCase): with mock.patch.object(wfile, 'close', lambda: None), \ mock.patch.object(rfile, 'close', lambda: None): eventlet.wsgi.server( - fake_listen_socket, dinky_app, - protocol=protocol_class, + fake_listen_socket, self.app, + protocol=self.protocol_class, custom_pool=FakePool(), log_output=False, # quiet the test run ) return wfile.getvalue() + +class TestSwiftHttpProtocolSomeMore(ProtocolTest): + protocol_class = wsgi.SwiftHttpProtocol + + @staticmethod + def app(env, start_response): + start_response("200 OK", []) + return [wsgi_to_bytes(env['RAW_PATH_INFO'])] + + def test_simple(self): + bytes_out = self._run_bytes_through_protocol(( + b"GET /someurl HTTP/1.0\r\n" + b"User-Agent: something or other\r\n" + b"\r\n" + )) + + lines = [l for l in bytes_out.split(b"\r\n") if l] + self.assertEqual(lines[0], b"HTTP/1.1 200 OK") # sanity check + self.assertEqual(lines[-1], b'/someurl') + + def test_quoted(self): + bytes_out = self._run_bytes_through_protocol(( + b"GET /some%fFpath%D8%AA HTTP/1.0\r\n" + b"User-Agent: something or other\r\n" + b"\r\n" + )) + + lines = [l for l in bytes_out.split(b"\r\n") if l] + self.assertEqual(lines[0], b"HTTP/1.1 200 OK") # sanity check + self.assertEqual(lines[-1], b'/some%fFpath%D8%AA') + + def test_messy(self): + bytes_out = self._run_bytes_through_protocol(( + b"GET /oh\xffboy%what$now%E2%80%bd HTTP/1.0\r\n" + b"User-Agent: something or other\r\n" + b"\r\n" + )) + + lines = [l for l in bytes_out.split(b"\r\n") if l] + self.assertEqual(lines[-1], b'/oh\xffboy%what$now%E2%80%bd') + + +class TestProxyProtocol(ProtocolTest): + protocol_class = wsgi.SwiftHttpProxiedProtocol + + @staticmethod + def app(env, start_response): + start_response("200 OK", []) + body = '\r\n'.join([ + 'got addr: %s %s' % ( + env.get("REMOTE_ADDR", ""), + env.get("REMOTE_PORT", "")), + 'on addr: %s %s' % ( + env.get("SERVER_ADDR", ""), + env.get("SERVER_PORT", "")), + 'https is %s (scheme %s)' % ( + env.get("HTTPS", ""), + env.get("wsgi.url_scheme", "")), + ]) + '\r\n' + return [body.encode("utf-8")] + def test_request_with_proxy(self): bytes_out = self._run_bytes_through_protocol(( b"PROXY TCP4 192.168.0.1 192.168.0.11 56423 4433\r\n" b"GET /someurl HTTP/1.0\r\n" b"User-Agent: something or other\r\n" b"\r\n" - ), wsgi.SwiftHttpProxiedProtocol) + )) lines = [l for l in bytes_out.split(b"\r\n") if l] self.assertEqual(lines[0], b"HTTP/1.1 200 OK") # sanity check @@ -1174,7 +1220,7 @@ class TestProxyProtocol(unittest.TestCase): b"GET /someurl HTTP/1.0\r\n" b"User-Agent: something or other\r\n" b"\r\n" - ), wsgi.SwiftHttpProxiedProtocol) + )) lines = [l for l in bytes_out.split(b"\r\n") if l] self.assertEqual(lines[0], b"HTTP/1.1 200 OK") # sanity check @@ -1194,7 +1240,7 @@ class TestProxyProtocol(unittest.TestCase): b"User-Agent: something or other\r\n" b"Connection: close\r\n" b"\r\n" - ), wsgi.SwiftHttpProxiedProtocol) + )) lines = bytes_out.split(b"\r\n") self.assertEqual(lines[0], b"HTTP/1.1 200 OK") # sanity check @@ -1213,7 +1259,7 @@ class TestProxyProtocol(unittest.TestCase): b"GET /someurl HTTP/1.0\r\n" b"User-Agent: something or other\r\n" b"\r\n" - ), wsgi.SwiftHttpProxiedProtocol) + )) lines = [l for l in bytes_out.split(b"\r\n") if l] self.assertIn(b"400 Invalid PROXY line", lines[0]) @@ -1223,8 +1269,7 @@ class TestProxyProtocol(unittest.TestCase): b'PROXYjojo a b c d e', b'PROXY a b c d e', # bad INET protocol and family ]: - bytes_out = self._run_bytes_through_protocol( - bad_line, wsgi.SwiftHttpProxiedProtocol) + bytes_out = self._run_bytes_through_protocol(bad_line) lines = [l for l in bytes_out.split(b"\r\n") if l] self.assertIn(b"400 Invalid PROXY line", lines[0]) @@ -1240,7 +1285,7 @@ class TestProxyProtocol(unittest.TestCase): b"GET /someurl HTTP/1.0\r\n" b"User-Agent: something or other\r\n" b"\r\n") - ), wsgi.SwiftHttpProxiedProtocol) + )) lines = [l for l in bytes_out.split(b"\r\n") if l] self.assertIn(b"200 OK", lines[0])