From 8561ce60924799a2969035fe00838b7a89563d2c Mon Sep 17 00:00:00 2001 From: David Butenhof Date: Fri, 16 Jun 2023 23:08:33 -0400 Subject: [PATCH] Fix JWT OIDC decode yet again (#3466) PBENCH-1182 JWT added yet another `DecodeError` in our API Key validation path in some recent update. Instead of continuing to add specific errors to be converted into a specific internal exception, just handle all exceptions in the primary OIDC decode by attempting the token as an API key. If that fails, report the API key validation error, and re-raise the original OIDC error. (Note that `verify_auth` will in turn just report this error and act as if no authentication token was given.) --- lib/pbench/server/auth/__init__.py | 35 ++++++------------- lib/pbench/server/auth/auth.py | 25 +++++-------- lib/pbench/test/unit/server/auth/test_auth.py | 25 +++++-------- 3 files changed, 28 insertions(+), 57 deletions(-) diff --git a/lib/pbench/server/auth/__init__.py b/lib/pbench/server/auth/__init__.py index 8f9037c53b..3dad4f59a1 100644 --- a/lib/pbench/server/auth/__init__.py +++ b/lib/pbench/server/auth/__init__.py @@ -27,10 +27,6 @@ def __str__(self) -> str: return self.message -class OpenIDTokenInvalid(Exception): - pass - - class Connection: """Helper connection class for use by an OpenIDClient instance.""" @@ -359,23 +355,14 @@ def token_introspect(self, token: str) -> JSON: "sub": } """ - try: - return jwt.decode( - token, - self._pem_public_key, - algorithms=[self._TOKEN_ALG], - audience=[self.client_id], - options={ - "verify_signature": True, - "verify_aud": True, - "verify_exp": True, - }, - ) - except ( - jwt.ExpiredSignatureError, - jwt.InvalidSignatureError, - jwt.InvalidAudienceError, - jwt.InvalidAlgorithmError, - ValueError, - ) as exc: - raise OpenIDTokenInvalid() from exc + return jwt.decode( + token, + self._pem_public_key, + algorithms=[self._TOKEN_ALG], + audience=[self.client_id], + options={ + "verify_signature": True, + "verify_aud": True, + "verify_exp": True, + }, + ) diff --git a/lib/pbench/server/auth/auth.py b/lib/pbench/server/auth/auth.py index 8df0d32c05..ede0ec46ef 100644 --- a/lib/pbench/server/auth/auth.py +++ b/lib/pbench/server/auth/auth.py @@ -6,7 +6,7 @@ from flask_restful import abort from pbench.server import PbenchServerConfig -from pbench.server.auth import OpenIDClient, OpenIDTokenInvalid +from pbench.server.auth import OpenIDClient from pbench.server.database.models.api_keys import APIKey from pbench.server.database.models.users import User @@ -143,14 +143,15 @@ def verify_auth_oidc(auth_token: str) -> Optional[User]: """ try: token_payload = oidc_client.token_introspect(token=auth_token) - except OpenIDTokenInvalid: - # The token is not a valid access token, fall through. - pass except Exception: - current_app.logger.exception( - "Unexpected exception occurred while verifying the auth token {}", - auth_token, - ) + try: + return verify_auth_api_key(auth_token) + except Exception: + current_app.logger.exception( + "Unexpected exception occurred while verifying the API key {}", + auth_token, + ) + raise else: # Extract what we want to cache from the access token user_id = token_payload["sub"] @@ -168,12 +169,4 @@ def verify_auth_oidc(auth_token: str) -> Optional[User]: user.update(username=username, roles=roles) return user - try: - return verify_auth_api_key(auth_token) - except Exception: - current_app.logger.exception( - "Unexpected exception occurred while verifying the API key {}", - auth_token, - ) - return None diff --git a/lib/pbench/test/unit/server/auth/test_auth.py b/lib/pbench/test/unit/server/auth/test_auth.py index 141230bafa..177e7772e6 100644 --- a/lib/pbench/test/unit/server/auth/test_auth.py +++ b/lib/pbench/test/unit/server/auth/test_auth.py @@ -12,12 +12,7 @@ from pbench.server import JSON import pbench.server.auth -from pbench.server.auth import ( - Connection, - OpenIDClient, - OpenIDClientError, - OpenIDTokenInvalid, -) +from pbench.server.auth import Connection, OpenIDClient, OpenIDClientError import pbench.server.auth.auth as Auth from pbench.test.unit.server import DRB_USER_ID from pbench.test.unit.server.conftest import jwt_secret @@ -435,9 +430,8 @@ def test_token_introspect_exp(self, monkeypatch, rsa_keys): ) oidc_client = OpenIDClient.construct_oidc_client(config) - with pytest.raises(OpenIDTokenInvalid) as exc: + with pytest.raises(jwt.exceptions.ExpiredSignatureError): oidc_client.token_introspect(token) - assert isinstance(exc.value.__cause__, jwt.exceptions.ExpiredSignatureError) def test_token_introspect_aud(self, monkeypatch, rsa_keys): """Verify .token_introspect() failure via audience error""" @@ -449,9 +443,8 @@ def test_token_introspect_aud(self, monkeypatch, rsa_keys): config = mock_connection(monkeypatch, "them", public_key=rsa_keys["public_key"]) oidc_client = OpenIDClient.construct_oidc_client(config) - with pytest.raises(OpenIDTokenInvalid) as exc: + with pytest.raises(jwt.exceptions.InvalidAudienceError): oidc_client.token_introspect(token) - assert isinstance(exc.value.__cause__, jwt.exceptions.InvalidAudienceError) def test_token_introspect_sig(self, monkeypatch, rsa_keys): """Verify .token_introspect() failure via signature error""" @@ -465,10 +458,9 @@ def test_token_introspect_sig(self, monkeypatch, rsa_keys): ) oidc_client = OpenIDClient.construct_oidc_client(config) - with pytest.raises(OpenIDTokenInvalid) as exc: + with pytest.raises(jwt.exceptions.InvalidSignatureError): # Make the signature invalid. oidc_client.token_introspect(token + "1") - assert isinstance(exc.value.__cause__, jwt.exceptions.InvalidSignatureError) def test_token_introspect_alg(self, monkeypatch, rsa_keys): """Verify .token_introspect() failure via algorithm error""" @@ -483,9 +475,8 @@ def test_token_introspect_alg(self, monkeypatch, rsa_keys): ) oidc_client = OpenIDClient.construct_oidc_client(config) - with pytest.raises(OpenIDTokenInvalid) as exc: + with pytest.raises(jwt.exceptions.InvalidAlgorithmError): oidc_client.token_introspect(generated_api_key) - assert isinstance(exc.value.__cause__, jwt.exceptions.InvalidAlgorithmError) @dataclass @@ -671,7 +662,7 @@ def test_verify_auth_oidc_invalid(self, monkeypatch, rsa_keys, make_logger): monkeypatch.setattr(Auth, "oidc_client", oidc_client) def tio_exc(token: str) -> JSON: - raise OpenIDTokenInvalid() + raise Exception("OIDC validation is disabled") app = Flask("test-verify-auth-oidc-invalid") app.logger = make_logger @@ -693,7 +684,7 @@ def test_verify_auth_api_key( monkeypatch.setattr(Auth, "oidc_client", oidc_client) def tio_exc(token: str) -> JSON: - raise OpenIDTokenInvalid() + raise Exception("OIDC validation is disabled") app = Flask("test_verify_auth_api_key") app.logger = make_logger @@ -716,7 +707,7 @@ def test_verify_auth_api_key_invalid( monkeypatch.setattr(Auth, "oidc_client", oidc_client) def tio_exc(token: str) -> JSON: - raise OpenIDTokenInvalid() + raise Exception("OIDC validation is disabled") app = Flask("test_verify_auth_api_key_invalid") app.logger = make_logger