diff --git a/api/changelog.d/12210.fixed.md b/api/changelog.d/12210.fixed.md new file mode 100644 index 0000000000..fd5dce268d --- /dev/null +++ b/api/changelog.d/12210.fixed.md @@ -0,0 +1 @@ +Authentication with an API key whose owning user was deleted now returns `401` instead of an unhandled `AttributeError`, and user deletion now revokes the user's API keys across all their tenants diff --git a/api/src/backend/api/authentication.py b/api/src/backend/api/authentication.py index 755bd64e39..af1a35c7c0 100644 --- a/api/src/backend/api/authentication.py +++ b/api/src/backend/api/authentication.py @@ -1,3 +1,4 @@ +import logging from math import isfinite from uuid import UUID @@ -5,6 +6,7 @@ from api.db_router import MainRouter from api.models import TenantAPIKey, TenantAPIKeyManager from cryptography.fernet import InvalidToken from django.core.exceptions import ObjectDoesNotExist +from django.db import transaction from django.utils import timezone from drf_simple_apikey.backends import APIKeyAuthentication as BaseAPIKeyAuth from drf_simple_apikey.crypto import get_crypto @@ -14,6 +16,16 @@ from rest_framework.exceptions import AuthenticationFailed from rest_framework.request import Request from rest_framework_simplejwt.authentication import JWTAuthentication +logger = logging.getLogger(__name__) + + +class OrphanedAPIKeyError(Exception): + """Raised when an API key outlived the user that owns it. + + Handled by `authenticate`, which commits the revocation written while detecting it + and then rejects the request with `AuthenticationFailed`. + """ + class TenantAPIKeyAuthentication(BaseAPIKeyAuth): model = TenantAPIKey @@ -24,10 +36,13 @@ class TenantAPIKeyAuthentication(BaseAPIKeyAuth): def _authenticate_credentials(self, request, key): """ Override to use admin connection, bypassing RLS during authentication. + + Returns the validated API key row, locked with `select_for_update`, so callers + must run inside `transaction.atomic(using=MainRouter.admin_db)`. """ try: payload = self.key_crypto.decrypt(key) - except ValueError: + except (ValueError, InvalidToken): raise AuthenticationFailed("Invalid API Key.") if not isinstance(payload, dict): @@ -52,13 +67,33 @@ class TenantAPIKeyAuthentication(BaseAPIKeyAuth): raise AuthenticationFailed("API Key has already expired.") try: - api_key = self.model.objects.using(MainRouter.admin_db).get(id=api_key_pk) + api_key = ( + self.model.objects.using(MainRouter.admin_db) + .select_for_update() + .get(id=api_key_pk) + ) except ObjectDoesNotExist: raise AuthenticationFailed("No entity matching this api key.") if api_key.revoked: raise AuthenticationFailed("This API Key has been revoked.") + # `entity` is nullable and `on_delete=SET_NULL` leaves the key behind when its + # owner is deleted, so a key can outlive its user. Reject it here: further down + # the authentication would return `None` as the authenticated user, which blows + # up while building the auth dict and surfaces as a 500 instead of a 401. + # Revoke it as well, so it stops showing up as active and later attempts fail + # the `revoked` check above like any other revoked key. + if api_key.entity_id is None: + api_key.revoked = True + api_key.save(update_fields=["revoked"], using=MainRouter.admin_db) + logger.warning( + "Revoked orphaned API key: prefix=%s tenant=%s", + api_key.prefix, + api_key.tenant_id, + ) + raise OrphanedAPIKeyError + client_ip = request.META.get(package_settings.IP_ADDRESS_HEADER) if api_key.blacklisted_ips and client_ip in api_key.blacklisted_ips: raise AuthenticationFailed("Access denied from blacklisted IP.") @@ -66,7 +101,7 @@ class TenantAPIKeyAuthentication(BaseAPIKeyAuth): if api_key.whitelisted_ips and client_ip not in api_key.whitelisted_ips: raise AuthenticationFailed("Access restricted to specific IP addresses.") - return api_key.entity, key + return api_key def authenticate(self, request: Request): prefixed_key = self.get_key(request) @@ -77,36 +112,34 @@ class TenantAPIKeyAuthentication(BaseAPIKeyAuth): except ValueError: raise AuthenticationFailed("Invalid API Key.") - try: - entity, _ = self._authenticate_credentials(request, key) - except InvalidToken: - raise AuthenticationFailed("Invalid API Key.") + # Validation, the `last_used_at` update and the auth claims all read the same + # row, locked until the transaction ends. Looking the key up a second time to + # build the claims used to leave a window where a key revoked or orphaned right + # after passing validation still authenticated. + with transaction.atomic(using=MainRouter.admin_db): + try: + api_key = self._authenticate_credentials(request, key) + except OrphanedAPIKeyError: + # Rejected below instead of here: leaving the block normally commits + # the revocation `_authenticate_credentials` wrote, while raising from + # inside would roll it back. + pass + else: + # The prefix used to be checked by the second lookup + if api_key.prefix != prefix: + raise AuthenticationFailed("Invalid API Key.") - # Get the API key instance to update last_used_at and retrieve tenant info - # We need to decrypt again to get the pk (already validated by _authenticate_credentials) - payload = self.key_crypto.decrypt(key) - api_key_pk = payload["_pk"] + api_key.last_used_at = timezone.now() + api_key.save(update_fields=["last_used_at"], using=MainRouter.admin_db) - # Convert string UUID back to UUID object for lookup - if isinstance(api_key_pk, str): - api_key_pk = UUID(api_key_pk) + entity = api_key.entity + return entity, { + "tenant_id": str(api_key.tenant_id), + "sub": str(entity.id), + "api_key_prefix": api_key.prefix, + } - try: - api_key_instance = TenantAPIKey.objects.using(MainRouter.admin_db).get( - id=api_key_pk, prefix=prefix - ) - except TenantAPIKey.DoesNotExist: - raise AuthenticationFailed("Invalid API Key.") - - # Update last_used_at - api_key_instance.last_used_at = timezone.now() - api_key_instance.save(update_fields=["last_used_at"], using=MainRouter.admin_db) - - return entity, { - "tenant_id": str(api_key_instance.tenant_id), - "sub": str(api_key_instance.entity.id), - "api_key_prefix": prefix, - } + raise AuthenticationFailed("No entity matching this api key.") class CombinedJWTOrAPIKeyAuthentication(BaseAuthentication): diff --git a/api/src/backend/api/signals.py b/api/src/backend/api/signals.py index 790779f087..d6b35a5b95 100644 --- a/api/src/backend/api/signals.py +++ b/api/src/backend/api/signals.py @@ -1,3 +1,4 @@ +from api.db_router import MainRouter from api.db_utils import delete_related_daily_task from api.models import ( LighthouseProviderConfiguration, @@ -47,8 +48,15 @@ def revoke_user_api_keys(sender, instance, **kwargs): # noqa: F841 The entity field will be set to NULL by on_delete=SET_NULL, but we explicitly revoke the keys to prevent further use. + + The update runs on the admin connection because `api_keys` is RLS protected and its + policy denies every row when `api.tenant_id` is unset. Users are deleted through the + admin connection and may belong to several tenants, so going through the default + connection would silently revoke nothing, or only the keys of the active tenant. """ - TenantAPIKey.objects.filter(entity=instance).update(revoked=True) + TenantAPIKey.objects.using(MainRouter.admin_db).filter(entity=instance).update( + revoked=True + ) @receiver(post_delete, sender=Membership) @@ -58,8 +66,12 @@ def revoke_membership_api_keys(sender, instance, **kwargs): # noqa: F841 When a membership is deleted, all API keys created by that user in that tenant should be revoked to prevent further access. + + Uses the admin connection for the same reason as `revoke_user_api_keys`: the RLS + policy on `api_keys` denies every row when `api.tenant_id` is unset, which is the + case when the membership is removed as a cascade of a user deletion. """ - TenantAPIKey.objects.filter( + TenantAPIKey.objects.using(MainRouter.admin_db).filter( entity_id=instance.user_id, tenant_id=instance.tenant_id ).update(revoked=True) diff --git a/api/src/backend/api/tests/integration/test_authentication.py b/api/src/backend/api/tests/integration/test_authentication.py index 67f498b238..923324564e 100644 --- a/api/src/backend/api/tests/integration/test_authentication.py +++ b/api/src/backend/api/tests/integration/test_authentication.py @@ -4,8 +4,11 @@ from datetime import UTC, datetime, timedelta from uuid import uuid4 import pytest +from api.db_router import MainRouter from api.models import Membership, Role, TenantAPIKey, User, UserRoleRelationship +from api.signals import revoke_membership_api_keys, revoke_user_api_keys from conftest import TEST_PASSWORD, get_api_tokens, get_authorization_header +from django.db.utils import ConnectionDoesNotExist from django.urls import reverse from drf_simple_apikey.crypto import get_crypto from rest_framework.test import APIClient @@ -625,6 +628,34 @@ class TestAPIKeyErrors: assert response.status_code == 401 assert "API Key has been revoked." in response.json()["errors"][0]["detail"] + def test_orphaned_api_key_rejected( + self, create_test_user, tenants_fixture, api_keys_fixture + ): + """Key whose owning user was deleted returns 401 instead of 500.""" + client = APIClient() + + api_key = api_keys_fixture[0] + # `on_delete=SET_NULL` leaves the key behind with no entity when the owner goes + TenantAPIKey.objects.filter(id=api_key.id).update(entity=None) + + api_key_headers = get_api_key_header(api_key._raw_key) + response = client.get(reverse("provider-list"), headers=api_key_headers) + + assert response.status_code == 401 + assert ( + "No entity matching this api key." in response.json()["errors"][0]["detail"] + ) + + # The orphaned key is revoked on use; retries fail the regular revoked check + api_key.refresh_from_db() + assert api_key.revoked is True + + retry_response = client.get(reverse("provider-list"), headers=api_key_headers) + assert retry_response.status_code == 401 + assert ( + "API Key has been revoked." in retry_response.json()["errors"][0]["detail"] + ) + def test_non_existent_api_key(self, create_test_user, tenants_fixture): """Key UUID doesn't exist in database.""" client = APIClient() @@ -817,6 +848,93 @@ class TestAPIKeyTenantIsolation: error_detail = response_json["errors"][0]["detail"] assert "revoked" in error_detail.lower() + def test_deleting_user_revokes_api_keys_in_every_tenant(self, tenants_fixture): + """Deleting a user revokes their keys in all their tenants, not just one.""" + first_tenant, second_tenant = tenants_fixture[0], tenants_fixture[1] + + test_user = User.objects.create_user( + name="multi_tenant_user", + email="multi_tenant_user@prowler.com", + password=TEST_PASSWORD, + ) + for tenant in (first_tenant, second_tenant): + Membership.objects.create( + user=test_user, tenant=tenant, role=Membership.RoleChoices.OWNER + ) + + first_key, _ = TenantAPIKey.objects.create_api_key( + name="Key in first tenant", tenant_id=first_tenant.id, entity=test_user + ) + second_key, _ = TenantAPIKey.objects.create_api_key( + name="Key in second tenant", tenant_id=second_tenant.id, entity=test_user + ) + + test_user.delete() + + first_key.refresh_from_db() + second_key.refresh_from_db() + assert first_key.revoked is True + assert second_key.revoked is True + # `on_delete=SET_NULL` orphans the keys, so revoking them is what keeps them + # from authenticating + assert first_key.entity_id is None + assert second_key.entity_id is None + + def test_revoke_user_api_keys_uses_the_admin_connection( + self, monkeypatch, tenants_fixture + ): + """The revocation must not go through the default connection. + + `api_keys` is RLS protected and its policy denies every row when `api.tenant_id` + is unset, which is the case while a user is deleted through the admin + connection: the update would silently revoke nothing and leave usable orphaned + keys behind. + + Pointing `admin_db` at a missing alias is the only way to assert the connection + here, because the test suite runs on a single superuser database with + `MainRouter.admin_db` patched to "default" (see `conftest.py`), so RLS never + applies and both connections are otherwise indistinguishable. + """ + test_user = User.objects.create_user( + name="admin_connection_user", + email="admin_connection_user@prowler.com", + password=TEST_PASSWORD, + ) + Membership.objects.create(user=test_user, tenant=tenants_fixture[0]) + TenantAPIKey.objects.create_api_key( + name="Key for admin connection check", + tenant_id=tenants_fixture[0].id, + entity=test_user, + ) + + monkeypatch.setattr(MainRouter, "admin_db", "missing_admin_alias") + + with pytest.raises(ConnectionDoesNotExist): + revoke_user_api_keys(sender=User, instance=test_user) + + def test_revoke_membership_api_keys_uses_the_admin_connection( + self, monkeypatch, tenants_fixture + ): + """Same as the user deletion case: this receiver also runs as its cascade.""" + test_user = User.objects.create_user( + name="admin_connection_membership_user", + email="admin_connection_membership_user@prowler.com", + password=TEST_PASSWORD, + ) + membership = Membership.objects.create( + user=test_user, tenant=tenants_fixture[0] + ) + TenantAPIKey.objects.create_api_key( + name="Key for membership admin connection check", + tenant_id=tenants_fixture[0].id, + entity=test_user, + ) + + monkeypatch.setattr(MainRouter, "admin_db", "missing_admin_alias") + + with pytest.raises(ConnectionDoesNotExist): + revoke_membership_api_keys(sender=Membership, instance=membership) + @pytest.mark.django_db class TestAPIKeyLifecycle: diff --git a/api/src/backend/api/tests/test_authentication.py b/api/src/backend/api/tests/test_authentication.py index d05a55ce5a..782bfe670e 100644 --- a/api/src/backend/api/tests/test_authentication.py +++ b/api/src/backend/api/tests/test_authentication.py @@ -4,11 +4,17 @@ from unittest.mock import MagicMock, patch from uuid import uuid4 import pytest -from api.authentication import SSEAuthentication, TenantAPIKeyAuthentication +from api.authentication import ( + OrphanedAPIKeyError, + SSEAuthentication, + TenantAPIKeyAuthentication, +) from api.db_router import MainRouter from api.models import TenantAPIKey +from django.db import connections from django.db.models.query import QuerySet from django.test import RequestFactory +from django.test.utils import CaptureQueriesContext from rest_framework.exceptions import AuthenticationFailed @@ -38,13 +44,12 @@ class TestTenantAPIKeyAuthentication: request = request_factory.get("/") # Call the method - entity, auth_dict = auth_backend._authenticate_credentials( - request, encrypted_key - ) + validated_key = auth_backend._authenticate_credentials(request, encrypted_key) # Verify that the entity is the user associated with the API key - assert entity == api_key.entity - assert entity.id == api_key.entity.id + assert validated_key.id == api_key.id + assert validated_key.entity == api_key.entity + assert validated_key.entity.id == api_key.entity.id def test_authenticate_credentials_restores_manager_on_success( self, auth_backend, api_keys_fixture, request_factory @@ -231,6 +236,120 @@ class TestTenantAPIKeyAuthentication: assert str(exc_info.value.detail) == "This API Key has been revoked." + def test_authenticate_credentials_orphaned_api_key( + self, auth_backend, api_keys_fixture, request_factory + ): + """Test credential validation fails when the owning user no longer exists.""" + api_key = api_keys_fixture[0] + _, encrypted_key = api_key._raw_key.split(TenantAPIKey.objects.separator, 1) + + # `entity` is what `on_delete=SET_NULL` leaves behind when the owner is deleted + TenantAPIKey.objects.filter(id=api_key.id).update(entity=None) + + request = request_factory.get("/") + + with pytest.raises(OrphanedAPIKeyError): + auth_backend._authenticate_credentials(request, encrypted_key) + + # The orphaned key is revoked on use, so it stops showing up as active + api_key.refresh_from_db() + assert api_key.revoked is True + + def test_authenticate_orphaned_api_key( + self, auth_backend, api_keys_fixture, request_factory + ): + """Test authentication fails with a key whose owning user was deleted. + + Regression test: this used to raise `AttributeError: 'NoneType' object has no + attribute 'id'` while building the auth dict, which DRF re-raises as + `WrappedAttributeError` and turns into a 500 instead of a 401. + """ + api_key = api_keys_fixture[0] + raw_key = api_key._raw_key + + TenantAPIKey.objects.filter(id=api_key.id).update(entity=None) + + request = request_factory.get("/") + request.META["HTTP_AUTHORIZATION"] = f"Api-Key {raw_key}" + + with pytest.raises(AuthenticationFailed) as exc_info: + auth_backend.authenticate(request) + + assert str(exc_info.value.detail) == "No entity matching this api key." + + # The orphaned key is revoked on use; retries fail the regular revoked check + api_key.refresh_from_db() + assert api_key.revoked is True + + with pytest.raises(AuthenticationFailed) as exc_info: + auth_backend.authenticate(request) + + assert str(exc_info.value.detail) == "This API Key has been revoked." + + def test_authenticate_reads_the_api_key_once_under_a_row_lock( + self, auth_backend, api_keys_fixture, request_factory + ): + """Test the API key is read a single time and the row is locked. + + Validation, the `last_used_at` update and the claims must all come from the + same authoritative row: a second, unlocked lookup would reopen the window + where a key revoked in between still authenticates. + """ + api_key = api_keys_fixture[0] + + request = request_factory.get("/") + request.META["HTTP_AUTHORIZATION"] = f"Api-Key {api_key._raw_key}" + + with CaptureQueriesContext(connections[MainRouter.admin_db]) as captured: + auth_backend.authenticate(request) + + api_key_selects = [ + query["sql"] + for query in captured.captured_queries + if query["sql"].startswith("SELECT") and '"api_keys"' in query["sql"] + ] + + assert len(api_key_selects) == 1 + assert "FOR UPDATE" in api_key_selects[0] + + def test_authenticate_ignores_revocation_after_the_locked_read( + self, auth_backend, api_keys_fixture, request_factory + ): + """Test the claims describe the row that was validated, not a later state. + + Regression test: the key used to be looked up again to build the auth dict, + without rechecking `revoked` or `entity`. A key revoked or orphaned between + both reads still authenticated, and the claims came from that stale row. With + a single locked read the write below cannot land mid-authentication, and the + revocation only takes effect on the next request. + """ + api_key = api_keys_fixture[0] + entity_at_validation = api_key.entity + original_save = TenantAPIKey.save + + def revoke_and_orphan_before_saving(instance, *args, **kwargs): + # Runs after validation, right before the claims are built: the exact + # window a concurrent revocation or user deletion used to slip into + TenantAPIKey.objects.filter(id=api_key.id).update(revoked=True, entity=None) + return original_save(instance, *args, **kwargs) + + request = request_factory.get("/") + request.META["HTTP_AUTHORIZATION"] = f"Api-Key {api_key._raw_key}" + + with patch.object(TenantAPIKey, "save", revoke_and_orphan_before_saving): + entity, auth_dict = auth_backend.authenticate(request) + + assert entity == entity_at_validation + assert auth_dict["sub"] == str(entity_at_validation.id) + assert auth_dict["tenant_id"] == str(api_key.tenant_id) + assert auth_dict["api_key_prefix"] == api_key.prefix + + # The revoked key is rejected from the next request on + with pytest.raises(AuthenticationFailed) as exc_info: + auth_backend.authenticate(request) + + assert str(exc_info.value.detail) == "This API Key has been revoked." + def test_authenticate_expired_api_key( self, auth_backend, create_test_user, tenants_fixture, request_factory ): diff --git a/api/src/backend/api/tests/test_views.py b/api/src/backend/api/tests/test_views.py index 96d366e847..7c2a03b965 100644 --- a/api/src/backend/api/tests/test_views.py +++ b/api/src/backend/api/tests/test_views.py @@ -15748,6 +15748,23 @@ class TestTenantApiKeyViewSet: data = response.json()["data"] assert len(data) == len(api_keys_fixture) + def test_api_keys_list_with_orphaned_key( + self, authenticated_client, api_keys_fixture + ): + """Test listing keys whose owner was deleted: `entity` is serialized as null.""" + orphaned_key = api_keys_fixture[0] + TenantAPIKey.objects.filter(id=orphaned_key.id).update(entity=None) + + response = authenticated_client.get(reverse("api-key-list")) + + assert response.status_code == status.HTTP_200_OK + data = response.json()["data"] + assert len(data) == len(api_keys_fixture) + serialized_key = next( + item for item in data if item["id"] == str(orphaned_key.id) + ) + assert serialized_key["relationships"]["entity"]["data"] is None + def test_api_keys_list_empty(self, authenticated_client, tenants_fixture): """Test listing API keys when none exist returns empty list.""" response = authenticated_client.get(reverse("api-key-list"))