From 8fbfd4aeb07d432fb2335f147f2a23367e1c7568 Mon Sep 17 00:00:00 2001 From: rcourtman Date: Tue, 30 Sep 2025 15:35:39 +0000 Subject: [PATCH] fix: improve alert system robustness and security MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Addresses multiple issues identified during comprehensive alert system audit: 1. Fix ZFS device loop lock issue - Moved lock acquisition outside loop in checkZFSPoolHealth - Changed clearAlert to clearAlertNoLock when lock already held - Prevents multiple lock acquisitions in same iteration 2. Add alert deduplication on restore - Prevents duplicate alerts after service restart - Tracks seen alert IDs during LoadActiveAlerts - Logs warnings for any duplicates found 3. Add API input validation - validateAlertID function prevents DOS attacks - Limit alert ID length to 500 characters - Whitelist allowed characters (alphanumeric, -, _, :, /, .) - Cap history limit parameter at 10,000 records - Applied validation to acknowledge, unacknowledge, and clear endpoints 4. Add panic recovery to goroutines - All SaveActiveAlerts goroutines now have defer/recover - Cleanup goroutines protected from panics - Contextual error logging for each goroutine type 5. Document lock ordering - Added comprehensive documentation for Manager mutexes - Explains m.mu and resolvedMutex relationship - Clarifies acquisition rules to prevent deadlocks - Inline comments for resolvedMutex field These fixes improve stability, security, data integrity, and maintainability of the alert system without breaking API compatibility. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude --- internal/alerts/alerts.go | 68 +++++++++++++++++++++++++++++++++++---- internal/api/alerts.go | 45 ++++++++++++++++++++------ 2 files changed, 96 insertions(+), 17 deletions(-) diff --git a/internal/alerts/alerts.go b/internal/alerts/alerts.go index a6fdfe610..c7100107e 100644 --- a/internal/alerts/alerts.go +++ b/internal/alerts/alerts.go @@ -175,6 +175,19 @@ type AlertConfig struct { } // Manager handles alert monitoring and state +// +// Lock Ordering Documentation: +// The Manager uses two mutexes to prevent deadlocks: +// 1. m.mu (primary lock) - protects most manager state +// 2. m.resolvedMutex - protects only recentlyResolved map +// +// Lock Ordering Rules: +// - NEVER hold m.mu when acquiring resolvedMutex +// - ALWAYS release m.mu before acquiring resolvedMutex +// - resolvedMutex can be held independently without m.mu +// - When both locks are needed, acquire m.mu first, then release it before acquiring resolvedMutex +// +// This ordering prevents deadlock scenarios where different goroutines acquire locks in different orders. type Manager struct { mu sync.RWMutex config AlertConfig @@ -190,7 +203,7 @@ type Manager struct { suppressedUntil map[string]time.Time // Track suppression windows // Recently resolved alerts (kept for 5 minutes) recentlyResolved map[string]*ResolvedAlert - resolvedMutex sync.RWMutex + resolvedMutex sync.RWMutex // Secondary lock - see Lock Ordering Documentation above // Time threshold tracking pendingAlerts map[string]time.Time // Track when thresholds were first exceeded // Offline confirmation tracking @@ -834,13 +847,14 @@ func (m *Manager) checkZFSPoolHealth(storage models.Storage) { } // Check individual devices for errors + // Lock once for the entire loop instead of inside it + m.mu.Lock() + defer m.mu.Unlock() + for _, device := range pool.Devices { if device.State != "ONLINE" || device.ReadErrors > 0 || device.WriteErrors > 0 || device.ChecksumErrors > 0 { alertID := fmt.Sprintf("zfs-device-%s-%s", storage.ID, device.Name) - m.mu.Lock() - defer m.mu.Unlock() - if _, exists := m.activeAlerts[alertID]; !exists { level := AlertLevelWarning if device.State == "FAULTED" || device.State == "UNAVAIL" { @@ -898,7 +912,7 @@ func (m *Manager) checkZFSPoolHealth(storage models.Storage) { } else { // Clear device alert if it's back to normal alertID := fmt.Sprintf("zfs-device-%s-%s", storage.ID, device.Name) - m.clearAlert(alertID) + m.clearAlertNoLock(alertID) } } } @@ -1089,6 +1103,11 @@ func (m *Manager) checkMetric(resourceID, resourceName, node, instance, resource // Save active alerts after adding new one go func() { + defer func() { + if r := recover(); r != nil { + log.Error().Interface("panic", r).Msg("Panic in SaveActiveAlerts goroutine") + } + }() if err := m.SaveActiveAlerts(); err != nil { log.Error().Err(err).Msg("Failed to save active alerts after creation") } @@ -1169,6 +1188,11 @@ func (m *Manager) checkMetric(resourceID, resourceName, node, instance, resource // Save active alerts after resolution go func() { + defer func() { + if r := recover(); r != nil { + log.Error().Interface("panic", r).Msg("Panic in SaveActiveAlerts goroutine (resolution)") + } + }() if err := m.SaveActiveAlerts(); err != nil { log.Error().Err(err).Msg("Failed to save active alerts after resolution") } @@ -1186,6 +1210,11 @@ func (m *Manager) checkMetric(resourceID, resourceName, node, instance, resource // Schedule cleanup after 5 minutes go func() { + defer func() { + if r := recover(); r != nil { + log.Error().Interface("panic", r).Str("alertID", alertID).Msg("Panic in cleanup goroutine") + } + }() time.Sleep(5 * time.Minute) m.resolvedMutex.Lock() delete(m.recentlyResolved, alertID) @@ -2210,10 +2239,21 @@ func (m *Manager) LoadActiveAlerts() error { return fmt.Errorf("failed to unmarshal active alerts: %w", err) } - // Restore alerts to the map + // Restore alerts to the map with deduplication now := time.Now() restoredCount := 0 + duplicateCount := 0 + seen := make(map[string]bool) + for _, alert := range alerts { + // Skip duplicates + if seen[alert.ID] { + duplicateCount++ + log.Warn().Str("alertID", alert.ID).Msg("Skipping duplicate alert during restore") + continue + } + seen[alert.ID] = true + // Skip very old alerts (older than 24 hours) if now.Sub(alert.StartTime) > 24*time.Hour { log.Debug().Str("alertID", alert.ID).Msg("Skipping old alert during restore") @@ -2230,7 +2270,11 @@ func (m *Manager) LoadActiveAlerts() error { restoredCount++ } - log.Info().Int("restored", restoredCount).Int("total", len(alerts)).Msg("Restored active alerts from disk") + log.Info(). + Int("restored", restoredCount). + Int("total", len(alerts)). + Int("duplicates", duplicateCount). + Msg("Restored active alerts from disk") return nil } @@ -2262,6 +2306,11 @@ func (m *Manager) CleanupAlertsForNodes(existingNodes map[string]bool) { log.Info().Int("removed", removedCount).Int("remaining", len(m.activeAlerts)).Msg("Cleaned up alerts for non-existent nodes") // Save the cleaned up state go func() { + defer func() { + if r := recover(); r != nil { + log.Error().Interface("panic", r).Msg("Panic in SaveActiveAlerts goroutine (cleanup)") + } + }() if err := m.SaveActiveAlerts(); err != nil { log.Error().Err(err).Msg("Failed to save alerts after cleanup") } @@ -2294,6 +2343,11 @@ func (m *Manager) ClearActiveAlerts() { log.Info().Msg("Cleared all active and pending alerts") go func() { + defer func() { + if r := recover(); r != nil { + log.Error().Interface("panic", r).Msg("Panic in SaveActiveAlerts goroutine (clear)") + } + }() if err := m.SaveActiveAlerts(); err != nil { log.Error().Err(err).Msg("Failed to persist cleared alerts") } diff --git a/internal/api/alerts.go b/internal/api/alerts.go index b245cc3e8..9a19d026d 100644 --- a/internal/api/alerts.go +++ b/internal/api/alerts.go @@ -28,6 +28,23 @@ func NewAlertHandlers(monitor *monitoring.Monitor, wsHub *websocket.Hub) *AlertH } } +// validateAlertID validates an alert ID for security +func validateAlertID(alertID string) bool { + // Check length to prevent DOS attacks with huge IDs + if len(alertID) == 0 || len(alertID) > 500 { + return false + } + // Alert IDs should only contain alphanumeric, hyphens, underscores, colons, and slashes + // (e.g., "pve1:qemu/101-cpu", "node-offline-pve1") + for _, c := range alertID { + if !((c >= 'a' && c <= 'z') || (c >= 'A' && c <= 'Z') || + (c >= '0' && c <= '9') || c == '-' || c == '_' || c == ':' || c == '/' || c == '.') { + return false + } + } + return true +} + // GetAlertConfig returns the current alert configuration func (h *AlertHandlers) GetAlertConfig(w http.ResponseWriter, r *http.Request) { config := h.monitor.GetAlertManager().GetConfig() @@ -85,8 +102,13 @@ func (h *AlertHandlers) GetActiveAlerts(w http.ResponseWriter, r *http.Request) func (h *AlertHandlers) GetAlertHistory(w http.ResponseWriter, r *http.Request) { limit := 100 if limitStr := r.URL.Query().Get("limit"); limitStr != "" { - if l, err := strconv.Atoi(limitStr); err == nil && l > 0 { + if l, err := strconv.Atoi(limitStr); err == nil && l > 0 && l <= 10000 { limit = l + } else if err != nil { + log.Warn().Str("limit", limitStr).Msg("Invalid limit parameter, using default") + } else { + log.Warn().Int("limit", l).Msg("Limit exceeds maximum, capping at 10000") + limit = 10000 } } @@ -139,11 +161,12 @@ func (h *AlertHandlers) UnacknowledgeAlert(w http.ResponseWriter, r *http.Reques // Extract alert ID by removing the suffix alertID := strings.TrimSuffix(path, suffix) - if alertID == "" { + if !validateAlertID(alertID) { log.Error(). Str("path", r.URL.Path). - Msg("Empty alert ID") - http.Error(w, "Invalid URL", http.StatusBadRequest) + Str("alertID", alertID). + Msg("Invalid alert ID") + http.Error(w, "Invalid alert ID", http.StatusBadRequest) return } @@ -200,11 +223,12 @@ func (h *AlertHandlers) AcknowledgeAlert(w http.ResponseWriter, r *http.Request) // Extract alert ID by removing the suffix alertID := strings.TrimSuffix(path, suffix) - if alertID == "" { + if !validateAlertID(alertID) { log.Error(). Str("path", r.URL.Path). - Msg("Empty alert ID") - http.Error(w, "Invalid URL", http.StatusBadRequest) + Str("alertID", alertID). + Msg("Invalid alert ID") + http.Error(w, "Invalid alert ID", http.StatusBadRequest) return } @@ -269,11 +293,12 @@ func (h *AlertHandlers) ClearAlert(w http.ResponseWriter, r *http.Request) { // Extract alert ID by removing the suffix alertID := strings.TrimSuffix(path, suffix) - if alertID == "" { + if !validateAlertID(alertID) { log.Error(). Str("path", r.URL.Path). - Msg("Empty alert ID") - http.Error(w, "Invalid URL", http.StatusBadRequest) + Str("alertID", alertID). + Msg("Invalid alert ID") + http.Error(w, "Invalid alert ID", http.StatusBadRequest) return }