diff --git a/AUDIT.md b/AUDIT.md index cf33c1f..2776b58 100644 --- a/AUDIT.md +++ b/AUDIT.md @@ -137,10 +137,13 @@ severity levels. All critical and high issues resolved. ### A-015: Incomplete Config Validation - **Severity**: Medium -- **Status**: Open +- **Status**: ~~Resolved~~ - **Description**: TLS files not checked for existence at startup. Token TTL, WebAuthn config not validated. -- **Plan**: Add file existence checks and config field validation. +- **Resolution**: Split validation into `validateFields()` and + `validateFiles()`. Added checks for: TLS file existence, token TTL + parseability and positivity, Argon2 params > 0, valid log level, + non-empty listen addresses. Full test coverage in config_test.go. ### A-016: Inconsistent Error Types - **Severity**: Low @@ -150,9 +153,9 @@ severity levels. All critical and high issues resolved. ### A-017: No Race Condition Testing - **Severity**: Low -- **Status**: Open +- **Status**: ~~Resolved~~ - **Description**: Test suite does not use `-race` flag. -- **Plan**: Add `make test-race` target. +- **Resolution**: Added `make test-race` target that runs `go test -race ./...`. --- @@ -174,9 +177,9 @@ severity levels. All critical and high issues resolved. | A-012 | Medium | Accepted | | A-013 | Medium | Accepted | | A-014 | Medium | Open | -| A-015 | Medium | Open | +| A-015 | Medium | ~~Resolved~~ | | A-016 | Low | Open | -| A-017 | Low | Open | +| A-017 | Low | ~~Resolved~~ | ## Design Note: Single-User Model diff --git a/Makefile b/Makefile index 688537b..a013b95 100644 --- a/Makefile +++ b/Makefile @@ -1,4 +1,4 @@ -.PHONY: build test vet lint proto proto-lint clean all +.PHONY: build test test-race vet lint proto proto-lint clean all LDFLAGS := -trimpath -ldflags="-s -w -X main.version=$(shell git describe --tags --always --dirty 2>/dev/null || echo dev)" @@ -11,6 +11,9 @@ build: test: go test ./... +test-race: + go test -race ./... + vet: go vet ./... diff --git a/internal/config/config.go b/internal/config/config.go index cc5c778..a973002 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -70,11 +70,65 @@ func Load(path string) (*Config, error) { } func (c *Config) validate() error { + if err := c.validateFields(); err != nil { + return err + } + if err := c.validateFiles(); err != nil { + return err + } + return nil +} + +// validateFields checks config values that don't require filesystem access. +func (c *Config) validateFields() error { if c.Database.Path == "" { return fmt.Errorf("database.path is required") } if c.Server.TLSCert == "" || c.Server.TLSKey == "" { return fmt.Errorf("server.tls_cert and server.tls_key are required") } + if c.Server.ListenAddr == "" { + return fmt.Errorf("server.listen_addr is required") + } + if c.Server.GRPCAddr == "" { + return fmt.Errorf("server.grpc_addr is required") + } + + d, err := c.Auth.TokenDuration() + if err != nil { + return fmt.Errorf("auth.token_ttl is invalid: %w", err) + } + if d <= 0 { + return fmt.Errorf("auth.token_ttl must be positive") + } + + if c.Auth.Argon2Memory == 0 { + return fmt.Errorf("auth.argon2_memory must be greater than zero") + } + if c.Auth.Argon2Time == 0 { + return fmt.Errorf("auth.argon2_time must be greater than zero") + } + if c.Auth.Argon2Threads == 0 { + return fmt.Errorf("auth.argon2_threads must be greater than zero") + } + + switch c.Log.Level { + case "debug", "info", "warn", "error": + // valid + default: + return fmt.Errorf("log.level must be one of: debug, info, warn, error (got %q)", c.Log.Level) + } + + return nil +} + +// validateFiles checks that referenced files exist on disk. +func (c *Config) validateFiles() error { + if _, err := os.Stat(c.Server.TLSCert); err != nil { + return fmt.Errorf("server.tls_cert file not found: %w", err) + } + if _, err := os.Stat(c.Server.TLSKey); err != nil { + return fmt.Errorf("server.tls_key file not found: %w", err) + } return nil } diff --git a/internal/config/config_test.go b/internal/config/config_test.go new file mode 100644 index 0000000..9e05730 --- /dev/null +++ b/internal/config/config_test.go @@ -0,0 +1,240 @@ +package config + +import ( + "os" + "path/filepath" + "testing" +) + +// validConfig returns a Config with all fields set to valid values. +// TLS cert/key paths must be overridden by the caller for file validation tests. +func validConfig() Config { + return Config{ + Server: ServerConfig{ + ListenAddr: ":8443", + GRPCAddr: ":9443", + TLSCert: "/tmp/cert.pem", + TLSKey: "/tmp/key.pem", + }, + Database: DatabaseConfig{ + Path: "/srv/eng-pad/eng-pad.db", + }, + Auth: AuthConfig{ + TokenTTL: "24h", + Argon2Memory: 65536, + Argon2Time: 3, + Argon2Threads: 4, + }, + Log: LogConfig{ + Level: "info", + }, + } +} + +func TestValidateFields_ValidConfig(t *testing.T) { + cfg := validConfig() + if err := cfg.validateFields(); err != nil { + t.Fatalf("valid config should pass field validation: %v", err) + } +} + +func TestValidateFields_MissingDatabasePath(t *testing.T) { + cfg := validConfig() + cfg.Database.Path = "" + if err := cfg.validateFields(); err == nil { + t.Fatal("expected error for missing database.path") + } +} + +func TestValidateFields_MissingTLSCert(t *testing.T) { + cfg := validConfig() + cfg.Server.TLSCert = "" + if err := cfg.validateFields(); err == nil { + t.Fatal("expected error for missing tls_cert") + } +} + +func TestValidateFields_MissingTLSKey(t *testing.T) { + cfg := validConfig() + cfg.Server.TLSKey = "" + if err := cfg.validateFields(); err == nil { + t.Fatal("expected error for missing tls_key") + } +} + +func TestValidateFields_MissingListenAddr(t *testing.T) { + cfg := validConfig() + cfg.Server.ListenAddr = "" + if err := cfg.validateFields(); err == nil { + t.Fatal("expected error for missing listen_addr") + } +} + +func TestValidateFields_MissingGRPCAddr(t *testing.T) { + cfg := validConfig() + cfg.Server.GRPCAddr = "" + if err := cfg.validateFields(); err == nil { + t.Fatal("expected error for missing grpc_addr") + } +} + +func TestValidateFields_InvalidTokenTTL(t *testing.T) { + cfg := validConfig() + cfg.Auth.TokenTTL = "not-a-duration" + if err := cfg.validateFields(); err == nil { + t.Fatal("expected error for invalid token_ttl") + } +} + +func TestValidateFields_NegativeTokenTTL(t *testing.T) { + cfg := validConfig() + cfg.Auth.TokenTTL = "-1h" + if err := cfg.validateFields(); err == nil { + t.Fatal("expected error for negative token_ttl") + } +} + +func TestValidateFields_ZeroTokenTTL(t *testing.T) { + cfg := validConfig() + cfg.Auth.TokenTTL = "0s" + if err := cfg.validateFields(); err == nil { + t.Fatal("expected error for zero token_ttl") + } +} + +func TestValidateFields_ZeroArgon2Memory(t *testing.T) { + cfg := validConfig() + cfg.Auth.Argon2Memory = 0 + if err := cfg.validateFields(); err == nil { + t.Fatal("expected error for zero argon2_memory") + } +} + +func TestValidateFields_ZeroArgon2Time(t *testing.T) { + cfg := validConfig() + cfg.Auth.Argon2Time = 0 + if err := cfg.validateFields(); err == nil { + t.Fatal("expected error for zero argon2_time") + } +} + +func TestValidateFields_ZeroArgon2Threads(t *testing.T) { + cfg := validConfig() + cfg.Auth.Argon2Threads = 0 + if err := cfg.validateFields(); err == nil { + t.Fatal("expected error for zero argon2_threads") + } +} + +func TestValidateFields_InvalidLogLevel(t *testing.T) { + cfg := validConfig() + cfg.Log.Level = "trace" + if err := cfg.validateFields(); err == nil { + t.Fatal("expected error for invalid log level") + } +} + +func TestValidateFields_AllLogLevels(t *testing.T) { + for _, level := range []string{"debug", "info", "warn", "error"} { + t.Run(level, func(t *testing.T) { + cfg := validConfig() + cfg.Log.Level = level + if err := cfg.validateFields(); err != nil { + t.Fatalf("log level %q should be valid: %v", level, err) + } + }) + } +} + +func TestValidateFiles_CertAndKeyExist(t *testing.T) { + dir := t.TempDir() + certPath := filepath.Join(dir, "cert.pem") + keyPath := filepath.Join(dir, "key.pem") + if err := os.WriteFile(certPath, []byte("cert"), 0600); err != nil { + t.Fatalf("write cert: %v", err) + } + if err := os.WriteFile(keyPath, []byte("key"), 0600); err != nil { + t.Fatalf("write key: %v", err) + } + + cfg := validConfig() + cfg.Server.TLSCert = certPath + cfg.Server.TLSKey = keyPath + if err := cfg.validateFiles(); err != nil { + t.Fatalf("expected no error when cert/key files exist: %v", err) + } +} + +func TestValidateFiles_MissingCertFile(t *testing.T) { + dir := t.TempDir() + keyPath := filepath.Join(dir, "key.pem") + if err := os.WriteFile(keyPath, []byte("key"), 0600); err != nil { + t.Fatalf("write key: %v", err) + } + + cfg := validConfig() + cfg.Server.TLSCert = filepath.Join(dir, "nonexistent-cert.pem") + cfg.Server.TLSKey = keyPath + if err := cfg.validateFiles(); err == nil { + t.Fatal("expected error for missing cert file") + } +} + +func TestValidateFiles_MissingKeyFile(t *testing.T) { + dir := t.TempDir() + certPath := filepath.Join(dir, "cert.pem") + if err := os.WriteFile(certPath, []byte("cert"), 0600); err != nil { + t.Fatalf("write cert: %v", err) + } + + cfg := validConfig() + cfg.Server.TLSCert = certPath + cfg.Server.TLSKey = filepath.Join(dir, "nonexistent-key.pem") + if err := cfg.validateFiles(); err == nil { + t.Fatal("expected error for missing key file") + } +} + +func TestLoad_ValidTOML(t *testing.T) { + dir := t.TempDir() + certPath := filepath.Join(dir, "cert.pem") + keyPath := filepath.Join(dir, "key.pem") + if err := os.WriteFile(certPath, []byte("cert"), 0600); err != nil { + t.Fatalf("write cert: %v", err) + } + if err := os.WriteFile(keyPath, []byte("key"), 0600); err != nil { + t.Fatalf("write key: %v", err) + } + + toml := ` +[server] +listen_addr = ":8443" +grpc_addr = ":9443" +tls_cert = "` + certPath + `" +tls_key = "` + keyPath + `" + +[database] +path = "/srv/eng-pad/eng-pad.db" + +[auth] +token_ttl = "24h" +argon2_memory = 65536 +argon2_time = 3 +argon2_threads = 4 + +[log] +level = "info" +` + configPath := filepath.Join(dir, "config.toml") + if err := os.WriteFile(configPath, []byte(toml), 0600); err != nil { + t.Fatalf("write config: %v", err) + } + + cfg, err := Load(configPath) + if err != nil { + t.Fatalf("load valid config: %v", err) + } + if cfg.Server.ListenAddr != ":8443" { + t.Fatalf("got listen_addr %q, want %q", cfg.Server.ListenAddr, ":8443") + } +}