diff --git a/client/cmd/up_daemon_test.go b/client/cmd/up_daemon_test.go index f61f11000..fe8d27ed7 100644 --- a/client/cmd/up_daemon_test.go +++ b/client/cmd/up_daemon_test.go @@ -4,10 +4,12 @@ import ( "context" "os" "os/user" + "path/filepath" "testing" "time" "github.com/netbirdio/netbird/client/internal" + "github.com/netbirdio/netbird/client/internal/ipcauth" "github.com/netbirdio/netbird/client/internal/profilemanager" ) @@ -18,8 +20,11 @@ func TestUpDaemon(t *testing.T) { tempDir := t.TempDir() origDefaultProfileDir := profilemanager.DefaultConfigPathDir origActiveProfileStatePath := profilemanager.ActiveProfileStatePath + origDefaultConfigPath := profilemanager.DefaultConfigPath profilemanager.DefaultConfigPathDir = tempDir profilemanager.ActiveProfileStatePath = tempDir + "/active_profile.json" + // Without this the loader reads the real /var/lib/netbird/default.json. + profilemanager.DefaultConfigPath = filepath.Join(tempDir, "default.json") profilemanager.ConfigDirOverride = tempDir currUser, err := user.Current() @@ -28,8 +33,14 @@ func TestUpDaemon(t *testing.T) { return } + identity, err := ipcauth.CurrentProcessIdentity() + if err != nil { + t.Fatalf("failed to read this process's identity: %v", err) + return + } + sm := profilemanager.ServiceManager{} - created, err := sm.AddProfile("test1", currUser.Username, nil) + created, err := sm.AddProfile("test1", &identity) if err != nil { t.Fatalf("failed to add profile: %v", err) return @@ -47,6 +58,7 @@ func TestUpDaemon(t *testing.T) { t.Cleanup(func() { profilemanager.DefaultConfigPathDir = origDefaultProfileDir profilemanager.ActiveProfileStatePath = origActiveProfileStatePath + profilemanager.DefaultConfigPath = origDefaultConfigPath profilemanager.ConfigDirOverride = "" }) diff --git a/client/internal/profilemanager/config.go b/client/internal/profilemanager/config.go index 95d4c564f..f14b955f9 100644 --- a/client/internal/profilemanager/config.go +++ b/client/internal/profilemanager/config.go @@ -276,7 +276,7 @@ func baseConfigDir() (string, error) { return os.UserConfigDir() } -func getConfigDirForUser(username string) (string, error) { +func getConfigDirForUserLegacy(username string) (string, error) { if ConfigDirOverride != "" { return ConfigDirOverride, nil } diff --git a/client/internal/profilemanager/error.go b/client/internal/profilemanager/error.go index d83fe5c1c..4f896a5f4 100644 --- a/client/internal/profilemanager/error.go +++ b/client/internal/profilemanager/error.go @@ -6,4 +6,9 @@ var ( ErrProfileNotFound = errors.New("profile not found") ErrProfileAlreadyExists = errors.New("profile already exists") ErrNoActiveProfile = errors.New("no active profile set") + + // ErrAmbiguousActiveProfile is returned when the active profile state names + // an ID that several profiles hold and does not say whose directory the + // active one sits in. + ErrAmbiguousActiveProfile = errors.New("active profile is ambiguous") ) diff --git a/client/internal/profilemanager/migration.go b/client/internal/profilemanager/migration.go new file mode 100644 index 000000000..a53d5aff3 --- /dev/null +++ b/client/internal/profilemanager/migration.go @@ -0,0 +1,227 @@ +package profilemanager + +import ( + "fmt" + "os" + "os/user" + "path/filepath" + "strconv" + "strings" + + log "github.com/sirupsen/logrus" + + "github.com/netbirdio/netbird/client/internal/getent" + "github.com/netbirdio/netbird/client/internal/ipcauth" +) + +// MigrateLegacyProfiles prepares the per-username layout for a daemon that +// addresses profiles by ID and owner. +// +// Two things have to be settled before a directory name can stop carrying +// meaning. Every profile needs an ID no other profile holds, since a legacy ID +// is a display name two accounts can each have, and the profiles of the account +// the machine last ran as need their owner recorded, which the active profile +// state is the only lossless record of. +// +// The profiles.v1 directory is the marker and is created only once both are +// done, so a run that fails leaves no marker and is retried on the next start. +// Callers log a failure and carry on rather than refusing to start. +func (s *ServiceManager) MigrateLegacyProfiles() error { + dest := s.profilesDirPath() + if _, err := os.Stat(dest); err == nil { + return nil + } + + profiles, err := s.loadAllProfiles() + if err != nil { + return fmt.Errorf("load profiles: %w", err) + } + + active, err := s.GetActiveProfileState() + if err != nil { + return fmt.Errorf("active profile state: %w", err) + } + + unresolved, err := s.rekeyDuplicateIDs(profiles, active) + if err != nil { + return err + } + + if err := s.stampActiveUserDir(profiles, active); err != nil { + return err + } + + if unresolved != "" { + return fmt.Errorf("%w: %q is still held by more than one profile, the active profile state does not say which one is active", + ErrAmbiguousActiveProfile, unresolved) + } + + if err := os.MkdirAll(dest, 0700); err != nil { + return fmt.Errorf("create shared profile directory: %w", err) + } + + log.Infof("profile migration complete, %s now marks it done", dest) + return nil +} + +// rekeyDuplicateIDs gives every profile sharing an ID a fresh one, in place. +// The file stays in its directory, only its name changes, so nothing that holds +// a path to a sibling file is disturbed. +// +// It returns the one ID it could not settle, empty when it settled them all. +func (s *ServiceManager) rekeyDuplicateIDs(profiles []Profile, active *ActiveProfileState) (ID, error) { + groups := make(map[ID][]*Profile, len(profiles)) + for i := range profiles { + p := &profiles[i] + if p.ID == defaultProfileName { + continue + } + groups[p.ID] = append(groups[p.ID], p) + } + + var unresolved ID + activeDir := sanitizeProfileName(active.Username) + for id, group := range groups { + if len(group) < 2 { + continue + } + + // Renaming the active profile without knowing which of the namesakes it + // is would leave the active state pointing at nothing. The recorded + // username is the only thing that tells them apart, so without it the + // safer move is to leave the group alone and keep resolving it the old + // way. + if id == active.ID && activeDir == "" { + log.Warnf("leaving %d profiles named %q as they are, the active profile state does not say which one is active", len(group), id) + unresolved = id + continue + } + + for _, p := range group { + wasActive := id == active.ID && filepath.Base(filepath.Dir(p.Path)) == activeDir + + fresh, err := generateProfileID() + if err != nil { + return "", fmt.Errorf("generate profile ID: %w", err) + } + if err := rekeyProfile(p, fresh); err != nil { + return "", err + } + + if wasActive { + active.ID = fresh + if err := s.SetActiveProfileState(active); err != nil { + return "", fmt.Errorf("repoint active profile: %w", err) + } + } + } + } + + return unresolved, nil +} + +// renameFile is os.Rename, replaced in tests that need a rename to fail. +var renameFile = os.Rename + +// movedFile is a completed rename, kept so it can be undone. +type movedFile struct{ at, was string } + +// rekeyProfile renames a profile and its state file to a fresh ID. +// +// The state file move first and the profile file last, we try to restore if +// renaming the profile file. +func rekeyProfile(p *Profile, fresh ID) error { + // A legacy profile's display name is its filename, so it has to be in the + // file before the filename stops meaning anything. The loader already + // falls back to the stem, so writing p.Name is a no-op when the file + // carries a name of its own. + if err := writeProfileName(p.Path, p.Name); err != nil { + return fmt.Errorf("record display name of %s: %w", p.ID, err) + } + + dir := filepath.Dir(p.Path) + + var moved []movedFile + for _, suffix := range []string{stateFileSuffix, prefsFileSuffix} { + src := filepath.Join(dir, p.ID.String()+suffix) + if _, err := os.Stat(src); err != nil { + continue + } + dst := filepath.Join(dir, fresh.String()+suffix) + if err := renameFile(src, dst); err != nil { + undoMoves(moved) + return fmt.Errorf("rekey %s alongside profile %s: %w", filepath.Base(src), p.ID, err) + } + moved = append(moved, movedFile{at: dst, was: src}) + } + + target := filepath.Join(dir, fresh.String()+".json") + if err := renameFile(p.Path, target); err != nil { + undoMoves(moved) + return fmt.Errorf("rekey %s: %w", p.ID, err) + } + + log.Infof("profile %q in %s now has the unique ID %s", p.ID, dir, fresh) + p.ID, p.Path = fresh, target + return nil +} + +// undoMoves puts back what a half-finished rekey moved, so the next start finds +// the profile as this one did and can rekey it from scratch. +func undoMoves(moved []movedFile) { + for _, m := range moved { + if err := renameFile(m.at, m.was); err != nil { + log.Errorf("could not move %s back to %s after a failed rekey, it is now orphaned: %v", m.at, m.was, err) + } + } +} + +// stampActiveUserDir records the owner of every unowned profile in the +// directory of the account the active profile state names. +// +// That name is the one lossless input the old layout left behind. Resolving it +// forward, from name to uid, avoids reversing a sanitized directory name, which +// no amount of enumeration does reliably. +func (s *ServiceManager) stampActiveUserDir(profiles []Profile, active *ActiveProfileState) error { + if active.Username == "" { + return nil + } + + u, err := getent.LookupUser(active.Username) + if err != nil { + return fmt.Errorf("resolve %q: %w", active.Username, err) + } + + principal, ok := principalForUser(u) + if !ok { + return fmt.Errorf("account %q has no usable id %q", active.Username, u.Uid) + } + + dir := sanitizeProfileName(active.Username) + for i := range profiles { + p := &profiles[i] + if len(p.Owners) > 0 || p.LegacyUserDir != dir { + continue + } + if err := stampPrincipal(p.Path, principal); err != nil { + log.Warnf("leaving %s unowned, its owner could not be recorded: %v", p.Path, err) + continue + } + log.Infof("recorded %s as the owner of %s, the directory it sits in is that account's", principal, p.Path) + } + + return nil +} + +// principalForUser turns a resolved account into an owner principal. os/user +// reports a numeric id on Unix and a SID on Windows, which is what tells the +// two kinds apart without a build tag. +func principalForUser(u *user.User) (string, bool) { + if uid, err := strconv.ParseUint(u.Uid, 10, 32); err == nil { + return ipcauth.UIDPrincipal(uint32(uid)), true + } + if strings.HasPrefix(u.Uid, "S-") { + return ipcauth.SIDPrincipal(u.Uid), true + } + return "", false +} diff --git a/client/internal/profilemanager/migration_test.go b/client/internal/profilemanager/migration_test.go new file mode 100644 index 000000000..1a9c53385 --- /dev/null +++ b/client/internal/profilemanager/migration_test.go @@ -0,0 +1,291 @@ +package profilemanager + +import ( + "errors" + "os" + "os/user" + "path/filepath" + "runtime" + "strconv" + "strings" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// currentUserPrincipal returns the running account's name and the owner +// principal migration should record for it. Migration resolves a real account +// through the host's user database, so there is nothing to stub. +func currentUserPrincipal(t *testing.T) (string, string) { + t.Helper() + if runtime.GOOS == "windows" { + t.Skip("the windows path records a SID rather than a numeric uid") + } + + u, err := user.Current() + require.NoError(t, err) + uid, err := strconv.ParseUint(u.Uid, 10, 32) + require.NoError(t, err) + + return u.Username, "uid:" + strconv.FormatUint(uid, 10) +} + +func TestMigrate_StampsTheActiveAccountsDirectory(t *testing.T) { + username, principal := currentUserPrincipal(t) + + withLegacyLayout(t, func(sm *ServiceManager, configDir string) { + dir := sanitizeProfileName(username) + mine := writeLegacyProfile(t, configDir, dir, "work", nil) + theirs := writeLegacyProfile(t, configDir, "someone-else", "theirs", nil) + require.NoError(t, sm.SetActiveProfileState(&ActiveProfileState{ID: "work", Username: username})) + + require.NoError(t, sm.MigrateLegacyProfiles()) + + assert.Equal(t, []string{principal}, readOwners(t, mine)) + assert.Empty(t, readOwners(t, theirs), + "only the account the active state names has a directory we can attribute") + assert.DirExists(t, filepath.Join(configDir, DefaultProfilePathDir), + "the marker is written once both halves are done") + }) +} + +func TestMigrate_IsIdempotentAndSkipsOnceMarked(t *testing.T) { + username, principal := currentUserPrincipal(t) + + withLegacyLayout(t, func(sm *ServiceManager, configDir string) { + path := writeLegacyProfile(t, configDir, sanitizeProfileName(username), "work", nil) + require.NoError(t, sm.SetActiveProfileState(&ActiveProfileState{ID: "work", Username: username})) + + require.NoError(t, sm.MigrateLegacyProfiles()) + require.NoError(t, sm.MigrateLegacyProfiles()) + assert.Equal(t, []string{principal}, readOwners(t, path)) + + // A profile appearing after the marker is left to the per-caller claim. + late := writeLegacyProfile(t, configDir, sanitizeProfileName(username), "late", nil) + require.NoError(t, sm.MigrateLegacyProfiles()) + assert.Empty(t, readOwners(t, late)) + }) +} + +func TestMigrate_RekeysDuplicateIDsAndKeepsTheActiveOne(t *testing.T) { + username, principal := currentUserPrincipal(t) + + withLegacyLayout(t, func(sm *ServiceManager, configDir string) { + dir := sanitizeProfileName(username) + writeLegacyProfile(t, configDir, dir, "work", nil) + writeLegacyProfile(t, configDir, "someone-else", "work", nil) + require.NoError(t, os.WriteFile(filepath.Join(configDir, dir, "work"+stateFileSuffix), []byte("{}"), 0600)) + require.NoError(t, sm.SetActiveProfileState(&ActiveProfileState{ID: "work", Username: username})) + + require.NoError(t, sm.MigrateLegacyProfiles()) + + profiles, err := sm.loadAllProfiles() + require.NoError(t, err) + + ids := map[ID]int{} + for _, p := range profiles { + ids[p.ID]++ + if p.ID != defaultProfileName { + assert.Equal(t, "work", p.Name, "the display name outlives the filename it came from") + } + } + for id, n := range ids { + assert.Equal(t, 1, n, "%s is still shared after migration", id) + } + + state, err := sm.GetActiveProfileState() + require.NoError(t, err) + assert.NotEqual(t, ID("work"), state.ID, "the active profile follows its new ID") + + active, err := sm.ActiveProfilePath(state) + require.NoError(t, err) + assert.Equal(t, dir, filepath.Base(filepath.Dir(active)), + "and still resolves inside the account that was running it") + assert.Equal(t, []string{principal}, readOwners(t, active)) + assert.FileExists(t, filepath.Join(configDir, dir, state.ID.String()+stateFileSuffix), + "the state file follows the profile it belongs to") + }) +} + +func TestMigrate_UnresolvableAccountLeavesNoMarker(t *testing.T) { + withLegacyLayout(t, func(sm *ServiceManager, configDir string) { + path := writeLegacyProfile(t, configDir, "ghost", "work", nil) + require.NoError(t, sm.SetActiveProfileState(&ActiveProfileState{ + ID: "work", Username: "no-such-account-here", + })) + + require.Error(t, sm.MigrateLegacyProfiles()) + assert.Empty(t, readOwners(t, path)) + assert.NoDirExists(t, filepath.Join(configDir, DefaultProfilePathDir), + "an unfinished run leaves no marker, so the next start tries again") + }) +} + +// failRenamesOf makes every rename of a file with the given suffix fail, and +// returns the function that lets them through again. +func failRenamesOf(t *testing.T, suffix string) func() { + t.Helper() + + orig := renameFile + failing := true + renameFile = func(from, to string) error { + if failing && strings.HasSuffix(from, suffix) { + return errors.New("simulated rename failure") + } + return orig(from, to) + } + t.Cleanup(func() { renameFile = orig }) + + return func() { failing = false } +} + +func TestMigrate_AStateFileThatCannotFollowRollsBackTheRekey(t *testing.T) { + username, _ := currentUserPrincipal(t) + + withLegacyLayout(t, func(sm *ServiceManager, configDir string) { + dir := sanitizeProfileName(username) + mine := writeLegacyProfile(t, configDir, dir, "work", nil) + writeLegacyProfile(t, configDir, "someone-else", "work", nil) + state := filepath.Join(configDir, dir, "work"+stateFileSuffix) + require.NoError(t, os.WriteFile(state, []byte("{}"), 0600)) + require.NoError(t, os.WriteFile(filepath.Join(configDir, dir, "work"+prefsFileSuffix), []byte("{}"), 0600)) + require.NoError(t, sm.SetActiveProfileState(&ActiveProfileState{ID: "work", Username: username})) + + allowRenames := failRenamesOf(t, prefsFileSuffix) + + require.Error(t, sm.MigrateLegacyProfiles()) + assert.NoDirExists(t, filepath.Join(configDir, DefaultProfilePathDir), + "a rekey that could not finish leaves no marker, so the next start tries again") + assert.FileExists(t, mine, "the profile is back under the ID its state file still carry") + assert.FileExists(t, state, "and so is the state file that had already moved") + + // The next start finds the profile exactly as the failed one did, and + // the state file are still beside it once the rekey goes through. + allowRenames() + require.NoError(t, sm.MigrateLegacyProfiles()) + + profiles, err := sm.loadAllProfiles() + require.NoError(t, err) + + ids := map[ID]int{} + var rolledBack *Profile + for i := range profiles { + p := &profiles[i] + ids[p.ID]++ + if p.ID != defaultProfileName && filepath.Dir(p.Path) == filepath.Join(configDir, dir) { + rolledBack = p + } + } + for id, n := range ids { + assert.Equal(t, 1, n, "%s is still shared after migration", id) + } + + require.NotNil(t, rolledBack) + for _, suffix := range []string{stateFileSuffix, prefsFileSuffix} { + assert.FileExists(t, filepath.Join(configDir, dir, rolledBack.ID.String()+suffix), + "%s follows the profile it belongs to", suffix) + } + }) +} + +func TestMigrate_NamesakesTheStateCannotTellApartLeaveNoMarker(t *testing.T) { + username, _ := currentUserPrincipal(t) + + withLegacyLayout(t, func(sm *ServiceManager, configDir string) { + dir := sanitizeProfileName(username) + mine := writeLegacyProfile(t, configDir, dir, "work", nil) + theirs := writeLegacyProfile(t, configDir, "someone-else", "work", nil) + require.NoError(t, sm.SetActiveProfileState(&ActiveProfileState{ID: "work"})) + + require.ErrorIs(t, sm.MigrateLegacyProfiles(), ErrAmbiguousActiveProfile) + assert.NoDirExists(t, filepath.Join(configDir, DefaultProfilePathDir), + "the marker would retire the only pass that can still separate them") + assert.FileExists(t, mine, "so both namesakes are left as they are") + assert.FileExists(t, theirs) + + profiles, err := sm.loadAllProfiles() + require.NoError(t, err) + assert.Equal(t, 2, countID(profiles, "work"), + "the group keeps the ID, since rekeying it would leave the state pointing at nothing") + + // And until they are separated the active profile does not resolve to + // whichever of them happened to sort first. + state, err := sm.GetActiveProfileState() + require.NoError(t, err) + _, err = sm.ActiveProfilePath(state) + require.ErrorIs(t, err, ErrAmbiguousActiveProfile) + + // Selecting a profile records the directory that tells them apart, and + // the start after that finishes the job. + require.NoError(t, sm.SetActiveProfileState(&ActiveProfileState{ID: "work", Username: username})) + require.NoError(t, sm.MigrateLegacyProfiles()) + assert.DirExists(t, filepath.Join(configDir, DefaultProfilePathDir)) + + state, err = sm.GetActiveProfileState() + require.NoError(t, err) + active, err := sm.ActiveProfilePath(state) + require.NoError(t, err) + assert.NotEqual(t, ID("work"), state.ID, "the namesakes are separated") + assert.Equal(t, dir, filepath.Base(filepath.Dir(active)), + "and the active one still sits in the account that was running it") + }) +} + +// countID reports how many of the loaded profiles hold id. +func countID(profiles []Profile, id ID) int { + n := 0 + for _, p := range profiles { + if p.ID == id { + n++ + } + } + return n +} + +func TestMigrate_LeavesAProfileThatAlreadyNamesAnOwnerAlone(t *testing.T) { + username, principal := currentUserPrincipal(t) + + // A SID is never what a Unix host stamps, so it says "someone else holds + // this" without depending on the uid the test happens to run as. + const otherOwner = "sid:S-1-5-21-0-0-0-1001" + + withLegacyLayout(t, func(sm *ServiceManager, configDir string) { + dir := sanitizeProfileName(username) + claimed := writeLegacyProfile(t, configDir, dir, "claimed", map[string]any{ + "Owners": []string{otherOwner}, + }) + unclaimed := writeLegacyProfile(t, configDir, dir, "unclaimed", nil) + require.NoError(t, sm.SetActiveProfileState(&ActiveProfileState{ID: "unclaimed", Username: username})) + + require.NoError(t, sm.MigrateLegacyProfiles()) + + assert.Equal(t, []string{otherOwner}, readOwners(t, claimed), + "the directory a profile sits in does not re-attribute one that already names an owner") + assert.Equal(t, []string{principal}, readOwners(t, unclaimed), + "only the profiles with no owner of their own are attributed") + }) +} + +func TestMigrate_SkipsAProfileItCannotStamp(t *testing.T) { + username, principal := currentUserPrincipal(t) + + withLegacyLayout(t, func(sm *ServiceManager, configDir string) { + dir := sanitizeProfileName(username) + good := writeLegacyProfile(t, configDir, dir, "work", nil) + broken := filepath.Join(configDir, dir, "broken.json") + require.NoError(t, os.WriteFile(broken, []byte("null"), 0600)) + require.NoError(t, sm.SetActiveProfileState(&ActiveProfileState{ID: "work", Username: username})) + + require.NoError(t, sm.MigrateLegacyProfiles()) + + assert.Equal(t, []string{principal}, readOwners(t, good), + "one profile that cannot be stamped does not hold up the rest") + assert.DirExists(t, filepath.Join(configDir, DefaultProfilePathDir), + "and the marker still lands, since the claim path retries the one left behind") + + data, err := os.ReadFile(broken) + require.NoError(t, err) + assert.Equal(t, "null", string(data), "the profile it could not stamp is untouched") + }) +} diff --git a/client/internal/profilemanager/prefs.go b/client/internal/profilemanager/prefs.go index 5613b0be3..2a6c59693 100644 --- a/client/internal/profilemanager/prefs.go +++ b/client/internal/profilemanager/prefs.go @@ -11,7 +11,10 @@ import ( "github.com/netbirdio/netbird/util" ) -const prefsFileSuffix = ".prefs.json" +const ( + prefsFileSuffix = ".prefs.json" + stateFileSuffix = ".state.json" +) var prefsMu sync.Mutex @@ -29,7 +32,7 @@ func (s *ServiceManager) ProfilePrefs(id ID, username string) (*Prefs, error) { if id == defaultProfileName { return &Prefs{path: filepath.Join(filepath.Dir(DefaultConfigPath), id.String()+prefsFileSuffix)}, nil } - configDir, err := s.getConfigDir(username) + configDir, err := s.getConfigDirLegacy(username) if err != nil { return nil, fmt.Errorf("get config directory for user %s: %w", username, err) } diff --git a/client/internal/profilemanager/prefs_test.go b/client/internal/profilemanager/prefs_test.go index 8cca6e513..6d1b6022c 100644 --- a/client/internal/profilemanager/prefs_test.go +++ b/client/internal/profilemanager/prefs_test.go @@ -3,11 +3,14 @@ package profilemanager import ( "errors" "os" + "os/user" "path/filepath" "testing" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" + + "github.com/netbirdio/netbird/client/internal/ipcauth" ) type testPrefsSection struct { @@ -15,9 +18,20 @@ type testPrefsSection struct { Dest string `json:"dest"` } +// currentUsername returns the account the prefs store keys its legacy config +// directory by. Profile ownership itself is carried by an ipcauth.Identity, but +// the on-disk layout is still per-username. +func currentUsername(t *testing.T) string { + t.Helper() + u, err := user.Current() + require.NoError(t, err) + return u.Username +} + func TestProfilePrefs_RoundTrip(t *testing.T) { - withTestSM(t, func(sm *ServiceManager, username string) { - created, err := sm.AddProfile("work", username, nil) + withTestSM(t, func(sm *ServiceManager, userID ipcauth.Identity) { + username := currentUsername(t) + created, err := sm.AddProfile("work", &userID) require.NoError(t, err) prefs, err := sm.ProfilePrefs(created.ID, username) @@ -41,8 +55,9 @@ func TestProfilePrefs_RoundTrip(t *testing.T) { } func TestProfilePrefs_GetMissingNamespace(t *testing.T) { - withTestSM(t, func(sm *ServiceManager, username string) { - created, err := sm.AddProfile("work", username, nil) + withTestSM(t, func(sm *ServiceManager, userID ipcauth.Identity) { + username := currentUsername(t) + created, err := sm.AddProfile("work", &userID) require.NoError(t, err) prefs, err := sm.ProfilePrefs(created.ID, username) @@ -56,8 +71,9 @@ func TestProfilePrefs_GetMissingNamespace(t *testing.T) { } func TestProfilePrefs_RemoveNamespace(t *testing.T) { - withTestSM(t, func(sm *ServiceManager, username string) { - created, err := sm.AddProfile("work", username, nil) + withTestSM(t, func(sm *ServiceManager, userID ipcauth.Identity) { + username := currentUsername(t) + created, err := sm.AddProfile("work", &userID) require.NoError(t, err) prefs, err := sm.ProfilePrefs(created.ID, username) @@ -82,15 +98,17 @@ func TestProfilePrefs_RemoveNamespace(t *testing.T) { } func TestProfilePrefs_RejectsInvalidID(t *testing.T) { - withTestSM(t, func(sm *ServiceManager, username string) { + withTestSM(t, func(sm *ServiceManager, userID ipcauth.Identity) { + username := currentUsername(t) _, err := sm.ProfilePrefs("../escape", username) assert.Error(t, err) }) } func TestProfilePrefs_RejectsEmptyNamespace(t *testing.T) { - withTestSM(t, func(sm *ServiceManager, username string) { - created, err := sm.AddProfile("work", username, nil) + withTestSM(t, func(sm *ServiceManager, userID ipcauth.Identity) { + username := currentUsername(t) + created, err := sm.AddProfile("work", &userID) require.NoError(t, err) prefs, err := sm.ProfilePrefs(created.ID, username) @@ -104,7 +122,8 @@ func TestProfilePrefs_RejectsEmptyNamespace(t *testing.T) { } func TestProfilePrefs_DefaultProfile(t *testing.T) { - withTestSM(t, func(sm *ServiceManager, username string) { + withTestSM(t, func(sm *ServiceManager, userID ipcauth.Identity) { + username := currentUsername(t) prefs, err := sm.ProfilePrefs(defaultProfileName, username) require.NoError(t, err) @@ -117,21 +136,22 @@ func TestProfilePrefs_DefaultProfile(t *testing.T) { } func TestRemoveProfile_DeletesPrefsFile(t *testing.T) { - withTestSM(t, func(sm *ServiceManager, username string) { - created, err := sm.AddProfile("work", username, nil) + withTestSM(t, func(sm *ServiceManager, userID ipcauth.Identity) { + username := currentUsername(t) + created, err := sm.AddProfile("work", &userID) require.NoError(t, err) prefs, err := sm.ProfilePrefs(created.ID, username) require.NoError(t, err) require.NoError(t, prefs.Put("filedrop", testPrefsSection{Mode: 2})) - configDir, err := sm.getConfigDir(username) + configDir, err := sm.getConfigDirLegacy(username) require.NoError(t, err) prefsPath := filepath.Join(configDir, created.ID.String()+prefsFileSuffix) _, err = os.Stat(prefsPath) require.NoError(t, err) - require.NoError(t, sm.RemoveProfile(created.ID, username)) + require.NoError(t, sm.RemoveProfile(created.ID, userID)) _, err = os.Stat(prefsPath) assert.True(t, errors.Is(err, os.ErrNotExist), "prefs file should be removed") }) diff --git a/client/internal/profilemanager/profilemanager.go b/client/internal/profilemanager/profilemanager.go index 44f578ae0..0ee037a3c 100644 --- a/client/internal/profilemanager/profilemanager.go +++ b/client/internal/profilemanager/profilemanager.go @@ -32,6 +32,28 @@ type Profile struct { Path string IsActive bool Owners []ipcauth.Principal + // LegacyUserDir is the sanitized username of the per-username directory the + // profile was found in, empty for the profiles.v1 directory and for the + // default profile. It is the only legacy evidence of profile ownership. + LegacyUserDir string +} + +// AccessibleBy reports whether a kernel-attested caller may address this +// profile. +func (p *Profile) AccessibleBy(id ipcauth.Identity) bool { + if !id.Known() { + return false + } + if ipcauth.IsPrivilegedCaller(id) { + return true + } + if len(p.Owners) == 0 { + // A profile in a per-username directory belonged to a use, unowned fails + // closed. The account named by the directory reclaims it on their next + // lookup or until claimed by a privileged caller. + return p.LegacyUserDir == "" && p.ID == DefaultProfileName + } + return p.Owners[0].Matches(id) } func (p *Profile) FilePath() (string, error) { @@ -60,7 +82,7 @@ func (p *Profile) FilePath() (string, error) { return "", fmt.Errorf("failed to get current user: %w", err) } - configDir, err := getConfigDirForUser(username.Username) + configDir, err := getConfigDirForUserLegacy(username.Username) if err != nil { return "", fmt.Errorf("failed to get config directory for user %s: %w", username.Username, err) } diff --git a/client/internal/profilemanager/profilemanager_test.go b/client/internal/profilemanager/profilemanager_test.go index 882a71d0a..811a00a6f 100644 --- a/client/internal/profilemanager/profilemanager_test.go +++ b/client/internal/profilemanager/profilemanager_test.go @@ -27,7 +27,10 @@ func withPatchedGlobals(t *testing.T, configDir string, testFunc func()) { DefaultConfigPath = filepath.Join(configDir, "default.json") ActiveProfileStatePath = filepath.Join(configDir, "active_profile.json") oldDefaultConfigPath = filepath.Join(configDir, "old_config.json") - ConfigDirOverride = configDir + // A subdirectory, mirroring production: loadAllProfiles only descends into + // directories under DefaultConfigPathDir, so profiles written straight into + // the config root would be invisible to it. + ConfigDirOverride = filepath.Join(configDir, DefaultProfilePathDir) // Clean up any files in the config dir to ensure isolation os.RemoveAll(configDir) os.MkdirAll(configDir, 0755) //nolint: errcheck diff --git a/client/internal/profilemanager/service.go b/client/internal/profilemanager/service.go index 68d1c23a1..dc365c325 100644 --- a/client/internal/profilemanager/service.go +++ b/client/internal/profilemanager/service.go @@ -10,11 +10,14 @@ import ( "path/filepath" "runtime" "sort" + "strconv" "strings" + "sync" "syscall" log "github.com/sirupsen/logrus" + "github.com/netbirdio/netbird/client/internal/getent" "github.com/netbirdio/netbird/client/internal/ipcauth" "github.com/netbirdio/netbird/util" ) @@ -27,6 +30,8 @@ var ( DefaultConfigPath = "" ActiveProfileStatePath = "" + DefaultProfilePathDir = "profiles.v1" + ErrorOldDefaultConfigNotFound = errors.New("old default config not found") ) @@ -54,11 +59,16 @@ type profileMeta struct { Name string } -// nolint:unused type ownerMeta struct { Owners []string } +// Config JSON keys on disk. +const ( + ownersFieldName = "Owners" + nameFieldName = "Name" +) + func (e *ErrAmbiguousHandle) Error() string { switch e.Kind { case AmbiguityKindIDPrefix: @@ -98,10 +108,21 @@ type ActiveProfileState struct { // before the ID-based config files. Legacy values were profile names, which // were also the legacy filename stems, so they still resolve to the correct // file on disk. - ID ID `json:"name"` + ID ID `json:"name"` + + // Username records which per-username directory a pre-migration profile's + // file lives in. It is a hint for reconstructing that path, not a statement + // about who owns the profile: ownership lives in the profile's own JSON, as + // typed principals. Profiles in the shared directory leave it empty, and + // the field goes away once no per-username directory is left. Username string `json:"username"` } +// FilePath rebuilds the profile's path from the per-username layout. +// +// Prefer ServiceManager.ActiveProfilePath: this reconstruction only holds for a +// profile that predates the ID-keyed layout, since a profile created after it +// lives in the shared directory instead, under no username at all. func (a *ActiveProfileState) FilePath() (string, error) { if a.ID == "" { return "", fmt.Errorf("active profile ID is empty") @@ -115,7 +136,7 @@ func (a *ActiveProfileState) FilePath() (string, error) { return "", fmt.Errorf("invalid profile ID: %q", a.ID) } - configDir, err := getConfigDirForUser(a.Username) + configDir, err := getConfigDirForUserLegacy(a.Username) if err != nil { return "", fmt.Errorf("failed to get config directory for user %s: %w", a.Username, err) } @@ -127,6 +148,74 @@ type ServiceManager struct { profilesDir string // If set, overrides ConfigDirOverride for profile operations } +// ActiveProfilePath returns the config file of the profile the active-profile +// state points at. +// +// The path is looked up through the loader rather than rebuilt from the +// recorded username, because a profile's directory is no longer a function of +// who owns it: profiles created since the ID-keyed layout share one directory, +// and only pre-migration ones sit under a per-username one. The username +// survives as a tiebreaker for the single case that still needs one, a legacy +// ID being a display name that two users can each hold. +// +// A state that points at a profile with no file yet still yields the path that +// file would have, so a caller reads "not created yet" from a stat rather than +// from an error. +func (s *ServiceManager) ActiveProfilePath(a *ActiveProfileState) (string, error) { + if a == nil || a.ID == "" { + return "", fmt.Errorf("active profile ID is empty") + } + if a.ID == defaultProfileName { + return DefaultConfigPath, nil + } + if !IsValidProfileFilenameStem(a.ID) { + return "", fmt.Errorf("invalid profile ID: %q", a.ID) + } + + profiles, err := s.loadAllProfiles() + if err != nil { + return "", fmt.Errorf("load profiles: %w", err) + } + + var matches []Profile + for _, p := range profiles { + if p.ID == a.ID { + matches = append(matches, p) + } + } + + if len(matches) == 0 { + // Nothing on disk under that ID, so the legacy layout is the only + // guess left for where the file would go. + return a.FilePath() + } + + if len(matches) == 1 { + return matches[0].Path, nil + } + + // Migration gives every profile an ID no other profile holds, so getting + // here means it has not run yet or did not finish. Until it does, the + // recorded account name is the only thing telling namesakes apart, and + // picking the wrong one would point the daemon at another user's config. + // State written before the field held a directory recorded the raw account + // name, which the old layout sanitized on its way to becoming one. + for _, want := range []string{a.Username, sanitizeProfileName(a.Username)} { + if want == "" { + continue + } + for _, p := range matches { + if filepath.Base(filepath.Dir(p.Path)) == want { + return p.Path, nil + } + } + } + + // Nothing left to tell them apart, so this fails rather than guesses. + return "", fmt.Errorf("%w: %d profiles hold the ID %q and the active profile state does not say which account's directory it is in", + ErrAmbiguousActiveProfile, len(matches), a.ID) +} + func NewServiceManager(defaultConfigPath string) *ServiceManager { if defaultConfigPath != "" { DefaultConfigPath = defaultConfigPath @@ -268,10 +357,6 @@ func (s *ServiceManager) SetActiveProfileState(a *ActiveProfileState) error { return errors.New("invalid active profile state") } - if a.ID != defaultProfileName && a.Username == "" { - return fmt.Errorf("username must be set for non-default profiles, got: %s", a.ID) - } - if a.ID != defaultProfileName && !IsValidProfileFilenameStem(a.ID) { return fmt.Errorf("invalid profile ID: %q", a.ID) } @@ -302,8 +387,8 @@ func (s *ServiceManager) DefaultProfilePath() string { // The returned Profile carries the freshly-generated ID so callers can // show it to the user (and so the gRPC AddProfileResponse can include // it). -func (s *ServiceManager) AddProfile(displayName string, username string, callerId *ipcauth.Identity) (*Profile, error) { - configDir, err := s.getConfigDir(username) +func (s *ServiceManager) AddProfile(displayName string, callerId *ipcauth.Identity) (*Profile, error) { + configDir, err := s.getConfigDir() if err != nil { return nil, fmt.Errorf("failed to get config directory: %w", err) } @@ -325,7 +410,7 @@ func (s *ServiceManager) AddProfile(displayName string, username string, callerI } cfg.Name = displayName - if err := util.WriteJson(context.Background(), profPath, cfg); err != nil { + if err := util.WriteJsonWithRestrictedPermission(context.Background(), profPath, cfg); err != nil { return nil, fmt.Errorf("failed to write profile config: %w", err) } @@ -336,7 +421,7 @@ func (s *ServiceManager) AddProfile(displayName string, username string, callerI }, nil } -func (s *ServiceManager) RenameProfile(id ID, username string, newName string) error { +func (s *ServiceManager) RenameProfile(id ID, userID ipcauth.Identity, newName string) error { displayName, err := sanitizeDisplayName(newName) if err != nil { return fmt.Errorf("invalid profile name: %w", err) @@ -346,7 +431,7 @@ func (s *ServiceManager) RenameProfile(id ID, username string, newName string) e return fmt.Errorf("invalid profile ID: %q", id) } - profiles, err := s.loadAllProfiles(username) + profiles, err := s.loadAllProfilesForIdentity(userID) if err != nil { return fmt.Errorf("load profiles: %w", err) } @@ -362,26 +447,13 @@ func (s *ServiceManager) RenameProfile(id ID, username string, newName string) e return ErrProfileNotFound } - data, err := os.ReadFile(target.Path) - if err != nil { - return err - } - var cfg Config - if err := json.Unmarshal(data, &cfg); err != nil { - return err - } - cfg.Name = displayName - - if err := util.WriteJson(context.Background(), target.Path, cfg); err != nil { - return fmt.Errorf("failed to write profile name: %w", err) - } - return nil + return writeProfileName(target.Path, displayName) } // RemoveProfile deletes the profile identified by id. Callers must have // already resolved any user-supplied handle to a concrete ID via // ResolveProfile. -func (s *ServiceManager) RemoveProfile(id ID, username string) error { +func (s *ServiceManager) RemoveProfile(id ID, userID ipcauth.Identity) error { if id == defaultProfileName { defaultName := readProfileName(DefaultConfigPath) if defaultName == "" { @@ -393,7 +465,7 @@ func (s *ServiceManager) RemoveProfile(id ID, username string) error { return fmt.Errorf("invalid profile ID: %q", id) } - profiles, err := s.loadAllProfiles(username) + profiles, err := s.loadAllProfilesForIdentity(userID) if err != nil { return fmt.Errorf("load profiles: %w", err) } @@ -436,8 +508,8 @@ func (s *ServiceManager) RemoveProfile(id ID, username string) error { // ListProfiles returns every profile for the given user, including the // default profile, with IsActive flags set. -func (s *ServiceManager) ListProfiles(username string) ([]Profile, error) { - return s.loadAllProfiles(username) +func (s *ServiceManager) ListProfiles(userID ipcauth.Identity) ([]Profile, error) { + return s.loadAllProfilesForIdentity(userID) } // GetStatePath returns the path to the state file based on the operating system @@ -468,22 +540,47 @@ func (s *ServiceManager) GetStatePath() string { return defaultStatePath } - configDir, err := s.getConfigDir(activeProf.Username) + configPath, err := s.ActiveProfilePath(activeProf) if err != nil { - log.Warnf("failed to get config directory for user %s: %v", activeProf.Username, err) + log.Warnf("failed to resolve the active profile's path: %v", err) return defaultStatePath } - return filepath.Join(configDir, activeProf.ID.String()+".state.json") + return filepath.Join(filepath.Dir(configPath), activeProf.ID.String()+".state.json") } -// getConfigDir returns the profiles directory, using profilesDir if set, otherwise getConfigDirForUser -func (s *ServiceManager) getConfigDir(username string) (string, error) { +// getConfigDirLegacy returns the profiles directory, using profilesDir if set, otherwise getConfigDirForUser +func (s *ServiceManager) getConfigDirLegacy(username string) (string, error) { if s.profilesDir != "" { return s.profilesDir, nil } - return getConfigDirForUser(username) + return getConfigDirForUserLegacy(username) +} + +func (s *ServiceManager) getConfigDir() (string, error) { + configDir := s.profilesDirPath() + if _, err := os.Stat(configDir); os.IsNotExist(err) { + if err := os.MkdirAll(configDir, 0700); err != nil { + return "", err + } + } + + return configDir, nil +} + +// profilesDirPath returns the directory new profiles are written to without +// creating it, so a read path can name it without leaving a directory behind. +func (s *ServiceManager) profilesDirPath() string { + if s.profilesDir != "" { + return s.profilesDir + } + + if ConfigDirOverride != "" { + return ConfigDirOverride + } + + return filepath.Join(DefaultConfigPathDir, DefaultProfilePathDir) } // loadAllProfiles returns every profile visible to the daemon for the @@ -493,31 +590,219 @@ func (s *ServiceManager) getConfigDir(username string) (string, error) { // Each Profile is fully populated: ID is the filename stem, Name comes // from the JSON's "name" field (falling back to the filename stem when absent) // and Path is built from a basename read off disk. -func (s *ServiceManager) loadAllProfiles(username string) ([]Profile, error) { - activeID, activeIsDefault := s.activeProfileID() +func (s *ServiceManager) loadAllProfilesForIdentity(userID ipcauth.Identity) ([]Profile, error) { + allProfiles, err := s.loadAllProfiles() + if err != nil { + return nil, err + } + + s.claimLegacyProfiles(allProfiles, userID) + + accessible := make([]Profile, 0, len(allProfiles)) + for _, p := range allProfiles { + if p.AccessibleBy(userID) { + accessible = append(accessible, p) + } + } + + return accessible, nil +} + +var ( + legacyDirMu sync.Mutex + legacyDirCache = map[string]string{} +) + +// claimLegacyProfiles stamps the caller on every unowned profile in the +// directory their own user name produced before the ownership model. +// +// Ownership lives in the file now, so the directory name is only a leftover. +// Flattening is a separate step we are doing in the future. Moving it would +// pull the state file out from under an engine that captured its path at +// connect time. +func (s *ServiceManager) claimLegacyProfiles(profiles []Profile, id ipcauth.Identity) { + // A privileged caller reaches every profile already and an internal load + // has no caller, so neither should leave an owner behind. + if ipcauth.IsPrivilegedCaller(id) { + return + } + + if !hasUnownedLegacyProfile(profiles) { + return + } + + dir, ok := legacyDirForIdentity(id) + if !ok { + return + } + + principal := ipcauth.OwnerPrincipalForIdentity(id) + parsed, ok := ipcauth.ParsePrincipal(principal) + if !ok { + log.Warnf("not claiming legacy profiles, %q is not a usable owner", principal) + return + } + + for i := range profiles { + p := &profiles[i] + if len(p.Owners) > 0 || p.LegacyUserDir == "" || p.LegacyUserDir != dir { + continue + } + + if err := StampOwner(p.Path, id); err != nil { + log.Warnf("could not claim legacy profile %s for %s: %v", p.Path, principal, err) + continue + } + + p.Owners = []ipcauth.Principal{parsed} + log.Infof("claimed legacy profile %s for %s, its directory is named after that account", p.Path, principal) + } +} + +func hasUnownedLegacyProfile(profiles []Profile) bool { + for i := range profiles { + if profiles[i].LegacyUserDir != "" && len(profiles[i].Owners) == 0 { + return true + } + } + return false +} + +// legacyDirForIdentity is a variable so a test can supply an account name +// without depending on the host's user database. +var legacyDirForIdentity = resolveLegacyDir + +// resolveLegacyDir returns the per-username directory the old layout would have +// created for a caller, and whether there is one. +// +// Successes are cached for the process, failures are not, so a directory +// service that is briefly unreachable does not lock its users out until the +// daemon restarts. +func resolveLegacyDir(id ipcauth.Identity) (string, bool) { + key := ipcauth.OwnerPrincipalForIdentity(id) + + legacyDirMu.Lock() + cached, hit := legacyDirCache[key] + legacyDirMu.Unlock() + if hit { + return cached, cached != "" + } + + lookup := strconv.FormatUint(uint64(id.UID), 10) + if id.IsWindows() { + lookup = id.SID + } + + u, err := getent.LookupUserID(lookup) + if err != nil { + log.Warnf("cannot resolve %s to an account name, its legacy profiles stay unowned: %v", key, err) + return "", false + } + + dir := sanitizeProfileName(u.Username) + + legacyDirMu.Lock() + legacyDirCache[key] = dir + legacyDirMu.Unlock() + + return dir, dir != "" +} + +func (s *ServiceManager) loadAllProfiles() ([]Profile, error) { + _, activeIsDefault := s.activeProfileID() defaultName := readProfileName(DefaultConfigPath) if defaultName == "" { defaultName = defaultProfileName } - profiles := []Profile{{ - ID: defaultProfileName, - Name: defaultName, - Path: DefaultConfigPath, - IsActive: activeIsDefault, - // TODO: determine how to seed default owners - Owners: []ipcauth.Principal{}, - }} - - configDir, err := s.getConfigDir(username) - if err != nil { - return nil, fmt.Errorf("get config directory: %w", err) + // The default profile is not seeded with an owner: it starts unowned, and + // the first claim stamps it like any other profile. A file that is not + // there yet is unowned rather than unreadable, since the daemon writes it + // on first run and every listing before that would otherwise fail. + var profiles []Profile + defaultOwners, err := readProfileOwners(DefaultConfigPath) + switch { + case err == nil, errors.Is(err, os.ErrNotExist): + profiles = append(profiles, Profile{ + ID: defaultProfileName, + Name: defaultName, + Path: DefaultConfigPath, + IsActive: activeIsDefault, + Owners: defaultOwners, + }) + default: + // Same rule as a discovered profile whose owners cannot be read: leave + // it out rather than treat it as unowned, and leave it out rather than + // fail, so one unreadable file does not take every other profile with + // it. + log.Warnf("leaving the default profile out of the listing, its owners could not be read: %v", err) } + // The directory new profiles go to, plus every per-username directory left + // from before the ID-keyed layout. The first is not necessarily under + // DefaultConfigPathDir: a ServiceManager can be pointed at a directory of + // its own, which is what the mobile bindings do. + dirs := []profileDir{{path: s.profilesDirPath()}} + + configPathDir, err := os.ReadDir(DefaultConfigPathDir) + if err != nil && !errors.Is(err, os.ErrNotExist) { + return nil, fmt.Errorf("read profile directory: %w", err) + } + for _, entry := range configPathDir { + if !entry.IsDir() || entry.Name() == DefaultProfilePathDir { + continue + } + // Every other subdirectory is named after the account that created the + // profiles in it. The dot in profiles.v1 is what keeps them apart, + // since sanitizeProfileName drops dots. + dirs = append(dirs, profileDir{ + path: filepath.Join(DefaultConfigPathDir, entry.Name()), + legacyUser: entry.Name(), + }) + } + + var fileProfiles []Profile + scanned := make(map[string]bool, len(dirs)) + for _, dir := range dirs { + // The profiles directory is usually one of the subdirectories above, + // so without this a profile would be listed twice. + if scanned[dir.path] { + continue + } + scanned[dir.path] = true + + dirProfiles, err := s.getProfilesFromDirectory(dir) + if err != nil { + return nil, err + } + fileProfiles = append(fileProfiles, dirProfiles...) + } + + sort.Slice(fileProfiles, func(i, j int) bool { + if fileProfiles[i].Name != fileProfiles[j].Name { + return fileProfiles[i].Name < fileProfiles[j].Name + } + // Sort tie-break on ID so duplicate names always render in the same order. + return fileProfiles[i].ID < fileProfiles[j].ID + }) + profiles = append(profiles, fileProfiles...) + return profiles, nil +} + +// profileDir is one directory the loader scans. legacyUser is the sanitized +// username it is named after, empty for the directory profiles go to now. +type profileDir struct { + path string + legacyUser string +} + +func (s *ServiceManager) getProfilesFromDirectory(dir profileDir) ([]Profile, error) { + configDir := dir.path + activeID, _ := s.activeProfileID() entries, err := os.ReadDir(configDir) if err != nil { if errors.Is(err, os.ErrNotExist) { - return profiles, nil + return []Profile{}, nil } return nil, fmt.Errorf("read profile directory: %w", err) } @@ -550,26 +835,19 @@ func (s *ServiceManager) loadAllProfiles(username string) ([]Profile, error) { owners, err := readProfileOwners(path) if err != nil { - return nil, err + log.Warnf("reading profile owner failed for %s: %v", path, err) + continue } fileProfiles = append(fileProfiles, Profile{ - ID: stem, - Name: name, - Path: path, - IsActive: stem == ID(activeID), - Owners: owners, + ID: stem, + Name: name, + Path: path, + IsActive: stem == ID(activeID), + Owners: owners, + LegacyUserDir: dir.legacyUser, }) } - - sort.Slice(fileProfiles, func(i, j int) bool { - if fileProfiles[i].Name != fileProfiles[j].Name { - return fileProfiles[i].Name < fileProfiles[j].Name - } - // Sort tie-break on ID so duplicate names always render in the same order. - return fileProfiles[i].ID < fileProfiles[j].ID - }) - profiles = append(profiles, fileProfiles...) - return profiles, nil + return fileProfiles, nil } // readProfileName parses just the "name" field from the profile Json. @@ -606,27 +884,67 @@ func readProfileOwners(path string) ([]ipcauth.Principal, error) { principal, ok := ipcauth.ParsePrincipal(meta.Owners[0]) if !ok { - // A malformed entry is ignored rather than trusted. - log.Warnf("ignoring unparseable owner %q in %s", meta.Owners[0], path) - return nil, nil + // An entry that cannot be parsed is not trusted, and it is not an + // absence of ownership either: the profile records an owner that cannot + // be matched against anyone. + return nil, fmt.Errorf("unparseable owner %q in %s", meta.Owners[0], path) } return []ipcauth.Principal{principal}, nil } -// nolint: unused,unusedfunc +// StampOwner records a caller as a profile's owner, replacing whoever is +// recorded now. func StampOwner(path string, owner ipcauth.Identity) error { + if !owner.Known() { + return fmt.Errorf("cannot stamp owner that is not verified by the kernel") + } + return stampPrincipal(path, ipcauth.OwnerPrincipalForIdentity(owner)) +} + +// stampPrincipal records an owner principal directly. Migration needs this: it +// resolves an account name rather than a caller, and a name the kernel never +// vouched for must not become an Identity on the way. +func stampPrincipal(path, principal string) error { + return setProfileField(path, ownersFieldName, []string{principal}) +} + +// writeProfileName sets a profile's display name. Renaming does it on request, +// migration does it to move a name out of a filename that is about to change. +func writeProfileName(path, name string) error { + return setProfileField(path, nameFieldName, name) +} + +// setProfileField replaces one top-level key of a profile's JSON and leaves the +// rest of the document as it found it. +func setProfileField(path, field string, value any) error { data, err := os.ReadFile(path) if err != nil { return err } - var cfg Config - if err := json.Unmarshal(data, &cfg); err != nil { + + doc := map[string]json.RawMessage{} + if err := json.Unmarshal(data, &doc); err != nil { return err } - cfg.Owners = []string{ipcauth.OwnerPrincipalForIdentity(owner)} + if doc == nil { + return fmt.Errorf("profile %s holds no object to set %s on", path, field) + } - if err := util.WriteJson(context.Background(), path, cfg); err != nil { - return fmt.Errorf("failed to write profile owner: %w", err) + raw, err := json.Marshal(value) + if err != nil { + return fmt.Errorf("encode %s of %s: %w", field, path, err) + } + + // Decoding matches keys case-insensitively + for k := range doc { + if k != field && strings.EqualFold(k, field) { + delete(doc, k) + } + } + doc[field] = raw + + if err := util.WriteJsonWithRestrictedPermission(context.Background(), path, doc); err != nil { + return fmt.Errorf("write profile %s: %w", path, err) } return nil } @@ -648,12 +966,12 @@ func (s *ServiceManager) activeProfileID() (ID, bool) { // precedence is: exact ID match, then unique exact name, then unique ID // prefix. Ambiguous matches return *ErrAmbiguousHandle so callers can // surface the candidates. -func (s *ServiceManager) ResolveProfile(handle, username string) (*Profile, error) { +func (s *ServiceManager) ResolveProfile(handle string, userID ipcauth.Identity) (*Profile, error) { if handle == "" { return nil, fmt.Errorf("profile handle is empty") } - profiles, err := s.loadAllProfiles(username) + profiles, err := s.loadAllProfilesForIdentity(userID) if err != nil { return nil, err } diff --git a/client/internal/profilemanager/service_test.go b/client/internal/profilemanager/service_test.go index 09251119a..de71190c5 100644 --- a/client/internal/profilemanager/service_test.go +++ b/client/internal/profilemanager/service_test.go @@ -2,22 +2,26 @@ package profilemanager import ( "context" + "encoding/json" "errors" "os" "os/user" "path/filepath" + "runtime" + "strconv" "strings" "testing" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" + "github.com/netbirdio/netbird/client/internal/ipcauth" "github.com/netbirdio/netbird/util" ) // withTestSM wires up patched globals + a clean config dir and returns a // fully initialized ServiceManager plus the username we are scoped to. -func withTestSM(t *testing.T, fn func(sm *ServiceManager, username string)) { +func withTestSM(t *testing.T, fn func(sm *ServiceManager, id ipcauth.Identity)) { t.Helper() withTempConfigDir(t, func(configDir string) { withPatchedGlobals(t, configDir, func() { @@ -25,17 +29,21 @@ func withTestSM(t *testing.T, fn func(sm *ServiceManager, username string)) { require.NoError(t, err) sm := &ServiceManager{} require.NoError(t, sm.CreateDefaultProfile()) - fn(sm, u.Username) + uid, err := strconv.ParseUint(u.Uid, 10, 32) + require.NoError(t, err) + userID := ipcauth.Identity{UID: uint32(uid)} + userID = ipcauth.KnownForTest(userID) + fn(sm, userID) }) }) } func TestServiceProfile_ExactID(t *testing.T) { - withTestSM(t, func(sm *ServiceManager, username string) { - created, err := sm.AddProfile("work", username, nil) + withTestSM(t, func(sm *ServiceManager, userID ipcauth.Identity) { + created, err := sm.AddProfile("work", nil) require.NoError(t, err) - got, err := sm.ResolveProfile(created.ID.String(), username) + got, err := sm.ResolveProfile(created.ID.String(), userID) require.NoError(t, err) assert.Equal(t, created.ID, got.ID) assert.Equal(t, "work", got.Name) @@ -43,29 +51,31 @@ func TestServiceProfile_ExactID(t *testing.T) { } func TestServiceProfile_IDPrefix(t *testing.T) { - withTestSM(t, func(sm *ServiceManager, username string) { - created, err := sm.AddProfile("work", username, nil) + withTestSM(t, func(sm *ServiceManager, userID ipcauth.Identity) { + created, err := sm.AddProfile("work", &userID) require.NoError(t, err) prefix := created.ID[:4] - got, err := sm.ResolveProfile(prefix.String(), username) + got, err := sm.ResolveProfile(prefix.String(), userID) require.NoError(t, err) assert.Equal(t, created.ID, got.ID) }) } func TestServiceProfile_AmbiguousPrefix(t *testing.T) { - withTestSM(t, func(sm *ServiceManager, username string) { + withTestSM(t, func(sm *ServiceManager, userID ipcauth.Identity) { // Plant two profiles whose IDs share a known prefix by writing // the files directly, since generated IDs are random. - configDir, err := sm.getConfigDir(username) + user, err := user.Current() + require.NoError(t, err) + configDir, err := sm.getConfigDirLegacy(user.Username) require.NoError(t, err) for _, id := range []string{"abcd1111aaaa", "abcd2222bbbb"} { path := filepath.Join(configDir, id+".json") require.NoError(t, util.WriteJson(context.Background(), path, &Config{Name: id})) } - _, err = sm.ResolveProfile("abcd", username) + _, err = sm.ResolveProfile("abcd", userID) var amb *ErrAmbiguousHandle require.ErrorAs(t, err, &amb) assert.Equal(t, AmbiguityKindIDPrefix, amb.Kind) @@ -74,24 +84,24 @@ func TestServiceProfile_AmbiguousPrefix(t *testing.T) { } func TestServiceProfile_ExactNameUnique(t *testing.T) { - withTestSM(t, func(sm *ServiceManager, username string) { - _, err := sm.AddProfile("work", username, nil) + withTestSM(t, func(sm *ServiceManager, userID ipcauth.Identity) { + _, err := sm.AddProfile("work", &userID) require.NoError(t, err) - got, err := sm.ResolveProfile("work", username) + got, err := sm.ResolveProfile("work", userID) require.NoError(t, err) assert.Equal(t, "work", got.Name) }) } func TestServiceProfile_AmbiguousName(t *testing.T) { - withTestSM(t, func(sm *ServiceManager, username string) { - _, err := sm.AddProfile("work", username, nil) + withTestSM(t, func(sm *ServiceManager, userID ipcauth.Identity) { + _, err := sm.AddProfile("work", &userID) require.NoError(t, err) - _, err = sm.AddProfile("work", username, nil) + _, err = sm.AddProfile("work", &userID) require.NoError(t, err) - _, err = sm.ResolveProfile("work", username) + _, err = sm.ResolveProfile("work", userID) var amb *ErrAmbiguousHandle require.ErrorAs(t, err, &amb) assert.Equal(t, AmbiguityKindName, amb.Kind) @@ -100,15 +110,15 @@ func TestServiceProfile_AmbiguousName(t *testing.T) { } func TestServiceProfile_NotFound(t *testing.T) { - withTestSM(t, func(sm *ServiceManager, username string) { - _, err := sm.ResolveProfile("nope", username) + withTestSM(t, func(sm *ServiceManager, userID ipcauth.Identity) { + _, err := sm.ResolveProfile("nope", userID) assert.ErrorIs(t, err, ErrProfileNotFound) }) } func TestServiceProfile_DefaultByExactID(t *testing.T) { - withTestSM(t, func(sm *ServiceManager, username string) { - got, err := sm.ResolveProfile(defaultProfileName, username) + withTestSM(t, func(sm *ServiceManager, userID ipcauth.Identity) { + got, err := sm.ResolveProfile(defaultProfileName, userID) require.NoError(t, err) assert.Equal(t, defaultProfileName, got.ID.String()) }) @@ -117,13 +127,15 @@ func TestServiceProfile_DefaultByExactID(t *testing.T) { func TestServiceProfile_LegacyFilenameCoexists(t *testing.T) { // Legacy profiles stored as .json with no "name" JSON field // should still be discoverable by name and removable by name. - withTestSM(t, func(sm *ServiceManager, username string) { - configDir, err := sm.getConfigDir(username) + withTestSM(t, func(sm *ServiceManager, userID ipcauth.Identity) { + user, err := user.Current() + require.NoError(t, err) + configDir, err := sm.getConfigDirLegacy(user.Username) require.NoError(t, err) path := filepath.Join(configDir, "legacy.json") require.NoError(t, util.WriteJson(context.Background(), path, &Config{})) - got, err := sm.ResolveProfile("legacy", username) + got, err := sm.ResolveProfile("legacy", userID) require.NoError(t, err) assert.Equal(t, "legacy", got.ID.String()) // Name falls back to the filename stem when JSON omits it. @@ -132,11 +144,11 @@ func TestServiceProfile_LegacyFilenameCoexists(t *testing.T) { } func TestAddProfile_AllowsDuplicateWithFlag(t *testing.T) { - withTestSM(t, func(sm *ServiceManager, username string) { - first, err := sm.AddProfile("work", username, nil) + withTestSM(t, func(sm *ServiceManager, userID ipcauth.Identity) { + first, err := sm.AddProfile("work", &userID) require.NoError(t, err) - second, err := sm.AddProfile("work", username, nil) + second, err := sm.AddProfile("work", &userID) require.NoError(t, err) assert.NotEqual(t, first.ID, second.ID) assert.Equal(t, "work", second.Name) @@ -144,22 +156,22 @@ func TestAddProfile_AllowsDuplicateWithFlag(t *testing.T) { } func TestAddProfile_RejectsInvalidNames(t *testing.T) { - withTestSM(t, func(sm *ServiceManager, username string) { + withTestSM(t, func(sm *ServiceManager, userID ipcauth.Identity) { cases := []string{ "", // empty "\x00\x01", // only control chars (becomes empty) strings.Repeat("a", maxProfileNameLen+1), // too long } for _, name := range cases { - _, err := sm.AddProfile(name, username, nil) + _, err := sm.AddProfile(name, &userID) assert.Error(t, err, "expected error for %q", name) } }) } func TestRemoveProfile_RejectsInvalidID(t *testing.T) { - withTestSM(t, func(sm *ServiceManager, username string) { - err := sm.RemoveProfile("../escape", username) + withTestSM(t, func(sm *ServiceManager, userID ipcauth.Identity) { + err := sm.RemoveProfile("../escape", userID) assert.Error(t, err) }) } @@ -214,17 +226,537 @@ func TestIsValidProfileFilenameStem(t *testing.T) { } func TestRemoveProfile_DeletesStateFile(t *testing.T) { - withTestSM(t, func(sm *ServiceManager, username string) { - created, err := sm.AddProfile("work", username, nil) + withTestSM(t, func(sm *ServiceManager, userID ipcauth.Identity) { + created, err := sm.AddProfile("work", &userID) require.NoError(t, err) - configDir, err := sm.getConfigDir(username) + user, err := user.Current() + require.NoError(t, err) + configDir, err := sm.getConfigDirLegacy(user.Username) require.NoError(t, err) statePath := filepath.Join(configDir, created.ID.String()+".state.json") require.NoError(t, os.WriteFile(statePath, []byte(`{"email":"a@b"}`), 0600)) - require.NoError(t, sm.RemoveProfile(created.ID, username)) + require.NoError(t, sm.RemoveProfile(created.ID, userID)) _, err = os.Stat(statePath) assert.True(t, errors.Is(err, os.ErrNotExist), "state file should be removed") }) } + +// profileIDs is the set of profile IDs in a listing, for membership assertions +// that do not care about the default profile always being present. +func profileIDs(profiles []Profile) []string { + ids := make([]string, 0, len(profiles)) + for _, p := range profiles { + ids = append(ids, p.ID.String()) + } + return ids +} + +func TestListProfiles_ScopedToOwner(t *testing.T) { + withTestSM(t, func(sm *ServiceManager, _ ipcauth.Identity) { + // Two synthetic users rather than the current one: this process is its + // own daemon, and IsPrivilegedCaller delegates to a caller sharing an + // unprivileged daemon's identity, so the current user resolves + // unfiltered here. + alice := ipcauth.KnownForTest(ipcauth.Identity{UID: 4242}) + bob := ipcauth.KnownForTest(ipcauth.Identity{UID: 4243}) + hers, err := sm.AddProfile("hers", &alice) + require.NoError(t, err) + his, err := sm.AddProfile("his", &bob) + require.NoError(t, err) + + got, err := sm.ListProfiles(alice) + require.NoError(t, err) + assert.Contains(t, profileIDs(got), hers.ID.String()) + assert.NotContains(t, profileIDs(got), his.ID.String(), + "another user's profile must not be listed") + }) +} + +func TestListProfiles_PrivilegedResolvesUnfiltered(t *testing.T) { + withTestSM(t, func(sm *ServiceManager, userID ipcauth.Identity) { + other := ipcauth.KnownForTest(ipcauth.Identity{UID: 4242}) + mine, err := sm.AddProfile("mine", &userID) + require.NoError(t, err) + theirs, err := sm.AddProfile("theirs", &other) + require.NoError(t, err) + + root := ipcauth.KnownForTest(ipcauth.Identity{UID: 0}) + got, err := sm.ListProfiles(root) + require.NoError(t, err) + assert.Contains(t, profileIDs(got), mine.ID.String()) + assert.Contains(t, profileIDs(got), theirs.ID.String()) + assert.Contains(t, profileIDs(got), defaultProfileName) + }) +} + +func TestListProfiles_OnlyTheDefaultFailsOpenWhenUnowned(t *testing.T) { + withTestSM(t, func(sm *ServiceManager, _ ipcauth.Identity) { + unowned, err := sm.AddProfile("unowned", nil) + require.NoError(t, err) + + alice := ipcauth.KnownForTest(ipcauth.Identity{UID: 4242}) + got, err := sm.ListProfiles(alice) + require.NoError(t, err) + assert.Contains(t, profileIDs(got), defaultProfileName, + "a fresh install has to be usable before anything is claimed") + assert.NotContains(t, profileIDs(got), unowned.ID.String(), + "every other profile needs an owner before anyone can address it") + + root := ipcauth.KnownForTest(ipcauth.Identity{UID: 0}) + got, err = sm.ListProfiles(root) + require.NoError(t, err) + assert.Contains(t, profileIDs(got), unowned.ID.String(), + "root still reaches it, which is how it gets assigned") + + nobody, err := sm.ListProfiles(ipcauth.Identity{}) + require.NoError(t, err) + assert.Empty(t, profileIDs(nobody), "an unattested caller reaches nothing") + }) +} + +// withLegacyLayout wires up the globals the way withTestSM does, without the +// identity, since these tests supply their own. +func withLegacyLayout(t *testing.T, fn func(sm *ServiceManager, configDir string)) { + t.Helper() + withTempConfigDir(t, func(configDir string) { + withPatchedGlobals(t, configDir, func() { + sm := &ServiceManager{} + require.NoError(t, sm.CreateDefaultProfile()) + fn(sm, configDir) + }) + }) +} + +// writeLegacyProfile drops a profile into a per-username directory, the layout +// every profile used before the ID-keyed one. It writes no name, since a legacy +// profile took its display name from the filename. +func writeLegacyProfile(t *testing.T, configDir, dirName, id string, fields map[string]any) string { + t.Helper() + dir := filepath.Join(configDir, dirName) + require.NoError(t, os.MkdirAll(dir, 0700)) + + doc := map[string]any{} + for k, v := range fields { + doc[k] = v + } + path := filepath.Join(dir, id+".json") + require.NoError(t, util.WriteJson(context.Background(), path, doc)) + return path +} + +// stubLegacyDir replaces the account lookup, so the claim path can be exercised +// without an account existing on the machine running the test. +func stubLegacyDir(t *testing.T, dir string) { + t.Helper() + orig := legacyDirForIdentity + legacyDirForIdentity = func(ipcauth.Identity) (string, bool) { return dir, dir != "" } + t.Cleanup(func() { legacyDirForIdentity = orig }) +} + +func readOwners(t *testing.T, path string) []string { + t.Helper() + data, err := os.ReadFile(path) + require.NoError(t, err) + var meta ownerMeta + require.NoError(t, json.Unmarshal(data, &meta)) + return meta.Owners +} + +func TestListProfiles_UnownedLegacyProfileIsPrivilegedOnly(t *testing.T) { + withLegacyLayout(t, func(sm *ServiceManager, configDir string) { + path := writeLegacyProfile(t, configDir, "alice", "work", nil) + stubLegacyDir(t, "bob") + + bob := ipcauth.KnownForTest(ipcauth.Identity{UID: 4242}) + got, err := sm.ListProfiles(bob) + require.NoError(t, err) + assert.NotContains(t, profileIDs(got), "work", + "a profile sitting in someone else's directory is not free to take") + assert.Empty(t, readOwners(t, path), "and it is not claimed on the way past") + + root := ipcauth.KnownForTest(ipcauth.Identity{UID: 0}) + got, err = sm.ListProfiles(root) + require.NoError(t, err) + assert.Contains(t, profileIDs(got), "work", + "root still reaches it, which is how it gets reassigned") + }) +} + +func TestListProfiles_ClaimsLegacyProfileForItsOwnAccount(t *testing.T) { + withLegacyLayout(t, func(sm *ServiceManager, configDir string) { + path := writeLegacyProfile(t, configDir, "alice", "work", nil) + stubLegacyDir(t, "alice") + + alice := ipcauth.KnownForTest(ipcauth.Identity{UID: 4242}) + got, err := sm.ListProfiles(alice) + require.NoError(t, err) + assert.Contains(t, profileIDs(got), "work", + "the claim lands before the listing is filtered, so the gap closes in one call") + assert.Equal(t, []string{"uid:4242"}, readOwners(t, path)) + + // The claim is on disk now, so it is the owner check and not the + // directory name that keeps the next caller out. + stubLegacyDir(t, "alice") + other := ipcauth.KnownForTest(ipcauth.Identity{UID: 5252}) + got, err = sm.ListProfiles(other) + require.NoError(t, err) + assert.NotContains(t, profileIDs(got), "work") + assert.Equal(t, []string{"uid:4242"}, readOwners(t, path), + "a second caller does not overwrite a stamped owner") + }) +} + +func TestClaimLegacyProfile_LeavesTheProfileWhereItIs(t *testing.T) { + withLegacyLayout(t, func(sm *ServiceManager, configDir string) { + path := writeLegacyProfile(t, configDir, "alice", "work", nil) + for _, suffix := range []string{".state.json", prefsFileSuffix} { + require.NoError(t, os.WriteFile(filepath.Join(configDir, "alice", "work"+suffix), []byte("{}"), 0600)) + } + stubLegacyDir(t, "alice") + + alice := ipcauth.KnownForTest(ipcauth.Identity{UID: 4242}) + got, err := sm.ListProfiles(alice) + require.NoError(t, err) + + claimed := ownedProfile(t, got, "uid:4242") + assert.Equal(t, ID("work"), claimed.ID, "claiming does not re-key the profile") + assert.Equal(t, path, claimed.Path) + assert.Equal(t, "alice", claimed.LegacyUserDir, + "the directory is a leftover now, not something the claim rewrites") + + // The engine captures its state file path at connect time and writes + // back to whatever it captured, so nothing here may move that file. + assert.FileExists(t, filepath.Join(configDir, "alice", "work.state.json")) + assert.FileExists(t, filepath.Join(configDir, "alice", "work"+prefsFileSuffix)) + assert.NoFileExists(t, filepath.Join(configDir, DefaultProfilePathDir, "work.json")) + }) +} + +func TestClaimLegacyProfile_NamesakeInAnotherDirectoryIsUntouched(t *testing.T) { + withLegacyLayout(t, func(sm *ServiceManager, configDir string) { + writeLegacyProfile(t, configDir, "alice", "work", nil) + bobs := writeLegacyProfile(t, configDir, "bob", "work", nil) + stubLegacyDir(t, "alice") + + alice := ipcauth.KnownForTest(ipcauth.Identity{UID: 4242}) + got, err := sm.ListProfiles(alice) + require.NoError(t, err) + + claimed := ownedProfile(t, got, "uid:4242") + assert.Equal(t, filepath.Join(configDir, "alice", "work.json"), claimed.Path, + "a legacy ID is a display name two accounts can hold, so the path is what tells them apart") + assert.Empty(t, readOwners(t, bobs), "bob's namesake is not claimed") + }) +} + +func ownedProfile(t *testing.T, profiles []Profile, principal string) Profile { + t.Helper() + var found []Profile + for _, p := range profiles { + if len(p.Owners) == 1 && p.Owners[0].String() == principal { + found = append(found, p) + } + } + require.Len(t, found, 1, "exactly one profile is owned by %s", principal) + return found[0] +} + +func TestClaimLegacyProfile_SkipsOneAlreadyOwnedInTheSameDirectory(t *testing.T) { + withLegacyLayout(t, func(sm *ServiceManager, configDir string) { + // One unowned profile keeps the claim running over the directory, so + // the owned one beside it is reached and has to be left alone. + taken := writeLegacyProfile(t, configDir, "alice", "taken", map[string]any{ + "Owners": []string{"uid:9999"}, + }) + free := writeLegacyProfile(t, configDir, "alice", "free", nil) + stubLegacyDir(t, "alice") + + alice := ipcauth.KnownForTest(ipcauth.Identity{UID: 4242}) + _, err := sm.ListProfiles(alice) + require.NoError(t, err) + + assert.Equal(t, []string{"uid:9999"}, readOwners(t, taken), + "a profile that already has an owner is not restamped") + assert.Equal(t, []string{"uid:4242"}, readOwners(t, free)) + }) +} + +func TestRenameProfile(t *testing.T) { + withTestSM(t, func(sm *ServiceManager, userID ipcauth.Identity) { + created, err := sm.AddProfile("work", &userID) + require.NoError(t, err) + + require.NoError(t, sm.RenameProfile(created.ID, userID, "weekend")) + + got, err := sm.ResolveProfile(created.ID.String(), userID) + require.NoError(t, err) + assert.Equal(t, "weekend", got.Name, "the new name is on disk") + assert.Equal(t, created.ID, got.ID, "renaming does not re-key the profile") + assert.Equal(t, created.Path, got.Path) + }) +} + +func TestRenameProfile_NotTheCallersProfile(t *testing.T) { + withTestSM(t, func(sm *ServiceManager, userID ipcauth.Identity) { + created, err := sm.AddProfile("work", &userID) + require.NoError(t, err) + + stranger := ipcauth.KnownForTest(ipcauth.Identity{UID: 4242}) + require.Error(t, sm.RenameProfile(created.ID, stranger, "weekend"), + "a profile the caller cannot address is not theirs to rename") + + got, err := sm.ResolveProfile(created.ID.String(), userID) + require.NoError(t, err) + assert.Equal(t, "work", got.Name) + }) +} + +func TestResolveProfile_ClaimsOnTheWayThrough(t *testing.T) { + withLegacyLayout(t, func(sm *ServiceManager, configDir string) { + path := writeLegacyProfile(t, configDir, "alice", "work", nil) + stubLegacyDir(t, "alice") + + // Resolution is what switching a profile goes through, so the claim has + // to land here and not only when something lists profiles. + alice := ipcauth.KnownForTest(ipcauth.Identity{UID: 4242}) + got, err := sm.ResolveProfile("work", alice) + require.NoError(t, err) + assert.Equal(t, path, got.Path) + assert.Equal(t, []string{"uid:4242"}, readOwners(t, path)) + }) +} + +func TestListProfiles_PrivilegedCallerDoesNotClaim(t *testing.T) { + withLegacyLayout(t, func(sm *ServiceManager, configDir string) { + path := writeLegacyProfile(t, configDir, "root", "work", nil) + stubLegacyDir(t, "root") + + root := ipcauth.KnownForTest(ipcauth.Identity{UID: 0}) + got, err := sm.ListProfiles(root) + require.NoError(t, err) + assert.Contains(t, profileIDs(got), "work") + assert.Empty(t, readOwners(t, path), + "root reaches every profile anyway, so a listing must not stamp one") + }) +} + +func TestStampOwner(t *testing.T) { + withLegacyLayout(t, func(_ *ServiceManager, configDir string) { + path := writeLegacyProfile(t, configDir, "alice", "work", map[string]any{ + "DisableAutoConnect": true, + }) + + require.NoError(t, StampOwner(path, ipcauth.KnownForTest(ipcauth.Identity{UID: 4242}))) + assert.Equal(t, []string{"uid:4242"}, readOwners(t, path)) + + var fields map[string]any + data, err := os.ReadFile(path) + require.NoError(t, err) + require.NoError(t, json.Unmarshal(data, &fields)) + assert.Equal(t, true, fields["DisableAutoConnect"], + "the rest of the config survives the stamp") + + require.NoError(t, StampOwner(path, ipcauth.KnownForTest(ipcauth.Identity{UID: 5252}))) + assert.Equal(t, []string{"uid:5252"}, readOwners(t, path), + "stamping replaces whoever is recorded, the caller decides whether to") + }) +} + +func TestClaimLegacyProfile_LeavesAnOwnerItCannotParse(t *testing.T) { + withLegacyLayout(t, func(sm *ServiceManager, configDir string) { + // A principal kind this build does not know, which is what a client + // newer than this one leaves behind after a downgrade. The loader + // drops such a profile rather than reporting it as unowned, which is + // what keeps the claim from treating it as free to take. + path := writeLegacyProfile(t, configDir, "alice", "work", map[string]any{ + "Owners": []string{"group:devs"}, + }) + stubLegacyDir(t, "alice") + + alice := ipcauth.KnownForTest(ipcauth.Identity{UID: 4242}) + got, err := sm.ListProfiles(alice) + require.NoError(t, err) + + assert.NotContains(t, profileIDs(got), "work") + assert.Equal(t, []string{"group:devs"}, readOwners(t, path), + "the profile is not taken over, it waits for an explicit claim") + }) +} + +func TestResolveLegacyDir_SanitizesTheSameStringTheOldLayoutDid(t *testing.T) { + if runtime.GOOS == "windows" { + t.Skip("the windows path resolves a SID rather than a numeric uid") + } + + u, err := user.Current() + require.NoError(t, err) + uid, err := strconv.ParseUint(u.Uid, 10, 32) + require.NoError(t, err) + + got, ok := resolveLegacyDir(ipcauth.KnownForTest(ipcauth.Identity{UID: uint32(uid)})) + require.True(t, ok) + assert.Equal(t, sanitizeProfileName(u.Username), got, + "the directory has to come from the account name, sanitized the way the old layout did") +} + +func TestListProfiles_UnreadableOwnersAreSkipped(t *testing.T) { + withTestSM(t, func(sm *ServiceManager, _ ipcauth.Identity) { + configDir, err := sm.getConfigDir() + require.NoError(t, err) + require.NoError(t, os.MkdirAll(configDir, 0700)) + // An owner entry that parses as JSON but names no principal: the + // profile records an owner, so it is not unowned, but nothing can match + // it. + const tampered = "abcd1111aaaa" + path := filepath.Join(configDir, tampered+".json") + require.NoError(t, os.WriteFile(path, []byte(`{"Name":"tampered","Owners":["garbage"]}`), 0600)) + + alice := ipcauth.KnownForTest(ipcauth.Identity{UID: 4242}) + got, err := sm.ListProfiles(alice) + require.NoError(t, err) + assert.NotContains(t, profileIDs(got), tampered, + "a profile whose owners cannot be read must not fall back to unowned") + + // Not even a privileged caller: the loader drops the profile before + // ownership is ever consulted, so corrupting the owner list hides the + // profile rather than unlocking it. + root := ipcauth.KnownForTest(ipcauth.Identity{UID: 0}) + got, err = sm.ListProfiles(root) + require.NoError(t, err) + assert.NotContains(t, profileIDs(got), tampered) + }) +} + +func TestListProfiles_UnidentifiedCallerGetsNothing(t *testing.T) { + withTestSM(t, func(sm *ServiceManager, userID ipcauth.Identity) { + _, err := sm.AddProfile("mine", &userID) + require.NoError(t, err) + _, err = sm.AddProfile("unowned", nil) + require.NoError(t, err) + + // The zero Identity carries uid 0, so an unidentified caller must be + // refused before privilege is ever considered. + got, err := sm.ListProfiles(ipcauth.Identity{}) + require.NoError(t, err) + assert.Empty(t, got) + }) +} + +func TestActiveProfilePath_RefusesToGuessBetweenNamesakes(t *testing.T) { + withLegacyLayout(t, func(sm *ServiceManager, configDir string) { + writeLegacyProfile(t, configDir, "alice", "work", nil) + writeLegacyProfile(t, configDir, "bob", "work", nil) + + _, err := sm.ActiveProfilePath(&ActiveProfileState{ID: "work"}) + require.ErrorIs(t, err, ErrAmbiguousActiveProfile, + "running one account's config under another account's name is worse than not running") + + path, err := sm.ActiveProfilePath(&ActiveProfileState{ID: "work", Username: "bob"}) + require.NoError(t, err) + assert.Equal(t, filepath.Join(configDir, "bob", "work.json"), path, + "the recorded directory is what tells the namesakes apart") + }) +} + +func TestListProfiles_ClaimKeepsFieldsThisVersionDoesNotModel(t *testing.T) { + withLegacyLayout(t, func(sm *ServiceManager, configDir string) { + // What a client newer than this one leaves behind: a key Config has no + // field for, next to one it does. + newer := map[string]any{"Enabled": true, "Hosts": []any{"a", "b"}} + path := writeLegacyProfile(t, configDir, "alice", "work", map[string]any{ + "MTU": 1280, + "SomethingNewer": newer, + }) + stubLegacyDir(t, "alice") + + alice := ipcauth.KnownForTest(ipcauth.Identity{UID: 4242}) + _, err := sm.ListProfiles(alice) + require.NoError(t, err) + assert.Equal(t, []string{"uid:4242"}, readOwners(t, path), "the claim still lands") + + data, err := os.ReadFile(path) + require.NoError(t, err) + var doc map[string]any + require.NoError(t, json.Unmarshal(data, &doc)) + + assert.Equal(t, newer, doc["SomethingNewer"], + "a listing must not drop the settings of a client that models more than this one") + assert.Equal(t, float64(1280), doc["MTU"], "and leaves the ones it does model alone") + assert.NotContains(t, doc, "PrivateKey", + "nor write out the rest of Config just because it has fields for it") + }) +} + +func TestSetProfileField_ReplacesAKeySpelledInAnotherCase(t *testing.T) { + withLegacyLayout(t, func(sm *ServiceManager, configDir string) { + path := writeLegacyProfile(t, configDir, "alice", "work", map[string]any{ + "owners": []any{"uid:1"}, + }) + + require.NoError(t, stampPrincipal(path, "uid:4242")) + + data, err := os.ReadFile(path) + require.NoError(t, err) + var doc map[string]any + require.NoError(t, json.Unmarshal(data, &doc)) + + assert.NotContains(t, doc, "owners", + "two spellings of one field would leave the reader to pick") + assert.Equal(t, []any{"uid:4242"}, doc["Owners"]) + assert.Equal(t, []string{"uid:4242"}, readOwners(t, path)) + }) +} + +func TestSetProfileField_RefusesADocumentThatIsNotAnObject(t *testing.T) { + withLegacyLayout(t, func(sm *ServiceManager, configDir string) { + path := filepath.Join(configDir, "alice", "work.json") + require.NoError(t, os.MkdirAll(filepath.Dir(path), 0700)) + require.NoError(t, os.WriteFile(path, []byte("null"), 0600)) + + require.Error(t, stampPrincipal(path, "uid:4242"), + "a profile that is not an object is not one an owner can be set on") + + data, err := os.ReadFile(path) + require.NoError(t, err) + assert.Equal(t, "null", string(data), "and it is left as it was found") + }) +} + +func TestSetProfileField_KeepsKeysItWasNotAskedToWrite(t *testing.T) { + withLegacyLayout(t, func(sm *ServiceManager, configDir string) { + // Keys Config has no field for, in every shape a newer client could + // leave one behind. + unknown := map[string]any{ + "String": "keep me", + "Number": float64(7), + "Bool": true, + "Null": nil, + "List": []any{"a", float64(2), false}, + "Object": map[string]any{"Nested": map[string]any{"Deep": []any{float64(1)}}}, + } + fields := map[string]any{"MTU": 1280} + for k, v := range unknown { + fields[k] = v + } + path := writeLegacyProfile(t, configDir, "alice", "work", fields) + + require.NoError(t, setProfileField(path, ownersFieldName, []string{"uid:4242"})) + require.NoError(t, setProfileField(path, nameFieldName, "Work")) + + data, err := os.ReadFile(path) + require.NoError(t, err) + var doc map[string]any + require.NoError(t, json.Unmarshal(data, &doc)) + + for k, want := range unknown { + assert.Equal(t, want, doc[k], + "%s is not a key this version models, so it is not this version's to drop", k) + } + assert.Equal(t, float64(1280), doc["MTU"], "a key it does model is left where it was too") + assert.Equal(t, []any{"uid:4242"}, doc["Owners"], "and the fields it was asked for are written") + assert.Equal(t, "Work", doc["Name"]) + assert.Len(t, doc, len(unknown)+3, "with nothing else added") + }) +} diff --git a/client/mobile/profile_manager.go b/client/mobile/profile_manager.go index 66a8ba2dc..8cb721c8d 100644 --- a/client/mobile/profile_manager.go +++ b/client/mobile/profile_manager.go @@ -11,6 +11,7 @@ import ( log "github.com/sirupsen/logrus" + "github.com/netbirdio/netbird/client/internal/ipcauth" "github.com/netbirdio/netbird/client/internal/profilemanager" "github.com/netbirdio/netbird/client/mdm" ) @@ -57,8 +58,12 @@ type Profile struct { // All profile identity is ID-based; the human-readable name lives inside the // profile config's Name field. type ProfileManager struct { - configDir string - username string + configDir string + username string + // identity scopes profile ownership. There is no IPC hop on mobile: the + // manager runs inside the app, so the owner of a profile is the app process + // itself, and the device has a single user anyway. + identity ipcauth.Identity serviceMgr *profilemanager.ServiceManager mdmLoader *mdm.Loader } @@ -82,9 +87,18 @@ func NewProfileManager(configDir, username string) *ProfileManager { profilesDir := filepath.Join(configDir, profilesSubdir) serviceMgr := profilemanager.NewServiceManagerWithProfilesDir(defaultConfigPath, profilesDir) + // A failed read leaves the zero Identity, which is not Known and therefore + // owns nothing: profile access fails closed rather than falling back to + // something permissive. + identity, err := ipcauth.CurrentProcessIdentity() + if err != nil { + log.Errorf("failed to read this process's identity, profiles will be inaccessible: %v", err) + } + return &ProfileManager{ configDir: configDir, username: username, + identity: identity, serviceMgr: serviceMgr, } } @@ -92,7 +106,7 @@ func NewProfileManager(configDir, username string) *ProfileManager { // ListProfiles returns all available profiles, including the default profile, // with their active status set. func (pm *ProfileManager) ListProfiles() ([]Profile, error) { - internalProfiles, err := pm.serviceMgr.ListProfiles(pm.username) + internalProfiles, err := pm.serviceMgr.ListProfiles(pm.identity) if err != nil { return nil, fmt.Errorf("list profiles: %w", err) } @@ -118,7 +132,7 @@ func (pm *ProfileManager) GetActiveProfile() (*Profile, error) { return nil, fmt.Errorf("get active profile: %w", err) } - prof, err := pm.serviceMgr.ResolveProfile(activeState.ID.String(), pm.username) + prof, err := pm.serviceMgr.ResolveProfile(activeState.ID.String(), pm.identity) if err != nil { return nil, fmt.Errorf("resolve active profile %q: %w", activeState.ID, err) } @@ -153,7 +167,7 @@ func (pm *ProfileManager) AddProfile(displayName string) (*Profile, error) { if err := pm.checkProfilesAllowed(); err != nil { return nil, err } - profile, err := pm.serviceMgr.AddProfile(displayName, pm.username, nil) + profile, err := pm.serviceMgr.AddProfile(displayName, &pm.identity) if err != nil { return nil, fmt.Errorf("add profile: %w", err) } @@ -168,7 +182,7 @@ func (pm *ProfileManager) RenameProfile(id string, newName string) error { if err := pm.checkProfilesAllowed(); err != nil { return err } - if err := pm.serviceMgr.RenameProfile(profilemanager.ID(id), pm.username, newName); err != nil { + if err := pm.serviceMgr.RenameProfile(profilemanager.ID(id), pm.identity, newName); err != nil { return fmt.Errorf("rename profile: %w", err) } @@ -222,7 +236,7 @@ func (pm *ProfileManager) RemoveProfile(id string) error { return err } - if err := pm.serviceMgr.RemoveProfile(profilemanager.ID(id), pm.username); err != nil { + if err := pm.serviceMgr.RemoveProfile(profilemanager.ID(id), pm.identity); err != nil { return fmt.Errorf("remove profile: %w", err) } diff --git a/client/server/login_gate_test.go b/client/server/login_gate_test.go index de62a8180..cff894ac7 100644 --- a/client/server/login_gate_test.go +++ b/client/server/login_gate_test.go @@ -31,6 +31,7 @@ func TestLogin_RefusedChangeLeavesTheProfileAlone(t *testing.T) { ConfigPath: filepath.Join(profilemanager.DefaultConfigPathDir, target+".json"), ManagementURL: "https://api.netbird.io:443", ServerSSHAllowed: boolPtr(true), + Owner: testProfileOwner(), }) require.NoError(t, err) @@ -63,6 +64,7 @@ func TestLogin_ChangeThatBecomesPrivilegedMidRequestHasNoSideEffects(t *testing. ConfigPath: targetPath, ManagementURL: "https://api.netbird.io:443", ServerSSHAllowed: boolPtr(false), + Owner: testProfileOwner(), }) require.NoError(t, err) @@ -110,6 +112,7 @@ func TestLogin_RefusedChangeLeavesAnInProgressLoginAlone(t *testing.T) { ConfigPath: filepath.Join(profilemanager.DefaultConfigPathDir, target+".json"), ManagementURL: "https://api.netbird.io:443", ServerSSHAllowed: boolPtr(true), + Owner: testProfileOwner(), }) require.NoError(t, err) diff --git a/client/server/login_overrides_test.go b/client/server/login_overrides_test.go index 5a2298764..59a15f5e4 100644 --- a/client/server/login_overrides_test.go +++ b/client/server/login_overrides_test.go @@ -80,7 +80,8 @@ func TestPersistLoginOverrides(t *testing.T) { require.NoError(t, err, "seed config") activeProf := &profilemanager.ActiveProfileState{ID: "default"} - err = persistLoginOverrides(activeProf, tt.newMgmtURL, tt.newPSK) + srv := &Server{profileManager: profilemanager.NewServiceManager("")} + err = srv.persistLoginOverrides(activeProf, tt.newMgmtURL, tt.newPSK) require.NoError(t, err, "persistLoginOverrides") cfg, err := profilemanager.ReadConfig(profilemanager.DefaultConfigPath) diff --git a/client/server/logout_gate_test.go b/client/server/logout_gate_test.go index 2d84d1b6a..4c9da943e 100644 --- a/client/server/logout_gate_test.go +++ b/client/server/logout_gate_test.go @@ -2,6 +2,7 @@ package server import ( "context" + "os" "path/filepath" "testing" "time" @@ -69,6 +70,7 @@ func TestLogout_OtherProfileStaysGatedWhenProfilesDisabled(t *testing.T) { _, err := profilemanager.UpdateOrCreateConfig(profilemanager.ConfigInput{ ConfigPath: filepath.Join(profilemanager.DefaultConfigPathDir, other+".json"), ManagementURL: unreachableManagementURL, + Owner: testProfileOwner(), }) require.NoError(t, err) @@ -86,28 +88,18 @@ func TestLogout_OtherProfileStaysGatedWhenProfilesDisabled(t *testing.T) { // A legacy profile ID is a display name, so two users can hold the same ID in // their own profile directories. Matching on the ID alone would let one user's -// logout pass the gate against the other user's active profile, so the username -// is part of the comparison. +// logout pass the gate against the other user's active profile, so the config +// file, not the ID, decides which profile is the active one. func TestLogout_ForeignUserProfileStaysGatedWhenProfilesDisabled(t *testing.T) { s, _, _, username, _ := setupServerWithProfile(t) s.rootCtx = internal.CtxInitState(context.Background()) - // A legacy-style profile whose ID is its filename stem, and an active state - // claiming that same ID for a different user. shared := "shared-legacy-name" - _, err := profilemanager.UpdateOrCreateConfig(profilemanager.ConfigInput{ - ConfigPath: filepath.Join(profilemanager.DefaultConfigPathDir, shared+".json"), - ManagementURL: unreachableManagementURL, - }) - require.NoError(t, err) - require.NoError(t, s.profileManager.SetActiveProfileState(&profilemanager.ActiveProfileState{ - ID: profilemanager.ID(shared), - Username: "someone-else", - })) + plantNamesakeProfiles(t, s, shared) s.profilesDisabled = true - _, err = s.Logout(userCtx(), &proto.LogoutRequest{ + _, err := s.Logout(userCtx(), &proto.LogoutRequest{ ProfileName: &shared, Username: &username, }) @@ -117,6 +109,35 @@ func TestLogout_ForeignUserProfileStaysGatedWhenProfilesDisabled(t *testing.T) { "another user's profile must not pass the gate on an ID match alone: %v", err) } +// plantNamesakeProfiles creates two profiles that share one legacy ID: the +// caller's own, and another user's in that user's legacy profile directory, +// which is the one made active. Only the caller's copy carries an owner, so +// that is the one a handle resolves to, while the active profile stays the +// other file. +func plantNamesakeProfiles(t *testing.T, s *Server, id string) { + t.Helper() + + foreignDir := filepath.Join(profilemanager.DefaultConfigPathDir, "someone-else") + require.NoError(t, os.MkdirAll(foreignDir, 0700)) + _, err := profilemanager.UpdateOrCreateConfig(profilemanager.ConfigInput{ + ConfigPath: filepath.Join(foreignDir, id+".json"), + ManagementURL: unreachableManagementURL, + }) + require.NoError(t, err) + + _, err = profilemanager.UpdateOrCreateConfig(profilemanager.ConfigInput{ + ConfigPath: filepath.Join(profilemanager.DefaultConfigPathDir, id+".json"), + ManagementURL: unreachableManagementURL, + Owner: testProfileOwner(), + }) + require.NoError(t, err) + + require.NoError(t, s.profileManager.SetActiveProfileState(&profilemanager.ActiveProfileState{ + ID: profilemanager.ID(id), + Username: "someone-else", + })) +} + // Deregistering a namesake profile must not go out with the running config. // logoutFromProfile reuses the connected client's config when the target is the // active profile, and on an ID-only match a shared legacy ID made it reuse it @@ -135,15 +156,7 @@ func TestLogout_ForeignUserProfileDoesNotUseTheRunningConfig(t *testing.T) { s.connectClient = newDummyConnectClient(context.Background()) shared := "shared-legacy-name" - _, err = profilemanager.UpdateOrCreateConfig(profilemanager.ConfigInput{ - ConfigPath: filepath.Join(profilemanager.DefaultConfigPathDir, shared+".json"), - ManagementURL: unreachableManagementURL, - }) - require.NoError(t, err) - require.NoError(t, s.profileManager.SetActiveProfileState(&profilemanager.ActiveProfileState{ - ID: profilemanager.ID(shared), - Username: "someone-else", - })) + plantNamesakeProfiles(t, s, shared) // Bounded so the deregistration the fixed path attempts fails on the dial // rather than sitting in gRPC backoff for the whole test timeout. @@ -165,18 +178,21 @@ func TestLogout_ForeignUserProfileDoesNotUseTheRunningConfig(t *testing.T) { // guardedConfigMu, which the logout path does not hold, so a login that landed // meanwhile must keep its connection. func TestCleanupAfterProfileLogout_FollowsTheCurrentActiveProfile(t *testing.T) { - s, _, activeProfile, username, _ := setupServerWithProfile(t) + s, _, activeProfile, _, cfgPath := setupServerWithProfile(t) s.rootCtx = internal.CtxInitState(context.Background()) state := internal.CtxGetState(s.rootCtx) - s.cleanupAfterProfileLogout("some-other-profile", username) + s.cleanupAfterProfileLogout(&profilemanager.Profile{ID: "some-other-profile"}) status, err := state.Status() require.NoError(t, err) require.NotEqual(t, internal.StatusNeedsLogin, status, "logging out of a profile that is not active must not ask for a new login") - s.cleanupAfterProfileLogout(profilemanager.ID(activeProfile), username) + s.cleanupAfterProfileLogout(&profilemanager.Profile{ + ID: profilemanager.ID(activeProfile), + Path: cfgPath, + }) status, err = state.Status() require.NoError(t, err) require.Equal(t, internal.StatusNeedsLogin, status, diff --git a/client/server/server.go b/client/server/server.go index cc6547a61..1099ab4cf 100644 --- a/client/server/server.go +++ b/client/server/server.go @@ -7,6 +7,7 @@ import ( "fmt" "os" "os/exec" + "path/filepath" "runtime" "strconv" "sync" @@ -287,12 +288,30 @@ func (s *Server) Start() error { return err } + // A half-migrated machine still runs, it just keeps resolving profiles the + // old way, so a failure here is logged and retried on the next start rather + // than kept from starting at all. + if err := s.profileManager.MigrateLegacyProfiles(); err != nil { + log.Errorf("profile migration did not finish, retrying on next start: %v", err) + } + activeProf, err := s.profileManager.GetActiveProfileState() if err != nil { return fmt.Errorf("failed to get active profile state: %w", err) } config, existingConfig, err := s.getConfig(activeProf) + if errors.Is(err, profilemanager.ErrAmbiguousActiveProfile) { + // Running one of the namesakes anyway could connect the machine as + // another users profile, so the daemon comes up on the default + // profile instead of refusing to start. + log.Errorf("starting on the default profile, the active one could not be resolved: %v", err) + if err := s.profileManager.SetActiveProfileStateToDefault(); err != nil { + return fmt.Errorf("set active profile to default: %w", err) + } + activeProf = &profilemanager.ActiveProfileState{ID: profilemanager.DefaultProfileName} + config, existingConfig, err = s.getConfig(activeProf) + } if err != nil { log.Errorf("failed to get active profile config: %v", err) @@ -513,7 +532,12 @@ func (s *Server) SetConfig(callerCtx context.Context, msg *proto.SetConfigReques return nil, err } - stored, err := s.storedProfileConfig(msg.ProfileName, msg.Username) + callerID, err := callerIdentity(callerCtx) + if err != nil { + return nil, err + } + + stored, err := s.storedProfileConfig(msg.ProfileName, callerID) if err != nil { return nil, err } @@ -521,7 +545,7 @@ func (s *Server) SetConfig(callerCtx context.Context, msg *proto.SetConfigReques return nil, err } - config, err := s.setConfigInputFromRequest(msg) + config, err := s.setConfigInputFromRequest(msg, callerID) if err != nil { return nil, err } @@ -533,7 +557,7 @@ func (s *Server) SetConfig(callerCtx context.Context, msg *proto.SetConfigReques } if activeProf, err := s.profileManager.GetActiveProfileState(); err == nil { - if activePath, err := activeProf.FilePath(); err == nil && activePath == config.ConfigPath { + if activePath, err := s.profileManager.ActiveProfilePath(activeProf); err == nil && activePath == config.ConfigPath { s.localMetrics.Reconcile(updatedConf.LocalMetricsEnabled, updatedConf.LocalMetricsAddress) } } @@ -551,10 +575,10 @@ func (s *Server) SetConfig(callerCtx context.Context, msg *proto.SetConfigReques // field is its own optional case. Returns the resolved ConfigInput // and a non-nil error only when the active profile file path cannot // be determined. -func (s *Server) setConfigInputFromRequest(msg *proto.SetConfigRequest) (profilemanager.ConfigInput, error) { +func (s *Server) setConfigInputFromRequest(msg *proto.SetConfigRequest, callerID ipcauth.Identity) (profilemanager.ConfigInput, error) { var config profilemanager.ConfigInput - resolved, err := s.resolveProfileHandle(msg.ProfileName, msg.Username) + resolved, err := s.resolveProfileHandle(msg.ProfileName, callerID) if err != nil { log.Errorf("failed to resolve profile %q: %v", msg.ProfileName, err) return config, err @@ -657,6 +681,11 @@ func (s *Server) Login(callerCtx context.Context, msg *proto.LoginRequest) (*pro } } + callerID, err := callerIdentity(callerCtx) + if err != nil { + return nil, err + } + activeProf, err := s.profileManager.GetActiveProfileState() if err != nil { log.Errorf("failed to get active profile state: %v", err) @@ -668,7 +697,7 @@ func (s *Server) Login(callerCtx context.Context, msg *proto.LoginRequest) (*pro // refused login neither switches the profile nor cancels a login already in // progress, and it reads the profile the request targets, which is the one the // switch below would activate. - stored, err := s.storedLoginConfig(activeProf, msg) + stored, err := s.storedLoginConfig(activeProf, msg, callerID) if err != nil { return nil, err } @@ -701,7 +730,7 @@ func (s *Server) Login(callerCtx context.Context, msg *proto.LoginRequest) (*pro return nil, err } - log.Infof("active profile: %s for %s", activeProf.ID, activeProf.Username) + log.Infof("active profile: %s", activeProf.ID) s.mutex.Lock() @@ -1062,6 +1091,12 @@ func (s *Server) Up(callerCtx context.Context, msg *proto.UpRequest) (*proto.UpR return nil, fmt.Errorf("config is not defined, please call login command first") } + callerID, err := callerIdentity(callerCtx) + if err != nil { + s.mutex.Unlock() + return nil, err + } + activeProf, err := s.profileManager.GetActiveProfileState() if err != nil { s.mutex.Unlock() @@ -1070,7 +1105,7 @@ func (s *Server) Up(callerCtx context.Context, msg *proto.UpRequest) (*proto.UpR } if msg != nil && msg.ProfileName != nil { - if _, err := s.switchProfileIfNeeded(*msg.ProfileName, msg.Username, activeProf); err != nil { + if _, err := s.switchProfileIfNeeded(*msg.ProfileName, callerID, activeProf); err != nil { s.mutex.Unlock() log.Errorf("failed to switch profile: %v", err) return nil, err @@ -1084,7 +1119,7 @@ func (s *Server) Up(callerCtx context.Context, msg *proto.UpRequest) (*proto.UpR return nil, fmt.Errorf("failed to get active profile state: %w", err) } - log.Infof("active profile: %s for %s", activeProf.ID, activeProf.Username) + log.Infof("active profile: %s", activeProf.ID) config, _, err := s.getConfig(activeProf) if err != nil { @@ -1136,8 +1171,8 @@ func (s *Server) waitForUp(callerCtx context.Context) (*proto.UpResponse, error) // targets, so a privileged-change decision can be made against the values the // profile currently holds. A profile that has no config file yet yields nil, // which every caller must read as "nothing enabled yet". -func (s *Server) storedProfileConfig(handle, username string) (*profilemanager.Config, error) { - resolved, err := s.resolveProfileHandle(handle, username) +func (s *Server) storedProfileConfig(handle string, callerID ipcauth.Identity) (*profilemanager.Config, error) { + resolved, err := s.resolveProfileHandle(handle, callerID) if err != nil { return nil, err } @@ -1153,23 +1188,18 @@ func (s *Server) storedProfileConfig(handle, username string) (*profilemanager.C // storedLoginConfig loads the on-disk config of the profile a login request // targets: the one it names, or the active one when it names none. Used to decide // a privileged change before the request is allowed to switch profiles. -func (s *Server) storedLoginConfig(activeProf *profilemanager.ActiveProfileState, msg *proto.LoginRequest) (*profilemanager.Config, error) { +func (s *Server) storedLoginConfig(activeProf *profilemanager.ActiveProfileState, msg *proto.LoginRequest, callerID ipcauth.Identity) (*profilemanager.Config, error) { if msg.ProfileName == nil { - cfgPath, err := activeProf.FilePath() + cfgPath, err := s.profileManager.ActiveProfilePath(activeProf) if err != nil { return nil, fmt.Errorf("active profile file path: %w", err) } return s.storedConfigAtPath(cfgPath) } - // Mirrors switchProfileIfNeeded: the default profile resolves without a - // username, so this reads the same profile the switch would activate. - handle := *msg.ProfileName - username := "" - if handle != profilemanager.DefaultProfileName { - username = msg.GetUsername() - } - return s.storedProfileConfig(handle, username) + // Mirrors switchProfileIfNeeded, so this reads the very profile the switch + // would activate. + return s.storedProfileConfig(*msg.ProfileName, callerID) } // storedConfigAtPath reads a profile config file, yielding nil when it does not @@ -1189,11 +1219,26 @@ func (s *Server) storedConfigAtPath(path string) (*profilemanager.Config, error) return cfg, nil } +// callerIdentity returns the kernel-authenticated identity of the RPC caller. +// +// Every profile-addressing RPC scopes itself with this rather than with the +// username its request carries: that field is whatever the client chose to +// send, so scoping by it lets any local caller address another user's profile. +// The username fields on the wire are kept for compatibility and ignored. +func callerIdentity(ctx context.Context) (ipcauth.Identity, error) { + id, ok := ipcauth.CallerIdentity(ctx) + if !ok { + return ipcauth.Identity{}, gstatus.Error(codes.Unauthenticated, "caller identity could not be verified on the daemon control channel") + } + return id, nil +} + // resolveProfileHandle resolves a wire-level profile handle (display -// name, ID, or unique ID prefix) to a concrete profile. Returns gRPC -// status errors so handlers can return them directly. -func (s *Server) resolveProfileHandle(handle, username string) (*profilemanager.Profile, error) { - p, err := s.profileManager.ResolveProfile(handle, username) +// name, ID, or unique ID prefix) to a concrete profile owned by, or open to, +// the calling identity. Returns gRPC status errors so handlers can return them +// directly. +func (s *Server) resolveProfileHandle(handle string, callerID ipcauth.Identity) (*profilemanager.Profile, error) { + p, err := s.profileManager.ResolveProfile(handle, callerID) if err == nil { return p, nil } @@ -1210,36 +1255,28 @@ func (s *Server) resolveProfileHandle(handle, username string) (*profilemanager. // switchProfileIfNeeded resolves the user-supplied handle, updates the // active profile state if it differs from the current one, and returns // the resolved profile so callers can include its ID in RPC responses. -func (s *Server) switchProfileIfNeeded(handle string, userName *string, activeProf *profilemanager.ActiveProfileState) (*profilemanager.Profile, error) { - if handle != profilemanager.DefaultProfileName && (userName == nil || *userName == "") { - log.Errorf("profile name is set to %s, but username is not provided", handle) - return nil, fmt.Errorf("profile name is set to %s, but username is not provided", handle) - } - - var username string - if handle != profilemanager.DefaultProfileName { - username = *userName - } - - resolved, err := s.resolveProfileHandle(handle, username) +func (s *Server) switchProfileIfNeeded(handle string, callerID ipcauth.Identity, activeProf *profilemanager.ActiveProfileState) (*profilemanager.Profile, error) { + resolved, err := s.resolveProfileHandle(handle, callerID) if err != nil { return nil, err } - if resolved.ID != activeProf.ID || username != activeProf.Username { - if s.checkProfilesDisabled() { - log.Errorf("profiles are disabled, you cannot use this feature without profiles enabled") - return nil, gstatus.Errorf(codes.Unavailable, errProfilesDisabled) - } + if s.isActiveProfile(activeProf, resolved) { + return resolved, nil + } - log.Infof("switching to profile %s (%s) for user %s", resolved.Name, resolved.ID, username) - if err := s.profileManager.SetActiveProfileState(&profilemanager.ActiveProfileState{ - ID: resolved.ID, - Username: username, - }); err != nil { - log.Errorf("failed to set active profile state: %v", err) - return nil, fmt.Errorf("failed to set active profile state: %w", err) - } + if s.checkProfilesDisabled() { + log.Errorf("profiles are disabled, you cannot use this feature without profiles enabled") + return nil, gstatus.Errorf(codes.Unavailable, errProfilesDisabled) + } + + log.Infof("switching to profile %s (%s) for %s", resolved.Name, resolved.ID, callerID) + if err := s.profileManager.SetActiveProfileState(&profilemanager.ActiveProfileState{ + ID: resolved.ID, + Username: legacyDirHint(resolved), + }); err != nil { + log.Errorf("failed to set active profile state: %v", err) + return nil, fmt.Errorf("failed to set active profile state: %w", err) } return resolved, nil @@ -1250,6 +1287,11 @@ func (s *Server) SwitchProfile(callerCtx context.Context, msg *proto.SwitchProfi s.mutex.Lock() defer s.mutex.Unlock() + callerID, err := callerIdentity(callerCtx) + if err != nil { + return nil, err + } + activeProf, err := s.profileManager.GetActiveProfileState() if err != nil { log.Errorf("failed to get active profile state: %v", err) @@ -1257,7 +1299,7 @@ func (s *Server) SwitchProfile(callerCtx context.Context, msg *proto.SwitchProfi } if msg != nil && msg.ProfileName != nil { - if _, err := s.switchProfileIfNeeded(*msg.ProfileName, msg.Username, activeProf); err != nil { + if _, err := s.switchProfileIfNeeded(*msg.ProfileName, callerID, activeProf); err != nil { log.Errorf("failed to switch profile: %v", err) return nil, err } @@ -1410,12 +1452,12 @@ func (s *Server) Logout(ctx context.Context, msg *proto.LogoutRequest) (*proto.L } func (s *Server) handleProfileLogout(ctx context.Context, msg *proto.LogoutRequest) (*proto.LogoutResponse, error) { - if msg.Username == nil || *msg.Username == "" { - return nil, gstatus.Errorf(codes.InvalidArgument, "username must be provided when profile name is specified") + callerID, err := callerIdentity(ctx) + if err != nil { + return nil, err } - username := *msg.Username - resolved, err := s.resolveProfileHandle(*msg.ProfileName, username) + resolved, err := s.resolveProfileHandle(*msg.ProfileName, callerID) if err != nil { return nil, err } @@ -1425,11 +1467,11 @@ func (s *Server) handleProfileLogout(ctx context.Context, msg *proto.LogoutReque return nil, gstatus.Errorf(codes.FailedPrecondition, "failed to get active profile state: %v", err) } - if err := s.validateProfileLogout(resolved.ID, isActiveProfile(activeProf, resolved.ID, username)); err != nil { + if err := s.validateProfileLogout(resolved.ID, s.isActiveProfile(activeProf, resolved)); err != nil { return nil, err } - if err := s.logoutFromProfile(ctx, resolved, username); err != nil { + if err := s.logoutFromProfile(ctx, resolved); err != nil { log.Errorf("failed to logout from profile %s: %v", resolved.ID, err) // A refused deregistration is already a status error carrying the reason // and the command to run; rewrapping it as Internal would flatten both @@ -1440,7 +1482,7 @@ func (s *Server) handleProfileLogout(ctx context.Context, msg *proto.LogoutReque return nil, gstatus.Errorf(codes.Internal, "logout: %v", err) } - s.cleanupAfterProfileLogout(resolved.ID, username) + s.cleanupAfterProfileLogout(resolved) return &proto.LogoutResponse{}, nil } @@ -1451,14 +1493,14 @@ func (s *Server) handleProfileLogout(ctx context.Context, msg *proto.LogoutReque // check: Login switches profiles under guardedConfigMu, which this path does not // hold, so a login that landed meanwhile must not have its fresh connection // dropped by a logout that targeted the profile it replaced. -func (s *Server) cleanupAfterProfileLogout(id profilemanager.ID, username string) { +func (s *Server) cleanupAfterProfileLogout(profile *profilemanager.Profile) { activeProf, err := s.profileManager.GetActiveProfileState() if err != nil { - log.Errorf("failed to get active profile state after logout from profile %s: %v", id, err) + log.Errorf("failed to get active profile state after logout from profile %s: %v", profile.ID, err) return } - if !isActiveProfile(activeProf, id, username) { + if !s.isActiveProfile(activeProf, profile) { return } @@ -1504,7 +1546,7 @@ func (s *Server) handleActiveProfileLogout(ctx context.Context) (*proto.LogoutRe // getConfig reads config file and returns Config and whether the config file already existed. Errors out if it does not exist func (s *Server) getConfig(activeProf *profilemanager.ActiveProfileState) (*profilemanager.Config, bool, error) { - cfgPath, err := activeProf.FilePath() + cfgPath, err := s.profileManager.ActiveProfilePath(activeProf) if err != nil { return nil, false, fmt.Errorf("failed to get active profile file path: %w", err) } @@ -1548,27 +1590,51 @@ func (s *Server) validateProfileLogout(id profilemanager.ID, isActive bool) erro return nil } -// isActiveProfile reports whether id is the profile the daemon runs for -// username. The username is part of the comparison because legacy profile IDs -// are display names, which two users can both hold; the default profile is -// shared by every user and carries no username. -func isActiveProfile(activeProf *profilemanager.ActiveProfileState, id profilemanager.ID, username string) bool { - if activeProf == nil || activeProf.ID != id { +// isActiveProfile reports whether profile is the one the daemon runs. +// +// The comparison ends on the config file rather than on the ID, because a +// legacy profile ID is a display name that two users can each hold: the path is +// what tells alice's `work` from bob's. It is not the caller's username, which +// says nothing about which profile the daemon activated. +func (s *Server) isActiveProfile(activeProf *profilemanager.ActiveProfileState, profile *profilemanager.Profile) bool { + if activeProf == nil || profile == nil || activeProf.ID != profile.ID { return false } + if profile.ID == profilemanager.DefaultProfileName { + return true + } - return id == profilemanager.DefaultProfileName || activeProf.Username == username + activePath, err := s.profileManager.ActiveProfilePath(activeProf) + if err != nil { + log.Warnf("cannot resolve the active profile's path, treating %s as not active: %v", profile.ID, err) + return false + } + return activePath == profile.Path +} + +// legacyDirHint records which per-username directory a pre-migration profile's +// file sits in, which is all the active-profile state still reads its username +// for. A profile in the shared directory needs no hint: its ID is unique. +func legacyDirHint(profile *profilemanager.Profile) string { + if profile.Path == "" || profile.ID == profilemanager.DefaultProfileName { + return "" + } + dir := filepath.Base(filepath.Dir(profile.Path)) + if dir == profilemanager.DefaultProfilePathDir { + return "" + } + return dir } // logoutFromProfile deregisters profile, reusing the running config when -// profile is the one the daemon is connected with. The username takes part in -// that decision for the same reason it does in the logout gate: a legacy -// profile ID is a display name two users can share, and sending the running -// config for a namesake would deregister the active peer instead of the +// profile is the one the daemon is connected with. That decision is made on the +// profile's file rather than on its ID, for the same reason the logout gate is: +// a legacy profile ID is a display name two users can share, and sending the +// running config for a namesake would deregister the active peer instead of the // requested one. -func (s *Server) logoutFromProfile(ctx context.Context, profile *profilemanager.Profile, username string) error { +func (s *Server) logoutFromProfile(ctx context.Context, profile *profilemanager.Profile) error { activeProf, err := s.profileManager.GetActiveProfileState() - if err == nil && isActiveProfile(activeProf, profile.ID, username) && s.connectClient != nil { + if err == nil && s.isActiveProfile(activeProf, profile) && s.connectClient != nil { return s.sendLogoutRequest(ctx) } @@ -2203,7 +2269,12 @@ func (s *Server) GetConfig(ctx context.Context, req *proto.GetConfigRequest) (*p return nil, ctx.Err() } - resolved, err := s.resolveProfileHandle(req.ProfileName, req.Username) + callerID, err := callerIdentity(ctx) + if err != nil { + return nil, err + } + + resolved, err := s.resolveProfileHandle(req.ProfileName, callerID) if err != nil { log.Errorf("failed to resolve profile %q: %v", req.ProfileName, err) return nil, err @@ -2316,15 +2387,16 @@ func (s *Server) AddProfile(ctx context.Context, msg *proto.AddProfileRequest) ( return nil, gstatus.Errorf(codes.Unavailable, errProfilesDisabled) } - if msg.ProfileName == "" || msg.Username == "" { - return nil, gstatus.Errorf(codes.InvalidArgument, "profile name and username must be provided") + if msg.ProfileName == "" { + return nil, gstatus.Errorf(codes.InvalidArgument, "profile name must be provided") } - callerId, ok := ipcauth.CallerIdentity(ctx) - if !ok { - return nil, fmt.Errorf("failed to get identity from context") + callerID, err := callerIdentity(ctx) + if err != nil { + return nil, err } - created, err := s.profileManager.AddProfile(msg.ProfileName, msg.Username, &callerId) + + created, err := s.profileManager.AddProfile(msg.ProfileName, &callerID) if err != nil { log.Errorf("failed to create profile: %v", err) return nil, fmt.Errorf("failed to create profile: %w", err) @@ -2343,16 +2415,21 @@ func (s *Server) RenameProfile(ctx context.Context, msg *proto.RenameProfileRequ return nil, gstatus.Errorf(codes.Unavailable, errProfilesDisabled) } - if msg.Handle == "" || msg.Username == "" || msg.NewProfileName == "" { - return nil, gstatus.Errorf(codes.InvalidArgument, "profile name, username and new profile name must be provided") + if msg.Handle == "" || msg.NewProfileName == "" { + return nil, gstatus.Errorf(codes.InvalidArgument, "profile name and new profile name must be provided") } - resolved, err := s.resolveProfileHandle(msg.Handle, msg.Username) + callerID, err := callerIdentity(ctx) if err != nil { return nil, err } - err = s.profileManager.RenameProfile(resolved.ID, msg.Username, msg.NewProfileName) + resolved, err := s.resolveProfileHandle(msg.Handle, callerID) + if err != nil { + return nil, err + } + + err = s.profileManager.RenameProfile(resolved.ID, callerID, msg.NewProfileName) if err != nil { log.Errorf("failed to rename profile: %v", err) return nil, fmt.Errorf("failed to rename profile: %w", err) @@ -2376,19 +2453,24 @@ func (s *Server) RemoveProfile(ctx context.Context, msg *proto.RemoveProfileRequ return nil, gstatus.Errorf(codes.InvalidArgument, "profile name must be provided") } - resolved, err := s.resolveProfileHandle(msg.ProfileName, msg.Username) + callerID, err := callerIdentity(ctx) if err != nil { return nil, err } - if err := s.logoutFromProfile(ctx, resolved, msg.Username); err != nil { + resolved, err := s.resolveProfileHandle(msg.ProfileName, callerID) + if err != nil { + return nil, err + } + + if err := s.logoutFromProfile(ctx, resolved); err != nil { // Deregistration is best-effort here: the local profile is removed // either way, so an unprivileged caller leaves the peer registered on // the management server rather than being blocked from removing it. log.Warnf("removing profile %s locally without deregistering it: %v", resolved.ID, err) } - if err := s.profileManager.RemoveProfile(resolved.ID, msg.Username); err != nil { + if err := s.profileManager.RemoveProfile(resolved.ID, callerID); err != nil { log.Errorf("failed to remove profile: %v", err) return nil, fmt.Errorf("failed to remove profile: %w", err) } @@ -2443,11 +2525,12 @@ func (s *Server) ListProfiles(ctx context.Context, msg *proto.ListProfilesReques s.mutex.Lock() defer s.mutex.Unlock() - if msg.Username == "" { - return nil, gstatus.Errorf(codes.InvalidArgument, "username must be provided") + callerID, err := callerIdentity(ctx) + if err != nil { + return nil, err } - profiles, err := s.profileManager.ListProfiles(msg.Username) + profiles, err := s.profileManager.ListProfiles(callerID) if err != nil { log.Errorf("failed to list profiles: %v", err) return nil, fmt.Errorf("failed to list profiles: %w", err) @@ -2480,10 +2563,15 @@ func (s *Server) GetActiveProfile(ctx context.Context, msg *proto.GetActiveProfi return nil, fmt.Errorf("failed to get active profile state: %w", err) } + userID, ok := ipcauth.CallerIdentity(ctx) + if !ok { + return nil, gstatus.Error(codes.Unauthenticated, "caller identity could not be resolved") + } + // Fallback to legacy name == ID displayName := activeProfile.ID.String() if activeProfile.ID != profilemanager.DefaultProfileName { - if profiles, lerr := s.profileManager.ListProfiles(activeProfile.Username); lerr == nil { + if profiles, lerr := s.profileManager.ListProfiles(userID); lerr == nil { for _, p := range profiles { if p.ID == activeProfile.ID { displayName = p.Name @@ -2701,10 +2789,15 @@ func (s *Server) authorizeAndPrepareLogin(callerCtx context.Context, msg *proto. afterLoginPreCheck() } + callerID, err := callerIdentity(callerCtx) + if err != nil { + return nil, nil, err + } + s.guardedConfigMu.Lock() defer s.guardedConfigMu.Unlock() - stored, err := s.storedLoginConfig(activeProf, msg) + stored, err := s.storedLoginConfig(activeProf, msg, callerID) if err != nil { return nil, nil, err } @@ -2728,7 +2821,7 @@ func (s *Server) authorizeAndPrepareLogin(callerCtx context.Context, msg *proto. } if msg.ProfileName != nil { - if _, err := s.switchProfileIfNeeded(*msg.ProfileName, msg.Username, activeProf); err != nil { + if _, err := s.switchProfileIfNeeded(*msg.ProfileName, callerID, activeProf); err != nil { return nil, nil, fmt.Errorf("switch profile: %w", err) } } @@ -2738,7 +2831,7 @@ func (s *Server) authorizeAndPrepareLogin(callerCtx context.Context, msg *proto. return nil, nil, fmt.Errorf("active profile state: %w", err) } - if err := persistLoginOverrides(activeProf, msg.ManagementUrl, msg.OptionalPreSharedKey); err != nil { + if err := s.persistLoginOverrides(activeProf, msg.ManagementUrl, msg.OptionalPreSharedKey); err != nil { return nil, nil, fmt.Errorf("persist login overrides: %w", err) } @@ -2769,12 +2862,85 @@ func (s *Server) SessionHolder() (ipcauth.Principal, bool) { return principal, true } +// OwnsProfile reports whether the profile the handle resolves to answers to +// this identity. +// +// This triggers stamping of legacy profiles, and reloads the active profile's +// config so the stamp is visible to SessionHolder. func (s *Server) OwnsProfile(id ipcauth.Identity, handle string) bool { - // TODO - return false + // Without the active profile there is nothing to fall back to and nothing + // to refresh, so the gate gets a no rather than a guess. + activeProfile, err := s.profileManager.GetActiveProfileState() + if err != nil { + log.Warnf("failed to get active profile: %v", err) + return false + } + if activeProfile == nil { + log.Warn("no active profile to authorize against") + return false + } + if handle == "" { + handle = activeProfile.ID.String() + } + + resolved, resolveErr := s.resolveProfileHandle(handle, id) + + if afterProfileResolve != nil { + afterProfileResolve() + } + + // Resolving stamps an owner on every legacy profile the caller can claim, + // not only the one the handle names, so the daemon's copy of the active + // profile's config goes stale whatever the handle was, and whether or not + // resolution succeeded. SessionHolder reads Owners off that copy, so + // refresh it before this answer reaches the gate. + s.reloadActiveConfig() + + if resolveErr != nil { + log.Errorf("failed to resolve profile %q: %v", handle, resolveErr) + return false + } + return resolved.AccessibleBy(id) } -func persistLoginOverrides(activeProf *profilemanager.ActiveProfileState, managementURL string, preSharedKey *string) error { +// afterProfileResolve is a seam for tests to run a concurrent profile switch +// between the resolution that stamps owners and the reload that publishes them. +var afterProfileResolve func() + +// reloadActiveConfig refreshes the daemon's copy of the active profile's config +// from disk, which is where SessionHolder reads the owner of a live session. +// +// The active profile is read here and the whole reload runs under s.mutex. +// SwitchProfile and Up change the active profile and install its config under +// that same lock. +func (s *Server) reloadActiveConfig() { + s.mutex.Lock() + defer s.mutex.Unlock() + + // The handlers that start a session read their own config off disk, no need + // for a reload. + if !s.clientRunning { + return + } + + activeProfile, err := s.profileManager.GetActiveProfileState() + if err != nil { + log.Errorf("failed to reload the active profile state: %v", err) + return + } + if activeProfile == nil { + return + } + + config, _, err := s.getConfig(activeProfile) + if err != nil { + log.Errorf("failed to reload active profile config: %v", err) + return + } + s.config = config +} + +func (s *Server) persistLoginOverrides(activeProf *profilemanager.ActiveProfileState, managementURL string, preSharedKey *string) error { if preSharedKey != nil && *preSharedKey == "" { preSharedKey = nil } @@ -2782,7 +2948,7 @@ func persistLoginOverrides(activeProf *profilemanager.ActiveProfileState, manage return nil } - cfgPath, err := activeProf.FilePath() + cfgPath, err := s.profileManager.ActiveProfilePath(activeProf) if err != nil { return fmt.Errorf("active profile file path: %w", err) } diff --git a/client/server/server_jwt_test.go b/client/server/server_jwt_test.go index 3fec5598a..1907ebf77 100644 --- a/client/server/server_jwt_test.go +++ b/client/server/server_jwt_test.go @@ -102,17 +102,20 @@ func TestSwitchProfile_ClearsJWTCache(t *testing.T) { // switchProfileIfNeeded rather than the no-op path a nil request takes. const target = "second" username := "tester" + owner := unprivilegedIdentity() _, err := profilemanager.UpdateOrCreateConfig(profilemanager.ConfigInput{ ConfigPath: filepath.Join(profilemanager.DefaultConfigPathDir, target+".json"), ManagementURL: "https://api.netbird.io:443", + Owner: &owner, }) require.NoError(t, err) - owner := unprivilegedIdentity() s.jwtCache.store("token", owner, testTTL, s.jwtCache.currentGeneration()) name := target - _, err = s.SwitchProfile(ctx, &proto.SwitchProfileRequest{ProfileName: &name, Username: &username}) + // The handler scopes the switch to the caller's identity, which a real + // caller gets from the daemon's transport credentials. + _, err = s.SwitchProfile(ctxWithIdentity(owner), &proto.SwitchProfileRequest{ProfileName: &name, Username: &username}) require.NoError(t, err) active, err := s.profileManager.GetActiveProfileState() diff --git a/client/server/server_ownsprofile_test.go b/client/server/server_ownsprofile_test.go new file mode 100644 index 000000000..e3f416a7e --- /dev/null +++ b/client/server/server_ownsprofile_test.go @@ -0,0 +1,135 @@ +package server + +import ( + "os" + "path/filepath" + "testing" + + "github.com/stretchr/testify/require" + + "github.com/netbirdio/netbird/client/internal/ipcauth" + "github.com/netbirdio/netbird/client/internal/profilemanager" +) + +// Resolving a handle claims every legacy profile the caller can take, the +// active one included, whatever profile the handle itself names. SessionHolder +// answers from the daemon's in-memory config, so OwnsProfile has to refresh it +// for any handle: a copy taken before the claim reports no owner at all, and a +// session with no owner is one every identified caller may take over. +func TestOwnsProfile_RefreshesActiveConfigForAnyHandle(t *testing.T) { + other := "second-profile" + + for _, tc := range []struct { + name string + handle string + }{ + {name: "no handle falls back to the active profile", handle: ""}, + {name: "the active profile by ID", handle: "test-profile-mdm"}, + {name: "another profile entirely", handle: other}, + } { + t.Run(tc.name, func(t *testing.T) { + s, _, activeProfile, _, _ := setupServerWithProfile(t) + require.Equal(t, "test-profile-mdm", activeProfile) + + owner := unprivilegedIdentity() + _, err := profilemanager.UpdateOrCreateConfig(profilemanager.ConfigInput{ + ConfigPath: filepath.Join(profilemanager.DefaultConfigPathDir, other+".json"), + ManagementURL: "https://api.netbird.io:443", + Owner: &owner, + }) + require.NoError(t, err) + + // The daemon's copy as it stood before the claim landed on disk. + s.config = &profilemanager.Config{} + s.clientRunning = true + _, running := s.SessionHolder() + require.False(t, running, "fixture is wrong: the stale copy already names an owner") + + require.True(t, s.OwnsProfile(owner, tc.handle), "the caller owns every profile in this fixture") + + holder, running := s.SessionHolder() + require.True(t, running, "the claimed owner never reached the daemon's config, so the live session is unowned") + require.True(t, holder.Matches(owner), "the session is held by %v, not by the profile's owner", holder) + }) + } +} + +// A caller whose profile the daemon cannot read is not the owner of anything. +// The answer has to be no rather than a panic in the authorization path. +func TestOwnsProfile_UnreadableActiveProfileStateDenies(t *testing.T) { + s, _, _, _, _ := setupServerWithProfile(t) + + require.NoError(t, os.WriteFile(profilemanager.ActiveProfileStatePath, []byte("{"), 0600)) + + require.False(t, s.OwnsProfile(unprivilegedIdentity(), "")) +} + +// A config the daemon cannot re-read leaves the one it already has in place. +// Dropping a nil in its stead would take down every reader of it, SessionHolder +// among them, which is the authorization path itself. +func TestOwnsProfile_UnreadableConfigKeepsTheOneInPlace(t *testing.T) { + s, _, _, _, _ := setupServerWithProfile(t) + + // An ID no path can be built for, which is what a hand-edited or + // downgrade-written state file can leave behind. + require.NoError(t, os.WriteFile(profilemanager.ActiveProfileStatePath, []byte(`{"name":"../escape"}`), 0600)) + + kept := &profilemanager.Config{Owners: []string{ipcauth.OwnerPrincipalForIdentity(unprivilegedIdentity())}} + s.config = kept + s.clientRunning = true + + require.False(t, s.OwnsProfile(unprivilegedIdentity(), "")) + require.Same(t, kept, s.config, "a failed reload replaced the daemon's config") + + holder, running := s.SessionHolder() + require.True(t, running) + require.True(t, holder.Matches(unprivilegedIdentity())) +} + +// A profile switch can land while the gate is still resolving: the resolution +// reads every profile off disk, and SwitchProfile only needs the daemon lock, +// which the gate does not hold. The config the reload publishes has to be the +// one the daemon is now on, not the one the check started out reading. +func TestOwnsProfile_ReloadFollowsASwitchThatLandsMidCheck(t *testing.T) { + s, _, activeProfile, _, _ := setupServerWithProfile(t) + owner := unprivilegedIdentity() + + switchedTo := "switched-to" + switchedToURL := "https://switched-to.example:443" + _, err := profilemanager.UpdateOrCreateConfig(profilemanager.ConfigInput{ + ConfigPath: filepath.Join(profilemanager.DefaultConfigPathDir, switchedTo+".json"), + ManagementURL: switchedToURL, + Owner: &owner, + }) + require.NoError(t, err) + + s.config = &profilemanager.Config{} + s.clientRunning = true + + // Stand in for a SwitchProfile that lands between the resolution and the + // reload, which is the whole window the profile files are being read in. + afterProfileResolve = func() { + require.NoError(t, s.profileManager.SetActiveProfileState(&profilemanager.ActiveProfileState{ + ID: profilemanager.ID(switchedTo), + })) + } + t.Cleanup(func() { afterProfileResolve = nil }) + + require.True(t, s.OwnsProfile(owner, activeProfile)) + + require.NotNil(t, s.config.ManagementURL) + require.Equal(t, switchedToURL, s.config.ManagementURL.String(), + "the reload published the config of a profile the daemon had already left") +} + +// The handlers that start a session read their config off disk themselves. +func TestOwnsProfile_IdleDaemonKeepsItsConfig(t *testing.T) { + s, _, activeProfile, _, _ := setupServerWithProfile(t) + + untouched := &profilemanager.Config{} + s.config = untouched + s.clientRunning = false + + require.True(t, s.OwnsProfile(unprivilegedIdentity(), activeProfile)) + require.Same(t, untouched, s.config) +} diff --git a/client/server/setconfig_mdm_test.go b/client/server/setconfig_mdm_test.go index a392af6d3..d3689c1d0 100644 --- a/client/server/setconfig_mdm_test.go +++ b/client/server/setconfig_mdm_test.go @@ -11,6 +11,7 @@ import ( "google.golang.org/grpc/codes" gstatus "google.golang.org/grpc/status" + "github.com/netbirdio/netbird/client/internal/ipcauth" "github.com/netbirdio/netbird/client/internal/profilemanager" "github.com/netbirdio/netbird/client/mdm" "github.com/netbirdio/netbird/client/proto" @@ -83,6 +84,7 @@ func setupServerWithProfile(t *testing.T) (s *Server, ctx context.Context, profN _, err = profilemanager.UpdateOrCreateConfig(profilemanager.ConfigInput{ ConfigPath: cfgPath, ManagementURL: "https://api.netbird.io:443", + Owner: testProfileOwner(), }) require.NoError(t, err) @@ -101,6 +103,13 @@ func setupServerWithProfile(t *testing.T) (s *Server, ctx context.Context, profN return s, ctx, profName, currUser.Username, cfgPath } +// testProfileOwner is the identity userCtx carries, which is who a fixture +// profile belongs to. +func testProfileOwner() *ipcauth.Identity { + id := unprivilegedIdentity() + return &id +} + // extractViolation pulls the MDMManagedFieldsViolation detail from a // FailedPrecondition error. Fails the test if absent or malformed. func extractViolation(t *testing.T, err error) *proto.MDMManagedFieldsViolation {