diff --git a/projects/msp-tools/guru-rmm/agent-legacy/GuruRMM-Agent.ps1 b/projects/msp-tools/guru-rmm/agent-legacy/GuruRMM-Agent.ps1 index 1bd6c97..75eced9 100644 --- a/projects/msp-tools/guru-rmm/agent-legacy/GuruRMM-Agent.ps1 +++ b/projects/msp-tools/guru-rmm/agent-legacy/GuruRMM-Agent.ps1 @@ -1,19 +1,56 @@ #Requires -Version 2.0 <# .SYNOPSIS - GuruRMM Legacy Agent - PowerShell-based agent for Windows Server 2008 R2 and older systems + GuruRMM Legacy Agent for Windows Server 2008 R2 and older systems. .DESCRIPTION - Lightweight RMM agent that: - - Registers with GuruRMM server using site code - - Reports system information - - Executes remote scripts/commands - - Monitors system health + This PowerShell-based agent is designed for legacy Windows systems that cannot + run the modern Rust-based GuruRMM agent. It provides basic RMM functionality + including registration, heartbeat, system info collection, and remote command + execution. + + IMPORTANT: This agent is intended for legacy systems only. For Windows 10/ + Server 2016 and newer, use the native Rust agent instead. + +.PARAMETER ConfigPath + Path to the agent configuration file. Default: $env:ProgramData\GuruRMM\agent.json + +.PARAMETER ServerUrl + The URL of the GuruRMM server (e.g., https://rmm.example.com) + +.PARAMETER SiteCode + The site code for agent registration (e.g., ACME-CORP-1234) + +.PARAMETER AllowInsecureTLS + [SECURITY RISK] Disables SSL/TLS certificate validation. Required ONLY for + systems with self-signed certificates or broken certificate chains. + + WARNING: This flag makes the connection vulnerable to man-in-the-middle + attacks. Only use on isolated networks or when absolutely necessary. + + This flag must be explicitly provided - certificate validation is enabled + by default. + +.PARAMETER Register + Register this agent with the server. + +.EXAMPLE + # Secure installation (recommended) + .\GuruRMM-Agent.ps1 -Register -ServerUrl "https://rmm.example.com" -SiteCode "ACME-CORP-1234" + +.EXAMPLE + # Insecure installation (legacy systems with self-signed certs ONLY) + .\GuruRMM-Agent.ps1 -Register -ServerUrl "https://rmm.example.com" -SiteCode "ACME-CORP-1234" -AllowInsecureTLS + +.EXAMPLE + # Run the agent + .\GuruRMM-Agent.ps1 .NOTES - Compatible with PowerShell 2.0+ (Windows Server 2008 R2) + Version: 1.1.0 + Requires: PowerShell 2.0+ + Platforms: Windows Server 2008 R2, Windows 7, and newer Author: GuruRMM - Version: 1.0.0 #> param( @@ -27,18 +64,23 @@ param( [string]$SiteCode, [Parameter()] - [string]$ServerUrl = "https://rmm-api.azcomputerguru.com" + [string]$ServerUrl = "https://rmm-api.azcomputerguru.com", + + [Parameter()] + [switch]$AllowInsecureTLS ) # ============================================================================ # Configuration # ============================================================================ -$script:Version = "1.0.0" +$script:Version = "1.1.0" $script:AgentType = "powershell-legacy" $script:ConfigDir = "$env:ProgramData\GuruRMM" $script:LogFile = "$script:ConfigDir\agent.log" $script:PollInterval = 60 # seconds +$script:AllowInsecureTLS = $AllowInsecureTLS +$script:TLSInitialized = $false # ============================================================================ # Logging @@ -67,6 +109,63 @@ function Write-Log { } catch {} } +# ============================================================================ +# TLS Initialization +# ============================================================================ + +function Initialize-TLS { + if ($script:TLSInitialized) { + return + } + + # Configure TLS - prefer TLS 1.2 + try { + [System.Net.ServicePointManager]::SecurityProtocol = [System.Net.SecurityProtocolType]::Tls12 + Write-Log "TLS 1.2 configured successfully" "INFO" + } catch { + Write-Log "TLS 1.2 not available, trying TLS 1.1" "WARN" + try { + [System.Net.ServicePointManager]::SecurityProtocol = [System.Net.SecurityProtocolType]::Tls11 + } catch { + Write-Log "TLS 1.1 not available - using system default TLS" "WARN" + try { + [System.Net.ServicePointManager]::SecurityProtocol = [System.Net.SecurityProtocolType]::Tls + } catch { + Write-Log "TLS configuration failed - connection security may be limited" "WARN" + } + } + } + + # Certificate validation - ONLY disable if explicitly requested + if ($script:AllowInsecureTLS) { + Write-Log "============================================" "WARN" + Write-Log "[SECURITY WARNING] Certificate validation DISABLED" "WARN" + Write-Log "This makes the connection vulnerable to MITM attacks" "WARN" + Write-Log "Only use on legacy systems with self-signed certificates" "WARN" + Write-Log "============================================" "WARN" + + # Log to Windows Event Log for audit trail + try { + $source = "GuruRMM" + if (-not [System.Diagnostics.EventLog]::SourceExists($source)) { + New-EventLog -LogName Application -Source $source -ErrorAction SilentlyContinue + } + Write-EventLog -LogName Application -Source $source -EventId 1001 -EntryType Warning ` + -Message "GuruRMM agent started with certificate validation disabled (-AllowInsecureTLS). This is a security risk." + } catch { + Write-Log "Could not write to Windows Event Log: $_" "WARN" + } + + [System.Net.ServicePointManager]::ServerCertificateValidationCallback = { $true } + } else { + Write-Log "Certificate validation ENABLED (secure mode)" "INFO" + # Ensure callback is reset to default (validate certificates) + [System.Net.ServicePointManager]::ServerCertificateValidationCallback = $null + } + + $script:TLSInitialized = $true +} + # ============================================================================ # HTTP Functions (PS 2.0 compatible) # ============================================================================ @@ -82,6 +181,9 @@ function Invoke-ApiRequest { $url = "$($script:Config.ServerUrl)$Endpoint" try { + # Initialize TLS settings (only runs once) + Initialize-TLS + # Use .NET WebClient for PS 2.0 compatibility $webClient = New-Object System.Net.WebClient $webClient.Headers.Add("Content-Type", "application/json") @@ -91,17 +193,6 @@ function Invoke-ApiRequest { $webClient.Headers.Add("Authorization", "Bearer $ApiKey") } - # Handle TLS (important for older systems) - try { - [System.Net.ServicePointManager]::SecurityProtocol = [System.Net.SecurityProtocolType]::Tls12 - } catch { - # Fallback for systems without TLS 1.2 - [System.Net.ServicePointManager]::SecurityProtocol = [System.Net.SecurityProtocolType]::Tls - } - - # Ignore certificate errors for self-signed certs (optional) - [System.Net.ServicePointManager]::ServerCertificateValidationCallback = { $true } - if ($Method -eq "GET") { $response = $webClient.DownloadString($url) } else { diff --git a/projects/msp-tools/guru-rmm/agent-legacy/Install-GuruRMM.ps1 b/projects/msp-tools/guru-rmm/agent-legacy/Install-GuruRMM.ps1 index 01be765..4bdbd8a 100644 --- a/projects/msp-tools/guru-rmm/agent-legacy/Install-GuruRMM.ps1 +++ b/projects/msp-tools/guru-rmm/agent-legacy/Install-GuruRMM.ps1 @@ -15,8 +15,20 @@ .PARAMETER ServerUrl The GuruRMM server URL (default: https://rmm-api.azcomputerguru.com) +.PARAMETER AllowInsecureTLS + [SECURITY RISK] Disables SSL/TLS certificate validation. Required ONLY for + systems with self-signed certificates or broken certificate chains. + + WARNING: This flag makes the connection vulnerable to man-in-the-middle + attacks. Only use on isolated networks or when absolutely necessary. + .EXAMPLE + # Secure installation (recommended) .\Install-GuruRMM.ps1 -SiteCode DARK-GROVE-7839 + +.EXAMPLE + # Insecure installation (legacy systems with self-signed certs ONLY) + .\Install-GuruRMM.ps1 -SiteCode DARK-GROVE-7839 -AllowInsecureTLS #> param( @@ -24,7 +36,10 @@ param( [string]$SiteCode, [Parameter()] - [string]$ServerUrl = "https://rmm-api.azcomputerguru.com" + [string]$ServerUrl = "https://rmm-api.azcomputerguru.com", + + [Parameter()] + [switch]$AllowInsecureTLS ) $ErrorActionPreference = "Stop" @@ -112,8 +127,15 @@ try { # Step 3: Register agent Write-Status "Registering with GuruRMM server..." +if ($AllowInsecureTLS) { + Write-Status "[SECURITY WARNING] Installing with certificate validation DISABLED" "WARN" + Write-Status "This makes the connection vulnerable to MITM attacks" "WARN" +} try { $registerArgs = "-ExecutionPolicy Bypass -File `"$destScript`" -SiteCode `"$SiteCode`" -ServerUrl `"$ServerUrl`"" + if ($AllowInsecureTLS) { + $registerArgs += " -AllowInsecureTLS" + } $process = Start-Process powershell.exe -ArgumentList $registerArgs -Wait -PassThru -NoNewWindow if ($process.ExitCode -ne 0) { @@ -137,13 +159,19 @@ try { # Step 5: Create scheduled task try { - # Create the task to run at startup and every 5 minutes + # Create the task to run at startup $taskCommand = "powershell.exe -ExecutionPolicy Bypass -WindowStyle Hidden -File `"$destScript`"" + if ($AllowInsecureTLS) { + $taskCommand += " -AllowInsecureTLS" + } # Create task that runs at system startup schtasks /create /tn $TaskName /tr $taskCommand /sc onstart /ru SYSTEM /rl HIGHEST /f | Out-Null Write-Status "Scheduled task created: $TaskName" "OK" + if ($AllowInsecureTLS) { + Write-Status "Task configured with -AllowInsecureTLS flag" "WARN" + } } catch { Write-Status "Failed to create scheduled task: $($_.Exception.Message)" "ERROR" Write-Status "You may need to manually create the task" "WARN" diff --git a/projects/msp-tools/guru-rmm/agent/Cargo.toml b/projects/msp-tools/guru-rmm/agent/Cargo.toml index 89af403..978d1ee 100644 --- a/projects/msp-tools/guru-rmm/agent/Cargo.toml +++ b/projects/msp-tools/guru-rmm/agent/Cargo.toml @@ -45,6 +45,9 @@ thiserror = "1" # UUID for identifiers uuid = { version = "1", features = ["v4", "serde"] } +# URL parsing for download validation +url = "2" + # SHA256 checksums for update verification sha2 = "0.10" diff --git a/projects/msp-tools/guru-rmm/agent/src/main.rs b/projects/msp-tools/guru-rmm/agent/src/main.rs index ad83c60..120d5cd 100644 --- a/projects/msp-tools/guru-rmm/agent/src/main.rs +++ b/projects/msp-tools/guru-rmm/agent/src/main.rs @@ -457,14 +457,14 @@ WantedBy=multi-user.target anyhow::bail!("systemctl enable failed"); } - println!("\n✓ GuruRMM Agent installed successfully!"); + println!("\n[OK] GuruRMM Agent installed successfully!"); println!("\nInstalled files:"); println!(" Binary: {}", binary_dest); println!(" Config: {}", config_dest); println!(" Service: {}", unit_file); if config_needs_manual_edit { - println!("\n⚠️ IMPORTANT: Edit {} with your server URL and API key!", config_dest); + println!("\n[WARNING] IMPORTANT: Edit {} with your server URL and API key!", config_dest); println!("\nNext steps:"); println!(" 1. Edit {} with your server URL and API key", config_dest); println!(" 2. Start the service: sudo systemctl start {}", SERVICE_NAME); @@ -475,9 +475,9 @@ WantedBy=multi-user.target .status(); if status.is_ok() && status.unwrap().success() { - println!("✓ Service started successfully!"); + println!("[OK] Service started successfully!"); } else { - println!("⚠️ Failed to start service. Check logs: sudo journalctl -u {} -f", SERVICE_NAME); + println!("[WARNING] Failed to start service. Check logs: sudo journalctl -u {} -f", SERVICE_NAME); } } @@ -556,7 +556,7 @@ async fn uninstall_systemd_service() -> Result<()> { .args(["daemon-reload"]) .status(); - println!("\n✓ GuruRMM Agent uninstalled successfully!"); + println!("\n[OK] GuruRMM Agent uninstalled successfully!"); println!("\nNote: Config directory {} was preserved.", CONFIG_DIR); println!("Remove it manually if no longer needed: sudo rm -rf {}", CONFIG_DIR); @@ -582,7 +582,7 @@ async fn start_service() -> Result<()> { .context("Failed to start service")?; if status.success() { - println!("** Service started successfully"); + println!("[OK] Service started successfully"); println!("Check status: sudo systemctl status gururmm-agent"); } else { anyhow::bail!("Failed to start service. Check: sudo journalctl -u gururmm-agent -n 50"); @@ -616,7 +616,7 @@ async fn stop_service() -> Result<()> { .context("Failed to stop service")?; if status.success() { - println!("** Service stopped successfully"); + println!("[OK] Service stopped successfully"); } else { anyhow::bail!("Failed to stop service"); } diff --git a/projects/msp-tools/guru-rmm/agent/src/updater/mod.rs b/projects/msp-tools/guru-rmm/agent/src/updater/mod.rs index 757ede3..c8a7b72 100644 --- a/projects/msp-tools/guru-rmm/agent/src/updater/mod.rs +++ b/projects/msp-tools/guru-rmm/agent/src/updater/mod.rs @@ -177,7 +177,36 @@ impl AgentUpdater { } /// Download the new binary to a temp file + /// + /// Security: Validates URL against allowed domains and requires HTTPS for external hosts async fn download_binary(&self, url: &str) -> Result { + // Validate URL is from trusted domain + let allowed_domains = [ + "rmm-api.azcomputerguru.com", + "downloads.azcomputerguru.com", + "172.16.3.30", // Internal server + ]; + + let parsed_url = url::Url::parse(url) + .context("Invalid download URL")?; + + let host = parsed_url.host_str() + .ok_or_else(|| anyhow::anyhow!("No host in download URL"))?; + + if !allowed_domains.iter().any(|d| host == *d || host.ends_with(&format!(".{}", d))) { + return Err(anyhow::anyhow!( + "Download URL host '{}' not in allowed domains", + host + )); + } + + // Require HTTPS (except for local/internal IPs) + if parsed_url.scheme() != "https" && !host.starts_with("172.16.") && !host.starts_with("192.168.") { + return Err(anyhow::anyhow!("Download URL must use HTTPS")); + } + + info!("[OK] URL validation passed: {}", url); + let response = self.http_client.get(url) .send() .await @@ -273,10 +302,26 @@ impl AgentUpdater { #[cfg(unix)] async fn create_unix_rollback_watchdog(&self) -> Result<()> { + use std::os::unix::fs::PermissionsExt; + let backup_path = self.config.backup_path(); let binary_path = &self.config.binary_path; let timeout = self.config.rollback_timeout_secs; + // Use secure directory instead of /tmp/ (world-writable) + let script_dir = PathBuf::from("/var/run/gururmm"); + + // Create directory if needed with restricted permissions (owner only) + if !script_dir.exists() { + tokio::fs::create_dir_all(&script_dir).await + .context("Failed to create secure script directory")?; + std::fs::set_permissions(&script_dir, std::fs::Permissions::from_mode(0o700)) + .context("Failed to set script directory permissions")?; + } + + // Use UUID in filename to prevent predictable paths + let script_path = script_dir.join(format!("rollback-{}.sh", Uuid::new_v4())); + let script = format!(r#"#!/bin/bash # GuruRMM Rollback Watchdog # Auto-generated - will be deleted after successful update @@ -284,49 +329,50 @@ impl AgentUpdater { BACKUP="{backup}" BINARY="{binary}" TIMEOUT={timeout} +SCRIPT_PATH="{script}" sleep $TIMEOUT # Check if agent service is running if ! systemctl is-active --quiet gururmm-agent 2>/dev/null; then - echo "Agent not running after update, rolling back..." + echo "[WARNING] Agent not running after update, rolling back..." if [ -f "$BACKUP" ]; then cp "$BACKUP" "$BINARY" chmod +x "$BINARY" systemctl start gururmm-agent - echo "Rollback completed" + echo "[OK] Rollback completed" else - echo "No backup file found!" + echo "[ERROR] No backup file found!" fi fi # Clean up this script -rm -f /tmp/gururmm-rollback.sh +rm -f "$SCRIPT_PATH" "#, backup = backup_path.display(), binary = binary_path.display(), - timeout = timeout + timeout = timeout, + script = script_path.display() ); - let script_path = PathBuf::from("/tmp/gururmm-rollback.sh"); - fs::write(&script_path, script).await?; + fs::write(&script_path, script).await + .context("Failed to write rollback script")?; - // Make executable and run in background - tokio::process::Command::new("chmod") - .arg("+x") - .arg(&script_path) - .status() - .await?; + // Set restrictive permissions (700 - owner only) + std::fs::set_permissions(&script_path, std::fs::Permissions::from_mode(0o700)) + .context("Failed to set rollback script permissions")?; - // Spawn as detached background process - tokio::process::Command::new("nohup") + // Spawn as detached background process using setsid (not nohup with "&" literal arg) + tokio::process::Command::new("setsid") .arg("bash") .arg(&script_path) - .arg("&") + .stdin(std::process::Stdio::null()) + .stdout(std::process::Stdio::null()) + .stderr(std::process::Stdio::null()) .spawn() .context("Failed to spawn rollback watchdog")?; - info!("Rollback watchdog started (timeout: {}s)", timeout); + info!("[OK] Rollback watchdog started (timeout: {}s)", timeout); Ok(()) } @@ -524,12 +570,29 @@ Remove-Item -Path $MyInvocation.MyCommand.Path -Force pub async fn cancel_rollback_watchdog(&self) { #[cfg(unix)] { - // Kill the watchdog script + // Kill any running rollback watchdog scripts let _ = tokio::process::Command::new("pkill") - .args(["-f", "gururmm-rollback.sh"]) + .args(["-f", "rollback-.*\\.sh"]) .status() .await; - let _ = fs::remove_file("/tmp/gururmm-rollback.sh").await; + + // Clean up the secure script directory + let script_dir = PathBuf::from("/var/run/gururmm"); + if script_dir.exists() { + // Remove all rollback scripts in the directory + if let Ok(mut entries) = tokio::fs::read_dir(&script_dir).await { + while let Ok(Some(entry)) = entries.next_entry().await { + let path = entry.path(); + if path.file_name() + .and_then(|n| n.to_str()) + .map(|n| n.starts_with("rollback-")) + .unwrap_or(false) + { + let _ = fs::remove_file(&path).await; + } + } + } + } } #[cfg(windows)] diff --git a/projects/msp-tools/guru-rmm/dashboard/src/api/client.ts b/projects/msp-tools/guru-rmm/dashboard/src/api/client.ts index 67c2f28..71794c1 100644 --- a/projects/msp-tools/guru-rmm/dashboard/src/api/client.ts +++ b/projects/msp-tools/guru-rmm/dashboard/src/api/client.ts @@ -1,4 +1,4 @@ -import axios from "axios"; +import axios, { AxiosError } from "axios"; // Default to production URL, override with VITE_API_URL for local dev const API_URL = import.meta.env.VITE_API_URL || "https://rmm-api.azcomputerguru.com"; @@ -10,22 +10,41 @@ export const api = axios.create({ }, }); -// Add auth token to requests +// Token management - use sessionStorage (cleared on tab close) instead of localStorage +// This provides better security against XSS attacks as tokens are not persisted +const TOKEN_KEY = "gururmm_auth_token"; + +export const getToken = (): string | null => { + return sessionStorage.getItem(TOKEN_KEY); +}; + +export const setToken = (token: string): void => { + sessionStorage.setItem(TOKEN_KEY, token); +}; + +export const clearToken = (): void => { + sessionStorage.removeItem(TOKEN_KEY); +}; + +// Request interceptor - add auth header api.interceptors.request.use((config) => { - const token = localStorage.getItem("token"); + const token = getToken(); if (token) { config.headers.Authorization = `Bearer ${token}`; } return config; }); -// Handle auth errors +// Response interceptor - handle 401 unauthorized api.interceptors.response.use( (response) => response, - (error) => { + (error: AxiosError) => { if (error.response?.status === 401) { - localStorage.removeItem("token"); - window.location.href = "/login"; + clearToken(); + // Use a more graceful redirect that preserves SPA state + if (window.location.pathname !== "/login") { + window.location.href = "/login"; + } } return Promise.reject(error); } @@ -156,9 +175,31 @@ export interface RegisterRequest { // API functions export const authApi = { - login: (data: LoginRequest) => api.post("/api/auth/login", data), - register: (data: RegisterRequest) => api.post("/api/auth/register", data), + login: async (data: LoginRequest): Promise => { + const response = await api.post("/api/auth/login", data); + if (response.data.token) { + setToken(response.data.token); + } + return response.data; + }, + + register: async (data: RegisterRequest): Promise => { + const response = await api.post("/api/auth/register", data); + if (response.data.token) { + setToken(response.data.token); + } + return response.data; + }, + me: () => api.get("/api/auth/me"), + + logout: (): void => { + clearToken(); + }, + + isAuthenticated: (): boolean => { + return !!getToken(); + }, }; export const agentsApi = { diff --git a/projects/msp-tools/guru-rmm/dashboard/src/hooks/useAuth.tsx b/projects/msp-tools/guru-rmm/dashboard/src/hooks/useAuth.tsx index c1f853a..574c0fd 100644 --- a/projects/msp-tools/guru-rmm/dashboard/src/hooks/useAuth.tsx +++ b/projects/msp-tools/guru-rmm/dashboard/src/hooks/useAuth.tsx @@ -1,9 +1,9 @@ import { createContext, useContext, useState, useEffect, ReactNode } from "react"; -import { User, authApi } from "../api/client"; +import { User, authApi, getToken, clearToken } from "../api/client"; interface AuthContextType { user: User | null; - token: string | null; + isAuthenticated: boolean; isLoading: boolean; login: (email: string, password: string) => Promise; register: (email: string, password: string, name?: string) => Promise; @@ -14,46 +14,49 @@ const AuthContext = createContext(null); export function AuthProvider({ children }: { children: ReactNode }) { const [user, setUser] = useState(null); - const [token, setToken] = useState(() => localStorage.getItem("token")); const [isLoading, setIsLoading] = useState(true); + // Check authentication status on mount useEffect(() => { - if (token) { - authApi - .me() - .then((res) => setUser(res.data)) - .catch(() => { - localStorage.removeItem("token"); - setToken(null); - }) - .finally(() => setIsLoading(false)); - } else { + const checkAuth = async () => { + const token = getToken(); + if (token) { + try { + const res = await authApi.me(); + setUser(res.data); + } catch { + // Token is invalid or expired, clear it + clearToken(); + setUser(null); + } + } setIsLoading(false); - } - }, [token]); + }; + + checkAuth(); + }, []); const login = async (email: string, password: string) => { - const res = await authApi.login({ email, password }); - localStorage.setItem("token", res.data.token); - setToken(res.data.token); - setUser(res.data.user); + const response = await authApi.login({ email, password }); + // Token is automatically stored by authApi.login + setUser(response.user); }; const register = async (email: string, password: string, name?: string) => { - const res = await authApi.register({ email, password, name }); - localStorage.setItem("token", res.data.token); - setToken(res.data.token); - setUser(res.data.user); + const response = await authApi.register({ email, password, name }); + // Token is automatically stored by authApi.register + setUser(response.user); }; const logout = () => { - localStorage.removeItem("token"); - setToken(null); + authApi.logout(); setUser(null); }; + const isAuthenticated = authApi.isAuthenticated(); + return ( - + {children} ); diff --git a/projects/msp-tools/guru-rmm/dashboard/src/pages/Login.tsx b/projects/msp-tools/guru-rmm/dashboard/src/pages/Login.tsx index fd3b70d..889a7a9 100644 --- a/projects/msp-tools/guru-rmm/dashboard/src/pages/Login.tsx +++ b/projects/msp-tools/guru-rmm/dashboard/src/pages/Login.tsx @@ -1,10 +1,16 @@ import { useState, FormEvent } from "react"; import { Link, useNavigate } from "react-router-dom"; +import { AxiosError } from "axios"; import { useAuth } from "../hooks/useAuth"; import { Card, CardHeader, CardTitle, CardDescription, CardContent } from "../components/Card"; import { Input } from "../components/Input"; import { Button } from "../components/Button"; +interface ApiErrorResponse { + error?: string; + message?: string; +} + export function Login() { const [email, setEmail] = useState(""); const [password, setPassword] = useState(""); @@ -21,8 +27,15 @@ export function Login() { try { await login(email, password); navigate("/"); - } catch (err: any) { - setError(err.response?.data?.error || "Login failed. Please try again."); + } catch (err) { + if (err instanceof AxiosError) { + const errorData = err.response?.data as ApiErrorResponse | undefined; + setError(errorData?.error || errorData?.message || err.message || "Login failed. Please try again."); + } else if (err instanceof Error) { + setError(err.message); + } else { + setError("An unexpected error occurred"); + } } finally { setIsLoading(false); } diff --git a/projects/msp-tools/guru-rmm/dashboard/src/pages/Register.tsx b/projects/msp-tools/guru-rmm/dashboard/src/pages/Register.tsx index a0cc45e..a865b0e 100644 --- a/projects/msp-tools/guru-rmm/dashboard/src/pages/Register.tsx +++ b/projects/msp-tools/guru-rmm/dashboard/src/pages/Register.tsx @@ -1,10 +1,16 @@ import { useState, FormEvent } from "react"; import { Link, useNavigate } from "react-router-dom"; +import { AxiosError } from "axios"; import { useAuth } from "../hooks/useAuth"; import { Card, CardHeader, CardTitle, CardDescription, CardContent } from "../components/Card"; import { Input } from "../components/Input"; import { Button } from "../components/Button"; +interface ApiErrorResponse { + error?: string; + message?: string; +} + export function Register() { const [email, setEmail] = useState(""); const [password, setPassword] = useState(""); @@ -34,8 +40,15 @@ export function Register() { try { await register(email, password, name || undefined); navigate("/"); - } catch (err: any) { - setError(err.response?.data?.error || "Registration failed. Please try again."); + } catch (err) { + if (err instanceof AxiosError) { + const errorData = err.response?.data as ApiErrorResponse | undefined; + setError(errorData?.error || errorData?.message || err.message || "Registration failed. Please try again."); + } else if (err instanceof Error) { + setError(err.message); + } else { + setError("An unexpected error occurred"); + } } finally { setIsLoading(false); } diff --git a/projects/msp-tools/guru-rmm/docs/REMEDIATION_PLAN.md b/projects/msp-tools/guru-rmm/docs/REMEDIATION_PLAN.md new file mode 100644 index 0000000..9e25e06 --- /dev/null +++ b/projects/msp-tools/guru-rmm/docs/REMEDIATION_PLAN.md @@ -0,0 +1,1276 @@ +# GuruRMM Remediation Plan + +**Generated:** 2026-01-20 +**Review Source:** Full-scale code review of all components +**Priority:** Critical issues first, then major, then warnings + +--- + +## Executive Summary + +This document provides specific code fixes for all critical and major issues identified during the comprehensive code review. Issues are organized by component and priority. + +**Issue Counts:** +- CRITICAL: 16 (must fix before production) +- MAJOR: 14 (should fix before production) +- WARNING: 28 (recommended improvements) + +--- + +## Phase 1: Critical Security Fixes (MUST DO) + +### 1.1 Server: Restrict CORS Configuration + +**File:** `server/src/main.rs` +**Lines:** 133-136 +**Severity:** CRITICAL + +**Current Code:** +```rust +let cors = CorsLayer::new() + .allow_origin(Any) + .allow_methods(Any) + .allow_headers(Any); +``` + +**Fixed Code:** +```rust +use tower_http::cors::{CorsLayer, AllowOrigin}; +use http::HeaderValue; + +// Get allowed origins from environment +let dashboard_origin = std::env::var("DASHBOARD_URL") + .unwrap_or_else(|_| "https://rmm.azcomputerguru.com".to_string()); + +let cors = CorsLayer::new() + .allow_origin(AllowOrigin::exact( + HeaderValue::from_str(&dashboard_origin).expect("Invalid DASHBOARD_URL") + )) + .allow_methods([ + http::Method::GET, + http::Method::POST, + http::Method::PUT, + http::Method::DELETE, + http::Method::OPTIONS, + ]) + .allow_headers([ + http::header::AUTHORIZATION, + http::header::CONTENT_TYPE, + http::header::ACCEPT, + ]) + .allow_credentials(true); +``` + +**Environment Variable:** +```bash +DASHBOARD_URL=https://rmm.azcomputerguru.com +``` + +--- + +### 1.2 Server: Add Authentication to All Endpoints + +**File:** `server/src/api/agents.rs` +**Severity:** CRITICAL + +**Problem:** Most endpoints lack the `AuthUser` extractor. + +**Fix Pattern - Apply to ALL endpoint functions:** + +```rust +// BEFORE (insecure) +pub async fn list_agents( + State(state): State, +) -> Result>, (StatusCode, String)> { + +// AFTER (secure) +pub async fn list_agents( + State(state): State, + _user: AuthUser, // Add this to require authentication +) -> Result>, (StatusCode, String)> { +``` + +**Endpoints requiring AuthUser addition:** + +**agents.rs:** +- `list_agents` (line 62) +- `get_agent` (line 74) +- `delete_agent` (line 87) +- `get_stats` (line 109) +- `move_agent` (line 126) +- `list_agents_with_details` (line 152) +- `list_unassigned_agents` (line 163) +- `get_agent_state` (line 175) +- `register_agent` (line 32) - Add auth OR implement rate limiting + +**commands.rs:** +- `send_command` (line 46) +- `list_commands` (line 103) +- `get_command` (line 117) + +**metrics.rs:** +- `get_agent_metrics` (line 29) +- `get_summary` (line 57) + +**Complete Fix for commands.rs:** +```rust +// server/src/api/commands.rs + +use crate::auth::AuthUser; + +pub async fn send_command( + State(state): State, + user: AuthUser, // ADD THIS + Path(agent_id): Path, + Json(req): Json, +) -> Result, (StatusCode, String)> { + // Log who sent the command + tracing::info!( + user_id = %user.user_id, + agent_id = %agent_id, + command_type = ?req.command_type, + "Command sent by user" + ); + + // Create command with user attribution + let command = db::create_command( + &state.db, + agent_id, + &req.command_type, + &req.command, + req.timeout_seconds, + req.elevated.unwrap_or(false), + Some(user.user_id), // Track who created it + ) + .await + .map_err(|e| (StatusCode::INTERNAL_SERVER_ERROR, e.to_string()))?; + + // ... rest of function +} + +pub async fn list_commands( + State(state): State, + _user: AuthUser, // ADD THIS +) -> Result>, (StatusCode, String)> { + // ... existing code +} + +pub async fn get_command( + State(state): State, + _user: AuthUser, // ADD THIS + Path(command_id): Path, +) -> Result, (StatusCode, String)> { + // ... existing code +} +``` + +--- + +### 1.3 Server: Add Rate Limiting to Registration + +**File:** `server/src/main.rs` +**Severity:** CRITICAL + +**Add dependency to Cargo.toml:** +```toml +tower-governor = "0.3" +``` + +**Implementation:** +```rust +// server/src/main.rs + +use tower_governor::{GovernorLayer, GovernorConfigBuilder, KeyExtractor}; +use std::net::IpAddr; + +// Custom key extractor for rate limiting by IP +#[derive(Clone)] +struct RealIpKeyExtractor; + +impl KeyExtractor for RealIpKeyExtractor { + type Key = IpAddr; + + fn extract(&self, req: &http::Request) -> Result { + // Try X-Forwarded-For first (for reverse proxy) + if let Some(forwarded) = req.headers().get("x-forwarded-for") { + if let Ok(s) = forwarded.to_str() { + if let Some(ip) = s.split(',').next() { + if let Ok(addr) = ip.trim().parse() { + return Ok(addr); + } + } + } + } + + // Fall back to connection info + req.extensions() + .get::>() + .map(|ci| ci.0.ip()) + .ok_or(GovernorError::UnableToExtractKey) + } +} + +// In main() or router setup: +let registration_limiter = GovernorConfigBuilder::default() + .per_second(1) // 1 request per second + .burst_size(5) // Allow burst of 5 + .key_extractor(RealIpKeyExtractor) + .finish() + .unwrap(); + +let app = Router::new() + // Rate-limited registration endpoints + .route("/api/agents/register", + post(register_agent).layer(GovernorLayer::new(®istration_limiter))) + .route("/api/agents/register-legacy", + post(register_legacy).layer(GovernorLayer::new(®istration_limiter))) + // ... other routes without rate limiting +``` + +--- + +### 1.4 Agent: Remove Unicode Characters + +**File:** `agent/src/main.rs` +**Lines:** 460, 467, 478, 480 +**Severity:** CRITICAL + +**Find and replace all Unicode characters:** + +```rust +// BEFORE +println!("\n✓ GuruRMM Agent installed successfully!"); +println!("\n⚠️ IMPORTANT: Edit {} with your server URL and API key!", config_dest); + +// AFTER +println!("\n[OK] GuruRMM Agent installed successfully!"); +println!("\n[WARNING] IMPORTANT: Edit {} with your server URL and API key!", config_dest); +``` + +**Complete list of replacements:** +| Before | After | +|--------|-------| +| `✓` | `[OK]` | +| `✗` | `[ERROR]` | +| `⚠️` | `[WARNING]` | +| `ℹ️` | `[INFO]` | +| `→` | `->` | +| `•` | `-` | + +--- + +### 1.5 Agent: Secure Update Download with Certificate Pinning + +**File:** `agent/src/updater/mod.rs` +**Severity:** CRITICAL + +**Add to Cargo.toml:** +```toml +[dependencies] +sha2 = "0.10" +``` + +**Implementation:** +```rust +// agent/src/updater/mod.rs + +use sha2::{Sha256, Digest}; + +impl Updater { + /// Download binary with certificate validation + async fn download_binary(&self, url: &str, expected_checksum: &str) -> Result { + // Validate URL is from trusted domain + let allowed_domains = [ + "rmm-api.azcomputerguru.com", + "downloads.azcomputerguru.com", + ]; + + let parsed_url = url::Url::parse(url) + .context("Invalid download URL")?; + + let host = parsed_url.host_str() + .ok_or_else(|| anyhow::anyhow!("No host in download URL"))?; + + if !allowed_domains.iter().any(|d| host == *d || host.ends_with(&format!(".{}", d))) { + return Err(anyhow::anyhow!( + "Download URL host '{}' not in allowed domains: {:?}", + host, allowed_domains + )); + } + + // Require HTTPS + if parsed_url.scheme() != "https" { + return Err(anyhow::anyhow!("Download URL must use HTTPS")); + } + + info!("Downloading update from: {}", url); + + let response = self.http_client + .get(url) + .send() + .await + .context("HTTP request failed")?; + + if !response.status().is_success() { + return Err(anyhow::anyhow!( + "Download failed with status: {}", + response.status() + )); + } + + let bytes = response.bytes().await + .context("Failed to read response body")?; + + // Verify checksum BEFORE writing to disk + let mut hasher = Sha256::new(); + hasher.update(&bytes); + let actual_checksum = format!("{:x}", hasher.finalize()); + + if actual_checksum != expected_checksum { + return Err(anyhow::anyhow!( + "Checksum mismatch! Expected: {}, Got: {}", + expected_checksum, actual_checksum + )); + } + + info!("Checksum verified: {}", actual_checksum); + + // Write to secure temp location + let temp_dir = std::env::temp_dir(); + let temp_path = temp_dir.join(format!("gururmm-update-{}.tmp", uuid::Uuid::new_v4())); + + tokio::fs::write(&temp_path, &bytes).await + .context("Failed to write update binary")?; + + // Set restrictive permissions on Unix + #[cfg(unix)] + { + use std::os::unix::fs::PermissionsExt; + let perms = std::fs::Permissions::from_mode(0o700); + std::fs::set_permissions(&temp_path, perms)?; + } + + Ok(temp_path) + } +} +``` + +--- + +### 1.6 Agent: Fix Insecure Temp File Creation + +**File:** `agent/src/updater/mod.rs` +**Lines:** 275-331 +**Severity:** CRITICAL + +**Problem:** Using world-writable `/tmp/` for rollback script. + +**Fixed Code:** +```rust +// agent/src/updater/mod.rs + +#[cfg(unix)] +async fn create_rollback_script(&self, backup_path: &Path, target_path: &Path) -> Result { + use std::os::unix::fs::PermissionsExt; + + // Use /var/run or agent's own directory instead of /tmp + let script_dir = if Path::new("/var/run/gururmm").exists() { + PathBuf::from("/var/run/gururmm") + } else { + // Fall back to agent's config directory + PathBuf::from("/etc/gururmm") + }; + + // Ensure directory exists with proper permissions + tokio::fs::create_dir_all(&script_dir).await?; + + let script_path = script_dir.join(format!("rollback-{}.sh", uuid::Uuid::new_v4())); + + let script = format!(r#"#!/bin/bash +set -e + +# Rollback script for GuruRMM agent update +BACKUP="{}" +TARGET="{}" +SCRIPT_PATH="$0" + +# Function to cleanup +cleanup() {{ + rm -f "$SCRIPT_PATH" +}} +trap cleanup EXIT + +# Wait for parent process to exit +sleep 2 + +# Restore backup +if [ -f "$BACKUP" ]; then + cp "$BACKUP" "$TARGET" + chmod +x "$TARGET" + + # Restart service + if command -v systemctl &> /dev/null; then + systemctl restart gururmm-agent + fi +fi +"#, + backup_path.display(), + target_path.display() + ); + + // Write script with restrictive permissions + tokio::fs::write(&script_path, &script).await?; + + // Set permissions to 700 (owner only) + let perms = std::fs::Permissions::from_mode(0o700); + std::fs::set_permissions(&script_path, perms)?; + + // Verify ownership (should be root) + let metadata = std::fs::metadata(&script_path)?; + if metadata.uid() != 0 { + warn!("Rollback script not owned by root - this may be a security issue"); + } + + Ok(script_path) +} +``` + +--- + +### 1.7 Dashboard: Improve Token Storage Security + +**File:** `dashboard/src/api/client.ts` +**Severity:** CRITICAL + +**Option A: Use httpOnly Cookies (Recommended)** + +This requires server-side changes. The server should set the token as an httpOnly cookie. + +**Server-side change (server/src/api/auth.rs):** +```rust +use axum::response::{IntoResponse, Response}; +use http::header::{SET_COOKIE, HeaderValue}; + +pub async fn login( + State(state): State, + Json(req): Json, +) -> Result { + // ... existing validation ... + + let token = create_jwt(&user)?; + + // Set httpOnly cookie instead of returning token in body + let cookie = format!( + "auth_token={}; HttpOnly; Secure; SameSite=Strict; Path=/; Max-Age=86400", + token + ); + + let mut response = Json(LoginResponse { + user_id: user.id, + email: user.email, + name: user.name, + role: user.role, + }).into_response(); + + response.headers_mut().insert( + SET_COOKIE, + HeaderValue::from_str(&cookie).unwrap() + ); + + Ok(response) +} +``` + +**Dashboard change (client.ts):** +```typescript +// dashboard/src/api/client.ts + +const api = axios.create({ + baseURL: import.meta.env.VITE_API_URL || 'https://rmm-api.azcomputerguru.com', + withCredentials: true, // Send cookies with requests +}); + +// Remove localStorage token handling - cookies are automatic + +export const authApi = { + login: async (email: string, password: string) => { + const response = await api.post('/api/auth/login', { email, password }); + // No need to store token - it's in httpOnly cookie + return response.data; + }, + + logout: async () => { + await api.post('/api/auth/logout'); // Server clears cookie + }, + + // ... +}; +``` + +**Option B: If httpOnly cookies aren't feasible (short-term)** + +Add XSS protection and token encryption: + +```typescript +// dashboard/src/api/client.ts + +import CryptoJS from 'crypto-js'; + +// Use session-specific key (generated on page load) +const SESSION_KEY = CryptoJS.lib.WordArray.random(32).toString(); + +const encryptToken = (token: string): string => { + return CryptoJS.AES.encrypt(token, SESSION_KEY).toString(); +}; + +const decryptToken = (encrypted: string): string | null => { + try { + const bytes = CryptoJS.AES.decrypt(encrypted, SESSION_KEY); + return bytes.toString(CryptoJS.enc.Utf8); + } catch { + return null; + } +}; + +export const authApi = { + login: async (email: string, password: string) => { + const response = await api.post('/api/auth/login', { email, password }); + if (response.data.token) { + // Store encrypted token in sessionStorage (cleared on tab close) + sessionStorage.setItem('auth_token', encryptToken(response.data.token)); + } + return response.data; + }, + + getToken: (): string | null => { + const encrypted = sessionStorage.getItem('auth_token'); + return encrypted ? decryptToken(encrypted) : null; + }, + + // ... +}; + +// Update interceptor +api.interceptors.request.use((config) => { + const token = authApi.getToken(); + if (token) { + config.headers.Authorization = `Bearer ${token}`; + } + return config; +}); +``` + +--- + +### 1.8 Dashboard: Add Command Input Validation + +**File:** `dashboard/src/pages/AgentDetail.tsx` +**Lines:** 236-240 +**Severity:** CRITICAL + +**Add validation and confirmation:** +```typescript +// dashboard/src/pages/AgentDetail.tsx + +import { useState, useCallback } from 'react'; + +// Add dangerous command patterns +const DANGEROUS_PATTERNS = [ + /rm\s+-rf\s+\//i, + /format\s+[a-z]:/i, + /del\s+\/[fqs]/i, + /shutdown/i, + /reboot/i, + /init\s+0/i, + /mkfs/i, + /dd\s+if=/i, + />\s*\/dev\//i, + /chmod\s+-R\s+777/i, +]; + +const WARN_PATTERNS = [ + /password/i, + /credential/i, + /secret/i, + /api.?key/i, + /token/i, +]; + +interface CommandValidation { + isValid: boolean; + isDangerous: boolean; + hasWarnings: boolean; + messages: string[]; +} + +const validateCommand = (command: string): CommandValidation => { + const messages: string[] = []; + let isDangerous = false; + let hasWarnings = false; + + // Check for empty command + if (!command.trim()) { + return { isValid: false, isDangerous: false, hasWarnings: false, messages: ['Command cannot be empty'] }; + } + + // Check for dangerous patterns + for (const pattern of DANGEROUS_PATTERNS) { + if (pattern.test(command)) { + isDangerous = true; + messages.push(`[DANGER] Potentially destructive command detected: ${pattern.source}`); + } + } + + // Check for warning patterns + for (const pattern of WARN_PATTERNS) { + if (pattern.test(command)) { + hasWarnings = true; + messages.push(`[WARNING] Command may expose sensitive data: ${pattern.source}`); + } + } + + // Check command length + if (command.length > 10000) { + messages.push('[WARNING] Command is very long - consider using a script file'); + hasWarnings = true; + } + + return { + isValid: true, + isDangerous, + hasWarnings, + messages, + }; +}; + +// In the component: +const [showConfirmDialog, setShowConfirmDialog] = useState(false); +const [pendingCommand, setPendingCommand] = useState(''); +const [validationResult, setValidationResult] = useState(null); + +const handleSendCommand = useCallback(async () => { + if (!command.trim() || !agent) return; + + const validation = validateCommand(command); + setValidationResult(validation); + + if (!validation.isValid) { + return; + } + + if (validation.isDangerous || validation.hasWarnings) { + setPendingCommand(command); + setShowConfirmDialog(true); + return; + } + + await executeCommand(command); +}, [command, agent]); + +const executeCommand = async (cmd: string) => { + try { + await agentsApi.sendCommand(agent!.id, { + command_type: commandType, + command: cmd, + timeout_seconds: 300, + elevated: false, + }); + setCommand(''); + setShowConfirmDialog(false); + setPendingCommand(''); + } catch (err) { + console.error('Failed to send command:', err); + } +}; + +// Add confirmation dialog JSX: +{showConfirmDialog && ( +
+
+

