From aeb8d3f6560e8a3c5be902a36638b45a1a45a4c0 Mon Sep 17 00:00:00 2001 From: Mike Aldred Date: Sun, 26 Apr 2026 03:12:36 +0800 Subject: [PATCH] pfexec become plugin: fix broken defaults for illumos/SmartOS (#11623) * pfexec become plugin: fix broken defaults for illumos/SmartOS The pfexec become plugin has had incorrect defaults since it was migrated from Ansible core, making it unusable on illumos without manual workarounds: 1. become_flags defaulted to '-H -S -n' which are sudo flags. pfexec does not accept any of these options, causing: 'exec: illegal option -- H' 2. wrap_exe defaulted to false. Unlike sudo, pfexec does not interpret shell constructs internally. Since Ansible generates compound commands (echo BECOME-SUCCESS-xxx ; python3), these must be wrapped in /bin/sh -c for pfexec to execute them. These issues were originally reported in 2016 (ansible/ansible#15642), migrated to community.general as #3671, and partially fixed by PR #3889 in 2022 (which corrected quoting but not the defaults). Users have had to work around this with explicit inventory settings ever since. Changes: - become_flags default: '-H -S -n' -> '' (empty) - wrap_exe default: false -> true - build_become_command: handle empty flags cleanly - Updated tests to match corrected defaults - Added test for custom flags - Improved wrap_exe description to explain why it should be enabled * Update changelog fragment with PR number * Fix ruff formatting in test_pfexec.py * Address review feedback from russoz Remove redundant 'should generally be left enabled' description line and simplify become command return by removing unnecessary flags conditional. * Fix unit test regexes for empty default flags Match double space in test assertions when become_flags defaults to empty string, consistent with doas, dzdo, and pbrun test patterns. * pfexec become plugin: deprecate wrap_exe default rather than flipping Changing the wrap_exe default from false to true is a breaking change for the narrow case (e.g. ansible.builtin.raw) where the current default does work, so deprecate instead: remove the default, emit a deprecation warning when the option is unset, and treat that as false for now. Build the become command with " ".join() so an empty become_flags no longer produces a stray double space. Tests set wrap_exe explicitly so the deprecation warning does not fire during unit runs. * pfexec become plugin: target 14.0.0 for wrap_exe deprecation Per felixfontein's review, switch the deprecation target for the wrap_exe default from community.general 15.0.0 to 14.0.0, and reword the option description to mark the current default as deprecated rather than just "changing in a future release". --- changelogs/fragments/pfexec-fix-defaults.yml | 4 ++ plugins/become/pfexec.py | 32 +++++++++--- tests/unit/plugins/become/test_pfexec.py | 55 +++++++++++++++----- 3 files changed, 73 insertions(+), 18 deletions(-) create mode 100644 changelogs/fragments/pfexec-fix-defaults.yml diff --git a/changelogs/fragments/pfexec-fix-defaults.yml b/changelogs/fragments/pfexec-fix-defaults.yml new file mode 100644 index 0000000000..acd5fddd7b --- /dev/null +++ b/changelogs/fragments/pfexec-fix-defaults.yml @@ -0,0 +1,4 @@ +bugfixes: + - "pfexec become plugin - fix default ``become_flags`` from ``-H -S -n`` (sudo flags) to empty string, as ``pfexec`` does not accept these options (https://github.com/ansible-collections/community.general/pull/11623)." +deprecated_features: + - "pfexec become plugin - the default value of the ``wrap_exe`` option will change from ``false`` to ``true`` in community.general 14.0.0. The current default only works in very limited cases because ``pfexec`` does not interpret shell constructs internally. Set ``wrap_exe`` explicitly to silence the deprecation warning (https://github.com/ansible-collections/community.general/pull/11623)." diff --git a/plugins/become/pfexec.py b/plugins/become/pfexec.py index 8863b946d7..ab57ac312c 100644 --- a/plugins/become/pfexec.py +++ b/plugins/become/pfexec.py @@ -46,7 +46,7 @@ options: become_flags: description: Options to pass to C(pfexec). type: string - default: -H -S -n + default: "" ini: - section: privilege_escalation key: become_flags @@ -73,8 +73,14 @@ options: - section: pfexec_become_plugin key: password wrap_exe: - description: Toggle to wrap the command C(pfexec) calls in C(shell -c) or not. - default: false + description: + - Toggle to wrap the command C(pfexec) calls in C(shell -c) or not. + - Unlike C(sudo), C(pfexec) does not interpret shell constructs internally, + so commands containing shell operators must be wrapped in a shell invocation. + - The current default of V(false) only works in very limited cases (for example + with M(ansible.builtin.raw)). + - The current default is B(deprecated) and will change to V(true) in community.general 14.0.0. + To avoid the deprecation message, you can explicitly set this option to a value. type: bool ini: - section: pfexec_become_plugin @@ -88,6 +94,9 @@ notes: """ from ansible.plugins.become import BecomeBase +from ansible.utils.display import Display + +display = Display() class BecomeModule(BecomeBase): @@ -100,7 +109,18 @@ class BecomeModule(BecomeBase): return cmd exe = self.get_option("become_exe") - flags = self.get_option("become_flags") - noexe = not self.get_option("wrap_exe") - return f"{exe} {flags} {self._build_success_command(cmd, shell, noexe=noexe)}" + + wrap_exe = self.get_option("wrap_exe") + if wrap_exe is None: + display.deprecated( + "The default value of the wrap_exe option for the community.general.pfexec " + "become plugin will change from false to true in community.general 14.0.0. " + "Set wrap_exe explicitly to silence this warning.", + version="14.0.0", + collection_name="community.general", + ) + wrap_exe = False + + become_cmd = self._build_success_command(cmd, shell, noexe=not wrap_exe) + return " ".join(part for part in (exe, flags, become_cmd) if part) diff --git a/tests/unit/plugins/become/test_pfexec.py b/tests/unit/plugins/become/test_pfexec.py index 9ccddabd61..2d249bb771 100644 --- a/tests/unit/plugins/become/test_pfexec.py +++ b/tests/unit/plugins/become/test_pfexec.py @@ -14,56 +14,85 @@ from ansible import context from .helper import call_become_plugin -def test_pfexec_basic(mocker, parser, reset_cli_args): +def test_pfexec_wrap(mocker, parser, reset_cli_args): + """Test pfexec with wrap_exe explicitly enabled.""" options = parser.parse_args([]) context._init_global_context(options) default_cmd = "/bin/foo" default_exe = "/bin/bash" pfexec_exe = "pfexec" - pfexec_flags = "-H -S -n" success = "BECOME-SUCCESS-.+?" task = { "become_method": "community.general.pfexec", } - var_options = {} + var_options = { + "ansible_pfexec_wrap_execution": "true", + } cmd = call_become_plugin(task, var_options, cmd=default_cmd, executable=default_exe) print(cmd) - assert re.match(f"""{pfexec_exe} {pfexec_flags} 'echo {success}; {default_cmd}'""", cmd) is not None + assert re.match(f"""{pfexec_exe} {default_exe} -c 'echo {success}; {default_cmd}'""", cmd) is not None -def test_pfexec(mocker, parser, reset_cli_args): +def test_pfexec_no_wrap(mocker, parser, reset_cli_args): + """Test pfexec with wrap_exe explicitly disabled.""" options = parser.parse_args([]) context._init_global_context(options) default_cmd = "/bin/foo" default_exe = "/bin/bash" pfexec_exe = "pfexec" - pfexec_flags = "" success = "BECOME-SUCCESS-.+?" task = { - "become_user": "foo", + "become_method": "community.general.pfexec", + "become_flags": "", + } + var_options = { + "ansible_pfexec_wrap_execution": "false", + } + cmd = call_become_plugin(task, var_options, cmd=default_cmd, executable=default_exe) + print(cmd) + assert re.match(f"""{pfexec_exe} 'echo {success}; {default_cmd}'""", cmd) is not None + + +def test_pfexec_custom_flags(mocker, parser, reset_cli_args): + """Test pfexec with custom flags and wrap_exe enabled.""" + options = parser.parse_args([]) + context._init_global_context(options) + + default_cmd = "/bin/foo" + default_exe = "/bin/bash" + pfexec_exe = "pfexec" + pfexec_flags = "-P basic" + + success = "BECOME-SUCCESS-.+?" + + task = { "become_method": "community.general.pfexec", "become_flags": pfexec_flags, } - var_options = {} + var_options = { + "ansible_pfexec_wrap_execution": "true", + } cmd = call_become_plugin(task, var_options, cmd=default_cmd, executable=default_exe) print(cmd) - assert re.match(f"""{pfexec_exe} {pfexec_flags} 'echo {success}; {default_cmd}'""", cmd) is not None + assert ( + re.match(f"""{pfexec_exe} {pfexec_flags} {default_exe} -c 'echo {success}; {default_cmd}'""", cmd) is not None + ) def test_pfexec_varoptions(mocker, parser, reset_cli_args): + """Test that var_options override task options.""" options = parser.parse_args([]) context._init_global_context(options) default_cmd = "/bin/foo" default_exe = "/bin/bash" pfexec_exe = "pfexec" - pfexec_flags = "" success = "BECOME-SUCCESS-.+?" @@ -74,8 +103,10 @@ def test_pfexec_varoptions(mocker, parser, reset_cli_args): } var_options = { "ansible_become_user": "bar", - "ansible_become_flags": pfexec_flags, + "ansible_become_flags": "", + "ansible_pfexec_wrap_execution": "true", } cmd = call_become_plugin(task, var_options, cmd=default_cmd, executable=default_exe) print(cmd) - assert re.match(f"""{pfexec_exe} {pfexec_flags} 'echo {success}; {default_cmd}'""", cmd) is not None + # var_options override task flags, so flags should be empty + assert re.match(f"""{pfexec_exe} {default_exe} -c 'echo {success}; {default_cmd}'""", cmd) is not None