diff --git a/client/internal/daemonaddr/identity_test.go b/client/internal/daemonaddr/identity_test.go index 73ba39f3d..2808b5017 100644 --- a/client/internal/daemonaddr/identity_test.go +++ b/client/internal/daemonaddr/identity_test.go @@ -1,6 +1,10 @@ package daemonaddr -import "testing" +import ( + "testing" + + "github.com/stretchr/testify/assert" +) func TestCarriesIdentity(t *testing.T) { tests := []struct { @@ -19,9 +23,7 @@ func TestCarriesIdentity(t *testing.T) { for _, tt := range tests { t.Run(tt.addr, func(t *testing.T) { - if got := CarriesIdentity(tt.addr); got != tt.want { - t.Errorf("CarriesIdentity(%q) = %v, want %v", tt.addr, got, tt.want) - } + assert.Equal(t, tt.want, CarriesIdentity(tt.addr), "address %q", tt.addr) }) } } diff --git a/client/internal/elevate/output.go b/client/internal/elevate/output.go new file mode 100644 index 000000000..2ff0bc143 --- /dev/null +++ b/client/internal/elevate/output.go @@ -0,0 +1,19 @@ +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" + +// firstLine trims captured output to something that reads in one line. +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 index 858e90793..6b0e4fc0d 100644 --- a/client/internal/elevate/run_darwin.go +++ b/client/internal/elevate/run_darwin.go @@ -2,6 +2,7 @@ package elevate import ( "context" + "errors" "fmt" "os" "runtime" @@ -49,11 +50,9 @@ import ( // runs under guard, which turns a panic out of the FFI layer into that same // fallback. // -// One thing that is not optional: the elevated process must be signed with the -// hardened runtime, which is what stops DYLD_INSERT_LIBRARIES in the environment -// the trampoline passes on from loading somebody's library into a root process. The -// released app is signed and notarised, so it is; see also trustedSelf, which -// refuses to elevate an executable others can write. +// 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" @@ -281,6 +280,13 @@ func execute(ctx context.Context, authorization uintptr, self string, args []str 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)) } @@ -310,7 +316,16 @@ func readPipe(ctx context.Context, pipe uintptr) (string, error) { if n > 0 { out.Write(buf[:n]) } - if n <= 0 || err != nil { + 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 } } @@ -342,14 +357,3 @@ func cString(pinner *runtime.Pinner, s string) *byte { pinner.Pin(&b[0]) return &b[0] } - -func firstLine(s string) string { - s = strings.TrimSpace(s) - if s == "" { - return "no output" - } - if i := strings.IndexByte(s, '\n'); i >= 0 { - return s[:i] - } - return s -} diff --git a/client/internal/elevate/run_darwin_test.go b/client/internal/elevate/run_darwin_test.go index 5b8cbba2f..f6c58c8cb 100644 --- a/client/internal/elevate/run_darwin_test.go +++ b/client/internal/elevate/run_darwin_test.go @@ -3,16 +3,17 @@ package elevate import ( "errors" "runtime" - "strings" "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" ) -// The framework has to load and the symbols has to resolve, or nothing else here +// The framework has to load and the symbols have to resolve, or nothing else here // means anything. func TestSecurityFrameworkLoads(t *testing.T) { - if err := load(); err != nil { - t.Fatalf("load() = %v, want the framework to open", err) - } + require.NoError(t, load(), "Security.framework must open") + for name, fn := range map[string]any{ "AuthorizationCreate": authorizationCreate, "AuthorizationExecuteWithPrivileges": authorizationExecuteWithPrivileges, @@ -20,9 +21,7 @@ func TestSecurityFrameworkLoads(t *testing.T) { "fileno": fileno, "fclose": fclose, } { - if fn == nil { - t.Errorf("%s did not resolve", name) - } + assert.NotNil(t, fn, "%s must resolve", name) } } @@ -40,10 +39,7 @@ func TestAuthorizationCreateWithoutInteraction(t *testing.T) { rights := itemSet(&pinner, authorizationItem{name: cString(&pinner, rightExecute)}) environment := itemSet(&pinner, promptItem(&pinner)) - - if got := rights.count; got != 1 { - t.Fatalf("rights.count = %d, want 1: the struct layout is wrong", got) - } + 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) @@ -55,7 +51,7 @@ func TestAuthorizationCreateWithoutInteraction(t *testing.T) { case errAuthorizationDenied, errAuthorizationInteractionNotAllowed: // The expected answers when nobody may be asked. default: - t.Fatalf("AuthorizationCreate returned OSStatus %d, want a known one", status) + require.Failf(t, "unknown OSStatus", "AuthorizationCreate returned %d, want a status we recognise", status) } } @@ -75,22 +71,22 @@ func TestAuthorizeUnknownRightIsNotDeclined(t *testing.T) { status := authorizationCreate(rights, nil, flagDefaults|flagExtendRights, &authorization) if status == errAuthorizationSuccess { authorizationFree(authorization, flagDestroyRights) - t.Fatal("a right that does not exist was granted") } + assert.NotEqual(t, int32(errAuthorizationSuccess), status, "a right that does not exist must not be granted") } func TestMechanismAvailable(t *testing.T) { - if !mechanismAvailable() { - t.Error("mechanismAvailable() = false on macOS, where the trampoline always exists") - } + 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 TestExecuteRequiresTheAppliedMarker(t *testing.T) { - if !strings.Contains(AppliedMarker, "netbird") { - t.Errorf("AppliedMarker = %q, want something the one-shot would not print by accident", AppliedMarker) - } +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 @@ -100,42 +96,16 @@ func TestGuardTurnsAPanicIntoUnavailable(t *testing.T) { panic("purego: signature it cannot map") }) - if !errors.Is(err, ErrUnavailable) { - t.Fatalf("guard() = %v, want it to be ErrUnavailable", err) - } - if !strings.Contains(err.Error(), "pretending to call something") { - t.Errorf("guard() = %v, want it to name what panicked", err) - } + 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") - if err := guard("calling", func() error { return sentinel }); !errors.Is(err, sentinel) { - t.Errorf("guard() = %v, want the error it was given", err) - } - if err := guard("calling", func() error { return nil }); err != nil { - t.Errorf("guard() = %v, want nil", err) - } -} - -func TestFirstLine(t *testing.T) { - tests := []struct{ in, want string }{ - {in: "", want: "no output"}, - {in: " \n ", want: "no output"}, - {in: "one line", want: "one line"}, - {in: "first\nsecond", want: "first"}, - } - for _, tt := range tests { - if got := firstLine(tt.in); got != tt.want { - t.Errorf("firstLine(%q) = %q, want %q", tt.in, got, tt.want) - } - } -} - -// Declined has to stay distinguishable after wrapping, which is what the callers -// switch on. -func TestErrDeclinedSurvivesWrapping(t *testing.T) { - if !errors.Is(errors.Join(ErrDeclined), ErrDeclined) { - t.Error("ErrDeclined does not match itself through errors.Is") - } + 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 index 6ffb245cc..4a37670e2 100644 --- a/client/internal/elevate/run_unix.go +++ b/client/internal/elevate/run_unix.go @@ -24,12 +24,16 @@ const ( exitNotAuthorized = 127 ) -// noAgentMarkers appear in pkexec's own complaint when it had no way to ask: no -// agent registered for the session, and no controlling terminal for the textual -// agent it falls back to. That is the one outcome behind exitNotAuthorized worth a -// message, so it has to be told from a plain refusal, and the only thing that tells -// them apart is what pkexec says about itself. Read with LC_ALL=C so the words are -// the ones written here. +// 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 @@ -67,21 +71,34 @@ func run(ctx context.Context, self string, args []string) error { // 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 := message(full) + out := firstLine(full) switch exitErr.ExitCode() { case exitDismissed: return ErrDeclined case exitNotAuthorized: - if hasAny(full, noAgentMarkers) { - return fmt.Errorf("%w: polkit had no way to ask: %s", ErrUnavailable, out) - } - // polkit asked and was not satisfied. Overwhelmingly that is the user - // saying no, which needs no message; that an account barred from + 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("elevated netbird exited with %d: %s", exitErr.ExitCode(), out) + return fmt.Errorf("pkexec could not run elevated netbird: %s", out) } } @@ -98,15 +115,3 @@ func mechanismAvailable() bool { _, err := exec.LookPath("pkexec") return err == nil } - -// message trims a captured stderr to something that reads in one line. -func message(s string) string { - s = strings.TrimSpace(s) - if s == "" { - return "no output" - } - if i := strings.IndexByte(s, '\n'); i >= 0 { - return s[:i] - } - return s -} diff --git a/client/internal/elevate/run_unix_test.go b/client/internal/elevate/run_unix_test.go index 827096928..d9a9f900a 100644 --- a/client/internal/elevate/run_unix_test.go +++ b/client/internal/elevate/run_unix_test.go @@ -4,12 +4,14 @@ package elevate import ( "context" - "errors" "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 @@ -20,9 +22,7 @@ func fakePkexec(t *testing.T, exitCode int, stderr string) { dir := t.TempDir() script := fmt.Sprintf("#!/bin/sh\necho %s >&2\nexit %d\n", shellQuote(stderr), exitCode) - if err := os.WriteFile(filepath.Join(dir, "pkexec"), []byte(script), 0o700); err != nil { - t.Fatalf("write fake pkexec: %v", err) - } + require.NoError(t, os.WriteFile(filepath.Join(dir, "pkexec"), []byte(script), 0o700), "write the fake pkexec") t.Setenv("PATH", dir) } @@ -59,6 +59,15 @@ func TestRunMapsPkexecExitCodes(t *testing.T) { 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 { @@ -66,14 +75,15 @@ func TestRunMapsPkexecExitCodes(t *testing.T) { fakePkexec(t, tt.exitCode, tt.stderr) err := run(context.Background(), "/nonexistent/netbird-ui", []string{"--flag"}) - if tt.wantErr == nil { - if err != nil { - t.Fatalf("run() = %v, want nil", err) - } - return - } - if !errors.Is(err, tt.wantErr) { - t.Fatalf("run() = %v, want %v", err, tt.wantErr) + 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") } }) } @@ -85,21 +95,16 @@ func TestRunReportsOneShotFailure(t *testing.T) { fakePkexec(t, 3, "the one-shot said no") err := run(context.Background(), "/nonexistent/netbird-ui", nil) - if err == nil { - t.Fatal("run() = nil, want an error") - } - if errors.Is(err, ErrDeclined) || errors.Is(err, ErrUnavailable) { - t.Fatalf("run() = %v, want a plain failure", err) - } + + 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()) - if err := run(context.Background(), "/nonexistent/netbird-ui", nil); !errors.Is(err, ErrUnavailable) { - t.Fatalf("run() = %v, want ErrUnavailable", err) - } - if mechanismAvailable() { - t.Error("mechanismAvailable() = true without pkexec on PATH") - } + 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/trusted_unix.go b/client/internal/elevate/trusted_unix.go index 65904580c..d7a691164 100644 --- a/client/internal/elevate/trusted_unix.go +++ b/client/internal/elevate/trusted_unix.go @@ -3,6 +3,7 @@ package elevate import ( + "bufio" "errors" "fmt" "os" @@ -10,11 +11,16 @@ import ( "path/filepath" "slices" "strconv" + "strings" "syscall" log "github.com/sirupsen/logrus" ) +// groupFile lists which accounts are in which group, for the membership a user +// private group's name does not state: see groupHasOtherMembers. +const groupFile = "/etc/group" + // 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, @@ -73,12 +79,11 @@ func writeBitsAllow(path string, perm os.FileMode, sticky, groupAllowed bool) er // 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 is the set of accounts that can -// answer the elevation prompt anyway. And a user private group, whose name is its -// only member's, is how Debian, Ubuntu and Fedora ship: their default umask of 002 -// makes a home directory and everything built in it group-writable, so refusing -// that would mean refusing every build that is not installed from a package, for a -// group nobody else is in. +// 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 @@ -94,5 +99,48 @@ func groupWriteAllowed(uid, gid uint32) bool { log.Debugf("cannot look up uid %d, treating its group as shared: %v", uid, err) return false } - return group.Name == owner.Username + + if group.Name != owner.Username { + return false + } + return !groupHasOtherMembers(groupFile, 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 this file does not describe, +// because it comes from LDAP or another NSS source, cannot be answered here and +// leaves the name as the only thing to go on. +func groupHasOtherMembers(path, name, owner string) bool { + file, err := os.Open(path) + if err != nil { + log.Debugf("cannot read %s for the members of group %q: %v", path, name, err) + return false + } + 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 + } + for member := range strings.SplitSeq(fields[3], ",") { + if member != "" && member != owner { + return true + } + } + } + if err := scanner.Err(); err != nil { + log.Debugf("read %s: %v", path, err) + } + return false } diff --git a/client/internal/elevate/trusted_unix_test.go b/client/internal/elevate/trusted_unix_test.go index affabcf1b..e69d5711e 100644 --- a/client/internal/elevate/trusted_unix_test.go +++ b/client/internal/elevate/trusted_unix_test.go @@ -4,8 +4,13 @@ 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 @@ -14,9 +19,7 @@ import ( func ownerOnlyDir(t *testing.T) string { t.Helper() dir := t.TempDir() - if err := os.Chmod(dir, 0o755); err != nil { - t.Fatalf("chmod %s: %v", dir, err) - } + require.NoError(t, os.Chmod(dir, 0o755), "tighten the temporary directory") return dir } @@ -24,30 +27,21 @@ func ownerOnlyDir(t *testing.T) string { func writeExecutable(t *testing.T, dir string) string { t.Helper() path := filepath.Join(dir, "netbird-ui") - if err := os.WriteFile(path, []byte("#!/bin/sh\n"), 0o755); err != nil { - t.Fatalf("write executable: %v", err) - } - if err := os.Chmod(path, 0o755); err != nil { - t.Fatalf("chmod %s: %v", path, err) - } + 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) { - if err := checkOnlyOwnerWritable(writeExecutable(t, ownerOnlyDir(t))); err != nil { - t.Errorf("owner-only writable executable rejected: %v", err) - } + 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)) - if err := os.Chmod(path, 0o777); err != nil { - t.Fatalf("chmod: %v", err) - } + require.NoError(t, os.Chmod(path, 0o777), "make the executable world-writable") - if err := checkOnlyOwnerWritable(path); err == nil { - t.Error("world-writable executable accepted") - } + 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 @@ -72,16 +66,10 @@ func TestWriteBitsAllow(t *testing.T) { t.Run(tt.name, func(t *testing.T) { err := writeBitsAllow("/path", tt.perm, tt.sticky, tt.groupAllowed) if tt.wantErr { - if err == nil { - t.Errorf("writeBitsAllow(%v, sticky=%v, groupAllowed=%v) = nil, want an error", - tt.perm, tt.sticky, tt.groupAllowed) - } + assert.Error(t, err, "perm %v, sticky %v, group allowed %v", tt.perm, tt.sticky, tt.groupAllowed) return } - if err != nil { - t.Errorf("writeBitsAllow(%v, sticky=%v, groupAllowed=%v) = %v, want nil", - tt.perm, tt.sticky, tt.groupAllowed, err) - } + assert.NoError(t, err, "perm %v, sticky %v, group allowed %v", tt.perm, tt.sticky, tt.groupAllowed) }) } } @@ -89,61 +77,101 @@ func TestWriteBitsAllow(t *testing.T) { // 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) { - if !groupWriteAllowed(uint32(os.Getuid()), uint32(os.Getgid())) { - t.Skip("the test user's primary group is shared, so there is nothing to assert here") - } + requirePrivatePrimaryGroup(t) dir := ownerOnlyDir(t) path := writeExecutable(t, dir) - if err := os.Chmod(dir, 0o775); err != nil { - t.Fatalf("chmod dir: %v", err) - } - if err := os.Chmod(path, 0o775); err != nil { - t.Fatalf("chmod: %v", err) + 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 that shares its owner's name but has gained another member is no longer +// private, and its write access reaches an account that could not elevate. +func TestGroupHasOtherMembers(t *testing.T) { + tests := []struct { + name string + entry string + want bool + }{ + {name: "no members", entry: "vma:x:1000:"}, + {name: "only the owner", entry: "vma:x:1000:vma"}, + {name: "another member", entry: "vma:x:1000:bob", want: true}, + {name: "the owner and another", entry: "vma:x:1000:vma,bob", want: true}, } - if err := checkOnlyOwnerWritable(path); err != nil { - t.Errorf("executable group-writable in its owner's private group rejected: %v", err) + 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") + + assert.Equal(t, tt.want, groupHasOtherMembers(path, "vma", "vma"), "entry %q", tt.entry) + }) } } +// A group file that says nothing about the group leaves the name as the only thing +// to go on, so the private-group allowance stands rather than collapsing on every +// host whose groups come from LDAP. +func TestGroupHasOtherMembersTolerantOfAnUnknownGroup(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") + + assert.False(t, groupHasOtherMembers(path, "vma", "vma"), "a group the file does not describe") + assert.False(t, groupHasOtherMembers(filepath.Join(t.TempDir(), "absent"), "vma", "vma"), + "no group file at all") +} + // 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") - if err := os.Mkdir(dir, 0o755); err != nil { - t.Fatalf("mkdir: %v", err) - } + require.NoError(t, os.Mkdir(dir, 0o755), "create the directory") path := writeExecutable(t, dir) - if err := os.Chmod(dir, 0o777); err != nil { - t.Fatalf("chmod dir: %v", err) - } + require.NoError(t, os.Chmod(dir, 0o777), "make the directory world-writable") - if err := checkOnlyOwnerWritable(path); err == nil { - t.Error("executable in a world-writable directory accepted") - } + 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") - if err := os.Mkdir(dir, 0o755); err != nil { - t.Fatalf("mkdir: %v", err) - } + require.NoError(t, os.Mkdir(dir, 0o755), "create the directory") path := writeExecutable(t, dir) - if err := os.Chmod(dir, 0o777|os.ModeSticky); err != nil { - t.Fatalf("chmod dir: %v", err) - } + require.NoError(t, os.Chmod(dir, 0o777|os.ModeSticky), "make the directory sticky and world-writable") - if err := checkOnlyOwnerWritable(path); err != nil { - t.Errorf("executable in a sticky directory rejected: %v", err) - } + err := checkOnlyOwnerWritable(path) + assert.NoError(t, err, "the sticky bit stops another user replacing the executable") } func TestCheckOnlyOwnerWritableRejectsMissingFile(t *testing.T) { - if err := checkOnlyOwnerWritable(filepath.Join(ownerOnlyDir(t), "absent")); err == nil { - t.Error("missing executable accepted") + 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(groupFile, 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 index a02503f80..8fb05fd88 100644 --- a/client/internal/elevate/trusted_windows.go +++ b/client/internal/elevate/trusted_windows.go @@ -1,49 +1,118 @@ package elevate import ( + "errors" "fmt" "path/filepath" + "slices" "unsafe" "golang.org/x/sys/windows" ) -// writeAccess are the rights that let a trustee replace or rewrite an -// executable, or take it over and then do so. -const writeAccess = windows.FILE_WRITE_DATA | windows.FILE_APPEND_DATA | +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" -// unprivilegedTrustees are the well-known groups that contain accounts which -// cannot elevate on their own. Granting any of them write access to the -// executable would mean an account that cannot pass the UAC prompt could still -// decide what runs behind it. -var unprivilegedTrustees = []windows.WELL_KNOWN_SID_TYPE{ - windows.WinWorldSid, // Everyone - windows.WinAuthenticatedUserSid, // Authenticated Users - windows.WinInteractiveSid, // INTERACTIVE - windows.WinBuiltinUsersSid, // BUILTIN\Users - windows.WinBuiltinGuestsSid, // BUILTIN\Guests -} - -// checkOnlyOwnerWritable reports an error unless path is owned by an account -// that can elevate (or by this user) and grants write access to no group of -// accounts that cannot. Its directory is checked the same way, because being -// able to write the directory is being able to replace the file in it. +// 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 { - for _, target := range []string{path, filepath.Dir(path)} { - if err := checkSecurity(target); err != nil { + 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 } - return nil } -func checkSecurity(path string) error { +// 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 { @@ -54,8 +123,8 @@ func checkSecurity(path string) error { if err != nil { return fmt.Errorf("read owner of %s: %w", path, err) } - if err := checkOwner(path, owner); err != nil { - return 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() @@ -68,78 +137,74 @@ func checkSecurity(path string) error { return fmt.Errorf("%s has no DACL, so it grants write access to everyone", path) } - return checkDACL(path, dacl) + return checkDACL(path, dacl, writeAccess, writers) } -// checkOwner accepts an owner that can elevate by itself, plus this user: their -// own executable is theirs to write, and code already running as them could -// prompt for anything anyway. -func checkOwner(path string, owner *windows.SID) error { - self, err := currentUserSID() - if err != nil { - return err - } - if owner.Equals(self) { - return nil - } - - for _, wellKnown := range []windows.WELL_KNOWN_SID_TYPE{ - windows.WinLocalSystemSid, - windows.WinBuiltinAdministratorsSid, - } { - sid, err := windows.CreateWellKnownSid(wellKnown) - if err != nil { - return fmt.Errorf("build well-known SID %d: %w", wellKnown, err) - } - if owner.Equals(sid) { - return nil - } - } - - installer, err := windows.StringToSid(trustedInstallerSID) - if err != nil { - return fmt.Errorf("parse TrustedInstaller SID: %w", err) - } - if owner.Equals(installer) { - return nil - } - - return fmt.Errorf("%s is owned by %s, which is neither this user nor an account that can elevate", path, owner) -} - -func checkDACL(path string, dacl *windows.ACL) error { - untrusted := make([]*windows.SID, 0, len(unprivilegedTrustees)) - for _, wellKnown := range unprivilegedTrustees { - sid, err := windows.CreateWellKnownSid(wellKnown) - if err != nil { - return fmt.Errorf("build well-known SID %d: %w", wellKnown, err) - } - untrusted = append(untrusted, sid) - } - +// 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) } - if ace.Header.AceType != windows.ACCESS_ALLOWED_ACE_TYPE { + // 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 + } - //nolint:gosec // SidStart is the first uint32 of the variable-length SID that follows the ACE header. - sid := (*windows.SID)(unsafe.Pointer(&ace.SidStart)) - for _, bad := range untrusted { - if sid.Equals(bad) { - return fmt.Errorf("%s grants write access to %s", path, bad) - } + 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() 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/ui/build/linux/nfpm/nfpm.yaml b/client/ui/build/linux/nfpm/nfpm.yaml index 8f06a9886..8ce62fd18 100644 --- a/client/ui/build/linux/nfpm/nfpm.yaml +++ b/client/ui/build/linux/nfpm/nfpm.yaml @@ -21,6 +21,10 @@ contents: dst: "/usr/local/bin/netbird-ui" - src: "./build/appicon.png" dst: "/usr/share/icons/hicolor/128x128/apps/netbird-ui.png" + # The name the polkit action's icon_name refers to, which the released packages + # install as /usr/share/pixmaps/netbird.png. + - src: "./build/appicon.png" + dst: "/usr/share/icons/hicolor/128x128/apps/netbird.png" - src: "./build/linux/netbird-ui.desktop" dst: "/usr/share/applications/netbird-ui.desktop" # Names the polkit action for the elevation prompt the app raises when an diff --git a/client/ui/build/linux/polkit/io.netbird.settings.policy b/client/ui/build/linux/polkit/io.netbird.settings.policy index f3e6920b8..e12f1ddc7 100644 --- a/client/ui/build/linux/polkit/io.netbird.settings.policy +++ b/client/ui/build/linux/polkit/io.netbird.settings.policy @@ -3,26 +3,17 @@ "http://www.freedesktop.org/standards/PolicyKit/1/policyconfig.dtd"> NetBird @@ -31,7 +22,7 @@ Change privileged NetBird settings Authentication is required to change NetBird settings that grant SSH access to this computer. - netbird-ui + netbird auth_admin auth_admin @@ -44,7 +35,7 @@ Change privileged NetBird settings Authentication is required to change NetBird settings that grant SSH access to this computer. - netbird-ui + netbird auth_admin auth_admin diff --git a/client/ui/frontend/src/contexts/SettingsContext.tsx b/client/ui/frontend/src/contexts/SettingsContext.tsx index df4ee3305..a7574c7e5 100644 --- a/client/ui/frontend/src/contexts/SettingsContext.tsx +++ b/client/ui/frontend/src/contexts/SettingsContext.tsx @@ -69,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; @@ -79,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 }); @@ -100,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; @@ -155,7 +167,7 @@ const useSettingsState = () => { }); // The change needed authorization and the user said no, so the // optimistic update is wrong. Nothing to report: they know. - if (declined) { + if (declined || reloadOwed.current) { await reload(profileName); } } catch (e) { diff --git a/client/ui/services/guarded.go b/client/ui/services/guarded.go index 13a47ccbd..ddc8f7ae0 100644 --- a/client/ui/services/guarded.go +++ b/client/ui/services/guarded.go @@ -16,11 +16,9 @@ import ( ) // The command line of the one-shot mode this binary runs itself in, elevated, to -// apply a setting the daemon restricts to root/administrator. Named here, next to -// the code that builds the arguments; parsed by runPrivilegedSettings in the main -// package. The setting flags deliberately spell the same words as `netbird up`, -// so what the user is shown as a command and what runs behind the prompt read -// alike. +// 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" @@ -39,17 +37,14 @@ const ( CodeElevationFailed = "elevation_failed" ) -// elevationTimeout bounds the wait for a prompt and the change behind it, so an -// authentication dialog nobody ever answers does not leave the control it belongs -// to disabled for the rest of 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). +// 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. // -// How much it can actually interrupt differs. On Linux the prompt is a child -// process and is killed with the context; on Windows the wait for it is -// interruptible. On macOS the dialog belongs to Security.framework, which offers no -// way to withdraw the request, so there the timeout only stops us waiting — the -// system's own dialog timeout is what ends it. +// 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. @@ -141,11 +136,9 @@ func (s *Settings) SetGuardedSettings(ctx context.Context, p GuardedSettings) (S ctx, cancel := context.WithTimeout(ctx, elevationTimeout) defer cancel() - // Both ends of it: when the prompt went up, and what came of it. These are - // changes that hand out shells on this host, so the log should say who was - // asked and when, and it is also the only account of a prompt that was slow to - // appear or never answered. The daemon records the change itself, against the - // identity it authorized. + // 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 { diff --git a/client/ui/services/guarded_test.go b/client/ui/services/guarded_test.go index 237468ea3..fe2a116d1 100644 --- a/client/ui/services/guarded_test.go +++ b/client/ui/services/guarded_test.go @@ -23,6 +23,10 @@ import ( // 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 { @@ -38,12 +42,15 @@ func (e *stubElevator) Run(_ context.Context, args ...string) error { func (e *stubElevator) Available() bool { return e.available } -// stubDaemon implements only the RPC under test. The embedded interface is nil, so +// 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 - requests []*proto.SetConfigRequest + // 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) { @@ -54,6 +61,10 @@ func (d *stubDaemon) SetConfig(_ context.Context, in *proto.SetConfigRequest, _ 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 } @@ -84,12 +95,14 @@ func settingsWithElevation(t *testing.T, outcome error) (*Settings, *stubElevato } // settingsRefusingOnce returns a Settings whose daemon refuses the first SetConfig -// for want of privileges and accepts anything after it. +// 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{} + daemon := &stubDaemon{stored: &proto.GetConfigResponse{ManagementUrl: storedManagementURL}} daemon.setConfig = func(*proto.SetConfigRequest) error { if len(daemon.requests) == 1 { return refusal @@ -275,6 +288,52 @@ func TestSetConfigReportsTheRefusalWhenItCannotElevate(t *testing.T) { 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) { diff --git a/client/ui/services/oneshot.go b/client/ui/services/oneshot.go index 676b08005..d20b390cd 100644 --- a/client/ui/services/oneshot.go +++ b/client/ui/services/oneshot.go @@ -14,6 +14,7 @@ import ( 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" ) @@ -64,6 +65,11 @@ var guardedFields = []guardedField{ 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 }, diff --git a/client/ui/services/oneshot_test.go b/client/ui/services/oneshot_test.go index 2c66a94f4..f8eb43066 100644 --- a/client/ui/services/oneshot_test.go +++ b/client/ui/services/oneshot_test.go @@ -3,6 +3,7 @@ package services import ( + "flag" "testing" "github.com/stretchr/testify/assert" @@ -123,44 +124,28 @@ func TestPrivilegedRequestRejectsAnUnparseableValue(t *testing.T) { } // parseRendered puts the settings through both ends: rendered as the arguments the -// elevated process is given, then parsed as that process parses them. +// 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) - values := make([]fieldValue, len(guardedFields)) + args := make([]string, 0, len(rendered)) for _, setting := range rendered { - flag, value, found := splitFlag(setting.arg) - require.True(t, found, "rendered %q without a value", setting.arg) - - matched := false - for i, field := range guardedFields { - if field.flag != flag { - continue - } - require.NoError(t, values[i].Set(value)) - matched = true - } - require.True(t, matched, "rendered %q, which no field claims", setting.arg) + 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 } - -// splitFlag takes "--name=value" apart the way the flag package does. -func splitFlag(arg string) (name, value string, found bool) { - trimmed := arg - for len(trimmed) > 0 && trimmed[0] == '-' { - trimmed = trimmed[1:] - } - for i := 0; i < len(trimmed); i++ { - if trimmed[i] == '=' { - return trimmed[:i], trimmed[i+1:], true - } - } - return trimmed, "", false -} diff --git a/client/ui/services/settings.go b/client/ui/services/settings.go index dd4dd9b8b..91aac0467 100644 --- a/client/ui/services/settings.go +++ b/client/ui/services/settings.go @@ -252,13 +252,10 @@ func (s *Settings) setConfigElevated(ctx context.Context, p SetConfigParams, req return SaveOutcome{}, s.classifier.classify(refusal) } - guarded := GuardedSettings{ - ProfileName: p.ProfileName, - Username: p.Username, - ManagementURL: p.ManagementURL, - ServerSSHAllowed: p.ServerSSHAllowed, - EnableSSHRoot: p.EnableSSHRoot, - DisableSSHAuth: p.DisableSSHAuth, + 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 @@ -281,6 +278,33 @@ func (s *Settings) setConfigElevated(ctx context.Context, p SetConfigParams, req 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, whether it can instead ask the // operating system for the privileges when the user wants one of them, and the @@ -363,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 {