Skip to content
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

fix(tasks: synchronize): wrap in sshpass if ssh password was provided #30743

Merged
merged 8 commits into from
Nov 5, 2018

Conversation

actionless
Copy link
Contributor

SUMMARY

The action plugin detects the inventory data and connection type then juggles the connection to local and sets the remote host args for rsync. If the remote host required a password for auth and the user gave a password via the available methods, we would wrap ssh calls with sshpass. However, we completely ignore that in the action plugin for synchronize where we would probably know that a password is needed based on contexts. We should look into detecting that situation and wrapping rsync with sshpass.
sshpass somewhere in the the exec arguments and sending the password when the prompt occurs.

Fixes #16616

ISSUE TYPE
  • Bugfix Pull Request
COMPONENT NAME

synchronize

ANSIBLE VERSION

(could be easily backported to older versions if needed)

$ ansible --version
ansible 2.5.0
  config file = /etc/ansible/ansible.cfg
  configured module search path = [u'/home/user/.ansible/plugins/modules', u'/usr/share/ansible/plugins/modules']
  ansible python module location = /usr/lib/python2.7/site-packages/ansible
  executable location = /usr/sbin/ansible
  python version = 2.7.13 (default, Jul 21 2017, 03:24:34) [GCC 7.1.1 20170630]

@actionless actionless force-pushed the rsync-password-fix branch 2 times, most recently from a443665 to d516b7c Compare September 22, 2017 10:22
@ansibot
Copy link
Contributor

ansibot commented Sep 22, 2017

cc @tima
click here for bot help

@ansibot ansibot added affects_2.5 This issue/PR affects Ansible v2.5 bugfix_pull_request core_review In order to be merged, this PR must follow the core review workflow. module This issue/PR relates to a module. needs_triage Needs a first human triage before being processed. plugins/action support:core This issue/PR relates to code supported by the Ansible Engineering Team. labels Sep 22, 2017
@s-hertel s-hertel removed the needs_triage Needs a first human triage before being processed. label Sep 22, 2017
@ansibot ansibot added the stale_ci This PR has been tested by CI more than one week ago. Close and re-open this PR to get it retested. label Sep 30, 2017
@ansibot ansibot added the new_contributor This PR is the first contribution by a new community member. label Oct 18, 2017
@xrow
Copy link

xrow commented Oct 21, 2017

Sorry that seem to not work for me. Maybe I am testing not to right version. I did:

pip install git+git://github.com/actionless/ansible@rsync-password-fix

and ran my playbook.

`TASK [openhab : Sync config] ******************************************************************************************
root@centos-rpi3.xrow.lan's password:
fatal: [centos-rpi3.xrow.lan]: FAILED! => {"changed": false, "cmd": "/bin/rsync --delay-updates -F --compress --delete-after --archive --rsh=/bin/ssh -S none -o StrictHostKeyChecking=no -o UserKnownHostsFile=/dev/null --rsync-path=sudo rsync --out-format=<>%i %n%L /scm/smarthome.infrastructure/roles/openhab/files/conf/ root@centos-rpi3.xrow.lan:/etc/openhab/", "msg": "Warning: Permanently added 'centos-rpi3.xrow.lan,192.168.0.60' (ECDSA) to the list of known hosts.\r\nConnection closed by 192.168.0.60\r\nrsync: connection unexpectedly closed (0 bytes received so far) [sender]\nrsync error: error in rsync protocol data stream (code 12) at io.c(605) [sender=3.0.9]\n", "rc": 12}
to retry, use: --limit @/scm/smarthome.infrastructure/openhab.retry

PLAY RECAP ************************************************************************************************************
centos-rpi3.xrow.lan : ok=13 changed=0 unreachable=0 failed=1 `

@actionless
Copy link
Contributor Author

actionless commented Oct 21, 2017

@xrow could you extract from your playbook the exact block which will allow me to reproduce this problem?

and also the host variables used (not their values, but just to know if ansible_password, andsible_become and so on are set or not)

@xrow
Copy link

xrow commented Oct 23, 2017

Thank for your feedback. Here is some test code/output:

First run remote (fails), second fails. Local is a vagrant box and remote a rasberry.

[root@localhost smarthome.infrastructure]# ansible-playbook test.yml -i hosts 

PLAY [Synchronize Test] *****************************************************************************************************************

