diff --git a/src/deadline_worker_agent/aws_credentials/aws_configs.py b/src/deadline_worker_agent/aws_credentials/aws_configs.py index c381f9f4..f92935c6 100644 --- a/src/deadline_worker_agent/aws_credentials/aws_configs.py +++ b/src/deadline_worker_agent/aws_credentials/aws_configs.py @@ -2,18 +2,19 @@ from __future__ import annotations -import stat -import os -import logging from abc import ABC, abstractmethod from configparser import ConfigParser from pathlib import Path +from shutil import chown from typing import Optional +import logging +import os +import stat + from openjd.sessions import PosixSessionUser, SessionUser -from subprocess import run, DEVNULL, PIPE, STDOUT + from ..file_system_operations import ( FileSystemPermissionEnum, - make_directory, touch_file, ) @@ -25,51 +26,26 @@ _logger = logging.getLogger(__name__) -def _run_cmd_as(*, user: PosixSessionUser, cmd: list[str]) -> None: - sudo = ["sudo", "-u", user.user, "-i"] - # Raises: CalledProcessError - run(sudo + cmd, stdin=DEVNULL, stderr=STDOUT, stdout=PIPE, check=True) - - -def _setup_parent_dir(*, dir_path: Path, owner: SessionUser | None = None) -> None: - if os.name == "posix": - if owner is None: - create_perms: int = stat.S_IRWXU - dir_path.mkdir(mode=create_perms, exist_ok=True) - else: - assert isinstance(owner, PosixSessionUser) - _run_cmd_as(user=owner, cmd=["mkdir", "-p", str(dir_path)]) - _run_cmd_as(user=owner, cmd=["chown", f"{owner.user}:{owner.group}", str(dir_path)]) - _run_cmd_as(user=owner, cmd=["chmod", "770", str(dir_path)]) - else: - if owner is None: - make_directory( - dir_path=dir_path, - agent_user_permission=FileSystemPermissionEnum.READ_WRITE, - ) - else: - make_directory( - dir_path=dir_path, - permitted_user=owner, - user_permission=FileSystemPermissionEnum.READ_WRITE, - agent_user_permission=FileSystemPermissionEnum.FULL_CONTROL, - parents=True, - exist_ok=True, - ) - - def _setup_file(*, file_path: Path, owner: SessionUser | None = None) -> None: if os.name == "posix": if owner is None: - if not file_path.exists(): - file_path.touch() + # Read-write for owner user mode = stat.S_IRUSR | stat.S_IWUSR + file_path.touch(mode=mode) file_path.chmod(mode=mode) else: assert isinstance(owner, PosixSessionUser) - _run_cmd_as(user=owner, cmd=["touch", str(file_path)]) - _run_cmd_as(user=owner, cmd=["chown", f"{owner.user}:{owner.group}", str(file_path)]) - _run_cmd_as(user=owner, cmd=["chmod", "660", str(file_path)]) + mode = ( + # Read/write for owner user + stat.S_IRUSR + | stat.S_IWUSR + | + # Read for owner group + stat.S_IRGRP + ) + file_path.touch(mode=mode) + file_path.chmod(mode=mode) + chown(file_path, group=owner.group) else: if owner is None: touch_file( @@ -80,7 +56,7 @@ def _setup_file(*, file_path: Path, owner: SessionUser | None = None) -> None: touch_file( file_path=file_path, permitted_user=owner, - user_permission=FileSystemPermissionEnum.READ_WRITE, + user_permission=FileSystemPermissionEnum.READ, agent_user_permission=FileSystemPermissionEnum.FULL_CONTROL, ) @@ -95,11 +71,16 @@ class _AWSConfigBase(ABC): uninstalling the config. """ - _config_path: Path _config_parser: ConfigParser _os_user: Optional[SessionUser] - - def __init__(self, os_user: Optional[SessionUser]) -> None: + _parent_dir: Path + + def __init__( + self, + *, + os_user: Optional[SessionUser], + parent_dir: Path, + ) -> None: """ Constructor for the AWSConfigBase class @@ -107,29 +88,23 @@ def __init__(self, os_user: Optional[SessionUser]) -> None: os_user (Optional[SessionUser]): If non-None, then this is the os user to add read permissions for. If None, then the only the process user will be able to read the credentials files. + parent_dir (Path): The directory where the AWS config and credentials files will be + written to. """ super().__init__() - self._config_path = self._get_path( - os_user=os_user.user if os_user is not None else "" # type: ignore - ) - self._config_parser = ConfigParser() + self._parent_dir = parent_dir - # setup the containing directory permissions and ownership - config_dir = self._config_path.parent - _setup_parent_dir( - dir_path=config_dir, - owner=os_user, - ) + self._config_parser = ConfigParser() # ensure the file exists and has correct permissions and ownership _setup_file( - file_path=self._config_path, + file_path=self.path, owner=os_user, ) # finally, read the config - self._config_parser.read(self._config_path) + self._config_parser.read(self.path) def install_credential_process(self, profile_name: str, script_path: Path) -> None: """ @@ -163,8 +138,8 @@ def _write(self) -> None: """ Writes the config to the config path given in the constructor """ - _logger.info(f"Writing updated {self._config_path} to disk.") - with self._config_path.open(mode="w") as fp: + _logger.info(f"Writing updated {self.path} to disk.") + with self.path.open(mode="w") as fp: self._config_parser.write(fp=fp, space_around_delimiters=False) @abstractmethod @@ -177,10 +152,13 @@ def _get_profile_name(self, profile_name: str) -> str: # pragma: no cover """ raise NotImplementedError("_get_profile_name is not implemented by _AWSConfigBase") - @staticmethod + @property @abstractmethod - def _get_path(os_user: str) -> Path: # pragma: no cover - raise NotImplementedError("_get_path is not implemented by _AWSConfigBase") + def path(self) -> Path: # pragma: no cover + typ = type(self) + raise NotImplementedError( + f"path property is not implemented by {typ.__module__}.{typ.__name__}" + ) class AWSConfig(_AWSConfigBase): @@ -191,9 +169,9 @@ class AWSConfig(_AWSConfigBase): def _get_profile_name(self, profile_name: str) -> str: return f"profile {profile_name}" - @staticmethod - def _get_path(os_user: str) -> Path: - return Path(f"~{os_user}/.aws/config").expanduser() + @property + def path(self) -> Path: + return self._parent_dir / "config" class AWSCredentials(_AWSConfigBase): @@ -204,6 +182,6 @@ class AWSCredentials(_AWSConfigBase): def _get_profile_name(self, profile_name: str) -> str: return profile_name - @staticmethod - def _get_path(os_user: str) -> Path: - return Path(f"~{os_user}/.aws/credentials").expanduser() + @property + def path(self) -> Path: + return self._parent_dir / "credentials" diff --git a/src/deadline_worker_agent/aws_credentials/queue_boto3_session.py b/src/deadline_worker_agent/aws_credentials/queue_boto3_session.py index dbabea30..e2b6963b 100644 --- a/src/deadline_worker_agent/aws_credentials/queue_boto3_session.py +++ b/src/deadline_worker_agent/aws_credentials/queue_boto3_session.py @@ -2,25 +2,29 @@ from __future__ import annotations -import os -import logging -from typing import Any, Optional, cast +# Built-in from pathlib import Path +import shlex +from threading import Event +from typing import Any, Optional, cast +import logging +import os import shutil import stat -from threading import Event +import subprocess +# Third-party from botocore.utils import JSONFileCache from openjd.sessions import PosixSessionUser, WindowsSessionUser, SessionUser +# First-party from ..boto import DeadlineClient from ..file_system_operations import ( make_directory, set_permissions, FileSystemPermissionEnum, ) - -from .temporary_credentials import TemporaryCredentials +from .aws_configs import AWSConfig, AWSCredentials from ..aws.deadline import ( DeadlineRequestUnrecoverableError, DeadlineRequestInterrupted, @@ -28,32 +32,60 @@ DeadlineRequestConditionallyRecoverableError, assume_queue_role_for_worker, ) -from .aws_configs import AWSConfig, AWSCredentials from .boto3_sessions import BaseBoto3Session, SettableCredentials +from .temporary_credentials import TemporaryCredentials + _logger = logging.getLogger(__name__) class QueueBoto3Session(BaseBoto3Session): """A Boto3 Session that contains Queue Role AWS Credentials for use by: - 1. Any service Session Action run within an Open Job Description Session; and - 2. The Worker when performing actions on behalf of a service Session Action for - an Open Job Description Session. + + 1. Any session action run within an Open Job Description session; and + 2. The Worker when performing actions on behalf of a session [action] for an Open Job + Description session. When created, this Session: - 1. Installs an AWS Credentials Process in the ~/.aws of the given os_user, or the current user if - not provided. - 2. Creates a directory in which to put: a/ a file containing this session's credentials; and b/ a - script file to use as an AWS Credentials Process for the Queue's job user. - - **If you create an instance of this class, then you must ensure that a code path will always - result in the cleanup() method of that instance being called when done with the instance object.** - - Calling QueueBoto3Session.refresh_credentials() will cause a service call to AssumeQueueRoleForWorker. - When successful, a refresh will: - 1. Update the AWS Credentials stored & used by this Boto3 Session; - 2. Persist the obtained AWS Credentials to disk for use in the credential's process; and - 3. Update this Boto3 Session's AWS Credentials will be updated with the result. + + 1. Creates a queue-specific directory under the worker agent's persistence directory. + The directory ownership and permissions are configured such that the OS user that the worker + agent runs as is able to write to it and the job user is able to read from it. No other + OS users are granted access to these files + + The directory contains: + + * an AWS config file + * an AWS credentials file + * a script file to use as an AWS Credentials Process for the Queue's job user + * a file containing a JSON representation of the session's credentials + + 2. Creates an aws profile with a "credential_process" within the AWS config and credentials + files. This looks like: + + ```ini + [profile queue-897585318504478c9bc7eeeae7785dbb] + credential_process=/var/lib/deadline/queues/queue-897585318504478c9bc7eeeae7785dbb/get_aws_credentials + ``` + + This feature is supported by official AWS SDKs and the CLI to provide IAM credentials. + See https://docs.aws.amazon.com/sdkref/latest/guide/feature-process-credentials.html + + 3. Calls AssumeQueueRoleForWorker and writes the resulting credentials in the format expected + by credential_process to the JSON file. + + ****************************************** IMPORTANT ******************************************* + If you successfully create an instance of this class, then you must ensure that a code path will + alwaysresult in the cleanup() method of that instance being called when done with the instance + object. + ****************************************** IMPORTANT ******************************************* + + Calling QueueBoto3Session.refresh_credentials() will cause a service call to + AssumeQueueRoleForWorker. When successful, a refresh will: + + 1. Update the AWS Credentials stored & used by this Boto3 Session; + 2. Persist the obtained AWS Credentials to disk for use in the credential's process; and + 3. Update this Boto3 Session's AWS Credentials will be updated with the result. """ _deadline_client: DeadlineClient @@ -76,7 +108,7 @@ class QueueBoto3Session(BaseBoto3Session): _file_cache: JSONFileCache # Basename of the filename (minus extension) of the file that credentials are written to - _credentials_filename: str + _credentials_filename_no_ext: str # Location of the credentials process script written to disk _credentials_process_script_path: Path @@ -108,9 +140,9 @@ def __init__( self._profile_name = f"deadline-{self._queue_id}" - self._credential_dir = worker_persistence_dir / "queues" / self._queue_id + self._credential_dir = self._get_credentials_dir(worker_persistence_dir, queue_id) self._file_cache = JSONFileCache(working_dir=self._credential_dir) - self._credentials_filename = ( + self._credentials_filename_no_ext = ( "aws_credentials" # note: .json extension added by JSONFileCache ) @@ -119,18 +151,65 @@ def __init__( else: self._credentials_process_script_path = self._credential_dir / "get_aws_credentials.cmd" - self._aws_config = AWSConfig(self._os_user) - self._aws_credentials = AWSCredentials(self._os_user) + self._create_credentials_directory(os_user) + + self._aws_config = AWSConfig(os_user=self._os_user, parent_dir=self._credential_dir) + self._aws_credentials = AWSCredentials( + os_user=self._os_user, parent_dir=self._credential_dir + ) - self._create_credentials_directory() self._install_credential_process() + # Output at debug level queue credential file ownership and permissions + self._debug_path_permissions(self._credential_dir) + self._debug_path_permissions(self._aws_config.path) + self._debug_path_permissions(self._aws_credentials.path) + self._debug_path_permissions(self._credentials_process_script_path) + try: self.refresh_credentials() except: self.cleanup() raise + def _get_credentials_dir(self, worker_persistence_dir: Path, queue_id: str) -> Path: + return worker_persistence_dir / "queues" / queue_id + + def _debug_path_permissions(self, path: Path, level: int = logging.DEBUG) -> None: + """Outputs information about the ownership and permissions of a path. + + The output format is: + + | user = | group = | mode = + + Argument + -------- + path (Path): The path + level (int): The logging level. Defaults to DEBUG. + """ + + # This is a performance optimization since production workers will log at INFO or higher + if os.name == "posix" and logging.root.isEnabledFor(level) and _logger.isEnabledFor(level): + # These imports are not at the top because otherwise mypy complains with: + # + # src\deadline_worker_agent\aws_credentials\queue_boto3_session.py:203: error: + # Module has no attribute "getpwuid" [attr-defined] + import grp + import pwd + + if not path.exists(): + _logger.log(level, "path does not exist: %s", path) + return + st = path.stat() + _logger.log( + level, + "%s | user = %s | group = %s | mode = %s", + path, + pwd.getpwuid(st.st_uid).pw_name, # type: ignore[attr-defined] + grp.getgrgid(st.st_gid).gr_name, # type: ignore[attr-defined] + oct(st.st_mode), + ) + def cleanup(self) -> None: """This must be called when you are done with the constructed object. It deletes any files that were written to disk, and undoes changes to the @@ -146,6 +225,16 @@ def credential_process_profile_name(self) -> str: """ return self._profile_name + @property + def aws_config(self) -> AWSConfig: + """The path to the AWS configuration file""" + return self._aws_config + + @property + def aws_credentials(self) -> AWSCredentials: + """The path to the AWS credentials file""" + return self._aws_credentials + @property def has_credentials(self) -> bool: """Query whether or not the Session has AWS Credentials. @@ -233,28 +322,38 @@ def refresh_credentials(self) -> None: # Something was bad with the response. That's unrecoverable. raise DeadlineRequestUnrecoverableError(e) + credentials_file_path = self._credentials_file_path() + if temporary_creds: - temporary_creds.cache(cache=self._file_cache, cache_key=self._credentials_filename) + temporary_creds.cache( + cache=self._file_cache, cache_key=self._credentials_filename_no_ext + ) + self._debug_path_permissions(credentials_file_path) if self._os_user is not None: if os.name == "posix": assert isinstance(self._os_user, PosixSessionUser) - (self._credential_dir / self._credentials_filename).with_suffix(".json").chmod( - stat.S_IRUSR | stat.S_IWUSR | stat.S_IRGRP - ) + credentials_file_path.chmod(stat.S_IRUSR | stat.S_IWUSR | stat.S_IRGRP) shutil.chown( - (self._credential_dir / self._credentials_filename).with_suffix(".json"), + credentials_file_path, group=self._os_user.group, ) + self._debug_path_permissions(credentials_file_path) else: assert isinstance(self._os_user, WindowsSessionUser) set_permissions( - file_path=(self._credential_dir / self._credentials_filename).with_suffix( - ".json" - ), + file_path=credentials_file_path, permitted_user=self._os_user, agent_user_permission=FileSystemPermissionEnum.READ_WRITE, user_permission=FileSystemPermissionEnum.READ, ) + elif os.name == "posix": + credentials_file_path.chmod(stat.S_IRUSR | stat.S_IWUSR) + else: + set_permissions( + file_path=credentials_file_path, + permitted_user=self._os_user, + agent_user_permission=FileSystemPermissionEnum.READ_WRITE, + ) credentials_object = cast(SettableCredentials, self.get_credentials()) credentials_object.set_credentials(temporary_creds.to_deadline()) @@ -266,24 +365,31 @@ def refresh_credentials(self) -> None: else: _logger.info("No AWS Credentials received for Queue %s.", self._queue_id) - def _create_credentials_directory(self) -> None: + def _create_credentials_directory(self, os_user: Optional[SessionUser] = None) -> None: """Creates the directory that we're going to write the credentials file to""" # make the /queues/ dir and set permissions if os.name == "posix": + mode: int = stat.S_IRWXU + if os_user: + mode = mode | stat.S_IXGRP | stat.S_IRGRP try: - self._credential_dir.mkdir( - exist_ok=True, parents=True, mode=(stat.S_IRWXU | stat.S_IXGRP | stat.S_IRGRP) - ) + self._credential_dir.mkdir(exist_ok=True, parents=True, mode=mode) + self._credential_dir.chmod(mode) except OSError: _logger.error( "Please check user permissions. Could not create directory: %s", str(self._credential_dir), ) raise - if self._os_user is not None: - if isinstance(self._os_user, PosixSessionUser): - shutil.chown(self._credential_dir, group=self._os_user.group) + if os_user is not None: + assert isinstance(os_user, PosixSessionUser) + _logger.debug( + "Changing group ownership of %s to %s", + self._credential_dir, + os_user.group, + ) + shutil.chown(self._credential_dir, group=os_user.group) else: if self._os_user is None: make_directory( @@ -336,11 +442,19 @@ def _install_credential_process(self) -> None: mode=mode, ) with open(descriptor, mode="w", encoding="utf-8") as f: - f.write(self._generate_credential_process_script()) + if os.name == "posix": + # If the file pre-existed, the mode argument in os.open(..., mode=...) will + # not be used. + os.chmod( + descriptor, + mode=mode, + ) + + # Change permissions if self._os_user is not None: if os.name == "posix": assert isinstance(self._os_user, PosixSessionUser) - shutil.chown(self._credentials_process_script_path, group=self._os_user.group) + shutil.chown(descriptor, group=self._os_user.group) else: assert isinstance(self._os_user, WindowsSessionUser) set_permissions( @@ -349,26 +463,38 @@ def _install_credential_process(self) -> None: agent_user_permission=FileSystemPermissionEnum.READ_WRITE, user_permission=FileSystemPermissionEnum.EXECUTE, ) + elif os.name == "nt": + # If the file pre-existed, the mode argument in os.open(..., mode=...) will + # not be used. + set_permissions( + file_path=self._credentials_process_script_path, + user_permission=FileSystemPermissionEnum.EXECUTE, + ) - # install credential process to ~/.aws/config and - # ~/.aws/credentials + f.write(self._generate_credential_process_script()) + + # install credential process to the AWS config and credentials files for aws_cred_file in (self._aws_config, self._aws_credentials): aws_cred_file.install_credential_process( self._profile_name, self._credentials_process_script_path ) + def _credentials_file_path(self) -> Path: + return (self._credential_dir / self._credentials_filename_no_ext).with_suffix(".json") + def _generate_credential_process_script(self) -> str: """ Generates the bash script which generates the credentials as JSON output on STDOUT. This script will be used by the installed credential process. """ + credential_files_path = self._credentials_file_path() if os.name == "posix": - return ("#!/bin/bash\nset -eu\ncat {0}\n").format( - (self._credential_dir / self._credentials_filename).with_suffix(".json") + return ("#!/bin/bash\nset -eu\n{0}").format( + " ".join(shlex.quote(arg) for arg in ["cat", str(credential_files_path)]) ) else: - return ('@echo off\ntype "{0}"\n').format( - (self._credential_dir / self._credentials_filename).with_suffix(".json") + return ("@echo off\n{0}\n").format( + subprocess.list2cmdline(["type", str(credential_files_path)]) ) def _uninstall_credential_process(self) -> None: diff --git a/src/deadline_worker_agent/scheduler/scheduler.py b/src/deadline_worker_agent/scheduler/scheduler.py index cf1ea388..4061b81a 100644 --- a/src/deadline_worker_agent/scheduler/scheduler.py +++ b/src/deadline_worker_agent/scheduler/scheduler.py @@ -15,6 +15,7 @@ from pathlib import Path from threading import Event, RLock, Lock, Timer from typing import Callable, Literal, Tuple, Union, cast, Optional, Any +import json import logging import os import stat @@ -786,8 +787,6 @@ def _create_new_sessions( ) as e: # Terminal error. We need to fail the Session. message = f"Unrecoverable error trying to obtain AWS Credentials for the Queue Role: {e}" - if str(e).startswith("Can't determine home directory"): - message += ". Possible non-valid username." self._fail_all_actions(session_spec, message) logger.warning("[%s] %s", new_session_id, message) # Force an immediate UpdateWorkerSchedule request @@ -826,21 +825,32 @@ def _create_new_sessions( self._wakeup.set() continue + env = { + "DEADLINE_SESSION_ID": new_session_id, + "DEADLINE_FARM_ID": self._farm_id, + "DEADLINE_QUEUE_ID": queue_id, + "DEADLINE_JOB_ID": job_id, + "DEADLINE_FLEET_ID": self._fleet_id, + "DEADLINE_WORKER_ID": self._worker_id, + } + if queue_credentials: + env.update( + { + "AWS_PROFILE": queue_credentials.session.credential_process_profile_name, + "AWS_CONFIG_FILE": str(queue_credentials.session.aws_config.path), + "AWS_SHARED_CREDENTIALS_FILE": str( + queue_credentials.session.aws_credentials.path + ), + } + ) + + logger.debug("env = \n%s", json.dumps(env, indent=2)) + session = Session( id=new_session_id, queue=queue, queue_id=queue_id, - env={ - "AWS_PROFILE": queue_credentials.session.credential_process_profile_name, - "DEADLINE_SESSION_ID": new_session_id, - "DEADLINE_FARM_ID": self._farm_id, - "DEADLINE_QUEUE_ID": queue_id, - "DEADLINE_JOB_ID": job_id, - "DEADLINE_FLEET_ID": self._fleet_id, - "DEADLINE_WORKER_ID": self._worker_id, - } - if queue_credentials - else None, + env=env, asset_sync=asset_sync, job_details=job_details, os_user=os_user, diff --git a/test/unit/aws_credentials/test_aws_configs.py b/test/unit/aws_credentials/test_aws_configs.py index 134e88c0..7adceeca 100644 --- a/test/unit/aws_credentials/test_aws_configs.py +++ b/test/unit/aws_credentials/test_aws_configs.py @@ -1,7 +1,7 @@ # Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved. import pytest -from unittest.mock import patch, MagicMock +from unittest.mock import patch, MagicMock, PropertyMock from pathlib import Path from typing import Type, Generator, Optional @@ -11,7 +11,6 @@ AWSCredentials, _AWSConfigBase, _setup_file, - _setup_parent_dir, ) from openjd.sessions import PosixSessionUser, WindowsSessionUser, SessionUser from deadline_worker_agent.file_system_operations import FileSystemPermissionEnum @@ -24,15 +23,9 @@ def profile_name() -> str: @pytest.fixture(autouse=True) -def mock_run_cmd_as() -> Generator[MagicMock, None, None]: - with patch.object(aws_configs_mod, "_run_cmd_as") as mock_run_cmd_as: - yield mock_run_cmd_as - - -@pytest.fixture(autouse=True) -def mock_make_directory() -> Generator[MagicMock, None, None]: - with patch.object(aws_configs_mod, "make_directory") as mock_make_directory: - yield mock_make_directory +def mock_chown() -> Generator[MagicMock, None, None]: + with patch.object(aws_configs_mod, "chown") as mock_chown: + yield mock_chown @pytest.fixture(autouse=True) @@ -41,99 +34,15 @@ def mock_touch_file() -> Generator[MagicMock, None, None]: yield mock_touch_file -@pytest.fixture -def os_user() -> Optional[SessionUser]: - if os.name == "posix": - return PosixSessionUser(user="user", group="group") +@pytest.fixture(params=[True, False]) +def os_user(request: pytest.FixtureRequest) -> Optional[SessionUser]: + if request.param: + if os.name == "posix": + return PosixSessionUser(user="user", group="group") + else: + return WindowsSessionUser(user="user", password="fakepassword") else: - return WindowsSessionUser(user="user", password="fakepassword") - - -class TestSetupParentDir: - """Tests for the _setup_parent_dir() function""" - - @pytest.fixture - def dir_path(self) -> MagicMock: - return MagicMock() - - def test_creates_dir( - self, - dir_path: MagicMock, - os_user: SessionUser, - mock_run_cmd_as: MagicMock, - mock_make_directory: MagicMock, - ) -> None: - """Tests that the directory is created if necessary with the expected permissions""" - # GIVEN - mkdir: MagicMock = dir_path.mkdir - - # WHEN - _setup_parent_dir(dir_path=dir_path, owner=os_user) - - # THEN - if os_user: - if os.name == "posix": - assert isinstance(os_user, PosixSessionUser) - mock_run_cmd_as.assert_any_call(user=os_user, cmd=["mkdir", "-p", str(dir_path)]) - mock_run_cmd_as.assert_any_call( - user=os_user, - cmd=["chown", f"{os_user.user}:{os_user.group}", str(dir_path)], - ) - mock_run_cmd_as.assert_any_call(user=os_user, cmd=["chmod", "770", str(dir_path)]) - else: - assert isinstance(os_user, WindowsSessionUser) - mock_make_directory.assert_called_once_with( - dir_path=dir_path, - permitted_user=os_user, - user_permission=FileSystemPermissionEnum.READ_WRITE, - agent_user_permission=FileSystemPermissionEnum.FULL_CONTROL, - parents=True, - exist_ok=True, - ) - else: # This branch isn't called? - if os.name == "posix": - mkdir.assert_called_once_with( - mode=0o700, - exist_ok=True, - ) - else: - mock_make_directory.assert_called_once() - - def test_sets_group_ownership( - self, - dir_path: MagicMock, - os_user: SessionUser, - mock_run_cmd_as: MagicMock, - mock_make_directory: MagicMock, - ) -> None: - """Tests that the directory group ownership is set as specified""" # Not clear how this test is different from the one above? - # GIVEN - - # WHEN - _setup_parent_dir(dir_path=dir_path, owner=os_user) - - # THEN - if os_user: - if os.name == "posix": - assert isinstance(os_user, PosixSessionUser) - mock_run_cmd_as.assert_any_call(user=os_user, cmd=["mkdir", "-p", str(dir_path)]) - mock_run_cmd_as.assert_any_call( - user=os_user, - cmd=["chown", f"{os_user.user}:{os_user.group}", str(dir_path)], - ) - mock_run_cmd_as.assert_any_call(user=os_user, cmd=["chmod", "770", str(dir_path)]) - else: - assert isinstance(os_user, WindowsSessionUser) - mock_make_directory.assert_called_once_with( - dir_path=dir_path, - permitted_user=os_user, - user_permission=FileSystemPermissionEnum.READ_WRITE, - agent_user_permission=FileSystemPermissionEnum.FULL_CONTROL, - parents=True, - exist_ok=True, - ) - else: # This branch isn't called ? - mock_run_cmd_as.assert_not_called() + return None class TestSetupFile: @@ -163,7 +72,6 @@ def test_creates_file_if_needed( file_path: MagicMock, os_user: Optional[SessionUser], exists: bool, - mock_run_cmd_as: MagicMock, mock_touch_file: MagicMock, ) -> None: """Tests the config/credentials file is created if necessary""" @@ -179,32 +87,29 @@ def test_creates_file_if_needed( if os_user: if os.name == "posix": assert isinstance(os_user, PosixSessionUser) - mock_run_cmd_as.assert_any_call(user=os_user, cmd=["touch", str(file_path)]) - mock_run_cmd_as.assert_any_call( - user=os_user, - cmd=["chown", f"{os_user.user}:{os_user.group}", str(file_path)], - ) - mock_run_cmd_as.assert_any_call(user=os_user, cmd=["chmod", "660", str(file_path)]) + file_path.touch.assert_called_once_with(mode=0o640) else: assert isinstance(os_user, WindowsSessionUser) mock_touch_file.assert_called_once_with( file_path=file_path, permitted_user=os_user, - user_permission=FileSystemPermissionEnum.READ_WRITE, + user_permission=FileSystemPermissionEnum.READ, agent_user_permission=FileSystemPermissionEnum.FULL_CONTROL, ) else: - file_path.exists.assert_called_once_with() - if exists: - file_path.touch.assert_not_called() + if os.name == "posix": + file_path.touch.assert_called_once_with(mode=0o600) + file_path.chmod.assert_called_once_with(mode=0o600) else: - file_path.touch.assert_called_once_with() + mock_touch_file( + file_path=file_path, + agent_user_permission=FileSystemPermissionEnum.READ_WRITE, + ) def test_changes_permissions( self, file_path: MagicMock, os_user: Optional[SessionUser], - mock_run_cmd_as: MagicMock, mock_touch_file: MagicMock, ) -> None: """Tests the config/credentials file is created if necessary""" @@ -218,29 +123,29 @@ def test_changes_permissions( if os_user: if os.name == "posix": assert isinstance(os_user, PosixSessionUser) - mock_run_cmd_as.assert_any_call(user=os_user, cmd=["touch", str(file_path)]) - mock_run_cmd_as.assert_any_call( - user=os_user, - cmd=["chown", f"{os_user.user}:{os_user.group}", str(file_path)], - ) - mock_run_cmd_as.assert_any_call(user=os_user, cmd=["chmod", "660", str(file_path)]) + file_path.chmod.assert_called_once_with(mode=0o640) else: assert isinstance(os_user, WindowsSessionUser) mock_touch_file.assert_called_once_with( file_path=file_path, permitted_user=os_user, - user_permission=FileSystemPermissionEnum.READ_WRITE, + user_permission=FileSystemPermissionEnum.READ, agent_user_permission=FileSystemPermissionEnum.FULL_CONTROL, ) + elif os.name == "posix": + chmod.assert_called_once_with(mode=0o600) else: - chmod.assert_called_once_with(mode=0o640 if os_user is not None else 0o600) + mock_touch_file.assert_called_once_with( + file_path=file_path, + agent_user_permission=FileSystemPermissionEnum.READ_WRITE, + ) def test_changes_group_ownership( self, file_path: MagicMock, os_user: Optional[SessionUser], - mock_run_cmd_as: MagicMock, mock_touch_file: MagicMock, + mock_chown: MagicMock, ) -> None: """Tests the config/credentials file is created if necessary""" # GIVEN @@ -255,22 +160,24 @@ def test_changes_group_ownership( if os_user: if os.name == "posix": assert isinstance(os_user, PosixSessionUser) - mock_run_cmd_as.assert_any_call(user=os_user, cmd=["touch", str(file_path)]) - mock_run_cmd_as.assert_any_call( - user=os_user, - cmd=["chown", f"{os_user.user}:{os_user.group}", str(file_path)], - ) - mock_run_cmd_as.assert_any_call(user=os_user, cmd=["chmod", "660", str(file_path)]) + file_path.touch.assert_called_once_with(mode=0o640) + mock_chown.assert_called_once_with(file_path, group=os_user.group) else: assert isinstance(os_user, WindowsSessionUser) mock_touch_file.assert_called_once_with( file_path=file_path, permitted_user=os_user, - user_permission=FileSystemPermissionEnum.READ_WRITE, + user_permission=FileSystemPermissionEnum.READ, agent_user_permission=FileSystemPermissionEnum.FULL_CONTROL, ) + elif os.name == "posix": + file_path.touch.assert_called_once_with(mode=0o600) + mock_chown.assert_not_called() else: - mock_run_cmd_as.assert_not_called() + mock_touch_file.assert_called_once_with( + file_path=file_path, + agent_user_permission=FileSystemPermissionEnum.READ_WRITE, + ) class AWSConfigTestBase: @@ -318,11 +225,16 @@ def side_effect() -> bool: # mock_exists.return_value = exists return mock_exists + @pytest.fixture + def parent_dir(self) -> MagicMock: + return MagicMock() + def test_init( self, config_class: Type[_AWSConfigBase], os_user: Optional[SessionUser], mock_config_parser: MagicMock, + parent_dir: MagicMock, ) -> None: # GIVEN if os.name == "posix": @@ -331,46 +243,41 @@ def test_init( assert isinstance(os_user, WindowsSessionUser) or os_user is None config_parser_read: MagicMock = mock_config_parser.read - with ( - patch.object(config_class, "_get_path") as get_path_mock, - patch.object(aws_configs_mod, "_setup_parent_dir") as setup_parent_dir_mock, - patch.object(aws_configs_mod, "_setup_file") as setup_file_mock, - ): - config_path: MagicMock = get_path_mock.return_value - + with patch.object(aws_configs_mod, "_setup_file") as setup_file_mock: # WHEN - config = config_class(os_user=os_user) + config = config_class( + os_user=os_user, + parent_dir=parent_dir, + ) # THEN - expected_user = os_user.user if os_user is not None else "" - get_path_mock.assert_called_once_with(os_user=expected_user) - assert config._config_path == config_path - setup_parent_dir_mock.assert_called_once_with( - dir_path=get_path_mock.return_value.parent, - owner=os_user, - ) setup_file_mock.assert_called_once_with( - file_path=get_path_mock.return_value, + file_path=config.path, owner=os_user, ) - config_parser_read.assert_called_once_with(config._config_path) + config_parser_read.assert_called_once_with(config.path) - def test_get_path( + def test_path( self, config_class: Type[_AWSConfigBase], - expected_path: str, + expected_path: Path, os_user: Optional[SessionUser], + parent_dir: MagicMock, ) -> None: # WHEN if os.name == "posix": assert isinstance(os_user, PosixSessionUser) or os_user is None else: assert isinstance(os_user, WindowsSessionUser) or os_user is None - result = config_class._get_path(os_user=os_user.user if os_user is not None else "") + + config = config_class( + os_user=os_user, + parent_dir=parent_dir, + ) + result = config.path # THEN - expected_path = expected_path.format(user=os_user.user if os_user is not None else "") - assert result == Path(expected_path).expanduser() + assert result == expected_path @patch.object(aws_configs_mod.Path, "absolute") def test_install_credential_process( @@ -381,13 +288,14 @@ def test_install_credential_process( expected_profile_name_section: str, os_user: Optional[SessionUser], mock_config_parser: MagicMock, + parent_dir: MagicMock, ) -> None: # GIVEN if os.name == "posix": assert isinstance(os_user, PosixSessionUser) or os_user is None else: assert isinstance(os_user, WindowsSessionUser) or os_user is None - config = config_class(os_user=os_user) + config = config_class(os_user=os_user, parent_dir=parent_dir) script_path = Path("/path/to/installdir/echo_them_credentials.sh") with patch.object(config, "_write") as write_mock: # WHEN @@ -411,13 +319,17 @@ def test_uninstall_credential_process( expected_profile_name_section: str, os_user: Optional[SessionUser], mock_config_parser: MagicMock, + parent_dir: MagicMock, ) -> None: # GIVEN if os.name == "posix": assert isinstance(os_user, PosixSessionUser) or os_user is None else: assert isinstance(os_user, WindowsSessionUser) or os_user is None - config = config_class(os_user=os_user) + config = config_class( + os_user=os_user, + parent_dir=parent_dir, + ) script_path = Path("/path/to/installdir/echo_them_credentials.sh") with patch.object(config, "_write") as write_mock: config.install_credential_process(profile_name=profile_name, script_path=script_path) @@ -443,6 +355,7 @@ def test_write( config_class: Type[_AWSConfigBase], os_user: Optional[SessionUser], mock_config_parser: MagicMock, + parent_dir: MagicMock, ) -> None: # GIVEN if os.name == "posix": @@ -451,17 +364,20 @@ def test_write( assert isinstance(os_user, WindowsSessionUser) or os_user is None with ( patch.object(aws_configs_mod, "_logger") as logger_mock, - patch.object(config_class, "_get_path") as get_path_mock, + patch.object(config_class, "path", new_callable=PropertyMock) as path_prop_mock, ): - path: MagicMock = get_path_mock.return_value - config = config_class(os_user) + path: MagicMock = path_prop_mock.return_value + config = config_class( + os_user=os_user, + parent_dir=parent_dir, + ) info_mock: MagicMock = logger_mock.info # WHEN config._write() # THEN - info_mock.assert_called_once_with(f"Writing updated {config._config_path} to disk.") + info_mock.assert_called_once_with(f"Writing updated {path} to disk.") path.open.assert_called_once_with(mode="w") mock_config_parser.write.assert_called_once_with( fp=path.open.return_value.__enter__.return_value, @@ -485,12 +401,8 @@ def expected_profile_name_section(self, profile_name: str) -> str: return f"profile {profile_name}" @pytest.fixture - def expected_path(self, os_user: Optional[SessionUser]) -> str: - if os.name == "posix": - assert isinstance(os_user, PosixSessionUser) or os_user is None - else: - assert isinstance(os_user, WindowsSessionUser) or os_user is None - return f"~{os_user.user if os_user is not None else ''}/.aws/config" + def expected_path(self, parent_dir: MagicMock) -> str: + return parent_dir / "config" class TestAWSCredentials(AWSConfigTestBase): @@ -509,9 +421,5 @@ def expected_profile_name_section(self, profile_name: str) -> str: return f"{profile_name}" @pytest.fixture - def expected_path(self, os_user: Optional[SessionUser]) -> str: - if os.name == "posix": - assert isinstance(os_user, PosixSessionUser) or os_user is None - else: - assert isinstance(os_user, WindowsSessionUser) or os_user is None - return f"~{os_user.user if os_user is not None else ''}/.aws/credentials" + def expected_path(self, parent_dir: MagicMock) -> str: + return parent_dir / "credentials" diff --git a/test/unit/aws_credentials/test_queue_boto3_session.py b/test/unit/aws_credentials/test_queue_boto3_session.py index a123e021..890a2ae6 100644 --- a/test/unit/aws_credentials/test_queue_boto3_session.py +++ b/test/unit/aws_credentials/test_queue_boto3_session.py @@ -1,15 +1,20 @@ # Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved. -import os -import stat -from typing import Optional, Generator -from unittest.mock import ANY, MagicMock, patch +# Built-in from datetime import datetime, timezone, timedelta -from threading import Event from pathlib import Path +from threading import Event +from typing import Optional, Generator +from unittest.mock import ANY, MagicMock, patch +import os +import stat import tempfile + +# Third-party +from openjd.sessions import PosixSessionUser, WindowsSessionUser, SessionUser import pytest +# First-party from deadline_worker_agent.aws.deadline import ( DeadlineRequestInterrupted, DeadlineRequestWorkerOfflineError, @@ -17,10 +22,9 @@ DeadlineRequestUnrecoverableError, DeadlineRequestError, ) -import deadline_worker_agent.aws_credentials.queue_boto3_session as queue_boto3_session_mod from deadline_worker_agent.aws_credentials.queue_boto3_session import QueueBoto3Session -from openjd.sessions import PosixSessionUser, WindowsSessionUser, SessionUser from deadline_worker_agent.file_system_operations import FileSystemPermissionEnum +import deadline_worker_agent.aws_credentials.queue_boto3_session as queue_boto3_session_mod @pytest.fixture(autouse=True) @@ -255,6 +259,7 @@ def test_uses_bootstrap_credentials( queue_id: str, file_cache_cls_mock: MagicMock, temporary_credentials_cls_mock: MagicMock, + os_user: SessionUser, ) -> None: # Test that if the Session contains credentials that ARE expired, # then it will use the given bootstrap_session credentials to do the refresh. @@ -273,7 +278,7 @@ def test_uses_bootstrap_credentials( fleet_id=fleet_id, worker_id=worker_id, queue_id=queue_id, - os_user=None, + os_user=os_user, interrupt_event=event, worker_persistence_dir=Path("/var/lib/deadline"), ) @@ -283,6 +288,9 @@ def test_uses_bootstrap_credentials( queue_boto3_session_mod, "assume_queue_role_for_worker" ) as assume_role_mock, patch.object(QueueBoto3Session, "get_credentials") as mock_get_credentials, + patch.object(queue_boto3_session_mod.shutil, "chown") as mock_chown, + patch.object(queue_boto3_session_mod, "set_permissions") as mock_set_permissions, + patch.object(QueueBoto3Session, "_credentials_file_path") as mock_credentials_file_path, ): assume_role_mock.return_value = SAMPLE_ASSUME_ROLE_RESPONSE mock_temporary_creds = MagicMock() @@ -310,11 +318,35 @@ def test_uses_bootstrap_credentials( api_name="AssumeQueueRoleForWorker", ) mock_temporary_creds.cache.assert_called_once_with( - cache=file_cache_cls_mock.return_value, cache_key=session._credentials_filename + cache=file_cache_cls_mock.return_value, + cache_key=session._credentials_filename_no_ext, ) mock_credentials_object.set_credentials.assert_called_once_with( mock_temporary_creds.to_deadline.return_value ) + if os_user: + if os.name == "posix": + assert isinstance(os_user, PosixSessionUser) + mock_credentials_file_path.return_value.chmod(0o640) + mock_chown.assert_called_once_with( + mock_credentials_file_path.return_value, group=os_user.group + ) + else: + mock_set_permissions.assert_called_once_with( + file_path=mock_credentials_file_path.return_value, + permitted_user=os_user, + agent_user_permission=FileSystemPermissionEnum.READ_WRITE, + user_permission=FileSystemPermissionEnum.READ, + ) + else: + if os.name == "posix": + mock_credentials_file_path.return_value.chmod(0o600) + mock_chown.assert_not_called() + else: + mock_set_permissions.assert_called_once_with( + file_path=mock_credentials_file_path.return_value, + agent_user_permission=FileSystemPermissionEnum.READ_WRITE, + ) @pytest.mark.parametrize( "exception", @@ -444,16 +476,20 @@ def test_success( # GIVEN event = Event() + expected_mode = 0o750 if os_user else 0o700 with ( # To get through __init__ - patch.object(QueueBoto3Session, "_create_credentials_directory"), + patch.object(QueueBoto3Session, "_credentials_file_path"), + patch.object(QueueBoto3Session, "_get_credentials_dir") as mock_get_credentials_dir, patch.object(QueueBoto3Session, "_install_credential_process"), patch.object(QueueBoto3Session, "refresh_credentials"), + patch.object(queue_boto3_session_mod, "make_directory") as mock_make_directory, + patch.object(queue_boto3_session_mod.shutil, "chown") as mock_chown, ): - mock_path = MagicMock() - mock_path.__truediv__.return_value = mock_path + worker_persistence_dir = MagicMock() - session = QueueBoto3Session( + # WHEN + QueueBoto3Session( deadline_client=deadline_client, farm_id=farm_id, fleet_id=fleet_id, @@ -461,34 +497,48 @@ def test_success( queue_id=queue_id, os_user=os_user, interrupt_event=event, - worker_persistence_dir=mock_path, + worker_persistence_dir=worker_persistence_dir, ) - with ( - patch.object(queue_boto3_session_mod, "make_directory") as mock_make_directory, - patch.object(queue_boto3_session_mod.shutil, "chown") as mock_chown, - ): - # WHEN - session._create_credentials_directory() - # THEN if isinstance(os_user, PosixSessionUser): - mock_path.mkdir.assert_called_once_with( + assert os.name == "posix" + mock_get_credentials_dir.return_value.mkdir.assert_called_once_with( exist_ok=True, parents=True, - mode=0o750, + mode=expected_mode, ) - mock_chown.assert_called_once_with(mock_path, group=os_user.group) - else: + mock_get_credentials_dir.return_value.chmod.assert_called_once_with(expected_mode) + mock_chown.assert_called_once_with( + mock_get_credentials_dir.return_value, group=os_user.group + ) + mock_get_credentials_dir.return_value.chmod.assert_called_once_with(expected_mode) + elif isinstance(os_user, WindowsSessionUser): + assert os.name != "posix" mock_make_directory.assert_called_once_with( - dir_path=mock_path, + dir_path=mock_get_credentials_dir.return_value, exist_ok=True, parents=True, permitted_user=os_user, agent_user_permission=FileSystemPermissionEnum.FULL_CONTROL, user_permission=FileSystemPermissionEnum.READ, ) + elif os.name == "posix": + mock_get_credentials_dir.return_value.mkdir.assert_called_once_with( + exist_ok=True, + parents=True, + mode=expected_mode, + ) + mock_get_credentials_dir.return_value.chmod.assert_called_once_with(expected_mode) + mock_chown.assert_not_called() + else: + mock_make_directory.assert_called_once_with( + dir_path=worker_persistence_dir, + exist_ok=True, + parents=True, + agent_user_permission=FileSystemPermissionEnum.FULL_CONTROL, + ) def test_reraises_oserror( self, @@ -645,9 +695,8 @@ def test_success( if os_user is None else (stat.S_IRWXU | stat.S_IRGRP | stat.S_IXGRP), ) - mock_builtins_open.assert_called_once_with( - mock_os_open.return_value, mode="w", encoding="utf-8" - ) + descriptor = mock_os_open.return_value + mock_builtins_open.assert_called_once_with(descriptor, mode="w", encoding="utf-8") mock_builtins_open.return_value.__enter__.assert_called_once() mock_builtins_open.return_value.__exit__.assert_called_once() mock_builtins_open.return_value.__enter__.return_value.write.assert_called_once_with( @@ -659,9 +708,7 @@ def test_success( # This assert for type checking. Expand the if-else chain when adding new user kinds. if os.name == "posix": assert isinstance(os_user, PosixSessionUser) - mock_chown.assert_called_once_with( - credentials_process_script_path, group=os_user.group - ) + mock_chown.assert_called_once_with(descriptor, group=os_user.group) else: assert isinstance(os_user, WindowsSessionUser) mock_set_permissions.assert_called_once_with(