From d70ec4e54069f32f46243b819f9cf84edbe1683a Mon Sep 17 00:00:00 2001 From: Alex Willmer Date: Thu, 28 Mar 2024 13:59:10 +0000 Subject: [PATCH] ansible_mitogen: Handle AnsibleUnsafeText et al in Ansible >= 7 Follwing fixes in Ansible 7-9 for CVE-2023-5764 cating `AnsibleUnsafeBytes` & `AnsibleUnsafeText` to `bytes()` or `str()` requires special handling. The handling is Ansible specific, so it shouldn't go in the mitogen package but rather the ansible_mitogen package. `ansible_mitogen.utils.unsafe.cast()` is most like `mitogen.utils.cast()`. During development it began as `ansible_mitogen.utils.unsafe.unwrap_var()`, closer to an inverse of `ansible.utils.unsafe_procy.wrap_var()`. Future enhancements may move in this direction. refs #977, refs #1046 See also - https://github.com/advisories/GHSA-7j69-qfc3-2fq9 - https://github.com/ansible/ansible/pull/82293 - https://github.com/mitogen-hq/mitogen/wiki/AnsibleUnsafe-notes --- ansible_mitogen/connection.py | 18 ++-- ansible_mitogen/mixins.py | 12 +-- ansible_mitogen/services.py | 4 +- ansible_mitogen/utils/unsafe.py | 79 ++++++++++++++++ docs/changelog.rst | 2 + .../integration/action/remote_expand_user.yml | 8 +- .../async/result_shell_echo_hi.yml | 8 +- .../_end_play_if_not_sudo_linux.yml | 2 +- .../stub_connections/setns_lxc.yml | 2 +- .../stub_connections/setns_lxd.yml | 2 +- tests/ansible/tests/utils_unsafe_test.py | 92 +++++++++++++++++++ tests/image_prep/_container_setup.yml | 2 +- 12 files changed, 203 insertions(+), 28 deletions(-) create mode 100644 ansible_mitogen/utils/unsafe.py create mode 100644 tests/ansible/tests/utils_unsafe_test.py diff --git a/ansible_mitogen/connection.py b/ansible_mitogen/connection.py index 44caf9ac..dfc3aec4 100644 --- a/ansible_mitogen/connection.py +++ b/ansible_mitogen/connection.py @@ -43,7 +43,6 @@ import ansible.errors import ansible.plugins.connection import mitogen.core -import mitogen.utils import ansible_mitogen.mixins import ansible_mitogen.parsing @@ -51,6 +50,7 @@ import ansible_mitogen.process import ansible_mitogen.services import ansible_mitogen.target import ansible_mitogen.transport_config +import ansible_mitogen.utils.unsafe LOG = logging.getLogger(__name__) @@ -797,7 +797,7 @@ class Connection(ansible.plugins.connection.ConnectionBase): call_context=self.binding.get_service_context(), service_name='ansible_mitogen.services.ContextService', method_name='get', - stack=mitogen.utils.cast(list(stack)), + stack=ansible_mitogen.utils.unsafe.cast(list(stack)), ) except mitogen.core.CallError: LOG.warning('Connection failed; stack configuration was:\n%s', @@ -848,7 +848,7 @@ class Connection(ansible.plugins.connection.ConnectionBase): inventory_name, stack = self._build_stack() worker_model = ansible_mitogen.process.get_worker_model() self.binding = worker_model.get_binding( - mitogen.utils.cast(inventory_name) + ansible_mitogen.utils.unsafe.cast(inventory_name) ) self._connect_stack(stack) @@ -933,7 +933,7 @@ class Connection(ansible.plugins.connection.ConnectionBase): call_context=binding.get_service_context(), service_name='ansible_mitogen.services.ContextService', method_name='reset', - stack=mitogen.utils.cast(list(stack)), + stack=ansible_mitogen.utils.unsafe.cast(list(stack)), ) finally: binding.close() @@ -1011,8 +1011,8 @@ class Connection(ansible.plugins.connection.ConnectionBase): emulate_tty = (not in_data and sudoable) rc, stdout, stderr = self.get_chain().call( ansible_mitogen.target.exec_command, - cmd=mitogen.utils.cast(cmd), - in_data=mitogen.utils.cast(in_data), + cmd=ansible_mitogen.utils.unsafe.cast(cmd), + in_data=ansible_mitogen.utils.unsafe.cast(in_data), chdir=mitogen_chdir or self.get_default_cwd(), emulate_tty=emulate_tty, ) @@ -1039,7 +1039,7 @@ class Connection(ansible.plugins.connection.ConnectionBase): ansible_mitogen.target.transfer_file( context=self.context, # in_path may be AnsibleUnicode - in_path=mitogen.utils.cast(in_path), + in_path=ansible_mitogen.utils.unsafe.cast(in_path), out_path=out_path ) @@ -1057,7 +1057,7 @@ class Connection(ansible.plugins.connection.ConnectionBase): """ self.get_chain().call_no_reply( ansible_mitogen.target.write_path, - mitogen.utils.cast(out_path), + ansible_mitogen.utils.unsafe.cast(out_path), mitogen.core.Blob(data), mode=mode, utimes=utimes, @@ -1119,7 +1119,7 @@ class Connection(ansible.plugins.connection.ConnectionBase): call_context=self.binding.get_service_context(), service_name='mitogen.service.FileService', method_name='register', - path=mitogen.utils.cast(in_path) + path=ansible_mitogen.utils.unsafe.cast(in_path) ) # For now this must remain synchronous, as the action plug-in may have diff --git a/ansible_mitogen/mixins.py b/ansible_mitogen/mixins.py index 690998f1..9cc97a48 100644 --- a/ansible_mitogen/mixins.py +++ b/ansible_mitogen/mixins.py @@ -50,12 +50,12 @@ import ansible.plugins.action import mitogen.core import mitogen.select -import mitogen.utils import ansible_mitogen.connection import ansible_mitogen.planner import ansible_mitogen.target import ansible_mitogen.utils +import ansible_mitogen.utils.unsafe from ansible.module_utils._text import to_text @@ -187,7 +187,7 @@ class ActionModuleMixin(ansible.plugins.action.ActionBase): LOG.debug('_remote_file_exists(%r)', path) return self._connection.get_chain().call( ansible_mitogen.target.file_exists, - mitogen.utils.cast(path) + ansible_mitogen.utils.unsafe.cast(path) ) def _configure_module(self, module_name, module_args, task_vars=None): @@ -324,7 +324,7 @@ class ActionModuleMixin(ansible.plugins.action.ActionBase): # ~root/.ansible -> /root/.ansible return self._connection.get_chain(use_login=(not sudoable)).call( os.path.expanduser, - mitogen.utils.cast(path), + ansible_mitogen.utils.unsafe.cast(path), ) def get_task_timeout_secs(self): @@ -387,11 +387,11 @@ class ActionModuleMixin(ansible.plugins.action.ActionBase): ansible_mitogen.planner.Invocation( action=self, connection=self._connection, - module_name=mitogen.core.to_text(module_name), - module_args=mitogen.utils.cast(module_args), + module_name=ansible_mitogen.utils.unsafe.cast(mitogen.core.to_text(module_name)), + module_args=ansible_mitogen.utils.unsafe.cast(module_args), task_vars=task_vars, templar=self._templar, - env=mitogen.utils.cast(env), + env=ansible_mitogen.utils.unsafe.cast(env), wrap_async=wrap_async, timeout_secs=self.get_task_timeout_secs(), ) diff --git a/ansible_mitogen/services.py b/ansible_mitogen/services.py index b0f5c70e..3e9de652 100644 --- a/ansible_mitogen/services.py +++ b/ansible_mitogen/services.py @@ -52,10 +52,10 @@ import ansible.constants import mitogen.core import mitogen.service -import mitogen.utils import ansible_mitogen.loaders import ansible_mitogen.module_finder import ansible_mitogen.target +import ansible_mitogen.utils.unsafe LOG = logging.getLogger(__name__) @@ -91,7 +91,7 @@ def _get_candidate_temp_dirs(): remote_tmp = ansible.constants.DEFAULT_REMOTE_TMP system_tmpdirs = ('/var/tmp', '/tmp') - return mitogen.utils.cast([remote_tmp] + list(system_tmpdirs)) + return ansible_mitogen.utils.unsafe.cast([remote_tmp] + list(system_tmpdirs)) def key_from_dict(**kwargs): diff --git a/ansible_mitogen/utils/unsafe.py b/ansible_mitogen/utils/unsafe.py new file mode 100644 index 00000000..b2c3d533 --- /dev/null +++ b/ansible_mitogen/utils/unsafe.py @@ -0,0 +1,79 @@ +from __future__ import absolute_import, division, print_function +__metaclass__ = type + +import ansible +import ansible.utils.unsafe_proxy + +import ansible_mitogen.utils + +import mitogen +import mitogen.core +import mitogen.utils + +__all__ = [ + 'cast', +] + +def _cast_to_dict(obj): return {cast(k): cast(v) for k, v in obj.items()} +def _cast_to_list(obj): return [cast(v) for v in obj] +def _cast_unsafe(obj): return obj._strip_unsafe() +def _passthrough(obj): return obj + + +# A dispatch table to cast objects based on their exact type. +# This is an optimisation, reliable fallbacks are required (e.g. isinstance()) +_CAST_DISPATCH = { + bytes: bytes, + dict: _cast_to_dict, + list: _cast_to_list, + tuple: _cast_to_list, + mitogen.core.UnicodeType: mitogen.core.UnicodeType, +} +_CAST_DISPATCH.update({t: _passthrough for t in mitogen.utils.PASSTHROUGH}) + +if hasattr(ansible.utils.unsafe_proxy.AnsibleUnsafeText, '_strip_unsafe'): + _CAST_DISPATCH.update({ + ansible.utils.unsafe_proxy.AnsibleUnsafeBytes: _cast_unsafe, + ansible.utils.unsafe_proxy.AnsibleUnsafeText: _cast_unsafe, + ansible.utils.unsafe_proxy.NativeJinjaUnsafeText: _cast_unsafe, + }) +elif ansible_mitogen.utils.ansible_version[:2] <= (2, 16): + _CAST_DISPATCH.update({ + ansible.utils.unsafe_proxy.AnsibleUnsafeBytes: bytes, + ansible.utils.unsafe_proxy.AnsibleUnsafeText: mitogen.core.UnicodeType, + }) +else: + mitogen_ver = '.'.join(str(v) for v in mitogen.__version__) + raise ImportError("Mitogen %s can't unwrap Ansible %s AnsibleUnsafe objects" + % (mitogen_ver, ansible.__version__)) + + +def cast(obj): + """ + Return obj (or a copy) with subtypes of builtins cast to their supertype. + + This is an enhanced version of :func:`mitogen.utils.cast`. In addition it + handles ``ansible.utils.unsafe_proxy.AnsibleUnsafeText`` and variants. + + There are types handled by :func:`ansible.utils.unsafe_proxy.wrap_var()` + that this function currently does not handle (e.g. `set()`), or preserve + preserve (e.g. `tuple()`). Future enhancements may change this. + + :param obj: + Object to undecorate. + :returns: + Undecorated object. + """ + # Fast path: obj is a known type, dispatch directly + try: + unwrapper = _CAST_DISPATCH[type(obj)] + except KeyError: + pass + else: + return unwrapper(obj) + + # Slow path: obj is some unknown subclass + if isinstance(obj, dict): return _cast_to_dict(obj) + if isinstance(obj, (list, tuple)): return _cast_to_list(obj) + + return mitogen.utils.cast(obj) diff --git a/docs/changelog.rst b/docs/changelog.rst index 18cf66d5..cca3d595 100644 --- a/docs/changelog.rst +++ b/docs/changelog.rst @@ -24,6 +24,8 @@ Unreleased * :gh:issue:`1046` Raise :py:exc:`TypeError` in :func:`` when casting a string subtype to `bytes()` or `str()` fails. This is potentially an API breaking change. Failures previously passed silently. +* :gh:issue:`1046` Add :func:``, to cast + :class:`ansible.utils.unsafe_proxy.AnsibleUnsafe` objects in Ansible 7+. v0.3.5 (2024-03-17) diff --git a/tests/ansible/integration/action/remote_expand_user.yml b/tests/ansible/integration/action/remote_expand_user.yml index 3a675635..97dd02f9 100644 --- a/tests/ansible/integration/action/remote_expand_user.yml +++ b/tests/ansible/integration/action/remote_expand_user.yml @@ -25,7 +25,7 @@ sudoable: false register: out - assert: - that: out.result == '{{user_facts.ansible_facts.ansible_user_dir}}/foo' + that: out.result == user_facts.ansible_facts.ansible_user_dir ~ '/foo' fail_msg: out={{out}} - name: "Expand ~/foo with become active. ~ is become_user's home." @@ -48,7 +48,7 @@ sudoable: false register: out - assert: - that: out.result == '{{user_facts.ansible_facts.ansible_user_dir}}/foo' + that: out.result == user_facts.ansible_facts.ansible_user_dir ~ '/foo' fail_msg: out={{out}} - name: "Expanding $HOME/foo has no effect." @@ -72,7 +72,7 @@ sudoable: true register: out - assert: - that: out.result == '{{user_facts.ansible_facts.ansible_user_dir}}/foo' + that: out.result == user_facts.ansible_facts.ansible_user_dir ~ '/foo' fail_msg: out={{out}} - name: "sudoable; Expand ~/foo with become active. ~ is become_user's home." @@ -96,7 +96,7 @@ sudoable: true register: out - assert: - that: out.result == '{{user_facts.ansible_facts.ansible_user_dir}}/foo' + that: out.result == user_facts.ansible_facts.ansible_user_dir ~ '/foo' fail_msg: out={{out}} - name: "sudoable; Expanding $HOME/foo has no effect." diff --git a/tests/ansible/integration/async/result_shell_echo_hi.yml b/tests/ansible/integration/async/result_shell_echo_hi.yml index f327a965..8cac07a8 100644 --- a/tests/ansible/integration/async/result_shell_echo_hi.yml +++ b/tests/ansible/integration/async/result_shell_echo_hi.yml @@ -32,10 +32,12 @@ - async_out.invocation.module_args.creates == None - async_out.invocation.module_args.executable == None - async_out.invocation.module_args.removes == None - # In Ansible 4 (ansible-core 2.11) the warn parameter is deprecated and defaults to false. - # It's scheduled for removal in ansible-core 2.13. - - (ansible_version.full is version("2.11", "<", strict=True) and async_out.invocation.module_args.warn == True) + # | Ansible <= 3 | ansible-core <= 2.10 | present | True | + # | Ansible 4 - 6 | ansible-core 2.11 - 2.13 | deprecated | False | + # | Ansible >= 7 | ansible-core >= 2.14 | absent | n/a | + - (ansible_version.full is version("2.14", ">=", strict=True) and async_out.invocation.module_args.warn is not defined) or (ansible_version.full is version("2.11", ">=", strict=True) and async_out.invocation.module_args.warn == False) + or (async_out.invocation.module_args.warn == True) - async_out.rc == 0 - async_out.start.startswith("20") - async_out.stderr == "there" diff --git a/tests/ansible/integration/stub_connections/_end_play_if_not_sudo_linux.yml b/tests/ansible/integration/stub_connections/_end_play_if_not_sudo_linux.yml index 55997a72..a53f75ed 100644 --- a/tests/ansible/integration/stub_connections/_end_play_if_not_sudo_linux.yml +++ b/tests/ansible/integration/stub_connections/_end_play_if_not_sudo_linux.yml @@ -9,7 +9,7 @@ - command: sudo -n whoami args: - warn: false + warn: "{{ False if ansible_version.full is version('2.10', '<=', strict=True) else omit }}" ignore_errors: true register: sudo_available diff --git a/tests/ansible/integration/stub_connections/setns_lxc.yml b/tests/ansible/integration/stub_connections/setns_lxc.yml index 75d4207b..489f9883 100644 --- a/tests/ansible/integration/stub_connections/setns_lxc.yml +++ b/tests/ansible/integration/stub_connections/setns_lxc.yml @@ -27,7 +27,7 @@ localhost args: chdir: ../.. - warn: false + warn: "{{ False if ansible_version.full is version('2.10', '<=', strict=True) else omit }}" register: result - assert: diff --git a/tests/ansible/integration/stub_connections/setns_lxd.yml b/tests/ansible/integration/stub_connections/setns_lxd.yml index 3f3bc291..d654b7bc 100644 --- a/tests/ansible/integration/stub_connections/setns_lxd.yml +++ b/tests/ansible/integration/stub_connections/setns_lxd.yml @@ -27,7 +27,7 @@ localhost args: chdir: ../.. - warn: false + warn: "{{ False if ansible_version.full is version('2.10', '<=', strict=True) else omit }}" register: result - assert: diff --git a/tests/ansible/tests/utils_unsafe_test.py b/tests/ansible/tests/utils_unsafe_test.py new file mode 100644 index 00000000..a020f55b --- /dev/null +++ b/tests/ansible/tests/utils_unsafe_test.py @@ -0,0 +1,92 @@ +import unittest + +from ansible.utils.unsafe_proxy import AnsibleUnsafeBytes +from ansible.utils.unsafe_proxy import AnsibleUnsafeText +from ansible.utils.unsafe_proxy import wrap_var + +import ansible_mitogen.utils.unsafe + +import mitogen.core + + +class Bytes(bytes): pass +class Dict(dict): pass +class List(list): pass +class Set(set): pass +class Text(mitogen.core.UnicodeType): pass +class Tuple(tuple): pass + + +class CastTest(unittest.TestCase): + def assertIsType(self, obj, cls, msg=None): + self.assertIs(type(obj), cls, msg) + + def assertUnchanged(self, obj): + self.assertIs(ansible_mitogen.utils.unsafe.cast(obj), obj) + + def assertCasts(self, obj, expected): + cast = ansible_mitogen.utils.unsafe.cast + self.assertEqual(cast(obj), expected) + self.assertIsType(cast(obj), type(expected)) + + def test_ansible_unsafe(self): + self.assertCasts(AnsibleUnsafeBytes(b'abc'), b'abc') + self.assertCasts(AnsibleUnsafeText(u'abc'), u'abc') + + def test_passthrough(self): + self.assertUnchanged(0) + self.assertUnchanged(0.0) + self.assertUnchanged(False) + self.assertUnchanged(True) + self.assertUnchanged(None) + self.assertUnchanged(b'') + self.assertUnchanged(u'') + + def test_builtins_roundtrip(self): + self.assertCasts(wrap_var(b''), b'') + self.assertCasts(wrap_var({}), {}) + self.assertCasts(wrap_var([]), []) + self.assertCasts(wrap_var(u''), u'') + self.assertCasts(wrap_var(()), []) + + def test_subtypes_roundtrip(self): + self.assertCasts(wrap_var(Bytes()), b'') + self.assertCasts(wrap_var(Dict()), {}) + self.assertCasts(wrap_var(List()), []) + self.assertCasts(wrap_var(Text()), u'') + self.assertCasts(wrap_var(Tuple()), []) + + def test_subtype_nested_dict(self): + obj = Dict(foo=Dict(bar=u'abc')) + wrapped = wrap_var(obj) + unwrapped = ansible_mitogen.utils.unsafe.cast(wrapped) + self.assertEqual(unwrapped, {'foo': {'bar': u'abc'}}) + self.assertIsType(unwrapped, dict) + self.assertIsType(unwrapped['foo'], dict) + self.assertIsType(unwrapped['foo']['bar'], mitogen.core.UnicodeType) + + def test_subtype_roundtrip_list(self): + # wrap_var() preserves sequence types, cast() does not (for now) + obj = List([List([u'abc'])]) + wrapped = wrap_var(obj) + unwrapped = ansible_mitogen.utils.unsafe.cast(wrapped) + self.assertEqual(unwrapped, [[u'abc']]) + self.assertIsType(unwrapped, list) + self.assertIsType(unwrapped[0], list) + self.assertIsType(unwrapped[0][0], mitogen.core.UnicodeType) + + def test_subtype_roundtrip_tuple(self): + # wrap_var() preserves sequence types, cast() does not (for now) + obj = Tuple([Tuple([u'abc'])]) + wrapped = wrap_var(obj) + unwrapped = ansible_mitogen.utils.unsafe.cast(wrapped) + self.assertEqual(unwrapped, [[u'abc']]) + self.assertIsType(unwrapped, list) + self.assertIsType(unwrapped[0], list) + self.assertIsType(unwrapped[0][0], mitogen.core.UnicodeType) + + def test_unknown_types_raise(self): + cast = ansible_mitogen.utils.unsafe.cast + self.assertRaises(TypeError, cast, set()) + self.assertRaises(TypeError, cast, Set()) + self.assertRaises(TypeError, cast, 4j) diff --git a/tests/image_prep/_container_setup.yml b/tests/image_prep/_container_setup.yml index 353a7d5b..d41d1326 100644 --- a/tests/image_prep/_container_setup.yml +++ b/tests/image_prep/_container_setup.yml @@ -57,7 +57,7 @@ dnf: dnf clean all command: "{{ clean_command[ansible_pkg_mgr] }}" args: - warn: false + warn: "{{ False if ansible_version.full is version('2.10', '<=', strict=True) else omit }}" - name: Clean up apt package lists shell: rm -rf {{item}}/*