fix: take the named pipe user SID from the process token

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.

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(),
+            ),
+        ]