From 54125a75fb056dbea115408610f90e7d6eee5139 Mon Sep 17 00:00:00 2001 From: melanie witt Date: Fri, 27 Sep 2019 02:10:22 +0000 Subject: [PATCH] Reduce scope of 'path' query parameter to noVNC consoles This is a partial revert of commit 9606c80402f6db20d62b689c58aa8f024183628a which added the 'path' query parameter to work with noVNC v1.1.0. This broke all other console types using websockify server (serial, spice) because the websockify server itself doesn't know how to handle the 'path' query parameter. It is the noVNC vnc_lite.html file which parses the 'path' variable and uses it as the url to the websockify server. So, all other console types should *not* be generating a console access url with a 'path' query parameter, only noVNC. Closes-Bug: #1845243 TODO(melwitt): Figure out how to test serial and/or spice console in the gate Change-Id: I9521f21a685edc44121d75bdf534c201fa87c2d7 --- .../get-rdp-console-post-resp.json | 4 +-- .../get-serial-console-post-resp.json | 4 +-- .../get-spice-console-post-resp.json | 2 +- nova/objects/console_auth_token.py | 14 +++++++-- .../get-rdp-console-post-resp.json.tpl | 2 +- .../get-serial-console-post-resp.json.tpl | 2 +- .../get-spice-console-post-resp.json.tpl | 2 +- .../test_console_auth_tokens.py | 4 +-- .../tests/unit/console/test_websocketproxy.py | 30 ++++++++----------- .../unit/objects/test_console_auth_token.py | 25 ++++++++++++---- 10 files changed, 53 insertions(+), 36 deletions(-) diff --git a/doc/api_samples/os-remote-consoles/get-rdp-console-post-resp.json b/doc/api_samples/os-remote-consoles/get-rdp-console-post-resp.json index 6cf16af42c8a..09f3ca3d8cc4 100644 --- a/doc/api_samples/os-remote-consoles/get-rdp-console-post-resp.json +++ b/doc/api_samples/os-remote-consoles/get-rdp-console-post-resp.json @@ -1,6 +1,6 @@ { "console": { "type": "rdp-html5", - "url": "http://127.0.0.1:6083/?path=%3Ftoken%3D21efbb20-b84c-4d1f-807d-4e14f6884b7f" + "url": "http://127.0.0.1:6083/?token=191996c3-7b0f-42f3-95a7-f1839f2da6ed" } -} \ No newline at end of file +} diff --git a/doc/api_samples/os-remote-consoles/get-serial-console-post-resp.json b/doc/api_samples/os-remote-consoles/get-serial-console-post-resp.json index f144653fafe0..990c9653cca0 100644 --- a/doc/api_samples/os-remote-consoles/get-serial-console-post-resp.json +++ b/doc/api_samples/os-remote-consoles/get-serial-console-post-resp.json @@ -1,6 +1,6 @@ { "console": { "type": "serial", - "url": "ws://127.0.0.1:6083/?path=%3Ftoken%3D6ac46b4c-2705-4d8b-baa3-1b6f1b0c7dd3" + "url":"ws://127.0.0.1:6083/?token=f9906a48-b71e-4f18-baca-c987da3ebdb3" } -} \ No newline at end of file +} diff --git a/doc/api_samples/os-remote-consoles/get-spice-console-post-resp.json b/doc/api_samples/os-remote-consoles/get-spice-console-post-resp.json index 5d2b2cfecd65..f0e09f47db99 100644 --- a/doc/api_samples/os-remote-consoles/get-spice-console-post-resp.json +++ b/doc/api_samples/os-remote-consoles/get-spice-console-post-resp.json @@ -1,6 +1,6 @@ { "console": { "type": "spice-html5", - "url": "http://127.0.0.1:6082/spice_auto.html?path=%3Ftoken%3Da7bd9607-421c-44b9-8689-18e87ada2f78" + "url": "http://127.0.0.1:6082/spice_auto.html?token=a30e5d08-6a20-4043-958f-0852440c6af4" } } \ No newline at end of file diff --git a/nova/objects/console_auth_token.py b/nova/objects/console_auth_token.py index 8dcdb610d8b9..bef5bb4c53f3 100644 --- a/nova/objects/console_auth_token.py +++ b/nova/objects/console_auth_token.py @@ -64,9 +64,17 @@ class ConsoleAuthToken(base.NovaTimestampObject, base.NovaObject): specific to this authorization. """ if self.obj_attr_is_set('id'): - qparams = {'path': '?token=%s' % self.token} - return '%s?%s' % (self.access_url_base, - urlparse.urlencode(qparams)) + if self.console_type == 'novnc': + # NOTE(melwitt): As of noVNC v1.1.0, we must use the 'path' + # query parameter to pass the auth token within, as the + # top-level 'token' query parameter was removed. The 'path' + # parameter is supported in older noVNC versions, so it is + # backward compatible. + qparams = {'path': '?token=%s' % self.token} + return '%s?%s' % (self.access_url_base, + urlparse.urlencode(qparams)) + else: + return '%s?token=%s' % (self.access_url_base, self.token) @staticmethod def _from_db_object(context, obj, db_obj): diff --git a/nova/tests/functional/api_sample_tests/api_samples/os-remote-consoles/get-rdp-console-post-resp.json.tpl b/nova/tests/functional/api_sample_tests/api_samples/os-remote-consoles/get-rdp-console-post-resp.json.tpl index cbe172ef8620..c3955d6ac0ec 100644 --- a/nova/tests/functional/api_sample_tests/api_samples/os-remote-consoles/get-rdp-console-post-resp.json.tpl +++ b/nova/tests/functional/api_sample_tests/api_samples/os-remote-consoles/get-rdp-console-post-resp.json.tpl @@ -1,6 +1,6 @@ { "console": { "type": "rdp-html5", - "url": "http://127.0.0.1:6083/?path=%%3Ftoken%%3D%(uuid)s" + "url": "http://127.0.0.1:6083/?token=%(uuid)s" } } diff --git a/nova/tests/functional/api_sample_tests/api_samples/os-remote-consoles/get-serial-console-post-resp.json.tpl b/nova/tests/functional/api_sample_tests/api_samples/os-remote-consoles/get-serial-console-post-resp.json.tpl index 384938463d92..721ce2b2ea8c 100644 --- a/nova/tests/functional/api_sample_tests/api_samples/os-remote-consoles/get-serial-console-post-resp.json.tpl +++ b/nova/tests/functional/api_sample_tests/api_samples/os-remote-consoles/get-serial-console-post-resp.json.tpl @@ -1,6 +1,6 @@ { "console": { "type": "serial", - "url": "ws://127.0.0.1:6083/?path=%%3Ftoken%%3D%(uuid)s" + "url": "ws://127.0.0.1:6083/?token=%(uuid)s" } } diff --git a/nova/tests/functional/api_sample_tests/api_samples/os-remote-consoles/get-spice-console-post-resp.json.tpl b/nova/tests/functional/api_sample_tests/api_samples/os-remote-consoles/get-spice-console-post-resp.json.tpl index 4a53a05f39ec..65b72a866ffe 100644 --- a/nova/tests/functional/api_sample_tests/api_samples/os-remote-consoles/get-spice-console-post-resp.json.tpl +++ b/nova/tests/functional/api_sample_tests/api_samples/os-remote-consoles/get-spice-console-post-resp.json.tpl @@ -1,6 +1,6 @@ { "console": { "type": "spice-html5", - "url": "http://127.0.0.1:6082/spice_auto.html?path=%%3Ftoken%%3D%(uuid)s" + "url": "http://127.0.0.1:6082/spice_auto.html?token=%(uuid)s" } } diff --git a/nova/tests/functional/api_sample_tests/test_console_auth_tokens.py b/nova/tests/functional/api_sample_tests/test_console_auth_tokens.py index b41b21b1c059..fcd79d54b6f7 100644 --- a/nova/tests/functional/api_sample_tests/test_console_auth_tokens.py +++ b/nova/tests/functional/api_sample_tests/test_console_auth_tokens.py @@ -15,7 +15,6 @@ import re from oslo_serialization import jsonutils -import six.moves.urllib.parse as urlparse from nova.tests.functional.api_sample_tests import test_servers @@ -33,8 +32,7 @@ class ConsoleAuthTokensSampleJsonTests(test_servers.ServersSampleBase): {'action': 'os-getRDPConsole'}) url = self._get_console_url(response.content) - path = urlparse.urlencode({'path': '?token='}) - return re.match('.+?%s([^&]+)' % path, url).groups()[0] + return re.match('.+?token=([^&]+)', url).groups()[0] def test_get_console_connect_info(self): self.flags(enabled=True, group='rdp') diff --git a/nova/tests/unit/console/test_websocketproxy.py b/nova/tests/unit/console/test_websocketproxy.py index 466b336b32b7..ce0c924cf411 100644 --- a/nova/tests/unit/console/test_websocketproxy.py +++ b/nova/tests/unit/console/test_websocketproxy.py @@ -19,7 +19,6 @@ import socket import mock from oslo_utils.fixture import uuidsentinel as uuids -import six.moves.urllib.parse as urlparse import nova.conf from nova.console.securityproxy import base @@ -48,7 +47,6 @@ class NovaProxyRequestHandlerDBTestCase(test.TestCase): self.wh.msg = mock.MagicMock() self.wh.do_proxy = mock.MagicMock() self.wh.headers = mock.MagicMock() - self.path = urlparse.urlencode({'path': '?token=123-456-789'}) def _fake_console_db(self, **updates): console_db = copy.deepcopy(fake_ca.fake_token_dict) @@ -97,7 +95,7 @@ class NovaProxyRequestHandlerDBTestCase(test.TestCase): tsock.recv.return_value = "HTTP/1.1 200 OK\r\n\r\n" self.wh.socket.return_value = tsock - self.wh.path = "http://127.0.0.1/?%s" % self.path + self.wh.path = "http://127.0.0.1/?token=123-456-789" self.wh.headers = self.fake_header if instance_not_found: @@ -143,8 +141,6 @@ class NovaProxyRequestHandlerBaseTestCase(test.NoDBTestCase): self.wh.msg = mock.MagicMock() self.wh.do_proxy = mock.MagicMock() self.wh.headers = mock.MagicMock() - self.path = urlparse.urlencode({'path': '?token=123-456-789'}) - self.path_invalid = urlparse.urlencode({'path': '?token=XXX'}) fake_header = { 'cookie': 'token="123-456-789"', @@ -215,7 +211,7 @@ class NovaProxyRequestHandlerBaseTestCase(test.NoDBTestCase): validate.return_value = objects.ConsoleAuthToken(**params) self.wh.socket.return_value = '' - self.wh.path = "http://127.0.0.1/?%s" % self.path + self.wh.path = "http://127.0.0.1/?token=123-456-789" self.wh.headers = self.fake_header self.wh.new_websocket_client() @@ -240,7 +236,7 @@ class NovaProxyRequestHandlerBaseTestCase(test.NoDBTestCase): validate.return_value = objects.ConsoleAuthToken(**params) self.wh.socket.return_value = '' - self.wh.path = "http://[2001:db8::1]/?%s" % self.path + self.wh.path = "http://[2001:db8::1]/?token=123-456-789" self.wh.headers = self.fake_header_ipv6 self.wh.new_websocket_client() @@ -253,7 +249,7 @@ class NovaProxyRequestHandlerBaseTestCase(test.NoDBTestCase): def test_new_websocket_client_token_invalid(self, validate): validate.side_effect = exception.InvalidToken(token='XXX') - self.wh.path = "http://127.0.0.1/?%s" % self.path_invalid + self.wh.path = "http://127.0.0.1/?token=XXX" self.wh.headers = self.fake_header_bad_token self.assertRaises(exception.InvalidToken, @@ -281,7 +277,7 @@ class NovaProxyRequestHandlerBaseTestCase(test.NoDBTestCase): tsock.recv.return_value = "HTTP/1.1 200 OK\r\n\r\n" self.wh.socket.return_value = tsock - self.wh.path = "http://127.0.0.1/?%s" % self.path + self.wh.path = "http://127.0.0.1/?token=123-456-789" self.wh.headers = self.fake_header self.wh.new_websocket_client() @@ -314,7 +310,7 @@ class NovaProxyRequestHandlerBaseTestCase(test.NoDBTestCase): tsock.recv.return_value = "HTTP/1.1 500 Internal Server Error\r\n\r\n" self.wh.socket.return_value = tsock - self.wh.path = "http://127.0.0.1/?%s" % self.path + self.wh.path = "http://127.0.0.1/?token=123-456-789" self.wh.headers = self.fake_header self.assertRaises(exception.InvalidConnectionInfo, @@ -346,7 +342,7 @@ class NovaProxyRequestHandlerBaseTestCase(test.NoDBTestCase): HTTP_RESP] self.wh.socket.return_value = tsock - self.wh.path = "http://127.0.0.1/?%s" % self.path + self.wh.path = "http://127.0.0.1/?token=123-456-789" self.wh.headers = self.fake_header self.wh.new_websocket_client() @@ -376,7 +372,7 @@ class NovaProxyRequestHandlerBaseTestCase(test.NoDBTestCase): validate.return_value = objects.ConsoleAuthToken(**params) self.wh.socket.return_value = '' - self.wh.path = "http://127.0.0.1/?%s" % self.path + self.wh.path = "http://127.0.0.1/?token=123-456-789" self.wh.headers = self.fake_header self.wh.new_websocket_client() @@ -388,9 +384,10 @@ class NovaProxyRequestHandlerBaseTestCase(test.NoDBTestCase): @mock.patch('socket.getfqdn') def test_address_string_doesnt_do_reverse_dns_lookup(self, getfqdn): request_mock = mock.MagicMock() - msg = 'GET /vnc.html?%s HTTP/1.1\r\n' % self.path - request_mock.makefile().readline.side_effect = [msg.encode('utf-8'), - b''] + request_mock.makefile().readline.side_effect = [ + b'GET /vnc.html?token=123-456-789 HTTP/1.1\r\n', + b'' + ] server_mock = mock.MagicMock() client_address = ('8.8.8.8', 54321) @@ -644,8 +641,7 @@ class NovaWebsocketSecurityProxyTestCase(test.NoDBTestCase): with mock.patch('websockify.ProxyRequestHandler'): self.wh = websocketproxy.NovaProxyRequestHandler() self.wh.server = self.server - path = urlparse.urlencode({'path': '?token=123-456-789'}) - self.wh.path = "http://127.0.0.1/?%s" % path + self.wh.path = "http://127.0.0.1/?token=123-456-789" self.wh.socket = mock.MagicMock() self.wh.msg = mock.MagicMock() self.wh.do_proxy = mock.MagicMock() diff --git a/nova/tests/unit/objects/test_console_auth_token.py b/nova/tests/unit/objects/test_console_auth_token.py index d931c978d610..721e10118303 100644 --- a/nova/tests/unit/objects/test_console_auth_token.py +++ b/nova/tests/unit/objects/test_console_auth_token.py @@ -30,7 +30,7 @@ from nova.tests.unit.objects import test_objects class _TestConsoleAuthToken(object): @mock.patch('nova.db.api.console_auth_token_create') - def test_authorize(self, mock_create): + def _test_authorize(self, console_type, mock_create): # the expires time is calculated from the current time and # a ttl value in the object. Fix the current time so we can # test expires is calculated correctly as expected @@ -41,10 +41,12 @@ class _TestConsoleAuthToken(object): db_dict = copy.deepcopy(fakes.fake_token_dict) db_dict['expires'] = expires + db_dict['console_type'] = console_type mock_create.return_value = db_dict create_dict = copy.deepcopy(fakes.fake_token_dict) create_dict['expires'] = expires + create_dict['console_type'] = console_type del create_dict['id'] del create_dict['created_at'] del create_dict['updated_at'] @@ -53,10 +55,11 @@ class _TestConsoleAuthToken(object): del expected['token_hash'] del expected['expires'] expected['token'] = fakes.fake_token + expected['console_type'] = console_type obj = token_obj.ConsoleAuthToken( context=self.context, - console_type=fakes.fake_token_dict['console_type'], + console_type=console_type, host=fakes.fake_token_dict['host'], port=fakes.fake_token_dict['port'], internal_access_path=fakes.fake_token_dict['internal_access_path'], @@ -71,11 +74,23 @@ class _TestConsoleAuthToken(object): self.compare_obj(obj, expected) url = obj.access_url - path = urlparse.urlencode({'path': '?token=%s' % fakes.fake_token}) - expected_url = '%s?%s' % ( - fakes.fake_token_dict['access_url_base'], path) + if console_type != 'novnc': + expected_url = '%s?token=%s' % ( + fakes.fake_token_dict['access_url_base'], + fakes.fake_token) + else: + path = urlparse.urlencode({'path': '?token=%s' % fakes.fake_token}) + expected_url = '%s?%s' % ( + fakes.fake_token_dict['access_url_base'], + path) self.assertEqual(expected_url, url) + def test_authorize(self): + self._test_authorize(fakes.fake_token_dict['console_type']) + + def test_authorize_novnc(self): + self._test_authorize('novnc') + @mock.patch('nova.db.api.console_auth_token_create') def test_authorize_duplicate_token(self, mock_create): mock_create.side_effect = DBDuplicateEntry()