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.

Sign up for GitHub

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

Jump to bottom

Use SSH askpass instead of sshpass for password logins #14034

Closed
wants to merge 6 commits into from
Closed

Use SSH askpass instead of sshpass for password logins #14034

wants to merge 6 commits into from

Conversation

saltsa
Copy link

@saltsa saltsa commented Jan 20, 2016

Description

This PR makes OpenSSH to use askpass directly without the need of external sshpass program.

This sets the password to the environment variable called ANSIBLE_SSH_PASS and sets the SSH_ASKPASS enviroment to "/usr/bin/ansible-pwecho". Itse also sets the DISPLAY enviroment variable to empty string, if it's not set. When the ssh needs password, it'll execute the askpass program which simply outputs the password from the enviroment variable.

Security considerations

The password can be read from the environment variable of the ansible-pwecho program. However, at least in Linux the permissions are limited to the current user.

On the other hand, the pwecho program is very simple.

TODO

  • Does the change to the enviroment variables affect Ansible or other programs?
  • I tested this only with simple inventory file containing hostname, ansible_ssh_user and ansible_ssh_pass
  • Should the feature be activated by default or with some command line switch?
  • Should the sshpass-program be removed completely or left as a fallback
    • Current patch removes support for that program
  • Does the fix work with sftp or scp
  • Test with other systems (currently tested only in Linux)

@bcoca
Copy link
Member

bcoca commented Jan 20, 2016

Wouldn't making it a config option in lib/ansible/constants.py work the same and be a much smaller change? Also make it conditional instead of substituting the existing working sshpass for those that do not want the envvar.

@saltsa
Copy link
Author

saltsa commented Jan 20, 2016

Sounds good! It's easy to support the sshpass too. But which one should be default? The old one or this, which has no external dependencies (only the openssh). Or should it first try with askpass and then fallback to sshpass.

@bcoca
Copy link
Member

bcoca commented Jan 20, 2016

default to current and have a toggle, if this is a good substitute we can eventually revers the default

@abadger abadger added feature_pull_request needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. labels Jan 22, 2016
@saltsa
added config option to use SSH_ASKPASS
bc8ede4 
There seems to be problem with askpass and sftp which is not problem
if using pipelining.

The subprocess commands were updated to call os.setsid before execution.
@saltsa
Copy link
Author

saltsa commented Jan 28, 2016

I added the option, which is not enabled by default, to constants.py. It seems, that this doesn't work when using sftp but only with scp. However, if there's controlsocket, it should work.

@cesarjorgemartinez
Copy link

For ask become pass exists a config option?

@philipmat philipmat mentioned this pull request May 10, 2016
Closed
@ansibot ansibot added affects_2.3 This issue/PR affects Ansible v2.3 needs_rebase https://docs.ansible.com/ansible/devel/dev_guide/developing_rebasing.html labels Dec 13, 2016
@ansibot ansibot added needs_rebase https://docs.ansible.com/ansible/devel/dev_guide/developing_rebasing.html and removed needs_rebase https://docs.ansible.com/ansible/devel/dev_guide/developing_rebasing.html labels Jan 2, 2017
@ansibot
Copy link
Contributor

ansibot commented Jan 5, 2017

@saltsa This PR was tested by travis-ci.org, which is no longer used. Please rebase your branch to trigger running of current tests.

click here for bot help

@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 needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. labels Jan 5, 2017
@ansibot
Copy link
Contributor

ansibot commented Jan 6, 2017

@saltsa This PR was tested by travis-ci.org, which is no longer used. Please rebase your branch to trigger running of current tests.

click here for bot help

@ansibot
Copy link
Contributor

ansibot commented Apr 11, 2017

@saltsa Greetings! Thanks for taking the time to open this pullrequest. In order for the community to handle your pullrequest effectively, we need a bit more information.

Here are the items we could not find in your description:

  • issue type

Please set the description of this pullrequest with this template:
https://raw.githubusercontent.com/ansible/ansible/devel/.github/PULL_REQUEST_TEMPLATE.md

click here for bot help

@ansibot ansibot added needs_info This issue requires further information. Please answer any outstanding questions. needs_template This issue/PR has an incomplete description. Please fill in the proposed template correctly. stale_ci This PR has been tested by CI more than one week ago. Close and re-open this PR to get it retested. labels Apr 11, 2017
@ansibot ansibot added the support:core This issue/PR relates to code supported by the Ansible Engineering Team. label Jun 29, 2017
@ansibot ansibot added the new_contributor This PR is the first contribution by a new community member. label Oct 18, 2017
@ansibot ansibot removed the new_contributor This PR is the first contribution by a new community member. label Nov 3, 2017
@ansibot ansibot removed needs_info This issue requires further information. Please answer any outstanding questions. needs_template This issue/PR has an incomplete description. Please fill in the proposed template correctly. labels Nov 22, 2017
@ansibot ansibot added the new_contributor This PR is the first contribution by a new community member. label Jan 25, 2018
@ansibot ansibot added feature This issue/PR relates to a feature request. and removed feature_pull_request labels Mar 2, 2018
@sivel
Copy link
Member

sivel commented Nov 16, 2018

Thank you very much for your submission. We have discussed this and decided against accepting this feature.

Our recommendation is to use the paramiko connection plugin, or switch to using key based authentication.

If you have further questions please stop by IRC or the mailing list:

@sivel sivel closed this Nov 16, 2018
@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.
Reviewers
No reviews
Assignees
No one assigned
Labels
affects_2.3 This issue/PR affects Ansible v2.3 c:cli/ c:constants c:plugins/connection/ssh c:plugins/connection cli/ constants feature This issue/PR relates to a feature request. 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. new_contributor This PR is the first contribution by a new community member. plugins/connection/ssh stale_ci This PR has been tested by CI more than one week ago. Close and re-open this PR to get it retested. support:core This issue/PR relates to code supported by the Ansible Engineering Team.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@saltsa @bcoca @cesarjorgemartinez @ansibot @sivel @abadger