Fix for core changes: stop stripping Unsafe, adapt adapter to config …

…manager changes (#416)

* limit docker SDK for localenv

* WIP update unsafe handling

* do not fail fast

* stringify other non-string ununsafe values

* try bytes I guess

* stop using options as internal storage (#417)

* stop relying on controller code in module utils

* another internal option (#417)

* fix connection unit

* fix option adapter tests

* sanity/cleanup

* new adapter checking on 2.17+ only

* actually use constraints, constrain requests

* stop stringifying with newer requests

* remove unused import

* requests constraints for py3.6

* add changelog fragment

* update requirements in user guide

.github/workflows/ansible-test.yml

@@ -31,6 +31,7 @@ jobs:
     name: Sanity (Ⓐ${{ matrix.ansible }})
     runs-on: ${{ matrix.runner }}
     strategy:
+      fail-fast: false
       matrix:
         runner:
           - ubuntu-latest
@@ -106,8 +107,7 @@ jobs:
     runs-on: ${{ matrix.runner }}
     name: Units (Ⓐ${{ matrix.ansible }})
     strategy:
-      # As soon as the first unit test fails, cancel the others to free up the CI queue
-      fail-fast: true
+      fail-fast: false
       matrix:
         runner:
           - ubuntu-latest

changelogs/fragments/416-core-changes.yml

@@ -0,0 +1,7 @@
+---
+major_changes:
+  - requirements - the ``requests`` package which is required by ``hvac`` now has a more restrictive range for this collection in certain use cases due to breaking security changes in ``ansible-core`` that were backported (https://github.com/ansible-collections/community.hashi_vault/pull/416).
+
+trivial:
+  - We previously sometimes used options, especially in plugins, as a sort of internal information store for things that needed to be passed between methods, or we overwrote some options internally with values that did not match their documentation-defined type. A breaking change in core that validates and converts types to match config manager definitions has broken this usage. Code has been updated to avoid that, and/or change documented types (https://github.com/ansible-collections/community.hashi_vault/pull/416).
+  - The docker SDK version used in localenv setup has been properly constrained (https://github.com/ansible-collections/community.hashi_vault/pull/416/commits/905ac5da0778ddc236e346dd55578837a546090e).

docs/docsite/rst/user_guide.rst

@@ -40,6 +40,7 @@ Other requirements
 
 * ``boto3`` (only if loading credentials from a boto session, for example using an AWS profile or IAM role credentials)
 * ``azure-identity`` (only if using a service principal or managed identity)
+* ``requests`` — with ``requests>=2.28,<2.29``, setting certain options (``token``, ``namespace``) to values that come from lookups will raise an exception, do to Ansible's marking of the values as "unsafe" for templating. We recommend using ``requests>=2.29``, which won't work with Python 3.6.
 
 Retrying failed requests
 ========================

plugins/lookup/hashi_vault.py

@@ -305,13 +305,14 @@ def field_ops(self):
             field = s_f[1]
         else:
             field = None
-        self.set_option('secret_field', field)
+
+        self._secret_field = field
 
     def get(self):
         '''gets a secret. should always return a list'''
 
+        field = self._secret_field
         secret = self.get_option('secret')
-        field = self.get_option('secret_field')
         return_as = self.get_option('return_format')
 
         try:

plugins/module_utils/_auth_method_token.py

@@ -81,7 +81,7 @@ def validate(self):
             raise HashiVaultValueError("No Vault Token specified or discovered.")
 
     def authenticate(self, client, use_token=True, lookup_self=False):
-        token = self._stringify(self._options.get_option('token'))
+        token = self._options.get_option('token')
         validate = self._options.get_option_default('token_validate')
 
         response = None

plugins/module_utils/_connection_options.py

@@ -100,11 +100,11 @@ def get_hvac_connection_options(self):
 
         # validate_certs is only used to optionally change the value of ca_cert
         def _filter(k, v):
-            return v is not None and k != 'validate_certs'
+            return v is not None and k not in ('validate_certs', 'ca_cert')
 
         # our transformed ca_cert value will become the verify parameter for the hvac client
         hvopts = self._options.get_filtered_options(_filter, *self.OPTIONS)
-        hvopts['verify'] = hvopts.pop('ca_cert')
+        hvopts['verify'] = self._conopt_verify
 
         retry_action = hvopts.pop('retry_action')
         if 'retries' in hvopts:
@@ -255,6 +255,6 @@ def _boolean_or_cacert(self):
                 validate_certs = True
 
         if not (validate_certs and ca_cert):
-            self._options.set_option('ca_cert', validate_certs)
+            self._conopt_verify = validate_certs
         else:
-            self._options.set_option('ca_cert', to_text(ca_cert, errors='surrogate_or_strict'))
+            self._conopt_verify = to_text(ca_cert, errors='surrogate_or_strict')

plugins/module_utils/_hashi_vault_common.py

@@ -25,54 +25,18 @@
     HAS_HVAC = False
 
 
-def _stringify(input):
-    '''
-    This method is primarily used to Un-Unsafe values that come from Ansible.
-    We want to remove the Unsafe context so that libraries don't get confused
-    by the values.
-    '''
-
-    # Since this is a module_util, and will be used by both plugins and modules,
-    # we cannot import the AnsibleUnsafe* types, because they are controller-only.
-    # However, they subclass the native types, so we can check for that.
-
-    # bytes is the only consistent type to check against in both py2 and py3
-    if isinstance(input, bytes):
-        # seems redundant, but this will give us a regular bytes object even
-        # when the input is AnsibleUnsafeBytes
-        return bytes(input)
-    else:
-        # instead of checking for py2 vs. py3 to cast to str or unicode,
-        # let's get the type from the literal.
-        return type(u'')(input)
-
-
 class HashiVaultValueError(ValueError):
     '''Use in common code to raise an Exception that can be turned into AnsibleError or used to fail_json()'''
 
 
 class HashiVaultHelper():
-
-    STRINGIFY_CANDIDATES = set([
-        'token',    # Token will end up in a header, requests requires headers to be str or bytes,
-                    # and newer versions of requests stopped converting automatically. Because our
-                    # token could have been passed in from a previous lookup call, it could be one
-                    # of the AnsibleUnsafe types instead, causing a failure. Tokens should always
-                    # be strings, so we will convert them.
-        'namespace',    # namespace is also set in a header
-    ])
-
     def __init__(self):
         # TODO move hvac checking here?
         pass
 
-    @staticmethod
-    def _stringify(input):
-        return _stringify(input)
-
     def get_vault_client(
         self,
-        hashi_vault_logout_inferred_token=True, hashi_vault_revoke_on_logout=False, hashi_vault_stringify_args=True,
+        hashi_vault_logout_inferred_token=True, hashi_vault_revoke_on_logout=False,
         **kwargs
     ):
         '''
@@ -84,16 +48,8 @@ def get_vault_client(
 
         :param hashi_vault_revoke_on_logout: if True revokes any current token on logout. Only used if a logout is performed. Not recommended.
         :type hashi_vault_revoke_on_logout: bool
-
-        :param hashi_vault_stringify_args: if True converts a specific set of defined kwargs to a string type.
-        :type hashi_vault_stringify_args: bool
         '''
 
-        if hashi_vault_stringify_args:
-            for key in kwargs.keys():
-                if key in self.STRINGIFY_CANDIDATES:
-                    kwargs[key] = self._stringify(kwargs[key])
-
         client = hvac.Client(**kwargs)
 
         # logout to prevent accidental use of inferred tokens
@@ -296,7 +252,3 @@ def warn(self, message):
 
     def deprecate(self, message, version=None, date=None, collection_name=None):
         self._deprecator(message, version=version, date=date, collection_name=collection_name)
-
-    @staticmethod
-    def _stringify(input):
-        return _stringify(input)

tests/utils/constraints.txt → tests/integration/constraints.txt

@@ -57,3 +57,8 @@ hvac >= 1.2.1 ; python_version >= '3.6'
 # urllib3
 # these should be satisfied naturally by the requests versions required by hvac anyway
 urllib3 >= 1.15 ; python_version >= '3.6' # we need raise_on_status for retry support to raise the correct exceptions https://github.com/urllib3/urllib3/blob/main/CHANGES.rst#115-2016-04-06
+
+# requests
+# https://github.com/psf/requests/pull/6356
+requests >= 2.29 ; python_version >= '3.7'
+requests <  2.28 ; python_version <  '3.7'

tests/integration/targets/setup_localenv_docker/files/requirements/constraints.txt

@@ -1 +1,3 @@
-docker >= 5.0.0 ; python_version >= '3.6'
+# https://github.com/docker/docker-py/issues/3194#issuecomment-1849016391
+# Must use docker SDK for Python < 7
+docker >= 5.0.0,<7 ; python_version >= '3.6'

tests/unit/constraints.txt

@@ -0,0 +1,64 @@
+coverage >= 4.2, < 5.0.0, != 4.3.2 ; python_version <= '3.7' # features in 4.2+ required, avoid known bug in 4.3.2 on python 2.6, coverage 5.0+ incompatible
+coverage >= 4.5.4, < 5.0.0 ; python_version > '3.7' # coverage had a bug in < 4.5.4 that would cause unit tests to hang in Python 3.8, coverage 5.0+ incompatible
+cryptography < 2.2 ; python_version < '2.7' # cryptography 2.2 drops support for python 2.6
+deepdiff < 4.0.0 ; python_version < '3' # deepdiff 4.0.0 and later require python 3
+jinja2 < 2.11 ; python_version < '2.7' # jinja2 2.11 and later require python 2.7 or later
+urllib3 < 1.24 ; python_version < '2.7' # urllib3 1.24 and later require python 2.7 or later
+pywinrm >= 0.3.0 # message encryption support
+sphinx < 1.6 ; python_version < '2.7' # sphinx 1.6 and later require python 2.7 or later
+sphinx < 1.8 ; python_version >= '2.7' # sphinx 1.8 and later are currently incompatible with rstcheck 3.3
+pygments >= 2.4.0 # Pygments 2.4.0 includes bugfixes for YAML and YAML+Jinja lexers
+wheel < 0.30.0 ; python_version < '2.7' # wheel 0.30.0 and later require python 2.7 or later
+yamllint != 1.8.0, < 1.14.0 ; python_version < '2.7' # yamllint 1.8.0 and 1.14.0+ require python 2.7+
+pycrypto >= 2.6 # Need features found in 2.6 and greater
+ncclient >= 0.5.2 # Need features added in 0.5.2 and greater
+idna < 2.6, >= 2.5 # linode requires idna < 2.9, >= 2.5, requests requires idna < 2.6, but cryptography will cause the latest version to be installed instead
+paramiko < 2.4.0 ; python_version < '2.7' # paramiko 2.4.0 drops support for python 2.6
+pytest < 3.3.0 ; python_version < '2.7' # pytest 3.3.0 drops support for python 2.6
+pytest < 5.0.0 ; python_version == '2.7' # pytest 5.0.0 and later will no longer support python 2.7
+pytest-forked < 1.0.2 ; python_version < '2.7' # pytest-forked 1.0.2 and later require python 2.7 or later
+pytest-forked >= 1.0.2 ; python_version >= '2.7' # pytest-forked before 1.0.2 does not work with pytest 4.2.0+ (which requires python 2.7+)
+ntlm-auth >= 1.3.0 # message encryption support using cryptography
+requests < 2.20.0 ; python_version < '2.7' # requests 2.20.0 drops support for python 2.6
+requests-ntlm >= 1.1.0 # message encryption support
+requests-credssp >= 0.1.0 # message encryption support
+voluptuous >= 0.11.0 # Schema recursion via Self
+openshift >= 0.6.2, < 0.9.0 # merge_type support
+virtualenv < 16.0.0 ; python_version < '2.7' # virtualenv 16.0.0 and later require python 2.7 or later
+pathspec < 0.6.0 ; python_version < '2.7' # pathspec 0.6.0 and later require python 2.7 or later
+pyopenssl < 18.0.0 ; python_version < '2.7' # pyOpenSSL 18.0.0 and later require python 2.7 or later
+pyfmg == 0.6.1 # newer versions do not pass current unit tests
+pyyaml < 5.1 ; python_version < '2.7' # pyyaml 5.1 and later require python 2.7 or later
+pycparser < 2.19 ; python_version < '2.7' # pycparser 2.19 and later require python 2.7 or later
+mock >= 2.0.0 # needed for features backported from Python 3.6 unittest.mock (assert_called, assert_called_once...)
+pytest-mock >= 1.4.0 # needed for mock_use_standalone_module pytest option
+xmltodict < 0.12.0 ; python_version < '2.7' # xmltodict 0.12.0 and later require python 2.7 or later
+lxml < 4.3.0 ; python_version < '2.7' # lxml 4.3.0 and later require python 2.7 or later
+pyvmomi < 6.0.0 ; python_version < '2.7' # pyvmomi 6.0.0 and later require python 2.7 or later
+pyone == 1.1.9 # newer versions do not pass current integration tests
+boto3 < 1.11 ; python_version < '2.7' # boto3 1.11 drops Python 2.6 support
+botocore >= 1.10.0, < 1.14 ; python_version < '2.7' # adds support for the following AWS services: secretsmanager, fms, and acm-pca; botocore 1.14 drops Python 2.6 support
+botocore >= 1.10.0 ; python_version >= '2.7' # adds support for the following AWS services: secretsmanager, fms, and acm-pca
+setuptools < 45 ; python_version <= '2.7' # setuptools 45 and later require python 3.5 or later
+cffi >= 1.14.2, != 1.14.3 # Yanked version which older versions of pip will still install:
+
+# freeze pylint and its requirements for consistent test results
+astroid == 2.2.5
+isort == 4.3.15
+lazy-object-proxy == 1.3.1
+mccabe == 0.6.1
+pylint == 2.3.1
+typed-ast == 1.4.0  # 1.4.0 is required to compile on Python 3.8
+wrapt == 1.11.1
+
+# hvac
+hvac >= 1.2.1 ; python_version >= '3.6'
+
+# urllib3
+# these should be satisfied naturally by the requests versions required by hvac anyway
+urllib3 >= 1.15 ; python_version >= '3.6' # we need raise_on_status for retry support to raise the correct exceptions https://github.com/urllib3/urllib3/blob/main/CHANGES.rst#115-2016-04-06
+
+# requests
+# https://github.com/psf/requests/pull/6356
+requests >= 2.29 ; python_version >= '3.7'
+requests <  2.28 ; python_version <  '3.7'

tests/unit/plugins/module_utils/authentication/test_hashi_vault_auth_method_base.py

@@ -8,13 +8,10 @@
 
 import pytest
 
-from ansible_collections.community.hashi_vault.tests.unit.compat import mock
-
-from ansible_collections.community.hashi_vault.plugins.module_utils._hashi_vault_common import (
+from ......plugins.module_utils._hashi_vault_common import (
     HashiVaultAuthMethodBase,
     HashiVaultOptionGroupBase,
     HashiVaultValueError,
-    _stringify,
 )
 
 
@@ -75,12 +72,3 @@ def test_deprecate_callback(self, auth_base, deprecator, version, date, collecti
         auth_base.deprecate(msg, version, date, collection_name)
 
         deprecator.assert_called_once_with(msg, version=version, date=date, collection_name=collection_name)
-
-    def test_has_stringify(self, auth_base):
-        v = 'X'
-        wrapper = mock.Mock(wraps=_stringify)
-        with mock.patch('ansible_collections.community.hashi_vault.plugins.module_utils._hashi_vault_common._stringify', wrapper):
-            r = auth_base._stringify(v)
-
-        wrapper.assert_called_once_with(v)
-        assert r == v

tests/unit/plugins/module_utils/option_adapter/test_hashi_vault_option_adapter.py

@@ -8,7 +8,7 @@
 
 import pytest
 
-from ansible_collections.community.hashi_vault.plugins.module_utils._hashi_vault_common import HashiVaultOptionAdapter
+from ......plugins.module_utils._hashi_vault_common import HashiVaultOptionAdapter
 
 
 SAMPLE_DICT = {
@@ -103,44 +103,80 @@ def test_get_option_default_missing_returns_default(self, adapter, option):
     def test_has_option(self, adapter, option, expected):
         assert adapter.has_option(option) == expected
 
-    @pytest.mark.parametrize('value', ['__VALUE'])
-    @pytest.mark.parametrize('option', (SAMPLE_KEYS + MISSING_KEYS))
-    def test_set_option(self, adapter, option, value, sample_dict):
+    @pytest.mark.parametrize('option', SAMPLE_KEYS)
+    def test_set_option_existing(self, adapter, option, sample_dict):
+        value = type(sample_dict.get(option, ""))()
         adapter.set_option(option, value)
-
         # first check the underlying data, then ensure the adapter refelcts the change too
         assert sample_dict[option] == value
         assert adapter.get_option(option) == value
 
+    @pytest.mark.parametrize('option', MISSING_KEYS)
+    def test_set_option_missing(self, request, adapter, option, sample_dict):
+        value = MARKER
+
+        for mark in request.node.own_markers:
+            if mark.name == 'option_adapter_raise_on_missing':
+                from ansible.errors import AnsibleError
+                with pytest.raises(AnsibleError, match=rf"^Requested entry.*?setting: {option}.*?was not defined in configuration"):
+                    adapter.set_option(option, value)
+                break
+        else:
+            adapter.set_option(option, value)
+            assert sample_dict[option] == value
+            assert adapter.get_option(option) == value
+
     @pytest.mark.parametrize('default', [MARKER])
-    @pytest.mark.parametrize('option,expected', [(o, SAMPLE_DICT[o]) for o in SAMPLE_KEYS] + [(o, MARKER) for o in MISSING_KEYS])
-    def test_set_option_default(self, adapter, option, default, expected, sample_dict):
+    @pytest.mark.parametrize('option,expected', [(o, SAMPLE_DICT[o]) for o in SAMPLE_KEYS])
+    def test_set_option_default_existing(self, adapter, option, default, expected, sample_dict):
         value = adapter.set_option_default(option, default)
 
         # check return data, underlying data structure, and adapter retrieval
         assert value == expected
         assert sample_dict[option] == expected
         assert adapter.get_option(option) == expected
 
-    @pytest.mark.parametrize('options', [[SAMPLE_KEYS[0], MISSING_KEYS[0]]])
-    def test_set_options(self, adapter, options, sample_dict):
-        update = dict([(o, '__VALUE_%i' % i) for i, o in enumerate(options)])
+    @pytest.mark.parametrize('default', [MARKER])
+    @pytest.mark.parametrize('option,expected', [(o, MARKER) for o in MISSING_KEYS])
+    def test_set_option_default_missing(self, request, adapter, option, default, expected, sample_dict):
+        for mark in request.node.own_markers:
+            if mark.name == 'option_adapter_raise_on_missing':
+                from ansible.errors import AnsibleError
+                with pytest.raises(AnsibleError, match=rf"^Requested entry.*?setting: {option}.*?was not defined in configuration"):
+                    adapter.set_option_default(option, default)
+                break
+        else:
+            value = adapter.set_option_default(option, default)
+            # check return data, underlying data structure, and adapter retrieval
+            assert value == expected
+            assert sample_dict[option] == expected
+            assert adapter.get_option(option) == expected
 
-        adapter.set_options(**update)
+    @pytest.mark.parametrize('options', [[SAMPLE_KEYS[0], MISSING_KEYS[0]]])
+    def test_set_options(self, request, adapter, options, sample_dict):
+        update = dict([(o, type(sample_dict.get(o, ""))(i)) for i, o in enumerate(options)])
+
+        for mark in request.node.own_markers:
+            if mark.name == 'option_adapter_raise_on_missing':
+                from ansible.errors import AnsibleError
+                with pytest.raises(AnsibleError, match=r"^Requested entry.*?setting:.*?was not defined in configuration"):
+                    adapter.set_options(**update)
+                break
+        else:
+            adapter.set_options(**update)
+            for k in MISSING_KEYS:
+                if k in update:
+                    assert sample_dict[k] == update[k]
+                    assert adapter.get_option(k) == update[k]
+                else:
+                    assert k not in sample_dict
+                    assert not adapter.has_option(k)
 
         for k in SAMPLE_KEYS:
             expected = update[k] if k in update else SAMPLE_DICT[k]
             assert sample_dict[k] == expected
             assert adapter.get_option(k) == expected
 
-        for k in MISSING_KEYS:
-            if k in update:
-                assert sample_dict[k] == update[k]
-                assert adapter.get_option(k) == update[k]
-            else:
-                assert k not in sample_dict
-                assert not adapter.has_option(k)
-
     @pytest.mark.parametrize('options', [[SAMPLE_KEYS[0], MISSING_KEYS[0]]])
     def test_get_options_mixed(self, adapter, options):
         with pytest.raises(KeyError):

tests/unit/plugins/module_utils/test_hashi_vault_connection_options.py

@@ -101,7 +101,7 @@ def test_boolean_or_cacert(self, connection_options, predefined_options, adapter
         with mock.patch.dict(os.environ, envpatch):
             connection_options._boolean_or_cacert()
 
-        assert predefined_options['ca_cert'] == expected
+        assert connection_options._conopt_verify == expected
 
     # _process_option_proxies
     # proxies can be specified as a dictionary where key is protocol/scheme
@@ -248,7 +248,7 @@ def test_get_hvac_connection_options(
 
         # these should always be returned
         assert 'url' in opts and opts['url'] == predefined_options['url']
-        assert 'verify' in opts and opts['verify'] == predefined_options['ca_cert']
+        assert 'verify' in opts and opts['verify'] == connection_options._conopt_verify
 
         # these are optional
         assert 'proxies' not in opts or opts['proxies'] == predefined_options['proxies']