[client] Derive Windows SSH privilege checks from the token and group membership - #6966
[client] Derive Windows SSH privilege checks from the token and group membership#6966lixmal wants to merge 5 commits into
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughWindows SSH privilege detection now uses platform-specific SID, token, S4U, and local-group helpers with injected routing and expanded tests. Username parsing is package-level and reused by Windows command, PTY, SFTP, and user-switching paths. ChangesWindows SSH privilege evaluation
Windows username parsing
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant SSHServer
participant isPrivilegedUsername
participant isWindowsAccountPrivileged
participant WindowsToken
participant NetAPI
SSHServer->>isPrivilegedUsername: evaluate Windows username
isPrivilegedUsername->>isWindowsAccountPrivileged: classify account
isWindowsAccountPrivileged->>WindowsToken: resolve SID and test token membership
isWindowsAccountPrivileged->>NetAPI: enumerate local groups
WindowsToken-->>isWindowsAccountPrivileged: elevation or S4U membership
NetAPI-->>isWindowsAccountPrivileged: local group names
isWindowsAccountPrivileged-->>SSHServer: privilege result
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
client/ssh/server/user_utils.go (1)
190-218: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMinor duplication between
userNameLookupanduserPrivilegeCheck.Both build an identical
PrivilegeCheckRequestand only differ in what they extract from the result.userNameLookupcould simply calluserPrivilegeCheckand returnresult.User, errto avoid the two staying in sync manually as the request shape evolves.♻️ Suggested refactor
func (s *Server) userNameLookup(username string) (*user.User, error) { - result := s.CheckPrivileges(PrivilegeCheckRequest{ - RequestedUsername: username, - FeatureSupportsUserSwitch: true, - FeatureName: FeatureSSHLogin, - }) - - if !result.Allowed { - return nil, result.Error - } - - return result.User, nil + result, err := s.userPrivilegeCheck(username) + if err != nil { + return nil, err + } + return result.User, nil }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@client/ssh/server/user_utils.go` around lines 190 - 218, Refactor userNameLookup to call userPrivilegeCheck with the username, then return the resulting User and error. Remove its duplicated CheckPrivileges request construction while preserving the existing userPrivilegeCheck behavior and return semantics.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@client/ssh/server/privileges_windows.go`:
- Around line 142-158: Update localGroupsContainSID to track whether any group
SID resolution succeeds and capture LookupSID errors; if all returned groups
fail resolution, return a non-nil error instead of false, nil, while preserving
true for matching SIDs and false, nil when at least one group resolves without
matching.
---
Nitpick comments:
In `@client/ssh/server/user_utils.go`:
- Around line 190-218: Refactor userNameLookup to call userPrivilegeCheck with
the username, then return the resulting User and error. Remove its duplicated
CheckPrivileges request construction while preserving the existing
userPrivilegeCheck behavior and return semantics.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: f08072d9-a472-4d4a-8c84-37d05a064cd5
📒 Files selected for processing (9)
client/ssh/server/command_execution_windows.goclient/ssh/server/privileges_other.goclient/ssh/server/privileges_windows.goclient/ssh/server/privileges_windows_test.goclient/ssh/server/server_config_test.goclient/ssh/server/sftp_windows.goclient/ssh/server/user_utils.goclient/ssh/server/user_utils_test.goclient/ssh/server/userswitching_windows.go
Release artifactsBuilt for PR head
GHCR images (amd64)
This comment is updated by the Release workflow. Artifact links expire according to the workflow retention policy. |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
client/ssh/server/privileges_windows_test.go (3)
128-168: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winRequire at least one S4U comparison.
Because ineligible users are skipped, this test can complete successfully without checking any account when all local users fail S4U logon. Track the number of comparisons and fail with a diagnostic if it remains zero.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@client/ssh/server/privileges_windows_test.go` around lines 128 - 168, The TestS4UMembershipAgreesWithLocalGroups test must require at least one successful S4U comparison. After iterating through users and incrementing checked only for completed comparisons, assert that checked is greater than zero with a diagnostic describing that all local accounts were skipped due to S4U logon failure.
79-104: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winResolve built-in accounts by SID instead of literal names.
isWindowsAccountPrivilegedtreats lookup failures as privileged, but these tests still call it with"Administrator"and"Guest". On systems where those accounts are renamed, the lookup can fail and the assertion can pass for the fail-closed path rather than the intended membership check. Drive these cases from well-known SIDs or a controlled fixture instead. This also affects thelocalGroupsContainSIDtests at lines 170-220.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@client/ssh/server/privileges_windows_test.go` around lines 79 - 104, Update TestIsWindowsAccountPrivileged and the localGroupsContainSID tests to resolve built-in Administrator and Guest accounts through their well-known SIDs or a controlled fixture instead of literal names. Ensure each assertion exercises actual SID-based membership behavior and does not pass via isWindowsAccountPrivileged’s fail-closed lookup path.
106-122: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winTest UAC elevation independently from Administrators membership.
Token.IsMemberchecks enabled Administrators membership, whereasisProcessElevated()usesTokenElevation. An Administrators token can be non-elevated under UAC, so this precondition can skip limited-admin cases or assert an unrelated property. InjectgetProcessElevatedor otherwise exercise the elevation result directly.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@client/ssh/server/privileges_windows_test.go` around lines 106 - 122, The TestIsProcessElevated test incorrectly derives its assertion from Administrators membership instead of independently testing TokenElevation. Update the test to inject or otherwise control the getProcessElevated result used by isProcessElevated, and assert elevation behavior directly without gating on Token(0).IsMember(adminSid); retain only membership checks if they are separately needed.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@client/ssh/server/privileges_windows_test.go`:
- Around line 128-168: The TestS4UMembershipAgreesWithLocalGroups test must
require at least one successful S4U comparison. After iterating through users
and incrementing checked only for completed comparisons, assert that checked is
greater than zero with a diagnostic describing that all local accounts were
skipped due to S4U logon failure.
- Around line 79-104: Update TestIsWindowsAccountPrivileged and the
localGroupsContainSID tests to resolve built-in Administrator and Guest accounts
through their well-known SIDs or a controlled fixture instead of literal names.
Ensure each assertion exercises actual SID-based membership behavior and does
not pass via isWindowsAccountPrivileged’s fail-closed lookup path.
- Around line 106-122: The TestIsProcessElevated test incorrectly derives its
assertion from Administrators membership instead of independently testing
TokenElevation. Update the test to inject or otherwise control the
getProcessElevated result used by isProcessElevated, and assert elevation
behavior directly without gating on Token(0).IsMember(adminSid); retain only
membership checks if they are separately needed.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 2d5566c6-4e70-4f94-b236-9a9d0f56eca2
📒 Files selected for processing (3)
client/ssh/server/privileges_windows.goclient/ssh/server/privileges_windows_test.goclient/ssh/server/user_utils.go
🚧 Files skipped from review as they are similar to previous changes (2)
- client/ssh/server/privileges_windows.go
- client/ssh/server/user_utils.go
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@client/ssh/server/privileges_windows_test.go`:
- Around line 169-177: Update TestLocalGroupsContainSID_Administrator to locate
the local account whose SID satisfies isBuiltinAdministratorSID (RID 500)
instead of passing the literal "Administrator" name, then pass that resolved
account name to localGroupsContainSID while preserving the existing assertions.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 5dc669af-14b2-46d5-b914-9bb49c890102
📒 Files selected for processing (2)
client/ssh/server/privileges_windows.goclient/ssh/server/privileges_windows_test.go
|
1 participant
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.
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
Checklist
Documentation
Select exactly one:
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:
Need help on this PR? Tag
@codesmith-botwith what you need. Autofix is disabled.Summary by CodeRabbit