From f6bacd3fde9d4f0de638dc2bb3ba2ac2520df874 Mon Sep 17 00:00:00 2001 From: melanie witt Date: Thu, 16 Jul 2020 00:55:25 +0000 Subject: [PATCH] Refactor and rename test_tcp_rst_no_compute_rpcapi The NovaProxyRequestHandlerTestCase.test_tcp_rst_no_compute_rpcapi test fails with mock==4.0.2 because mock is calling the NovaProxyRequestHandler class's compute_rpcapi @property while creating mock autospecs. The test asserts that the @property is not called when a TCP RST is received, so mock calling the @property itself causes the test to fail erroneously. This is also the case in the built-in unittest.mock in python 3.8 [1]. We can refactor (and rename) this unit test to more concisely test the desired behavior when TCP RST or otherwise unvalidated requests are handled by the console proxy request handler. This has the benefit of (1) making the test work with mock==4.0.2 and python 3.8 and (2) removing unnecessary dependency and potentially incorrect assumptions about the internal details of websockify. Closes-Bug: #1887735 [1] https://bugs.python.org/issue41768 Change-Id: I58b0382c86d4ef798572edb63d311e0e3e6937bb --- .../tests/unit/console/test_websocketproxy.py | 40 ++++++++++++++----- 1 file changed, 31 insertions(+), 9 deletions(-) diff --git a/nova/tests/unit/console/test_websocketproxy.py b/nova/tests/unit/console/test_websocketproxy.py index 3c234df891..33351abbca 100644 --- a/nova/tests/unit/console/test_websocketproxy.py +++ b/nova/tests/unit/console/test_websocketproxy.py @@ -616,15 +616,37 @@ class NovaProxyRequestHandlerTestCase(test.NoDBTestCase): self.wh.socket.assert_called_with('node1', 10000, connect=True) self.wh.do_proxy.assert_called_with('') - def test_tcp_rst_no_compute_rpcapi(self): - # Tests that we don't create a ComputeAPI object if we receive a - # TCP RST message. Simulate by raising the socket.err upon recv. - err = socket.error('[Errno 104] Connection reset by peer') - self.wh.socket.recv.side_effect = err - conn = mock.MagicMock() - address = mock.MagicMock() - self.wh.server.top_new_client(conn, address) - self.assertIsNone(self.wh._compute_rpcapi) + @mock.patch('nova.objects.ConsoleAuthToken.validate') + def test_no_compute_rpcapi_with_invalid_token(self, mock_validate): + """Tests that we don't create a ComputeAPI object until we actually + need to use it to call the internal compute RPC API after token + validation succeeds. This way, we will not perform expensive object + creations when we receive unauthenticated (via token) messages. In the + past, it was possible for unauthenticated requests such as TCP RST or + requests with invalid tokens to be used to DOS the console proxy + service. + """ + # We will simulate a request with an invalid token and verify it + # will not trigger a ComputeAPI object creation. + mock_req = mock.MagicMock() + mock_req.makefile().readline.side_effect = [ + b'GET /vnc.html?token=123-456-789 HTTP/1.1\r\n', + b'' + ] + client_addr = ('8.8.8.8', 54321) + mock_server = mock.MagicMock() + handler = websocketproxy.NovaProxyRequestHandler( + mock_req, client_addr, mock_server) + # Internal ComputeAPI reference should be None when the request handler + # is initially created. + self.assertIsNone(handler._compute_rpcapi) + # Set up a token validation to fail when the new_websocket_client + # is called to handle the request. + mock_validate.side_effect = exception.InvalidToken(token='123-456-789') + # We expect InvalidToken to be raised during handling. + self.assertRaises(exception.InvalidToken, handler.new_websocket_client) + # And our internal ComputeAPI reference should still be None. + self.assertIsNone(handler._compute_rpcapi) @mock.patch('websockify.websocketproxy.select_ssl_version') def test_ssl_min_version_is_not_set(self, mock_select_ssl):