[client] Gate settings updates on value, not on field presence

The update-settings kill switch (--disable-update-settings / NB_DISABLE_UPDATE_SETTINGS / the MDM DisableUpdateSettings key) forbids changing settings, but it decided what a "change" was by looking at whether a field was present in the request. The CLI fills the whole config surface of SetConfigRequest and LoginRequest from its flags and environment on every `netbird up` (setupSetConfigReq in cmd/up.go), so a client configured by environment restates its own configuration on every start and tripped the gate every time. SetConfig only warned about that, but Login carries the same fields and was gated the same way, and Login runs inside the CLI's backoff loop: the daemon answered every attempt with codes.Unavailable, `netbird up` never completed, and a container with NB_DISABLE_UPDATE_SETTINGS plus any config env var (NB_MANAGEMENT_URL, for one) could not come up at all. Both gates now compare values. Config.WouldChange is the dry-run half of UpdateConfig: it runs the very same diff logic (Config.apply) against a copy of the stored config, so the gate cannot drift from what an actual update would do, nor go stale when a field is added. A request that restates what the profile already holds changes nothing and is allowed; a request that diverges is refused exactly as before, and a dry run that cannot be evaluated fails closed. A profile with no config on disk yet is judged against the config the daemon would create for it. For Login, the compared input comes from loginOverridesInput, which persistLoginOverrides also uses to perform the write, so the gate judges precisely the two fields a login can persist (management URL, pre-shared key) and no field it ignores. Two adjacent defects surfaced while making the comparison exact: - Config.apply compared URLs as raw strings, so the same endpoint spelled without its default port ("https://api.netbird.io" vs "https://api.netbird.io:443") counted as a new value and rewrote the config. It now compares the parsed forms. - UpdateConfig did not collapse the redacted pre-shared key, unlike UpdateOrCreateConfig and DirectUpdateConfig, so a UI round-trip of the mask replaced the stored key with asterisks. The CLI warning for a refused SetConfig said the method was not available in the daemon, which sent people looking for a version mismatch that was not there; it now reports the refusal.
2026-09-15 11:19:08 +02:00 · 2026-09-02 12:57:14 +02:00
parent c3cf7c0c37
commit ef88c4de5f
8 changed files with 526 additions and 154 deletions
@@ -328,20 +328,20 @@ func (config *Config) apply(input ConfigInput) (updated bool, err error) {
 			return false, err
 		}
 	}
-	if input.ManagementURL != "" && input.ManagementURL != config.ManagementURL.String() {
-		log.Infof("new Management URL provided, updated to %#v (old value %#v)",
-			input.ManagementURL, config.ManagementURL.String())
+	// The comparison is between parsed URLs, not raw strings: the same
+	// endpoint can be written differently (an implicit :443, say), and
+	// treating an equivalent URL as new would rewrite the config and report a
+	// settings change where the configuration does not actually change.
+	if input.ManagementURL != "" {
 		URL, err := parseURL("Management URL", input.ManagementURL)
 		if err != nil {
 			return false, err
 		}
-		config.ManagementURL = URL
-		updated = true
-	} else if config.ManagementURL == nil {
-		log.Infof("using default Management URL %s", DefaultManagementURL)
-		config.ManagementURL, err = parseURL("Management URL", DefaultManagementURL)
-		if err != nil {
-			return false, err
+		if URL.String() != config.ManagementURL.String() {
+			log.Infof("new Management URL provided, updated to %#v (old value %#v)",
+				URL.String(), config.ManagementURL.String())
+			config.ManagementURL = URL
+			updated = true
 		}
 	}

@@ -352,15 +352,18 @@ func (config *Config) apply(input ConfigInput) (updated bool, err error) {
 			return false, err
 		}
 	}
-	if input.AdminURL != "" && input.AdminURL != config.AdminURL.String() {
-		log.Infof("new Admin Panel URL provided, updated to %#v (old value %#v)",
-			input.AdminURL, config.AdminURL.String())
+	// Same parsed-form comparison as the Management URL above.
+	if input.AdminURL != "" {
 		newURL, err := parseURL("Admin Panel URL", input.AdminURL)
 		if err != nil {
 			return updated, err
 		}
-		config.AdminURL = newURL
-		updated = true
+		if newURL.String() != config.AdminURL.String() {
+			log.Infof("new Admin Panel URL provided, updated to %#v (old value %#v)",
+				newURL.String(), config.AdminURL.String())
+			config.AdminURL = newURL
+			updated = true
+		}
 	}

 	if config.PrivateKey == "" {
@@ -920,6 +923,58 @@ func isPreSharedKeyHidden(preSharedKey *string) bool {
 	return false
 }

+// WouldChange reports whether applying input would modify any field the
+// config persists, leaving the receiver untouched. It is the dry-run half of
+// UpdateConfig and reuses the very same diff logic (Config.apply), so a
+// caller asking "is this a settings change?" cannot drift from what an
+// actual update would do, nor go stale when a new field is added.
+//
+// A redacted pre-shared key is collapsed to "unset" exactly as
+// UpdateOrCreateConfig does, so a UI that round-trips the mask is not read as
+// a request for a new key.
+//
+// A nil receiver means the profile holds no config yet, so the baseline is the
+// config the daemon would create for it: input values matching those defaults
+// change nothing, anything else does.
+func (config *Config) WouldChange(input ConfigInput) (bool, error) {
+	probe := config.clone()
+	if probe == nil {
+		baseline, err := createNewConfig(ConfigInput{ConfigPath: input.ConfigPath})
+		if err != nil {
+			return true, fmt.Errorf("build default config baseline: %w", err)
+		}
+		probe = baseline
+	}
+
+	if isPreSharedKeyHidden(input.PreSharedKey) {
+		input.PreSharedKey = nil
+	}
+
+	return probe.apply(input)
+}
+
+// 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
+// to, and SyncMessageVersion, which it writes through the pointer. The
+// remaining pointer fields are reassigned, not written through, and
+// ClientCertKeyPair is only overwritten.
+func (config *Config) clone() *Config {
+	if config == nil {
+		return nil
+	}
+
+	probe := *config
+	probe.IFaceBlackList = slices.Clone(config.IFaceBlackList)
+	probe.NATExternalIPs = slices.Clone(config.NATExternalIPs)
+	probe.DNSLabels = slices.Clone(config.DNSLabels)
+	if config.SyncMessageVersion != nil {
+		version := *config.SyncMessageVersion
+		probe.SyncMessageVersion = &version
+	}
+	return &probe
+}
+
 // UpdateConfig update existing configuration according to input configuration and return with the configuration
 func UpdateConfig(input ConfigInput) (*Config, error) {
 	configExists, err := fileExists(input.ConfigPath)
@@ -930,6 +985,14 @@ func UpdateConfig(input ConfigInput) (*Config, error) {
 		return nil, fmt.Errorf("config file %s does not exist", input.ConfigPath)
 	}

+	// A UI that round-trips the mask GetConfig hands it back is asking to keep
+	// the stored key, not to set the mask as the new one. UpdateOrCreateConfig
+	// and DirectUpdateConfig already collapse it; this one did not, so the
+	// same round-trip through SetConfig replaced the key with asterisks.
+	if isPreSharedKeyHidden(input.PreSharedKey) {
+		input.PreSharedKey = nil
+	}
+
 	return update(input)
 }

@@ -0,0 +1,101 @@
+package profilemanager
+
+import (
+	"path/filepath"
+	"testing"
+
+	"github.com/stretchr/testify/require"
+
+	"github.com/netbirdio/netbird/shared/management/domain"
+)
+
+func seededConfig(t *testing.T) *Config {
+	t.Helper()
+
+	path := filepath.Join(t.TempDir(), "seeded.json")
+	cfg, err := UpdateOrCreateConfig(ConfigInput{
+		ConfigPath:    path,
+		ManagementURL: "https://api.netbird.io:443",
+		PreSharedKey:  strPointer("stored-key"),
+	})
+	require.NoError(t, err)
+	return cfg
+}
+
+func strPointer(s string) *string { return &s }
+
+func TestWouldChange(t *testing.T) {
+	tests := []struct {
+		name  string
+		input ConfigInput
+		want  bool
+	}{
+		{name: "empty input", input: ConfigInput{}, want: false},
+		{name: "same management URL", input: ConfigInput{ManagementURL: "https://api.netbird.io:443"}, want: false},
+		{name: "management URL without its default port", input: ConfigInput{ManagementURL: "https://api.netbird.io"}, want: false},
+		{name: "different management URL", input: ConfigInput{ManagementURL: "https://other.example:443"}, want: true},
+		{name: "same pre-shared key", input: ConfigInput{PreSharedKey: strPointer("stored-key")}, want: false},
+		{name: "redacted pre-shared key", input: ConfigInput{PreSharedKey: strPointer("**********")}, want: false},
+		{name: "different pre-shared key", input: ConfigInput{PreSharedKey: strPointer("other-key")}, want: true},
+		{name: "new interface blacklist entry", input: ConfigInput{ExtraIFaceBlackList: []string{"nb-probe0"}}, want: true},
+		{name: "blacklist entry already present", input: ConfigInput{ExtraIFaceBlackList: []string{"lo"}}, want: false},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			cfg := seededConfig(t)
+
+			changed, err := cfg.WouldChange(tt.input)
+			require.NoError(t, err)
+			require.Equal(t, tt.want, changed)
+		})
+	}
+}
+
+// The dry run must not be observable on the config it is run against: it
+// decides whether a write is allowed, it does not perform one.
+func TestWouldChangeLeavesTheConfigAlone(t *testing.T) {
+	cfg := seededConfig(t)
+	blacklist := len(cfg.IFaceBlackList)
+
+	changed, err := cfg.WouldChange(ConfigInput{
+		ManagementURL:       "https://other.example:443",
+		PreSharedKey:        strPointer("other-key"),
+		ExtraIFaceBlackList: []string{"nb-probe0"},
+		DNSLabels:           domain.FromPunycodeList([]string{"probe"}),
+		NATExternalIPs:      []string{"1.2.3.4"},
+	})
+	require.NoError(t, err)
+	require.True(t, changed)
+
+	require.Equal(t, "https://api.netbird.io:443", cfg.ManagementURL.String())
+	require.Equal(t, "stored-key", cfg.PreSharedKey)
+	require.Len(t, cfg.IFaceBlackList, blacklist)
+	require.Empty(t, cfg.DNSLabels)
+	require.Empty(t, cfg.NATExternalIPs)
+}
+
+// A nil config means the profile holds nothing yet, so the baseline is what
+// the daemon would create for it.
+func TestWouldChangeWithoutAStoredConfig(t *testing.T) {
+	var cfg *Config
+
+	changed, err := cfg.WouldChange(ConfigInput{})
+	require.NoError(t, err)
+	require.False(t, changed, "a request carrying nothing cannot change anything")
+
+	changed, err = cfg.WouldChange(ConfigInput{ManagementURL: DefaultManagementURL})
+	require.NoError(t, err)
+	require.False(t, changed, "the default management URL is what would be written anyway")
+
+	changed, err = cfg.WouldChange(ConfigInput{ManagementURL: "https://other.example:443"})
+	require.NoError(t, err)
+	require.True(t, changed)
+}
+
+func TestWouldChangeReportsAnInvalidInput(t *testing.T) {
+	cfg := seededConfig(t)
+
+	_, err := cfg.WouldChange(ConfigInput{ManagementURL: "not-a-url"})
+	require.Error(t, err)
+}