New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

psrp: Added new Windows connection plugin #41729

Merged
merged 2 commits into from Aug 21, 2018

Conversation

Projects
None yet
4 participants
@jborean93
Copy link
Contributor

jborean93 commented Jun 20, 2018

SUMMARY

Initial WIP PR that adds supports for executing commands on Windows host through PSRP. This still uses WinRM as the underlying transport but instead executes scripts in a PowerShell runspace theoretically saving time on each task invocation (still to be proven). The changes so far is the new connection plugin as well as changes to the powershell shell plugin to accomodate these changes.

Note: this has not been fully tested to be backwards compatible with the existing winrm connection yet

Requires https://github.com/jborean93/pypsrp to be installed until a release is made.

ISSUE TYPE
  • Feature Pull Request
COMPONENT NAME

connection/psrp

ANSIBLE VERSION
devel
@ansibot

This comment has been minimized.

Copy link
Contributor

ansibot commented Jun 20, 2018

@ansibot

This comment was marked as resolved.

Copy link
Contributor

ansibot commented Jun 20, 2018

The test ansible-test sanity --test pep8 [explain] failed with 25 errors:

lib/ansible/plugins/connection/psrp.py:135:1: W293 blank line contains whitespace
lib/ansible/plugins/connection/psrp.py:365:1: W293 blank line contains whitespace
lib/ansible/plugins/connection/psrp.py:425:72: E502 the backslash is redundant between brackets
lib/ansible/plugins/connection/psrp.py:426:24: E128 continuation line under-indented for visual indent
lib/ansible/plugins/connection/psrp.py:531:29: E222 multiple spaces after operator
lib/ansible/plugins/shell/powershell.py:1146:1: W293 blank line contains whitespace
lib/ansible/plugins/shell/powershell.py:1158:1: W293 blank line contains whitespace
lib/ansible/plugins/shell/powershell.py:1163:1: W293 blank line contains whitespace
lib/ansible/plugins/shell/powershell.py:1180:69: W291 trailing whitespace
lib/ansible/plugins/shell/powershell.py:1181:1: W293 blank line contains whitespace
lib/ansible/plugins/shell/powershell.py:1185:1: W293 blank line contains whitespace
lib/ansible/plugins/shell/powershell.py:1187:1: W293 blank line contains whitespace
lib/ansible/plugins/shell/powershell.py:1190:1: W293 blank line contains whitespace
lib/ansible/plugins/shell/powershell.py:1192:1: W293 blank line contains whitespace
lib/ansible/plugins/shell/powershell.py:1196:1: W293 blank line contains whitespace
lib/ansible/plugins/shell/powershell.py:1198:1: W293 blank line contains whitespace
lib/ansible/plugins/shell/powershell.py:1201:1: W293 blank line contains whitespace
lib/ansible/plugins/shell/powershell.py:1203:1: W293 blank line contains whitespace
lib/ansible/plugins/shell/powershell.py:1209:1: W293 blank line contains whitespace
lib/ansible/plugins/shell/powershell.py:1211:1: W293 blank line contains whitespace
lib/ansible/plugins/shell/powershell.py:1224:1: W293 blank line contains whitespace
lib/ansible/plugins/shell/powershell.py:1265:1: W293 blank line contains whitespace
lib/ansible/plugins/shell/powershell.py:1268:1: W293 blank line contains whitespace
lib/ansible/plugins/shell/powershell.py:1286:1: W293 blank line contains whitespace
lib/ansible/plugins/shell/powershell.py:1300:1: W293 blank line contains whitespace

click here for bot help

@jborean93 jborean93 removed the needs_triage label Jun 21, 2018

@jborean93 jborean93 force-pushed the jborean93:win-psrp branch Jul 12, 2018

@jborean93 jborean93 force-pushed the jborean93:win-psrp branch 3 times, most recently Jul 12, 2018

@ansibot

This comment has been minimized.

Copy link
Contributor

ansibot commented Jul 13, 2018

@ansibot ansibot added the module label Jul 13, 2018

@jborean93 jborean93 force-pushed the jborean93:win-psrp branch to 5c0543f Jul 20, 2018

@jborean93 jborean93 force-pushed the jborean93:win-psrp branch from 5c0543f Jul 24, 2018

@jborean93 jborean93 changed the title [WIP] psrp: Added new Windows connection plugin psrp: Added new Windows connection plugin Jul 24, 2018

