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

Test that file ownership and permissions prevent file fetch #54742

Open
wants to merge 10 commits into
base: devel
from

Conversation

Projects
None yet
3 participants
@sbonds
Copy link
Contributor

commented Apr 2, 2019

SUMMARY

When permissions don't allow a file to be fetched, the attempt should fail.

The actual results suggest that when using a Docker container, Ansible always connects as root even when remote_user: <other user> is specified. This test demonstrates that observed behavior and currently fails.

This seems similar to the closed issue #13388. Pull #15556 also mentions some similar observations.

We probably do not want to merge this test enhancement until we identify what causes the remote_user to be ignored for Docker containers within the test suite. However, I thought you all could benefit from seeing a test that demonstrated the issue sooner rather than later.

ISSUE TYPE
  • Bugfix Pull Request
COMPONENT NAME

fetch

ADDITIONAL INFORMATION

While reproducing an issue where fetch seemed to be ignoring the become directive, I created a test case using Docker. It appears that when run using

$ ansible-test integration --docker -v fetch

even though the test requested a fetch using a new, nonprivileged user via remote_user: '{{ remote_unprivileged_user }}', Ansible instead fetches it as root.

For example, here's what a triple-v log shows for this task (note the ESTABLISH LOCAL CONNECTION FOR USER: root):

TASK [fetch : attempt to fetch the only-root-readable test file using a non-root user, should fail] ***
task path: /root/.ansible/test/tmp/fetch-e6t19wo0-ÅÑŚÌβŁÈ/test/integration/targets/fetch/tasks/main.yml:187
<testhost> ESTABLISH LOCAL CONNECTION FOR USER: root
<testhost> EXEC /bin/sh -c 'echo ~root && sleep 0'
<testhost> EXEC /bin/sh -c '( umask 77 && mkdir -p "` echo /root/.ansible/tmp/ansible-tmp-1554236816.4309068-133802087670756 `" && echo ansible-tmp-1554236816.4309068-133802087670756="` echo /root/.ansible/tmp/ansible-tmp-1554236816.4309068-133802087670756 `" ) && sleep 0'
Using module file /root/ansible/lib/ansible/modules/files/stat.py
<testhost> PUT /root/.ansible/tmp/ansible-local-12052xkzkq6/tmpp8mhdv_d TO /root/.ansible/tmp/ansible-tmp-1554236816.4309068-133802087670756/AnsiballZ_stat.py
<testhost> EXEC /bin/sh -c 'chmod u+x /root/.ansible/tmp/ansible-tmp-1554236816.4309068-133802087670756/ /root/.ansible/tmp/ansible-tmp-1554236816.4309068-133802087670756/AnsiballZ_stat.py && sleep 0'
<testhost> EXEC /bin/sh -c '/tmp/python-lejid8t9-ansible/python /root/.ansible/tmp/ansible-tmp-1554236816.4309068-133802087670756/AnsiballZ_stat.py && sleep 0'
<testhost> FETCH /tmp/ansible.h2o14eo8.test/orig.root TO /root/ansible_testing/fetched/testhost/tmp/ansible.h2o14eo8.test/orig.root
<testhost> PUT /tmp/ansible.h2o14eo8.test/orig.root TO /root/ansible_testing/fetched/testhost/tmp/ansible.h2o14eo8.test/orig.root
<testhost> EXEC /bin/sh -c 'rm -f -r /root/.ansible/tmp/ansible-tmp-1554236816.4309068-133802087670756/ > /dev/null 2>&1 && sleep 0'
changed: [testhost] => {
    "changed": true,
    "checksum": "a94a8fe5ccb19ba61c4c0873d391e987982fbbd3",
    "dest": "/root/ansible_testing/fetched/testhost/tmp/ansible.h2o14eo8.test/orig.root",
    "md5sum": "098f6bcd4621d373cade4e832627b4f6",
    "remote_checksum": "a94a8fe5ccb19ba61c4c0873d391e987982fbbd3",
    "remote_md5sum": null
}
Read vars_file 'integration_config.yml'

TASK [fetch : debug] ***********************************************************
task path: /root/.ansible/test/tmp/fetch-e6t19wo0-ÅÑŚÌβŁÈ/test/integration/targets/fetch/tasks/main.yml:193
ok: [testhost] => {
    "failed_fetch_root_owned_file": {
        "changed": true,
        "checksum": "a94a8fe5ccb19ba61c4c0873d391e987982fbbd3",
        "dest": "/root/ansible_testing/fetched/testhost/tmp/ansible.h2o14eo8.test/orig.root",
        "failed": false,
        "md5sum": "098f6bcd4621d373cade4e832627b4f6",
        "remote_checksum": "a94a8fe5ccb19ba61c4c0873d391e987982fbbd3",
        "remote_md5sum": null
    }
}

The file created by the test was mode 0600 owned by root. A non-root user should get a permission denied or file not found error attempting to retrieve it.

sbonds added some commits Apr 2, 2019

Borrow from copy test
To test fetch with 'become' we need many of the same things as testing copy with become. Borrow some code from there since it's already available.

Signed-off-by: Steve Bonds <sbonds@gmail.com>
copy module used an intermediate variable
However we don't need to use an intermediate here.
Ideally in order to create a root-owned file we need to become root
This works without 'become' due to the same bug/issue the other test demonstrates.
Neaten things up and include needed become: lines
Make the location of become: more consistent. Replace some double quotes with the single quotes used throughout.

Basically make this pretty since I may ask others to review it. 😁
# Docker-specific challenges of the "become" process.
- name: attempt to fetch the only-root-readable test file using a non-root user, should fail
fetch: src={{ remote_tmp_dir }}/orig.root dest={{ output_dir }}/fetched
remote_user: '{{ remote_unprivileged_user }}'

This comment has been minimized.

Copy link
@mattclay

mattclay Apr 2, 2019

Member

Tests are run with ansible_connection=local. The local connection plugin ignores remote_user, so this will have no effect.

This comment has been minimized.

Copy link
@sbonds

sbonds Apr 3, 2019

Author Contributor

Thanks, @mattclay. What would be a good way to test this?

In #13388 there was some mention of applying a different user to the docker exec commands, and in pull request #13424 it looks like it was implemented. However, as you mentioned, when using local.py none of that matters since docker.py won't even be used.

If remote_user gets ignored, that definitely makes it a challenge to test. Any thoughts?

This comment has been minimized.

Copy link
@mattclay

mattclay Apr 10, 2019

Member

That sounds like something that should be added to the docker_connection integration test.

You could rename the existing runme.sh for that test to test.sh and create a new runme.sh that invokes the original tests as well as the new ones you'd need to add.

@ansibot ansibot added the stale_ci label Apr 18, 2019

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.