TASK [Gathering Facts] ******************************************************************************************************************
ok: [centos-rpi3.xrow.lan]

TASK [Echo] *****************************************************************************************************************************
changed: [centos-rpi3.xrow.lan]

TASK [Copy /etc/hostname] ***************************************************************************************************************
root@centos-rpi3.xrow.lan's password: 
 [ERROR]: User interrupted execution

[root@localhost smarthome.infrastructure]# ansible-playbook test.yml -i hosts --connection=local

PLAY [Synchronize Test] *****************************************************************************************************************

TASK [Gathering Facts] ******************************************************************************************************************
ok: [centos-rpi3.xrow.lan]

TASK [Echo] *****************************************************************************************************************************
changed: [centos-rpi3.xrow.lan]

TASK [Copy /etc/hostname] ***************************************************************************************************************
ok: [centos-rpi3.xrow.lan]

PLAY RECAP ******************************************************************************************************************************
centos-rpi3.xrow.lan       : ok=3    changed=1    unreachable=0    failed=0  

playbook

---

# ansible-playbook test.yml -i hosts 
# ansible-playbook test.yml -i hosts --connection=local

- name: Synchronize Test
  hosts: openhab
  become: true

  tasks:
  - name: Echo
    shell: echo "Hello"
  - name: Copy /etc/hostname
    synchronize: src=/etc/hostname dest=/root/test

@xrow
Copy link

xrow commented Oct 23, 2017

Sorry and hosts:

---

[openhab]
centos-rpi3.xrow.lan ansible_ssh_user=root ansible_ssh_pass=centos

@xrow
Copy link

xrow commented Oct 23, 2017

By the way, if I which to key based auth it works.

@actionless
Copy link
Contributor Author

actionless commented Oct 25, 2017

@xrow
i was not able to reproduce the issue

could it be the case what password authentication for root is disabled on the server?

are you able to manually run rsync with the same credentials?

@xrow
Copy link

xrow commented Oct 25, 2017

@actionless

