[PR #6966] [client] Derive Windows SSH privilege checks from the token and group membership #30054

Open
opened 2026-08-05 08:09:54 -04:00 by saavagebueno · 0 comments
Owner

📋 Pull Request Information

Original PR: https://github.com/netbirdio/netbird/pull/6966
Author: @lixmal
Created: 7/29/2026
Status: 🔄 Open

Base: mainHead: ssh-windows-privilege-check


📝 Commits (5)

  • c4bc479 Detect Windows privilege from the token and group membership instead of name matching
  • 3c745a8 Fail closed when a local group name cannot be resolved to a SID
  • 6f8dd82 Identify the Administrators group by the name resolved from its well-known SID
  • c4fec48 Resolve test account names from well-known SIDs and RIDs instead of English literals
  • 7aa9395 Require an S4U comparison and check elevation against the token elevation type

📊 Changes

9 files changed (+632 additions, -264 deletions)

View changed files

📝 client/ssh/server/command_execution_windows.go (+2 -2)
client/ssh/server/privileges_other.go (+15 -0)
client/ssh/server/privileges_windows.go (+223 -0)
client/ssh/server/privileges_windows_test.go (+293 -0)
📝 client/ssh/server/server_config_test.go (+12 -33)
📝 client/ssh/server/sftp_windows.go (+1 -1)
📝 client/ssh/server/user_utils.go (+53 -154)
📝 client/ssh/server/user_utils_test.go (+30 -71)
📝 client/ssh/server/userswitching_windows.go (+3 -3)

📄 Description

Describe your changes

The SSH server decided Windows privilege from usernames and SID string patterns, which does not match how Windows expresses privilege, so the allow-root-login setting was enforced against the wrong set of accounts. Privilege is now derived from the process token and from actual group membership.

  • Determine whether the daemon runs elevated from its process token instead of matching the current user's name or SID
  • Classify a target account from its SID (well-known service accounts, built-in Administrator by RID) and from local Administrators membership, replacing the hardcoded list of privileged usernames
  • Resolve Administrators membership for domain accounts through an S4U token so nested and universal groups are covered, with local group enumeration as fallback
  • Deny when an account cannot be resolved or its membership cannot be determined
  • Add tests for the SID and membership checks

With root login disabled, the set of refused accounts changes: members of the local Administrators group are now included, and unprivileged accounts whose names merely resemble privileged ones (such as admin) are not.

Stack

Checklist

  • Is it a bug fix
  • Is a typo/documentation fix
  • Is a feature enhancement
  • It is a refactor
  • Created tests that fail without the change (if possible)
  • This change does not modify the public API, gRPC protocols, functionality behavior, CLI / service flags, or introduce a new feature — OR I have discussed it with the NetBird team beforehand (link the issue / Slack thread in the description). See CONTRIBUTING.md

By submitting this pull request, you confirm that you have read and agree to the terms of the Contributor License Agreement.

Documentation

Select exactly one:

  • I added/updated documentation for this change
  • Documentation is not needed for this change (explain why)

No configuration, flags or documented behaviour change: the allow-root-login setting keeps its documented meaning, this makes enforcement match it on Windows.

Docs PR URL (required if "docs added" is checked)

Paste the PR link from https://github.com/netbirdio/docs here:


View with [code]smith Autofix with [code]smith
Need help on this PR? Tag @codesmith-bot with what you need. Autofix is disabled.

Summary by CodeRabbit

  • New Features
    • Strengthened Windows SSH privilege handling with fail-closed checks for privileged accounts and more reliable Administrators membership evaluation for both local and domain users.
    • Added an option to control whether root login is allowed.
    • Standardized username/domain parsing across Windows SSH command execution, SFTP command creation, and user switching.
  • Tests
    • Expanded Windows privilege and local group membership coverage, including localized account names and consistency/failure scenarios; updated non-Windows expectations and skip behavior.
  • Refactor
    • Reworked the cross-platform privilege-check flow for consistent, platform-appropriate results.

🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.

## 📋 Pull Request Information **Original PR:** https://github.com/netbirdio/netbird/pull/6966 **Author:** [@lixmal](https://github.com/lixmal) **Created:** 7/29/2026 **Status:** 🔄 Open **Base:** `main` ← **Head:** `ssh-windows-privilege-check` --- ### 📝 Commits (5) - [`c4bc479`](https://github.com/netbirdio/netbird/commit/c4bc479df6b74b12edc316738ec43f86bbc6b119) Detect Windows privilege from the token and group membership instead of name matching - [`3c745a8`](https://github.com/netbirdio/netbird/commit/3c745a8228901a039d18a30fbecf5a409499ba24) Fail closed when a local group name cannot be resolved to a SID - [`6f8dd82`](https://github.com/netbirdio/netbird/commit/6f8dd82ddc4ca1e2e382c85703fe4fe132fa624c) Identify the Administrators group by the name resolved from its well-known SID - [`c4fec48`](https://github.com/netbirdio/netbird/commit/c4fec48a71b59e1e3d09c23419f19e392893ee73) Resolve test account names from well-known SIDs and RIDs instead of English literals - [`7aa9395`](https://github.com/netbirdio/netbird/commit/7aa93955d5e13082ade1c0ccadcf6563fdde766b) Require an S4U comparison and check elevation against the token elevation type ### 📊 Changes **9 files changed** (+632 additions, -264 deletions) <details> <summary>View changed files</summary> 📝 `client/ssh/server/command_execution_windows.go` (+2 -2) ➕ `client/ssh/server/privileges_other.go` (+15 -0) ➕ `client/ssh/server/privileges_windows.go` (+223 -0) ➕ `client/ssh/server/privileges_windows_test.go` (+293 -0) 📝 `client/ssh/server/server_config_test.go` (+12 -33) 📝 `client/ssh/server/sftp_windows.go` (+1 -1) 📝 `client/ssh/server/user_utils.go` (+53 -154) 📝 `client/ssh/server/user_utils_test.go` (+30 -71) 📝 `client/ssh/server/userswitching_windows.go` (+3 -3) </details> ### 📄 Description ## Describe your changes The SSH server decided Windows privilege from usernames and SID string patterns, which does not match how Windows expresses privilege, so the allow-root-login setting was enforced against the wrong set of accounts. Privilege is now derived from the process token and from actual group membership. - Determine whether the daemon runs elevated from its process token instead of matching the current user's name or SID - Classify a target account from its SID (well-known service accounts, built-in Administrator by RID) and from local Administrators membership, replacing the hardcoded list of privileged usernames - Resolve Administrators membership for domain accounts through an S4U token so nested and universal groups are covered, with local group enumeration as fallback - Deny when an account cannot be resolved or its membership cannot be determined - Add tests for the SID and membership checks With root login disabled, the set of refused accounts changes: members of the local Administrators group are now included, and unprivileged accounts whose names merely resemble privileged ones (such as `admin`) are not. ## Issue ticket number and link ## Stack <!-- branch-stack --> ### Checklist - [x] Is it a bug fix - [ ] Is a typo/documentation fix - [ ] Is a feature enhancement - [ ] It is a refactor - [x] Created tests that fail without the change (if possible) - [ ] This change does **not** modify the public API, gRPC protocols, functionality behavior, CLI / service flags, or introduce a new feature — **OR** I have discussed it with the NetBird team beforehand (link the issue / Slack thread in the description). See [CONTRIBUTING.md](https://github.com/netbirdio/netbird/blob/main/CONTRIBUTING.md#discuss-changes-with-the-netbird-team-first) > By submitting this pull request, you confirm that you have read and agree to the terms of the [Contributor License Agreement](https://github.com/netbirdio/netbird/blob/main/CONTRIBUTOR_LICENSE_AGREEMENT.md). ## Documentation Select exactly one: - [ ] I added/updated documentation for this change - [x] Documentation is **not needed** for this change (explain why) No configuration, flags or documented behaviour change: the allow-root-login setting keeps its documented meaning, this makes enforcement match it on Windows. ### Docs PR URL (required if "docs added" is checked) Paste the PR link from https://github.com/netbirdio/docs here: <!-- codesmith:footer --> --- <a href="https://app.blacksmith.sh/netbirdio/codesmith/netbird/pr/6966"><picture><source media="(prefers-color-scheme: dark)" srcset="https://pr-comments-assets.blacksmith.sh/codesmith/view-with-codesmith-dark-v2.svg"><source media="(prefers-color-scheme: light)" srcset="https://pr-comments-assets.blacksmith.sh/codesmith/view-with-codesmith-light-v2.svg"><img alt="View with [code]smith" src="https://pr-comments-assets.blacksmith.sh/codesmith/view-with-codesmith-dark-v2.svg"></picture></a> <a href="https://backend.blacksmith.sh/track/enable-autofix?expires=1787930852&installation_model_id=427504&pr_number=6966&repository=netbirdio%2Fnetbird&return_to=https%3A%2F%2Fgithub.com%2Fnetbirdio%2Fnetbird%2Fpull%2F6966&signature=ff94db08960948ad5b3e59d9e9bbb62586db4957ccf697301c9786146a0d24f4"><picture><source media="(prefers-color-scheme: dark)" srcset="https://pr-comments-assets.blacksmith.sh/codesmith/autofix-with-codesmith-dark.svg"><source media="(prefers-color-scheme: light)" srcset="https://pr-comments-assets.blacksmith.sh/codesmith/autofix-with-codesmith-light.svg"><img alt="Autofix with [code]smith" src="https://pr-comments-assets.blacksmith.sh/codesmith/autofix-with-codesmith-dark.svg"></picture></a> <sup>Need help on this PR? Tag <code>@codesmith-bot</code> with what you need. Autofix is disabled.</sup> <!-- codesmith:autofix:disabled --> <!-- /codesmith:footer --> <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **New Features** * Strengthened Windows SSH privilege handling with fail-closed checks for privileged accounts and more reliable Administrators membership evaluation for both local and domain users. * Added an option to control whether root login is allowed. * Standardized username/domain parsing across Windows SSH command execution, SFTP command creation, and user switching. * **Tests** * Expanded Windows privilege and local group membership coverage, including localized account names and consistency/failure scenarios; updated non-Windows expectations and skip behavior. * **Refactor** * Reworked the cross-platform privilege-check flow for consistent, platform-appropriate results. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --- <sub>🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.</sub>
saavagebueno added the pull-request label 2026-08-05 08:09:54 -04:00
Sign in to join this conversation.
No Label pull-request
1 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: DYNR/netbird#30054