Fixes issue # 663, connection file load failure

Fixes issues where connection file load from yaml not type validated.
Also extends exception messages to clarify problem in each case.

The original problem that caused this was a side issue, somewhere
invalid data had gotten into a pywbemcli_connection_definitions.yaml
file (ca_certs had a list type). The old pywbem did not catch this
but the spec was always string and not list of strings.

However, it pointed out that we were not doing enough testing in
the Pywbem_Server class constructor and methods.

docs/changes.rst

@@ -91,6 +91,12 @@ Released: not yet
   _cmd_class.py cmd_class_tree function to eliminate boundary conditions, and
   clarify code.
 
+* Extended parameter type testing in class PywbemServer so that all
+  constructor parameters are value tested.  This specifically fixes issue
+  where we were depending on WBEMConnection to test types of ca_certs
+  and invalid data types could get into the connections file. (See issue
+  #663).
+
 **Known issues:**
 
 * See `list of open issues`_.

docs/pywbemcli/cmdshelp.rst

@@ -70,13 +70,12 @@ Help text for ``pywbemcli``:
                                       during TLS/SSL handshake. If --no-verify client bypasses verification. Default: EnvVar
                                       PYWBEMCLI_VERIFY, or "--verify".
 
-      --ca-certs CACERTS              Certificates to be used for validating the certificate presented by the WBEM server
-                                      during TLS/SSL handshake: FILE: Use the certs in the specified PEM file; DIR: Use the
-                                      certs in the PEM files in the specified directory; "certifi" (pywbem 1.0 or later):
-                                      Use the certs provided by the certifi Python package; Default: EnvVar
-                                      PYWBEMCLI_CA_CERTS, or "certifi" (pywbem 1.0 or later), or the certs in the PEM files
-                                      in the first existing directory from from a list of system directories (pywbem before
-                                      1.0).
+      --ca-certs CACERTS              Certificates used to validate the certificate presented by the WBEM server during
+                                      TLS/SSL handshake: FILE: Use the certs in the specified PEM file; DIR: Use the certs
+                                      in the PEM files in the specified directory; "certifi" (pywbem 1.0 or later): Use the
+                                      certs provided by the certifi Python package; Default: EnvVar PYWBEMCLI_CA_CERTS, or
+                                      "certifi" (pywbem 1.0 or later), or the certs in the PEM files in the first existing
+                                      directory from from a system defined list of directories (pywbem before 1.0).
 
       -c, --certfile FILE             Path name of a PEM file containing a X.509 client certificate that is used to enable
                                       TLS/SSL 2-way authentication by presenting the certificate to the WBEM server during

pywbemtools/pywbemcli/_connection_repository.py

@@ -25,8 +25,8 @@
 
 import os
 import yaml
-import yamlloader
 import six
+import yamlloader
 import click
 
 from ._pywbem_server import PywbemServer
@@ -221,8 +221,6 @@ def _load_connections_file(self):
                     dict_ = yaml.safe_load(_fp)
                     # put all the connection definitions into a group
                     # in the connection file
-                    # TODO: on file created with touch this responds
-                    # TypeError: 'NoneType' object is not subscriptable
                     connections_dict = dict_[
                         ConnectionRepository.connections_group_name]
 
@@ -236,8 +234,14 @@ def _load_connections_file(self):
                             self._loaded = True
                     except KeyError as ke:
                         raise KeyError("Items missing from record {0} in "
-                                       "connections file {1}".format
+                                       "connections file. Exception {1}".format
                                        (ke, self._connections_file))
+                    except TypeError as te:
+                        raise TypeError('Invalid object type in connections '
+                                        'file: "{0}"; server name: "{1}". '
+                                        'Item: {2}'.
+                                        format(self._connections_file, name,
+                                               te))
                 except ValueError as ve:
                     raise ValueError("Invalid YAML in connections file {0}. "
                                      "Exception {1}".format

pywbemtools/pywbemcli/_pywbem_server.py

@@ -21,6 +21,7 @@
 
 import re
 from collections import OrderedDict
+import six
 import click
 
 import pywbem
@@ -35,7 +36,16 @@
 PYWBEMCLI_LOG = 'pywbemcli.log'
 
 
-def _validate_server(server):
+def _raise_typeerror(name, value, rqd_type):
+    """
+    Generate a TypeError for a property in pywbem_server setter that has an
+    invalid type
+    """
+    raise TypeError('Property "{0}" value: {1} must be type: "{2}", not type: '
+                    '"{3}"'.format(name, value, rqd_type, type(value)))
+
+
+def _validate_server_url(server):
     """
     Validate  and possibly complete the wbemserver url provided.
 
@@ -58,6 +68,8 @@ def _validate_server(server):
     elif re.match(r"^https{0,1}://", server) is not None:
         url = server
 
+    # TODO Future: We are trying to make exceptions in these support classes
+    # independent of click so this should become ValueError
     elif re.match(r"^[a-zA-Z0-9]+://", server) is not None:
         raise click.ClickException('Invalid scheme on server argument. {}'
                                    ' Use "http" or "https"'.format(server))
@@ -166,7 +178,7 @@ def server(self, server):
 
         # pylint: disable=attribute-defined-outside-init
         if server:
-            self._server = _validate_server(server)
+            self._server = _validate_server_url(server)
         else:
             self._server = server
 
@@ -180,7 +192,12 @@ def mock_server(self):
     @mock_server.setter
     def mock_server(self, mock_server):
         """Setter method; for a description see the getter method."""
-
+        if mock_server:
+            if not isinstance(mock_server, (list, six.string_types)):
+                _raise_typeerror("mock_server", mock_server, 'list, string')
+            for ms in mock_server:
+                if not isinstance(ms, six.string_types):
+                    _raise_typeerror("mock_server item", mock_server, 'string')
         # assure this is list type in yaml output
         if mock_server is None:
             mock_server = []
@@ -197,6 +214,9 @@ def name(self):
     @name.setter
     def name(self, name):
         """Setter method; for a description see the getter method."""
+        if name:
+            if not isinstance(name, six.string_types):
+                _raise_typeerror("name", name, 'string')
 
         # pylint: disable=attribute-defined-outside-init
         self._name = name
@@ -212,6 +232,9 @@ def user(self):
     def user(self, user):
         """Setter method; for a description see the getter method."""
 
+        if user and not isinstance(user, six.string_types):
+            _raise_typeerror("user", user, 'string')
+
         # pylint: disable=attribute-defined-outside-init
         self._user = user
 
@@ -225,7 +248,8 @@ def password(self):
     @password.setter
     def password(self, password):
         """Setter method; for a description see the getter method."""
-
+        if password and not isinstance(password, six.string_types):
+            _raise_typeerror("password", password, 'string')
         # pylint: disable=attribute-defined-outside-init
         self._password = password
 
@@ -239,7 +263,9 @@ def default_namespace(self):
     @default_namespace.setter
     def default_namespace(self, default_namespace):
         """Setter method; for a description see the getter method."""
-
+        if default_namespace and not isinstance(default_namespace,
+                                                six.string_types):
+            _raise_typeerror("default-namespace", default_namespace, 'string')
         # pylint: disable=attribute-defined-outside-init
         self._default_namespace = default_namespace
 
@@ -248,17 +274,20 @@ def timeout(self):
         """
         :term:`int`: Connection timeout to be used on requests in seconds
         """
+        if self._timeout:
+            assert isinstance(self._timeout, int)
         return self._timeout
 
     @timeout.setter
     def timeout(self, timeout):
         """Setter method; for a description see the getter method."""
-
         if timeout is None:   # disallow None
-            ValueError('Timout of None not allowed')
-        if timeout < 0 or timeout > MAX_TIMEOUT:
-            ValueError('Timeout option({}) out of range {} to {} sec'
-                       .format(timeout, 0, MAX_TIMEOUT))
+            raise ValueError('timeout option of None not allowed')
+        if not isinstance(timeout, int):
+            _raise_typeerror("timeout", timeout, 'integer')
+        if not 0 < timeout <= MAX_TIMEOUT:
+            raise ValueError('Timeout option "{0}" out of range {1} to {2} sec'
+                             .format(timeout, 0, MAX_TIMEOUT))
         # pylint: disable=attribute-defined-outside-init
         self._timeout = timeout
 
@@ -274,10 +303,10 @@ def use_pull(self, use_pull):
         """Setter method; for a description see the getter method."""
 
         if use_pull is None or isinstance(use_pull, bool):
+            # pylint: disable=attribute-defined-outside-init
             self._use_pull = use_pull
         else:
-            ValueError("use_pull must be boolean, not {}.".
-                       format(type(use_pull)))
+            _raise_typeerror("use-pull", use_pull, 'boolean')
 
     @property
     def verify(self):
@@ -290,22 +319,26 @@ def verify(self):
     @verify.setter
     def verify(self, verify):
         """Setter method; for a description see the getter method."""
-
+        if verify and not isinstance(verify, bool):
+            _raise_typeerror("verify", verify, 'boolean')
         # pylint: disable=attribute-defined-outside-init
         self._verify = verify
 
     @property
     def certfile(self):
         """
-        :term:`string`: certtificate for server or None if parameter not
+        :term:`string`: certificate for server or None if parameter not
         provided on input
         """
+        if self._certfile:
+            assert isinstance(self._certfile, six.string_types)
         return self._certfile
 
     @certfile.setter
     def certfile(self, certfile):
         """Setter method; for a description see the getter method."""
-
+        if certfile and not isinstance(certfile, six.string_types):
+            _raise_typeerror("certfile", certfile, 'string')
         # pylint: disable=attribute-defined-outside-init
         self._certfile = certfile
 
@@ -314,26 +347,33 @@ def keyfile(self):
         """
         :term:`string`: keyfile or None if no keyfile parameter input
         """
+        if self._keyfile:
+            assert isinstance(self._keyfile, six.string_types)
         return self._keyfile
 
     @keyfile.setter
     def keyfile(self, keyfile):
         """Setter method; for a description see the getter method."""
+        if keyfile and not isinstance(keyfile, six.string_types):
+            _raise_typeerror("keyfile", keyfile, 'string')
 
         # pylint: disable=attribute-defined-outside-init
         self._keyfile = keyfile
 
     @property
     def ca_certs(self):
         """
-        list of :term:`string`: List of ca_certs if provided on cmd line
+        :term:`string`: String that defines certs for server validation"
         """
+        if self._ca_certs:
+            assert isinstance(self._ca_certs, six.string_types)
         return self._ca_certs
 
     @ca_certs.setter
     def ca_certs(self, ca_certs):
         """Setter method; for a description see the getter method."""
-
+        if ca_certs and not isinstance(ca_certs, six.string_types):
+            _raise_typeerror("ca_certs", ca_certs, 'string')
         # pylint: disable=attribute-defined-outside-init
         self._ca_certs = ca_certs
 
@@ -365,8 +405,9 @@ def password_prompt(self, ctx):
                 confirmation_prompt=False, type=str, err=True)
             ctx.spinner_start()
             # pylint: disable=attribute-defined-outside-init
-            self._password = password
+            self.password = password
         else:
+            # TODO: Again we want to isolate the click excpetions in future
             raise click.ClickException("{cmd} requires user/password, but "
                                        "no password provided."
                                        .format(cmd=ctx.invoked_subcommand))

pywbemtools/pywbemcli/pywbemcli.py

@@ -149,16 +149,16 @@ def validate_connections_file(connections_repo):
 @click.option('--ca-certs', type=str, metavar="CACERTS",
               default=None,  # defaulted in code
               envvar=PywbemServer.ca_certs_envvar,
-              help='Certificates to be used for validating the certificate '
-                   'presented by the WBEM server during TLS/SSL handshake: '
+              help='Certificates used to validate the certificate presented '
+                   'by the WBEM server during TLS/SSL handshake: '
                    'FILE: Use the certs in the specified PEM file; '
                    'DIR: Use the certs in the PEM files in the specified '
                    'directory; '
                    '"certifi" (pywbem 1.0 or later): Use the certs provided by '
                    'the certifi Python package; '
                    'Default: EnvVar {ev}, or "certifi" (pywbem 1.0 or later), '
                    'or the certs in the PEM files in the first existing '
-                   'directory from from a list of system directories '
+                   'directory from from a system defined list of directories '
                    '(pywbem before 1.0).'.
                    format(ev=PywbemServer.ca_certs_envvar))
 @click.option('-c', '--certfile', type=str, metavar="FILE",
@@ -426,6 +426,8 @@ def create_server_instance(connection_name):
     resolved_default_namespace = default_namespace or DEFAULT_NAMESPACE
     resolved_timestats = timestats or DEFAULT_TIMESTATS
     resolved_verify = DEFAULT_VERIFY if verify is None else verify
+
+    # There is no default ca_certs
     resolved_ca_certs = ca_certs  # None should be passed on
 
     # Create the connections repository object that will be included in the

tests/unit/test_general_options.py

@@ -114,17 +114,17 @@
                                   client bypasses verification. Default: EnvVar
                                   PYWBEMCLI_VERIFY, or "--verify".
 
-  --ca-certs CACERTS              Certificates to be used for validating the
-                                  certificate presented by the WBEM server
-                                  during TLS/SSL handshake: FILE: Use the certs
-                                  in the specified PEM file; DIR: Use the certs
-                                  in the PEM files in the specified directory;
+  --ca-certs CACERTS              Certificates used to validate the certificate
+                                  presented by the WBEM server during TLS/SSL
+                                  handshake: FILE: Use the certs in the
+                                  specified PEM file; DIR: Use the certs in the
+                                  PEM files in the specified directory;
                                   "certifi" (pywbem 1.0 or later): Use the certs
                                   provided by the certifi Python package;
                                   Default: EnvVar PYWBEMCLI_CA_CERTS, or
                                   "certifi" (pywbem 1.0 or later), or the certs
                                   in the PEM files in the first existing
-                                  directory from from a list of system
+                                  directory from from a system defined list of
                                   directories (pywbem before 1.0).
 
   -c, --certfile FILE             Path name of a PEM file containing a X.509
@@ -563,21 +563,22 @@ class       Command group for CIM classes.
     #  Test errors with --name option, --mock-server option
     #
 
-    ['Verify -n option with non-existend connection file fails',
-     {'general': ['-n', 'fred', '--connections-file', './blah.yaml'],
+    ['Verify -n option with non-existent connection file fails',
+     {'general': ['-n', 'fred', '--connections-file', './filenotfound.yaml'],
       'cmdgrp': 'connection',
       'args': ['show']},
-     {'stderr': ['Connections file: "./blah.yaml" does not exist.',
+     {'stderr': ['Connections file: "./filenotfound.yaml" does not exist.',
                  'Aborted!'],
       'rc': 1,
       'test': 'innows'},
      None, OK],
 
     ['Verify --name with non-existend connection file fails',
-     {'general': ['--name', 'fred', '--connections-file', './blah.yaml'],
+     {'general': ['--name', 'fred', '--connections-file',
+                  './filenotfound.yaml'],
       'cmdgrp': 'connection',
       'args': ['show']},
-     {'stderr': ['Connections file: "./blah.yaml" does not exist.',
+     {'stderr': ['Connections file: "./filenotfound.yaml" does not exist.',
                  'Aborted!'],
       'rc': 1,
       'test': 'innows'},