From b1036c8bbcd9a0d941ea4e7c5a88e2c92af8780e Mon Sep 17 00:00:00 2001 From: riccardom Date: Wed, 23 Sep 2026 13:41:48 +0200 Subject: [PATCH] [client] Provision the peer identity under the config lock (review item) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Login took the authoritative update-settings and privilege decisions under guardedConfigMu, then released it and called getConfig, which mints the peer's identity and writes the config out. Between that read and that write, a SetConfig holding the same lock could land a change and answer its caller — and then be overwritten by the config the login had already read. The window is narrow: getConfig only writes when the profile has no identity or no file, so in practice a first login racing a settings change on the same profile. It is also narrower than before this branch, where the write happened inside the reader on every read that filled in a default. Provisioning now runs where the decision it belongs to runs: at the end of authorizeAndPrepareLogin, with the lock already held, next to persistLoginOverrides, which writes there too. No lock is taken that was not held before, so the documented guardedConfigMu-then-mutex order is untouched. getConfig keeps its behaviour by calling the same extracted helper; on the login path it now finds the identity already there and writes nothing. The other callers are unchanged, and still provision outside any lock — a concurrent SetConfig is not part of their flow. Reported by cubic on the PR. --- client/server/provision_identity_test.go | 59 ++++++++++++++++++++++++ client/server/server.go | 38 +++++++++++---- 2 files changed, 89 insertions(+), 8 deletions(-) create mode 100644 client/server/provision_identity_test.go diff --git a/client/server/provision_identity_test.go b/client/server/provision_identity_test.go new file mode 100644 index 000000000..c944cdb1a --- /dev/null +++ b/client/server/provision_identity_test.go @@ -0,0 +1,59 @@ +package server + +import ( + "os" + "path/filepath" + "testing" + + "github.com/stretchr/testify/require" + + "github.com/netbirdio/netbird/client/internal/profilemanager" +) + +// The daemon provisions the peer's identity and persists it, because a key that +// stayed in memory would come back different on the next start and register a +// second peer. Provisioning is idempotent: a profile that already has an +// identity keeps the one on disk. +func TestProvisionProfileIdentity(t *testing.T) { + origDir := profilemanager.DefaultConfigPathDir + origPath := profilemanager.DefaultConfigPath + t.Cleanup(func() { + profilemanager.DefaultConfigPathDir = origDir + profilemanager.DefaultConfigPath = origPath + }) + + dir := t.TempDir() + profilemanager.DefaultConfigPathDir = dir + profilemanager.DefaultConfigPath = filepath.Join(dir, "default.json") + + activeProf := &profilemanager.ActiveProfileState{ID: "default"} + + t.Run("a profile with no file is provisioned and written", func(t *testing.T) { + _, err := os.Stat(profilemanager.DefaultConfigPath) + require.True(t, os.IsNotExist(err), "the fixture starts without a config file") + + config, existed, err := provisionProfileIdentity(activeProf) + require.NoError(t, err) + require.False(t, existed, "the file was reported as pre-existing") + require.NotEmpty(t, config.PrivateKey) + + stored, err := profilemanager.GetExistingConfig(profilemanager.DefaultConfigPath) + require.NoError(t, err, "provisioning did not write the config out") + require.Equal(t, config.PrivateKey, stored.PrivateKey, "the persisted identity is not the one returned") + require.NotEmpty(t, stored.SSHKey) + }) + + t.Run("a second call keeps the identity on disk", func(t *testing.T) { + before, err := profilemanager.GetExistingConfig(profilemanager.DefaultConfigPath) + require.NoError(t, err) + + config, existed, err := provisionProfileIdentity(activeProf) + require.NoError(t, err) + require.True(t, existed) + require.Equal(t, before.PrivateKey, config.PrivateKey, "provisioning minted a second identity") + + after, err := profilemanager.GetExistingConfig(profilemanager.DefaultConfigPath) + require.NoError(t, err) + require.Equal(t, before.PrivateKey, after.PrivateKey, "provisioning rewrote the stored identity") + }) +} diff --git a/client/server/server.go b/client/server/server.go index eb7d4aaa3..c57bbc6a3 100644 --- a/client/server/server.go +++ b/client/server/server.go @@ -1504,9 +1504,16 @@ func (s *Server) handleActiveProfileLogout(ctx context.Context) (*proto.LogoutRe return &proto.LogoutResponse{}, nil } -// getConfig resolves the active profile's config, provisions its identity and -// reports whether the config file already existed. -func (s *Server) getConfig(activeProf *profilemanager.ActiveProfileState) (*profilemanager.Config, bool, error) { +// provisionProfileIdentity resolves the active profile's config and puts the +// keys that identify the peer on disk, reporting whether the config file +// already existed. +// +// This is the daemon's provisioning point: the config resolved here is the one +// the peer runs with, so it needs its identity, and that has to reach disk — a +// key that stays in memory would come back different on the next start and +// re-register the peer. Reads themselves are pure, so the write is here, in +// the open, instead of hiding inside the reader. +func provisionProfileIdentity(activeProf *profilemanager.ActiveProfileState) (*profilemanager.Config, bool, error) { cfgPath, err := activeProf.FilePath() if err != nil { return nil, false, fmt.Errorf("failed to get active profile file path: %w", err) @@ -1522,11 +1529,6 @@ func (s *Server) getConfig(activeProf *profilemanager.ActiveProfileState) (*prof return nil, false, fmt.Errorf("failed to get config: %w", err) } - // This is the daemon's provisioning point: the config resolved here is the - // one the peer runs with, so it needs the keys that identify it, and those - // have to reach disk — a key that stays in memory would come back different - // on the next start and re-register the peer. Reads themselves are pure, so - // the write is here, in the open, instead of hiding inside ReadConfig. generated, err := config.EnsureIdentity() if err != nil { return nil, false, fmt.Errorf("ensure profile identity: %w", err) @@ -1538,6 +1540,17 @@ func (s *Server) getConfig(activeProf *profilemanager.ActiveProfileState) (*prof } } + return config, configExisted, nil +} + +// getConfig resolves the active profile's config, provisions its identity and +// reports whether the config file already existed. +func (s *Server) getConfig(activeProf *profilemanager.ActiveProfileState) (*profilemanager.Config, bool, error) { + config, configExisted, err := provisionProfileIdentity(activeProf) + if err != nil { + return nil, false, err + } + // Apply the daemon-owned MDM policy on top of the just-resolved Config. // profilemanager's apply() initialises the policy to empty — the Loader // lives outside Config, so this overlay step is driven externally here. @@ -2778,6 +2791,15 @@ func (s *Server) authorizeAndPrepareLogin(callerCtx context.Context, msg *proto. return nil, nil, fmt.Errorf("persist login overrides: %w", err) } + // Provisioning under the same lock as the decision above, and next to the + // write it guards. getConfig would otherwise mint the identity and persist + // it once this returns: between its read and its write, a SetConfig that + // had already answered its caller would be overwritten by the config this + // login read before it landed. + if _, _, err := provisionProfileIdentity(activeProf); err != nil { + return nil, nil, err + } + return ctx, activeProf, nil }