diff --git a/client/server/server.go b/client/server/server.go index c89d1f22d..09d533305 100644 --- a/client/server/server.go +++ b/client/server/server.go @@ -2837,36 +2837,81 @@ func (s *Server) SessionHolder() (ipcauth.Principal, bool) { // OwnsProfile reports whether the profile the handle resolves to answers to // this identity. // -// This triggers stamping of legacy profiles, and reloads the current config -// if the handle is the active profile. +// 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 { - var activeProfile *profilemanager.ActiveProfileState - if handle == "" { - activeState, err := s.profileManager.GetActiveProfileState() - if err != nil { - log.Warnf("failed to get active profile: %v", err) - } - activeProfile = activeState - handle = activeProfile.ID.String() - } - resolved, err := s.resolveProfileHandle(handle, id) + // 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.Errorf("failed to resolve profile %q: %v", handle, err) + log.Warnf("failed to get active profile: %v", err) return false } - // resolveProfileHandle might stamp legacy profile owners - if activeProfile != nil { - config, _, err := s.getConfig(activeProfile) - if err != nil { - log.Errorf("failed to get active profile config: %v", err) - } - s.mutex.Lock() - s.config = config - s.mutex.Unlock() + 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) } +// 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 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) +}