[client] Make config reads pure and provision the identity explicitly

Reading a config wrote it back. profilemanager.readConfig persisted whatever
apply() had filled in, and ReadConfig created and wrote the file outright when
it was absent, so every reader was quietly a writer: a gate deciding whether to
refuse a request, a UI listing profiles, a mobile getter reading one preference.
The previous commit worked around that with a PeekConfig variant, which left
two read functions with opposite side effects and the antipattern still there
for everyone else.

Only one thing in a read genuinely had to be persisted: apply() generated the
WireGuard and SSH keys when it found them empty, and a generated key cannot be
recomputed — losing it means the peer comes back with a different identity and
registers again. Everything else apply() fills in is a deterministic default
that the next read recomputes anyway.

So identity provisioning is now its own step, Config.EnsureIdentity, and the
callers that provision write the result out themselves, in the open:

- Server.getConfig, the daemon's provisioning point;
- the CLI's foreground login, which is about to dial management;
- update() / directUpdate(), the config write paths — a stored profile can
  legitimately carry no identity, since a mobile logout clears the keys in
  place, and the next write is what has to mint a new one.

ReadConfig and GetConfig no longer write anything, PeekConfig is gone, and the
dry-run baseline no longer needs placeholder keys to keep apply() from minting
real ones.

One deliberate leftover: readConfig still calls util.EnforcePermission, which
chmods a config file whose permissions are too broad. It changes no content and
is idempotent, and dropping it would leave a legacy file world-readable until
its first write.

client/cmd/login.go

@@ -331,6 +331,18 @@ func doForegroundLogin(ctx context.Context, cmd *cobra.Command, setupKey string,
 		return fmt.Errorf("read config file %s: %v", configFilePath, err)
 	}
 
+	// Reading a config does not provision one: this login is about to dial
+	// management with the profile's identity, so mint the keys if the profile
+	// has none yet and put them on disk — a key that stayed in memory would
+	// come back different on the next run and register a second peer.
+	if generated, err := config.EnsureIdentity(); err != nil {
+		return fmt.Errorf("ensure profile identity: %v", err)
+	} else if generated {
+		if err := profilemanager.WriteOutConfig(configFilePath, config); err != nil {
+			return fmt.Errorf("write out config file %s: %v", configFilePath, err)
+		}
+	}
+
 	// Mirror runInForegroundMode: recover residual state (DNS, firewall,
 	// ssh config, legacy routing) from a previous unclean shutdown and
 	// enable advanced routing before dialing management.

client/internal/profilemanager/config.go

@@ -306,10 +306,15 @@ func newConfigSkeleton() *Config {
 	}
 }
 
