From ccf8f43cb1c4a5be497f9d725ae673ace2bdd5c7 Mon Sep 17 00:00:00 2001 From: Viktor Liu <17948409+lixmal@users.noreply.github.com> Date: Wed, 26 Aug 2026 03:15:16 +0900 Subject: [PATCH] [client] Ask the OS for privileges when a guarded SSH setting is changed (#7066) --- .goreleaser_ui.yaml | 10 + client/internal/daemonaddr/identity.go | 17 + client/internal/daemonaddr/identity_test.go | 29 ++ client/internal/elevate/elevate.go | 74 ++++ client/internal/elevate/output.go | 18 + client/internal/elevate/output_test.go | 21 + client/internal/elevate/run_darwin.go | 359 ++++++++++++++++++ client/internal/elevate/run_darwin_test.go | 111 ++++++ client/internal/elevate/run_unix.go | 117 ++++++ client/internal/elevate/run_unix_test.go | 110 ++++++ client/internal/elevate/run_unsupported.go | 19 + client/internal/elevate/run_windows.go | 187 +++++++++ client/internal/elevate/trusted.go | 40 ++ .../internal/elevate/trusted_group_darwin.go | 10 + client/internal/elevate/trusted_group_unix.go | 9 + client/internal/elevate/trusted_unix.go | 119 ++++++ client/internal/elevate/trusted_unix_test.go | 148 ++++++++ client/internal/elevate/trusted_windows.go | 215 +++++++++++ .../internal/elevate/trusted_windows_test.go | 126 ++++++ client/internal/getent/cgo_unix.go | 36 ++ client/internal/getent/getent.go | 6 + .../server => internal/getent}/getent_test.go | 105 +++-- client/internal/getent/nocgo_unix.go | 110 ++++++ client/internal/getent/unix.go | 224 +++++++++++ .../getent/unix_test.go} | 248 +++++++----- client/internal/getent/windows.go | 36 ++ client/internal/ipcauth/privileged.go | 16 + client/ssh/server/getent_cgo_unix.go | 24 -- client/ssh/server/getent_nocgo_unix.go | 74 ---- client/ssh/server/getent_unix.go | 127 ------- client/ssh/server/getent_windows.go | 26 -- client/ssh/server/shell.go | 8 +- client/ssh/server/shell_unix_test.go | 94 +++++ client/ssh/server/user_utils.go | 6 +- client/ssh/server/userswitching_unix.go | 4 +- client/ui/build/linux/netbird.desktop | 3 +- .../linux/polkit/io.netbird.settings.policy | 47 +++ .../frontend/src/contexts/SettingsContext.tsx | 100 ++++- client/ui/frontend/src/hooks/usePrivilege.ts | 2 +- .../src/modules/settings/SettingsSSH.tsx | 157 ++++++-- client/ui/i18n/locales/de/common.json | 25 +- client/ui/i18n/locales/en/common.json | 28 +- client/ui/i18n/locales/es/common.json | 25 +- client/ui/i18n/locales/fr/common.json | 25 +- client/ui/i18n/locales/hu/common.json | 25 +- client/ui/i18n/locales/it/common.json | 25 +- client/ui/i18n/locales/ja/common.json | 19 +- client/ui/i18n/locales/pt/common.json | 25 +- client/ui/i18n/locales/ru/common.json | 25 +- client/ui/i18n/locales/zh-CN/common.json | 25 +- client/ui/main.go | 9 + client/ui/privileged_settings.go | 27 ++ client/ui/services/guarded.go | 231 +++++++++++ client/ui/services/guarded_test.go | 355 +++++++++++++++++ client/ui/services/oneshot.go | 239 ++++++++++++ client/ui/services/oneshot_test.go | 151 ++++++++ client/ui/services/settings.go | 147 ++++++- 57 files changed, 4075 insertions(+), 523 deletions(-) create mode 100644 client/internal/daemonaddr/identity.go create mode 100644 client/internal/daemonaddr/identity_test.go create mode 100644 client/internal/elevate/elevate.go create mode 100644 client/internal/elevate/output.go create mode 100644 client/internal/elevate/output_test.go create mode 100644 client/internal/elevate/run_darwin.go create mode 100644 client/internal/elevate/run_darwin_test.go create mode 100644 client/internal/elevate/run_unix.go create mode 100644 client/internal/elevate/run_unix_test.go create mode 100644 client/internal/elevate/run_unsupported.go create mode 100644 client/internal/elevate/run_windows.go create mode 100644 client/internal/elevate/trusted.go create mode 100644 client/internal/elevate/trusted_group_darwin.go create mode 100644 client/internal/elevate/trusted_group_unix.go create mode 100644 client/internal/elevate/trusted_unix.go create mode 100644 client/internal/elevate/trusted_unix_test.go create mode 100644 client/internal/elevate/trusted_windows.go create mode 100644 client/internal/elevate/trusted_windows_test.go create mode 100644 client/internal/getent/cgo_unix.go create mode 100644 client/internal/getent/getent.go rename client/{ssh/server => internal/getent}/getent_test.go (53%) create mode 100644 client/internal/getent/nocgo_unix.go create mode 100644 client/internal/getent/unix.go rename client/{ssh/server/getent_unix_test.go => internal/getent/unix_test.go} (63%) create mode 100644 client/internal/getent/windows.go delete mode 100644 client/ssh/server/getent_cgo_unix.go delete mode 100644 client/ssh/server/getent_nocgo_unix.go delete mode 100644 client/ssh/server/getent_unix.go delete mode 100644 client/ssh/server/getent_windows.go create mode 100644 client/ssh/server/shell_unix_test.go create mode 100644 client/ui/build/linux/polkit/io.netbird.settings.policy create mode 100644 client/ui/privileged_settings.go create mode 100644 client/ui/services/guarded.go create mode 100644 client/ui/services/guarded_test.go create mode 100644 client/ui/services/oneshot.go create mode 100644 client/ui/services/oneshot_test.go diff --git a/.goreleaser_ui.yaml b/.goreleaser_ui.yaml index 1c5bc41ac..24903188f 100644 --- a/.goreleaser_ui.yaml +++ b/.goreleaser_ui.yaml @@ -92,6 +92,11 @@ nfpms: dst: /usr/share/applications/org.wails.netbird.desktop - src: client/ui/build/appicon.png dst: /usr/share/pixmaps/netbird.png + # Names the polkit action for the elevation prompt the app raises when an + # unprivileged user changes a privileged setting; without it the dialog + # shows a raw command line. + - src: client/ui/build/linux/polkit/io.netbird.settings.policy + dst: /usr/share/polkit-1/actions/io.netbird.settings.policy dependencies: - netbird (>= 0.75.0) - libgtk-4-1 (>= 4.14) @@ -116,6 +121,11 @@ nfpms: dst: /usr/share/applications/org.wails.netbird.desktop - src: client/ui/build/appicon.png dst: /usr/share/pixmaps/netbird.png + # Names the polkit action for the elevation prompt the app raises when an + # unprivileged user changes a privileged setting; without it the dialog + # shows a raw command line. + - src: client/ui/build/linux/polkit/io.netbird.settings.policy + dst: /usr/share/polkit-1/actions/io.netbird.settings.policy dependencies: - netbird >= 0.75.0 - (gtk4 >= 4.14 or libgtk-4-1 >= 4.14) diff --git a/client/internal/daemonaddr/identity.go b/client/internal/daemonaddr/identity.go new file mode 100644 index 000000000..b6af515b7 --- /dev/null +++ b/client/internal/daemonaddr/identity.go @@ -0,0 +1,17 @@ +package daemonaddr + +import "strings" + +// CarriesIdentity reports whether the control channel at addr conveys the +// connecting process's identity to the daemon. A Unix socket carries peer +// credentials and a named pipe carries the client's token. Nothing else does, TCP +// included, and there the daemon can authorize a privileged operation for nobody +// at all: see ResolveDaemonAddr, which says as much to anyone still reaching the +// Windows daemon on the address it served before it had a pipe. +// +// A client uses this to tell whether becoming privileged would get it anywhere. +// It answers from the scheme and nothing else, so an address it does not +// recognise counts as carrying no identity. +func CarriesIdentity(addr string) bool { + return strings.HasPrefix(addr, "unix://") || strings.HasPrefix(addr, pipeScheme) +} diff --git a/client/internal/daemonaddr/identity_test.go b/client/internal/daemonaddr/identity_test.go new file mode 100644 index 000000000..2808b5017 --- /dev/null +++ b/client/internal/daemonaddr/identity_test.go @@ -0,0 +1,29 @@ +package daemonaddr + +import ( + "testing" + + "github.com/stretchr/testify/assert" +) + +func TestCarriesIdentity(t *testing.T) { + tests := []struct { + addr string + want bool + }{ + {"unix:///var/run/netbird.sock", true}, + {"unix:///var/run/netbird/default.sock", true}, + {"npipe://netbird", true}, + {`npipe://\\.\pipe\ProtectedPrefix\Administrators\netbird`, true}, + {"tcp://127.0.0.1:41731", false}, + {"tcp://localhost:41731", false}, + {"", false}, + {"/var/run/netbird.sock", false}, + } + + for _, tt := range tests { + t.Run(tt.addr, func(t *testing.T) { + assert.Equal(t, tt.want, CarriesIdentity(tt.addr), "address %q", tt.addr) + }) + } +} diff --git a/client/internal/elevate/elevate.go b/client/internal/elevate/elevate.go new file mode 100644 index 000000000..aa5a5da78 --- /dev/null +++ b/client/internal/elevate/elevate.go @@ -0,0 +1,74 @@ +// Package elevate re-runs this very executable under the operating system's own +// privilege-elevation mechanism and waits for it to finish. +// +// It exists so that a change the daemon restricts to root/administrator can be +// authorized from the GUI, by the user, at the moment they ask for it: Windows +// shows the UAC consent dialog, macOS the system authentication dialog, and +// Linux/FreeBSD the session's polkit agent. The credentials, where any are +// asked for, are collected by the operating system and never pass through +// NetBird. +// +// What the elevated process then does is the caller's business: it is the same +// binary, in a one-shot mode, and it is authorized by the daemon exactly like +// any other privileged caller, from the identity the kernel reports on the +// control channel. Nothing here grants privilege, and the daemon gains no new +// way to be talked into something: elevation only changes who is calling it. +package elevate + +import ( + "context" + "errors" + + log "github.com/sirupsen/logrus" +) + +// AppliedMarker is what the elevated process prints on standard output once it has +// done what it was run for. +// +// macOS's AuthorizationExecuteWithPrivileges reports no exit status and does not +// say which process it started, so there this line is the only evidence that the +// change was applied. The other platforms have an exit code and ignore it. +const AppliedMarker = "netbird-elevated: applied" + +var ( + // ErrDeclined reports that the user dismissed the prompt or did not + // authenticate. Nothing happened and nothing is wrong: a caller undoes its + // optimistic update and stays quiet. + ErrDeclined = errors.New("authorization declined") + + // ErrUnavailable reports that this host has no elevation mechanism we can + // drive: no polkit on a Unix desktop, or an executable we decline to run as + // root. A caller falls back to telling the user which command to run. + ErrUnavailable = errors.New("no privilege elevation mechanism available") +) + +// Run runs this executable with args under the platform's elevation mechanism +// and waits for it to exit. A non-zero exit is returned as an error, so the +// caller can treat a completed Run as the operation having succeeded. +// +// The args are the caller's own command line, so they cross no privilege +// boundary: only a user who has just authenticated as an administrator can get +// them run at all. +func Run(ctx context.Context, args ...string) error { + self, err := trustedSelf() + if err != nil { + return err + } + return run(ctx, self, args) +} + +// Available reports whether Run has a mechanism to use on this host, so a caller +// can offer the prompt only when there is one and otherwise fall back to +// guidance the user can act on. It answers from what is installed, not from what +// the user is allowed to do: an administrator's password may still be required +// and may still not be given, which is ErrDeclined from Run. +func Available() bool { + if _, err := trustedSelf(); err != nil { + // Worth a line: this is also what a build run from a group-writable + // directory hits, and there is nothing in the UI to say why the offer is + // missing. + log.Debugf("not offering privilege elevation: %v", err) + return false + } + return mechanismAvailable() +} diff --git a/client/internal/elevate/output.go b/client/internal/elevate/output.go new file mode 100644 index 000000000..6e1646bd3 --- /dev/null +++ b/client/internal/elevate/output.go @@ -0,0 +1,18 @@ +package elevate + +import "strings" + +// noOutput stands in for a process that said nothing, so that a report of what it +// said still reads as a sentence. +const noOutput = "no output" + +func firstLine(s string) string { + s = strings.TrimSpace(s) + if s == "" { + return noOutput + } + if i := strings.IndexByte(s, '\n'); i >= 0 { + return s[:i] + } + return s +} diff --git a/client/internal/elevate/output_test.go b/client/internal/elevate/output_test.go new file mode 100644 index 000000000..3faacf53a --- /dev/null +++ b/client/internal/elevate/output_test.go @@ -0,0 +1,21 @@ +package elevate + +import ( + "testing" + + "github.com/stretchr/testify/assert" +) + +func TestFirstLine(t *testing.T) { + tests := []struct{ in, want string }{ + {in: "", want: noOutput}, + {in: " \n ", want: noOutput}, + {in: "one line", want: "one line"}, + {in: "first\nsecond", want: "first"}, + {in: "\nsecond\n", want: "second"}, + } + + for _, tt := range tests { + assert.Equal(t, tt.want, firstLine(tt.in), "input %q", tt.in) + } +} diff --git a/client/internal/elevate/run_darwin.go b/client/internal/elevate/run_darwin.go new file mode 100644 index 000000000..6b0e4fc0d --- /dev/null +++ b/client/internal/elevate/run_darwin.go @@ -0,0 +1,359 @@ +package elevate + +import ( + "context" + "errors" + "fmt" + "os" + "runtime" + "strings" + "sync" + "syscall" + "unsafe" + + "github.com/ebitengine/purego" + log "github.com/sirupsen/logrus" +) + +// Authorization Services, reached through purego rather than cgo so the released +// binaries keep building with CGO_ENABLED=0. +// +// The prompt belongs to this process, which is what makes it carry the +// application's name and our own explanation. Going through osascript instead puts +// the very same trampoline behind a dialog attributed to osascript, and means +// handing a shell a command line to re-parse. +// +// # On AuthorizationExecuteWithPrivileges +// +// It is deprecated, and Apple's guidance (Quinn, "BSD Privilege Escalation on +// macOS", developer.apple.com/forums/thread/708765) is "while it still works, it's +// been deprecated for many years. Do not use it in a widely distributed product." +// It is used here anyway, knowingly, because the alternatives Apple offers are for +// *obtaining* ongoing privileges — an installer package, SMAppService, SMJobBless — +// and NetBird already has what they would install: a launchd daemon running as +// root. What is missing is only a way for an unprivileged client to ask it to act. +// +// The way to that without a deprecated call is to authorize the client instead of +// elevating one: the app takes the right with AuthorizationCreate, passes the +// AuthorizationExternalForm to the daemon, and the daemon checks it with +// AuthorizationCopyRights before acting — none of which is deprecated. It is the +// better design and it is where this should end up. It also means the daemon +// accepting an authorization over its control socket, which is a new way to be +// asked for privileged work and wants reviewing as such, so it is deliberately not +// bundled in with the rest of this. +// +// Until then, three things keep the deprecation from being a trap. Every symbol is +// resolved with an error rather than a panic, so a macOS that has dropped this +// function leaves the app offering the user a command instead of crashing on the +// way to a prompt. A failure to run the tool is reported as ErrUnavailable, so the +// fallback is the same one an agent-less Linux session gets. And the whole path +// runs under guard, which turns a panic out of the FFI layer into that same +// fallback. +// +// The trampoline passes on the environment it was given, so what it starts as root +// must be an executable this user's peers cannot influence: that is what +// trustedSelf refuses, and what signing the binary settles for the loader. + +const ( + securityFramework = "/System/Library/Frameworks/Security.framework/Security" + libSystem = "/usr/lib/libSystem.B.dylib" + + // trampoline is what the framework hands the tool to. Present on every macOS, + // and worth confirming before offering a prompt rather than mid-prompt. + trampoline = "/usr/libexec/security_authtrampoline" +) + +// rightExecute is the right an administrator holds, and what +// AuthorizationExecuteWithPrivileges requires of us. +const rightExecute = "system.privilege.admin" + +// promptKey is kAuthorizationEnvironmentPrompt, which puts a sentence of ours above +// the system's in the dialog. It is about the change rather than the mechanism. +const ( + promptKey = "prompt" + promptText = "NetBird needs to change a setting that grants SSH access to this computer." +) + +// OSStatus values from SecBase.h that mean something to us; anything else is +// reported as it comes. +const ( + errAuthorizationSuccess = 0 + errAuthorizationDenied = -60005 + errAuthorizationCanceled = -60006 + errAuthorizationInteractionNotAllowed = -60007 + errAuthorizationToolExecuteFailure = -60031 + errAuthorizationToolEnvironmentError = -60032 +) + +// AuthorizationFlags from Authorization.h. +const ( + flagDefaults = 0 + flagInteractionAllowed = 1 << 0 + flagExtendRights = 1 << 1 + flagDestroyRights = 1 << 3 + flagPreAuthorize = 1 << 4 +) + +// authorizationItem mirrors AuthorizationItem: a name, and a value the name gives +// meaning to. 32 bytes on both amd64 and arm64. +type authorizationItem struct { + name *byte + valueLength uintptr + value unsafe.Pointer + // flags is reserved by the API and always zero. Declared because the layout + // is the contract: without it the struct is 24 bytes where C reads 32. + flags uint32 //nolint:unused // part of the C layout +} + +// authorizationItemSet mirrors AuthorizationItemSet, which serves as both an +// AuthorizationRights and an AuthorizationEnvironment. +type authorizationItemSet struct { + count uint32 + items *authorizationItem +} + +var ( + authorizationCreate func(rights, environment *authorizationItemSet, flags uint32, authorization *uintptr) int32 + authorizationExecuteWithPrivileges func(authorization uintptr, pathToTool string, options uint32, arguments *uintptr, communicationsPipe *uintptr) int32 + authorizationFree func(authorization uintptr, flags uint32) int32 + fileno func(stream uintptr) int32 + fclose func(stream uintptr) int32 + + loadOnce sync.Once + loadErr error +) + +// load resolves the functions once. A framework that cannot be opened, or a symbol +// that is no longer there, leaves the host without a mechanism rather than taking +// the process down with it: see the note on deprecation above. +func load() error { + loadOnce.Do(func() { loadErr = guard("loading Security.framework", resolve) }) + return loadErr +} + +// guard turns a panic out of the FFI layer into an error, so an API that has +// changed under us costs the user a prompt rather than the window they were +// clicking in. purego panics on a signature it cannot map, and this is the one +// place in the client that calls a deprecated system function. +// +// It catches Go panics, which is what purego raises. A fault inside the framework +// itself is not a panic and not recoverable; the layout the tests pin down is what +// stands between us and that. +func guard(what string, fn func() error) (err error) { + defer func() { + r := recover() + if r == nil { + return + } + log.Errorf("%s panicked: %v", what, r) + err = fmt.Errorf("%w: %s: %v", ErrUnavailable, what, r) + }() + return fn() +} + +func resolve() error { + security, err := purego.Dlopen(securityFramework, purego.RTLD_LAZY|purego.RTLD_GLOBAL) + if err != nil { + return fmt.Errorf("open %s: %w", securityFramework, err) + } + system, err := purego.Dlopen(libSystem, purego.RTLD_LAZY|purego.RTLD_GLOBAL) + if err != nil { + return fmt.Errorf("open %s: %w", libSystem, err) + } + + // purego.RegisterLibFunc panics on a symbol it cannot find, which is not how a + // deprecated function's disappearance should reach the user. + for _, fn := range []struct { + ptr any + handle uintptr + name string + }{ + {&authorizationCreate, security, "AuthorizationCreate"}, + {&authorizationExecuteWithPrivileges, security, "AuthorizationExecuteWithPrivileges"}, + {&authorizationFree, security, "AuthorizationFree"}, + {&fileno, system, "fileno"}, + {&fclose, system, "fclose"}, + } { + symbol, err := purego.Dlsym(fn.handle, fn.name) + if err != nil { + return fmt.Errorf("resolve %s: %w", fn.name, err) + } + if symbol == 0 { + return fmt.Errorf("resolve %s: not present on this system", fn.name) + } + purego.RegisterFunc(fn.ptr, symbol) + } + return nil +} + +// run asks the system to run self as root: first for the right, which is what puts +// up the authentication dialog and collects the password or takes the Touch ID, +// then for the tool. The credentials go to the system's authorization trampoline +// and never to us. +// +// The context bounds only our own waiting; the dialog belongs to the system and +// closes when the user answers it. +func run(ctx context.Context, self string, args []string) error { + if err := load(); err != nil { + return fmt.Errorf("%w: %v", ErrUnavailable, err) + } + + return guard("asking for privileges", func() error { + authorization, err := authorize() + if err != nil { + return err + } + defer authorizationFree(authorization, flagDestroyRights) + + return execute(ctx, authorization, self, args) + }) +} + +func mechanismAvailable() bool { + if err := load(); err != nil { + return false + } + info, err := os.Stat(trampoline) + return err == nil && !info.IsDir() +} + +// authorize obtains the right, prompting for it. A dismissed dialog comes back as +// errAuthorizationCanceled and a password given up on as errAuthorizationDenied; +// both are the user's answer rather than a failure. +func authorize() (uintptr, error) { + var pinner runtime.Pinner + defer pinner.Unpin() + + rights := itemSet(&pinner, authorizationItem{name: cString(&pinner, rightExecute)}) + environment := itemSet(&pinner, promptItem(&pinner)) + + var authorization uintptr + status := authorizationCreate(rights, environment, + flagDefaults|flagInteractionAllowed|flagPreAuthorize|flagExtendRights, &authorization) + + switch status { + case errAuthorizationSuccess: + return authorization, nil + case errAuthorizationCanceled, errAuthorizationDenied: + return 0, ErrDeclined + case errAuthorizationInteractionNotAllowed: + // Nowhere to put a dialog, so there is nobody to ask: a launch daemon, or + // a session with no window server. + return 0, fmt.Errorf("%w: this session cannot show an authorization prompt", ErrUnavailable) + default: + return 0, fmt.Errorf("request %s: OSStatus %d", rightExecute, status) + } +} + +// execute runs the tool with the right in hand and waits for it by reading the pipe +// it is given until the tool closes it. +// +// AuthorizationExecuteWithPrivileges reports no exit status and does not say what +// process it started, which is why the one-shot says so itself: what it prints is +// the only evidence that the change was applied. +func execute(ctx context.Context, authorization uintptr, self string, args []string) error { + var pinner runtime.Pinner + defer pinner.Unpin() + + argv := make([]uintptr, 0, len(args)+1) + for _, arg := range args { + argv = append(argv, uintptr(unsafe.Pointer(cString(&pinner, arg)))) + } + argv = append(argv, 0) + pinner.Pin(&argv[0]) + + var pipe uintptr + status := authorizationExecuteWithPrivileges(authorization, self, flagDefaults, &argv[0], &pipe) + switch status { + case errAuthorizationSuccess: + case errAuthorizationCanceled: + return ErrDeclined + case errAuthorizationToolExecuteFailure, errAuthorizationToolEnvironmentError: + // The right was granted and the tool still did not start. Nothing the user + // can do about it from here, so point them at the command instead. + return fmt.Errorf("%w: the system would not run %s elevated (OSStatus %d)", ErrUnavailable, self, status) + default: + return fmt.Errorf("run %s elevated: OSStatus %d", self, status) + } + + out, err := readPipe(ctx, pipe) + if err != nil { + return err + } + return checkApplied(out) +} + +// checkApplied reads the one-shot's report, which stands in for the exit status +// there is no way to ask for here. A run that said nothing did not apply the +// change, whatever else went on. +func checkApplied(out string) error { + if !strings.Contains(out, AppliedMarker) { + return fmt.Errorf("elevated netbird did not report the change as applied: %s", firstLine(out)) + } + return nil +} + +// readPipe drains the tool's output, which ends when the tool exits and is +// therefore also how we wait for it. +func readPipe(ctx context.Context, pipe uintptr) (string, error) { + if pipe == 0 { + return "", nil + } + defer fclose(pipe) + + fd := int(fileno(pipe)) + if fd < 0 { + return "", nil + } + + var out strings.Builder + buf := make([]byte, 4096) + for { + if err := ctx.Err(); err != nil { + return out.String(), err + } + n, err := syscall.Read(fd, buf) + if n > 0 { + out.Write(buf[:n]) + } + switch { + case errors.Is(err, syscall.EINTR): + // A signal landed mid-read, which says nothing about the tool. + continue + case err != nil: + log.Debugf("read the elevated process's output: %v", err) + return out.String(), nil + case n <= 0: + // End of file: the tool closed the pipe, which is how it exiting + // reaches us. + return out.String(), nil + } + } +} + +// itemSet builds an AuthorizationItemSet over items, pinned for the call. +func itemSet(pinner *runtime.Pinner, items ...authorizationItem) *authorizationItemSet { + pinner.Pin(&items[0]) + set := &authorizationItemSet{count: uint32(len(items)), items: &items[0]} + pinner.Pin(set) + return set +} + +// promptItem is the environment entry carrying our sentence for the dialog. +func promptItem(pinner *runtime.Pinner) authorizationItem { + value := []byte(promptText) + pinner.Pin(&value[0]) + return authorizationItem{ + name: cString(pinner, promptKey), + valueLength: uintptr(len(value)), + value: unsafe.Pointer(&value[0]), + } +} + +// cString returns a NUL-terminated copy of s, pinned so the C side may hold it for +// the duration of the call. +func cString(pinner *runtime.Pinner, s string) *byte { + b := append([]byte(s), 0) + pinner.Pin(&b[0]) + return &b[0] +} diff --git a/client/internal/elevate/run_darwin_test.go b/client/internal/elevate/run_darwin_test.go new file mode 100644 index 000000000..f6c58c8cb --- /dev/null +++ b/client/internal/elevate/run_darwin_test.go @@ -0,0 +1,111 @@ +package elevate + +import ( + "errors" + "runtime" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// The framework has to load and the symbols have to resolve, or nothing else here +// means anything. +func TestSecurityFrameworkLoads(t *testing.T) { + require.NoError(t, load(), "Security.framework must open") + + for name, fn := range map[string]any{ + "AuthorizationCreate": authorizationCreate, + "AuthorizationExecuteWithPrivileges": authorizationExecuteWithPrivileges, + "AuthorizationFree": authorizationFree, + "fileno": fileno, + "fclose": fclose, + } { + assert.NotNil(t, fn, "%s must resolve", name) + } +} + +// A request with no interaction allowed exercises the whole call — the rights and +// environment structs, and the OSStatus that comes back — without a dialog anybody +// has to answer. What the system decides is its business; that it decides at all is +// what this asserts. +func TestAuthorizationCreateWithoutInteraction(t *testing.T) { + if err := load(); err != nil { + t.Skipf("Security.framework did not open: %v", err) + } + + var pinner runtime.Pinner + defer pinner.Unpin() + + rights := itemSet(&pinner, authorizationItem{name: cString(&pinner, rightExecute)}) + environment := itemSet(&pinner, promptItem(&pinner)) + require.EqualValues(t, 1, rights.count, "the rights struct layout must match the C one") + + var authorization uintptr + status := authorizationCreate(rights, environment, flagDefaults|flagExtendRights, &authorization) + + switch status { + case errAuthorizationSuccess: + // Credentials were already cached for this session. + authorizationFree(authorization, flagDestroyRights) + case errAuthorizationDenied, errAuthorizationInteractionNotAllowed: + // The expected answers when nobody may be asked. + default: + require.Failf(t, "unknown OSStatus", "AuthorizationCreate returned %d, want a status we recognise", status) + } +} + +// Asking with a right nobody has must not be mistaken for a declined prompt: the +// caller would report nothing at all. +func TestAuthorizeUnknownRightIsNotDeclined(t *testing.T) { + if err := load(); err != nil { + t.Skipf("Security.framework did not open: %v", err) + } + + var pinner runtime.Pinner + defer pinner.Unpin() + + rights := itemSet(&pinner, authorizationItem{name: cString(&pinner, "io.netbird.right.that.does.not.exist")}) + + var authorization uintptr + status := authorizationCreate(rights, nil, flagDefaults|flagExtendRights, &authorization) + if status == errAuthorizationSuccess { + authorizationFree(authorization, flagDestroyRights) + } + assert.NotEqual(t, int32(errAuthorizationSuccess), status, "a right that does not exist must not be granted") +} + +func TestMechanismAvailable(t *testing.T) { + assert.True(t, mechanismAvailable(), "the trampoline exists on every macOS") +} + +// The one-shot's report is what stands in for an exit status here, so a run that +// says nothing must not read as success. +func TestCheckApplied(t *testing.T) { + require.NoError(t, checkApplied(AppliedMarker+"\n"), "the report the one-shot prints") + require.NoError(t, checkApplied("some warning\n"+AppliedMarker+"\n"), "the report after other output") + + assert.Error(t, checkApplied(""), "a run that printed nothing did not apply the change") + assert.Error(t, checkApplied("dyld: library not loaded\n"), "output that is not the report") +} + +// A panic out of the FFI layer has to reach the caller as "no mechanism", which is +// the outcome that offers the user the command instead of taking the window down. +func TestGuardTurnsAPanicIntoUnavailable(t *testing.T) { + err := guard("pretending to call something", func() error { + panic("purego: signature it cannot map") + }) + + require.ErrorIs(t, err, ErrUnavailable, "a panic must read as a missing mechanism") + assert.Contains(t, err.Error(), "pretending to call something", "what panicked") +} + +// guard wraps every darwin path, so what a caller switches on has to survive it. +func TestGuardPassesErrorsThrough(t *testing.T) { + sentinel := errors.New("the call itself failed") + assert.ErrorIs(t, guard("calling", func() error { return sentinel }), sentinel, + "the error it was given") + assert.ErrorIs(t, guard("calling", func() error { return ErrDeclined }), ErrDeclined, + "a declined prompt stays declined") + assert.NoError(t, guard("calling", func() error { return nil }), "a call that worked") +} diff --git a/client/internal/elevate/run_unix.go b/client/internal/elevate/run_unix.go new file mode 100644 index 000000000..b2de09a49 --- /dev/null +++ b/client/internal/elevate/run_unix.go @@ -0,0 +1,117 @@ +//go:build linux + +package elevate + +import ( + "context" + "errors" + "fmt" + "io" + "os" + "os/exec" + "strings" +) + +// pkexec exit codes that are about the authorization rather than about the program +// we asked it to run. The manual page reserves both. +const ( + // exitDismissed is returned when the user dismissed the authentication + // dialog. + exitDismissed = 126 + // exitNotAuthorized is returned when the authorization was not obtained. That + // covers the user saying no as well as pkexec having had nobody to ask: see + // noAgentMarkers. + exitNotAuthorized = 127 +) + +// exitNotAuthorized covers three different endings that only pkexec's own words +// tell apart, so they are matched here. Read with LC_ALL=C so the words are the +// ones written below. +// +// refusedMarker is a refusal: the user said no, gave up on the password, or holds +// an account that may not elevate at all. +const refusedMarker = "Not authorized" + +// noAgentMarkers say pkexec had no way to ask: no agent registered for the +// session, and no controlling terminal for the textual agent it falls back to. +var noAgentMarkers = []string{"authentication agent", "controlling terminal"} + +// run asks polkit to run self as root. pkexec hands the request to the session's +// polkit agent, which is what prompts and what collects any password; we see only +// its verdict. +// +// The environment is otherwise deliberately not passed through: pkexec clears it +// bar a small allowlist, and the one-shot needs nothing from it. +func run(ctx context.Context, self string, args []string) error { + pkexec, err := exec.LookPath("pkexec") + if err != nil { + return fmt.Errorf("%w: pkexec is not installed", ErrUnavailable) + } + + cmd := exec.CommandContext(ctx, pkexec, append([]string{self}, args...)...) + // C locale so pkexec's own diagnostics are the ones noAgentMarkers knows. + cmd.Env = append(os.Environ(), "LC_ALL=C") + var stderr strings.Builder + cmd.Stderr = &stderr + // The one-shot reports itself on stdout for macOS's sake, where there is no + // exit status to read. Here there is one, so that line is noise. + cmd.Stdout = io.Discard + + err = cmd.Run() + if err == nil { + return nil + } + + var exitErr *exec.ExitError + if !errors.As(err, &exitErr) { + return fmt.Errorf("run pkexec: %w", err) + } + + // Matched against everything pkexec said, reported as one line: a complaint + // that is not the first thing printed still has to be recognised, and reading + // it as a refusal would swallow it. + full := stderr.String() + out := firstLine(full) + + switch exitErr.ExitCode() { + case exitDismissed: + return ErrDeclined + case exitNotAuthorized: + return notAuthorized(full, out) + default: + return fmt.Errorf("elevated netbird exited with %d: %s", exitErr.ExitCode(), out) + } +} + +// notAuthorized sorts out the three endings pkexec reports as exitNotAuthorized. +// +// It also returns that code when the authorization succeeded and it then could +// not run the program, so a refusal has to be recognised rather than assumed: +// reading every one of these as "the user said no" would revert the control in +// silence on a host where elevation is broken. +func notAuthorized(full, out string) error { + switch { + case hasAny(full, noAgentMarkers): + return fmt.Errorf("%w: polkit had no way to ask: %s", ErrUnavailable, out) + case out == noOutput, strings.Contains(full, refusedMarker): + // The user said no, which needs no message; that an account barred from + // elevating altogether lands here too is why the reason is kept. + return fmt.Errorf("%w: %s", ErrDeclined, out) + default: + return fmt.Errorf("pkexec could not run elevated netbird: %s", out) + } +} + +func hasAny(s string, markers []string) bool { + for _, marker := range markers { + if strings.Contains(s, marker) { + return true + } + } + return false +} + +func mechanismAvailable() bool { + _, err := exec.LookPath("pkexec") + return err == nil +} diff --git a/client/internal/elevate/run_unix_test.go b/client/internal/elevate/run_unix_test.go new file mode 100644 index 000000000..c868f9a74 --- /dev/null +++ b/client/internal/elevate/run_unix_test.go @@ -0,0 +1,110 @@ +//go:build linux + +package elevate + +import ( + "context" + "fmt" + "os" + "path/filepath" + "strings" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// fakePkexec puts a pkexec on PATH that exits with the given code, so the +// mapping from polkit's exit codes onto our errors can be exercised without a +// polkit agent. +func fakePkexec(t *testing.T, exitCode int, stderr string) { + t.Helper() + + dir := t.TempDir() + script := fmt.Sprintf("#!/bin/sh\necho %s >&2\nexit %d\n", shellQuote(stderr), exitCode) + require.NoError(t, os.WriteFile(filepath.Join(dir, "pkexec"), []byte(script), 0o700), "write the fake pkexec") + t.Setenv("PATH", dir) +} + +func shellQuote(s string) string { + return "'" + strings.ReplaceAll(s, "'", `'\''`) + "'" +} + +func TestRunMapsPkexecExitCodes(t *testing.T) { + tests := []struct { + name string + exitCode int + stderr string + wantErr error + }{ + {name: "applied", exitCode: 0}, + { + name: "dialog dismissed", + exitCode: exitDismissed, + stderr: "Error executing command as another user: Request dismissed", + wantErr: ErrDeclined, + }, + { + // What a graphical agent reports for a cancelled prompt. Not a + // failure: the user was asked and answered. + name: "prompt cancelled", + exitCode: exitNotAuthorized, + stderr: "Error executing command as another user: Not authorized", + wantErr: ErrDeclined, + }, + { + // The same status, but pkexec never got to ask anybody. + name: "no agent and no terminal to fall back on", + exitCode: exitNotAuthorized, + stderr: "Error creating textual authentication agent: Error opening current controlling terminal for the process (`/dev/tty'): No such device or address", + wantErr: ErrUnavailable, + }, + { + // And the same status again once the authorization succeeded and + // pkexec could not run what it had been authorized to run. Reading + // that as a refusal would revert the control in silence on a host + // where elevation is broken. + name: "authorized but not runnable", + exitCode: exitNotAuthorized, + stderr: "Error executing command as another user: No such file or directory", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + fakePkexec(t, tt.exitCode, tt.stderr) + + err := run(context.Background(), "/nonexistent/netbird-ui", []string{"--flag"}) + switch { + case tt.wantErr != nil: + require.ErrorIs(t, err, tt.wantErr, "exit %d said %q", tt.exitCode, tt.stderr) + case tt.exitCode == 0: + require.NoError(t, err, "a pkexec that exited cleanly applied the change") + default: + require.Error(t, err, "exit %d said %q", tt.exitCode, tt.stderr) + assert.NotErrorIs(t, err, ErrDeclined, "not the user's answer") + assert.NotErrorIs(t, err, ErrUnavailable, "not a missing mechanism") + } + }) + } +} + +// An exit code that is not polkit's is the one-shot's own failure, and has to +// stay distinguishable from a declined prompt: the caller reports it. +func TestRunReportsOneShotFailure(t *testing.T) { + fakePkexec(t, 3, "the one-shot said no") + + err := run(context.Background(), "/nonexistent/netbird-ui", nil) + + require.Error(t, err, "a one-shot that failed is not a prompt that was answered") + assert.NotErrorIs(t, err, ErrDeclined, "not the user's answer") + assert.NotErrorIs(t, err, ErrUnavailable, "not a missing mechanism") +} + +func TestRunWithoutPkexecIsUnavailable(t *testing.T) { + t.Setenv("PATH", t.TempDir()) + + err := run(context.Background(), "/nonexistent/netbird-ui", nil) + require.ErrorIs(t, err, ErrUnavailable, "no pkexec means no mechanism") + assert.False(t, mechanismAvailable(), "mechanismAvailable without pkexec on PATH") +} diff --git a/client/internal/elevate/run_unsupported.go b/client/internal/elevate/run_unsupported.go new file mode 100644 index 000000000..d1daf3184 --- /dev/null +++ b/client/internal/elevate/run_unsupported.go @@ -0,0 +1,19 @@ +//go:build !windows && !darwin && !linux + +package elevate + +import "context" + +// run reports that this platform has no elevation prompt to drive. +// +// The desktop app is the only caller and is not built for any of these: mobile +// and WASM have no local user to ask, and the FreeBSD client ships without a UI. +// pkexec would be the mechanism there, and run_unix.go is what to widen if that +// changes. +func run(context.Context, string, []string) error { + return ErrUnavailable +} + +func mechanismAvailable() bool { + return false +} diff --git a/client/internal/elevate/run_windows.go b/client/internal/elevate/run_windows.go new file mode 100644 index 000000000..eef4c23ce --- /dev/null +++ b/client/internal/elevate/run_windows.go @@ -0,0 +1,187 @@ +package elevate + +import ( + "context" + "errors" + "fmt" + "runtime" + "unsafe" + + log "github.com/sirupsen/logrus" + "golang.org/x/sys/windows" +) + +const ( + // seeMaskNoCloseProcess keeps the started process's handle open in + // hProcess so we can wait for it. + seeMaskNoCloseProcess = 0x00000040 + // seeMaskNoAsync makes ShellExecuteExW finish its work before returning, + // which it must when the calling thread does not pump messages. + seeMaskNoAsync = 0x00000100 + // seeMaskFlagNoUI suppresses the shell's own error dialogs; the UAC consent + // dialog is not one of them and still appears. + seeMaskFlagNoUI = 0x00000400 + + // swHide: the one-shot has no window to show. + swHide = 0 +) + +// shellExecuteInfoW mirrors SHELLEXECUTEINFOW. The field order and Go's own +// padding match the C layout on both 386 and amd64. +type shellExecuteInfoW struct { + cbSize uint32 + fMask uint32 + hwnd windows.HWND + lpVerb *uint16 + lpFile *uint16 + lpParameters *uint16 + lpDirectory *uint16 + nShow int32 + hInstApp windows.Handle + lpIDList uintptr + lpClass *uint16 + hkeyClass windows.Handle + dwHotKey uint32 + hIconOrMonitor windows.Handle + hProcess windows.Handle +} + +var ( + shell32 = windows.NewLazySystemDLL("shell32.dll") + procShellExecuteEx = shell32.NewProc("ShellExecuteExW") +) + +// run starts self elevated with the "runas" verb, which is what raises the UAC +// consent dialog, and waits for it to finish. Windows decides whether consent is +// enough or an administrator's credentials are needed, and collects them itself. +func run(ctx context.Context, self string, args []string) error { + verb, err := windows.UTF16PtrFromString("runas") + if err != nil { + return fmt.Errorf("encode verb: %w", err) + } + file, err := windows.UTF16PtrFromString(self) + if err != nil { + return fmt.Errorf("encode %s: %w", self, err) + } + params, err := windows.UTF16PtrFromString(windows.ComposeCommandLine(args)) + if err != nil { + return fmt.Errorf("encode arguments: %w", err) + } + + info := shellExecuteInfoW{ + fMask: seeMaskNoCloseProcess | seeMaskNoAsync | seeMaskFlagNoUI, + hwnd: ownerWindow(), + lpVerb: verb, + lpFile: file, + lpParameters: params, + nShow: swHide, + } + info.cbSize = uint32(unsafe.Sizeof(info)) + + process, err := shellExecute(&info) + if err != nil { + return err + } + defer func() { + if err := windows.CloseHandle(process); err != nil { + log.Debugf("close elevated process handle: %v", err) + } + }() + + return waitForProcess(ctx, process) +} + +// shellExecute performs the call itself. ShellExecuteExW wants COM initialised on +// the calling thread, so the goroutine is pinned to one for the duration and COM +// is set up on it; an "already initialised, different mode" answer is fine, +// because then somebody else has done it for us. +func shellExecute(info *shellExecuteInfoW) (windows.Handle, error) { + runtime.LockOSThread() + defer runtime.UnlockOSThread() + + switch err := windows.CoInitializeEx(0, windows.COINIT_APARTMENTTHREADED); { + case err == nil, isHResult(err, windows.S_FALSE): + // Ours, or already initialised in the same mode: either way this call + // counts and has to be balanced. + defer windows.CoUninitialize() + case isHResult(err, windows.RPC_E_CHANGED_MODE): + // The thread is already in the other apartment model. ShellExecuteExW + // works there too, and there is nothing of ours to balance. + default: + return 0, fmt.Errorf("initialise COM: %w", err) + } + + ret, _, lastErr := procShellExecuteEx.Call(uintptr(unsafe.Pointer(info))) + if ret != 0 { + return info.hProcess, nil + } + + if errors.Is(lastErr, windows.ERROR_CANCELLED) { + return 0, ErrDeclined + } + return 0, fmt.Errorf("run elevated: %w", lastErr) +} + +// ownerWindow returns this process's foreground window, and 0 when the window in +// front belongs to somebody else or cannot be attributed. ShellExecuteExW takes it +// as the parent for the UI it raises, which is what keeps the consent dialog in +// front of the window the user was just clicking in instead of behind it. It is +// also what a remote-desktop session needs to place the dialog at all when the +// secure desktop is switched off. +func ownerWindow() windows.HWND { + hwnd := windows.GetForegroundWindow() + if hwnd == 0 { + return 0 + } + + var pid uint32 + if _, err := windows.GetWindowThreadProcessId(hwnd, &pid); err != nil { + log.Debugf("cannot attribute the foreground window, raising the prompt without an owner: %v", err) + return 0 + } + if pid != windows.GetCurrentProcessId() { + return 0 + } + return hwnd +} + +// isHResult reports whether err carries the given HRESULT. CoInitializeEx +// returns its HRESULT as an Errno, so the comparison is on the raw value. +func isHResult(err error, hresult windows.Handle) bool { + var errno windows.Errno + return errors.As(err, &errno) && uintptr(errno) == uintptr(hresult) +} + +func waitForProcess(ctx context.Context, process windows.Handle) error { + // The wait is interruptible so a cancelled context stops us waiting on a + // consent dialog nobody is going to answer. The elevated process is not + // ours to kill, and it either applies the change or does not. + for { + event, err := windows.WaitForSingleObject(process, 250) + if err != nil { + return fmt.Errorf("wait for the elevated process: %w", err) + } + if event == uint32(windows.WAIT_OBJECT_0) { + break + } + if err := ctx.Err(); err != nil { + return err + } + } + + var code uint32 + if err := windows.GetExitCodeProcess(process, &code); err != nil { + return fmt.Errorf("read the elevated process's exit code: %w", err) + } + if code != 0 { + return fmt.Errorf("elevated netbird exited with %d", code) + } + return nil +} + +// mechanismAvailable is true on Windows: UAC prompts for consent when the user +// is an administrator and for an administrator's credentials when they are not, +// so there is always something to ask. +func mechanismAvailable() bool { + return true +} diff --git a/client/internal/elevate/trusted.go b/client/internal/elevate/trusted.go new file mode 100644 index 000000000..c11054c45 --- /dev/null +++ b/client/internal/elevate/trusted.go @@ -0,0 +1,40 @@ +package elevate + +import ( + "fmt" + "os" + "path/filepath" +) + +// trustedSelf returns the path of this executable, provided it is one we are +// willing to have run as root. +// +// The check is what keeps elevation from becoming a way to launder someone +// else's code into a root process: the user consents to NetBird being elevated, +// having been shown NetBird's name, so what runs must be the file NetBird was +// installed as and not something a third party could have swapped for it. An +// executable only its owner can write is that; anything wider is refused, and +// the caller falls back to showing the command instead. +// +// The owner writing to their own executable is not part of that threat: code +// running as the user can already prompt them for anything, and could just as +// well ask them to run the command by hand. What matters is that no *other* +// unprivileged account can reach it. +func trustedSelf() (string, error) { + exe, err := os.Executable() + if err != nil { + return "", fmt.Errorf("locate this executable: %w", err) + } + + // Resolve symlinks so the checks below apply to the file that would actually + // be executed, not to a link somebody else may control. + resolved, err := filepath.EvalSymlinks(exe) + if err != nil { + return "", fmt.Errorf("resolve %s: %w", exe, err) + } + + if err := checkOnlyOwnerWritable(resolved); err != nil { + return "", fmt.Errorf("%w: %s cannot be trusted to run as root: %w", ErrUnavailable, resolved, err) + } + return resolved, nil +} diff --git a/client/internal/elevate/trusted_group_darwin.go b/client/internal/elevate/trusted_group_darwin.go new file mode 100644 index 000000000..a4b387ec4 --- /dev/null +++ b/client/internal/elevate/trusted_group_darwin.go @@ -0,0 +1,10 @@ +package elevate + +// adminWriteGIDs are the groups whose write access to an executable does not +// widen who could authorize elevating it. +// +// macOS installs applications as root:admin, mode 0775, /Applications included, +// so requiring owner-only write would reject every normal install. Group admin +// (gid 80) is exactly the set of accounts that can answer the authentication +// dialog, so its write access grants nothing the prompt would not. +var adminWriteGIDs = []uint32{0, 80} diff --git a/client/internal/elevate/trusted_group_unix.go b/client/internal/elevate/trusted_group_unix.go new file mode 100644 index 000000000..7aa336423 --- /dev/null +++ b/client/internal/elevate/trusted_group_unix.go @@ -0,0 +1,9 @@ +//go:build !windows && !darwin + +package elevate + +// adminWriteGIDs are the groups whose write access to an executable does not +// widen who could authorize elevating it. Only root's own group qualifies here: +// a distribution installs into root-owned directories, and there is no +// system-wide administrators group that both writes them and answers polkit. +var adminWriteGIDs = []uint32{0} diff --git a/client/internal/elevate/trusted_unix.go b/client/internal/elevate/trusted_unix.go new file mode 100644 index 000000000..f9d1a1b7e --- /dev/null +++ b/client/internal/elevate/trusted_unix.go @@ -0,0 +1,119 @@ +//go:build !windows + +package elevate + +import ( + "errors" + "fmt" + "os" + "path/filepath" + "slices" + "strconv" + "syscall" + + log "github.com/sirupsen/logrus" + + "github.com/netbirdio/netbird/client/internal/getent" +) + +// checkOnlyOwnerWritable reports an error unless path, and every directory leading +// to it, is owned by either root or this user and writable by nobody who could not +// already act as its owner. A writable directory is as good as a writable file, +// since anything in it can be replaced, so the whole chain is checked. +func checkOnlyOwnerWritable(path string) error { + self := uint32(os.Getuid()) + + for dir := path; ; dir = filepath.Dir(dir) { + info, err := os.Lstat(dir) + if err != nil { + return fmt.Errorf("stat %s: %w", dir, err) + } + + stat, ok := info.Sys().(*syscall.Stat_t) + if !ok { + return errors.New("file ownership is unavailable on this platform") + } + if stat.Uid != 0 && stat.Uid != self { + return fmt.Errorf("%s is owned by uid %d, neither root nor this user", dir, stat.Uid) + } + + if err := checkWriteBits(dir, info, stat.Uid, stat.Gid); err != nil { + return err + } + + if parent := filepath.Dir(dir); parent == dir { + return nil + } + } +} + +func checkWriteBits(path string, info os.FileInfo, uid, gid uint32) error { + // On a directory the sticky bit stands in for the write bits: whoever may + // write there still cannot replace an entry they do not own, which is the + // only thing that would matter to us. /tmp is the usual example. + sticky := info.IsDir() && info.Mode()&os.ModeSticky != 0 + + return writeBitsAllow(path, info.Mode().Perm(), sticky, groupWriteAllowed(uid, gid)) +} + +// writeBitsAllow decides on the permission bits alone, given whether the group's +// write access has been vouched for. +func writeBitsAllow(path string, perm os.FileMode, sticky, groupAllowed bool) error { + if sticky { + return nil + } + if perm&0o020 != 0 && !groupAllowed { + return fmt.Errorf("%s is writable by a group with members other than its owner (%v)", path, perm) + } + if perm&0o002 != 0 { + return fmt.Errorf("%s is world-writable (%v)", path, perm) + } + return nil +} + +// groupWriteAllowed reports whether a group's write access to a file owned by uid +// puts it in reach of anyone who could not already act as that owner. +// +// Two ways it does not. A group in adminWriteGIDs holds the accounts that can +// answer the elevation prompt anyway. And a user private group is how Debian, +// Ubuntu and Fedora ship: their umask of 002 makes a home directory and +// everything built in it group-writable, so refusing that would refuse every +// build not installed from a package. +func groupWriteAllowed(uid, gid uint32) bool { + if slices.Contains(adminWriteGIDs, gid) { + return true + } + + group, err := getent.LookupGroupID(strconv.FormatUint(uint64(gid), 10)) + if err != nil { + log.Debugf("cannot look up group %d, treating it as shared: %v", gid, err) + return false + } + owner, err := getent.LookupUserID(strconv.FormatUint(uint64(uid), 10)) + if err != nil { + log.Debugf("cannot look up uid %d, treating its group as shared: %v", uid, err) + return false + } + + if group.Name != owner.Username { + return false + } + return !groupHasOtherMembers(group.Name, owner.Username) +} + +// groupHasOtherMembers reports whether the group lists a member besides owner. +// +// Sharing the owner's name is what a user private group is recognised by, and it +// says nothing about who is in it: a group that has since gained a member is +// still named that way, and that member can write whatever the group can. So the +// membership is read rather than assumed. A group whose members cannot be +// listed, because no source on this host describes it, is treated as shared: +// the name alone cannot vouch for who writes through it. +func groupHasOtherMembers(name, owner string) bool { + members, err := getent.GroupMembers(name) + if err != nil { + log.Debugf("cannot list the members of group %q, treating it as shared: %v", name, err) + return true + } + return slices.ContainsFunc(members, func(member string) bool { return member != owner }) +} diff --git a/client/internal/elevate/trusted_unix_test.go b/client/internal/elevate/trusted_unix_test.go new file mode 100644 index 000000000..7c0c5a966 --- /dev/null +++ b/client/internal/elevate/trusted_unix_test.go @@ -0,0 +1,148 @@ +//go:build !windows + +package elevate + +import ( + "os" + "os/user" + "path/filepath" + "strconv" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// ownerOnlyDir is t.TempDir() with the write bits tightened. testing creates its +// numbered directory with 0777 minus the umask, so under the common 002 umask it +// is group-writable and would fail the check under test on its own. +func ownerOnlyDir(t *testing.T) string { + t.Helper() + dir := t.TempDir() + require.NoError(t, os.Chmod(dir, 0o755), "tighten the temporary directory") + return dir +} + +// writeExecutable creates a plain executable file, the shape trustedSelf checks. +func writeExecutable(t *testing.T, dir string) string { + t.Helper() + path := filepath.Join(dir, "netbird-ui") + require.NoError(t, os.WriteFile(path, []byte("#!/bin/sh\n"), 0o755), "write the executable") + require.NoError(t, os.Chmod(path, 0o755), "set the executable's mode") + return path +} + +func TestCheckOnlyOwnerWritableAcceptsOwnerOnly(t *testing.T) { + err := checkOnlyOwnerWritable(writeExecutable(t, ownerOnlyDir(t))) + assert.NoError(t, err, "an owner-only writable executable is trustworthy") +} + +func TestCheckOnlyOwnerWritableRejectsWorldWritableFile(t *testing.T) { + path := writeExecutable(t, ownerOnlyDir(t)) + require.NoError(t, os.Chmod(path, 0o777), "make the executable world-writable") + + assert.Error(t, checkOnlyOwnerWritable(path), "a world-writable executable must be refused") +} + +// The permission policy on its own, without a filesystem to arrange: whether the +// group has been vouched for is the only thing that makes group write acceptable. +func TestWriteBitsAllow(t *testing.T) { + tests := []struct { + name string + perm os.FileMode + sticky bool + groupAllowed bool + wantErr bool + }{ + {name: "owner only", perm: 0o755}, + {name: "group write in a private group", perm: 0o775, groupAllowed: true}, + {name: "group write in a shared group", perm: 0o775, wantErr: true}, + {name: "world write", perm: 0o777, groupAllowed: true, wantErr: true}, + {name: "world write on a sticky directory", perm: 0o777, sticky: true}, + {name: "group write on a sticky directory", perm: 0o775, sticky: true}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + err := writeBitsAllow("/path", tt.perm, tt.sticky, tt.groupAllowed) + if tt.wantErr { + assert.Error(t, err, "perm %v, sticky %v, group allowed %v", tt.perm, tt.sticky, tt.groupAllowed) + return + } + assert.NoError(t, err, "perm %v, sticky %v, group allowed %v", tt.perm, tt.sticky, tt.groupAllowed) + }) + } +} + +// A build under a home directory on a distribution with a 002 umask, which is what +// a locally built or tarball-installed binary looks like. Its group has no members +// but its owner, so it is as good as owner-only. +// +// Whether this host is such a distribution is read from the environment rather than +// from groupWriteAllowed: asking the function under test whether to run would let +// it skip its own coverage away if it regressed to refusing everything. +func TestCheckOnlyOwnerWritableAcceptsOwnPrivateGroup(t *testing.T) { + requirePrivatePrimaryGroup(t) + + dir := ownerOnlyDir(t) + path := writeExecutable(t, dir) + require.NoError(t, os.Chmod(dir, 0o775), "make the directory group-writable") + require.NoError(t, os.Chmod(path, 0o775), "make the executable group-writable") + + err := checkOnlyOwnerWritable(path) + assert.NoError(t, err, "group write in the owner's own private group reaches nobody else") +} + +// A group whose membership no source can answer for is treated as shared: the +// private-group allowance must not stand on a name nobody can vouch for. The +// membership listing itself lives in the getent package and is tested there. +func TestGroupHasOtherMembersRejectsAnUnknownGroup(t *testing.T) { + assert.True(t, groupHasOtherMembers("nonexistent_group_xyzzy_12345", "vma"), + "a group no source describes") +} + +// A writable directory is as good as a writable file: whoever can write the +// directory can put a different binary at the same path. +func TestCheckOnlyOwnerWritableRejectsWritableDirectory(t *testing.T) { + dir := filepath.Join(ownerOnlyDir(t), "bin") + require.NoError(t, os.Mkdir(dir, 0o755), "create the directory") + path := writeExecutable(t, dir) + require.NoError(t, os.Chmod(dir, 0o777), "make the directory world-writable") + + assert.Error(t, checkOnlyOwnerWritable(path), "an executable in a world-writable directory must be refused") +} + +// A sticky world-writable directory is exempt: the sticky bit is what stops one +// user replacing another's entries. /tmp is why this matters. +func TestCheckOnlyOwnerWritableAcceptsStickyDirectory(t *testing.T) { + dir := filepath.Join(ownerOnlyDir(t), "sticky") + require.NoError(t, os.Mkdir(dir, 0o755), "create the directory") + path := writeExecutable(t, dir) + require.NoError(t, os.Chmod(dir, 0o777|os.ModeSticky), "make the directory sticky and world-writable") + + err := checkOnlyOwnerWritable(path) + assert.NoError(t, err, "the sticky bit stops another user replacing the executable") +} + +func TestCheckOnlyOwnerWritableRejectsMissingFile(t *testing.T) { + err := checkOnlyOwnerWritable(filepath.Join(ownerOnlyDir(t), "absent")) + assert.Error(t, err, "an executable that is not there must be refused") +} + +// requirePrivatePrimaryGroup skips unless this user's primary group is their own, +// which is what the user-private-group allowance is about. +func requirePrivatePrimaryGroup(t *testing.T) { + t.Helper() + + self, err := user.Current() + require.NoError(t, err, "look up the test user") + group, err := user.LookupGroupId(strconv.Itoa(os.Getgid())) + require.NoError(t, err, "look up the test user's primary group") + + if group.Name != self.Username { + t.Skipf("the test user's primary group is %q, not their own, so there is nothing to assert here", group.Name) + } + if groupHasOtherMembers(group.Name, self.Username) { + t.Skipf("group %q has other members, so it is not a private group", group.Name) + } +} diff --git a/client/internal/elevate/trusted_windows.go b/client/internal/elevate/trusted_windows.go new file mode 100644 index 000000000..8fb05fd88 --- /dev/null +++ b/client/internal/elevate/trusted_windows.go @@ -0,0 +1,215 @@ +package elevate + +import ( + "errors" + "fmt" + "path/filepath" + "slices" + "unsafe" + + "golang.org/x/sys/windows" +) + +const ( + // fileDeleteChild is FILE_DELETE_CHILD, which x/sys does not define: the + // right to delete an entry of a directory without holding DELETE on it. + fileDeleteChild = 0x00000040 + + // accessAllowedCallbackACEType is an allow ACE with a condition appended to + // the ACCESS_ALLOWED_ACE layout, so its trustee is still at SidStart. + accessAllowedCallbackACEType = 0x9 + + // The allow ACE types that carry object GUIDs ahead of the trustee, so the + // SID is not at SidStart. They occur on directory-service objects rather + // than files, and are refused rather than skipped: see aceTrustee. + accessAllowedObjectACEType = 0x5 + accessAllowedCallbackObjectACEType = 0xB +) + +// fileWriteAccess are the rights that let a trustee rewrite or replace a file, +// or take it over and then do so. +const fileWriteAccess = windows.FILE_WRITE_DATA | windows.FILE_APPEND_DATA | + windows.DELETE | windows.WRITE_DAC | windows.WRITE_OWNER | + windows.GENERIC_WRITE | windows.GENERIC_ALL + +// dirWriteAccess are the rights over a directory that let a trustee replace an +// entry somebody else owns. Creating a new entry is not one of them, which is +// what the Unix sticky bit says in one bit: the root of every volume grants +// BUILTIN\Users the right to add directories under it, and that reaches nothing +// already there. +const dirWriteAccess = fileDeleteChild | windows.DELETE | + windows.WRITE_DAC | windows.WRITE_OWNER | windows.GENERIC_ALL + +// trustedInstallerSID owns much of what Windows itself installs. x/sys has no +// well-known constant for it. +const trustedInstallerSID = "S-1-5-80-956008885-3418522649-1831038044-1853292631-2271478464" + +// checkOnlyOwnerWritable reports an error unless path, and every directory +// leading to it, is owned by an account that can elevate (or by this user) and +// grants write access to nobody else. A writable directory is as good as a +// writable file, since an entry in it can be replaced, so the whole chain is +// checked. +func checkOnlyOwnerWritable(path string) error { + owners, err := trustedOwners() + if err != nil { + return err + } + writers, err := trustedWriters(owners) + if err != nil { + return err + } + + writeAccess := windows.ACCESS_MASK(fileWriteAccess) + for target := path; ; target = filepath.Dir(target) { + if err := checkSecurity(target, writeAccess, owners, writers); err != nil { + return err + } + if parent := filepath.Dir(target); parent == target { + return nil + } + writeAccess = dirWriteAccess + } +} + +// trustedOwners are the accounts we accept as the owner of the executable and of +// the directories above it: the ones that can already answer the UAC prompt, +// plus this user, whose own executable is theirs to write. Code running as the +// user could prompt them for anything anyway; what matters is that no *other* +// unprivileged account can reach it. +func trustedOwners() ([]*windows.SID, error) { + self, err := currentUserSID() + if err != nil { + return nil, err + } + + owners := []*windows.SID{self} + for _, wellKnown := range []windows.WELL_KNOWN_SID_TYPE{ + windows.WinLocalSystemSid, + windows.WinBuiltinAdministratorsSid, + } { + sid, err := windows.CreateWellKnownSid(wellKnown) + if err != nil { + return nil, fmt.Errorf("build well-known SID %d: %w", wellKnown, err) + } + owners = append(owners, sid) + } + + installer, err := windows.StringToSid(trustedInstallerSID) + if err != nil { + return nil, fmt.Errorf("parse TrustedInstaller SID: %w", err) + } + return append(owners, installer), nil +} + +// trustedWriters are the trustees whose write access does not widen who could +// decide what runs behind the prompt. The owners, and CREATOR OWNER, which +// resolves to the object's owner and is therefore already vetted. +func trustedWriters(owners []*windows.SID) ([]*windows.SID, error) { + creatorOwner, err := windows.CreateWellKnownSid(windows.WinCreatorOwnerSid) + if err != nil { + return nil, fmt.Errorf("build the CREATOR OWNER SID: %w", err) + } + return append(slices.Clone(owners), creatorOwner), nil +} + +func checkSecurity(path string, writeAccess windows.ACCESS_MASK, owners, writers []*windows.SID) error { + sd, err := windows.GetNamedSecurityInfo(path, windows.SE_FILE_OBJECT, + windows.OWNER_SECURITY_INFORMATION|windows.DACL_SECURITY_INFORMATION) + if err != nil { + return fmt.Errorf("read security descriptor of %s: %w", path, err) + } + + owner, _, err := sd.Owner() + if err != nil { + return fmt.Errorf("read owner of %s: %w", path, err) + } + if !containsSID(owners, owner) { + return fmt.Errorf("%s is owned by %s, which is neither this user nor an account that can elevate", path, owner) + } + + dacl, _, err := sd.DACL() + if err != nil { + return fmt.Errorf("read DACL of %s: %w", path, err) + } + // A NULL DACL grants everyone everything; only an absent security + // descriptor would have got us here without one, and neither is trustworthy. + if dacl == nil { + return fmt.Errorf("%s has no DACL, so it grants write access to everyone", path) + } + + return checkDACL(path, dacl, writeAccess, writers) +} + +// checkDACL refuses an ACL that grants write access to a trustee outside +// writers. +// +// An allowlist, because the trustees that must not have it cannot be listed: an +// ACE naming an ordinary user account hands that account the same power as one +// naming Everyone, and only the accounts that may hold it are knowable. +func checkDACL(path string, dacl *windows.ACL, writeAccess windows.ACCESS_MASK, writers []*windows.SID) error { + for i := uint32(0); i < uint32(dacl.AceCount); i++ { + var ace *windows.ACCESS_ALLOWED_ACE + if err := windows.GetAce(dacl, i, &ace); err != nil { + return fmt.Errorf("read ACE %d of %s: %w", i, path, err) + } + // An inherit-only ACE says what children of this object get, not what + // this object grants. + if ace.Header.AceFlags&windows.INHERIT_ONLY_ACE != 0 { + continue + } + if ace.Mask&writeAccess == 0 { + continue + } + // Only an allow ACE grants anything; a deny ACE narrows what one gave. + if !isAllowACE(ace.Header.AceType) { + continue + } + + trustee, err := aceTrustee(ace) + if err != nil { + return fmt.Errorf("read the trustee of ACE %d of %s: %w", i, path, err) + } + if !containsSID(writers, trustee) { + return fmt.Errorf("%s grants write access to %s", path, trustee) + } + } + return nil +} + +// isAllowACE reports whether an ACE type grants rights, rather than denying, +// auditing or labelling them. +func isAllowACE(aceType uint8) bool { + switch aceType { + case windows.ACCESS_ALLOWED_ACE_TYPE, accessAllowedCallbackACEType, + accessAllowedObjectACEType, accessAllowedCallbackObjectACEType: + return true + default: + return false + } +} + +// aceTrustee returns who an allow ACE grants its rights to. An ACE whose trustee +// cannot be located is an error rather than something to skip past: being unable +// to read who is being given write access is a refusal. +func aceTrustee(ace *windows.ACCESS_ALLOWED_ACE) (*windows.SID, error) { + switch ace.Header.AceType { + case windows.ACCESS_ALLOWED_ACE_TYPE, accessAllowedCallbackACEType: + //nolint:gosec // SidStart is the first uint32 of the variable-length SID that follows the ACE header. + return (*windows.SID)(unsafe.Pointer(&ace.SidStart)), nil + default: + return nil, errors.New("an object-type allow ACE does not carry its trustee where we can read it") + } +} + +func containsSID(sids []*windows.SID, sid *windows.SID) bool { + return slices.ContainsFunc(sids, sid.Equals) +} + +func currentUserSID() (*windows.SID, error) { + token := windows.GetCurrentProcessToken() + user, err := token.GetTokenUser() + if err != nil { + return nil, fmt.Errorf("read this process's user: %w", err) + } + return user.User.Sid, nil +} diff --git a/client/internal/elevate/trusted_windows_test.go b/client/internal/elevate/trusted_windows_test.go new file mode 100644 index 000000000..946e7b7c8 --- /dev/null +++ b/client/internal/elevate/trusted_windows_test.go @@ -0,0 +1,126 @@ +package elevate + +import ( + "os" + "path/filepath" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + "golang.org/x/sys/windows" +) + +// A file the test user created under their own profile, which is what a per-user +// install looks like. The whole chain up to the volume root is walked, so this is +// also what says the walk does not refuse an ordinary Windows installation: the +// root of every volume grants BUILTIN\Users rights that are not ours to worry +// about. +func TestCheckOnlyOwnerWritableAcceptsOwnFile(t *testing.T) { + err := checkOnlyOwnerWritable(writeExecutable(t)) + assert.NoError(t, err, "a file the test user owns, under directories only administrators can write") +} + +// Write access held by an account that cannot answer the UAC prompt means that +// account decides what runs behind it, whoever the ACE names. The trustees that +// must not have it cannot be listed, so the check names the ones that may. +func TestCheckOnlyOwnerWritableRejectsUntrustedWriters(t *testing.T) { + tests := []struct { + name string + wellKnown windows.WELL_KNOWN_SID_TYPE + }{ + {name: "everyone", wellKnown: windows.WinWorldSid}, + {name: "authenticated users", wellKnown: windows.WinAuthenticatedUserSid}, + {name: "builtin users", wellKnown: windows.WinBuiltinUsersSid}, + // A service account, which no denylist of the obvious groups would name + // and which cannot elevate any more than Everyone can. + {name: "local service", wellKnown: windows.WinLocalServiceSid}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + path := writeExecutable(t) + grantWrite(t, path, tt.wellKnown) + + assert.Error(t, checkOnlyOwnerWritable(path), + "write access for %s must be refused", tt.name) + }) + } +} + +// The masks are the policy: on a file any write reaches its contents, while on a +// directory only deleting or taking over an entry reaches something already +// there. Adding an entry does not, which is why the walk survives a volume root. +func TestWriteAccessMasks(t *testing.T) { + assert.NotZero(t, fileWriteAccess&windows.FILE_WRITE_DATA, "writing a file's data reaches its contents") + assert.NotZero(t, fileWriteAccess&windows.FILE_APPEND_DATA, "appending to a file reaches its contents") + + assert.Zero(t, dirWriteAccess&windows.FILE_WRITE_DATA, "adding a file to a directory replaces nothing") + assert.Zero(t, dirWriteAccess&windows.FILE_APPEND_DATA, "adding a subdirectory replaces nothing") + assert.NotZero(t, dirWriteAccess&fileDeleteChild, "deleting an entry replaces it") + assert.NotZero(t, dirWriteAccess&windows.DELETE, "deleting the directory takes its entries with it") +} + +func TestIsAllowACE(t *testing.T) { + tests := []struct { + name string + aceType uint8 + want bool + }{ + {name: "allowed", aceType: windows.ACCESS_ALLOWED_ACE_TYPE, want: true}, + {name: "allowed callback", aceType: accessAllowedCallbackACEType, want: true}, + {name: "allowed object", aceType: accessAllowedObjectACEType, want: true}, + {name: "allowed callback object", aceType: accessAllowedCallbackObjectACEType, want: true}, + {name: "denied", aceType: windows.ACCESS_DENIED_ACE_TYPE}, + // SYSTEM_AUDIT_ACE_TYPE, which x/sys does not define: an ACE that records + // access rather than granting it. + {name: "audit", aceType: 0x2}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + assert.Equal(t, tt.want, isAllowACE(tt.aceType), "ACE type %#x", tt.aceType) + }) + } +} + +// writeExecutable creates a plain file under the test's own directory, the shape +// trustedSelf checks. +func writeExecutable(t *testing.T) string { + t.Helper() + path := filepath.Join(t.TempDir(), "netbird-ui.exe") + require.NoError(t, os.WriteFile(path, []byte("MZ"), 0o755), "write the executable") + return path +} + +// grantWrite replaces the file's DACL with one that grants a well-known trustee +// everything, keeping the test user's own access so the file stays deletable. +func grantWrite(t *testing.T, path string, wellKnown windows.WELL_KNOWN_SID_TYPE) { + t.Helper() + + trustee, err := windows.CreateWellKnownSid(wellKnown) + require.NoError(t, err, "build the trustee SID") + self, err := currentUserSID() + require.NoError(t, err, "read the test user's SID") + + acl, err := windows.ACLFromEntries([]windows.EXPLICIT_ACCESS{ + fullControl(self, windows.TRUSTEE_IS_USER), + fullControl(trustee, windows.TRUSTEE_IS_WELL_KNOWN_GROUP), + }, nil) + require.NoError(t, err, "build the ACL") + + require.NoError(t, windows.SetNamedSecurityInfo(path, windows.SE_FILE_OBJECT, + windows.DACL_SECURITY_INFORMATION|windows.PROTECTED_DACL_SECURITY_INFORMATION, + nil, nil, acl, nil), "set the DACL") +} + +func fullControl(sid *windows.SID, trusteeType uint32) windows.EXPLICIT_ACCESS { + return windows.EXPLICIT_ACCESS{ + AccessPermissions: windows.GENERIC_ALL, + AccessMode: windows.GRANT_ACCESS, + Trustee: windows.TRUSTEE{ + TrusteeForm: windows.TRUSTEE_IS_SID, + TrusteeType: windows.TRUSTEE_TYPE(trusteeType), + TrusteeValue: windows.TrusteeValueFromSID(sid), + }, + } +} diff --git a/client/internal/getent/cgo_unix.go b/client/internal/getent/cgo_unix.go new file mode 100644 index 000000000..2853aafff --- /dev/null +++ b/client/internal/getent/cgo_unix.go @@ -0,0 +1,36 @@ +//go:build cgo && !osusergo && !windows + +package getent + +import "os/user" + +// Built with cgo, os/user resolves through libc (getpwnam_r and friends), +// which goes through the host's NSS stack natively. Whatever it fails to +// find, the getent command would not find either, so there is nothing to +// fall back to. + +// LookupUser looks up a user by name. +func LookupUser(username string) (*user.User, error) { + return user.Lookup(username) +} + +// LookupUserID looks up a user by UID. +func LookupUserID(uid string) (*user.User, error) { + return user.LookupId(uid) +} + +// CurrentUser returns the user this process runs as. +func CurrentUser() (*user.User, error) { + return user.Current() +} + +// LookupGroupID looks up a group by GID. +func LookupGroupID(gid string) (*user.Group, error) { + return user.LookupGroupId(gid) +} + +// GroupIDs returns the IDs of the groups the user is a member of; libc's +// getgrouplist handles NSS groups natively. +func GroupIDs(u *user.User) ([]string, error) { + return u.GroupIds() +} diff --git a/client/internal/getent/getent.go b/client/internal/getent/getent.go new file mode 100644 index 000000000..9cfebe64b --- /dev/null +++ b/client/internal/getent/getent.go @@ -0,0 +1,6 @@ +// Package getent resolves users and groups through the host's NSS stack. +// Built without cgo, os/user reads /etc/passwd and /etc/group alone and misses +// anything LDAP, SSSD or winbind provide; the getent and id commands resolve +// through NSS whatever the build. The lookups here try the standard library +// first, which needs no subprocess, and fall back to those commands. +package getent diff --git a/client/ssh/server/getent_test.go b/client/internal/getent/getent_test.go similarity index 53% rename from client/ssh/server/getent_test.go rename to client/internal/getent/getent_test.go index 5eac2fdbe..8176eba36 100644 --- a/client/ssh/server/getent_test.go +++ b/client/internal/getent/getent_test.go @@ -1,4 +1,4 @@ -package server +package getent import ( "os/user" @@ -10,38 +10,48 @@ import ( "github.com/stretchr/testify/require" ) -func TestLookupWithGetent_CurrentUser(t *testing.T) { +func TestLookupUser_CurrentUser(t *testing.T) { // The current user should always be resolvable on any platform current, err := user.Current() require.NoError(t, err) - u, err := lookupWithGetent(current.Username) + u, err := LookupUser(current.Username) require.NoError(t, err) assert.Equal(t, current.Username, u.Username) assert.Equal(t, current.Uid, u.Uid) assert.Equal(t, current.Gid, u.Gid) } -func TestLookupWithGetent_NonexistentUser(t *testing.T) { - _, err := lookupWithGetent("nonexistent_user_xyzzy_12345") +func TestLookupUser_NonexistentUser(t *testing.T) { + _, err := LookupUser("nonexistent_user_xyzzy_12345") require.Error(t, err, "should fail for nonexistent user") } -func TestCurrentUserWithGetent(t *testing.T) { +func TestLookupUserID_CurrentUser(t *testing.T) { + current, err := user.Current() + require.NoError(t, err) + + u, err := LookupUserID(current.Uid) + require.NoError(t, err) + assert.Equal(t, current.Username, u.Username) + assert.Equal(t, current.Uid, u.Uid) +} + +func TestCurrentUser(t *testing.T) { stdUser, err := user.Current() require.NoError(t, err) - u, err := currentUserWithGetent() + u, err := CurrentUser() require.NoError(t, err) assert.Equal(t, stdUser.Uid, u.Uid) assert.Equal(t, stdUser.Username, u.Username) } -func TestGroupIdsWithFallback_CurrentUser(t *testing.T) { +func TestGroupIDs_CurrentUser(t *testing.T) { current, err := user.Current() require.NoError(t, err) - groups, err := groupIdsWithFallback(current) + groups, err := GroupIDs(current) require.NoError(t, err) require.NotEmpty(t, groups, "current user should have at least one group") @@ -53,32 +63,30 @@ func TestGroupIdsWithFallback_CurrentUser(t *testing.T) { } } -func TestGetShellFromGetent_CurrentUser(t *testing.T) { - if runtime.GOOS == "windows" { - // Windows stub always returns empty, which is correct - shell := getShellFromGetent("1000") - assert.Empty(t, shell, "Windows stub should return empty") - return - } - +func TestUserShell_CurrentUser(t *testing.T) { current, err := user.Current() require.NoError(t, err) - // getent may not be available on all systems (e.g., macOS without Homebrew getent) - shell := getShellFromGetent(current.Uid) + // getent may not be available on all systems (e.g., macOS without + // Homebrew getent), and Windows has no login shells at all. + shell, err := UserShell(current.Uid) + if err != nil { + t.Logf("UserShell failed, getent may not be available: %v", err) + return + } if shell == "" { - t.Log("getShellFromGetent returned empty, getent may not be available") + t.Log("UserShell returned empty, the user has no shell set") return } assert.True(t, shell[0] == '/', "shell should be an absolute path, got %q", shell) } -func TestLookupWithGetent_RootUser(t *testing.T) { +func TestLookupUser_RootUser(t *testing.T) { if runtime.GOOS == "windows" { t.Skip("no root user on Windows") } - u, err := lookupWithGetent("root") + u, err := LookupUser("root") if err != nil { t.Skip("root user not available on this system") } @@ -86,25 +94,25 @@ func TestLookupWithGetent_RootUser(t *testing.T) { } // TestIntegration_FullLookupChain exercises the complete user lookup chain -// against the real system, testing that all wrappers (lookupWithGetent, -// currentUserWithGetent, groupIdsWithFallback, getShellFromGetent) produce -// consistent and correct results when composed together. +// against the real system, testing that all wrappers (LookupUser, +// CurrentUser, GroupIDs, UserShell) produce consistent and correct results +// when composed together. func TestIntegration_FullLookupChain(t *testing.T) { - // Step 1: currentUserWithGetent must resolve the running user. - current, err := currentUserWithGetent() - require.NoError(t, err, "currentUserWithGetent must resolve the running user") + // Step 1: CurrentUser must resolve the running user. + current, err := CurrentUser() + require.NoError(t, err, "CurrentUser must resolve the running user") require.NotEmpty(t, current.Uid) require.NotEmpty(t, current.Username) - // Step 2: lookupWithGetent by the same username must return matching identity. - byName, err := lookupWithGetent(current.Username) + // Step 2: LookupUser by the same username must return matching identity. + byName, err := LookupUser(current.Username) require.NoError(t, err) assert.Equal(t, current.Uid, byName.Uid, "lookup by name should return same UID") assert.Equal(t, current.Gid, byName.Gid, "lookup by name should return same GID") assert.Equal(t, current.HomeDir, byName.HomeDir, "lookup by name should return same home") - // Step 3: groupIdsWithFallback must return at least the primary GID. - groups, err := groupIdsWithFallback(current) + // Step 3: GroupIDs must return at least the primary GID. + groups, err := GroupIDs(current) require.NoError(t, err) require.NotEmpty(t, groups, "user must have at least one group") @@ -119,29 +127,20 @@ func TestIntegration_FullLookupChain(t *testing.T) { } } assert.True(t, foundPrimary, "primary GID %s should appear in supplementary groups", current.Gid) - - // Step 4: getShellFromGetent should either return a valid shell path or empty - // (empty is OK when getent is not available, e.g. macOS without Homebrew getent). - if runtime.GOOS != "windows" { - shell := getShellFromGetent(current.Uid) - if shell != "" { - assert.True(t, shell[0] == '/', "shell should be an absolute path, got %q", shell) - } - } } // TestIntegration_LookupAndGroupsConsistency verifies that a user resolved via -// lookupWithGetent can have their groups resolved via groupIdsWithFallback, -// testing the handoff between the two functions as used by the SSH server. +// LookupUser can have their groups resolved via GroupIDs, testing the handoff +// between the two functions as used by the SSH server. func TestIntegration_LookupAndGroupsConsistency(t *testing.T) { current, err := user.Current() require.NoError(t, err) // Simulate the SSH server flow: lookup user, then get their groups. - resolved, err := lookupWithGetent(current.Username) + resolved, err := LookupUser(current.Username) require.NoError(t, err) - groups, err := groupIdsWithFallback(resolved) + groups, err := GroupIDs(resolved) require.NoError(t, err) require.NotEmpty(t, groups, "resolved user must have groups") @@ -154,19 +153,3 @@ func TestIntegration_LookupAndGroupsConsistency(t *testing.T) { } } } - -// TestIntegration_ShellLookupChain tests the full shell resolution chain -// (getShellFromPasswd -> getShellFromGetent -> $SHELL -> default) on Unix. -func TestIntegration_ShellLookupChain(t *testing.T) { - if runtime.GOOS == "windows" { - t.Skip("Unix shell lookup not applicable on Windows") - } - - current, err := user.Current() - require.NoError(t, err) - - // getUserShell is the top-level function used by the SSH server. - shell := getUserShell(current.Uid) - require.NotEmpty(t, shell, "getUserShell must always return a shell") - assert.True(t, shell[0] == '/', "shell should be an absolute path, got %q", shell) -} diff --git a/client/internal/getent/nocgo_unix.go b/client/internal/getent/nocgo_unix.go new file mode 100644 index 000000000..94d8ea6a9 --- /dev/null +++ b/client/internal/getent/nocgo_unix.go @@ -0,0 +1,110 @@ +//go:build (!cgo || osusergo) && !windows + +package getent + +import ( + "os" + "os/user" + "strconv" + + log "github.com/sirupsen/logrus" +) + +// Without cgo, os/user only reads /etc/passwd and /etc/group and misses +// NSS-provided users and groups; the getent and id commands go through the +// host's NSS stack. + +// LookupUser looks up a user by name, falling back to getent if os/user fails. +func LookupUser(username string) (*user.User, error) { + u, err := user.Lookup(username) + if err == nil { + return u, nil + } + + stdErr := err + log.Debugf("os/user.Lookup(%q) failed, trying getent: %v", username, err) + + u, _, getentErr := passwdLookup(username) + if getentErr != nil { + log.Debugf("getent fallback for %q also failed: %v", username, getentErr) + return nil, stdErr + } + return u, nil +} + +// LookupUserID looks up a user by UID, falling back to getent if os/user fails. +func LookupUserID(uid string) (*user.User, error) { + u, err := user.LookupId(uid) + if err == nil { + return u, nil + } + + stdErr := err + log.Debugf("os/user.LookupId(%q) failed, trying getent: %v", uid, err) + + u, _, getentErr := passwdLookup(uid) + if getentErr != nil { + log.Debugf("getent fallback for uid %s also failed: %v", uid, getentErr) + return nil, stdErr + } + return u, nil +} + +// CurrentUser returns the user this process runs as, falling back to getent +// if os/user fails. +func CurrentUser() (*user.User, error) { + u, err := user.Current() + if err == nil { + return u, nil + } + + stdErr := err + uid := strconv.Itoa(os.Getuid()) + log.Debugf("os/user.Current() failed, trying getent with UID %s: %v", uid, err) + + u, _, getentErr := passwdLookup(uid) + if getentErr != nil { + return nil, stdErr + } + return u, nil +} + +// LookupGroupID looks up a group by GID, falling back to getent if os/user +// fails. +func LookupGroupID(gid string) (*user.Group, error) { + g, err := user.LookupGroupId(gid) + if err == nil { + return g, nil + } + + stdErr := err + log.Debugf("os/user.LookupGroupId(%q) failed, trying getent: %v", gid, err) + + g, _, getentErr := groupLookup(gid) + if getentErr != nil { + log.Debugf("getent fallback for gid %s also failed: %v", gid, getentErr) + return nil, stdErr + } + return g, nil +} + +// GroupIDs returns the IDs of the groups the user is a member of. +// NOTE: unlike the lookups above, which try the standard library first, this +// intentionally tries `id -G` first because without cgo, user.GroupIds only +// reads /etc/group and silently returns incomplete results for NSS users +// (no error, just missing groups). The id command goes through NSS and +// returns the full set. +func GroupIDs(u *user.User) ([]string, error) { + ids, err := idGroups(u.Username) + if err == nil { + return ids, nil + } + + log.Debugf("id -G %q failed, falling back to user.GroupIds(): %v", u.Username, err) + + ids, stdErr := u.GroupIds() + if stdErr != nil { + return nil, stdErr + } + return ids, nil +} diff --git a/client/internal/getent/unix.go b/client/internal/getent/unix.go new file mode 100644 index 000000000..7d29810f5 --- /dev/null +++ b/client/internal/getent/unix.go @@ -0,0 +1,224 @@ +//go:build !windows + +package getent + +import ( + "bufio" + "context" + "fmt" + "os" + "os/exec" + "os/user" + "runtime" + "strings" + "time" + + log "github.com/sirupsen/logrus" +) + +const commandTimeout = 5 * time.Second + +// groupFile lists which accounts are in which group, for hosts where the +// getent command is not available (macOS ships without it). +const groupFile = "/etc/group" + +// UserShell returns the login shell getent reports for the user with this UID. +// It reaches shells that /etc/passwd does not list, because getent resolves +// through the host's NSS stack. +func UserShell(uid string) (string, error) { + _, shell, err := passwdLookup(uid) + if err != nil { + return "", err + } + return shell, nil +} + +// GroupMembers returns the names of the group's members: from getent, which +// resolves through NSS, or from /etc/group where getent is not available. A +// group neither source describes is an error; an empty member list is not, +// since accounts with the group as their primary one are not listed in it. +func GroupMembers(name string) ([]string, error) { + _, members, err := groupLookup(name) + if err == nil { + return members, nil + } + log.Debugf("getent cannot list group %q, reading %s: %v", name, groupFile, err) + return groupMembersFromFile(groupFile, name) +} + +// passwdLookup executes `getent passwd `, where query is a username or +// UID, and returns the user and login shell. +func passwdLookup(query string) (*user.User, string, error) { + out, err := run("passwd", query) + if err != nil { + return nil, "", err + } + return parsePasswd(string(out)) +} + +// groupLookup executes `getent group `, where query is a group name or +// GID, and returns the group and its member names. +func groupLookup(query string) (*user.Group, []string, error) { + out, err := run("group", query) + if err != nil { + return nil, nil, err + } + return parseGroup(string(out)) +} + +// run executes `getent ` with a timeout. +func run(database, key string) ([]byte, error) { + if !validateInput(key) { + return nil, fmt.Errorf("invalid getent input: %q", key) + } + + ctx, cancel := context.WithTimeout(context.Background(), commandTimeout) + defer cancel() + + out, err := exec.CommandContext(ctx, "getent", database, key).Output() + if err != nil { + return nil, fmt.Errorf("getent %s %s: %w", database, key, err) + } + return out, nil +} + +// parsePasswd parses getent passwd output: "name:x:uid:gid:gecos:home:shell" +func parsePasswd(output string) (*user.User, string, error) { + fields := strings.SplitN(strings.TrimSpace(output), ":", 8) + if len(fields) < 6 { + return nil, "", fmt.Errorf("unexpected getent output (need 6+ fields): %q", output) + } + + if fields[0] == "" || fields[2] == "" || fields[3] == "" { + return nil, "", fmt.Errorf("missing required fields in getent output: %q", output) + } + + var shell string + if len(fields) >= 7 { + shell = fields[6] + } + + return &user.User{ + Username: fields[0], + Uid: fields[2], + Gid: fields[3], + Name: fields[4], + HomeDir: fields[5], + }, shell, nil +} + +// parseGroup parses getent group output: "name:x:gid:member,member" +func parseGroup(output string) (*user.Group, []string, error) { + fields := strings.SplitN(strings.TrimSpace(output), ":", 4) + if len(fields) < 3 { + return nil, nil, fmt.Errorf("unexpected getent output (need 3+ fields): %q", output) + } + + if fields[0] == "" || fields[2] == "" { + return nil, nil, fmt.Errorf("missing required fields in getent output: %q", output) + } + + var members []string + if len(fields) >= 4 { + members = splitMembers(fields[3]) + } + return &user.Group{Name: fields[0], Gid: fields[2]}, members, nil +} + +func splitMembers(list string) []string { + var members []string + for member := range strings.SplitSeq(list, ",") { + if member != "" { + members = append(members, member) + } + } + return members +} + +// groupMembersFromFile finds the group's member list in a file of /etc/group's +// format. A group the file does not describe, because it comes from LDAP or +// another NSS source, is an error rather than an empty list. +func groupMembersFromFile(path, name string) ([]string, error) { + file, err := os.Open(path) + if err != nil { + return nil, fmt.Errorf("open %s: %w", path, err) + } + defer func() { + if err := file.Close(); err != nil { + log.Debugf("close %s: %v", path, err) + } + }() + + scanner := bufio.NewScanner(file) + for scanner.Scan() { + // name:password:gid:member,member + fields := strings.Split(scanner.Text(), ":") + if len(fields) < 4 || fields[0] != name { + continue + } + return splitMembers(fields[3]), nil + } + if err := scanner.Err(); err != nil { + return nil, fmt.Errorf("read %s: %w", path, err) + } + return nil, fmt.Errorf("%s does not describe group %q", path, name) +} + +// validateInput checks that the input is safe to pass to getent or id. +// Allows POSIX usernames, numeric IDs, and common NSS extensions +// (@ for Kerberos, $ for Samba, + for NIS compat). A leading hyphen is +// rejected so the input can never be parsed as a command-line flag. +func validateInput(input string) bool { + maxLen := 32 + if runtime.GOOS == "linux" { + maxLen = 256 + } + + if len(input) == 0 || len(input) > maxLen { + return false + } + + if input[0] == '-' { + return false + } + + for _, r := range input { + if isAllowedChar(r) { + continue + } + return false + } + return true +} + +func isAllowedChar(r rune) bool { + if r >= 'a' && r <= 'z' || r >= 'A' && r <= 'Z' || r >= '0' && r <= '9' { + return true + } + switch r { + case '.', '_', '-', '@', '+', '$': + return true + } + return false +} + +// idGroups runs `id -G ` and returns the space-separated group IDs. +func idGroups(username string) ([]string, error) { + if !validateInput(username) { + return nil, fmt.Errorf("invalid username for id command: %q", username) + } + + ctx, cancel := context.WithTimeout(context.Background(), commandTimeout) + defer cancel() + + out, err := exec.CommandContext(ctx, "id", "-G", username).Output() + if err != nil { + return nil, fmt.Errorf("id -G %s: %w", username, err) + } + + trimmed := strings.TrimSpace(string(out)) + if trimmed == "" { + return nil, fmt.Errorf("id -G %s: empty output", username) + } + return strings.Fields(trimmed), nil +} diff --git a/client/ssh/server/getent_unix_test.go b/client/internal/getent/unix_test.go similarity index 63% rename from client/ssh/server/getent_unix_test.go rename to client/internal/getent/unix_test.go index a73214e17..5ab100ce5 100644 --- a/client/ssh/server/getent_unix_test.go +++ b/client/internal/getent/unix_test.go @@ -1,10 +1,12 @@ //go:build !windows -package server +package getent import ( + "os" "os/exec" "os/user" + "path/filepath" "runtime" "strconv" "testing" @@ -13,7 +15,7 @@ import ( "github.com/stretchr/testify/require" ) -func TestParseGetentPasswd(t *testing.T) { +func TestParsePasswd(t *testing.T) { tests := []struct { name string input string @@ -128,7 +130,7 @@ func TestParseGetentPasswd(t *testing.T) { for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - u, shell, err := parseGetentPasswd(tt.input) + u, shell, err := parsePasswd(tt.input) if tt.wantErr { require.Error(t, err) if tt.errContains != "" { @@ -147,7 +149,120 @@ func TestParseGetentPasswd(t *testing.T) { } } -func TestValidateGetentInput(t *testing.T) { +func TestParseGroup(t *testing.T) { + tests := []struct { + name string + input string + wantGroup *user.Group + wantMembers []string + wantErr bool + }{ + { + name: "no members", + input: "vma:x:1000:\n", + wantGroup: &user.Group{Name: "vma", Gid: "1000"}, + }, + { + name: "one member", + input: "sudo:x:27:alice", + wantGroup: &user.Group{Name: "sudo", Gid: "27"}, + wantMembers: []string{"alice"}, + }, + { + name: "several members", + input: "docker:x:998:alice,bob\n", + wantGroup: &user.Group{Name: "docker", Gid: "998"}, + wantMembers: []string{"alice", "bob"}, + }, + { + name: "too few fields", + input: "bad:x", + wantErr: true, + }, + { + name: "empty group name", + input: ":x:1000:alice", + wantErr: true, + }, + { + name: "empty GID", + input: "vma:x::alice", + wantErr: true, + }, + { + name: "empty input", + input: "", + wantErr: true, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + g, members, err := parseGroup(tt.input) + if tt.wantErr { + require.Error(t, err) + return + } + require.NoError(t, err) + assert.Equal(t, tt.wantGroup.Name, g.Name, "group name") + assert.Equal(t, tt.wantGroup.Gid, g.Gid, "GID") + assert.Equal(t, tt.wantMembers, members, "members") + }) + } +} + +func TestGroupMembersFromFile(t *testing.T) { + tests := []struct { + name string + entry string + want []string + }{ + {name: "no members", entry: "vma:x:1000:"}, + {name: "only the owner", entry: "vma:x:1000:vma", want: []string{"vma"}}, + {name: "two members", entry: "vma:x:1000:vma,bob", want: []string{"vma", "bob"}}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + path := filepath.Join(t.TempDir(), "group") + body := "root:x:0:\n" + tt.entry + "\nsudo:x:27:vma\n" + require.NoError(t, os.WriteFile(path, []byte(body), 0o644), "write the group file") + + members, err := groupMembersFromFile(path, "vma") + require.NoError(t, err, "entry %q", tt.entry) + assert.Equal(t, tt.want, members, "entry %q", tt.entry) + }) + } +} + +// A group the file does not describe, because it comes from LDAP or another +// NSS source, is an error rather than an empty member list: the caller must +// be able to tell "no members" from "no answer". +func TestGroupMembersFromFileUnknownGroup(t *testing.T) { + path := filepath.Join(t.TempDir(), "group") + require.NoError(t, os.WriteFile(path, []byte("root:x:0:\n"), 0o644), "write the group file") + + _, err := groupMembersFromFile(path, "vma") + assert.Error(t, err, "a group the file does not describe") + + _, err = groupMembersFromFile(filepath.Join(t.TempDir(), "absent"), "vma") + assert.Error(t, err, "no group file at all") +} + +// GroupMembers on the root group, which every Unix has, whichever source +// answers for it. +func TestGroupMembers_RootGroup(t *testing.T) { + rootGroup := "root" + switch runtime.GOOS { + case "darwin", "dragonfly", "freebsd", "netbsd", "openbsd": + rootGroup = "wheel" + } + + _, err := GroupMembers(rootGroup) + assert.NoError(t, err, "the %s group must be describable", rootGroup) +} + +func TestValidateInput(t *testing.T) { tests := []struct { name string input string @@ -180,7 +295,7 @@ func TestValidateGetentInput(t *testing.T) { for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - assert.Equal(t, tt.want, validateGetentInput(tt.input)) + assert.Equal(t, tt.want, validateInput(tt.input)) }) } } @@ -193,12 +308,12 @@ func makeLongString(n int) string { return string(b) } -func TestRunGetent_RootUser(t *testing.T) { +func TestPasswdLookup_RootUser(t *testing.T) { if _, err := exec.LookPath("getent"); err != nil { t.Skip("getent not available on this system") } - u, shell, err := runGetent("root") + u, shell, err := passwdLookup("root") require.NoError(t, err) assert.Equal(t, "root", u.Username) assert.Equal(t, "0", u.Uid) @@ -206,44 +321,55 @@ func TestRunGetent_RootUser(t *testing.T) { assert.NotEmpty(t, shell, "root should have a shell") } -func TestRunGetent_ByUID(t *testing.T) { +func TestPasswdLookup_ByUID(t *testing.T) { if _, err := exec.LookPath("getent"); err != nil { t.Skip("getent not available on this system") } - u, _, err := runGetent("0") + u, _, err := passwdLookup("0") require.NoError(t, err) assert.Equal(t, "root", u.Username) assert.Equal(t, "0", u.Uid) } -func TestRunGetent_NonexistentUser(t *testing.T) { +func TestPasswdLookup_NonexistentUser(t *testing.T) { if _, err := exec.LookPath("getent"); err != nil { t.Skip("getent not available on this system") } - _, _, err := runGetent("nonexistent_user_xyzzy_12345") + _, _, err := passwdLookup("nonexistent_user_xyzzy_12345") assert.Error(t, err) } -func TestRunGetent_InvalidInput(t *testing.T) { - _, _, err := runGetent("") +func TestPasswdLookup_InvalidInput(t *testing.T) { + _, _, err := passwdLookup("") assert.Error(t, err) - _, _, err = runGetent("user\x00name") + _, _, err = passwdLookup("user\x00name") assert.Error(t, err) } -func TestRunGetent_NotAvailable(t *testing.T) { +func TestPasswdLookup_NotAvailable(t *testing.T) { if _, err := exec.LookPath("getent"); err == nil { t.Skip("getent is available, can't test missing case") } - _, _, err := runGetent("root") + _, _, err := passwdLookup("root") assert.Error(t, err, "should fail when getent is not installed") } -func TestRunIdGroups_CurrentUser(t *testing.T) { +func TestGroupLookup_RootGroup(t *testing.T) { + if _, err := exec.LookPath("getent"); err != nil { + t.Skip("getent not available on this system") + } + + g, _, err := groupLookup("0") + require.NoError(t, err) + assert.Equal(t, "0", g.Gid, "GID 0 resolves to the root group") + assert.NotEmpty(t, g.Name, "the root group has a name") +} + +func TestIdGroups_CurrentUser(t *testing.T) { if _, err := exec.LookPath("id"); err != nil { t.Skip("id not available on this system") } @@ -251,7 +377,7 @@ func TestRunIdGroups_CurrentUser(t *testing.T) { current, err := user.Current() require.NoError(t, err) - groups, err := runIdGroups(current.Username) + groups, err := idGroups(current.Username) require.NoError(t, err) require.NotEmpty(t, groups, "current user should have at least one group") @@ -261,20 +387,20 @@ func TestRunIdGroups_CurrentUser(t *testing.T) { } } -func TestRunIdGroups_NonexistentUser(t *testing.T) { +func TestIdGroups_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") + _, err := idGroups("nonexistent_user_xyzzy_12345") assert.Error(t, err) } -func TestRunIdGroups_InvalidInput(t *testing.T) { - _, err := runIdGroups("") +func TestIdGroups_InvalidInput(t *testing.T) { + _, err := idGroups("") assert.Error(t, err) - _, err = runIdGroups("user\x00name") + _, err = idGroups("user\x00name") assert.Error(t, err) } @@ -286,7 +412,7 @@ func TestGetentResultsMatchStdlib(t *testing.T) { current, err := user.Current() require.NoError(t, err) - getentUser, _, err := runGetent(current.Username) + getentUser, _, err := passwdLookup(current.Username) require.NoError(t, err) assert.Equal(t, current.Username, getentUser.Username, "username should match") @@ -303,7 +429,7 @@ func TestGetentResultsMatchStdlib_ByUID(t *testing.T) { current, err := user.Current() require.NoError(t, err) - getentUser, _, err := runGetent(current.Uid) + getentUser, _, err := passwdLookup(current.Uid) require.NoError(t, err) assert.Equal(t, current.Username, getentUser.Username, "username should match when looked up by UID") @@ -323,12 +449,12 @@ func TestIdGroupsMatchStdlib(t *testing.T) { t.Skip("os/user.GroupIds() not working, likely CGO_ENABLED=0") } - idGroups, err := runIdGroups(current.Username) + idGroupIDs, err := idGroups(current.Username) require.NoError(t, err) // Deduplicate both lists: id -G can return duplicates (e.g., root in Docker) // and ElementsMatch treats duplicates as distinct. - assert.ElementsMatch(t, uniqueStrings(stdGroups), uniqueStrings(idGroups), "id -G should return same groups as os/user") + assert.ElementsMatch(t, uniqueStrings(stdGroups), uniqueStrings(idGroupIDs), "id -G should return same groups as os/user") } func uniqueStrings(ss []string) []string { @@ -343,71 +469,3 @@ func uniqueStrings(ss []string) []string { } return out } - -// TestGetShellFromPasswd_CurrentUser verifies that getShellFromPasswd correctly -// reads the current user's shell from /etc/passwd by comparing it against what -// getent reports (which goes through NSS). -func TestGetShellFromPasswd_CurrentUser(t *testing.T) { - current, err := user.Current() - require.NoError(t, err) - - shell := getShellFromPasswd(current.Uid) - if shell == "" { - t.Skip("current user not found in /etc/passwd (may be an NSS-only user)") - } - - assert.True(t, shell[0] == '/', "shell should be an absolute path, got %q", shell) - - if _, err := exec.LookPath("getent"); err == nil { - _, getentShell, getentErr := runGetent(current.Uid) - if getentErr == nil && getentShell != "" { - assert.Equal(t, getentShell, shell, "shell from /etc/passwd should match getent") - } - } -} - -// TestGetShellFromPasswd_RootUser verifies that getShellFromPasswd can read -// root's shell from /etc/passwd. Root is guaranteed to be in /etc/passwd on -// any standard Unix system. -func TestGetShellFromPasswd_RootUser(t *testing.T) { - shell := getShellFromPasswd("0") - require.NotEmpty(t, shell, "root (UID 0) must be in /etc/passwd") - assert.True(t, shell[0] == '/', "root shell should be an absolute path, got %q", shell) -} - -// TestGetShellFromPasswd_NonexistentUID verifies that getShellFromPasswd -// returns empty for a UID that doesn't exist in /etc/passwd. -func TestGetShellFromPasswd_NonexistentUID(t *testing.T) { - shell := getShellFromPasswd("4294967294") - assert.Empty(t, shell, "nonexistent UID should return empty shell") -} - -// TestGetShellFromPasswd_MatchesGetentForKnownUsers reads /etc/passwd directly -// and cross-validates every entry against getent to ensure parseGetentPasswd -// and getShellFromPasswd agree on shell values. -func TestGetShellFromPasswd_MatchesGetentForKnownUsers(t *testing.T) { - if _, err := exec.LookPath("getent"); err != nil { - t.Skip("getent not available") - } - - // Pick a few well-known system UIDs that are virtually always in /etc/passwd. - uids := []string{"0"} // root - - current, err := user.Current() - require.NoError(t, err) - uids = append(uids, current.Uid) - - for _, uid := range uids { - passwdShell := getShellFromPasswd(uid) - if passwdShell == "" { - continue - } - - _, getentShell, err := runGetent(uid) - if err != nil { - continue - } - - assert.Equal(t, getentShell, passwdShell, "shell mismatch for UID %s", uid) - } -} diff --git a/client/internal/getent/windows.go b/client/internal/getent/windows.go new file mode 100644 index 000000000..61881d162 --- /dev/null +++ b/client/internal/getent/windows.go @@ -0,0 +1,36 @@ +//go:build windows + +package getent + +import ( + "errors" + "os/user" +) + +// Windows does not use NSS or getent; os/user resolves accounts there +// without cgo, so everything delegates to it. + +// LookupUser looks up a user by name. +func LookupUser(username string) (*user.User, error) { + return user.Lookup(username) +} + +// LookupUserID looks up a user by UID. +func LookupUserID(uid string) (*user.User, error) { + return user.LookupId(uid) +} + +// CurrentUser returns the user this process runs as. +func CurrentUser() (*user.User, error) { + return user.Current() +} + +// GroupIDs returns the IDs of the groups the user is a member of. +func GroupIDs(u *user.User) ([]string, error) { + return u.GroupIds() +} + +// UserShell is unanswerable on Windows, which has no login-shell database. +func UserShell(string) (string, error) { + return "", errors.ErrUnsupported +} diff --git a/client/internal/ipcauth/privileged.go b/client/internal/ipcauth/privileged.go index 95f2a50e9..3c2e68432 100644 --- a/client/internal/ipcauth/privileged.go +++ b/client/internal/ipcauth/privileged.go @@ -91,6 +91,12 @@ func SelfDelegatesTo() (Identity, bool) { return selfIdentity, true } +// The values PrivilegedActorKey returns. +const ( + ActorKeyAdministrator = "administrator" + ActorKeyRoot = "root" +) + // PrivilegedActor names the principal a privileged operation requires, for use // in messages shown to the user. func PrivilegedActor() string { @@ -100,6 +106,16 @@ func PrivilegedActor() string { return "root" } +// PrivilegedActorKey identifies that principal without wording it, for a client +// that writes its own message in the user's language. The words PrivilegedActor +// returns are English, and a translated sentence cannot borrow them. +func PrivilegedActorKey() string { + if runtime.GOOS == "windows" { + return ActorKeyAdministrator + } + return ActorKeyRoot +} + // ElevatedCommand renders a command so that running it grants the privileges the // operation needs. Windows has no in-line equivalent of sudo, so the command is // returned unchanged and the user is expected to run it from an elevated diff --git a/client/ssh/server/getent_cgo_unix.go b/client/ssh/server/getent_cgo_unix.go deleted file mode 100644 index 4afbfc627..000000000 --- a/client/ssh/server/getent_cgo_unix.go +++ /dev/null @@ -1,24 +0,0 @@ -//go:build cgo && !osusergo && !windows - -package server - -import "os/user" - -// lookupWithGetent with CGO delegates directly to os/user.Lookup. -// When CGO is enabled, os/user uses libc (getpwnam_r) which goes through -// the NSS stack natively. If it fails, the user truly doesn't exist and -// getent would also fail. -func lookupWithGetent(username string) (*user.User, error) { - return user.Lookup(username) -} - -// currentUserWithGetent with CGO delegates directly to os/user.Current. -func currentUserWithGetent() (*user.User, error) { - return user.Current() -} - -// groupIdsWithFallback with CGO delegates directly to user.GroupIds. -// libc's getgrouplist handles NSS groups natively. -func groupIdsWithFallback(u *user.User) ([]string, error) { - return u.GroupIds() -} diff --git a/client/ssh/server/getent_nocgo_unix.go b/client/ssh/server/getent_nocgo_unix.go deleted file mode 100644 index 314daae4c..000000000 --- a/client/ssh/server/getent_nocgo_unix.go +++ /dev/null @@ -1,74 +0,0 @@ -//go:build (!cgo || osusergo) && !windows - -package server - -import ( - "os" - "os/user" - "strconv" - - log "github.com/sirupsen/logrus" -) - -// lookupWithGetent looks up a user by name, falling back to getent if os/user fails. -// Without CGO, os/user only reads /etc/passwd and misses NSS-provided users. -// getent goes through the host's NSS stack. -func lookupWithGetent(username string) (*user.User, error) { - u, err := user.Lookup(username) - if err == nil { - return u, nil - } - - stdErr := err - log.Debugf("os/user.Lookup(%q) failed, trying getent: %v", username, err) - - u, _, getentErr := runGetent(username) - if getentErr != nil { - log.Debugf("getent fallback for %q also failed: %v", username, getentErr) - return nil, stdErr - } - - return u, nil -} - -// currentUserWithGetent gets the current user, falling back to getent if os/user fails. -func currentUserWithGetent() (*user.User, error) { - u, err := user.Current() - if err == nil { - return u, nil - } - - stdErr := err - uid := strconv.Itoa(os.Getuid()) - log.Debugf("os/user.Current() failed, trying getent with UID %s: %v", uid, err) - - u, _, getentErr := runGetent(uid) - if getentErr != nil { - return nil, stdErr - } - - return u, nil -} - -// groupIdsWithFallback gets group IDs for a user via the id command first, -// falling back to user.GroupIds(). -// NOTE: unlike lookupWithGetent/currentUserWithGetent which try stdlib first, -// this intentionally tries `id -G` first because without CGO, user.GroupIds() -// only reads /etc/group and silently returns incomplete results for NSS users -// (no error, just missing groups). The id command goes through NSS and returns -// the full set. -func groupIdsWithFallback(u *user.User) ([]string, error) { - ids, err := runIdGroups(u.Username) - if err == nil { - return ids, nil - } - - log.Debugf("id -G %q failed, falling back to user.GroupIds(): %v", u.Username, err) - - ids, stdErr := u.GroupIds() - if stdErr != nil { - return nil, stdErr - } - - return ids, nil -} diff --git a/client/ssh/server/getent_unix.go b/client/ssh/server/getent_unix.go deleted file mode 100644 index a3a9641f8..000000000 --- a/client/ssh/server/getent_unix.go +++ /dev/null @@ -1,127 +0,0 @@ -//go:build !windows - -package server - -import ( - "context" - "fmt" - "os/exec" - "os/user" - "runtime" - "strings" - "time" -) - -const getentTimeout = 5 * time.Second - -// getShellFromGetent gets a user's login shell via getent by UID. -// This is needed even with CGO because getShellFromPasswd reads /etc/passwd -// directly and won't find NSS-provided users there. -func getShellFromGetent(userID string) string { - _, shell, err := runGetent(userID) - if err != nil { - return "" - } - return shell -} - -// runGetent executes `getent passwd ` and returns the user and login shell. -func runGetent(query string) (*user.User, string, error) { - if !validateGetentInput(query) { - return nil, "", fmt.Errorf("invalid getent input: %q", query) - } - - ctx, cancel := context.WithTimeout(context.Background(), getentTimeout) - defer cancel() - - out, err := exec.CommandContext(ctx, "getent", "passwd", query).Output() - if err != nil { - return nil, "", fmt.Errorf("getent passwd %s: %w", query, err) - } - - return parseGetentPasswd(string(out)) -} - -// parseGetentPasswd parses getent passwd output: "name:x:uid:gid:gecos:home:shell" -func parseGetentPasswd(output string) (*user.User, string, error) { - fields := strings.SplitN(strings.TrimSpace(output), ":", 8) - if len(fields) < 6 { - return nil, "", fmt.Errorf("unexpected getent output (need 6+ fields): %q", output) - } - - if fields[0] == "" || fields[2] == "" || fields[3] == "" { - return nil, "", fmt.Errorf("missing required fields in getent output: %q", output) - } - - var shell string - if len(fields) >= 7 { - shell = fields[6] - } - - return &user.User{ - Username: fields[0], - Uid: fields[2], - Gid: fields[3], - Name: fields[4], - HomeDir: fields[5], - }, shell, nil -} - -// validateGetentInput checks that the input is safe to pass to getent or id. -// Allows POSIX usernames, numeric UIDs, and common NSS extensions -// (@ for Kerberos, $ for Samba, + for NIS compat). A leading hyphen is -// rejected so the input can never be parsed as a command-line flag. -func validateGetentInput(input string) bool { - maxLen := 32 - if runtime.GOOS == "linux" { - maxLen = 256 - } - - if len(input) == 0 || len(input) > maxLen { - return false - } - - if input[0] == '-' { - return false - } - - for _, r := range input { - if isAllowedGetentChar(r) { - continue - } - return false - } - return true -} - -func isAllowedGetentChar(r rune) bool { - if r >= 'a' && r <= 'z' || r >= 'A' && r <= 'Z' || r >= '0' && r <= '9' { - return true - } - switch r { - case '.', '_', '-', '@', '+', '$': - return true - } - return false -} - -// runIdGroups runs `id -G ` and returns the space-separated group IDs. -func runIdGroups(username string) ([]string, error) { - if !validateGetentInput(username) { - return nil, fmt.Errorf("invalid username for id command: %q", username) - } - - ctx, cancel := context.WithTimeout(context.Background(), getentTimeout) - defer cancel() - - out, err := exec.CommandContext(ctx, "id", "-G", username).Output() - if err != nil { - return nil, fmt.Errorf("id -G %s: %w", username, err) - } - - trimmed := strings.TrimSpace(string(out)) - if trimmed == "" { - return nil, fmt.Errorf("id -G %s: empty output", username) - } - return strings.Fields(trimmed), nil -} diff --git a/client/ssh/server/getent_windows.go b/client/ssh/server/getent_windows.go deleted file mode 100644 index 3e76b3e8e..000000000 --- a/client/ssh/server/getent_windows.go +++ /dev/null @@ -1,26 +0,0 @@ -//go:build windows - -package server - -import "os/user" - -// lookupWithGetent on Windows just delegates to os/user.Lookup. -// Windows does not use NSS/getent; its user lookup works without CGO. -func lookupWithGetent(username string) (*user.User, error) { - return user.Lookup(username) -} - -// currentUserWithGetent on Windows just delegates to os/user.Current. -func currentUserWithGetent() (*user.User, error) { - return user.Current() -} - -// getShellFromGetent is a no-op on Windows; shell resolution uses PowerShell detection. -func getShellFromGetent(_ string) string { - return "" -} - -// groupIdsWithFallback on Windows just delegates to u.GroupIds(). -func groupIdsWithFallback(u *user.User) ([]string, error) { - return u.GroupIds() -} diff --git a/client/ssh/server/shell.go b/client/ssh/server/shell.go index 1e8ff5e31..7b356b2a0 100644 --- a/client/ssh/server/shell.go +++ b/client/ssh/server/shell.go @@ -13,6 +13,8 @@ import ( "github.com/gliderlabs/ssh" log "github.com/sirupsen/logrus" + + "github.com/netbirdio/netbird/client/internal/getent" ) const ( @@ -56,7 +58,11 @@ func getUnixUserShell(userID string) string { return shell } - if shell := getShellFromGetent(userID); shell != "" { + shell, err := getent.UserShell(userID) + if err != nil { + log.Debugf("look up the shell for uid %s through getent: %v", userID, err) + } + if shell != "" { return shell } diff --git a/client/ssh/server/shell_unix_test.go b/client/ssh/server/shell_unix_test.go new file mode 100644 index 000000000..c5e65e535 --- /dev/null +++ b/client/ssh/server/shell_unix_test.go @@ -0,0 +1,94 @@ +//go:build !windows + +package server + +import ( + "os/exec" + "os/user" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + "github.com/netbirdio/netbird/client/internal/getent" +) + +// TestGetShellFromPasswd_CurrentUser verifies that getShellFromPasswd correctly +// reads the current user's shell from /etc/passwd by comparing it against what +// getent reports (which goes through NSS). +func TestGetShellFromPasswd_CurrentUser(t *testing.T) { + current, err := user.Current() + require.NoError(t, err) + + shell := getShellFromPasswd(current.Uid) + if shell == "" { + t.Skip("current user not found in /etc/passwd (may be an NSS-only user)") + } + + assert.True(t, shell[0] == '/', "shell should be an absolute path, got %q", shell) + + if _, err := exec.LookPath("getent"); err == nil { + getentShell, getentErr := getent.UserShell(current.Uid) + if getentErr == nil && getentShell != "" { + assert.Equal(t, getentShell, shell, "shell from /etc/passwd should match getent") + } + } +} + +// TestGetShellFromPasswd_RootUser verifies that getShellFromPasswd can read +// root's shell from /etc/passwd. Root is guaranteed to be in /etc/passwd on +// any standard Unix system. +func TestGetShellFromPasswd_RootUser(t *testing.T) { + shell := getShellFromPasswd("0") + require.NotEmpty(t, shell, "root (UID 0) must be in /etc/passwd") + assert.True(t, shell[0] == '/', "root shell should be an absolute path, got %q", shell) +} + +// TestGetShellFromPasswd_NonexistentUID verifies that getShellFromPasswd +// returns empty for a UID that doesn't exist in /etc/passwd. +func TestGetShellFromPasswd_NonexistentUID(t *testing.T) { + shell := getShellFromPasswd("4294967294") + assert.Empty(t, shell, "nonexistent UID should return empty shell") +} + +// TestGetShellFromPasswd_MatchesGetentForKnownUsers reads /etc/passwd directly +// and cross-validates every entry against getent to ensure the two shell +// sources agree. +func TestGetShellFromPasswd_MatchesGetentForKnownUsers(t *testing.T) { + if _, err := exec.LookPath("getent"); err != nil { + t.Skip("getent not available") + } + + // Pick a few well-known system UIDs that are virtually always in /etc/passwd. + uids := []string{"0"} // root + + current, err := user.Current() + require.NoError(t, err) + uids = append(uids, current.Uid) + + for _, uid := range uids { + passwdShell := getShellFromPasswd(uid) + if passwdShell == "" { + continue + } + + getentShell, err := getent.UserShell(uid) + if err != nil { + continue + } + + assert.Equal(t, getentShell, passwdShell, "shell mismatch for UID %s", uid) + } +} + +// TestIntegration_ShellLookupChain tests the full shell resolution chain +// (getShellFromPasswd -> getent -> $SHELL -> default). +func TestIntegration_ShellLookupChain(t *testing.T) { + current, err := user.Current() + require.NoError(t, err) + + // getUserShell is the top-level function used by the SSH server. + shell := getUserShell(current.Uid) + require.NotEmpty(t, shell, "getUserShell must always return a shell") + assert.True(t, shell[0] == '/', "shell should be an absolute path, got %q", shell) +} diff --git a/client/ssh/server/user_utils.go b/client/ssh/server/user_utils.go index 6c8142b30..f2f33b3d7 100644 --- a/client/ssh/server/user_utils.go +++ b/client/ssh/server/user_utils.go @@ -9,6 +9,8 @@ import ( "strings" log "github.com/sirupsen/logrus" + + "github.com/netbirdio/netbird/client/internal/getent" ) var ( @@ -18,8 +20,8 @@ var ( // Dependency injection variables for testing - allows mocking dynamic runtime checks var ( - getCurrentUser = currentUserWithGetent - lookupUser = lookupWithGetent + getCurrentUser = getent.CurrentUser + lookupUser = getent.LookupUser getCurrentOS = func() string { return runtime.GOOS } getIsProcessPrivileged = isCurrentProcessPrivileged diff --git a/client/ssh/server/userswitching_unix.go b/client/ssh/server/userswitching_unix.go index 220e2240f..ae60ec64c 100644 --- a/client/ssh/server/userswitching_unix.go +++ b/client/ssh/server/userswitching_unix.go @@ -16,6 +16,8 @@ import ( "github.com/gliderlabs/ssh" log "github.com/sirupsen/logrus" + + "github.com/netbirdio/netbird/client/internal/getent" ) // POSIX portable filename character set regex: [a-zA-Z0-9._-] @@ -160,7 +162,7 @@ func (s *Server) parseUserCredentials(localUser *user.User) (uint32, uint32, []u // getSupplementaryGroups retrieves supplementary group IDs for a user. // Uses id/getent fallback for NSS users in CGO_ENABLED=0 builds. func (s *Server) getSupplementaryGroups(u *user.User) ([]uint32, error) { - groupIDStrings, err := groupIdsWithFallback(u) + groupIDStrings, err := getent.GroupIDs(u) if err != nil { return nil, fmt.Errorf("get group IDs for user %s: %w", u.Username, err) } diff --git a/client/ui/build/linux/netbird.desktop b/client/ui/build/linux/netbird.desktop index a81f3698a..0d43b62a2 100644 --- a/client/ui/build/linux/netbird.desktop +++ b/client/ui/build/linux/netbird.desktop @@ -1,5 +1,6 @@ [Desktop Entry] -Name=Netbird +Name=NetBird +Comment=NetBird desktop client Exec=env WEBKIT_DISABLE_DMABUF_RENDERER=1 /usr/bin/netbird-ui Icon=netbird Type=Application diff --git a/client/ui/build/linux/polkit/io.netbird.settings.policy b/client/ui/build/linux/polkit/io.netbird.settings.policy new file mode 100644 index 000000000..e12f1ddc7 --- /dev/null +++ b/client/ui/build/linux/polkit/io.netbird.settings.policy @@ -0,0 +1,47 @@ + + + + + + NetBird + https://netbird.io + + + Change privileged NetBird settings + Authentication is required to change NetBird settings that grant SSH access to this computer. + netbird + + auth_admin + auth_admin + auth_admin + + /usr/bin/netbird-ui + --apply-privileged-settings + + + + Change privileged NetBird settings + Authentication is required to change NetBird settings that grant SSH access to this computer. + netbird + + auth_admin + auth_admin + auth_admin + + /usr/local/bin/netbird-ui + --apply-privileged-settings + + diff --git a/client/ui/frontend/src/contexts/SettingsContext.tsx b/client/ui/frontend/src/contexts/SettingsContext.tsx index 3f4b2d0d2..a7574c7e5 100644 --- a/client/ui/frontend/src/contexts/SettingsContext.tsx +++ b/client/ui/frontend/src/contexts/SettingsContext.tsx @@ -22,12 +22,18 @@ const logSaveError = (err: unknown) => console.error("[SettingsContext] save fai export type AutostartState = { supported: boolean; enabled: boolean }; +// GuardedField is a setting the daemon only accepts from root/administrator. +// Turning one on goes through saveGuardedField, which asks the operating system +// for the privileges rather than sending a request that would be refused. +export type GuardedField = "serverSshAllowed" | "enableSshRoot" | "disableSshAuth"; + type SettingsContextValue = { config: Config; guiVersion: string; setField: (k: K, v: Config[K]) => void; saveField: (k: K, v: Config[K]) => Promise; saveFields: (partial: Partial, opts?: { preSharedKey?: string }) => Promise; + saveGuardedField: (k: GuardedField, v: boolean) => Promise; saveNow: () => Promise; }; @@ -63,6 +69,12 @@ const useSettingsState = () => { const [guiVersion, setGuiVersion] = useState("—"); const saveTimer = useRef | null>(null); const loadedRef = useRef(null); + // Set when the daemon's config changed while a save was pending, so the read + // that was skipped to protect the pending edit happens once it is through. + // Without it the form keeps values the daemon no longer has and the next save + // submits them, which for a guarded setting means asking the user to authorize + // a change they never made. + const reloadOwed = useRef(false); useEffect(() => { loadedRef.current = loaded; @@ -73,6 +85,7 @@ const useSettingsState = () => { // update the daemon then rejected. const reload = useCallback( async (profileName: string) => { + reloadOwed.current = false; try { const data = await SettingsSvc.GetConfig({ profileName, username }); setLoaded({ profileName, data }); @@ -94,7 +107,12 @@ const useSettingsState = () => { username, }); if (cancelled) return; - if (saveTimer.current) return; + // A pending edit outranks the daemon's copy until it is saved, so + // the read is owed rather than dropped: see reloadOwed. + if (saveTimer.current) { + reloadOwed.current = true; + return; + } setLoaded({ profileName: activeProfileId, data }); } catch (e) { if (cancelled || !showError) return; @@ -141,12 +159,17 @@ const useSettingsState = () => { async (profileName: string, next: Config, preSharedKey?: string) => { const preSharedKeyWrite = preSharedKey === undefined ? {} : { preSharedKey }; try { - await SettingsSvc.SetConfig({ + const { declined } = await SettingsSvc.SetConfig({ ...next, ...preSharedKeyWrite, profileName, username, }); + // The change needed authorization and the user said no, so the + // optimistic update is wrong. Nothing to report: they know. + if (declined || reloadOwed.current) { + await reload(profileName); + } } catch (e) { // The optimistic update is wrong now: the daemon refused it // (a change that needs elevated privileges, an MDM-managed @@ -206,6 +229,59 @@ const useSettingsState = () => { [loaded, save], ); + // saveGuardedField applies a setting the daemon restricts to + // root/administrator by having the Go side run the app again under the + // platform's elevation prompt (UAC, the macOS authentication dialog, polkit). + // The prompt is the user's, so the call is made straight from their gesture + // and never from the debounce. + const saveGuardedField = useCallback( + async (k: GuardedField, v: boolean) => { + const cur = loadedRef.current; + if (!cur) return; + + // Flush what the debounce still owes, before the optimistic update + // below joins it: a later save carrying the guarded value would be + // refused, and its error dialog would be the second one for a change + // the user already authorized. + if (saveTimer.current) { + clearTimeout(saveTimer.current); + saveTimer.current = null; + await save(cur.profileName, cur.data); + } + + const next: LoadedConfig = { + profileName: cur.profileName, + data: { ...cur.data, [k]: v }, + }; + loadedRef.current = next; + setLoaded(next); + + try { + await SettingsSvc.SetGuardedSettings({ + profileName: cur.profileName, + username, + [k]: v, + }); + } catch (e) { + // The daemon is authoritative either way, so re-read before + // reporting. A declined prompt is not an error and does not come + // through here at all; this is a prompt that could not be raised, + // which carries the command that would have done it. + await reload(cur.profileName); + await errorDialog({ + Title: i18next.t("settings.error.saveTitle"), + Message: errorMessage(e), + Command: errorCommand(e), + }); + return; + } + // Either the change went through or the user declined it. The daemon + // says which. + await reload(cur.profileName); + }, + [username, save, reload], + ); + const saveFields = useCallback( async (partial: Partial, opts?: { preSharedKey?: string }) => { if (!loaded) return; @@ -225,15 +301,27 @@ const useSettingsState = () => { [loaded, save], ); - return { config: loaded?.data ?? null, guiVersion, setField, saveField, saveFields, saveNow }; + return { + config: loaded?.data ?? null, + guiVersion, + setField, + saveField, + saveFields, + saveGuardedField, + saveNow, + }; }; export const SettingsProvider = ({ children }: { children: ReactNode }) => { - const { config, guiVersion, setField, saveField, saveFields, saveNow } = useSettingsState(); + const { config, guiVersion, setField, saveField, saveFields, saveGuardedField, saveNow } = + useSettingsState(); const value = useMemo( - () => (config ? { config, guiVersion, setField, saveField, saveFields, saveNow } : null), - [config, guiVersion, setField, saveField, saveFields, saveNow], + () => + config + ? { config, guiVersion, setField, saveField, saveFields, saveGuardedField, saveNow } + : null, + [config, guiVersion, setField, saveField, saveFields, saveGuardedField, saveNow], ); if (!value) { diff --git a/client/ui/frontend/src/hooks/usePrivilege.ts b/client/ui/frontend/src/hooks/usePrivilege.ts index 05e9a7ce0..d67fcc4b1 100644 --- a/client/ui/frontend/src/hooks/usePrivilege.ts +++ b/client/ui/frontend/src/hooks/usePrivilege.ts @@ -1,6 +1,6 @@ import { useEffect, useState } from "react"; import { Settings as SettingsSvc } from "@bindings/services"; -import { Privilege } from "@bindings/services/models.js"; +import { type Privilege } from "@bindings/services/models.js"; // usePrivilege reports whether this UI process may perform the changes the daemon // restricts to root/administrator. It is answered in-process from our own token diff --git a/client/ui/frontend/src/modules/settings/SettingsSSH.tsx b/client/ui/frontend/src/modules/settings/SettingsSSH.tsx index bd91e520c..d74afae73 100644 --- a/client/ui/frontend/src/modules/settings/SettingsSSH.tsx +++ b/client/ui/frontend/src/modules/settings/SettingsSSH.tsx @@ -1,3 +1,4 @@ +import { type TFunction } from "i18next"; import { useTranslation } from "react-i18next"; import { CopyToClipboard } from "@/components/CopyToClipboard"; import FancyToggleSwitch from "@/components/switches/FancyToggleSwitch"; @@ -6,51 +7,91 @@ import { Input } from "@/components/inputs/Input"; import { Label } from "@/components/typography/Label"; import { cn } from "@/lib/cn"; import { SectionGroup } from "@/modules/settings/SettingsSection.tsx"; -import { useSettings } from "@/contexts/SettingsContext.tsx"; +import { type GuardedField, useSettings } from "@/contexts/SettingsContext.tsx"; import { usePrivilege } from "@/hooks/usePrivilege.ts"; -import { Privilege } from "@bindings/services/models.js"; +import type { Privilege } from "@bindings/services/models.js"; import { type ChangeEvent, type ReactNode, useEffect, useId, useState } from "react"; export function SettingsSSH() { const { t } = useTranslation(); - const { config, setField } = useSettings(); + const { config, setField, saveGuardedField } = useSettings(); const privilege = usePrivilege(); + // The field whose elevation prompt is currently up, if any. The prompt is + // modal to the operating system, not to us, so the guarded controls are held + // still meanwhile rather than allowed to stack a second one behind it. + const [authorizing, setAuthorizing] = useState(null); const isSSHServerEnabled = config.serverSshAllowed; + const authorize = async (field: GuardedField, value: boolean) => { + setAuthorizing(field); + try { + await saveGuardedField(field, value); + } finally { + setAuthorizing(null); + } + }; + // The daemon restricts only the direction that hands out shells from a process - // running as root. So for an unprivileged user a guarded control is either - // unavailable (it is off and only they could turn it on) or a one-way switch - // (it is on, they may turn it off, but not back on) — say which, either way. + // running as root: for all three settings that is switching the field on. + // + // An unprivileged user gets that direction routed through the platform's + // elevation prompt where there is one to raise, and otherwise the old + // arrangement, where the control is either unavailable (it is off and only a + // privileged caller could turn it on) or a one-way switch (it is on, they may + // turn it off but not back on) with the command that does it. // // A null privilege means we could not determine it: leave the control alone // rather than greying it out with nothing to explain why. The daemon enforces // this regardless, and a rejected save reports its own guidance. const guarded = ( - guardedDirectionActive: boolean, + field: GuardedField, command: (p: Privilege) => string, // inverted marks a control whose guarded direction is switching it off, so // the one-way warning has to read the other way round. inverted = false, ) => { + const plain = (value: boolean) => setField(field, value); if (!privilege || privilege.privileged) { - return { disabled: false, hint: undefined }; + return { apply: plain, disabled: false, hint: undefined }; } - const hint = ( - ( + ); - return { disabled: !guardedDirectionActive, hint }; + + if (privilege.canElevate) { + return { + // Switching off is ours to do; only switching on is authorized. + apply: (value: boolean) => { + if (!value) { + plain(value); + return; + } + void authorize(field, value); + }, + disabled: authorizing !== null, + hint: hint(authorizing === field), + }; + } + return { + apply: plain, + disabled: !guardedDirectionActive, + hint: hint(false, command(privilege)), + }; }; - const sshServer = guarded(config.serverSshAllowed, (p) => p.allowSshServer); - const sshRoot = guarded(config.enableSshRoot, (p) => p.enableSshRoot); + const sshServer = guarded("serverSshAllowed", (p) => p.allowSshServer); + const sshRoot = guarded("enableSshRoot", (p) => p.enableSshRoot); // Inverted control: the guarded direction is switching authentication off, so // it is the already-disabled state that is the one-way one. - const sshAuth = guarded(config.disableSshAuth, (p) => p.disableSshAuth, true); + const sshAuth = guarded("disableSshAuth", (p) => p.disableSshAuth, true); const jwtTtlId = useId(); const [jwtTtlInput, setJwtTtlInput] = useState(String(config.sshJwtCacheTtl)); @@ -84,7 +125,7 @@ export function SettingsSSH() { setField("serverSshAllowed", v)} + onChange={sshServer.apply} disabled={sshServer.disabled} label={t("settings.ssh.server.label")} helpText={t("settings.ssh.server.help")} @@ -98,7 +139,7 @@ export function SettingsSSH() { > setField("enableSshRoot", v)} + onChange={sshRoot.apply} disabled={sshRoot.disabled} label={t("settings.ssh.root.label")} helpText={t("settings.ssh.root.help")} @@ -130,7 +171,7 @@ export function SettingsSSH() { > setField("disableSshAuth", !v)} + onChange={(v) => sshAuth.apply(!v)} disabled={sshAuth.disabled} label={t("settings.ssh.jwt.label")} helpText={t("settings.ssh.jwt.help")} @@ -163,41 +204,81 @@ export function SettingsSSH() { ); } -// PrivilegeHint explains what an unprivileged user can and cannot do with a -// guarded control, and offers the command that does it with the privileges the -// daemon requires. oneWay covers the control being in the guarded state already: -// switching it back is the part that needs privileges. -function PrivilegeHint({ +// actorLabel names the principal the daemon requires, in the user's language. The +// Go side reports which one it is rather than wording it, because "administrator +// privileges" is English and a translated sentence cannot borrow it. +function actorLabel(privilege: Privilege, t: TFunction): string { + return privilege.actorKey === "administrator" + ? t("settings.ssh.privilege.actorAdministrator") + : t("settings.ssh.privilege.actorRoot"); +} + +// GuardedHint is what a control the daemon guards says to an unprivileged user. +// There are three things worth saying, and it says at most one: +// +// - A prompt is open. Worth a line because it can take a few seconds to appear, +// long enough that a control which merely went inert would read as a hang. +// - The setting is in its guarded state already (oneWay), so the user may switch +// it back as they please and it is switching it away again that will ask. No +// command either way: the direction they can take is theirs to take. +// - Only a privileged caller can move it at all, and there is no prompt to +// raise: the command that does it belongs here, and nothing else will do. +// +// Which leaves the case of a control whose guarded direction is still ahead of the +// user and a prompt that can be raised for it: nothing to say, because clicking it +// raises the prompt and the prompt explains itself. +function GuardedHint({ actor, - command, oneWay, inverted, + pending, + command, }: { actor: string; - command: string; oneWay: boolean; inverted: boolean; + pending: boolean; + command?: string; }): ReactNode { const { t } = useTranslation(); + + if (pending) { + return {t("settings.ssh.privilege.authorizePending")}; + } + if (oneWay) { + return ( + + + {inverted + ? t("settings.ssh.privilege.oneWayInverted", { actor }) + : t("settings.ssh.privilege.oneWay", { actor })} + + + ); + } if (!command) return null; + return ( + + {t("settings.ssh.privilege.hint", { actor })} + + + {command} + + + + ); +} + +// HintBox is the box a guarded control puts its explanation in, directly under the +// control it belongs to. +function HintBox({ children }: { children: ReactNode }): ReactNode { return (
- - {!oneWay - ? t("settings.ssh.privilege.hint", { actor }) - : inverted - ? t("settings.ssh.privilege.oneWayInverted", { actor }) - : t("settings.ssh.privilege.oneWay", { actor })} - - - - {command} - - + {children}
); } diff --git a/client/ui/i18n/locales/de/common.json b/client/ui/i18n/locales/de/common.json index 1208a37fe..11e085927 100644 --- a/client/ui/i18n/locales/de/common.json +++ b/client/ui/i18n/locales/de/common.json @@ -401,9 +401,6 @@ "networks.bulk.label": { "message": "Alle sichtbaren Ressourcen umschalten" }, - "settings.nav.label": { - "message": "Einstellungsbereiche" - }, "profile.switch.title": { "message": "Zu Profil \"{name}\" wechseln?" }, @@ -497,6 +494,9 @@ "settings.error.debugBundleTitle": { "message": "Debug-Paket fehlgeschlagen" }, + "settings.nav.label": { + "message": "Einstellungsbereiche" + }, "settings.tabs.general": { "message": "Allgemein" }, @@ -1351,13 +1351,28 @@ "error.unknown": { "message": "Vorgang fehlgeschlagen." }, + "error.elevation_unavailable": { + "message": "NetBird konnte auf diesem System nicht die nötigen Rechte anfordern. Führen Sie stattdessen dies aus:" + }, + "error.elevation_failed": { + "message": "Die Änderung konnte mit erhöhten Rechten nicht angewendet werden. Führen Sie stattdessen dies aus:" + }, + "settings.ssh.privilege.actorRoot": { + "message": "root-Rechte" + }, + "settings.ssh.privilege.actorAdministrator": { + "message": "Administratorrechte" + }, "settings.ssh.privilege.hint": { "message": "Erfordert {actor}. Führen Sie stattdessen dies aus:" }, "settings.ssh.privilege.oneWay": { - "message": "Sie können dies deaktivieren, aber zum erneuten Aktivieren sind {actor} erforderlich:" + "message": "Sie können dies deaktivieren, zum erneuten Aktivieren sind {actor} erforderlich." }, "settings.ssh.privilege.oneWayInverted": { - "message": "Sie können dies aktivieren, aber zum erneuten Deaktivieren sind {actor} erforderlich:" + "message": "Sie können dies aktivieren, zum erneuten Deaktivieren sind {actor} erforderlich." + }, + "settings.ssh.privilege.authorizePending": { + "message": "Warten auf Autorisierung…" } } diff --git a/client/ui/i18n/locales/en/common.json b/client/ui/i18n/locales/en/common.json index 694444497..36f00e4bd 100644 --- a/client/ui/i18n/locales/en/common.json +++ b/client/ui/i18n/locales/en/common.json @@ -1799,16 +1799,36 @@ "message": "Operation failed.", "description": "Generic fallback error message used when no specific error applies." }, + "error.elevation_unavailable": { + "message": "NetBird could not ask this system for the privileges the change needs. Run this instead:", + "description": "Error: this computer has no way to prompt for elevated privileges. Followed by a copyable command that applies the setting from a terminal." + }, + "error.elevation_failed": { + "message": "The change could not be applied with elevated privileges. Run this instead:", + "description": "Error: the authorization succeeded but applying the setting afterwards failed. Followed by a copyable command that applies the setting from a terminal." + }, + "settings.ssh.privilege.actorRoot": { + "message": "root", + "description": "Fills {actor} in the settings.ssh.privilege.* messages on Linux, macOS and BSD, where the daemon requires the root account. 'root' is an account name and stays as it is; add the word for privileges or rights around it if the sentence needs one to read naturally." + }, + "settings.ssh.privilege.actorAdministrator": { + "message": "administrator privileges", + "description": "Fills {actor} in the settings.ssh.privilege.* messages on Windows, where the daemon requires an elevated administrator. The Windows term for the rights an account is asked to elevate to." + }, "settings.ssh.privilege.hint": { "message": "Requires {actor}. Run this instead:", "description": "Help text under an SSH setting the user cannot change: it needs elevated privileges. {actor} is 'root' on Linux/macOS or 'administrator privileges' on Windows. Followed by a copyable command." }, "settings.ssh.privilege.oneWay": { - "message": "You can switch this off, but switching it back on needs {actor}:", - "description": "Warning under an SSH setting an unprivileged user may disable but not re-enable. {actor} is 'root' on Linux/macOS or 'administrator privileges' on Windows. Followed by a copyable command." + "message": "You can switch this off, but switching it back on needs {actor}.", + "description": "Help text under an SSH setting that is already on: an unprivileged user may switch it off freely, and switching it on again is what needs the privileges. No command follows, since the direction they can take is theirs to take. {actor} is 'root' on Linux/macOS or 'administrator privileges' on Windows." }, "settings.ssh.privilege.oneWayInverted": { - "message": "You can switch this on, but switching it back off needs {actor}:", - "description": "Warning under the SSH authentication setting, which an unprivileged user may re-enable but not disable again. {actor} is 'root' on Linux/macOS or 'administrator privileges' on Windows. Followed by a copyable command." + "message": "You can switch this on, but switching it back off needs {actor}.", + "description": "Same as settings.ssh.privilege.oneWay, for the SSH authentication setting once it has been switched off: switching it off again is what needs the privileges." + }, + "settings.ssh.privilege.authorizePending": { + "message": "Waiting for authorization…", + "description": "Replaces the help text under a guarded SSH setting while the authorization prompt is open, which can take a few seconds to appear. Keep the trailing ellipsis." } } diff --git a/client/ui/i18n/locales/es/common.json b/client/ui/i18n/locales/es/common.json index 6dc4ffd0b..41872d7a0 100644 --- a/client/ui/i18n/locales/es/common.json +++ b/client/ui/i18n/locales/es/common.json @@ -401,9 +401,6 @@ "networks.bulk.label": { "message": "Conmutar todos los recursos visibles" }, - "settings.nav.label": { - "message": "Secciones de configuración" - }, "profile.switch.title": { "message": "¿Cambiar el perfil a «{name}»?" }, @@ -497,6 +494,9 @@ "settings.error.debugBundleTitle": { "message": "Error en el paquete de diagnóstico" }, + "settings.nav.label": { + "message": "Secciones de configuración" + }, "settings.tabs.general": { "message": "General" }, @@ -1351,13 +1351,28 @@ "error.unknown": { "message": "La operación falló." }, + "error.elevation_unavailable": { + "message": "NetBird no pudo solicitar a este sistema los privilegios necesarios. Ejecute esto en su lugar:" + }, + "error.elevation_failed": { + "message": "No se pudo aplicar el cambio con privilegios elevados. Ejecute esto en su lugar:" + }, + "settings.ssh.privilege.actorRoot": { + "message": "privilegios de root" + }, + "settings.ssh.privilege.actorAdministrator": { + "message": "privilegios de administrador" + }, "settings.ssh.privilege.hint": { "message": "Requiere {actor}. Ejecute esto en su lugar:" }, "settings.ssh.privilege.oneWay": { - "message": "Puede desactivarlo, pero volver a activarlo requiere {actor}:" + "message": "Puede desactivarlo, pero volver a activarlo requiere {actor}." }, "settings.ssh.privilege.oneWayInverted": { - "message": "Puede activarlo, pero volver a desactivarlo requiere {actor}:" + "message": "Puede activarlo, pero volver a desactivarlo requiere {actor}." + }, + "settings.ssh.privilege.authorizePending": { + "message": "Esperando la autorización…" } } diff --git a/client/ui/i18n/locales/fr/common.json b/client/ui/i18n/locales/fr/common.json index d3e54440c..920ef8343 100644 --- a/client/ui/i18n/locales/fr/common.json +++ b/client/ui/i18n/locales/fr/common.json @@ -401,9 +401,6 @@ "networks.bulk.label": { "message": "Activer/désactiver toutes les ressources visibles" }, - "settings.nav.label": { - "message": "Sections des paramètres" - }, "profile.switch.title": { "message": "Basculer vers le profil « {name} » ?" }, @@ -497,6 +494,9 @@ "settings.error.debugBundleTitle": { "message": "Échec du lot de diagnostic" }, + "settings.nav.label": { + "message": "Sections des paramètres" + }, "settings.tabs.general": { "message": "Général" }, @@ -1351,13 +1351,28 @@ "error.unknown": { "message": "L’opération a échoué." }, + "error.elevation_unavailable": { + "message": "NetBird n’a pas pu demander à ce système les privilèges nécessaires. Exécutez plutôt ceci :" + }, + "error.elevation_failed": { + "message": "La modification n’a pas pu être appliquée avec des privilèges élevés. Exécutez plutôt ceci :" + }, + "settings.ssh.privilege.actorRoot": { + "message": "les privilèges root" + }, + "settings.ssh.privilege.actorAdministrator": { + "message": "les privilèges administrateur" + }, "settings.ssh.privilege.hint": { "message": "Nécessite {actor}. Exécutez plutôt ceci :" }, "settings.ssh.privilege.oneWay": { - "message": "Vous pouvez le désactiver, mais le réactiver nécessite {actor} :" + "message": "Vous pouvez le désactiver, mais le réactiver nécessite {actor}." }, "settings.ssh.privilege.oneWayInverted": { - "message": "Vous pouvez l’activer, mais le désactiver de nouveau nécessite {actor} :" + "message": "Vous pouvez l’activer, mais le désactiver de nouveau nécessite {actor}." + }, + "settings.ssh.privilege.authorizePending": { + "message": "En attente de l’autorisation…" } } diff --git a/client/ui/i18n/locales/hu/common.json b/client/ui/i18n/locales/hu/common.json index 19aede17f..82996e3d3 100644 --- a/client/ui/i18n/locales/hu/common.json +++ b/client/ui/i18n/locales/hu/common.json @@ -401,9 +401,6 @@ "networks.bulk.label": { "message": "Összes látható erőforrás be/ki" }, - "settings.nav.label": { - "message": "Beállítások szakaszai" - }, "profile.switch.title": { "message": "Váltás a(z) \"{name}\" profilra?" }, @@ -497,6 +494,9 @@ "settings.error.debugBundleTitle": { "message": "Hibakeresési csomag sikertelen" }, + "settings.nav.label": { + "message": "Beállítások szakaszai" + }, "settings.tabs.general": { "message": "Általános" }, @@ -1351,13 +1351,28 @@ "error.unknown": { "message": "A művelet meghiúsult." }, + "error.elevation_unavailable": { + "message": "A NetBird nem tudta bekérni a rendszertől a szükséges jogosultságokat. Futtassa inkább ezt:" + }, + "error.elevation_failed": { + "message": "A módosítást emelt szintű jogosultságokkal sem sikerült alkalmazni. Futtassa inkább ezt:" + }, + "settings.ssh.privilege.actorRoot": { + "message": "root jogosultság" + }, + "settings.ssh.privilege.actorAdministrator": { + "message": "rendszergazdai jogosultság" + }, "settings.ssh.privilege.hint": { "message": "{actor} szükséges hozzá. Futtassa inkább ezt:" }, "settings.ssh.privilege.oneWay": { - "message": "Kikapcsolhatja, de a visszakapcsolásához {actor} szükséges:" + "message": "Kikapcsolhatja, de a visszakapcsolásához {actor} szükséges." }, "settings.ssh.privilege.oneWayInverted": { - "message": "Bekapcsolhatja, de az ismételt kikapcsolásához {actor} szükséges:" + "message": "Bekapcsolhatja, de az ismételt kikapcsolásához {actor} szükséges." + }, + "settings.ssh.privilege.authorizePending": { + "message": "Várakozás az engedélyezésre…" } } diff --git a/client/ui/i18n/locales/it/common.json b/client/ui/i18n/locales/it/common.json index dab9e0cb4..b8166aa6e 100644 --- a/client/ui/i18n/locales/it/common.json +++ b/client/ui/i18n/locales/it/common.json @@ -401,9 +401,6 @@ "networks.bulk.label": { "message": "Attiva/disattiva tutte le risorse visibili" }, - "settings.nav.label": { - "message": "Sezioni delle impostazioni" - }, "profile.switch.title": { "message": "Passare al profilo «{name}»?" }, @@ -497,6 +494,9 @@ "settings.error.debugBundleTitle": { "message": "Pacchetto di debug non riuscito" }, + "settings.nav.label": { + "message": "Sezioni delle impostazioni" + }, "settings.tabs.general": { "message": "Generale" }, @@ -1351,13 +1351,28 @@ "error.unknown": { "message": "Operazione non riuscita." }, + "error.elevation_unavailable": { + "message": "NetBird non ha potuto richiedere a questo sistema i privilegi necessari. Esegua invece questo:" + }, + "error.elevation_failed": { + "message": "Non è stato possibile applicare la modifica con privilegi elevati. Esegua invece questo:" + }, + "settings.ssh.privilege.actorRoot": { + "message": "i privilegi di root" + }, + "settings.ssh.privilege.actorAdministrator": { + "message": "i privilegi di amministratore" + }, "settings.ssh.privilege.hint": { "message": "Richiede {actor}. Esegua invece questo:" }, "settings.ssh.privilege.oneWay": { - "message": "Può disabilitarlo, ma riabilitarlo richiede {actor}:" + "message": "Può disabilitarlo, ma riabilitarlo richiede {actor}." }, "settings.ssh.privilege.oneWayInverted": { - "message": "Può abilitarlo, ma disabilitarlo di nuovo richiede {actor}:" + "message": "Può abilitarlo, ma disabilitarlo di nuovo richiede {actor}." + }, + "settings.ssh.privilege.authorizePending": { + "message": "In attesa dell'autorizzazione…" } } diff --git a/client/ui/i18n/locales/ja/common.json b/client/ui/i18n/locales/ja/common.json index 246c232a8..6ffe05e1c 100644 --- a/client/ui/i18n/locales/ja/common.json +++ b/client/ui/i18n/locales/ja/common.json @@ -1351,13 +1351,28 @@ "error.unknown": { "message": "操作に失敗しました。" }, + "error.elevation_unavailable": { + "message": "NetBird はこのシステムに必要な権限を要求できませんでした。代わりに次のコマンドを実行してください:" + }, + "error.elevation_failed": { + "message": "昇格した権限でも変更を適用できませんでした。代わりに次のコマンドを実行してください:" + }, + "settings.ssh.privilege.actorRoot": { + "message": "root 権限" + }, + "settings.ssh.privilege.actorAdministrator": { + "message": "管理者権限" + }, "settings.ssh.privilege.hint": { "message": "{actor}が必要です。代わりに次のコマンドを実行してください:" }, "settings.ssh.privilege.oneWay": { - "message": "無効にはできますが、再度有効にするには{actor}が必要です:" + "message": "無効にはできますが、再度有効にするには{actor}が必要です。" }, "settings.ssh.privilege.oneWayInverted": { - "message": "有効にはできますが、再度無効にするには{actor}が必要です:" + "message": "有効にはできますが、再度無効にするには{actor}が必要です。" + }, + "settings.ssh.privilege.authorizePending": { + "message": "承認を待っています…" } } diff --git a/client/ui/i18n/locales/pt/common.json b/client/ui/i18n/locales/pt/common.json index 418e93717..123e7a042 100644 --- a/client/ui/i18n/locales/pt/common.json +++ b/client/ui/i18n/locales/pt/common.json @@ -401,9 +401,6 @@ "networks.bulk.label": { "message": "Alternar todos os recursos visíveis" }, - "settings.nav.label": { - "message": "Seções das configurações" - }, "profile.switch.title": { "message": "Alternar perfil para \"{name}\"?" }, @@ -497,6 +494,9 @@ "settings.error.debugBundleTitle": { "message": "Falha no pacote de depuração" }, + "settings.nav.label": { + "message": "Seções das configurações" + }, "settings.tabs.general": { "message": "Geral" }, @@ -1351,13 +1351,28 @@ "error.unknown": { "message": "A operação falhou." }, + "error.elevation_unavailable": { + "message": "O NetBird não conseguiu solicitar a este sistema os privilégios necessários. Execute isto em vez disso:" + }, + "error.elevation_failed": { + "message": "Não foi possível aplicar a alteração com privilégios elevados. Execute isto em vez disso:" + }, + "settings.ssh.privilege.actorRoot": { + "message": "privilégios de root" + }, + "settings.ssh.privilege.actorAdministrator": { + "message": "privilégios de administrador" + }, "settings.ssh.privilege.hint": { "message": "Requer {actor}. Execute isto em vez disso:" }, "settings.ssh.privilege.oneWay": { - "message": "Você pode desativar isto, mas ativar novamente requer {actor}:" + "message": "Você pode desativar isto, mas ativar novamente requer {actor}." }, "settings.ssh.privilege.oneWayInverted": { - "message": "Você pode ativar isto, mas desativar novamente requer {actor}:" + "message": "Você pode ativar isto, mas desativar novamente requer {actor}." + }, + "settings.ssh.privilege.authorizePending": { + "message": "Aguardando a autorização…" } } diff --git a/client/ui/i18n/locales/ru/common.json b/client/ui/i18n/locales/ru/common.json index 958b5a21c..3881a3783 100644 --- a/client/ui/i18n/locales/ru/common.json +++ b/client/ui/i18n/locales/ru/common.json @@ -401,9 +401,6 @@ "networks.bulk.label": { "message": "Переключить все видимые ресурсы" }, - "settings.nav.label": { - "message": "Разделы настроек" - }, "profile.switch.title": { "message": "Переключиться на профиль «{name}»?" }, @@ -497,6 +494,9 @@ "settings.error.debugBundleTitle": { "message": "Не удалось создать отладочный пакет" }, + "settings.nav.label": { + "message": "Разделы настроек" + }, "settings.tabs.general": { "message": "Общие" }, @@ -1351,13 +1351,28 @@ "error.unknown": { "message": "Не удалось выполнить операцию." }, + "error.elevation_unavailable": { + "message": "NetBird не смог запросить у этой системы нужные права. Выполните вместо этого:" + }, + "error.elevation_failed": { + "message": "Не удалось применить изменение с повышенными правами. Выполните вместо этого:" + }, + "settings.ssh.privilege.actorRoot": { + "message": "права root" + }, + "settings.ssh.privilege.actorAdministrator": { + "message": "права администратора" + }, "settings.ssh.privilege.hint": { "message": "Требуются {actor}. Выполните вместо этого:" }, "settings.ssh.privilege.oneWay": { - "message": "Отключить можно, но чтобы включить снова, нужны {actor}:" + "message": "Отключить можно, но чтобы включить снова, нужны {actor}." }, "settings.ssh.privilege.oneWayInverted": { - "message": "Включить можно, но чтобы отключить снова, нужны {actor}:" + "message": "Включить можно, но чтобы отключить снова, нужны {actor}." + }, + "settings.ssh.privilege.authorizePending": { + "message": "Ожидание авторизации…" } } diff --git a/client/ui/i18n/locales/zh-CN/common.json b/client/ui/i18n/locales/zh-CN/common.json index 90ae5e003..b1ff3370d 100644 --- a/client/ui/i18n/locales/zh-CN/common.json +++ b/client/ui/i18n/locales/zh-CN/common.json @@ -401,9 +401,6 @@ "networks.bulk.label": { "message": "切换所有可见资源" }, - "settings.nav.label": { - "message": "设置部分" - }, "profile.switch.title": { "message": "切换到配置文件“{name}”？" }, @@ -497,6 +494,9 @@ "settings.error.debugBundleTitle": { "message": "创建调试包失败" }, + "settings.nav.label": { + "message": "设置部分" + }, "settings.tabs.general": { "message": "常规" }, @@ -1351,13 +1351,28 @@ "error.unknown": { "message": "操作失败。" }, + "error.elevation_unavailable": { + "message": "NetBird 无法向此系统请求所需的权限。请改为运行：" + }, + "error.elevation_failed": { + "message": "即使使用提升的权限也无法应用此更改。请改为运行：" + }, + "settings.ssh.privilege.actorRoot": { + "message": "root 权限" + }, + "settings.ssh.privilege.actorAdministrator": { + "message": "管理员权限" + }, "settings.ssh.privilege.hint": { "message": "需要{actor}。请改为运行：" }, "settings.ssh.privilege.oneWay": { - "message": "您可以关闭此项，但重新开启需要{actor}：" + "message": "您可以关闭此项，但重新开启需要{actor}。" }, "settings.ssh.privilege.oneWayInverted": { - "message": "您可以开启此项，但再次关闭需要{actor}：" + "message": "您可以开启此项，但再次关闭需要{actor}。" + }, + "settings.ssh.privilege.authorizePending": { + "message": "正在等待授权…" } } diff --git a/client/ui/main.go b/client/ui/main.go index e20bfe074..5652efcf2 100644 --- a/client/ui/main.go +++ b/client/ui/main.go @@ -8,6 +8,7 @@ import ( "flag" "io/fs" "log" + "os" "runtime" "strings" @@ -79,6 +80,14 @@ func init() { } func main() { + // The one-shot that applies the settings the daemon restricts to + // root/administrator, which this binary runs itself as under the platform's + // elevation prompt. Handled before anything GUI so no window, tray or + // single-instance lock is involved. + if services.IsPrivilegedSettingsRun(os.Args[1:]) { + os.Exit(runPrivilegedSettings(os.Args[1:])) + } + daemonAddr, userSetLogFile := parseFlagsAndInitLog() conn := NewConn(daemonAddr) diff --git a/client/ui/privileged_settings.go b/client/ui/privileged_settings.go new file mode 100644 index 000000000..1e8b4bbf6 --- /dev/null +++ b/client/ui/privileged_settings.go @@ -0,0 +1,27 @@ +//go:build !android && !ios && !freebsd && !js + +package main + +import ( + "github.com/netbirdio/netbird/client/proto" + "github.com/netbirdio/netbird/client/ui/services" +) + +// The one-shot mode this binary runs itself in, elevated, to apply the settings the +// daemon restricts to root/administrator. It is handled before anything GUI, so no +// window, tray or single-instance lock is involved. +// +// Only the wiring is here: what the mode accepts and does lives beside the code +// that asks for it, in services.RunPrivilegedSettings, so the settings it will +// apply are declared once. There is nothing privileged about the mode itself; it +// sends the same request the frontend would have sent, and the daemon authorizes it +// from the identity the kernel reports on the control channel exactly as it does +// for `sudo netbird up`. +func runPrivilegedSettings(args []string) int { + return services.RunPrivilegedSettings(args, func(addr string) (proto.DaemonServiceClient, error) { + if addr == "" { + addr = DaemonAddr() + } + return NewConn(addr).Client() + }) +} diff --git a/client/ui/services/guarded.go b/client/ui/services/guarded.go new file mode 100644 index 000000000..f425428b5 --- /dev/null +++ b/client/ui/services/guarded.go @@ -0,0 +1,231 @@ +//go:build !android && !ios && !freebsd && !js + +package services + +import ( + "context" + "errors" + "fmt" + "strings" + "time" + + log "github.com/sirupsen/logrus" + + "github.com/netbirdio/netbird/client/internal/elevate" + "github.com/netbirdio/netbird/client/internal/ipcauth" +) + +// The command line of the one-shot mode this binary runs itself in, elevated, to +// apply a setting the daemon restricts to root/administrator. The setting flags +// spell the same words as `netbird up`, so the command a user is shown and what +// runs behind the prompt read alike. Parsed in oneshot.go. +const ( + FlagApplyPrivilegedSettings = "apply-privileged-settings" + FlagDaemonAddr = "daemon-addr" + FlagProfile = "profile" + FlagUser = "user" + FlagLogLevel = "log-level" + FlagManagementURL = "management-url" + FlagAllowServerSSH = "allow-server-ssh" + FlagEnableSSHRoot = "enable-ssh-root" + FlagDisableSSHAuth = "disable-ssh-auth" +) + +// Error codes for the ways asking for privileges can fail. +const ( + CodeElevationUnavailable = "elevation_unavailable" + CodeElevationFailed = "elevation_failed" +) + +// elevationTimeout bounds the wait for a prompt and the change behind it, so a +// dialog nobody answers does not leave its control disabled for the session. Long +// enough to find a password manager, and no shorter than the platforms' own prompt +// timeouts: Windows gives up on its consent dialog after two minutes by itself. +// +// It always ends our waiting, and not always the prompt: Security.framework offers +// no way to withdraw a request, so on macOS the system's own timeout is what closes +// the dialog. +const elevationTimeout = 5 * time.Minute + +// elevator raises the platform's privilege prompt and runs the change behind it. +// An interface so tests can answer without a prompt. +type elevator interface { + // Run runs this binary again, elevated, with the given arguments. + Run(ctx context.Context, args ...string) error + // Available reports whether there is a prompt to raise on this host at all. + Available() bool +} + +// osElevator is the real thing: see the elevate package. +type osElevator struct{} + +func (osElevator) Run(ctx context.Context, args ...string) error { + return elevate.Run(ctx, args...) +} + +func (osElevator) Available() bool { + return elevate.Available() +} + +// SaveOutcome reports what became of a change that needed authorization. +// +// A declined prompt is a result, not an error: the user was asked and said no, so +// nothing was applied and nothing went wrong. Reporting it as an error would have +// every cancelled prompt logged as one. +type SaveOutcome struct { + // Declined is set when the user dismissed the authorization prompt, or was + // refused by policy. Nothing was changed. + Declined bool `json:"declined"` +} + +// GuardedSettings is the subset of the config the daemon restricts to +// root/administrator. Only the fields that are set are changed: a nil pointer, or +// an empty management URL, leaves that setting alone. +// +// The management URL is in here because pointing a host with the SSH server +// running at another management identity hands the decision of who may open a +// shell on it to whoever runs that server, which is the same power as enabling +// the SSH server in the first place. +type GuardedSettings struct { + ProfileName string `json:"profileName"` + Username string `json:"username"` + ManagementURL string `json:"managementUrl,omitempty"` + ServerSSHAllowed *bool `json:"serverSshAllowed,omitempty"` + EnableSSHRoot *bool `json:"enableSshRoot,omitempty"` + DisableSSHAuth *bool `json:"disableSshAuth,omitempty"` +} + +// guardedSetting is one setting to change, in the two spellings this needs: the +// one-shot's own flag, and the `netbird up` flag that does the same thing from a +// terminal, for when there is no prompt to raise. +type guardedSetting struct { + arg string + flag string +} + +// SetGuardedSettings applies settings the daemon refuses from an unprivileged +// caller, by having the operating system run this binary again, elevated, to send +// the same request the frontend would have sent itself. +// +// The user authorizes it at the platform's own prompt: the UAC consent dialog, +// the macOS authentication dialog, or the polkit agent's. Any credentials are the +// operating system's business; NetBird neither sees nor asks for them. Nothing +// about the daemon's rules changes, and the elevated process is authorized like +// any other privileged caller, from the identity the kernel reports for it. +// +// A declined prompt comes back as SaveOutcome.Declined with no error. When there is +// no prompt to raise, or the elevated run failed, the error carries the command +// that does the same thing from a terminal. +func (s *Settings) SetGuardedSettings(ctx context.Context, p GuardedSettings) (SaveOutcome, error) { + settings := guardedSettings(p) + if len(settings) == 0 { + return SaveOutcome{}, &ClientError{ + Code: CodeElevationFailed, + Short: "no setting to apply", + Long: "no setting to apply", + } + } + + // The elevated run has no window and, on Linux, an environment pkexec has + // cleared, so what it writes to stderr is all there is to go on. It follows + // this process's level so that starting the app with --log-level debug says + // something about the run behind the prompt too. + args := append([]string{ + "--" + FlagApplyPrivilegedSettings, + "--" + FlagDaemonAddr, s.daemonAddr, + "--" + FlagProfile, p.ProfileName, + "--" + FlagUser, p.Username, + "--" + FlagLogLevel, log.GetLevel().String(), + }, oneShotArgs(settings)...) + + ctx, cancel := context.WithTimeout(ctx, elevationTimeout) + defer cancel() + + // These changes hand out shells on this host, so both ends are logged: when the + // prompt went up, and what came of it. It is also the only account of a prompt + // that was slow to appear or never answered. + log.Infof("asking for privileges to apply %s", guardedSummary(p)) + + if err := s.elevator.Run(ctx, args...); err != nil { + return s.elevationOutcome(err, p) + } + + log.Infof("applied %s with the privileges the user authorized", guardedSummary(p)) + return SaveOutcome{}, nil +} + +// elevationOutcome sorts what came back into the one normal ending and the two +// that need reporting, with the command that does the same thing by hand. +func (s *Settings) elevationOutcome(err error, p GuardedSettings) (SaveOutcome, error) { + switch { + case errors.Is(err, elevate.ErrDeclined): + // With the reason: an account that may not elevate at all lands here too, + // and the log is the only place that says which it was. + log.Infof("the elevation prompt for %s was declined: %v", guardedSummary(p), err) + return SaveOutcome{Declined: true}, nil + case errors.Is(err, elevate.ErrUnavailable): + log.Warnf("cannot ask for privileges to apply %s: %v", guardedSummary(p), err) + return SaveOutcome{}, &ClientError{ + Code: CodeElevationUnavailable, + Short: s.classifier.translateShort(CodeElevationUnavailable), + Long: err.Error(), + Command: guardedCommand(p), + } + default: + log.Errorf("applying %s with elevated privileges failed: %v", guardedSummary(p), err) + return SaveOutcome{}, &ClientError{ + Code: CodeElevationFailed, + Short: s.classifier.translateShort(CodeElevationFailed), + Long: err.Error(), + Command: guardedCommand(p), + } + } +} + +// guardedSettings renders the settings that are actually being changed, from the +// same table the one-shot parses them with: see oneshot.go. +func guardedSettings(p GuardedSettings) []guardedSetting { + var settings []guardedSetting + for _, field := range guardedFields { + value, ok := field.read(p) + if !ok { + continue + } + settings = append(settings, guardedSetting{ + arg: "--" + field.flag + "=" + value, + flag: field.up(value), + }) + } + return settings +} + +func oneShotArgs(settings []guardedSetting) []string { + args := make([]string, 0, len(settings)) + for _, setting := range settings { + args = append(args, setting.arg) + } + return args +} + +func upFlags(settings []guardedSetting) []string { + flags := make([]string, 0, len(settings)) + for _, setting := range settings { + flags = append(flags, setting.flag) + } + return flags +} + +// guardedCommand is the elevated command line equivalent to the requested +// change, the same shape the daemon names in its own refusals. +func guardedCommand(p GuardedSettings) string { + settings := guardedSettings(p) + if len(settings) == 0 { + return "" + } + return ipcauth.UpCommand(strings.Join(upFlags(settings), " ")) +} + +// guardedSummary names the change for the log. +func guardedSummary(p GuardedSettings) string { + return fmt.Sprintf("%v for profile %q", oneShotArgs(guardedSettings(p)), p.ProfileName) +} diff --git a/client/ui/services/guarded_test.go b/client/ui/services/guarded_test.go new file mode 100644 index 000000000..42c00ce4f --- /dev/null +++ b/client/ui/services/guarded_test.go @@ -0,0 +1,355 @@ +//go:build !android && !ios && !freebsd && !js + +package services + +import ( + "context" + "errors" + "testing" + + log "github.com/sirupsen/logrus" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + "google.golang.org/genproto/googleapis/rpc/errdetails" + "google.golang.org/grpc" + "google.golang.org/grpc/codes" + gstatus "google.golang.org/grpc/status" + + "github.com/netbirdio/netbird/client/internal/elevate" + "github.com/netbirdio/netbird/client/internal/ipcauth" + "github.com/netbirdio/netbird/client/proto" +) + +// A Unix socket, so the daemon address is one that carries a caller's identity and +// elevation is worth offering at all: see Settings.canElevate. +const testDaemonAddr = "unix:///var/run/netbird.sock" + +// storedManagementURL is what the stub daemon already holds, so that a request +// naming a different one is a change: see Settings.guardedChanges. +const storedManagementURL = "https://stored.example.com" + +// stubElevator stands in for the platform's prompt: it records what would have run +// and answers with a fixed outcome. +type stubElevator struct { + outcome error + available bool + calls [][]string +} + +func (e *stubElevator) Run(_ context.Context, args ...string) error { + e.calls = append(e.calls, args) + return e.outcome +} + +func (e *stubElevator) Available() bool { return e.available } + +// stubDaemon implements only the RPCs under test. The embedded interface is nil, so +// any other call panics rather than passing quietly. +type stubDaemon struct { + proto.DaemonServiceClient + setConfig func(*proto.SetConfigRequest) error + // stored is what GetConfig reports, which is what a refused request's guarded + // settings are compared against. + stored *proto.GetConfigResponse + requests []*proto.SetConfigRequest +} + +func (d *stubDaemon) SetConfig(_ context.Context, in *proto.SetConfigRequest, _ ...grpc.CallOption) (*proto.SetConfigResponse, error) { + d.requests = append(d.requests, in) + if err := d.setConfig(in); err != nil { + return nil, err + } + return &proto.SetConfigResponse{}, nil +} + +func (d *stubDaemon) GetConfig(_ context.Context, _ *proto.GetConfigRequest, _ ...grpc.CallOption) (*proto.GetConfigResponse, error) { + return d.stored, nil +} + +type stubConn struct{ client proto.DaemonServiceClient } + +func (c stubConn) Client() (proto.DaemonServiceClient, error) { return c.client, nil } + +// privilegeRefusal is the error the daemon raises for a change it restricts to +// root, detail and all: see server.privilegeError. +func privilegeRefusal(t *testing.T) error { + t.Helper() + + st, err := gstatus.New(codes.PermissionDenied, "Changing the management URL requires root."). + WithDetails(&errdetails.ErrorInfo{ + Reason: ipcauth.ErrorReasonPrivilegeRequired, + Domain: ipcauth.ErrorDomain, + Metadata: map[string]string{ + ipcauth.ErrorMetaSummary: "Changing the management URL requires root.", + ipcauth.ErrorMetaCommand: "sudo netbird down; sudo netbird up -m https://mgmt.example.com", + }, + }) + require.NoError(t, err, "build the refusal detail") + return st.Err() +} + +func settingsWithElevation(t *testing.T, outcome error) (*Settings, *stubElevator) { + t.Helper() + + elev := &stubElevator{outcome: outcome, available: true} + return &Settings{daemonAddr: testDaemonAddr, elevator: elev}, elev +} + +// settingsRefusingOnce returns a Settings whose daemon refuses the first SetConfig +// for want of privileges and accepts anything after it. Its stored config holds +// another management server and no SSH grants, so a request naming either is a +// change rather than a restatement. +func settingsRefusingOnce(t *testing.T, elev *stubElevator) (*Settings, *stubDaemon) { + t.Helper() + + refusal := privilegeRefusal(t) + daemon := &stubDaemon{stored: &proto.GetConfigResponse{ManagementUrl: storedManagementURL}} + daemon.setConfig = func(*proto.SetConfigRequest) error { + if len(daemon.requests) == 1 { + return refusal + } + return nil + } + return &Settings{conn: stubConn{client: daemon}, daemonAddr: testDaemonAddr, elevator: elev}, daemon +} + +func TestSetGuardedSettingsPassesOnlyTheChangedSettings(t *testing.T) { + s, elev := settingsWithElevation(t, nil) + + root := true + outcome, err := s.SetGuardedSettings(context.Background(), GuardedSettings{ + ProfileName: "work", + Username: "vma", + EnableSSHRoot: &root, + }) + require.NoError(t, err) + assert.False(t, outcome.Declined, "the prompt was answered") + + want := []string{ + "--" + FlagApplyPrivilegedSettings, + "--" + FlagDaemonAddr, testDaemonAddr, + "--" + FlagProfile, "work", + "--" + FlagUser, "vma", + "--" + FlagLogLevel, log.GetLevel().String(), + "--" + FlagEnableSSHRoot + "=true", + } + require.Len(t, elev.calls, 1, "one prompt for one change") + assert.Equal(t, want, elev.calls[0], "elevated arguments") + + // argv[1] is what the polkit action is pinned to, so the marker has to stay + // first however the rest of the line grows. + assert.Equal(t, "--"+FlagApplyPrivilegedSettings, elev.calls[0][0], "the flag polkit matches on") +} + +// Turning a setting off has to be as explicit as turning it on: a bare flag would +// read as "on" to the one-shot's parser. +func TestSetGuardedSettingsSpellsOutFalse(t *testing.T) { + s, elev := settingsWithElevation(t, nil) + + off := false + _, err := s.SetGuardedSettings(context.Background(), GuardedSettings{ + ProfileName: "default", + ServerSSHAllowed: &off, + DisableSSHAuth: &off, + }) + require.NoError(t, err) + + args := elev.calls[0] + assert.Contains(t, args, "--"+FlagAllowServerSSH+"=false", "the setting being switched off") + assert.Contains(t, args, "--"+FlagDisableSSHAuth+"=false", "the setting being switched off") + assert.NotContains(t, args, "--"+FlagEnableSSHRoot+"=false", "no flag for a setting nobody touched") +} + +func TestSetGuardedSettingsPassesTheManagementURL(t *testing.T) { + s, elev := settingsWithElevation(t, nil) + + _, err := s.SetGuardedSettings(context.Background(), GuardedSettings{ + ProfileName: "default", + ManagementURL: "https://mgmt.example.com:33073", + }) + require.NoError(t, err) + + assert.Contains(t, elev.calls[0], "--"+FlagManagementURL+"=https://mgmt.example.com:33073", + "the management URL to point the profile at") +} + +func TestSetGuardedSettingsWithoutASettingDoesNotElevate(t *testing.T) { + s, elev := settingsWithElevation(t, nil) + + _, err := s.SetGuardedSettings(context.Background(), GuardedSettings{ProfileName: "default"}) + + require.Error(t, err, "nothing to apply is not something to prompt for") + assert.Empty(t, elev.calls, "no prompt at all") +} + +// A declined prompt is the one ending that is not an error: reporting it as one +// would have every cancelled prompt logged as a failure. +func TestSetGuardedSettingsReportsADeclinedPromptAsAnOutcome(t *testing.T) { + s, _ := settingsWithElevation(t, elevate.ErrDeclined) + + root := true + outcome, err := s.SetGuardedSettings(context.Background(), GuardedSettings{ + ProfileName: "default", + EnableSSHRoot: &root, + }) + + require.NoError(t, err, "the user was asked and answered; nothing went wrong") + assert.True(t, outcome.Declined, "nothing was applied") +} + +func TestSetGuardedSettingsMapsFailures(t *testing.T) { + tests := []struct { + name string + outcome error + wantCode string + }{ + { + // Nothing to raise a prompt with: the user needs the command. + name: "no mechanism falls back to the command", + outcome: elevate.ErrUnavailable, + wantCode: CodeElevationUnavailable, + }, + { + name: "a failed run falls back to the command", + outcome: errors.New("elevated netbird exited with 1"), + wantCode: CodeElevationFailed, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + s, _ := settingsWithElevation(t, tt.outcome) + + root := true + _, err := s.SetGuardedSettings(context.Background(), GuardedSettings{ + ProfileName: "default", + EnableSSHRoot: &root, + }) + + var clientErr *ClientError + require.ErrorAs(t, err, &clientErr, "the frontend needs a code to act on") + assert.Equal(t, tt.wantCode, clientErr.Code, "error code") + assert.Contains(t, clientErr.Command, "--"+FlagEnableSSHRoot+"=true", + "the setting in the fallback command") + assert.Contains(t, clientErr.Command, "netbird up", "the fallback command") + }) + } +} + +// Changing the management URL is only privileged while the host runs the SSH +// server, which no control can know up front, so the refusal is what triggers the +// prompt. The original request goes again afterwards, so the fields the one-shot +// does not understand are applied too. +func TestSetConfigElevatesAfterARefusalAndRetries(t *testing.T) { + elev := &stubElevator{available: true} + s, daemon := settingsRefusingOnce(t, elev) + + mtu := int64(1280) + outcome, err := s.SetConfig(context.Background(), SetConfigParams{ + ProfileName: "default", + ManagementURL: "https://mgmt.example.com", + MTU: &mtu, + }) + require.NoError(t, err) + assert.False(t, outcome.Declined, "the prompt was answered") + + require.Len(t, elev.calls, 1, "one prompt") + assert.Contains(t, elev.calls[0], "--"+FlagManagementURL+"=https://mgmt.example.com", + "the guarded part of the request") + require.Len(t, daemon.requests, 2, "the refused request and the retry") + assert.Equal(t, mtu, daemon.requests[1].GetMtu(), + "the retry carries the rest of the request, which the one-shot does not understand") +} + +func TestSetConfigDoesNotRetryWhenTheUserDeclines(t *testing.T) { + elev := &stubElevator{outcome: elevate.ErrDeclined, available: true} + s, daemon := settingsRefusingOnce(t, elev) + + outcome, err := s.SetConfig(context.Background(), SetConfigParams{ + ProfileName: "default", + ManagementURL: "https://mgmt.example.com", + }) + + require.NoError(t, err, "a declined prompt is not an error") + assert.True(t, outcome.Declined, "nothing was applied") + assert.Len(t, daemon.requests, 1, "only the refused request") +} + +// With no prompt to raise, the refusal is reported as the daemon wrote it, which is +// the guidance that was there before elevation existed. +func TestSetConfigReportsTheRefusalWhenItCannotElevate(t *testing.T) { + elev := &stubElevator{available: false} + s, _ := settingsRefusingOnce(t, elev) + + _, err := s.SetConfig(context.Background(), SetConfigParams{ + ProfileName: "default", + ManagementURL: "https://mgmt.example.com", + }) + + var clientErr *ClientError + require.ErrorAs(t, err, &clientErr) + assert.Equal(t, "privilege_required", clientErr.Code, "error code") + assert.Contains(t, clientErr.Command, "netbird up -m https://mgmt.example.com", + "the daemon's own command") + assert.Empty(t, elev.calls, "no prompt where there is none to raise") +} + +// One authorization must buy only the change the user made. A settings form +// submits every field it holds, so most of a refused request restates what the +// daemon already has, and elevating those too would apply a guarded setting the +// user never touched — a value gone stale since the form loaded above all. +func TestSetConfigElevatesOnlyTheGuardedSettingsThatChange(t *testing.T) { + elev := &stubElevator{available: true} + s, _ := settingsRefusingOnce(t, elev) + + on, off := true, false + _, err := s.SetConfig(context.Background(), SetConfigParams{ + ProfileName: "default", + ManagementURL: storedManagementURL, + ServerSSHAllowed: &off, + EnableSSHRoot: &off, + DisableSSHAuth: &on, + }) + require.NoError(t, err) + + require.Len(t, elev.calls, 1, "one prompt") + args := elev.calls[0] + assert.Contains(t, args, "--"+FlagDisableSSHAuth+"=true", "the setting that changes") + assert.NotContains(t, args, "--"+FlagManagementURL+"="+storedManagementURL, + "a management URL the daemon already holds") + assert.NotContains(t, args, "--"+FlagAllowServerSSH+"=false", "a setting already off") + assert.NotContains(t, args, "--"+FlagEnableSSHRoot+"=false", "a setting already off") +} + +// A request that changes no guarded setting has nothing an elevated run could +// apply, so the refusal must have come from somewhere a prompt cannot reach. +func TestSetConfigDoesNotElevateWhenNoGuardedSettingChanges(t *testing.T) { + elev := &stubElevator{available: true} + s, _ := settingsRefusingOnce(t, elev) + + off := false + _, err := s.SetConfig(context.Background(), SetConfigParams{ + ProfileName: "default", + ManagementURL: storedManagementURL, + ServerSSHAllowed: &off, + }) + + var clientErr *ClientError + require.ErrorAs(t, err, &clientErr) + assert.Equal(t, "privilege_required", clientErr.Code, "error code") + assert.Empty(t, elev.calls, "no prompt for a change nobody made") +} + +// A refusal with nothing in the request the one-shot could apply: the daemon +// cannot see who is calling, and being root would not help either. +func TestSetConfigReportsARefusalWithNothingToElevate(t *testing.T) { + elev := &stubElevator{available: true} + s, _ := settingsRefusingOnce(t, elev) + + _, err := s.SetConfig(context.Background(), SetConfigParams{ProfileName: "default"}) + + var clientErr *ClientError + require.ErrorAs(t, err, &clientErr) + assert.Equal(t, "privilege_required", clientErr.Code, "error code") + assert.Empty(t, elev.calls, "no prompt") +} diff --git a/client/ui/services/oneshot.go b/client/ui/services/oneshot.go new file mode 100644 index 000000000..d20b390cd --- /dev/null +++ b/client/ui/services/oneshot.go @@ -0,0 +1,239 @@ +//go:build !android && !ios && !freebsd && !js + +package services + +import ( + "context" + "errors" + "flag" + "fmt" + "os" + "strconv" + "time" + + gstatus "google.golang.org/grpc/status" + + "github.com/netbirdio/netbird/client/internal/elevate" + "github.com/netbirdio/netbird/client/internal/profilemanager" + "github.com/netbirdio/netbird/client/proto" + "github.com/netbirdio/netbird/util" +) + +// The other end of SetGuardedSettings: the mode this binary runs itself in, +// elevated, to apply the settings the daemon restricts to root/administrator. +// +// Both ends are here on purpose. What may be changed this way is an allowlist, and +// an allowlist declared twice is one that will eventually disagree with itself, so +// the arguments are rendered and parsed from a single table: guardedFields. Adding +// a setting is one row; nothing generic passes through, and no field outside the +// table can be reached with an elevated request no matter what lands on the command +// line. + +// oneShotTimeout bounds the whole one-shot: connect, one RPC, exit. Generous +// because the user has just waited for an authentication dialog, and a failure here +// costs them the entire round trip. +const oneShotTimeout = 30 * time.Second + +// Exit codes the parent reads where the platform gives it one. +const ( + exitOK = 0 + exitFailure = 1 + exitUsage = 2 +) + +// guardedField is one setting the one-shot understands, in the two spellings it +// needs and with the two halves of its plumbing. +type guardedField struct { + // flag names it on the one-shot's command line. + flag string + usage string + // read returns the value to send and whether the caller asked for this setting + // at all. + read func(GuardedSettings) (string, bool) + // write parses a value from the command line onto the request. It is the only + // thing that validates the value, so it fails on anything it does not + // recognise rather than guessing. + write func(*proto.SetConfigRequest, string) error + // up renders the equivalent `netbird up` flag, for the fallback command shown + // when there is no prompt to raise. + up func(value string) string +} + +var guardedFields = []guardedField{ + { + flag: FlagManagementURL, + usage: "Management server the profile registers with.", + read: func(p GuardedSettings) (string, bool) { return p.ManagementURL, p.ManagementURL != "" }, + write: func(req *proto.SetConfigRequest, value string) error { + // Parsed with the config layer's own parser, so what the elevated run + // accepts cannot drift from what the daemon would store. + if _, err := profilemanager.ParseServiceURL("Management URL", value); err != nil { + return err + } + req.ManagementUrl = value + return nil + }, + // The daemon names this one as `-m ` in its own refusals. + up: func(value string) string { return "-m " + value }, + }, + boolField(FlagAllowServerSSH, "Run the NetBird SSH server.", + func(p GuardedSettings) *bool { return p.ServerSSHAllowed }, + func(req *proto.SetConfigRequest, v *bool) { req.ServerSSHAllowed = v }), + boolField(FlagEnableSSHRoot, "Allow SSH sessions to privileged accounts.", + func(p GuardedSettings) *bool { return p.EnableSSHRoot }, + func(req *proto.SetConfigRequest, v *bool) { req.EnableSSHRoot = v }), + boolField(FlagDisableSSHAuth, "Accept SSH sessions without authentication.", + func(p GuardedSettings) *bool { return p.DisableSSHAuth }, + func(req *proto.SetConfigRequest, v *bool) { req.DisableSSHAuth = v }), +} + +// fieldValue is a flag that remembers whether it was given, and requires a value: +// the renderer always writes one, so a bare flag is a caller that got it wrong. +type fieldValue struct { + set bool + value string +} + +func (v *fieldValue) String() string { + if v == nil { + return "" + } + return v.value +} + +func (v *fieldValue) Set(value string) error { + v.set, v.value = true, value + return nil +} + +// boolField describes a setting that is on or off. The value is always spelled out, +// so that turning a setting off is as unambiguous as turning it on and a flag with +// no value is a mistake rather than an "on". +func boolField( + name, usage string, + read func(GuardedSettings) *bool, + write func(*proto.SetConfigRequest, *bool), +) guardedField { + return guardedField{ + flag: name, + usage: usage, + read: func(p GuardedSettings) (string, bool) { + value := read(p) + if value == nil { + return "", false + } + return strconv.FormatBool(*value), true + }, + write: func(req *proto.SetConfigRequest, value string) error { + parsed, err := strconv.ParseBool(value) + if err != nil { + return fmt.Errorf("parse %q as a boolean: %w", value, err) + } + write(req, &parsed) + return nil + }, + up: func(value string) string { return "--" + name + "=" + value }, + } +} + +// IsPrivilegedSettingsRun reports whether this process was started as the one-shot. +// The flag is a marker rather than a value, so only the bare forms count: reading a +// value would mean "--flag=false" started it too. +func IsPrivilegedSettingsRun(args []string) bool { + for _, arg := range args { + if arg == "--"+FlagApplyPrivilegedSettings || arg == "-"+FlagApplyPrivilegedSettings { + return true + } + } + return false +} + +// RunPrivilegedSettings applies the requested settings and returns the process exit +// code. connect dials the daemon, which is the caller's business because only it +// knows how this build talks to it. +// +// Everything it reports goes to stderr, which is what the parent captures where the +// platform lets it. On success it says so on standard output, because macOS gives +// the parent no exit status to read: see elevate.AppliedMarker. +func RunPrivilegedSettings(args []string, connect func(addr string) (proto.DaemonServiceClient, error)) int { + fs := flag.NewFlagSet("netbird-ui --"+FlagApplyPrivilegedSettings, flag.ContinueOnError) + fs.Bool(FlagApplyPrivilegedSettings, false, "Apply the settings the daemon restricts to root/administrator and exit.") + daemonAddr := fs.String(FlagDaemonAddr, "", "Daemon gRPC address: unix:///path, npipe://name or tcp://host:port") + logLevel := fs.String(FlagLogLevel, "info", "Log level: trace|debug|info|warn|error.") + profile := fs.String(FlagProfile, "", "Profile to change.") + username := fs.String(FlagUser, "", "Owner of the profile.") + + values := make([]fieldValue, len(guardedFields)) + for i, field := range guardedFields { + fs.Var(&values[i], field.flag, field.usage) + } + + if err := fs.Parse(args); err != nil { + return exitUsage + } + + if err := util.InitLog(*logLevel, "console"); err != nil { + fmt.Fprintf(os.Stderr, "init log: %v\n", err) + return exitFailure + } + + req, err := privilegedRequest(*profile, *username, values) + if err != nil { + fmt.Fprintf(os.Stderr, "%v\n", err) + return exitUsage + } + + ctx, cancel := context.WithTimeout(context.Background(), oneShotTimeout) + defer cancel() + + if err := applyPrivilegedSettings(ctx, *daemonAddr, req, connect); err != nil { + fmt.Fprintf(os.Stderr, "apply settings: %v\n", err) + return exitFailure + } + + fmt.Fprintln(os.Stdout, elevate.AppliedMarker) + return exitOK +} + +// privilegedRequest builds the request from the flags that were given, and refuses +// one that asks for nothing. +func privilegedRequest(profile, username string, values []fieldValue) (*proto.SetConfigRequest, error) { + req := &proto.SetConfigRequest{ProfileName: profile, Username: username} + + given := 0 + for i, field := range guardedFields { + if !values[i].set { + continue + } + if err := field.write(req, values[i].value); err != nil { + return nil, fmt.Errorf("--%s: %w", field.flag, err) + } + given++ + } + if given == 0 { + return nil, errors.New("no setting to apply") + } + return req, nil +} + +func applyPrivilegedSettings( + ctx context.Context, + daemonAddr string, + req *proto.SetConfigRequest, + connect func(addr string) (proto.DaemonServiceClient, error), +) error { + client, err := connect(daemonAddr) + if err != nil { + return err + } + if _, err := client.SetConfig(ctx, req); err != nil { + // Unwrapped: the daemon's message is written for a person, and a refusal + // elevation cannot fix has to say so where the parent can read it off + // stderr. + return errors.New(gstatus.Convert(err).Message()) + } + return nil +} + +// interface guard: the one-shot's flags are flag.Value. +var _ flag.Value = (*fieldValue)(nil) diff --git a/client/ui/services/oneshot_test.go b/client/ui/services/oneshot_test.go new file mode 100644 index 000000000..f8eb43066 --- /dev/null +++ b/client/ui/services/oneshot_test.go @@ -0,0 +1,151 @@ +//go:build !android && !ios && !freebsd && !js + +package services + +import ( + "flag" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + "github.com/netbirdio/netbird/client/proto" +) + +func TestIsPrivilegedSettingsRun(t *testing.T) { + tests := []struct { + name string + args []string + want bool + }{ + {name: "no arguments"}, + {name: "double dash", args: []string{"--" + FlagApplyPrivilegedSettings}, want: true}, + {name: "single dash", args: []string{"-" + FlagApplyPrivilegedSettings}, want: true}, + { + name: "among other flags", + args: []string{"--daemon-addr", "unix:///tmp/x.sock", "--" + FlagApplyPrivilegedSettings}, + want: true, + }, + // A marker, not a value: the caller never passes one, and reading a value + // would mean "--flag=false" started the one-shot too. + {name: "with a value", args: []string{"--" + FlagApplyPrivilegedSettings + "=true"}}, + {name: "unrelated flags", args: []string{"--log-level", "debug"}}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + assert.Equal(t, tt.want, IsPrivilegedSettingsRun(tt.args), "args %v", tt.args) + }) + } +} + +// What SetGuardedSettings renders has to be what the one-shot reads back, for every +// setting in the table. This is the property that keeps the two ends of an allowlist +// from drifting, so it is checked field by field rather than by example. +func TestGuardedFieldsRoundTrip(t *testing.T) { + on, off := true, false + tests := []struct { + name string + settings GuardedSettings + want func(*testing.T, *proto.SetConfigRequest) + }{ + { + name: "management url", + settings: GuardedSettings{ManagementURL: "https://mgmt.example.com:33073"}, + want: func(t *testing.T, req *proto.SetConfigRequest) { + assert.Equal(t, "https://mgmt.example.com:33073", req.GetManagementUrl()) + }, + }, + { + name: "ssh server on", + settings: GuardedSettings{ServerSSHAllowed: &on}, + want: func(t *testing.T, req *proto.SetConfigRequest) { + require.NotNil(t, req.ServerSSHAllowed) + assert.True(t, *req.ServerSSHAllowed) + }, + }, + { + name: "ssh root off", + settings: GuardedSettings{EnableSSHRoot: &off}, + want: func(t *testing.T, req *proto.SetConfigRequest) { + require.NotNil(t, req.EnableSSHRoot, "an explicit false must survive, not read as absent") + assert.False(t, *req.EnableSSHRoot) + }, + }, + { + name: "ssh auth off", + settings: GuardedSettings{DisableSSHAuth: &on}, + want: func(t *testing.T, req *proto.SetConfigRequest) { + require.NotNil(t, req.DisableSSHAuth) + assert.True(t, *req.DisableSSHAuth) + }, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + req := parseRendered(t, tt.settings) + tt.want(t, req) + }) + } +} + +// A setting nobody asked about must not arrive at the daemon at all: sending its +// zero value would change it. +func TestGuardedFieldsCarryOnlyWhatWasAsked(t *testing.T) { + on := true + req := parseRendered(t, GuardedSettings{ProfileName: "work", EnableSSHRoot: &on}) + + assert.Equal(t, "work", req.GetProfileName(), "profile") + require.NotNil(t, req.EnableSSHRoot) + assert.Nil(t, req.ServerSSHAllowed, "untouched setting") + assert.Nil(t, req.DisableSSHAuth, "untouched setting") + assert.Empty(t, req.GetManagementUrl(), "untouched setting") +} + +func TestPrivilegedRequestRejectsAnEmptyChange(t *testing.T) { + _, err := privilegedRequest("default", "vma", make([]fieldValue, len(guardedFields))) + require.Error(t, err, "nothing to apply is not a request worth sending as root") +} + +// A value the table cannot parse is refused rather than guessed at. +func TestPrivilegedRequestRejectsAnUnparseableValue(t *testing.T) { + values := make([]fieldValue, len(guardedFields)) + for i, field := range guardedFields { + if field.flag != FlagEnableSSHRoot { + continue + } + require.NoError(t, values[i].Set("perhaps")) + } + + _, err := privilegedRequest("default", "vma", values) + require.Error(t, err) + assert.Contains(t, err.Error(), FlagEnableSSHRoot, "which flag was wrong") +} + +// parseRendered puts the settings through both ends: rendered as the arguments the +// elevated process is given, then parsed by a flag set registered from the same +// table, which is what the one-shot itself parses them with. Anything hand-rolled +// here would pin down a parser nothing uses. +func parseRendered(t *testing.T, p GuardedSettings) *proto.SetConfigRequest { + t.Helper() + + rendered := guardedSettings(p) + require.NotEmpty(t, rendered, "nothing rendered for %+v", p) + + args := make([]string, 0, len(rendered)) + for _, setting := range rendered { + args = append(args, setting.arg) + } + + fs := flag.NewFlagSet(t.Name(), flag.ContinueOnError) + values := make([]fieldValue, len(guardedFields)) + for i, field := range guardedFields { + fs.Var(&values[i], field.flag, field.usage) + } + require.NoError(t, fs.Parse(args), "the one-shot's own flag set must accept %v", args) + + req, err := privilegedRequest(p.ProfileName, p.Username, values) + require.NoError(t, err) + return req +} diff --git a/client/ui/services/settings.go b/client/ui/services/settings.go index 74e6f913c..91aac0467 100644 --- a/client/ui/services/settings.go +++ b/client/ui/services/settings.go @@ -44,12 +44,19 @@ type Restrictions struct { } // Privilege tells the frontend whether this process may perform the changes the -// daemon restricts to root/administrator, and carries the command for each so a -// disabled control can show the way to do it. +// daemon restricts to root/administrator, whether it can ask the operating +// system for the privileges instead, and the command for each so a control that +// can do neither can still show the way. type Privilege struct { Privileged bool `json:"privileged"` - // Actor names what the operation requires ("root", "administrator privileges"). - Actor string `json:"actor"` + // ActorKey identifies the principal the operation requires without wording it, + // so the frontend can name it in the user's language: see + // ipcauth.PrivilegedActorKey. The words are not sent, because English ones + // cannot be dropped into a translated sentence. + ActorKey string `json:"actorKey"` + // CanElevate reports whether a guarded control can offer to authorize the + // change through the platform's own prompt: see SetGuardedSettings. + CanElevate bool `json:"canElevate"` // Commands equivalent to the settings the daemon guards, ready to copy. AllowSSHServer string `json:"allowSshServer"` EnableSSHRoot string `json:"enableSshRoot"` @@ -128,6 +135,9 @@ type Settings struct { // daemonAddr is where the daemon listens, used to tell whether it runs as // this user and would therefore authorize us: see Privilege. daemonAddr string + // elevator raises the platform's privilege prompt when a change needs more + // rights than this process has. + elevator elevator } func NewSettings(conn DaemonConn, translator ErrorTranslator, prefs LanguagePreference, daemonAddr string) *Settings { @@ -135,6 +145,7 @@ func NewSettings(conn DaemonConn, translator ErrorTranslator, prefs LanguagePref conn: conn, classifier: errorClassifier{translator: translator, prefs: prefs}, daemonAddr: daemonAddr, + elevator: osElevator{}, } } @@ -180,10 +191,10 @@ func (s *Settings) GetConfig(ctx context.Context, p ConfigParams) (Config, error }, nil } -func (s *Settings) SetConfig(ctx context.Context, p SetConfigParams) error { +func (s *Settings) SetConfig(ctx context.Context, p SetConfigParams) (SaveOutcome, error) { cli, err := s.conn.Client() if err != nil { - return err + return SaveOutcome{}, err } req := &proto.SetConfigRequest{ ProfileName: p.ProfileName, @@ -215,19 +226,92 @@ func (s *Settings) SetConfig(ctx context.Context, p SetConfigParams) error { SshJWTCacheTTL: p.SSHJWTCacheTTL, } if _, err := cli.SetConfig(ctx, req); err != nil { + if _, refused := privilegeErrorInfo(err); refused { + return s.setConfigElevated(ctx, p, req, err) + } // Classified so the frontend gets the daemon's guidance instead of the - // gRPC envelope, which is what a refused privileged change looks like. - return s.classifier.classify(err) + // gRPC envelope. + return SaveOutcome{}, s.classifier.classify(err) } - return nil + return SaveOutcome{}, nil +} + +// setConfigElevated answers a request the daemon refused for want of privileges by +// asking the user to authorize it, and sending it again if they do. It is the same +// offer the SSH settings make up front, for the changes a control cannot know are +// guarded until it is told: repointing a profile at another management server is +// only privileged while that host runs the SSH server. +// +// Two steps, because the elevated one-shot deliberately understands only the +// settings the daemon guards: it applies those, and the original request then goes +// through as this user, its privileged parts now asking for nothing that is not +// already stored. Nothing was applied by the refused attempt — the daemon decides +// before it writes — so there is no half-applied state to undo either way. +func (s *Settings) setConfigElevated(ctx context.Context, p SetConfigParams, req *proto.SetConfigRequest, refusal error) (SaveOutcome, error) { + if !s.canElevate() { + return SaveOutcome{}, s.classifier.classify(refusal) + } + + guarded, err := s.guardedChanges(ctx, p) + if err != nil { + log.Warnf("cannot tell which guarded settings this request changes: %v", err) + return SaveOutcome{}, s.classifier.classify(refusal) + } + if len(guardedSettings(guarded)) == 0 { + // Refused over something no prompt can settle, such as a control channel + // that carries no caller identity. Report the daemon's own guidance. + return SaveOutcome{}, s.classifier.classify(refusal) + } + + outcome, err := s.SetGuardedSettings(ctx, guarded) + if err != nil || outcome.Declined { + return outcome, err + } + + cli, err := s.conn.Client() + if err != nil { + return SaveOutcome{}, err + } + if _, err := cli.SetConfig(ctx, req); err != nil { + return SaveOutcome{}, s.classifier.classify(err) + } + return SaveOutcome{}, nil +} + +// guardedChanges is the guarded part of a request, reduced to what it actually +// changes. +// +// A settings form submits every field it holds, so a request restates values the +// daemon already has. Carrying those into the elevated run would spend one +// authorization on more than the user asked for, and a value that has gone stale +// since the form was loaded would spend it on something they never asked about. +func (s *Settings) guardedChanges(ctx context.Context, p SetConfigParams) (GuardedSettings, error) { + stored, err := s.GetConfig(ctx, ConfigParams{ProfileName: p.ProfileName, Username: p.Username}) + if err != nil { + return GuardedSettings{}, fmt.Errorf("read the stored config: %w", err) + } + + guarded := GuardedSettings{ + ProfileName: p.ProfileName, + Username: p.Username, + ServerSSHAllowed: changedFlag(p.ServerSSHAllowed, stored.ServerSSHAllowed), + EnableSSHRoot: changedFlag(p.EnableSSHRoot, stored.EnableSSHRoot), + DisableSSHAuth: changedFlag(p.DisableSSHAuth, stored.DisableSSHAuth), + } + // An empty URL leaves the setting alone, which is the daemon's rule too. + if p.ManagementURL != "" && p.ManagementURL != stored.ManagementURL { + guarded.ManagementURL = p.ManagementURL + } + return guarded, nil } // Privilege reports whether this UI process could carry out the changes the -// daemon restricts to root/administrator, and the command that performs the one -// users hit in the SSH settings. It applies the daemon's own rule to what it can -// see locally, so the frontend can present those controls as unavailable up front -// instead of letting a save fail. No daemon round-trip, so it also works while the -// daemon is down. +// daemon restricts to root/administrator, whether it can instead ask the +// operating system for the privileges when the user wants one of them, and the +// command that performs the ones users hit in the SSH settings. It applies the +// daemon's own rule to what it can see locally, so the frontend can decide up +// front how to present those controls instead of letting a save fail. No daemon +// round-trip, so it also works while the daemon is down. // // Being root or an elevated administrator is one way. The other is running as the // daemon's own user while the daemon is unprivileged, which the daemon accepts @@ -237,26 +321,40 @@ func (s *Settings) SetConfig(ctx context.Context, p SetConfigParams) error { func (s *Settings) Privilege() Privilege { id, err := ipcauth.CurrentProcessIdentity() if err != nil { - // Fail closed: report unprivileged, which only ever disables controls. + // Fail closed: report unprivileged, which only ever asks for more. log.Warnf("cannot read this process's identity, treating it as unprivileged: %v", err) - return newPrivilege(false) + return s.newPrivilege(false) } if id.IsPrivileged() { - return newPrivilege(true) + return s.newPrivilege(true) } - return newPrivilege(daemonaddr.DaemonRunsAsSelf(s.daemonAddr)) + return s.newPrivilege(daemonaddr.DaemonRunsAsSelf(s.daemonAddr)) } -func newPrivilege(privileged bool) Privilege { +func (s *Settings) newPrivilege(privileged bool) Privilege { return Privilege{ Privileged: privileged, - Actor: ipcauth.PrivilegedActor(), + ActorKey: ipcauth.PrivilegedActorKey(), + CanElevate: s.canElevate(), AllowSSHServer: ipcauth.UpCommand("--allow-server-ssh"), EnableSSHRoot: ipcauth.UpCommand("--enable-ssh-root"), DisableSSHAuth: ipcauth.UpCommand("--disable-ssh-auth"), } } +// canElevate reports whether offering the platform's elevation prompt would get +// the user anywhere. It needs a mechanism to raise the prompt with and a control +// channel that tells the daemon who is calling: on loopback TCP the daemon +// refuses these changes to everybody, root included, so a prompt there would +// only waste the user's password. +func (s *Settings) canElevate() bool { + if !daemonaddr.CarriesIdentity(s.daemonAddr) { + log.Debugf("not offering elevation: the daemon address %s carries no caller identity", s.daemonAddr) + return false + } + return s.elevator.Available() +} + func (s *Settings) GetRestrictions(ctx context.Context) (Restrictions, error) { cli, err := s.conn.Client() if err != nil { @@ -289,6 +387,15 @@ func (s *Settings) GetRestrictions(ctx context.Context) (Restrictions, error) { return r, nil } +// changedFlag returns requested only when it differs from what is stored, so a +// setting the request merely restates is left out of the elevated run. +func changedFlag(requested *bool, stored bool) *bool { + if requested == nil || *requested == stored { + return nil + } + return requested +} + func applyMDMRestrictions(mdm *MDMFields, cfgResp *proto.GetConfigResponse) { managed := cfgResp.GetMDMManagedFields() if len(managed) == 0 {