[client] Fall back to getent/id for SSH user lookup in static builds#5510
Merged
[client] Fall back to getent/id for SSH user lookup in static builds#5510
Conversation
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds NSS/getent-based user/group lookup and shell detection with platform-specific shims (CGO, non-CGO, Windows), wires them into SSH server user-switching and shell resolution, and adds comprehensive unit tests for parsing, validation, and fallback behavior. Changes
Sequence Diagram(s)sequenceDiagram
participant SSH as "SSH Server"
participant StdLib as "os/user (stdlib)"
participant Getent as "getent / id (system)"
participant Logger as "Logger"
rect rgba(100,150,200,0.5)
Note over SSH,StdLib: User lookup flow
SSH->>StdLib: user.Lookup(username)
alt StdLib returns user
StdLib-->>SSH: *user.User
else StdLib returns error
StdLib-->>SSH: error
SSH->>Logger: debug fallback to getent
SSH->>Getent: exec "getent passwd <username>"
Getent-->>SSH: passwd entry / error
SSH->>SSH: parseGetentPasswd()
SSH-->>SSH: *user.User or original error
end
end
rect rgba(150,200,100,0.5)
Note over SSH,Getent: Group IDs flow
SSH->>StdLib: u.GroupIds()
alt StdLib returns ids
StdLib-->>SSH: []string
else StdLib returns error
StdLib-->>SSH: error
SSH->>Logger: debug fallback to id
SSH->>Getent: exec "id -G <username>"
Getent-->>SSH: "gid1 gid2" / error
SSH->>SSH: parse -> []string or original error
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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 |
lixmal
force-pushed
the
fix/ssh-nss-user-lookup
branch
from
bc94487 to
40542b7
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
client/ssh/server/getent_test.go (1)
67-71: Minor: Redundant assertion after empty check.Line 70's assertion
len(shell) > 0is redundant since line 67's condition already ensuresshell != ""at this point.♻️ Suggested simplification
shell := getShellFromGetent(current.Uid) if shell == "" { t.Log("getShellFromGetent returned empty, getent may not be available") - } else { - assert.True(t, len(shell) > 0, "shell should be non-empty when getent is available") } + // If we reach here with non-empty shell, the test passed }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@client/ssh/server/getent_test.go` around lines 67 - 71, The test contains a redundant assertion after checking for an empty shell: in getent_test.go the if/else around getShellFromGetent() logs when shell == "" and then asserts len(shell) > 0 in the else branch, which is unnecessary; simplify by removing the redundant assert in the else branch (or replace the else with a single assert that fails with a clear message when shell == ""), keeping reference to getShellFromGetent and the test's logging/assertion logic.client/ssh/server/getent_unix.go (1)
103-106: Returningnil, nilfor empty output may cause confusion.When
id -Greturns empty output, this returns(nil, nil). Callers may interpret this as "no groups" vs "error occurred". Consider returning an empty slice[]string{}instead ofnilto make the "no groups" case explicit.♻️ Suggested improvement
trimmed := strings.TrimSpace(string(out)) if trimmed == "" { - return nil, nil + return []string{}, nil } return strings.Fields(trimmed), nil🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@client/ssh/server/getent_unix.go` around lines 103 - 106, The function that handles the output of `id -G` currently returns (nil, nil) when `trimmed == ""`, which is ambiguous; update the branch that checks `trimmed := strings.TrimSpace(string(out))` so it returns an explicit empty slice (e.g. `[]string{}`) and nil error instead of `nil, nil`—locate the function that invokes `id -G`/parses groups (the block using `trimmed` in getent_unix.go) and change the return to `return []string{}, nil` to clearly signal “no groups.”client/ssh/server/getent_unix_test.go (1)
228-235:TestRunGetent_NotAvailableis non-deterministic and likely always skipped in CI.Consider moving command lookup/execution behind a small injectable function so this path can be unit-tested deterministically instead of depending on host tooling.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@client/ssh/server/getent_unix_test.go` around lines 228 - 235, TestRunGetent_NotAvailable relies on the host having/ not having the "getent" binary and is non-deterministic; refactor runGetent to accept or call an injectable lookup/exec hook (e.g., replace direct exec.LookPath/exec.Command usage with a package-level variable/function like lookupPath(name string) (string, error) and commandRunner(name string, args ...string) *Cmd) so tests can override them. Change runGetent to use these hooks (defaulting to exec.LookPath and exec.Command) and update TestRunGetent_NotAvailable to override lookupPath to return an error, asserting runGetent("root") fails deterministically. Ensure the hooks are only used in runGetent and exposed for tests without changing public API semantics.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@client/ssh/server/getent_unix_test.go`:
- Around line 237-253: Tests calling runIdGroups (TestRunIdGroups_CurrentUser,
TestRunIdGroups_NonexistentUser, and TestIdGroupsMatchStdlib) assume the
external `id` binary exists; locate these test functions and add the same guard
used for getent checks: call exec.LookPath("id") at the start of each test and
t.Skipf("skipping test: %v", err) when LookPath returns an error so tests are
skipped on minimal Unix images without `id`. Ensure you reference/run the guard
before invoking runIdGroups to avoid infrastructure-dependent failures.
---
Nitpick comments:
In `@client/ssh/server/getent_test.go`:
- Around line 67-71: The test contains a redundant assertion after checking for
an empty shell: in getent_test.go the if/else around getShellFromGetent() logs
when shell == "" and then asserts len(shell) > 0 in the else branch, which is
unnecessary; simplify by removing the redundant assert in the else branch (or
replace the else with a single assert that fails with a clear message when shell
== ""), keeping reference to getShellFromGetent and the test's logging/assertion
logic.
In `@client/ssh/server/getent_unix_test.go`:
- Around line 228-235: TestRunGetent_NotAvailable relies on the host having/ not
having the "getent" binary and is non-deterministic; refactor runGetent to
accept or call an injectable lookup/exec hook (e.g., replace direct
exec.LookPath/exec.Command usage with a package-level variable/function like
lookupPath(name string) (string, error) and commandRunner(name string, args
...string) *Cmd) so tests can override them. Change runGetent to use these hooks
(defaulting to exec.LookPath and exec.Command) and update
TestRunGetent_NotAvailable to override lookupPath to return an error, asserting
runGetent("root") fails deterministically. Ensure the hooks are only used in
runGetent and exposed for tests without changing public API semantics.
In `@client/ssh/server/getent_unix.go`:
- Around line 103-106: The function that handles the output of `id -G` currently
returns (nil, nil) when `trimmed == ""`, which is ambiguous; update the branch
that checks `trimmed := strings.TrimSpace(string(out))` so it returns an
explicit empty slice (e.g. `[]string{}`) and nil error instead of `nil,
nil`—locate the function that invokes `id -G`/parses groups (the block using
`trimmed` in getent_unix.go) and change the return to `return []string{}, nil`
to clearly signal “no groups.”
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: e961c2cb-cdb7-4dcb-ae71-d9f77c48076d
📒 Files selected for processing (9)
client/ssh/server/getent_cgo_unix.goclient/ssh/server/getent_nocgo_unix.goclient/ssh/server/getent_test.goclient/ssh/server/getent_unix.goclient/ssh/server/getent_unix_test.goclient/ssh/server/getent_windows.goclient/ssh/server/shell.goclient/ssh/server/user_utils.goclient/ssh/server/userswitching_unix.go
lixmal
force-pushed
the
fix/ssh-nss-user-lookup
branch
from
40542b7 to
94d8675
Compare
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
client/ssh/server/getent_unix_test.go (1)
237-253:⚠️ Potential issue | 🟡 MinorAdd
id-binary availability guards torunIdGroupstests.These tests directly execute
id -Gbut don’t skip whenidis missing, which makes them infra-dependent on minimal Unix environments.Proposed fix
func TestRunIdGroups_CurrentUser(t *testing.T) { + if _, err := exec.LookPath("id"); err != nil { + t.Skip("id not available on this system") + } + current, err := user.Current() require.NoError(t, err) @@ func TestRunIdGroups_NonexistentUser(t *testing.T) { + if _, err := exec.LookPath("id"); err != nil { + t.Skip("id not available on this system") + } + _, err := runIdGroups("nonexistent_user_xyzzy_12345") assert.Error(t, err) } @@ func TestIdGroupsMatchStdlib(t *testing.T) { + if _, err := exec.LookPath("id"); err != nil { + t.Skip("id not available on this system") + } + current, err := user.Current() require.NoError(t, err)Also applies to: 296-310
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@client/ssh/server/getent_unix_test.go` around lines 237 - 253, The tests TestRunIdGroups_CurrentUser and TestRunIdGroups_NonexistentUser call the external "id -G" binary and must be skipped if that binary is not available; update both tests (and the similar tests at lines 296-310) to early-check exec.LookPath("id") and call t.Skipf("skipping test: id binary not found") when LookPath returns an error so the tests are not infra-dependent; keep the guard at the top of each test before calling runIdGroups to ensure the skip happens before any assertions.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@client/ssh/server/getent_unix_test.go`:
- Around line 237-253: The tests TestRunIdGroups_CurrentUser and
TestRunIdGroups_NonexistentUser call the external "id -G" binary and must be
skipped if that binary is not available; update both tests (and the similar
tests at lines 296-310) to early-check exec.LookPath("id") and call
t.Skipf("skipping test: id binary not found") when LookPath returns an error so
the tests are not infra-dependent; keep the guard at the top of each test before
calling runIdGroups to ensure the skip happens before any assertions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 80a61b48-3534-4dfd-8777-5b7544748412
📒 Files selected for processing (9)
client/ssh/server/getent_cgo_unix.goclient/ssh/server/getent_nocgo_unix.goclient/ssh/server/getent_test.goclient/ssh/server/getent_unix.goclient/ssh/server/getent_unix_test.goclient/ssh/server/getent_windows.goclient/ssh/server/shell.goclient/ssh/server/user_utils.goclient/ssh/server/userswitching_unix.go
🚧 Files skipped from review as they are similar to previous changes (1)
- client/ssh/server/shell.go
lixmal
force-pushed
the
fix/ssh-nss-user-lookup
branch
from
94d8675 to
3234df8
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@client/ssh/server/getent_test.go`:
- Around line 163-166: The test in getent_test.go asserts that
getUserShell(current.Uid) returns a path starting with '/', which is stricter
than runtime behavior and can make the test flaky; remove (or replace) the
absolute-path assertion so the test only requires a non-empty shell (keep
require.NotEmpty(t, shell, ...) as the check) and optionally replace the removed
assertion with a t.Logf noting non-absolute shells for debugging; update the
test around getUserShell(current.Uid) accordingly.
In `@client/ssh/server/getent_unix.go`:
- Around line 117-120: The current check that returns nil, nil when `trimmed ==
""` should instead return an error so the no-CGO path's fallback runs; replace
the `return nil, nil` at the `trimmed == ""` branch in getent_unix.go with an
error return (e.g., fmt.Errorf("empty output from id -G for user %s", username))
so that `groupIdsWithFallback` will call `u.GroupIds()` and not silently drop
NSS groups.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 82c8c6a4-5b8e-4396-89bd-a7d5435f0506
📒 Files selected for processing (9)
client/ssh/server/getent_cgo_unix.goclient/ssh/server/getent_nocgo_unix.goclient/ssh/server/getent_test.goclient/ssh/server/getent_unix.goclient/ssh/server/getent_unix_test.goclient/ssh/server/getent_windows.goclient/ssh/server/shell.goclient/ssh/server/user_utils.goclient/ssh/server/userswitching_unix.go
🚧 Files skipped from review as they are similar to previous changes (3)
- client/ssh/server/getent_nocgo_unix.go
- client/ssh/server/shell.go
- client/ssh/server/getent_unix_test.go
lixmal
force-pushed
the
fix/ssh-nss-user-lookup
branch
from
3234df8 to
9ee4097
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
client/ssh/server/getent_test.go (1)
71-72:⚠️ Potential issue | 🟡 MinorRelax strict absolute-shell assertions to match runtime fallback behavior.
These checks can be flaky because the runtime can fall back to
$SHELL, which may be non-absolute in valid environments.Proposed test adjustment
+import "os/exec" @@ - assert.True(t, shell[0] == '/', "shell should be an absolute path, got %q", shell) + if shell[0] != '/' { + _, err := exec.LookPath(shell) + assert.NoError(t, err, "shell should be absolute or resolvable in PATH, got %q", shell) + }Also applies to: 124-125, 164-165
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@client/ssh/server/getent_test.go` around lines 71 - 72, The test currently asserts the shell is an absolute path with assert.True(t, shell[0] == '/', ...), which is flaky because runtime can legitimately fall back to a non-absolute $SHELL; replace that strict check with a relaxed one using path/filepath.IsAbs(shell) (or strings.HasPrefix(shell, "/")) OR simply assert that shell is non-empty and valid for the runtime (e.g. assert.NotEmpty(t, shell) and skip absolute-path enforcement). Apply the same change to the other occurrences in this file corresponding to the same assertion pattern (the lines around the current occurrence and the ones mentioned at 124-125 and 164-165) so all absolute-path assertions are relaxed consistently.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@client/ssh/server/getent_test.go`:
- Around line 48-51: The test currently assumes every group ID in the groups
slice can be parsed via strconv.ParseUint, which fails for Windows SID-style IDs
like "S-1-..."; update the loop that iterates over groups (and the other two
identical occurrences) to first check if the gid string is a SID (e.g.,
strings.HasPrefix(gid, "S-")) and skip the numeric parse/assert for those
entries, otherwise call strconv.ParseUint(gid, 10, 32) and assert no error as
before; ensure you import strings if not already present.
---
Duplicate comments:
In `@client/ssh/server/getent_test.go`:
- Around line 71-72: The test currently asserts the shell is an absolute path
with assert.True(t, shell[0] == '/', ...), which is flaky because runtime can
legitimately fall back to a non-absolute $SHELL; replace that strict check with
a relaxed one using path/filepath.IsAbs(shell) (or strings.HasPrefix(shell,
"/")) OR simply assert that shell is non-empty and valid for the runtime (e.g.
assert.NotEmpty(t, shell) and skip absolute-path enforcement). Apply the same
change to the other occurrences in this file corresponding to the same assertion
pattern (the lines around the current occurrence and the ones mentioned at
124-125 and 164-165) so all absolute-path assertions are relaxed consistently.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: b6a3e21d-d53f-4a81-a81d-72e2020ce896
📒 Files selected for processing (9)
client/ssh/server/getent_cgo_unix.goclient/ssh/server/getent_nocgo_unix.goclient/ssh/server/getent_test.goclient/ssh/server/getent_unix.goclient/ssh/server/getent_unix_test.goclient/ssh/server/getent_windows.goclient/ssh/server/shell.goclient/ssh/server/user_utils.goclient/ssh/server/userswitching_unix.go
🚧 Files skipped from review as they are similar to previous changes (3)
- client/ssh/server/user_utils.go
- client/ssh/server/getent_nocgo_unix.go
- client/ssh/server/getent_cgo_unix.go
lixmal
force-pushed
the
fix/ssh-nss-user-lookup
branch
from
9ee4097 to
9c9483b
Compare
|
pappz
approved these changes
Mar 13, 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
2 participants
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.
Describe your changes
When CGO is disabled (
CGO_ENABLED=0), Go'sos/useronly reads/etc/passwdand/etc/groupdirectly, bypassing the system's NSS stack. This means users provided by SSSD, LDAP, Active Directory, or other NSS modules can't be resolved.This adds
getent passwd/id -Gfallback for user lookup, group resolution, and shell detection. These commands go through the host's NSS stack regardless of how the Go binary was built. The fallback is only compiled in non-CGO builds (or whenosusergotag is set), since libc handles NSS natively when CGO is enabled.Issue ticket number and link
Closes #4919
Stack
Checklist
Documentation
Select exactly one:
No user-facing API or configuration changes. Behavior is transparent.
Docs PR URL (required if "docs added" is checked)
Paste the PR link from https://github.com/netbirdio/docs here:
https://github.com/netbirdio/docs/pull/__
Summary by CodeRabbit
New Features
Tests