jborean93 / pypsrp

PowerShell Remoting Protocol for Python
MIT License
324 stars 49 forks source link

Variable "protocol_version" uncontrolled #173

Open gcilleruelobeltran opened 1 year ago

gcilleruelobeltran commented 1 year ago

Hi,

I've got next error:

Traceback (most recent call last):
  File "/usr/local/lib/python3.11/site-packages/ansible/executor/task_executor.py", line 158, in run
    res = self._execute()
          ^^^^^^^^^^^^^^^
  File "/usr/local/lib/python3.11/site-packages/ansible/executor/task_executor.py", line 645, in _execute
    self._handler.cleanup()
  File "/usr/local/lib/python3.11/site-packages/ansible/plugins/action/__init__.py", line 183, in cleanup
    self._remove_tmp_path(self._connection._shell.tmpdir)
  File "/usr/local/lib/python3.11/site-packages/ansible/plugins/action/__init__.py", line 521, in _remove_tmp_path
    tmp_rm_res = self._low_level_execute_command(cmd, sudoable=False)
                 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/usr/local/lib/python3.11/site-packages/ansible/plugins/action/__init__.py", line 1320, in _low_level_execute_command
    rc, stdout, stderr = self._connection.exec_command(cmd, in_data=in_data, sudoable=sudoable)
                         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/usr/local/lib/python3.11/site-packages/ansible/plugins/connection/psrp.py", line 463, in exec_command
    rc, stdout, stderr = self._exec_psrp_script(script, in_data)
                         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/usr/local/lib/python3.11/site-packages/ansible/plugins/connection/psrp.py", line 815, in _exec_psrp_script
    ps.add_script(script, use_local_scope=use_local_scope)
  File "/usr/local/lib/python3.11/site-packages/pypsrp/powershell.py", line 1017, in add_script
    command = Command(
              ^^^^^^^^
  File "/usr/local/lib/python3.11/site-packages/pypsrp/complex_objects.py", line 579, in __init__
    if version_equal_or_newer(protocol_version, "2.2"):
       ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/usr/local/lib/python3.11/site-packages/pypsrp/_utils.py", line 84, in version_equal_or_newer
    if int(version) < current_version:
       ^^^^^^^^^^^^
ValueError: invalid literal for int() with base 10: ''

The thing is that the "version" (which is linked to _procotolversion variable) variable comes empty but I think this could be controlled by assigning a default value before _"if int(version) < currentversion:" evaluation. It is quite curious that this hits an error when variable _"protocolversion" can be empty sometimes. The next code in script "/pypsrp/powershell.py" contemplates it to be empty:

command = Command(
            script,
            protocol_version=self.runspace_pool.protocol_version or "",
            is_script=True,
            use_local_scope=use_local_scope,
        )

Would this be a bug to be fixed or this error comes from another reason that I do not see?

I must say that the error ocurred when executing ansible module "win_updates".

Than you very much,

jborean93 commented 1 year ago

This does sound familiar but I'm unsure why it would be happening. The protocol_version should be set as it's returned by the first runspace pool setup message received from the server. This error would potentially indicate that the PowerShell class was created from a RunspacePool that hasn't been initialized yet. Theoretically this could happen if the connection was reset as per the following:

https://github.com/ansible/ansible/blob/7ef8e0e102388ae422b214eccffc381deeecadf1/lib/ansible/plugins/connection/psrp.py#L411-L425

This will remove the existing RunspacePool object and call _connect which is meant to create the RunspacePool. It seems like in your case it failed to do so and kept the lingering RunspacePool object around for use with the cleanup task that occurred at the end. This sounds like more of a bug in the psrp connection plugin rather than something wrong here. Are you able to open an issue on https://github.com/ansible/ansible/tree/devel?

gcilleruelobeltran commented 1 year ago

Hi thanks for the reply, is it possible that ansible module reset the RunspacePool by rebooting the machine? I use parameter reboot to delegate the module to manage rebooting if needed. The code looks like this:

name: Install updates.
win_updates:
  state: installed
  category_names: "..."
  reject_list: "..."
  log_path: ...
  reboot: True
  reboot_timeout: "{% if ... %}3600{% else %}5400{% endif %}"
register: windows_install
become: True
become_method: runas
become_user: "..."
vars:
  ansible_runas_flags: logon_type=new_credentials logon_flags=with_profile,netcredentials_only
  ansible_runas_pass: "..."
timeout: 10800

By the way, I've tried to open an issue in the link you sent me but it was impossible. When I hit "submit" It gets an error saying that the url does not exist.

jborean93 commented 1 year ago

It's most likely the module failed during the reboot phase for some reason and the reboot will continually reset the runspace pool. If it failed in a way where the runspace pool was closed/set to None but failed to be opened again then the cleanup task that runs as the end will most likely fail.

On a side note, any reason why you are using become/runas with this task?

gcilleruelobeltran commented 1 year ago

Hi, sorry i didn't reply earlier.

According to ansible documentation (https://docs.ansible.com/ansible/latest/os_guide/windows_winrm.html#winrm-limitations) "Use become to bypass all WinRM restrictions and run a command as it would locally. Unlike using an authentication transport like credssp, this will also remove the non-interactive restriction and API restrictions like WUA and DPAPI"

That is the main reason. We need to avoid the restrictions

jborean93 commented 1 year ago

The win_update module already works around it for you. The WUA example was only there if you call it yourself outside the module.

gcilleruelobeltran commented 1 year ago

All right that is good to know, we've changed that temporarly to see the behavior. Sometimes we got some problems with some machines that were resolved with that combination. The become_user was the same in all cases.