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

Create gss_auth option for paramiko_ssh connection plugin #71190

Open
wants to merge 4 commits into
base: devel
Choose a base branch
from

Conversation

arnoxit
Copy link

@arnoxit arnoxit commented Aug 10, 2020

SUMMARY

Add a new gss_auth option for the paramiko_ssh connection plugin that will be passed to the upstream paramiko library connect() method. This will enable GSS-API authentication for paramiko ssh connections, which is required for
Kerberos authentication to succeed.

Set the default for the gss_auth option in the [paramiko_connection] section to False to ensure backwards compatibility.

Fixes #71201

ISSUE TYPE
  • Feature Pull Request
COMPONENT NAME

paramiko_ssh

ADDITIONAL INFORMATION

Without gss_auth enabled for paramiko_ssh against a kerberos authenticated host:

(venv) kim.covil@dragon:~/src/ansible$ ansible -m ping -c paramiko -i $HOSTNAME, $HOSTNAME
[WARNING]: You are running the development version of Ansible. You should only run Ansible from "devel" if you are modifying the Ansible engine, or trying out features under development. This is a rapidly changing source of code and can become unstable at any point.
dragon.office.transficc.net | UNREACHABLE! => {
    "changed": false,
    "msg": "Failed to authenticate: Authentication failed.",
    "unreachable": true
}

With gss_auth enabled for paramiko_ssh against a kerberos authenticated host:

(venv) kim.covil@dragon:~/src/ansible-arnoxit$ cat ansible.cfg
[paramiko_connection]
gss_auth = True
(venv) kim.covil@dragon:~/src/ansible-arnoxit$ ansible -m ping -c paramiko -i $HOSTNAME, $HOSTNAME
[WARNING]: You are running the development version of Ansible. You should only run Ansible from "devel" if you are modifying the Ansible engine, or trying out features under development. This is a rapidly changing source of code and can become unstable at any point.
[WARNING]: Platform linux on host dragon.office.transficc.net is using the discovered Python interpreter at /usr/bin/python, but future installation of another Python interpreter could change the meaning of that path. See https://docs.ansible.com/ansible/devel/reference_appendices/interpreter_discovery.html for
more information.
dragon.office.transficc.net | SUCCESS => {
    "ansible_facts": {
        "discovered_interpreter_python": "/usr/bin/python"
    },
    "changed": false,
    "ping": "pong"
}

- Add a new gss_auth option for the paramiko_ssh connection plugin that will
  be passed to the upstream paramiko library connect method. This will enable
  GSS-API authentication for paramiko ssh connections which is required for
  Kerberos authentication.
- Set the default for the gss_auth option in the paramiko_connection section
  to false to ensure backwards compatibility
@ansibot ansibot added affects_2.11 core_review In order to be merged, this PR must follow the core review workflow. feature This issue/PR relates to a feature request. needs_triage Needs a first human triage before being processed. new_contributor This PR is the first contribution by a new community member. support:core This issue/PR relates to code supported by the Ansible Engineering Team. 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. needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. labels Aug 10, 2020
@ansibot ansibot added 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. labels Aug 11, 2020
@arnoxit arnoxit force-pushed the paramiko_ssh-gss_auth branch from 7307eb7 to 4099c6a Compare Aug 11, 2020
@ansibot ansibot added core_review In order to be merged, this PR must follow the core review workflow. has_issue 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. core_review In order to be merged, this PR must follow the core review workflow. labels Aug 11, 2020
@arnoxit arnoxit force-pushed the paramiko_ssh-gss_auth branch from 4099c6a to 8add567 Compare Aug 11, 2020
@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 Aug 11, 2020
@arnoxit arnoxit force-pushed the paramiko_ssh-gss_auth branch from 8add567 to 69a9a4d Compare Aug 11, 2020
- Only pass the gss_auth option to paramiko.SSHClient().connect() if the paramiko version
  supports it (paramiko added support for GSS-API in 1.15.0)
@arnoxit arnoxit force-pushed the paramiko_ssh-gss_auth branch from 69a9a4d to bb79a18 Compare Aug 11, 2020
@samdoran samdoran removed the needs_triage Needs a first human triage before being processed. label Aug 11, 2020
lib/ansible/config/base.yml Outdated Show resolved Hide resolved
@samdoran
Copy link
Contributor

