mirror of
https://github.com/hansjone/netx.git
synced 2026-10-08 22:20:58 +08:00
Audit only intentional auth actions, not automatic 401/403 gates.
Stop recording unauthorized/forbidden/password-gate and silent refresh; keep login, logout, and failed-login style events. Co-authored-by: Cursor <cursoragent@cursor.com>
This commit is contained in:
parent
d35d3992c2
commit
61531e524f
5 changed files with 35 additions and 127 deletions
|
|
@ -5,7 +5,6 @@ from __future__ import annotations
|
|||
import logging
|
||||
import queue
|
||||
import threading
|
||||
import time
|
||||
from typing import Any
|
||||
|
||||
from .config import settings
|
||||
|
|
@ -19,13 +18,6 @@ _lock = threading.Lock()
|
|||
_counter = 0
|
||||
_dropped = 0
|
||||
|
||||
# Dedupe unauthenticated request floods (SPA fires many parallel 401s).
|
||||
_unauth_lock = threading.Lock()
|
||||
_unauth_recent: dict[str, float] = {}
|
||||
_UNAUTH_DEDUP_GET_SEC = 60.0
|
||||
_UNAUTH_DEDUP_WRITE_SEC = 15.0
|
||||
_UNAUTH_RECENT_MAX = 4000
|
||||
|
||||
# Middleware-tagged HTTP wrappers that duplicate semantic business audits.
|
||||
_MIDDLEWARE_NOISE_ACTIONS = frozenset(
|
||||
{
|
||||
|
|
@ -37,6 +29,10 @@ _MIDDLEWARE_NOISE_ACTIONS = frozenset(
|
|||
"webcrt.delete",
|
||||
"users.get",
|
||||
"api_tokens.get",
|
||||
# Legacy auto-gate noise (no longer written; hide historical rows).
|
||||
"auth.unauthorized",
|
||||
"auth.password_change_required",
|
||||
"auth.forbidden_scope",
|
||||
}
|
||||
)
|
||||
|
||||
|
|
@ -46,6 +42,7 @@ _AUTH_READ_NOISE_ACTIONS = frozenset(
|
|||
{
|
||||
"auth.me",
|
||||
"auth.sessions",
|
||||
"auth.refresh",
|
||||
}
|
||||
)
|
||||
|
||||
|
|
@ -69,32 +66,6 @@ def _sample_ok() -> bool:
|
|||
return (_counter % n) == 0
|
||||
|
||||
|
||||
def should_audit_unauthorized(*, client_ip: str, method: str, path: str) -> bool:
|
||||
"""Rate-limit auth.unauthorized writes: one row per IP+method+path per window.
|
||||
|
||||
Unauthenticated page loads often fan out dozens of GETs in the same second;
|
||||
keeping every 401 drowns real login/security events.
|
||||
"""
|
||||
method_u = str(method or "GET").upper()
|
||||
window = (
|
||||
_UNAUTH_DEDUP_WRITE_SEC
|
||||
if method_u in ("POST", "PUT", "PATCH", "DELETE")
|
||||
else _UNAUTH_DEDUP_GET_SEC
|
||||
)
|
||||
key = f"{client_ip or '-'}|{method_u}|{path or '/'}"
|
||||
now = time.monotonic()
|
||||
with _unauth_lock:
|
||||
global _unauth_recent
|
||||
last = float(_unauth_recent.get(key) or 0.0)
|
||||
if now - last < window:
|
||||
return False
|
||||
_unauth_recent[key] = now
|
||||
if len(_unauth_recent) > _UNAUTH_RECENT_MAX:
|
||||
cutoff = now - max(_UNAUTH_DEDUP_GET_SEC * 5, 300.0)
|
||||
_unauth_recent = {k: v for k, v in _unauth_recent.items() if float(v) >= cutoff}
|
||||
return True
|
||||
|
||||
|
||||
def audit_should_persist(
|
||||
*,
|
||||
action: str,
|
||||
|
|
@ -105,13 +76,12 @@ def audit_should_persist(
|
|||
"""Decide whether a candidate audit event is worth writing.
|
||||
|
||||
Policy:
|
||||
- Always keep auth login/logout/security events, users/tokens/NE/port_traffic/config_sync,
|
||||
and semantic webcrt session/command events.
|
||||
- Drop successful ``auth.me`` / ``auth.sessions`` polls (UI session checks).
|
||||
- Always keep HTTP failures (status >= 400), except pure list noise.
|
||||
- Drop successful GET/HEAD/OPTIONS ``http.*`` (page polling).
|
||||
- Always keep mutating ``http.*`` (POST/PUT/PATCH/DELETE) — no sampling.
|
||||
- Drop middleware ``webcrt.{method}`` / ``audit.list`` (covered by business events).
|
||||
- Keep intentional ops: auth.login/logout/login_failed, users/tokens/NE/port_traffic/
|
||||
config_sync, semantic webcrt session/command.
|
||||
- Drop anonymous 401 / auto-gate noise (unauthorized, forbidden_scope, …).
|
||||
- Drop successful ``auth.me`` / ``auth.sessions`` / ``auth.refresh`` polls.
|
||||
- Drop successful GET/HEAD/OPTIONS ``http.*``; keep mutating http.* and failures.
|
||||
- Drop middleware ``webcrt.{method}`` / ``audit.list``.
|
||||
"""
|
||||
del path # reserved for future path allow/deny lists
|
||||
act = str(action or "")
|
||||
|
|
|
|||
|
|
@ -12,7 +12,6 @@ from starlette.responses import JSONResponse, Response
|
|||
|
||||
from .auth_deps import resolve_user_from_token
|
||||
from .auth_scopes import has_scope, required_scope_for_request
|
||||
from .auth_service import write_audit
|
||||
from .config import settings
|
||||
from .db import SessionLocal
|
||||
|
||||
|
|
@ -113,55 +112,19 @@ class AuthAuditMiddleware(BaseHTTPMiddleware):
|
|||
try:
|
||||
resolved = resolve_user_from_token(db, token) if token else None
|
||||
if resolved is None:
|
||||
client_ip = _client_ip(request)
|
||||
from .audit_async import should_audit_unauthorized
|
||||
|
||||
if should_audit_unauthorized(client_ip=client_ip, method=request.method, path=path):
|
||||
write_audit(
|
||||
db,
|
||||
action="auth.unauthorized",
|
||||
method=request.method,
|
||||
path=path,
|
||||
status_code=401,
|
||||
client_ip=client_ip,
|
||||
user_agent=str(request.headers.get("user-agent") or "")[:512],
|
||||
detail={},
|
||||
)
|
||||
# Do not audit anonymous 401s — SPA fan-out floods the log.
|
||||
# Intentional auth ops (login/logout/login_failed) are written in auth_router.
|
||||
return JSONResponse(status_code=401, content={"detail": "unauthorized"})
|
||||
user, via, scopes, token_id, jti = resolved
|
||||
if bool(getattr(user, "must_change_password", False)):
|
||||
allow_key = (request.method.upper(), path.rstrip("/") if len(path) > 1 else path)
|
||||
if allow_key not in _PASSWORD_CHANGE_ALLOW:
|
||||
write_audit(
|
||||
db,
|
||||
action="auth.password_change_required",
|
||||
actor_user_id=str(user.id),
|
||||
actor_username=str(user.username),
|
||||
method=request.method,
|
||||
path=path,
|
||||
status_code=403,
|
||||
client_ip=_client_ip(request),
|
||||
user_agent=str(request.headers.get("user-agent") or "")[:512],
|
||||
detail={"auth_via": via},
|
||||
)
|
||||
return JSONResponse(
|
||||
status_code=403,
|
||||
content={"detail": "password_change_required"},
|
||||
)
|
||||
need = required_scope_for_request(request.method, path)
|
||||
if need and not has_scope(scopes, need):
|
||||
write_audit(
|
||||
db,
|
||||
action="auth.forbidden_scope",
|
||||
actor_user_id=str(user.id),
|
||||
actor_username=str(user.username),
|
||||
method=request.method,
|
||||
path=path,
|
||||
status_code=403,
|
||||
client_ip=_client_ip(request),
|
||||
user_agent=str(request.headers.get("user-agent") or "")[:512],
|
||||
detail={"required": need, "granted": sorted(scopes), "auth_via": via},
|
||||
)
|
||||
return JSONResponse(
|
||||
status_code=403,
|
||||
content={
|
||||
|
|
|
|||
|
|
@ -153,19 +153,7 @@ def api_refresh(
|
|||
detail={},
|
||||
)
|
||||
raise
|
||||
user = out.get("user") or {}
|
||||
write_audit(
|
||||
db,
|
||||
action="auth.refresh",
|
||||
actor_user_id=str(user.get("id") or ""),
|
||||
actor_username=str(user.get("username") or ""),
|
||||
method="POST",
|
||||
path="/v1/auth/refresh",
|
||||
status_code=200,
|
||||
client_ip=ip,
|
||||
user_agent=ua,
|
||||
detail={},
|
||||
)
|
||||
# Successful silent token refresh is not an operator action — skip audit.
|
||||
return _token_response(response, request, out)
|
||||
|
||||
|
||||
|
|
|
|||
|
|
@ -747,7 +747,10 @@ def list_audit_logs(
|
|||
"api_tokens.get",
|
||||
"auth.me",
|
||||
"auth.sessions",
|
||||
"auth.refresh",
|
||||
"auth.unauthorized",
|
||||
"auth.password_change_required",
|
||||
"auth.forbidden_scope",
|
||||
)
|
||||
q = q.filter(~AuditLog.action.like("http.%"))
|
||||
q = q.filter(~AuditLog.action.in_(noise_actions))
|
||||
|
|
|
|||
|
|
@ -31,7 +31,16 @@ class AuditShouldPersistTests(unittest.TestCase):
|
|||
)
|
||||
|
||||
def test_drop_middleware_noise(self) -> None:
|
||||
for action in ("audit.list", "webcrt.get", "webcrt.post", "users.get", "api_tokens.get"):
|
||||
for action in (
|
||||
"audit.list",
|
||||
"webcrt.get",
|
||||
"webcrt.post",
|
||||
"users.get",
|
||||
"api_tokens.get",
|
||||
"auth.unauthorized",
|
||||
"auth.password_change_required",
|
||||
"auth.forbidden_scope",
|
||||
):
|
||||
self.assertFalse(
|
||||
audit_should_persist(action=action, method="GET", status_code=200),
|
||||
msg=action,
|
||||
|
|
@ -42,15 +51,18 @@ class AuditShouldPersistTests(unittest.TestCase):
|
|||
self.assertTrue(audit_should_persist(action="webcrt.command", status_code=0))
|
||||
self.assertTrue(audit_should_persist(action="webcrt.session_closed", status_code=0))
|
||||
|
||||
def test_drop_auth_me_poll(self) -> None:
|
||||
def test_drop_auth_polls(self) -> None:
|
||||
self.assertFalse(audit_should_persist(action="auth.me", method="GET", status_code=200))
|
||||
self.assertFalse(audit_should_persist(action="auth.sessions", method="GET", status_code=200))
|
||||
# Failures still useful (expired session / forbidden).
|
||||
self.assertTrue(audit_should_persist(action="auth.me", method="GET", status_code=401))
|
||||
self.assertFalse(audit_should_persist(action="auth.refresh", method="POST", status_code=200))
|
||||
|
||||
def test_keep_intentional_auth_ops(self) -> None:
|
||||
self.assertTrue(audit_should_persist(action="auth.login", status_code=200))
|
||||
self.assertTrue(audit_should_persist(action="auth.login_failed", status_code=401))
|
||||
self.assertTrue(audit_should_persist(action="auth.logout", status_code=200))
|
||||
self.assertTrue(audit_should_persist(action="auth.change_password", status_code=200))
|
||||
|
||||
def test_keep_business_prefixes(self) -> None:
|
||||
self.assertTrue(audit_should_persist(action="auth.login", status_code=200))
|
||||
self.assertTrue(audit_should_persist(action="auth.logout", status_code=200))
|
||||
self.assertTrue(audit_should_persist(action="ne.exec", status_code=200))
|
||||
self.assertTrue(audit_should_persist(action="port_traffic.device.start", status_code=200))
|
||||
self.assertTrue(audit_should_persist(action="config_sync.start", status_code=200))
|
||||
|
|
@ -60,33 +72,5 @@ class AuditShouldPersistTests(unittest.TestCase):
|
|||
self.assertFalse(audit_should_persist(action="ume.token.get", method="GET", status_code=200))
|
||||
|
||||
|
||||
class UnauthorizedDedupeTests(unittest.TestCase):
|
||||
def setUp(self) -> None:
|
||||
from netx_api import audit_async as aa
|
||||
|
||||
with aa._unauth_lock:
|
||||
aa._unauth_recent.clear()
|
||||
|
||||
def test_dedupes_same_ip_path(self) -> None:
|
||||
from netx_api.audit_async import should_audit_unauthorized
|
||||
|
||||
self.assertTrue(
|
||||
should_audit_unauthorized(client_ip="127.0.0.1", method="GET", path="/v1/topology")
|
||||
)
|
||||
self.assertFalse(
|
||||
should_audit_unauthorized(client_ip="127.0.0.1", method="GET", path="/v1/topology")
|
||||
)
|
||||
# Different path still recorded once.
|
||||
self.assertTrue(
|
||||
should_audit_unauthorized(client_ip="127.0.0.1", method="GET", path="/v1/managed-ne")
|
||||
)
|
||||
|
||||
def test_different_ip_not_deduped(self) -> None:
|
||||
from netx_api.audit_async import should_audit_unauthorized
|
||||
|
||||
self.assertTrue(should_audit_unauthorized(client_ip="1.1.1.1", method="GET", path="/v1/x"))
|
||||
self.assertTrue(should_audit_unauthorized(client_ip="2.2.2.2", method="GET", path="/v1/x"))
|
||||
|
||||
|
||||
if __name__ == "__main__":
|
||||
unittest.main()
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue