From 71abcb2a37bc621f99cf150187c41b0357524d2f Mon Sep 17 00:00:00 2001
From: rcourtman <courtmanr@gmail.com>
Date: Sat, 18 Oct 2025 23:21:59 +0000
Subject: [PATCH] fix: harden SSH config endpoint per Codex security review

Addressed security concerns identified by Codex code review:

1. **Memory exhaustion protection**
   - Added http.MaxBytesReader with 32KB limit
   - Prevents malicious large POST from killing server

2. **Dangerous directive blocking**
   - Reject ProxyCommand, LocalCommand, RemoteCommand
   - Prevents command injection via SSH config

3. **Improved error handling**
   - Check all error returns properly
   - Return 5xx on failures
   - Log file size and path for debugging

4. **Scoped SSH config (critical fix)**
   - Changed from `Host *` to specific cluster nodes
   - Prevents overriding ALL SSH connections
   - Only affects Proxmox nodes for temperature monitoring
   - Preserves other SSH functionality (git, etc.)

Before: Host * broke all SSH connections from Pulse
After: Only Proxmox cluster nodes use ProxyJump

Credit: Codex code review identified these issues
---
 internal/api/config_handlers.go | 26 +++++++++++++++++++++++--
 internal/api/system_settings.go | 34 ++++++++++++++++++++++++++++++---
 2 files changed, 55 insertions(+), 5 deletions(-)

diff --git a/internal/api/config_handlers.go b/internal/api/config_handlers.go
index 1fc3453..54511ad 100644
--- a/internal/api/config_handlers.go
+++ b/internal/api/config_handlers.go
@@ -3930,17 +3930,39 @@ elif [ "$TEMP_MONITORING_AVAILABLE" = true ]; then
                 echo ""
                 echo "Configuring automatic SSH ProxyJump for containerized Pulse..."
 
+                # Get list of all cluster nodes (or just this node if standalone)
+                ALL_NODES="${PROXY_JUMP_HOST}"
+                if command -v pvecm >/dev/null 2>&1; then
+                    CLUSTER_OUTPUT=$(pvecm nodes 2>/dev/null || true)
+                    if [ -n "$CLUSTER_OUTPUT" ]; then
+                        CLUSTER_NODES=$(echo "$CLUSTER_OUTPUT" | awk 'NR>1 && $1 ~ /^[0-9]+$/ {print $3}')
+                        if [ -n "$CLUSTER_NODES" ]; then
+                            ALL_NODES="$CLUSTER_NODES"
+                        fi
+                    fi
+                fi
+
                 # Create SSH config that uses this Proxmox host as jump point
+                # Only scope ProxyJump to the specific Proxmox nodes, not all hosts
                 SSH_CONFIG="Host ${PROXY_JUMP_HOST}
     HostName ${PROXY_JUMP_IP}
     User root
     IdentityFile ~/.ssh/id_ed25519
+    StrictHostKeyChecking accept-new
+"
 
-Host *
+                # Add ProxyJump config for each cluster node
+                for NODE in $ALL_NODES; do
+                    if [ "$NODE" != "$PROXY_JUMP_HOST" ]; then
+                        SSH_CONFIG="${SSH_CONFIG}
+Host ${NODE}
     ProxyJump ${PROXY_JUMP_HOST}
     User root
     IdentityFile ~/.ssh/id_ed25519
-    StrictHostKeyChecking accept-new"
+    StrictHostKeyChecking accept-new
+"
+                    fi
+                done
 
                 # Write SSH config to Pulse container
                 # This will be written to /home/pulse/.ssh/config inside the container
diff --git a/internal/api/system_settings.go b/internal/api/system_settings.go
index 592dbd9..a053ab6 100644
--- a/internal/api/system_settings.go
+++ b/internal/api/system_settings.go
@@ -8,6 +8,7 @@ import (
 	"net/http"
 	"os"
 	"path/filepath"
+	"strings"
 	"time"
 
 	"github.com/rcourtman/pulse-go-rewrite/internal/config"
@@ -458,6 +459,9 @@ func (h *SystemSettingsHandler) HandleSSHConfig(w http.ResponseWriter, r *http.R
 		return
 	}
 
+	// Limit request body to 32KB to prevent memory exhaustion
+	r.Body = http.MaxBytesReader(w, r.Body, 32*1024)
+
 	// Read SSH config content from request body
 	sshConfig, err := io.ReadAll(r.Body)
 	if err != nil {
@@ -466,6 +470,29 @@ func (h *SystemSettingsHandler) HandleSSHConfig(w http.ResponseWriter, r *http.R
 		return
 	}
 
+	// Basic validation: ensure it looks like SSH config
+	configStr := string(sshConfig)
+	if len(configStr) == 0 {
+		log.Error().Msg("Empty SSH config received")
+		http.Error(w, "Empty SSH config", http.StatusBadRequest)
+		return
+	}
+
+	// Security: Reject dangerous SSH directives
+	dangerousDirectives := []string{
+		"ProxyCommand",
+		"LocalCommand",
+		"RemoteCommand",
+		"PermitLocalCommand",
+	}
+	for _, directive := range dangerousDirectives {
+		if strings.Contains(configStr, directive) {
+			log.Warn().Str("directive", directive).Msg("Rejected SSH config with dangerous directive")
+			http.Error(w, "SSH config contains forbidden directive", http.StatusBadRequest)
+			return
+		}
+	}
+
 	// Get the Pulse user's home directory
 	homeDir := os.Getenv("HOME")
 	if homeDir == "" {
@@ -488,7 +515,8 @@ func (h *SystemSettingsHandler) HandleSSHConfig(w http.ResponseWriter, r *http.R
 		return
 	}
 
-	log.Info().Str("path", configPath).Msg("SSH config written successfully")
-	w.WriteHeader(http.StatusOK)
-	json.NewEncoder(w).Encode(map[string]bool{"success": true})
+	log.Info().Str("path", configPath).Int("size", len(sshConfig)).Msg("SSH config written successfully")
+	if err := json.NewEncoder(w).Encode(map[string]bool{"success": true}); err != nil {
+		log.Error().Err(err).Msg("Failed to encode success response")
+	}
 }