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 delegate_to using incorrect ansible_become_* vars (#34259) #56082

Open
wants to merge 9 commits into
base: devel
from

Conversation

Projects
None yet
5 participants
@bauen1
Copy link

commented May 4, 2019

SUMMARY

This Pull Request Fixes #34259
Ansible was using the hosts become_pass variables and not the variables from the delegated_to host, this leads to authentication errors when using delegate_to and become: true (this is also a security bug, since passwords will be send to the wrong host).

ISSUE TYPE
  • Bugfix Pull Request
COMPONENT NAME
ADDITIONAL INFORMATION

This patch was provided by @klondi here.

@kondi

This comment has been minimized.

Copy link

commented May 4, 2019

@bauen1

This comment has been minimized.

Copy link
Author

commented May 4, 2019

ready_for_review

@bcoca

This comment has been minimized.

Copy link
Member

commented May 6, 2019

@bauen1 not sure this is correct since we don't rely on play_context at this point (we did in past), become is now handled by the plugins and the variables are passed in from the executor

@bauen1

This comment has been minimized.

Copy link
Author

commented May 6, 2019

@bcoca From my understanding the executor relies on play_context to provided the variables, see https://github.com/ansible/ansible/blob/devel/lib/ansible/executor/task_executor.py#L513-L516.
I also tested this and it worked in my case.

@bcoca

This comment has been minimized.

Copy link
Member

commented May 6, 2019

that is there for backwards compatibility, but this is what sets the used options from variables:
https://github.com/ansible/ansible/blob/devel/lib/ansible/executor/task_executor.py#L949

and here you can see how we override play_context for most of those settings:
https://github.com/ansible/ansible/blob/devel/lib/ansible/executor/task_executor.py#L1004

@bauen1

This comment has been minimized.

Copy link
Author

commented May 11, 2019

I'm not too familiar with the code base of ansible.
However, as far as I understand the code, play_context is responsible for setting become_pass to the right value, it does however currently not take delegate_to into account when reading sudo_pass and su_pass, which is the bug described in #34259.
I'm therefor quite sure that fixing play_context is correct, since it's the root of the issue.

@bcoca

This comment has been minimized.

Copy link
Member

commented May 13, 2019

@bauen1 that used to be the case, it is not anymore

@ansibot ansibot added the stale_ci label May 13, 2019

@samdoran samdoran removed the needs_triage label May 14, 2019

@ansibot ansibot removed the stale_ci label May 20, 2019

@bauen1

This comment has been minimized.

Copy link
Author

commented May 20, 2019

@bcoca After doing some more experimenting, you're right after all.

Switching to ansible_become_password makes this patch useless.
However there is another issue, since ansible will currently evaluate ansible_become_password in the context of the host and not in the context of the delegated host.

Example:

# hosts.yml
---
all:
    hosts:
        a:
            ansible_become_password: "a"
        b:
            ansible_become_password: "b-{{ inventory_hostname }}"
            # should normally evaluate to b-b
#!/usr/bin/env ansible-playbook
# site.yml
---
- hosts:
    - a
    - b
  gather_facts: false
  tasks:
    - name: print become password
      debug:
        msg: "password: {{ ansible_become_password }}"

- hosts: a
  gather_facts: false
  tasks:
    - name: test1
      become: true
      ping:
      delegate_to: b
      # ansible_become_password will evaluate to b-a instead of b-b
      # ansible will fail to authenticate with the remote host

I'm not really sure if this can be fixed easily.

@ansibot ansibot added needs_revision and removed core_review labels May 20, 2019

@bcoca

This comment has been minimized.

Copy link
Member

commented May 20, 2019

i was doing very similar patch, just using set_variables on templar to change the context to be the specific host, there is another bug that requires slightly different templating than you do here.

i.e ansible_host: {{ ansible_fqdn| process}} as that will template using the inventory_hostname variables instead of the delegated host ones. So i have a vm.get_vars(task=self._task, host=self._task.delegate_to) to create the dict needed to handle all cases, you are further ahead than me so if you want to incorporate that here ...

@ansibot ansibot added core_review and removed needs_revision labels May 20, 2019

@bauen1

This comment has been minimized.

Copy link
Author

commented May 20, 2019

So this would increase the example to this:

# hosts.yml
---
all:
    hosts:
        a:
            ansible_host: "host-a-{{ inventory_hostname }}"
            # should normally evaluate to "host-a-a"
            ansible_become_password: "a"
        b:
            ansible_host: "host-b-{{ inventory_hostname }}"
            # should normally evaluate to "host-b-b"
            ansible_become_password: "b-{{ inventory_hostname }}"
            # should normally evaluate to "b-b"
#!/usr/bin/env ansible-playbook
# site.yml
---
- hosts:
    - a
    - b
  gather_facts: false
  tasks:
    - debug:
        msg: "password: {{ ansible_become_password }}\nhost: {{ ansible_host }}"

- hosts: a
  gather_facts: false
  tasks:
    - name: test1
      become: true
      ping:
      delegate_to: b
      # ansible_host will evaluate to "host-b-a" instead of "host-b-b"
      # ansible_become_password will evaluate to "b-a" instead of "b-b"
      # ansible would fail to authenticate with the remote host

Also it seems like #39413 is caused by this too.

@bauen1

This comment has been minimized.

Copy link
Author

commented May 20, 2019

@bcoca I would love to incorporate your changes, could you push them somewhere for me to look at, I'm also at a bit of a loss where ansible_host for the delegated connection is set.
There are also a few more cases where something from final_vars is templated using the host currently run.

@bcoca

This comment has been minimized.

Copy link
Member

commented May 20, 2019

I was trying to get #55219, to ensure all resolution happens outside of play_contxt, which simplifies the patch to only update task_executor

@bcoca

This comment has been minimized.

Copy link
Member

commented May 23, 2019

probably fixes #33516

@bauen1

This comment has been minimized.

Copy link
Author

commented May 23, 2019

Using the test provided in #55644 says it's still an issue.

@ansibot

This comment has been minimized.

Copy link
Contributor

commented May 23, 2019

@bauen1 this PR contains the following merge commits:

Please rebase your branch to remove these commits.

click here for bot help

@bauen1 bauen1 force-pushed the bauen1:fix-delegate-to-become-vars branch from a5e7a28 to b30ffa3 May 23, 2019

bauen1 added some commits May 20, 2019

fix delegated host variable evaluation context (Fixes #34259)
When a task is delegated, the correct variables are used for the
connection, but they are evaluated in the context of the current host,
this can lead to issues, if for example ansible_become_password contains
references to the host:
a:
  ansible_become_password: "a"
b:
  ansible_become_password: "b-{{ inventory_hostname  }}"

If a task is run for host a, but is delegated to b, ansible will use b-a
instead of b-b as password for become.
This also fixes potential other issues / bugs where a variable is
incorrectly evaluated when using delegate_to.
Fixes everything related to variables and delegate_to (hopefully)
This probably needs a bit of a cleanup
fix delegate_to not being templated in some cases, leading to some very
interesting issues (eg. ssh is successfull, but sudo is not)
@bauen1

This comment has been minimized.

Copy link
Author

commented May 25, 2019

@bcoca could you give this another look ? Apart from the small git-mess it should be working now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.