From 1947c759c7b4e65e3b6db96d7457289f28853081 Mon Sep 17 00:00:00 2001 From: HardMax71 Date: Mon, 19 Oct 2026 02:04:32 +0200 Subject: [PATCH] refactor: security and rate limit metrics come from spans, and SecurityMetrics and RateLimitMetrics are gone Each login runs in an auth.login span, and token and CSRF rejections, admin checks, lockouts, weak passwords and password resets leave an auth.* span where the metric used to be recorded. RateLimitService leaves rate_limit.request and rate_limit.decision spans. The collector's signaltometrics connector makes the same 15 series from them, and every label survives as a span attribute (decision 3 on #389). The bypass counter now comes from requests whose algorithm is bypassed. Part of #389. --- backend/app/core/metrics/__init__.py | 4 - backend/app/core/metrics/rate_limit.py | 44 ----- backend/app/core/metrics/security.py | 113 ------------- backend/app/core/providers.py | 26 +-- backend/app/core/security.py | 20 +-- .../app/services/admin/admin_user_service.py | 8 +- backend/app/services/auth_service.py | 30 ++-- backend/app/services/rate_limit_service.py | 36 ++-- .../dashboards/rate-limiting-dashboard.json | 30 ++-- .../dashboards/security-auth.json | 8 +- backend/otel-collector-config.yaml | 158 ++++++++++++++++++ backend/tests/e2e/conftest.py | 6 - .../rate_limit/test_rate_limit_service.py | 14 +- backend/tests/unit/conftest.py | 12 -- .../test_replay_and_security_metrics.py | 22 --- backend/tests/unit/core/test_csrf.py | 78 ++++----- .../unit/core/test_rate_limit_middleware.py | 16 +- backend/tests/unit/core/test_security.py | 50 +++--- .../services/test_auth_service_metrics.py | 43 +++-- .../unit/services/test_rate_limit_service.py | 22 ++- docs/operations/metrics-reference.md | 46 ++--- 21 files changed, 392 insertions(+), 394 deletions(-) delete mode 100644 backend/app/core/metrics/rate_limit.py delete mode 100644 backend/app/core/metrics/security.py delete mode 100644 backend/tests/unit/core/metrics/test_replay_and_security_metrics.py diff --git a/backend/app/core/metrics/__init__.py b/backend/app/core/metrics/__init__.py index 3f2d08b05..cbd091483 100644 --- a/backend/app/core/metrics/__init__.py +++ b/backend/app/core/metrics/__init__.py @@ -2,15 +2,11 @@ from app.core.metrics.connections import ConnectionMetrics from app.core.metrics.database import IdempotencyMetrics from app.core.metrics.events import EventMetrics -from app.core.metrics.rate_limit import RateLimitMetrics -from app.core.metrics.security import SecurityMetrics __all__ = [ "BaseMetrics", "ConnectionMetrics", "IdempotencyMetrics", "EventMetrics", - "RateLimitMetrics", - "SecurityMetrics", "setup_metrics", ] diff --git a/backend/app/core/metrics/rate_limit.py b/backend/app/core/metrics/rate_limit.py deleted file mode 100644 index a0059feb9..000000000 --- a/backend/app/core/metrics/rate_limit.py +++ /dev/null @@ -1,44 +0,0 @@ -from app.core.metrics.base import BaseMetrics - - -class RateLimitMetrics(BaseMetrics): - """Metrics for rate limiting system.""" - - def _create_instruments(self) -> None: - self._requests = self._meter.create_counter( - name="rate_limit.requests.total", - description="Total number of rate limit checks", - unit="1", - ) - self._allowed = self._meter.create_counter( - name="rate_limit.allowed.total", - description="Number of allowed requests", - unit="1", - ) - self._rejected = self._meter.create_counter( - name="rate_limit.rejected.total", - description="Number of rejected requests", - unit="1", - ) - self._bypass = self._meter.create_counter( - name="rate_limit.bypass.total", - description="Number of bypassed rate limit checks", - unit="1", - ) - - def record_request(self, endpoint: str, authenticated: bool, algorithm: str) -> None: - """Record a rate limit check request.""" - attrs = {"endpoint": endpoint, "authenticated": str(authenticated).lower(), "algorithm": algorithm} - self._requests.add(1, attrs) - - def record_allowed(self, endpoint: str, group: str) -> None: - """Record an allowed request.""" - self._allowed.add(1, {"endpoint": endpoint, "group": group}) - - def record_rejected(self, endpoint: str, group: str) -> None: - """Record a rejected request.""" - self._rejected.add(1, {"endpoint": endpoint, "group": group}) - - def record_bypass(self, endpoint: str) -> None: - """Record a bypassed rate limit check.""" - self._bypass.add(1, {"endpoint": endpoint}) diff --git a/backend/app/core/metrics/security.py b/backend/app/core/metrics/security.py deleted file mode 100644 index e783de6e2..000000000 --- a/backend/app/core/metrics/security.py +++ /dev/null @@ -1,113 +0,0 @@ -import time -from collections.abc import Generator -from contextlib import contextmanager - -from app.core.metrics.base import BaseMetrics - - -class SecurityMetrics(BaseMetrics): - """Metrics for security events.""" - - def _create_instruments(self) -> None: - # Authentication metrics - self.authentication_attempts = self._meter.create_counter( - name="authentication.attempts.total", description="Total number of authentication attempts", unit="1" - ) - - self.authentication_failures = self._meter.create_counter( - name="authentication.failures.total", description="Total number of failed authentications", unit="1" - ) - - self.authentication_duration = self._meter.create_histogram( - name="authentication.duration", description="Time taken for authentication in seconds", unit="s" - ) - - # Token metrics - self.token_validation_failures = self._meter.create_counter( - name="token.validation.failures.total", - description="Access tokens rejected while decoding, by reason", - unit="1", - ) - - # Authorization metrics - self.authorization_checks = self._meter.create_counter( - name="authorization.checks.total", description="Total number of authorization checks", unit="1" - ) - - self.authorization_denials = self._meter.create_counter( - name="authorization.denials.total", description="Total number of authorization denials", unit="1" - ) - - # CSRF protection metrics - self.csrf_validation_failures = self._meter.create_counter( - name="csrf.validation.failures.total", description="Total number of CSRF validation failures", unit="1" - ) - - # Password metrics - self.password_reset_requests = self._meter.create_counter( - name="password.reset.requests.total", description="Total number of password reset requests", unit="1" - ) - - self.weak_password_attempts = self._meter.create_counter( - name="weak.password.attempts.total", description="Total number of weak password attempts", unit="1" - ) - - # Brute force detection - self.brute_force_attempts = self._meter.create_counter( - name="brute.force.attempts.total", - description="Login attempts rejected because the account is locked", - unit="1", - ) - - self.accounts_locked = self._meter.create_counter( - name="accounts.locked.total", description="Total number of accounts locked due to security", unit="1" - ) - - @contextmanager - def track_authentication(self, method: str) -> Generator[None, None, None]: - start = time.monotonic() - success = False - try: - yield - success = True - finally: - self.authentication_attempts.add(1, attributes={"method": method, "success": str(success)}) - if not success: - self.authentication_failures.add(1, attributes={"method": method}) - self.authentication_duration.record(time.monotonic() - start, attributes={"method": method}) - - def record_token_validation_failure(self, reason: str) -> None: - self.token_validation_failures.add(1, attributes={"reason": reason}) - - def record_authorization_check( - self, resource: str, action: str, allowed: bool, user_role: str | None = None - ) -> None: - self.authorization_checks.add( - 1, - attributes={ - "resource": resource, - "action": action, - "allowed": str(allowed), - "user_role": user_role or "unknown", - }, - ) - - if not allowed: - self.authorization_denials.add( - 1, attributes={"resource": resource, "action": action, "user_role": user_role or "unknown"} - ) - - def record_csrf_validation_failure(self, reason: str) -> None: - self.csrf_validation_failures.add(1, attributes={"reason": reason}) - - def record_password_reset_request(self, method: str = "admin") -> None: - self.password_reset_requests.add(1, attributes={"method": method}) - - def record_weak_password_attempt(self, weakness_type: str) -> None: - self.weak_password_attempts.add(1, attributes={"weakness_type": weakness_type}) - - def record_brute_force_attempt(self) -> None: - self.brute_force_attempts.add(1) - - def record_account_locked(self, reason: str) -> None: - self.accounts_locked.add(1, attributes={"reason": reason}) diff --git a/backend/app/core/providers.py b/backend/app/core/providers.py index cd7570898..405fe4066 100644 --- a/backend/app/core/providers.py +++ b/backend/app/core/providers.py @@ -16,8 +16,6 @@ ConnectionMetrics, EventMetrics, IdempotencyMetrics, - RateLimitMetrics, - SecurityMetrics, ) from app.core.security import SecurityService from app.core.tracing import Tracer @@ -143,18 +141,18 @@ def get_rate_limit_service( self, redis_client: redis.Redis, settings: Settings, - rate_limit_metrics: RateLimitMetrics, + tracer: trace.Tracer, logger: structlog.stdlib.BoundLogger, ) -> RateLimitService: - return RateLimitService(redis_client, settings, rate_limit_metrics, logger) + return RateLimitService(redis_client, settings, tracer, logger) class CoreServicesProvider(Provider): scope = Scope.APP @provide - def get_security_service(self, settings: Settings, security_metrics: SecurityMetrics) -> SecurityService: - return SecurityService(settings, security_metrics) + def get_security_service(self, settings: Settings, tracer: trace.Tracer) -> SecurityService: + return SecurityService(settings, tracer) @provide def get_tracer( @@ -257,18 +255,10 @@ def get_event_metrics(self, settings: Settings) -> EventMetrics: def get_connection_metrics(self, settings: Settings) -> ConnectionMetrics: return ConnectionMetrics(settings) - @provide - def get_rate_limit_metrics(self, settings: Settings) -> RateLimitMetrics: - return RateLimitMetrics(settings) - @provide def get_idempotency_metrics(self, settings: Settings) -> IdempotencyMetrics: return IdempotencyMetrics(settings) - @provide - def get_security_metrics(self, settings: Settings) -> SecurityMetrics: - return SecurityMetrics(settings) - class RepositoryProvider(Provider): """Provides all repository instances. @@ -360,7 +350,7 @@ def get_auth_service( self, user_repository: UserRepository, security_service: SecurityService, - security_metrics: SecurityMetrics, + tracer: trace.Tracer, logger: structlog.stdlib.BoundLogger, lockout_service: LoginLockoutService, runtime_settings: RuntimeSettingsLoader, @@ -370,7 +360,7 @@ def get_auth_service( return AuthService( user_repo=user_repository, security_service=security_service, - security_metrics=security_metrics, + tracer=tracer, logger=logger, lockout_service=lockout_service, runtime_settings=runtime_settings, @@ -560,7 +550,7 @@ def get_admin_user_service( execution_service: ExecutionService, rate_limit_service: RateLimitService, security_service: SecurityService, - security_metrics: SecurityMetrics, + tracer: trace.Tracer, logger: structlog.stdlib.BoundLogger, ) -> AdminUserService: return AdminUserService( @@ -569,7 +559,7 @@ def get_admin_user_service( execution_service=execution_service, rate_limit_service=rate_limit_service, security_service=security_service, - security_metrics=security_metrics, + tracer=tracer, logger=logger, ) diff --git a/backend/app/core/security.py b/backend/app/core/security.py index 66790bb71..b6e25a824 100644 --- a/backend/app/core/security.py +++ b/backend/app/core/security.py @@ -6,11 +6,11 @@ import jwt from fastapi import Request from fastapi.security import OAuth2PasswordBearer +from opentelemetry import trace from pwdlib import PasswordHash from pwdlib.hashers.bcrypt import BcryptHasher from pydantic import TypeAdapter, ValidationError -from app.core.metrics import SecurityMetrics from app.domain.user import CSRFValidationError, InvalidCredentialsError from app.settings import Settings @@ -30,9 +30,9 @@ class AccessTokenClaims(SubjectClaims): class SecurityService: - def __init__(self, settings: Settings, security_metrics: SecurityMetrics) -> None: + def __init__(self, settings: Settings, tracer: trace.Tracer) -> None: self.settings = settings - self._security_metrics = security_metrics + self._tracer = tracer # --8<-- [start:password_hashing] self._password_hash = PasswordHash((BcryptHasher(rounds=self.settings.BCRYPT_ROUNDS),)) @@ -69,18 +69,18 @@ def decode_token(self, token: str, *, allow_expired: bool = False) -> str: options={"verify_exp": not allow_expired}, ) except jwt.ExpiredSignatureError: - self._security_metrics.record_token_validation_failure("expired") + self._tracer.start_span("auth.token_rejected", attributes={"auth.reason": "expired"}).end() raise InvalidCredentialsError() from None except jwt.InvalidSignatureError: - self._security_metrics.record_token_validation_failure("invalid_signature") + self._tracer.start_span("auth.token_rejected", attributes={"auth.reason": "invalid_signature"}).end() raise InvalidCredentialsError() from None except jwt.PyJWTError: - self._security_metrics.record_token_validation_failure("invalid") + self._tracer.start_span("auth.token_rejected", attributes={"auth.reason": "invalid"}).end() raise InvalidCredentialsError() from None try: claims = _subject_adapter.validate_python(payload) except ValidationError: - self._security_metrics.record_token_validation_failure("missing_subject") + self._tracer.start_span("auth.token_rejected", attributes={"auth.reason": "missing_subject"}).end() raise InvalidCredentialsError() from None return claims["sub"] @@ -175,15 +175,15 @@ def validate_csrf_from_request(self, request: Request) -> str: cookie_token = request.cookies.get("csrf_token", "") if not header_token: - self._security_metrics.record_csrf_validation_failure("missing_header") + self._tracer.start_span("auth.csrf_rejected", attributes={"auth.reason": "missing_header"}).end() raise CSRFValidationError("CSRF token missing from X-CSRF-Token header") if not self.validate_csrf_token(header_token, cookie_token): - self._security_metrics.record_csrf_validation_failure("token_mismatch") + self._tracer.start_span("auth.csrf_rejected", attributes={"auth.reason": "token_mismatch"}).end() raise CSRFValidationError("CSRF token invalid or does not match cookie") if not self._verify_csrf_signature(header_token, access_token): - self._security_metrics.record_csrf_validation_failure("invalid_signature") + self._tracer.start_span("auth.csrf_rejected", attributes={"auth.reason": "invalid_signature"}).end() raise CSRFValidationError("CSRF token signature invalid") return header_token diff --git a/backend/app/services/admin/admin_user_service.py b/backend/app/services/admin/admin_user_service.py index f6f0de6be..88969cc0b 100644 --- a/backend/app/services/admin/admin_user_service.py +++ b/backend/app/services/admin/admin_user_service.py @@ -2,8 +2,8 @@ from datetime import datetime, timedelta, timezone import structlog +from opentelemetry import trace -from app.core.metrics import SecurityMetrics from app.core.security import SecurityService from app.db import UserRepository from app.domain.admin import AdminUserOverviewDomain, DerivedCountsDomain @@ -39,7 +39,7 @@ def __init__( execution_service: ExecutionService, rate_limit_service: RateLimitService, security_service: SecurityService, - security_metrics: SecurityMetrics, + tracer: trace.Tracer, logger: structlog.stdlib.BoundLogger, ) -> None: self._users = user_repository @@ -47,7 +47,7 @@ def __init__( self._executions = execution_service self._rate_limits = rate_limit_service self._security = security_service - self._security_metrics = security_metrics + self._tracer = tracer self.logger = logger async def get_user_overview(self, user_id: str, hours: int = 24) -> AdminUserOverviewDomain: @@ -266,7 +266,7 @@ async def delete_user(self, *, admin_user_id: str, user_id: str, cascade: bool) async def reset_user_password(self, *, admin_user_id: str, user_id: str, new_password: str) -> None: self.logger.info("Admin resetting user password", admin_user_id=admin_user_id, target_user_id=user_id) - self._security_metrics.record_password_reset_request(method="admin") + self._tracer.start_span("auth.password_reset", attributes={"auth.method": "admin"}).end() hashed = self._security.get_password_hash(new_password) pr = PasswordReset(user_id=user_id, new_password=hashed) if not await self._users.reset_user_password(pr): diff --git a/backend/app/services/auth_service.py b/backend/app/services/auth_service.py index 68be201d3..09b5b04be 100644 --- a/backend/app/services/auth_service.py +++ b/backend/app/services/auth_service.py @@ -3,8 +3,9 @@ import structlog from fastapi import Request +from opentelemetry import trace +from opentelemetry.util.types import AttributeValue -from app.core.metrics import SecurityMetrics from app.core.security import SecurityService from app.db import UserRepository from app.domain.enums import LoginMethod, UserRole @@ -43,7 +44,7 @@ def __init__( self, user_repo: UserRepository, security_service: SecurityService, - security_metrics: SecurityMetrics, + tracer: trace.Tracer, logger: structlog.stdlib.BoundLogger, lockout_service: LoginLockoutService, runtime_settings: RuntimeSettingsLoader, @@ -52,7 +53,7 @@ def __init__( ): self.user_repo = user_repo self.security_service = security_service - self.security_metrics = security_metrics + self._tracer = tracer self.logger = logger self._lockout = lockout_service self._runtime_settings = runtime_settings @@ -84,12 +85,13 @@ async def get_current_user(self, request: Request) -> User: async def get_admin(self, request: Request) -> User: user = await self.get_current_user(request) - self.security_metrics.record_authorization_check( - "/admin", - request.method, - user.role == UserRole.ADMIN, - user_role=user.role, - ) + attributes: dict[str, AttributeValue] = { + "auth.resource": "/admin", + "auth.action": request.method, + "auth.allowed": user.role == UserRole.ADMIN, + "auth.user_role": user.role, + } + self._tracer.start_span("auth.authorization", attributes=attributes).end() if user.role != UserRole.ADMIN: self.logger.warning("Admin access denied", username=user.username, role=user.role) raise AdminAccessRequiredError(user.username) @@ -121,7 +123,7 @@ async def _fail_login( key=username, ) if locked: - self.security_metrics.record_account_locked("brute_force") + self._tracer.start_span("auth.account_locked", attributes={"auth.reason": "brute_force"}).end() raise AccountLockedError("Account locked due to too many failed attempts") raise InvalidCredentialsError() @@ -133,10 +135,11 @@ async def login( user_agent: str | None, ) -> LoginResult: if await self._lockout.check_locked(username): - self.security_metrics.record_brute_force_attempt() + self._tracer.start_span("auth.locked_login").end() raise AccountLockedError("Account temporarily locked due to too many failed attempts") - with self.security_metrics.track_authentication("login"): + login_attributes: dict[str, AttributeValue] = {"auth.method": "login", "auth.success": False} + with self._tracer.start_as_current_span("auth.login", attributes=login_attributes) as span: user = await self.user_repo.get_user(username) if not user: @@ -186,6 +189,7 @@ async def login( key=user.username, ) + span.set_attribute("auth.success", True) return LoginResult( username=user.username, role=user.role, @@ -205,7 +209,7 @@ async def register( effective = await self._runtime_settings.get_effective_settings() min_len = effective.password_min_length if len(password) < min_len: - self.security_metrics.record_weak_password_attempt("too_short") + self._tracer.start_span("auth.weak_password", attributes={"auth.weakness_type": "too_short"}).end() raise ValidationError(f"Password must be at least {min_len} characters") existing = await self.user_repo.get_user(username) diff --git a/backend/app/services/rate_limit_service.py b/backend/app/services/rate_limit_service.py index 93a443643..95bee6aa2 100644 --- a/backend/app/services/rate_limit_service.py +++ b/backend/app/services/rate_limit_service.py @@ -9,9 +9,10 @@ import redis.asyncio as redis import structlog +from opentelemetry import trace +from opentelemetry.util.types import AttributeValue from pydantic import Json, TypeAdapter -from app.core.metrics import RateLimitMetrics from app.domain.rate_limit import ( EndpointUsageStats, RateLimitAlgorithm, @@ -34,20 +35,28 @@ def __init__( self, redis_client: redis.Redis, settings: Settings, - metrics: RateLimitMetrics, + tracer: trace.Tracer, logger: structlog.stdlib.BoundLogger, ): self.redis = redis_client self.settings = settings self.logger = logger self.prefix = settings.RATE_LIMIT_REDIS_PREFIX - self.metrics = metrics + self._tracer = tracer self._pattern_cache: dict[str, re.Pattern[str]] = {} # Patterns to match IDs and replace with * self._uuid_pattern = re.compile(r"[0-9a-fA-F]{8}-[0-9a-fA-F]{4}-[0-9a-fA-F]{4}-[0-9a-fA-F]{4}-[0-9a-fA-F]{12}") self._id_pattern = re.compile(r"/[0-9a-zA-Z]{20,}(?=/|$)") + def _report_request(self, endpoint: str, authenticated: bool, algorithm: str) -> None: + attributes: dict[str, AttributeValue] = { + "rate_limit.endpoint": endpoint, + "rate_limit.authenticated": authenticated, + "rate_limit.algorithm": algorithm, + } + self._tracer.start_span("rate_limit.request", attributes=attributes).end() + def _index_key(self, user_id: str) -> str: """Key of the Redis set that indexes all per-user rate limit state keys.""" return f"{self.prefix}index:{user_id}" @@ -88,9 +97,7 @@ async def check_rate_limit(self, user_id: str, endpoint: str) -> RateLimitStatus return await self._check(user_id, endpoint, await self._get_config()) except redis.RedisError as e: self.logger.error("Rate limit check failed, allowing the request", endpoint=endpoint, error=str(e)) - self.metrics.record_request( - self._normalize_endpoint(endpoint), not user_id.startswith("ip:"), "redis_error" - ) + self._report_request(self._normalize_endpoint(endpoint), not user_id.startswith("ip:"), "redis_error") return self._unlimited() async def _check(self, user_id: str, endpoint: str, config: RateLimitConfig) -> RateLimitStatus: @@ -102,21 +109,20 @@ async def _check(self, user_id: str, endpoint: str, config: RateLimitConfig) -> user_config = self._user_limit(config, user_id) if user_config.bypass_rate_limit: - self.metrics.record_bypass(normalized_endpoint) - self.metrics.record_request(normalized_endpoint, authenticated, "bypassed") + self._report_request(normalized_endpoint, authenticated, "bypassed") return self._unlimited() for rule in self._matching_rules(endpoint, user_config, config): return await self._apply_rule(user_id, endpoint, rule, user_config.global_multiplier) - self.metrics.record_request(normalized_endpoint, authenticated, "no_limit") + self._report_request(normalized_endpoint, authenticated, "no_limit") return self._unlimited() async def _apply_rule(self, user_id: str, endpoint: str, rule: RateLimitRule, multiplier: float) -> RateLimitStatus: normalized_endpoint = self._normalize_endpoint(endpoint) effective_limit = int(rule.requests * multiplier) - self.metrics.record_request(normalized_endpoint, not user_id.startswith("ip:"), rule.algorithm) + self._report_request(normalized_endpoint, not user_id.startswith("ip:"), rule.algorithm) if rule.algorithm == RateLimitAlgorithm.TOKEN_BUCKET: status = await self._check_token_bucket( @@ -125,10 +131,12 @@ async def _apply_rule(self, user_id: str, endpoint: str, rule: RateLimitRule, mu else: status = await self._check_sliding_window(user_id, endpoint, effective_limit, rule.window_seconds, rule) - if status.allowed: - self.metrics.record_allowed(normalized_endpoint, rule.group) - else: - self.metrics.record_rejected(normalized_endpoint, rule.group) + attributes: dict[str, AttributeValue] = { + "rate_limit.endpoint": normalized_endpoint, + "rate_limit.group": rule.group, + "rate_limit.allowed": status.allowed, + } + self._tracer.start_span("rate_limit.decision", attributes=attributes).end() return status diff --git a/backend/grafana/provisioning/dashboards/rate-limiting-dashboard.json b/backend/grafana/provisioning/dashboards/rate-limiting-dashboard.json index be01119bf..63b3ae4e2 100644 --- a/backend/grafana/provisioning/dashboards/rate-limiting-dashboard.json +++ b/backend/grafana/provisioning/dashboards/rate-limiting-dashboard.json @@ -364,13 +364,13 @@ "pluginVersion": "8.3.3", "targets": [ { - "expr": "sum(rate(rate_limit_requests_total{endpoint!~\"/api/v1/result/.*\"}[5m])) by (endpoint)", - "legendFormat": "{{endpoint}}", + "expr": "sum(rate(rate_limit_requests_total{rate_limit_endpoint!~\"/api/v1/result/.*\"}[5m])) by (rate_limit_endpoint)", + "legendFormat": "{{rate_limit_endpoint}}", "refId": "A", "datasource": "Victoria Metrics" }, { - "expr": "sum(rate(rate_limit_requests_total{endpoint=~\"/api/v1/result/.*\"}[5m]))", + "expr": "sum(rate(rate_limit_requests_total{rate_limit_endpoint=~\"/api/v1/result/.*\"}[5m]))", "legendFormat": "/api/v1/result/*", "refId": "B", "datasource": "Victoria Metrics" @@ -451,8 +451,8 @@ "pluginVersion": "8.3.3", "targets": [ { - "expr": "sum(rate(rate_limit_requests_total[5m])) by (authenticated)", - "legendFormat": "authenticated={{authenticated}}", + "expr": "sum(rate(rate_limit_requests_total[5m])) by (rate_limit_authenticated)", + "legendFormat": "authenticated={{rate_limit_authenticated}}", "refId": "A", "datasource": "Victoria Metrics" } @@ -535,8 +535,8 @@ "pluginVersion": "8.3.3", "targets": [ { - "expr": "sum(increase(rate_limit_requests_total[1h])) by (algorithm)", - "legendFormat": "{{algorithm}}", + "expr": "sum(increase(rate_limit_requests_total[1h])) by (rate_limit_algorithm)", + "legendFormat": "{{rate_limit_algorithm}}", "refId": "A", "datasource": "Victoria Metrics" } @@ -616,14 +616,14 @@ "pluginVersion": "8.3.3", "targets": [ { - "expr": "sum(rate(rate_limit_allowed_total[5m])) by (group)", - "legendFormat": "allowed: {{group}}", + "expr": "sum(rate(rate_limit_allowed_total[5m])) by (rate_limit_group)", + "legendFormat": "allowed: {{rate_limit_group}}", "refId": "A", "datasource": "Victoria Metrics" }, { - "expr": "sum(rate(rate_limit_rejected_total[5m])) by (group)", - "legendFormat": "rejected: {{group}}", + "expr": "sum(rate(rate_limit_rejected_total[5m])) by (rate_limit_group)", + "legendFormat": "rejected: {{rate_limit_group}}", "refId": "B", "datasource": "Victoria Metrics" } @@ -717,8 +717,8 @@ "pluginVersion": "8.3.3", "targets": [ { - "expr": "sum(rate(rate_limit_bypass_total[5m])) by (endpoint)", - "legendFormat": "{{endpoint}}", + "expr": "sum(rate(rate_limit_bypass_total[5m])) by (rate_limit_endpoint)", + "legendFormat": "{{rate_limit_endpoint}}", "refId": "A", "datasource": "Victoria Metrics" } @@ -798,8 +798,8 @@ "pluginVersion": "8.3.3", "targets": [ { - "expr": "sum(rate(rate_limit_rejected_total[5m])) by (endpoint)", - "legendFormat": "{{endpoint}}", + "expr": "sum(rate(rate_limit_rejected_total[5m])) by (rate_limit_endpoint)", + "legendFormat": "{{rate_limit_endpoint}}", "refId": "A", "datasource": "Victoria Metrics" } diff --git a/backend/grafana/provisioning/dashboards/security-auth.json b/backend/grafana/provisioning/dashboards/security-auth.json index 11e2286d2..2f18defc4 100644 --- a/backend/grafana/provisioning/dashboards/security-auth.json +++ b/backend/grafana/provisioning/dashboards/security-auth.json @@ -486,8 +486,8 @@ "pluginVersion": "8.3.3", "targets": [ { - "expr": "sum by (reason) (rate(token_validation_failures_total[5m]))", - "legendFormat": "{{reason}}", + "expr": "sum by (auth_reason) (rate(token_validation_failures_total[5m]))", + "legendFormat": "{{auth_reason}}", "refId": "C", "datasource": "Victoria Metrics" } @@ -567,8 +567,8 @@ "pluginVersion": "8.3.3", "targets": [ { - "expr": "sum by (reason) (rate(csrf_validation_failures_total[5m]))", - "legendFormat": "{{reason}}", + "expr": "sum by (auth_reason) (rate(csrf_validation_failures_total[5m]))", + "legendFormat": "{{auth_reason}}", "refId": "B", "datasource": "Victoria Metrics" } diff --git a/backend/otel-collector-config.yaml b/backend/otel-collector-config.yaml index 3063e00b3..8a14b9deb 100644 --- a/backend/otel-collector-config.yaml +++ b/backend/otel-collector-config.yaml @@ -620,6 +620,164 @@ connectors: - span.name == "replay.report" gauge: value: Int(span.attributes["replay.queued_events"]) + - name: authentication.attempts + description: Password logins, by success + unit: "{attempt}" + attributes: + - key: auth.method + - key: auth.success + conditions: + - span.name == "auth.login" + sum: + value: "1" + monotonic: true + - name: authentication.failures + description: Password logins that failed + unit: "{attempt}" + attributes: + - key: auth.method + conditions: + - span.name == "auth.login" and span.attributes["auth.success"] == false + sum: + value: "1" + monotonic: true + - name: authentication.duration + description: Time to check a password login + unit: s + attributes: + - key: auth.method + conditions: + - span.name == "auth.login" + histogram: + buckets: [0.01, 0.05, 0.1, 0.25, 0.5, 1, 2.5, 5] + value: Seconds(span.end_time - span.start_time) + - name: token.validation.failures + description: Access tokens rejected, by reason + unit: "{token}" + attributes: + - key: auth.reason + conditions: + - span.name == "auth.token_rejected" + sum: + value: "1" + monotonic: true + - name: csrf.validation.failures + description: Requests rejected by the CSRF check, by reason + unit: "{request}" + attributes: + - key: auth.reason + conditions: + - span.name == "auth.csrf_rejected" + sum: + value: "1" + monotonic: true + - name: authorization.checks + description: Admin route checks, by result + unit: "{check}" + attributes: + - key: auth.resource + - key: auth.action + - key: auth.allowed + - key: auth.user_role + conditions: + - span.name == "auth.authorization" + sum: + value: "1" + monotonic: true + - name: authorization.denials + description: Admin route checks that denied access + unit: "{check}" + attributes: + - key: auth.resource + - key: auth.action + - key: auth.user_role + conditions: + - span.name == "auth.authorization" and span.attributes["auth.allowed"] == false + sum: + value: "1" + monotonic: true + - name: password.reset.requests + description: Password resets + unit: "{reset}" + attributes: + - key: auth.method + conditions: + - span.name == "auth.password_reset" + sum: + value: "1" + monotonic: true + - name: weak.password.attempts + description: Registrations refused for a weak password + unit: "{attempt}" + attributes: + - key: auth.weakness_type + conditions: + - span.name == "auth.weak_password" + sum: + value: "1" + monotonic: true + - name: brute.force.attempts + description: Logins against an account that is already locked + unit: "{attempt}" + conditions: + - span.name == "auth.locked_login" + sum: + value: "1" + monotonic: true + - name: accounts.locked + description: Accounts locked after too many failed logins + unit: "{account}" + attributes: + - key: auth.reason + conditions: + - span.name == "auth.account_locked" + sum: + value: "1" + monotonic: true + - name: rate_limit.requests + description: Requests the rate limiter checked, by algorithm + unit: "{request}" + attributes: + - key: rate_limit.endpoint + - key: rate_limit.authenticated + - key: rate_limit.algorithm + conditions: + - span.name == "rate_limit.request" + sum: + value: "1" + monotonic: true + - name: rate_limit.allowed + description: Requests a rate limit rule let through + unit: "{request}" + attributes: + - key: rate_limit.endpoint + - key: rate_limit.group + conditions: + - span.name == "rate_limit.decision" and span.attributes["rate_limit.allowed"] == true + sum: + value: "1" + monotonic: true + - name: rate_limit.rejected + description: Requests a rate limit rule rejected + unit: "{request}" + attributes: + - key: rate_limit.endpoint + - key: rate_limit.group + conditions: + - span.name == "rate_limit.decision" and span.attributes["rate_limit.allowed"] == false + sum: + value: "1" + monotonic: true + - name: rate_limit.bypass + description: Requests from users who bypass rate limits + unit: "{request}" + attributes: + - key: rate_limit.endpoint + conditions: + - span.name == "rate_limit.request" and span.attributes["rate_limit.algorithm"] == "bypassed" + sum: + value: "1" + monotonic: true processors: batch: diff --git a/backend/tests/e2e/conftest.py b/backend/tests/e2e/conftest.py index 82220b812..a623f5529 100644 --- a/backend/tests/e2e/conftest.py +++ b/backend/tests/e2e/conftest.py @@ -13,7 +13,6 @@ from httpx2 import AsyncClient from starlette.types import ASGIApp, Message, Scope -from app.core.metrics import RateLimitMetrics from app.core.security import SecurityService from app.db.docs import EventDocument, NotificationDocument from app.db.docs.saga import SagaDocument @@ -265,11 +264,6 @@ async def admin_events_repository(scope: AsyncContainer) -> AdminEventsRepositor return await scope.get(AdminEventsRepository) -@pytest.fixture -async def rate_limit_metrics(scope: AsyncContainer) -> RateLimitMetrics: - return await scope.get(RateLimitMetrics) - - @pytest.fixture def exec_request() -> ExecutionRequest: """Simple python print execution.""" diff --git a/backend/tests/e2e/services/rate_limit/test_rate_limit_service.py b/backend/tests/e2e/services/rate_limit/test_rate_limit_service.py index 12ed44895..4eb59f4c4 100644 --- a/backend/tests/e2e/services/rate_limit/test_rate_limit_service.py +++ b/backend/tests/e2e/services/rate_limit/test_rate_limit_service.py @@ -4,8 +4,8 @@ import pytest import redis.asyncio as redis import structlog +from opentelemetry import trace -from app.core.metrics import RateLimitMetrics from app.domain.rate_limit import ( EndpointGroup, RateLimitAlgorithm, @@ -20,12 +20,10 @@ @pytest.fixture -async def svc( - redis_client: redis.Redis, rate_limit_metrics: RateLimitMetrics, test_settings: Settings -) -> RateLimitService: +async def svc(redis_client: redis.Redis, test_settings: Settings) -> RateLimitService: """A service of its own, so a test's config and counters live under a prefix no other test reads.""" settings = test_settings.model_copy(update={"RATE_LIMIT_REDIS_PREFIX": f"rate_limit_test:{uuid4().hex}:"}) - return RateLimitService(redis_client, settings, rate_limit_metrics, structlog.get_logger("test")) + return RateLimitService(redis_client, settings, trace.NoOpTracer(), structlog.get_logger("test")) async def test_normalize_and_bypass_and_no_rule(svc: RateLimitService) -> None: @@ -177,7 +175,7 @@ async def test_sliding_window_pipeline_failure(test_settings: Settings) -> None: client.sadd = AsyncMock() client.pipeline = MagicMock() client.pipeline.return_value.execute = AsyncMock(side_effect=ConnectionError("Pipeline failed")) - svc = RateLimitService(client, test_settings, MagicMock(spec=RateLimitMetrics), structlog.get_logger("test")) + svc = RateLimitService(client, test_settings, trace.NoOpTracer(), structlog.get_logger("test")) rule = RateLimitRule( endpoint_pattern=r"^/api", group=EndpointGroup.API, @@ -211,7 +209,7 @@ async def test_token_bucket_invalid_data(svc: RateLimitService) -> None: async def test_update_config_serialization_error(test_settings: Settings) -> None: client = AsyncMock(spec=redis.Redis) client.set.side_effect = ValueError("Serialization failed") - svc = RateLimitService(client, test_settings, MagicMock(spec=RateLimitMetrics), structlog.get_logger("test")) + svc = RateLimitService(client, test_settings, trace.NoOpTracer(), structlog.get_logger("test")) with pytest.raises(ValueError): await svc.update_config(RateLimitConfig(default_rules=[])) @@ -230,7 +228,7 @@ async def test_user_without_override_gets_the_defaults(svc: RateLimitService) -> async def test_reset_user_limits_error(test_settings: Settings) -> None: client = AsyncMock(spec=redis.Redis) client.smembers.side_effect = ConnectionError("smembers failed") - svc = RateLimitService(client, test_settings, MagicMock(spec=RateLimitMetrics), structlog.get_logger("test")) + svc = RateLimitService(client, test_settings, trace.NoOpTracer(), structlog.get_logger("test")) with pytest.raises(ConnectionError): await svc.reset_user_limits("user") diff --git a/backend/tests/unit/conftest.py b/backend/tests/unit/conftest.py index 35f3e81db..4c1a49828 100644 --- a/backend/tests/unit/conftest.py +++ b/backend/tests/unit/conftest.py @@ -26,8 +26,6 @@ ConnectionMetrics, EventMetrics, IdempotencyMetrics, - RateLimitMetrics, - SecurityMetrics, ) from app.domain.enums import ExecutionStatus, QueuePriority from app.domain.execution import DomainExecution @@ -237,16 +235,6 @@ def event_metrics(test_settings: Settings) -> EventMetrics: return EventMetrics(test_settings) -@pytest.fixture -def rate_limit_metrics(test_settings: Settings) -> RateLimitMetrics: - return RateLimitMetrics(test_settings) - - -@pytest.fixture -def security_metrics(test_settings: Settings) -> SecurityMetrics: - return SecurityMetrics(test_settings) - - @pytest.fixture def db() -> NoReturn: raise RuntimeError("Unit tests should not access DB; use mocks, or move the test to tests/e2e") diff --git a/backend/tests/unit/core/metrics/test_replay_and_security_metrics.py b/backend/tests/unit/core/metrics/test_replay_and_security_metrics.py deleted file mode 100644 index 4a325a26a..000000000 --- a/backend/tests/unit/core/metrics/test_replay_and_security_metrics.py +++ /dev/null @@ -1,22 +0,0 @@ -from unittest.mock import MagicMock - -import pytest - -from app.core.metrics import SecurityMetrics -from app.settings import Settings - -pytestmark = pytest.mark.unit - - -def test_track_authentication_counts_a_raised_login_as_failure(test_settings: Settings) -> None: - m = SecurityMetrics(test_settings) - m.authentication_attempts = MagicMock() - m.authentication_failures = MagicMock() - m.authentication_duration = MagicMock() - - with pytest.raises(ValueError), m.track_authentication("login"): - raise ValueError("bad password") - - m.authentication_attempts.add.assert_called_once_with(1, attributes={"method": "login", "success": "False"}) - m.authentication_failures.add.assert_called_once_with(1, attributes={"method": "login"}) - m.authentication_duration.record.assert_called_once() diff --git a/backend/tests/unit/core/test_csrf.py b/backend/tests/unit/core/test_csrf.py index 20da35227..cbcdea71b 100644 --- a/backend/tests/unit/core/test_csrf.py +++ b/backend/tests/unit/core/test_csrf.py @@ -1,7 +1,7 @@ import pytest +from opentelemetry import trace from starlette.requests import Request -from app.core.metrics import SecurityMetrics from app.core.security import SecurityService from app.domain.user import CSRFValidationError from app.settings import Settings @@ -30,9 +30,9 @@ def make_request( class TestCSRFTokenGeneration: """Tests for CSRF token generation.""" - def test_generates_signed_token_format(self, test_settings: Settings, security_metrics: SecurityMetrics) -> None: + def test_generates_signed_token_format(self, test_settings: Settings, tracer: trace.Tracer) -> None: """CSRF token has nonce.signature format.""" - security = SecurityService(test_settings, security_metrics) + security = SecurityService(test_settings, tracer) token = security.generate_csrf_token("session-abc") @@ -42,9 +42,9 @@ def test_generates_signed_token_format(self, test_settings: Settings, security_m assert len(nonce) > 0 assert len(signature) == 64 # sha256 hexdigest - def test_generates_unique_tokens(self, test_settings: Settings, security_metrics: SecurityMetrics) -> None: + def test_generates_unique_tokens(self, test_settings: Settings, tracer: trace.Tracer) -> None: """Each CSRF token is unique (different nonce each time).""" - security = SecurityService(test_settings, security_metrics) + security = SecurityService(test_settings, tracer) tokens = {security.generate_csrf_token("same-session") for _ in range(100)} @@ -53,10 +53,10 @@ def test_generates_unique_tokens(self, test_settings: Settings, security_metrics def test_different_sessions_produce_different_tokens( self, test_settings: Settings, - security_metrics: SecurityMetrics, + tracer: trace.Tracer, ) -> None: """Tokens for different sessions differ even with same nonce derivation.""" - security = SecurityService(test_settings, security_metrics) + security = SecurityService(test_settings, tracer) token_a = security.generate_csrf_token("session-a") token_b = security.generate_csrf_token("session-b") @@ -67,18 +67,18 @@ def test_different_sessions_produce_different_tokens( class TestCSRFTokenValidation: """Tests for CSRF token validation (double-submit check).""" - def test_validates_matching_tokens(self, test_settings: Settings, security_metrics: SecurityMetrics) -> None: + def test_validates_matching_tokens(self, test_settings: Settings, tracer: trace.Tracer) -> None: """Matching CSRF tokens pass validation.""" - security = SecurityService(test_settings, security_metrics) + security = SecurityService(test_settings, tracer) token = security.generate_csrf_token("session-1") result = security.validate_csrf_token(token, token) assert result is True - def test_rejects_mismatched_tokens(self, test_settings: Settings, security_metrics: SecurityMetrics) -> None: + def test_rejects_mismatched_tokens(self, test_settings: Settings, tracer: trace.Tracer) -> None: """Mismatched CSRF tokens fail validation.""" - security = SecurityService(test_settings, security_metrics) + security = SecurityService(test_settings, tracer) token1 = security.generate_csrf_token("session-1") token2 = security.generate_csrf_token("session-2") @@ -99,12 +99,12 @@ def test_rejects_mismatched_tokens(self, test_settings: Settings, security_metri def test_rejects_empty_tokens( self, test_settings: Settings, - security_metrics: SecurityMetrics, + tracer: trace.Tracer, header_token: str, cookie_token: str, ) -> None: """Empty CSRF tokens fail validation.""" - security = SecurityService(test_settings, security_metrics) + security = SecurityService(test_settings, tracer) result = security.validate_csrf_token(header_token, cookie_token) @@ -114,29 +114,29 @@ def test_rejects_empty_tokens( class TestCSRFSignatureVerification: """Tests for CSRF HMAC signature verification.""" - def test_valid_signature_passes(self, test_settings: Settings, security_metrics: SecurityMetrics) -> None: + def test_valid_signature_passes(self, test_settings: Settings, tracer: trace.Tracer) -> None: """Token verified against the same session_id succeeds.""" - security = SecurityService(test_settings, security_metrics) + security = SecurityService(test_settings, tracer) token = security.generate_csrf_token("my-session") assert security._verify_csrf_signature(token, "my-session") is True - def test_wrong_session_rejected(self, test_settings: Settings, security_metrics: SecurityMetrics) -> None: + def test_wrong_session_rejected(self, test_settings: Settings, tracer: trace.Tracer) -> None: """Token signed for session A fails verification against session B.""" - security = SecurityService(test_settings, security_metrics) + security = SecurityService(test_settings, tracer) token = security.generate_csrf_token("session-a") assert security._verify_csrf_signature(token, "session-b") is False - def test_unsigned_token_rejected(self, test_settings: Settings, security_metrics: SecurityMetrics) -> None: + def test_unsigned_token_rejected(self, test_settings: Settings, tracer: trace.Tracer) -> None: """A plain random string without signature structure is rejected.""" - security = SecurityService(test_settings, security_metrics) + security = SecurityService(test_settings, tracer) assert security._verify_csrf_signature("plain-random-token", "session") is False - def test_tampered_signature_rejected(self, test_settings: Settings, security_metrics: SecurityMetrics) -> None: + def test_tampered_signature_rejected(self, test_settings: Settings, tracer: trace.Tracer) -> None: """Modifying the signature portion causes rejection.""" - security = SecurityService(test_settings, security_metrics) + security = SecurityService(test_settings, tracer) token = security.generate_csrf_token("session-x") nonce, sig = token.split(".", 1) @@ -151,23 +151,23 @@ class TestCSRFExemptPaths: def test_exempt_paths_includes_login_and_register( self, test_settings: Settings, - security_metrics: SecurityMetrics, + tracer: trace.Tracer, ) -> None: """CSRF exempt paths include login and register.""" - security = SecurityService(test_settings, security_metrics) + security = SecurityService(test_settings, tracer) assert "/api/v1/auth/login" in security.CSRF_EXEMPT_PATHS assert "/api/v1/auth/register" in security.CSRF_EXEMPT_PATHS - def test_logout_is_not_exempt(self, test_settings: Settings, security_metrics: SecurityMetrics) -> None: + def test_logout_is_not_exempt(self, test_settings: Settings, tracer: trace.Tracer) -> None: """Logout is NOT exempt from CSRF validation.""" - security = SecurityService(test_settings, security_metrics) + security = SecurityService(test_settings, tracer) assert "/api/v1/auth/logout" not in security.CSRF_EXEMPT_PATHS - def test_exempt_paths_is_frozenset(self, test_settings: Settings, security_metrics: SecurityMetrics) -> None: + def test_exempt_paths_is_frozenset(self, test_settings: Settings, tracer: trace.Tracer) -> None: """CSRF exempt paths is a frozenset (immutable).""" - security = SecurityService(test_settings, security_metrics) + security = SecurityService(test_settings, tracer) assert isinstance(security.CSRF_EXEMPT_PATHS, frozenset) @@ -175,9 +175,9 @@ def test_exempt_paths_is_frozenset(self, test_settings: Settings, security_metri class TestCSRFRequestValidation: """Tests for CSRF validation from HTTP requests.""" - def test_skips_get_requests(self, test_settings: Settings, security_metrics: SecurityMetrics) -> None: + def test_skips_get_requests(self, test_settings: Settings, tracer: trace.Tracer) -> None: """GET requests skip CSRF validation.""" - security = SecurityService(test_settings, security_metrics) + security = SecurityService(test_settings, tracer) req = make_request("GET", "/api/v1/anything") assert security.validate_csrf_from_request(req) == "skip" @@ -185,10 +185,10 @@ def test_skips_get_requests(self, test_settings: Settings, security_metrics: Sec def test_missing_header_raises_when_authenticated( self, test_settings: Settings, - security_metrics: SecurityMetrics, + tracer: trace.Tracer, ) -> None: """Missing CSRF header raises error for authenticated POST.""" - security = SecurityService(test_settings, security_metrics) + security = SecurityService(test_settings, tracer) req = make_request( "POST", "/api/v1/items", @@ -198,9 +198,9 @@ def test_missing_header_raises_when_authenticated( with pytest.raises(CSRFValidationError): security.validate_csrf_from_request(req) - def test_valid_tokens_pass(self, test_settings: Settings, security_metrics: SecurityMetrics) -> None: + def test_valid_tokens_pass(self, test_settings: Settings, tracer: trace.Tracer) -> None: """Valid signed CSRF tokens pass full request validation.""" - security = SecurityService(test_settings, security_metrics) + security = SecurityService(test_settings, tracer) access_token = "my-access-token-value" token = security.generate_csrf_token(access_token) req = make_request( @@ -212,9 +212,9 @@ def test_valid_tokens_pass(self, test_settings: Settings, security_metrics: Secu assert security.validate_csrf_from_request(req) == token - def test_forged_token_rejected(self, test_settings: Settings, security_metrics: SecurityMetrics) -> None: + def test_forged_token_rejected(self, test_settings: Settings, tracer: trace.Tracer) -> None: """Unsigned token matching in header+cookie is rejected (signature check).""" - security = SecurityService(test_settings, security_metrics) + security = SecurityService(test_settings, tracer) forged = "forged-random-value" req = make_request( "POST", @@ -226,9 +226,9 @@ def test_forged_token_rejected(self, test_settings: Settings, security_metrics: with pytest.raises(CSRFValidationError, match="signature invalid"): security.validate_csrf_from_request(req) - def test_wrong_session_token_rejected(self, test_settings: Settings, security_metrics: SecurityMetrics) -> None: + def test_wrong_session_token_rejected(self, test_settings: Settings, tracer: trace.Tracer) -> None: """Token signed for one session rejected when presented with different access_token.""" - security = SecurityService(test_settings, security_metrics) + security = SecurityService(test_settings, tracer) token = security.generate_csrf_token("session-A-jwt") req = make_request( "POST", @@ -240,9 +240,9 @@ def test_wrong_session_token_rejected(self, test_settings: Settings, security_me with pytest.raises(CSRFValidationError, match="signature invalid"): security.validate_csrf_from_request(req) - def test_logout_requires_csrf(self, test_settings: Settings, security_metrics: SecurityMetrics) -> None: + def test_logout_requires_csrf(self, test_settings: Settings, tracer: trace.Tracer) -> None: """POST to logout with authentication requires CSRF token.""" - security = SecurityService(test_settings, security_metrics) + security = SecurityService(test_settings, tracer) req = make_request( "POST", "/api/v1/auth/logout", diff --git a/backend/tests/unit/core/test_rate_limit_middleware.py b/backend/tests/unit/core/test_rate_limit_middleware.py index 5c07d27fa..2b33d5aa9 100644 --- a/backend/tests/unit/core/test_rate_limit_middleware.py +++ b/backend/tests/unit/core/test_rate_limit_middleware.py @@ -3,10 +3,10 @@ import pytest from dishka import AsyncContainer +from opentelemetry import trace from starlette.responses import PlainTextResponse from starlette.types import Message, Receive, Scope, Send -from app.core.metrics import SecurityMetrics from app.core.middlewares import RateLimitMiddleware from app.core.security import SecurityService from app.domain.rate_limit import RateLimitStatus @@ -80,19 +80,17 @@ def _middleware(service: AsyncMock, security: SecurityService, settings: Setting ], ) async def test_only_health_and_auth_routes_skip_rate_limiting( - test_settings: Settings, security_metrics: SecurityMetrics, path: str, expected_status: int, checked: bool + test_settings: Settings, tracer: trace.Tracer, path: str, expected_status: int, checked: bool ) -> None: service = _rate_limit_service(allowed=False) - middleware = _middleware(service, SecurityService(test_settings, security_metrics), test_settings) + middleware = _middleware(service, SecurityService(test_settings, tracer), test_settings) assert await _request(middleware, path) == expected_status assert service.check_rate_limit.await_count == int(checked) -async def test_a_valid_token_keys_the_request_by_user_id( - test_settings: Settings, security_metrics: SecurityMetrics -) -> None: - security = SecurityService(test_settings, security_metrics) +async def test_a_valid_token_keys_the_request_by_user_id(test_settings: Settings, tracer: trace.Tracer) -> None: + security = SecurityService(test_settings, tracer) token = security.create_access_token({"sub": "u", "user_id": "user-1"}, expires_delta=timedelta(minutes=5)) service = _rate_limit_service(allowed=True) @@ -104,10 +102,10 @@ async def test_a_valid_token_keys_the_request_by_user_id( @pytest.mark.parametrize("cookies", ["access_token=forged.token.value", ""], ids=["forged_token", "anonymous"]) async def test_other_requests_are_keyed_by_client_ip( - test_settings: Settings, security_metrics: SecurityMetrics, cookies: str + test_settings: Settings, tracer: trace.Tracer, cookies: str ) -> None: service = _rate_limit_service(allowed=True) - middleware = _middleware(service, SecurityService(test_settings, security_metrics), test_settings) + middleware = _middleware(service, SecurityService(test_settings, tracer), test_settings) assert await _request(middleware, "/api/v1/execute", cookies) == 200 service.check_rate_limit.assert_awaited_once_with(user_id="ip:203.0.113.7", endpoint="/api/v1/execute") diff --git a/backend/tests/unit/core/test_security.py b/backend/tests/unit/core/test_security.py index 961cce4ae..bc4aae1d6 100644 --- a/backend/tests/unit/core/test_security.py +++ b/backend/tests/unit/core/test_security.py @@ -1,12 +1,12 @@ import time from datetime import datetime, timedelta, timezone -from unittest.mock import MagicMock from uuid import uuid4 import jwt import pytest +from opentelemetry import trace +from opentelemetry.sdk.trace.export.in_memory_span_exporter import InMemorySpanExporter -from app.core.metrics import SecurityMetrics from app.core.security import SecurityService from app.domain.enums import UserRole from app.domain.user import InvalidCredentialsError @@ -17,9 +17,9 @@ class TestPasswordHashing: """Test password hashing functionality.""" @pytest.fixture - def security_svc(self, test_settings: Settings, security_metrics: SecurityMetrics) -> SecurityService: + def security_svc(self, test_settings: Settings, tracer: trace.Tracer) -> SecurityService: """Create SecurityService instance.""" - return SecurityService(test_settings, security_metrics) + return SecurityService(test_settings, tracer) def test_password_hash_creates_different_hash(self, security_svc: SecurityService) -> None: """Test that password hashing creates unique hashes.""" @@ -74,9 +74,9 @@ class TestSecurityService: """Test SecurityService functionality.""" @pytest.fixture - def security_service(self, test_settings: Settings, security_metrics: SecurityMetrics) -> SecurityService: + def security_service(self, test_settings: Settings, tracer: trace.Tracer) -> SecurityService: """Create SecurityService instance using test settings.""" - return SecurityService(test_settings, security_metrics) + return SecurityService(test_settings, tracer) def test_create_access_token_basic(self, security_service: SecurityService) -> None: """Test basic access token creation.""" @@ -172,8 +172,8 @@ class TestDecodeToken: """Test SecurityService.decode_token method.""" @pytest.fixture - def security_service(self, test_settings: Settings, security_metrics: SecurityMetrics) -> SecurityService: - return SecurityService(test_settings, security_metrics) + def security_service(self, test_settings: Settings, tracer: trace.Tracer) -> SecurityService: + return SecurityService(test_settings, tracer) def test_valid_token_returns_username(self, security_service: SecurityService) -> None: token = security_service.create_access_token({"sub": "testuser"}, expires_delta=timedelta(minutes=15)) @@ -225,9 +225,10 @@ def test_rejected_token_records_reason( lifetime: timedelta, key: str | None, reason: str, + tracer: trace.Tracer, + span_exporter: InMemorySpanExporter, ) -> None: - metrics = MagicMock(spec=SecurityMetrics) - svc = SecurityService(test_settings, metrics) + svc = SecurityService(test_settings, tracer) token = ( "not.a.token" if claims is None @@ -241,22 +242,24 @@ def test_rejected_token_records_reason( with pytest.raises(InvalidCredentialsError): svc.decode_token(token) - metrics.record_token_validation_failure.assert_called_once_with(reason) + [span] = span_exporter.get_finished_spans() + assert (span.name, span.attributes) == ("auth.token_rejected", {"auth.reason": reason}) - def test_valid_token_records_no_failure(self, test_settings: Settings) -> None: - metrics = MagicMock(spec=SecurityMetrics) - svc = SecurityService(test_settings, metrics) + def test_valid_token_records_no_failure( + self, test_settings: Settings, tracer: trace.Tracer, span_exporter: InMemorySpanExporter + ) -> None: + svc = SecurityService(test_settings, tracer) svc.decode_token(svc.create_access_token({"sub": "u"}, expires_delta=timedelta(minutes=5))) - metrics.record_token_validation_failure.assert_not_called() + assert span_exporter.get_finished_spans() == () class TestDecodeUserId: """SecurityService.decode_user_id gives an id only for a valid token that carries one.""" - def test_valid_token_returns_user_id(self, test_settings: Settings, security_metrics: SecurityMetrics) -> None: - svc = SecurityService(test_settings, security_metrics) + def test_valid_token_returns_user_id(self, test_settings: Settings, tracer: trace.Tracer) -> None: + svc = SecurityService(test_settings, tracer) token = svc.create_access_token({"sub": "u", "user_id": "user-1"}, expires_delta=timedelta(minutes=5)) assert svc.decode_user_id(token) == "user-1" @@ -272,10 +275,15 @@ def test_valid_token_returns_user_id(self, test_settings: Settings, security_met ids=["expired", "wrong_signature", "no_user_id", "malformed"], ) def test_unusable_token_raises_without_counting_a_failure( - self, test_settings: Settings, claims: dict[str, object] | None, lifetime: timedelta, key: str | None + self, + test_settings: Settings, + tracer: trace.Tracer, + span_exporter: InMemorySpanExporter, + claims: dict[str, object] | None, + lifetime: timedelta, + key: str | None, ) -> None: - metrics = MagicMock(spec=SecurityMetrics) - svc = SecurityService(test_settings, metrics) + svc = SecurityService(test_settings, tracer) token = ( "not.a.token" if claims is None @@ -288,4 +296,4 @@ def test_unusable_token_raises_without_counting_a_failure( with pytest.raises(InvalidCredentialsError): svc.decode_user_id(token) - metrics.record_token_validation_failure.assert_not_called() + assert span_exporter.get_finished_spans() == () diff --git a/backend/tests/unit/services/test_auth_service_metrics.py b/backend/tests/unit/services/test_auth_service_metrics.py index 4d7db09e6..d0fe021a7 100644 --- a/backend/tests/unit/services/test_auth_service_metrics.py +++ b/backend/tests/unit/services/test_auth_service_metrics.py @@ -2,11 +2,13 @@ import pytest import structlog +from opentelemetry import trace +from opentelemetry.sdk.trace.export.in_memory_span_exporter import InMemorySpanExporter -from app.core.metrics import SecurityMetrics from app.core.security import SecurityService from app.db import UserRepository from app.domain.exceptions import AccountLockedError +from app.domain.user import InvalidCredentialsError from app.events import UnifiedProducer from app.services.auth_service import AuthService from app.services.login_lockout import LoginLockoutService @@ -16,15 +18,11 @@ pytestmark = pytest.mark.unit -async def test_login_on_locked_account_counts_a_blocked_attempt(test_settings: Settings) -> None: - metrics = MagicMock(spec=SecurityMetrics) - lockout = AsyncMock(spec=LoginLockoutService) - lockout.check_locked.return_value = True - user_repo = AsyncMock(spec=UserRepository) - service = AuthService( +def _service(test_settings: Settings, tracer: trace.Tracer, lockout: AsyncMock, user_repo: AsyncMock) -> AuthService: + return AuthService( user_repo=user_repo, security_service=MagicMock(spec=SecurityService), - security_metrics=metrics, + tracer=tracer, logger=structlog.get_logger("test.auth_service_metrics"), lockout_service=lockout, runtime_settings=AsyncMock(spec=RuntimeSettingsLoader), @@ -32,9 +30,34 @@ async def test_login_on_locked_account_counts_a_blocked_attempt(test_settings: S settings=test_settings, ) + +async def test_login_on_locked_account_counts_a_blocked_attempt( + test_settings: Settings, tracer: trace.Tracer, span_exporter: InMemorySpanExporter +) -> None: + lockout = AsyncMock(spec=LoginLockoutService) + lockout.check_locked.return_value = True + user_repo = AsyncMock(spec=UserRepository) + service = _service(test_settings, tracer, lockout, user_repo) + with pytest.raises(AccountLockedError): await service.login("alice", "secret", ip_address="10.0.0.1", user_agent=None) - metrics.record_brute_force_attempt.assert_called_once_with() - metrics.track_authentication.assert_not_called() + assert [span.name for span in span_exporter.get_finished_spans()] == ["auth.locked_login"] user_repo.get_user.assert_not_awaited() + + +async def test_failed_login_reports_an_unsuccessful_attempt( + test_settings: Settings, tracer: trace.Tracer, span_exporter: InMemorySpanExporter +) -> None: + lockout = AsyncMock(spec=LoginLockoutService) + lockout.check_locked.return_value = False + lockout.record_failed_attempt.return_value = False + user_repo = AsyncMock(spec=UserRepository) + user_repo.get_user.return_value = None + service = _service(test_settings, tracer, lockout, user_repo) + + with pytest.raises(InvalidCredentialsError): + await service.login("alice", "secret", ip_address="10.0.0.1", user_agent=None) + + [span] = span_exporter.get_finished_spans() + assert (span.name, span.attributes) == ("auth.login", {"auth.method": "login", "auth.success": False}) diff --git a/backend/tests/unit/services/test_rate_limit_service.py b/backend/tests/unit/services/test_rate_limit_service.py index 7dcfd184c..cee371e2d 100644 --- a/backend/tests/unit/services/test_rate_limit_service.py +++ b/backend/tests/unit/services/test_rate_limit_service.py @@ -1,10 +1,11 @@ -from unittest.mock import AsyncMock, MagicMock +from unittest.mock import AsyncMock import pytest import redis.asyncio as redis import structlog +from opentelemetry import trace +from opentelemetry.sdk.trace.export.in_memory_span_exporter import InMemorySpanExporter -from app.core.metrics import RateLimitMetrics from app.services.rate_limit_service import RateLimitService from app.settings import Settings @@ -12,13 +13,22 @@ @pytest.mark.parametrize("user_id", ["user-1", "ip:203.0.113.7"]) -async def test_redis_failure_lets_the_request_through(test_settings: Settings, user_id: str) -> None: +async def test_redis_failure_lets_the_request_through( + test_settings: Settings, tracer: trace.Tracer, span_exporter: InMemorySpanExporter, user_id: str +) -> None: client = AsyncMock(spec=redis.Redis) client.get.side_effect = redis.ConnectionError("Redis is down") - metrics = MagicMock(spec=RateLimitMetrics) - service = RateLimitService(client, test_settings, metrics, structlog.get_logger("test")) + service = RateLimitService(client, test_settings, tracer, structlog.get_logger("test")) status = await service.check_rate_limit(user_id, "/api/v1/execute") assert status.allowed is True - metrics.record_request.assert_called_once_with("/api/v1/execute", not user_id.startswith("ip:"), "redis_error") + [span] = span_exporter.get_finished_spans() + assert (span.name, span.attributes) == ( + "rate_limit.request", + { + "rate_limit.endpoint": "/api/v1/execute", + "rate_limit.authenticated": not user_id.startswith("ip:"), + "rate_limit.algorithm": "redis_error", + }, + ) diff --git a/docs/operations/metrics-reference.md b/docs/operations/metrics-reference.md index 5167a16b1..66d27dfa9 100644 --- a/docs/operations/metrics-reference.md +++ b/docs/operations/metrics-reference.md @@ -24,7 +24,7 @@ The collector's metrics pipeline (`backend/otel-collector-config.yaml`) reads th ## Metrics made from spans -Some metrics have no instrument in the code. A service puts the facts on a span, and the collector's `signaltometrics` connector (alpha in collector v0.162.0) turns them into counters, histograms and gauges, defined under `connectors` in `backend/otel-collector-config.yaml`. The SDK sends every span (`TRACING_SAMPLING_RATE = 1.0`), and the traces pipeline gives each span to the connector before `tail_sampling` decides what Jaeger keeps, so the counts don't depend on sampling. The connector emits a point only when a matching span arrives. Sent straight to VictoriaMetrics, a counter that sees one event every half hour would get one point every half hour, and `increase` and `rate` stop finding the previous point once two points are more than about 8 to 10 minutes apart (measured on v1.153.0): `increase` then counts the whole total again, and `rate` returns nothing. So counters and histograms go another way. `deltatocumulative` adds up the connector's deltas and keeps each total through 24 hours without spans (its default is 5 minutes, after which an idle counter would start again from zero). The collector's `prometheus` exporter serves the totals on `otel-collector:8889`, and VictoriaMetrics scrapes it every 15 seconds (`backend/victoria-metrics/scrape.yaml`, with `honor_labels` so each service's `job` and `instance` stay). A series the exporter hasn't updated for 24 hours drops out until its next span. Gauges still go by remote write, so a gauge goes stale when the job that reports it on a timer stops. Panels wrap counter rates in `or vector(0)` so a series that hasn't started yet reads 0. The execution, pod, queue, dead letter queue, notification and replay metrics work this way so far ([#389](https://github.com/HardMax71/Integr8sCode/issues/389)). +Some metrics have no instrument in the code. A service puts the facts on a span, and the collector's `signaltometrics` connector (alpha in collector v0.162.0) turns them into counters, histograms and gauges, defined under `connectors` in `backend/otel-collector-config.yaml`. The SDK sends every span (`TRACING_SAMPLING_RATE = 1.0`), and the traces pipeline gives each span to the connector before `tail_sampling` decides what Jaeger keeps, so the counts don't depend on sampling. The connector emits a point only when a matching span arrives. Sent straight to VictoriaMetrics, a counter that sees one event every half hour would get one point every half hour, and `increase` and `rate` stop finding the previous point once two points are more than about 8 to 10 minutes apart (measured on v1.153.0): `increase` then counts the whole total again, and `rate` returns nothing. So counters and histograms go another way. `deltatocumulative` adds up the connector's deltas and keeps each total through 24 hours without spans (its default is 5 minutes, after which an idle counter would start again from zero). The collector's `prometheus` exporter serves the totals on `otel-collector:8889`, and VictoriaMetrics scrapes it every 15 seconds (`backend/victoria-metrics/scrape.yaml`, with `honor_labels` so each service's `job` and `instance` stay). A series the exporter hasn't updated for 24 hours drops out until its next span. Gauges still go by remote write, so a gauge goes stale when the job that reports it on a timer stops. Panels wrap counter rates in `or vector(0)` so a series that hasn't started yet reads 0. The execution, pod, queue, dead letter queue, notification, replay, security and rate limit metrics work this way so far ([#389](https://github.com/HardMax71/Integr8sCode/issues/389)). ## Names and labels in VictoriaMetrics @@ -156,30 +156,32 @@ The status snapshot a stream sends first comes from the database, not Redis, and ### Authentication and authorization (backend) -| Metric | Type | Labels | Recorded in | -|--------|------|--------|-------------| -| `authentication_attempts_total` | counter | method, success | `AuthService.login` through `track_authentication`; method is `login` | -| `authentication_failures_total` | counter | method | the same | -| `authentication_duration_seconds` | histogram | method | the same | -| `accounts_locked_total` | counter | reason: brute_force | `AuthService`, when a failed login triggers the lockout | -| `brute_force_attempts_total` | counter | none | `AuthService.login`, a login against an account that is already locked | -| `weak_password_attempts_total` | counter | weakness_type: too_short | `AuthService.register` | -| `token_validation_failures_total` | counter | reason: expired, invalid_signature, invalid, missing_subject | `SecurityService.decode_token` | -| `csrf_validation_failures_total` | counter | reason: missing_header, token_mismatch, invalid_signature | `SecurityService.validate_csrf_from_request` | -| `authorization_checks_total` | counter | resource, action, allowed, user_role | `AuthService.get_admin`, on every admin route | -| `authorization_denials_total` | counter | resource, action, user_role | the same | -| `password_reset_requests_total` | counter | method: admin | `AdminUserService` | +Each login runs in an `auth.login` span; the other events leave a span named in the table where the code used to record the metric. + +| Metric | Type | Labels | Recorded from | +|--------|------|--------|---------------| +| `authentication_attempts_total` | counter | auth_method, auth_success | span `auth.login` from `AuthService.login`; the method is `login` | +| `authentication_failures_total` | counter | auth_method | the same span with `auth.success` false | +| `authentication_duration_seconds` | histogram | auth_method | the length of that span | +| `accounts_locked_total` | counter | auth_reason: brute_force | span `auth.account_locked`, when a failed login triggers the lockout | +| `brute_force_attempts_total` | counter | none | span `auth.locked_login`, a login against an account that is already locked | +| `weak_password_attempts_total` | counter | auth_weakness_type: too_short | span `auth.weak_password` from `AuthService.register` | +| `token_validation_failures_total` | counter | auth_reason: expired, invalid_signature, invalid, missing_subject | span `auth.token_rejected` from `SecurityService.decode_token` | +| `csrf_validation_failures_total` | counter | auth_reason: missing_header, token_mismatch, invalid_signature | span `auth.csrf_rejected` from `SecurityService.validate_csrf_from_request` | +| `authorization_checks_total` | counter | auth_resource, auth_action, auth_allowed, auth_user_role | span `auth.authorization` from `AuthService.get_admin`, on every admin route | +| `authorization_denials_total` | counter | auth_resource, auth_action, auth_user_role | the same span with `auth.allowed` false | +| `password_reset_requests_total` | counter | auth_method: admin | span `auth.password_reset` from `AdminUserService` | ### Rate limiting (backend) -`RateLimitService.check_rate_limit`, called from `RateLimitMiddleware`. `endpoint` is the path with UUIDs and any segment of 20 or more letters and digits replaced by `*`. Shorter IDs such as `/42` stay as they are. +`RateLimitService.check_rate_limit`, called from `RateLimitMiddleware`, leaves a `rate_limit.request` span for each check and a `rate_limit.decision` span when a rule applies. `rate_limit_endpoint` is the path with UUIDs and any segment of 20 or more letters and digits replaced by `*`. Shorter IDs such as `/42` stay as they are. -| Metric | Type | Labels | Notes | -|--------|------|--------|-------| -| `rate_limit_requests_total` | counter | endpoint, authenticated, algorithm | the matched rule's algorithm, or `bypassed` and `no_limit` | -| `rate_limit_allowed_total` | counter | endpoint, group | | -| `rate_limit_rejected_total` | counter | endpoint, group | | -| `rate_limit_bypass_total` | counter | endpoint | users with `bypass_rate_limit` | +| Metric | Type | Labels | Recorded from | +|--------|------|--------|---------------| +| `rate_limit_requests_total` | counter | rate_limit_endpoint, rate_limit_authenticated, rate_limit_algorithm | `rate_limit.request`: the matched rule's algorithm, or `bypassed`, `no_limit` and `redis_error` | +| `rate_limit_allowed_total` | counter | rate_limit_endpoint, rate_limit_group | `rate_limit.decision` with `rate_limit.allowed` true | +| `rate_limit_rejected_total` | counter | rate_limit_endpoint, rate_limit_group | the same span with `rate_limit.allowed` false | +| `rate_limit_bypass_total` | counter | rate_limit_endpoint | `rate_limit.request` with algorithm `bypassed`: users with `bypass_rate_limit` | ### Dead letter queue @@ -246,7 +248,7 @@ sum(rate(rate_limit_rejected_total[5m])) / sum(rate(rate_limit_requests_total[5m sum by (messaging_destination_name) (rate(messaging_publish_messages_message_total[5m])) # rejected access tokens by reason -sum by (reason) (rate(token_validation_failures_total[5m])) +sum by (auth_reason) (rate(token_validation_failures_total[5m])) # MongoDB cache hit ratio sum(rate(mongodb_cache_operations_total{type="hit"}[5m])) / sum(rate(mongodb_cache_operations_total[5m]))