Copy module ignores setgid bit on parent directories

[rdupz]

SUMMARY

The copy module ignores setgid bit.
While touching or scping a file into a directory with a setgid bit, the group of the touched or scped file is the same group as the directory one: this is the purpose of the setgid bit.
But when the file is created by ansible, the group of the created file is the default group of the user who copy the file.

I tried various combination. I also tried to play with the code of some of actions plugins including chown calls, but without any result...

Not tested with latest ansible 😟.
Affects 2.6.1.

ISSUE TYPE

• Bug Report

COMPONENT NAME

• copy
• template

ANSIBLE VERSION

ansible 2.6.1
  config file = None
  configured module search path = [u'/home/rdupz/.ansible/plugins/modules', u'/usr/share/ansible/plugins/modules']
  ansible python module location = /usr/lib/python2.7/site-packages/ansible-2.6.1-py2.7.egg/ansible
  executable location = /usr/bin/ansible
  python version = 2.7.5 (default, Apr 11 2018, 07:36:10) [GCC 4.8.5 20150623 (Red Hat 4.8.5-28)]

CONFIGURATION

(nothing)

OS / ENVIRONMENT

CentOS 7 to CentOS 7

STEPS TO REPRODUCE

foo_user:@dest_machine# mkdir /tmp/issue_tst
foo_user:@dest_machine# chown foo_user:bar_group /tmp/issue_tst
foo_user:@dest_machine# chmod g+s /tmp/issue_tst
foo_user:@dest_machine# touch /tmp/issue_tst/touched
foo_user:@dest_machine# ls -la /tmp/issue_tst/
drwxrwsr-x.  2 foo_user  bar_group 4096 Oct 10 09:14 .
drwxrwxrwt. 19 root root         4096 Oct 10 09:14 ..
-rw-rw-r--.  1 foo_user  bar_group    0 Oct 10 09:14 touched <--- setgid works 

---
- hosts: all
  remote_user: foo_user
  tasks:
    - name: "Reproducer"
      copy:
        content: ""
        dest: "/tmp/issue_tst/file_tst"
    - name: "Directory creation is ok"
      file:
        state: "directory"
        dest: "/tmp/issue_tst/dir_tst"
...

EXPECTED RESULTS

The file file_tst should be owned by foo_user:bar_group instead of foo_user:foo_user thanks to the setgid bit on /tmp/issue_tst/.

foo_user:@dest_machine# ls -la /tmp/issue_tst/
drwxrwsr-x.  3 foo_user  bar_group  4096 Oct 10 09:18 .
drwxrwxrwt. 19 root root                   4096 Oct 10 09:18 ..
drwxrwsr-x.  2 foo_user  bar_group  4096 Oct 10 09:18 dir_tst
-rw-rw-r--.  1 foo_user  bar_group       0 Oct 10 09:18 file_tst   <--- setgid should work
-rw-rw-r--.  1 foo_user  bar_group    0 Oct 10 09:14 touched

ACTUAL RESULTS

The file file_tst is actually owned by foo_user:foo_user, ignoring the setgid bit on /tmp/issue_tst/.

foo_user:@dest_machine# ls -la /tmp/issue_tst/
drwxrwsr-x.  3 foo_user  bar_group  4096 Oct 10 09:18 .
drwxrwxrwt. 19 root root                   4096 Oct 10 09:18 ..
drwxrwsr-x.  2 foo_user  bar_group  4096 Oct 10 09:18 dir_tst
-rw-rw-r--.  1 foo_user  foo_user       0 Oct 10 09:18 file_tst   <--- setgid ignored
-rw-rw-r--.  1 foo_user  bar_group    0 Oct 10 09:14 touched

[ansibot]

Files identified in the description:

• lib/ansible/modules/files/copy.py
• lib/ansible/modules/files/template.py

If these files are inaccurate, please update the component name section of the description or use the !component bot command.

click here for bot help

[rdupz]

So after deeper analysis, the bug is in basic.py.

