From 127d54b3630c65043ec12c4af2024f8ef0bc6d09 Mon Sep 17 00:00:00 2001 From: Jordan Borean Date: Wed, 25 Mar 2020 08:08:23 +1000 Subject: [PATCH] galaxy - preserve mode properly on artifact (#68418) * galaxy - preserve mode properly on artifact * Fix py2 encoding issue * Update lib/ansible/galaxy/collection.py Co-Authored-By: Matt Clay * Use sane defaults instead of sourcing from tarfile Co-authored-by: Matt Clay --- .../fragments/collection-install-mode.yaml | 2 ++ lib/ansible/galaxy/collection.py | 14 +++++++++-- test/units/cli/test_galaxy.py | 25 +++++++++++++------ test/units/galaxy/test_collection_install.py | 25 +++++++++++++++---- 4 files changed, 52 insertions(+), 14 deletions(-) create mode 100644 changelogs/fragments/collection-install-mode.yaml diff --git a/changelogs/fragments/collection-install-mode.yaml b/changelogs/fragments/collection-install-mode.yaml new file mode 100644 index 00000000000..7fc72816ced --- /dev/null +++ b/changelogs/fragments/collection-install-mode.yaml @@ -0,0 +1,2 @@ +bugfixes: +- ansible-galaxy collection - Preserve executable bit on build and preserve mode on install from what tar member is set to - https://github.com/ansible/ansible/issues/68415 diff --git a/lib/ansible/galaxy/collection.py b/lib/ansible/galaxy/collection.py index f0077384873..1b7a258df6e 100644 --- a/lib/ansible/galaxy/collection.py +++ b/lib/ansible/galaxy/collection.py @@ -9,6 +9,7 @@ import json import operator import os import shutil +import stat import sys import tarfile import tempfile @@ -925,7 +926,8 @@ def _build_collection_tar(b_collection_path, b_tar_path, collection_manifest, fi b_src_path = os.path.join(b_collection_path, to_bytes(filename, errors='surrogate_or_strict')) def reset_stat(tarinfo): - tarinfo.mode = 0o0755 if tarinfo.isdir() else 0o0644 + existing_is_exec = tarinfo.mode & stat.S_IXUSR + tarinfo.mode = 0o0755 if existing_is_exec or tarinfo.isdir() else 0o0644 tarinfo.uid = tarinfo.gid = 0 tarinfo.uname = tarinfo.gname = '' return tarinfo @@ -1085,10 +1087,18 @@ def _extract_tar_file(tar, filename, b_dest, b_temp_path, expected_hash=None): if not os.path.exists(b_parent_dir): # Seems like Galaxy does not validate if all file entries have a corresponding dir ftype entry. This check # makes sure we create the parent directory even if it wasn't set in the metadata. - os.makedirs(b_parent_dir) + os.makedirs(b_parent_dir, mode=0o0755) shutil.move(to_bytes(tmpfile_obj.name, errors='surrogate_or_strict'), b_dest_filepath) + # Default to rw-r--r-- and only add execute if the tar file has execute. + tar_member = tar.getmember(to_native(filename, errors='surrogate_or_strict')) + new_mode = 0o644 + if stat.S_IMODE(tar_member.mode) & stat.S_IXUSR: + new_mode |= 0o0111 + + os.chmod(b_dest_filepath, new_mode) + def _get_tar_file_member(tar, filename): n_filename = to_native(filename, errors='surrogate_or_strict') diff --git a/test/units/cli/test_galaxy.py b/test/units/cli/test_galaxy.py index f8b544e09f8..67d1c03f3c0 100644 --- a/test/units/cli/test_galaxy.py +++ b/test/units/cli/test_galaxy.py @@ -25,6 +25,7 @@ import json import os import pytest import shutil +import stat import tarfile import tempfile import yaml @@ -579,6 +580,14 @@ def collection_artifact(collection_skeleton, tmp_path_factory): ''' Creates a collection artifact tarball that is ready to be published and installed ''' output_dir = to_text(tmp_path_factory.mktemp('test-ÅÑŚÌβŁÈ Output')) + # Create a file with +x in the collection so we can test the permissions + execute_path = os.path.join(collection_skeleton, 'runme.sh') + with open(execute_path, mode='wb') as fd: + fd.write(b"echo hi") + + # S_ISUID should not be present on extraction. + os.chmod(execute_path, os.stat(execute_path).st_mode | stat.S_ISUID | stat.S_IEXEC) + # Because we call GalaxyCLI in collection_skeleton we need to reset the singleton back to None so it uses the new # args, we reset the original args once it is done. orig_cli_args = co.GlobalCLIArgs._Singleton__instance @@ -644,8 +653,9 @@ def test_collection_build(collection_artifact): with tarfile.open(tar_path, mode='r') as tar: tar_members = tar.getmembers() - valid_files = ['MANIFEST.json', 'FILES.json', 'roles', 'docs', 'plugins', 'plugins/README.md', 'README.md'] - assert len(tar_members) == 7 + valid_files = ['MANIFEST.json', 'FILES.json', 'roles', 'docs', 'plugins', 'plugins/README.md', 'README.md', + 'runme.sh'] + assert len(tar_members) == len(valid_files) # Verify the uid and gid is 0 and the correct perms are set for member in tar_members: @@ -655,7 +665,7 @@ def test_collection_build(collection_artifact): assert member.gname == '' assert member.uid == 0 assert member.uname == '' - if member.isdir(): + if member.isdir() or member.name == 'runme.sh': assert member.mode == 0o0755 else: assert member.mode == 0o0644 @@ -700,19 +710,20 @@ def test_collection_build(collection_artifact): finally: files_file.close() - assert len(files['files']) == 6 + assert len(files['files']) == 7 assert files['format'] == 1 assert len(files.keys()) == 2 - valid_files_entries = ['.', 'roles', 'docs', 'plugins', 'plugins/README.md', 'README.md'] + valid_files_entries = ['.', 'roles', 'docs', 'plugins', 'plugins/README.md', 'README.md', 'runme.sh'] for file_entry in files['files']: assert file_entry['name'] in valid_files_entries assert file_entry['format'] == 1 - if file_entry['name'] == 'plugins/README.md': + if file_entry['name'] in ['plugins/README.md', 'runme.sh']: assert file_entry['ftype'] == 'file' assert file_entry['chksum_type'] == 'sha256' - # Can't test the actual checksum as the html link changes based on the version. + # Can't test the actual checksum as the html link changes based on the version or the file contents + # don't matter assert file_entry['chksum_sha256'] is not None elif file_entry['name'] == 'README.md': assert file_entry['ftype'] == 'file' diff --git a/test/units/galaxy/test_collection_install.py b/test/units/galaxy/test_collection_install.py index 9318d31dc7c..8c821d7423a 100644 --- a/test/units/galaxy/test_collection_install.py +++ b/test/units/galaxy/test_collection_install.py @@ -12,6 +12,7 @@ import os import pytest import re import shutil +import stat import tarfile import yaml @@ -139,6 +140,12 @@ def collection_artifact(request, tmp_path_factory): galaxy_obj.write(to_bytes(yaml.safe_dump(existing_yaml))) galaxy_obj.truncate() + # Create a file with +x in the collection so we can test the permissions + execute_path = os.path.join(collection_path, 'runme.sh') + with open(execute_path, mode='wb') as fd: + fd.write(b"echo hi") + os.chmod(execute_path, os.stat(execute_path).st_mode | stat.S_IEXEC) + call_galaxy_cli(['build', collection_path, '--output-path', test_dir]) collection_tar = os.path.join(test_dir, '%s-%s-0.1.0.tar.gz' % (namespace, collection)) @@ -634,7 +641,12 @@ def test_install_collection(collection_artifact, monkeypatch): actual_files = os.listdir(collection_path) actual_files.sort() - assert actual_files == [b'FILES.json', b'MANIFEST.json', b'README.md', b'docs', b'playbooks', b'plugins', b'roles'] + assert actual_files == [b'FILES.json', b'MANIFEST.json', b'README.md', b'docs', b'playbooks', b'plugins', b'roles', + b'runme.sh'] + + assert stat.S_IMODE(os.stat(os.path.join(collection_path, b'plugins')).st_mode) == 0o0755 + assert stat.S_IMODE(os.stat(os.path.join(collection_path, b'README.md')).st_mode) == 0o0644 + assert stat.S_IMODE(os.stat(os.path.join(collection_path, b'runme.sh')).st_mode) == 0o0755 assert mock_display.call_count == 1 assert mock_display.mock_calls[0][1][0] == "Installing 'ansible_namespace.collection:0.1.0' to '%s'" \ @@ -668,7 +680,8 @@ def test_install_collection_with_download(galaxy_server, collection_artifact, mo actual_files = os.listdir(collection_path) actual_files.sort() - assert actual_files == [b'FILES.json', b'MANIFEST.json', b'README.md', b'docs', b'playbooks', b'plugins', b'roles'] + assert actual_files == [b'FILES.json', b'MANIFEST.json', b'README.md', b'docs', b'playbooks', b'plugins', b'roles', + b'runme.sh'] assert mock_display.call_count == 1 assert mock_display.mock_calls[0][1][0] == "Installing 'ansible_namespace.collection:0.1.0' to '%s'" \ @@ -696,7 +709,8 @@ def test_install_collections_from_tar(collection_artifact, monkeypatch): actual_files = os.listdir(collection_path) actual_files.sort() - assert actual_files == [b'FILES.json', b'MANIFEST.json', b'README.md', b'docs', b'playbooks', b'plugins', b'roles'] + assert actual_files == [b'FILES.json', b'MANIFEST.json', b'README.md', b'docs', b'playbooks', b'plugins', b'roles', + b'runme.sh'] with open(os.path.join(collection_path, b'MANIFEST.json'), 'rb') as manifest_obj: actual_manifest = json.loads(to_text(manifest_obj.read())) @@ -728,7 +742,7 @@ def test_install_collections_existing_without_force(collection_artifact, monkeyp actual_files = os.listdir(collection_path) actual_files.sort() - assert actual_files == [b'README.md', b'docs', b'galaxy.yml', b'playbooks', b'plugins', b'roles'] + assert actual_files == [b'README.md', b'docs', b'galaxy.yml', b'playbooks', b'plugins', b'roles', b'runme.sh'] # Filter out the progress cursor display calls. display_msgs = [m[1][0] for m in mock_display.mock_calls if 'newline' not in m[2] and len(m[1]) == 1] @@ -759,7 +773,8 @@ def test_install_collection_with_circular_dependency(collection_artifact, monkey actual_files = os.listdir(collection_path) actual_files.sort() - assert actual_files == [b'FILES.json', b'MANIFEST.json', b'README.md', b'docs', b'playbooks', b'plugins', b'roles'] + assert actual_files == [b'FILES.json', b'MANIFEST.json', b'README.md', b'docs', b'playbooks', b'plugins', b'roles', + b'runme.sh'] with open(os.path.join(collection_path, b'MANIFEST.json'), 'rb') as manifest_obj: actual_manifest = json.loads(to_text(manifest_obj.read()))