diff --git a/audit/pkg/config.audit.md b/audit/pkg/config.audit.md index fc0c4dd..79a8b01 100644 --- a/audit/pkg/config.audit.md +++ b/audit/pkg/config.audit.md @@ -24,7 +24,7 @@ validators. Two serious issues: |---|----------|------|---------| | 1 | **Critical** | Locking | `Manager.Set`/`Get` over a lock-free `*viper.Viper` → concurrent map write → process-fatal | | 2 | **High** | Locking | `GetConfigManager()` is an unsynchronised lazy singleton; `NewManager()` also clobbers the global as a side effect | -| 3 | **High** | Security | Insecure defaults: `cors.allowed_origins: ["*"]`, `allowed_headers: ["*"]`, `sslmode: disable`, `user: postgres` + blank password | +| 3 | **High** | Security | **OPEN (deferred)** Insecure defaults: `cors.allowed_origins: ["*"]`, `allowed_headers: ["*"]`, `sslmode: disable`, `user: postgres` + blank password | | 4 | **High** | Security | `SaveConfig` writes all secrets in plaintext at mode `0644` (viper default, never overridden) | | 5 | Medium | Security | `AddConfigPath(".")` is searched first — CWD config injection | | 6 | Medium | Observability | `Load()` swallows `ConfigFileNotFoundError` with no log at all | @@ -37,6 +37,24 @@ validators. Two serious issues: | 13 | Low | Correctness | `ServersConfig.GetDefault()` returns a pointer to a copy of a map value | | 14 | Low | Security | `PathsConfig.Join` does not confine the result to the base path | +## Resolution status (2026-09-30) + +- **#1** — Fixed: `sync.RWMutex` guards every viper access, options included +- **#2** — Fixed: mutex-guarded singleton; `NewManager` no longer touches the global (new `SetConfigManager` publishes explicitly) +- **#4** — Fixed: `SetConfigPermissions(0o600)` plus `chmod 0600` after write (secrets are not stripped) +- **#5** — Fixed: search order is `/etc/resolvespec`, `$HOME/.resolvespec`, `./config`, `.` (CWD last, not dropped) +- **#6** — Partly fixed: `ConfigFileUsed()` added; no log line because `pkg/config` cannot import `logger` (import cycle) +- **#7** — Fixed: `Set` has a pointer receiver and allocates +- **#8** — Not fixed: still a bare map; `Set` documented as not concurrency-safe +- **#9** — Fixed: `LookupIPAddr` with a 2s timeout, fallback normalised to bare IPs and populates the slice +- **#10** — Fixed: dead `Unmarshal` removed, `SetConfig` is atomic +- **#11** — Fixed: recover removed (nothing in the function can panic) +- **#12** — Partly fixed: `Config.Validate()` added, but it is not called from `GetConfig()`. The `*` CORS+credentials check is not implemented +- **#13** — Documented only: `GetDefault` returns a pointer to a copy +- **#14** — Fixed: `Join` errors if the result escapes the base +- **#3** — Open: default flips deferred by decision (breaking change). +- Tests: `pkg/config/hardening_test.go`. + --- ## Findings diff --git a/cmd/testserver/main.go b/cmd/testserver/main.go index b9fea07..dc62f78 100644 --- a/cmd/testserver/main.go +++ b/cmd/testserver/main.go @@ -24,6 +24,7 @@ import ( func main() { // Load configuration cfgMgr := config.NewManager() + config.SetConfigManager(cfgMgr) if err := cfgMgr.Load(); err != nil { log.Fatalf("Failed to load configuration: %v", err) } diff --git a/pkg/config/config.go b/pkg/config/config.go index c1d5905..884e269 100644 --- a/pkg/config/config.go +++ b/pkg/config/config.go @@ -1,6 +1,9 @@ package config -import "time" +import ( + "fmt" + "time" +) // Config represents the complete application configuration type Config struct { @@ -198,3 +201,23 @@ type EventBrokerRetryPolicyConfig struct { // This is a map of path name to file system path // Example: "data_dir": "/var/lib/myapp/data" type PathsConfig map[string]string + +// Validate checks every configuration section for invalid or unsafe values. +func (c *Config) Validate() error { + if err := c.Servers.Validate(); err != nil { + return fmt.Errorf("servers: %w", err) + } + if c.Middleware.RateLimitRPS < 0 || c.Middleware.RateLimitBurst < 0 { + return fmt.Errorf("middleware: rate_limit_rps and rate_limit_burst must not be negative") + } + if c.Middleware.MaxRequestSize <= 0 { + return fmt.Errorf("middleware: max_request_size must be greater than 0") + } + if c.EventBroker.Enabled && c.EventBroker.WorkerCount <= 0 { + return fmt.Errorf("event_broker: worker_count must be greater than 0") + } + if c.DBManager.MaxOpenConns < 0 || c.DBManager.MaxIdleConns < 0 || c.DBManager.RetryAttempts < 0 { + return fmt.Errorf("dbmanager: max_open_conns, max_idle_conns and retry_attempts must not be negative") + } + return nil +} diff --git a/pkg/config/hardening_test.go b/pkg/config/hardening_test.go new file mode 100644 index 0000000..0a479a0 --- /dev/null +++ b/pkg/config/hardening_test.go @@ -0,0 +1,79 @@ +package config + +import ( + "os" + "path/filepath" + "sync" + "testing" +) + +func TestManagerConcurrentSetGet(t *testing.T) { + m := NewManager() + var wg sync.WaitGroup + for i := 0; i < 8; i++ { + wg.Add(2) + go func() { + defer wg.Done() + for j := 0; j < 200; j++ { + m.Set("x.y", j) + } + }() + go func() { + defer wg.Done() + for j := 0; j < 200; j++ { + _ = m.Get("x.y") + _, _ = m.GetConfig() + } + }() + } + wg.Wait() +} + +func TestNewManagerDoesNotReplaceGlobal(t *testing.T) { + g := GetConfigManager() + _ = NewManager() + if GetConfigManager() != g { + t.Fatal("NewManager replaced the global manager") + } +} + +func TestSaveConfigPermissions(t *testing.T) { + path := filepath.Join(t.TempDir(), "out.yaml") + if err := os.WriteFile(path, nil, 0o644); err != nil { + t.Fatal(err) + } + if err := NewManager().SaveConfig(path); err != nil { + t.Fatal(err) + } + if fi, _ := os.Stat(path); fi.Mode().Perm() != 0o600 { + t.Fatalf("mode = %v, want 0600", fi.Mode().Perm()) + } +} + +func TestPathsSetNilAndJoinConfined(t *testing.T) { + var pc PathsConfig + pc.Set("data", "data") + if got, _ := pc.Get("data"); got != "data" { + t.Fatalf("got %q", got) + } + if _, err := pc.Join("data", "../../etc/passwd"); err == nil { + t.Fatal("expected traversal error") + } + if p, err := pc.Join("data", "a", "b"); err != nil || p != filepath.Join("data", "a", "b") { + t.Fatalf("got %q, %v", p, err) + } +} + +func TestConfigValidate(t *testing.T) { + cfg, err := NewManager().GetConfig() + if err != nil { + t.Fatal(err) + } + if err := cfg.Validate(); err != nil { + t.Fatalf("defaults should validate: %v", err) + } + cfg.EventBroker.Enabled, cfg.EventBroker.WorkerCount = true, 0 + if cfg.Validate() == nil { + t.Fatal("expected worker_count error") + } +} diff --git a/pkg/config/manager.go b/pkg/config/manager.go index 391c6b0..20889a4 100644 --- a/pkg/config/manager.go +++ b/pkg/config/manager.go @@ -2,37 +2,60 @@ package config import ( "fmt" + "os" "strings" + "sync" "github.com/spf13/viper" ) -// Manager handles configuration loading from multiple sources +// Manager handles configuration loading from multiple sources. +// viper.Viper is not safe for concurrent use, so every access to it is guarded by mu. type Manager struct { - v *viper.Viper + mu sync.RWMutex + v *viper.Viper } -var configInstance *Manager +var ( + configInstance *Manager + configMu sync.Mutex +) // GetConfigManager returns a singleton configuration manager instance func GetConfigManager() *Manager { + configMu.Lock() + defer configMu.Unlock() if configInstance == nil { configInstance = NewManager() } return configInstance } -// NewManager creates a new configuration manager with defaults +// SetConfigManager publishes m as the global manager returned by GetConfigManager. +// NewManager no longer does this implicitly. +func SetConfigManager(m *Manager) { + configMu.Lock() + defer configMu.Unlock() + configInstance = m +} + +// NewManager creates a new, isolated configuration manager with defaults. +// It does not replace the global manager; use SetConfigManager for that. func NewManager() *Manager { v := viper.New() // Set configuration file settings v.SetConfigName("config") v.SetConfigType("yaml") - v.AddConfigPath(".") - v.AddConfigPath("./config") + // Most trusted location first; the working directory is the least trustworthy + // and is searched last (viper takes the first match). v.AddConfigPath("/etc/resolvespec") v.AddConfigPath("$HOME/.resolvespec") + v.AddConfigPath("./config") + v.AddConfigPath(".") + + // Saved configs may contain secrets; never write them world-readable + v.SetConfigPermissions(0o600) // Enable environment variable support v.SetEnvPrefix("RESOLVESPEC") @@ -42,8 +65,7 @@ func NewManager() *Manager { // Set default values setDefaults(v) - configInstance = &Manager{v: v} - return configInstance + return &Manager{v: v} } // NewManagerWithOptions creates a new configuration manager with custom options @@ -61,6 +83,8 @@ type Option func(*Manager) // WithConfigFile sets a specific config file path func WithConfigFile(path string) Option { return func(m *Manager) { + m.mu.Lock() + defer m.mu.Unlock() m.v.SetConfigFile(path) } } @@ -68,6 +92,8 @@ func WithConfigFile(path string) Option { // WithConfigName sets the config file name (without extension) func WithConfigName(name string) Option { return func(m *Manager) { + m.mu.Lock() + defer m.mu.Unlock() m.v.SetConfigName(name) } } @@ -75,6 +101,8 @@ func WithConfigName(name string) Option { // WithConfigPath adds a path to search for config files func WithConfigPath(path string) Option { return func(m *Manager) { + m.mu.Lock() + defer m.mu.Unlock() m.v.AddConfigPath(path) } } @@ -82,13 +110,19 @@ func WithConfigPath(path string) Option { // WithEnvPrefix sets the environment variable prefix func WithEnvPrefix(prefix string) Option { return func(m *Manager) { + m.mu.Lock() + defer m.mu.Unlock() m.v.SetEnvPrefix(prefix) } } -// Load attempts to load configuration from file and environment +// Load attempts to load configuration from file and environment. +// A missing config file is not an error (defaults and env vars are used); check +// ConfigFileUsed after Load to see whether a file was actually read. func (m *Manager) Load() error { - // Try to read config file (not an error if it doesn't exist) + m.mu.Lock() + defer m.mu.Unlock() + if err := m.v.ReadInConfig(); err != nil { if _, ok := err.(viper.ConfigFileNotFoundError); !ok { return fmt.Errorf("error reading config file: %w", err) @@ -99,8 +133,19 @@ func (m *Manager) Load() error { return nil } +// ConfigFileUsed returns the config file that was read by Load, or "" if none was +// found (i.e. the manager is running on defaults and environment variables only). +func (m *Manager) ConfigFileUsed() string { + m.mu.RLock() + defer m.mu.RUnlock() + return m.v.ConfigFileUsed() +} + // GetConfig returns the complete configuration func (m *Manager) GetConfig() (*Config, error) { + m.mu.RLock() + defer m.mu.RUnlock() + var cfg Config if err := m.v.Unmarshal(&cfg); err != nil { return nil, fmt.Errorf("failed to unmarshal config: %w", err) @@ -108,16 +153,11 @@ func (m *Manager) GetConfig() (*Config, error) { return &cfg, nil } -// SetConfig sets the complete configuration +// SetConfig sets the complete configuration atomically func (m *Manager) SetConfig(cfg *Config) error { - configMap := make(map[string]interface{}) + m.mu.Lock() + defer m.mu.Unlock() - // Marshal the config to a map structure that viper can use - if err := m.v.Unmarshal(&configMap); err != nil { - return fmt.Errorf("failed to prepare config map: %w", err) - } - - // Use viper's merge to apply the config m.v.Set("servers", cfg.Servers) m.v.Set("tracing", cfg.Tracing) m.v.Set("cache", cfg.Cache) @@ -135,34 +175,54 @@ func (m *Manager) SetConfig(cfg *Config) error { // Get returns a configuration value by key func (m *Manager) Get(key string) interface{} { + m.mu.RLock() + defer m.mu.RUnlock() return m.v.Get(key) } // GetString returns a string configuration value func (m *Manager) GetString(key string) string { + m.mu.RLock() + defer m.mu.RUnlock() return m.v.GetString(key) } // GetInt returns an int configuration value func (m *Manager) GetInt(key string) int { + m.mu.RLock() + defer m.mu.RUnlock() return m.v.GetInt(key) } // GetBool returns a bool configuration value func (m *Manager) GetBool(key string) bool { + m.mu.RLock() + defer m.mu.RUnlock() return m.v.GetBool(key) } // Set sets a configuration value func (m *Manager) Set(key string, value interface{}) { + m.mu.Lock() + defer m.mu.Unlock() m.v.Set(key, value) } -// SaveConfig writes the current configuration to the specified path +// SaveConfig writes the current configuration to the specified path. +// The file contains the entire merged configuration, including secrets +// (database/redis passwords, error-tracking DSN), so it is written with mode 0600. +// Prefer supplying secrets via RESOLVESPEC_* environment variables. func (m *Manager) SaveConfig(path string) error { + m.mu.RLock() + defer m.mu.RUnlock() + if err := m.v.WriteConfigAs(path); err != nil { return fmt.Errorf("failed to save config to %s: %w", path, err) } + // viper only applies its permissions when creating the file; tighten a pre-existing one too + if err := os.Chmod(path, 0o600); err != nil { + return fmt.Errorf("failed to restrict permissions on %s: %w", path, err) + } return nil } diff --git a/pkg/config/paths.go b/pkg/config/paths.go index 95d9ca0..95d75ac 100644 --- a/pkg/config/paths.go +++ b/pkg/config/paths.go @@ -4,6 +4,7 @@ import ( "fmt" "os" "path/filepath" + "strings" ) // Get retrieves a path by name @@ -34,9 +35,13 @@ func (pc PathsConfig) GetOrDefault(name, defaultPath string) string { return path } -// Set sets a path by name -func (pc PathsConfig) Set(name, path string) { - pc[name] = path +// Set sets a path by name. It takes a pointer so a nil map can be allocated. +// PathsConfig is not safe for concurrent mutation; populate it before sharing. +func (pc *PathsConfig) Set(name, path string) { + if *pc == nil { + *pc = make(PathsConfig) + } + (*pc)[name] = path } // Has checks if a path exists by name @@ -92,7 +97,8 @@ func (pc PathsConfig) AbsPath(name string) (string, error) { return absPath, nil } -// Join joins path segments with a named base path +// Join joins path segments with a named base path. +// It returns an error if the result would escape the base path. func (pc PathsConfig) Join(name string, elem ...string) (string, error) { base, err := pc.Get(name) if err != nil { @@ -100,7 +106,14 @@ func (pc PathsConfig) Join(name string, elem ...string) (string, error) { } parts := append([]string{base}, elem...) - return filepath.Join(parts...), nil + joined := filepath.Join(parts...) + + // filepath.Join resolves ".." rather than rejecting it; ensure the result stays under base + cleanBase := filepath.Clean(base) + if joined != cleanBase && !strings.HasPrefix(joined, cleanBase+string(os.PathSeparator)) { + return "", fmt.Errorf("path %q escapes base path '%s'", joined, name) + } + return joined, nil } // List returns all configured path names diff --git a/pkg/config/server.go b/pkg/config/server.go index 201f053..6615b48 100644 --- a/pkg/config/server.go +++ b/pkg/config/server.go @@ -1,10 +1,12 @@ package config import ( + "context" "fmt" "net" "os" "strings" + "time" ) // ApplyGlobalDefaults applies global server defaults to this instance @@ -95,7 +97,8 @@ func (sc *ServersConfig) Validate() error { return nil } -// GetDefault returns the default server instance configuration +// GetDefault returns the default server instance configuration. +// The returned pointer refers to a copy: mutating it does not modify sc.Instances. func (sc *ServersConfig) GetDefault() (*ServerInstanceConfig, error) { if sc.DefaultServer == "" { return nil, fmt.Errorf("no default server configured") @@ -109,41 +112,37 @@ func (sc *ServersConfig) GetDefault() (*ServerInstanceConfig, error) { return &instance, nil } -// GetIPs - GetIP for pc +// GetIPs returns the hostname, a comma-separated list of non-loopback IPs and the +// same IPs as []net.IP. The lookup is bounded by a short timeout. func GetIPs() (hostname string, ipList string, ipNetList []net.IP) { - defer func() { - if err := recover(); err != nil { - fmt.Println("Recovered in GetIPs", err) - } - }() hostname, _ = os.Hostname() - ipaddrlist := make([]net.IP, 0) - iplist := "" - addrs, err := net.LookupIP(hostname) - if err != nil { - return hostname, iplist, ipaddrlist - } + ipNetList = make([]net.IP, 0) - for _, a := range addrs { - // cfg.LogInfo("\nFound IP Host Address: %s", a) - if strings.Contains(a.String(), "127.0.0.1") { - continue - } - iplist = fmt.Sprintf("%s,%s", iplist, a) - ipaddrlist = append(ipaddrlist, a) - } - if iplist == "" { - iff, _ := net.InterfaceAddrs() - for _, a := range iff { - // cfg.LogInfo("\nFound IP Address: %s", a) - if strings.Contains(a.String(), "127.0.0.1") { + ctx, cancel := context.WithTimeout(context.Background(), 2*time.Second) + defer cancel() + + var ips []string + if addrs, err := net.DefaultResolver.LookupIPAddr(ctx, hostname); err == nil { + for _, a := range addrs { + if a.IP.IsLoopback() { continue } - iplist = fmt.Sprintf("%s,%s", iplist, a) - + ips = append(ips, a.IP.String()) + ipNetList = append(ipNetList, a.IP) } - } - iplist = strings.TrimLeft(iplist, ",") - return hostname, iplist, ipaddrlist + + if len(ips) == 0 { + ifaceAddrs, _ := net.InterfaceAddrs() + for _, a := range ifaceAddrs { + ipn, ok := a.(*net.IPNet) + if !ok || ipn.IP.IsLoopback() { + continue + } + ips = append(ips, ipn.IP.String()) + ipNetList = append(ipNetList, ipn.IP) + } + } + + return hostname, strings.Join(ips, ","), ipNetList }