Fix accumulated nits

Fix nits from the following reviews:

- https://review.openstack.org/#/c/345397/35
- https://review.openstack.org/#/c/345398/36
- https://review.openstack.org/#/c/345399/42

Change-Id: Iafa5b4432a85353a216da4d55e46e1eda30842e3
Blueprint: websocket-proxy-to-host-security

nova/conf/vnc.py

@@ -230,7 +230,12 @@ first.
 
 Possible values:
 
-* "none": allow connection without authentication
+* ``none``: allow connection without authentication
+* ``vencrypt``: use VeNCrypt authentication scheme
+
+Related options:
+
+* ``[vnc]vencrypt_client_key``, ``[vnc]vencrypt_client_cert``: must also be set
 """),
     cfg.StrOpt(
         'vencrypt_client_key',
@@ -241,8 +246,8 @@ proxy server presents to the compute node during VNC authentication.
 
 Related options:
 
-* ``vnc.auth_schemes``: must include "vencrypt"
+* ``vnc.auth_schemes``: must include ``vencrypt``
-* ``vnc.vencrypt_client_cert```: must also be set
+* ``vnc.vencrypt_client_cert``: must also be set
 """),
     cfg.StrOpt(
         'vencrypt_client_cert',
@@ -253,8 +258,8 @@ the VNC proxy server presents to the compute node during VNC authentication.
 
 Realted options:
 
-* ``vnc.auth_schemes``: must include "vencrypt"
+* ``vnc.auth_schemes``: must include ``vencrypt``
-* ``vnc.vencrypt_client_key```: must also be set
+* ``vnc.vencrypt_client_key``: must also be set
 """),
     cfg.StrOpt(
         'vencrypt_ca_certs',
@@ -265,7 +270,7 @@ for the certificate authorities used by the compute node VNC server.
 
 Related options:
 
-* ``vnc.auth_schemes``: must include "vencrypt"
+* ``vnc.auth_schemes``: must include ``vencrypt``
 """),
 ]
 

nova/console/rfb/auth.py

@@ -40,19 +40,6 @@ class AuthType(enum.IntEnum):
     MSLOGON = 0xfffffffa  # Used by UltraVNC
 
 
-class AuthVeNCryptSubtype(enum.IntEnum):
-
-    PLAIN = 256
-    TLSNONE = 257
-    TLSVNC = 258
-    TLSPLAIN = 259
-    X509NONE = 260
-    X509VNC = 261
-    X509PLAIN = 262
-    X509SASL = 263
-    TLSSASL = 264
-
-
 @six.add_metaclass(abc.ABCMeta)
 class RFBAuthScheme(object):
 
@@ -74,5 +61,7 @@ class RFBAuthScheme(object):
 
         Should raise exception.RFBAuthHandshakeFailed if
         an error occurs
+
+        :param compute_sock: socket connected to the compute node instance
         """
         pass

nova/console/rfb/auths.py

@@ -37,6 +37,13 @@ class RFBAuthSchemeList(object):
             self.schemes[scheme.security_type()] = scheme
 
     def find_scheme(self, desired_types):
+        """Find a suitable authentication scheme to use with compute node.
+
+        Identify which of the ``desired_types`` we can accept.
+
+        :param desired_types: A list of ints corresponding to the various
+            authentication types supported.
+        """
         for security_type in desired_types:
             if security_type in self.schemes:
                 return self.schemes[security_type]

nova/console/rfb/authvencrypt.py

@@ -12,20 +12,39 @@
 # License for the specific language governing permissions and limitations
 # under the License.
 
+import enum
 import ssl
 import struct
 
 from oslo_config import cfg
 from oslo_log import log as logging
+import six
 
 from nova.console.rfb import auth
 from nova import exception
-from nova.i18n import _, _LI
+from nova.i18n import _
 
 LOG = logging.getLogger(__name__)
 CONF = cfg.CONF
 
 
+class AuthVeNCryptSubtype(enum.IntEnum):
+    """Possible VeNCrypt subtypes.
+
+    From https://github.com/rfbproto/rfbproto/blob/master/rfbproto.rst
+    """
+
+    PLAIN = 256
+    TLSNONE = 257
+    TLSVNC = 258
+    TLSPLAIN = 259
+    X509NONE = 260
+    X509VNC = 261
+    X509PLAIN = 262
+    X509SASL = 263
+    TLSSASL = 264
+
+
 class RFBAuthSchemeVeNCrypt(auth.RFBAuthScheme):
     """A security proxy helper which uses VeNCrypt.
 
@@ -81,17 +100,20 @@ class RFBAuthSchemeVeNCrypt(auth.RFBAuthScheme):
 
         LOG.debug("Server supports VeNCrypt sub-types %s", sub_types)
 
-        if auth.AuthVeNCryptSubtype.X509NONE not in sub_types:
+        # We use X509None as we're only seeking to encrypt the channel (ruling
+        # out PLAIN) and prevent MITM (ruling out TLS*, which uses trivially
+        # MITM'd Anonymous Diffie Hellmann (DH) cyphers)
+        if AuthVeNCryptSubtype.X509NONE not in sub_types:
             reason = _("Server does not support the x509None (%s) VeNCrypt"
                        " sub-auth type") % \
-                       auth.AuthVeNCryptSubtype.X509NONE
+                       AuthVeNCryptSubtype.X509NONE
             raise exception.RFBAuthHandshakeFailed(reason=reason)
 
         LOG.debug("Attempting to use the x509None (%s) auth sub-type",
-                  auth.AuthVeNCryptSubtype.X509NONE)
+                  AuthVeNCryptSubtype.X509NONE)
 
         compute_sock.sendall(struct.pack(
-            '!I', auth.AuthVeNCryptSubtype.X509NONE))
+            '!I', AuthVeNCryptSubtype.X509NONE))
 
         # NB(sross): the spec is missing a U8 here that's used in
         # multiple implementations (e.g. QEMU, GTK-VNC).  1 means
@@ -120,9 +142,10 @@ class RFBAuthSchemeVeNCrypt(auth.RFBAuthScheme):
                 cert_reqs=ssl.CERT_REQUIRED,
                 ca_certs=CONF.vnc.vencrypt_ca_certs)
 
-            LOG.info(_LI("VeNCrypt security handshake accepted"))
+            LOG.info("VeNCrypt security handshake accepted")
             return wrapped_sock
 
         except ssl.SSLError as e:
-            reason = _("Error establishing TLS connection to server: %s") % e
+            reason = _("Error establishing TLS connection to server: %s") % (
+                six.text_type(e))
             raise exception.RFBAuthHandshakeFailed(reason=reason)

nova/console/securityproxy/rfb.py

@@ -23,7 +23,7 @@ from nova.console.rfb import auth
 from nova.console.rfb import auths
 from nova.console.securityproxy import base
 from nova import exception
-from nova.i18n import _, _LI
+from nova.i18n import _
 
 LOG = logging.getLogger(__name__)
 
@@ -46,6 +46,10 @@ class RFBSecurityProxy(base.SecurityProxy):
     See the general RFB specification at:
 
       https://tools.ietf.org/html/rfc6143
+
+    See an updated, maintained RDB specification at:
+
+      https://github.com/rfbproto/rfbproto/blob/master/rfbproto.rst
     """
 
     def __init__(self):
@@ -67,7 +71,13 @@ class RFBSecurityProxy(base.SecurityProxy):
             # by sending the "Invalid" security type
             compute_sock.sendall(auth.AUTH_STATUS_FAIL)
 
-    def _parse_version(self, version_str):
+    @staticmethod
+    def _parse_version(version_str):
+        r"""Convert a version string to a float.
+
+        >>> RFBSecurityProxy._parse_version('RFB 003.008\n')
+        0.2
+        """
         maj_str = version_str[4:7]
         min_str = version_str[8:11]
 
@@ -119,6 +129,7 @@ class RFBSecurityProxy(base.SecurityProxy):
         permitted_auth_types_cnt = six.byte2int(recv(compute_sock, 1))
 
         if permitted_auth_types_cnt == 0:
+            # Decode the reason why the request failed
             reason_len_raw = recv(compute_sock, 4)
             reason_len = struct.unpack('!I', reason_len_raw)[0]
             reason = recv(compute_sock, reason_len)
@@ -148,9 +159,8 @@ class RFBSecurityProxy(base.SecurityProxy):
                        _("Only the security type None (%d) is supported") %
                        auth.AuthType.NONE)
 
-            reason = _("Client requested a security type other than "
+            reason = _("Client requested a security type other than None "
-                       " None (%(none_code)d): "
+                       "(%(none_code)d): %(auth_type)s") % {
-                       "%(auth_type)s") % {
                            'auth_type': client_auth,
                            'none_code': auth.AuthType.NONE}
             raise exception.SecurityProxyNegotiationFailed(reason=reason)
@@ -179,10 +189,10 @@ class RFBSecurityProxy(base.SecurityProxy):
                        _("Unable to negotiate security with server"))
             LOG.debug("Auth failed %s", six.text_type(e))
             raise exception.SecurityProxyNegotiationFailed(
-                reason="Auth handshake failed")
+                reason=_("Auth handshake failed"))
 
-        LOG.info(_LI("Finished security handshake, resuming normal proxy "
+        LOG.info("Finished security handshake, resuming normal proxy "
-                     "mode using secured socket"))
+                 "mode using secured socket")
 
         # we can just proxy the security result -- if the server security
         # negotiation fails, we want the client to think it has failed

nova/tests/unit/console/rfb/test_auth.py

@@ -32,8 +32,8 @@ class RFBAuthSchemeListTestCase(test.NoDBTestCase):
         schemelist = auths.RFBAuthSchemeList()
 
         security_types = sorted(schemelist.schemes.keys())
-        self.assertEqual(security_types, [auth.AuthType.NONE,
+        self.assertEqual([auth.AuthType.NONE, auth.AuthType.VENCRYPT],
-                                          auth.AuthType.VENCRYPT])
+                         security_types)
 
     def test_load_unknown(self):
         """Ensure invalid auth schemes are not supported.

nova/tests/unit/console/rfb/test_authvencrypt.py

@@ -58,8 +58,8 @@ class RFBAuthSchemeVeNCryptTestCase(test.NoDBTestCase):
         self._expect_recv(1, "\x00")
 
         self._expect_recv(1, "\x02")
-        subtypes_raw = [auth.AuthVeNCryptSubtype.X509NONE,
+        subtypes_raw = [authvencrypt.AuthVeNCryptSubtype.X509NONE,
-                        auth.AuthVeNCryptSubtype.X509VNC]
+                        authvencrypt.AuthVeNCryptSubtype.X509VNC]
         subtypes = struct.pack('!2I', *subtypes_raw)
         self._expect_recv(8, subtypes)
 
@@ -89,8 +89,8 @@ class RFBAuthSchemeVeNCryptTestCase(test.NoDBTestCase):
         self._expect_recv(1, "\x00")
 
         self._expect_recv(1, "\x02")
-        subtypes_raw = [auth.AuthVeNCryptSubtype.X509NONE,
+        subtypes_raw = [authvencrypt.AuthVeNCryptSubtype.X509NONE,
-                        auth.AuthVeNCryptSubtype.X509VNC]
+                        authvencrypt.AuthVeNCryptSubtype.X509VNC]
         subtypes = struct.pack('!2I', *subtypes_raw)
         self._expect_recv(8, subtypes)
 
@@ -140,7 +140,7 @@ class RFBAuthSchemeVeNCryptTestCase(test.NoDBTestCase):
         self._expect_recv(1, "\x00")
 
         self._expect_recv(1, "\x01")
-        subtypes_raw = [auth.AuthVeNCryptSubtype.X509VNC]
+        subtypes_raw = [authvencrypt.AuthVeNCryptSubtype.X509VNC]
         subtypes = struct.pack('!I', *subtypes_raw)
         self._expect_recv(4, subtypes)
 
@@ -154,8 +154,8 @@ class RFBAuthSchemeVeNCryptTestCase(test.NoDBTestCase):
         self._expect_recv(1, "\x00")
 
         self._expect_recv(1, "\x02")
-        subtypes_raw = [auth.AuthVeNCryptSubtype.X509NONE,
+        subtypes_raw = [authvencrypt.AuthVeNCryptSubtype.X509NONE,
-                        auth.AuthVeNCryptSubtype.X509VNC]
+                        authvencrypt.AuthVeNCryptSubtype.X509VNC]
         subtypes = struct.pack('!2I', *subtypes_raw)
         self._expect_recv(8, subtypes)
 
@@ -174,8 +174,8 @@ class RFBAuthSchemeVeNCryptTestCase(test.NoDBTestCase):
         self._expect_recv(1, "\x00")
 
         self._expect_recv(1, "\x02")
-        subtypes_raw = [auth.AuthVeNCryptSubtype.X509NONE,
+        subtypes_raw = [authvencrypt.AuthVeNCryptSubtype.X509NONE,
-                        auth.AuthVeNCryptSubtype.X509VNC]
+                        authvencrypt.AuthVeNCryptSubtype.X509VNC]
         subtypes = struct.pack('!2I', *subtypes_raw)
         self._expect_recv(8, subtypes)
 

nova/tests/unit/console/securityproxy/test_rfb.py

@@ -62,7 +62,7 @@ class RFBSecurityProxyTestCase(test.NoDBTestCase):
         self._expect_tenant_recv(auth.VERSION_LENGTH, full_version_str)
 
     def _to_binary(self, val):
-        if type(val) != six.binary_type:
+        if not isinstance(val, six.binary_type):
             val = six.binary_type(val, 'utf-8')
         return val
 
@@ -87,6 +87,11 @@ class RFBSecurityProxyTestCase(test.NoDBTestCase):
             list(self.compute_sock.recv.side_effect) + [ret_val])
 
     def test_fail(self):
+        """Validate behavior for invalid initial message from tenant.
+
+        The spec defines the sequence that should be used in the handshaking
+        process. Anything outside of this is invalid.
+        """
         self._expect_tenant_send("\x00\x00\x00\x01\x00\x00\x00\x04blah")
 
         self.proxy._fail(self.tenant_sock, None, 'blah')
@@ -94,6 +99,11 @@ class RFBSecurityProxyTestCase(test.NoDBTestCase):
         self._assert_expected_calls()
 
     def test_fail_server_message(self):
+        """Validate behavior for invalid initial message from server.
+
+        The spec defines the sequence that should be used in the handshaking
+        process. Anything outside of this is invalid.
+        """
         self._expect_tenant_send("\x00\x00\x00\x01\x00\x00\x00\x04blah")
         self._expect_compute_send("\x00")
 
@@ -102,20 +112,30 @@ class RFBSecurityProxyTestCase(test.NoDBTestCase):
         self._assert_expected_calls()
 
     def test_parse_version(self):
+        """Validate behavior of version parser."""
         res = self.proxy._parse_version("RFB 012.034\n")
         self.assertEqual(12.34, res)
 
     def test_fails_on_compute_version(self):
+        """Validate behavior for unsupported compute RFB version.
+
+        We only support RFB protocol version 3.8.
+        """
         for full_version_str in ["RFB 003.007\n", "RFB 003.009\n"]:
             self._expect_compute_recv(auth.VERSION_LENGTH, full_version_str)
 
-            self.assertRaises(exception.SecurityProxyNegotiationFailed,
+            ex = self.assertRaises(exception.SecurityProxyNegotiationFailed,
-                              self.proxy.connect,
+                                   self.proxy.connect,
-                              self.tenant_sock,
+                                   self.tenant_sock,
-                              self.compute_sock)
+                                   self.compute_sock)
+            self.assertIn('version 3.8, but server', six.text_type(ex))
             self._assert_expected_calls()
 
     def test_fails_on_tenant_version(self):
+        """Validate behavior for unsupported tenant RFB version.
+
+        We only support RFB protocol version 3.8.
+        """
         full_version_str = "RFB 003.008\n"
 
         for full_version_str_invalid in ["RFB 003.007\n", "RFB 003.009\n"]:
@@ -126,13 +146,19 @@ class RFBSecurityProxyTestCase(test.NoDBTestCase):
             self._expect_tenant_recv(auth.VERSION_LENGTH,
                                      full_version_str_invalid)
 
-            self.assertRaises(exception.SecurityProxyNegotiationFailed,
+            ex = self.assertRaises(exception.SecurityProxyNegotiationFailed,
-                              self.proxy.connect,
+                                   self.proxy.connect,
-                              self.tenant_sock,
+                                   self.tenant_sock,
-                              self.compute_sock)
+                                   self.compute_sock)
+            self.assertIn('version 3.8, but tenant', six.text_type(ex))
             self._assert_expected_calls()
 
     def test_fails_on_sec_type_cnt_zero(self):
+        """Validate behavior if a server returns 0 supported security types.
+
+        This indicates a random issue and the cause of that issues should be
+        decoded and reported in the exception.
+        """
         self.proxy._fail = mock.Mock()
 
         self._version_handshake()
@@ -142,15 +168,17 @@ class RFBSecurityProxyTestCase(test.NoDBTestCase):
         self._expect_compute_recv(6, "cheese")
         self._expect_tenant_send("\x00\x00\x00\x00\x06cheese")
 
-        self.assertRaises(exception.SecurityProxyNegotiationFailed,
+        ex = self.assertRaises(exception.SecurityProxyNegotiationFailed,
-                          self.proxy.connect,
+                               self.proxy.connect,
-                          self.tenant_sock,
+                               self.tenant_sock,
-                          self.compute_sock)
+                               self.compute_sock)
+        self.assertIn('cheese', six.text_type(ex))
 
         self._assert_expected_calls()
 
     @mock.patch.object(authnone.RFBAuthSchemeNone, "security_handshake")
     def test_full_run(self, mock_handshake):
+        """Validate correct behavior."""
         new_sock = mock.MagicMock()
         mock_handshake.return_value = new_sock
 
@@ -171,6 +199,7 @@ class RFBSecurityProxyTestCase(test.NoDBTestCase):
         self._assert_expected_calls()
 
     def test_client_auth_invalid_fails(self):
+        """Validate behavior if no security types are supported."""
         self.proxy._fail = self.manager.proxy._fail
         self.proxy.security_handshake = self.manager.proxy.security_handshake
 
@@ -195,6 +224,7 @@ class RFBSecurityProxyTestCase(test.NoDBTestCase):
         self._assert_expected_calls()
 
     def test_exception_in_choose_security_type_fails(self):
+        """Validate behavior if a given security type isn't supported."""
         self.proxy._fail = self.manager.proxy._fail
         self.proxy.security_handshake = self.manager.proxy.security_handshake
 
@@ -220,6 +250,7 @@ class RFBSecurityProxyTestCase(test.NoDBTestCase):
 
     @mock.patch.object(authnone.RFBAuthSchemeNone, "security_handshake")
     def test_exception_security_handshake_fails(self, mock_auth):
+        """Validate behavior if the security handshake fails for any reason."""
         self.proxy._fail = self.manager.proxy._fail
 
         self._version_handshake()

releasenotes/notes/websocket-proxy-to-host-security-c3eca0647b0cbc02.yaml

@@ -17,8 +17,7 @@ features:
     ensure it is connecting to a valid compute node. The proxy can also send
     its own x509 cert to allow the compute node to validate that the connection
     comes from the official proxy server.
-upgrade:
+
-  - |
     To make use of VeNCrypt, configuration steps are required for both the
     `nova-novncproxy` service and libvirt on all the compute nodes. The
     ``/etc/libvirt/qemu.conf`` file should be modified to set the ``vnc_tls``