winrm - Remove pexpect kinit code (#84735)

Removes the use of pexpect in the winrm connection plugin and rely on
just subprocess. In the past pexpect was used for macOS compatibility so
that it could handle the TTY prompt but after testing it seems like
subprocess with `start_new_session=True` is enough to get it reading
from stdin on all platforms. This simplifies the code as there's no
longer an optional library changing how things are called and will work
out of the box.

changelogs/fragments/winrm-kinit-pexpect.yml

@@ -0,0 +1,5 @@
+minor_changes:
+  - >-
+    winrm - Remove need for pexpect on macOS hosts when using ``kinit`` to retrieve the Kerberos TGT.
+    By default the code will now only use the builtin ``subprocess`` library which should handle issues
+    with select and a high fd count and also simplify the code.

lib/ansible/plugins/connection/winrm.py

@@ -117,10 +117,6 @@ DOCUMENTATION = """
             - kerberos usage mode.
             - The managed option means Ansible will obtain kerberos ticket.
             - While the manual one means a ticket must already have been obtained by the user.
-            - If having issues with Ansible freezing when trying to obtain the
-              Kerberos ticket, you can either set this to V(manual) and obtain
-              it outside Ansible or install C(pexpect) through pip and try
-              again.
         choices: [managed, manual]
         vars:
           - name: ansible_winrm_kinit_mode
@@ -223,19 +219,6 @@ except ImportError as e:
     HAS_XMLTODICT = False
     XMLTODICT_IMPORT_ERR = e
 
-HAS_PEXPECT = False
-try:
-    import pexpect
-    # echo was added in pexpect 3.3+ which is newer than the RHEL package
-    # we can only use pexpect for kerb auth if echo is a valid kwarg
-    # https://github.com/ansible/ansible/issues/43462
-    if hasattr(pexpect, 'spawn'):
-        argspec = getfullargspec(pexpect.spawn.__init__)
-        if 'echo' in argspec.args:
-            HAS_PEXPECT = True
-except ImportError as e:
-    pass
-
 # used to try and parse the hostname and detect if IPv6 is being used
 try:
     import ipaddress
@@ -350,6 +333,7 @@ class Connection(ConnectionBase):
     def _kerb_auth(self, principal: str, password: str) -> None:
         if password is None:
             password = ""
+        b_password = to_bytes(password, encoding='utf-8', errors='surrogate_or_strict')
 
         self._kerb_ccache = tempfile.NamedTemporaryFile()
         display.vvvvv("creating Kerberos CC at %s" % self._kerb_ccache.name)
@@ -376,60 +360,28 @@ class Connection(ConnectionBase):
 
         kinit_cmdline.append(principal)
 
-        # pexpect runs the process in its own pty so it can correctly send
-        # the password as input even on MacOS which blocks subprocess from
-        # doing so. Unfortunately it is not available on the built in Python
-        # so we can only use it if someone has installed it
-        if HAS_PEXPECT:
-            proc_mechanism = "pexpect"
-            command = kinit_cmdline.pop(0)
-            password = to_text(password, encoding='utf-8',
-                               errors='surrogate_or_strict')
-
-            display.vvvv("calling kinit with pexpect for principal %s"
-                         % principal)
-            try:
-                child = pexpect.spawn(command, kinit_cmdline, timeout=60,
-                                      env=krb5env, echo=False)
-            except pexpect.ExceptionPexpect as err:
-                err_msg = "Kerberos auth failure when calling kinit cmd " \
-                          "'%s': %s" % (command, to_native(err))
-                raise AnsibleConnectionFailure(err_msg)
-
-            try:
-                child.expect(".*:")
-                child.sendline(password)
-            except OSError as err:
-                # child exited before the pass was sent, Ansible will raise
-                # error based on the rc below, just display the error here
-                display.vvvv("kinit with pexpect raised OSError: %s"
-                             % to_native(err))
-
-            # technically this is the stdout + stderr but to match the
-            # subprocess error checking behaviour, we will call it stderr
-            stderr = child.read()
-            child.wait()
-            rc = child.exitstatus
-        else:
-            proc_mechanism = "subprocess"
-            b_password = to_bytes(password, encoding='utf-8',
-                                  errors='surrogate_or_strict')
+        display.vvvv(f"calling kinit for principal {principal}")
 
-            display.vvvv("calling kinit with subprocess for principal %s"
-                         % principal)
-            try:
-                p = subprocess.Popen(kinit_cmdline, stdin=subprocess.PIPE,
-                                     stdout=subprocess.PIPE,
-                                     stderr=subprocess.PIPE,
-                                     env=krb5env)
+        # It is important to use start_new_session which spawns the process
+        # with setsid() to avoid it inheriting the current tty. On macOS it
+        # will force it to read from stdin rather than the tty.
+        try:
+            p = subprocess.Popen(
+                kinit_cmdline,
+                start_new_session=True,
+                stdin=subprocess.PIPE,
+                stdout=subprocess.PIPE,
+                stderr=subprocess.PIPE,
+                env=krb5env,
+            )
 
-            except OSError as err:
-                err_msg = "Kerberos auth failure when calling kinit cmd " \
-                          "'%s': %s" % (self._kinit_cmd, to_native(err))
-                raise AnsibleConnectionFailure(err_msg)
+        except OSError as err:
+            err_msg = "Kerberos auth failure when calling kinit cmd " \
+                      "'%s': %s" % (self._kinit_cmd, to_native(err))
+            raise AnsibleConnectionFailure(err_msg)
 
-            stdout, stderr = p.communicate(b_password + b'\n')
-            rc = p.returncode != 0
+        stdout, stderr = p.communicate(b_password + b'\n')
+        rc = p.returncode
 
         if rc != 0:
             # one last attempt at making sure the password does not exist
@@ -437,8 +389,7 @@ class Connection(ConnectionBase):
             exp_msg = to_native(stderr.strip())
             exp_msg = exp_msg.replace(to_native(password), "<redacted>")
 
-            err_msg = "Kerberos auth failure for principal %s with %s: %s" \
-                      % (principal, proc_mechanism, exp_msg)
+            err_msg = f"Kerberos auth failure for principal {principal}: {exp_msg}"
             raise AnsibleConnectionFailure(err_msg)
 
         display.vvvvv("kinit succeeded for principal %s" % principal)

test/units/plugins/connection/test_winrm.py

@@ -242,7 +242,6 @@ class TestWinRMKerbAuth(object):
         mock_popen.return_value.returncode = 0
         monkeypatch.setattr("subprocess.Popen", mock_popen)
 
-        winrm.HAS_PEXPECT = False
         pc = PlayContext()
         new_stdin = StringIO()
         conn = connection_loader.get('winrm', pc, new_stdin)
@@ -258,46 +257,6 @@ class TestWinRMKerbAuth(object):
         assert actual_env['KRB5CCNAME'].startswith("FILE:/")
         assert actual_env['PATH'] == os.environ['PATH']
 
-    @pytest.mark.parametrize('options, expected', [
-        [{"_extras": {}},
-         ("kinit", ["user@domain"],)],
-        [{"_extras": {}, 'ansible_winrm_kinit_cmd': 'kinit2'},
-         ("kinit2", ["user@domain"],)],
-        [{"_extras": {'ansible_winrm_kerberos_delegation': True}},
-         ("kinit", ["-f", "user@domain"],)],
-        [{"_extras": {}, 'ansible_winrm_kinit_args': '-f -p'},
-         ("kinit", ["-f", "-p", "user@domain"],)],
-        [{"_extras": {}, 'ansible_winrm_kerberos_delegation': True, 'ansible_winrm_kinit_args': '-p'},
-         ("kinit", ["-p", "user@domain"],)]
-    ])
-    def test_kinit_success_pexpect(self, monkeypatch, options, expected):
-        pytest.importorskip("pexpect")
-        mock_pexpect = MagicMock()
-        mock_pexpect.return_value.exitstatus = 0
-        monkeypatch.setattr("pexpect.spawn", mock_pexpect)
-
-        winrm.HAS_PEXPECT = True
-        pc = PlayContext()
-        new_stdin = StringIO()
-        conn = connection_loader.get('winrm', pc, new_stdin)
-        conn.set_options(var_options=options)
-        conn._build_winrm_kwargs()
-
-        conn._kerb_auth("user@domain", "pass")
-        mock_calls = mock_pexpect.mock_calls
-        assert mock_calls[0][1] == expected
-        actual_env = mock_calls[0][2]['env']
-        assert sorted(list(actual_env.keys())) == ['KRB5CCNAME', 'PATH']
-        assert actual_env['KRB5CCNAME'].startswith("FILE:/")
-        assert actual_env['PATH'] == os.environ['PATH']
-        assert mock_calls[0][2]['echo'] is False
-        assert mock_calls[1][0] == "().expect"
-        assert mock_calls[1][1] == (".*:",)
-        assert mock_calls[2][0] == "().sendline"
-        assert mock_calls[2][1] == ("pass",)
-        assert mock_calls[3][0] == "().read"
-        assert mock_calls[4][0] == "().wait"
-
     def test_kinit_with_missing_executable_subprocess(self, monkeypatch):
         expected_err = "[Errno 2] No such file or directory: " \
                        "'/fake/kinit': '/fake/kinit'"
@@ -305,30 +264,6 @@ class TestWinRMKerbAuth(object):
 
         monkeypatch.setattr("subprocess.Popen", mock_popen)
 
-        winrm.HAS_PEXPECT = False
-        pc = PlayContext()
-        new_stdin = StringIO()
-        conn = connection_loader.get('winrm', pc, new_stdin)
-        options = {"_extras": {}, "ansible_winrm_kinit_cmd": "/fake/kinit"}
-        conn.set_options(var_options=options)
-        conn._build_winrm_kwargs()
-
-        with pytest.raises(AnsibleConnectionFailure) as err:
-            conn._kerb_auth("user@domain", "pass")
-        assert str(err.value) == "Kerberos auth failure when calling " \
-                                 "kinit cmd '/fake/kinit': %s" % expected_err
-
-    def test_kinit_with_missing_executable_pexpect(self, monkeypatch):
-        pexpect = pytest.importorskip("pexpect")
-
-        expected_err = "The command was not found or was not " \
-                       "executable: /fake/kinit"
-        mock_pexpect = \
-            MagicMock(side_effect=pexpect.ExceptionPexpect(expected_err))
-
-        monkeypatch.setattr("pexpect.spawn", mock_pexpect)
-
-        winrm.HAS_PEXPECT = True
         pc = PlayContext()
         new_stdin = StringIO()
         conn = connection_loader.get('winrm', pc, new_stdin)
@@ -353,32 +288,6 @@ class TestWinRMKerbAuth(object):
         mock_popen.return_value.returncode = 1
         monkeypatch.setattr("subprocess.Popen", mock_popen)
 
-        winrm.HAS_PEXPECT = False
-        pc = PlayContext()
-        new_stdin = StringIO()
-        conn = connection_loader.get('winrm', pc, new_stdin)
-        conn.set_options(var_options={"_extras": {}})
-        conn._build_winrm_kwargs()
-
-        with pytest.raises(AnsibleConnectionFailure) as err:
-            conn._kerb_auth("invaliduser", "pass")
-
-        assert str(err.value) == \
-            "Kerberos auth failure for principal invaliduser with " \
-            "subprocess: %s" % (expected_err)
-
-    def test_kinit_error_pexpect(self, monkeypatch):
-        pytest.importorskip("pexpect")
-
-        expected_err = "Configuration file does not specify default realm"
-        mock_pexpect = MagicMock()
-        mock_pexpect.return_value.expect = MagicMock(side_effect=OSError)
-        mock_pexpect.return_value.read.return_value = to_bytes(expected_err)
-        mock_pexpect.return_value.exitstatus = 1
-
-        monkeypatch.setattr("pexpect.spawn", mock_pexpect)
-
-        winrm.HAS_PEXPECT = True
         pc = PlayContext()
         new_stdin = StringIO()
         conn = connection_loader.get('winrm', pc, new_stdin)
@@ -389,8 +298,7 @@ class TestWinRMKerbAuth(object):
             conn._kerb_auth("invaliduser", "pass")
 
         assert str(err.value) == \
-            "Kerberos auth failure for principal invaliduser with " \
-            "pexpect: %s" % (expected_err)
+            "Kerberos auth failure for principal invaliduser: %s" % (expected_err)
 
     def test_kinit_error_pass_in_output_subprocess(self, monkeypatch):
         def mock_communicate(input=None, timeout=None):
@@ -401,32 +309,6 @@ class TestWinRMKerbAuth(object):
         mock_popen.return_value.returncode = 1
         monkeypatch.setattr("subprocess.Popen", mock_popen)
 
-        winrm.HAS_PEXPECT = False
-        pc = PlayContext()
-        new_stdin = StringIO()
-        conn = connection_loader.get('winrm', pc, new_stdin)
-        conn.set_options(var_options={"_extras": {}})
-        conn._build_winrm_kwargs()
-
-        with pytest.raises(AnsibleConnectionFailure) as err:
-            conn._kerb_auth("username", "password")
-        assert str(err.value) == \
-            "Kerberos auth failure for principal username with subprocess: " \
-            "Error with kinit\n<redacted>"
-
-    def test_kinit_error_pass_in_output_pexpect(self, monkeypatch):
-        pytest.importorskip("pexpect")
-
-        mock_pexpect = MagicMock()
-        mock_pexpect.return_value.expect = MagicMock()
-        mock_pexpect.return_value.read.return_value = \
-            b"Error with kinit\npassword\n"
-        mock_pexpect.return_value.exitstatus = 1
-
-        monkeypatch.setattr("pexpect.spawn", mock_pexpect)
-
-        winrm.HAS_PEXPECT = True
-        pc = PlayContext()
         pc = PlayContext()
         new_stdin = StringIO()
         conn = connection_loader.get('winrm', pc, new_stdin)
@@ -436,7 +318,7 @@ class TestWinRMKerbAuth(object):
         with pytest.raises(AnsibleConnectionFailure) as err:
             conn._kerb_auth("username", "password")
         assert str(err.value) == \
-            "Kerberos auth failure for principal username with pexpect: " \
+            "Kerberos auth failure for principal username: " \
             "Error with kinit\n<redacted>"
 
     def test_exec_command_with_timeout(self, monkeypatch):