@jborean93 jborean93 force-pushed the jborean93:win-psrp branch 3 times, most recently Jul 24, 2018

@ansibot

This comment has been minimized.

Copy link
Contributor

ansibot commented Jul 25, 2018

@ansibot ansibot added core_review and removed needs_revision labels Jul 25, 2018

@ansibot ansibot added the stale_ci label Aug 2, 2018

@jborean93 jborean93 force-pushed the jborean93:win-psrp branch Aug 5, 2018

@jborean93 jborean93 force-pushed the jborean93:win-psrp branch to 1773582 Aug 5, 2018

@ansibot ansibot removed the stale_ci label Aug 5, 2018

@jborean93 jborean93 requested a review from nitzmahone Aug 6, 2018

@ansibot ansibot added the stale_ci label Aug 19, 2018

@nitzmahone
Copy link
Member

nitzmahone left a comment

few minor typos and such, and a couple name/value bikeshedding things, but looks great

@@ -182,6 +182,7 @@ Function Extract-Zip($src, $dest) {
}
}
}
$archive.Dispose() # release the handle of the zip file

This comment has been minimized.

@nitzmahone

nitzmahone Aug 20, 2018

Member

I assume this was a problem for persistence (probably one of many)?

This comment has been minimized.

@jborean93

jborean93 Aug 21, 2018

Contributor

Yep, was something I came across when testing it, was a simple fix so thought I would just include it here anyway.

description:
- Run commands or put/fetch on a target via PSRP (WinRM plugin)
- This is similar to the I(winrm) connection plugin which uses the same
underlying transport but insteads runs in a PowerShell interpreter.

This comment has been minimized.

@nitzmahone

nitzmahone Aug 20, 2018

Member

instead

vars:
- name: ansible_port
- name: ansible_psrp_port
ssl:

This comment has been minimized.

@nitzmahone

nitzmahone Aug 20, 2018

Member

"ssl" is kind of passe these days- maybe: protocol with choices: [http, https] and vars: ansible_psrp_protocol?
Also future-proofs the config for a hypothetical "ssh" on the same connection plugin in the future...

cert_validation:
description:
- Whether to validate the remote server's certificate or not.
- Can also be set to the path of a PEM certificate chain to use in the

This comment has been minimized.

@nitzmahone

nitzmahone Aug 20, 2018

Member

Just because requests conflates validation and the source of validation truth doesn't necessarily mean we should- I've always disliked that in requests, as it prevents more granular cert validation behavior in the future (or at least makes it more complicated by requiring more config args). That's why I did the pywinrm one the way I did (left the separate arg for CA path and made validation mode a string/enum-y thing rather than boolean)

vars:
- name: ansible_psrp_connection_timeout
default: 30
encryption:

This comment has been minimized.

@nitzmahone

nitzmahone Aug 20, 2018

Member

should probably be message_encryption just to be clear

default: 30
encryption:
description:
- Controls the encryption settings.

This comment has been minimized.

@nitzmahone

nitzmahone Aug 20, 2018

Member

"message encryption settings"

- Set the proxy URL to use when connecting to the remote host.
vars:
- name: ansible_psrp_proxy
no_proxy:

This comment has been minimized.

@nitzmahone

nitzmahone Aug 20, 2018

Member

Maybe ignore_env_proxy? Granted, it's Friday and it's been a long week, but no_proxy: no (but ignored if proxy is set) made me a little https://media2.giphy.com/media/l2JegIy7RlfhNVsli/200.webp

@ansibot ansibot added needs_revision and removed core_review labels Aug 20, 2018

@jborean93

This comment has been minimized.

Copy link
Contributor

jborean93 commented Aug 21, 2018

Thanks @nitzmahone made the changes to the PR.

@ansibot ansibot removed the stale_ci label Aug 21, 2018

@dagwieers
Copy link
Member

dagwieers left a comment

Can't wait to see this hit the master branch.

- ntlm
- credssp
default: negotiate
cert_validation:

This comment has been minimized.

@dagwieers

dagwieers Aug 21, 2018

Member

Isn't the option for modules validate_certs ? It feels strange to introduce something new here.

Looking through sources I can only see module_utils/azure_rm_common.py doing this.

This comment has been minimized.

@dagwieers

dagwieers Aug 21, 2018

Member