-// createNewConfig creates a new config generating a new Wireguard key and saving to file
+// createNewConfig creates a new config in memory, generating the keys that
+// identify the peer. Writing it out is the caller's job.
 func createNewConfig(input ConfigInput) (*Config, error) {
 	config := newConfigSkeleton()
 
+	if _, err := config.EnsureIdentity(); err != nil {
+		return nil, err
+	}
+
 	if _, err := config.apply(input); err != nil {
 		return nil, err
 	}
@@ -317,6 +322,37 @@ func createNewConfig(input ConfigInput) (*Config, error) {
 	return config, nil
 }
 
+// EnsureIdentity generates the keys that identify this peer if the config does
+// not carry them yet, reporting whether it had to generate any.
+//
+// It is deliberately not part of apply(). Everything apply() fills in is a
+// default it can recompute on the next read, but a generated key is not: it
+// has to be persisted, or the peer comes back with a different WireGuard
+// identity and re-registers. Having apply() generate keys is what forced every
+// read of a config to write it back — so identity provisioning is its own step
+// now, and the callers that perform it write the result out explicitly.
+func (config *Config) EnsureIdentity() (bool, error) {
+	generated := false
+
+	if config.PrivateKey == "" {
+		log.Infof("generated new Wireguard key")
+		config.PrivateKey = generateKey()
+		generated = true
+	}
+
+	if config.SSHKey == "" {
+		log.Infof("generated new SSH key")
+		pem, err := ssh.GeneratePrivateKey(ssh.ED25519)
+		if err != nil {
+			return generated, err
+		}
+		config.SSHKey = string(pem)
+		generated = true
+	}
+
+	return generated, nil
+}
+
 func (config *Config) apply(input ConfigInput) (updated bool, err error) {
 	if config.Name != "" {
 		sanitized, err := sanitizeDisplayName(config.Name)
@@ -374,22 +410,6 @@ func (config *Config) apply(input ConfigInput) (updated bool, err error) {
 		}
 	}
 
-	if config.PrivateKey == "" {
-		log.Infof("generated new Wireguard key")
-		config.PrivateKey = generateKey()
-		updated = true
-	}
-
-	if config.SSHKey == "" {
-		log.Infof("generated new SSH key")
-		pem, err := ssh.GeneratePrivateKey(ssh.ED25519)
-		if err != nil {
-			return false, err
-		}
-		config.SSHKey = string(pem)
-		updated = true
-	}
-
 	if input.WireguardPort != nil && *input.WireguardPort != config.WgPort {
 		log.Infof("updating Wireguard port %d (old value %d)",
 			*input.WireguardPort, config.WgPort)
@@ -960,11 +980,6 @@ func generateKey() string {
 	return key.String()
 }
 
-// dryRunKeyPlaceholder stands in for the WireGuard and SSH keys of a config
-// that is only ever compared against, never persisted or used to connect. It
-// keeps apply() from generating real keys for a throwaway baseline.
-const dryRunKeyPlaceholder = "dry-run"
-
 // don't overwrite pre-shared key if we receive asterisks from UI
 func isPreSharedKeyHidden(preSharedKey *string) bool {
 	if preSharedKey != nil && *preSharedKey == "**********" {
@@ -1004,19 +1019,11 @@ func (config *Config) WouldChange(input ConfigInput) (bool, error) {
 }
 
 // newDryRunBaseline builds the config a brand-new profile would start from, for
-// a dry run to compare an input against.
-//
-// It is createNewConfig with the key generation skipped: apply() generates a
-// WireGuard and an SSH key whenever it finds those fields empty, and this
-// config exists only to be compared against and thrown away. Generating a
-// keypair per evaluation is waste on its own, and it logs "generated new
-// Wireguard key" once per attempt — in the CLI's login backoff loop that reads
-// like the client rotating its peer key. No ConfigInput field maps to either
-// key, so a placeholder cannot affect the comparison.
+// a dry run to compare an input against. It is createNewConfig without the
+// identity: this config exists only to be compared against and thrown away, and
+// no ConfigInput field maps to either key.
 func newDryRunBaseline(configPath string) (*Config, error) {
 	baseline := newConfigSkeleton()
-	baseline.PrivateKey = dryRunKeyPlaceholder
-	baseline.SSHKey = dryRunKeyPlaceholder
 
 	if _, err := baseline.apply(ConfigInput{ConfigPath: configPath}); err != nil {
 		return nil, err
@@ -1101,12 +1108,20 @@ func update(input ConfigInput) (*Config, error) {
 		return nil, err
 	}
 
+	// A write path is a provisioning point: a stored profile can legitimately
+	// carry no identity (a mobile logout clears the keys in place), and the
+	// next config write is what has to mint a new one. Reads leave that alone.
+	identityGenerated, err := config.EnsureIdentity()
+	if err != nil {
+		return nil, err
+	}
+
 	updated, err := config.apply(input)
 	if err != nil {
 		return nil, err
 	}
 
-	if updated {
+	if updated || identityGenerated {
 		if err := util.WriteJson(context.Background(), input.ConfigPath, config); err != nil {
 			return nil, err
 		}
@@ -1117,7 +1132,7 @@ func update(input ConfigInput) (*Config, error) {
 
 // GetConfig read config file and return with Config and if it was created. Errors out if it does not exist
 func GetConfig(configPath string) (*Config, error) {
-	return readConfig(configPath, false, true)
+	return readConfig(configPath, false)
 }
 
 // UpdateOldManagementURL checks whether client can switch to the new Management URL with port 443 and the management domain.
@@ -1204,26 +1219,24 @@ func CreateInMemoryConfig(input ConfigInput) (*Config, error) {
 	return createNewConfig(input)
 }
 
-// ReadConfig read config file and return with Config. If it is not exists create a new with default values
+// ReadConfig reads the profile config at configPath, resolving a default config
+// in memory when the file does not exist.
+//
+// It never writes. A caller that wants what it got back to be on disk calls
+// WriteOutConfig itself, and one that resolved a config for a peer to run with
+// calls EnsureIdentity first — see Server.getConfig for that pair.
 func ReadConfig(configPath string) (*Config, error) {
-	return readConfig(configPath, true, true)
+	return readConfig(configPath, true)
 }
 
-// PeekConfig reads an existing profile config without writing anything back.
-// GetConfig persists the normalization whenever apply() fills in a default,
-// which a caller that only inspects the stored settings must not do: the
-// daemon's update-settings gate reads the config to decide whether to refuse a
-// request, and a refused request has to leave the profile file exactly as it
-// found it. Errors out when the config does not exist.
-func PeekConfig(configPath string) (*Config, error) {
-	return readConfig(configPath, false, false)
-}
-
-// readConfig reads the profile config at configPath. createIfMissing generates
-// a default config (and writes it out) when the file is absent, rather than
-// erroring. persistNormalization writes the config back when apply() had to
-// fill in defaults the file was missing; a read-only caller passes false.
-func readConfig(configPath string, createIfMissing, persistNormalization bool) (*Config, error) {
+// readConfig reads the profile config at configPath. createIfMissing resolves a
+// default config in memory when the file is absent, rather than erroring.
+//
+// Reads are pure. This used to write the config back whenever apply() had to
+// fill in a default the file was missing, which quietly made every reader a
+// writer: a gate deciding whether to refuse a request, a UI listing profiles,
+// a mobile getter reading a single preference.
+func readConfig(configPath string, createIfMissing bool) (*Config, error) {
 	configExists, err := fileExists(configPath)
 	if err != nil {
 		return nil, fmt.Errorf("failed to check if config file exists: %w", err)
@@ -1240,12 +1253,8 @@ func readConfig(configPath string, createIfMissing, persistNormalization bool) (
 			return nil, err
 		}
 		// initialize through apply() without changes
-		if changed, err := config.apply(ConfigInput{}); err != nil {
+		if _, err := config.apply(ConfigInput{}); err != nil {
 			return nil, err
-		} else if changed && persistNormalization {
-			if err = WriteOutConfig(configPath, config); err != nil {
-				return nil, err
-			}
 		}
 
 		return config, nil
@@ -1253,13 +1262,7 @@ func readConfig(configPath string, createIfMissing, persistNormalization bool) (
 		return nil, fmt.Errorf("config file %s does not exist", configPath)
 	}
 
-	cfg, err := createNewConfig(ConfigInput{ConfigPath: configPath})
-	if err != nil {
-		return nil, err
-	}
-
-	err = WriteOutConfig(configPath, cfg)
-	return cfg, err
+	return createNewConfig(ConfigInput{ConfigPath: configPath})
 }
 
 // WriteOutConfig write put the prepared config to the given path
@@ -1309,12 +1312,18 @@ func directUpdate(input ConfigInput) (*Config, error) {
 		return nil, err
 	}
 
+	// Same provisioning point as update(); see the note there.
+	identityGenerated, err := config.EnsureIdentity()
+	if err != nil {
+		return nil, err
+	}
+
 	updated, err := config.apply(input)
 	if err != nil {
 		return nil, err
 	}
 
-	if updated {
+	if updated || identityGenerated {
 		if err := util.DirectWriteJson(context.Background(), input.ConfigPath, config); err != nil {
 			return nil, err
 		}

client/internal/profilemanager/config_would_change_test.go

@@ -102,36 +102,60 @@ func TestWouldChangeReportsAnInvalidInput(t *testing.T) {
 	require.Error(t, err)
 }
 
-// GetConfig persists the normalization it performs; PeekConfig must not, so a
-// caller that only inspects the stored settings leaves the file alone.
-func TestPeekConfigDoesNotWriteBack(t *testing.T) {
-	// A config file missing a field apply() fills in (MTU) is what makes the
-	// normalization write fire.
+// Reads must not write. A config file missing a field apply() fills in (MTU,
+// here) is what used to trigger the write-back.
+func TestReadsDoNotWriteTheConfigBack(t *testing.T) {
 	denormalized := []byte(`{"WgIface":"wt0"}`)
 
-	peekPath := filepath.Join(t.TempDir(), "peek.json")
-	require.NoError(t, os.WriteFile(peekPath, denormalized, 0o600))
-	before, err := os.ReadFile(peekPath)
-	require.NoError(t, err)
+	for name, read := range map[string]func(string) (*Config, error){
+		"GetConfig":  GetConfig,
+		"ReadConfig": ReadConfig,
+	} {
+		t.Run(name, func(t *testing.T) {
+			path := filepath.Join(t.TempDir(), "profile.json")
+			require.NoError(t, os.WriteFile(path, denormalized, 0o600))
 
-	cfg, err := PeekConfig(peekPath)
-	require.NoError(t, err)
-	require.Equal(t, uint16(iface.DefaultMTU), cfg.MTU, "the returned config is still normalized in memory")
+			cfg, err := read(path)
+			require.NoError(t, err)
+			require.Equal(t, uint16(iface.DefaultMTU), cfg.MTU, "the returned config is still normalized in memory")
+			require.Empty(t, cfg.PrivateKey, "a read must not mint an identity either")
 
-	after, err := os.ReadFile(peekPath)
-	require.NoError(t, err)
-	require.Equal(t, string(before), string(after), "PeekConfig rewrote the config file")
+			after, err := os.ReadFile(path)
+			require.NoError(t, err)
+			require.Equal(t, string(denormalized), string(after), "%s rewrote the config file", name)
+		})
+	}
+}
 
-	// Same file through GetConfig, which is expected to persist it.
-	getPath := filepath.Join(t.TempDir(), "get.json")
-	require.NoError(t, os.WriteFile(getPath, denormalized, 0o600))
+// ReadConfig resolves a default config for a profile that has no file yet, and
+// that must not create the file either.
+func TestReadConfigDoesNotCreateTheFile(t *testing.T) {
+	path := filepath.Join(t.TempDir(), "absent.json")
 
-	_, err = GetConfig(getPath)
+	cfg, err := ReadConfig(path)
 	require.NoError(t, err)
+	require.Equal(t, DefaultManagementURL, cfg.ManagementURL.String())
 
-	persisted, err := os.ReadFile(getPath)
+	_, err = os.Stat(path)
+	require.True(t, os.IsNotExist(err), "ReadConfig created the config file")
+}
+
+// The identity is the one thing a read cannot recompute, so it is provisioned
+// on request and its caller persists it.
+func TestEnsureIdentity(t *testing.T) {
+	cfg := newConfigSkeleton()
+
+	generated, err := cfg.EnsureIdentity()
 	require.NoError(t, err)
-	require.NotEqual(t, string(denormalized), string(persisted), "GetConfig is the variant that normalizes on disk")
+	require.True(t, generated)
+	require.NotEmpty(t, cfg.PrivateKey)
+	require.NotEmpty(t, cfg.SSHKey)
+
+	key := cfg.PrivateKey
+	generated, err = cfg.EnsureIdentity()
+	require.NoError(t, err)
+	require.False(t, generated, "a config that already has an identity keeps it")
+	require.Equal(t, key, cfg.PrivateKey)
 }
 
 // One endpoint written several ways is one endpoint. A gate that compared
@@ -201,11 +225,39 @@ func TestDryRunBaselineDoesNotGenerateKeys(t *testing.T) {
 	baseline, err := newDryRunBaseline(filepath.Join(t.TempDir(), "absent.json"))
 	require.NoError(t, err)
 
-	require.Equal(t, dryRunKeyPlaceholder, baseline.PrivateKey, "generated a WireGuard key for a throwaway config")
-	require.Equal(t, dryRunKeyPlaceholder, baseline.SSHKey, "generated an SSH key for a throwaway config")
+	require.Empty(t, baseline.PrivateKey, "generated a WireGuard key for a throwaway config")
+	require.Empty(t, baseline.SSHKey, "generated an SSH key for a throwaway config")
 
 	// Everything the comparison actually looks at is still the default config.
 	require.Equal(t, DefaultManagementURL, baseline.ManagementURL.String())
 	require.Equal(t, uint16(iface.DefaultMTU), baseline.MTU)
 	require.Equal(t, iface.DefaultWgPort, baseline.WgPort)
 }
+
+// A stored profile can carry no identity — a mobile logout clears the keys in
+// place — so the next config write has to mint one, which is what keeps the
+// following login from dialing management with an empty key.
+func TestUpdateConfigProvisionsAMissingIdentity(t *testing.T) {
+	path := filepath.Join(t.TempDir(), "logged-out.json")
+	_, err := UpdateOrCreateConfig(ConfigInput{
+		ConfigPath:    path,
+		ManagementURL: "https://api.netbird.io:443",
+	})
+	require.NoError(t, err)
+
+	// Stand in for the logout, which zeroes the keys and writes the config out.
+	loggedOut, err := GetConfig(path)
+	require.NoError(t, err)
+	loggedOut.PrivateKey = ""
+	loggedOut.SSHKey = ""
+	require.NoError(t, WriteOutConfig(path, loggedOut))
+
+	cfg, err := UpdateOrCreateConfig(ConfigInput{ConfigPath: path})
+	require.NoError(t, err)
+	require.NotEmpty(t, cfg.PrivateKey, "the write path did not provision an identity")
+	require.NotEmpty(t, cfg.SSHKey)
+
+	persisted, err := GetConfig(path)
+	require.NoError(t, err)
+	require.Equal(t, cfg.PrivateKey, persisted.PrivateKey, "the provisioned identity was not persisted")
+}

client/server/server.go

@@ -1162,10 +1162,9 @@ func (s *Server) storedLoginConfig(activeProf *profilemanager.ActiveProfileState
 // storedConfigAtPath reads a profile config file, yielding nil when it does not
 // exist yet.
 //
-// It peeks rather than reads: every caller here feeds a gate that may refuse
-// the request, and profilemanager.GetConfig writes the config back whenever it
-// has to fill in a default the file was missing. A refused request must leave
-// the profile file exactly as it found it.
+// Reading it has no side effect: profilemanager.GetConfig does not write, so a
+// request that the gates go on to refuse leaves the profile file as it found
+// it.
 func (s *Server) storedConfigAtPath(path string) (*profilemanager.Config, error) {
 	if _, err := os.Stat(path); err != nil {
 		if os.IsNotExist(err) {
@@ -1174,7 +1173,7 @@ func (s *Server) storedConfigAtPath(path string) (*profilemanager.Config, error)
 		return nil, fmt.Errorf("stat profile config: %w", err)
 	}
 
-	cfg, err := profilemanager.PeekConfig(path)
+	cfg, err := profilemanager.GetConfig(path)
 	if err != nil {
 		return nil, fmt.Errorf("read profile config: %w", err)
 	}
@@ -1490,6 +1489,22 @@ 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)
+	}
+
+	if generated || !configExisted {
+		if err := profilemanager.WriteOutConfig(cfgPath, config); err != nil {
+			return nil, false, fmt.Errorf("write out profile config: %w", err)
+		}
+	}
+
 	return config, configExisted, nil
 }
 

client/server/update_settings_gate_test.go

@@ -336,7 +336,7 @@ func TestLogin_ChangeThatAppearsMidRequestIsRefused(t *testing.T) {
 	require.Equal(t, codes.Unavailable, gstatus.Code(err), "want the update-settings refusal, got %v", err)
 	require.False(t, cancelled, "the refused login cancelled the login already in progress")
 
-	stored, err := profilemanager.PeekConfig(targetPath)
+	stored, err := profilemanager.GetConfig(targetPath)
 	require.NoError(t, err)
 	require.Equal(t, "https://mgmt.elsewhere.example:443", stored.ManagementURL.String(),
 		"the refused login wrote the management URL it was asked for")