From 53d091dbdd46af9aea2529253ed916cf105236b3 Mon Sep 17 00:00:00 2001 From: Stephen Rosen Date: Fri, 11 Oct 2024 12:43:35 -0500 Subject: [PATCH] Fix caching and checking of introspect responses (#173) - Ensure introspect is cached even if 'active=False' - Don't check claims pointlessly: exp and nbf can fail if your clock is drifted, so it's not safe to be checking them unless users can configure a leeway. Also, 'active' tells you everything you need in OAuth2. - Capture scope validation failures with an exception, not None. - Cache by token hash, not full token string. --- ...08_171503_sirosen_fix_introspect_check.rst | 19 +++++ .../authentication.py | 84 +++++++++++-------- tests/test_authentication.py | 29 +++++-- 3 files changed, 90 insertions(+), 42 deletions(-) create mode 100644 changelog.d/20241008_171503_sirosen_fix_introspect_check.rst diff --git a/changelog.d/20241008_171503_sirosen_fix_introspect_check.rst b/changelog.d/20241008_171503_sirosen_fix_introspect_check.rst new file mode 100644 index 0000000..3a662a5 --- /dev/null +++ b/changelog.d/20241008_171503_sirosen_fix_introspect_check.rst @@ -0,0 +1,19 @@ +Features +-------- + +- The token introspect checking and caching performed in ``AuthState`` has + been improved. + + - The cache is keyed off of token hashes, rather than raw token strings. + + - The ``exp`` and ``nbf`` values are no longer verified, removing the + possibility of incorrect treatment of valid tokens as invalid due to clock + drift. + + - Introspect response caching caches the raw response even for invalid + tokens, meaning that Action Providers will no longer repeatedly introspect + a token once it is known to be invalid. + + - Scope validation raises a new, dedicated error class, + ``globus_action_provider_tools.authentication.InvalidTokenScopesError``, on + failure. diff --git a/src/globus_action_provider_tools/authentication.py b/src/globus_action_provider_tools/authentication.py index 149c7ad..5b0ddd1 100644 --- a/src/globus_action_provider_tools/authentication.py +++ b/src/globus_action_provider_tools/authentication.py @@ -1,5 +1,6 @@ from __future__ import annotations +import functools import hashlib import logging from time import time @@ -34,6 +35,19 @@ def identity_principal(id_: str) -> str: return f"urn:globus:auth:identity:{id_}" +class InvalidTokenScopesError(ValueError): + def __init__( + self, expected_scopes: frozenset[str], actual_scopes: frozenset[str] + ) -> None: + self.expected_scopes = expected_scopes + self.actual_scopes = actual_scopes + super().__init__( + f"Token scopes were not valid. " + f"The valid scopes for this service are {self.expected_scopes} but the " + f"token contained {self.actual_scopes} upon inspection." + ) + + class AuthState: # Cache for introspection operations, max lifetime: 30 seconds introspect_cache: TTLCache = TTLCache(maxsize=100, ttl=30) @@ -49,7 +63,7 @@ def __init__( self, auth_client: ConfidentialAppAuthClient, bearer_token: str, - expected_scopes: Iterable[str], + expected_scopes: frozenset[str], ) -> None: self.auth_client = auth_client self.bearer_token = bearer_token @@ -59,48 +73,46 @@ def __init__( self.errors: list[Exception] = [] self._groups_client: GroupsClient | None = None - def introspect_token(self) -> GlobusHTTPResponse | None: - # There are cases where a null or empty string bearer token are present as a - # placeholder - if self.bearer_token is None: - return None + @functools.cached_property + def _token_hash(self) -> str: + return _hash_token(self.bearer_token) - resp = AuthState.introspect_cache.get(self.bearer_token) - if resp is not None: - log.info( - f"Using cached introspection response for token ***{self.sanitized_token}" + def _cached_introspect_call(self) -> GlobusHTTPResponse: + introspect_result = AuthState.introspect_cache.get(self._token_hash) + if introspect_result is not None: + log.debug( + f"Using cached introspection introspect_resultonse for " ) - return resp + return introspect_result - log.info(f"Introspecting token ***{self.sanitized_token}") - resp = self.auth_client.oauth2_token_introspect( + log.debug(f"Introspecting token ") + introspect_result = self.auth_client.oauth2_token_introspect( self.bearer_token, include="identity_set" ) - now = time() + self.introspect_cache[self._token_hash] = introspect_result - try: - if resp.get("active", False) is not True: - raise AssertionError("Invalid token.") - if not resp.get("nbf", now + 4) < (time() + 3): - raise AssertionError("Token not yet valid -- check system clock?") - if not resp.get("exp", 0) > (time() - 3): - raise AssertionError("Token expired.") - scopes = frozenset(resp.get("scope", "").split()) - if not scopes & set(self.expected_scopes): - raise AssertionError( - "Token invalid scopes. " - f"Expected one of: {self.expected_scopes}, got: {scopes}" - ) - if "identity_set" not in resp: - raise AssertionError("Missing identity_set") - except AssertionError as err: - self.errors.append(err) - log.info(err) + return introspect_result + + def introspect_token(self) -> GlobusHTTPResponse | None: + introspect_result = self._cached_introspect_call() + + # FIXME: convert this to an exception, rather than 'None' + # the exception could be raised in _verify_introspect_result + if not introspect_result["active"]: return None - else: - log.info(f"Caching token response for token ***{self.sanitized_token}") - AuthState.introspect_cache[self.bearer_token] = resp - return resp + + self._verify_introspect_result(introspect_result) + return introspect_result + + def _verify_introspect_result(self, introspect_result: GlobusHTTPResponse) -> None: + """ + A helper which checks token introspect properties and raises exceptions on failure. + """ + # validate scopes, ensuring that the token provided accords with the service's + # notion of what operations exist and are supported + scopes = set(introspect_result.get("scope", "").split()) + if any(s not in self.expected_scopes for s in scopes): + raise InvalidTokenScopesError(self.expected_scopes, frozenset(scopes)) @property def effective_identity(self) -> str | None: diff --git a/tests/test_authentication.py b/tests/test_authentication.py index b5c384c..04002b4 100644 --- a/tests/test_authentication.py +++ b/tests/test_authentication.py @@ -6,24 +6,31 @@ import pytest from globus_sdk._testing import load_response -from globus_action_provider_tools.authentication import AuthState, identity_principal +from globus_action_provider_tools.authentication import ( + AuthState, + InvalidTokenScopesError, + identity_principal, +) def get_auth_state_instance(expected_scopes: t.Iterable[str]) -> AuthState: return AuthState( auth_client=globus_sdk.ConfidentialAppAuthClient("bogus", "bogus"), bearer_token="bogus", - expected_scopes=expected_scopes, + expected_scopes=frozenset(expected_scopes), ) -@pytest.fixture -def auth_state(mocked_responses) -> AuthState: - """Create an AuthState instance.""" - +@pytest.fixture(autouse=True) +def _clear_auth_state_cache(): AuthState.dependent_tokens_cache.clear() AuthState.group_membership_cache.clear() AuthState.introspect_cache.clear() + + +@pytest.fixture +def auth_state(mocked_responses) -> AuthState: + """Create an AuthState instance.""" # note that expected-scope MUST match the fixture data return get_auth_state_instance(["expected-scope"]) @@ -90,3 +97,13 @@ def test_invalid_grant_exception(auth_state): load_response("token-introspect", case="success") load_response("token", case="invalid-grant") assert auth_state.get_authorizer_for_scope("doesn't matter") is None + + +def test_invalid_scopes_error(): + auth_state = get_auth_state_instance(["bad-scope"]) + load_response("token-introspect", case="success") + with pytest.raises(InvalidTokenScopesError) as excinfo: + auth_state.introspect_token() + + assert excinfo.value.expected_scopes == {"bad-scope"} + assert excinfo.value.actual_scopes == {"expected-scope"}