For the httpapi plugin the parameter is validate_certs and config item ansible_httpapi_validate_certs.

This comment has been minimized.

@jborean93

jborean93 Aug 21, 2018

Contributor

This was mostly to replicate the actual pypsrp library but I don't mind changing it. In reality people would be setting ansible_psrp_cert_validation and not just cert_validation so not the exactly the same as module options. Probably makes sense to have this as a similar name though.

This comment has been minimized.

@dagwieers

dagwieers Aug 21, 2018

Member

TBH I prefer cert_validation as it uses namespace prefixes (and related options sort together). Maybe we ought to fix the httpapi connection plugin instead ? ;-)

@nitzmahone nitzmahone merged commit 6982dfc into ansible:devel Aug 21, 2018

1 check passed

Shippable Run 79807 status is SUCCESS.
Details

@jborean93 jborean93 deleted the jborean93:win-psrp branch Aug 22, 2018

@dagwieers

This comment has been minimized.

Copy link
Member

dagwieers commented Aug 23, 2018

@jborean93 The psrp connection plugin is working great here. There is one thing though, when providing -vvv it spews out a lot of connection-related output (and with a lot, I mean a huge lot) making -vvv useless for the common user.

(This is related to messages from: requests.packages.urllib3, pyprsp.messages, pypsrp.powershell, pypsrp.encryption, but mostly pypsrp.wsman can be quite verbose, I would move all this to -vvvvv or even ANSIBLE_DEBUG)

One reason for adding -vvv is to get pretty indented output from Ansible.

@jborean93

This comment has been minimized.

Copy link
Contributor

jborean93 commented Aug 27, 2018

@dagwieers are you sure you don't have ANSIBLE_DEBUG enabled? It sounds like the logging component of pypsrp is enabled if you are getting all those messages. Here's an output I have, I tried to make the output similar to the winrm plugin.

(ansible-py36) jborean:~/dev/module-tester$ ansible-playbook adhoc.yml -vvv
ansible-playbook 2.7.0.dev0 (ps_basic_utils 70d7513542) last updated 2018/08/27 06:47:31 (GMT +1000)
  config file = /Users/jborean/dev/module-tester/ansible.cfg
  configured module search path = ['/Users/jborean/.ansible/plugins/modules', '/usr/share/ansible/plugins/modules']
  ansible python module location = /Users/jborean/dev/ansible/lib/ansible
  executable location = /Users/jborean/dev/ansible/bin/ansible-playbook
  python version = 3.6.4 (default, Jan 18 2018, 12:35:18) [GCC 4.2.1 Compatible Apple LLVM 9.0.0 (clang-900.0.39.2)]
Using /Users/jborean/dev/module-tester/ansible.cfg as config file
Parsed /Users/jborean/dev/module-tester/inventory.ini inventory source with ini plugin

PLAYBOOK: adhoc.yml *************************************************************************************************************************
1 plays in adhoc.yml

PLAY [2016] *********************************************************************************************************************************
META: ran handlers

TASK [win_ping] *****************************************************************************************************************************
task path: /Users/jborean/dev/module-tester/adhoc.yml:6
Using module file /Users/jborean/dev/ansible/lib/ansible/modules/windows/win_ping.ps1
<SERVER2016.domain.local> ESTABLISH PSRP CONNECTION FOR USER: vagrant-domain@DOMAIN.LOCAL ON PORT 5986 TO SERVER2016.domain.local
<SERVER2016.domain.local> PSRP: EXEC (via pipeline wrapper)
ok: [SERVER2016.domain.local] => {
    "changed": false,
    "ping": "pong"
}

Note: I could be doing something wrong in the pypsrp side but logs should only be enabled unless someone sets PYPSRP_LOG_CFG to the path of a json formatted Python log config file.

@dagwieers

This comment has been minimized.

Copy link
Member

dagwieers commented Aug 27, 2018

I did not have ANSIBLE_DEBUG enabled, and switching to winrm connection did not show that many details. (There weren't any other messages being shown that are being shown when enabling ANSIBLE_DEBUG) I'll troubleshoot next attempt at using psrp (I had some issues with read timeouts and async).

Tomorrow9 added a commit to Tomorrow9/ansible that referenced this pull request Nov 20, 2018

psrp: Added new Windows connection plugin (ansible#41729)
* psrp: Added new Windows connection plugin

* Tweaks to connection options from review
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment