From f8ea46d07506f2798d334b32c2f19b150d723e24 Mon Sep 17 00:00:00 2001 From: Graham McIntire Date: Tue, 24 Mar 2026 09:47:05 -0500 Subject: [PATCH] cleanup --- FIXES.md | 184 ------------------------------------------------------- 1 file changed, 184 deletions(-) delete mode 100644 FIXES.md diff --git a/FIXES.md b/FIXES.md deleted file mode 100644 index 4418f2f..0000000 --- a/FIXES.md +++ /dev/null @@ -1,184 +0,0 @@ -# TowerOps Agent - Comprehensive Audit Fixes - -**Branch**: `fix/audit-issues-comprehensive` -**Date**: 2026-03-24 -**Audit Coverage**: Error Handling, Concurrency, Resource Leaks, Performance - ---- - -## 🔴 CRITICAL ISSUES - -### ✅ 1. Silent Result Loss - Data Loss Under Load -**Status**: FIXED -**Files**: `snmp.go`, `mikrotik.go`, `ssh.go` -**Issue**: All result channels used non-blocking sends with silent failure when channels filled -**Impact**: Production data loss - monitoring results silently disappeared when channels reached capacity -**Fix**: -- Added `sendSnmpResultWithTimeout()` helper with 5s timeout + error logging -- Added `sendMikrotikResultWithTimeout()` helper with 5s timeout + error logging -- Added `sendMonitoringCheckResultWithTimeout()` helper with 5s timeout + error logging (pending) -- Replaced all `select { case ch <- result: default: warn }` patterns with timeout-based sends - -**Locations Fixed**: -- ✅ `snmp.go:114-118` (executeSnmpJob result) -- ✅ `snmp.go:133-142` (executeCredentialTest connection error) -- ✅ `snmp.go:149-159` (executeCredentialTest SNMP error) -- ✅ `snmp.go:167-176` (executeCredentialTest success) -- ✅ `mikrotik.go:284-293` (executeMikrotikJob connection error) -- ✅ `mikrotik.go:321-331` (executeMikrotikJob result) -- ✅ `mikrotik.go:340-350` (executeMikrotikBackupViaSSH error) -- ✅ `mikrotik.go:353-364` (executeMikrotikBackupViaSSH success) -- ✅ `ssh.go:68-76` (executePingJob error) - FIXED -- ✅ `ssh.go:81-90` (executePingJob success) - FIXED - ---- - -### ✅ 2. Unchecked SetDeadline Errors - Connection Hangs -**Status**: FIXED -**Files**: `websocket.go`, `checks.go`, `mikrotik.go` -**Issue**: All deadline errors ignored with `_` - if SetDeadline fails, connections hang indefinitely -**Impact**: Connections hang forever without timeout protection -**Fix**: Check all SetDeadline errors, close connection and return error on failure - -**Locations Fixed**: -- ✅ `websocket.go:102` (handshake deadline) - FIXED -- ✅ `websocket.go:163` (clear handshake deadline) - FIXED -- ✅ `websocket.go:207` (read deadline) - FIXED -- ✅ `websocket.go:266` (write deadline) - FIXED -- ✅ `checks.go:171` (TCP check deadline) - FIXED -- ✅ `mikrotik.go:166` (read deadline) - FIXED - ---- - -### ✅ 3. Missing Result Reporting on Early Returns -**Status**: FIXED -**Files**: `snmp.go`, `ssh.go`, `mikrotik.go` -**Issue**: Jobs failed validation but never sent error result to server - server UI shows "in progress" forever -**Impact**: No way to detect job failures in server UI -**Fix**: Send error results before all early returns - -**Locations Fixed**: -- ✅ `snmp.go:39-40` (missing device) - FIXED -- ✅ `snmp.go:125-126` (missing device in credential test) - FIXED -- ✅ `ssh.go:59-60` (missing device) - FIXED -- ✅ `mikrotik.go:268-269` (missing device) - FIXED - ---- - -### ✅ 4. LLDP Silent Error Swallowing -**Status**: FIXED -**File**: `lldp.go:109, 119, 129` -**Issue**: SNMP Walk errors completely ignored with `_` assignment -**Impact**: LLDP topology discovery silently fails - incomplete neighbor data returned -**Fix**: Check errors, log warnings, append to error message in result - ---- - -## 🟠 HIGH SEVERITY ISSUES - -### ✅ 5. Reader Goroutine Leak on Every Disconnect -**Status**: FIXED -**File**: `agent.go:183-204` -**Issue**: Reader goroutine spawned without WaitGroup tracking (writer had tracking) -**Impact**: Goroutine leak on every session disconnect - accumulates over agent lifetime -**Fix**: -- Added `var readerWg sync.WaitGroup` and `readerWg.Add(1)` -- Added dedicated goroutine to close WebSocket on context cancellation (enables fast shutdown) -- Added `readerWg.Wait()` in cleanup defer -- Reader exits immediately when connection closes (no 90s timeout wait) - ---- - -### ✅ 6. Timer Leak in Worker Pool Shutdown -**Status**: FIXED -**File**: `workerpool.go:72` -**Issue**: `time.After(timeout)` creates timer that leaks if done closes first -**Impact**: Timer leak on every graceful shutdown -**Fix**: Use `time.NewTimer()` with explicit `defer timer.Stop()` - ---- - -### ✅ 8. HTTP Client Per-Check - Connection Exhaustion -**Status**: FIXED -**File**: `checks.go:18-36, 95-120` -**Issue**: Each HTTP check creates new client with own connection pool -**Impact**: Connection exhaustion under load with 50+ concurrent checks -**Fix**: -- Created shared HTTP transports (`defaultHTTPTransport` and `insecureHTTPTransport`) with connection pooling -- Client created per-check but reuses shared transport -- Per-check timeout from `timeoutMs` parameter (not hardcoded) -- CheckRedirect function respects `FollowRedirects` config - ---- - -## ⚡ PERFORMANCE OPTIMIZATIONS - -### ✅ 9. SNMP Batch Append Without Pre-allocation -**Status**: FIXED -**File**: `agent.go:277` -**Issue**: `var snmpBatch []*pb.SnmpResult` grows without capacity hint -**Impact**: 10-15 allocations per batch cycle (batch size = 50) -**Fix**: Changed to `snmpBatch := make([]*pb.SnmpResult, 0, 50)` - ---- - -### ✅ 10. WebSocket Payload Masking Loop -**Status**: FIXED -**File**: `websocket.go:292-294` -**Issue**: Byte-by-byte append in every frame write -**Impact**: 20-30% of frame write latency -**Fix**: Pre-allocate masked buffer, mask in-place, append once - ---- - -### 🔵 11. JSON Marshal in Hot Paths -**Status**: DEFERRED (Lower Priority) -**File**: `agent.go:141, 176` -**Issue**: JSON encoder allocated on every message -**Impact**: 15-25% of message send latency -**Note**: Deferred for future optimization - current performance acceptable - ---- - -## 🔒 SECURITY IMPROVEMENTS - -### ✅ 12. Plaintext WebSocket Warning -**Status**: FIXED -**File**: `websocket.go` WSDial function -**Issue**: Agent supports `ws://` (unencrypted) without warning -**Impact**: Credentials flow in plaintext if configured with http:// URL -**Fix**: Log prominent warning when scheme is `ws://` - ---- - -## 📊 COMPLETION STATUS - -**Total Issues**: 12 categories -**Fixed**: 11 ✅ -**Deferred**: 1 🔵 -**In Progress**: 0 ⏳ -**Pending**: 0 ⏳ - -**Progress**: 100% (11/11 critical+high issues COMPLETE, 1 medium optimization deferred) - ---- - -## ✅ ALL CRITICAL & HIGH SEVERITY FIXES COMPLETE - -### Additional Fixes During Testing -- ✅ Fixed reader goroutine context handling - added dedicated goroutine to close connection on cancellation -- ✅ Fixed HTTP client timeout - per-check timeout instead of hardcoded 10s -- ✅ Fixed HTTP redirect handling - CheckRedirect function respects config -- ✅ Updated job executor tests to expect error results on validation failures -- ✅ Fixed TestWSDialWriteHandshakeError to accept both error types -- ✅ Added missing 'strings' import to snmp_test.go - -### Final Verification -- ✅ All 249 tests pass (97.6% coverage) -- ✅ go vet clean -- ✅ go build successful -- ✅ 3 commits pushed to branch - ---- - -**Last Updated**: 2026-03-24 (ALL FIXES COMPLETE - 100%)