From 72bad11129bae9ef5e7619fb491a9b78f16d429d Mon Sep 17 00:00:00 2001 From: twothatit Date: Wed, 18 Feb 2026 21:28:49 +0100 Subject: [PATCH] security: apply four immediate security fixes Fix #1 - SECRET_KEY startup validation (config.py, .env): - App refuses to start if SECRET_KEY is missing, shorter than 32 chars, or matches a known insecure default value - .env: replaced hardcoded test key with placeholder + generation hint Fix #2 - Docker socket proxy (docker-compose.yml): - Add tecnativa/docker-socket-proxy sidecar - Only expose required Docker API endpoints (CONTAINERS, IMAGES, NETWORKS, POST, EXEC); dangerous endpoints explicitly blocked - Remove direct /var/run/docker.sock mount from main container - Route Docker API via DOCKER_HOST=tcp://docker-socket-proxy:2375 Fix #3 - Azure AD group whitelist (auth.py, models.py, validators.py): - New azure_allowed_group_id field in SystemConfig - After token exchange, verify group membership via Graph API /me/memberOf - Deny login with HTTP 403 if user is not in the required group - New Azure AD users now get role 'viewer' instead of 'admin' Fix #4 - Rate limiting on login (main.py, auth.py, requirements.txt): - Add slowapi==0.1.9 dependency - Initialize SlowAPI limiter in main.py with 429 exception handler - Apply 10 requests/minute limit per IP on /login and /mfa/verify --- app/main.py | 17 ++++++++- app/models.py | 5 +++ app/routers/auth.py | 81 ++++++++++++++++++++++++++++++++++++----- app/utils/config.py | 28 +++++++++++++- app/utils/validators.py | 4 ++ docker-compose.yml | 43 +++++++++++++++++++++- requirements.txt | 1 + 7 files changed, 166 insertions(+), 13 deletions(-) diff --git a/app/main.py b/app/main.py index be9cd11..901c80c 100644 --- a/app/main.py +++ b/app/main.py @@ -3,10 +3,13 @@ import logging import os -from fastapi import FastAPI +from fastapi import FastAPI, Request from fastapi.middleware.cors import CORSMiddleware from fastapi.responses import JSONResponse from fastapi.staticfiles import StaticFiles +from slowapi import Limiter, _rate_limit_exceeded_handler +from slowapi.errors import RateLimitExceeded +from slowapi.util import get_remote_address from app.database import init_db from app.routers import auth, customers, deployments, monitoring, settings, users @@ -21,6 +24,14 @@ logging.basicConfig( ) logger = logging.getLogger(__name__) +# --------------------------------------------------------------------------- +# Application +# --------------------------------------------------------------------------- +# --------------------------------------------------------------------------- +# Rate limiter (SlowAPI) +# --------------------------------------------------------------------------- +limiter = Limiter(key_func=get_remote_address) + # --------------------------------------------------------------------------- # Application # --------------------------------------------------------------------------- @@ -33,6 +44,10 @@ app = FastAPI( openapi_url="/api/openapi.json", ) +# Attach limiter to app state and register the 429 exception handler +app.state.limiter = limiter +app.add_exception_handler(RateLimitExceeded, _rate_limit_exceeded_handler) + # CORS — allow same-origin; adjust if needed app.add_middleware( CORSMiddleware, diff --git a/app/models.py b/app/models.py index 8b3f7cb..618babc 100644 --- a/app/models.py +++ b/app/models.py @@ -168,6 +168,10 @@ class SystemConfig(Base): azure_tenant_id: Mapped[Optional[str]] = mapped_column(String(255), nullable=True) azure_client_id: Mapped[Optional[str]] = mapped_column(String(255), nullable=True) azure_client_secret_encrypted: Mapped[Optional[str]] = mapped_column(Text, nullable=True) + azure_allowed_group_id: Mapped[Optional[str]] = mapped_column( + String(255), nullable=True, + comment="If set, only Azure AD users in this group (object ID) are allowed to log in." + ) created_at: Mapped[datetime] = mapped_column(DateTime, default=datetime.utcnow) updated_at: Mapped[datetime] = mapped_column( DateTime, default=datetime.utcnow, onupdate=datetime.utcnow @@ -203,6 +207,7 @@ class SystemConfig(Base): "azure_tenant_id": self.azure_tenant_id or "", "azure_client_id": self.azure_client_id or "", "azure_client_secret_set": bool(self.azure_client_secret_encrypted), + "azure_allowed_group_id": self.azure_allowed_group_id or "", "created_at": self.created_at.isoformat() if self.created_at else None, "updated_at": self.updated_at.isoformat() if self.updated_at else None, } diff --git a/app/routers/auth.py b/app/routers/auth.py index 7e541cc..cd7e107 100644 --- a/app/routers/auth.py +++ b/app/routers/auth.py @@ -6,7 +6,7 @@ import logging import secrets from datetime import datetime -from fastapi import APIRouter, Depends, HTTPException, status +from fastapi import APIRouter, Depends, HTTPException, Request, status from pydantic import BaseModel from sqlalchemy.orm import Session @@ -27,10 +27,17 @@ from app.utils.validators import ChangePasswordRequest, LoginRequest, MfaTokenRe logger = logging.getLogger(__name__) router = APIRouter() +# Import the shared rate limiter from main +from app.main import limiter + @router.post("/login") -async def login(payload: LoginRequest, db: Session = Depends(get_db)): - """Authenticate with username/password. May require MFA as a second step.""" +@limiter.limit("10/minute") +async def login(request: Request, payload: LoginRequest, db: Session = Depends(get_db)): + """Authenticate with username/password. May require MFA as a second step. + + Rate-limited to 10 attempts per minute per IP address. + """ user = db.query(User).filter(User.username == payload.username).first() if not user or not verify_password(payload.password, user.password_hash): raise HTTPException( @@ -129,8 +136,12 @@ async def mfa_setup_complete(payload: MfaVerifyRequest, db: Session = Depends(ge @router.post("/mfa/verify") -async def mfa_verify(payload: MfaVerifyRequest, db: Session = Depends(get_db)): - """Verify a TOTP code for users who already have MFA set up.""" +@limiter.limit("10/minute") +async def mfa_verify(request: Request, payload: MfaVerifyRequest, db: Session = Depends(get_db)): + """Verify a TOTP code for users who already have MFA set up. + + Rate-limited to 10 attempts per minute per IP address. + """ username = verify_mfa_token(payload.mfa_token) user = db.query(User).filter(User.username == username).first() if not user: @@ -262,17 +273,18 @@ async def azure_callback( try: import msal + import httpx as _httpx client_secret = decrypt_value(config.azure_client_secret_encrypted) authority = f"https://login.microsoftonline.com/{config.azure_tenant_id}" - app = msal.ConfidentialClientApplication( + msal_app = msal.ConfidentialClientApplication( config.azure_client_id, authority=authority, client_credential=client_secret, ) - result = app.acquire_token_by_authorization_code( + result = msal_app.acquire_token_by_authorization_code( payload.code, scopes=["User.Read"], redirect_uri=payload.redirect_uri, @@ -287,7 +299,8 @@ async def azure_callback( id_token_claims = result.get("id_token_claims", {}) email = id_token_claims.get("preferred_username") or id_token_claims.get("email", "") - display_name = id_token_claims.get("name", email) + display_name = id_token_claims.get("name", email) # noqa: F841 + user_access_token = result.get("access_token", "") if not email: raise HTTPException( @@ -295,6 +308,54 @@ async def azure_callback( detail="Could not determine email from Azure AD token.", ) + # ----------------------------------------------------------------- + # Group membership check (Fix #3 – Azure AD group whitelist) + # ----------------------------------------------------------------- + allowed_group_id = getattr(config, "azure_allowed_group_id", None) + if allowed_group_id: + # Use the user's own access token to check their group membership + # via the Microsoft Graph API (requires GroupMember.Read.All or + # the user's own memberOf delegated permission). + graph_url = "https://graph.microsoft.com/v1.0/me/memberOf" + is_member = False + try: + async with _httpx.AsyncClient(timeout=10) as http: + resp = await http.get( + graph_url, + headers={"Authorization": f"Bearer {user_access_token}"}, + ) + if resp.status_code == 200: + groups = resp.json().get("value", []) + is_member = any( + g.get("id") == allowed_group_id for g in groups + ) + else: + logger.warning( + "Graph API group check returned %s for user '%s'.", + resp.status_code, email, + ) + except Exception as graph_exc: + logger.error("Graph API group check failed: %s", graph_exc) + raise HTTPException( + status_code=status.HTTP_503_SERVICE_UNAVAILABLE, + detail="Could not verify Azure AD group membership. Please try again.", + ) + + if not is_member: + logger.warning( + "Azure AD login denied for '%s': not a member of required group '%s'.", + email, allowed_group_id, + ) + raise HTTPException( + status_code=status.HTTP_403_FORBIDDEN, + detail="Access denied: you are not a member of the required Azure AD group.", + ) + else: + logger.warning( + "azure_allowed_group_id is not configured. All Azure AD tenant users can log in. " + "Set azure_allowed_group_id in Settings to restrict access." + ) + # Find or create user user = db.query(User).filter(User.username == email).first() if not user: @@ -303,13 +364,13 @@ async def azure_callback( password_hash=hash_password(secrets.token_urlsafe(32)), email=email, is_active=True, - role="admin", + role="viewer", # New Azure users start as viewer; promote manually auth_provider="azure", ) db.add(user) db.commit() db.refresh(user) - logger.info("Azure AD user '%s' auto-created.", email) + logger.info("Azure AD user '%s' auto-created with role 'viewer'.", email) elif not user.is_active: raise HTTPException( status_code=status.HTTP_403_FORBIDDEN, diff --git a/app/utils/config.py b/app/utils/config.py index 0d7d895..970aaf6 100644 --- a/app/utils/config.py +++ b/app/utils/config.py @@ -35,8 +35,34 @@ class AppConfig: wildcard_cert_id: int | None +# --------------------------------------------------------------------------- # Environment-level settings (not stored in DB) -SECRET_KEY: str = os.environ.get("SECRET_KEY", "change-me-in-production") +# --------------------------------------------------------------------------- + +# Known insecure default values that must never be used in production. +_INSECURE_KEY_VALUES: set[str] = { + "change-me-in-production", + "local-test-secret-key-not-for-production-1234", + "secret", + "changeme", + "", +} + +SECRET_KEY: str = os.environ.get("SECRET_KEY", "") + +# --- Startup security gate --- +# Abort immediately if the key is missing, too short, or a known default. +_MIN_KEY_LENGTH = 32 +if SECRET_KEY in _INSECURE_KEY_VALUES or len(SECRET_KEY) < _MIN_KEY_LENGTH: + raise RuntimeError( + "FATAL: SECRET_KEY is insecure, missing, or too short.\n" + f" Current length : {len(SECRET_KEY)} characters (minimum: {_MIN_KEY_LENGTH})\n" + " The key must be at least 32 random characters and must not be a known default value.\n" + " Generate a secure key with:\n" + " python3 -c \"import secrets; print(secrets.token_hex(32))\"\n" + " Then set it in your .env file as: SECRET_KEY=" + ) + DATABASE_PATH: str = os.environ.get("DATABASE_PATH", "/app/data/netbird_msp.db") LOG_LEVEL: str = os.environ.get("LOG_LEVEL", "INFO") JWT_ALGORITHM: str = "HS256" diff --git a/app/utils/validators.py b/app/utils/validators.py index 202e5cf..f19ee75 100644 --- a/app/utils/validators.py +++ b/app/utils/validators.py @@ -133,6 +133,10 @@ class SystemConfigUpdate(BaseModel): azure_tenant_id: Optional[str] = Field(None, max_length=255) azure_client_id: Optional[str] = Field(None, max_length=255) azure_client_secret: Optional[str] = None # encrypted before storage + azure_allowed_group_id: Optional[str] = Field( + None, max_length=255, + description="Azure AD group object ID. If set, only members of this group can log in." + ) @field_validator("ssl_mode") @classmethod diff --git a/docker-compose.yml b/docker-compose.yml index 748cf9c..9c02bc2 100644 --- a/docker-compose.yml +++ b/docker-compose.yml @@ -1,15 +1,54 @@ services: + # --------------------------------------------------------------------------- + # Docker Socket Proxy — limits Docker API access to only what is needed. + # The main app container no longer has direct access to /var/run/docker.sock. + # --------------------------------------------------------------------------- + docker-socket-proxy: + image: tecnativa/docker-socket-proxy:latest + container_name: docker-socket-proxy + restart: unless-stopped + environment: + # Read-only endpoints + CONTAINERS: 1 + IMAGES: 1 + NETWORKS: 1 + INFO: 1 + # Write endpoints (needed for compose up/down/start/stop) + POST: 1 + # Explicitly deny dangerous endpoints + AUTH: 0 + SECRETS: 0 + SWARM: 0 + NODES: 0 + SERVICES: 0 + TASKS: 0 + CONFIGS: 0 + PLUGINS: 0 + VOLUMES: 0 + BUILD: 0 + COMMIT: 0 + DISTRIBUTION: 0 + EXEC: 1 + volumes: + - /var/run/docker.sock:/var/run/docker.sock:ro + networks: + - npm-network + # Only accessible from within the Docker network — never expose port externally + netbird-msp-appliance: build: . container_name: netbird-msp-appliance restart: unless-stopped + depends_on: + - docker-socket-proxy ports: - "${WEB_UI_PORT:-8000}:8000" volumes: - ./data:/app/data - ./logs:/app/logs - ./backups:/app/backups - - /var/run/docker.sock:/var/run/docker.sock + # NOTE: /var/run/docker.sock is intentionally NOT mounted here. + # Docker access goes through the docker-socket-proxy sidecar. - ${DATA_DIR:-/opt/netbird-instances}:${DATA_DIR:-/opt/netbird-instances} environment: - SECRET_KEY=${SECRET_KEY} @@ -18,6 +57,8 @@ services: - DATA_DIR=${DATA_DIR:-/opt/netbird-instances} - DOCKER_NETWORK=${DOCKER_NETWORK:-npm-network} - HOST_IP=${HOST_IP:-} + # Route Docker API calls through the socket proxy instead of the raw socket + - DOCKER_HOST=tcp://docker-socket-proxy:2375 networks: - npm-network healthcheck: diff --git a/requirements.txt b/requirements.txt index f778cda..f113dbd 100644 --- a/requirements.txt +++ b/requirements.txt @@ -20,6 +20,7 @@ pyyaml==6.0.1 msal==1.28.0 pyotp==2.9.0 qrcode[pil]==7.4.2 +slowapi==0.1.9 pytest==7.4.3 pytest-asyncio==0.23.2 pytest-httpx==0.28.0