Repository navigation
fix: take the named pipe user SID from the process token - #286
Merged
leongdl merged 2 commits intoNov 4, 2026
Merged
Conversation
NamedPipeHelper.create_security_attributes() resolved the DACL's user SID
with LookupAccountName("", getpass.getuser()). getpass.getuser() only reads
the LOGNAME, USER, LNAME and USERNAME environment variables, and the name
lookup goes through LSA, which can fail with ERROR_NONE_MAPPED (1332). A
Windows CI host running as SYSTEM hit 1332 on both the server path
(create_named_pipe_server) and the client path
(establish_named_pipe_connection), and the 3-attempt retry from OpenJobDescription#262 ran
out.
- Read the SID from the process token: OpenProcessToken(GetCurrentProcess(),
TOKEN_QUERY) and GetTokenInformation(token, TokenUser)[0], closing the
token in a finally. No name lookup, so 1332 cannot occur. The OpenJobDescription#262 retry
and the getpass import are removed.
- The client no longer builds a DACL. CreateFile ignores the security
descriptor when opening an existing pipe and reads only bInheritHandle. It
passes SECURITY_ATTRIBUTES with SECURITY_DESCRIPTOR = None and
bInheritHandle = True. pywin32's SECURITY_ATTRIBUTES() defaults
bInheritHandle to TRUE (PySECURITY_ATTRIBUTES.cpp), so the client handle
stays inheritable, as before. Passing None would have made it
non-inheritable.
- The server DACL is unchanged: GENERIC_READ|GENERIC_WRITE allowed to the
user SID, then GENERIC_ALL denied to S-1-5-2 (NETWORK).
Tests:
- Unit: the token SID ends up in the DACL; with the env user name set to
an unmapped name, LookupAccountName is not called; the token is closed
when GetTokenInformation raises; the client passes no descriptor, an
inheritable handle and OPEN_EXISTING, and does not call
create_security_attributes. The three LookupAccountName/retry tests
pinned the removed behaviour and are replaced.
- Integ: test_pipe_security_does_not_depend_on_user_name runs a real pipe
round-trip with LOGNAME, USER, LNAME and USERNAME set to
no-such-user-1332, once for server and client and once for the client
only, plus an ambient-environment control. It asserts the precondition
that LookupAccountName("", "no-such-user-1332") raises 1332, and reads the
DACL back with GetSecurityInfo. The old code fails this test
deterministically.
- CI: code_quality.yml runs windows-latest x Python 3.9-3.14 through
OpenJobDescription/.github reusable_python_build.yml, whose last step
(line 72) is `hatch run test`, which is `pytest ... test -vv`. That
collects test/openjd/adaptor_runtime/integ/_named_pipe/. No marker gates
the new test.
Ran on macOS (Python 3.13, hatch 1.14.1):
- hatch run lint, before and after: ruff "All checks passed!", black
"146 files would be left unchanged", mypy "no issues found in 146
source files".
- hatch run mypy --platform win32 src test, before and after: no issues
in 146 source files.
- hatch run test: before (mainline d9044ea) 482 passed, 26 skipped,
coverage 93.39%. After: 482 passed, 29 skipped, coverage 93.39%. The 3
new skips are the 3 new integ cases. The Windows-only unit module is
one module-level skip before and after.
- Windows tests skip on macOS. To run them anyway, a fake pywin32 plugin
(not committed) models the APIs these tests touch, using the pywin32 C
sources. Fidelity control: mainline code with mainline tests passes on
it (11 passed). Branch: 15 passed, 1 skipped (the GITHUB_ACTIONS-only
different-user test).
Mutation check, EXECUTED on the fake (sha256 restore, __pycache__
cleared between mutants), all caught:
- M1 restore LookupAccountName(getpass.getuser()) plus retry: fails
test_create_security_attributes_uses_process_token_user,
..._does_not_look_up_user_name, ..._closes_token_when_query_fails and
integ [unmapped-user-name-for-server-and-client].
- M2 restore the client DACL: fails
test_establish_named_pipe_connection_builds_no_dacl.
- M3 original code (M1+M2): also fails integ
[unmapped-user-name-for-client].
- M4 no finally, M5 no close, M6 client not inheritable, M7 client None,
M8 client keeps blank descriptor, M9 allow Everyone, M10 drop network
deny: each caught. M4-M8 by the token-close or client unit test, M9
and M10 also by all three integ cases.
ARGUED for real Windows, not run: with the old code, LookupAccountName
raises 1332 on every attempt (the test asserts this as a precondition),
so both unmapped integ cases fail. M2 alone is equivalent at the OS level,
because CreateFile ignores the descriptor. Only the unit test catches it.
Auditors (three, independent; each returned PASS):
- Pin/mutation: 27 of their own mutants, 26 killed. The survivor removes
the bInheritHandle = True line, which is equivalent because pywin32
defaults it to TRUE. Kept the explicit line for the reader. The
CloseHandle-call assertion rejects an equivalent token.Close(). Kept,
since it matches the repo's mock style.
- Gap analysis: 9 mutants, all killed. Pre-existing:
test_fail_to_connect_to_named_pipe_with_different_user has its assert
inside pytest.raises, so the assert never runs. Not in this diff, and
tightening it to winerror 5 would gate CI on a claim nobody has run.
Recorded as a follow-up. _secure_open.py has the same LookupAccountName
pattern; follow-up.
- Craft/cross-platform: lint, win32 mypy and collection green; pywin32
API shapes checked against source. Fixed: the client-only case now sets
the unmapped name inside the try, so a failed precondition still closes
the server handle. Declined: cross-checking the SID with an LSA name
lookup. It would bring back the lookup this fix removes. The test
helper hard-codes TokenUser independently, and a TokenOwner mutant is
killed.
Follow-up, out of scope: openjd-sessions-for-python _get_process_user_sid()
uses the same name-lookup pattern.
Signed-off-by: David Leong <116610336+leongdl@users.noreply.github.com>
|
Claude review · advisory Reviewed Open: 0 blocking · 0 should-fix · 0 nit. ✅ Nothing blocking. Fix or reply to each thread; the next revision's review re-checks open threads and resolves those it agrees are handled. Resolving a thread yourself also closes it. Later revisions review only what changed. |
Windows CI showed the pipe DACL reads back with the NETWORK deny ACE before the user allow ACE, both in the in-memory security attributes and on the created pipe. That is canonical order, and the order that makes the deny apply to the user's own network logons. The new tests asserted allow-first and failed on all six Windows jobs; production code was not involved, since mainline builds the same DACL. Assert the canonical order exactly, and find the allowed SID by ACE type rather than by index. Signed-off-by: David Leong <116610336+leongdl@users.noreply.github.com>
crowecawcaw
approved these changes
Nov 3, 2026
mwiebe
approved these changes
Nov 3, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes: no GitHub issue. Follow-up to #262.
What was the problem/requirement? (What/Why)
NamedPipeHelper.create_security_attributes()resolved the current user's SID withLookupAccountName("", getpass.getuser()).getpass.getuser()reads only the LOGNAME, USER, LNAME and USERNAME environment variables, and the lookup goes through LSA, which can fail withERROR_NONE_MAPPED(1332).A Windows integration build of deadline-cloud-for-blender, running as SYSTEM, failed 7 tests with
(1332, 'LookupAccountName', 'No mapping between account names and security IDs was done.'). The error hit both the server path (create_named_pipe_server) and the client path (establish_named_pipe_connection), after the retry added in #262 (sleeps of 1 s and 2 s) ran out. The same commit passed on the builds before and after, so this is a transient host-state failure of the name lookup.What was the solution? (How)
OpenProcessToken(GetCurrentProcess(), TOKEN_QUERY)andGetTokenInformation(token, TokenUser)[0], closing the token in afinally. No name lookup is made, so 1332 cannot occur, and the SID no longer depends on environment variables. The fix: retry LookupAccountName with exponential backoff to prevent ERROR_NONE_MAPPED on fresh EC2 instances #262 retry and thegetpassimport are removed.CreateFileignores the security descriptor when it opens an existing pipe and uses onlybInheritHandle. The client passesSECURITY_ATTRIBUTESwith no descriptor andbInheritHandle = True, which matches pywin32'sSECURITY_ATTRIBUTES()default that it used before. PassingNonewould have made the handle non-inheritable.What is the impact of this change?
Windows named pipe servers and clients no longer fail when LSA cannot map the environment's user name. The pipe is granted to the account the process runs as. Where the environment named a different account, the old code granted the pipe to that named account instead.
How was this change tested?
LookupAccountNameis not called when the environment names an unmapped user, the token is closed whenGetTokenInformationraises, and the client passes no descriptor, an inheritable handle andOPEN_EXISTING. The three tests that pinned the removed lookup and retry are replaced.test_pipe_security_does_not_depend_on_user_name: sets LOGNAME, USER, LNAME and USERNAME to a non-existent account, asserts thatLookupAccountNameraises 1332 for that name, runs a real pipe round-trip and reads the pipe's DACL back withGetSecurityInfo. It runs for server and client, for the client only, and with the normal environment as a control. It is collected by the existinghatch run teststep in the Windows job ofcode_quality.yml.hatch run lint,hatch run mypy --platform win32 src testandhatch run testpass (482 passed, 31 skipped, 93.39% coverage). The Windows tests have not run on real Windows before this PR; this PR's Windows CI is their first run.Was this change documented?
Docstrings and comments in
named_pipe_helper.pyare updated. No user-facing docs change.Is this a breaking change?
No. No public interface changes.
Does this change impact security?
Yes, narrowly. It changes how the pipe DACL picks its user: the process's own account instead of the account named by the environment. That matches the documented intent (only the current user, network logons denied). The NETWORK deny ACE is unchanged and no new principal gains access. Maintainers may want to add the "security" label.
Known gaps, not in this PR:
_secure_open.pyresolves its SID with the sameLookupAccountNamepattern.test_fail_to_connect_to_named_pipe_with_different_usersits insidepytest.raises, so it never runs.By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.