This is an automated email from the ASF dual-hosted git repository. akm pushed a commit to branch invalidate-pats-manually-598 in repository https://gitbox.apache.org/repos/asf/tooling-trusted-releases.git
commit 8dd0e3ba0a8e10c17af4d5719d99bb2ded2462ba Author: Andrew K. Musselman <[email protected]> AuthorDate: Thu Feb 19 13:00:34 2026 -0800 Manual PAT removal; fixes #598 --- atr/docs/authentication-security.md | 5 +- atr/docs/authorization-security.md | 5 - atr/server.py | 9 -- atr/token_cleanup.py | 92 ------------ tests/unit/test_token_cleanup.py | 283 ------------------------------------ 5 files changed, 1 insertion(+), 393 deletions(-) diff --git a/atr/docs/authentication-security.md b/atr/docs/authentication-security.md index c2742781..fb0cf117 100644 --- a/atr/docs/authentication-security.md +++ b/atr/docs/authentication-security.md @@ -70,12 +70,11 @@ Committers can obtain PATs from the `/tokens` page on the ATR website. PATs have * **Validity**: 180 days from creation, while LDAP account is still active * **Storage**: ATR stores only SHA3-256 hashes, never the plaintext PAT * **Revocation**: Users can revoke their own PATs at any time; admins can revoke all PATs for any user via the admin "Revoke user tokens" page -* **Automatic cleanup**: A background loop ([`token_cleanup`](/ref/atr/token_cleanup.py)) polls LDAP approximately every hour and automatically revokes all PATs belonging to banned or deleted accounts * **Purpose**: PATs are used solely to obtain JWTs; they cannot be used directly for API access Only authenticated committers (signed in via ASF OAuth) can create PATs. Each user can have multiple active PATs. -PATs are rejected if the user who created them has been banned in or removed from LDAP. This is enforced at three layers: the JWT exchange endpoint checks LDAP status before issuing a JWT (immediate), a background cleanup loop revokes PATs for banned or deleted accounts (within ~1 hour), and administrators can revoke PATs immediately through the admin interface. +PATs are rejected if the user who created them has been banned in or removed from LDAP. ### JSON Web Tokens (JWTs) @@ -141,7 +140,6 @@ For web users, authentication happens once via ASF OAuth, and the session persis * Stored as SHA3-256 hashes * Can be revoked immediately by the user or in bulk by administrators -* Automatically revoked when the owning account is banned or deleted in LDAP * Limited purpose (only for JWT issuance) reduces impact of compromise * Long validity (180 days) balanced by easy revocation @@ -164,5 +162,4 @@ Tokens must be protected by the user at all times: * [`principal.py`](/ref/atr/principal.py) - Session caching and authorization data * [`jwtoken.py`](/ref/atr/jwtoken.py) - JWT creation, verification, and decorators -* [`token_cleanup.py`](/ref/atr/token_cleanup.py) - Automated PAT revocation for banned/deleted accounts * [`storage/writers/tokens.py`](/ref/atr/storage/writers/tokens.py) - Token creation, deletion, and admin revocation diff --git a/atr/docs/authorization-security.md b/atr/docs/authorization-security.md index 6d58b2eb..41029ebd 100644 --- a/atr/docs/authorization-security.md +++ b/atr/docs/authorization-security.md @@ -128,15 +128,10 @@ Token operations apply to the authenticated user: * Interface: Admin "Revoke user tokens" page * Constraint: Requires typing "REVOKE" as confirmation -**Automated token revocation**: - -* The [`token_cleanup`](/ref/atr/token_cleanup.py) background loop runs approximately every hour, queries LDAP for all users who hold PATs, and revokes tokens belonging to accounts that are banned or deleted. This runs without human intervention and is audit logged. - **Exchange PAT for JWT**: * Allowed for: Anyone with a valid PAT * Note: This is an unauthenticated endpoint; the PAT serves as the credential -* Constraint: LDAP is checked at exchange time; banned or deleted accounts are rejected even if the PAT has not yet been cleaned up ## Access control for check ignores diff --git a/atr/server.py b/atr/server.py index 78632217..f32269a2 100644 --- a/atr/server.py +++ b/atr/server.py @@ -61,7 +61,6 @@ import atr.ssh as ssh import atr.svn.pubsub as pubsub import atr.tasks as tasks import atr.template as template -import atr.token_cleanup as token_cleanup import atr.user as user import atr.util as util @@ -270,9 +269,6 @@ def _app_setup_lifecycle(app: base.QuartApp, app_config: type[config.AppConfig]) admins_task = asyncio.create_task(cache.admins_refresh_loop()) app.extensions["admins_task"] = admins_task - pat_cleanup_task = asyncio.create_task(token_cleanup.cleanup_loop()) - app.extensions["pat_cleanup_task"] = pat_cleanup_task - worker_manager = manager.get_worker_manager() await worker_manager.start() @@ -316,11 +312,6 @@ def _app_setup_lifecycle(app: base.QuartApp, app_config: type[config.AppConfig]) with contextlib.suppress(asyncio.CancelledError): await task - if task := app.extensions.get("pat_cleanup_task"): - task.cancel() - with contextlib.suppress(asyncio.CancelledError): - await task - await db.shutdown_database() await _app_shutdown_log_listeners(app) diff --git a/atr/token_cleanup.py b/atr/token_cleanup.py deleted file mode 100644 index f7f0cce2..00000000 --- a/atr/token_cleanup.py +++ /dev/null @@ -1,92 +0,0 @@ -# Licensed to the Apache Software Foundation (ASF) under one -# or more contributor license agreements. See the NOTICE file -# distributed with this work for additional information -# regarding copyright ownership. The ASF licenses this file -# to you under the Apache License, Version 2.0 (the -# "License"); you may not use this file except in compliance -# with the License. You may obtain a copy of the License at -# -# http://www.apache.org/licenses/LICENSE-2.0 -# -# Unless required by applicable law or agreed to in writing, -# software distributed under the License is distributed on an -# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY -# KIND, either express or implied. See the License for the -# specific language governing permissions and limitations -# under the License. - -"""Periodic cleanup of Personal Access Tokens for banned or deleted accounts.""" - -import asyncio -from typing import Final - -import sqlalchemy -import sqlmodel - -import atr.db as db -import atr.ldap as ldap -import atr.log as log -import atr.models.sql as sql -import atr.storage as storage - -# ~1 hour, deliberately offset from the admin poll interval (3631s) -# to avoid simultaneous LDAP request spikes -POLL_INTERVAL_SECONDS: Final[int] = 3617 - - -async def cleanup_loop() -> None: - """Periodically revoke PATs belonging to banned or deleted LDAP accounts.""" - while True: - await asyncio.sleep(POLL_INTERVAL_SECONDS) - try: - await revoke_pats_for_banned_users() - except Exception as e: - log.warning(f"PAT banned-user cleanup failed: {e}") - - -async def revoke_pats_for_banned_users() -> int: - """Check all PAT-holding users against LDAP and revoke tokens for banned/deleted accounts. - - Returns the total number of tokens revoked. - """ - # Step 1: Get distinct UIDs that have PATs - async with db.session() as data: - stmt = sqlmodel.select(sql.PersonalAccessToken.asfuid).distinct() - rows = await data.execute_query(stmt) - uids_with_pats = [row[0] for row in rows] - - if not uids_with_pats: - return 0 - - # Step 2: Check each against LDAP, revoke if banned/deleted - revoked_total = 0 - for uid in uids_with_pats: - try: - account = await ldap.account_lookup(uid) - if (account is not None) and (not ldap.is_banned(account)): - continue - - # Account is gone or banned — delete all their tokens - async with db.session() as data: - delete_stmt = sqlalchemy.delete(sql.PersonalAccessToken).where(sql.PersonalAccessToken.asfuid == uid) - result = await data.execute_query(delete_stmt) - await data.commit() - count: int = getattr(result, "rowcount", 0) - - if count > 0: - storage.audit( - target_asf_uid=uid, - tokens_revoked=count, - reason="account_banned_or_deleted", - source="pat_cleanup_loop", - ) - log.info(f"Auto-revoked {count} PAT(s) for banned/deleted user {uid}") - revoked_total += count - - except Exception as e: - log.warning(f"PAT cleanup: failed to check/revoke for {uid}: {e}") - - if revoked_total > 0: - log.info(f"PAT cleanup cycle complete: revoked {revoked_total} total token(s)") - - return revoked_total diff --git a/tests/unit/test_token_cleanup.py b/tests/unit/test_token_cleanup.py deleted file mode 100644 index 6113e2a2..00000000 --- a/tests/unit/test_token_cleanup.py +++ /dev/null @@ -1,283 +0,0 @@ -# Licensed to the Apache Software Foundation (ASF) under one -# or more contributor license agreements. See the NOTICE file -# distributed with this work for additional information -# regarding copyright ownership. The ASF licenses this file -# to you under the Apache License, Version 2.0 (the -# "License"); you may not use this file except in compliance -# with the License. You may obtain a copy of the License at -# -# http://www.apache.org/licenses/LICENSE-2.0 -# -# Unless required by applicable law or agreed to in writing, -# software distributed under the License is distributed on an -# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY -# KIND, either express or implied. See the License for the -# specific language governing permissions and limitations -# under the License. - -"""Tests for periodic PAT cleanup of banned/deleted accounts.""" - -from typing import TYPE_CHECKING - -import pytest - -import atr.token_cleanup as token_cleanup - -if TYPE_CHECKING: - from pytest import MonkeyPatch - - -class MockDBSession: - """Minimal mock for db.Session used in PAT cleanup tests.""" - - def __init__(self, pat_uids: list[str] | None = None, delete_rowcount: int = 0): - self._pat_uids = pat_uids or [] - self._delete_rowcount = delete_rowcount - self._committed = False - self._executed_stmts: list[object] = [] - - async def execute_query(self, stmt: object) -> "MockResult": - self._executed_stmts.append(stmt) - # SELECT returns uid rows, DELETE returns rowcount - stmt_type = str(type(stmt)).lower() - if "select" in stmt_type: - return MockResult(rows=[(uid,) for uid in self._pat_uids]) - return MockResult(rowcount=self._delete_rowcount) - - async def commit(self) -> None: - self._committed = True - - async def __aenter__(self) -> "MockDBSession": - return self - - async def __aexit__(self, *args: object) -> None: - pass - - -class MockResult: - def __init__(self, rows: list[tuple[str]] | None = None, rowcount: int = 0): - self._rows = rows or [] - self.rowcount = rowcount - - def __iter__(self): - return iter(self._rows) - - [email protected] -async def test_cleanup_does_nothing_when_no_pats_exist(monkeypatch: "MonkeyPatch") -> None: - """When no users have PATs, the cleanup should not call LDAP at all.""" - lookup_called = False - - async def mock_account_lookup(uid: str) -> dict[str, str]: - nonlocal lookup_called - lookup_called = True - return {"uid": uid} - - monkeypatch.setattr("atr.token_cleanup.ldap.account_lookup", mock_account_lookup) - - mock_session = MockDBSession(pat_uids=[]) - monkeypatch.setattr("atr.token_cleanup.db.session", lambda: mock_session) - - revoked = await token_cleanup.revoke_pats_for_banned_users() - - assert revoked == 0 - assert not lookup_called - - [email protected] -async def test_cleanup_skips_active_accounts(monkeypatch: "MonkeyPatch") -> None: - """Active LDAP accounts should not have their PATs revoked.""" - lookup_calls: list[str] = [] - - async def mock_account_lookup(uid: str) -> dict[str, str]: - lookup_calls.append(uid) - return {"uid": uid, "asf-banned": "no"} - - monkeypatch.setattr("atr.token_cleanup.ldap.account_lookup", mock_account_lookup) - monkeypatch.setattr("atr.token_cleanup.ldap.is_banned", lambda account: False) - monkeypatch.setattr("atr.token_cleanup.storage.audit", lambda **kwargs: None) - - mock_session = MockDBSession(pat_uids=["active_user"]) - monkeypatch.setattr("atr.token_cleanup.db.session", lambda: mock_session) - - revoked = await token_cleanup.revoke_pats_for_banned_users() - - assert revoked == 0 - assert "active_user" in lookup_calls - # Only the SELECT should have been executed, no DELETE - assert len(mock_session._executed_stmts) == 1 - assert not mock_session._committed - - [email protected] -async def test_cleanup_revokes_for_banned_account(monkeypatch: "MonkeyPatch") -> None: - """Banned LDAP accounts should have all PATs deleted.""" - audit_calls: list[dict] = [] - - async def mock_account_lookup(uid: str) -> dict[str, str | list[str]]: - return {"uid": [uid], "asf-banned": "yes"} - - monkeypatch.setattr("atr.token_cleanup.ldap.account_lookup", mock_account_lookup) - monkeypatch.setattr("atr.token_cleanup.ldap.is_banned", lambda account: True) - monkeypatch.setattr("atr.token_cleanup.storage.audit", lambda **kwargs: audit_calls.append(kwargs)) - - # Two sessions: first returns UIDs, second handles the DELETE - call_count = 0 - select_session = MockDBSession(pat_uids=["banned_user"]) - delete_session = MockDBSession(delete_rowcount=3) - - def mock_db_session(): - nonlocal call_count - call_count += 1 - if call_count == 1: - return select_session - return delete_session - - monkeypatch.setattr("atr.token_cleanup.db.session", mock_db_session) - - revoked = await token_cleanup.revoke_pats_for_banned_users() - - assert revoked == 3 - assert delete_session._committed - assert len(audit_calls) == 1 - assert audit_calls[0]["target_asf_uid"] == "banned_user" - assert audit_calls[0]["tokens_revoked"] == 3 - assert audit_calls[0]["reason"] == "account_banned_or_deleted" - - [email protected] -async def test_cleanup_revokes_for_deleted_account(monkeypatch: "MonkeyPatch") -> None: - """Accounts not found in LDAP (returns None) should have PATs deleted.""" - audit_calls: list[dict] = [] - - async def mock_account_lookup(uid: str) -> None: - return None - - monkeypatch.setattr("atr.token_cleanup.ldap.account_lookup", mock_account_lookup) - monkeypatch.setattr("atr.token_cleanup.storage.audit", lambda **kwargs: audit_calls.append(kwargs)) - - call_count = 0 - select_session = MockDBSession(pat_uids=["deleted_user"]) - delete_session = MockDBSession(delete_rowcount=2) - - def mock_db_session(): - nonlocal call_count - call_count += 1 - if call_count == 1: - return select_session - return delete_session - - monkeypatch.setattr("atr.token_cleanup.db.session", mock_db_session) - - revoked = await token_cleanup.revoke_pats_for_banned_users() - - assert revoked == 2 - assert delete_session._committed - assert len(audit_calls) == 1 - assert audit_calls[0]["target_asf_uid"] == "deleted_user" - assert audit_calls[0]["tokens_revoked"] == 2 - - [email protected] -async def test_cleanup_continues_after_single_ldap_failure(monkeypatch: "MonkeyPatch") -> None: - """One LDAP failure should not prevent cleanup of other users.""" - audit_calls: list[dict] = [] - lookup_calls: list[str] = [] - - async def mock_account_lookup(uid: str) -> dict[str, str | list[str]] | None: - lookup_calls.append(uid) - if uid == "failing_user": - raise ConnectionError("LDAP timeout") - return None - - monkeypatch.setattr("atr.token_cleanup.ldap.account_lookup", mock_account_lookup) - monkeypatch.setattr("atr.token_cleanup.storage.audit", lambda **kwargs: audit_calls.append(kwargs)) - - call_count = 0 - select_session = MockDBSession(pat_uids=["failing_user", "deleted_user"]) - delete_session = MockDBSession(delete_rowcount=1) - - def mock_db_session(): - nonlocal call_count - call_count += 1 - if call_count == 1: - return select_session - return delete_session - - monkeypatch.setattr("atr.token_cleanup.db.session", mock_db_session) - - revoked = await token_cleanup.revoke_pats_for_banned_users() - - assert revoked == 1 - # Both users should have been checked - assert "failing_user" in lookup_calls - assert "deleted_user" in lookup_calls - # Only the non-failing user should have been cleaned up - assert len(audit_calls) == 1 - assert audit_calls[0]["target_asf_uid"] == "deleted_user" - - [email protected] -async def test_cleanup_does_not_audit_when_zero_tokens_deleted(monkeypatch: "MonkeyPatch") -> None: - """If a banned user has 0 tokens (race condition), no audit entry should be written.""" - audit_calls: list[dict] = [] - - async def mock_account_lookup(uid: str) -> None: - return None - - monkeypatch.setattr("atr.token_cleanup.ldap.account_lookup", mock_account_lookup) - monkeypatch.setattr("atr.token_cleanup.storage.audit", lambda **kwargs: audit_calls.append(kwargs)) - - call_count = 0 - select_session = MockDBSession(pat_uids=["ghost_user"]) - delete_session = MockDBSession(delete_rowcount=0) - - def mock_db_session(): - nonlocal call_count - call_count += 1 - if call_count == 1: - return select_session - return delete_session - - monkeypatch.setattr("atr.token_cleanup.db.session", mock_db_session) - - revoked = await token_cleanup.revoke_pats_for_banned_users() - - assert revoked == 0 - assert len(audit_calls) == 0 - - [email protected] -async def test_cleanup_handles_multiple_banned_users(monkeypatch: "MonkeyPatch") -> None: - """Multiple banned users should each have their tokens revoked independently.""" - audit_calls: list[dict] = [] - - async def mock_account_lookup(uid: str) -> None: - return None - - monkeypatch.setattr("atr.token_cleanup.ldap.account_lookup", mock_account_lookup) - monkeypatch.setattr("atr.token_cleanup.storage.audit", lambda **kwargs: audit_calls.append(kwargs)) - - call_count = 0 - select_session = MockDBSession(pat_uids=["user_a", "user_b", "user_c"]) - delete_sessions = [ - MockDBSession(delete_rowcount=2), - MockDBSession(delete_rowcount=1), - MockDBSession(delete_rowcount=4), - ] - - def mock_db_session(): - nonlocal call_count - call_count += 1 - if call_count == 1: - return select_session - return delete_sessions[call_count - 2] - - monkeypatch.setattr("atr.token_cleanup.db.session", mock_db_session) - - revoked = await token_cleanup.revoke_pats_for_banned_users() - - assert revoked == 7 - assert len(audit_calls) == 3 - revoked_uids = {call["target_asf_uid"] for call in audit_calls} - assert revoked_uids == {"user_a", "user_b", "user_c"} --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