My understanding is it uses os.rename for the sake of atomicity, and then tries to manually reimplement some of the work that could be done by shutil.move (fixing umask,acl,ownership,...).
But looks like shutil.move is atomic too (when possible). The oldest known state of basic.py seems to use os.rename so I don't know the reason behind this choice over shutil.move.

So IMO 2 possible fixes:

• Use shutil.move instead of os.rename and do some cleaning in the atomic_move method.
• Keep the same philosophy for whatever reason and add something like the following block in the atomic_move method:

# if setgid bit is set on parent directory then chgrp the file to the right group
b_dest_parent_stat = os.stat(os.path.dirname(b_dest))
if b_dest_parent_stat.st_mode & stat.S_ISGID:
  os.chown(b_dest, os.geteuid(), b_dest_parent_stat.st_gid)

[ansibot]

cc @ptux
click here for bot help

[ansibot]

Files identified in the description:

• lib/ansible/modules/copy.py
• lib/ansible/modules/template.py

If these files are incorrect, please update the component name section of the description or use the !component bot command.

click here for bot help

[jantari]

I also have this problem. Here's an excerpt from the playbook I tried to use:

- name: Set up nginx www root
  become: yes
  file:
    path: /var/www
    state: directory
    mode: '6750'
    recurse: yes
    owner: "{{ ansible_user_id }}"
    group: www-data

I intentionally set the UID and GID bits because I intend on later copying a file into this directory that should be served by nginx.
This is the result:

  drwsr-s---  3 packer www-data 4096 Aug 20 14:30 .
  drwxr-xr-x 14 root   root     4096 Aug 20 14:30 ..
  drwsr-s---  2 packer www-data 4096 Aug 20 14:30 html

Then I tried to create a test file to serve:

- name: Create a test file in nginx www root
  copy:
    content: "TEST FILE"
    dest: /var/www/test.txt

I don't use become because my ansible user (the remote_user, "packer") is already the owner of the directory and has mode-7 permissions. I also don't explicitly set a mode because I want to dynamically let the parent permissions propagate. But this is what I get instead:

  drwsr-s---  3 packer www-data 4096 Aug 20 14:30 .
  drwxr-xr-x 14 root   root     4096 Aug 20 14:30 ..
  drwsr-s---  2 packer www-data 4096 Aug 20 14:30 html
  -rw-------  1 packer packer      9 Aug 20 14:03 test.txt

which means nginx can't serve the file!

I understand forcing 600 permissions by default - ok. But there needs to be a special mode keyword, just like we already have "preserve" that means "respect the UID/GID bits and generally let the default permissions be".

[rdupz]

@jantari Note that the setuid bit is useless on directories. Also note that the setgid bit won't give you control over permissions of the created file but only over the group of the created file.
If you want all the files to be owned by www-data, then you indeed need the setgid bit to work.
But this won't be sufficient and you actually also want g+rx. So you need umask or acl settings (or manual/explicit permission settings).

I don't get why this issue is not getting any attention. Looks like no one need unix anymore theses days.

[4wk-]

Hello. I think this bug also hit ansible.builtin.get_url module: the file created doesnt get the right group.
I think this is the same bug as: #67177
As a workaround, I can force the group as a parameter of get_url, but that's kinda sad.
Let me know if you need an example. Cheers.

[shivaraj-arch]

Also the copy module does not recurse the setype ( not sure if setype can be recursed though but if chcon is taken as equivalent then it does have -R option ) when non standard directories are created through ansible and accessed via links in allowed directory root.

file:
        path: /webdir
        state: directory
        mode: '02775'
        group: apache
        setype: httpd_sys_content_t
        recurse: true

even with this set on a non root directory,
we will need to add setype to any subsequent copies to links created on directory root
like

file:
        dest: /var/www/html/webdir
        src: /webdir
        state: link

copy:
        content: Welcome to Index on {{ ansible_fqdn }}
        dest: /webdir/index.html
        setype: httpd_sys_content_t

and group too will not recurse but does not matter.
tested on : ansible v2.9.15

[4wk-]

Hi,

