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
This commit is contained in:
17
app/main.py
17
app/main.py
@@ -3,10 +3,13 @@
|
|||||||
import logging
|
import logging
|
||||||
import os
|
import os
|
||||||
|
|
||||||
from fastapi import FastAPI
|
from fastapi import FastAPI, Request
|
||||||
from fastapi.middleware.cors import CORSMiddleware
|
from fastapi.middleware.cors import CORSMiddleware
|
||||||
from fastapi.responses import JSONResponse
|
from fastapi.responses import JSONResponse
|
||||||
from fastapi.staticfiles import StaticFiles
|
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.database import init_db
|
||||||
from app.routers import auth, customers, deployments, monitoring, settings, users
|
from app.routers import auth, customers, deployments, monitoring, settings, users
|
||||||
@@ -21,6 +24,14 @@ logging.basicConfig(
|
|||||||
)
|
)
|
||||||
logger = logging.getLogger(__name__)
|
logger = logging.getLogger(__name__)
|
||||||
|
|
||||||
|
# ---------------------------------------------------------------------------
|
||||||
|
# Application
|
||||||
|
# ---------------------------------------------------------------------------
|
||||||
|
# ---------------------------------------------------------------------------
|
||||||
|
# Rate limiter (SlowAPI)
|
||||||
|
# ---------------------------------------------------------------------------
|
||||||
|
limiter = Limiter(key_func=get_remote_address)
|
||||||
|
|
||||||
# ---------------------------------------------------------------------------
|
# ---------------------------------------------------------------------------
|
||||||
# Application
|
# Application
|
||||||
# ---------------------------------------------------------------------------
|
# ---------------------------------------------------------------------------
|
||||||
@@ -33,6 +44,10 @@ app = FastAPI(
|
|||||||
openapi_url="/api/openapi.json",
|
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
|
# CORS — allow same-origin; adjust if needed
|
||||||
app.add_middleware(
|
app.add_middleware(
|
||||||
CORSMiddleware,
|
CORSMiddleware,
|
||||||
|
|||||||
@@ -168,6 +168,10 @@ class SystemConfig(Base):
|
|||||||
azure_tenant_id: Mapped[Optional[str]] = mapped_column(String(255), nullable=True)
|
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_id: Mapped[Optional[str]] = mapped_column(String(255), nullable=True)
|
||||||
azure_client_secret_encrypted: Mapped[Optional[str]] = mapped_column(Text, 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)
|
created_at: Mapped[datetime] = mapped_column(DateTime, default=datetime.utcnow)
|
||||||
updated_at: Mapped[datetime] = mapped_column(
|
updated_at: Mapped[datetime] = mapped_column(
|
||||||
DateTime, default=datetime.utcnow, onupdate=datetime.utcnow
|
DateTime, default=datetime.utcnow, onupdate=datetime.utcnow
|
||||||
@@ -203,6 +207,7 @@ class SystemConfig(Base):
|
|||||||
"azure_tenant_id": self.azure_tenant_id or "",
|
"azure_tenant_id": self.azure_tenant_id or "",
|
||||||
"azure_client_id": self.azure_client_id or "",
|
"azure_client_id": self.azure_client_id or "",
|
||||||
"azure_client_secret_set": bool(self.azure_client_secret_encrypted),
|
"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,
|
"created_at": self.created_at.isoformat() if self.created_at else None,
|
||||||
"updated_at": self.updated_at.isoformat() if self.updated_at else None,
|
"updated_at": self.updated_at.isoformat() if self.updated_at else None,
|
||||||
}
|
}
|
||||||
|
|||||||
@@ -6,7 +6,7 @@ import logging
|
|||||||
import secrets
|
import secrets
|
||||||
from datetime import datetime
|
from datetime import datetime
|
||||||
|
|
||||||
from fastapi import APIRouter, Depends, HTTPException, status
|
from fastapi import APIRouter, Depends, HTTPException, Request, status
|
||||||
from pydantic import BaseModel
|
from pydantic import BaseModel
|
||||||
from sqlalchemy.orm import Session
|
from sqlalchemy.orm import Session
|
||||||
|
|
||||||
@@ -27,10 +27,17 @@ from app.utils.validators import ChangePasswordRequest, LoginRequest, MfaTokenRe
|
|||||||
logger = logging.getLogger(__name__)
|
logger = logging.getLogger(__name__)
|
||||||
router = APIRouter()
|
router = APIRouter()
|
||||||
|
|
||||||
|
# Import the shared rate limiter from main
|
||||||
|
from app.main import limiter
|
||||||
|
|
||||||
|
|
||||||
@router.post("/login")
|
@router.post("/login")
|
||||||
async def login(payload: LoginRequest, db: Session = Depends(get_db)):
|
@limiter.limit("10/minute")
|
||||||
"""Authenticate with username/password. May require MFA as a second step."""
|
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()
|
user = db.query(User).filter(User.username == payload.username).first()
|
||||||
if not user or not verify_password(payload.password, user.password_hash):
|
if not user or not verify_password(payload.password, user.password_hash):
|
||||||
raise HTTPException(
|
raise HTTPException(
|
||||||
@@ -129,8 +136,12 @@ async def mfa_setup_complete(payload: MfaVerifyRequest, db: Session = Depends(ge
|
|||||||
|
|
||||||
|
|
||||||
@router.post("/mfa/verify")
|
@router.post("/mfa/verify")
|
||||||
async def mfa_verify(payload: MfaVerifyRequest, db: Session = Depends(get_db)):
|
@limiter.limit("10/minute")
|
||||||
"""Verify a TOTP code for users who already have MFA set up."""
|
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)
|
username = verify_mfa_token(payload.mfa_token)
|
||||||
user = db.query(User).filter(User.username == username).first()
|
user = db.query(User).filter(User.username == username).first()
|
||||||
if not user:
|
if not user:
|
||||||
@@ -262,17 +273,18 @@ async def azure_callback(
|
|||||||
|
|
||||||
try:
|
try:
|
||||||
import msal
|
import msal
|
||||||
|
import httpx as _httpx
|
||||||
|
|
||||||
client_secret = decrypt_value(config.azure_client_secret_encrypted)
|
client_secret = decrypt_value(config.azure_client_secret_encrypted)
|
||||||
authority = f"https://login.microsoftonline.com/{config.azure_tenant_id}"
|
authority = f"https://login.microsoftonline.com/{config.azure_tenant_id}"
|
||||||
|
|
||||||
app = msal.ConfidentialClientApplication(
|
msal_app = msal.ConfidentialClientApplication(
|
||||||
config.azure_client_id,
|
config.azure_client_id,
|
||||||
authority=authority,
|
authority=authority,
|
||||||
client_credential=client_secret,
|
client_credential=client_secret,
|
||||||
)
|
)
|
||||||
|
|
||||||
result = app.acquire_token_by_authorization_code(
|
result = msal_app.acquire_token_by_authorization_code(
|
||||||
payload.code,
|
payload.code,
|
||||||
scopes=["User.Read"],
|
scopes=["User.Read"],
|
||||||
redirect_uri=payload.redirect_uri,
|
redirect_uri=payload.redirect_uri,
|
||||||
@@ -287,7 +299,8 @@ async def azure_callback(
|
|||||||
|
|
||||||
id_token_claims = result.get("id_token_claims", {})
|
id_token_claims = result.get("id_token_claims", {})
|
||||||
email = id_token_claims.get("preferred_username") or id_token_claims.get("email", "")
|
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:
|
if not email:
|
||||||
raise HTTPException(
|
raise HTTPException(
|
||||||
@@ -295,6 +308,54 @@ async def azure_callback(
|
|||||||
detail="Could not determine email from Azure AD token.",
|
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
|
# Find or create user
|
||||||
user = db.query(User).filter(User.username == email).first()
|
user = db.query(User).filter(User.username == email).first()
|
||||||
if not user:
|
if not user:
|
||||||
@@ -303,13 +364,13 @@ async def azure_callback(
|
|||||||
password_hash=hash_password(secrets.token_urlsafe(32)),
|
password_hash=hash_password(secrets.token_urlsafe(32)),
|
||||||
email=email,
|
email=email,
|
||||||
is_active=True,
|
is_active=True,
|
||||||
role="admin",
|
role="viewer", # New Azure users start as viewer; promote manually
|
||||||
auth_provider="azure",
|
auth_provider="azure",
|
||||||
)
|
)
|
||||||
db.add(user)
|
db.add(user)
|
||||||
db.commit()
|
db.commit()
|
||||||
db.refresh(user)
|
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:
|
elif not user.is_active:
|
||||||
raise HTTPException(
|
raise HTTPException(
|
||||||
status_code=status.HTTP_403_FORBIDDEN,
|
status_code=status.HTTP_403_FORBIDDEN,
|
||||||
|
|||||||
@@ -35,8 +35,34 @@ class AppConfig:
|
|||||||
wildcard_cert_id: int | None
|
wildcard_cert_id: int | None
|
||||||
|
|
||||||
|
|
||||||
|
# ---------------------------------------------------------------------------
|
||||||
# Environment-level settings (not stored in DB)
|
# 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=<generated-value>"
|
||||||
|
)
|
||||||
|
|
||||||
DATABASE_PATH: str = os.environ.get("DATABASE_PATH", "/app/data/netbird_msp.db")
|
DATABASE_PATH: str = os.environ.get("DATABASE_PATH", "/app/data/netbird_msp.db")
|
||||||
LOG_LEVEL: str = os.environ.get("LOG_LEVEL", "INFO")
|
LOG_LEVEL: str = os.environ.get("LOG_LEVEL", "INFO")
|
||||||
JWT_ALGORITHM: str = "HS256"
|
JWT_ALGORITHM: str = "HS256"
|
||||||
|
|||||||
@@ -133,6 +133,10 @@ class SystemConfigUpdate(BaseModel):
|
|||||||
azure_tenant_id: Optional[str] = Field(None, max_length=255)
|
azure_tenant_id: Optional[str] = Field(None, max_length=255)
|
||||||
azure_client_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_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")
|
@field_validator("ssl_mode")
|
||||||
@classmethod
|
@classmethod
|
||||||
|
|||||||
@@ -1,15 +1,54 @@
|
|||||||
services:
|
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:
|
netbird-msp-appliance:
|
||||||
build: .
|
build: .
|
||||||
container_name: netbird-msp-appliance
|
container_name: netbird-msp-appliance
|
||||||
restart: unless-stopped
|
restart: unless-stopped
|
||||||
|
depends_on:
|
||||||
|
- docker-socket-proxy
|
||||||
ports:
|
ports:
|
||||||
- "${WEB_UI_PORT:-8000}:8000"
|
- "${WEB_UI_PORT:-8000}:8000"
|
||||||
volumes:
|
volumes:
|
||||||
- ./data:/app/data
|
- ./data:/app/data
|
||||||
- ./logs:/app/logs
|
- ./logs:/app/logs
|
||||||
- ./backups:/app/backups
|
- ./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}
|
- ${DATA_DIR:-/opt/netbird-instances}:${DATA_DIR:-/opt/netbird-instances}
|
||||||
environment:
|
environment:
|
||||||
- SECRET_KEY=${SECRET_KEY}
|
- SECRET_KEY=${SECRET_KEY}
|
||||||
@@ -18,6 +57,8 @@ services:
|
|||||||
- DATA_DIR=${DATA_DIR:-/opt/netbird-instances}
|
- DATA_DIR=${DATA_DIR:-/opt/netbird-instances}
|
||||||
- DOCKER_NETWORK=${DOCKER_NETWORK:-npm-network}
|
- DOCKER_NETWORK=${DOCKER_NETWORK:-npm-network}
|
||||||
- HOST_IP=${HOST_IP:-}
|
- 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:
|
networks:
|
||||||
- npm-network
|
- npm-network
|
||||||
healthcheck:
|
healthcheck:
|
||||||
|
|||||||
@@ -20,6 +20,7 @@ pyyaml==6.0.1
|
|||||||
msal==1.28.0
|
msal==1.28.0
|
||||||
pyotp==2.9.0
|
pyotp==2.9.0
|
||||||
qrcode[pil]==7.4.2
|
qrcode[pil]==7.4.2
|
||||||
|
slowapi==0.1.9
|
||||||
pytest==7.4.3
|
pytest==7.4.3
|
||||||
pytest-asyncio==0.23.2
|
pytest-asyncio==0.23.2
|
||||||
pytest-httpx==0.28.0
|
pytest-httpx==0.28.0
|
||||||
|
|||||||
Reference in New Issue
Block a user