diff --git a/client/internal/profilemanager/config.go b/client/internal/profilemanager/config.go index eab5e22da..9ef0873f2 100644 --- a/client/internal/profilemanager/config.go +++ b/client/internal/profilemanager/config.go @@ -378,6 +378,43 @@ func (config *Config) apply(input ConfigInput) (updated bool, err error) { updated = true } } + + // 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) + 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) @@ -482,21 +519,12 @@ func (config *Config) apply(input ConfigInput) (updated bool, err error) { updated = true } - if input.NetworkMonitor != nil && (config.NetworkMonitor == nil || *input.NetworkMonitor != *config.NetworkMonitor) { + if input.NetworkMonitor != nil && *input.NetworkMonitor != *config.NetworkMonitor { log.Infof("switching Network Monitor to %t", *input.NetworkMonitor) config.NetworkMonitor = input.NetworkMonitor updated = true } - if config.NetworkMonitor == nil { - // enable network monitoring by default on windows and darwin clients - if runtime.GOOS == "windows" || runtime.GOOS == "darwin" { - enabled := true - config.NetworkMonitor = &enabled - updated = true - } - } - if input.CustomDNSAddress != nil && string(input.CustomDNSAddress) != config.CustomDNSAddress { log.Infof("updating custom DNS address %#v (old value %#v)", string(input.CustomDNSAddress), config.CustomDNSAddress) @@ -565,7 +593,7 @@ func (config *Config) apply(input ConfigInput) (updated bool, err error) { updated = true } - if input.EnableSSHRoot != nil && (config.EnableSSHRoot == nil || *input.EnableSSHRoot != *config.EnableSSHRoot) { + if input.EnableSSHRoot != nil && *input.EnableSSHRoot != *config.EnableSSHRoot { if *input.EnableSSHRoot { log.Infof("enabling SSH root login") } else { @@ -575,7 +603,7 @@ func (config *Config) apply(input ConfigInput) (updated bool, err error) { updated = true } - if input.EnableSSHSFTP != nil && (config.EnableSSHSFTP == nil || *input.EnableSSHSFTP != *config.EnableSSHSFTP) { + if input.EnableSSHSFTP != nil && *input.EnableSSHSFTP != *config.EnableSSHSFTP { if *input.EnableSSHSFTP { log.Infof("enabling SSH SFTP subsystem") } else { @@ -585,7 +613,7 @@ func (config *Config) apply(input ConfigInput) (updated bool, err error) { updated = true } - if input.EnableSSHLocalPortForwarding != nil && (config.EnableSSHLocalPortForwarding == nil || *input.EnableSSHLocalPortForwarding != *config.EnableSSHLocalPortForwarding) { + if input.EnableSSHLocalPortForwarding != nil && *input.EnableSSHLocalPortForwarding != *config.EnableSSHLocalPortForwarding { if *input.EnableSSHLocalPortForwarding { log.Infof("enabling SSH local port forwarding") } else { @@ -595,7 +623,7 @@ func (config *Config) apply(input ConfigInput) (updated bool, err error) { updated = true } - if input.EnableSSHRemotePortForwarding != nil && (config.EnableSSHRemotePortForwarding == nil || *input.EnableSSHRemotePortForwarding != *config.EnableSSHRemotePortForwarding) { + if input.EnableSSHRemotePortForwarding != nil && *input.EnableSSHRemotePortForwarding != *config.EnableSSHRemotePortForwarding { if *input.EnableSSHRemotePortForwarding { log.Infof("enabling SSH remote port forwarding") } else { @@ -605,7 +633,7 @@ func (config *Config) apply(input ConfigInput) (updated bool, err error) { updated = true } - if input.DisableSSHAuth != nil && (config.DisableSSHAuth == nil || *input.DisableSSHAuth != *config.DisableSSHAuth) { + if input.DisableSSHAuth != nil && *input.DisableSSHAuth != *config.DisableSSHAuth { if *input.DisableSSHAuth { log.Infof("disabling SSH authentication") } else { @@ -615,7 +643,7 @@ func (config *Config) apply(input ConfigInput) (updated bool, err error) { updated = true } - if input.SSHJWTCacheTTL != nil && (config.SSHJWTCacheTTL == nil || *input.SSHJWTCacheTTL != *config.SSHJWTCacheTTL) { + if input.SSHJWTCacheTTL != nil && *input.SSHJWTCacheTTL != *config.SSHJWTCacheTTL { log.Infof("updating SSH JWT cache TTL to %d seconds", *input.SSHJWTCacheTTL) config.SSHJWTCacheTTL = input.SSHJWTCacheTTL updated = true diff --git a/client/internal/profilemanager/config_would_change_test.go b/client/internal/profilemanager/config_would_change_test.go index 563e985f9..f0458f0d5 100644 --- a/client/internal/profilemanager/config_would_change_test.go +++ b/client/internal/profilemanager/config_would_change_test.go @@ -1,8 +1,10 @@ package profilemanager import ( + "encoding/json" "os" "path/filepath" + "runtime" "testing" "github.com/stretchr/testify/require" @@ -26,6 +28,8 @@ func seededConfig(t *testing.T) *Config { func strPointer(s string) *string { return &s } +func intPtr(i int) *int { return &i } + func TestWouldChange(t *testing.T) { tests := []struct { name string @@ -366,6 +370,84 @@ func TestAdminURLPathIsPartOfTheIdentity(t *testing.T) { require.Equal(t, "https://app.example.com:443/other", updated.AdminURL.String(), "the new panel path was not persisted") } +// unsetOnDisk rewrites the stored config so the named fields carry a JSON null, +// which is how a profile that was never asked about them looks on disk. +func unsetOnDisk(t *testing.T, path string, fields ...string) { + t.Helper() + + raw, err := os.ReadFile(path) + require.NoError(t, err) + + var stored map[string]json.RawMessage + require.NoError(t, json.Unmarshal(raw, &stored)) + + for _, field := range fields { + _, present := stored[field] + require.True(t, present, "%s is not a field of the stored config", field) + stored[field] = json.RawMessage("null") + } + + rewritten, err := json.Marshal(stored) + require.NoError(t, err) + require.NoError(t, os.WriteFile(path, rewritten, 0600)) +} + +// Seven fields mean "the effective default" when they hold no value, and the +// config a plain login writes leaves every one of them unset. Restating that +// default is asking for no change — and the CLI restates it on every `netbird +// up`, because a flag set through an environment variable is a flag pflag +// reports as Changed. Judging those restatements as changes made the +// update-settings gate refuse `netbird up` outright for a client configured +// through the environment, which is the shape of a Kubernetes deployment. +func TestWouldChangeIgnoresRestatedDefaultsOfUnsetFields(t *testing.T) { + networkMonitorDefault := runtime.GOOS == "windows" || runtime.GOOS == "darwin" + + tests := []struct { + field string + theDefault ConfigInput + theOtherWay ConfigInput + }{ + {"EnableSSHRoot", + ConfigInput{EnableSSHRoot: boolPtr(false)}, ConfigInput{EnableSSHRoot: boolPtr(true)}}, + {"EnableSSHSFTP", + ConfigInput{EnableSSHSFTP: boolPtr(false)}, ConfigInput{EnableSSHSFTP: boolPtr(true)}}, + {"EnableSSHLocalPortForwarding", + ConfigInput{EnableSSHLocalPortForwarding: boolPtr(false)}, ConfigInput{EnableSSHLocalPortForwarding: boolPtr(true)}}, + {"EnableSSHRemotePortForwarding", + ConfigInput{EnableSSHRemotePortForwarding: boolPtr(false)}, ConfigInput{EnableSSHRemotePortForwarding: boolPtr(true)}}, + {"DisableSSHAuth", + ConfigInput{DisableSSHAuth: boolPtr(false)}, ConfigInput{DisableSSHAuth: boolPtr(true)}}, + {"SSHJWTCacheTTL", + ConfigInput{SSHJWTCacheTTL: intPtr(0)}, ConfigInput{SSHJWTCacheTTL: intPtr(300)}}, + {"NetworkMonitor", + ConfigInput{NetworkMonitor: boolPtr(networkMonitorDefault)}, ConfigInput{NetworkMonitor: boolPtr(!networkMonitorDefault)}}, + } + + for _, tt := range tests { + t.Run(tt.field, func(t *testing.T) { + path := filepath.Join(t.TempDir(), "unset.json") + _, err := UpdateOrCreateConfig(ConfigInput{ + ConfigPath: path, + ManagementURL: "https://api.netbird.io:443", + }) + require.NoError(t, err) + unsetOnDisk(t, path, tt.field) + + cfg, err := GetExistingConfig(path) + require.NoError(t, err) + + changed, err := cfg.WouldChange(tt.theDefault) + require.NoError(t, err) + require.False(t, changed, "restating the default of an unset %s was judged a change", tt.field) + + // The gate still has to refuse a request that does ask for something. + changed, err = cfg.WouldChange(tt.theOtherWay) + require.NoError(t, err) + require.True(t, changed, "asking for a non-default %s is a change", tt.field) + }) + } +} + // A zero-padded port addresses the same port. func TestServiceURLPortIsNormalizedNumerically(t *testing.T) { padded, err := ParseServiceURL("padded", "https://mgmt.example.com:0443")