samdoran commented Aug 11, 2020

This seems like the first of several GSS-API related options that can be added. paramiko offers several GSS-API related options that others may need in the future. This opens the door to adding those other options, which we may or may not be ok with.

@ansibot ansibot added the needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. label Aug 11, 2020
@arnoxit arnoxit requested a review from bcoca Aug 11, 2020
Copy link
Contributor

@samdoran samdoran left a comment

This change looks correct now. Please create a changelog fragment. See this fragment as an example.

It would be nice to have tests for this. I'm not sure how comfortable you are writing unit tests. We don't have many for this plugin currently, but that's probably the best way to test this since integration tests would require setting up kerberos auth. This is not a blocker, though.

@arnoxit
Copy link
Author

arnoxit commented Aug 13, 2020

This change looks correct now. Please create a changelog fragment. See this fragment as an example.

It would be nice to have tests for this. I'm not sure how comfortable you are writing unit tests. We don't have many for this plugin currently, but that's probably the best way to test this since integration tests would require setting up kerberos auth. This is not a blocker, though.

I couldn't see how to set up a test for it without having a kerberos auth environment configured. It might be possible to test that enabling the option means the ssh client attempts gss-api (but then continues to other options). Do you have any pointers for other auth type option tests I could look at?

@ansibot ansibot added the support:community This issue/PR relates to code supported by the Ansible community. label Aug 13, 2020
@arnoxit arnoxit requested a review from samdoran Aug 13, 2020
@samdoran
Copy link
Contributor

samdoran commented Aug 13, 2020

I couldn't see how to set up a test for it without having a kerberos auth environment configured.

That's why I suggested that unit tests might be easier. Setting up kerberos just for a test is probably quite difficult.

Just run _connect_uncached() directly in the unit test and make sure it does the expected thing when it hits the new code path.

You can take a look at test/units/plugins/connection/test_ssh.py and add new unit tests to test/units/plugins/connection/test_paramiko.py.

@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 Aug 21, 2020
@ansibot ansibot added pre_azp This PR was last tested before migration to Azure Pipelines. 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. stale_review Updates were made after the last review and the last review is more than 7 days old. labels Dec 3, 2020
@mattclay
Copy link
Member

mattclay commented Dec 3, 2020

/azp run

@azure-pipelines
Copy link

azure-pipelines bot commented Dec 3, 2020

Azure Pipelines successfully started running 1 pipeline(s).

@ansibot ansibot added core_review In order to be merged, this PR must follow the core review workflow. 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. and removed needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. pre_azp This PR was last tested before migration to Azure Pipelines. core_review In order to be merged, this PR must follow the core review workflow. labels Dec 3, 2020
@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 Dec 11, 2020
@ansibot ansibot removed the support:community This issue/PR relates to code supported by the Ansible community. label Mar 4, 2021
@s-hertel s-hertel added the P3 Priority 3 - Approved, No Time Limitation label Aug 20, 2021
@@ -340,6 +350,10 @@ def _connect_uncached(self):
if LooseVersion(paramiko.__version__) >= LooseVersion('2.2.0'):
ssh_connect_kwargs['auth_timeout'] = self._play_context.timeout

# paramiko 1.15.0 introduced gss_auth parameter
if LooseVersion(paramiko.__version__) >= LooseVersion('1.15.0'):
Copy link
Contributor

@s-hertel s-hertel Aug 20, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should give a warning if this is not True and gss_auth is set.

@ansibot ansibot added needs_rebase https://docs.ansible.com/ansible/devel/dev_guide/developing_rebasing.html and removed stale_review Updates were made after the last review and the last review is more than 7 days old. labels Sep 5, 2021
@ansibot ansibot added needs_ci This PR requires CI testing to be performed. Please close and re-open this PR to trigger CI. 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. labels Nov 22, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
affects_2.11 feature This issue/PR relates to a feature request. has_issue needs_ci This PR requires CI testing to be performed. Please close and re-open this PR to trigger CI. 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. P3 Priority 3 - Approved, No Time Limitation 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.

Add support for the gss_auth option to the paramiko_ssh connection plugin
8 participants