From 9fb85715cb7400835f6d69f0d877cd2e224f7a33 Mon Sep 17 00:00:00 2001 From: Dannon Baker Date: Mon, 8 Feb 2021 21:48:37 -0500 Subject: [PATCH 1/7] Consolidate token decode handling in custos_authnz, fix 'aud' audience validation --- lib/galaxy/authnz/custos_authnz.py | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/lib/galaxy/authnz/custos_authnz.py b/lib/galaxy/authnz/custos_authnz.py index 39c5b5d3653..33bd6a1c1f1 100644 --- a/lib/galaxy/authnz/custos_authnz.py +++ b/lib/galaxy/authnz/custos_authnz.py @@ -42,6 +42,9 @@ class CustosAuthnz(IdentityProvider): elif provider == 'keycloak': self._load_config_for_keycloak() + def _decode_token(self, token): + return jwt.decode(token, audience=self.config['client_id'], options={"verify_signature": False}) + def authenticate(self, trans, idphint=None): base_authorize_url = self.config['authorization_endpoint'] scopes = ['openid', 'email', 'profile'] @@ -77,7 +80,7 @@ class CustosAuthnz(IdentityProvider): # Get nonce from token['id_token'] and validate. 'nonce' in the # id_token is a hash of the nonce stored in the NONCE_COOKIE_NAME # cookie. - id_token_decoded = jwt.decode(id_token, options={"verify_signature": False}) + id_token_decoded = self._decode_token(id_token) nonce_hash = id_token_decoded['nonce'] self._validate_nonce(trans, nonce_hash) @@ -144,7 +147,7 @@ class CustosAuthnz(IdentityProvider): # Get nonce from token['id_token'] and validate. 'nonce' in the # id_token is a hash of the nonce stored in the NONCE_COOKIE_NAME # cookie. - userinfo = jwt.decode(id_token, options={"verify_signature": False}) + userinfo = self._decode_token(id_token) # Get userinfo and create Galaxy user record email = userinfo['email'] @@ -180,7 +183,7 @@ class CustosAuthnz(IdentityProvider): raise Exception("User is not associated with provider {}".format(self.config["provider"])) if len(provider_tokens) > 1: for idx, token in enumerate(provider_tokens): - id_token_decoded = jwt.decode(token.id_token, options={"verify_signature": False}) + id_token_decoded = self._decode_token(token.id_token) if (id_token_decoded['email'] == email): index = idx trans.sa_session.delete(provider_tokens[index]) From b3290b57a943f3cf7be7b079987290f6d440cb42 Mon Sep 17 00:00:00 2001 From: Dannon Baker Date: Tue, 9 Feb 2021 11:31:36 -0500 Subject: [PATCH 2/7] Allow decoding of expired tokens for display, since a user might want to kill these --- lib/galaxy/webapps/galaxy/controllers/authnz.py | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/lib/galaxy/webapps/galaxy/controllers/authnz.py b/lib/galaxy/webapps/galaxy/controllers/authnz.py index f3b11fcde4b..80ff2cf9922 100644 --- a/lib/galaxy/webapps/galaxy/controllers/authnz.py +++ b/lib/galaxy/webapps/galaxy/controllers/authnz.py @@ -42,7 +42,10 @@ class OIDC(JSAppLauncher): rtv.append({'id': trans.app.security.encode_id(authnz.id), 'provider': authnz.provider, 'email': authnz.uid}) # Add cilogon and custos identities for token in trans.user.custos_auth: - userinfo = jwt.decode(token.id_token, options={"verify_signature": False}) + # for purely displaying the info to user, we bypass verification of + # signature, audience, and expiration as that's potentially useful + # information to share with the end user + userinfo = jwt.decode(token.id_token, options={'verify_signature': False, 'verify_aud': False, 'verify_exp': False}) rtv.append({'id': trans.app.security.encode_id(token.id), 'provider': token.provider, 'email': userinfo['email']}) return rtv From 8d5188b32c016c5e642415d5ede856daf84dfec4 Mon Sep 17 00:00:00 2001 From: Dannon Baker Date: Tue, 9 Feb 2021 11:52:20 -0500 Subject: [PATCH 3/7] Include token expiration in API response --- lib/galaxy/webapps/galaxy/controllers/authnz.py | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/lib/galaxy/webapps/galaxy/controllers/authnz.py b/lib/galaxy/webapps/galaxy/controllers/authnz.py index 80ff2cf9922..a79fe92e7b0 100644 --- a/lib/galaxy/webapps/galaxy/controllers/authnz.py +++ b/lib/galaxy/webapps/galaxy/controllers/authnz.py @@ -3,6 +3,7 @@ OAuth 2.0 and OpenID Connect Authentication and Authorization Controller. """ +import datetime import json import logging @@ -46,7 +47,12 @@ class OIDC(JSAppLauncher): # signature, audience, and expiration as that's potentially useful # information to share with the end user userinfo = jwt.decode(token.id_token, options={'verify_signature': False, 'verify_aud': False, 'verify_exp': False}) - rtv.append({'id': trans.app.security.encode_id(token.id), 'provider': token.provider, 'email': userinfo['email']}) + rtv.append({ + 'id': trans.app.security.encode_id(token.id), + 'provider': token.provider, + 'email': userinfo['email'], + 'expiration': str(datetime.datetime.utcfromtimestamp(userinfo['exp'])) + }) return rtv @web.json From 888a909a35e29029a703ea5dcf5bee4f123b7e7c Mon Sep 17 00:00:00 2001 From: Dannon Baker Date: Tue, 9 Feb 2021 11:52:54 -0500 Subject: [PATCH 4/7] Wrap this in exception handling, provide error message in response when it goes weird --- .../webapps/galaxy/controllers/authnz.py | 21 ++++++++++++------- 1 file changed, 14 insertions(+), 7 deletions(-) diff --git a/lib/galaxy/webapps/galaxy/controllers/authnz.py b/lib/galaxy/webapps/galaxy/controllers/authnz.py index a79fe92e7b0..5b7208622f5 100644 --- a/lib/galaxy/webapps/galaxy/controllers/authnz.py +++ b/lib/galaxy/webapps/galaxy/controllers/authnz.py @@ -46,13 +46,20 @@ class OIDC(JSAppLauncher): # for purely displaying the info to user, we bypass verification of # signature, audience, and expiration as that's potentially useful # information to share with the end user - userinfo = jwt.decode(token.id_token, options={'verify_signature': False, 'verify_aud': False, 'verify_exp': False}) - rtv.append({ - 'id': trans.app.security.encode_id(token.id), - 'provider': token.provider, - 'email': userinfo['email'], - 'expiration': str(datetime.datetime.utcfromtimestamp(userinfo['exp'])) - }) + try: + userinfo = jwt.decode(token.id_token, options={'verify_signature': False, 'verify_aud': False, 'verify_exp': False}) + rtv.append({ + 'id': trans.app.security.encode_id(token.id), + 'provider': token.provider, + 'email': userinfo['email'], + 'expiration': str(datetime.datetime.utcfromtimestamp(userinfo['exp'])) + }) + except Exception: + rtv.append({ + 'id': trans.app.security.encode_id(token.id), + 'provider': token.provider, + 'error': "Unable to decode token" + }) return rtv @web.json From 90c62d01cf73a55c057b3998fdf02caec049fe3b Mon Sep 17 00:00:00 2001 From: Dannon Baker Date: Wed, 10 Feb 2021 10:40:44 -0500 Subject: [PATCH 5/7] Fix test, some cleanup --- test/unit/authnz/test_custos_authnz.py | 48 +++++++++++++++----------- 1 file changed, 28 insertions(+), 20 deletions(-) diff --git a/test/unit/authnz/test_custos_authnz.py b/test/unit/authnz/test_custos_authnz.py index 4a201a5cd9f..2f9bcbe1c5e 100644 --- a/test/unit/authnz/test_custos_authnz.py +++ b/test/unit/authnz/test_custos_authnz.py @@ -39,26 +39,28 @@ class CustosAuthnzTestCase(unittest.TestCase): def setUp(self): self.orig_requests_get = requests.get - requests.get = self.mockRequest({ - self._get_well_known_url(): { - "authorization_endpoint": "https://test-auth-endpoint", - "token_endpoint": "https://test-token-endpoint", - "userinfo_endpoint": "https://test-userinfo-endpoint", - "end_session_endpoint": "https://test-end-session-endpoint" - }, - self._get_credential_url(): { - "iam_client_secret": "TESTSECRET" + requests.get = self.mockRequest( + { + self._get_well_known_url(): { + "authorization_endpoint": "https://test-auth-endpoint", + "token_endpoint": "https://test-token-endpoint", + "userinfo_endpoint": "https://test-userinfo-endpoint", + "end_session_endpoint": "https://test-end-session-endpoint", + }, + self._get_credential_url(): {"iam_client_secret": "TESTSECRET"}, } - }) - self.custos_authnz = custos_authnz.CustosAuthnz('Custos', { - 'VERIFY_SSL': True - }, { - 'url': self._get_idp_url(), - 'client_id': 'test-client-id', - 'client_secret': 'test-client-secret', - 'redirect_uri': 'https://test-redirect-uri', - 'realm': 'test-realm' - }) + ) + self.custos_authnz = custos_authnz.CustosAuthnz( + "Custos", + {"VERIFY_SSL": True}, + { + "url": self._get_idp_url(), + "client_id": "test-client-id", + "client_secret": "test-client-secret", + "redirect_uri": "https://test-redirect-uri", + "realm": "test-realm", + }, + ) self.setupMocks() self.test_state = "abc123" self.test_nonce = b"4662892146306485421546981092" @@ -84,7 +86,13 @@ class CustosAuthnzTestCase(unittest.TestCase): @property def test_id_token(self): - return unicodify(jwt.encode({'nonce': self.test_nonce_hash}, key=None, algorithm=None)) + return unicodify( + jwt.encode( + {"nonce": self.test_nonce_hash, "aud": "test-client-id"}, + key=None, + algorithm=None, + ) + ) def mock_create_oauth2_session(self, custos_authnz): orig_create_oauth2_session = custos_authnz._create_oauth2_session From bc930e15b29ea98b2483de1c37be6a0ced4d3730 Mon Sep 17 00:00:00 2001 From: Dannon Baker Date: Wed, 10 Feb 2021 10:42:06 -0500 Subject: [PATCH 6/7] Custos test fix -- the last --- test/unit/authnz/test_custos_authnz.py | 19 +++++++++++++------ 1 file changed, 13 insertions(+), 6 deletions(-) diff --git a/test/unit/authnz/test_custos_authnz.py b/test/unit/authnz/test_custos_authnz.py index 2f9bcbe1c5e..a85081c6dd2 100644 --- a/test/unit/authnz/test_custos_authnz.py +++ b/test/unit/authnz/test_custos_authnz.py @@ -369,12 +369,19 @@ class CustosAuthnzTestCase(unittest.TestCase): ) self.assertEqual(0, len(self.trans.sa_session.items)) - test_id_token = unicodify(jwt.encode({ - 'nonce': self.test_nonce_hash, - 'email': self.test_email, - 'preferred_username': self.test_username, - 'sub': self.test_sub - }, key=None, algorithm=None)) + test_id_token = unicodify( + jwt.encode( + { + "nonce": self.test_nonce_hash, + "email": self.test_email, + "preferred_username": self.test_username, + "sub": self.test_sub, + "aud": "test-client-id", + }, + key=None, + algorithm=None, + ) + ) self._raw_token = { "access_token": self.test_access_token, From 6ddf49c7556655ed58d286a33db066c8f00c6673 Mon Sep 17 00:00:00 2001 From: Dannon Baker Date: Sun, 14 Feb 2021 08:39:50 -0500 Subject: [PATCH 7/7] Clarify decode token method name (no point in parameterizing this, it needs to be used consistently like this, only here in the custos module) --- lib/galaxy/authnz/custos_authnz.py | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/lib/galaxy/authnz/custos_authnz.py b/lib/galaxy/authnz/custos_authnz.py index 33bd6a1c1f1..92c3074becd 100644 --- a/lib/galaxy/authnz/custos_authnz.py +++ b/lib/galaxy/authnz/custos_authnz.py @@ -42,7 +42,7 @@ class CustosAuthnz(IdentityProvider): elif provider == 'keycloak': self._load_config_for_keycloak() - def _decode_token(self, token): + def _decode_token_no_signature(self, token): return jwt.decode(token, audience=self.config['client_id'], options={"verify_signature": False}) def authenticate(self, trans, idphint=None): @@ -80,7 +80,7 @@ class CustosAuthnz(IdentityProvider): # Get nonce from token['id_token'] and validate. 'nonce' in the # id_token is a hash of the nonce stored in the NONCE_COOKIE_NAME # cookie. - id_token_decoded = self._decode_token(id_token) + id_token_decoded = self._decode_token_no_signature(id_token) nonce_hash = id_token_decoded['nonce'] self._validate_nonce(trans, nonce_hash) @@ -147,7 +147,7 @@ class CustosAuthnz(IdentityProvider): # Get nonce from token['id_token'] and validate. 'nonce' in the # id_token is a hash of the nonce stored in the NONCE_COOKIE_NAME # cookie. - userinfo = self._decode_token(id_token) + userinfo = self._decode_token_no_signature(id_token) # Get userinfo and create Galaxy user record email = userinfo['email'] @@ -183,7 +183,7 @@ class CustosAuthnz(IdentityProvider): raise Exception("User is not associated with provider {}".format(self.config["provider"])) if len(provider_tokens) > 1: for idx, token in enumerate(provider_tokens): - id_token_decoded = self._decode_token(token.id_token) + id_token_decoded = self._decode_token_no_signature(token.id_token) if (id_token_decoded['email'] == email): index = idx trans.sa_session.delete(provider_tokens[index])