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

@@ -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)."

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.
+    description:
-    default: false
+      - 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)

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