feat: comprehensive Go development review improvements (#372)

## Summary

Comprehensive review of the ldap-manager codebase using the
go-development skill and related skills (security-audit,
enterprise-readiness, github-project).

### Changes

- **feat(retry)**: Add retry package with exponential backoff
- **refactor(options)**: Return errors instead of calling Fatal()
- **feat(ldap_cache)**: Add retry logic and context propagation
- **feat(web)**: Add rate limiting for authentication endpoints
- **feat(main)**: Add graceful shutdown with signal handling
- **chore(lint)**: Add durationcheck, fatcontext, and forcetypeassert
linters
- **chore(test)**: Add gremlins mutation testing configuration
- **test(web)**: Add health handler tests for improved coverage
- **chore(make)**: Document CGO requirement and add gremlins target
- **chore(make)**: Add standalone vet and vuln-check targets
- **style**: Fix gofumpt formatting in tests and server
- **docs**: Add SECURITY.md for vulnerability reporting

### Quality Checks

- golangci-lint: 0 issues
- go vet: passed
- staticcheck: passed
- govulncheck: no vulnerabilities
- All tests: passed

Author: CybotTM

.golangci.yml

@@ -115,6 +115,9 @@ linters:
     - asciicheck # Check for non-ASCII identifiers
     - bodyclose # Check HTTP response body closes
     - dupl # Check code duplication
+    - durationcheck # Check for duration multiplication issues
+    - fatcontext # Detect nested context in loops
+    - forcetypeassert # Find forced type assertions
     - errname # Check error naming conventions
     - errorlint # Find error handling issues
     - exhaustive # Check switch statements

.gremlins.yaml

@@ -0,0 +1,43 @@
+# Gremlins Mutation Testing Configuration
+# https://github.com/go-gremlins/gremlins
+
+# Packages to test
+test-packages:
+  - ./internal/ldap_cache/...
+  - ./internal/options/...
+  - ./internal/retry/...
+  - ./internal/web/...
+
+# Mutator types to enable
+mutators:
+  - CONDITIONALS_BOUNDARY # Change < to <=, > to >=
+  - CONDITIONALS_NEGATION # Negate conditions (== to !=)
+  - INCREMENT_DECREMENT # Change ++ to --
+  - INVERT_LOGICAL # Invert && to ||
+  - INVERT_NEGATIVES # Remove negation operators
+  - INVERT_LOOPCTRL # Change break to continue
+
+# Files/patterns to exclude
+exclude:
+  - "**/*_test.go" # Test files
+  - "**/test/**" # Test helpers
+  - "**/mock/**" # Mock implementations
+  - "**/generated/**" # Generated code
+  - "**/*_templ.go" # Generated templ files
+
+# Only mutate code covered by tests
+coverage: true
+
+# Minimum acceptable mutation score (%)
+threshold: 60
+
+# Timeout multiplier for test runs
+timeout-coefficient: 5
+
+# Output reports
+output:
+  json: mutation-reports/gremlins-report.json
+  html: mutation-reports/gremlins-report.html
+
+# Test timeout
+test-timeout: 120s

Makefile

@@ -29,7 +29,7 @@ COVERAGE_DIR := coverage-reports
 COVERAGE_FILE := coverage.out
 HTML_COVERAGE_FILE := $(COVERAGE_DIR)/coverage.html
 
-.PHONY: help setup build test lint clean dev docker docker-dev docker-test docker-lint docker-check docker-shell docker-clean
+.PHONY: help setup build test lint clean dev docker docker-dev docker-test docker-lint docker-check docker-shell docker-clean vet vuln-check
 
 # Default target
 all: setup lint test build
@@ -104,11 +104,11 @@ build-release: build-assets
 	@CGO_ENABLED=0 GOOS=windows GOARCH=amd64 go build $(BUILDFLAGS) -o bin/ldap-manager-windows-amd64.exe ./cmd/ldap-manager
 	@echo "$(GREEN)✓ Release binaries built$(RESET)"
 
-## Test: Run comprehensive test suite with coverage
+## Test: Run comprehensive test suite with coverage (requires CGO_ENABLED=1 for race detector)
 test:
 	@echo "$(BLUE)Running comprehensive test suite...$(RESET)"
 	@mkdir -p $(COVERAGE_DIR)
-	@if go test -v -race -coverprofile=$(COVERAGE_FILE) -covermode=atomic ./...; then \
+	@if CGO_ENABLED=1 go test -v -race -coverprofile=$(COVERAGE_FILE) -covermode=atomic ./...; then \
 		echo "$(GREEN)✅ All tests passed$(RESET)"; \
 	else \
 		echo "$(RED)❌ Some tests failed$(RESET)"; \
@@ -142,10 +142,11 @@ test-short:
 	@echo "$(BLUE)Running short tests...$(RESET)"
 	@go test -short ./...
 
-## Test Race: Run race detection tests
+## Test Race: Run race detection tests (requires CGO_ENABLED=1)
 test-race:
 	@echo "$(BLUE)Running race detection tests...$(RESET)"
-	@if go test -race -short ./...; then \
+	@echo "$(YELLOW)Note: Race detector requires CGO_ENABLED=1$(RESET)"
+	@if CGO_ENABLED=1 go test -race -short ./...; then \
 		echo "$(GREEN)✅ No race conditions detected$(RESET)"; \
 	else \
 		echo "$(RED)❌ Race conditions detected$(RESET)"; \
@@ -192,11 +193,18 @@ test-fuzz:
 	@go test -fuzz=FuzzQueryParams -fuzztime=15s ./internal/web || true
 	@echo "$(GREEN)✅ Fuzz testing completed$(RESET)"
 
-## Test Mutation: Run mutation testing
+## Test Mutation: Run mutation testing with go-mutesting
 test-mutation:
-	@echo "$(BLUE)Running mutation testing...$(RESET)"
+	@echo "$(BLUE)Running mutation testing (go-mutesting)...$(RESET)"
 	@./scripts/mutation-test.sh
 
+## Test Mutation Gremlins: Run mutation testing with gremlins (recommended)
+test-mutation-gremlins:
+	@echo "$(BLUE)Running mutation testing (gremlins)...$(RESET)"
+	@command -v gremlins >/dev/null 2>&1 || go install github.com/go-gremlins/gremlins/cmd/gremlins@v0.6.0
+	@mkdir -p mutation-reports
+	@gremlins unleash --config=.gremlins.yaml
+
 ## Test All: Run all test types
 test-all: test test-integration
 	@echo "$(GREEN)✅ All tests completed$(RESET)"
@@ -210,10 +218,21 @@ lint-go:
 	@echo "$(BLUE)Running golangci-lint...$(RESET)"
 	@golangci-lint run --config .golangci.yml ./...
 
-## Lint Security: Run security vulnerability checks  
-lint-security:
-	@echo "$(BLUE)Running security checks...$(RESET)"
+## Vet: Run go vet static analysis
+vet:
+	@echo "$(BLUE)Running go vet...$(RESET)"
+	@go vet ./...
+	@echo "$(GREEN)✓ go vet passed$(RESET)"
+
+## Vuln Check: Run vulnerability scanning
+vuln-check:
+	@echo "$(BLUE)Running govulncheck...$(RESET)"
 	@govulncheck ./...
+	@echo "$(GREEN)✓ No vulnerabilities found$(RESET)"
+
+## Lint Security: Run security vulnerability checks
+lint-security: vuln-check
+	@echo "$(GREEN)✓ Security checks passed$(RESET)"
 
 ## Lint Format: Check code formatting
 lint-format:

SECURITY.md

@@ -0,0 +1,42 @@
+# Security Policy
+
+## Supported Versions
+
+| Version | Supported          |
+| ------- | ------------------ |
+| latest  | :white_check_mark: |
+
+## Reporting a Vulnerability
+
+If you discover a security vulnerability in ldap-manager, please report it responsibly:
+
+1. **Do NOT** open a public GitHub issue for security vulnerabilities
+2. **Use** [GitHub's private vulnerability reporting](https://github.com/netresearch/ldap-manager/security/advisories/new)
+3. **Include** a detailed description of the vulnerability and steps to reproduce
+
+### What to Expect
+
+- **Acknowledgment**: We will acknowledge receipt within 48 hours
+- **Assessment**: We will assess the vulnerability and determine its severity
+- **Fix Timeline**: Critical vulnerabilities will be addressed within 7 days
+- **Disclosure**: We will coordinate disclosure timing with the reporter
+
+## Security Measures
+
+This project implements the following security controls:
+
+- **CSRF Protection**: All state-changing operations require valid CSRF tokens
+- **Rate Limiting**: Authentication endpoints are rate-limited to prevent brute force
+- **Session Security**: Sessions use secure, HTTP-only cookies with configurable expiry
+- **Input Validation**: All user input is validated server-side
+- **Dependency Scanning**: Automated vulnerability scanning via Trivy and gosec
+- **Code Analysis**: Static analysis with golangci-lint and CodeQL
+
+## Security Configuration
+
+See the [README](README.md) for security-related configuration options:
+
+- `--cookie-secure`: Require HTTPS for cookies (recommended for production)
+- `--tls-skip-verify`: Disable TLS verification (development only)
+
+Rate limiting is enabled by default (5 failed attempts per 15 minutes triggers a 15-minute block).

cmd/ldap-manager/main.go

@@ -3,7 +3,11 @@
 package main
 
 import (
+	"context"
 	"os"
+	"os/signal"
+	"syscall"
+	"time"
 
 	"github.com/rs/zerolog"
 	"github.com/rs/zerolog/log"
@@ -13,20 +17,60 @@ import (
 	"github.com/netresearch/ldap-manager/internal/web"
 )
 
+const shutdownTimeout = 30 * time.Second
+
 func main() {
 	log.Logger = log.Output(zerolog.ConsoleWriter{Out: os.Stderr})
 
 	log.Info().Msgf("LDAP Manager %s starting...", version.FormatVersion())
 
-	opts := options.Parse()
+	opts, err := options.Parse()
+	if err != nil {
+		log.Fatal().Err(err).Msg("failed to parse configuration")
+	}
 	log.Logger = log.Logger.Level(opts.LogLevel)
 
 	app, err := web.NewApp(opts)
 	if err != nil {
 		log.Fatal().Err(err).Msg("could not initialize web app")
 	}
 
-	if err := app.Listen(":3000"); err != nil {
-		log.Fatal().Err(err).Msg("could not start web server")
+	// Create context for graceful shutdown
+	ctx, cancel := context.WithCancel(context.Background())
+	defer cancel()
+
+	// Handle shutdown signals
+	sigChan := make(chan os.Signal, 1)
+	signal.Notify(sigChan, syscall.SIGTERM, syscall.SIGINT, syscall.SIGHUP)
+
+	// Start server in goroutine
+	serverErr := make(chan error, 1)
+	go func() {
+		if err := app.Listen(ctx, ":3000"); err != nil {
+			serverErr <- err
+		}
+	}()
+
+	// Wait for shutdown signal or server error
+	select {
+	case sig := <-sigChan:
+		log.Info().Str("signal", sig.String()).Msg("Received shutdown signal")
+	case err := <-serverErr:
+		log.Error().Err(err).Msg("Server error")
 	}
+
+	// Initiate graceful shutdown
+	log.Info().Msg("Initiating graceful shutdown...")
+	cancel() // Signal all goroutines to stop
+
+	shutdownCtx, shutdownCancel := context.WithTimeout(context.Background(), shutdownTimeout)
+	defer shutdownCancel()
+
+	if err := app.Shutdown(shutdownCtx); err != nil {
+		log.Error().Err(err).Msg("Error during shutdown")
+		shutdownCancel() // Required: os.Exit does not run deferred functions
+		os.Exit(1)       //nolint:gocritic // Exit is intentional after shutdown error
+	}
+
+	log.Info().Msg("Graceful shutdown complete")
 }

internal/ldap_cache/manager.go

@@ -6,10 +6,14 @@
 package ldap_cache
 
 import (
+	"context"
+	"sync"
 	"time"
 
 	ldap "github.com/netresearch/simple-ldap-go"
 	"github.com/rs/zerolog/log"
+
+	"github.com/netresearch/ldap-manager/internal/retry"
 )
 
 // LDAPClient interface defines the LDAP operations needed by the cache manager.
@@ -27,13 +31,17 @@ type LDAPClient interface {
 // All operations are concurrent-safe and provide immediate access to cached data.
 // Includes comprehensive metrics tracking for performance monitoring and observability.
 // Supports cache warming for faster startup and configurable refresh strategies.
+// LDAP operations are automatically retried with exponential backoff for resilience.
 type Manager struct {
-	stop chan struct{} // Channel for graceful shutdown signaling
+	stop     chan struct{} // Channel for graceful shutdown signaling
+	stopOnce sync.Once     // Ensures Stop() is idempotent
+	ctx      context.Context
 
 	client          LDAPClient    // LDAP client for directory operations
 	metrics         *Metrics      // Performance metrics and health monitoring
 	refreshInterval time.Duration // Configurable refresh interval (default 30s)
 	warmupComplete  bool          // Tracks if initial cache warming is complete
+	retryConfig     retry.Config  // Retry configuration for LDAP operations
 
 	Users     Cache[ldap.User]     // Cached user entries with O(1) indexed lookups
 	Groups    Cache[ldap.Group]    // Cached group entries with O(1) indexed lookups
@@ -73,16 +81,20 @@ func New(client LDAPClient) *Manager {
 // NewWithConfig creates a new LDAP cache manager with configurable refresh interval.
 // The manager is initialized with empty caches and custom refresh timing.
 // Includes comprehensive metrics tracking for performance monitoring.
+// LDAP operations are automatically retried with exponential backoff for resilience.
 // Call Run() to start the background refresh goroutine.
 func NewWithConfig(client LDAPClient, refreshInterval time.Duration) *Manager {
 	metrics := NewMetrics()
 
 	return &Manager{
-		stop:            make(chan struct{}),
+		stop: make(chan struct{}),
+		// ctx is initialized with Background and replaced in Run() before any operations
+		ctx:             context.Background(),
 		client:          client,
 		metrics:         metrics,
 		refreshInterval: refreshInterval,
 		warmupComplete:  false,
+		retryConfig:     retry.LDAPConfig(),
 		Users:           NewCachedWithMetrics[ldap.User](metrics),
 		Groups:          NewCachedWithMetrics[ldap.Group](metrics),
 		Computers:       NewCachedWithMetrics[ldap.Computer](metrics),
@@ -91,8 +103,14 @@ func NewWithConfig(client LDAPClient, refreshInterval time.Duration) *Manager {
 
 // Run starts the background cache refresh loop.
 // It performs initial cache warming, then continues refreshing at the configured interval.
-// This method blocks until Stop() is called. Should be run in a separate goroutine.
-func (m *Manager) Run() {
+// The context is used for graceful shutdown - when canceled, the loop terminates.
+// This method blocks until the context is canceled or Stop() is called.
+// Should be run in a separate goroutine.
+func (m *Manager) Run(ctx context.Context) {
+	// Store context for use in refresh operations.
+	// Safe: Run() is called once from Listen() before any cache operations.
+	m.ctx = ctx
+
 	// Use configurable refresh interval
 	t := time.NewTicker(m.refreshInterval)
 	defer t.Stop()
@@ -105,6 +123,10 @@ func (m *Manager) Run() {
 
 	for {
 		select {
+		case <-ctx.Done():
+			log.Info().Msg("LDAP cache stopping (context canceled)")
+
+			return
 		case <-m.stop:
 			log.Info().Msg("LDAP cache stopped")
 
@@ -116,9 +138,12 @@ func (m *Manager) Run() {
 }
 
 // Stop gracefully shuts down the background refresh loop.
-// It sends a signal to the Run() method to terminate the refresh cycle.
+// It closes the stop channel to signal Run() to terminate the refresh cycle.
+// Safe to call multiple times; subsequent calls are no-ops due to sync.Once.
 func (m *Manager) Stop() {
-	m.stop <- struct{}{}
+	m.stopOnce.Do(func() {
+		close(m.stop)
+	})
 }
 
 // WarmupCache performs initial cache population with enhanced logging and error handling.
@@ -203,9 +228,12 @@ func (m *Manager) IsWarmedUp() bool {
 }
 
 // RefreshUsers fetches all users from LDAP and updates the user cache.
-// Returns an error if the LDAP query fails, otherwise replaces the entire user cache.
+// Uses retry logic with exponential backoff for resilience against transient failures.
+// Returns an error if all retry attempts fail, otherwise replaces the entire user cache.
 func (m *Manager) RefreshUsers() error {
-	users, err := m.client.FindUsers()
+	users, err := retry.DoWithResultConfig(m.ctx, m.retryConfig, func() ([]ldap.User, error) {
+		return m.client.FindUsers()
+	})
 	if err != nil {
 		return err
 	}
@@ -216,9 +244,12 @@ func (m *Manager) RefreshUsers() error {
 }
 
 // RefreshGroups fetches all groups from LDAP and updates the group cache.
-// Returns an error if the LDAP query fails, otherwise replaces the entire group cache.
+// Uses retry logic with exponential backoff for resilience against transient failures.
+// Returns an error if all retry attempts fail, otherwise replaces the entire group cache.
 func (m *Manager) RefreshGroups() error {
-	groups, err := m.client.FindGroups()
+	groups, err := retry.DoWithResultConfig(m.ctx, m.retryConfig, func() ([]ldap.Group, error) {
+		return m.client.FindGroups()
+	})
 	if err != nil {
 		return err
 	}
@@ -229,9 +260,12 @@ func (m *Manager) RefreshGroups() error {
 }
 
 // RefreshComputers fetches all computers from LDAP and updates the computer cache.
-// Returns an error if the LDAP query fails, otherwise replaces the entire computer cache.
+// Uses retry logic with exponential backoff for resilience against transient failures.
+// Returns an error if all retry attempts fail, otherwise replaces the entire computer cache.
 func (m *Manager) RefreshComputers() error {
-	computers, err := m.client.FindComputers()
+	computers, err := retry.DoWithResultConfig(m.ctx, m.retryConfig, func() ([]ldap.Computer, error) {
+		return m.client.FindComputers()
+	})
 	if err != nil {
 		return err
 	}