From 991918e9d2ce30e7a8914d337e490e7c6b54293e Mon Sep 17 00:00:00 2001 From: chriskarel Date: Fri, 11 Aug 2017 14:36:46 -0500 Subject: [PATCH] Fix idempotency for Unix permissions in zip files. (#24580) * Fix idempotency for Unix permissions in zip files. This fix prevents the unarchive module from reporting 'changed' when a zipfile contains items with Unix permissions that differ from the system default. * Update zip unarchive tests. Additional tests for the unarchive module with zip files: - Test file in zip archive with non-default permissions - Test file added to zip archive with Windows permissions * Additional fix for mixed win/unix archives. Turns out my original fix fails under some mixed archives, as setting the umask to zero can be applied to those files. This creates a per-file umask variable, so a mix of permission types don't cause problems. * CI Checks CI checks for archives with: * non default Unix permissions * Windows permissions * Workaround for BSD differences. Using Zipinfo due to lack of support in BSD unzip. Permissions handling is also different in BSD -- always applies UMASK to file permissions. * Added checks for creating directories and SSH keys for existing users. --- lib/ansible/modules/files/unarchive.py | 19 +++++- .../targets/unarchive/tasks/main.yml | 66 +++++++++++++++---- 2 files changed, 71 insertions(+), 14 deletions(-) diff --git a/lib/ansible/modules/files/unarchive.py b/lib/ansible/modules/files/unarchive.py index 8520066f528..597cb2feb38 100644 --- a/lib/ansible/modules/files/unarchive.py +++ b/lib/ansible/modules/files/unarchive.py @@ -125,6 +125,7 @@ import codecs import datetime import grp import os +import platform import pwd import re import stat @@ -180,6 +181,7 @@ class ZipArchive(object): self.excludes = module.params['exclude'] self.includes = [] self.cmd_path = self.module.get_bin_path('unzip') + self.zipinfocmd_path = self.module.get_bin_path('zipinfo') self._files_in_archive = [] self._infodict = dict() @@ -261,7 +263,8 @@ class ZipArchive(object): return self._files_in_archive def is_unarchived(self): - cmd = [self.cmd_path, '-ZT', '-s', self.src] + # BSD unzip doesn't support zipinfo listings with timestamp. + cmd = [self.zipinfocmd_path, '-T', '-s', self.src] if self.excludes: cmd.extend(['-x', ] + self.excludes) rc, out, err = self.module.run_command(cmd) @@ -277,6 +280,7 @@ class ZipArchive(object): # Get some information related to user/group ownership umask = os.umask(0) os.umask(umask) + systemtype = platform.system() # Get current user and group information groups = os.getgroups() @@ -376,6 +380,12 @@ class ZipArchive(object): ftype = 'f' # Some files may be storing FAT permissions, not Unix permissions + # For FAT permissions, we will use a base permissions set of 777 if the item is a directory or has the execute bit set. Otherwise, 666. + # This permission will then be modified by the system UMask. + # BSD always applies the Umask, even to Unix permissions. + # For Unix style permissions on Linux or Mac, we want to use them directly. + # So we set the UMask for this file to zero. That permission set will then be unchanged when calling _permstr_to_octal + if len(permstr) == 6: if path[-1] == '/': permstr = 'rwxrwxrwx' @@ -383,6 +393,11 @@ class ZipArchive(object): permstr = 'rwxrwxrwx' else: permstr = 'rw-rw-rw-' + file_umask = umask + elif 'bsd' in systemtype.lower(): + file_umask = umask + else: + file_umask = 0 # Test string conformity if len(permstr) != 9 or not ZIP_FILE_MODE_RE.match(permstr): @@ -484,7 +499,7 @@ class ZipArchive(object): elif ztype == '?': mode = self._permstr_to_octal(permstr, 0) else: - mode = self._permstr_to_octal(permstr, umask) + mode = self._permstr_to_octal(permstr, file_umask) if mode != stat.S_IMODE(st.st_mode): change = True diff --git a/test/integration/targets/unarchive/tasks/main.yml b/test/integration/targets/unarchive/tasks/main.yml index e17e5b2ccb6..71cfaa454b6 100644 --- a/test/integration/targets/unarchive/tasks/main.yml +++ b/test/integration/targets/unarchive/tasks/main.yml @@ -17,18 +17,23 @@ # along with Ansible. If not, see . # Make sure we start fresh -- name: Ensure zip is present to create test archive (yum) - yum: name=zip state=latest +# Need unzup for unarchive module, and zip for archive creation. +- name: Ensure zip and unzip is present to create test archive (yum) + yum: name=zip,unzip state=latest when: ansible_pkg_mgr == 'yum' #- name: Ensure zip is present to create test archive (dnf) # dnf: name=zip state=latest # when: ansible_pkg_mgr == 'dnf' -- name: Ensure zip is present to create test archive (apt) - apt: name=zip state=latest +- name: Ensure zip & unzup is present to create test archive (apt) + apt: name=zip,unzip state=latest when: ansible_pkg_mgr == 'apt' +- name: Ensure zip & unzup is present to create test archive (pkg) + pkgng: name=zip,unzip state=present + when: ansible_pkg_mgr == 'pkgng' + - name: prep our file copy: src=foo.txt dest={{output_dir}}/foo-unarchive.txt @@ -38,9 +43,27 @@ - name: prep a tar.gz file shell: tar czvf test-unarchive.tar.gz foo-unarchive.txt chdir={{output_dir}} -- name: prep a zip file - shell: zip test-unarchive.zip foo-unarchive.txt chdir={{output_dir}} +- name: prep a chmodded file for zip + copy: src=foo.txt dest={{output_dir}}/foo-unarchive-777.txt mode=0777 +- name: prep a windows permission file for our zip + copy: src=foo.txt dest={{output_dir}}/FOO-UNAR.TXT + +# This gets around an unzip timestamp bug in some distributions +# Recent unzip on Fedora, Ubuntu, and BSD will randomly round some timestamps up. +# But that doesn't seem to happen when the timestamp has an even second. +- name: Bug work around + command: touch -t "201705111530.00" {{output_dir}}/foo-unarchive.txt {{output_dir}}/foo-unarchive-777.txt {{output_dir}}/FOO-UNAR.TXT +# See Fedora bug 1451953: https://bugzilla.redhat.com/show_bug.cgi?id=1451953 +# See Ubuntu bug 1691636: https://bugs.launchpad.net/ubuntu/+source/unzip/+bug/1691636 +# When these are fixed, this code should be removed. + +- name: prep a zip file + shell: zip test-unarchive.zip foo-unarchive.txt foo-unarchive-777.txt chdir={{output_dir}} + +- name: add a file with Windows permissions to zip file + shell: zip -k test-unarchive.zip FOO-UNAR.TXT chdir={{output_dir}} + - name: prep a subdirectory file: path={{output_dir}}/unarchive-dir state=directory @@ -141,17 +164,36 @@ - "unarchive03.changed == true" # Verify that file list is generated - "'files' in unarchive03" - - "{{unarchive03['files']| length}} == 1" + - "{{unarchive03['files']| length}} == 3" - "'foo-unarchive.txt' in unarchive03['files']" - + - "'foo-unarchive-777.txt' in unarchive03['files']" + - "'FOO-UNAR.TXT' in unarchive03['files']" + - name: verify that the file was unarchived - file: path={{output_dir}}/test-unarchive-zip/foo-unarchive.txt state=file - + file: path={{output_dir}}/test-unarchive-zip/{{item}} state=file + with_items: + - foo-unarchive.txt + - foo-unarchive-777.txt + - FOO-UNAR.TXT + +- name: repeat the last request to verify no changes + unarchive: src={{output_dir}}/test-unarchive.zip dest={{output_dir | expanduser}}/test-unarchive-zip remote_src=yes list_files=True + register: unarchive03b + +- name: verify that the task was not marked as changed + assert: + that: + - "unarchive03b.changed == false" + - name: remove our zip unarchive destination file: path={{output_dir}}/test-unarchive-zip state=absent -- name: remove our test file for the archive - file: path={{output_dir}}/foo-unarchive.txt state=absent +- name: remove our test files for the archive + file: path={{output_dir}}/{{item}} state=absent + with_items: + - foo-unarchive.txt + - foo-unarchive-777.txt + - FOO-UNAR.TXT - name: check if /tmp/foo-unarchive.text exists stat: path=/tmp/foo-unarchive.txt