diff --git a/client/internal/profilemanager/config.go b/client/internal/profilemanager/config.go index c8b5c16b2..acd8f3110 100644 --- a/client/internal/profilemanager/config.go +++ b/client/internal/profilemanager/config.go @@ -1045,7 +1045,7 @@ func update(input ConfigInput) (*Config, error) { // GetConfig read config file and return with Config and if it was created. Errors out if it does not exist func GetConfig(configPath string) (*Config, error) { - return readConfig(configPath, false) + return readConfig(configPath, false, true) } // UpdateOldManagementURL checks whether client can switch to the new Management URL with port 443 and the management domain. @@ -1134,11 +1134,24 @@ func CreateInMemoryConfig(input ConfigInput) (*Config, error) { // ReadConfig read config file and return with Config. If it is not exists create a new with default values func ReadConfig(configPath string) (*Config, error) { - return readConfig(configPath, true) + return readConfig(configPath, true, true) } -// ReadConfig read config file and return with Config. If it is not exists create a new with default values -func readConfig(configPath string, createIfMissing bool) (*Config, error) { +// PeekConfig reads an existing profile config without writing anything back. +// GetConfig persists the normalization whenever apply() fills in a default, +// which a caller that only inspects the stored settings must not do: the +// daemon's update-settings gate reads the config to decide whether to refuse a +// request, and a refused request has to leave the profile file exactly as it +// found it. Errors out when the config does not exist. +func PeekConfig(configPath string) (*Config, error) { + return readConfig(configPath, false, false) +} + +// readConfig reads the profile config at configPath. createIfMissing generates +// a default config (and writes it out) when the file is absent, rather than +// erroring. persistNormalization writes the config back when apply() had to +// fill in defaults the file was missing; a read-only caller passes false. +func readConfig(configPath string, createIfMissing, persistNormalization bool) (*Config, error) { configExists, err := fileExists(configPath) if err != nil { return nil, fmt.Errorf("failed to check if config file exists: %w", err) @@ -1157,7 +1170,7 @@ func readConfig(configPath string, createIfMissing bool) (*Config, error) { // initialize through apply() without changes if changed, err := config.apply(ConfigInput{}); err != nil { return nil, err - } else if changed { + } else if changed && persistNormalization { if err = WriteOutConfig(configPath, config); 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 243559bd0..0670528a5 100644 --- a/client/internal/profilemanager/config_would_change_test.go +++ b/client/internal/profilemanager/config_would_change_test.go @@ -1,11 +1,13 @@ package profilemanager import ( + "os" "path/filepath" "testing" "github.com/stretchr/testify/require" + "github.com/netbirdio/netbird/client/iface" "github.com/netbirdio/netbird/shared/management/domain" ) @@ -99,3 +101,35 @@ func TestWouldChangeReportsAnInvalidInput(t *testing.T) { _, err := cfg.WouldChange(ConfigInput{ManagementURL: "not-a-url"}) require.Error(t, err) } + +// GetConfig persists the normalization it performs; PeekConfig must not, so a +// caller that only inspects the stored settings leaves the file alone. +func TestPeekConfigDoesNotWriteBack(t *testing.T) { + // A config file missing a field apply() fills in (MTU) is what makes the + // normalization write fire. + denormalized := []byte(`{"WgIface":"wt0"}`) + + peekPath := filepath.Join(t.TempDir(), "peek.json") + require.NoError(t, os.WriteFile(peekPath, denormalized, 0o600)) + before, err := os.ReadFile(peekPath) + require.NoError(t, err) + + cfg, err := PeekConfig(peekPath) + require.NoError(t, err) + require.Equal(t, uint16(iface.DefaultMTU), cfg.MTU, "the returned config is still normalized in memory") + + after, err := os.ReadFile(peekPath) + require.NoError(t, err) + require.Equal(t, string(before), string(after), "PeekConfig rewrote the config file") + + // Same file through GetConfig, which is expected to persist it. + getPath := filepath.Join(t.TempDir(), "get.json") + require.NoError(t, os.WriteFile(getPath, denormalized, 0o600)) + + _, err = GetConfig(getPath) + require.NoError(t, err) + + persisted, err := os.ReadFile(getPath) + require.NoError(t, err) + require.NotEqual(t, string(denormalized), string(persisted), "GetConfig is the variant that normalizes on disk") +} diff --git a/client/server/server.go b/client/server/server.go index dc4f0589a..d1dafa518 100644 --- a/client/server/server.go +++ b/client/server/server.go @@ -1161,6 +1161,11 @@ func (s *Server) storedLoginConfig(activeProf *profilemanager.ActiveProfileState // storedConfigAtPath reads a profile config file, yielding nil when it does not // exist yet. +// +// It peeks rather than reads: every caller here feeds a gate that may refuse +// the request, and profilemanager.GetConfig writes the config back whenever it +// has to fill in a default the file was missing. A refused request must leave +// the profile file exactly as it found it. func (s *Server) storedConfigAtPath(path string) (*profilemanager.Config, error) { if _, err := os.Stat(path); err != nil { if os.IsNotExist(err) { @@ -1169,7 +1174,7 @@ func (s *Server) storedConfigAtPath(path string) (*profilemanager.Config, error) return nil, fmt.Errorf("stat profile config: %w", err) } - cfg, err := profilemanager.GetConfig(path) + cfg, err := profilemanager.PeekConfig(path) if err != nil { return nil, fmt.Errorf("read profile config: %w", err) } diff --git a/client/server/update_settings_gate_test.go b/client/server/update_settings_gate_test.go index 49edc137d..b9ca4623a 100644 --- a/client/server/update_settings_gate_test.go +++ b/client/server/update_settings_gate_test.go @@ -2,6 +2,7 @@ package server import ( "context" + "os" "path/filepath" "testing" @@ -218,3 +219,28 @@ func TestGateDecisionFailsClosedOnAnInvalidRequest(t *testing.T) { require.True(t, configChangeRequested(nil, profilemanager.ConfigInput{ManagementURL: "not-a-url"}), "an unevaluable request must count as a change") } + +// The gate reads the stored config to decide, and reading it must not write it: +// a refused request has to leave the profile file byte-for-byte as it was. +// A config file missing a field the config layer fills in (MTU, here) is what +// makes the normalization write fire. +func TestSetConfig_RefusedRequestLeavesTheConfigFileUntouched(t *testing.T) { + s, ctx, profName, username, cfgPath := setupServerWithProfile(t) + s.updateSettingsDisabled = true + + require.NoError(t, os.WriteFile(cfgPath, []byte(`{"WgIface":"wt0"}`), 0o600)) + before, err := os.ReadFile(cfgPath) + require.NoError(t, err) + + _, err = s.SetConfig(ctx, &proto.SetConfigRequest{ + ProfileName: profName, + Username: username, + ManagementUrl: "https://mgmt.elsewhere.example:443", + }) + require.Error(t, err) + require.Equal(t, codes.Unavailable, gstatus.Code(err), "want the update-settings refusal, got %v", err) + + after, err := os.ReadFile(cfgPath) + require.NoError(t, err) + require.Equal(t, string(before), string(after), "the refused request rewrote the profile config") +}