SSH: prefer using runuser instead of login#4873
SSH: prefer using runuser instead of login#4873afonsofrancof wants to merge 1 commit intonetbirdio:mainfrom
runuser instead of login#4873Conversation
WalkthroughRenames createPtyLoginCommand to createUserSwitchPtyCommand and updates its call site. getLoginCmd now prefers Linux's Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20–30 minutes
Possibly related issues
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ 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 |
|
|
afonsofrancof
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
client/ssh/server/userswitching_unix.go(5 hunks)
🔇 Additional comments (4)
client/ssh/server/userswitching_unix.go (4)
16-16: LGTM!The
stringsimport is correctly added to support the newisDebian()function.
63-64: LGTM!The function rename from
createPtyLoginCommandtocreateUserSwitchPtyCommandis more accurate, as the function now handles multiple user-switching mechanisms (bothloginandrunuser).
222-222: LGTM!The call site is correctly updated to use the renamed function
createUserSwitchPtyCommand.
91-97:runuser -lis the correct approach for Debian user switching and provides proper login environment initialization.The web search confirms that
runuser -lon Debian (via util-linux) correctly:
- Simulates a full login session for the target user
- Initializes required environment variables (HOME, SHELL, USER, LOGNAME, PATH)
- Changes to the user's home directory
- Sets the shell as a login shell (argv[0] = "-")
- Uses PAM for proper authentication/authorization
The implementation is correct.
runuseris standard on Debian systems (provided by the util-linux package), and the fallback tologinifrunuseris unavailable is appropriate defensive programming.
| cmd := exec.Command("lsb_release", "-is") | ||
| distro, err := cmd.Output() | ||
| if err != nil { | ||
| return false | ||
| } | ||
|
|
||
| if strings.TrimSpace(string(distro)) != "Debian" { | ||
| return false | ||
| } | ||
|
|
||
| return true | ||
| } | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# First, understand the file structure and current implementation
echo "=== Examining userswitching_unix.go ==="
wc -l client/ssh/server/userswitching_unix.go
echo ""
echo "=== Reading lines 100-130 for full context ==="
sed -n '100,130p' client/ssh/server/userswitching_unix.goRepository: netbirdio/netbird
Length of output: 178
🌐 Web query:
lsb_release availability minimal Debian installation base-files
💡 Result:
Short answer: lsb_release is provided by the lsb-release package (not by base-files) and is not guaranteed in a minimal Debian install — base-files supplies /etc/os-release, which lsb_release reads, but recent testing/unstable base-files releases omit VERSION_* fields so lsb_release may show "No LSB modules are available" or lack full version info. Install lsb-release to get the command: apt install lsb-release. [1][2][3]
Sources:
- lsb-release package details. [1]
- base-files package (provides /etc/os-release). [2]
- Debian bug reports explaining missing VERSION_* in base-files causing lsb_release to lack version info. [3]
🌐 Web query:
/etc/os-release format ID field Debian systemd specification
💡 Result:
The ID field must be a lower-case identifier (no spaces) containing only characters 0–9, a–z, ".", "_" and "-" (suitable for scripts/filenames). If unset it defaults to "linux". Examples: ID=debian, ID=fedora. [1][2]
Sources: systemd os-release man page (ID field) — Debian/manpages and Mankier. [1] (manpages.debian.org) [2] (mankier.com)
Citations:
- 1: https://manpages.debian.org/unstable/systemd/os-release.5.en.html?utm_source=openai
- 2: https://www.mankier.com/5/os-release?utm_source=openai
🌐 Web query:
which Linux distributions have /etc/os-release vs lsb_release
💡 Result:
Short answer:
- /etc/os-release (the freedesktop/systemd standard) is present on essentially all modern distributions (Debian, Ubuntu, Fedora, RHEL/CentOS/Alma/Rocky, openSUSE/SUSE, Arch/Manjaro, Alpine, many others). [1][2]
- lsb_release is a separate utility (from the lsb-release package); it is not a filesystem-standard and may or may not be installed by default. Debian/Ubuntu and most major distros provide an lsb-release package; lightweight images (some Alpine, minimal containers) often do not install it by default. If present, lsb_release typically reads /etc/os-release. [3][4]
Notes/sources:
- os-release spec (freedesktop/systemd). [1]
- community summaries and comparisons (/etc/lsb-release vs /etc/os-release). [2]
- Alpine package listing showing /etc/os-release provided by alpine-release. [3]
- Debian/Fedora package pages for lsb-release (shows lsb_release is a package that may use os-release as data source). [4]
🌐 Web query:
Go os.ReadFile function import location package
💡 Result:
Use the standard library os package: import "os". os.ReadFile was added in Go 1.16 (io/ioutil.ReadFile is deprecated). Example: data, err := os.ReadFile("file.txt"). [1][2]
Sources:
[1] pkg.go.dev/os (ReadFile documentation).
[2] pkg.go.dev/io/ioutil (ReadFile deprecated; forwards to os.ReadFile).
🌐 Web query:
Go strings.Split strings.HasPrefix strings.TrimPrefix functions
💡 Result:
Here are concise descriptions, behaviors, edge cases, and short examples for strings.Split, strings.HasPrefix, and strings.TrimPrefix in Go (from the official strings package docs).
- strings.Split
- What it does: returns a slice of substrings of s separated by sep; equivalent to SplitN(s, sep, -1).
- Important edge cases: if sep == "" it splits after each UTF‑8 sequence; if sep is non‑empty and not found it returns []string{s}; if both s and sep are empty it returns an empty slice. (pkg.go.dev)
- Example:
fmt.Println(strings.Split("a,b,c", ",")) // ["a","b","c"]
- strings.HasPrefix
- What it does: reports whether s begins with prefix (returns bool). (pkg.go.dev)
- Example:
fmt.Println(strings.HasPrefix("hello", "he")) // true
- strings.TrimPrefix
- What it does: returns s with the provided prefix removed if s starts with that prefix; returns s unchanged if the prefix is not present. (pkg.go.dev)
- Example:
fmt.Println(strings.TrimPrefix("prefix_value", "prefix_")) // "value"
Reference: Go strings package documentation. (pkg.go.dev)
If you want, I can show more examples (SplitN, SplitAfter, behavior with Unicode, or code snippets you can run).
Citations:
- 1: https://pkg.go.dev/strings?utm_source=openai
- 2: https://pkg.go.dev/strings?utm_source=openai
- 3: https://pkg.go.dev/strings?utm_source=openai
- 4: https://pkg.go.dev/strings?utm_source=openai
Perfect. The web search confirms all the technical details:
os.ReadFile()is from the standardospackage (available since Go 1.16)- All string functions used (
Split,HasPrefix,TrimPrefix,Trim) are from the standardstringspackage - The proposed implementation logic is sound:
- Correctly splits by newlines
- Properly extracts the ID field
- Handles both quoted and unquoted formats in
/etc/os-release - Compares to lowercase "debian" per the freedesktop specification
The review comment is factually accurate and the proposed solution is technically correct. The suggestion to use /etc/os-release instead of lsb_release is a legitimate improvement for minimal Debian installations where the lsb-release package may not be installed.
Use /etc/os-release for better compatibility on minimal Debian systems.
The current implementation depends on lsb_release, which requires the optional lsb-release package and is not guaranteed on minimal Debian installations. The /etc/os-release file, provided by base-files, is universally available on all modern Linux distributions including minimal Debian installs.
Apply this diff to use /etc/os-release instead:
-// isDebian checks if the system is running Debian
+// isDebian checks if the system is running Debian
func (s *Server) isDebian() bool {
- cmd := exec.Command("lsb_release", "-is")
- distro, err := cmd.Output()
+ data, err := os.ReadFile("/etc/os-release")
if err != nil {
return false
}
-
- if strings.TrimSpace(string(distro)) != "Debian" {
- return false
+
+ for _, line := range strings.Split(string(data), "\n") {
+ if strings.HasPrefix(line, "ID=") {
+ id := strings.TrimPrefix(line, "ID=")
+ id = strings.Trim(id, "\"")
+ return id == "debian"
+ }
}
-
- return true
+
+ return false
}🤖 Prompt for AI Agents
In client/ssh/server/userswitching_unix.go around lines 111 to 123, replace the
lsb_release-based detection with reading /etc/os-release: read the file using
os.ReadFile, split by newlines, find the line that starts with "ID=", strip the
"ID=" prefix and any surrounding quotes, normalize to lowercase with
strings.Trim/strings.ToLower, then return true only if it equals "debian"; on
any read/parse error return false. Ensure you import os and use only stdlib
strings functions.
|
Will fix these implementation details |
afonsofrancof
force-pushed
the
feature/debian-ssh
branch
from
ec53438 to
b88bf95
Compare
|
Changed it :) |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
client/ssh/server/userswitching_unix.go (1)
91-97: Fix comment formatting.Missing space after
//in the comment. The Debian detection logic using/etc/debian_versionis simple and appropriate, with good fallback behavior ifrunuseris unavailable.Apply this diff:
- //Special handling for Debian + // Special handling for Debian
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
client/ssh/server/userswitching_unix.go(4 hunks)
🔇 Additional comments (3)
client/ssh/server/userswitching_unix.go (3)
63-64: LGTM! Function rename improves clarity.The rename from
createPtyLoginCommandtocreateUserSwitchPtyCommandbetter reflects the function's purpose of switching users rather than just using the login command.
207-207: LGTM! Call site correctly updated.The function call has been properly updated to match the renamed function.
16-16: I encountered an issue accessing the repository to verify thestringsimport usage. Since I cannot execute the verification script, I cannot definitively determine whether the import is used elsewhere in the file.Based on the limitations, here is the rewritten review comment:
Verify that the
stringsimport is used in the file.The
stringspackage is imported but doesn't appear to be used in any of the visible code changes. If it's not used elsewhere in the file, remove it to avoid unused imports.
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
client/ssh/server/userswitching_unix.go (1)
62-74: Update comment oncreateUserSwitchPtyCommandto reflectrunuserusageOn Line 62 the comment says this creates a Pty command “using login”, but on Debian you now return
runuserfromgetLoginCmd. This is a small doc/maintenance mismatch and can confuse future readers.Consider something like:
-// createUserSwitchPtyCommand creates a Pty command using login for privileged processes +// createUserSwitchPtyCommand creates a Pty command for privileged user switching +// using the system login tool (e.g. runuser on Debian-based systems).
🧹 Nitpick comments (1)
client/ssh/server/userswitching_unix.go (1)
88-101: Align Debianrunuserhandling with the stated fallback behaviorThe Debian branch (Lines 90–96) correctly prefers
runuser -l <username>when/etc/debian_versionexists andrunuseris onPATH, which matches the intent of the PR.However,
exec.LookPath("login")is called before the switch (Lines 78–81). This means that on a Debian(-based) system whereloginis not installed butrunuseris,getLoginCmdwill fail before it ever reaches the Debianrunuserlogic. That slightly contradicts the description “use runuser, and if it’s not present, fall back to login”.If you want the behavior to truly be “prefer
runuser, then fall back tologin” on Debian, consider restructuringgetLoginCmdroughly like this (conceptually):-func (s *Server) getLoginCmd(username string, remoteAddr net.Addr) (string, []string, error) { - loginPath, err := exec.LookPath("login") - if err != nil { - return "", nil, fmt.Errorf("login command not available: %w", err) - } - - addrPort, err := netip.ParseAddrPort(remoteAddr.String()) +func (s *Server) getLoginCmd(username string, remoteAddr net.Addr) (string, []string, error) { + addrPort, err := netip.ParseAddrPort(remoteAddr.String()) if err != nil { return "", nil, fmt.Errorf("parse remote address: %w", err) } switch runtime.GOOS { case "linux": - //Special handling for Debian - if s.fileExists("/etc/debian_version") { - runuserPath, err := exec.LookPath("runuser") - if err == nil { - return runuserPath, []string{"-l", username}, nil - } - } + // Special handling for Debian and Debian-based systems: prefer runuser if available. + if s.fileExists("/etc/debian_version") { + if runuserPath, err := exec.LookPath("runuser"); err == nil { + return runuserPath, []string{"-l", username}, nil + } + } + + loginPath, err := exec.LookPath("login") + if err != nil { + return "", nil, fmt.Errorf("login command not available: %w", err) + } // Special handling for Arch Linux without /etc/pam.d/remote if s.fileExists("/etc/arch-release") && !s.fileExists("/etc/pam.d/remote") { return loginPath, []string{"-f", username, "-p"}, nil } return loginPath, []string{"-f", username, "-h", addrPort.Addr().String(), "-p"}, nil case "darwin", "freebsd", "openbsd", "netbsd", "dragonfly": + loginPath, err := exec.LookPath("login") + if err != nil { + return "", nil, fmt.Errorf("login command not available: %w", err) + } return loginPath, []string{"-fp", "-h", addrPort.Addr().String(), username}, nilThis keeps existing behavior for non-Debian hosts, but on Debian(-based) systems it no longer requires
loginto be present in cases whererunuseris sufficient.Also, tiny nit: add a space after
//in the Debian comment for consistency with the rest of the file.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
client/ssh/server/userswitching_unix.go(3 hunks)
🔇 Additional comments (1)
client/ssh/server/userswitching_unix.go (1)
195-207: Rename tocreateUserSwitchPtyCommandlooks goodThe change in Line 206 to call
createUserSwitchPtyCommandinstead of the oldcreatePtyLoginCommandis consistent with the new naming and keeps the control flow intact (fallback vs. user-switching path). No functional concerns here.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
client/ssh/server/userswitching_unix.go(5 hunks)
🔇 Additional comments (4)
client/ssh/server/userswitching_unix.go (4)
16-16: LGTM!The
stringsimport is necessary for the newstrings.Contains()call inisUtilLinuxLogin().
63-64: LGTM!The rename from
createPtyLoginCommandtocreateUserSwitchPtyCommandimproves clarity and better describes the function's purpose.
218-218: LGTM!The call site correctly uses the renamed
createUserSwitchPtyCommandfunction.
91-97: I'm unable to access the repository to verify the call frequency concern due to clone failures. Without being able to inspect the codebase, I cannot definitively verify:
- How many times
getLoginCmd()is called per SSH session- Whether
isUtilLinuxLogin()is called in loops- The actual performance impact of repeated
login -VexecutionsBased on the available information, here is my rewritten review comment:
Verify whether
getLoginCmd()is called multiple times per SSH session before optimizing.The util-linux detection and
runuserfallback logic is sound and handles graceful degradation correctly. However, ifgetLoginCmd()is invoked multiple times per SSH session, the repeated execution oflogin -VinsideisUtilLinuxLogin()could add measurable latency. Consider profiling the SSH session flow to determine if caching the detection result is warranted.
| cmd := exec.Command("login", "-V") | ||
| output, err := cmd.CombinedOutput() | ||
| if err != nil { | ||
| return false | ||
| } | ||
| // util-linux login includes "util-linux" in version output | ||
| // Example: "login from util-linux 2.41" | ||
| return strings.Contains(string(output), "util-linux") | ||
| } | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Add timeout context to prevent potential hangs.
The function executes an external command without a timeout. If login -V hangs or is unusually slow, it will block SSH session establishment indefinitely.
Apply this diff to add a timeout:
func (s *Server) isUtilLinuxLogin() bool {
- cmd := exec.Command("login", "-V")
+ ctx, cancel := context.WithTimeout(context.Background(), 2*time.Second)
+ defer cancel()
+
+ cmd := exec.CommandContext(ctx, "login", "-V")
output, err := cmd.CombinedOutput()
if err != nil {
return false
}
// util-linux login includes "util-linux" in version output
// Example: "login from util-linux 2.41"
return strings.Contains(string(output), "util-linux")
}You'll also need to add these imports at the top of the file:
import (
+ "context"
"errors"
"fmt"
"net"
"net/netip"
"os"
"os/exec"
"os/user"
"regexp"
"runtime"
"strconv"
"strings"
+ "time"
"github.com/gliderlabs/ssh"
log "github.com/sirupsen/logrus"
)Consider caching the detection result for better performance.
The login implementation doesn't change during the process lifetime. If this function is called multiple times, caching the result would eliminate redundant executions.
Apply this diff to cache the result:
+var (
+ utilLinuxLoginOnce sync.Once
+ utilLinuxLoginResult bool
+)
+
func (s *Server) isUtilLinuxLogin() bool {
+ utilLinuxLoginOnce.Do(func() {
+ ctx, cancel := context.WithTimeout(context.Background(), 2*time.Second)
+ defer cancel()
+
+ cmd := exec.CommandContext(ctx, "login", "-V")
+ output, err := cmd.CombinedOutput()
+ if err != nil {
+ utilLinuxLoginResult = false
+ return
+ }
+ // util-linux login includes "util-linux" in version output
+ // Example: "login from util-linux 2.41"
+ utilLinuxLoginResult = strings.Contains(string(output), "util-linux")
+ })
+ return utilLinuxLoginResult
- ctx, cancel := context.WithTimeout(context.Background(), 2*time.Second)
- defer cancel()
-
- cmd := exec.CommandContext(ctx, "login", "-V")
- output, err := cmd.CombinedOutput()
- if err != nil {
- return false
- }
- // util-linux login includes "util-linux" in version output
- // Example: "login from util-linux 2.41"
- return strings.Contains(string(output), "util-linux")
}You'll also need to add the sync import:
import (
"context"
"errors"
"fmt"
"net"
"net/netip"
"os"
"os/exec"
"os/user"
"regexp"
"runtime"
"strconv"
"strings"
+ "sync"
"time"
"github.com/gliderlabs/ssh"
log "github.com/sirupsen/logrus"
)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| func (s *Server) isUtilLinuxLogin() bool { | |
| cmd := exec.Command("login", "-V") | |
| output, err := cmd.CombinedOutput() | |
| if err != nil { | |
| return false | |
| } | |
| // util-linux login includes "util-linux" in version output | |
| // Example: "login from util-linux 2.41" | |
| return strings.Contains(string(output), "util-linux") | |
| } | |
| func (s *Server) isUtilLinuxLogin() bool { | |
| ctx, cancel := context.WithTimeout(context.Background(), 2*time.Second) | |
| defer cancel() | |
| cmd := exec.CommandContext(ctx, "login", "-V") | |
| output, err := cmd.CombinedOutput() | |
| if err != nil { | |
| return false | |
| } | |
| // util-linux login includes "util-linux" in version output | |
| // Example: "login from util-linux 2.41" | |
| return strings.Contains(string(output), "util-linux") | |
| } |
🤖 Prompt for AI Agents
In client/ssh/server/userswitching_unix.go around lines 110-119, the call to
exec.Command("login", "-V") can hang because it has no timeout and the detection
result is recomputed on every call; fix by creating a context with timeout
(e.g., context.WithTimeout(ctx, 2*time.Second)), run the command with
exec.CommandContext, defer cancel, and handle context deadline errors by
returning false on timeout; additionally, cache the boolean result using a
package-level variable and sync.Once (or a mutex and a cached value) so the
detection runs only once per process; add the imports "context", "time", and
"sync" at the top of the file.
login (Debian 13, Ubuntu 25.10, etc)
afonsofrancof
changed the title
login (Debian 13, Ubuntu 25.10, etc)login (Debian 13, Ubuntu 25.10, etc)
fix(ssh): Remove unused import feat(ssh): Use `runuser` on any distro that uses login from util-linux
afonsofrancof
force-pushed
the
feature/debian-ssh
branch
from
4b61258 to
fb7a5eb
Compare
|
This PR appears to have fixed the issue. I started my testing on an Ubuntu 24.04 LTS VM which was using I have built the client from this PR and the issues appear to have been resolved, it now SSH's straight away with zero problems. I have also tested this PR on a 24.04 server with |
afonsofrancof
changed the title
login (Debian 13, Ubuntu 25.10, etc)runuser instead of login
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
client/ssh/server/userswitching_unix.go (2)
62-74: Update comment to reflect that we may use runuser or login, not just login.The comment still says “creates a Pty command using login”, but
getLoginCmdcan return eitherrunuserorloginon Unix. It’s worth updating the wording to avoid confusion for future maintainers.-// createUserSwitchPtyCommand creates a Pty command using login for privileged processes +// createUserSwitchPtyCommand creates a Pty command using the platform's user-switching +// mechanism (e.g. runuser/login) for privileged processes
77-109: Make remote address parsing best-effort so user switching still works if parsing fails.
netip.ParseAddrPort(remoteAddr.String())is currently a hard precondition for all platforms, even though the parsed host is only used to feed-hintologin. If for any reasonremoteAddr.String()is not inAddrPortform, user switching will fail entirely—even on Linux where the preferredrunuserpath doesn’t need the host.Consider treating host extraction as best-effort and only using it when available, rather than returning an error:
-func (s *Server) getLoginCmd(username string, remoteAddr net.Addr) (string, []string, error) { - addrPort, err := netip.ParseAddrPort(remoteAddr.String()) - if err != nil { - return "", nil, fmt.Errorf("parse remote address: %w", err) - } - - switch runtime.GOOS { +func (s *Server) getLoginCmd(username string, remoteAddr net.Addr) (string, []string, error) { + var host string + if remoteAddr != nil { + if addrPort, err := netip.ParseAddrPort(remoteAddr.String()); err == nil { + host = addrPort.Addr().String() + } else { + log.Warnf("failed to parse remote address %q: %v", remoteAddr.String(), err) + } + } + + switch runtime.GOOS { case "linux": - // Try runuser first + // Try runuser first runuserPath, err := exec.LookPath("runuser") if err == nil { return runuserPath, []string{"-l", username}, nil } - // Fall back to login + // Fall back to login loginPath, err := exec.LookPath("login") if err == nil { - return loginPath, []string{"-f", username, "-h", addrPort.Addr().String(), "-p"}, nil + args := []string{"-f", username} + if host != "" { + args = append(args, "-h", host) + } + args = append(args, "-p") + return loginPath, args, nil } return "", nil, fmt.Errorf("neither runuser nor login commands available: %w", err) case "darwin", "freebsd", "openbsd", "netbsd", "dragonfly": loginPath, err := exec.LookPath("login") if err == nil { - return loginPath, []string{"-fp", "-h", addrPort.Addr().String(), username}, nil + args := []string{"-fp"} + if host != "" { + args = append(args, "-h", host) + } + args = append(args, username) + return loginPath, args, nil } return "", nil, fmt.Errorf("login command not available: %w", err)This keeps the existing behaviour when parsing succeeds, but avoids failing the entire SSH session setup when only the remote host decoration is unavailable.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
client/ssh/server/userswitching_unix.go(3 hunks)
🔇 Additional comments (1)
client/ssh/server/userswitching_unix.go (1)
199-210: LGTM: call-site rename matches the more generic user-switching helper.The switch from
createPtyLoginCommandtocreateUserSwitchPtyCommandat the call site aligns with the new helper name and the broader “runuser or login” behaviour, without changing the decision logic aroundUsedFallback.
|
@alexmoras I changed the PR to always use runuser if available on linux systems. I know this works on recent versions, but could you try it on your Ubuntu 24.04 again, just to make sure there are no regressions? Sorry for all the trouble. (I squashed some commits, so you might have to reclone or delete the commits and then pull from my branch) |
|
I can confirm that it doesn't have any regression on Debian 12 (which uses shadow-login) |
|
I changed the branch name and it closed the PR... perfect 😅 |
|
|
Likewise, no regression introduced on 24.04 LTS or Debian 11 (using |
|
Nice, it should be ready to merge then |
|
Hi @afonsofrancof, thanks for the PR! Unfortunately, it has one flaw: The proper way should be to try So we need an intermediate process that can check righ after a (manual, not through the Go stdlib) |
|
Hey @lixmal ! Is there anyone working on this ATM? Could I implement this? Thanks :) |
|
Nobody works on it right now; that's more like a mid-term fix. As a short-term fix, omitting the |
|
That won't work for distros that use util-linux's login binary, like debian 13 and Ubuntu 25.10. Login does not allow that behavior, so it can't be used at all in those cases |
|
I have tested it on Debian 13, and it works for me. Although it seems there is a race condition that fails something like 4 of 5 attempts, at least for me. |
|
As above, exact same behaviour for me. Any system running |
|
Testing with Given that this is affecting so many users, would it be a beneficial temporary measure to place the |
|
@alexmoras I tried it with I am trying to understand and work on the solution that lixmal suggested, but it will take a bit of time. I agree that this should probably be shipped, as it fixes a lot of people's workflows, and we could keep working on a more permanent fix meanwhile. EDIT: It should probably be shipped with |
|
A better fix was implemented in #4900. |
4 participants
Describe your changes
Some distros use util-linux's
loginbinary, which doesn't work with NetBird's SSH.Debian, for example, started using this binary recently in Debian 13, which broke NetBird's SSH.
runuseris the recommended utility for this use case and is now tried first on all Linux systems.If
runuserdoesn't exist, it falls back to the standard login method.I also removed the special handling for arch-linux, since it just uses
runuserfor that distro now.Issue ticket number and link
#4869
Stack
Checklist
Documentation
Select exactly one: