From e027ba11f9f832ac0e45c2201ce6266b9825cfd1 Mon Sep 17 00:00:00 2001 From: riccardom Date: Wed, 2 Sep 2026 15:47:24 +0200 Subject: [PATCH] [client] Keep the peer identity out of a read that finds no file ReadOrGenerateConfig resolves a default config when the profile has no file yet, and createNewConfig was minting the WireGuard and SSH keys while doing so. That defeated the provisioning pair it was meant to serve: the CLI's foreground login calls EnsureIdentity to find out whether it has to persist the keys, got generated == false because the read had already generated them, and so never wrote them out. The login then dialed management with an identity that only existed in memory, and the next login registered a second peer. createNewConfig no longer provisions. createProvisionedConfig is the variant that does, and the callers whose contract is "usable as it comes back" use it: CreateInMemoryConfig, whose callers connect with the result, and the two create-and-write branches. A read gets a config with no identity, so the caller's own EnsureIdentity reports the work and triggers the write. Reported by CodeRabbit and cubic-dev-ai on PR #7398, both on the same defect. --- client/internal/profilemanager/config.go | 30 ++++++++++++++----- .../config_would_change_test.go | 27 +++++++++++++++++ 2 files changed, 49 insertions(+), 8 deletions(-) diff --git a/client/internal/profilemanager/config.go b/client/internal/profilemanager/config.go index 718ff722f..7d294505c 100644 --- a/client/internal/profilemanager/config.go +++ b/client/internal/profilemanager/config.go @@ -306,16 +306,29 @@ func newConfigSkeleton() *Config { } } -// createNewConfig creates a new config in memory, generating the keys that -// identify the peer. Writing it out is the caller's job. +// createNewConfig resolves a new config in memory, with no identity: whoever +// needs the peer's keys calls EnsureIdentity and persists the result, so a read +// that lands on a missing file cannot hand back a config carrying keys that +// nothing will ever write down. func createNewConfig(input ConfigInput) (*Config, error) { config := newConfigSkeleton() - if _, err := config.EnsureIdentity(); err != nil { + if _, err := config.apply(input); err != nil { return nil, err } - if _, err := config.apply(input); err != nil { + return config, nil +} + +// createProvisionedConfig is createNewConfig plus the peer's identity, for the +// callers that go on to persist the config or to connect with it. +func createProvisionedConfig(input ConfigInput) (*Config, error) { + config, err := createNewConfig(input) + if err != nil { + return nil, err + } + + if _, err := config.EnsureIdentity(); err != nil { return nil, err } @@ -1084,7 +1097,7 @@ func UpdateOrCreateConfig(input ConfigInput) (*Config, error) { } if !configExists { log.Infof("generating new config %s", input.ConfigPath) - cfg, err := createNewConfig(input) + cfg, err := createProvisionedConfig(input) if err != nil { return nil, err } @@ -1215,9 +1228,10 @@ func UpdateOldManagementURL(ctx context.Context, config *Config, configPath stri return newConfig, nil } -// CreateInMemoryConfig generate a new config but do not write out it to the store +// CreateInMemoryConfig generate a new config but do not write out it to the store. +// It carries an identity: callers connect with what they get back. func CreateInMemoryConfig(input ConfigInput) (*Config, error) { - return createNewConfig(input) + return createProvisionedConfig(input) } // ReadOrGenerateConfig reads the profile config at configPath if it exists, or @@ -1282,7 +1296,7 @@ func DirectUpdateOrCreateConfig(input ConfigInput) (*Config, error) { } if !configExists { log.Infof("generating new config %s", input.ConfigPath) - cfg, err := createNewConfig(input) + cfg, err := createProvisionedConfig(input) if 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 00d8fc4e8..8bc863fb7 100644 --- a/client/internal/profilemanager/config_would_change_test.go +++ b/client/internal/profilemanager/config_would_change_test.go @@ -302,3 +302,30 @@ func TestWouldChangeIgnoresRestatedCertificatePaths(t *testing.T) { require.NoError(t, err) require.True(t, changed, "a different certificate path is a change") } + +// A read that lands on a missing file must not hand back keys: nothing would +// write them down, so the caller would connect with an identity that changes on +// the next run and registers a second peer. +func TestReadOrGenerateConfigCarriesNoIdentity(t *testing.T) { + cfg, err := ReadOrGenerateConfig(filepath.Join(t.TempDir(), "absent.json")) + require.NoError(t, err) + + require.Empty(t, cfg.PrivateKey, "a read minted a WireGuard key") + require.Empty(t, cfg.SSHKey, "a read minted an SSH key") + + // So the caller's own EnsureIdentity is the one that reports the work, and + // therefore the one that triggers the write. + generated, err := cfg.EnsureIdentity() + require.NoError(t, err) + require.True(t, generated, "the provisioning caller could not tell it had to persist the identity") +} + +// CreateInMemoryConfig is the opposite contract: its callers connect with what +// they get back, so it does carry an identity. +func TestCreateInMemoryConfigCarriesAnIdentity(t *testing.T) { + cfg, err := CreateInMemoryConfig(ConfigInput{ManagementURL: "https://api.netbird.io:443"}) + require.NoError(t, err) + + require.NotEmpty(t, cfg.PrivateKey) + require.NotEmpty(t, cfg.SSHKey) +}