From f3ad28f63a32649f491fead9afbc1078dd74b6f6 Mon Sep 17 00:00:00 2001 From: jnfrati Date: Wed, 2 Sep 2026 17:04:35 +0200 Subject: [PATCH] [management,signal,proxy,relay,misc] Increase test surface for configuration backwards compatibility The shared loader changes how each service reads environment variables, flags and configuration files. These tests assert the behavior main produced for concrete inputs (env names and aliases, empty and invalid values, list parsing, flag precedence, file decoding shapes, key persistence), so every intentional break shows up as a failing test that can be reviewed and either restored or documented. --- combined/cmd/config_test.go | 1217 +++++++++++++++++++ management/cmd/config_test.go | 1794 ++++++++++++++++++++++++++++ proxy/cmd/proxy/cmd/config_test.go | 1054 +++++++++++++++- relay/cmd/config_test.go | 639 ++++++++++ signal/cmd/config_test.go | 555 ++++++++- 5 files changed, 5247 insertions(+), 12 deletions(-) diff --git a/combined/cmd/config_test.go b/combined/cmd/config_test.go index 070416cfc..a94eb8de8 100644 --- a/combined/cmd/config_test.go +++ b/combined/cmd/config_test.go @@ -1,15 +1,21 @@ package cmd import ( + "context" "os" "path/filepath" + "reflect" + "strings" "testing" + log "github.com/sirupsen/logrus" + "github.com/spf13/cobra" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" ) func TestLoadConfigEnvironmentWithoutFile(t *testing.T) { + clearCombinedConfigEnvironment(t) t.Setenv("NB_SERVER_LOGLEVEL", "debug") cfg, err := LoadConfig("") @@ -17,7 +23,164 @@ func TestLoadConfigEnvironmentWithoutFile(t *testing.T) { assert.Equal(t, "debug", cfg.Server.LogLevel, "Environment should override defaults without a file") } +func TestLoadConfigIgnoresEmptyNumericEnvironment(t *testing.T) { + clearCombinedConfigEnvironment(t) + t.Setenv("NB_SERVER_METRICSPORT", "") + + cfg, err := LoadConfig("") + require.NoError(t, err) + assert.Equal(t, DefaultConfig().Server.MetricsPort, cfg.Server.MetricsPort, + "An empty numeric environment variable should retain the previous default") +} + +func TestLoadConfigPreservesLegacyYAMLSemantics(t *testing.T) { + clearCombinedConfigEnvironment(t) + tests := []struct { + name string + contents string + wantErr bool + validate func(*testing.T, *CombinedConfig) + }{ + { + name: "wrong case key is ignored", + contents: `server: + metricsport: 9191 +`, + validate: func(t *testing.T, cfg *CombinedConfig) { + assert.Equal(t, 9090, cfg.Server.MetricsPort, "YAML field names should remain case-sensitive") + }, + }, + { + name: "unknown key is ignored", + contents: `server: + unknownSetting: true +`, + validate: func(t *testing.T, cfg *CombinedConfig) { + assert.Equal(t, DefaultConfig().Server, cfg.Server, "Unknown YAML fields should remain ignored") + }, + }, + { + name: "null scalar retains initialized default", + contents: `server: + metricsPort: null +`, + validate: func(t *testing.T, cfg *CombinedConfig) { + assert.Equal(t, 9090, cfg.Server.MetricsPort, "Null scalar values should retain initialized defaults") + }, + }, + { + name: "empty nested value retains initialized defaults", + contents: `server: + auth: + storage: {} +`, + validate: func(t *testing.T, cfg *CombinedConfig) { + assert.Equal(t, "sqlite3", cfg.Server.Auth.Storage.Type, + "Empty nested values should retain initialized defaults") + }, + }, + { + name: "empty pointer object remains present", + contents: `server: + auth: + owner: {} +`, + validate: func(t *testing.T, cfg *CombinedConfig) { + assert.NotNil(t, cfg.Server.Auth.Owner, "An explicitly configured empty object should remain present") + }, + }, + { + name: "empty sequence remains non nil", + contents: `server: + stunPorts: [] +`, + validate: func(t *testing.T, cfg *CombinedConfig) { + assert.NotNil(t, cfg.Server.StunPorts, "An explicitly configured empty sequence should remain non-nil") + assert.Empty(t, cfg.Server.StunPorts, "An explicitly configured empty sequence should remain empty") + }, + }, + { + name: "null sequence becomes nil", + contents: `server: + stunPorts: null +`, + validate: func(t *testing.T, cfg *CombinedConfig) { + assert.Nil(t, cfg.Server.StunPorts, "A null sequence should retain legacy nil semantics") + }, + }, + { + name: "empty map remains non nil", + contents: `server: + perAccountSupportedSyncMessageVersions: {} +`, + validate: func(t *testing.T, cfg *CombinedConfig) { + assert.NotNil(t, cfg.Server.PerAccountSupportedSyncMessageVersions, + "An explicitly configured empty map should remain non-nil") + }, + }, + { + name: "map key case is retained", + contents: `server: + perAccountSupportedSyncMessageVersions: + AccountA: 1 +`, + validate: func(t *testing.T, cfg *CombinedConfig) { + assert.Equal(t, map[string]int{"AccountA": 1}, cfg.Server.PerAccountSupportedSyncMessageVersions, + "YAML map keys should retain their case") + }, + }, + { + name: "legacy boolean spelling remains accepted", + contents: `server: + disableAnonymousMetrics: yes +`, + validate: func(t *testing.T, cfg *CombinedConfig) { + assert.True(t, cfg.Server.DisableAnonymousMetrics, "Legacy YAML booleans should remain accepted") + }, + }, + } + + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + configPath := writeCombinedConfig(t, "config.yaml", test.contents) + cfg, err := LoadConfig(configPath) + if test.wantErr { + assert.Error(t, err, "Legacy-invalid YAML should remain rejected") + return + } + if !assert.NoError(t, err) { + return + } + test.validate(t, cfg) + }) + } +} + +func TestLoadConfigTreatsEveryLegacyFileExtensionAsYAML(t *testing.T) { + clearCombinedConfigEnvironment(t) + for _, name := range []string{"config.json", "config.toml"} { + t.Run(name, func(t *testing.T) { + configPath := writeCombinedConfig(t, name, `server: + metricsPort: 9191 +`) + + cfg, err := LoadConfig(configPath) + if !assert.NoError(t, err, "Combined configuration files were historically decoded as YAML regardless of extension") { + return + } + assert.Equal(t, 9191, cfg.Server.MetricsPort, "YAML content should load regardless of its file extension") + }) + } +} + +func TestLegacyCombinedConfigFlagRemainsRegistered(t *testing.T) { + flag := rootCmd.PersistentFlags().Lookup("config") + require.NotNil(t, flag, "Legacy config flag should remain registered") + assert.Equal(t, "c", flag.Shorthand, "Legacy config shorthand should remain unchanged") +} + func TestLoadConfigEnvironmentOverridesFileAndDefaults(t *testing.T) { + clearCombinedConfigEnvironment(t) configPath := writeCombinedConfig(t, "config.yaml", ` server: exposedAddress: "https://netbird.example.com" @@ -44,6 +207,7 @@ management: } func TestLoadConfigEnvironmentCreatesOptionalConfig(t *testing.T) { + clearCombinedConfigEnvironment(t) configPath := writeCombinedConfig(t, "config.yaml", ` server: exposedAddress: "https://netbird.example.com" @@ -60,6 +224,7 @@ server: } func TestLoadConfigSupportsYAMLWithoutKnownExtension(t *testing.T) { + clearCombinedConfigEnvironment(t) for _, name := range []string{"config", "config.conf"} { t.Run(name, func(t *testing.T) { configPath := writeCombinedConfig(t, name, ` @@ -76,6 +241,7 @@ server: } func TestLoadConfigSupportsLegacyYAMLBooleans(t *testing.T) { + clearCombinedConfigEnvironment(t) configPath := writeCombinedConfig(t, "config.yaml", ` server: exposedAddress: "https://netbird.example.com" @@ -89,6 +255,7 @@ server: } func TestLoadConfigSupportsTOML(t *testing.T) { + clearCombinedConfigEnvironment(t) configPath := writeCombinedConfig(t, "config.toml", ` [server] exposedAddress = "https://netbird.example.com" @@ -104,6 +271,45 @@ logLevel = "warn" assert.Equal(t, ":443", cfg.Server.ListenAddress, "Defaults should survive decoding") } +func clearCombinedConfigEnvironment(t *testing.T) { + t.Helper() + clearCombinedEnvironmentType(t, reflect.TypeOf(CombinedConfig{}), "", make(map[reflect.Type]bool)) +} + +func clearCombinedEnvironmentType(t *testing.T, configType reflect.Type, prefix string, visiting map[reflect.Type]bool) { + t.Helper() + + for configType.Kind() == reflect.Pointer { + configType = configType.Elem() + } + if configType.Kind() != reflect.Struct || visiting[configType] { + return + } + visiting[configType] = true + defer delete(visiting, configType) + + for i := range configType.NumField() { + field := configType.Field(i) + if !field.IsExported() { + continue + } + key := strings.Split(field.Tag.Get("yaml"), ",")[0] + if key == "-" { + continue + } + if key == "" { + key = field.Name + } + if prefix != "" { + key = prefix + "." + key + } + environmentName := "NB_" + strings.ToUpper(strings.NewReplacer(".", "_", "-", "_").Replace(key)) + t.Setenv(environmentName, "") + require.NoError(t, os.Unsetenv(environmentName)) + clearCombinedEnvironmentType(t, field.Type, key, visiting) + } +} + func writeCombinedConfig(t *testing.T, name, contents string) string { t.Helper() @@ -111,3 +317,1014 @@ func writeCombinedConfig(t *testing.T, name, contents string) string { require.NoError(t, os.WriteFile(configPath, []byte(contents), 0o600)) return configPath } + +// legacyCombinedServerFile is a minimal file that satisfied Validate on main. +const legacyCombinedServerFile = `server: + exposedAddress: "https://netbird.example.com" + authSecret: "file-secret" +` + +func TestLoadConfigPreservesLegacyEnvironmentIsNotAConfigSource(t *testing.T) { + clearCombinedConfigEnvironment(t) + tests := []struct { + name string + contents string + env map[string]string + validate func(*testing.T, *CombinedConfig) + }{ + { + name: "NB_SERVER_LOGLEVEL does not override file", + contents: legacyCombinedServerFile + " logLevel: info\n", + env: map[string]string{"NB_SERVER_LOGLEVEL": "debug"}, + validate: func(t *testing.T, cfg *CombinedConfig) { + assert.Equal(t, "info", cfg.Server.LogLevel, "Legacy loader consulted no environment variables; the file value must win") + assert.Equal(t, "info", cfg.Relay.LogLevel, "Legacy relay log level was inherited from the file value") + assert.Equal(t, "info", cfg.Signal.LogLevel, "Legacy signal log level was inherited from the file value") + assert.Equal(t, "info", cfg.Management.LogLevel, "Legacy management log level was inherited from the file value") + }, + }, + { + name: "NB_SERVER_AUTHSECRET does not override file", + contents: legacyCombinedServerFile, + env: map[string]string{"NB_SERVER_AUTHSECRET": "env-secret"}, + validate: func(t *testing.T, cfg *CombinedConfig) { + assert.Equal(t, "file-secret", cfg.Server.AuthSecret, "Legacy loader consulted no environment variables; the file secret must win") + assert.Equal(t, "file-secret", cfg.Relay.AuthSecret, "Legacy relay secret was copied from the file value") + }, + }, + { + name: "NB_SERVER_METRICSPORT does not override default", + contents: legacyCombinedServerFile, + env: map[string]string{"NB_SERVER_METRICSPORT": "9191"}, + validate: func(t *testing.T, cfg *CombinedConfig) { + assert.Equal(t, 9090, cfg.Server.MetricsPort, "Legacy loader consulted no environment variables; the default must be kept") + }, + }, + { + name: "NB_SERVER_AUTH_ISSUER does not set nested field", + contents: legacyCombinedServerFile, + env: map[string]string{"NB_SERVER_AUTH_ISSUER": "https://idp.example.com"}, + validate: func(t *testing.T, cfg *CombinedConfig) { + assert.Empty(t, cfg.Server.Auth.Issuer, "Legacy loader consulted no environment variables for nested fields") + assert.Empty(t, cfg.Management.Auth.Issuer, "Legacy management auth was not populated from the environment") + }, + }, + { + name: "NB_SERVER_TLS_LETSENCRYPT_DOMAINS does not set list field", + contents: legacyCombinedServerFile, + env: map[string]string{"NB_SERVER_TLS_LETSENCRYPT_DOMAINS": "a.com"}, + validate: func(t *testing.T, cfg *CombinedConfig) { + assert.Nil(t, cfg.Server.TLS.LetsEncrypt.Domains, "Legacy loader consulted no environment variables for list fields") + }, + }, + { + name: "NB_SERVER_LOG_LEVEL with word separator is ignored", + contents: legacyCombinedServerFile, + env: map[string]string{"NB_SERVER_LOG_LEVEL": "debug"}, + validate: func(t *testing.T, cfg *CombinedConfig) { + assert.Equal(t, "info", cfg.Server.LogLevel, "Legacy loader consulted no environment variables") + }, + }, + } + + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + for name, value := range test.env { + t.Setenv(name, value) + } + configPath := writeCombinedConfig(t, "config.yaml", test.contents) + cfg, err := LoadConfig(configPath) + require.NoError(t, err) + test.validate(t, cfg) + }) + } +} + +func TestLoadConfigPreservesLegacyEmptyPathReturnsPlainDefaults(t *testing.T) { + clearCombinedConfigEnvironment(t) + t.Setenv("NB_SERVER_EXPOSEDADDRESS", "https://env.example.com") + t.Setenv("NB_SERVER_AUTHSECRET", "env-secret") + + cfg, err := LoadConfig("") + require.NoError(t, err) + assert.Equal(t, DefaultConfig(), cfg, + "Legacy LoadConfig(\"\") returned DefaultConfig() before consulting anything else, including ApplySimplifiedDefaults") + assert.False(t, cfg.Relay.Enabled, "Legacy empty path never enabled the embedded relay") + assert.False(t, cfg.Signal.Enabled, "Legacy empty path never enabled the embedded signal") + assert.False(t, cfg.Management.Enabled, "Legacy empty path never enabled management") + assert.EqualError(t, cfg.Validate(), "server.exposedAddress is required", + "Legacy empty path produced a config that failed validation because the environment was not consulted") +} + +func TestLoadConfigPreservesLegacyEmptyStringEnvironmentIgnored(t *testing.T) { + clearCombinedConfigEnvironment(t) + tests := []struct { + name string + envName string + contents string + validate func(*testing.T, *CombinedConfig) + }{ + { + name: "empty NB_SERVER_LOGLEVEL keeps log level", + envName: "NB_SERVER_LOGLEVEL", + contents: legacyCombinedServerFile, + validate: func(t *testing.T, cfg *CombinedConfig) { + assert.Equal(t, "info", cfg.Server.LogLevel, "Legacy loader ignored the environment; an empty variable must not blank the log level") + assert.Equal(t, "info", cfg.Relay.LogLevel, "Legacy relay log level inherited info") + assert.Equal(t, "info", cfg.Management.LogLevel, "Legacy management log level inherited info") + }, + }, + { + name: "empty NB_SERVER_EXPOSEDADDRESS keeps file value", + envName: "NB_SERVER_EXPOSEDADDRESS", + contents: legacyCombinedServerFile, + validate: func(t *testing.T, cfg *CombinedConfig) { + assert.Equal(t, "https://netbird.example.com", cfg.Server.ExposedAddress, "Legacy loader ignored the environment; the file exposed address must be kept") + assert.True(t, cfg.Management.Enabled, "Legacy ApplySimplifiedDefaults ran with the file exposed address") + assert.NoError(t, cfg.Validate(), "Legacy config remained valid") + }, + }, + { + name: "empty NB_SERVER_DATADIR keeps default", + envName: "NB_SERVER_DATADIR", + contents: legacyCombinedServerFile, + validate: func(t *testing.T, cfg *CombinedConfig) { + assert.Equal(t, "/var/lib/netbird/", cfg.Server.DataDir, "Legacy loader ignored the environment; the default data dir must be kept") + assert.Equal(t, "/var/lib/netbird/", cfg.Management.DataDir, "Legacy management data dir came from the default") + assert.NoError(t, cfg.Validate(), "Legacy config remained valid") + }, + }, + { + name: "empty NB_SERVER_AUTHSECRET keeps file value", + envName: "NB_SERVER_AUTHSECRET", + contents: legacyCombinedServerFile, + validate: func(t *testing.T, cfg *CombinedConfig) { + assert.Equal(t, "file-secret", cfg.Server.AuthSecret, "Legacy loader ignored the environment; the file secret must be kept") + assert.NoError(t, cfg.Validate(), "Legacy config remained valid") + }, + }, + { + name: "empty NB_SERVER_LISTENADDRESS keeps default", + envName: "NB_SERVER_LISTENADDRESS", + contents: legacyCombinedServerFile, + validate: func(t *testing.T, cfg *CombinedConfig) { + assert.Equal(t, ":443", cfg.Server.ListenAddress, "Legacy loader ignored the environment; the default listen address must be kept") + }, + }, + { + name: "empty NB_SERVER_HEALTHCHECKADDRESS keeps default", + envName: "NB_SERVER_HEALTHCHECKADDRESS", + contents: legacyCombinedServerFile, + validate: func(t *testing.T, cfg *CombinedConfig) { + assert.Equal(t, ":9000", cfg.Server.HealthcheckAddress, "Legacy loader ignored the environment; the default healthcheck address must be kept") + }, + }, + { + name: "empty NB_SERVER_STORE_ENGINE keeps default", + envName: "NB_SERVER_STORE_ENGINE", + contents: legacyCombinedServerFile, + validate: func(t *testing.T, cfg *CombinedConfig) { + assert.Equal(t, "sqlite", cfg.Server.Store.Engine, "Legacy loader ignored the environment; the default store engine must be kept") + assert.Equal(t, "sqlite", cfg.Management.Store.Engine, "Legacy management store engine came from the default") + }, + }, + { + name: "empty NB_SERVER_LOGFILE keeps default", + envName: "NB_SERVER_LOGFILE", + contents: legacyCombinedServerFile, + validate: func(t *testing.T, cfg *CombinedConfig) { + assert.Equal(t, "console", cfg.Server.LogFile, "Legacy loader ignored the environment; the default console log target must be kept") + }, + }, + } + + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + t.Setenv(test.envName, "") + configPath := writeCombinedConfig(t, "config.yaml", test.contents) + cfg, err := LoadConfig(configPath) + require.NoError(t, err) + test.validate(t, cfg) + }) + } +} + +func TestLoadConfigPreservesLegacyUnparsableEnvironmentIgnored(t *testing.T) { + clearCombinedConfigEnvironment(t) + tests := []struct { + name string + envName string + envValue string + validate func(*testing.T, *CombinedConfig) + }{ + {name: "metrics port word", envName: "NB_SERVER_METRICSPORT", envValue: "abc", validate: func(t *testing.T, cfg *CombinedConfig) { + assert.Equal(t, 9090, cfg.Server.MetricsPort, "Legacy loader ignored the environment; default metrics port must be kept") + }}, + {name: "metrics port padded", envName: "NB_SERVER_METRICSPORT", envValue: " 9191 ", validate: func(t *testing.T, cfg *CombinedConfig) { + assert.Equal(t, 9090, cfg.Server.MetricsPort, "Legacy loader ignored the environment; default metrics port must be kept") + }}, + {name: "metrics port float", envName: "NB_SERVER_METRICSPORT", envValue: "9191.0", validate: func(t *testing.T, cfg *CombinedConfig) { + assert.Equal(t, 9090, cfg.Server.MetricsPort, "Legacy loader ignored the environment; default metrics port must be kept") + }}, + {name: "metrics port leading zero", envName: "NB_SERVER_METRICSPORT", envValue: "09090", validate: func(t *testing.T, cfg *CombinedConfig) { + assert.Equal(t, 9090, cfg.Server.MetricsPort, "Legacy loader ignored the environment; default metrics port must be kept") + }}, + {name: "metrics port hex", envName: "NB_SERVER_METRICSPORT", envValue: "0x10", validate: func(t *testing.T, cfg *CombinedConfig) { + assert.Equal(t, 9090, cfg.Server.MetricsPort, "Legacy loader ignored the environment; default metrics port must be kept") + }}, + {name: "negative uint", envName: "NB_SERVER_REVERSEPROXY_TRUSTEDHTTPPROXIESCOUNT", envValue: "-1", validate: func(t *testing.T, cfg *CombinedConfig) { + assert.Equal(t, uint(0), cfg.Server.ReverseProxy.TrustedHTTPProxiesCount, "Legacy loader ignored the environment; default proxy count must be kept") + }}, + {name: "sync version word", envName: "NB_SERVER_SUPPORTEDSYNCMESSAGEVERSIONS", envValue: "x", validate: func(t *testing.T, cfg *CombinedConfig) { + assert.Nil(t, cfg.Server.SupportedSyncMessageVersions, "Legacy loader ignored the environment; sync version pointer must remain nil") + }}, + {name: "boolean word", envName: "NB_SERVER_DISABLEANONYMOUSMETRICS", envValue: "maybe", validate: func(t *testing.T, cfg *CombinedConfig) { + assert.False(t, cfg.Server.DisableAnonymousMetrics, "Legacy loader ignored the environment; default boolean must be kept") + }}, + } + + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + t.Setenv(test.envName, test.envValue) + configPath := writeCombinedConfig(t, "config.yaml", legacyCombinedServerFile) + cfg, err := LoadConfig(configPath) + if !assert.NoError(t, err, "Legacy loader never failed because of an environment variable") { + return + } + test.validate(t, cfg) + assert.NoError(t, cfg.Validate(), "Legacy config remained valid") + }) + } +} + +func TestLoadConfigPreservesLegacyEmptyBooleanEnvironmentIgnored(t *testing.T) { + clearCombinedConfigEnvironment(t) + tests := []struct { + name string + envName string + contents string + value func(*CombinedConfig) bool + }{ + { + name: "disableAnonymousMetrics", + envName: "NB_SERVER_DISABLEANONYMOUSMETRICS", + contents: legacyCombinedServerFile + " disableAnonymousMetrics: true\n", + value: func(cfg *CombinedConfig) bool { + return cfg.Server.DisableAnonymousMetrics && cfg.Management.DisableAnonymousMetrics + }, + }, + { + name: "disableGeoliteUpdate", + envName: "NB_SERVER_DISABLEGEOLITEUPDATE", + contents: legacyCombinedServerFile + " disableGeoliteUpdate: true\n", + value: func(cfg *CombinedConfig) bool { + return cfg.Server.DisableGeoliteUpdate && cfg.Management.DisableGeoliteUpdate + }, + }, + { + name: "tls.letsencrypt.enabled", + envName: "NB_SERVER_TLS_LETSENCRYPT_ENABLED", + contents: legacyCombinedServerFile + " tls:\n letsencrypt:\n enabled: true\n", + value: func(cfg *CombinedConfig) bool { return cfg.Server.TLS.LetsEncrypt.Enabled }, + }, + { + name: "auth.localAuthDisabled", + envName: "NB_SERVER_AUTH_LOCALAUTHDISABLED", + contents: legacyCombinedServerFile + " auth:\n localAuthDisabled: true\n", + value: func(cfg *CombinedConfig) bool { return cfg.Server.Auth.LocalAuthDisabled }, + }, + } + + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + t.Setenv(test.envName, "") + configPath := writeCombinedConfig(t, "config.yaml", test.contents) + cfg, err := LoadConfig(configPath) + require.NoError(t, err) + assert.True(t, test.value(cfg), "Legacy loader ignored the environment; a boolean set to true in the file must stay true") + }) + } +} + +func TestLoadConfigPreservesLegacyBooleanEnvironmentSpellingsIgnored(t *testing.T) { + clearCombinedConfigEnvironment(t) + for _, value := range []string{"1", "0", "t", "f", "T", "F", "TRUE", "true", "True", "FALSE", "yes", "no", "y", "n", "on", "off", "ON", "Y"} { + t.Run("default_"+value, func(t *testing.T) { + t.Setenv("NB_SERVER_DISABLEANONYMOUSMETRICS", value) + configPath := writeCombinedConfig(t, "config.yaml", legacyCombinedServerFile) + cfg, err := LoadConfig(configPath) + require.NoError(t, err) + assert.False(t, cfg.Server.DisableAnonymousMetrics, + "Legacy booleans came only from YAML or the false default; environment spellings were not consulted") + }) + } + for _, value := range []string{"0", "f", "off", "no", "FALSE"} { + t.Run("file_true_"+value, func(t *testing.T) { + t.Setenv("NB_SERVER_DISABLEANONYMOUSMETRICS", value) + configPath := writeCombinedConfig(t, "config.yaml", legacyCombinedServerFile+" disableAnonymousMetrics: true\n") + cfg, err := LoadConfig(configPath) + require.NoError(t, err) + assert.True(t, cfg.Server.DisableAnonymousMetrics, + "Legacy booleans came only from YAML; an environment false spelling must not override the file") + }) + } +} + +func TestLoadConfigPreservesLegacyListEnvironmentIgnored(t *testing.T) { + clearCombinedConfigEnvironment(t) + tests := []struct { + name string + envName string + envValue string + validate func(*testing.T, *CombinedConfig) + }{ + {name: "stun ports csv", envName: "NB_SERVER_STUNPORTS", envValue: "3478,3479", validate: func(t *testing.T, cfg *CombinedConfig) { + assert.Equal(t, []int{3478}, cfg.Server.StunPorts, "Legacy lists were only settable via YAML sequences") + }}, + {name: "stun ports csv with space", envName: "NB_SERVER_STUNPORTS", envValue: "3478, 3479", validate: func(t *testing.T, cfg *CombinedConfig) { + assert.Equal(t, []int{3478}, cfg.Server.StunPorts, "Legacy lists were only settable via YAML sequences") + }}, + {name: "stun ports space separated", envName: "NB_SERVER_STUNPORTS", envValue: "3478 3479", validate: func(t *testing.T, cfg *CombinedConfig) { + assert.Equal(t, []int{3478}, cfg.Server.StunPorts, "Legacy lists were only settable via YAML sequences") + }}, + {name: "stun ports trailing comma", envName: "NB_SERVER_STUNPORTS", envValue: "3478,", validate: func(t *testing.T, cfg *CombinedConfig) { + assert.Equal(t, []int{3478}, cfg.Server.StunPorts, "Legacy lists were only settable via YAML sequences") + }}, + {name: "domains csv with space", envName: "NB_SERVER_TLS_LETSENCRYPT_DOMAINS", envValue: "a.com, b.com", validate: func(t *testing.T, cfg *CombinedConfig) { + assert.Nil(t, cfg.Server.TLS.LetsEncrypt.Domains, "Legacy lists were only settable via YAML sequences") + }}, + {name: "domains trailing comma", envName: "NB_SERVER_TLS_LETSENCRYPT_DOMAINS", envValue: "a.com,", validate: func(t *testing.T, cfg *CombinedConfig) { + assert.Nil(t, cfg.Server.TLS.LetsEncrypt.Domains, "Legacy lists were only settable via YAML sequences") + }}, + {name: "domains quoted", envName: "NB_SERVER_TLS_LETSENCRYPT_DOMAINS", envValue: `"a.com","b.com"`, validate: func(t *testing.T, cfg *CombinedConfig) { + assert.Nil(t, cfg.Server.TLS.LetsEncrypt.Domains, "Legacy lists were only settable via YAML sequences") + }}, + {name: "trusted proxies", envName: "NB_SERVER_REVERSEPROXY_TRUSTEDHTTPPROXIES", envValue: "10.0.0.0/8,192.168.0.0/16", validate: func(t *testing.T, cfg *CombinedConfig) { + assert.Nil(t, cfg.Server.ReverseProxy.TrustedHTTPProxies, "Legacy lists were only settable via YAML sequences") + }}, + {name: "grant types", envName: "NB_SERVER_AUTH_GRANTTYPES", envValue: "authorization_code,refresh_token", validate: func(t *testing.T, cfg *CombinedConfig) { + assert.Nil(t, cfg.Server.Auth.GrantTypes, "Legacy lists were only settable via YAML sequences") + }}, + } + + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + t.Setenv(test.envName, test.envValue) + configPath := writeCombinedConfig(t, "config.yaml", legacyCombinedServerFile) + cfg, err := LoadConfig(configPath) + if !assert.NoError(t, err, "Legacy loader never failed because of an environment variable") { + return + } + test.validate(t, cfg) + assert.NoError(t, cfg.Validate(), "Legacy config remained valid") + }) + } +} + +func TestLoadConfigPreservesLegacyEmptyListEnvironmentIgnored(t *testing.T) { + clearCombinedConfigEnvironment(t) + tests := []struct { + name string + envName string + contents string + validate func(*testing.T, *CombinedConfig) + }{ + {name: "stun ports with exposed address", envName: "NB_SERVER_STUNPORTS", contents: legacyCombinedServerFile, validate: func(t *testing.T, cfg *CombinedConfig) { + assert.Equal(t, []int{3478}, cfg.Server.StunPorts, "Legacy default STUN ports were kept") + }}, + {name: "stun ports without exposed address", envName: "NB_SERVER_STUNPORTS", contents: "server: {}\n", validate: func(t *testing.T, cfg *CombinedConfig) { + assert.Equal(t, []int{3478}, cfg.Server.StunPorts, "Legacy default STUN ports were kept even without exposedAddress") + }}, + {name: "domains", envName: "NB_SERVER_TLS_LETSENCRYPT_DOMAINS", contents: legacyCombinedServerFile, validate: func(t *testing.T, cfg *CombinedConfig) { + assert.Nil(t, cfg.Server.TLS.LetsEncrypt.Domains, "Legacy domains stayed nil when absent from the file") + }}, + {name: "relay addresses", envName: "NB_SERVER_RELAYS_ADDRESSES", contents: legacyCombinedServerFile, validate: func(t *testing.T, cfg *CombinedConfig) { + assert.Nil(t, cfg.Server.Relays.Addresses, "Legacy relay addresses stayed nil when absent from the file") + }}, + {name: "stuns", envName: "NB_SERVER_STUNS", contents: legacyCombinedServerFile, validate: func(t *testing.T, cfg *CombinedConfig) { + assert.Nil(t, cfg.Server.Stuns, "Legacy external STUN list stayed nil when absent from the file") + }}, + } + + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + t.Setenv(test.envName, "") + configPath := writeCombinedConfig(t, "config.yaml", test.contents) + cfg, err := LoadConfig(configPath) + require.NoError(t, err) + test.validate(t, cfg) + }) + } +} + +func TestLoadConfigPreservesLegacyMapEnvironmentIgnored(t *testing.T) { + clearCombinedConfigEnvironment(t) + for _, value := range []string{"", "acc=1", `{"acc":1}`} { + t.Run("value_"+value, func(t *testing.T) { + t.Setenv("NB_SERVER_PERACCOUNTSUPPORTEDSYNCMESSAGEVERSIONS", value) + configPath := writeCombinedConfig(t, "config.yaml", legacyCombinedServerFile) + cfg, err := LoadConfig(configPath) + if !assert.NoError(t, err, "Legacy loader never failed because of an environment variable") { + return + } + assert.Nil(t, cfg.Server.PerAccountSupportedSyncMessageVersions, "Legacy map came only from YAML") + }) + } +} + +func TestLoadConfigPreservesLegacyStunsEnvironmentIgnored(t *testing.T) { + clearCombinedConfigEnvironment(t) + tests := []struct { + name string + envName string + }{ + {name: "stuns list", envName: "NB_SERVER_STUNS"}, + {name: "stuns uri", envName: "NB_SERVER_STUNS_URI"}, + } + + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + t.Setenv(test.envName, "stun:stun.example.com:3478") + configPath := writeCombinedConfig(t, "config.yaml", legacyCombinedServerFile) + cfg, err := LoadConfig(configPath) + if !assert.NoError(t, err, "Legacy loader never failed because of an environment variable") { + return + } + assert.Nil(t, cfg.Server.Stuns, "Legacy external STUN servers were only configurable via YAML") + assert.True(t, cfg.Relay.Stun.Enabled, "Legacy local STUN stayed enabled") + assert.Equal(t, []HostConfig{{URI: "stun:netbird.example.com:3478"}}, cfg.Management.Stuns, + "Legacy clients were pointed at the local STUN server") + }) + } +} + +func TestLoadConfigPreservesLegacyOwnerEnvironmentIgnored(t *testing.T) { + clearCombinedConfigEnvironment(t) + tests := []struct { + name string + env map[string]string + }{ + {name: "email and password", env: map[string]string{"NB_SERVER_AUTH_OWNER_EMAIL": "a@b", "NB_SERVER_AUTH_OWNER_PASSWORD": "hash"}}, + {name: "empty email", env: map[string]string{"NB_SERVER_AUTH_OWNER_EMAIL": ""}}, + } + + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + for name, value := range test.env { + t.Setenv(name, value) + } + configPath := writeCombinedConfig(t, "config.yaml", legacyCombinedServerFile) + cfg, err := LoadConfig(configPath) + require.NoError(t, err) + assert.Nil(t, cfg.Server.Auth.Owner, "Legacy owner was nil unless server.auth.owner was present in YAML") + }) + } +} + +func TestLoadConfigPreservesLegacyEmbeddedTopologyIgnoresEnvironment(t *testing.T) { + clearCombinedConfigEnvironment(t) + + t.Run("relay addresses", func(t *testing.T) { + t.Setenv("NB_SERVER_RELAYS_ADDRESSES", "rels://r1:443,rels://r2:443") + configPath := writeCombinedConfig(t, "config.yaml", legacyCombinedServerFile) + cfg, err := LoadConfig(configPath) + require.NoError(t, err) + assert.True(t, cfg.Relay.Enabled, "Legacy embedded relay stayed enabled because the environment was not consulted") + assert.Equal(t, []string{"rels://netbird.example.com"}, cfg.Management.Relays.Addresses, + "Legacy clients were pointed at the embedded relay") + }) + + t.Run("signal uri", func(t *testing.T) { + t.Setenv("NB_SERVER_SIGNALURI", "https://sig.example.com:443") + configPath := writeCombinedConfig(t, "config.yaml", legacyCombinedServerFile) + cfg, err := LoadConfig(configPath) + require.NoError(t, err) + assert.True(t, cfg.Signal.Enabled, "Legacy embedded signal stayed enabled because the environment was not consulted") + assert.Equal(t, "https://netbird.example.com", cfg.Management.SignalURI, + "Legacy clients were pointed at the embedded signal") + }) +} + +func TestLoadConfigPreservesLegacyStoreEnvironmentIgnored(t *testing.T) { + clearCombinedConfigEnvironment(t) + for _, name := range []string{"NB_STORE_ENGINE_POSTGRES_DSN", "NB_STORE_ENGINE_SQLITE_FILE", "NB_ACTIVITY_EVENT_STORE_ENGINE", "NB_ACTIVITY_EVENT_POSTGRES_DSN", "NB_ACTIVITY_EVENT_SQLITE_FILE"} { + t.Setenv(name, "") + require.NoError(t, os.Unsetenv(name)) + } + + t.Run("store engine and dsn", func(t *testing.T) { + t.Setenv("NB_SERVER_STORE_ENGINE", "postgres") + t.Setenv("NB_SERVER_STORE_DSN", "host=x") + configPath := writeCombinedConfig(t, "config.yaml", legacyCombinedServerFile) + cfg, err := LoadConfig(configPath) + require.NoError(t, err) + assert.Equal(t, StoreConfig{Engine: "sqlite"}, cfg.Server.Store, "Legacy store engine came only from YAML") + assert.Equal(t, StoreConfig{Engine: "sqlite"}, cfg.Management.Store, "Legacy management store came only from YAML") + applyServerStoreEnv(cfg.Server.Store) + assert.Empty(t, os.Getenv("NB_STORE_ENGINE_POSTGRES_DSN"), "Legacy chained NB_STORE_ENGINE_POSTGRES_DSN export was derived from the file only") + }) + + t.Run("store file", func(t *testing.T) { + t.Setenv("NB_SERVER_STORE_FILE", "/db/x.db") + configPath := writeCombinedConfig(t, "config.yaml", legacyCombinedServerFile) + cfg, err := LoadConfig(configPath) + require.NoError(t, err) + assert.Empty(t, cfg.Management.Store.File, "Legacy store file came only from YAML") + applyServerStoreEnv(cfg.Server.Store) + assert.Empty(t, os.Getenv("NB_STORE_ENGINE_SQLITE_FILE"), "Legacy chained NB_STORE_ENGINE_SQLITE_FILE export was derived from the file only") + }) + + t.Run("encryption key", func(t *testing.T) { + t.Setenv("NB_SERVER_STORE_ENCRYPTIONKEY", "k") + configPath := writeCombinedConfig(t, "config.yaml", legacyCombinedServerFile) + cfg, err := LoadConfig(configPath) + require.NoError(t, err) + mgmtConfig, err := cfg.ToManagementConfig() + require.NoError(t, err) + assert.Empty(t, mgmtConfig.DataStoreEncryptionKey, "Legacy encryption key came only from YAML, so one was auto-generated") + }) + + t.Run("activity store engine without dsn", func(t *testing.T) { + t.Setenv("NB_SERVER_ACTIVITYSTORE_ENGINE", "postgres") + configPath := writeCombinedConfig(t, "config.yaml", legacyCombinedServerFile) + cfg, err := LoadConfig(configPath) + require.NoError(t, err) + assert.Empty(t, cfg.Server.ActivityStore.Engine, "Legacy activity store engine came only from YAML") + assert.NoError(t, applyActivityStoreEnv(cfg.Server.ActivityStore), "Legacy startup did not fail on an environment-only activity store engine") + }) + + t.Run("auth store engine without dsn", func(t *testing.T) { + t.Setenv("NB_SERVER_AUTHSTORE_ENGINE", "postgres") + configPath := writeCombinedConfig(t, "config.yaml", legacyCombinedServerFile) + cfg, err := LoadConfig(configPath) + require.NoError(t, err) + assert.Empty(t, cfg.Server.AuthStore.Engine, "Legacy auth store engine came only from YAML") + _, err = cfg.ToManagementConfig() + assert.NoError(t, err, "Legacy management config did not fail on an environment-only auth store engine") + }) +} + +func TestLoadConfigPreservesLegacyAdminCommandsIgnoreEnvironment(t *testing.T) { + clearCombinedConfigEnvironment(t) + for _, name := range []string{"NB_STORE_ENGINE_POSTGRES_DSN", "NB_STORE_ENGINE_SQLITE_FILE"} { + t.Setenv(name, "") + require.NoError(t, os.Unsetenv(name)) + } + + previousConfigPath := configPath + previousLevel := log.GetLevel() + t.Cleanup(func() { + configPath = previousConfigPath + log.SetLevel(previousLevel) + }) + + runAdmin := func(t *testing.T) (*CombinedConfig, error) { + t.Helper() + cmd := &cobra.Command{} + cmd.SetContext(context.Background()) + var loaded *CombinedConfig + err := withAdminConfig(cmd, func(_ context.Context, cfg *CombinedConfig) error { + loaded = cfg + return nil + }) + return loaded, err + } + + t.Run("store and data dir", func(t *testing.T) { + t.Setenv("NB_SERVER_STORE_ENGINE", "postgres") + t.Setenv("NB_SERVER_STORE_DSN", "host=x") + t.Setenv("NB_SERVER_DATADIR", "/other") + configPath = writeCombinedConfig(t, "config.yaml", legacyCombinedServerFile+" dataDir: /srv/netbird\n") + + cfg, err := runAdmin(t) + require.NoError(t, err) + assert.Equal(t, "/srv/netbird", cfg.Management.DataDir, "Legacy admin commands operated on the data dir named in the file") + assert.Equal(t, StoreConfig{Engine: "sqlite"}, cfg.Management.Store, "Legacy admin commands operated on the store named in the file") + assert.Empty(t, os.Getenv("NB_STORE_ENGINE_POSTGRES_DSN"), "Legacy admin commands derived NB_STORE_ENGINE_POSTGRES_DSN from the file only") + }) + + t.Run("malformed numeric environment", func(t *testing.T) { + t.Setenv("NB_SERVER_METRICSPORT", "abc") + configPath = writeCombinedConfig(t, "config.yaml", legacyCombinedServerFile) + + _, err := runAdmin(t) + assert.NoError(t, err, "Legacy admin commands never failed because of an environment variable") + }) +} + +func TestLoadConfigPreservesLegacySectionNamedEnvironmentIgnored(t *testing.T) { + clearCombinedConfigEnvironment(t) + const contents = `server: + exposedAddress: "https://netbird.example.com" + authSecret: "file-secret" + tls: + certFile: /certs/cert.pem + keyFile: /certs/key.pem + store: + engine: postgres + dsn: host=x + auth: + issuer: https://netbird.example.com/oauth2 + owner: + email: owner@example.com + relays: + addresses: + - rels://r1:443 +` + tests := []struct { + name string + envName string + envValue string + }{ + {name: "NB_SERVER empty", envName: "NB_SERVER", envValue: ""}, + {name: "NB_SERVER_STORE", envName: "NB_SERVER_STORE", envValue: "x"}, + {name: "NB_SERVER_TLS empty", envName: "NB_SERVER_TLS", envValue: ""}, + {name: "NB_SERVER_AUTH", envName: "NB_SERVER_AUTH", envValue: "1"}, + {name: "NB_SERVER_AUTH_OWNER", envName: "NB_SERVER_AUTH_OWNER", envValue: "x"}, + {name: "NB_SERVER_RELAYS", envName: "NB_SERVER_RELAYS", envValue: "x"}, + } + + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + t.Setenv(test.envName, test.envValue) + configPath := writeCombinedConfig(t, "config.yaml", contents) + cfg, err := LoadConfig(configPath) + require.NoError(t, err) + assert.Equal(t, "https://netbird.example.com", cfg.Server.ExposedAddress, "Legacy file section was decoded regardless of a section-named environment variable") + assert.Equal(t, "/certs/cert.pem", cfg.Server.TLS.CertFile, "Legacy TLS section was decoded regardless of a section-named environment variable") + assert.Equal(t, StoreConfig{Engine: "postgres", DSN: "host=x"}, cfg.Server.Store, "Legacy store section was decoded regardless of a section-named environment variable") + assert.Equal(t, "https://netbird.example.com/oauth2", cfg.Server.Auth.Issuer, "Legacy auth section was decoded regardless of a section-named environment variable") + if assert.NotNil(t, cfg.Server.Auth.Owner, "Legacy owner section was decoded regardless of a section-named environment variable") { + assert.Equal(t, "owner@example.com", cfg.Server.Auth.Owner.Email) + } + assert.Equal(t, []string{"rels://r1:443"}, cfg.Server.Relays.Addresses, "Legacy relays section was decoded regardless of a section-named environment variable") + assert.NoError(t, cfg.Validate(), "Legacy config remained valid") + }) + } +} + +func TestLegacyCombinedConfigHelpTextRemainsUnchanged(t *testing.T) { + flag := rootCmd.PersistentFlags().Lookup("config") + require.NotNil(t, flag, "Legacy config flag should remain registered") + assert.Equal(t, "path to YAML configuration file (required)", flag.Usage, "Legacy --config help text should remain unchanged") + assert.Contains(t, rootCmd.Long, "Configuration is loaded from a YAML file specified with --config.", + "Legacy command description should remain unchanged") + assert.NotContains(t, rootCmd.Long, "NB_SERVER_LOGLEVEL", + "Legacy command description did not advertise environment overrides") +} + +func TestLoadConfigPreservesLegacyRootAndListItemKeyCaseSensitivity(t *testing.T) { + clearCombinedConfigEnvironment(t) + tests := []struct { + name string + contents string + validate func(*testing.T, *CombinedConfig) + }{ + { + name: "capitalized root key is ignored", + contents: `Server: + metricsPort: 9191 +`, + validate: func(t *testing.T, cfg *CombinedConfig) { + assert.Equal(t, 9090, cfg.Server.MetricsPort, "Legacy YAML root keys were case-sensitive") + }, + }, + { + name: "uppercase root key is ignored", + contents: `SERVER: + exposedAddress: https://netbird.example.com +`, + validate: func(t *testing.T, cfg *CombinedConfig) { + assert.Empty(t, cfg.Server.ExposedAddress, "Legacy YAML root keys were case-sensitive") + assert.EqualError(t, cfg.Validate(), "server.exposedAddress is required", "Legacy startup failed validation") + }, + }, + { + name: "wrong case list item key is ignored", + contents: `server: + stuns: + - URI: stun:x:3478 +`, + validate: func(t *testing.T, cfg *CombinedConfig) { + require.Len(t, cfg.Server.Stuns, 1, "Legacy list item was still created") + assert.Empty(t, cfg.Server.Stuns[0].URI, "Legacy YAML keys inside list items were case-sensitive") + }, + }, + } + + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + configPath := writeCombinedConfig(t, "config.yaml", test.contents) + cfg, err := LoadConfig(configPath) + require.NoError(t, err) + test.validate(t, cfg) + }) + } +} + +func TestLoadConfigPreservesLegacyMixedCaseDuplicateKeyResolution(t *testing.T) { + clearCombinedConfigEnvironment(t) + configPath := writeCombinedConfig(t, "config.yaml", `server: + metricsPort: 1 + MetricsPort: 2 +`) + + for i := 0; i < 25; i++ { + cfg, err := LoadConfig(configPath) + require.NoError(t, err) + if !assert.Equal(t, 1, cfg.Server.MetricsPort, + "Legacy YAML applied only the exactly matching key deterministically on every load") { + return + } + } +} + +func TestLoadConfigTreatsAdditionalLegacyFileExtensionsAsYAML(t *testing.T) { + clearCombinedConfigEnvironment(t) + for _, name := range []string{"config.env", "config.dotenv", "config.ini", "config.properties", "config.props", "config.prop", "config.hcl", "config.tfvars", "config.JSON"} { + t.Run(name, func(t *testing.T) { + configPath := writeCombinedConfig(t, name, `server: + metricsPort: 9191 +`) + + cfg, err := LoadConfig(configPath) + if !assert.NoError(t, err, "Combined configuration files were historically decoded as YAML regardless of extension") { + return + } + assert.Equal(t, 9191, cfg.Server.MetricsPort, "YAML content should load regardless of its file extension") + }) + } +} + +func TestLoadConfigPreservesLegacyStrictYAMLTyping(t *testing.T) { + clearCombinedConfigEnvironment(t) + tests := []struct { + name string + contents string + }{ + {name: "quoted int", contents: "server:\n metricsPort: \"9191\"\n"}, + {name: "str tagged int", contents: "server:\n metricsPort: !!str 9191\n"}, + {name: "quoted uint", contents: "server:\n reverseProxy:\n trustedHTTPProxiesCount: \"3\"\n"}, + {name: "quoted int pointer", contents: "server:\n supportedSyncMessageVersions: \"2\"\n"}, + {name: "quoted map value", contents: "server:\n perAccountSupportedSyncMessageVersions: {abc: \"2\"}\n"}, + {name: "int as bool", contents: "server:\n disableAnonymousMetrics: 1\n"}, + {name: "float as bool", contents: "server:\n disableAnonymousMetrics: 1.0\n"}, + {name: "quoted true as bool", contents: "server:\n disableAnonymousMetrics: \"true\"\n"}, + {name: "quoted zero as bool", contents: "server:\n disableAnonymousMetrics: \"0\"\n"}, + {name: "t as bool", contents: "server:\n disableAnonymousMetrics: t\n"}, + {name: "yEs as bool", contents: "server:\n disableAnonymousMetrics: yEs\n"}, + {name: "scalar into int slice", contents: "server:\n stunPorts: 3479\n"}, + {name: "csv into int slice", contents: "server:\n stunPorts: \"3479,3480\"\n"}, + {name: "quoted element in int slice", contents: "server:\n stunPorts: [3479, \"3480\"]\n"}, + {name: "csv into string slice", contents: "server:\n tls:\n letsencrypt:\n domains: a.com,b.com\n"}, + {name: "empty string into int", contents: "server:\n metricsPort: \"\"\n"}, + {name: "float overflow into int", contents: "server:\n metricsPort: 99999999999999999999\n"}, + {name: "int overflow into int", contents: "server:\n metricsPort: 9223372036854775808\n"}, + {name: "negative into uint", contents: "server:\n reverseProxy:\n trustedHTTPProxiesCount: -1\n"}, + {name: "map into struct slice", contents: "server:\n stuns:\n uri: stun:x:3478\n"}, + {name: "map into int slice", contents: "server:\n stunPorts: {}\n"}, + } + + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + configPath := writeCombinedConfig(t, "config.yaml", test.contents) + _, err := LoadConfig(configPath) + if !assert.Error(t, err, "Legacy yaml.v3 decoding rejected values whose YAML type did not match the Go field") { + return + } + assert.ErrorContains(t, err, "cannot unmarshal", "Legacy error came from yaml.v3 strict typing") + }) + } +} + +func TestLoadConfigPreservesLegacyUnquotedStringScalarText(t *testing.T) { + clearCombinedConfigEnvironment(t) + for _, value := range []string{"0x10", "0755", "0123456789", "1234567890123456789012345", "1.50", "1e3", ".inf", "true", "2001-12-14"} { + t.Run(value, func(t *testing.T) { + configPath := writeCombinedConfig(t, "config.yaml", "server:\n authSecret: "+value+"\n") + cfg, err := LoadConfig(configPath) + if !assert.NoError(t, err, "Legacy yaml.v3 decoded any unquoted scalar into a string field") { + return + } + assert.Equal(t, value, cfg.Server.AuthSecret, "Legacy yaml.v3 kept the original scalar text for string fields") + }) + } +} + +func TestLoadConfigPreservesLegacyLoadErrorWording(t *testing.T) { + clearCombinedConfigEnvironment(t) + + t.Run("missing file", func(t *testing.T) { + _, err := LoadConfig(filepath.Join(t.TempDir(), "missing.yaml")) + require.Error(t, err) + assert.ErrorContains(t, err, "failed to read config file:", "Legacy read errors were prefixed with 'failed to read config file:'") + assert.ErrorContains(t, err, "no such file or directory") + }) + + t.Run("directory", func(t *testing.T) { + _, err := LoadConfig(t.TempDir()) + require.Error(t, err) + assert.ErrorContains(t, err, "failed to read config file:", "Legacy read errors were prefixed with 'failed to read config file:'") + }) + + t.Run("syntax error", func(t *testing.T) { + configPath := writeCombinedConfig(t, "config.yaml", "server:\n\tmetricsPort: 1\n") + _, err := LoadConfig(configPath) + require.Error(t, err) + assert.ErrorContains(t, err, "failed to parse config file:", "Legacy parse errors were prefixed with 'failed to parse config file:'") + assert.ErrorContains(t, err, "found character that cannot start any token") + }) + + t.Run("type error", func(t *testing.T) { + configPath := writeCombinedConfig(t, "config.yaml", "server: foo\n") + _, err := LoadConfig(configPath) + require.Error(t, err) + assert.ErrorContains(t, err, "failed to parse config file:", "Legacy parse errors were prefixed with 'failed to parse config file:'") + assert.ErrorContains(t, err, "cannot unmarshal !!str `foo` into cmd.ServerConfig") + }) +} + +func TestLoadConfigPreservesLegacyEmptyIntegerEnvironmentKeepsFileValue(t *testing.T) { + clearCombinedConfigEnvironment(t) + const contents = legacyCombinedServerFile + ` reverseProxy: + trustedHTTPProxiesCount: 3 + accessLogRetentionDays: 30 + accessLogCleanupIntervalHours: 12 +` + tests := []struct { + name string + envName string + validate func(*testing.T, *CombinedConfig) + }{ + { + name: "empty NB_SERVER_REVERSEPROXY_TRUSTEDHTTPPROXIESCOUNT keeps uint from file", + envName: "NB_SERVER_REVERSEPROXY_TRUSTEDHTTPPROXIESCOUNT", + validate: func(t *testing.T, cfg *CombinedConfig) { + assert.Equal(t, uint(3), cfg.Server.ReverseProxy.TrustedHTTPProxiesCount, + "Legacy loader ignored the environment; an empty variable must not blank the uint proxy count set in the file") + assert.Equal(t, uint(3), cfg.Management.ReverseProxy.TrustedHTTPProxiesCount, + "Legacy management reverse proxy was copied from the file because the proxy count was non-zero") + }, + }, + { + name: "empty NB_SERVER_REVERSEPROXY_ACCESSLOGRETENTIONDAYS keeps int from file", + envName: "NB_SERVER_REVERSEPROXY_ACCESSLOGRETENTIONDAYS", + validate: func(t *testing.T, cfg *CombinedConfig) { + assert.Equal(t, 30, cfg.Server.ReverseProxy.AccessLogRetentionDays, + "Legacy loader ignored the environment; an empty variable must not blank the retention days set in the file") + assert.Equal(t, 30, cfg.Management.ReverseProxy.AccessLogRetentionDays, + "Legacy management reverse proxy carried the 30 day retention from the file") + }, + }, + { + name: "empty NB_SERVER_REVERSEPROXY_ACCESSLOGCLEANUPINTERVALHOURS keeps int from file", + envName: "NB_SERVER_REVERSEPROXY_ACCESSLOGCLEANUPINTERVALHOURS", + validate: func(t *testing.T, cfg *CombinedConfig) { + assert.Equal(t, 12, cfg.Server.ReverseProxy.AccessLogCleanupIntervalHours, + "Legacy loader ignored the environment; an empty variable must not blank the cleanup interval set in the file") + assert.Equal(t, 12, cfg.Management.ReverseProxy.AccessLogCleanupIntervalHours, + "Legacy management reverse proxy carried the 12 hour cleanup interval from the file") + }, + }, + { + name: "empty NB_SERVER_METRICSPORT keeps int from file", + envName: "NB_SERVER_METRICSPORT", + validate: func(t *testing.T, cfg *CombinedConfig) { + assert.Equal(t, 9191, cfg.Server.MetricsPort, + "Legacy loader ignored the environment; an empty variable must not blank the metrics port set in the file") + }, + }, + } + + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + t.Setenv(test.envName, "") + configPath := writeCombinedConfig(t, "config.yaml", contents+" metricsPort: 9191\n") + cfg, err := LoadConfig(configPath) + require.NoError(t, err) + test.validate(t, cfg) + assert.NoError(t, cfg.Validate(), "Legacy config remained valid") + }) + } +} + +func TestLoadConfigPreservesLegacyNullSequenceItemsAreDropped(t *testing.T) { + clearCombinedConfigEnvironment(t) + tests := []struct { + name string + contents string + validate func(*testing.T, *CombinedConfig) + }{ + { + name: "null stuns item", + contents: legacyCombinedServerFile + " stuns:\n - null\n", + validate: func(t *testing.T, cfg *CombinedConfig) { + assert.Empty(t, cfg.Server.Stuns, "Legacy yaml.v3 dropped null sequence items, leaving no external STUN servers") + assert.True(t, cfg.Relay.Stun.Enabled, "Legacy local STUN stayed enabled because no external STUN server survived decoding") + assert.Equal(t, []HostConfig{{URI: "stun:netbird.example.com:3478"}}, cfg.Management.Stuns, + "Legacy clients were pointed at the local STUN server") + }, + }, + { + name: "null relay address item", + contents: legacyCombinedServerFile + " relays:\n addresses:\n - null\n", + validate: func(t *testing.T, cfg *CombinedConfig) { + assert.Empty(t, cfg.Server.Relays.Addresses, "Legacy yaml.v3 dropped null sequence items, leaving no external relay addresses") + assert.True(t, cfg.Relay.Enabled, "Legacy embedded relay stayed enabled because no external relay address survived decoding") + assert.Equal(t, []string{"rels://netbird.example.com"}, cfg.Management.Relays.Addresses, + "Legacy clients were pointed at the embedded relay") + }, + }, + { + name: "null letsencrypt domain item", + contents: legacyCombinedServerFile + " tls:\n letsencrypt:\n enabled: true\n dataDir: /le\n domains:\n - null\n", + validate: func(t *testing.T, cfg *CombinedConfig) { + assert.Empty(t, cfg.Server.TLS.LetsEncrypt.Domains, "Legacy yaml.v3 dropped null sequence items, leaving no Let's Encrypt domains") + assert.False(t, cfg.HasLetsEncrypt(), "Legacy Let's Encrypt was not considered configured without a surviving domain") + }, + }, + { + name: "null stun port item after a valid port", + contents: legacyCombinedServerFile + " stunPorts:\n - 3478\n - null\n", + validate: func(t *testing.T, cfg *CombinedConfig) { + assert.Equal(t, []int{3478}, cfg.Server.StunPorts, "Legacy yaml.v3 dropped the null port item and kept only 3478") + assert.NoError(t, cfg.Validate(), "Legacy config validated because no zero port was retained") + assert.Equal(t, []int{3478}, cfg.Relay.Stun.Ports, "Legacy local STUN listened only on 3478") + }, + }, + } + + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + configPath := writeCombinedConfig(t, "config.yaml", test.contents) + cfg, err := LoadConfig(configPath) + require.NoError(t, err) + test.validate(t, cfg) + }) + } +} + +func TestLoadConfigPreservesLegacyDottedKeysAreLiteralUnknownKeys(t *testing.T) { + clearCombinedConfigEnvironment(t) + tests := []struct { + name string + contents string + validate func(*testing.T, *CombinedConfig) + }{ + { + name: "dotted root key is ignored", + contents: legacyCombinedServerFile + "server.metricsPort: 1\n", + validate: func(t *testing.T, cfg *CombinedConfig) { + assert.Equal(t, 9090, cfg.Server.MetricsPort, + "Legacy yaml.v3 matched keys literally; 'server.metricsPort' was an unknown root key and left the default in place") + }, + }, + { + name: "dotted nested key is ignored", + contents: legacyCombinedServerFile + " tls.certFile: /a\n tls.keyFile: /b\n", + validate: func(t *testing.T, cfg *CombinedConfig) { + assert.Empty(t, cfg.Server.TLS.CertFile, + "Legacy yaml.v3 matched keys literally; 'tls.certFile' was an unknown key under server and did not enable file-based TLS") + assert.Empty(t, cfg.Server.TLS.KeyFile, + "Legacy yaml.v3 matched keys literally; 'tls.keyFile' was an unknown key under server") + assert.False(t, cfg.HasTLSCert(), "Legacy file-based TLS stayed disabled") + }, + }, + { + name: "dotted store key is ignored", + contents: legacyCombinedServerFile + " store.engine: postgres\n", + validate: func(t *testing.T, cfg *CombinedConfig) { + assert.Equal(t, "sqlite", cfg.Server.Store.Engine, + "Legacy yaml.v3 matched keys literally; 'store.engine' was an unknown key under server and kept the sqlite default") + assert.Equal(t, "sqlite", cfg.Management.Store.Engine, "Legacy management store engine came from the default") + }, + }, + } + + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + configPath := writeCombinedConfig(t, "config.yaml", test.contents) + cfg, err := LoadConfig(configPath) + require.NoError(t, err) + test.validate(t, cfg) + assert.NoError(t, cfg.Validate(), "Legacy config remained valid") + }) + } +} + +func TestLoadConfigPreservesLegacyRejectionOfRealTOMLContent(t *testing.T) { + clearCombinedConfigEnvironment(t) + configPath := writeCombinedConfig(t, "config.toml", `[server] +exposedAddress = "https://netbird.example.com" +authSecret = "s" +metricsPort = 9191 +`) + + _, err := LoadConfig(configPath) + if !assert.Error(t, err, "Legacy loader always decoded the file as YAML regardless of the .toml extension; a TOML table header parsed as a YAML sequence and was rejected, so the server did not start") { + return + } + assert.ErrorContains(t, err, "failed to parse config file:", "Legacy parse errors were prefixed with 'failed to parse config file:'") + assert.ErrorContains(t, err, "cannot unmarshal !!seq into cmd.CombinedConfig", + "Legacy yaml.v3 rejected the TOML table header '[server]' as a sequence at the document root") +} diff --git a/management/cmd/config_test.go b/management/cmd/config_test.go index 009ba702f..45f7ce842 100644 --- a/management/cmd/config_test.go +++ b/management/cmd/config_test.go @@ -1,16 +1,25 @@ package cmd import ( + "bytes" + "context" + "encoding/json" "os" "path/filepath" + "reflect" + "strings" "testing" "time" + "github.com/spf13/pflag" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" + + nbconfig "github.com/netbirdio/netbird/management/internals/server/config" ) func TestLoadManagementConfigUsesDefaultDataDirForEmptyValue(t *testing.T) { + clearManagementConfigEnvironment(t) configPath := filepath.Join(t.TempDir(), "management.json") require.NoError(t, os.WriteFile(configPath, []byte(`{"Datadir":""}`), 0o600)) @@ -19,7 +28,352 @@ func TestLoadManagementConfigUsesDefaultDataDirForEmptyValue(t *testing.T) { assert.Equal(t, defaultMgmtDataDir, cfg.Datadir, "Empty legacy values should use the default data directory") } +func TestLoadManagementConfigPreservesExtraConfigKeyCase(t *testing.T) { + clearManagementConfigEnvironment(t) + configPath := filepath.Join(t.TempDir(), "management.json") + require.NoError(t, os.WriteFile(configPath, []byte(`{ + "IdpManagerConfig": { + "ExtraConfig": { + "ServiceAccountKey": "service-account-key", + "CustomerId": "customer-id" + } + } +}`), 0o600)) + + cfg, err := loadManagementConfig(configPath) + require.NoError(t, err) + require.NotNil(t, cfg.IdpManagerConfig, "IDP configuration should be decoded") + assert.Equal(t, map[string]string{ + "ServiceAccountKey": "service-account-key", + "CustomerId": "customer-id", + }, map[string]string(cfg.IdpManagerConfig.ExtraConfig), "IDP-specific keys should retain their case") +} + +func TestLoadManagementConfigPreservesEmptyHTTPConfig(t *testing.T) { + clearManagementConfigEnvironment(t) + configPath := filepath.Join(t.TempDir(), "management.json") + require.NoError(t, os.WriteFile(configPath, []byte(`{"HttpConfig":{}}`), 0o600)) + + cfg, err := loadManagementConfig(configPath) + require.NoError(t, err) + assert.NotNil(t, cfg.HttpConfig, "An explicitly configured empty HTTP section should remain present") +} + +func TestLoadManagementConfigPreservesJSONShapes(t *testing.T) { + clearManagementConfigEnvironment(t) + tests := []struct { + name string + contents string + validate func(*testing.T, *nbconfig.Config) + }{ + { + name: "mixed case struct field", + contents: `{"dAtAdIr":"/mixed-case"}`, + validate: func(t *testing.T, cfg *nbconfig.Config) { + assert.Equal(t, "/mixed-case", cfg.Datadir, "JSON struct fields should remain case-insensitive") + }, + }, + { + name: "empty nested pointer", + contents: `{"IdpManagerConfig":{"ClientConfig":{}}}`, + validate: func(t *testing.T, cfg *nbconfig.Config) { + require.NotNil(t, cfg.IdpManagerConfig, "IDP configuration should remain present") + assert.NotNil(t, cfg.IdpManagerConfig.ClientConfig, + "An explicitly configured empty nested pointer should remain present") + }, + }, + { + name: "empty map", + contents: `{"IdpManagerConfig":{"ExtraConfig":{}}}`, + validate: func(t *testing.T, cfg *nbconfig.Config) { + require.NotNil(t, cfg.IdpManagerConfig, "IDP configuration should remain present") + assert.NotNil(t, cfg.IdpManagerConfig.ExtraConfig, + "An explicitly configured empty map should remain non-nil") + }, + }, + { + name: "empty slice", + contents: `{"Stuns":[]}`, + validate: func(t *testing.T, cfg *nbconfig.Config) { + assert.NotNil(t, cfg.Stuns, "An explicitly configured empty slice should remain non-nil") + assert.Empty(t, cfg.Stuns, "An explicitly configured empty slice should remain empty") + }, + }, + { + name: "empty account map", + contents: `{"PerAccountHighestSupportedSyncMessageVersion":{}}`, + validate: func(t *testing.T, cfg *nbconfig.Config) { + assert.NotNil(t, cfg.PerAccountHighestSupportedSyncMessageVersion, + "An explicitly configured empty account map should remain non-nil") + }, + }, + { + name: "null pointer", + contents: `{"HttpConfig":null}`, + validate: func(t *testing.T, cfg *nbconfig.Config) { + assert.Nil(t, cfg.HttpConfig, "A null pointer should remain nil") + }, + }, + { + name: "unknown field", + contents: `{"Datadir":"/known-data","UnknownSetting":true}`, + validate: func(t *testing.T, cfg *nbconfig.Config) { + assert.Equal(t, "/known-data", cfg.Datadir, "Unknown JSON fields should remain ignored") + }, + }, + } + + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + configPath := filepath.Join(t.TempDir(), "management.json") + require.NoError(t, os.WriteFile(configPath, []byte(test.contents), 0o600)) + + cfg, err := loadManagementConfig(configPath) + require.NoError(t, err) + test.validate(t, cfg) + }) + } +} + +func TestLoadAdminMgmtConfigPreservesLegacyEmptyDataDir(t *testing.T) { + clearManagementConfigEnvironment(t) + configPath := filepath.Join(t.TempDir(), "management.json") + require.NoError(t, os.WriteFile(configPath, []byte(`{}`), 0o600)) + + oldConfigPath := nbconfig.MgmtConfigPath + oldAdminDatadir := adminDatadir + nbconfig.MgmtConfigPath = configPath + adminDatadir = "" + t.Cleanup(func() { + nbconfig.MgmtConfigPath = oldConfigPath + adminDatadir = oldAdminDatadir + }) + + cfg, datadir, err := loadAdminMgmtConfig(context.Background(), false) + require.NoError(t, err) + assert.Empty(t, cfg.Datadir, "Admin commands should retain the legacy empty data directory") + assert.Empty(t, datadir, "Admin commands should retain the legacy empty effective data directory") +} + +func TestEnsureEncryptionKeyDoesNotPersistUnreferencedEnvironmentSecrets(t *testing.T) { + const environmentSecret = "environment-client-secret" + + clearManagementConfigEnvironment(t) + t.Setenv("NB_IDPMANAGERCONFIG_CLIENTCONFIG_CLIENTSECRET", environmentSecret) + configPath := filepath.Join(t.TempDir(), "management.json") + require.NoError(t, os.WriteFile(configPath, []byte(`{ + "IdpManagerConfig": { + "ClientConfig": {} + } +}`), 0o600)) + + cfg, err := loadManagementConfig(configPath) + require.NoError(t, err) + require.NoError(t, EnsureEncryptionKey(context.Background(), configPath, cfg)) + + persistedConfig, err := os.ReadFile(configPath) + require.NoError(t, err) + assert.False(t, bytes.Contains(persistedConfig, []byte(environmentSecret)), + "An environment value absent from the file should not be persisted") +} + +func TestEnsureEncryptionKeyPreservesTemplateExpandedValues(t *testing.T) { + const templateSecret = "template-client-secret" + + clearManagementConfigEnvironment(t) + t.Setenv("MANAGEMENT_TEMPLATE_SECRET", templateSecret) + configPath := filepath.Join(t.TempDir(), "management.json") + require.NoError(t, os.WriteFile(configPath, []byte(`{ + "IdpManagerConfig": { + "ClientConfig": { + "ClientSecret": "{{ .MANAGEMENT_TEMPLATE_SECRET }}" + } + } +}`), 0o600)) + + cfg, err := loadManagementConfig(configPath) + require.NoError(t, err) + require.NoError(t, EnsureEncryptionKey(context.Background(), configPath, cfg)) + + persistedConfig, err := os.ReadFile(configPath) + require.NoError(t, err) + assert.True(t, bytes.Contains(persistedConfig, []byte(templateSecret)), + "Template-expanded values were historically persisted during key generation") +} + +func TestLoadMgmtConfigPreservesLegacyDataDirPrecedence(t *testing.T) { + clearManagementConfigEnvironment(t) + configPath := filepath.Join(t.TempDir(), "management.json") + require.NoError(t, os.WriteFile(configPath, []byte(`{ + "Datadir": "/stale-config-directory", + "DataStoreEncryptionKey": "configured-key", + "HttpConfig": { + "AuthAudience": "test-audience" + } +}`), 0o600)) + + cfg, err := LoadMgmtConfig(context.Background(), configPath, unchangedManagementFlags()) + require.NoError(t, err) + assert.Equal(t, defaultMgmtDataDir, cfg.Datadir, + "The historical command default should continue to override a stale config value") +} + +func TestApplyCommandLineOverridesPreservesLegacyEmptyValues(t *testing.T) { + oldDatadir := mgmtDataDir + oldLetsencryptDomain := mgmtLetsencryptDomain + oldCertFile := certFile + oldCertKey := certKey + t.Cleanup(func() { + mgmtDataDir = oldDatadir + mgmtLetsencryptDomain = oldLetsencryptDomain + certFile = oldCertFile + certKey = oldCertKey + }) + + tests := []struct { + name string + configure func(*testing.T, *pflag.FlagSet) + validate func(*testing.T, *nbconfig.Config) + }{ + { + name: "empty datadir", + configure: func(t *testing.T, flags *pflag.FlagSet) { + mgmtDataDir = "" + require.NoError(t, flags.Set("datadir", "")) + }, + validate: func(t *testing.T, cfg *nbconfig.Config) { + assert.Equal(t, "/file-data", cfg.Datadir, "An empty datadir flag should not clear the file value") + }, + }, + { + name: "empty letsencrypt domain", + configure: func(t *testing.T, flags *pflag.FlagSet) { + mgmtLetsencryptDomain = "" + require.NoError(t, flags.Set("letsencrypt-domain", "")) + }, + validate: func(t *testing.T, cfg *nbconfig.Config) { + assert.Equal(t, "file.example.com", cfg.HttpConfig.LetsEncryptDomain, + "An empty letsencrypt flag should not clear the file value") + }, + }, + { + name: "empty certificate pair", + configure: func(t *testing.T, flags *pflag.FlagSet) { + certFile = "" + certKey = "" + require.NoError(t, flags.Set("cert-file", "")) + require.NoError(t, flags.Set("cert-key", "")) + }, + validate: func(t *testing.T, cfg *nbconfig.Config) { + assert.Equal(t, "/file/tls.crt", cfg.HttpConfig.CertFile, + "An empty certificate pair should not clear the file certificate") + assert.Equal(t, "/file/tls.key", cfg.HttpConfig.CertKey, + "An empty certificate pair should not clear the file key") + }, + }, + } + + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + mgmtDataDir = defaultMgmtDataDir + mgmtLetsencryptDomain = "" + certFile = "" + certKey = "" + flags := unchangedManagementFlags() + test.configure(t, flags) + cfg := &nbconfig.Config{ + Datadir: "/file-data", + HttpConfig: &nbconfig.HttpServerConfig{ + LetsEncryptDomain: "file.example.com", + CertFile: "/file/tls.crt", + CertKey: "/file/tls.key", + }, + } + + ApplyCommandLineOverrides(cfg, flags) + test.validate(t, cfg) + }) + } +} + +func TestManagementPortDefaultsRemainCompatible(t *testing.T) { + tests := []struct { + name string + httpConfig string + flags map[string]string + expected int + }{ + { + name: "no TLS", + httpConfig: `{"AuthAudience":"test-audience"}`, + expected: 80, + }, + { + name: "file-only letsencrypt", + httpConfig: `{"AuthAudience":"test-audience","LetsEncryptDomain":"management.example.com"}`, + expected: 80, + }, + { + name: "file certificate pair", + httpConfig: `{"AuthAudience":"test-audience","CertFile":"/tls.crt","CertKey":"/tls.key"}`, + expected: 443, + }, + { + name: "letsencrypt flag", + httpConfig: `{"AuthAudience":"test-audience"}`, + flags: map[string]string{"letsencrypt-domain": "management.example.com"}, + expected: 443, + }, + { + name: "explicit zero port", + httpConfig: `{"AuthAudience":"test-audience"}`, + flags: map[string]string{"port": "0"}, + expected: 0, + }, + { + name: "explicit nonzero port", + httpConfig: `{"AuthAudience":"test-audience"}`, + flags: map[string]string{"port": "10002"}, + expected: 10002, + }, + } + + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + actual := runManagementPreRun(t, test.httpConfig, test.flags) + assert.Equal(t, test.expected, actual, "Management implicit port selection should retain legacy behavior") + }) + } +} + +func TestLegacyManagementFlagsRemainRegistered(t *testing.T) { + for _, name := range []string{ + "port", + "disable-legacy-port", + "metrics-port", + "datadir", + "config", + "letsencrypt-domain", + "single-account-mode-domain", + "disable-single-account-mode", + "cert-file", + "cert-key", + "disable-anonymous-metrics", + "dns-domain", + idpSignKeyRefreshEnabledFlagName, + "user-delete-from-idp", + "disable-geolite-update", + } { + assert.NotNil(t, mgmtCmd.Flags().Lookup(name), "Legacy Management flag %s should remain registered", name) + } + for _, name := range []string{"log-level", "log-file"} { + assert.NotNil(t, rootCmd.PersistentFlags().Lookup(name), + "Legacy persistent Management flag %s should remain registered", name) + } +} + func TestLoadManagementConfigSources(t *testing.T) { + clearManagementConfigEnvironment(t) t.Setenv("MANAGEMENT_DATA_DIR", "/template-data") t.Setenv("MANAGEMENT_ENCRYPTION_KEY", "template-key") t.Setenv("NB_DATADIR", "/environment-data") @@ -57,3 +411,1443 @@ func TestLoadManagementConfigSources(t *testing.T) { ApplyCommandLineOverrides(cfg, mgmtCmd.Flags()) assert.Equal(t, "/flag-data", cfg.Datadir, "Flags should override environment values") } + +// management-01: environment variables were never read directly by the legacy loader. +func TestLoadManagementConfigPreservesLegacyFileValuesOverEnvironment(t *testing.T) { + const templateAudience = "template-audience" + + clearManagementConfigEnvironment(t) + t.Setenv("MANAGEMENT_TEMPLATE_AUDIENCE", templateAudience) + + tests := []struct { + name string + environment map[string]string + contents string + validate func(*testing.T, *nbconfig.Config) + }{ + { + name: "http audience", + environment: map[string]string{"NB_HTTPCONFIG_AUTHAUDIENCE": "env-aud"}, + contents: `{"HttpConfig":{"AuthAudience":"file-aud"}}`, + validate: func(t *testing.T, cfg *nbconfig.Config) { + require.NotNil(t, cfg.HttpConfig, "HTTP configuration should be decoded") + assert.Equal(t, "file-aud", cfg.HttpConfig.AuthAudience, + "The legacy loader never read NB_ environment variables; the file value must win") + }, + }, + { + name: "relay secret", + environment: map[string]string{"NB_RELAY_SECRET": "env-secret"}, + contents: `{"Relay":{"Addresses":["rel://a"],"Secret":"file-secret"}}`, + validate: func(t *testing.T, cfg *nbconfig.Config) { + require.NotNil(t, cfg.Relay, "Relay configuration should be decoded") + assert.Equal(t, "file-secret", cfg.Relay.Secret, + "The legacy loader never read NB_ environment variables; the file value must win") + }, + }, + { + name: "turn secret", + environment: map[string]string{"NB_TURNCONFIG_SECRET": "env-secret"}, + contents: `{"TURNConfig":{"Secret":"file-secret"}}`, + validate: func(t *testing.T, cfg *nbconfig.Config) { + require.NotNil(t, cfg.TURNConfig, "TURN configuration should be decoded") + assert.Equal(t, "file-secret", cfg.TURNConfig.Secret, + "The legacy loader never read NB_ environment variables; the file value must win") + }, + }, + { + name: "idp client secret", + environment: map[string]string{"NB_IDPMANAGERCONFIG_CLIENTCONFIG_CLIENTSECRET": "env-secret"}, + contents: `{"IdpManagerConfig":{"ClientConfig":{"ClientSecret":"file-secret"}}}`, + validate: func(t *testing.T, cfg *nbconfig.Config) { + require.NotNil(t, cfg.IdpManagerConfig, "IDP configuration should be decoded") + require.NotNil(t, cfg.IdpManagerConfig.ClientConfig, "IDP client configuration should be decoded") + assert.Equal(t, "file-secret", cfg.IdpManagerConfig.ClientConfig.ClientSecret, + "The legacy loader never read NB_ environment variables; the file value must win") + }, + }, + { + name: "sign key refresh", + environment: map[string]string{"NB_HTTPCONFIG_IDPSIGNKEYREFRESHENABLED": "true"}, + contents: `{"HttpConfig":{"IdpSignKeyRefreshEnabled":false}}`, + validate: func(t *testing.T, cfg *nbconfig.Config) { + require.NotNil(t, cfg.HttpConfig, "HTTP configuration should be decoded") + assert.False(t, cfg.HttpConfig.IdpSignKeyRefreshEnabled, + "Only the --idp-sign-key-refresh-enabled flag could toggle the refresh setting outside the file") + }, + }, + { + name: "embedded idp storage dsn", + environment: map[string]string{"NB_EMBEDDEDIDP_STORAGE_CONFIG_DSN": "env-dsn"}, + contents: `{"EmbeddedIdP":{"Enabled":true,"Storage":{"Type":"postgres","Config":{"DSN":"file-dsn"}}}}`, + validate: func(t *testing.T, cfg *nbconfig.Config) { + require.NotNil(t, cfg.EmbeddedIdP, "Embedded IdP configuration should be decoded") + assert.Equal(t, "file-dsn", cfg.EmbeddedIdP.Storage.Config.DSN, + "The legacy loader never read NB_ environment variables; the file value must win") + }, + }, + { + name: "template expanded value", + environment: map[string]string{"NB_HTTPCONFIG_AUTHAUDIENCE": "env-aud"}, + contents: `{"HttpConfig":{"AuthAudience":"{{ .MANAGEMENT_TEMPLATE_AUDIENCE }}"}}`, + validate: func(t *testing.T, cfg *nbconfig.Config) { + require.NotNil(t, cfg.HttpConfig, "HTTP configuration should be decoded") + assert.Equal(t, templateAudience, cfg.HttpConfig.AuthAudience, + "Template references were the only supported environment input and must win over NB_ variables") + }, + }, + } + + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + for name, value := range test.environment { + t.Setenv(name, value) + } + configPath := filepath.Join(t.TempDir(), "management.json") + require.NoError(t, os.WriteFile(configPath, []byte(test.contents), 0o600)) + + cfg, err := loadManagementConfig(configPath) + require.NoError(t, err) + test.validate(t, cfg) + }) + } +} + +// management-02 / management-03: the encryption key came only from the file and the file +// was only rewritten when it contained no key at all. +func TestLoadManagementConfigPreservesLegacyFileEncryptionKey(t *testing.T) { + clearManagementConfigEnvironment(t) + + tests := []struct { + name string + environmentKey string + }{ + {name: "empty environment key", environmentKey: ""}, + {name: "different environment key", environmentKey: "environment-key"}, + } + + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + t.Setenv("NB_DATASTOREENCRYPTIONKEY", test.environmentKey) + configPath := filepath.Join(t.TempDir(), "management.json") + contents := []byte(`{"DataStoreEncryptionKey":"operator-key","HttpConfig":{"AuthAudience":"file-aud"}}`) + require.NoError(t, os.WriteFile(configPath, contents, 0o600)) + + cfg, err := loadManagementConfig(configPath) + require.NoError(t, err) + assert.Equal(t, "operator-key", cfg.DataStoreEncryptionKey, + "NB_DATASTOREENCRYPTIONKEY was never read; the file key must be used") + + require.NoError(t, EnsureEncryptionKey(context.Background(), configPath, cfg)) + assert.Equal(t, "operator-key", cfg.DataStoreEncryptionKey, + "EnsureEncryptionKey must not replace an operator-provided key") + persistedConfig, err := os.ReadFile(configPath) + require.NoError(t, err) + assert.Equal(t, string(contents), string(persistedConfig), + "The config file must not be rewritten when it already contains an encryption key") + }) + } +} + +// management-04 / management-05: implicit TLS port selection only considered the +// --letsencrypt-domain flag and the certificate pair from the file. +func TestManagementPortDefaultsIgnoreEnvironmentTLS(t *testing.T) { + tests := []struct { + name string + environment map[string]string + expected int + }{ + { + name: "letsencrypt domain from environment", + environment: map[string]string{"NB_HTTPCONFIG_LETSENCRYPTDOMAIN": "management.example.com"}, + expected: 80, + }, + { + name: "certificate pair from environment", + environment: map[string]string{ + "NB_HTTPCONFIG_CERTFILE": "/tls.crt", + "NB_HTTPCONFIG_CERTKEY": "/tls.key", + }, + expected: 80, + }, + } + + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + actual := runManagementPreRunWithEnvironment(t, `{"AuthAudience":"test-audience"}`, nil, test.environment) + assert.Equal(t, test.expected, actual, + "Environment variables never enabled TLS, so the implicit port must remain the plain HTTP default") + }) + } +} + +// management-06: an empty-but-set environment variable was ignored and never cleared file values. +func TestLoadManagementConfigPreservesLegacyFileValuesOverEmptyEnvironment(t *testing.T) { + clearManagementConfigEnvironment(t) + + tests := []struct { + name string + environmentName string + contents string + validate func(*testing.T, *nbconfig.Config) + }{ + { + name: "http audience", + environmentName: "NB_HTTPCONFIG_AUTHAUDIENCE", + contents: `{"HttpConfig":{"AuthAudience":"file-aud"}}`, + validate: func(t *testing.T, cfg *nbconfig.Config) { + require.NotNil(t, cfg.HttpConfig, "HTTP configuration should be decoded") + assert.Equal(t, "file-aud", cfg.HttpConfig.AuthAudience, + "An empty NB_ variable was ignored and must not clear the file value") + }, + }, + { + name: "relay addresses", + environmentName: "NB_RELAY_ADDRESSES", + contents: `{"Relay":{"Addresses":["rel://a"]}}`, + validate: func(t *testing.T, cfg *nbconfig.Config) { + require.NotNil(t, cfg.Relay, "Relay configuration should be decoded") + assert.Equal(t, []string{"rel://a"}, cfg.Relay.Addresses, + "An empty NB_ variable was ignored and must not empty the relay list") + }, + }, + { + name: "stuns", + environmentName: "NB_STUNS", + contents: `{"Stuns":[{"Proto":"udp","URI":"stun:stun.example.com:3478"}]}`, + validate: func(t *testing.T, cfg *nbconfig.Config) { + assert.Len(t, cfg.Stuns, 1, "An empty NB_ variable was ignored and must not empty the STUN list") + }, + }, + { + name: "trusted proxies", + environmentName: "NB_REVERSEPROXY_TRUSTEDHTTPPROXIES", + contents: `{"ReverseProxy":{"TrustedHTTPProxies":["10.0.0.0/8"]}}`, + validate: func(t *testing.T, cfg *nbconfig.Config) { + assert.Len(t, cfg.ReverseProxy.TrustedHTTPProxies, 1, + "An empty NB_ variable was ignored and must not empty the trusted proxy list") + }, + }, + { + name: "datadir", + environmentName: "NB_DATADIR", + contents: `{"Datadir":"/file-data"}`, + validate: func(t *testing.T, cfg *nbconfig.Config) { + assert.Equal(t, "/file-data", cfg.Datadir, + "An empty NB_ variable was ignored and must not discard the file data directory") + }, + }, + } + + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + t.Setenv(test.environmentName, "") + configPath := filepath.Join(t.TempDir(), "management.json") + require.NoError(t, os.WriteFile(configPath, []byte(test.contents), 0o600)) + + cfg, err := loadManagementConfig(configPath) + require.NoError(t, err) + test.validate(t, cfg) + }) + } +} + +// management-07: NB_DATADIR had no effect on either the server or the admin commands. +func TestLoadMgmtConfigIgnoresEnvironmentDataDir(t *testing.T) { + clearManagementConfigEnvironment(t) + t.Setenv("NB_DATADIR", "/env-data") + + oldDatadir := mgmtDataDir + mgmtDataDir = defaultMgmtDataDir + t.Cleanup(func() { mgmtDataDir = oldDatadir }) + + configPath := filepath.Join(t.TempDir(), "management.json") + require.NoError(t, os.WriteFile(configPath, []byte(`{ + "DataStoreEncryptionKey": "configured-key", + "HttpConfig": { + "AuthAudience": "test-audience" + } +}`), 0o600)) + + cfg, err := LoadMgmtConfig(context.Background(), configPath, unchangedManagementFlags()) + require.NoError(t, err) + assert.Equal(t, defaultMgmtDataDir, cfg.Datadir, + "The server data directory came from the --datadir flag default; NB_DATADIR was never read") +} + +func TestLoadAdminMgmtConfigIgnoresEnvironmentDataDir(t *testing.T) { + clearManagementConfigEnvironment(t) + t.Setenv("NB_DATADIR", "/env-data") + configPath := filepath.Join(t.TempDir(), "management.json") + require.NoError(t, os.WriteFile(configPath, []byte(`{"Datadir":"/file-data"}`), 0o600)) + setAdminConfigPath(t, configPath) + + cfg, datadir, err := loadAdminMgmtConfig(context.Background(), false) + require.NoError(t, err) + assert.Equal(t, "/file-data", cfg.Datadir, "Admin commands read the data directory from the file only") + assert.Equal(t, "/file-data", datadir, "Admin commands read the effective data directory from the file only") +} + +// management-08: unparsable environment values were ignored instead of aborting startup. +func TestLoadManagementConfigIgnoresInvalidScalarEnvironmentValues(t *testing.T) { + clearManagementConfigEnvironment(t) + + tests := []struct { + name string + environmentName string + value string + }{ + {name: "invalid bool", environmentName: "NB_DISABLEDEFAULTPOLICY", value: "maybe"}, + {name: "padded bool", environmentName: "NB_DISABLEDEFAULTPOLICY", value: " true"}, + {name: "invalid int", environmentName: "NB_HIGHESTSUPPORTEDSYNCMESSAGEVERSION", value: "abc"}, + {name: "float int", environmentName: "NB_HIGHESTSUPPORTEDSYNCMESSAGEVERSION", value: "1.0"}, + {name: "negative uint", environmentName: "NB_REVERSEPROXY_TRUSTEDHTTPPROXIESCOUNT", value: "-1"}, + {name: "empty duration", environmentName: "NB_TURNCONFIG_CREDENTIALSTTL", value: ""}, + {name: "invalid duration", environmentName: "NB_TURNCONFIG_CREDENTIALSTTL", value: "abc"}, + {name: "numeric duration", environmentName: "NB_TURNCONFIG_CREDENTIALSTTL", value: "3600000000000"}, + {name: "invalid prefix", environmentName: "NB_REVERSEPROXY_TRUSTEDHTTPPROXIES", value: "bad"}, + {name: "address instead of prefix", environmentName: "NB_REVERSEPROXY_TRUSTEDHTTPPROXIES", value: "10.0.0.1"}, + {name: "overflowing login flag", environmentName: "NB_PKCEAUTHORIZATIONFLOW_PROVIDERCONFIG_LOGINFLAG", value: "300"}, + } + + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + t.Setenv(test.environmentName, test.value) + configPath := filepath.Join(t.TempDir(), "management.json") + require.NoError(t, os.WriteFile(configPath, []byte(`{ + "Datadir": "/file-data", + "TURNConfig": {"CredentialsTTL": "1h"}, + "PKCEAuthorizationFlow": {"ProviderConfig": {"LoginFlag": 1}} +}`), 0o600)) + + cfg, err := loadManagementConfig(configPath) + require.NoError(t, err, "Environment variables were never read, so an unparsable value must not fail startup") + assert.Equal(t, "/file-data", cfg.Datadir, "File values should be used when the environment is ignored") + }) + } +} + +// management-09: a variable named after a configuration section had no effect. +func TestLoadManagementConfigIgnoresSectionEnvironmentVariables(t *testing.T) { + clearManagementConfigEnvironment(t) + + tests := []struct { + name string + environmentName string + value string + contents string + validate func(*testing.T, *nbconfig.Config) + }{ + { + name: "http config", + environmentName: "NB_HTTPCONFIG", + value: "x", + contents: `{"HttpConfig":{"AuthAudience":"file-aud"}}`, + validate: func(t *testing.T, cfg *nbconfig.Config) { + require.NotNil(t, cfg.HttpConfig, "A section-named variable was ignored and must not drop the HTTP section") + assert.Equal(t, "file-aud", cfg.HttpConfig.AuthAudience, "The HTTP section should be loaded from the file") + }, + }, + { + name: "relay", + environmentName: "NB_RELAY", + value: "x", + contents: `{"Relay":{"Addresses":["rel://a"]}}`, + validate: func(t *testing.T, cfg *nbconfig.Config) { + require.NotNil(t, cfg.Relay, "A section-named variable was ignored and must not drop the relay section") + assert.Equal(t, []string{"rel://a"}, cfg.Relay.Addresses, "The relay section should be loaded from the file") + }, + }, + { + name: "empty idp manager config", + environmentName: "NB_IDPMANAGERCONFIG", + value: "", + contents: `{"IdpManagerConfig":{"ManagerType":"none"}}`, + validate: func(t *testing.T, cfg *nbconfig.Config) { + require.NotNil(t, cfg.IdpManagerConfig, "A section-named variable was ignored and must not drop the IDP section") + assert.Equal(t, "none", cfg.IdpManagerConfig.ManagerType, "The IDP section should be loaded from the file") + }, + }, + } + + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + t.Setenv(test.environmentName, test.value) + configPath := filepath.Join(t.TempDir(), "management.json") + require.NoError(t, os.WriteFile(configPath, []byte(test.contents), 0o600)) + + cfg, err := loadManagementConfig(configPath) + require.NoError(t, err) + test.validate(t, cfg) + }) + } +} + +// management-10: the optional embedded IdP section existed only when present in the file. +func TestLoadManagementConfigDoesNotMaterializeEmbeddedIdPFromEnvironment(t *testing.T) { + clearManagementConfigEnvironment(t) + + tests := []struct { + name string + environmentName string + value string + }{ + {name: "enabled", environmentName: "NB_EMBEDDEDIDP_ENABLED", value: "true"}, + {name: "empty issuer", environmentName: "NB_EMBEDDEDIDP_ISSUER", value: ""}, + } + + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + t.Setenv(test.environmentName, test.value) + configPath := filepath.Join(t.TempDir(), "management.json") + require.NoError(t, os.WriteFile(configPath, []byte(`{"HttpConfig":{"AuthAudience":"file-aud"}}`), 0o600)) + + cfg, err := loadManagementConfig(configPath) + require.NoError(t, err) + assert.Nil(t, cfg.EmbeddedIdP, + "Without an EmbeddedIdP object in the file the section stayed nil; environment variables were never read") + }) + } +} + +// management-11: optional pointer sections stayed nil unless present in the file. +func TestLoadManagementConfigDoesNotMaterializePointersFromEnvironment(t *testing.T) { + clearManagementConfigEnvironment(t) + + tests := []struct { + name string + environmentName string + value string + contents string + validate func(*testing.T, *nbconfig.Config) + }{ + { + name: "missing http config", + environmentName: "NB_HTTPCONFIG_AUTHAUDIENCE", + value: "env-aud", + contents: `{}`, + validate: func(t *testing.T, cfg *nbconfig.Config) { + assert.Nil(t, cfg.HttpConfig, "A missing HTTP section stayed nil; environment variables were never read") + }, + }, + { + name: "null http config", + environmentName: "NB_HTTPCONFIG_AUTHAUDIENCE", + value: "", + contents: `{"HttpConfig":null}`, + validate: func(t *testing.T, cfg *nbconfig.Config) { + assert.Nil(t, cfg.HttpConfig, "A null HTTP section stayed nil; environment variables were never read") + }, + }, + { + name: "signal", + environmentName: "NB_SIGNAL_URI", + value: "sig:x", + contents: `{}`, + validate: func(t *testing.T, cfg *nbconfig.Config) { + assert.Nil(t, cfg.Signal, "A missing signal section stayed nil; environment variables were never read") + }, + }, + { + name: "relay", + environmentName: "NB_RELAY_SECRET", + value: "s", + contents: `{}`, + validate: func(t *testing.T, cfg *nbconfig.Config) { + assert.Nil(t, cfg.Relay, "A missing relay section stayed nil; environment variables were never read") + }, + }, + { + name: "embedded idp owner", + environmentName: "NB_EMBEDDEDIDP_OWNER_EMAIL", + value: "owner@example.com", + contents: `{"EmbeddedIdP":{"Enabled":true,"Issuer":"https://management.example.com/oauth2"}}`, + validate: func(t *testing.T, cfg *nbconfig.Config) { + require.NotNil(t, cfg.EmbeddedIdP, "Embedded IdP configuration should be decoded") + assert.Nil(t, cfg.EmbeddedIdP.Owner, "A missing owner stayed nil; environment variables were never read") + }, + }, + } + + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + t.Setenv(test.environmentName, test.value) + configPath := filepath.Join(t.TempDir(), "management.json") + require.NoError(t, os.WriteFile(configPath, []byte(test.contents), 0o600)) + + cfg, err := loadManagementConfig(configPath) + require.NoError(t, err) + test.validate(t, cfg) + }) + } +} + +// management-12: variables named after struct slices or maps were ignored instead of aborting startup. +func TestLoadManagementConfigIgnoresCollectionEnvironmentValues(t *testing.T) { + clearManagementConfigEnvironment(t) + + tests := []struct { + name string + environmentName string + value string + }{ + {name: "stuns", environmentName: "NB_STUNS", value: "stun:stun.example.com:3478"}, + {name: "stuns json", environmentName: "NB_STUNS", value: `[{"Proto":"udp","URI":"stun:x"}]`}, + {name: "turns", environmentName: "NB_TURNCONFIG_TURNS", value: "turn:x"}, + {name: "static connectors", environmentName: "NB_EMBEDDEDIDP_STATICCONNECTORS", value: "x"}, + {name: "empty per account versions", environmentName: "NB_PERACCOUNTHIGHESTSUPPORTEDSYNCMESSAGEVERSION", value: ""}, + {name: "per account versions", environmentName: "NB_PERACCOUNTHIGHESTSUPPORTEDSYNCMESSAGEVERSION", value: "acc1=1"}, + {name: "empty extra config", environmentName: "NB_IDPMANAGERCONFIG_EXTRACONFIG", value: ""}, + } + + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + t.Setenv(test.environmentName, test.value) + configPath := filepath.Join(t.TempDir(), "management.json") + require.NoError(t, os.WriteFile(configPath, []byte(`{ + "Stuns": [{"Proto":"udp","URI":"stun:file.example.com:3478"}], + "TURNConfig": {"Turns": [{"Proto":"udp","URI":"turn:file.example.com:3478"}]}, + "EmbeddedIdP": {"Enabled": true, "Issuer": "https://management.example.com/oauth2"}, + "PerAccountHighestSupportedSyncMessageVersion": {"acc1": 1}, + "IdpManagerConfig": {"ExtraConfig": {"CustomerId": "customer-id"}} +}`), 0o600)) + + cfg, err := loadManagementConfig(configPath) + require.NoError(t, err, "Environment variables were never read, so a collection-named variable must not fail startup") + assert.Len(t, cfg.Stuns, 1, "File STUN servers should be retained") + require.NotNil(t, cfg.TURNConfig, "TURN configuration should be decoded") + assert.Len(t, cfg.TURNConfig.Turns, 1, "File TURN servers should be retained") + assert.Equal(t, map[string]int{"acc1": 1}, cfg.PerAccountHighestSupportedSyncMessageVersion, + "File per-account versions should be retained") + require.NotNil(t, cfg.IdpManagerConfig, "IDP configuration should be decoded") + assert.Equal(t, map[string]string{"CustomerId": "customer-id"}, map[string]string(cfg.IdpManagerConfig.ExtraConfig), + "File extra IDP configuration should be retained") + }) + } +} + +// management-13: list-valued environment variables did not exist; lists came only from JSON arrays. +func TestLoadManagementConfigPreservesLegacyFileListsOverEnvironment(t *testing.T) { + clearManagementConfigEnvironment(t) + + tests := []struct { + name string + environmentName string + value string + contents string + validate func(*testing.T, *nbconfig.Config) + }{ + { + name: "relay addresses with spaces", + environmentName: "NB_RELAY_ADDRESSES", + value: "rel://a, rel://b", + contents: `{"Relay":{"Addresses":["rel://file"]}}`, + validate: func(t *testing.T, cfg *nbconfig.Config) { + require.NotNil(t, cfg.Relay, "Relay configuration should be decoded") + assert.Equal(t, []string{"rel://file"}, cfg.Relay.Addresses, + "Relay addresses came only from the JSON array; environment lists were never parsed") + }, + }, + { + name: "relay addresses trailing comma", + environmentName: "NB_RELAY_ADDRESSES", + value: "rel://a,rel://b,", + contents: `{"Relay":{"Addresses":["rel://file"]}}`, + validate: func(t *testing.T, cfg *nbconfig.Config) { + require.NotNil(t, cfg.Relay, "Relay configuration should be decoded") + assert.Equal(t, []string{"rel://file"}, cfg.Relay.Addresses, + "Relay addresses came only from the JSON array; environment lists were never parsed") + }, + }, + { + name: "relay addresses json", + environmentName: "NB_RELAY_ADDRESSES", + value: `["rel://a","rel://b"]`, + contents: `{"Relay":{"Addresses":["rel://file"]}}`, + validate: func(t *testing.T, cfg *nbconfig.Config) { + require.NotNil(t, cfg.Relay, "Relay configuration should be decoded") + assert.Equal(t, []string{"rel://file"}, cfg.Relay.Addresses, + "Relay addresses came only from the JSON array; environment lists were never parsed") + }, + }, + { + name: "grant types", + environmentName: "NB_EMBEDDEDIDP_GRANTTYPES", + value: "authorization_code,refresh_token", + contents: `{"EmbeddedIdP":{"Enabled":true,"GrantTypes":["device_code"]}}`, + validate: func(t *testing.T, cfg *nbconfig.Config) { + require.NotNil(t, cfg.EmbeddedIdP, "Embedded IdP configuration should be decoded") + assert.Equal(t, []string{"device_code"}, cfg.EmbeddedIdP.GrantTypes, + "Grant types came only from the JSON array; environment lists were never parsed") + }, + }, + { + name: "trusted proxies with spaces", + environmentName: "NB_REVERSEPROXY_TRUSTEDHTTPPROXIES", + value: "10.0.0.0/8, 192.168.0.0/16", + contents: `{"ReverseProxy":{"TrustedHTTPProxies":["172.16.0.0/12"]}}`, + validate: func(t *testing.T, cfg *nbconfig.Config) { + require.Len(t, cfg.ReverseProxy.TrustedHTTPProxies, 1, + "Trusted proxies came only from the JSON array; environment lists were never parsed") + assert.Equal(t, "172.16.0.0/12", cfg.ReverseProxy.TrustedHTTPProxies[0].String(), + "Trusted proxies came only from the JSON array; environment lists were never parsed") + }, + }, + } + + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + t.Setenv(test.environmentName, test.value) + configPath := filepath.Join(t.TempDir(), "management.json") + require.NoError(t, os.WriteFile(configPath, []byte(test.contents), 0o600)) + + cfg, err := loadManagementConfig(configPath) + require.NoError(t, err, "Environment lists were never parsed, so they must not fail startup") + test.validate(t, cfg) + }) + } +} + +// management-14: boolean spellings in the environment were never interpreted. +func TestLoadManagementConfigIgnoresBooleanEnvironmentSpellings(t *testing.T) { + clearManagementConfigEnvironment(t) + + tests := []struct { + name string + value string + contents string + expected bool + }{ + {name: "one", value: "1", contents: `{"DisableDefaultPolicy":false}`, expected: false}, + {name: "true", value: "TRUE", contents: `{"DisableDefaultPolicy":false}`, expected: false}, + {name: "yes", value: "yes", contents: `{"DisableDefaultPolicy":false}`, expected: false}, + {name: "on", value: "on", contents: `{"DisableDefaultPolicy":false}`, expected: false}, + {name: "zero", value: "0", contents: `{"DisableDefaultPolicy":true}`, expected: true}, + {name: "no", value: "no", contents: `{"DisableDefaultPolicy":true}`, expected: true}, + {name: "off", value: "off", contents: `{"DisableDefaultPolicy":true}`, expected: true}, + {name: "empty", value: "", contents: `{"DisableDefaultPolicy":true}`, expected: true}, + {name: "invalid", value: "maybe", contents: `{"DisableDefaultPolicy":true}`, expected: true}, + } + + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + t.Setenv("NB_DISABLEDEFAULTPOLICY", test.value) + configPath := filepath.Join(t.TempDir(), "management.json") + require.NoError(t, os.WriteFile(configPath, []byte(test.contents), 0o600)) + + cfg, err := loadManagementConfig(configPath) + require.NoError(t, err, "Boolean environment spellings were never interpreted, so they must not fail startup") + assert.Equal(t, test.expected, cfg.DisableDefaultPolicy, + "NB_DISABLEDEFAULTPOLICY was never read; the file value must be used") + }) + } +} + +// management-15: an empty environment value never zeroed numeric fields. +func TestLoadManagementConfigPreservesLegacyNumericFileValuesOverEmptyEnvironment(t *testing.T) { + clearManagementConfigEnvironment(t) + + tests := []struct { + name string + environmentName string + contents string + validate func(*testing.T, *nbconfig.Config) + }{ + { + name: "missing sync version", + environmentName: "NB_HIGHESTSUPPORTEDSYNCMESSAGEVERSION", + contents: `{}`, + validate: func(t *testing.T, cfg *nbconfig.Config) { + assert.Nil(t, cfg.HighestSupportedSyncMessageVersion, + "An empty NB_ variable was ignored, so a missing sync version must stay nil") + }, + }, + { + name: "configured sync version", + environmentName: "NB_HIGHESTSUPPORTEDSYNCMESSAGEVERSION", + contents: `{"HighestSupportedSyncMessageVersion":1}`, + validate: func(t *testing.T, cfg *nbconfig.Config) { + require.NotNil(t, cfg.HighestSupportedSyncMessageVersion, "Sync version should be decoded") + assert.Equal(t, 1, *cfg.HighestSupportedSyncMessageVersion, + "An empty NB_ variable was ignored, so the file sync version must be retained") + }, + }, + { + name: "trusted proxies count", + environmentName: "NB_REVERSEPROXY_TRUSTEDHTTPPROXIESCOUNT", + contents: `{"ReverseProxy":{"TrustedHTTPProxiesCount":2}}`, + validate: func(t *testing.T, cfg *nbconfig.Config) { + assert.Equal(t, uint(2), cfg.ReverseProxy.TrustedHTTPProxiesCount, + "An empty NB_ variable was ignored, so the file proxy count must be retained") + }, + }, + { + name: "access log retention", + environmentName: "NB_REVERSEPROXY_ACCESSLOGRETENTIONDAYS", + contents: `{"ReverseProxy":{"AccessLogRetentionDays":30}}`, + validate: func(t *testing.T, cfg *nbconfig.Config) { + assert.Equal(t, 30, cfg.ReverseProxy.AccessLogRetentionDays, + "An empty NB_ variable was ignored, so the file retention must be retained") + }, + }, + } + + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + t.Setenv(test.environmentName, "") + configPath := filepath.Join(t.TempDir(), "management.json") + require.NoError(t, os.WriteFile(configPath, []byte(test.contents), 0o600)) + + cfg, err := loadManagementConfig(configPath) + require.NoError(t, err) + test.validate(t, cfg) + }) + } +} + +// management-16: admin and legacy token commands operated strictly on the file's view of the deployment. +func TestLoadAdminMgmtConfigIgnoresEnvironment(t *testing.T) { + clearManagementConfigEnvironment(t) + + tests := []struct { + name string + environment map[string]string + contents string + applyIDPDefaults bool + validate func(*testing.T, *nbconfig.Config) + }{ + { + name: "store engine", + environment: map[string]string{"NB_STORECONFIG_ENGINE": "postgres"}, + contents: `{"StoreConfig":{"Engine":"sqlite"}}`, + validate: func(t *testing.T, cfg *nbconfig.Config) { + assert.Equal(t, "sqlite", string(cfg.StoreConfig.Engine), + "Admin commands read the store engine from the file only") + }, + }, + { + name: "encryption key", + environment: map[string]string{"NB_DATASTOREENCRYPTIONKEY": "env-key"}, + contents: `{"DataStoreEncryptionKey":"file-key"}`, + validate: func(t *testing.T, cfg *nbconfig.Config) { + assert.Equal(t, "file-key", cfg.DataStoreEncryptionKey, + "Admin commands read the encryption key from the file only") + }, + }, + { + name: "embedded idp enabled", + environment: map[string]string{"NB_EMBEDDEDIDP_ENABLED": "true"}, + contents: `{"HttpConfig":{"AuthAudience":"file-aud"}}`, + applyIDPDefaults: true, + validate: func(t *testing.T, cfg *nbconfig.Config) { + assert.Nil(t, cfg.EmbeddedIdP, "Admin commands never enabled the embedded IdP from the environment") + require.NotNil(t, cfg.HttpConfig, "HTTP configuration should be decoded") + assert.Equal(t, "file-aud", cfg.HttpConfig.AuthAudience, + "HTTP configuration must not be rewritten by an environment-enabled embedded IdP") + }, + }, + { + name: "invalid value", + environment: map[string]string{"NB_DISABLEDEFAULTPOLICY": "maybe"}, + contents: `{"DisableDefaultPolicy":true}`, + validate: func(t *testing.T, cfg *nbconfig.Config) { + assert.True(t, cfg.DisableDefaultPolicy, "Admin commands read the policy flag from the file only") + }, + }, + } + + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + for name, value := range test.environment { + t.Setenv(name, value) + } + configPath := filepath.Join(t.TempDir(), "management.json") + require.NoError(t, os.WriteFile(configPath, []byte(test.contents), 0o600)) + setAdminConfigPath(t, configPath) + + cfg, _, err := loadAdminMgmtConfig(context.Background(), test.applyIDPDefaults) + require.NoError(t, err, "Admin commands never read the environment, so it must not fail loading") + test.validate(t, cfg) + }) + } +} + +// management-17: the store engine in the config came from the file only; NETBIRD_STORE_ENGINE +// was consulted later by the store package. +func TestLoadManagementConfigIgnoresEnvironmentStoreEngine(t *testing.T) { + clearManagementConfigEnvironment(t) + t.Setenv("NETBIRD_STORE_ENGINE", "sqlite") + + for _, value := range []string{"postgres", "Postgres"} { + t.Run(value, func(t *testing.T) { + t.Setenv("NB_STORECONFIG_ENGINE", value) + configPath := filepath.Join(t.TempDir(), "management.json") + require.NoError(t, os.WriteFile(configPath, []byte(`{"StoreConfig":{"Engine":""}}`), 0o600)) + + cfg, err := loadManagementConfig(configPath) + require.NoError(t, err) + assert.Empty(t, string(cfg.StoreConfig.Engine), + "NB_STORECONFIG_ENGINE was never read; an empty file engine left NETBIRD_STORE_ENGINE in charge") + }) + } +} + +// management-20: the certificate override only fired when both flag values were non-empty. +func TestApplyCommandLineOverridesPreservesLegacyMixedCertificatePair(t *testing.T) { + oldCertFile := certFile + oldCertKey := certKey + t.Cleanup(func() { + certFile = oldCertFile + certKey = oldCertKey + }) + + tests := []struct { + name string + certFile string + certKey string + }{ + {name: "empty key", certFile: "/new.crt", certKey: ""}, + {name: "empty file", certFile: "", certKey: "/new.key"}, + } + + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + certFile = test.certFile + certKey = test.certKey + flags := unchangedManagementFlags() + require.NoError(t, flags.Set("cert-file", test.certFile)) + require.NoError(t, flags.Set("cert-key", test.certKey)) + cfg := &nbconfig.Config{ + HttpConfig: &nbconfig.HttpServerConfig{ + CertFile: "/file/tls.crt", + CertKey: "/file/tls.key", + }, + } + + ApplyCommandLineOverrides(cfg, flags) + assert.Equal(t, "/file/tls.crt", cfg.HttpConfig.CertFile, + "A partially empty certificate pair never overrode the file certificate") + assert.Equal(t, "/file/tls.key", cfg.HttpConfig.CertKey, + "A partially empty certificate pair never overrode the file key") + }) + } +} + +// management-21: TLS flags with a missing HttpConfig section crashed instead of continuing. +func TestApplyCommandLineOverridesPreservesLegacyMissingHTTPConfigPanic(t *testing.T) { + oldLetsencryptDomain := mgmtLetsencryptDomain + oldCertFile := certFile + oldCertKey := certKey + t.Cleanup(func() { + mgmtLetsencryptDomain = oldLetsencryptDomain + certFile = oldCertFile + certKey = oldCertKey + }) + + tests := []struct { + name string + configure func(*testing.T, *pflag.FlagSet) + }{ + { + name: "letsencrypt domain", + configure: func(t *testing.T, flags *pflag.FlagSet) { + mgmtLetsencryptDomain = "example.com" + require.NoError(t, flags.Set("letsencrypt-domain", "example.com")) + }, + }, + { + name: "certificate pair", + configure: func(t *testing.T, flags *pflag.FlagSet) { + certFile = "/tls.crt" + certKey = "/tls.key" + require.NoError(t, flags.Set("cert-file", "/tls.crt")) + require.NoError(t, flags.Set("cert-key", "/tls.key")) + }, + }, + } + + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + mgmtLetsencryptDomain = "" + certFile = "" + certKey = "" + flags := unchangedManagementFlags() + test.configure(t, flags) + cfg := &nbconfig.Config{Datadir: "/d", DataStoreEncryptionKey: "k"} + + assert.Panics(t, func() { ApplyCommandLineOverrides(cfg, flags) }, + "TLS flags with no HttpConfig section historically dereferenced a nil pointer and crashed startup") + }) + } +} + +// management-23: the config file was always parsed as JSON regardless of its extension. +func TestLoadManagementConfigParsesJSONRegardlessOfExtension(t *testing.T) { + clearManagementConfigEnvironment(t) + + for _, extension := range []string{".toml", ".ini", ".env", ".dotenv", ".properties", ".props", ".prop", ".hcl", ".tfvars"} { + t.Run(extension, func(t *testing.T) { + configPath := filepath.Join(t.TempDir(), "management"+extension) + require.NoError(t, os.WriteFile(configPath, []byte(`{"Datadir":"/file-data"}`), 0o600)) + + cfg, err := loadManagementConfig(configPath) + require.NoError(t, err, "JSON content was always accepted regardless of the file extension") + assert.Equal(t, "/file-data", cfg.Datadir, "JSON content should be decoded regardless of the file extension") + }) + } +} + +// management-24: account IDs used as map keys kept their case. +func TestLoadManagementConfigPreservesPerAccountKeyCase(t *testing.T) { + clearManagementConfigEnvironment(t) + configPath := filepath.Join(t.TempDir(), "management.json") + require.NoError(t, os.WriteFile(configPath, []byte(`{"PerAccountHighestSupportedSyncMessageVersion":{"AbC123":1}}`), 0o600)) + + cfg, err := loadManagementConfig(configPath) + require.NoError(t, err) + assert.Equal(t, map[string]int{"AbC123": 1}, cfg.PerAccountHighestSupportedSyncMessageVersion, + "Per-account sync version keys are account IDs and must retain their case") +} + +// management-25: connector configuration keys inside slices kept their case. +func TestLoadManagementConfigPreservesStaticConnectorConfigKeyCase(t *testing.T) { + clearManagementConfigEnvironment(t) + configPath := filepath.Join(t.TempDir(), "management.json") + require.NoError(t, os.WriteFile(configPath, []byte(`{ + "EmbeddedIdP": { + "Enabled": true, + "StaticConnectors": [ + {"type": "oidc", "id": "x", "config": {"clientID": "a", "redirectURI": "b", "insecureEnableGroups": false}} + ] + } +}`), 0o600)) + + cfg, err := loadManagementConfig(configPath) + require.NoError(t, err) + require.NotNil(t, cfg.EmbeddedIdP, "Embedded IdP configuration should be decoded") + require.Len(t, cfg.EmbeddedIdP.StaticConnectors, 1, "Static connectors should be decoded") + assert.Equal(t, map[string]any{ + "clientID": "a", + "redirectURI": "b", + "insecureEnableGroups": false, + }, cfg.EmbeddedIdP.StaticConnectors[0].Config, "Dex connector option keys are case-sensitive and must retain their case") +} + +// management-26: mistyped scalars in the file were rejected instead of coerced. +func TestLoadManagementConfigRejectsWeaklyTypedFileValues(t *testing.T) { + clearManagementConfigEnvironment(t) + + tests := []struct { + name string + contents string + }{ + {name: "string sync version", contents: `{"HighestSupportedSyncMessageVersion":"1"}`}, + {name: "string per account version", contents: `{"PerAccountHighestSupportedSyncMessageVersion":{"a":"1"}}`}, + {name: "numeric datadir", contents: `{"Datadir":123}`}, + {name: "numeric engine", contents: `{"StoreConfig":{"Engine":1}}`}, + {name: "numeric proto", contents: `{"Signal":{"Proto":1}}`}, + {name: "string bool", contents: `{"DisableDefaultPolicy":"true"}`}, + {name: "upper string bool", contents: `{"DisableDefaultPolicy":"TRUE"}`}, + {name: "yes bool", contents: `{"DisableDefaultPolicy":"yes"}`}, + {name: "on bool", contents: `{"DisableDefaultPolicy":"on"}`}, + {name: "numeric bool", contents: `{"DisableDefaultPolicy":1}`}, + {name: "string login flag", contents: `{"PKCEAuthorizationFlow":{"ProviderConfig":{"LoginFlag":"1"}}}`}, + } + + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + configPath := filepath.Join(t.TempDir(), "management.json") + require.NoError(t, os.WriteFile(configPath, []byte(test.contents), 0o600)) + + _, err := loadManagementConfig(configPath) + assert.Error(t, err, "encoding/json rejected mistyped scalars, so loading must fail instead of coercing the value") + }) + } +} + +// management-27: numeric edge cases were rejected or decoded exactly rather than silently mangled. +func TestLoadManagementConfigPreservesLegacyNumericDecoding(t *testing.T) { + clearManagementConfigEnvironment(t) + + tests := []struct { + name string + contents string + validate func(*testing.T, *nbconfig.Config, error) + }{ + { + name: "fractional sync version", + contents: `{"HighestSupportedSyncMessageVersion":1.7}`, + validate: func(t *testing.T, _ *nbconfig.Config, err error) { + assert.Error(t, err, "encoding/json rejected a fractional number for an int field") + }, + }, + { + name: "whole float sync version", + contents: `{"HighestSupportedSyncMessageVersion":1.0}`, + validate: func(t *testing.T, _ *nbconfig.Config, err error) { + assert.Error(t, err, "encoding/json rejected a float literal for an int field") + }, + }, + { + name: "fractional retention days", + contents: `{"ReverseProxy":{"AccessLogRetentionDays":7.9}}`, + validate: func(t *testing.T, _ *nbconfig.Config, err error) { + assert.Error(t, err, "encoding/json rejected a fractional number for an int field") + }, + }, + { + name: "negative proxy count", + contents: `{"ReverseProxy":{"TrustedHTTPProxiesCount":-1}}`, + validate: func(t *testing.T, _ *nbconfig.Config, err error) { + assert.Error(t, err, "encoding/json rejected a negative number for a uint field") + }, + }, + { + name: "overflowing login flag", + contents: `{"PKCEAuthorizationFlow":{"ProviderConfig":{"LoginFlag":300}}}`, + validate: func(t *testing.T, _ *nbconfig.Config, err error) { + assert.Error(t, err, "encoding/json rejected a number overflowing a uint8 field") + }, + }, + { + name: "large sync version", + contents: `{"HighestSupportedSyncMessageVersion":9007199254740993}`, + validate: func(t *testing.T, cfg *nbconfig.Config, err error) { + require.NoError(t, err) + require.NotNil(t, cfg.HighestSupportedSyncMessageVersion, "Sync version should be decoded") + assert.Equal(t, 9007199254740993, *cfg.HighestSupportedSyncMessageVersion, + "encoding/json decoded large integers exactly without float64 precision loss") + }, + }, + } + + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + configPath := filepath.Join(t.TempDir(), "management.json") + require.NoError(t, os.WriteFile(configPath, []byte(test.contents), 0o600)) + + cfg, err := loadManagementConfig(configPath) + test.validate(t, cfg, err) + }) + } +} + +// management-28: the rewritten config reproduced the decoded struct shape. +func TestEnsureEncryptionKeyPersistsLegacyConfigShape(t *testing.T) { + clearManagementConfigEnvironment(t) + configPath := filepath.Join(t.TempDir(), "management.json") + require.NoError(t, os.WriteFile(configPath, []byte(`{ + "HttpConfig": {}, + "Relay": {}, + "IdpManagerConfig": { + "ExtraConfig": { + "ServiceAccountKey": "service-account-key" + } + }, + "PerAccountHighestSupportedSyncMessageVersion": {"AbC123": 1} +}`), 0o600)) + + cfg, err := loadManagementConfig(configPath) + require.NoError(t, err) + require.NoError(t, EnsureEncryptionKey(context.Background(), configPath, cfg)) + + persistedConfig, err := os.ReadFile(configPath) + require.NoError(t, err) + var persisted map[string]any + require.NoError(t, json.Unmarshal(persistedConfig, &persisted)) + + _, httpConfigIsObject := persisted["HttpConfig"].(map[string]any) + assert.True(t, httpConfigIsObject, "An explicitly configured empty HttpConfig was persisted as an object, not null") + _, relayIsObject := persisted["Relay"].(map[string]any) + assert.True(t, relayIsObject, "An explicitly configured empty Relay section was persisted as an object, not null") + + idpConfig, _ := persisted["IdpManagerConfig"].(map[string]any) + extraConfig, _ := idpConfig["ExtraConfig"].(map[string]any) + assert.Contains(t, extraConfig, "ServiceAccountKey", "Persisted IDP-specific keys retained their case") + + perAccount, _ := persisted["PerAccountHighestSupportedSyncMessageVersion"].(map[string]any) + assert.Contains(t, perAccount, "AbC123", "Persisted per-account keys retained their case") +} + +// management-29: an empty config file failed to load instead of silently producing defaults. +func TestLoadManagementConfigRejectsEmptyFile(t *testing.T) { + clearManagementConfigEnvironment(t) + configPath := filepath.Join(t.TempDir(), "management.json") + require.NoError(t, os.WriteFile(configPath, nil, 0o600)) + + _, err := loadManagementConfig(configPath) + assert.Error(t, err, "json.Unmarshal rejected a zero-byte file with 'unexpected end of JSON input'") +} + +// management-30: duplicate object keys differing only in case were merged into the same struct. +func TestLoadManagementConfigMergesCaseVariantDuplicateObjects(t *testing.T) { + clearManagementConfigEnvironment(t) + configPath := filepath.Join(t.TempDir(), "management.json") + require.NoError(t, os.WriteFile(configPath, []byte(`{"Signal":{"URI":"signal.example.com:10000"},"signal":{"Proto":"udp"}}`), 0o600)) + + cfg, err := loadManagementConfig(configPath) + require.NoError(t, err) + require.NotNil(t, cfg.Signal, "Signal configuration should be decoded") + assert.Equal(t, "signal.example.com:10000", cfg.Signal.URI, + "encoding/json decoded case-variant duplicate objects into the same struct, keeping both fields") + assert.Equal(t, nbconfig.UDP, cfg.Signal.Proto, + "encoding/json decoded case-variant duplicate objects into the same struct, keeping both fields") +} + +// management-31: a scalar string for a slice field in the file was rejected. +func TestLoadManagementConfigRejectsScalarStringsForSlices(t *testing.T) { + clearManagementConfigEnvironment(t) + + tests := []struct { + name string + contents string + }{ + {name: "relay addresses", contents: `{"Relay":{"Addresses":"rel://a,rel://b"}}`}, + {name: "grant types", contents: `{"EmbeddedIdP":{"GrantTypes":"authorization_code"}}`}, + {name: "trusted proxies", contents: `{"ReverseProxy":{"TrustedHTTPProxies":"10.0.0.0/8,192.168.0.0/16"}}`}, + } + + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + configPath := filepath.Join(t.TempDir(), "management.json") + require.NoError(t, os.WriteFile(configPath, []byte(test.contents), 0o600)) + + _, err := loadManagementConfig(configPath) + assert.Error(t, err, "encoding/json rejected a string for a slice field instead of splitting it on commas") + }) + } +} + +// management-32: a null duration in the file was rejected. +func TestLoadManagementConfigRejectsNullDurations(t *testing.T) { + clearManagementConfigEnvironment(t) + + tests := []struct { + name string + contents string + }{ + {name: "turn credentials ttl", contents: `{"TURNConfig":{"CredentialsTTL":null}}`}, + {name: "relay credentials ttl", contents: `{"Relay":{"CredentialsTTL":null}}`}, + } + + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + configPath := filepath.Join(t.TempDir(), "management.json") + require.NoError(t, os.WriteFile(configPath, []byte(test.contents), 0o600)) + + _, err := loadManagementConfig(configPath) + assert.Error(t, err, "util.Duration rejected JSON null with 'invalid duration' instead of defaulting to 0s") + }) + } +} + +// management-critic-1: duplicate same-case object keys were decoded into the same struct, merging both blocks. +func TestLoadManagementConfigMergesSameCaseDuplicateObjects(t *testing.T) { + clearManagementConfigEnvironment(t) + + tests := []struct { + name string + contents string + validate func(*testing.T, *nbconfig.Config) + }{ + { + name: "signal", + contents: `{"Signal":{"URI":"signal.example.com:10000"},"Signal":{"Proto":"udp"}}`, + validate: func(t *testing.T, cfg *nbconfig.Config) { + require.NotNil(t, cfg.Signal, "Signal configuration should be decoded") + assert.Equal(t, "signal.example.com:10000", cfg.Signal.URI, + "encoding/json decoded duplicate Signal objects into the same struct, keeping the first block's fields") + assert.Equal(t, nbconfig.UDP, cfg.Signal.Proto, + "encoding/json decoded duplicate Signal objects into the same struct, keeping the second block's fields") + }, + }, + { + name: "http config", + contents: `{"HttpConfig":{"AuthAudience":"a"},"HttpConfig":{"AuthIssuer":"i"}}`, + validate: func(t *testing.T, cfg *nbconfig.Config) { + require.NotNil(t, cfg.HttpConfig, "HTTP configuration should be decoded") + assert.Equal(t, "a", cfg.HttpConfig.AuthAudience, + "encoding/json decoded duplicate HttpConfig objects into the same struct, keeping the first block's fields") + assert.Equal(t, "i", cfg.HttpConfig.AuthIssuer, + "encoding/json decoded duplicate HttpConfig objects into the same struct, keeping the second block's fields") + }, + }, + } + + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + configPath := filepath.Join(t.TempDir(), "management.json") + require.NoError(t, os.WriteFile(configPath, []byte(test.contents), 0o600)) + + cfg, err := loadManagementConfig(configPath) + require.NoError(t, err) + test.validate(t, cfg) + }) + } +} + +// management-critic-2: case-variant duplicate scalar keys were applied in document order, so the last one won. +func TestLoadManagementConfigPreservesLegacyDocumentOrderForCaseVariantScalars(t *testing.T) { + clearManagementConfigEnvironment(t) + + tests := []struct { + name string + contents string + expected string + }{ + { + name: "uppercase first", + contents: `{"Datadir":"/first-data","datadir":"/second-data"}`, + expected: "/second-data", + }, + { + name: "lowercase first", + contents: `{"datadir":"/first-data","Datadir":"/second-data"}`, + expected: "/second-data", + }, + } + + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + configPath := filepath.Join(t.TempDir(), "management.json") + require.NoError(t, os.WriteFile(configPath, []byte(test.contents), 0o600)) + + cfg, err := loadManagementConfig(configPath) + require.NoError(t, err) + assert.Equal(t, test.expected, cfg.Datadir, + "encoding/json applied case-variant duplicate scalar keys in document order, so the last key in the file won") + }) + } +} + +// management-critic-3: NB_DATASTOREENCRYPTIONKEY was never read, so a file without a key always got a generated one persisted. +func TestEnsureEncryptionKeyIgnoresEnvironmentKeyWhenFileHasNone(t *testing.T) { + const environmentKey = "environment-key" + + clearManagementConfigEnvironment(t) + + tests := []struct { + name string + contents string + }{ + {name: "missing key", contents: `{"HttpConfig":{"AuthAudience":"file-aud"}}`}, + {name: "empty key", contents: `{"DataStoreEncryptionKey":"","HttpConfig":{"AuthAudience":"file-aud"}}`}, + } + + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + t.Setenv("NB_DATASTOREENCRYPTIONKEY", environmentKey) + configPath := filepath.Join(t.TempDir(), "management.json") + require.NoError(t, os.WriteFile(configPath, []byte(test.contents), 0o600)) + + cfg, err := loadManagementConfig(configPath) + require.NoError(t, err) + assert.Empty(t, cfg.DataStoreEncryptionKey, + "NB_DATASTOREENCRYPTIONKEY was never read; a file without a key loaded with an empty key") + + require.NoError(t, EnsureEncryptionKey(context.Background(), configPath, cfg)) + assert.NotEmpty(t, cfg.DataStoreEncryptionKey, + "EnsureEncryptionKey generated a fresh key when the file had none") + assert.NotEqual(t, environmentKey, cfg.DataStoreEncryptionKey, + "EnsureEncryptionKey generated a random key instead of adopting the environment value") + + persistedConfig, err := os.ReadFile(configPath) + require.NoError(t, err) + var persisted map[string]any + require.NoError(t, json.Unmarshal(persistedConfig, &persisted)) + persistedKey, _ := persisted["DataStoreEncryptionKey"].(string) + assert.NotEmpty(t, persistedKey, + "The generated key was written back to management.json so later starts reuse it") + assert.NotEqual(t, environmentKey, persistedKey, + "The persisted key was the generated one, never the environment value") + }) + } +} + +// management-critic-4: a single JSON object where an array of hosts was expected was rejected. +func TestLoadManagementConfigRejectsObjectsForHostSlices(t *testing.T) { + clearManagementConfigEnvironment(t) + + tests := []struct { + name string + contents string + }{ + {name: "stuns", contents: `{"Stuns":{"Proto":"udp","URI":"stun:x"}}`}, + {name: "turns", contents: `{"TURNConfig":{"Turns":{"Proto":"udp","URI":"turn:x"}}}`}, + } + + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + configPath := filepath.Join(t.TempDir(), "management.json") + require.NoError(t, os.WriteFile(configPath, []byte(test.contents), 0o600)) + + _, err := loadManagementConfig(configPath) + assert.Error(t, err, + "encoding/json rejected an object for a []*Host field ('cannot unmarshal object into Go struct field') instead of wrapping it into a one-element slice") + }) + } +} + +func setAdminConfigPath(t *testing.T, configPath string) { + t.Helper() + + oldConfigPath := nbconfig.MgmtConfigPath + oldAdminDatadir := adminDatadir + nbconfig.MgmtConfigPath = configPath + adminDatadir = "" + t.Cleanup(func() { + nbconfig.MgmtConfigPath = oldConfigPath + adminDatadir = oldAdminDatadir + }) +} + +func clearManagementConfigEnvironment(t *testing.T) { + t.Helper() + clearManagementEnvironmentType(t, reflect.TypeOf(nbconfig.Config{}), "", make(map[reflect.Type]bool)) +} + +func clearManagementEnvironmentType(t *testing.T, configType reflect.Type, prefix string, visiting map[reflect.Type]bool) { + t.Helper() + + for configType.Kind() == reflect.Pointer { + configType = configType.Elem() + } + if configType.Kind() != reflect.Struct || visiting[configType] { + return + } + visiting[configType] = true + defer delete(visiting, configType) + + for i := range configType.NumField() { + field := configType.Field(i) + if !field.IsExported() { + continue + } + key := strings.Split(field.Tag.Get("json"), ",")[0] + if key == "-" { + continue + } + if key == "" { + key = field.Name + } + if prefix != "" { + key = prefix + "." + key + } + environmentName := "NB_" + strings.ToUpper(strings.NewReplacer(".", "_", "-", "_").Replace(key)) + t.Setenv(environmentName, "") + require.NoError(t, os.Unsetenv(environmentName)) + clearManagementEnvironmentType(t, field.Type, key, visiting) + } +} + +func runManagementPreRun(t *testing.T, httpConfig string, changedFlags map[string]string) int { + t.Helper() + return runManagementPreRunWithEnvironment(t, httpConfig, changedFlags, nil) +} + +func runManagementPreRunWithEnvironment(t *testing.T, httpConfig string, changedFlags map[string]string, environment map[string]string) int { + t.Helper() + + for _, name := range []string{ + "NB_DATADIR", + "NB_DATASTOREENCRYPTIONKEY", + "NB_HTTPCONFIG_LETSENCRYPTDOMAIN", + "NB_HTTPCONFIG_CERTFILE", + "NB_HTTPCONFIG_CERTKEY", + } { + t.Setenv(name, "") + require.NoError(t, os.Unsetenv(name)) + } + for name, value := range environment { + t.Setenv(name, value) + } + + configPath := filepath.Join(t.TempDir(), "management.json") + contents := `{"DataStoreEncryptionKey":"configured-key","HttpConfig":` + httpConfig + `}` + require.NoError(t, os.WriteFile(configPath, []byte(contents), 0o600)) + + flags := map[string]*pflag.Flag{ + "port": mgmtCmd.Flags().Lookup("port"), + "datadir": mgmtCmd.Flags().Lookup("datadir"), + "letsencrypt-domain": mgmtCmd.Flags().Lookup("letsencrypt-domain"), + "cert-file": mgmtCmd.Flags().Lookup("cert-file"), + "cert-key": mgmtCmd.Flags().Lookup("cert-key"), + } + for name, flag := range flags { + require.NotNil(t, flag, "Management compatibility flag %s should be registered", name) + } + + oldConfigPath := nbconfig.MgmtConfigPath + oldCommandContext := mgmtCmd.Context() + oldMgmtPort := mgmtPort + oldMgmtDataDir := mgmtDataDir + oldLogLevel := logLevel + oldLogFile := logFile + oldDNSDomain := dnsDomain + oldLetsencryptDomain := mgmtLetsencryptDomain + oldCertFile := certFile + oldCertKey := certKey + oldConfig := config + oldFlagStates := make(map[*pflag.Flag]struct { + value string + changed bool + }, len(flags)) + for _, flag := range flags { + oldFlagStates[flag] = struct { + value string + changed bool + }{value: flag.Value.String(), changed: flag.Changed} + } + t.Cleanup(func() { + for flag, state := range oldFlagStates { + require.NoError(t, flag.Value.Set(state.value)) + flag.Changed = state.changed + } + nbconfig.MgmtConfigPath = oldConfigPath + mgmtCmd.SetContext(oldCommandContext) + mgmtPort = oldMgmtPort + mgmtDataDir = oldMgmtDataDir + logLevel = oldLogLevel + logFile = oldLogFile + dnsDomain = oldDNSDomain + mgmtLetsencryptDomain = oldLetsencryptDomain + certFile = oldCertFile + certKey = oldCertKey + config = oldConfig + }) + + baseline := map[string]string{ + "port": "80", + "datadir": defaultMgmtDataDir, + "letsencrypt-domain": "", + "cert-file": "", + "cert-key": "", + } + for name, value := range baseline { + require.NoError(t, flags[name].Value.Set(value)) + flags[name].Changed = false + } + for name, value := range changedFlags { + flag, ok := flags[name] + require.True(t, ok, "Compatibility scenario should reference a known flag") + require.NoError(t, flag.Value.Set(value)) + flag.Changed = true + } + + nbconfig.MgmtConfigPath = configPath + mgmtCmd.SetContext(context.Background()) + logLevel = "info" + logFile = "console" + dnsDomain = defaultSingleAccModeDomain + require.NoError(t, mgmtCmd.PreRunE(mgmtCmd, nil)) + return mgmtPort +} + +func unchangedManagementFlags() *pflag.FlagSet { + flags := pflag.NewFlagSet("management", pflag.ContinueOnError) + flags.String("datadir", defaultMgmtDataDir, "") + flags.String("letsencrypt-domain", "", "") + flags.String("cert-key", "", "") + flags.String("cert-file", "", "") + return flags +} diff --git a/proxy/cmd/proxy/cmd/config_test.go b/proxy/cmd/proxy/cmd/config_test.go index cf4812de5..858d3f0b8 100644 --- a/proxy/cmd/proxy/cmd/config_test.go +++ b/proxy/cmd/proxy/cmd/config_test.go @@ -1,23 +1,299 @@ package cmd import ( + "io" "os" "path/filepath" "testing" "time" + "github.com/spf13/cobra" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" + + "github.com/netbirdio/netbird/trustedproxy" ) func TestExampleConfig(t *testing.T) { - require.NoError(t, rootCmd.ParseFlags(nil)) - cfg, err := loadConfig(rootCmd, filepath.Join("..", "..", "..", "config.example.yaml")) + clearProxyConfigEnvironment(t) + cmd := newLegacyProxyCommand(t) + cfg, err := loadConfig(cmd, filepath.Join("..", "..", "..", "config.example.yaml")) require.NoError(t, err) assert.Equal(t, "proxy.example.com", cfg.ProxyURL, "Example config should load") } +func TestLoadConfigPreservesLegacyDefaults(t *testing.T) { + clearProxyConfigEnvironment(t) + cmd := newLegacyProxyCommand(t) + + cfg, err := loadConfig(cmd, "") + require.NoError(t, err) + assert.Equal(t, legacyProxyDefaults(t), cfg, "Proxy defaults should remain unchanged") +} + +func TestLoadConfigPreservesLegacyEnvironmentBindings(t *testing.T) { + clearProxyConfigEnvironment(t) + t.Setenv("NB_PROXY_LOG_LEVEL", "debug") + t.Setenv("NB_PROXY_ADDRESS", ":8443") + t.Setenv("NB_PROXY_MANAGEMENT_ADDRESS", "https://management.example.com:443") + t.Setenv("NB_PROXY_DOMAIN", "proxy.example.com") + t.Setenv("NB_PROXY_TOKEN", "proxy-token") + t.Setenv("NB_PROXY_CERTIFICATE_DIRECTORY", "/var/lib/proxy/certs") + t.Setenv("NB_PROXY_CERTIFICATE_FILE", "proxy.crt") + t.Setenv("NB_PROXY_CERTIFICATE_KEY_FILE", "proxy.key") + t.Setenv("NB_PROXY_ACME_CERTIFICATES", "true") + t.Setenv("NB_PROXY_ACME_ADDRESS", ":8080") + t.Setenv("NB_PROXY_ACME_DIRECTORY", "https://acme.example.com/directory") + t.Setenv("NB_PROXY_ACME_EAB_KID", "eab-kid") + t.Setenv("NB_PROXY_ACME_EAB_HMAC_KEY", "eab-hmac") + t.Setenv("NB_PROXY_ACME_CHALLENGE_TYPE", "http-01") + t.Setenv("NB_PROXY_CERT_LOCK_METHOD", "flock") + t.Setenv("NB_PROXY_WILDCARD_CERT_DIR", "/var/lib/proxy/wildcards") + t.Setenv("NB_PROXY_DEBUG_ENDPOINT", "true") + t.Setenv("NB_PROXY_DEBUG_ENDPOINT_ADDRESS", "localhost:9444") + t.Setenv("NB_PROXY_HEALTH_ADDRESS", "localhost:9080") + t.Setenv("NB_PROXY_FORWARDED_PROTO", "https") + t.Setenv("NB_PROXY_TRUSTED_PROXIES", "192.0.2.0/24") + t.Setenv("NB_PROXY_WG_PORT", "51820") + t.Setenv("NB_PROXY_PROXY_PROTOCOL", "true") + t.Setenv("NB_PROXY_PRESHARED_KEY", "pre-shared-key") + t.Setenv("NB_PROXY_SUPPORTS_CUSTOM_PORTS", "false") + t.Setenv("NB_PROXY_REQUIRE_SUBDOMAIN", "true") + t.Setenv("NB_PROXY_PRIVATE", "true") + t.Setenv("NB_PROXY_MAX_DIAL_TIMEOUT", "5s") + t.Setenv("NB_PROXY_MAX_SESSION_IDLE_TIMEOUT", "10m") + t.Setenv("NB_PROXY_MAPPING_BATCH_WATCHDOG", "30s") + t.Setenv("NB_PROXY_GEO_DATA_DIR", "/var/lib/proxy/geo") + t.Setenv("NB_PROXY_CROWDSEC_API_URL", "https://crowdsec.example.com") + t.Setenv("NB_PROXY_CROWDSEC_API_KEY", "crowdsec-key") + t.Setenv("NB_PROXY_PREALLOCATED_BUFFERS", "1024") + t.Setenv("NB_PROXY_MAX_BATCH_SIZE", "64") + cmd := newLegacyProxyCommand(t) + + cfg, err := loadConfig(cmd, "") + require.NoError(t, err) + assert.Equal(t, "debug", cfg.LogLevel, "Legacy log-level environment binding should remain supported") + assert.Equal(t, ":8443", cfg.ListenAddr, "Legacy address environment binding should remain supported") + assert.Equal(t, "https://management.example.com:443", cfg.ManagementAddress, + "Legacy management environment binding should remain supported") + assert.Equal(t, "proxy.example.com", cfg.ProxyURL, "Legacy domain environment binding should remain supported") + assert.Equal(t, "proxy-token", cfg.ProxyToken, "Legacy token environment binding should remain supported") + assert.Equal(t, "/var/lib/proxy/certs", cfg.CertificateDirectory, "Legacy certificate directory should remain supported") + assert.Equal(t, "proxy.crt", cfg.CertificateFile, "Legacy certificate file should remain supported") + assert.Equal(t, "proxy.key", cfg.CertificateKeyFile, "Legacy certificate key should remain supported") + assert.True(t, cfg.GenerateACMECertificates, "Legacy ACME toggle should remain supported") + assert.Equal(t, ":8080", cfg.ACMEChallengeAddress, "Legacy ACME address should remain supported") + assert.Equal(t, "https://acme.example.com/directory", cfg.ACMEDirectory, "Legacy ACME directory should remain supported") + assert.Equal(t, "eab-kid", cfg.ACMEEABKID, "Legacy EAB KID should remain supported") + assert.Equal(t, "eab-hmac", cfg.ACMEEABHMACKey, "Legacy EAB HMAC key should remain supported") + assert.Equal(t, "http-01", cfg.ACMEChallengeType, "Legacy ACME challenge type should remain supported") + assert.Equal(t, "flock", string(cfg.CertLockMethod), "Legacy certificate lock method should remain supported") + assert.Equal(t, "/var/lib/proxy/wildcards", cfg.WildcardCertDir, "Legacy wildcard certificate directory should remain supported") + assert.True(t, cfg.DebugEndpointEnabled, "Legacy debug endpoint toggle should remain supported") + assert.Equal(t, "localhost:9444", cfg.DebugEndpointAddress, "Legacy debug endpoint address should remain supported") + assert.Equal(t, "localhost:9080", cfg.HealthAddr, "Legacy health address should remain supported") + assert.Equal(t, "https", cfg.ForwardedProto, "Legacy forwarded-proto environment binding should remain supported") + require.NotNil(t, cfg.TrustedProxies, "Legacy trusted proxy environment binding should remain supported") + assert.False(t, cfg.TrustedProxies.Empty(), "Legacy trusted proxy environment binding should remain populated") + assert.Equal(t, uint16(51820), cfg.WireguardPort, "Legacy tunnel port should remain supported") + assert.True(t, cfg.ProxyProtocol, "Legacy PROXY protocol toggle should remain supported") + assert.Equal(t, "pre-shared-key", cfg.PreSharedKey, "Legacy pre-shared key should remain supported") + assert.False(t, cfg.SupportsCustomPorts, "Legacy custom-port toggle should remain supported") + assert.True(t, cfg.RequireSubdomain, "Legacy subdomain toggle should remain supported") + assert.True(t, cfg.Private, "Legacy private toggle should remain supported") + assert.Equal(t, 5*time.Second, cfg.MaxDialTimeout, "Legacy dial timeout should remain supported") + assert.Equal(t, 10*time.Minute, cfg.MaxSessionIdleTimeout, "Legacy idle timeout should remain supported") + assert.Equal(t, 30*time.Second, cfg.MappingBatchWatchdog, "Legacy mapping watchdog should remain supported") + assert.Equal(t, "/var/lib/proxy/geo", cfg.GeoDataDir, "Legacy geodata directory should remain supported") + assert.Equal(t, "https://crowdsec.example.com", cfg.CrowdSecAPIURL, "Legacy CrowdSec URL should remain supported") + assert.Equal(t, "crowdsec-key", cfg.CrowdSecAPIKey, "Legacy CrowdSec key should remain supported") + require.NotNil(t, cfg.PreallocatedBuffers, "Legacy preallocated buffer environment binding should remain supported") + assert.Equal(t, uint32(1024), *cfg.PreallocatedBuffers, "Legacy preallocated buffer value should remain supported") + require.NotNil(t, cfg.MaxBatchSize, "Legacy maximum batch environment binding should remain supported") + assert.Equal(t, uint32(64), *cfg.MaxBatchSize, "Legacy maximum batch value should remain supported") +} + +func TestLoadConfigPreservesLegacyInvalidEnvironmentBehavior(t *testing.T) { + tests := []struct { + name string + envName string + value string + wantErr bool + validate func(*testing.T, *commandConfig) + }{ + { + name: "invalid boolean uses default", + envName: "NB_PROXY_SUPPORTS_CUSTOM_PORTS", + value: "invalid", + validate: func(t *testing.T, cfg *commandConfig) { + assert.True(t, cfg.SupportsCustomPorts, "Invalid legacy booleans should retain their default") + }, + }, + { + name: "invalid uint16 uses default", + envName: "NB_PROXY_WG_PORT", + value: "invalid", + validate: func(t *testing.T, cfg *commandConfig) { + assert.Zero(t, cfg.WireguardPort, "Invalid legacy uint16 values should retain their default") + }, + }, + { + name: "invalid duration uses default", + envName: "NB_PROXY_MAX_DIAL_TIMEOUT", + value: "invalid", + validate: func(t *testing.T, cfg *commandConfig) { + assert.Zero(t, cfg.MaxDialTimeout, "Invalid legacy durations should retain their default") + }, + }, + { + name: "invalid watchdog uses default", + envName: "NB_PROXY_MAPPING_BATCH_WATCHDOG", + value: "invalid", + validate: func(t *testing.T, cfg *commandConfig) { + assert.Zero(t, cfg.MappingBatchWatchdog, "Invalid legacy watchdog values should retain their default") + }, + }, + { + name: "empty performance value remains absent", + envName: "NB_PROXY_PREALLOCATED_BUFFERS", + value: "", + validate: func(t *testing.T, cfg *commandConfig) { + assert.Nil(t, cfg.PreallocatedBuffers, "Empty legacy performance values should remain absent") + }, + }, + { + name: "invalid performance value remains fatal", + envName: "NB_PROXY_PREALLOCATED_BUFFERS", + value: "invalid", + wantErr: true, + }, + { + name: "empty maximum batch remains absent", + envName: "NB_PROXY_MAX_BATCH_SIZE", + value: "", + validate: func(t *testing.T, cfg *commandConfig) { + assert.Nil(t, cfg.MaxBatchSize, "Empty legacy maximum batch values should remain absent") + }, + }, + { + name: "invalid maximum batch remains fatal", + envName: "NB_PROXY_MAX_BATCH_SIZE", + value: "invalid", + wantErr: true, + }, + { + name: "empty string clears default", + envName: "NB_PROXY_MANAGEMENT_ADDRESS", + value: "", + validate: func(t *testing.T, cfg *commandConfig) { + assert.Empty(t, cfg.ManagementAddress, "Empty legacy strings should continue to clear their default") + }, + }, + } + + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + clearProxyConfigEnvironment(t) + t.Setenv(test.envName, test.value) + cmd := newLegacyProxyCommand(t) + + cfg, err := loadConfig(cmd, "") + if test.wantErr { + assert.Error(t, err, "Legacy-fatal environment input should remain fatal") + return + } + if !assert.NoError(t, err, "Legacy fallback parsing should not abort Proxy startup") { + return + } + test.validate(t, cfg) + }) + } +} + +func TestLegacyProxyFlagsRemainRegistered(t *testing.T) { + for _, name := range []string{ + "mgmt", + "addr", + "domain", + "cert-dir", + "acme-certs", + "acme-addr", + "acme-dir", + "acme-eab-kid", + "acme-eab-hmac-key", + "acme-challenge-type", + "debug-endpoint", + "debug-endpoint-addr", + "health-addr", + "forwarded-proto", + "trusted-proxies", + "cert-file", + "cert-key-file", + "cert-lock-method", + "wildcard-cert-dir", + "wg-port", + "proxy-protocol", + "preshared-key", + "supports-custom-ports", + "require-subdomain", + "private", + "max-dial-timeout", + "max-session-idle-timeout", + "geo-data-dir", + "crowdsec-api-url", + "crowdsec-api-key", + } { + assert.NotNil(t, rootCmd.Flags().Lookup(name), "Legacy flag %s should remain registered", name) + } + for _, name := range []string{"log-level", "debug"} { + assert.NotNil(t, rootCmd.PersistentFlags().Lookup(name), "Legacy persistent flag %s should remain registered", name) + } +} + +func TestLoadConfigIgnoresEmptyBooleanEnvironment(t *testing.T) { + clearProxyConfigEnvironment(t) + t.Setenv("NB_PROXY_SUPPORTS_CUSTOM_PORTS", "") + cmd := newLegacyProxyCommand(t) + + cfg, err := loadConfig(cmd, "") + require.NoError(t, err) + assert.True(t, cfg.SupportsCustomPorts, + "An empty boolean environment variable should retain the previous default") +} + +func TestDeprecatedDebugEnvironmentOverridesLogLevelFlag(t *testing.T) { + clearProxyConfigEnvironment(t) + t.Setenv("NB_PROXY_TOKEN", "test-token") + t.Setenv("NB_PROXY_DOMAIN", "invalid domain") + t.Setenv("NB_PROXY_DEBUG_LOGS", "true") + cmd := newLegacyProxyCommand(t) + + logLevelFlag := cmd.Flags().Lookup("log-level") + require.NotNil(t, logLevelFlag, "Log level flag should be registered") + oldDebugLogs := debugLogs + oldConfigPath := configPath + t.Cleanup(func() { + debugLogs = oldDebugLogs + configPath = oldConfigPath + }) + + require.NoError(t, logLevelFlag.Value.Set("trace")) + logLevelFlag.Changed = true + debugLogs = envBoolOrDefault("NB_PROXY_DEBUG_LOGS", false) + configPath = "" + require.True(t, debugLogs, "Deprecated debug environment variable should be enabled") + + var runErr error + output := captureStderr(t, func() { + runErr = runServer(cmd, nil) + }) + require.Error(t, runErr) + assert.Contains(t, output, "configured log level: debug", + "The deprecated debug environment variable should retain its previous precedence") +} + func TestLoadConfigPrecedence(t *testing.T) { + clearProxyConfigEnvironment(t) configPath := filepath.Join(t.TempDir(), "proxy.yaml") require.NoError(t, os.WriteFile(configPath, []byte(` domain: file.example.com @@ -25,19 +301,13 @@ trustedProxies: 192.0.2.0/24 maxDialTimeout: 5s `), 0o600)) t.Setenv("NB_PROXY_TOKEN", "environment-token") - require.NoError(t, rootCmd.ParseFlags(nil)) + cmd := newLegacyProxyCommand(t) - domainFlag := rootCmd.Flags().Lookup("domain") - oldDomain := domainFlag.Value.String() - oldChanged := domainFlag.Changed - t.Cleanup(func() { - require.NoError(t, domainFlag.Value.Set(oldDomain)) - domainFlag.Changed = oldChanged - }) + domainFlag := cmd.Flags().Lookup("domain") require.NoError(t, domainFlag.Value.Set("flag.example.com")) domainFlag.Changed = true - cfg, err := loadConfig(rootCmd, configPath) + cfg, err := loadConfig(cmd, configPath) require.NoError(t, err) assert.Equal(t, "flag.example.com", cfg.ProxyURL, "Flags should override the configuration file") assert.Equal(t, "environment-token", cfg.ProxyToken, "Environment should populate secrets") @@ -45,3 +315,765 @@ maxDialTimeout: 5s require.NotNil(t, cfg.TrustedProxies, "Trusted proxies should be decoded") assert.False(t, cfg.TrustedProxies.Empty(), "Configured trusted proxies should not be empty") } + +// proxy-01: legacy read only the explicit NB_PROXY_* names; NB_ style +// variables were never consulted. +func TestLoadConfigIgnoresAutomaticEnvironmentAliases(t *testing.T) { + tests := []struct { + name string + alias string + value string + legacy map[string]string + validate func(*testing.T, *commandConfig) + }{ + { + name: "listen address alias is ignored", + alias: "NB_LISTENADDRESS", + value: ":9999", + legacy: map[string]string{"NB_PROXY_ADDRESS": ":8443"}, + validate: func(t *testing.T, cfg *commandConfig) { + assert.Equal(t, ":8443", cfg.ListenAddr, + "Legacy only read NB_PROXY_ADDRESS; NB_LISTENADDRESS must not override it") + }, + }, + { + name: "log level alias is ignored", + alias: "NB_LOGLEVEL", + value: "trace", + legacy: map[string]string{"NB_PROXY_LOG_LEVEL": "warn"}, + validate: func(t *testing.T, cfg *commandConfig) { + assert.Equal(t, "warn", cfg.LogLevel, + "Legacy only read NB_PROXY_LOG_LEVEL; NB_LOGLEVEL must not override it") + }, + }, + { + name: "token alias is ignored", + alias: "NB_PROXYTOKEN", + value: "alias-token", + validate: func(t *testing.T, cfg *commandConfig) { + assert.Empty(t, cfg.ProxyToken, + "Legacy only read NB_PROXY_TOKEN; NB_PROXYTOKEN must not satisfy the token requirement") + }, + }, + { + name: "private alias is ignored", + alias: "NB_PRIVATE", + value: "true", + validate: func(t *testing.T, cfg *commandConfig) { + assert.False(t, cfg.Private, + "Legacy only read NB_PROXY_PRIVATE; NB_PRIVATE must not enable private mode") + }, + }, + { + name: "id alias is ignored", + alias: "NB_ID", + value: "alias-id", + validate: func(t *testing.T, cfg *commandConfig) { + assert.Empty(t, cfg.ID, "Legacy never read NB_ID; the proxy ID must stay empty") + }, + }, + { + name: "management address alias is ignored", + alias: "NB_MANAGEMENTADDRESS", + value: "https://alias.example.com:443", + legacy: map[string]string{"NB_PROXY_MANAGEMENT_ADDRESS": "https://management.example.com:443"}, + validate: func(t *testing.T, cfg *commandConfig) { + assert.Equal(t, "https://management.example.com:443", cfg.ManagementAddress, + "Legacy only read NB_PROXY_MANAGEMENT_ADDRESS; NB_MANAGEMENTADDRESS must not override it") + }, + }, + { + name: "wireguard port alias is ignored", + alias: "NB_WIREGUARDPORT", + value: "51820", + validate: func(t *testing.T, cfg *commandConfig) { + assert.Zero(t, cfg.WireguardPort, + "Legacy only read NB_PROXY_WG_PORT; NB_WIREGUARDPORT must not set the tunnel port") + }, + }, + } + + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + clearProxyConfigEnvironment(t) + for name, value := range test.legacy { + t.Setenv(name, value) + } + t.Setenv(test.alias, test.value) + cmd := newLegacyProxyCommand(t) + + cfg, err := loadConfig(cmd, "") + require.NoError(t, err) + test.validate(t, cfg) + }) + } +} + +// proxy-02: NB_DOMAIN is a common self-hosted variable; legacy ignored it and +// only honoured NB_PROXY_DOMAIN / --domain. +func TestLoadConfigIgnoresSharedDomainEnvironment(t *testing.T) { + tests := []struct { + name string + domain string + }{ + {name: "valid shared domain is ignored", domain: "netbird.example.org"}, + {name: "invalid shared domain is ignored", domain: "bad domain"}, + } + + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + clearProxyConfigEnvironment(t) + t.Setenv("NB_PROXY_DOMAIN", "proxy.example.com") + t.Setenv("NB_DOMAIN", test.domain) + cmd := newLegacyProxyCommand(t) + + cfg, err := loadConfig(cmd, "") + require.NoError(t, err) + assert.Equal(t, "proxy.example.com", cfg.ProxyURL, + "Legacy only read NB_PROXY_DOMAIN; the shared NB_DOMAIN variable must not override it") + }) + } +} + +// proxy-03: legacy had no NB_PROXY_ID binding; the ID was always generated at +// Server.Start time. +func TestLoadConfigIgnoresProxyIDEnvironment(t *testing.T) { + clearProxyConfigEnvironment(t) + t.Setenv("NB_PROXY_ID", "my-proxy") + cmd := newLegacyProxyCommand(t) + + cfg, err := loadConfig(cmd, "") + require.NoError(t, err) + assert.Empty(t, cfg.ID, + "Legacy never read NB_PROXY_ID; the proxy ID must stay empty so Server.Start generates it") +} + +// proxy-04: legacy strconv.ParseBool rejected yes/no/on/off/y/n, logging a +// warning and keeping the flag default. +func TestLoadConfigPreservesLegacyBooleanVocabulary(t *testing.T) { + tests := []struct { + name string + envName string + value string + validate func(*testing.T, *commandConfig) + }{ + { + name: "no keeps custom ports enabled", + envName: "NB_PROXY_SUPPORTS_CUSTOM_PORTS", + value: "no", + validate: func(t *testing.T, cfg *commandConfig) { + assert.True(t, cfg.SupportsCustomPorts, "Legacy ParseBool rejected \"no\" and kept the default true") + }, + }, + { + name: "off keeps custom ports enabled", + envName: "NB_PROXY_SUPPORTS_CUSTOM_PORTS", + value: "off", + validate: func(t *testing.T, cfg *commandConfig) { + assert.True(t, cfg.SupportsCustomPorts, "Legacy ParseBool rejected \"off\" and kept the default true") + }, + }, + { + name: "n keeps custom ports enabled", + envName: "NB_PROXY_SUPPORTS_CUSTOM_PORTS", + value: "n", + validate: func(t *testing.T, cfg *commandConfig) { + assert.True(t, cfg.SupportsCustomPorts, "Legacy ParseBool rejected \"n\" and kept the default true") + }, + }, + { + name: "uppercase OFF keeps custom ports enabled", + envName: "NB_PROXY_SUPPORTS_CUSTOM_PORTS", + value: "OFF", + validate: func(t *testing.T, cfg *commandConfig) { + assert.True(t, cfg.SupportsCustomPorts, "Legacy ParseBool rejected \"OFF\" and kept the default true") + }, + }, + { + name: "yes keeps ACME disabled", + envName: "NB_PROXY_ACME_CERTIFICATES", + value: "yes", + validate: func(t *testing.T, cfg *commandConfig) { + assert.False(t, cfg.GenerateACMECertificates, "Legacy ParseBool rejected \"yes\" and kept the default false") + }, + }, + { + name: "uppercase ON keeps ACME disabled", + envName: "NB_PROXY_ACME_CERTIFICATES", + value: "ON", + validate: func(t *testing.T, cfg *commandConfig) { + assert.False(t, cfg.GenerateACMECertificates, "Legacy ParseBool rejected \"ON\" and kept the default false") + }, + }, + { + name: "y keeps ACME disabled", + envName: "NB_PROXY_ACME_CERTIFICATES", + value: "y", + validate: func(t *testing.T, cfg *commandConfig) { + assert.False(t, cfg.GenerateACMECertificates, "Legacy ParseBool rejected \"y\" and kept the default false") + }, + }, + { + name: "on keeps PROXY protocol disabled", + envName: "NB_PROXY_PROXY_PROTOCOL", + value: "on", + validate: func(t *testing.T, cfg *commandConfig) { + assert.False(t, cfg.ProxyProtocol, "Legacy ParseBool rejected \"on\" and kept the default false") + }, + }, + { + name: "Y keeps subdomain requirement disabled", + envName: "NB_PROXY_REQUIRE_SUBDOMAIN", + value: "Y", + validate: func(t *testing.T, cfg *commandConfig) { + assert.False(t, cfg.RequireSubdomain, "Legacy ParseBool rejected \"Y\" and kept the default false") + }, + }, + { + name: "on keeps private mode disabled", + envName: "NB_PROXY_PRIVATE", + value: "on", + validate: func(t *testing.T, cfg *commandConfig) { + assert.False(t, cfg.Private, "Legacy ParseBool rejected \"on\" and kept the default false") + }, + }, + } + + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + clearProxyConfigEnvironment(t) + t.Setenv(test.envName, test.value) + cmd := newLegacyProxyCommand(t) + + cfg, err := loadConfig(cmd, "") + if !assert.NoError(t, err, "Legacy boolean fallback parsing should not abort Proxy startup") { + return + } + test.validate(t, cfg) + }) + } +} + +// proxy-05: legacy parsed NB_PROXY_WG_PORT with strconv.ParseUint(v, 10, 16). +func TestLoadConfigPreservesLegacyWireguardPortParsing(t *testing.T) { + tests := []struct { + name string + value string + want uint16 + }{ + {name: "leading zero is decimal", value: "010", want: 10}, + {name: "hex prefix falls back to default", value: "0x1F", want: 0}, + {name: "octal prefix falls back to default", value: "0o17", want: 0}, + {name: "underscore separator falls back to default", value: "1_000", want: 0}, + } + + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + clearProxyConfigEnvironment(t) + t.Setenv("NB_PROXY_WG_PORT", test.value) + cmd := newLegacyProxyCommand(t) + + cfg, err := loadConfig(cmd, "") + if !assert.NoError(t, err, "Legacy uint16 fallback parsing should not abort Proxy startup") { + return + } + assert.Equal(t, test.want, cfg.WireguardPort, + "Legacy parsed NB_PROXY_WG_PORT=%q in base 10 (parse errors fell back to 0)", test.value) + }) + } +} + +// proxy-07: legacy time.ParseDuration("") failed, logging a warning and +// keeping the default of 0; startup continued. +func TestLoadConfigPreservesLegacyEmptyDurationEnvironment(t *testing.T) { + tests := []struct { + name string + envName string + validate func(*testing.T, *commandConfig) + }{ + { + name: "empty dial timeout uses default", + envName: "NB_PROXY_MAX_DIAL_TIMEOUT", + validate: func(t *testing.T, cfg *commandConfig) { + assert.Zero(t, cfg.MaxDialTimeout, "Legacy treated an empty dial timeout as the default 0") + }, + }, + { + name: "empty idle timeout uses default", + envName: "NB_PROXY_MAX_SESSION_IDLE_TIMEOUT", + validate: func(t *testing.T, cfg *commandConfig) { + assert.Zero(t, cfg.MaxSessionIdleTimeout, "Legacy treated an empty idle timeout as the default 0") + }, + }, + { + name: "empty watchdog uses default", + envName: "NB_PROXY_MAPPING_BATCH_WATCHDOG", + validate: func(t *testing.T, cfg *commandConfig) { + assert.Zero(t, cfg.MappingBatchWatchdog, "Legacy treated an empty watchdog as the default 0") + }, + }, + } + + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + clearProxyConfigEnvironment(t) + t.Setenv(test.envName, "") + cmd := newLegacyProxyCommand(t) + + cfg, err := loadConfig(cmd, "") + if !assert.NoError(t, err, "Legacy treated an empty duration environment value as a warning, not a fatal error") { + return + } + test.validate(t, cfg) + }) + } +} + +// proxy-08: legacy time.ParseDuration errors (missing unit / invalid) logged a +// warning and kept the default of 0; startup continued. +func TestLoadConfigPreservesLegacyDurationFallbacks(t *testing.T) { + tests := []struct { + name string + envName string + value string + validate func(*testing.T, *commandConfig) + }{ + { + name: "unit-less dial timeout uses default", + envName: "NB_PROXY_MAX_DIAL_TIMEOUT", + value: "5", + validate: func(t *testing.T, cfg *commandConfig) { + assert.Zero(t, cfg.MaxDialTimeout, "Legacy treated a unit-less dial timeout as the default 0") + }, + }, + { + name: "unit-less idle timeout uses default", + envName: "NB_PROXY_MAX_SESSION_IDLE_TIMEOUT", + value: "5", + validate: func(t *testing.T, cfg *commandConfig) { + assert.Zero(t, cfg.MaxSessionIdleTimeout, "Legacy treated a unit-less idle timeout as the default 0") + }, + }, + { + name: "invalid idle timeout uses default", + envName: "NB_PROXY_MAX_SESSION_IDLE_TIMEOUT", + value: "invalid", + validate: func(t *testing.T, cfg *commandConfig) { + assert.Zero(t, cfg.MaxSessionIdleTimeout, "Legacy treated an invalid idle timeout as the default 0") + }, + }, + } + + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + clearProxyConfigEnvironment(t) + t.Setenv(test.envName, test.value) + cmd := newLegacyProxyCommand(t) + + cfg, err := loadConfig(cmd, "") + if !assert.NoError(t, err, "Legacy treated an unparseable duration environment value as a warning, not a fatal error") { + return + } + test.validate(t, cfg) + }) + } +} + +// proxy-09: legacy parsed the performance variables with +// strconv.ParseUint(raw, 10, 32) and aborted startup on any parse error. +func TestLoadConfigPreservesLegacyPerformanceParsing(t *testing.T) { + tests := []struct { + name string + envName string + value string + want uint32 + wantErr bool + }{ + {name: "preallocated buffers leading zero is decimal", envName: "NB_PROXY_PREALLOCATED_BUFFERS", value: "010", want: 10}, + {name: "preallocated buffers two leading zeros is decimal", envName: "NB_PROXY_PREALLOCATED_BUFFERS", value: "0100", want: 100}, + {name: "preallocated buffers hex remains fatal", envName: "NB_PROXY_PREALLOCATED_BUFFERS", value: "0x10", wantErr: true}, + {name: "preallocated buffers long hex remains fatal", envName: "NB_PROXY_PREALLOCATED_BUFFERS", value: "0x100", wantErr: true}, + {name: "preallocated buffers underscore remains fatal", envName: "NB_PROXY_PREALLOCATED_BUFFERS", value: "1_000", wantErr: true}, + {name: "maximum batch leading zero is decimal", envName: "NB_PROXY_MAX_BATCH_SIZE", value: "010", want: 10}, + {name: "maximum batch two leading zeros is decimal", envName: "NB_PROXY_MAX_BATCH_SIZE", value: "0100", want: 100}, + {name: "maximum batch hex remains fatal", envName: "NB_PROXY_MAX_BATCH_SIZE", value: "0x10", wantErr: true}, + {name: "maximum batch long hex remains fatal", envName: "NB_PROXY_MAX_BATCH_SIZE", value: "0x100", wantErr: true}, + {name: "maximum batch underscore remains fatal", envName: "NB_PROXY_MAX_BATCH_SIZE", value: "1_000", wantErr: true}, + } + + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + clearProxyConfigEnvironment(t) + t.Setenv(test.envName, test.value) + cmd := newLegacyProxyCommand(t) + + cfg, err := loadConfig(cmd, "") + if test.wantErr { + assert.Error(t, err, + "Legacy strconv.ParseUint(%q, 10, 32) failed and aborted Proxy startup", test.value) + return + } + if !assert.NoError(t, err, "Legacy base-10 performance values should not abort Proxy startup") { + return + } + got := cfg.PreallocatedBuffers + if test.envName == "NB_PROXY_MAX_BATCH_SIZE" { + got = cfg.MaxBatchSize + } + require.NotNil(t, got, "Legacy performance values should be present when set") + assert.Equal(t, test.want, *got, + "Legacy parsed %s=%q in base 10", test.envName, test.value) + }) + } +} + +// proxy-10 / proxy-12: legacy read the token only from NB_PROXY_TOKEN, checked +// it before anything else, and reported a fixed message when it was missing. +func TestRunServerPreservesLegacyTokenRequirement(t *testing.T) { + const legacyTokenError = "proxy token is required: set NB_PROXY_TOKEN environment variable" + + tests := []struct { + name string + env map[string]string + fileConfig string + }{ + { + name: "missing token without other sources", + }, + { + name: "file token does not satisfy the requirement", + fileConfig: "proxyToken: file-token\ndomain: \"invalid domain\"\n", + }, + { + name: "token alias does not satisfy the requirement", + env: map[string]string{"NB_PROXYTOKEN": "alias-token", "NB_PROXY_DOMAIN": "invalid domain"}, + }, + { + name: "empty token with file token remains fatal", + env: map[string]string{"NB_PROXY_TOKEN": ""}, + fileConfig: "proxyToken: file-token\ndomain: \"invalid domain\"\n", + }, + { + name: "missing token is reported before performance parsing", + env: map[string]string{"NB_PROXY_PREALLOCATED_BUFFERS": "invalid"}, + }, + } + + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + clearProxyConfigEnvironment(t) + for name, value := range test.env { + t.Setenv(name, value) + } + cmd := newLegacyProxyCommand(t) + + oldDebugLogs := debugLogs + oldConfigPath := configPath + t.Cleanup(func() { + debugLogs = oldDebugLogs + configPath = oldConfigPath + }) + debugLogs = false + configPath = "" + if test.fileConfig != "" { + configPath = filepath.Join(t.TempDir(), "proxy.yaml") + require.NoError(t, os.WriteFile(configPath, []byte(test.fileConfig), 0o600)) + } + + var runErr error + captureStderr(t, func() { + runErr = runServer(cmd, nil) + }) + require.Error(t, runErr) + assert.EqualError(t, runErr, legacyTokenError, + "Legacy accepted the token only from NB_PROXY_TOKEN and reported it before any other startup step") + }) + } +} + +// proxy-12: with a valid token, legacy initialised the logger before parsing +// the performance variables, so the log-level line preceded the parse error. +func TestRunServerPreservesLegacyPerformanceErrorOrdering(t *testing.T) { + clearProxyConfigEnvironment(t) + t.Setenv("NB_PROXY_TOKEN", "test-token") + t.Setenv("NB_PROXY_PREALLOCATED_BUFFERS", "invalid") + cmd := newLegacyProxyCommand(t) + + oldDebugLogs := debugLogs + oldConfigPath := configPath + t.Cleanup(func() { + debugLogs = oldDebugLogs + configPath = oldConfigPath + }) + debugLogs = false + configPath = "" + + var runErr error + output := captureStderr(t, func() { + runErr = runServer(cmd, nil) + }) + require.Error(t, runErr) + assert.Contains(t, output, "configured log level: info", + "Legacy initialised the logger before parsing the performance environment variables") + assert.EqualError(t, runErr, `invalid NB_PROXY_PREALLOCATED_BUFFERS "invalid": strconv.ParseUint: parsing "invalid": invalid syntax`, + "Legacy reported the performance parse error with the variable name after logger initialisation") +} + +// proxy-13: legacy had no --config flag; cobra rejected it as unknown. +func TestLegacyProxyRejectsConfigFlag(t *testing.T) { + oldConfigPath := configPath + t.Cleanup(func() { + configPath = oldConfigPath + if flag := rootCmd.Flags().Lookup("config"); flag != nil { + require.NoError(t, flag.Value.Set(oldConfigPath)) + flag.Changed = false + } + }) + + err := rootCmd.ParseFlags([]string{"--config", filepath.Join(t.TempDir(), "proxy.yaml")}) + require.Error(t, err, "Legacy proxy command did not register a --config flag") + assert.EqualError(t, err, "unknown flag: --config", + "Legacy cobra rejected --config as an unknown flag") +} + +// proxy-critic-1: legacy parsed --trusted-proxies / NB_PROXY_TRUSTED_PROXIES +// last in runServer, after the token check, logger initialisation and the +// forwarded-proto and domain validation, and reported it as +// `invalid --trusted-proxies: ...`. +func TestRunServerPreservesLegacyTrustedProxiesValidationOrdering(t *testing.T) { + const legacyTokenError = "proxy token is required: set NB_PROXY_TOKEN environment variable" + + tests := []struct { + name string + env map[string]string + flagValue string + wantErr string + wantErrPart string + wantLogLine bool + }{ + { + name: "missing token is reported before trusted proxy parsing", + env: map[string]string{"NB_PROXY_TRUSTED_PROXIES": "not-an-ip"}, + wantErr: legacyTokenError, + }, + { + name: "invalid forwarded-proto is reported before trusted proxy parsing", + env: map[string]string{ + "NB_PROXY_TOKEN": "test-token", + "NB_PROXY_FORWARDED_PROTO": "bogus", + "NB_PROXY_TRUSTED_PROXIES": "not-an-ip", + }, + wantErr: `invalid --forwarded-proto value "bogus": must be auto, http, or https`, + wantLogLine: true, + }, + { + name: "invalid domain is reported before trusted proxy parsing", + env: map[string]string{ + "NB_PROXY_TOKEN": "test-token", + "NB_PROXY_DOMAIN": "invalid domain", + "NB_PROXY_TRUSTED_PROXIES": "not-an-ip", + }, + wantErrPart: `invalid domain value "invalid domain"`, + wantLogLine: true, + }, + { + name: "invalid trusted proxy environment is reported after logger initialisation", + env: map[string]string{ + "NB_PROXY_TOKEN": "test-token", + "NB_PROXY_DOMAIN": "proxy.example.com", + "NB_PROXY_TRUSTED_PROXIES": "not-an-ip", + }, + wantErr: `invalid --trusted-proxies: parse trusted proxy "not-an-ip": not a valid CIDR or IP: ParseAddr("not-an-ip"): unable to parse IP`, + wantLogLine: true, + }, + { + name: "invalid trusted proxy flag is reported after logger initialisation", + env: map[string]string{ + "NB_PROXY_TOKEN": "test-token", + "NB_PROXY_DOMAIN": "proxy.example.com", + }, + flagValue: "10.0.0.0/8,garbage", + wantErr: `invalid --trusted-proxies: parse trusted proxy "garbage": not a valid CIDR or IP: ParseAddr("garbage"): unable to parse IP`, + wantLogLine: true, + }, + } + + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + clearProxyConfigEnvironment(t) + for name, value := range test.env { + t.Setenv(name, value) + } + cmd := newLegacyProxyCommand(t) + if test.flagValue != "" { + trustedProxiesFlag := cmd.Flags().Lookup("trusted-proxies") + require.NotNil(t, trustedProxiesFlag, "Trusted proxies flag should be registered") + require.NoError(t, trustedProxiesFlag.Value.Set(test.flagValue)) + trustedProxiesFlag.Changed = true + } + + oldDebugLogs := debugLogs + oldConfigPath := configPath + t.Cleanup(func() { + debugLogs = oldDebugLogs + configPath = oldConfigPath + }) + debugLogs = false + configPath = "" + + var runErr error + output := captureStderr(t, func() { + runErr = runServer(cmd, nil) + }) + require.Error(t, runErr) + if test.wantLogLine { + assert.Contains(t, output, "configured log level: info", + "Legacy initialised the logger before validating the trusted proxy list") + } else { + assert.NotContains(t, output, "configured log level", + "Legacy checked the token before initialising the logger") + } + if test.wantErr != "" { + assert.EqualError(t, runErr, test.wantErr, + "Legacy parsed the trusted proxy list last in runServer and reported it as invalid --trusted-proxies") + } + if test.wantErrPart != "" { + assert.ErrorContains(t, runErr, test.wantErrPart, + "Legacy validated the domain before parsing the trusted proxy list") + assert.NotContains(t, runErr.Error(), "trusted prox", + "Legacy never mentioned trusted proxies when an earlier validation step failed") + } + }) + } +} + +func newLegacyProxyCommand(t *testing.T) *cobra.Command { + t.Helper() + + defaults := legacyProxyDefaults(t) + cmd := &cobra.Command{Use: "proxy-test"} + flags := cmd.Flags() + flags.String("log-level", defaults.LogLevel, "") + flags.Bool("debug", false, "") + flags.String("mgmt", defaults.ManagementAddress, "") + flags.String("addr", defaults.ListenAddr, "") + flags.String("domain", defaults.ProxyURL, "") + flags.String("cert-dir", defaults.CertificateDirectory, "") + flags.Bool("acme-certs", defaults.GenerateACMECertificates, "") + flags.String("acme-addr", defaults.ACMEChallengeAddress, "") + flags.String("acme-dir", defaults.ACMEDirectory, "") + flags.String("acme-eab-kid", defaults.ACMEEABKID, "") + flags.String("acme-eab-hmac-key", defaults.ACMEEABHMACKey, "") + flags.String("acme-challenge-type", defaults.ACMEChallengeType, "") + flags.Bool("debug-endpoint", defaults.DebugEndpointEnabled, "") + flags.String("debug-endpoint-addr", defaults.DebugEndpointAddress, "") + flags.String("health-addr", defaults.HealthAddr, "") + flags.String("forwarded-proto", defaults.ForwardedProto, "") + flags.String("trusted-proxies", "", "") + flags.String("cert-file", defaults.CertificateFile, "") + flags.String("cert-key-file", defaults.CertificateKeyFile, "") + flags.String("cert-lock-method", string(defaults.CertLockMethod), "") + flags.String("wildcard-cert-dir", defaults.WildcardCertDir, "") + flags.Uint16("wg-port", defaults.WireguardPort, "") + flags.Bool("proxy-protocol", defaults.ProxyProtocol, "") + flags.String("preshared-key", defaults.PreSharedKey, "") + flags.Bool("supports-custom-ports", defaults.SupportsCustomPorts, "") + flags.Bool("require-subdomain", defaults.RequireSubdomain, "") + flags.Bool("private", defaults.Private, "") + flags.Duration("max-dial-timeout", defaults.MaxDialTimeout, "") + flags.Duration("max-session-idle-timeout", defaults.MaxSessionIdleTimeout, "") + flags.String("geo-data-dir", defaults.GeoDataDir, "") + flags.String("crowdsec-api-url", defaults.CrowdSecAPIURL, "") + flags.String("crowdsec-api-key", defaults.CrowdSecAPIKey, "") + return cmd +} + +func legacyProxyDefaults(t *testing.T) *commandConfig { + t.Helper() + + trustedProxies, err := trustedproxy.Parse("") + require.NoError(t, err) + cfg := &commandConfig{LogLevel: "info"} + cfg.ListenAddr = ":443" + cfg.ManagementAddress = DefaultManagementURL + cfg.CertificateDirectory = "./certs" + cfg.CertificateFile = "tls.crt" + cfg.CertificateKeyFile = "tls.key" + cfg.ACMEChallengeAddress = ":80" + cfg.ACMEDirectory = "https://acme-v02.api.letsencrypt.org/directory" + cfg.ACMEChallengeType = "tls-alpn-01" + cfg.CertLockMethod = "auto" + cfg.DebugEndpointAddress = "localhost:8444" + cfg.HealthAddr = "localhost:8080" + cfg.ForwardedProto = "auto" + cfg.TrustedProxies = trustedProxies + cfg.SupportsCustomPorts = true + cfg.GeoDataDir = "/var/lib/netbird/geolocation" + return cfg +} + +func clearProxyConfigEnvironment(t *testing.T) { + t.Helper() + + for _, name := range []string{ + "NB_PROXY_LOG_LEVEL", + "NB_PROXY_DEBUG_LOGS", + "NB_PROXY_MANAGEMENT_ADDRESS", + "NB_PROXY_ADDRESS", + "NB_PROXY_DOMAIN", + "NB_PROXY_TOKEN", + "NB_PROXY_CERTIFICATE_DIRECTORY", + "NB_PROXY_CERTIFICATE_FILE", + "NB_PROXY_CERTIFICATE_KEY_FILE", + "NB_PROXY_ACME_CERTIFICATES", + "NB_PROXY_ACME_ADDRESS", + "NB_PROXY_ACME_DIRECTORY", + "NB_PROXY_ACME_EAB_KID", + "NB_PROXY_ACME_EAB_HMAC_KEY", + "NB_PROXY_ACME_CHALLENGE_TYPE", + "NB_PROXY_CERT_LOCK_METHOD", + "NB_PROXY_WILDCARD_CERT_DIR", + "NB_PROXY_DEBUG_ENDPOINT", + "NB_PROXY_DEBUG_ENDPOINT_ADDRESS", + "NB_PROXY_HEALTH_ADDRESS", + "NB_PROXY_FORWARDED_PROTO", + "NB_PROXY_TRUSTED_PROXIES", + "NB_PROXY_WG_PORT", + "NB_PROXY_PROXY_PROTOCOL", + "NB_PROXY_PRESHARED_KEY", + "NB_PROXY_SUPPORTS_CUSTOM_PORTS", + "NB_PROXY_REQUIRE_SUBDOMAIN", + "NB_PROXY_PRIVATE", + "NB_PROXY_MAX_DIAL_TIMEOUT", + "NB_PROXY_MAX_SESSION_IDLE_TIMEOUT", + "NB_PROXY_MAPPING_BATCH_WATCHDOG", + "NB_PROXY_GEO_DATA_DIR", + "NB_PROXY_CROWDSEC_API_URL", + "NB_PROXY_CROWDSEC_API_KEY", + "NB_PROXY_PREALLOCATED_BUFFERS", + "NB_PROXY_MAX_BATCH_SIZE", + } { + t.Setenv(name, "") + require.NoError(t, os.Unsetenv(name)) + } +} + +func captureStderr(t *testing.T, fn func()) string { + t.Helper() + + reader, writer, err := os.Pipe() + require.NoError(t, err) + oldStderr := os.Stderr + os.Stderr = writer + defer func() { + os.Stderr = oldStderr + }() + fn() + os.Stderr = oldStderr + require.NoError(t, writer.Close()) + + output, err := io.ReadAll(reader) + require.NoError(t, err) + require.NoError(t, reader.Close()) + return string(output) +} diff --git a/relay/cmd/config_test.go b/relay/cmd/config_test.go index d93b12be6..d037a66fb 100644 --- a/relay/cmd/config_test.go +++ b/relay/cmd/config_test.go @@ -1,15 +1,20 @@ package cmd import ( + "bytes" + "math" "os" "path/filepath" "testing" + log "github.com/sirupsen/logrus" + "github.com/spf13/pflag" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" ) func TestExampleConfig(t *testing.T) { + clearRelayConfigEnvironment(t) require.NoError(t, rootCmd.ParseFlags(nil)) oldConfigPath := configPath configPath = filepath.Join("..", "config.example.yaml") @@ -23,7 +28,168 @@ func TestExampleConfig(t *testing.T) { assert.True(t, cfg.EnableSTUN, "Example config should enable STUN") } +func TestLoadConfigPreservesLegacyEffectiveDefaults(t *testing.T) { + clearRelayConfigEnvironment(t) + + cfg, err := loadRelayConfigWithoutFile(t) + require.NoError(t, err) + cfg.LetsencryptDomains = nil + assert.Equal(t, defaultConfig(), cfg, "Relay effective defaults should remain unchanged") +} + +func TestLoadConfigPreservesLegacyEnvironmentBindings(t *testing.T) { + clearRelayConfigEnvironment(t) + t.Setenv("NB_LISTEN_ADDRESS", ":7443") + t.Setenv("NB_EXPOSED_ADDRESS", "rels://relay.example.com:443") + t.Setenv("NB_METRICS_PORT", "9191") + t.Setenv("NB_LETSENCRYPT_EMAIL", "admin@example.com") + t.Setenv("NB_LETSENCRYPT_DATA_DIR", "/var/lib/relay/certs") + t.Setenv("NB_LETSENCRYPT_DOMAINS", "relay.example.com,relay-alt.example.com") + t.Setenv("NB_LETSENCRYPT_AWS_ROUTE53", "true") + t.Setenv("NB_TLS_CERT_FILE", "/etc/relay/tls.crt") + t.Setenv("NB_TLS_KEY_FILE", "/etc/relay/tls.key") + t.Setenv("NB_AUTH_SECRET", "relay-secret") + t.Setenv("NB_LOG_LEVEL", "debug") + t.Setenv("NB_LOG_FILE", "/var/log/relay.log") + t.Setenv("NB_HEALTH_LISTEN_ADDRESS", ":9001") + t.Setenv("NB_TRUSTED_PROXIES", "192.0.2.0/24") + t.Setenv("NB_ENABLE_STUN", "true") + t.Setenv("NB_STUN_PORTS", "3479,3480") + t.Setenv("NB_STUN_LOG_LEVEL", "trace") + + cfg, err := loadRelayConfigWithoutFile(t) + require.NoError(t, err) + assert.Equal(t, &Config{ + ListenAddress: ":7443", + ExposedAddress: "rels://relay.example.com:443", + MetricsPort: 9191, + LetsencryptEmail: "admin@example.com", + LetsencryptDataDir: "/var/lib/relay/certs", + LetsencryptDomains: []string{"relay.example.com", "relay-alt.example.com"}, + LetsencryptAWSRoute53: true, + TlsCertFile: "/etc/relay/tls.crt", + TlsKeyFile: "/etc/relay/tls.key", + AuthSecret: "relay-secret", + LogLevel: "debug", + LogFile: "/var/log/relay.log", + HealthcheckListenAddress: ":9001", + TrustedProxies: "192.0.2.0/24", + EnableSTUN: true, + STUNPorts: []int{3479, 3480}, + STUNLogLevel: "trace", + }, cfg, "Every legacy Relay environment binding should remain supported") +} + +func TestLoadConfigPreservesLegacyEnvironmentParsing(t *testing.T) { + tests := []struct { + name string + envName string + value string + validate func(*testing.T, *Config) + }{ + { + name: "invalid integer becomes zero", + envName: "NB_METRICS_PORT", + value: "invalid", + validate: func(t *testing.T, cfg *Config) { + assert.Zero(t, cfg.MetricsPort, "Invalid legacy integer input should retain its parsed zero value") + }, + }, + { + name: "legacy boolean spelling remains false", + envName: "NB_ENABLE_STUN", + value: "yes", + validate: func(t *testing.T, cfg *Config) { + assert.False(t, cfg.EnableSTUN, "Unsupported legacy boolean spelling should remain disabled") + }, + }, + { + name: "invalid boolean remains false", + envName: "NB_ENABLE_STUN", + value: "invalid", + validate: func(t *testing.T, cfg *Config) { + assert.False(t, cfg.EnableSTUN, "Invalid legacy boolean input should retain its default") + }, + }, + { + name: "empty integer list retains default", + envName: "NB_STUN_PORTS", + value: "", + validate: func(t *testing.T, cfg *Config) { + assert.Equal(t, []int{3478}, cfg.STUNPorts, "Empty legacy integer lists should retain their default") + }, + }, + { + name: "trailing comma integer list retains default", + envName: "NB_STUN_PORTS", + value: "3479,", + validate: func(t *testing.T, cfg *Config) { + assert.Equal(t, []int{3478}, cfg.STUNPorts, "Invalid legacy integer lists should retain their default") + }, + }, + { + name: "quoted comma in string list", + envName: "NB_LETSENCRYPT_DOMAINS", + value: `"relay,one.example.com",relay-two.example.com`, + validate: func(t *testing.T, cfg *Config) { + assert.Equal(t, []string{"relay,one.example.com", "relay-two.example.com"}, cfg.LetsencryptDomains, + "Legacy string lists should retain CSV quoting") + }, + }, + { + name: "malformed quoted string list is ignored", + envName: "NB_LETSENCRYPT_DOMAINS", + value: `"relay.example.com`, + validate: func(t *testing.T, cfg *Config) { + assert.Nil(t, cfg.LetsencryptDomains, "Malformed legacy string lists should retain their default") + }, + }, + } + + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + clearRelayConfigEnvironment(t) + t.Setenv(test.envName, test.value) + + cfg, err := loadRelayConfigWithoutFile(t) + if !assert.NoError(t, err, "Legacy environment parse errors should not abort Relay startup") { + return + } + test.validate(t, cfg) + }) + } +} + +func TestLegacyRelayFlagsRemainRegistered(t *testing.T) { + expected := map[string]string{ + "listen-address": "l", + "exposed-address": "e", + "metrics-port": "", + "letsencrypt-data-dir": "d", + "letsencrypt-domains": "a", + "letsencrypt-email": "", + "letsencrypt-aws-route53": "", + "tls-cert-file": "c", + "tls-key-file": "k", + "auth-secret": "s", + "log-level": "", + "log-file": "", + "health-listen-address": "H", + "trusted-proxies": "", + "enable-stun": "", + "stun-ports": "", + "stun-log-level": "", + } + + for name, shorthand := range expected { + flag := rootCmd.Flags().Lookup(name) + require.NotNil(t, flag, "Legacy flag %s should remain registered", name) + assert.Equal(t, shorthand, flag.Shorthand, "Legacy shorthand for %s should remain unchanged", name) + } +} + func TestLoadConfigPrecedence(t *testing.T) { + clearRelayConfigEnvironment(t) path := filepath.Join(t.TempDir(), "relay.yaml") require.NoError(t, os.WriteFile(path, []byte(` listenAddress: ":8443" @@ -60,3 +226,476 @@ stunPorts: [3478, 3479] assert.True(t, cfg.EnableSTUN, "STUN should be configurable from the file") assert.Equal(t, []int{3478, 3479}, cfg.STUNPorts, "STUN ports should load from the file") } + +func loadRelayConfigWithoutFile(t *testing.T) (*Config, error) { + t.Helper() + + require.NoError(t, rootCmd.ParseFlags(nil)) + oldConfigPath := configPath + configPath = "" + t.Cleanup(func() { + configPath = oldConfigPath + }) + return loadConfig(rootCmd) +} + +func clearRelayConfigEnvironment(t *testing.T) { + t.Helper() + + for _, name := range []string{ + "NB_LISTEN_ADDRESS", + "NB_EXPOSED_ADDRESS", + "NB_METRICS_PORT", + "NB_LETSENCRYPT_EMAIL", + "NB_LETSENCRYPT_DATA_DIR", + "NB_LETSENCRYPT_DOMAINS", + "NB_LETSENCRYPT_AWS_ROUTE53", + "NB_TLS_CERT_FILE", + "NB_TLS_KEY_FILE", + "NB_AUTH_SECRET", + "NB_LOG_LEVEL", + "NB_LOG_FILE", + "NB_HEALTH_LISTEN_ADDRESS", + "NB_TRUSTED_PROXIES", + "NB_ENABLE_STUN", + "NB_STUN_PORTS", + "NB_STUN_LOG_LEVEL", + } { + t.Setenv(name, "") + require.NoError(t, os.Unsetenv(name)) + } +} + +func TestLoadConfigPreservesLegacyIntegerOverflowParsing(t *testing.T) { + clearRelayConfigEnvironment(t) + t.Setenv("NB_METRICS_PORT", "99999999999999999999") + + cfg, err := loadRelayConfigWithoutFile(t) + if !assert.NoError(t, err, "Legacy pflag stored the clamped integer and only logged the range error, so loading continued") { + return + } + assert.Equal(t, math.MaxInt64, cfg.MetricsPort, + "Legacy strconv.ParseInt range errors left math.MaxInt64 in the metrics port instead of aborting startup") +} + +func TestLoadConfigPreservesLegacyRoute53BooleanSpelling(t *testing.T) { + for _, value := range []string{"yes", "y", "on", "YES", "On"} { + t.Run(value, func(t *testing.T) { + clearRelayConfigEnvironment(t) + t.Setenv("NB_LETSENCRYPT_AWS_ROUTE53", value) + + cfg, err := loadRelayConfigWithoutFile(t) + if !assert.NoError(t, err, "Legacy boolean parse errors should not abort Relay startup") { + return + } + assert.False(t, cfg.LetsencryptAWSRoute53, + "Legacy strconv.ParseBool rejected %q, leaving Route 53 disabled so the Route 53 TLS branch was never taken", value) + }) + } +} + +func TestLoadConfigPreservesLegacyRoute53InvalidBoolean(t *testing.T) { + tests := []struct { + name string + value string + }{ + {name: "invalid word", value: "invalid"}, + {name: "numeric two", value: "2"}, + {name: "mixed case true", value: "tRuE"}, + {name: "trailing space", value: "true "}, + {name: "enabled", value: "enabled"}, + } + + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + clearRelayConfigEnvironment(t) + t.Setenv("NB_LETSENCRYPT_AWS_ROUTE53", test.value) + + cfg, err := loadRelayConfigWithoutFile(t) + if !assert.NoError(t, err, "Legacy invalid boolean input was logged at Info and ignored, not fatal") { + return + } + assert.False(t, cfg.LetsencryptAWSRoute53, + "Legacy strconv.ParseBool rejected %q and pflag stored false, so Relay started with Route 53 disabled", test.value) + }) + } +} + +func TestLoadConfigPreservesLegacySTUNPortListRejection(t *testing.T) { + tests := []struct { + name string + value string + }{ + {name: "non-numeric trailing element", value: "3479,abc"}, + {name: "non-numeric single element", value: "abc"}, + {name: "space after comma", value: "3479, 3480"}, + } + + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + clearRelayConfigEnvironment(t) + t.Setenv("NB_STUN_PORTS", test.value) + + cfg, err := loadRelayConfigWithoutFile(t) + if !assert.NoError(t, err, "Legacy integer list parse errors were logged at Info and ignored, not fatal") { + return + } + assert.Equal(t, []int{3478}, cfg.STUNPorts, + "Legacy pflag intSlice ran strconv.Atoi on every element and rejected the whole value %q, retaining the default", test.value) + }) + } +} + +func TestLoadConfigPreservesLegacySTUNPortDecimalParsing(t *testing.T) { + tests := []struct { + name string + value string + expected []int + }{ + {name: "leading zero", value: "010", expected: []int{10}}, + {name: "double leading zero", value: "0010", expected: []int{10}}, + {name: "leading zero element", value: "3478,010", expected: []int{3478, 10}}, + } + + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + clearRelayConfigEnvironment(t) + t.Setenv("NB_STUN_PORTS", test.value) + + cfg, err := loadRelayConfigWithoutFile(t) + require.NoError(t, err) + assert.Equal(t, test.expected, cfg.STUNPorts, + "Legacy pflag intSlice used strconv.Atoi (base 10 only), so leading zeros were decimal and never octal") + }) + } +} + +func TestLoadConfigPreservesLegacySTUNPortStrictDecimalSyntax(t *testing.T) { + tests := []struct { + name string + value string + }{ + {name: "hex prefix", value: "0x10"}, + {name: "underscore separator", value: "3_479"}, + } + + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + clearRelayConfigEnvironment(t) + t.Setenv("NB_STUN_PORTS", test.value) + + cfg, err := loadRelayConfigWithoutFile(t) + if !assert.NoError(t, err, "Legacy integer list parse errors were logged at Info and ignored, not fatal") { + return + } + assert.Equal(t, []int{3478}, cfg.STUNPorts, + "Legacy strconv.Atoi rejected %q (no hex prefix or underscores), so the default STUN port list was retained", test.value) + }) + } +} + +func TestLoadConfigPreservesLegacyNewlineInDomainList(t *testing.T) { + clearRelayConfigEnvironment(t) + t.Setenv("NB_LETSENCRYPT_DOMAINS", "a.example.com\nb.example.com") + + cfg, err := loadRelayConfigWithoutFile(t) + require.NoError(t, err) + assert.Equal(t, []string{"a.example.com"}, cfg.LetsencryptDomains, + "Legacy pflag readAsCSV performed a single csv.Reader.Read, so only the first line of a newline separated value was used") +} + +func TestLoadConfigPreservesLegacyEnvAndFlagDomainAppend(t *testing.T) { + tests := []struct { + name string + flagValues []string + expected []string + }{ + { + name: "single flag value", + flagValues: []string{"b.example.com"}, + expected: []string{"a.example.com", "b.example.com"}, + }, + { + name: "repeated flag values", + flagValues: []string{"b.example.com", "c.example.com"}, + expected: []string{"a.example.com", "b.example.com", "c.example.com"}, + }, + } + + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + clearRelayConfigEnvironment(t) + t.Setenv("NB_LETSENCRYPT_DOMAINS", "a.example.com") + setRelaySliceFlag(t, "letsencrypt-domains", test.flagValues) + + cfg, err := loadRelayConfigWithoutFile(t) + require.NoError(t, err) + assert.Equal(t, test.expected, cfg.LetsencryptDomains, + "Legacy applied NB_LETSENCRYPT_DOMAINS via flags.Set before argv parsing, so command line domains were appended to the environment list") + }) + } +} + +func TestLoadConfigPreservesLegacyEnvAndFlagSTUNPortAppend(t *testing.T) { + clearRelayConfigEnvironment(t) + t.Setenv("NB_STUN_PORTS", "3479") + setRelaySliceFlag(t, "stun-ports", []string{"3480"}) + + cfg, err := loadRelayConfigWithoutFile(t) + require.NoError(t, err) + assert.Equal(t, []int{3479, 3480}, cfg.STUNPorts, + "Legacy applied NB_STUN_PORTS via flags.Set before argv parsing, so --stun-ports appended to the environment list and both UDP ports were bound") +} + +func TestLoadConfigPreservesLegacyDuplicateSTUNPortFromEnvAndFlag(t *testing.T) { + clearRelayConfigEnvironment(t) + t.Setenv("NB_EXPOSED_ADDRESS", "rels://relay.example.com:443") + t.Setenv("NB_AUTH_SECRET", "relay-secret") + t.Setenv("NB_ENABLE_STUN", "true") + t.Setenv("NB_STUN_PORTS", "3478") + setRelaySliceFlag(t, "stun-ports", []string{"3478"}) + + cfg, err := loadRelayConfigWithoutFile(t) + require.NoError(t, err) + assert.Equal(t, []int{3478, 3478}, cfg.STUNPorts, + "Legacy appended the --stun-ports value to the NB_STUN_PORTS value, producing a duplicated port list") + + err = cfg.Validate() + if assert.Error(t, err, "Legacy Validate rejected the duplicated STUN port list and Relay exited") { + assert.Contains(t, err.Error(), "duplicate STUN port 3478", "Legacy reported the duplicate STUN port") + } +} + +func TestLoadConfigPreservesLegacyEnvironmentNameMatching(t *testing.T) { + tests := []struct { + name string + env map[string]string + validate func(*testing.T, *Config) + }{ + { + name: "listen address alias does not outrank legacy name", + env: map[string]string{"NB_LISTENADDRESS": ":1", "NB_LISTEN_ADDRESS": ":2"}, + validate: func(t *testing.T, cfg *Config) { + assert.Equal(t, ":2", cfg.ListenAddress, + "Legacy only read NB_LISTEN_ADDRESS; the un-underscored NB_LISTENADDRESS was ignored") + }, + }, + { + name: "healthcheck alias is ignored", + env: map[string]string{"NB_HEALTHCHECKLISTENADDRESS": ":1"}, + validate: func(t *testing.T, cfg *Config) { + assert.Equal(t, ":9000", cfg.HealthcheckListenAddress, + "Legacy only read NB_HEALTH_LISTEN_ADDRESS; NB_HEALTHCHECKLISTENADDRESS was ignored and the default retained") + }, + }, + { + name: "stun ports alias is ignored", + env: map[string]string{"NB_STUNPORTS": "1111"}, + validate: func(t *testing.T, cfg *Config) { + assert.Equal(t, []int{3478}, cfg.STUNPorts, + "Legacy only read NB_STUN_PORTS; NB_STUNPORTS was ignored and the default retained") + }, + }, + { + name: "auth secret alias is ignored", + env: map[string]string{"NB_AUTHSECRET": "alias-secret"}, + validate: func(t *testing.T, cfg *Config) { + assert.Empty(t, cfg.AuthSecret, + "Legacy only read NB_AUTH_SECRET; NB_AUTHSECRET was ignored") + }, + }, + } + + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + clearRelayConfigEnvironment(t) + for name, value := range test.env { + t.Setenv(name, value) + } + + cfg, err := loadRelayConfigWithoutFile(t) + require.NoError(t, err) + test.validate(t, cfg) + }) + } +} + +func TestLoadConfigPreservesLegacyValuesWithEmptyEnvironmentAliases(t *testing.T) { + tests := []struct { + name string + env map[string]string + validate func(*testing.T, *Config) + }{ + { + name: "empty log level alias keeps default", + env: map[string]string{"NB_LOGLEVEL": ""}, + validate: func(t *testing.T, cfg *Config) { + assert.Equal(t, "info", cfg.LogLevel, + "Legacy never read NB_LOGLEVEL, so an empty alias could not blank the log level and break InitLog") + }, + }, + { + name: "empty listen address alias keeps legacy name", + env: map[string]string{"NB_LISTENADDRESS": "", "NB_LISTEN_ADDRESS": ":2"}, + validate: func(t *testing.T, cfg *Config) { + assert.Equal(t, ":2", cfg.ListenAddress, + "Legacy never read NB_LISTENADDRESS, so NB_LISTEN_ADDRESS remained effective") + }, + }, + { + name: "empty healthcheck alias keeps legacy name", + env: map[string]string{"NB_HEALTHCHECKLISTENADDRESS": "", "NB_HEALTH_LISTEN_ADDRESS": ":1"}, + validate: func(t *testing.T, cfg *Config) { + assert.Equal(t, ":1", cfg.HealthcheckListenAddress, + "Legacy never read NB_HEALTHCHECKLISTENADDRESS, so NB_HEALTH_LISTEN_ADDRESS remained effective") + }, + }, + } + + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + clearRelayConfigEnvironment(t) + for name, value := range test.env { + t.Setenv(name, value) + } + + cfg, err := loadRelayConfigWithoutFile(t) + require.NoError(t, err) + test.validate(t, cfg) + }) + } +} + +func TestLegacyRelayHasNoConfigFlag(t *testing.T) { + require.NoError(t, rootCmd.ParseFlags(nil)) + + configFlag := rootCmd.Flags().Lookup("config") + assert.Nil(t, configFlag, "Legacy Relay registered no --config flag, so --help did not list it") + + if configFlag != nil { + oldValue := configFlag.Value.String() + oldChanged := configFlag.Changed + oldConfigPath := configPath + t.Cleanup(func() { + require.NoError(t, configFlag.Value.Set(oldValue)) + configFlag.Changed = oldChanged + configPath = oldConfigPath + }) + } + + err := rootCmd.ParseFlags([]string{"--config", filepath.Join(t.TempDir(), "relay.yaml")}) + if assert.Error(t, err, "Legacy Relay rejected --config on the command line and exited with 'failed to execute command'") { + assert.Contains(t, err.Error(), "unknown flag: --config", "Legacy cobra reported --config as an unknown flag") + } +} + +func TestLoadConfigPreservesLegacyNBConfigIgnored(t *testing.T) { + clearRelayConfigEnvironment(t) + path := filepath.Join(t.TempDir(), "relay.yaml") + require.NoError(t, os.WriteFile(path, []byte("authSecret: file-secret\n"), 0o600)) + t.Setenv("NB_CONFIG", path) + + cfg, err := loadRelayConfigWithoutFile(t) + require.NoError(t, err) + assert.Empty(t, cfg.AuthSecret, "Legacy Relay never read NB_CONFIG, so no configuration file was loaded from it") +} + +func TestLoadConfigPreservesLegacyNilDomainsDefault(t *testing.T) { + clearRelayConfigEnvironment(t) + + cfg, err := loadRelayConfigWithoutFile(t) + require.NoError(t, err) + assert.Nil(t, cfg.LetsencryptDomains, + "Legacy left LetsencryptDomains nil when neither NB_LETSENCRYPT_DOMAINS nor --letsencrypt-domains was given, and Route53TLS.Domains received nil") +} + +func TestLoadConfigPreservesLegacyEnvironmentParseDiagnostics(t *testing.T) { + clearRelayConfigEnvironment(t) + t.Setenv("NB_METRICS_PORT", "abc") + + logger := log.StandardLogger() + oldOutput := logger.Out + oldLevel := logger.GetLevel() + var logs bytes.Buffer + logger.SetOutput(&logs) + logger.SetLevel(log.TraceLevel) + t.Cleanup(func() { + logger.SetOutput(oldOutput) + logger.SetLevel(oldLevel) + }) + + _, err := loadRelayConfigWithoutFile(t) + assert.NoError(t, err, "Legacy rejected environment values were logged and ignored; startup continued") + assert.Contains(t, logs.String(), "unable to configure flag metrics-port using variable NB_METRICS_PORT", + "Legacy emitted an Info diagnostic naming the flag and the NB_ variable when an environment value was rejected") +} + +func setRelaySliceFlag(t *testing.T, name string, values []string) { + t.Helper() + + require.NoError(t, rootCmd.ParseFlags(nil)) + flag := rootCmd.Flags().Lookup(name) + require.NotNil(t, flag, "flag %s should be registered", name) + sliceValue, ok := flag.Value.(pflag.SliceValue) + require.True(t, ok, "flag %s should be a slice flag", name) + + oldValues := sliceValue.GetSlice() + oldChanged := flag.Changed + t.Cleanup(func() { + require.NoError(t, sliceValue.Replace(oldValues)) + flag.Changed = oldChanged + }) + require.NoError(t, sliceValue.Replace(values)) + flag.Changed = true +} + +func TestLoadConfigPreservesLegacyQuotedDomainListElements(t *testing.T) { + tests := []struct { + name string + value string + expected []string + }{ + {name: "single quoted element", value: `"relay.example.com"`, expected: []string{"relay.example.com"}}, + {name: "quoted trailing element", value: `a.example.com,"b.example.com"`, expected: []string{"a.example.com", "b.example.com"}}, + {name: "csv escaped double quote", value: `"a""b"`, expected: []string{`a"b`}}, + } + + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + clearRelayConfigEnvironment(t) + t.Setenv("NB_LETSENCRYPT_DATA_DIR", "/var/lib/relay/certs") + t.Setenv("NB_LETSENCRYPT_DOMAINS", test.value) + + cfg, err := loadRelayConfigWithoutFile(t) + require.NoError(t, err) + assert.Equal(t, test.expected, cfg.LetsencryptDomains, + "Legacy pflag readAsCSV used encoding/csv, which stripped surrounding quotes and unescaped doubled quotes in %q, so the Let's Encrypt host whitelist held the clean hostname", test.value) + assert.True(t, cfg.HasLetsEncrypt(), "Legacy still entered the Let's Encrypt path for a quoted domain list") + }) + } +} + +func TestLoadConfigPreservesLegacyTrailingNewlineInDomainList(t *testing.T) { + tests := []struct { + name string + value string + expected []string + }{ + {name: "single domain", value: "relay.example.com\n", expected: []string{"relay.example.com"}}, + {name: "multiple domains", value: "a.example.com,b.example.com\n", expected: []string{"a.example.com", "b.example.com"}}, + } + + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + clearRelayConfigEnvironment(t) + t.Setenv("NB_LETSENCRYPT_DATA_DIR", "/var/lib/relay/certs") + t.Setenv("NB_LETSENCRYPT_DOMAINS", test.value) + + cfg, err := loadRelayConfigWithoutFile(t) + require.NoError(t, err) + assert.Equal(t, test.expected, cfg.LetsencryptDomains, + "Legacy pflag readAsCSV let csv.Reader.Read consume the trailing newline in %q as the record terminator, so the Let's Encrypt host whitelist held the clean hostname without a newline", test.value) + assert.True(t, cfg.HasLetsEncrypt(), "Legacy still entered the Let's Encrypt path for a newline terminated domain list") + }) + } +} diff --git a/signal/cmd/config_test.go b/signal/cmd/config_test.go index 163ce1693..f5fb78556 100644 --- a/signal/cmd/config_test.go +++ b/signal/cmd/config_test.go @@ -1,22 +1,186 @@ package cmd import ( + "bytes" "os" + "os/exec" "path/filepath" + "strings" "testing" + log "github.com/sirupsen/logrus" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" ) func TestExampleConfig(t *testing.T) { + clearSignalConfigEnvironment(t) require.NoError(t, runCmd.ParseFlags(nil)) cfg, err := loadConfig(runCmd, filepath.Join("..", "config.example.yaml")) require.NoError(t, err) assert.Equal(t, "/etc/netbird/tls.crt", cfg.CertFile, "Example config should load") } +func TestLoadConfigPreservesLegacyDefaults(t *testing.T) { + clearSignalConfigEnvironment(t) + require.NoError(t, runCmd.ParseFlags(nil)) + + cfg, err := loadConfig(runCmd, "") + require.NoError(t, err) + assert.Equal(t, defaultConfig(), cfg, "Signal defaults should remain unchanged") +} + +func TestLoadConfigPreservesLegacyEnvironmentBindings(t *testing.T) { + clearSignalConfigEnvironment(t) + t.Setenv("NB_PORT", "10001") + t.Setenv("NB_LETSENCRYPT_DOMAIN", "signal.example.com") + t.Setenv("NB_LETSENCRYPT_EMAIL", "admin@example.com") + t.Setenv("NB_LETSENCRYPT_DATA_DIR", "/var/lib/signal/certs") + t.Setenv("NB_CERT_FILE", "/etc/signal/tls.crt") + t.Setenv("NB_CERT_KEY", "/etc/signal/tls.key") + t.Setenv("NB_PPROF_ADDR", "localhost:6060") + require.NoError(t, runCmd.ParseFlags(nil)) + + cfg, err := loadConfig(runCmd, "") + require.NoError(t, err) + assert.Equal(t, &Config{ + Port: 10001, + MetricsPort: 9090, + LetsencryptDomain: "signal.example.com", + LetsencryptEmail: "admin@example.com", + LetsencryptDataDir: "/var/lib/signal/certs", + CertFile: "/etc/signal/tls.crt", + CertKey: "/etc/signal/tls.key", + LogLevel: "info", + LogFile: defaultConfig().LogFile, + PprofAddress: "localhost:6060", + }, cfg, "Every legacy Signal environment binding should remain supported") +} + +func TestLoadConfigIgnoresPreviouslyUnboundEmptyEnvironment(t *testing.T) { + clearSignalConfigEnvironment(t) + t.Setenv("NB_METRICS_PORT", "") + require.NoError(t, runCmd.ParseFlags(nil)) + + cfg, err := loadConfig(runCmd, "") + require.NoError(t, err) + assert.Equal(t, defaultConfig().MetricsPort, cfg.MetricsPort, + "An environment variable that was previously unbound should not alter the default when empty") +} + +func TestLoadConfigPreservesLegacyEnvironmentAliasPrecedence(t *testing.T) { + clearSignalConfigEnvironment(t) + t.Setenv("NB_LETSENCRYPT_DATA_DIR", "/preferred-certs") + t.Setenv("NB_SSL_DIR", "/legacy-certs") + require.NoError(t, runCmd.ParseFlags(nil)) + + cfg, err := loadConfig(runCmd, "") + require.NoError(t, err) + assert.Equal(t, "/legacy-certs", cfg.LetsencryptDataDir, + "The legacy alias should retain its previous precedence when both variables are set") +} + +func TestSignalPortDefaultsRemainCompatible(t *testing.T) { + tests := []struct { + name string + env map[string]string + expected int + }{ + {name: "no TLS", expected: 80}, + {name: "letsencrypt", env: map[string]string{"NB_LETSENCRYPT_DOMAIN": "signal.example.com"}, expected: 443}, + {name: "certificate pair", env: map[string]string{"NB_CERT_FILE": "/tls.crt", "NB_CERT_KEY": "/tls.key"}, expected: 443}, + {name: "explicit nonzero port", env: map[string]string{"NB_PORT": "10002"}, expected: 10002}, + {name: "explicit zero port", env: map[string]string{"NB_PORT": "0"}, expected: 0}, + {name: "invalid port falls back", env: map[string]string{"NB_PORT": "invalid"}, expected: 80}, + } + + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + actual := runSignalPreRun(t, test.env) + assert.Equal(t, test.expected, actual, "Signal implicit port selection should retain legacy behavior") + }) + } +} + +func TestSignalFlagAliasesRetainArgumentOrder(t *testing.T) { + tests := []struct { + name string + first string + second string + expected string + }{ + { + name: "legacy alias last", + first: "letsencrypt-data-dir", + second: "ssl-dir", + expected: "/ssl-dir", + }, + { + name: "preferred alias last", + first: "ssl-dir", + second: "letsencrypt-data-dir", + expected: "/letsencrypt-data-dir", + }, + } + + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + clearSignalConfigEnvironment(t) + require.NoError(t, runCmd.ParseFlags(nil)) + firstFlag := runCmd.PersistentFlags().Lookup(test.first) + secondFlag := runCmd.PersistentFlags().Lookup(test.second) + require.NotNil(t, firstFlag, "First alias should be registered") + require.NotNil(t, secondFlag, "Second alias should be registered") + oldDataDir := signalLetsencryptDataDir + oldFirstValue, oldFirstChanged := firstFlag.Value.String(), firstFlag.Changed + oldSecondValue, oldSecondChanged := secondFlag.Value.String(), secondFlag.Changed + t.Cleanup(func() { + require.NoError(t, firstFlag.Value.Set(oldFirstValue)) + firstFlag.Changed = oldFirstChanged + require.NoError(t, secondFlag.Value.Set(oldSecondValue)) + secondFlag.Changed = oldSecondChanged + signalLetsencryptDataDir = oldDataDir + }) + + require.NoError(t, firstFlag.Value.Set("/"+test.first)) + firstFlag.Changed = true + require.NoError(t, secondFlag.Value.Set("/"+test.second)) + secondFlag.Changed = true + + cfg, err := loadConfig(runCmd, "") + require.NoError(t, err) + assert.Equal(t, test.expected, cfg.LetsencryptDataDir, + "When both CLI aliases are supplied, the last value should retain precedence") + }) + } +} + +func TestLegacySignalFlagsRemainRegistered(t *testing.T) { + for _, name := range []string{ + "port", + "letsencrypt-data-dir", + "ssl-dir", + "letsencrypt-domain", + "letsencrypt-email", + "cert-file", + "cert-key", + } { + flag := runCmd.PersistentFlags().Lookup(name) + require.NotNil(t, flag, "Legacy persistent flag %s should remain registered", name) + assert.Empty(t, flag.Shorthand, "Legacy Signal flag %s should remain without a shorthand", name) + } + metricsFlag := runCmd.Flags().Lookup("metrics-port") + require.NotNil(t, metricsFlag, "Legacy metrics flag should remain registered") + assert.Empty(t, metricsFlag.Shorthand, "Legacy metrics flag should remain without a shorthand") + for _, name := range []string{"log-level", "log-file"} { + flag := rootCmd.PersistentFlags().Lookup(name) + require.NotNil(t, flag, "Legacy root flag %s should remain registered", name) + assert.Empty(t, flag.Shorthand, "Legacy Signal flag %s should remain without a shorthand", name) + } +} + func TestLoadConfigPrecedence(t *testing.T) { + clearSignalConfigEnvironment(t) configPath := filepath.Join(t.TempDir(), "signal.yaml") require.NoError(t, os.WriteFile(configPath, []byte(` port: 10001 @@ -28,7 +192,7 @@ pprofAddress: localhost:6060 t.Setenv("NB_SSL_DIR", "/legacy-certs") require.NoError(t, runCmd.ParseFlags(nil)) - portFlag := runCmd.Flags().Lookup("port") + portFlag := runCmd.PersistentFlags().Lookup("port") oldPort := portFlag.Value.String() oldChanged := portFlag.Changed t.Cleanup(func() { @@ -46,3 +210,392 @@ pprofAddress: localhost:6060 assert.Equal(t, "/legacy-certs", cfg.LetsencryptDataDir, "Legacy environment aliases should remain supported") assert.Equal(t, "localhost:6060", cfg.PprofAddress, "Environment-only settings should load from the file") } + +func runSignalPreRun(t *testing.T, environment map[string]string) int { + t.Helper() + + clearSignalConfigEnvironment(t) + for name, value := range environment { + t.Setenv(name, value) + } + t.Setenv("NB_LOG_FILE", "console") + require.NoError(t, runCmd.ParseFlags(nil)) + require.NoError(t, executeSignalPreRun(t, "0", false)) + return signalPort +} + +// executeSignalPreRun snapshots the Signal runtime globals, the port flag and the standard logger, +// forces the port flag into the requested state, runs runCmd.PreRunE against the current +// environment and returns its error. Callers prepare the environment and call ParseFlags first. +func executeSignalPreRun(t *testing.T, portValue string, portChanged bool) error { + t.Helper() + + oldSignalPort := signalPort + oldMetricsPort := metricsPort + oldLetsencryptDomain := signalLetsencryptDomain + oldLetsencryptEmail := signalLetsencryptEmail + oldLetsencryptDataDir := signalLetsencryptDataDir + oldCertFile := signalCertFile + oldCertKey := signalCertKey + oldLogLevel := logLevel + oldLogFile := logFile + oldPprofAddress := signalPprofAddress + oldConfigPath := signalConfigPath + portFlag := runCmd.PersistentFlags().Lookup("port") + require.NotNil(t, portFlag, "Signal port flag should be registered") + oldPortValue := portFlag.Value.String() + oldPortChanged := portFlag.Changed + logger := log.StandardLogger() + oldLoggerLevel, oldLoggerOut, oldLoggerFormatter := logger.GetLevel(), logger.Out, logger.Formatter + t.Cleanup(func() { + logger.SetLevel(oldLoggerLevel) + logger.SetOutput(oldLoggerOut) + logger.SetFormatter(oldLoggerFormatter) + require.NoError(t, portFlag.Value.Set(oldPortValue)) + portFlag.Changed = oldPortChanged + signalPort = oldSignalPort + metricsPort = oldMetricsPort + signalLetsencryptDomain = oldLetsencryptDomain + signalLetsencryptEmail = oldLetsencryptEmail + signalLetsencryptDataDir = oldLetsencryptDataDir + signalCertFile = oldCertFile + signalCertKey = oldCertKey + logLevel = oldLogLevel + logFile = oldLogFile + signalPprofAddress = oldPprofAddress + signalConfigPath = oldConfigPath + }) + + signalConfigPath = "" + require.NoError(t, portFlag.Value.Set(portValue)) + portFlag.Changed = portChanged + return runCmd.PreRunE(runCmd, nil) +} + +func clearSignalConfigEnvironment(t *testing.T) { + t.Helper() + + for _, name := range []string{ + "NB_PORT", + "NB_METRICS_PORT", + "NB_LETSENCRYPT_DOMAIN", + "NB_LETSENCRYPT_EMAIL", + "NB_LETSENCRYPT_DATA_DIR", + "NB_SSL_DIR", + "NB_CERT_FILE", + "NB_CERT_KEY", + "NB_LOG_LEVEL", + "NB_LOG_FILE", + "NB_PPROF_ADDR", + // Collapsed camelCase spellings that the shared loader derives automatically; they never + // existed in the legacy NB_ mapping, so the suite must not inherit them either. + "NB_METRICSPORT", + "NB_LETSENCRYPTDOMAIN", + "NB_LETSENCRYPTEMAIL", + "NB_LETSENCRYPTDATADIR", + "NB_CERTFILE", + "NB_CERTKEY", + "NB_LOGLEVEL", + "NB_LOGFILE", + "NB_PPROFADDRESS", + } { + t.Setenv(name, "") + require.NoError(t, os.Unsetenv(name)) + } +} + +func TestLoadConfigIgnoresPreviouslyUnboundMetricsPortEnvironment(t *testing.T) { + tests := []struct { + name string + value string + }{ + {name: "valid value", value: "9191"}, + {name: "invalid value", value: "abc"}, + } + + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + clearSignalConfigEnvironment(t) + t.Setenv("NB_METRICS_PORT", test.value) + require.NoError(t, runCmd.ParseFlags(nil)) + + cfg, err := loadConfig(runCmd, "") + assert.NoError(t, err, + "metrics-port was a non-persistent flag that the legacy NB_ mapping never visited, so NB_METRICS_PORT could not fail startup") + if err != nil { + return + } + assert.Equal(t, defaultConfig().MetricsPort, cfg.MetricsPort, + "metrics-port was a non-persistent flag that the legacy NB_ mapping never visited, so NB_METRICS_PORT must not move the metrics endpoint") + }) + } +} + +func TestLoadConfigPreservesLegacyEnvironmentAliasPrecedenceWithEmptyValues(t *testing.T) { + tests := []struct { + name string + env map[string]string + expected string + }{ + { + name: "empty preferred alias keeps legacy alias", + env: map[string]string{"NB_LETSENCRYPT_DATA_DIR": "", "NB_SSL_DIR": "/legacy-certs"}, + expected: "/legacy-certs", + }, + { + name: "empty legacy alias clears preferred alias", + env: map[string]string{"NB_LETSENCRYPT_DATA_DIR": "/preferred-certs", "NB_SSL_DIR": ""}, + expected: "", + }, + } + + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + clearSignalConfigEnvironment(t) + for name, value := range test.env { + t.Setenv(name, value) + } + require.NoError(t, runCmd.ParseFlags(nil)) + + cfg, err := loadConfig(runCmd, "") + require.NoError(t, err) + assert.Equal(t, test.expected, cfg.LetsencryptDataDir, + "Legacy applied every present NB_ variable in flag order (letsencrypt-data-dir, then ssl-dir) into the same variable, so the last present alias won even when it was empty") + }) + } +} + +func TestLoadConfigIgnoresPreviouslyUnboundLogEnvironment(t *testing.T) { + tests := []struct { + name string + env map[string]string + field string + }{ + {name: "valid log level", env: map[string]string{"NB_LOG_LEVEL": "debug"}}, + {name: "invalid log level", env: map[string]string{"NB_LOG_LEVEL": "verbose"}}, + {name: "empty log level", env: map[string]string{"NB_LOG_LEVEL": ""}}, + {name: "console log file", env: map[string]string{"NB_LOG_FILE": "console"}}, + {name: "syslog log file", env: map[string]string{"NB_LOG_FILE": "syslog"}}, + {name: "writable log file", env: map[string]string{"NB_LOG_FILE": filepath.Join(t.TempDir(), "signal.log")}}, + {name: "unwritable log file", env: map[string]string{"NB_LOG_FILE": filepath.Join(t.TempDir(), "missing", "dir", "signal.log")}}, + {name: "empty log file", env: map[string]string{"NB_LOG_FILE": ""}}, + } + + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + clearSignalConfigEnvironment(t) + for name, value := range test.env { + t.Setenv(name, value) + } + require.NoError(t, runCmd.ParseFlags(nil)) + + cfg, err := loadConfig(runCmd, "") + require.NoError(t, err) + assert.Equal(t, defaultConfig().LogLevel, cfg.LogLevel, + "log-level lived on the root command; the legacy NB_ mapping only visited run persistent flags, so NB_LOG_LEVEL was ignored and the level stayed info") + assert.Equal(t, defaultConfig().LogFile, cfg.LogFile, + "log-file lived on the root command; the legacy NB_ mapping only visited run persistent flags, so NB_LOG_FILE was ignored and logs went to the default file") + }) + } +} + +func TestSignalPreRunIgnoresPreviouslyUnboundLogLevelEnvironment(t *testing.T) { + tests := []struct { + name string + value string + }{ + {name: "valid log level", value: "debug"}, + {name: "invalid log level", value: "verbose"}, + {name: "empty log level", value: ""}, + } + + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + clearSignalConfigEnvironment(t) + t.Setenv("NB_LOG_LEVEL", test.value) + t.Setenv("NB_LOG_FILE", "console") + require.NoError(t, runCmd.ParseFlags(nil)) + + err := executeSignalPreRun(t, "0", false) + assert.NoError(t, err, + "NB_LOG_LEVEL was never applied by the legacy NB_ mapping, so any value left the service starting at level info") + assert.Equal(t, "info", logLevel, + "NB_LOG_LEVEL was never applied by the legacy NB_ mapping, so the effective level must remain info") + if err == nil { + assert.Equal(t, log.InfoLevel, log.StandardLogger().GetLevel(), + "Legacy initialized the logger with the flag-only level info regardless of NB_LOG_LEVEL") + } + }) + } +} + +func TestLoadConfigIgnoresCollapsedEnvironmentNames(t *testing.T) { + clearSignalConfigEnvironment(t) + t.Setenv("NB_METRICSPORT", "1234") + t.Setenv("NB_LETSENCRYPTDOMAIN", "auto.example.com") + t.Setenv("NB_LETSENCRYPTEMAIL", "auto@example.com") + t.Setenv("NB_LETSENCRYPTDATADIR", "/auto-certs") + t.Setenv("NB_CERTFILE", "/auto/tls.crt") + t.Setenv("NB_CERTKEY", "/auto/tls.key") + t.Setenv("NB_LOGLEVEL", "trace") + t.Setenv("NB_LOGFILE", "console") + t.Setenv("NB_PPROFADDRESS", "localhost:1") + require.NoError(t, runCmd.ParseFlags(nil)) + + cfg, err := loadConfig(runCmd, "") + require.NoError(t, err) + assert.Equal(t, defaultConfig(), cfg, + "Legacy only mapped NB_ with underscores between words; collapsed camelCase spellings such as NB_METRICSPORT did not exist and were ignored") +} + +func TestLoadConfigPreservesDocumentedEnvironmentNamesOverCollapsedNames(t *testing.T) { + tests := []struct { + name string + env map[string]string + actual func(*Config) any + expected any + }{ + { + name: "pprof address", + env: map[string]string{"NB_PPROFADDRESS": "auto:2", "NB_PPROF_ADDR": "explicit:1"}, + actual: func(cfg *Config) any { return cfg.PprofAddress }, + expected: "explicit:1", + }, + { + name: "metrics port", + env: map[string]string{"NB_METRICSPORT": "7777", "NB_METRICS_PORT": "8888"}, + actual: func(cfg *Config) any { return cfg.MetricsPort }, + expected: defaultConfig().MetricsPort, + }, + { + name: "letsencrypt data dir", + env: map[string]string{"NB_LETSENCRYPTDATADIR": "/auto-certs", "NB_LETSENCRYPT_DATA_DIR": "/preferred-certs", "NB_SSL_DIR": "/legacy-certs"}, + actual: func(cfg *Config) any { return cfg.LetsencryptDataDir }, + expected: "/legacy-certs", + }, + } + + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + clearSignalConfigEnvironment(t) + for name, value := range test.env { + t.Setenv(name, value) + } + require.NoError(t, runCmd.ParseFlags(nil)) + + cfg, err := loadConfig(runCmd, "") + require.NoError(t, err) + assert.Equal(t, test.expected, test.actual(cfg), + "Legacy read only the documented NB_ names (NB_PPROF_ADDR directly, NB_ for persistent flags); an undocumented collapsed spelling must not override them") + }) + } +} + +func TestSignalExplicitZeroPortFlagRemainsCompatible(t *testing.T) { + tests := []struct { + name string + env map[string]string + }{ + {name: "no TLS"}, + {name: "letsencrypt", env: map[string]string{"NB_LETSENCRYPT_DOMAIN": "signal.example.com"}}, + } + + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + clearSignalConfigEnvironment(t) + for name, value := range test.env { + t.Setenv(name, value) + } + t.Setenv("NB_LOG_FILE", "console") + require.NoError(t, runCmd.ParseFlags(nil)) + + require.NoError(t, executeSignalPreRun(t, "0", true)) + assert.Equal(t, 0, signalPort, + "Legacy skipped the 80/443 default heuristic whenever the port flag was Changed, so `--port 0` kept 0 and bound an ephemeral port") + }) + } +} + +func TestLegacySignalRunCommandHadNoConfigFlag(t *testing.T) { + clearSignalConfigEnvironment(t) + require.NoError(t, runCmd.ParseFlags(nil)) + + configFlag := runCmd.PersistentFlags().Lookup("config") + assert.Nil(t, configFlag, "Legacy Signal had no --config flag; cobra rejected it as an unknown flag") + + if configFlag != nil { + oldValue, oldChanged := configFlag.Value.String(), configFlag.Changed + oldConfigPath := signalConfigPath + t.Cleanup(func() { + require.NoError(t, configFlag.Value.Set(oldValue)) + configFlag.Changed = oldChanged + signalConfigPath = oldConfigPath + }) + } + + err := runCmd.ParseFlags([]string{"--config", filepath.Join(t.TempDir(), "signal.yaml")}) + assert.Error(t, err, "Legacy Signal rejected `run --config ` with `unknown flag: --config` and exited with a usage error") +} + +func TestSignalPortFlagHelpRetainsLegacyDefault(t *testing.T) { + portFlag := runCmd.PersistentFlags().Lookup("port") + require.NotNil(t, portFlag, "Signal port flag should be registered") + + assert.Equal(t, "80", portFlag.DefValue, + "Legacy registered --port with default 80, so `run --help` rendered a `(default 80)` suffix") + assert.Regexp(t, `--port int\s+Server port to listen on .*\(default 80\)`, runCmd.PersistentFlags().FlagUsages(), + "Legacy `run --help` printed `(default 80)` after the --port description") +} + +func TestLegacySignalInformationalCommandsAppliedEnvironmentAtStartup(t *testing.T) { + tests := []struct { + name string + args []string + }{ + {name: "version", args: []string{"--version"}}, + {name: "run help", args: []string{"run", "--help"}}, + } + + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + clearSignalConfigEnvironment(t) + + cmd := exec.Command(os.Args[0], "-test.run=^TestSignalHelperProcess$") + cmd.Env = append(os.Environ(), + "NB_SIGNAL_TEST_HELPER_PROCESS=1", + "NB_SIGNAL_TEST_HELPER_ARGS="+strings.Join(test.args, " "), + "NB_PORT=invalid", + ) + var stdout, stderr bytes.Buffer + cmd.Stdout = &stdout + cmd.Stderr = &stderr + require.NoError(t, cmd.Run(), "helper process should exit cleanly, stdout: %s stderr: %s", stdout.String(), stderr.String()) + + assert.Contains(t, stderr.String(), "unable to configure flag port using variable NB_PORT", + "Legacy applied NB_ variables to flags in init() for every invocation, so an invalid NB_PORT logged a warning even for informational commands") + }) + } +} + +// TestSignalHelperProcess executes the Signal root command inside a child test process. It is only +// active when spawned by TestLegacySignalInformationalCommandsAppliedEnvironmentAtStartup. +func TestSignalHelperProcess(t *testing.T) { + if os.Getenv("NB_SIGNAL_TEST_HELPER_PROCESS") != "1" { + t.Skip("helper process for subprocess-based tests") + } + + rootCmd.SetArgs(strings.Fields(os.Getenv("NB_SIGNAL_TEST_HELPER_ARGS"))) + require.NoError(t, rootCmd.Execute()) +} + +func TestLoadConfigFileCannotEnablePprofAsLegacy(t *testing.T) { + clearSignalConfigEnvironment(t) + configPath := filepath.Join(t.TempDir(), "signal.yaml") + require.NoError(t, os.WriteFile(configPath, []byte("pprofAddress: localhost:6060\n"), 0o600)) + require.NoError(t, runCmd.ParseFlags(nil)) + + cfg, err := loadConfig(runCmd, configPath) + require.NoError(t, err) + assert.Equal(t, "", cfg.PprofAddress, + "Legacy enabled pprof only from os.Getenv(\"NB_PPROF_ADDR\") at run time; no file source could turn it on") +}