From e9e6001263f51103e96e58ad382660df0f3d0e39 Mon Sep 17 00:00:00 2001 From: Jordan Borean Date: Tue, 25 Feb 2025 04:59:04 +1000 Subject: [PATCH] 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 | 5 + lib/ansible/plugins/connection/winrm.py | 91 ++++---------- test/units/plugins/connection/test_winrm.py | 122 +------------------ 3 files changed, 28 insertions(+), 190 deletions(-) create mode 100644 changelogs/fragments/winrm-kinit-pexpect.yml diff --git a/changelogs/fragments/winrm-kinit-pexpect.yml b/changelogs/fragments/winrm-kinit-pexpect.yml new file mode 100644 index 00000000000..004987f6751 --- /dev/null +++ b/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. diff --git a/lib/ansible/plugins/connection/winrm.py b/lib/ansible/plugins/connection/winrm.py index 354acce7fad..86014690540 100644 --- a/lib/ansible/plugins/connection/winrm.py +++ b/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), "") - 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) diff --git a/test/units/plugins/connection/test_winrm.py b/test/units/plugins/connection/test_winrm.py index d5b76ca8f26..d11d60469db 100644 --- a/test/units/plugins/connection/test_winrm.py +++ b/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" - - 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" def test_exec_command_with_timeout(self, monkeypatch):