From b8fb4e7f4a4bb843cf5fda95cad18e9fb549bdb5 Mon Sep 17 00:00:00 2001 From: kaffa Date: Sun, 1 Feb 2026 13:50:47 +0000 Subject: [PATCH] fix: Add security and reliability improvements 1. File locking for config operations - add_server_to_config() and remove_server_from_config() now use exclusive file locking (fcntl.LOCK_EX) to prevent race conditions - Prevents data loss from concurrent modifications 2. Bulk server limit - Add MAX_BULK_SERVERS = 10 constant - haproxy_add_servers() now rejects requests exceeding limit - Prevents potential DoS via large payloads 3. HAProxy command response validation - Add haproxy_cmd_checked() helper function - Validates responses for error indicators (No such, not found, etc.) - State-modifying commands now properly detect and report failures - Read-only commands continue using haproxy_cmd() Co-Authored-By: Claude Opus 4.5 --- mcp/server.py | 88 ++++++++++++++++++++++++++++++++++++--------------- 1 file changed, 62 insertions(+), 26 deletions(-) diff --git a/mcp/server.py b/mcp/server.py index e1f96d4..c3f1362 100644 --- a/mcp/server.py +++ b/mcp/server.py @@ -82,6 +82,7 @@ STARTUP_RETRY_COUNT = 10 # HAProxy ready check retries STATE_MIN_COLUMNS = 19 # Minimum columns in HAProxy server state output SOCKET_TIMEOUT = 5 # seconds for HAProxy socket connection SOCKET_RECV_TIMEOUT = 30 # seconds for HAProxy socket recv loop +MAX_BULK_SERVERS = 10 # Max servers per bulk add call class HaproxyError(Exception): @@ -169,6 +170,29 @@ def haproxy_cmd(command: str) -> str: raise HaproxyError(str(e)) from e +def haproxy_cmd_checked(command: str) -> str: + """Send command to HAProxy and raise on error response. + + Args: + command: HAProxy command to execute + + Returns: + Command response + + Raises: + HaproxyError: If HAProxy returns an error message + """ + result = haproxy_cmd(command) + # HAProxy returns empty string on success, error messages on failure + error_indicators = ["No such", "not found", "error", "failed", "invalid", "unknown"] + if result: + result_lower = result.lower() + for indicator in error_indicators: + if indicator.lower() in result_lower: + raise HaproxyError(f"HAProxy command failed: {result.strip()}") + return result + + def reload_haproxy() -> Tuple[bool, str]: """Validate and reload HAProxy configuration. @@ -485,7 +509,7 @@ def save_servers_config(config: Dict[str, Any]) -> None: def add_server_to_config(domain: str, slot: int, ip: str, http_port: int) -> None: - """Add server configuration to persistent storage. + """Add server configuration to persistent storage with file locking. Args: domain: Domain name @@ -493,29 +517,38 @@ def add_server_to_config(domain: str, slot: int, ip: str, http_port: int) -> Non ip: Server IP address http_port: HTTP port """ - config = load_servers_config() - if domain not in config: - config[domain] = {} - config[domain][str(slot)] = { - "ip": ip, - "http_port": http_port - } - save_servers_config(config) + lock_path = f"{SERVERS_FILE}.lock" + with open(lock_path, 'w') as lock_file: + fcntl.flock(lock_file.fileno(), fcntl.LOCK_EX) + try: + config = load_servers_config() + if domain not in config: + config[domain] = {} + config[domain][str(slot)] = {"ip": ip, "http_port": http_port} + save_servers_config(config) + finally: + fcntl.flock(lock_file.fileno(), fcntl.LOCK_UN) def remove_server_from_config(domain: str, slot: int) -> None: - """Remove server configuration from persistent storage. + """Remove server configuration from persistent storage with file locking. Args: domain: Domain name slot: Server slot to remove """ - config = load_servers_config() - if domain in config and str(slot) in config[domain]: - del config[domain][str(slot)] - if not config[domain]: - del config[domain] - save_servers_config(config) + lock_path = f"{SERVERS_FILE}.lock" + with open(lock_path, 'w') as lock_file: + fcntl.flock(lock_file.fileno(), fcntl.LOCK_EX) + try: + config = load_servers_config() + if domain in config and str(slot) in config[domain]: + del config[domain][str(slot)] + if not config[domain]: + del config[domain] + save_servers_config(config) + finally: + fcntl.flock(lock_file.fileno(), fcntl.LOCK_UN) def remove_domain_from_config(domain: str) -> None: @@ -891,8 +924,8 @@ def haproxy_add_server(domain: str, slot: int, ip: str, http_port: int = 80) -> results = [] for suffix, port in get_server_suffixes(http_port): server = f"{server_prefix}{suffix}_{slot}" - haproxy_cmd(f"set server {backend}/{server} addr {ip} port {port}") - haproxy_cmd(f"set server {backend}/{server} state ready") + haproxy_cmd_checked(f"set server {backend}/{server} addr {ip} port {port}") + haproxy_cmd_checked(f"set server {backend}/{server} state ready") results.append(f"{server} → {ip}:{port}") # Save to persistent config @@ -942,6 +975,9 @@ def haproxy_add_servers(domain: str, servers: str) -> str: if not server_list: return "Error: servers array is empty" + if len(server_list) > MAX_BULK_SERVERS: + return f"Error: Cannot add more than {MAX_BULK_SERVERS} servers at once" + # Validate all servers first before adding any validated_servers = [] validation_errors = [] @@ -1014,8 +1050,8 @@ def haproxy_add_servers(domain: str, servers: str) -> str: try: for suffix, port in get_server_suffixes(http_port): server = f"{server_prefix}{suffix}_{slot}" - haproxy_cmd(f"set server {backend}/{server} addr {ip} port {port}") - haproxy_cmd(f"set server {backend}/{server} state ready") + haproxy_cmd_checked(f"set server {backend}/{server} addr {ip} port {port}") + haproxy_cmd_checked(f"set server {backend}/{server} state ready") # Save to persistent config add_server_to_config(domain, slot, ip, http_port) @@ -1063,8 +1099,8 @@ def haproxy_remove_server(domain: str, slot: int) -> str: # HTTP only - single server per slot server = f"{server_prefix}_{slot}" - haproxy_cmd(f"set server {backend}/{server} state maint") - haproxy_cmd(f"set server {backend}/{server} addr 0.0.0.0 port 0") + haproxy_cmd_checked(f"set server {backend}/{server} state maint") + haproxy_cmd_checked(f"set server {backend}/{server} addr 0.0.0.0 port 0") # Remove from persistent config remove_server_from_config(domain, slot) @@ -1323,8 +1359,8 @@ def haproxy_set_server_state(backend: str, server: str, state: str) -> str: if state not in ["ready", "drain", "maint"]: return "Error: state must be 'ready', 'drain', or 'maint'" try: - result = haproxy_cmd(f"set server {backend}/{server} state {state}") - return result if result else f"Server {backend}/{server} set to {state}" + haproxy_cmd_checked(f"set server {backend}/{server} state {state}") + return f"Server {backend}/{server} set to {state}" except HaproxyError as e: return f"Error: {e}" @@ -1392,8 +1428,8 @@ def haproxy_set_server_weight(backend: str, server: str, weight: int) -> str: if not (0 <= weight <= 256): return "Error: weight must be between 0 and 256" try: - result = haproxy_cmd(f"set server {backend}/{server} weight {weight}") - return result if result else f"Server {backend}/{server} weight set to {weight}" + haproxy_cmd_checked(f"set server {backend}/{server} weight {weight}") + return f"Server {backend}/{server} weight set to {weight}" except HaproxyError as e: return f"Error: {e}"