fix(security): close MEDIUM ASVS findings — JWT pinning, SSE tickets, SSRF, mTLS pin, rate limits + correctness bugs
Auth (V2.1.1/V3.1.2, V2.1.3, V3.1.1): - Pin JWT iss/aud/typ at mint and require+verify them at decode; revocation (jti denylist + tokens_valid_from) still enforced. - Change-password now requires min_length=12. - SSE auth moves off JWT-in-URL to a single-use 60s opaque ticket (POST /auth/sse-ticket); raw JWT in query no longer authenticates a stream. Removed dead fail-open get_stream_user helper. Egress (V5.1.1, V9.1.1/V14.1.3): - Webhook delivery + CRUD reject SSRF destinations (private/loopback/link-local/ metadata, IPv4-mapped, multi-A-record) via resolved-IP validation, pin to the vetted IP, and never auto-follow redirects. Opt-out via DECNET_WEBHOOK_ALLOW_PRIVATE. - UpdaterClient pins the worker leaf cert SHA-256 against the stored per-host fingerprint (fail closed on missing/mismatch); DECNET_VERIFY_HOSTNAME now defaults True. Hardening (V13.1.3, V4.1.4, V13.1.2): - Rate-limit change-password (5/min), enroll-bundle (10/min), webhook-create (20/min), host-delete (20/min) via the existing slowapi limiter. - Correct false 'global auth middleware' comment; document enroll-bundle proxy trust. Correctness (BUG-7..11): - BUG-7 unbound bus in finally; BUG-8 apply_ceiling clamps to min(base,ceiling); BUG-9 commit before emit; BUG-10 multi-actor rearm for sub-threshold identities; BUG-11 normalize naive timestamps to UTC. Already-closed (no change): V14.1.1, V2.1.2/V3.1.3, V5.1.2. Tests added for every fix; unanimous adversarial review.
This commit is contained in:
@@ -4,6 +4,7 @@ from fastapi import APIRouter
|
||||
from .auth.api_login import router as login_router
|
||||
from .auth.api_change_pass import router as change_pass_router
|
||||
from .auth.api_logout import router as logout_router
|
||||
from .auth.api_sse_ticket import router as sse_ticket_router
|
||||
from .logs.api_get_logs import router as logs_router
|
||||
from .logs.api_get_histogram import router as histogram_router
|
||||
from .bounty.api_get_bounties import router as bounty_router
|
||||
@@ -75,9 +76,12 @@ from .ttp.api_export_navigator import router as ttp_navigator_router
|
||||
from .ttp.api_get_groups_for_technique import router as ttp_groups_for_technique_router
|
||||
|
||||
api_router = APIRouter(
|
||||
# Every route under /api/v1 is auth-guarded (either by an explicit
|
||||
# require_* Depends or by the global auth middleware). Document 401/403
|
||||
# here so the OpenAPI schema reflects reality for contract tests.
|
||||
# Auth is enforced PER ROUTE via explicit ``require_*`` Depends (see
|
||||
# decnet.web.dependencies) — there is NO global auth middleware. A route
|
||||
# without a require_* dependency is unauthenticated BY DESIGN; the only such
|
||||
# routes are /health (liveness) and /auth/login (credential exchange).
|
||||
# The 401/403 entries below are documented here so the OpenAPI schema
|
||||
# reflects reality for contract tests, not because a middleware applies them.
|
||||
responses={
|
||||
400: {"description": "Malformed request body"},
|
||||
401: {"description": "Missing or invalid credentials"},
|
||||
@@ -91,6 +95,7 @@ api_router = APIRouter(
|
||||
api_router.include_router(login_router)
|
||||
api_router.include_router(change_pass_router)
|
||||
api_router.include_router(logout_router)
|
||||
api_router.include_router(sse_ticket_router)
|
||||
|
||||
# Logs & Analytics
|
||||
api_router.include_router(logs_router)
|
||||
|
||||
@@ -10,8 +10,9 @@ stream's attacker. Emits a one-shot snapshot on connect (latest
|
||||
observation per primitive) so the panel hydrates immediately.
|
||||
|
||||
Authorization mirrors :mod:`decnet.web.router.topology.api_events` —
|
||||
JWT via the ``?token=`` query parameter (EventSource can't set
|
||||
arbitrary headers) + ``require_stream_viewer`` role gate. The 404
|
||||
a single-use opaque ticket via the ``?ticket=`` query parameter
|
||||
(EventSource can't set arbitrary headers) + ``require_stream_viewer``
|
||||
role gate. The 404
|
||||
fires after auth so an existence probe can't leak an attacker UUID
|
||||
to an unauthenticated caller.
|
||||
|
||||
|
||||
@@ -2,12 +2,13 @@
|
||||
from datetime import datetime, timezone
|
||||
from typing import Any, Optional
|
||||
|
||||
from fastapi import APIRouter, Depends, HTTPException, status
|
||||
from fastapi import APIRouter, Depends, HTTPException, Request, status
|
||||
|
||||
from decnet.telemetry import traced as _traced
|
||||
from decnet.web.auth import ahash_password, averify_password
|
||||
from decnet.web.dependencies import get_current_user_unchecked, invalidate_user_cache, repo
|
||||
from decnet.web.db.models import ChangePasswordRequest, MessageResponse
|
||||
from decnet.web.limiter import limiter
|
||||
|
||||
router = APIRouter()
|
||||
|
||||
@@ -19,19 +20,21 @@ router = APIRouter()
|
||||
responses={
|
||||
400: {"description": "Bad Request (e.g. malformed JSON)"},
|
||||
401: {"description": "Could not validate credentials"},
|
||||
422: {"description": "Validation error"}
|
||||
422: {"description": "Validation error"},
|
||||
429: {"description": "Too many password-change attempts — retry after the window resets"},
|
||||
},
|
||||
)
|
||||
@limiter.limit("5/minute")
|
||||
@_traced("api.change_password")
|
||||
async def change_password(request: ChangePasswordRequest, current_user: str = Depends(get_current_user_unchecked)) -> dict[str, str]:
|
||||
async def change_password(request: Request, body: ChangePasswordRequest, current_user: str = Depends(get_current_user_unchecked)) -> dict[str, str]:
|
||||
_user: Optional[dict[str, Any]] = await repo.get_user_by_uuid(current_user)
|
||||
if not _user or not await averify_password(request.old_password, _user["password_hash"]):
|
||||
if not _user or not await averify_password(body.old_password, _user["password_hash"]):
|
||||
raise HTTPException(
|
||||
status_code=status.HTTP_401_UNAUTHORIZED,
|
||||
detail="Incorrect old password",
|
||||
)
|
||||
|
||||
_new_hash: str = await ahash_password(request.new_password)
|
||||
_new_hash: str = await ahash_password(body.new_password)
|
||||
await repo.update_user_password(current_user, _new_hash, must_change_password=False)
|
||||
# Changing a password revokes every existing session for this user (incl.
|
||||
# the current one): the caller's next request 401s and re-authenticates.
|
||||
|
||||
39
decnet/web/router/auth/api_sse_ticket.py
Normal file
39
decnet/web/router/auth/api_sse_ticket.py
Normal file
@@ -0,0 +1,39 @@
|
||||
# SPDX-License-Identifier: AGPL-3.0-or-later
|
||||
"""Mint a single-use, short-lived SSE stream ticket (V3.1.1).
|
||||
|
||||
EventSource cannot send an Authorization header, so SSE auth used to ride in
|
||||
``?token=<JWT>`` — leaking the full-lifetime bearer into access/proxy logs,
|
||||
browser history, and Referer. This endpoint lets an already-authenticated
|
||||
client (gated by the NORMAL header JWT via ``require_viewer``) exchange that
|
||||
header credential for an opaque ``secrets.token_urlsafe(32)`` ticket, valid for
|
||||
60s and single-use, which it then passes to the SSE endpoint as ``?ticket=``.
|
||||
The JWT never appears in any URL.
|
||||
|
||||
The ticket store lives in-process (decnet.web.dependencies); multi-process
|
||||
deployments need a shared store — out of scope, see that module's note.
|
||||
"""
|
||||
from fastapi import APIRouter, Depends
|
||||
|
||||
from decnet.telemetry import traced as _traced
|
||||
from decnet.web.dependencies import mint_sse_ticket, require_viewer, _SSE_TICKET_TTL
|
||||
from decnet.web.db.models.auth import SSETicketResponse
|
||||
|
||||
router = APIRouter()
|
||||
|
||||
|
||||
@router.post(
|
||||
"/auth/sse-ticket",
|
||||
tags=["Authentication"],
|
||||
response_model=SSETicketResponse,
|
||||
responses={
|
||||
400: {"description": "Malformed request body"},
|
||||
401: {"description": "Missing or invalid credentials"},
|
||||
403: {"description": "Authenticated but not authorized"},
|
||||
},
|
||||
)
|
||||
@_traced("api.sse_ticket")
|
||||
async def mint_stream_ticket(user: dict = Depends(require_viewer)) -> SSETicketResponse:
|
||||
"""Exchange the presented header JWT for a single-use 60s SSE ticket bound to
|
||||
this user's uuid + role. Any authenticated (viewer or admin) user may mint."""
|
||||
ticket = mint_sse_ticket(user["uuid"], user["role"])
|
||||
return SSETicketResponse(ticket=ticket, expires_in=int(_SSE_TICKET_TTL))
|
||||
@@ -6,8 +6,9 @@ request and forwards each matching event as a Server-Sent Event.
|
||||
Emits a one-shot snapshot on connect (current paginated campaign
|
||||
list).
|
||||
|
||||
Mirror of :mod:`decnet.web.router.identities.api_events`. Auth: JWT
|
||||
via ``?token=`` query param + ``require_stream_viewer`` role.
|
||||
Mirror of :mod:`decnet.web.router.identities.api_events`. Auth:
|
||||
single-use opaque ticket via ``?ticket=`` query param +
|
||||
``require_stream_viewer`` role.
|
||||
"""
|
||||
from __future__ import annotations
|
||||
|
||||
|
||||
@@ -8,8 +8,9 @@ Server-Sent Event to the browser. Emits a one-shot snapshot on connect
|
||||
fetch to initialise.
|
||||
|
||||
Authorization mirrors :mod:`decnet.web.router.topology.api_events` — a
|
||||
JWT passed via the ``?token=`` query parameter (EventSource can't set
|
||||
arbitrary headers) + ``require_stream_viewer`` role gate.
|
||||
single-use opaque ticket passed via the ``?ticket=`` query parameter
|
||||
(EventSource can't set arbitrary headers) + ``require_stream_viewer``
|
||||
role gate.
|
||||
|
||||
The endpoint is broadly scoped (every identity event, not per-uuid)
|
||||
because both ``AttackerDetail`` and ``IdentityDetail`` need the same
|
||||
|
||||
@@ -12,12 +12,13 @@ from __future__ import annotations
|
||||
|
||||
import pathlib
|
||||
|
||||
from fastapi import APIRouter, Depends, HTTPException, status
|
||||
from fastapi import APIRouter, Depends, HTTPException, Request, status
|
||||
|
||||
from decnet.logging import get_logger
|
||||
from decnet.swarm.client import AgentClient
|
||||
from decnet.web.db.repository import BaseRepository
|
||||
from decnet.web.dependencies import get_repo, require_admin
|
||||
from decnet.web.limiter import limiter
|
||||
from decnet.web.router.swarm._mtls import PeerCert, require_operator_cert
|
||||
|
||||
log = get_logger("swarm.decommission")
|
||||
@@ -32,10 +33,13 @@ router = APIRouter()
|
||||
401: {"description": "Missing or invalid admin JWT"},
|
||||
403: {"description": "Authenticated user is not an admin, or operator cert missing"},
|
||||
404: {"description": "No host with this UUID is enrolled"},
|
||||
429: {"description": "Too many decommission requests — retry after the window resets"},
|
||||
},
|
||||
)
|
||||
@limiter.limit("20/minute")
|
||||
async def api_decommission_host(
|
||||
uuid: str,
|
||||
request: Request,
|
||||
repo: BaseRepository = Depends(get_repo),
|
||||
_admin: dict = Depends(require_admin),
|
||||
_operator: PeerCert = Depends(require_operator_cert),
|
||||
|
||||
@@ -34,6 +34,7 @@ from decnet.swarm.bundle_builder import build_tarball, render_bootstrap
|
||||
from decnet.web.db.models.swarm import EnrollBundleRequest, EnrollBundleResponse
|
||||
from decnet.web.db.repository import BaseRepository
|
||||
from decnet.web.dependencies import get_repo, require_admin
|
||||
from decnet.web.limiter import limiter
|
||||
|
||||
log = get_logger("swarm_mgmt.enroll_bundle")
|
||||
|
||||
@@ -117,8 +118,10 @@ async def _lookup_live(token: str) -> _Bundle:
|
||||
403: {"description": "Insufficient permissions"},
|
||||
409: {"description": "A worker with this name is already enrolled"},
|
||||
422: {"description": "Request body validation error"},
|
||||
429: {"description": "Too many enroll-bundle requests — retry after the window resets"},
|
||||
},
|
||||
)
|
||||
@limiter.limit("10/minute")
|
||||
async def create_enroll_bundle(
|
||||
req: EnrollBundleRequest,
|
||||
request: Request,
|
||||
@@ -251,6 +254,14 @@ async def get_payload(
|
||||
# The agent's first connect-back — its source IP is the reachable address
|
||||
# the master will later use to probe it. Backfill the SwarmHost row here
|
||||
# so the operator sees the real address instead of an empty placeholder.
|
||||
#
|
||||
# PROXY TRUST WARNING: `request.client.host` is the TCP peer's IP.
|
||||
# If this endpoint sits behind a TCP-terminating reverse proxy (nginx,
|
||||
# HAProxy, etc.) the recorded address will be the proxy's IP, not the
|
||||
# agent's. Either bind the API directly on the network reachable by
|
||||
# agents, or configure the proxy to preserve the original source IP
|
||||
# (e.g. PROXY Protocol on a loopback listener, *not* X-Forwarded-For
|
||||
# which is trivially spoofable). See THREAT_MODEL.md §DA-08.
|
||||
client_host = request.client.host if request.client else ""
|
||||
if client_host:
|
||||
try:
|
||||
|
||||
@@ -8,8 +8,9 @@ a Server-Sent Event to the browser. Emits a one-shot snapshot on connect
|
||||
separate fetch to initialise the "pending" buffer.
|
||||
|
||||
Authorization matches :mod:`decnet.web.router.stream.api_stream_events`
|
||||
— a JWT passed via the ``?token=`` query parameter (EventSource can't
|
||||
set arbitrary headers) + ``require_stream_viewer`` role gate. The
|
||||
— a single-use opaque ticket passed via the ``?ticket=`` query
|
||||
parameter (EventSource can't set arbitrary headers) +
|
||||
``require_stream_viewer`` role gate. The
|
||||
per-topology 404 is enforced after auth so existence probes can't leak
|
||||
a topology id to an unauthenticated caller.
|
||||
"""
|
||||
|
||||
@@ -7,7 +7,7 @@ import secrets
|
||||
from datetime import datetime, timezone
|
||||
from typing import Any, cast
|
||||
|
||||
from fastapi import APIRouter, Depends, HTTPException
|
||||
from fastapi import APIRouter, Depends, HTTPException, Request
|
||||
|
||||
from decnet.bus import topics as _topics
|
||||
from decnet.bus.app import get_app_bus
|
||||
@@ -22,13 +22,28 @@ from decnet.web.db.models import (
|
||||
)
|
||||
from decnet.web.db.models.webhooks import _row_to_response_dict
|
||||
from decnet.web.dependencies import repo, require_admin
|
||||
from decnet.web.limiter import limiter
|
||||
from decnet.webhook.enums import merge_patterns
|
||||
from decnet.webhook.ssrf import WebhookDestinationError, validate_webhook_url
|
||||
|
||||
log = get_logger("api.webhooks")
|
||||
|
||||
router = APIRouter()
|
||||
|
||||
|
||||
def _validate_url_or_422(url: str) -> None:
|
||||
"""Reject a webhook URL that resolves to a forbidden destination.
|
||||
|
||||
Runs the same SSRF guard the delivery path enforces, but at
|
||||
registration time so a bad URL is surfaced to the operator as a clear
|
||||
422 instead of being silently dropped on every delivery attempt.
|
||||
"""
|
||||
try:
|
||||
validate_webhook_url(url)
|
||||
except WebhookDestinationError as e:
|
||||
raise HTTPException(status_code=422, detail=str(e)) from e
|
||||
|
||||
|
||||
async def _notify_subscriptions_changed() -> None:
|
||||
"""Publish `system.webhook.subscriptions_changed` on the bus.
|
||||
|
||||
@@ -60,10 +75,14 @@ def _row_to_response(row: dict[str, Any]) -> WebhookResponse:
|
||||
responses={
|
||||
400: {"description": "At least one of simple_events / topic_patterns required"},
|
||||
409: {"description": "Name already in use"},
|
||||
422: {"description": "URL resolves to a forbidden (internal) destination"},
|
||||
429: {"description": "Too many webhook-create requests — retry after the window resets"},
|
||||
},
|
||||
)
|
||||
@limiter.limit("20/minute")
|
||||
@_traced("api.webhook.create")
|
||||
async def api_create_webhook(
|
||||
request: Request,
|
||||
req: WebhookCreateRequest,
|
||||
admin: dict = Depends(require_admin),
|
||||
) -> WebhookCreateResponse:
|
||||
@@ -78,6 +97,8 @@ async def api_create_webhook(
|
||||
if existing:
|
||||
raise HTTPException(status_code=409, detail="Webhook name already exists")
|
||||
|
||||
_validate_url_or_422(str(req.url))
|
||||
|
||||
# Auto-generate a URL-safe secret if the caller didn't provide one.
|
||||
# 32 bytes of os-entropy is the same ballpark as a CSRF token.
|
||||
secret = req.secret or secrets.token_urlsafe(32)
|
||||
@@ -146,6 +167,7 @@ async def api_get_webhook(
|
||||
400: {"description": "Empty or invalid patch"},
|
||||
404: {"description": "Webhook not found"},
|
||||
409: {"description": "Name already in use"},
|
||||
422: {"description": "URL resolves to a forbidden (internal) destination"},
|
||||
},
|
||||
)
|
||||
@_traced("api.webhook.update")
|
||||
@@ -167,6 +189,7 @@ async def api_update_webhook(
|
||||
patch["name"] = req.name
|
||||
|
||||
if req.url is not None:
|
||||
_validate_url_or_422(str(req.url))
|
||||
patch["url"] = str(req.url)
|
||||
|
||||
if req.secret is not None:
|
||||
|
||||
Reference in New Issue
Block a user