mirror of
https://github.com/netbirdio/netbird.git
synced 2026-09-25 08:09:07 +02:00
[client] Provision the peer identity under the config lock (review item)
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.
This commit is contained in:
+30
-8
@@ -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
|
||||
}
|
||||
|
||||
|
||||
Reference in New Issue
Block a user