fix(security): authenticate VNC/terminal WebSockets and harden SSH/config
Some checks failed
Backend Tests / pytest (Python 3.12) (push) Failing after 46s
Some checks failed
Backend Tests / pytest (Python 3.12) (push) Failing after 46s
Critical security and robustness fixes from a code audit: - WS terminal/VNC now require a JWT (?token=), validate the session, vms.console permission and VM ownership before bridging to Proxmox (previously the LXC terminal gave unauthenticated root shell access). Frontend ConsolePage passes the token in both WS URLs. - Fix SSH fallback for `pvecm delnode`: SSHClient now supports password auth (it previously crashed with TypeError); ProxmoxClient stores the password for SSH fallbacks and the host port suffix is stripped. - Add production startup warnings for default ADMIN_PASSWORD and wildcard CORS_ORIGINS. - dashboard: use tz-aware utcnow() instead of naive datetime.utcnow(). - crypto: narrow decrypt_value except to InvalidToken and log failures. - ssh_client: guard the shared connection cache with a lock; make unknown-host-key rejection opt-in via SSH_REJECT_UNKNOWN_HOSTS. - logging: send loguru console output to stderr instead of stdout. - Sync version fallback to 1.5.1; remove one-off refactor scripts. - Add CODE_REVIEW.md audit report. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
This commit is contained in:
@@ -1,11 +1,12 @@
|
||||
from fastapi import APIRouter, Depends, Request
|
||||
from fastapi.responses import JSONResponse
|
||||
from sqlalchemy.orm import Session
|
||||
from datetime import datetime, timedelta
|
||||
from datetime import timedelta
|
||||
|
||||
from ..db import get_db
|
||||
from ..models import ProxmoxServer, AuditLog, User
|
||||
from ..auth import PermissionChecker
|
||||
from ..config import utcnow
|
||||
|
||||
router = APIRouter()
|
||||
|
||||
@@ -39,7 +40,7 @@ def dashboard_stats(
|
||||
cluster_q = cluster_q.filter(ProxmoxServer.id.in_(server_ids))
|
||||
total_clusters = cluster_q.scalar() or 0
|
||||
|
||||
since_24h = datetime.utcnow() - timedelta(hours=24)
|
||||
since_24h = utcnow() - timedelta(hours=24)
|
||||
total_alerts = db.query(AuditLog).filter(
|
||||
AuditLog.level.in_(['error', 'critical']),
|
||||
AuditLog.created_at >= since_24h
|
||||
@@ -88,7 +89,7 @@ def get_dashboard_alerts(
|
||||
current_user: User = Depends(PermissionChecker("logs.view"))
|
||||
):
|
||||
"""Вернуть события error/critical за последние 24 ч для дашборда"""
|
||||
since_24h = datetime.utcnow() - timedelta(hours=24)
|
||||
since_24h = utcnow() - timedelta(hours=24)
|
||||
alerts = db.query(AuditLog).filter(
|
||||
AuditLog.level.in_(['error', 'critical']),
|
||||
AuditLog.created_at >= since_24h
|
||||
|
||||
@@ -13,7 +13,9 @@ from ...db import get_db
|
||||
from ...models import ProxmoxServer, VMInstance, User, IPAMAllocation, IPAMNetwork, VMSnapshotArchive
|
||||
from ...schemas import ProxmoxServerCreate, ProxmoxServerUpdate, ProxmoxServerResponse
|
||||
from ...proxmox import ProxmoxClient, get_proxmox_resources
|
||||
from ...auth import get_current_user, PermissionChecker, require_permission, check_permission
|
||||
from ...auth import (get_current_user, PermissionChecker, require_permission,
|
||||
check_permission, authenticate_ws_token)
|
||||
from ...rbac import PermissionEngine
|
||||
from ...logging_service import LoggingService
|
||||
from ...ipam_service import IPAMService
|
||||
from ._helpers import (check_vm_access, require_vm_access, _get_proxmox_client,
|
||||
@@ -2001,15 +2003,28 @@ async def vnc_websocket_proxy(
|
||||
vncticket: str,
|
||||
vnc_password: str = None, # VNC пароль сгенерированный через generate-password
|
||||
auth_ticket: str = None, # Auth ticket переданный с frontend
|
||||
token: str = Query(None), # JWT панели (?token=) — браузеры не шлют заголовки на WS
|
||||
db: Session = Depends(get_db)
|
||||
):
|
||||
"""WebSocket прокси для VNC подключения к Proxmox"""
|
||||
import websockets
|
||||
import urllib.parse
|
||||
|
||||
|
||||
# --- Authenticate the panel user before bridging to Proxmox ------------
|
||||
user = authenticate_ws_token(token, db)
|
||||
if not user:
|
||||
await websocket.close(code=4001, reason="Authentication required")
|
||||
return
|
||||
if not PermissionEngine.has_permission(user, "vms.console"):
|
||||
await websocket.close(code=4003, reason="Permission denied")
|
||||
return
|
||||
if not check_vm_access(db, user, server_id, vmid):
|
||||
await websocket.close(code=4003, reason="Access denied")
|
||||
return
|
||||
|
||||
await websocket.accept()
|
||||
logger.info(f"VNC WebSocket connection accepted for {vmtype}/{vmid}")
|
||||
|
||||
logger.info(f"VNC WebSocket connection accepted for {vmtype}/{vmid} (user={user.username})")
|
||||
|
||||
server = db.query(ProxmoxServer).filter(ProxmoxServer.id == server_id).first()
|
||||
if not server:
|
||||
logger.error(f"Proxmox server {server_id} not found")
|
||||
@@ -2954,6 +2969,7 @@ async def terminal_websocket_fixed(
|
||||
server_id: int,
|
||||
node: str,
|
||||
vmid: int,
|
||||
token: str = Query(None), # JWT панели (?token=) — браузеры не шлют заголовки на WS
|
||||
db: Session = Depends(get_db)
|
||||
):
|
||||
"""WebSocket терминал для LXC контейнеров через Proxmox termproxy API"""
|
||||
@@ -2961,8 +2977,20 @@ async def terminal_websocket_fixed(
|
||||
import httpx
|
||||
from urllib.parse import quote
|
||||
|
||||
# --- Authenticate the panel user before opening a root shell -----------
|
||||
user = authenticate_ws_token(token, db)
|
||||
if not user:
|
||||
await websocket.close(code=4001, reason="Authentication required")
|
||||
return
|
||||
if not PermissionEngine.has_permission(user, "vms.console"):
|
||||
await websocket.close(code=4003, reason="Permission denied")
|
||||
return
|
||||
if not check_vm_access(db, user, server_id, vmid):
|
||||
await websocket.close(code=4003, reason="Access denied")
|
||||
return
|
||||
|
||||
await websocket.accept()
|
||||
logger.info(f"Terminal WebSocket accepted for container {vmid} on node {node}")
|
||||
logger.info(f"Terminal WebSocket accepted for container {vmid} on node {node} (user={user.username})")
|
||||
|
||||
server = db.query(ProxmoxServer).filter(ProxmoxServer.id == server_id).first()
|
||||
if not server:
|
||||
|
||||
@@ -135,6 +135,42 @@ def get_current_session_token(
|
||||
return payload.get("session")
|
||||
|
||||
|
||||
def authenticate_ws_token(token: Optional[str], db: Session) -> Optional[User]:
|
||||
"""Authenticate a WebSocket connection from a JWT passed as a query parameter.
|
||||
|
||||
Browsers cannot set Authorization headers on WebSocket handshakes, so the
|
||||
token is supplied via ?token=<jwt>. Returns the authenticated, active User
|
||||
(with a valid session, if the token is session-bound) or ``None`` on any
|
||||
failure. Unlike ``decode_access_token`` this never raises — callers close
|
||||
the socket with an appropriate code instead.
|
||||
"""
|
||||
if not token:
|
||||
return None
|
||||
try:
|
||||
payload = jwt.decode(token, settings.SECRET_KEY, algorithms=[settings.ALGORITHM])
|
||||
except jwt.PyJWTError:
|
||||
return None
|
||||
|
||||
username = payload.get("sub")
|
||||
if not username:
|
||||
return None
|
||||
|
||||
# Validate the bound session, mirroring get_current_user.
|
||||
session_token = payload.get("session")
|
||||
if session_token:
|
||||
from .services.security_service import SecurityService
|
||||
if not SecurityService.validate_session(db, session_token):
|
||||
return None
|
||||
|
||||
user = db.query(User).options(joinedload(User.role)).filter(User.username == username).first()
|
||||
if user is None or not user.is_active:
|
||||
return None
|
||||
if user.is_locked():
|
||||
return None
|
||||
|
||||
return user
|
||||
|
||||
|
||||
def get_current_active_admin(current_user: User = Depends(get_current_user)) -> User:
|
||||
"""Get current active admin user"""
|
||||
if not current_user.is_admin and (not current_user.role or current_user.role.name != "admin"):
|
||||
|
||||
@@ -16,7 +16,8 @@ def get_version() -> str:
|
||||
for version_file in possible_paths:
|
||||
if version_file.exists():
|
||||
return version_file.read_text().strip()
|
||||
return "1.7.0"
|
||||
# Fallback only if the VERSION file is missing — keep in sync with /VERSION.
|
||||
return "1.5.1"
|
||||
|
||||
|
||||
class Settings(BaseSettings):
|
||||
@@ -46,6 +47,10 @@ class Settings(BaseSettings):
|
||||
SSH_TIMEOUT: int = Field(default=10, env="SSH_TIMEOUT")
|
||||
DEFAULT_SSH_USER: str = Field(default="root", env="DEFAULT_SSH_USER")
|
||||
DEFAULT_SSH_PORT: int = Field(default=22, env="DEFAULT_SSH_PORT")
|
||||
# Reject SSH connections to hosts with unknown/changed host keys (mitigates MITM).
|
||||
# Default False preserves legacy behaviour (warn & accept); enable in production
|
||||
# once your servers' host keys are in known_hosts.
|
||||
SSH_REJECT_UNKNOWN_HOSTS: bool = Field(default=False, env="SSH_REJECT_UNKNOWN_HOSTS")
|
||||
|
||||
# CORS — comma-separated list of allowed origins; "*" is insecure in production
|
||||
CORS_ORIGINS: str = Field(default="*", env="CORS_ORIGINS")
|
||||
|
||||
@@ -56,8 +56,15 @@ def decrypt_value(value: str) -> str:
|
||||
return value
|
||||
try:
|
||||
return f.decrypt(value.encode()).decode()
|
||||
except (InvalidToken, Exception):
|
||||
# Value is likely plaintext (pre-encryption legacy row)
|
||||
except InvalidToken:
|
||||
# Value is likely plaintext (pre-encryption legacy row) or was
|
||||
# encrypted with a different FERNET_KEY. Return as-is for backward
|
||||
# compatibility, but warn so key-rotation mistakes are visible.
|
||||
logger.warning(
|
||||
"Fernet decryption failed (InvalidToken). Value returned as-is — "
|
||||
"this is expected for legacy plaintext rows, but check FERNET_KEY "
|
||||
"if previously-encrypted data is now unreadable."
|
||||
)
|
||||
return value
|
||||
|
||||
|
||||
|
||||
@@ -37,10 +37,11 @@ def setup_logging():
|
||||
|
||||
# Remove default logger
|
||||
logger.remove()
|
||||
|
||||
# Add console logging
|
||||
|
||||
# Add console logging (stderr keeps logs off stdout, which may carry data)
|
||||
import sys
|
||||
logger.add(
|
||||
sink=lambda msg: print(msg, end=""),
|
||||
sink=sys.stderr,
|
||||
level=settings.LOG_LEVEL,
|
||||
format="<green>{time:YYYY-MM-DD HH:mm:ss}</green> | <level>{level: <8}</level> | <cyan>{name}</cyan>:<cyan>{function}</cyan>:<cyan>{line}</cyan> - <level>{message}</level>"
|
||||
)
|
||||
@@ -123,6 +124,19 @@ async def lifespan(app: FastAPI):
|
||||
if not settings.FERNET_KEY:
|
||||
logger.warning("FERNET_KEY is not set. Sensitive fields (like passwords) will be stored as plaintext.")
|
||||
logger.warning("To enable encryption, generate a key and set FERNET_KEY in your .env file.")
|
||||
|
||||
# Production hardening warnings (DEBUG is off in production)
|
||||
if not settings.DEBUG:
|
||||
if settings.ADMIN_PASSWORD == "admin123":
|
||||
logger.warning(
|
||||
"ADMIN_PASSWORD uses the insecure default 'admin123'. "
|
||||
"Set a strong ADMIN_PASSWORD env var for production."
|
||||
)
|
||||
if settings.CORS_ORIGINS.strip() == "*":
|
||||
logger.warning(
|
||||
"CORS_ORIGINS is '*' (any origin). Set an explicit comma-separated "
|
||||
"origin list (e.g. https://panel.example.com) for production."
|
||||
)
|
||||
|
||||
# Check database connection
|
||||
if not check_db_connection():
|
||||
|
||||
@@ -101,6 +101,8 @@ class ProxmoxClient(VmMixin, LxcMixin, ClusterMixin, StorageMixin, NetworkMixin,
|
||||
self.host = host
|
||||
self.user = user
|
||||
self.timeout = timeout
|
||||
# Stored for SSH fallbacks (e.g. pvecm delnode) that need root password auth
|
||||
self._password = password
|
||||
# Улучшенный ключ кеша - включаем token_name для уникальности
|
||||
self.connection_key = f"{host}:{user}:{token_name or 'password'}"
|
||||
self.proxmox = None
|
||||
|
||||
@@ -286,8 +286,10 @@ class ClusterMixin:
|
||||
def _delete_cluster_node_via_ssh(self, node_name: str) -> Dict:
|
||||
"""SSH fallback для удаления ноды через pvecm delnode"""
|
||||
from app.ssh_client import SSHClient
|
||||
# self.host may carry an API port suffix (e.g. "1.2.3.4:8006"); SSH needs the bare host.
|
||||
ssh_host = self.host.split(':')[0] if self.host else self.host
|
||||
ssh = SSHClient(
|
||||
hostname=self.host,
|
||||
hostname=ssh_host,
|
||||
username='root',
|
||||
password=getattr(self, '_password', None),
|
||||
)
|
||||
|
||||
@@ -1,5 +1,6 @@
|
||||
import logging
|
||||
import asyncio
|
||||
import threading
|
||||
from typing import Dict, Optional, Union
|
||||
from concurrent.futures import ThreadPoolExecutor
|
||||
import paramiko
|
||||
@@ -16,17 +17,20 @@ ssh_executor = ThreadPoolExecutor(max_workers=10, thread_name_prefix="ssh_")
|
||||
|
||||
class SSHClient:
|
||||
"""Улучшенный SSH клиент для подключения к серверам с кешированием"""
|
||||
|
||||
# Connection cache
|
||||
|
||||
# Connection cache (shared across threads — guard every access with _cache_lock)
|
||||
_connections = {}
|
||||
_connection_times = {}
|
||||
_cache_lock = threading.Lock()
|
||||
|
||||
def __init__(self, hostname: str, port: int = 22, username: str = "root",
|
||||
key_path: Optional[str] = None, timeout: int = 10, max_retries: int = 3):
|
||||
def __init__(self, hostname: str, port: int = 22, username: str = "root",
|
||||
key_path: Optional[str] = None, timeout: int = 10, max_retries: int = 3,
|
||||
password: Optional[str] = None):
|
||||
self.hostname = hostname
|
||||
self.port = port
|
||||
self.username = username
|
||||
self.key_path = key_path
|
||||
self.password = password
|
||||
self.timeout = timeout
|
||||
self.max_retries = max_retries
|
||||
self.client = None
|
||||
@@ -35,10 +39,12 @@ class SSHClient:
|
||||
|
||||
def _get_cached_connection(self) -> Optional[paramiko.SSHClient]:
|
||||
"""Get cached connection if available and valid"""
|
||||
if self.connection_key in self._connections:
|
||||
client = self._connections[self.connection_key]
|
||||
with self._cache_lock:
|
||||
client = self._connections.get(self.connection_key)
|
||||
if client is None:
|
||||
return None
|
||||
connection_time = self._connection_times.get(self.connection_key, 0)
|
||||
|
||||
|
||||
# Check if connection is still valid (max 5 minutes)
|
||||
if time.time() - connection_time < 300:
|
||||
try:
|
||||
@@ -48,26 +54,31 @@ class SSHClient:
|
||||
return client
|
||||
except Exception:
|
||||
pass
|
||||
|
||||
|
||||
# Remove invalid connection
|
||||
self._cleanup_connection()
|
||||
|
||||
self._cleanup_connection_locked()
|
||||
return None
|
||||
|
||||
|
||||
def _cache_connection(self, client: paramiko.SSHClient):
|
||||
"""Cache successful connection"""
|
||||
self._connections[self.connection_key] = client
|
||||
self._connection_times[self.connection_key] = time.time()
|
||||
|
||||
with self._cache_lock:
|
||||
self._connections[self.connection_key] = client
|
||||
self._connection_times[self.connection_key] = time.time()
|
||||
|
||||
def _cleanup_connection(self):
|
||||
"""Clean up cached connection"""
|
||||
if self.connection_key in self._connections:
|
||||
"""Clean up cached connection (acquires the cache lock)."""
|
||||
with self._cache_lock:
|
||||
self._cleanup_connection_locked()
|
||||
|
||||
def _cleanup_connection_locked(self):
|
||||
"""Clean up cached connection. Caller must hold _cache_lock."""
|
||||
client = self._connections.pop(self.connection_key, None)
|
||||
if client is not None:
|
||||
try:
|
||||
self._connections[self.connection_key].close()
|
||||
client.close()
|
||||
except Exception:
|
||||
pass
|
||||
del self._connections[self.connection_key]
|
||||
del self._connection_times[self.connection_key]
|
||||
self._connection_times.pop(self.connection_key, None)
|
||||
|
||||
def connect(self) -> bool:
|
||||
"""Установить SSH соединение с повторными попытками"""
|
||||
@@ -83,7 +94,17 @@ class SSHClient:
|
||||
try:
|
||||
client = paramiko.SSHClient()
|
||||
client.load_system_host_keys()
|
||||
client.set_missing_host_key_policy(paramiko.WarningPolicy())
|
||||
# Strict host-key checking is opt-in to avoid breaking existing
|
||||
# deployments whose known_hosts isn't populated yet.
|
||||
try:
|
||||
from .config import settings
|
||||
strict = settings.SSH_REJECT_UNKNOWN_HOSTS
|
||||
except Exception:
|
||||
strict = False
|
||||
if strict:
|
||||
client.set_missing_host_key_policy(paramiko.RejectPolicy())
|
||||
else:
|
||||
client.set_missing_host_key_policy(paramiko.WarningPolicy())
|
||||
|
||||
connect_kwargs = {
|
||||
'hostname': self.hostname,
|
||||
@@ -93,7 +114,9 @@ class SSHClient:
|
||||
'banner_timeout': self.timeout,
|
||||
'auth_timeout': self.timeout,
|
||||
}
|
||||
|
||||
if self.password:
|
||||
connect_kwargs['password'] = self.password
|
||||
|
||||
# Try key authentication first
|
||||
if self.key_path:
|
||||
try:
|
||||
@@ -101,7 +124,7 @@ class SSHClient:
|
||||
client.connect(**connect_kwargs, look_for_keys=False, allow_agent=False)
|
||||
except paramiko.AuthenticationException:
|
||||
logger.debug(f"Key auth failed for {self.hostname}, trying other methods")
|
||||
# Remove key and try other methods
|
||||
# Remove key and try other methods (password / agent / default keys)
|
||||
del connect_kwargs['key_filename']
|
||||
client.connect(**connect_kwargs, look_for_keys=True, allow_agent=True)
|
||||
else:
|
||||
@@ -233,13 +256,14 @@ class SSHClient:
|
||||
@classmethod
|
||||
def cleanup_all_connections(cls):
|
||||
"""Clean up all cached connections"""
|
||||
for key in list(cls._connections.keys()):
|
||||
try:
|
||||
cls._connections[key].close()
|
||||
except Exception:
|
||||
pass
|
||||
cls._connections.clear()
|
||||
cls._connection_times.clear()
|
||||
with cls._cache_lock:
|
||||
for key in list(cls._connections.keys()):
|
||||
try:
|
||||
cls._connections[key].close()
|
||||
except Exception:
|
||||
pass
|
||||
cls._connections.clear()
|
||||
cls._connection_times.clear()
|
||||
|
||||
|
||||
def check_server_status(hostname: str, port: int = 22, username: str = "root",
|
||||
@@ -400,21 +424,21 @@ async def cleanup_ssh_connections():
|
||||
try:
|
||||
await asyncio.sleep(300) # Check every 5 minutes
|
||||
current_time = time.time()
|
||||
expired_keys = []
|
||||
|
||||
for key, connection_time in SSHClient._connection_times.items():
|
||||
if current_time - connection_time > 600: # 10 minutes
|
||||
expired_keys.append(key)
|
||||
|
||||
for key in expired_keys:
|
||||
if key in SSHClient._connections:
|
||||
try:
|
||||
SSHClient._connections[key].close()
|
||||
except Exception:
|
||||
pass
|
||||
del SSHClient._connections[key]
|
||||
del SSHClient._connection_times[key]
|
||||
logger.debug(f"Cleaned up expired SSH connection: {key}")
|
||||
|
||||
with SSHClient._cache_lock:
|
||||
expired_keys = [
|
||||
key for key, connection_time in SSHClient._connection_times.items()
|
||||
if current_time - connection_time > 600 # 10 minutes
|
||||
]
|
||||
for key in expired_keys:
|
||||
client = SSHClient._connections.pop(key, None)
|
||||
if client is not None:
|
||||
try:
|
||||
client.close()
|
||||
except Exception:
|
||||
pass
|
||||
SSHClient._connection_times.pop(key, None)
|
||||
logger.debug(f"Cleaned up expired SSH connection: {key}")
|
||||
except Exception as e:
|
||||
logger.error(f"Error in SSH cleanup task: {e}")
|
||||
await asyncio.sleep(60) # Wait a minute on error
|
||||
|
||||
Reference in New Issue
Block a user