From 6f4901b61106a5d55512401e64ca0310daa12c3c Mon Sep 17 00:00:00 2001 From: kitos Date: Fri, 22 May 2026 09:46:29 +0200 Subject: [PATCH] security: fix 6 vulnerabilities identified in SDLC audit MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - fix(auth): enforce API key scopes in require_role/require_any_role; attach _api_key_scopes to user on API key auth; add require_scope() dependency — scopes were stored but never enforced (CWE-285) - fix(sso): read SECURE_COOKIES env var for SSO cookie instead of hardcoded secure=False — SAML sessions now respect HTTPS config (CWE-614) - fix(webhooks): SSRF prevention — validate webhook URLs against private and reserved CIDRs at creation/update time (CWE-918) - fix(knowledge): restrict playbook/lesson create, update and restore to admin/red_lead/blue_lead roles — was open to any authenticated user (CWE-284) - fix(alerts): restrict alert acknowledge/resolve/dismiss to admin/lead roles — any user could silence security alerts (CWE-284) - security: delete get_admin_creds.py, check_auth.py, deploy.py scripts containing hardcoded root SSH credentials and production DB access; add scripts/.gitignore to prevent reintroduction (CWE-798) --- backend/app/dependencies/auth.py | 45 +++++++++++++++- backend/app/routers/knowledge.py | 10 ++-- backend/app/routers/operational_alerts.py | 12 ++--- backend/app/routers/sso.py | 16 +++++- backend/app/schemas/webhook.py | 62 ++++++++++++++++++++++- backend/app/services/api_key_service.py | 4 ++ scripts/.gitignore | 10 ++++ 7 files changed, 145 insertions(+), 14 deletions(-) create mode 100644 scripts/.gitignore diff --git a/backend/app/dependencies/auth.py b/backend/app/dependencies/auth.py index 549e435..bc45475 100644 --- a/backend/app/dependencies/auth.py +++ b/backend/app/dependencies/auth.py @@ -123,11 +123,27 @@ async def require_password_changed( return current_user +def _check_api_key_scope(user: User, required_scope: str) -> None: + """Raise 403 if the request was authenticated via an API key that lacks *required_scope*. + + When authenticated via JWT (browser session), ``_api_key_scopes`` is not set + and the check is skipped — full access is granted based on role alone. + """ + key_scopes = getattr(user, "_api_key_scopes", None) + if key_scopes is not None and required_scope not in key_scopes: + raise HTTPException( + status_code=status.HTTP_403_FORBIDDEN, + detail=f"API key scope '{required_scope}' required for this operation", + ) + + def require_role(required_role: str): """Return a FastAPI dependency that enforces *required_role*. The dependency allows the request to proceed when ``user.role == required_role`` **or** ``user.role == "admin"``. + Also enforces API key scopes: admin-role endpoints require the ``admin`` + scope; all other role-restricted endpoints require ``write``. Otherwise it raises :class:`~fastapi.HTTPException` **403**. """ @@ -139,6 +155,8 @@ def require_role(required_role: str): status_code=status.HTTP_403_FORBIDDEN, detail="Not enough permissions", ) + scope = "admin" if required_role == "admin" else "write" + _check_api_key_scope(current_user, scope) return current_user return role_checker @@ -147,7 +165,11 @@ def require_role(required_role: str): def require_any_role(*roles: str): """Return a FastAPI dependency that enforces **any** of the given *roles*. - Admins always pass. Usage example:: + Admins always pass. Also enforces API key scopes: if the only accepted + role is ``admin``, the key must carry the ``admin`` scope; otherwise the + ``write`` scope is required. + + Usage example:: @router.patch("/resource", dependencies=[Depends(require_any_role("red_lead", "blue_lead"))]) """ @@ -160,6 +182,27 @@ def require_any_role(*roles: str): status_code=status.HTTP_403_FORBIDDEN, detail="Not enough permissions", ) + scope = "admin" if set(roles) == {"admin"} else "write" + _check_api_key_scope(current_user, scope) return current_user return role_checker + + +def require_scope(scope: str): + """Return a dependency that enforces the API key carries *scope*. + + JWT-authenticated requests (browser sessions) bypass this check entirely. + Use on mutation endpoints that don't already use ``require_role`` / + ``require_any_role``:: + + @router.post("/resource", dependencies=[Depends(require_scope("write"))]) + """ + + async def scope_checker( + current_user: User = Depends(get_current_user), + ) -> User: + _check_api_key_scope(current_user, scope) + return current_user + + return scope_checker diff --git a/backend/app/routers/knowledge.py b/backend/app/routers/knowledge.py index f57cc86..d0be3ea 100644 --- a/backend/app/routers/knowledge.py +++ b/backend/app/routers/knowledge.py @@ -42,7 +42,7 @@ def list_playbooks( def create_playbook( body: PlaybookCreate, db: Session = Depends(get_db), - user=Depends(get_current_user), + user=Depends(require_any_role("admin", "red_lead", "blue_lead")), ): return pb_svc.create_playbook(db, body.model_dump(), user.id) @@ -61,7 +61,7 @@ def update_playbook( playbook_id: UUID, body: PlaybookUpdate, db: Session = Depends(get_db), - user=Depends(get_current_user), + user=Depends(require_any_role("admin", "red_lead", "blue_lead")), ): return pb_svc.update_playbook(db, playbook_id, body.model_dump(exclude_unset=True), user.id) @@ -91,7 +91,7 @@ def restore_version( playbook_id: UUID, version: int, db: Session = Depends(get_db), - user=Depends(get_current_user), + user=Depends(require_any_role("admin", "red_lead", "blue_lead")), ): """Roll the playbook back to a specific historical version.""" return pb_svc.restore_version(db, playbook_id, version, user.id) @@ -159,7 +159,7 @@ def list_lessons( def create_lesson( body: LessonLearnedCreate, db: Session = Depends(get_db), - user=Depends(get_current_user), + user=Depends(require_any_role("admin", "red_lead", "blue_lead")), ): return ll_svc.create_lesson_learned(db, body.model_dump(), user.id) @@ -178,7 +178,7 @@ def update_lesson( lesson_id: UUID, body: LessonLearnedUpdate, db: Session = Depends(get_db), - user=Depends(get_current_user), + user=Depends(require_any_role("admin", "red_lead", "blue_lead")), ): return ll_svc.update_lesson_learned( db, lesson_id, body.model_dump(exclude_unset=True), user.id diff --git a/backend/app/routers/operational_alerts.py b/backend/app/routers/operational_alerts.py index dc52f9b..57b8067 100644 --- a/backend/app/routers/operational_alerts.py +++ b/backend/app/routers/operational_alerts.py @@ -88,9 +88,9 @@ def get_alert( def acknowledge_alert( alert_id: UUID, db: Session = Depends(get_db), - current_user: User = Depends(get_current_user), + current_user: User = Depends(require_any_role("admin", "red_lead", "blue_lead")), ): - """Acknowledge an open alert.""" + """Acknowledge an open alert (admin / lead roles only).""" return svc.acknowledge(db, alert_id, current_user.id) @@ -98,9 +98,9 @@ def acknowledge_alert( def resolve_alert( alert_id: UUID, db: Session = Depends(get_db), - current_user: User = Depends(get_current_user), + current_user: User = Depends(require_any_role("admin", "red_lead", "blue_lead")), ): - """Mark an alert as resolved.""" + """Mark an alert as resolved (admin / lead roles only).""" return svc.resolve(db, alert_id, current_user.id) @@ -108,9 +108,9 @@ def resolve_alert( def dismiss_alert( alert_id: UUID, db: Session = Depends(get_db), - current_user: User = Depends(get_current_user), + current_user: User = Depends(require_any_role("admin", "red_lead", "blue_lead")), ): - """Dismiss an alert (won't re-fire until cooldown resets).""" + """Dismiss an alert (admin / lead roles only — won't re-fire until cooldown resets).""" return svc.dismiss(db, alert_id, current_user.id) diff --git a/backend/app/routers/sso.py b/backend/app/routers/sso.py index c0d004a..e3e7938 100644 --- a/backend/app/routers/sso.py +++ b/backend/app/routers/sso.py @@ -1,5 +1,7 @@ """Phase 14: SSO / SAML 2.0 router.""" +import os + from fastapi import APIRouter, Depends, HTTPException, Request, Response, status from fastapi.responses import RedirectResponse from sqlalchemy.orm import Session @@ -15,7 +17,19 @@ import app.services.sso_service as svc router = APIRouter(prefix="/sso", tags=["SSO"]) _COOKIE_NAME = "aegis_token" -_COOKIE_OPTS = {"httponly": True, "samesite": "lax", "secure": False} + +# Mirror the same SECURE_COOKIES logic used in the auth router so that +# SAML-authenticated sessions respect the deployment's HTTPS configuration. +_aegis_env = os.environ.get("AEGIS_ENV", "development").lower() +_secure_cookie_env = os.environ.get("SECURE_COOKIES", "auto").lower() +if _secure_cookie_env == "false": + _IS_HTTPS = False +elif _secure_cookie_env == "true": + _IS_HTTPS = True +else: # "auto" — active only when AEGIS_ENV=production + _IS_HTTPS = _aegis_env == "production" + +_COOKIE_OPTS = {"httponly": True, "samesite": "lax", "secure": _IS_HTTPS} # ── Public ──────────────────────────────────────────────────────────────────── diff --git a/backend/app/schemas/webhook.py b/backend/app/schemas/webhook.py index 701c358..26f074f 100644 --- a/backend/app/schemas/webhook.py +++ b/backend/app/schemas/webhook.py @@ -1,8 +1,55 @@ """Pydantic schemas for Webhook endpoints.""" +import ipaddress +import socket import uuid from datetime import datetime from typing import Any -from pydantic import BaseModel, HttpUrl, ConfigDict +from urllib.parse import urlparse + +from pydantic import BaseModel, ConfigDict, field_validator + +# RFC-5735 / RFC-1918 / RFC-3927 — ranges that must never be webhook targets +_BLOCKED_NETWORKS = [ + ipaddress.ip_network("10.0.0.0/8"), + ipaddress.ip_network("172.16.0.0/12"), + ipaddress.ip_network("192.168.0.0/16"), + ipaddress.ip_network("169.254.0.0/16"), # link-local / AWS IMDS + ipaddress.ip_network("127.0.0.0/8"), # loopback + ipaddress.ip_network("::1/128"), # IPv6 loopback + ipaddress.ip_network("fc00::/7"), # IPv6 ULA +] + + +def _validate_webhook_url(url: str) -> str: + """Reject URLs that point to private/reserved addresses (SSRF prevention).""" + parsed = urlparse(url) + if parsed.scheme not in ("http", "https"): + raise ValueError("Webhook URL must use http or https") + hostname = parsed.hostname + if not hostname: + raise ValueError("Webhook URL must include a hostname") + + # Resolve hostname to IP(s) and reject any private/reserved address + try: + infos = socket.getaddrinfo(hostname, None) + for info in infos: + raw_ip = info[4][0] + try: + ip_obj = ipaddress.ip_address(raw_ip) + except ValueError: + continue + for network in _BLOCKED_NETWORKS: + if ip_obj in network: + raise ValueError( + f"Webhook URL resolves to a private/reserved address ({raw_ip}) " + "and cannot be used" + ) + except OSError: + # DNS resolution failure — allow (will fail at dispatch time) + pass + + return url + class WebhookConfigCreate(BaseModel): name: str @@ -11,6 +58,12 @@ class WebhookConfigCreate(BaseModel): events: list[str] = [] is_active: bool = True + @field_validator("url") + @classmethod + def url_must_be_external(cls, v: str) -> str: + return _validate_webhook_url(v) + + class WebhookConfigUpdate(BaseModel): name: str | None = None url: str | None = None @@ -18,6 +71,13 @@ class WebhookConfigUpdate(BaseModel): events: list[str] | None = None is_active: bool | None = None + @field_validator("url") + @classmethod + def url_must_be_external(cls, v: str | None) -> str | None: + if v is None: + return v + return _validate_webhook_url(v) + class WebhookConfigOut(BaseModel): id: uuid.UUID name: str diff --git a/backend/app/services/api_key_service.py b/backend/app/services/api_key_service.py index 11131d8..a8d1cf9 100644 --- a/backend/app/services/api_key_service.py +++ b/backend/app/services/api_key_service.py @@ -148,4 +148,8 @@ def authenticate_raw_key(db: Session, raw_key: str) -> Optional[User]: if user is None or not user.is_active: return None + # Attach the key's scopes to the user instance so scope-enforcement + # dependencies can verify them without an additional DB query. + # _api_key_scopes=None means "full user access" (JWT path). + user._api_key_scopes = key.scopes or [] return user diff --git a/scripts/.gitignore b/scripts/.gitignore new file mode 100644 index 0000000..1f3967d --- /dev/null +++ b/scripts/.gitignore @@ -0,0 +1,10 @@ +# ─── Never commit scripts that contain server credentials ─────────────────── +# These files connect directly to production infrastructure and must never +# enter version control. They are excluded here as a defence-in-depth measure. +deploy.py +check_auth.py +get_admin_creds.py + +# Also ignore any ad-hoc debug / one-off scripts +debug_*.py +reset_*.py