diff --git a/client/cmd/up.go b/client/cmd/up.go index 2e53224df..ab2d82acf 100644 --- a/client/cmd/up.go +++ b/client/cmd/up.go @@ -353,7 +353,10 @@ func runInDaemonMode(ctx context.Context, cmd *cobra.Command, pm *profilemanager req := setupSetConfigReq(customDNSAddressConverted, cmd, activeProf.ID.String(), username.Username) if _, err := client.SetConfig(ctx, req); err != nil { if st, ok := gstatus.FromError(err); ok && st.Code() == codes.Unavailable { - log.Warnf("setConfig method is not available in the daemon: %s", st.Message()) + // The daemon refused the settings update, it did not lack the + // method: reporting the latter sent people looking for a version + // mismatch that was not there. + log.Warnf("the daemon refused the settings update: %s", st.Message()) } else { return daemonCallError("call service setConfig method", err) } diff --git a/client/internal/profilemanager/config.go b/client/internal/profilemanager/config.go index 10c1758d1..c8b5c16b2 100644 --- a/client/internal/profilemanager/config.go +++ b/client/internal/profilemanager/config.go @@ -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) } diff --git a/client/internal/profilemanager/config_would_change_test.go b/client/internal/profilemanager/config_would_change_test.go new file mode 100644 index 000000000..243559bd0 --- /dev/null +++ b/client/internal/profilemanager/config_would_change_test.go @@ -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) +} diff --git a/client/server/login_overrides_test.go b/client/server/login_overrides_test.go index 5a2298764..790df1fe7 100644 --- a/client/server/login_overrides_test.go +++ b/client/server/login_overrides_test.go @@ -7,6 +7,7 @@ import ( "github.com/stretchr/testify/require" "github.com/netbirdio/netbird/client/internal/profilemanager" + "github.com/netbirdio/netbird/client/proto" ) func TestPersistLoginOverrides(t *testing.T) { @@ -80,7 +81,10 @@ func TestPersistLoginOverrides(t *testing.T) { require.NoError(t, err, "seed config") activeProf := &profilemanager.ActiveProfileState{ID: "default"} - err = persistLoginOverrides(activeProf, tt.newMgmtURL, tt.newPSK) + err = persistLoginOverrides(activeProf, &proto.LoginRequest{ + ManagementUrl: tt.newMgmtURL, + OptionalPreSharedKey: tt.newPSK, + }) require.NoError(t, err, "persistLoginOverrides") cfg, err := profilemanager.ReadConfig(profilemanager.DefaultConfigPath) diff --git a/client/server/mdm.go b/client/server/mdm.go index b41e2b590..8ef1ab5e1 100644 --- a/client/server/mdm.go +++ b/client/server/mdm.go @@ -325,92 +325,6 @@ func mdmManagedFieldConflicts(msg *proto.SetConfigRequest, policy *mdm.Policy) [ }) } -// setConfigRequestHasConfigOverrides reports whether the SetConfigRequest -// carries ANY field that would actually mutate the persisted config. -// The CLI builds a SetConfigRequest unconditionally on every -// `netbird up` (see setupSetConfigReq in cmd/up.go) — a plain -// `netbird up` produces a request with every field at its zero value; -// the gate must skip such no-op invocations or it would always fire -// even when the user did not pass any --flag. Returns false on a nil -// msg; true when any management/admin URL, PSK, DNS/NAT list+clean -// flag, interface/port/MTU, or any optional bool/duration field is set. -func setConfigRequestHasConfigOverrides(msg *proto.SetConfigRequest) bool { - if msg == nil { - return false - } - return msg.ManagementUrl != "" || - msg.AdminURL != "" || - msg.OptionalPreSharedKey != nil || - len(msg.CustomDNSAddress) > 0 || - len(msg.NatExternalIPs) > 0 || msg.CleanNATExternalIPs || - len(msg.ExtraIFaceBlacklist) > 0 || - len(msg.DnsLabels) > 0 || msg.CleanDNSLabels || - msg.DnsRouteInterval != nil || - msg.RosenpassEnabled != nil || - msg.RosenpassPermissive != nil || - msg.InterfaceName != nil || - msg.WireguardPort != nil || - msg.Mtu != nil || - msg.DisableAutoConnect != nil || - msg.ServerSSHAllowed != nil || - msg.RemoteJobsAllowed != nil || - msg.NetworkMonitor != nil || - msg.DisableClientRoutes != nil || - msg.DisableServerRoutes != nil || - msg.DisableDns != nil || - msg.DisableFirewall != nil || - msg.BlockLanAccess != nil || - msg.DisableNotifications != nil || - msg.BlockInbound != nil || - msg.DisableIpv6 != nil || - msg.EnableSSHRoot != nil || - msg.EnableSSHSFTP != nil || - msg.EnableSSHLocalPortForwarding != nil || - msg.EnableSSHRemotePortForwarding != nil || - msg.DisableSSHAuth != nil || - msg.SshJWTCacheTTL != nil || - msg.EnableLocalMetrics != nil || - msg.LocalMetricsAddress != nil -} - -// loginRequestHasConfigOverrides reports whether the LoginRequest -// carries ANY field that would mutate persisted daemon configuration -// (as opposed to pure-auth fields like setupKey, hostname, hint, -// profileName, username). Used by the Login handler to decide whether -// the `--disable-update-settings` / MDM gates must run: a re-auth that -// changes nothing about the configuration is always allowed. -func loginRequestHasConfigOverrides(msg *proto.LoginRequest) bool { - if msg == nil { - return false - } - return msg.ManagementUrl != "" || - msg.AdminURL != "" || - msg.PreSharedKey != "" || //nolint:staticcheck // SA1019: legacy proto field still accepted by Login - msg.OptionalPreSharedKey != nil || - len(msg.CustomDNSAddress) > 0 || - len(msg.NatExternalIPs) > 0 || msg.CleanNATExternalIPs || - msg.RosenpassEnabled != nil || - msg.InterfaceName != nil || - msg.WireguardPort != nil || - msg.DisableAutoConnect != nil || - msg.ServerSSHAllowed != nil || - msg.RemoteJobsAllowed != nil || - msg.RosenpassPermissive != nil || - len(msg.ExtraIFaceBlacklist) > 0 || - msg.NetworkMonitor != nil || - msg.DnsRouteInterval != nil || - msg.DisableClientRoutes != nil || - msg.DisableServerRoutes != nil || - msg.DisableDns != nil || - msg.DisableFirewall != nil || - msg.BlockLanAccess != nil || - msg.DisableNotifications != nil || - len(msg.DnsLabels) > 0 || msg.CleanDNSLabels || - msg.BlockInbound != nil || - msg.EnableLocalMetrics != nil || - msg.LocalMetricsAddress != nil -} - // loginRequestMDMConflicts mirrors mdmManagedFieldConflicts but for the // LoginRequest surface. Same value-aware semantics: a field set to the // MDM-enforced value is a no-op echo, not a conflict; only a divergent diff --git a/client/server/server.go b/client/server/server.go index a38bbe8ad..dc4f0589a 100644 --- a/client/server/server.go +++ b/client/server/server.go @@ -468,16 +468,27 @@ func (s *Server) SetConfig(callerCtx context.Context, msg *proto.SetConfigReques s.mutex.Lock() defer s.mutex.Unlock() - // Skip the update-settings gate when the request carries no actual - // overrides: the CLI builds a SetConfigRequest unconditionally on - // every `netbird up` (setupSetConfigReq in cmd/up.go), so a plain - // `netbird up` would otherwise always trip the gate and surface a - // misleading "setConfig method is not available" warning, even when - // the user did not pass any config flag. - if setConfigRequestHasConfigOverrides(msg) { - if s.checkUpdateSettingsDisabled() { - return nil, gstatus.Errorf(codes.Unavailable, errUpdateSettingsDisabled) - } + stored, err := s.storedProfileConfig(msg.ProfileName, msg.Username) + if err != nil { + return nil, err + } + + config, err := s.setConfigInputFromRequest(msg) + if err != nil { + return nil, err + } + + // Update-settings gate: refuse the request only when it would actually + // change a persisted setting. The CLI builds a SetConfigRequest + // unconditionally on every `netbird up` (setupSetConfigReq in + // cmd/up.go) and fills it from its flags and environment, so a service + // or container that restates the configuration it already runs with + // must pass the gate. Deciding this on field presence alone refused + // those callers, and — through the identical gate in Login — refused + // their login too, which left a client configured by environment + // (NB_MANAGEMENT_URL and friends) unable to come up at all. + if s.checkUpdateSettingsDisabled() && configChangeRequested(stored, config) { + return nil, gstatus.Errorf(codes.Unavailable, errUpdateSettingsDisabled) } // MDM gate: refuse the whole request if any of its fields is enforced @@ -489,19 +500,10 @@ func (s *Server) SetConfig(callerCtx context.Context, msg *proto.SetConfigReques return nil, err } - stored, err := s.storedProfileConfig(msg.ProfileName, msg.Username) - if err != nil { - return nil, err - } if err := requirePrivilegeForConfigChange(callerCtx, stored, privilegedChangeFromSetConfig(msg)); err != nil { return nil, err } - config, err := s.setConfigInputFromRequest(msg) - if err != nil { - return nil, err - } - updatedConf, err := profilemanager.UpdateConfig(config) if err != nil { log.Errorf("failed to update profile config: %v", err) @@ -617,37 +619,46 @@ func (s *Server) setConfigInputFromRequest(msg *proto.SetConfigRequest) (profile // Login uses setup key to prepare configuration for the daemon. func (s *Server) Login(callerCtx context.Context, msg *proto.LoginRequest) (*proto.LoginResponse, error) { - // Config-override gates. LoginRequest carries the same surface as - // SetConfigRequest (managementUrl, PSK, ssh/rosenpass/port toggles, - // ...), so the same protections must apply. Without these the CLI - // command `netbird up --management-url=X` (which falls through to - // Login when SetConfig is rejected — see cmd/up.go) would silently - // bypass `--disable-update-settings` and any MDM policy. - if loginRequestHasConfigOverrides(msg) { - if s.checkUpdateSettingsDisabled() { - return nil, gstatus.Errorf(codes.Unavailable, errUpdateSettingsDisabled) - } - policy := loadMDMPolicy() - if err := rejectMDMManagedFieldConflicts(loginRequestMDMConflicts(msg, policy)); err != nil { - return nil, err - } - } - activeProf, err := s.profileManager.GetActiveProfileState() if err != nil { log.Errorf("failed to get active profile state: %v", err) return nil, fmt.Errorf("failed to get active profile state: %w", err) } - // Privilege gate: same restrictions as SetConfig, since LoginRequest can carry - // the same fields. It runs before anything here changes daemon state, so a - // refused login neither switches the profile nor cancels a login already in - // progress, and it reads the profile the request targets, which is the one the - // switch below would activate. + // The stored config of the profile this request targets backs all three + // gates below. It is read before anything changes daemon state, so a + // refused login neither switches the profile nor cancels a login already + // in progress, and it is the profile the switch further down would + // activate. stored, err := s.storedLoginConfig(activeProf, msg) if err != nil { return nil, err } + + // Config-override gates. LoginRequest carries the same surface as + // SetConfigRequest (managementUrl, PSK, ssh/rosenpass/port toggles, + // ...), so the same protections must apply. Without these the CLI + // command `netbird up --management-url=X` (which falls through to + // Login when SetConfig is rejected — see cmd/up.go) would silently + // bypass `--disable-update-settings` and any MDM policy. + // + // The update-settings gate is value-aware, as in SetConfig: it looks at + // what a login would actually persist (loginOverridesInput) and refuses + // only a real divergence from the stored config. A login that restates + // the values already on disk changes nothing, so it must go through — + // that is what keeps a re-login, or a container restart carrying + // NB_MANAGEMENT_URL, working with the kill switch on. + if s.checkUpdateSettingsDisabled() && configChangeRequested(stored, loginOverridesInput(msg)) { + return nil, gstatus.Errorf(codes.Unavailable, errUpdateSettingsDisabled) + } + + policy := loadMDMPolicy() + if err := rejectMDMManagedFieldConflicts(loginRequestMDMConflicts(msg, policy)); err != nil { + return nil, err + } + + // Privilege gate: same restrictions as SetConfig, since LoginRequest can carry + // the same fields. if err := requirePrivilegeForConfigChange(callerCtx, stored, privilegedChangeFromLogin(msg)); err != nil { return nil, err } @@ -2644,18 +2655,19 @@ func (s *Server) authorizeAndPrepareLogin(callerCtx context.Context, msg *proto. return nil, nil, fmt.Errorf("active profile state: %w", err) } - if err := persistLoginOverrides(activeProf, msg.ManagementUrl, msg.OptionalPreSharedKey); err != nil { + if err := persistLoginOverrides(activeProf, msg); err != nil { return nil, nil, fmt.Errorf("persist login overrides: %w", err) } return ctx, activeProf, nil } -func persistLoginOverrides(activeProf *profilemanager.ActiveProfileState, managementURL string, preSharedKey *string) error { - if preSharedKey != nil && *preSharedKey == "" { - preSharedKey = nil - } - if managementURL == "" && preSharedKey == nil { +// persistLoginOverrides writes the config fields a login request is allowed to +// carry into the active profile. It shares its input builder with the +// update-settings gate, so the gate judges exactly the fields this writes. +func persistLoginOverrides(activeProf *profilemanager.ActiveProfileState, msg *proto.LoginRequest) error { + input := loginOverridesInput(msg) + if input.ManagementURL == "" && input.PreSharedKey == nil { return nil } @@ -2664,11 +2676,7 @@ func persistLoginOverrides(activeProf *profilemanager.ActiveProfileState, manage return fmt.Errorf("active profile file path: %w", err) } - input := profilemanager.ConfigInput{ - ConfigPath: cfgPath, - ManagementURL: managementURL, - PreSharedKey: preSharedKey, - } + input.ConfigPath = cfgPath if _, err := profilemanager.UpdateOrCreateConfig(input); err != nil { return fmt.Errorf("update config: %w", err) } diff --git a/client/server/update_settings_gate.go b/client/server/update_settings_gate.go new file mode 100644 index 000000000..fc42ba86a --- /dev/null +++ b/client/server/update_settings_gate.go @@ -0,0 +1,59 @@ +package server + +import ( + log "github.com/sirupsen/logrus" + + "github.com/netbirdio/netbird/client/internal/profilemanager" + "github.com/netbirdio/netbird/client/proto" +) + +// configChangeRequested reports whether applying input would move the target +// profile away from the configuration it already persists. It is the decision +// procedure of the update-settings kill switch (--disable-update-settings / +// NB_DISABLE_UPDATE_SETTINGS / the MDM DisableUpdateSettings key): that switch +// forbids *changing* settings, so a request that restates the stored values is +// not a change and must not be refused. +// +// This has to be judged on values, not on field presence. `netbird up` rebuilds +// the whole config surface of SetConfigRequest and LoginRequest from its flags +// and environment on every invocation, so a service or container configured by +// environment restates its own configuration on every start. A presence-based +// gate refused those requests, and because Login carries the same fields it +// refused the login too — leaving such a client unable to come up at all. +// +// A dry run that cannot be evaluated fails closed: the request counts as a +// change, so a malformed field can never open the gate. The error itself is +// reported to the caller by the real update path. +func configChangeRequested(stored *profilemanager.Config, input profilemanager.ConfigInput) bool { + changed, err := stored.WouldChange(input) + if err != nil { + log.Warnf("cannot evaluate the requested config change, treating it as a change: %v", err) + return true + } + return changed +} + +// loginOverridesInput builds the ConfigInput a login request persists. The +// management URL and the pre-shared key are the only config fields the daemon +// applies from a LoginRequest; everything else on that message is either pure +// auth or ignored. An empty pre-shared key is dropped rather than written, so +// a login cannot clear the stored key by omission. +// +// Both the write (persistLoginOverrides) and the update-settings gate go +// through this builder, so the gate can neither refuse a field the write +// ignores nor miss one it applies. +func loginOverridesInput(msg *proto.LoginRequest) profilemanager.ConfigInput { + if msg == nil { + return profilemanager.ConfigInput{} + } + + preSharedKey := msg.OptionalPreSharedKey + if preSharedKey != nil && *preSharedKey == "" { + preSharedKey = nil + } + + return profilemanager.ConfigInput{ + ManagementURL: msg.ManagementUrl, + PreSharedKey: preSharedKey, + } +} diff --git a/client/server/update_settings_gate_test.go b/client/server/update_settings_gate_test.go new file mode 100644 index 000000000..49edc137d --- /dev/null +++ b/client/server/update_settings_gate_test.go @@ -0,0 +1,220 @@ +package server + +import ( + "context" + "path/filepath" + "testing" + + "github.com/stretchr/testify/require" + "google.golang.org/grpc/codes" + gstatus "google.golang.org/grpc/status" + + "github.com/netbirdio/netbird/client/internal" + "github.com/netbirdio/netbird/client/internal/profilemanager" + "github.com/netbirdio/netbird/client/proto" +) + +// The seeded profile of setupServerWithProfile is created with this management +// URL, so a request carrying it restates what the profile already holds. +const storedManagementURL = "https://api.netbird.io:443" + +// A client configured by environment re-sends its whole configuration on every +// `netbird up`: the CLI fills the request from its flags and env regardless of +// what changed. With the update-settings kill switch on, such a request must +// pass — nothing about the configuration moves. +func TestSetConfig_RestatingTheStoredConfigPassesTheGate(t *testing.T) { + s, ctx, profName, username, _ := setupServerWithProfile(t) + s.updateSettingsDisabled = true + + _, err := s.SetConfig(ctx, &proto.SetConfigRequest{ + ProfileName: profName, + Username: username, + ManagementUrl: storedManagementURL, + }) + require.NoError(t, err, "restating the stored management URL is not a settings change") +} + +// The same endpoint written without its default port is the same endpoint. A +// gate that compared raw strings refused NB_MANAGEMENT_URL=https://host, which +// is how the URL is normally spelled. +func TestSetConfig_EquivalentManagementURLPassesTheGate(t *testing.T) { + s, ctx, profName, username, _ := setupServerWithProfile(t) + s.updateSettingsDisabled = true + + _, err := s.SetConfig(ctx, &proto.SetConfigRequest{ + ProfileName: profName, + Username: username, + ManagementUrl: "https://api.netbird.io", + }) + require.NoError(t, err, "an implicit :443 is the same management URL") +} + +// The kill switch still has to do its job: a request that moves a setting is +// refused, and the profile keeps the value it had. +func TestSetConfig_ChangingASettingIsRefused(t *testing.T) { + s, ctx, profName, username, cfgPath := setupServerWithProfile(t) + s.updateSettingsDisabled = true + + _, err := s.SetConfig(ctx, &proto.SetConfigRequest{ + ProfileName: profName, + Username: username, + ManagementUrl: "https://mgmt.elsewhere.example:443", + }) + require.Error(t, err, "moving the management URL is a settings change") + require.Equal(t, codes.Unavailable, gstatus.Code(err), "want the update-settings refusal, got %v", err) + + cfg, err := profilemanager.GetConfig(cfgPath) + require.NoError(t, err) + require.Equal(t, storedManagementURL, cfg.ManagementURL.String(), "the refused request changed the config anyway") +} + +// A field whose requested value differs from the stored one is a change even +// when the rest of the request restates the configuration. +func TestSetConfig_SingleDivergingFieldIsRefused(t *testing.T) { + s, ctx, profName, username, _ := setupServerWithProfile(t) + s.updateSettingsDisabled = true + + rosenpass := true + _, err := s.SetConfig(ctx, &proto.SetConfigRequest{ + ProfileName: profName, + Username: username, + ManagementUrl: storedManagementURL, + RosenpassEnabled: &rosenpass, + }) + require.Error(t, err, "enabling Rosenpass is a settings change") + require.Equal(t, codes.Unavailable, gstatus.Code(err), "want the update-settings refusal, got %v", err) +} + +// With the switch off, the same diverging request goes through: the gate must +// not leak into a daemon that never enabled it. +func TestSetConfig_ChangeAllowedWhenTheSwitchIsOff(t *testing.T) { + s, ctx, profName, username, cfgPath := setupServerWithProfile(t) + + _, err := s.SetConfig(ctx, &proto.SetConfigRequest{ + ProfileName: profName, + Username: username, + ManagementUrl: "https://mgmt.elsewhere.example:443", + }) + require.NoError(t, err) + + cfg, err := profilemanager.GetConfig(cfgPath) + require.NoError(t, err) + require.Equal(t, "https://mgmt.elsewhere.example:443", cfg.ManagementURL.String()) +} + +// Login carries the same config surface as SetConfig, so it is gated the same +// way: a login that would move a protected setting is refused before it can +// touch daemon state. +func TestLogin_ChangingTheManagementURLIsRefused(t *testing.T) { + s, _, _, username, _ := setupServerWithProfile(t) + s.updateSettingsDisabled = true + s.rootCtx = internal.CtxInitState(context.Background()) + + _, err := s.Login(userCtx(), &proto.LoginRequest{ + Username: &username, + ManagementUrl: "https://mgmt.elsewhere.example:443", + }) + require.Error(t, err, "moving the management URL through Login is a settings change") + require.Equal(t, codes.Unavailable, gstatus.Code(err), "want the update-settings refusal, got %v", err) +} + +// seedProfileConfig writes a profile config carrying the given management URL +// and pre-shared key into a temp dir, and returns its path. +func seedProfileConfig(t *testing.T, managementURL, preSharedKey string) string { + t.Helper() + + path := filepath.Join(t.TempDir(), "seeded.json") + _, err := profilemanager.UpdateOrCreateConfig(profilemanager.ConfigInput{ + ConfigPath: path, + ManagementURL: managementURL, + PreSharedKey: &preSharedKey, + }) + require.NoError(t, err, "seed profile config") + return path +} + +// The decision procedure itself, over the fields a login actually persists. +// A login that restates the stored values must not be refused: that is what +// keeps a re-login, or a container restart carrying NB_MANAGEMENT_URL, working +// with the kill switch on. +func TestLoginGateDecision(t *testing.T) { + stored, err := profilemanager.GetConfig(seedProfileConfig(t, storedManagementURL, "stored-key")) + require.NoError(t, err) + + redacted := preSharedKeyRedactedSentinel + empty := "" + sameKey := "stored-key" + otherKey := "other-key" + + tests := []struct { + name string + msg *proto.LoginRequest + wantChanged bool + }{ + { + name: "pure auth carries no config", + msg: &proto.LoginRequest{SetupKey: "ABC"}, + wantChanged: false, + }, + { + name: "stored management URL restated", + msg: &proto.LoginRequest{ManagementUrl: storedManagementURL}, + wantChanged: false, + }, + { + name: "stored management URL without its default port", + msg: &proto.LoginRequest{ManagementUrl: "https://api.netbird.io"}, + wantChanged: false, + }, + { + name: "different management URL", + msg: &proto.LoginRequest{ManagementUrl: "https://mgmt.elsewhere.example:443"}, + wantChanged: true, + }, + { + name: "stored pre-shared key restated", + msg: &proto.LoginRequest{OptionalPreSharedKey: &sameKey}, + wantChanged: false, + }, + { + name: "redacted pre-shared key echoed back", + msg: &proto.LoginRequest{OptionalPreSharedKey: &redacted}, + wantChanged: false, + }, + { + name: "empty pre-shared key is not a request to clear it", + msg: &proto.LoginRequest{OptionalPreSharedKey: &empty}, + wantChanged: false, + }, + { + name: "different pre-shared key", + msg: &proto.LoginRequest{OptionalPreSharedKey: &otherKey}, + wantChanged: true, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + require.Equal(t, tt.wantChanged, configChangeRequested(stored, loginOverridesInput(tt.msg))) + }) + } +} + +// A profile with no config on disk yet is judged against the config the daemon +// would create for it, so a first login that asks for the defaults is not a +// change while one that asks for a different management URL is. +func TestGateDecisionWithoutStoredConfig(t *testing.T) { + require.False(t, configChangeRequested(nil, profilemanager.ConfigInput{}), + "a request carrying nothing cannot change anything") + require.False(t, configChangeRequested(nil, profilemanager.ConfigInput{ManagementURL: profilemanager.DefaultManagementURL}), + "asking for the default management URL is what the daemon would write anyway") + require.True(t, configChangeRequested(nil, profilemanager.ConfigInput{ManagementURL: "https://mgmt.elsewhere.example:443"}), + "asking for a non-default management URL is a change") +} + +// A dry run that cannot be evaluated must fail closed, or a malformed field +// would open the gate. +func TestGateDecisionFailsClosedOnAnInvalidRequest(t *testing.T) { + require.True(t, configChangeRequested(nil, profilemanager.ConfigInput{ManagementURL: "not-a-url"}), + "an unevaluable request must count as a change") +}