From 905bf68040287a21afa6045653ac130636dfe4eb Mon Sep 17 00:00:00 2001 From: "Theodor S. Midtlien" Date: Fri, 11 Sep 2026 11:56:05 +0200 Subject: [PATCH] Add one-shot migration --- client/internal/profilemanager/migration.go | 191 ++++++++++++++++++ .../internal/profilemanager/migration_test.go | 121 +++++++++++ client/internal/profilemanager/prefs.go | 5 +- client/internal/profilemanager/service.go | 77 ++++--- .../internal/profilemanager/service_test.go | 30 +++ client/server/server.go | 7 + 6 files changed, 399 insertions(+), 32 deletions(-) create mode 100644 client/internal/profilemanager/migration.go create mode 100644 client/internal/profilemanager/migration_test.go diff --git a/client/internal/profilemanager/migration.go b/client/internal/profilemanager/migration.go new file mode 100644 index 000000000..e0057cd62 --- /dev/null +++ b/client/internal/profilemanager/migration.go @@ -0,0 +1,191 @@ +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) + } + + if err := s.rekeyDuplicateIDs(profiles, active); err != nil { + return err + } + + if err := s.stampActiveUserDir(profiles, active); err != nil { + return err + } + + 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. +func (s *ServiceManager) rekeyDuplicateIDs(profiles []Profile, active *ActiveProfileState) 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) + } + + 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) + 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 nil +} + +// rekeyProfile renames a profile and its sidecars to a fresh ID. +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) + target := filepath.Join(dir, fresh.String()+".json") + if err := os.Rename(p.Path, target); err != nil { + return fmt.Errorf("rekey %s: %w", p.ID, err) + } + + for _, suffix := range []string{stateFileSuffix, prefsFileSuffix} { + src := filepath.Join(dir, p.ID.String()+suffix) + if _, err := os.Stat(src); err != nil { + continue + } + if err := os.Rename(src, filepath.Join(dir, fresh.String()+suffix)); err != nil { + log.Warnf("could not rename %s alongside its profile: %v", src, 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 +} + +// 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 { + return fmt.Errorf("stamp %s: %w", p.ID, err) + } + 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..207fc8027 --- /dev/null +++ b/client/internal/profilemanager/migration_test.go @@ -0,0 +1,121 @@ +package profilemanager + +import ( + "os" + "os/user" + "path/filepath" + "runtime" + "strconv" + "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") + }) +} diff --git a/client/internal/profilemanager/prefs.go b/client/internal/profilemanager/prefs.go index e00ea51e0..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 diff --git a/client/internal/profilemanager/service.go b/client/internal/profilemanager/service.go index b2337bced..46e496fb2 100644 --- a/client/internal/profilemanager/service.go +++ b/client/internal/profilemanager/service.go @@ -179,20 +179,26 @@ func (s *ServiceManager) ActiveProfilePath(a *ActiveProfileState) (string, error } } - switch len(matches) { - case 0: + 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() - case 1: + } + + if len(matches) == 1 { return matches[0].Path, nil } - // Two directories hold the same legacy ID, so the recorded hint says which - // one the daemon activated. State written before the hint was a directory - // recorded the raw account name, which the legacy layout sanitized on its - // way to becoming a directory, so try it both ways. + // 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 @@ -200,8 +206,8 @@ func (s *ServiceManager) ActiveProfilePath(a *ActiveProfileState) (string, error } } - log.Warnf("active profile %q exists in %d directories and none of them is %q, using %s", - a.ID, len(matches), a.Username, matches[0].Path) + log.Warnf("%d profiles share the ID %q and none of them sits in %q, using %s", + len(matches), a.ID, a.Username, matches[0].Path) return matches[0].Path, nil } @@ -346,10 +352,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) } @@ -440,20 +442,7 @@ func (s *ServiceManager) RenameProfile(id ID, userID ipcauth.Identity, newName s 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 @@ -898,9 +887,35 @@ func readProfileOwners(path string) ([]ipcauth.Principal, error) { return []ipcauth.Principal{principal}, nil } -// StampOwner records owner as a profile's owner, replacing whoever is recorded -// now. +// StampOwner records a caller as a profile's owner, replacing whoever is +// recorded now. func StampOwner(path string, owner ipcauth.Identity) error { + 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 updateProfileConfig(path, func(cfg *Config) { + cfg.Owners = []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 updateProfileConfig(path, func(cfg *Config) { + cfg.Name = name + }) +} + +// updateProfileConfig reads a profile, applies mutate and writes it back. +// +// The whole config makes the round trip, which is what every writer here does, +// so a field this version does not model is dropped. That only happens after a +// downgrade, and a downgrade already drops the owners it cannot read. +func updateProfileConfig(path string, mutate func(*Config)) error { data, err := os.ReadFile(path) if err != nil { return err @@ -910,10 +925,10 @@ func StampOwner(path string, owner ipcauth.Identity) error { if err := json.Unmarshal(data, &cfg); err != nil { return err } - cfg.Owners = []string{ipcauth.OwnerPrincipalForIdentity(owner)} + mutate(&cfg) if err := util.WriteJson(context.Background(), path, cfg); err != nil { - return fmt.Errorf("write profile owner: %w", err) + return fmt.Errorf("write profile %s: %w", path, err) } return nil } diff --git a/client/internal/profilemanager/service_test.go b/client/internal/profilemanager/service_test.go index d5b8f5fd5..dda9a5363 100644 --- a/client/internal/profilemanager/service_test.go +++ b/client/internal/profilemanager/service_test.go @@ -477,6 +477,36 @@ func TestClaimLegacyProfile_SkipsOneAlreadyOwnedInTheSameDirectory(t *testing.T) }) } +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 TestListProfiles_PrivilegedCallerDoesNotClaim(t *testing.T) { withLegacyLayout(t, func(sm *ServiceManager, configDir string) { path := writeLegacyProfile(t, configDir, "root", "work", nil) diff --git a/client/server/server.go b/client/server/server.go index 1793fd9e4..295d3231a 100644 --- a/client/server/server.go +++ b/client/server/server.go @@ -288,6 +288,13 @@ 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)