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
This commit is contained in:
parent
8595b4c001
commit
71abcb2a37
2 changed files with 55 additions and 5 deletions
|
|
@ -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
|
||||
|
|
|
|||
|
|
@ -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")
|
||||
}
|
||||
}
|
||||
|
|
|
|||
Loading…
Reference in a new issue