+ {validationResult?.isDangerous ? '[DANGER] Confirm Dangerous Command' : '[WARNING] Confirm Command'} +

+ +
+ {validationResult?.messages.map((msg, i) => ( +

+ {msg} +

+ ))} +
+ + 
+        {pendingCommand}
+
+ +

+ This command will be executed on: {agent?.hostname} +

+ +
+ + +
+
+
+)} +``` + +--- + +### 1.9 Legacy Agent: Document Certificate Validation Bypass + +**File:** `agent-legacy/GuruRMM-Agent.ps1` +**Line:** 103 +**Severity:** CRITICAL + +**Cannot be fully fixed** for legacy systems that lack proper certificate chains. + +**Mitigation: Add explicit parameter and logging:** +```powershell +# GuruRMM-Agent.ps1 + +param( + [string]$ServerUrl = "", + [string]$SiteCode = "", + [switch]$AllowInsecureTLS, # NEW: Must be explicitly set + [switch]$Register, + [switch]$Run +) + +# Near line 103, replace: +function Initialize-TLS { + # Configure TLS + try { + [System.Net.ServicePointManager]::SecurityProtocol = [System.Net.SecurityProtocolType]::Tls12 + Write-Log "TLS 1.2 configured successfully" "INFO" + } catch { + Write-Log "TLS 1.2 not available, falling back to TLS 1.1" "WARN" + try { + [System.Net.ServicePointManager]::SecurityProtocol = [System.Net.SecurityProtocolType]::Tls11 + } catch { + Write-Log "TLS 1.1 not available - connection may be insecure" "ERROR" + } + } + + # Certificate validation - ONLY disable if explicitly requested + if ($script:AllowInsecureTLS) { + Write-Log "[SECURITY WARNING] Certificate validation DISABLED - vulnerable to MITM attacks" "WARN" + Write-Log "[SECURITY WARNING] This should only be used on legacy systems with self-signed certificates" "WARN" + + # Log to Windows Event Log for audit trail + try { + Write-EventLog -LogName Application -Source "GuruRMM" -EventId 1001 -EntryType Warning ` + -Message "GuruRMM agent started with certificate validation disabled. This is a security risk." + } catch { + Write-Log "Could not write to Windows Event Log" "WARN" + } + + [System.Net.ServicePointManager]::ServerCertificateValidationCallback = { $true } + } else { + Write-Log "Certificate validation ENABLED (secure mode)" "INFO" + # Ensure callback is not set (use default validation) + [System.Net.ServicePointManager]::ServerCertificateValidationCallback = $null + } +} + +# Update installation documentation: +<# +.SYNOPSIS + GuruRMM Legacy Agent for Windows Server 2008 R2 and older systems. + +.DESCRIPTION + This agent is designed for systems that cannot run the modern Rust-based agent. + + SECURITY WARNING: The -AllowInsecureTLS switch disables certificate validation, + making the connection vulnerable to man-in-the-middle attacks. Only use this + option on isolated networks or when using self-signed certificates that cannot + be properly installed in the system certificate store. + +.PARAMETER AllowInsecureTLS + [SECURITY RISK] Disables SSL/TLS certificate validation. Required for systems + with self-signed certificates or broken certificate chains. This flag must be + explicitly provided - it is not enabled by default. + +.EXAMPLE + # Secure installation (recommended) + .\GuruRMM-Agent.ps1 -Register -ServerUrl "https://rmm.example.com" -SiteCode "ACME-CORP-1234" + + # Insecure installation (legacy systems only) + .\GuruRMM-Agent.ps1 -Register -ServerUrl "https://rmm.example.com" -SiteCode "ACME-CORP-1234" -AllowInsecureTLS +#> +``` + +--- + +## Phase 2: Major Issues (Should Fix) + +### 2.1 Server: Add Missing Database Index + +**File:** `server/migrations/005_add_missing_indexes.sql` (new file) + +```sql +-- Migration: Add missing indexes for performance +-- This migration adds indexes that were identified during code review + +-- Index for agent API key lookups (authentication) +CREATE INDEX IF NOT EXISTS idx_agents_api_key_hash ON agents(api_key_hash); + +-- Index for site API key lookups +CREATE INDEX IF NOT EXISTS idx_sites_api_key_hash ON sites(api_key_hash); + +-- Index for command status queries +CREATE INDEX IF NOT EXISTS idx_commands_status ON commands(status); + +-- Index for metrics time-range queries +CREATE INDEX IF NOT EXISTS idx_metrics_agent_timestamp ON metrics(agent_id, timestamp DESC); + +-- Index for finding online agents +CREATE INDEX IF NOT EXISTS idx_agents_status_last_seen ON agents(status, last_seen DESC); +``` + +--- + +### 2.2 Server: Fix WebSocket Ping/Pong Protocol + +**File:** `server/src/ws/mod.rs` +**Lines:** 379-386 + +**Current (incorrect):** +```rust +Ok(Message::Ping(data)) => { + if tx + .send(ServerMessage::Ack { message_id: None }) + .await + .is_err() + { + break; + } +} +``` + +**Fixed:** +```rust +Ok(Message::Ping(data)) => { + // WebSocket protocol requires Pong response to Ping + if ws_sender + .send(Message::Pong(data)) + .await + .is_err() + { + warn!("Failed to send Pong response"); + break; + } +} +``` + +--- + +### 2.3 Agent: Fix Service Path Construction + +**File:** `agent/src/service.rs` +**Line:** 93 + +**Current:** +```rust +let config_path = PathBuf::from(format!(r"{}\\agent.toml", CONFIG_DIR)); +``` + +**Fixed:** +```rust +let config_path = PathBuf::from(CONFIG_DIR).join("agent.toml"); +``` + +--- + +### 2.4 Agent: Fix Rollback Script Backgrounding + +**File:** `agent/src/updater/mod.rs` +**Lines:** 321-327 + +**Current (broken):** +```rust +tokio::process::Command::new("nohup") + .arg("bash") + .arg(&script_path) + .arg("&") // This doesn't work - & is passed as literal argument + .spawn() +``` + +**Fixed:** +```rust +// Use setsid to create new session and properly detach +tokio::process::Command::new("setsid") + .arg("bash") + .arg(&script_path) + .stdin(std::process::Stdio::null()) + .stdout(std::process::Stdio::null()) + .stderr(std::process::Stdio::null()) + .spawn() + .context("Failed to spawn rollback script")?; + +// Alternative using nohup correctly: +tokio::process::Command::new("bash") + .arg("-c") + .arg(format!("nohup {} /dev/null 2>&1 &", script_path.display())) + .spawn() + .context("Failed to spawn rollback script")?; +``` + +--- + +### 2.5 Agent: Replace todo!() with Proper Error + +**File:** `agent/src/main.rs` +**Lines:** 227-229 + +**Current:** +```rust +#[cfg(target_os = "macos")] +{ + todo!("macOS launchd service installation not yet implemented"); +} +``` + +**Fixed:** +```rust +#[cfg(target_os = "macos")] +{ + return Err(anyhow::anyhow!( + "macOS launchd service installation is not yet implemented. \ + Please run the agent manually or create a launchd plist manually. \ + See: https://developer.apple.com/library/archive/documentation/MacOSX/Conceptual/BPSystemStartup/Chapters/CreatingLaunchdJobs.html" + )); +} +``` + +--- + +### 2.6 Dashboard: Fix Duplicate Filter Options + +**File:** `dashboard/src/pages/Agents.tsx` +**Lines:** 175-176 + +**Current:** +```tsx + + +``` + +**Fixed:** +```tsx + + +``` + +**Update filtering logic:** +```typescript +const filteredAgents = useMemo(() => { + let result = agents || []; + + if (clientFilter === '__unassigned__') { + result = result.filter(a => !a.site_id && !a.client_id); + } else if (clientFilter) { + result = result.filter(a => a.client_id === clientFilter); + } + + if (searchTerm) { + const term = searchTerm.toLowerCase(); + result = result.filter(a => + a.hostname.toLowerCase().includes(term) || + a.os_type?.toLowerCase().includes(term) + ); + } + + return result; +}, [agents, clientFilter, searchTerm]); +``` + +--- + +### 2.7 Dashboard: Add Error Handlers to Mutations + +**File:** `dashboard/src/pages/Agents.tsx` +**Lines:** 98-114 + +**Current:** +```typescript +const deleteMutation = useMutation({ + mutationFn: agentsApi.delete, + onSuccess: () => { + queryClient.invalidateQueries({ queryKey: ['agents'] }); + setDeleteConfirm(null); + }, +}); +``` + +**Fixed:** +```typescript +const deleteMutation = useMutation({ + mutationFn: agentsApi.delete, + onSuccess: () => { + queryClient.invalidateQueries({ queryKey: ['agents'] }); + setDeleteConfirm(null); + // Optional: Show success toast + }, + onError: (error: AxiosError<{ error: string }>) => { + const message = error.response?.data?.error || error.message || 'Failed to delete agent'; + // Show error to user + alert(`[ERROR] ${message}`); + // Or use a toast notification library + }, +}); + +const moveMutation = useMutation({ + mutationFn: ({ agentId, siteId }: { agentId: string; siteId: string | null }) => + agentsApi.move(agentId, siteId), + onSuccess: () => { + queryClient.invalidateQueries({ queryKey: ['agents'] }); + setMoveAgent(null); + }, + onError: (error: AxiosError<{ error: string }>) => { + const message = error.response?.data?.error || error.message || 'Failed to move agent'; + alert(`[ERROR] ${message}`); + }, +}); +``` + +--- + +### 2.8 Dashboard: Replace `any` Types + +**File:** `dashboard/src/pages/Login.tsx` +**Line:** 24 + +**Current:** +```typescript +} catch (err: any) { + setError(err.response?.data?.error || 'Login failed'); +} +``` + +**Fixed:** +```typescript +import { AxiosError } from 'axios'; + +interface ApiError { + error: string; +} + +// In the component: +} catch (err) { + if (err instanceof AxiosError) { + const apiError = err.response?.data as ApiError | undefined; + setError(apiError?.error || err.message || 'Login failed'); + } else if (err instanceof Error) { + setError(err.message); + } else { + setError('An unexpected error occurred'); + } +} +``` + +**Apply same pattern to Register.tsx line 37.** + +--- + +## Phase 3: Warning Level Issues (Recommended) + +### 3.1 Add Request Logging/Audit Trail + +**File:** `server/src/main.rs` + +```rust +use tower_http::trace::{TraceLayer, DefaultMakeSpan, DefaultOnResponse}; +use tracing::Level; + +// Add to router +let app = Router::new() + // ... routes ... + .layer( + TraceLayer::new_for_http() + .make_span_with(DefaultMakeSpan::new().level(Level::INFO)) + .on_response(DefaultOnResponse::new().level(Level::INFO)) + ); +``` + +--- + +### 3.2 Add Health Check with Database Verification + +**File:** `server/src/main.rs` + +**Current:** +```rust +async fn health_check() -> &'static str { + "OK" +} +``` + +**Fixed:** +```rust +use serde::Serialize; + +#[derive(Serialize)] +struct HealthResponse { + status: &'static str, + database: &'static str, + version: &'static str, +} + +async fn health_check(State(state): State) -> Json { + let db_status = match sqlx::query("SELECT 1") + .fetch_one(&state.db) + .await + { + Ok(_) => "healthy", + Err(_) => "unhealthy", + }; + + Json(HealthResponse { + status: if db_status == "healthy" { "ok" } else { "degraded" }, + database: db_status, + version: env!("CARGO_PKG_VERSION"), + }) +} +``` + +--- + +### 3.3 Add Graceful Shutdown + +**File:** `server/src/main.rs` + +```rust +use tokio::signal; + +async fn shutdown_signal() { + let ctrl_c = async { + signal::ctrl_c() + .await + .expect("Failed to install Ctrl+C handler"); + }; + + #[cfg(unix)] + let terminate = async { + signal::unix::signal(signal::unix::SignalKind::terminate()) + .expect("Failed to install signal handler") + .recv() + .await; + }; + + #[cfg(not(unix))] + let terminate = std::future::pending::<()>(); + + tokio::select! { + _ = ctrl_c => {}, + _ = terminate => {}, + } + + tracing::info!("Shutdown signal received, starting graceful shutdown"); +} + +// In main(): +let listener = tokio::net::TcpListener::bind(&addr).await?; + +axum::serve(listener, app) + .with_graceful_shutdown(shutdown_signal()) + .await?; +``` + +--- + +## Implementation Checklist + +### Critical (Do First) +- [ ] 1.1 Restrict CORS configuration +- [ ] 1.2 Add AuthUser to all endpoints +- [ ] 1.3 Add rate limiting to registration +- [ ] 1.4 Remove Unicode characters from agent output +- [ ] 1.5 Add certificate pinning for updates +- [ ] 1.6 Fix insecure temp file creation +- [ ] 1.7 Improve dashboard token storage +- [ ] 1.8 Add command input validation +- [ ] 1.9 Document/mitigate legacy agent cert bypass + +### Major (Do Second) +- [ ] 2.1 Add missing database indexes +- [ ] 2.2 Fix WebSocket ping/pong +- [ ] 2.3 Fix service path construction +- [ ] 2.4 Fix rollback script backgrounding +- [ ] 2.5 Replace todo!() with proper errors +- [ ] 2.6 Fix duplicate filter options +- [ ] 2.7 Add error handlers to mutations +- [ ] 2.8 Replace `any` types with proper types + +### Warnings (Do Third) +- [ ] 3.1 Add request logging/audit trail +- [ ] 3.2 Add comprehensive health check +- [ ] 3.3 Add graceful shutdown handling +- [ ] Add pagination to commands endpoint +- [ ] Add input validation to all request types +- [ ] Implement token expiration handling in dashboard +- [ ] Add dark mode toggle (CSS already exists) +- [ ] Add log rotation to legacy agent + +--- + +## Testing Requirements + +After implementing fixes: + +1. **Authentication Tests** + - Verify all protected endpoints return 401 without token + - Verify all protected endpoints work with valid token + - Verify rate limiting on registration endpoints + +2. **Security Tests** + - Verify CORS blocks unauthorized origins + - Verify command validation catches dangerous patterns + - Verify update downloads only from allowed domains + +3. **Functional Tests** + - Verify agent registration still works + - Verify command execution still works + - Verify metrics collection still works + - Verify WebSocket reconnection works + +4. **Legacy Agent Tests** + - Verify agent works without -AllowInsecureTLS (with valid cert) + - Verify agent works with -AllowInsecureTLS (with self-signed) + - Verify Windows Event Log entries are created + +--- + +## Estimated Effort + +| Phase | Items | Estimated Time | +|-------|-------|----------------| +| Phase 1 (Critical) | 9 items | 8-12 hours | +| Phase 2 (Major) | 8 items | 4-6 hours | +| Phase 3 (Warnings) | 8+ items | 4-8 hours | +| Testing | All | 4-6 hours | +| **Total** | | **20-32 hours** | + +--- + +**Document Version:** 1.0 +**Created:** 2026-01-20 +**Author:** Claude Code Review System diff --git a/projects/msp-tools/guru-rmm/server/Cargo.toml b/projects/msp-tools/guru-rmm/server/Cargo.toml index 944b088..9bb9bc1 100644 --- a/projects/msp-tools/guru-rmm/server/Cargo.toml +++ b/projects/msp-tools/guru-rmm/server/Cargo.toml @@ -11,6 +11,7 @@ axum = { version = "0.7", features = ["ws", "macros"] } axum-extra = { version = "0.9", features = ["typed-header"] } tower = { version = "0.5", features = ["util", "timeout"] } tower-http = { version = "0.6", features = ["cors", "trace", "compression-gzip"] } +http = "1" # Async runtime tokio = { version = "1", features = ["full"] } diff --git a/projects/msp-tools/guru-rmm/server/src/api/agents.rs b/projects/msp-tools/guru-rmm/server/src/api/agents.rs index b029cd4..4cb493d 100644 --- a/projects/msp-tools/guru-rmm/server/src/api/agents.rs +++ b/projects/msp-tools/guru-rmm/server/src/api/agents.rs @@ -8,6 +8,7 @@ use axum::{ use serde::{Deserialize, Serialize}; use uuid::Uuid; +use crate::auth::AuthUser; use crate::db::{self, AgentResponse, AgentStats}; use crate::ws::{generate_api_key, hash_api_key}; use crate::AppState; @@ -29,10 +30,20 @@ pub struct RegisterAgentRequest { } /// Register a new agent (generates API key) +/// Requires authentication to prevent unauthorized agent registration. pub async fn register_agent( State(state): State, + user: AuthUser, Json(req): Json, ) -> Result, (StatusCode, String)> { + // Log who is registering the agent + tracing::info!( + user_id = %user.user_id, + hostname = %req.hostname, + os_type = %req.os_type, + "Agent registration initiated by user" + ); + // Generate a new API key let api_key = generate_api_key(&state.config.auth.api_key_prefix); let api_key_hash = hash_api_key(&api_key); @@ -50,6 +61,12 @@ pub async fn register_agent( .await .map_err(|e| (StatusCode::INTERNAL_SERVER_ERROR, e.to_string()))?; + tracing::info!( + user_id = %user.user_id, + agent_id = %agent.id, + "Agent registered successfully" + ); + Ok(Json(RegisterAgentResponse { agent_id: agent.id, api_key, // Return the plain API key (only shown once!) @@ -59,8 +76,10 @@ pub async fn register_agent( } /// List all agents +/// Requires authentication. pub async fn list_agents( State(state): State, + _user: AuthUser, ) -> Result>, (StatusCode, String)> { let agents = db::get_all_agents(&state.db) .await @@ -71,8 +90,10 @@ pub async fn list_agents( } /// Get a specific agent +/// Requires authentication. pub async fn get_agent( State(state): State, + _user: AuthUser, Path(id): Path, ) -> Result, (StatusCode, String)> { let agent = db::get_agent_by_id(&state.db, id) @@ -84,8 +105,10 @@ pub async fn get_agent( } /// Delete an agent +/// Requires authentication. pub async fn delete_agent( State(state): State, + _user: AuthUser, Path(id): Path, ) -> Result { // Check if agent is connected and disconnect it @@ -106,8 +129,10 @@ pub async fn delete_agent( } /// Get agent statistics +/// Requires authentication. pub async fn get_stats( State(state): State, + _user: AuthUser, ) -> Result, (StatusCode, String)> { let stats = db::get_agent_stats(&state.db) .await @@ -123,8 +148,10 @@ pub struct MoveAgentRequest { } /// Move an agent to a different site +/// Requires authentication. pub async fn move_agent( State(state): State, + _user: AuthUser, Path(id): Path, Json(req): Json, ) -> Result, (StatusCode, String)> { @@ -149,8 +176,10 @@ pub async fn move_agent( } /// List all agents with full details (site/client info) +/// Requires authentication. pub async fn list_agents_with_details( State(state): State, + _user: AuthUser, ) -> Result>, (StatusCode, String)> { let agents = db::get_all_agents_with_details(&state.db) .await @@ -160,8 +189,10 @@ pub async fn list_agents_with_details( } /// List unassigned agents (not belonging to any site) +/// Requires authentication. pub async fn list_unassigned_agents( State(state): State, + _user: AuthUser, ) -> Result>, (StatusCode, String)> { let agents = db::get_unassigned_agents(&state.db) .await @@ -172,8 +203,10 @@ pub async fn list_unassigned_agents( } /// Get extended state for an agent (network interfaces, uptime, etc.) +/// Requires authentication. pub async fn get_agent_state( State(state): State, + _user: AuthUser, Path(id): Path, ) -> Result, (StatusCode, String)> { let agent_state = db::get_agent_state(&state.db, id) diff --git a/projects/msp-tools/guru-rmm/server/src/api/commands.rs b/projects/msp-tools/guru-rmm/server/src/api/commands.rs index 6074198..b4aa7da 100644 --- a/projects/msp-tools/guru-rmm/server/src/api/commands.rs +++ b/projects/msp-tools/guru-rmm/server/src/api/commands.rs @@ -8,6 +8,7 @@ use axum::{ use serde::{Deserialize, Serialize}; use uuid::Uuid; +use crate::auth::AuthUser; use crate::db::{self, Command}; use crate::ws::{CommandPayload, ServerMessage}; use crate::AppState; @@ -43,23 +44,33 @@ pub struct CommandsQuery { } /// Send a command to an agent +/// Requires authentication. Logs the user who sent the command for audit trail. pub async fn send_command( State(state): State, + user: AuthUser, Path(agent_id): Path, Json(req): Json, ) -> Result, (StatusCode, String)> { + // Log the command being sent for audit trail + tracing::info!( + user_id = %user.user_id, + agent_id = %agent_id, + command_type = %req.command_type, + "Command sent by user" + ); + // Verify agent exists - let agent = db::get_agent_by_id(&state.db, agent_id) + let _agent = db::get_agent_by_id(&state.db, agent_id) .await .map_err(|e| (StatusCode::INTERNAL_SERVER_ERROR, e.to_string()))? .ok_or((StatusCode::NOT_FOUND, "Agent not found".to_string()))?; - // Create command record + // Create command record with user ID for audit trail let create = db::CreateCommand { agent_id, command_type: req.command_type.clone(), command_text: req.command.clone(), - created_by: None, // TODO: Get from JWT + created_by: Some(user.user_id), }; let command = db::create_command(&state.db, create) @@ -100,8 +111,10 @@ pub async fn send_command( } /// List recent commands +/// Requires authentication. pub async fn list_commands( State(state): State, + _user: AuthUser, Query(query): Query, ) -> Result>, (StatusCode, String)> { let limit = query.limit.unwrap_or(50).min(500); @@ -114,8 +127,10 @@ pub async fn list_commands( } /// Get a specific command by ID +/// Requires authentication. pub async fn get_command( State(state): State, + _user: AuthUser, Path(id): Path, ) -> Result, (StatusCode, String)> { let command = db::get_command_by_id(&state.db, id) diff --git a/projects/msp-tools/guru-rmm/server/src/api/metrics.rs b/projects/msp-tools/guru-rmm/server/src/api/metrics.rs index 63d0c6f..deb3566 100644 --- a/projects/msp-tools/guru-rmm/server/src/api/metrics.rs +++ b/projects/msp-tools/guru-rmm/server/src/api/metrics.rs @@ -5,10 +5,11 @@ use axum::{ http::StatusCode, Json, }; -use chrono::{DateTime, Duration, Utc}; +use chrono::{DateTime, Utc}; use serde::Deserialize; use uuid::Uuid; +use crate::auth::AuthUser; use crate::db::{self, Metrics, MetricsSummary}; use crate::AppState; @@ -26,13 +27,15 @@ pub struct MetricsQuery { } /// Get metrics for a specific agent +/// Requires authentication. pub async fn get_agent_metrics( State(state): State, + _user: AuthUser, Path(id): Path, Query(query): Query, ) -> Result>, (StatusCode, String)> { // First verify the agent exists - let agent = db::get_agent_by_id(&state.db, id) + let _agent = db::get_agent_by_id(&state.db, id) .await .map_err(|e| (StatusCode::INTERNAL_SERVER_ERROR, e.to_string()))? .ok_or((StatusCode::NOT_FOUND, "Agent not found".to_string()))?; @@ -54,8 +57,10 @@ pub async fn get_agent_metrics( } /// Get summary metrics across all agents +/// Requires authentication. pub async fn get_summary( State(state): State, + _user: AuthUser, ) -> Result, (StatusCode, String)> { let summary = db::get_metrics_summary(&state.db) .await diff --git a/projects/msp-tools/guru-rmm/server/src/main.rs b/projects/msp-tools/guru-rmm/server/src/main.rs index bdda54d..32e3e98 100644 --- a/projects/msp-tools/guru-rmm/server/src/main.rs +++ b/projects/msp-tools/guru-rmm/server/src/main.rs @@ -24,7 +24,8 @@ use axum::{ }; use sqlx::postgres::PgPoolOptions; use tokio::sync::RwLock; -use tower_http::cors::{Any, CorsLayer}; +use http::HeaderValue; +use tower_http::cors::{AllowOrigin, CorsLayer}; use tower_http::trace::TraceLayer; use tracing::info; @@ -129,11 +130,34 @@ async fn main() -> Result<()> { /// Build the application router fn build_router(state: AppState) -> Router { - // CORS configuration (allow dashboard access) + // TODO: Add rate limiting for registration endpoints using tower-governor + // Currently, registration is protected by AuthUser authentication. + // For additional protection against brute-force attacks, consider adding: + // - tower-governor crate for per-IP rate limiting on /api/agents/register + // - Configurable limits via environment variables + // Reference: https://docs.rs/tower-governor/latest/tower_governor/ + + // CORS configuration - restrict to specific dashboard origin + let dashboard_origin = std::env::var("DASHBOARD_URL") + .unwrap_or_else(|_| "https://rmm.azcomputerguru.com".to_string()); + let cors = CorsLayer::new() - .allow_origin(Any) - .allow_methods(Any) - .allow_headers(Any); + .allow_origin(AllowOrigin::exact( + HeaderValue::from_str(&dashboard_origin).expect("Invalid DASHBOARD_URL"), + )) + .allow_methods([ + http::Method::GET, + http::Method::POST, + http::Method::PUT, + http::Method::DELETE, + http::Method::OPTIONS, + ]) + .allow_headers([ + http::header::AUTHORIZATION, + http::header::CONTENT_TYPE, + http::header::ACCEPT, + ]) + .allow_credentials(true); Router::new() // Health check