Skip to content

Fix username check for env vars#851

Open
tgauth wants to merge 4 commits into
PowerShell:latestw_allfrom
tgauth:fix-username-check-for-env-vars
Open

Fix username check for env vars#851
tgauth wants to merge 4 commits into
PowerShell:latestw_allfrom
tgauth:fix-username-check-for-env-vars

Conversation

@tgauth
Copy link
Copy Markdown
Collaborator

@tgauth tgauth commented May 19, 2026

PR Summary

  • fix check for "sshd" account that was too restrictive, when loading user environment block
  • refactor related code by renaming get_username_from_token() to is_sshd_service_token() and utilizing early return
  • add pester tests for a local user with impacted username: sshd_user

Copilot AI review requested due to automatic review settings May 19, 2026 18:55
Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Note

Copilot was unable to run its full agentic suite in this review.

Updates the Windows process-spawn path to correctly detect the NT SERVICE\sshd virtual account (instead of treating any username starting with sshd as the service), and adds E2E coverage to prevent regressions for local users like sshd_user.

Changes:

  • Replace username-prefix matching with token-based detection of the NT SERVICE\sshd account when deciding whether to load a user environment block.
  • Add new Pester E2E tests validating user environment variables and PATH ordering for an sshd_user account.
  • Extend test helper environment to provision and expose SshdUser and its profile path.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 5 comments.

File Description
regress/pesterTests/UserEnvironment.Tests.ps1 Adds new Pester E2E scenarios verifying user env vars and user PATH entries for sshd_user.
contrib/win32/win32compat/w32fd.c Refactors the “sshd account” detection logic to use token SID lookup and skips env block only for the service virtual account.
contrib/win32/openssh/OpenSSHTestHelper.psm1 Adds SshdUser test account and profile info to the shared test environment.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread regress/pesterTests/UserEnvironment.Tests.ps1 Outdated
Comment thread regress/pesterTests/UserEnvironment.Tests.ps1
Comment thread regress/pesterTests/UserEnvironment.Tests.ps1
Comment thread contrib/win32/win32compat/w32fd.c Outdated
Comment thread contrib/win32/win32compat/w32fd.c
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
@tgauth
Copy link
Copy Markdown
Collaborator Author

tgauth commented May 19, 2026

/azp run

@azure-pipelines
Copy link
Copy Markdown

Azure Pipelines successfully started running 1 pipeline(s).

@tgauth tgauth requested review from TravisEz13 and vthiebaut10 May 19, 2026 20:54
@o-sdn-o
Copy link
Copy Markdown

o-sdn-o commented May 19, 2026

Please forgive me for the inaccuracy, I may be missing something as the context has faded from my mind over time.

Filtering service account tokens by the "sshd" prefix - rather than strictly matching the single "sshd" string - is essential for supporting multi-instance OpenSSH environments and ensuring future-proof architecture. In advanced or enterprise Windows setups, administrators often register multiple SSH services on different ports or for isolation (e.g., sshd_2, sshd-prod), which automatically generate virtual accounts like NT SERVICE\sshd_2.

@o-sdn-o
Copy link
Copy Markdown

o-sdn-o commented May 20, 2026

Also, matching the NT SERVICE domain will fail if the service is configured to run under a custom local or domain account (e.g., sshd_svc_user). In this case, the domain will return the host or AD name instead of NT SERVICE, breaking the profile-loading logic for the legitimate service account.

@tgauth
Copy link
Copy Markdown
Collaborator Author

tgauth commented May 20, 2026

Also, matching the NT SERVICE domain will fail if the service is configured to run under a custom local or domain account (e.g., sshd_svc_user). In this case, the domain will return the host or AD name instead of NT SERVICE, breaking the profile-loading logic for the legitimate service account.

Thanks for pointing this out! May need to think more on ways to account for this but still ensure the profile is loaded for all valid session users

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants