From ea8e64e6ef37f26430f645516a9c5128e92e9cde Mon Sep 17 00:00:00 2001 From: riccardom Date: Tue, 8 Sep 2026 09:30:46 +0200 Subject: [PATCH] [client] Gather the optional-field defaults into one function MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Resolving an unset optional field was spread over five places: the two values newConfigSkeleton pre-sets, the block this branch added for the SSH toggles, the network monitor's own if, the `else if` tails of ServerSSHAllowed and RemoteJobsAllowed, and a trailing if for DisableNotifications several hundred lines further down. Reading apply() left no single answer to "what does this field default to, and who decides". They now live in Config.resolveUnsetDefaults, which apply() calls before it compares anything — the ordering being the point, since it is what lets every comparison below diff values instead of presence. The comparisons for ServerSSHAllowed, RemoteJobsAllowed and DisableNotifications lose their `config.X == nil ||` clauses accordingly, as the other six already had. newConfigSkeleton keeps its two, and that is the one asymmetry worth naming: ServerSSHAllowed defaults to false for a new profile and to true for a legacy one, and it only works because the skeleton runs first. The doc comment says so, where before it was implied by the order of two distant blocks. Pure refactor. Verified as one: for the four fields whose branches moved, plus two that did not and the JWT TTL, all 63 combinations of stored value (nil/false/true) against input value (absent/false/true) produce byte- identical resolved values and `updated` verdicts before and after. --- client/internal/profilemanager/config.go | 142 +++++++++++++---------- 1 file changed, 83 insertions(+), 59 deletions(-) diff --git a/client/internal/profilemanager/config.go b/client/internal/profilemanager/config.go index a4bc0ebbb..09bab0ead 100644 --- a/client/internal/profilemanager/config.go +++ b/client/internal/profilemanager/config.go @@ -307,6 +307,83 @@ func newConfigSkeleton() *Config { } } +// resolveUnsetDefaults is the single place where an optional field that carries +// no value gets one, and the only place that states what each of those defaults +// is. apply() runs it before it compares anything, and that ordering is the +// point: with the values named, every comparison below it diffs values instead +// of presence. +// +// Presence-based comparison is what broke `netbird up` for a client configured +// through the environment. These fields mean "the effective default" when they +// hold nothing — every consumer already reads a nil as the value resolved here, +// the SSH toggles in engine_ssh.go and the network monitor in +// createEngineConfig — so naming them changes nothing about what runs. But +// while they stayed nil, an input restating the default read as a change, and +// since the CLI sends every flag whose value came from an environment variable +// on each `netbird up`, a client with NB_ENABLE_SSH_ROOT=false restated it +// every time and the update-settings gate refused it. +// +// Filling a field in is not a settings change, so a caller measuring change +// must not read the returned bool as one: see WouldChange, which runs a pass +// for this and discards its verdict. +// +// ServerSSHAllowed is the one field whose default depends on the config's age. +// A brand-new profile gets false from newConfigSkeleton, which runs before +// this, so what is resolved here is only the legacy case: a config written by a +// version that had no such field keeps SSH on, for backwards compatibility. +func (config *Config) resolveUnsetDefaults() (updated bool) { + // Fields that default to false on every platform. + for _, field := range []**bool{ + &config.EnableSSHRoot, + &config.EnableSSHSFTP, + &config.EnableSSHLocalPortForwarding, + &config.EnableSSHRemotePortForwarding, + &config.DisableSSHAuth, + // Remote jobs are an explicit opt-in: unlike SSH, a pre-existing config + // with no value defaults to disabled rather than being turned on. + &config.RemoteJobsAllowed, + } { + if *field == nil { + *field = util.False() + updated = true + } + } + + if config.DisableNotifications == nil { + log.Infof("setting notifications to disabled by default") + config.DisableNotifications = util.True() + updated = true + } + + if config.SSHJWTCacheTTL == nil { + // A zero TTL disables the JWT cache, which is what no value meant. + config.SSHJWTCacheTTL = new(int) + updated = true + } + + if config.NetworkMonitor == nil { + // network monitoring is on by default on windows and darwin clients + enabled := runtime.GOOS == "windows" || runtime.GOOS == "darwin" + config.NetworkMonitor = &enabled + updated = true + } + + if config.ServerSSHAllowed == nil { + if runtime.GOOS == "android" { + // default to disabled SSH on Android for security + log.Infof("setting SSH server to false by default on Android") + config.ServerSSHAllowed = util.False() + } else { + // enables SSH for configs from old versions to preserve backwards compatibility + log.Infof("falling back to enabled SSH server for pre-existing configuration") + config.ServerSSHAllowed = util.True() + } + updated = true + } + + return updated +} + // 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 @@ -379,42 +456,12 @@ func (config *Config) apply(input ConfigInput) (updated bool, err error) { } } - // Fields whose nil means "the effective default" rather than "no opinion": - // every consumer already reads a nil as the value resolved here — the SSH - // toggles in engine_ssh.go, the network monitor in createEngineConfig — so - // naming it changes nothing about what runs. - // - // Resolving them up front is what lets the comparisons below diff values - // instead of presence. While they stayed nil, an input restating the - // default read as a change, and since the CLI sends every flag set through - // an environment variable on each `netbird up`, a client configured with - // NB_ENABLE_SSH_ROOT=false restated it every time and the update-settings - // gate refused the restatement. - for _, field := range []**bool{ - &config.EnableSSHRoot, - &config.EnableSSHSFTP, - &config.EnableSSHLocalPortForwarding, - &config.EnableSSHRemotePortForwarding, - &config.DisableSSHAuth, - } { - if *field == nil { - *field = util.False() - updated = true - } - } - - if config.SSHJWTCacheTTL == nil { - // A zero TTL disables the JWT cache, which is what no value meant. - config.SSHJWTCacheTTL = new(int) + // Every optional field gets its value here, before anything below compares + // one. See resolveUnsetDefaults for why that ordering is the point. + if config.resolveUnsetDefaults() { updated = true } - if config.NetworkMonitor == nil { - // network monitoring is on by default on windows and darwin clients - enabled := runtime.GOOS == "windows" || runtime.GOOS == "darwin" - config.NetworkMonitor = &enabled - updated = true - } if config.ManagementURL == nil { log.Infof("using default Management URL %s", DefaultManagementURL) config.ManagementURL, err = parseURL("Management URL", DefaultManagementURL) @@ -557,7 +604,7 @@ func (config *Config) apply(input ConfigInput) (updated bool, err error) { updated = true } - if input.ServerSSHAllowed != nil && (config.ServerSSHAllowed == nil || *input.ServerSSHAllowed != *config.ServerSSHAllowed) { + if input.ServerSSHAllowed != nil && *input.ServerSSHAllowed != *config.ServerSSHAllowed { if *input.ServerSSHAllowed { log.Infof("enabling SSH server") } else { @@ -565,20 +612,9 @@ func (config *Config) apply(input ConfigInput) (updated bool, err error) { } config.ServerSSHAllowed = input.ServerSSHAllowed updated = true - } else if config.ServerSSHAllowed == nil { - if runtime.GOOS == "android" { - // default to disabled SSH on Android for security - log.Infof("setting SSH server to false by default on Android") - config.ServerSSHAllowed = util.False() - } else { - // enables SSH for configs from old versions to preserve backwards compatibility - log.Infof("falling back to enabled SSH server for pre-existing configuration") - config.ServerSSHAllowed = util.True() - } - updated = true } - if input.RemoteJobsAllowed != nil && (config.RemoteJobsAllowed == nil || *input.RemoteJobsAllowed != *config.RemoteJobsAllowed) { + if input.RemoteJobsAllowed != nil && *input.RemoteJobsAllowed != *config.RemoteJobsAllowed { if *input.RemoteJobsAllowed { log.Infof("enabling remote jobs") } else { @@ -586,11 +622,6 @@ func (config *Config) apply(input ConfigInput) (updated bool, err error) { } config.RemoteJobsAllowed = input.RemoteJobsAllowed updated = true - } else if config.RemoteJobsAllowed == nil { - // Remote jobs are an explicit opt-in: unlike SSH, a pre-existing config - // with no value defaults to disabled rather than being turned on. - config.RemoteJobsAllowed = util.False() - updated = true } if input.EnableSSHRoot != nil && *input.EnableSSHRoot != *config.EnableSSHRoot { @@ -735,7 +766,7 @@ func (config *Config) apply(input ConfigInput) (updated bool, err error) { updated = true } - if input.DisableNotifications != nil && (config.DisableNotifications == nil || *input.DisableNotifications != *config.DisableNotifications) { + if input.DisableNotifications != nil && *input.DisableNotifications != *config.DisableNotifications { if *input.DisableNotifications { log.Infof("disabling notifications") } else { @@ -745,13 +776,6 @@ func (config *Config) apply(input ConfigInput) (updated bool, err error) { updated = true } - if config.DisableNotifications == nil { - disabled := true - config.DisableNotifications = &disabled - log.Infof("setting notifications to disabled by default") - updated = true - } - // Compared, not just assigned: restating the path a config already holds // changes nothing, and reporting it as an update makes a caller that // re-sends its own configuration look like one asking to change it.