From 8e2148943b84d9cf9314c35bcd0035b48a04070f Mon Sep 17 00:00:00 2001 From: Matt Clay Date: Tue, 14 Mar 2023 15:56:21 -0700 Subject: [stable-2.13] ansible-test - Fix file permissions for delegation (#80204) * [stable-2.13] ansible-test - Fix file permissions for delegation (#79932) * ansible-test - Fix file permissions for delegation * Set more restrictive permissions for SSH key * Check all execute bits, not just owner * Add a breaking_changes changelog entry. (cherry picked from commit c8c1402ff66cf971469b7d49ada9fde894dabe0d) Co-authored-by: Matt Clay * ansible-test - Fix collection delegation (#79947) (cherry picked from commit 079383384790310dd6722b08ac18990e2a4d0ed9) --------- Co-authored-by: Felix Fontein --- .../ansible-test-payload-file-permissions.yml | 9 +++ .../_internal/commands/coverage/combine.py | 4 +- .../_internal/commands/integration/__init__.py | 8 ++- .../commands/integration/cloud/__init__.py | 4 +- test/lib/ansible_test/_internal/config.py | 12 +++- test/lib/ansible_test/_internal/core_ci.py | 9 ++- test/lib/ansible_test/_internal/data.py | 12 +++- test/lib/ansible_test/_internal/delegation.py | 1 - test/lib/ansible_test/_internal/payload.py | 73 ++++++++++++++++++++-- 9 files changed, 115 insertions(+), 17 deletions(-) create mode 100644 changelogs/fragments/ansible-test-payload-file-permissions.yml diff --git a/changelogs/fragments/ansible-test-payload-file-permissions.yml b/changelogs/fragments/ansible-test-payload-file-permissions.yml new file mode 100644 index 0000000000..e6f0fb8c53 --- /dev/null +++ b/changelogs/fragments/ansible-test-payload-file-permissions.yml @@ -0,0 +1,9 @@ +bugfixes: + - ansible-test - Use consistent file permissions when delegating tests to a container or remote host. + Files with any execute bit set will use permissions ``755``. + All other files will use permissions ``644``. + (Resolves issue https://github.com/ansible/ansible/issues/75079) +breaking_changes: + - ansible-test - Integration tests which depend on specific file permissions when running in an ansible-test managed + host environment may require changes. Tests that require permissions other than ``755`` or ``644`` + may need to be updated to set the necessary permissions as part of the test run. diff --git a/test/lib/ansible_test/_internal/commands/coverage/combine.py b/test/lib/ansible_test/_internal/commands/coverage/combine.py index 8458081f05..9d43cb041d 100644 --- a/test/lib/ansible_test/_internal/commands/coverage/combine.py +++ b/test/lib/ansible_test/_internal/commands/coverage/combine.py @@ -33,6 +33,7 @@ from ...executor import ( from ...data import ( data_context, + PayloadConfig, ) from ...host_configs import ( @@ -81,9 +82,10 @@ def combine_coverage_files(args, host_state): # type: (CoverageCombineConfig, H pairs = [(path, os.path.relpath(path, data_context().content.root)) for path in exported_paths] - def coverage_callback(files): # type: (t.List[t.Tuple[str, str]]) -> None + def coverage_callback(payload_config: PayloadConfig) -> None: """Add the coverage files to the payload file list.""" display.info('Including %d exported coverage file(s) in payload.' % len(pairs), verbosity=1) + files = payload_config.files files.extend(pairs) data_context().register_payload_callback(coverage_callback) diff --git a/test/lib/ansible_test/_internal/commands/integration/__init__.py b/test/lib/ansible_test/_internal/commands/integration/__init__.py index 2ae1e39c9d..21e68037d3 100644 --- a/test/lib/ansible_test/_internal/commands/integration/__init__.py +++ b/test/lib/ansible_test/_internal/commands/integration/__init__.py @@ -89,6 +89,7 @@ from .cloud import ( from ...data import ( data_context, + PayloadConfig, ) from ...host_configs import ( @@ -213,11 +214,13 @@ def delegate_inventory(args, inventory_path_src): # type: (IntegrationConfig, s if isinstance(args, PosixIntegrationConfig): return - def inventory_callback(files): # type: (t.List[t.Tuple[str, str]]) -> None + def inventory_callback(payload_config: PayloadConfig) -> None: """ Add the inventory file to the payload file list. This will preserve the file during delegation even if it is ignored or is outside the content and install roots. """ + files = payload_config.files + inventory_path = get_inventory_relative_path(args) inventory_tuple = inventory_path_src, inventory_path @@ -940,11 +943,12 @@ def command_integration_filter(args, # type: TIntegrationConfig vars_file_src = os.path.join(data_context().content.root, data_context().content.integration_vars_path) if os.path.exists(vars_file_src): - def integration_config_callback(files): # type: (t.List[t.Tuple[str, str]]) -> None + def integration_config_callback(payload_config: PayloadConfig) -> None: """ Add the integration config vars file to the payload file list. This will preserve the file during delegation even if the file is ignored by source control. """ + files = payload_config.files files.append((vars_file_src, data_context().content.integration_vars_path)) data_context().register_payload_callback(integration_config_callback) diff --git a/test/lib/ansible_test/_internal/commands/integration/cloud/__init__.py b/test/lib/ansible_test/_internal/commands/integration/cloud/__init__.py index 3ca8171947..92540bc3bf 100644 --- a/test/lib/ansible_test/_internal/commands/integration/cloud/__init__.py +++ b/test/lib/ansible_test/_internal/commands/integration/cloud/__init__.py @@ -47,6 +47,7 @@ from ....ci import ( from ....data import ( data_context, + PayloadConfig, ) from ....docker_util import ( @@ -189,13 +190,14 @@ class CloudBase(metaclass=abc.ABCMeta): self.args = args self.platform = self.__module__.rsplit('.', 1)[-1] - def config_callback(files): # type: (t.List[t.Tuple[str, str]]) -> None + def config_callback(payload_config: PayloadConfig) -> None: """Add the config file to the payload file list.""" if self.platform not in self.args.metadata.cloud_config: return # platform was initialized, but not used -- such as being skipped due to all tests being disabled if self._get_cloud_config(self._CONFIG_PATH, ''): pair = (self.config_path, os.path.relpath(self.config_path, data_context().content.root)) + files = payload_config.files if pair not in files: display.info('Including %s config: %s -> %s' % (self.platform, pair[0], pair[1]), verbosity=3) diff --git a/test/lib/ansible_test/_internal/config.py b/test/lib/ansible_test/_internal/config.py index 4061dd8ae5..6c5ff8c4b6 100644 --- a/test/lib/ansible_test/_internal/config.py +++ b/test/lib/ansible_test/_internal/config.py @@ -24,6 +24,7 @@ from .metadata import ( from .data import ( data_context, + PayloadConfig, ) from .host_configs import ( @@ -114,7 +115,7 @@ class EnvironmentConfig(CommonConfig): self.dev_systemd_debug: bool = args.dev_systemd_debug self.dev_probe_cgroups: t.Optional[str] = args.dev_probe_cgroups - def host_callback(files): # type: (t.List[t.Tuple[str, str]]) -> None + def host_callback(payload_config: PayloadConfig) -> None: """Add the host files to the payload file list.""" config = self @@ -123,6 +124,8 @@ class EnvironmentConfig(CommonConfig): state_path = os.path.join(config.host_path, 'state.dat') config_path = os.path.join(config.host_path, 'config.dat') + files = payload_config.files + files.append((os.path.abspath(settings_path), settings_path)) files.append((os.path.abspath(state_path), state_path)) files.append((os.path.abspath(config_path), config_path)) @@ -225,9 +228,10 @@ class TestConfig(EnvironmentConfig): if self.coverage_check: self.coverage = True - def metadata_callback(files): # type: (t.List[t.Tuple[str, str]]) -> None + def metadata_callback(payload_config: PayloadConfig) -> None: """Add the metadata file to the payload file list.""" config = self + files = payload_config.files if config.metadata_path: files.append((os.path.abspath(config.metadata_path), config.metadata_path)) @@ -264,8 +268,10 @@ class SanityConfig(TestConfig): self.display_stderr = self.lint or self.list_tests if self.keep_git: - def git_callback(files): # type: (t.List[t.Tuple[str, str]]) -> None + def git_callback(payload_config: PayloadConfig) -> None: """Add files from the content root .git directory to the payload file list.""" + files = payload_config.files + for dirpath, _dirnames, filenames in os.walk(os.path.join(data_context().content.root, '.git')): paths = [os.path.join(dirpath, filename) for filename in filenames] files.extend((path, os.path.relpath(path, data_context().content.root)) for path in paths) diff --git a/test/lib/ansible_test/_internal/core_ci.py b/test/lib/ansible_test/_internal/core_ci.py index 62d063b2b7..ee9520dd9b 100644 --- a/test/lib/ansible_test/_internal/core_ci.py +++ b/test/lib/ansible_test/_internal/core_ci.py @@ -6,6 +6,7 @@ import dataclasses import json import os import re +import stat import traceback import uuid import errno @@ -47,6 +48,7 @@ from .ci import ( from .data import ( data_context, + PayloadConfig, ) @@ -447,14 +449,19 @@ class SshKey: key, pub = key_pair key_dst, pub_dst = self.get_in_tree_key_pair_paths() - def ssh_key_callback(files): # type: (t.List[t.Tuple[str, str]]) -> None + def ssh_key_callback(payload_config: PayloadConfig) -> None: """ Add the SSH keys to the payload file list. They are either outside the source tree or in the cache dir which is ignored by default. """ + files = payload_config.files + permissions = payload_config.permissions + files.append((key, os.path.relpath(key_dst, data_context().content.root))) files.append((pub, os.path.relpath(pub_dst, data_context().content.root))) + permissions[os.path.relpath(key_dst, data_context().content.root)] = stat.S_IRUSR | stat.S_IWUSR + data_context().register_payload_callback(ssh_key_callback) self.key, self.pub = key, pub diff --git a/test/lib/ansible_test/_internal/data.py b/test/lib/ansible_test/_internal/data.py index 42fa5a2ac7..b4de2c60df 100644 --- a/test/lib/ansible_test/_internal/data.py +++ b/test/lib/ansible_test/_internal/data.py @@ -1,6 +1,7 @@ """Context information for the current invocation of ansible-test.""" from __future__ import annotations +import collections.abc as c import dataclasses import os import typing as t @@ -49,6 +50,13 @@ from .provider.layout.unsupported import ( ) +@dataclasses.dataclass(frozen=True) +class PayloadConfig: + """Configuration required to build a source tree payload for delegation.""" + files: list[tuple[str, str]] + permissions: dict[str, int] + + class DataContext: """Data context providing details about the current execution environment for ansible-test.""" def __init__(self): @@ -62,7 +70,7 @@ class DataContext: self.__source_providers = source_providers self.__ansible_source = None # type: t.Optional[t.Tuple[t.Tuple[str, str], ...]] - self.payload_callbacks = [] # type: t.List[t.Callable[[t.List[t.Tuple[str, str]]], None]] + self.payload_callbacks: list[c.Callable[[PayloadConfig], None]] = [] if content_path: content = self.__create_content_layout(layout_providers, source_providers, content_path, False) @@ -172,7 +180,7 @@ class DataContext: return self.__ansible_source - def register_payload_callback(self, callback): # type: (t.Callable[[t.List[t.Tuple[str, str]]], None]) -> None + def register_payload_callback(self, callback: c.Callable[[PayloadConfig], None]) -> None: """Register the given payload callback.""" self.payload_callbacks.append(callback) diff --git a/test/lib/ansible_test/_internal/delegation.py b/test/lib/ansible_test/_internal/delegation.py index b3b8ad51dc..770ccc0b27 100644 --- a/test/lib/ansible_test/_internal/delegation.py +++ b/test/lib/ansible_test/_internal/delegation.py @@ -172,7 +172,6 @@ def delegate_command(args, host_state, exclude, require): # type: (EnvironmentC con.run(['mkdir', '-p'] + writable_dirs, capture=True) con.run(['chmod', '777'] + writable_dirs, capture=True) con.run(['chmod', '755', working_directory], capture=True) - con.run(['chmod', '644', os.path.join(content_root, args.metadata_path)], capture=True) con.run(['useradd', pytest_user, '--create-home'], capture=True) con.run(insert_options(command, options + ['--requirements-mode', 'only']), capture=False) diff --git a/test/lib/ansible_test/_internal/payload.py b/test/lib/ansible_test/_internal/payload.py index e6ccc6ed5f..9bb234faec 100644 --- a/test/lib/ansible_test/_internal/payload.py +++ b/test/lib/ansible_test/_internal/payload.py @@ -27,6 +27,7 @@ from .util import ( from .data import ( data_context, + PayloadConfig, ) from .util_common import ( @@ -44,11 +45,66 @@ def create_payload(args, dst_path): # type: (CommonConfig, str) -> None return files = list(data_context().ansible_source) - filters = {} + permissions: dict[str, int] = {} + filters: dict[str, t.Callable[[tarfile.TarInfo], t.Optional[tarfile.TarInfo]]] = {} + + def apply_permissions(tar_info: tarfile.TarInfo, mode: int) -> t.Optional[tarfile.TarInfo]: + """ + Apply the specified permissions to the given file. + Existing file type bits are preserved. + """ + tar_info.mode &= ~(stat.S_IRWXU | stat.S_IRWXG | stat.S_IRWXO) + tar_info.mode |= mode + + return tar_info + + def make_executable(tar_info: tarfile.TarInfo) -> t.Optional[tarfile.TarInfo]: + """ + Make the given file executable and readable by all, and writeable by the owner. + Existing file type bits are preserved. + This ensures consistency of test results when using unprivileged users. + """ + return apply_permissions( + tar_info, + stat.S_IRUSR | stat.S_IRGRP | stat.S_IROTH | + stat.S_IXUSR | stat.S_IXGRP | stat.S_IXOTH | + stat.S_IWUSR + ) + + def make_non_executable(tar_info: tarfile.TarInfo) -> t.Optional[tarfile.TarInfo]: + """ + Make the given file readable by all, and writeable by the owner. + Existing file type bits are preserved. + This ensures consistency of test results when using unprivileged users. + """ + return apply_permissions( + tar_info, + stat.S_IRUSR | stat.S_IRGRP | stat.S_IROTH | + stat.S_IWUSR + ) + + def detect_permissions(tar_info: tarfile.TarInfo) -> t.Optional[tarfile.TarInfo]: + """ + Detect and apply the appropriate permissions for a file. + Existing file type bits are preserved. + This ensures consistency of test results when using unprivileged users. + """ + if tar_info.path.startswith('ansible/'): + mode = permissions.get(os.path.relpath(tar_info.path, 'ansible')) + elif data_context().content.collection and is_subdir(tar_info.path, data_context().content.collection.directory): + mode = permissions.get(os.path.relpath(tar_info.path, data_context().content.collection.directory)) + else: + mode = None + + if mode: + tar_info = apply_permissions(tar_info, mode) + elif tar_info.mode & (stat.S_IXUSR | stat.S_IXGRP | stat.S_IXOTH): + # If any execute bit is set, treat the file as executable. + # This ensures that sanity tests which check execute bits behave correctly. + tar_info = make_executable(tar_info) + else: + tar_info = make_non_executable(tar_info) - def make_executable(tar_info): # type: (tarfile.TarInfo) -> t.Optional[tarfile.TarInfo] - """Make the given file executable.""" - tar_info.mode |= stat.S_IXUSR | stat.S_IXOTH | stat.S_IXGRP return tar_info if not ANSIBLE_SOURCE_ROOT: @@ -85,10 +141,15 @@ def create_payload(args, dst_path): # type: (CommonConfig, str) -> None # there are no extra files when testing ansible itself extra_files = [] + payload_config = PayloadConfig( + files=content_files, + permissions=permissions, + ) + for callback in data_context().payload_callbacks: # execute callbacks only on the content paths # this is done before placing them in the appropriate subdirectory (see below) - callback(content_files) + callback(payload_config) # place ansible source files under the 'ansible' directory on the delegated host files = [(src, os.path.join('ansible', dst)) for src, dst in files] @@ -109,7 +170,7 @@ def create_payload(args, dst_path): # type: (CommonConfig, str) -> None with tarfile.open(dst_path, mode='w:gz', compresslevel=4, format=tarfile.GNU_FORMAT) as tar: for src, dst in files: display.info('%s -> %s' % (src, dst), verbosity=4) - tar.add(src, dst, filter=filters.get(dst)) + tar.add(src, dst, filter=filters.get(dst, detect_permissions)) duration = time.time() - start payload_size_bytes = os.path.getsize(dst_path) -- cgit v1.2.1