diff --git a/src/openjd/adaptor_runtime_client/named_pipe/named_pipe_helper.py b/src/openjd/adaptor_runtime_client/named_pipe/named_pipe_helper.py index 82b664c1..9416d15c 100644 --- a/src/openjd/adaptor_runtime_client/named_pipe/named_pipe_helper.py +++ b/src/openjd/adaptor_runtime_client/named_pipe/named_pipe_helper.py @@ -4,11 +4,11 @@ from concurrent.futures import ThreadPoolExecutor, TimeoutError from random import randint +import win32api import win32file import pywintypes import win32security import win32con -import getpass import winerror import time import win32pipe @@ -116,7 +116,7 @@ class NamedPipeHelper: """ @staticmethod - def create_security_attributes(): + def create_security_attributes() -> win32security.SECURITY_ATTRIBUTES: """ Creates and returns security attributes for a named pipe, allowing access only to the current user and denying network access. @@ -125,26 +125,14 @@ def create_security_attributes(): win32security.SECURITY_ATTRIBUTES: A SECURITY_ATTRIBUTES object configured with the custom security descriptor. """ - # Get the username of the current user - username = getpass.getuser() - - # Retry with exponential backoff to handle ERROR_NONE_MAPPED (1332) which occurs - # when the LSA service hasn't finished initializing on freshly started EC2 instances. - _LOOKUP_MAX_RETRIES = 3 - for attempt in range(_LOOKUP_MAX_RETRIES): - try: - user_sid, _, _ = win32security.LookupAccountName( - "", # systemName: The name of the system or server where the account resides. - # Search for the account on the local computer. - # If Domain/User Format is used here, it will fetch the Name from the AD. - username, - ) - break - except pywintypes.error as e: - if e.winerror == 1332 and attempt < _LOOKUP_MAX_RETRIES - 1: - time.sleep(2**attempt) - continue - raise + # Take the SID from the process token, not from a user name. A name lookup goes through + # LSA and can fail with ERROR_NONE_MAPPED (1332), and getpass.getuser() only reads + # environment variables, which need not name the process's account. + token = win32security.OpenProcessToken(win32api.GetCurrentProcess(), win32con.TOKEN_QUERY) + try: + user_sid = win32security.GetTokenInformation(token, win32security.TokenUser)[0] + finally: + win32api.CloseHandle(token) # Users who log on across a network. "S-1-5-2" is a group identifier added to the token of a process # when it was logged on across a network. @@ -320,6 +308,13 @@ def establish_named_pipe_connection(pipe_name: str, timeout_in_seconds: float) - or due to other errors. """ + # CreateFile ignores the security descriptor when opening an existing pipe and reads only + # bInheritHandle, so build no DACL here. Inheritable matches pywin32's SECURITY_ATTRIBUTES + # default, which the server pipe handle also uses. + security_attributes = win32security.SECURITY_ATTRIBUTES() + security_attributes.SECURITY_DESCRIPTOR = None + security_attributes.bInheritHandle = True + start_time = time.time() # Wait for the server pipe to become available. handle = None @@ -330,7 +325,7 @@ def establish_named_pipe_connection(pipe_name: str, timeout_in_seconds: float) - # Give the read / write permission win32file.GENERIC_READ | win32file.GENERIC_WRITE, 0, # Disable the sharing Mode - NamedPipeHelper.create_security_attributes(), + security_attributes, win32file.OPEN_EXISTING, # Open existing pipe 0, # No Additional flags None, # A valid handle to a template file, This parameter is ignored when opening an existing pipe. diff --git a/test/openjd/adaptor_runtime/integ/_named_pipe/test_named_pipe_helper.py b/test/openjd/adaptor_runtime/integ/_named_pipe/test_named_pipe_helper.py index 84dcba9f..7a9a04cf 100644 --- a/test/openjd/adaptor_runtime/integ/_named_pipe/test_named_pipe_helper.py +++ b/test/openjd/adaptor_runtime/integ/_named_pipe/test_named_pipe_helper.py @@ -2,17 +2,23 @@ import time from openjd.adaptor_runtime._osname import OSName +import getpass import json import os import pytest import threading +import uuid +from typing import Any, List, Optional, Tuple if OSName.is_windows(): + import ntsecuritycon import pywintypes + import win32con import win32file import win32pipe import win32security import win32api + import winerror from openjd.adaptor_runtime_client.named_pipe.named_pipe_helper import NamedPipeHelper else: # Cannot put this on the top of this file or mypy will complain @@ -21,6 +27,12 @@ PIPE_NAME = r"\\.\pipe\TestPipe" TIMEOUT_SECONDS = 5 +# getpass.getuser() returns the first of these that is set. No account has this name, so +# resolving it with LookupAccountName fails with ERROR_NONE_MAPPED (1332). +USER_NAME_ENV_VARS = ("LOGNAME", "USER", "LNAME", "USERNAME") +UNMAPPED_USER_NAME = "no-such-user-1332" +NETWORK_SID = "S-1-5-2" + def pipe_server(pipe_name, message_to_send, return_message): """ @@ -52,6 +64,64 @@ def start_pipe_server(): server_thread.join() +def set_unmapped_user_name(monkeypatch: pytest.MonkeyPatch) -> None: + """Makes getpass.getuser() return a name that LookupAccountName cannot resolve.""" + for name in USER_NAME_ENV_VARS: + monkeypatch.setenv(name, UNMAPPED_USER_NAME) + assert getpass.getuser() == UNMAPPED_USER_NAME + # Precondition: if this name resolved, a name-based SID lookup would pass too and the + # tests using it would pin nothing. + with pytest.raises(pywintypes.error) as excinfo: + win32security.LookupAccountName("", UNMAPPED_USER_NAME) + assert excinfo.value.winerror == winerror.ERROR_NONE_MAPPED + + +def process_token_user_sid() -> str: + token = win32security.OpenProcessToken(win32api.GetCurrentProcess(), win32con.TOKEN_QUERY) + try: + sid = win32security.GetTokenInformation(token, win32security.TokenUser)[0] + finally: + win32api.CloseHandle(token) + return win32security.ConvertSidToStringSid(sid) + + +def as_file_rights(mask: int) -> int: + """Maps generic rights to file rights, since Windows may store an ACE mask in either form.""" + mask &= 0xFFFFFFFF + for generic, specific in ( + (win32con.GENERIC_READ & 0xFFFFFFFF, ntsecuritycon.FILE_GENERIC_READ), + (win32con.GENERIC_WRITE, ntsecuritycon.FILE_GENERIC_WRITE), + (win32con.GENERIC_ALL, ntsecuritycon.FILE_ALL_ACCESS), + ): + if mask & generic: + mask = (mask & ~generic) | specific + return mask + + +def read_pipe_dacl(pipe_handle: Any) -> List[Tuple[int, int, str]]: + """Returns (ACE type, access mask as file rights, SID string) for each ACE on the pipe.""" + security_descriptor = win32security.GetSecurityInfo( + pipe_handle, win32security.SE_KERNEL_OBJECT, win32security.DACL_SECURITY_INFORMATION + ) + dacl = security_descriptor.GetSecurityDescriptorDacl() + entries = [] + for index in range(dacl.GetAceCount()): + (ace_type, _ace_flags), mask, sid = dacl.GetAce(index) + entries.append((ace_type, as_file_rights(mask), win32security.ConvertSidToStringSid(sid))) + return entries + + +def serve_one_request( + server_handle: Any, response: str, received: List[str], errors: List[Exception] +) -> None: + try: + win32pipe.ConnectNamedPipe(server_handle, None) + received.append(NamedPipeHelper.read_from_pipe(server_handle)) + NamedPipeHelper.write_to_pipe(server_handle, response) + except Exception as e: + errors.append(e) + + @pytest.mark.skipif(not OSName.is_windows(), reason="NamedPipe is only implemented in Windows.") class TestNamedPipeHelper: def test_named_pipe_communication(self, start_pipe_server): @@ -129,3 +199,71 @@ def test_fail_to_connect_to_named_pipe_with_different_user( # Send a message to unblock the I/O NamedPipeHelper.send_named_pipe_request(PIPE_NAME, TIMEOUT_SECONDS, **message_to_send) + + @pytest.mark.parametrize( + "unmapped_user_name_for", + [ + pytest.param(None, id="ambient-user-name"), + pytest.param("server-and-client", id="unmapped-user-name-for-server-and-client"), + pytest.param("client", id="unmapped-user-name-for-client"), + ], + ) + def test_pipe_security_does_not_depend_on_user_name( + self, monkeypatch: pytest.MonkeyPatch, unmapped_user_name_for: Optional[str] + ) -> None: + """ + The pipe's DACL comes from the process token, so a getpass.getuser() name that maps to + no account (ERROR_NONE_MAPPED, 1332) breaks neither the server nor the client. + The ambient-user-name case is the control: the normal environment must still work. + """ + # GIVEN + pipe_name = rf"\\.\pipe\openjd_token_sid_{os.getpid()}_{uuid.uuid4().hex}" + request_body = {"message": "Hello from client"} + server_response = '{"Response": "Hello from server"}' + received: List[str] = [] + errors: List[Exception] = [] + if unmapped_user_name_for == "server-and-client": + set_unmapped_user_name(monkeypatch) + server_handle = NamedPipeHelper.create_named_pipe_server(pipe_name, TIMEOUT_SECONDS) + assert server_handle is not None + # A daemon thread, so a failed client cannot hang the run in ConnectNamedPipe. + server_thread = threading.Thread( + target=serve_one_request, + args=(server_handle, server_response, received, errors), + daemon=True, + ) + + try: + if unmapped_user_name_for == "client": + set_unmapped_user_name(monkeypatch) + dacl = read_pipe_dacl(server_handle) + server_thread.start() + + # WHEN + response = NamedPipeHelper.send_named_pipe_request( + pipe_name, TIMEOUT_SECONDS, "POST", "/token-sid", json_body=request_body + ) + server_thread.join(TIMEOUT_SECONDS) + finally: + win32file.CloseHandle(server_handle) + + # THEN + assert not server_thread.is_alive() + assert errors == [] + assert [json.loads(message) for message in received] == [ + {"method": "POST", "path": "/token-sid", "body": json.dumps(request_body)} + ] + assert response == json.loads(server_response) + # Canonical order: the NETWORK deny comes first, so it applies to the user's own network logons. + assert dacl == [ + ( + win32security.ACCESS_DENIED_ACE_TYPE, + as_file_rights(win32con.GENERIC_ALL), + NETWORK_SID, + ), + ( + win32security.ACCESS_ALLOWED_ACE_TYPE, + as_file_rights(win32con.GENERIC_READ | win32con.GENERIC_WRITE), + process_token_user_sid(), + ), + ] diff --git a/test/openjd/adaptor_runtime/unit/named_pipe/test_named_pipe_helper.py b/test/openjd/adaptor_runtime/unit/named_pipe/test_named_pipe_helper.py index 0c8237d1..a5c86f70 100644 --- a/test/openjd/adaptor_runtime/unit/named_pipe/test_named_pipe_helper.py +++ b/test/openjd/adaptor_runtime/unit/named_pipe/test_named_pipe_helper.py @@ -1,19 +1,35 @@ # Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved. from openjd.adaptor_runtime._osname import OSName +from typing import Any, List, Tuple from unittest.mock import patch, MagicMock import pytest import os import time pywintypes = pytest.importorskip("pywintypes") +win32api = pytest.importorskip("win32api") +win32con = pytest.importorskip("win32con") win32pipe = pytest.importorskip("win32pipe") win32file = pytest.importorskip("win32file") +win32security = pytest.importorskip("win32security") winerror = pytest.importorskip("winerror") named_pipe_helper = pytest.importorskip( "openjd.adaptor_runtime_client.named_pipe.named_pipe_helper" ) +FAKE_USER_SID = "S-1-5-21-1-2-3-1001" + + +def dacl_entries(security_attributes: Any) -> List[Tuple[int, int, str]]: + """Returns (ACE type, access mask, SID string) for each ACE in the attributes' DACL.""" + dacl = security_attributes.SECURITY_DESCRIPTOR.GetSecurityDescriptorDacl() + entries = [] + for index in range(dacl.GetAceCount()): + (ace_type, _ace_flags), mask, sid = dacl.GetAce(index) + entries.append((ace_type, mask & 0xFFFFFFFF, win32security.ConvertSidToStringSid(sid))) + return entries + class MockReadFile: @staticmethod @@ -101,42 +117,102 @@ def test_failed_to_generate_pipe_name(self, mock_check_named_pipe_exists, mock_g ): named_pipe_helper.NamedPipeHelper.generate_pipe_name("AdaptorTest") - @patch("getpass.getuser", return_value="regularuser") - def test_create_security_attributes_uses_lookup(self, mock_getuser): - win32security = pytest.importorskip("win32security") - fake_sid = win32security.ConvertStringSidToSid("S-1-1-0") - with patch.object( - win32security, "LookupAccountName", return_value=(fake_sid, None, None) - ) as mock_lookup: - named_pipe_helper.NamedPipeHelper.create_security_attributes() - mock_lookup.assert_called_once_with("", "regularuser") - - @patch("time.sleep") - @patch("getpass.getuser", return_value="regularuser") - def test_create_security_attributes_retries_on_lsa_not_ready(self, mock_getuser, mock_sleep): - # ERROR_NONE_MAPPED (1332) is raised when LSA hasn't finished initializing on a - # fresh EC2 instance. The lookup should retry with exponential backoff and succeed. - win32security = pytest.importorskip("win32security") - fake_sid = win32security.ConvertStringSidToSid("S-1-1-0") - lsa_error = pywintypes.error( - 1332, "LookupAccountName", "No mapping between account names and security IDs was done." + def test_create_security_attributes_uses_process_token_user(self) -> None: + # GIVEN + token = MagicMock() + token_user = (win32security.ConvertStringSidToSid(FAKE_USER_SID), 0) + with ( + patch.object(win32security, "OpenProcessToken", return_value=token) as mock_open_token, + patch.object( + win32security, "GetTokenInformation", return_value=token_user + ) as mock_get_token_information, + patch.object(win32api, "CloseHandle") as mock_close_handle, + ): + # WHEN + security_attributes = named_pipe_helper.NamedPipeHelper.create_security_attributes() + + # THEN + mock_open_token.assert_called_once_with(win32api.GetCurrentProcess(), win32con.TOKEN_QUERY) + mock_get_token_information.assert_called_once_with(token, win32security.TokenUser) + mock_close_handle.assert_called_once_with(token) + # Canonical order: the NETWORK deny comes first, so it applies to the user's own network logons. + assert dacl_entries(security_attributes) == [ + (win32security.ACCESS_DENIED_ACE_TYPE, win32con.GENERIC_ALL, "S-1-5-2"), + ( + win32security.ACCESS_ALLOWED_ACE_TYPE, + (win32con.GENERIC_READ | win32con.GENERIC_WRITE) & 0xFFFFFFFF, + FAKE_USER_SID, + ), + ] + + def test_create_security_attributes_does_not_look_up_user_name( + self, monkeypatch: pytest.MonkeyPatch + ) -> None: + # GIVEN + for name in ("LOGNAME", "USER", "LNAME", "USERNAME"): + monkeypatch.setenv(name, "no-such-user-1332") + none_mapped = pywintypes.error( + winerror.ERROR_NONE_MAPPED, + "LookupAccountName", + "No mapping between account names and security IDs was done.", ) + token = win32security.OpenProcessToken(win32api.GetCurrentProcess(), win32con.TOKEN_QUERY) + try: + expected_sid = win32security.GetTokenInformation(token, win32security.TokenUser)[0] + finally: + win32api.CloseHandle(token) + with patch.object( - win32security, "LookupAccountName", side_effect=[lsa_error, (fake_sid, None, None)] - ) as mock_lookup: - named_pipe_helper.NamedPipeHelper.create_security_attributes() - assert mock_lookup.call_count == 2 - mock_sleep.assert_called_once_with(1) # 2**0 = 1s after first failure - - @patch("time.sleep") - @patch("getpass.getuser", return_value="regularuser") - def test_create_security_attributes_raises_after_max_retries(self, mock_getuser, mock_sleep): - # After exhausting all retries, the original error should be re-raised. - win32security = pytest.importorskip("win32security") - lsa_error = pywintypes.error( - 1332, "LookupAccountName", "No mapping between account names and security IDs was done." + win32security, "LookupAccountName", side_effect=none_mapped + ) as mock_lookup_account_name: + # WHEN + security_attributes = named_pipe_helper.NamedPipeHelper.create_security_attributes() + + # THEN + mock_lookup_account_name.assert_not_called() + allowed_sids = [ + sid + for ace_type, _mask, sid in dacl_entries(security_attributes) + if ace_type == win32security.ACCESS_ALLOWED_ACE_TYPE + ] + assert allowed_sids == [win32security.ConvertSidToStringSid(expected_sid)] + + def test_create_security_attributes_closes_token_when_query_fails(self) -> None: + # GIVEN + token = MagicMock() + query_error = pywintypes.error( + winerror.ERROR_ACCESS_DENIED, "GetTokenInformation", "Access is denied." ) - with patch.object(win32security, "LookupAccountName", side_effect=lsa_error): + with ( + patch.object(win32security, "OpenProcessToken", return_value=token), + patch.object(win32security, "GetTokenInformation", side_effect=query_error), + patch.object(win32api, "CloseHandle") as mock_close_handle, + ): + # WHEN with pytest.raises(pywintypes.error) as exc_info: named_pipe_helper.NamedPipeHelper.create_security_attributes() - assert exc_info.value.winerror == 1332 + + # THEN + assert exc_info.value is query_error + mock_close_handle.assert_called_once_with(token) + + @patch.object(win32pipe, "SetNamedPipeHandleState") + @patch.object(win32file, "CreateFile") + @patch.object(named_pipe_helper.NamedPipeHelper, "create_security_attributes") + def test_establish_named_pipe_connection_builds_no_dacl( + self, + mock_create_security_attributes: MagicMock, + mock_create_file: MagicMock, + mock_set_named_pipe_handle_state: MagicMock, + ) -> None: + # WHEN + handle = named_pipe_helper.NamedPipeHelper.establish_named_pipe_connection("fakepipe", 1.0) + + # THEN + assert handle is mock_create_file.return_value + mock_create_security_attributes.assert_not_called() + # Opening an existing pipe ignores the security descriptor; inheritability is unchanged. + security_attributes = mock_create_file.call_args.args[3] + assert security_attributes.SECURITY_DESCRIPTOR is None + assert security_attributes.bInheritHandle == 1 + assert mock_create_file.call_args.args[4] == win32file.OPEN_EXISTING