Could we have any update / plan on this please? 🙏
As already mentioned, it concerns a lot of Ansible module (ansible.builtin.copy, ansible.builtin.get_url #67177, but I just noticed today ansible.builtin.unarchive too) @bcoca @s-hertel @ptux

The OP @rdupz suggested 2 approach to fix in 2018.
Is there anything we can test to help you?

[jschwartz-cray]

EDIT: The unprivileged user issue mentioned below must be coming from elsewhere or perhaps was already fixed because the code affected by this patch has been wrapped in a try/except block to ignore errors from the chown for quite some time.

The second possible solution @rdupz suggested for this can't be used as written because it doesn't account for the possibility that the user is not root.

It also doesn't make sense to me why we would need to chgrp when setgid is set even if we are root... if setgid is set we shouldn't be messing with the group ownership at all because that's the entire point of setgid... that it will be set automatically.

If anything ansible should only verify that a manually specified group matches the setgid-resultant group, and error if that's not the case, otherwise if setgid is set and group is not specified, ansible should ignore the resulting group entirely and just let setgid do its thing.

EDIT: The chgrp was necessary (and glad to see it was included in the patch) because this was implemented using move and SETGID does not apply to moves, so it's necessary to change the permissions afterward to get copy semantics.

In any case, it's important that this be handled properly for both root and non-root use cases and in cases where we're not root ansible should not be trying to call chgrp (as it does today on setgid directories, which is how I ended up here).

I should be able to run ansible.builtin.copy as non-root and ideally leave group unspecified and create a file in a setgid directory without chgrp being invoked because the resulting group doesn't match my own. Worst case scenario I should at least be able to specify group as the value I expect to result from creating the file in said setgid directory and avoid the chgrp that way. Today neither of these work.

[jschwartz-cray]

Just FYI because of this issue and similar problems with calling chgrp/chown unconditionally even when running as non-root users, this is canonical ansible code:

- name: Copy scripts to cache
  ansible.builtin.copy:
    src: "{{ item }}"
    dest: "{{ library_script_cache }}/{{ item }}"
  loop:
    - link-if-identical.sh
    - set-execute.sh
  changed_when: false
  register: copy_result
  # ansible does not handle setgid directories properly so ignore chgrp failures
  # https://github.com/ansible/ansible/issues/46742
  # similarly it doesn't handle the case where a non-root user is copying and
  # can't chown, so ignore that as well
  failed_when: |
    copy_result is failed and
    copy_result.msg is not search('chgrp failed') and
    copy_result.msg is not search('chown failed')

- name: |
    Make sure scripts in cache are executable (we ignore chgrp/chown
    errors from ansible above when we aren't root, so we have to set these
    manually)
  ansible.builtin.command: chmod 755 "{{ library_script_cache }}/{{ item }}"
  loop:
    - link-if-identical.sh
    - set-execute.sh
  changed_when: false

[4wk-]

FWIW, I've rerun the test setup by OP, and as expected the issue is still there:

$ ls -la /tmp/issue_tst/
total 44
drwxr-s---   3 foo_user bar_group  4096 Jul 17 18:25 .
drwxrwxrwt 145 root     root      36864 Jul 17 18:25 ..
drwxr-s---   2 root     bar_group  4096 Jul 17 18:25 dir_tst
-rw-r-----   1 root     root          0 Jul 17 18:25 file_tst
-rw-r-----   1 foo_user bar_group     0 Jul 17 18:24 touched

@ansibot Please change label.

ansible [core 2.17.2]
  config file = /home/foobar/ansible/ansible.cfg
  configured module search path = ['/home/foobar/.ansible/plugins/modules', '/usr/share/ansible/plugins/modules']
  ansible python module location = /opt/pipx/venvs/ansible/lib/python3.11/site-packages/ansible
  ansible collection location = /home/foobar/.ansible/collections:/usr/share/ansible/collections
  executable location = /usr/local/bin/ansible
  python version = 3.11.2 (main, May  2 2024, 11:59:08) [GCC 12.2.0] (/opt/pipx/venvs/ansible/bin/python)
  jinja version = 3.1.4
  libyaml = True

[s-hertel]

This should be working correctly now on devel, and I've opened backports for 2.16 and 2.17 (#83765, #83764). Thanks for all the reports and the good assessment #46742 (comment).