From fd01d15d144caa4d5a482301d05cf724c75c4500 Mon Sep 17 00:00:00 2001
From: Leo Henken <lh236s@att.com>
Date: Fri, 2 Aug 2019 11:42:52 -0500
Subject: [PATCH] Fix test_novnc to adequately validate websocket upgrade

Currently, test_novnc validates the websocket upgrade by verifying
that the websocket response reports a protocol switch and that the
response includes a server name specified in the configuration
field vnc_server_header. This explicit server name configuration
field introduces a security concern and convolutes the code base.

HTTP RFC7231 (https://tools.ietf.org/html/rfc7231) section 6.2.2
says that when switching protocols, the response "MUST generate
an Upgrade header field that indicates which protocols will be
switched to".

This patchset uses this required Upgrade field to validate the
websocket upgrade instead of an environment-based configuration
field, making the code base cleaner, safer, and more reliable.

vnc_server_header is deprecated and necessary release notes are
created.

Change-Id: I5d3c9bdd0d20a15ade672f276dd0f24b654e3de5
Closes-bug: #1838777
Closes-bug: #1840788
---
 ...te-vnc-server-header-529f07d592aefb62.yaml | 12 +++++++++++
 tempest/api/compute/servers/test_novnc.py     | 20 ++++++++++---------
 tempest/config.py                             |  7 ++++++-
 3 files changed, 29 insertions(+), 10 deletions(-)
 create mode 100644 releasenotes/notes/deprecate-vnc-server-header-529f07d592aefb62.yaml

diff --git a/releasenotes/notes/deprecate-vnc-server-header-529f07d592aefb62.yaml b/releasenotes/notes/deprecate-vnc-server-header-529f07d592aefb62.yaml
new file mode 100644
index 0000000000..d7e144da0e
--- /dev/null
+++ b/releasenotes/notes/deprecate-vnc-server-header-529f07d592aefb62.yaml
@@ -0,0 +1,12 @@
+---
+deprecations:
+  - |
+    The config option ``CONF.compute.vnc_server_header`` is deprecated because
+    it has become obsolete with the usage of different response header fields
+    to accomplish the same goal in accordance with RFC7231 Section 6.2.2.
+
+fixes:
+  - |
+    Adequately validates WebSocket upgrade in test_novnc and removes unneeded
+    configuration complexity. Closes bug #1838777 and #1840788.
+
diff --git a/tempest/api/compute/servers/test_novnc.py b/tempest/api/compute/servers/test_novnc.py
index 50ffb2159a..7cf6d83d2b 100644
--- a/tempest/api/compute/servers/test_novnc.py
+++ b/tempest/api/compute/servers/test_novnc.py
@@ -156,22 +156,24 @@ class NoVNCConsoleTestJSON(base.BaseV2ComputeTest):
                                                        data[20:24])[0] + 24))
 
     def _validate_websocket_upgrade(self):
+        """Verify that the websocket upgrade was successful.
+
+        Parses response and ensures that required response
+        fields are present and accurate.
+        (https://tools.ietf.org/html/rfc7231#section-6.2.2)
+        """
+
         self.assertTrue(
             self._websocket.response.startswith(b'HTTP/1.1 101 Switching '
-                                                b'Protocols\r\n'),
-            'Did not get the expected 101 on the {} call: {}'.format(
-                CONF.compute_feature_enabled.vnc_server_header,
+                                                b'Protocols'),
+            'Incorrect HTTP return status code: {}'.format(
                 six.text_type(self._websocket.response)
             )
         )
-        # Since every other server type returns Headers with different case
-        # (for example 'nginx'), lowercase must be applied to eliminate issues.
-        _desired_header = "server: {0}".format(
-            CONF.compute_feature_enabled.vnc_server_header
-        ).lower()
+        _required_header = 'upgrade: websocket'
         _response = six.text_type(self._websocket.response).lower()
         self.assertIn(
-            _desired_header,
+            _required_header,
             _response,
             'Did not get the expected WebSocket HTTP Response.'
         )
diff --git a/tempest/config.py b/tempest/config.py
index c50ebbe59a..5f91a6d2e2 100644
--- a/tempest/config.py
+++ b/tempest/config.py
@@ -483,7 +483,12 @@ ComputeFeaturesGroup = [
     cfg.StrOpt('vnc_server_header',
                default='WebSockify',
                help='Expected VNC server name (WebSockify, nginx, etc) '
-                    'in response header.'),
+                    'in response header.',
+               deprecated_for_removal=True,
+               deprecated_reason='This option will be ignored because the '
+                                 'usage of different response header fields '
+                                 'to accomplish the same goal (in accordance '
+                                 'with RFC7231 S6.2.2) makes it obsolete.'),
     cfg.BoolOpt('spice_console',
                 default=False,
                 help='Enable Spice console. This configuration value should '