From 7a1f095eb5b08870de9651ec4492521ca4ff9841 Mon Sep 17 00:00:00 2001 From: riccardom Date: Wed, 2 Sep 2026 15:04:46 +0200 Subject: [PATCH] [client] Make config reads pure and provision the identity explicitly MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 | 12 ++ client/internal/profilemanager/config.go | 139 ++++++++++-------- .../config_would_change_test.go | 98 +++++++++--- client/server/server.go | 25 +++- client/server/update_settings_gate_test.go | 2 +- 5 files changed, 182 insertions(+), 94 deletions(-) diff --git a/client/cmd/login.go b/client/cmd/login.go index 4e08334eb..ec4f7fdb8 100644 --- a/client/cmd/login.go +++ b/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. diff --git a/client/internal/profilemanager/config.go b/client/internal/profilemanager/config.go index 029eb14ec..29592a999 100644 --- a/client/internal/profilemanager/config.go +++ b/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 } diff --git a/client/internal/profilemanager/config_would_change_test.go b/client/internal/profilemanager/config_would_change_test.go index 42e8ddf75..540cd0f8f 100644 --- a/client/internal/profilemanager/config_would_change_test.go +++ b/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") +} diff --git a/client/server/server.go b/client/server/server.go index 09d47d91f..e107e1f48 100644 --- a/client/server/server.go +++ b/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 } diff --git a/client/server/update_settings_gate_test.go b/client/server/update_settings_gate_test.go index d9dc41b13..5d70e4b99 100644 --- a/client/server/update_settings_gate_test.go +++ b/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")