fixes to config manager (#58530)

* skip unreadable ansible.cfg

* all types should check for type

* patch access for tests and fix tests that relied on missing files not being checked

changelogs/fragments/dont_choose_unreadable.yml

@@ -0,0 +1,2 @@
+bugfixes:
+    - avoid choosing an unreadable ansible.cfg just because it exists.

lib/ansible/cli/config.py

@@ -14,7 +14,7 @@ from ansible.cli import CLI
 from ansible.cli.arguments import option_helpers as opt_help
 from ansible.config.manager import ConfigManager, Setting, find_ini_config_file
 from ansible.errors import AnsibleError, AnsibleOptionsError
-from ansible.module_utils._text import to_native, to_text
+from ansible.module_utils._text import to_native, to_text, to_bytes
 from ansible.parsing.yaml.dumper import AnsibleDumper
 from ansible.utils.color import stringc
 from ansible.utils.display import Display
@@ -79,7 +79,11 @@ class ConfigCLI(CLI):
 
         if context.CLIARGS['config_file']:
             self.config_file = unfrackpath(context.CLIARGS['config_file'], follow=False)
-            self.config = ConfigManager(self.config_file)
+            b_config = to_bytes(self.config_file)
+            if os.path.exists(b_config) and os.access(b_config, os.R_OK):
+                self.config = ConfigManager(self.config_file)
+            else:
+                raise AnsibleOptionsError('The provided configuration file is missing or not accessible: %s' % to_native(self.config_file))
         else:
             self.config = ConfigManager()
             self.config_file = find_ini_config_file()

lib/ansible/config/manager.py

@@ -23,9 +23,10 @@ except ImportError:
 
 from ansible.config.data import ConfigData
 from ansible.errors import AnsibleOptionsError, AnsibleError
+from ansible.module_utils._text import to_text, to_bytes, to_native
+from ansible.module_utils.common._collections_compat import Sequence
 from ansible.module_utils.six import PY3, string_types
 from ansible.module_utils.six.moves import configparser
-from ansible.module_utils._text import to_text, to_bytes, to_native
 from ansible.module_utils.parsing.convert_bool import boolean
 from ansible.parsing.quoting import unquote
 from ansible.utils import py3compat
@@ -76,6 +77,7 @@ def ensure_type(value, value_type, origin=None):
         :string: Same as 'str'
     '''
 
+    errmsg = ''
     basedir = None
     if origin and os.path.isabs(origin) and os.path.exists(to_bytes(origin)):
         basedir = origin
@@ -96,39 +98,64 @@ def ensure_type(value, value_type, origin=None):
         elif value_type == 'list':
             if isinstance(value, string_types):
                 value = [x.strip() for x in value.split(',')]
+            elif not isinstance(value, Sequence):
+                errmsg = 'list'
 
         elif value_type == 'none':
             if value == "None":
                 value = None
 
+            if value is not None:
+                errmsg = 'None'
+
         elif value_type == 'path':
-            value = resolve_path(value, basedir=basedir)
+            if isinstance(value, string_types):
+                value = resolve_path(value, basedir=basedir)
+            else:
+                errmsg = 'path'
 
         elif value_type in ('tmp', 'temppath', 'tmppath'):
-            value = resolve_path(value, basedir=basedir)
-            if not os.path.exists(value):
-                makedirs_safe(value, 0o700)
-            prefix = 'ansible-local-%s' % os.getpid()
-            value = tempfile.mkdtemp(prefix=prefix, dir=value)
-            atexit.register(cleanup_tmp_file, value, warn=True)
+            if isinstance(value, string_types):
+                value = resolve_path(value, basedir=basedir)
+                if not os.path.exists(value):
+                    makedirs_safe(value, 0o700)
+                prefix = 'ansible-local-%s' % os.getpid()
+                value = tempfile.mkdtemp(prefix=prefix, dir=value)
+                atexit.register(cleanup_tmp_file, value, warn=True)
+            else:
+                errmsg = 'temppath'
 
         elif value_type == 'pathspec':
             if isinstance(value, string_types):
                 value = value.split(os.pathsep)
-            value = [resolve_path(x, basedir=basedir) for x in value]
+
+            if isinstance(value, Sequence):
+                value = [resolve_path(x, basedir=basedir) for x in value]
+            else:
+                errmsg = 'pathspec'
 
         elif value_type == 'pathlist':
             if isinstance(value, string_types):
                 value = value.split(',')
-            value = [resolve_path(x, basedir=basedir) for x in value]
+
+            if isinstance(value, Sequence):
+                value = [resolve_path(x, basedir=basedir) for x in value]
+            else:
+                errmsg = 'pathlist'
 
         elif value_type in ('str', 'string'):
-            value = unquote(to_text(value, errors='surrogate_or_strict'))
+            if isinstance(value, string_types):
+                value = unquote(to_text(value, errors='surrogate_or_strict'))
+            else:
+                errmsg = 'string'
 
         # defaults to string type
         elif isinstance(value, string_types):
             value = unquote(value)
 
+        if errmsg:
+            raise ValueError('Invalid type provided for "%s": %s' % (errmsg, to_native(value)))
+
     return to_text(value, errors='surrogate_or_strict', nonstring='passthru')
 
 
@@ -215,7 +242,8 @@ def find_ini_config_file(warnings=None):
     potential_paths.append("/etc/ansible/ansible.cfg")
 
     for path in potential_paths:
-        if os.path.exists(to_bytes(path)):
+        b_path = to_bytes(path)
+        if os.path.exists(b_path) and os.access(b_path, os.R_OK):
             break
     else:
         path = None
@@ -255,9 +283,8 @@ class ConfigManager(object):
 
         # consume configuration
         if self._config_file:
-            if os.path.exists(to_bytes(self._config_file)):
-                # initialize parser and read config
-                self._parse_config_file()
+            # initialize parser and read config
+            self._parse_config_file()
 
         # update constants
         self.update_config_data()

test/integration/targets/ansible/no-extension

@@ -0,0 +1,2 @@
+[defaults]
+remote_user = admin

test/integration/targets/ansible/runme.sh

@@ -14,7 +14,7 @@ ANSIBLE_REMOTE_USER=administrator ansible-config dump| grep 'DEFAULT_REMOTE_USER
 ansible-config list | grep 'DEFAULT_REMOTE_USER'
 
 # 'view' command must fail when config file is missing or has an invalid file extension
-ansible-config view -c ./ansible-non-existent.cfg 2> err1.txt || grep -Eq '(FileNotFoundError|IOError): \[Errno [0-9]+\] No such file or directory' err1.txt || (cat err*.txt; rm -f err1.txt; exit 1)
+ansible-config view -c ./ansible-non-existent.cfg 2> err1.txt || grep -Eq 'ERROR! The provided configuration file is missing or not accessible:' err1.txt || (cat err*.txt; rm -f err1.txt; exit 1)
 ansible-config view -c ./no-extension 2> err2.txt || grep -q 'Unsupported configuration file extension' err2.txt || (cat err2.txt; rm -f err*.txt; exit 1)
 rm -f err*.txt
 

test/units/config/manager/test_find_ini_config_file.py

@@ -53,8 +53,15 @@ def setup_existing_files(request, monkeypatch):
         else:
             return False
 
+    def _os_access(path, access):
+        if to_text(path) in (request.param[0]):
+            return True
+        else:
+            return False
+
     # Enable user and system dirs so that we know cwd takes precedence
     monkeypatch.setattr("os.path.exists", _os_path_exists)
+    monkeypatch.setattr("os.access", _os_access)
     monkeypatch.setattr("os.getcwd", lambda: os.path.dirname(cfg_dir))
     monkeypatch.setattr("os.path.isdir", lambda path: True if to_text(path) == cfg_dir else real_isdir(path))