To what setting are you refferring? If you are referring to sshd config, the sample echo task shouldn`t have been executed, correct?

@xrow
Copy link

xrow commented Oct 25, 2017

@actionless

This works by the way:

[root@localhost ~]# sshpass -p centos rsync /etc/hostname root@centos-rpi3.xrow.lan:/root/test
[root@localhost ~]# 

@actionless
Copy link
Contributor Author

actionless commented Oct 25, 2017

also i've just revisited the log you've attached above and saw a strange thing:

"cmd": "/bin/rsync --delay-updates -F --compress --delete-after --archive --rsh=/bin/ssh -S none -o StrictHostKeyChecking=no -o UserKnownHostsFile=/dev/null --rsync-path=sudo rsync --out-format=<>%i %n%L /scm/smarthome.infrastructure/roles/openhab/files/conf/ root@centos-rpi3.xrow.lan:/etc/openhab/", 

while in my case it was

"cmd": "sshpass -p ******** /usr/sbin/rsync --delay-updates -F --compress --archive --rsh=/usr/sbin/ssh -S none -o StrictHostKeyChecking=no -o UserKnownHostsFile=/dev/null --rsync-path=sudo rsync --out-format=<<CHANGED>>%i %n%L /home/user/testfile user@192.168.1.3:/opt/testfile"

@actionless
Copy link
Contributor Author

actionless commented Oct 25, 2017

mb try creating a new virtualenv and running ansible task there isolated?

$ virtualenv -p python2 env
$ source env/bin/activate
(env)$ pip install git+git://github.com/actionless/ansible@rsync-password-fix
(env)$ which ansible-playbook  # just to double-check what we're using ansible from the virtualenv
(env)$ ansible-playbook .......

@vTNT
Copy link

vTNT commented Oct 26, 2017

my ansible version is 2.3.2.0
and this pr it can work. thank you!!

@ansibot ansibot added needs_rebase https://docs.ansible.com/ansible/devel/dev_guide/developing_rebasing.html needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. and removed core_review In order to be merged, this PR must follow the core review workflow. new_contributor This PR is the first contribution by a new community member. labels Nov 3, 2017
@sivel
Copy link
Member

sivel commented Sep 21, 2018

now only one question remains, what to do with disallowed usage of subprocess.Popen

There are a few modules that need to use Popen instead of run_command, and in those cases we just have to add an ignore to test/sanity/validate-modules/ignore.txt.

Something like:

lib/ansible/modules/files/synchronize.py E210

Note: we try to keep that file sorted by file path

@actionless
Copy link
Contributor Author

@sivel and what do you think about the solution proposed in the latest commits?

… of subprocess.Popen

pass_fds only if they passed to run_command()
@actionless
Copy link
Contributor Author

at the moment only aws test are failing, but it seems what for some unrelated to this PR reason

@ansibot ansibot added stale_ci This PR has been tested by CI more than one week ago. Close and re-open this PR to get it retested. stale_review Updates were made after the last review and the last review is more than 7 days old. labels Oct 2, 2018
@abadger abadger dismissed their stale review October 31, 2018 15:20

I only used the change requested to stop the automatic rebuild_merge

@ansibot ansibot added core_review In order to be merged, this PR must follow the core review workflow. and removed needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. stale_review Updates were made after the last review and the last review is more than 7 days old. labels Oct 31, 2018
@abadger
Copy link
Contributor

abadger commented Oct 31, 2018

rebuild_merge

@ansibot ansibot added needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. and removed stale_ci This PR has been tested by CI more than one week ago. Close and re-open this PR to get it retested. core_review In order to be merged, this PR must follow the core review workflow. labels Oct 31, 2018
@actionless
Copy link
Contributor Author

aws tests are still unstable

@maxamillion
Copy link
Contributor

rebuild_merge

@actionless
Copy link
Contributor Author

@maxamillion aws still unstable but after this testrun also osx failed

@maxamillion
Copy link
Contributor

rebuild_merge

Dear CI Gods, please be kind.

@maxamillion
Copy link
Contributor

rebuild_merge

@ansibot ansibot added core_review In order to be merged, this PR must follow the core review workflow. and removed needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. labels Nov 5, 2018
@ansibot ansibot merged commit 1403744 into ansible:devel Nov 5, 2018
@actionless
Copy link
Contributor Author

🎆 🌵 🐱 🚬 🎆

mjmayer pushed a commit to mjmayer/ansible that referenced this pull request Nov 30, 2018
…ansible#30743)

* fix(tasks: synchronize): wrap in sshpass if ssh password was provided

Closes ansible#16616

* fix(tasks: synchronize): pass rsync password to sshpass via fd

* fix(tasks: synchronize): use fail_json instead of AnsibleError

* fixup! fix(tasks: synchronize): use fail_json instead of AnsibleError

fix python2 handling

* feat(module_utils: basic: run_command): add optional arguments `pass_fds` and `before_communicate_callback`

* fix(tasks: synchronize): use module.run_command instead of subprocess.Popen

* fixup! fix(tasks: synchronize): use module.run_command instead of subprocess.Popen

remove unused import

* fixup! fixup! fix(tasks: synchronize): use module.run_command instead of subprocess.Popen

pass_fds only if they passed to run_command()
Tomorrow9 pushed a commit to Tomorrow9/ansible that referenced this pull request Dec 4, 2018
…ansible#30743)

* fix(tasks: synchronize): wrap in sshpass if ssh password was provided

Closes ansible#16616

* fix(tasks: synchronize): pass rsync password to sshpass via fd

* fix(tasks: synchronize): use fail_json instead of AnsibleError

* fixup! fix(tasks: synchronize): use fail_json instead of AnsibleError

fix python2 handling

* feat(module_utils: basic: run_command): add optional arguments `pass_fds` and `before_communicate_callback`

* fix(tasks: synchronize): use module.run_command instead of subprocess.Popen

* fixup! fix(tasks: synchronize): use module.run_command instead of subprocess.Popen

remove unused import

* fixup! fixup! fix(tasks: synchronize): use module.run_command instead of subprocess.Popen

pass_fds only if they passed to run_command()
@ansible ansible locked and limited conversation to collaborators Jul 22, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
affects_2.5 This issue/PR affects Ansible v2.5 bug This issue/PR relates to a bug. core_review In order to be merged, this PR must follow the core review workflow. module This issue/PR relates to a module. new_contributor This PR is the first contribution by a new community member. plugins/action support:community This issue/PR relates to code supported by the Ansible community. support:core This issue/PR relates to code supported by the Ansible Engineering Team.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

sychronize module could wrap rsync with sshpass in certain cases
10 participants