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

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.

test/lib/ansible_test/_internal/commands/coverage/combine.py

@@ -34,6 +34,7 @@ from ...executor import (
 
 from ...data import (
     data_context,
+    PayloadConfig,
 )
 
 from ...host_configs import (
@@ -82,9 +83,10 @@ def combine_coverage_files(args: CoverageCombineConfig, host_state: HostState) -
 
             pairs = [(path, os.path.relpath(path, data_context().content.root)) for path in exported_paths]
 
-            def coverage_callback(files: list[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)

test/lib/ansible_test/_internal/commands/integration/__init__.py

@@ -90,6 +90,7 @@ from .cloud import (
 
 from ...data import (
     data_context,
+    PayloadConfig,
 )
 
 from ...host_configs import (
@@ -214,11 +215,13 @@ def delegate_inventory(args: IntegrationConfig, inventory_path_src: str) -> None
     if isinstance(args, PosixIntegrationConfig):
         return
 
-    def inventory_callback(files: list[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
 
@@ -937,11 +940,12 @@ def command_integration_filter(args: 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: list[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)

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: list[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)

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: list[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: list[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: list[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)

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 time
@@ -46,6 +47,7 @@ from .ci import (
 
 from .data import (
     data_context,
+    PayloadConfig,
 )
 
 
@@ -441,14 +443,19 @@ class SshKey:
         key, pub = key_pair
         key_dst, pub_dst = self.get_in_tree_key_pair_paths()
 
-        def ssh_key_callback(files: list[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

test/lib/ansible_test/_internal/data.py

@@ -50,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) -> None:
@@ -63,7 +70,7 @@ class DataContext:
         self.__source_providers = source_providers
         self.__ansible_source: t.Optional[tuple[tuple[str, str], ...]] = None
 
-        self.payload_callbacks: list[c.Callable[[list[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)
@@ -173,7 +180,7 @@ class DataContext:
 
         return self.__ansible_source
 
-    def register_payload_callback(self, callback: c.Callable[[list[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)
 

test/lib/ansible_test/_internal/delegation.py

@@ -177,7 +177,6 @@ def delegate_command(args: EnvironmentConfig, host_state: HostState, exclude: li
             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)

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,64 @@ def create_payload(args: CommonConfig, dst_path: 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."""
-        tar_info.mode |= stat.S_IXUSR | stat.S_IXOTH | stat.S_IXGRP
+        """
+        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'))
+        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)
+
         return tar_info
 
     if not ANSIBLE_SOURCE_ROOT:
@@ -85,10 +139,15 @@ def create_payload(args: CommonConfig, dst_path: 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 +168,7 @@ def create_payload(args: CommonConfig, dst_path: 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)