From ea972f7847f84102e30570f1e82e9725c45f83bc Mon Sep 17 00:00:00 2001 From: riccardom Date: Wed, 2 Sep 2026 14:32:34 +0200 Subject: [PATCH] [client] Stop the config dry run from generating throwaway keys MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The dry run's baseline for a profile with no config file yet went through createNewConfig, and apply() generates a WireGuard and an SSH key whenever it finds those fields empty. The baseline is compared against and discarded, so every evaluation minted a keypair it threw away — and logged "generated new Wireguard key". The CLI retries Login in a backoff loop, so a first `netbird up` on a fresh profile filled the daemon log with what reads like peer-key rotation. The baseline now starts from the shared skeleton with placeholder keys, so apply() has nothing to generate. No ConfigInput field maps to either key, so the comparison is unaffected. --- client/internal/profilemanager/config.go | 42 +++++++++++++++++-- .../config_would_change_test.go | 16 +++++++ 2 files changed, 54 insertions(+), 4 deletions(-) diff --git a/client/internal/profilemanager/config.go b/client/internal/profilemanager/config.go index d84a0edc3..f690a1bd1 100644 --- a/client/internal/profilemanager/config.go +++ b/client/internal/profilemanager/config.go @@ -292,9 +292,11 @@ func fileExists(path string) (bool, error) { return false, err } -// createNewConfig creates a new config generating a new Wireguard key and saving to file -func createNewConfig(input ConfigInput) (*Config, error) { - config := &Config{ +// newConfigSkeleton returns the field values a brand-new profile config starts +// from, before apply() fills in the rest. Shared with the dry-run baseline so +// the two cannot disagree about what "a new config" means. +func newConfigSkeleton() *Config { + return &Config{ // defaults to false only for new (post 0.26) configurations ServerSSHAllowed: util.False(), // Remote jobs are an explicit opt-in and default off, including for @@ -302,6 +304,11 @@ func createNewConfig(input ConfigInput) (*Config, error) { RemoteJobsAllowed: util.False(), WgPort: iface.DefaultWgPort, } +} + +// createNewConfig creates a new config generating a new Wireguard key and saving to file +func createNewConfig(input ConfigInput) (*Config, error) { + config := newConfigSkeleton() if _, err := config.apply(input); err != nil { return nil, err @@ -953,6 +960,11 @@ 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 == "**********" { @@ -977,7 +989,7 @@ func isPreSharedKeyHidden(preSharedKey *string) bool { func (config *Config) WouldChange(input ConfigInput) (bool, error) { probe := config.clone() if probe == nil { - baseline, err := createNewConfig(ConfigInput{ConfigPath: input.ConfigPath}) + baseline, err := newDryRunBaseline(input.ConfigPath) if err != nil { return true, fmt.Errorf("build default config baseline: %w", err) } @@ -991,6 +1003,28 @@ func (config *Config) WouldChange(input ConfigInput) (bool, error) { return probe.apply(input) } +// 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. +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 + } + + return baseline, nil +} + // clone returns a copy of the config that apply can be run against without // the original observing the writes, or nil for a nil receiver. Only what // apply mutates in place needs detaching: the slices it replaces or appends diff --git a/client/internal/profilemanager/config_would_change_test.go b/client/internal/profilemanager/config_would_change_test.go index f899923a7..42e8ddf75 100644 --- a/client/internal/profilemanager/config_would_change_test.go +++ b/client/internal/profilemanager/config_would_change_test.go @@ -193,3 +193,19 @@ func TestWouldChangeIgnoresURLSpelling(t *testing.T) { require.NoError(t, err) require.True(t, changed, "a different port is a different endpoint") } + +// The dry-run baseline exists to be compared against and discarded, so it must +// not mint keys — the CLI's login backoff loop would otherwise log a fresh +// "generated new Wireguard key" on every attempt. +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") + + // 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) +}