From daa51d766f0490cb07b57db4891ac45f652d45a2 Mon Sep 17 00:00:00 2001 From: Matthieu Huin Date: Wed, 23 Dec 2020 10:26:08 +0100 Subject: [PATCH] Bump pyjwt to 2.0.0 The release of pyjwt 2.0.0 changed the behavior of some functions, which caused errors. Fix the errors, use pyjwt 2.0.0's better handling of JWKS, and pin requirement to 2.X to avoid future potential API breaking changes. Change-Id: Ibef736e0f635dfaf4477cc2a90a22665da9f1959 --- requirements.txt | 3 +- tests/unit/test_auth.py | 44 ++++++++++++++-------- tests/unit/test_client.py | 4 +- tests/unit/test_web.py | 52 +++++++++++++------------- tests/zuul_client/test_zuulclient.py | 10 ++--- zuul/cmd/client.py | 2 +- zuul/driver/auth/jwt.py | 34 ++++++----------- zuul/driver/github/githubconnection.py | 2 +- zuul/lib/auth.py | 2 +- 9 files changed, 76 insertions(+), 77 deletions(-) diff --git a/requirements.txt b/requirements.txt index da3932b84a..6dd38dd824 100644 --- a/requirements.txt +++ b/requirements.txt @@ -24,8 +24,7 @@ alembic cryptography>=1.6 cachecontrol cachetools -# PyJWT 2.0.0 has API breaking changes -pyjwt<2.0.0 +pyjwt>=2.0.0,<3.0 iso8601 psutil fb-re2>=1.0.6 diff --git a/tests/unit/test_auth.py b/tests/unit/test_auth.py index 768db8d734..74fdf1800b 100644 --- a/tests/unit/test_auth.py +++ b/tests/unit/test_auth.py @@ -17,6 +17,7 @@ import json from unittest import mock import os.path import jwt +from io import StringIO import time from zuul.driver import auth @@ -31,10 +32,10 @@ with open(os.path.join(FIXTURE_DIR, algo = jwt.algorithms.RSAAlgorithm(jwt.algorithms.RSAAlgorithm.SHA256) with open(os.path.join(FIXTURE_DIR, 'auth/oidc-key'), 'r') as k: - OIDC_PRIVATE_KEY = algo.prepare_key(k.read().encode('utf-8')) + OIDC_PRIVATE_KEY = algo.prepare_key(k.read()) with open(os.path.join(FIXTURE_DIR, 'auth/oidc-key.pub'), 'r') as k: - pub_key = algo.prepare_key(k.read().encode('utf-8')) + pub_key = algo.prepare_key(k.read()) pub_jwk = algo.to_jwk(pub_key) key = { "kid": "OwO", @@ -52,17 +53,6 @@ with open(os.path.join(FIXTURE_DIR, } -def mock_get(url, params=None, **kwargs): - if url == ("https://my.oidc.provider/auth/realms/realm-one/" - ".well-known/openid-configuration"): - return FakeResponse(FAKE_WELL_KNOWN_CONFIG) - elif url == ("https://my.oidc.provider/auth/realms/realm-one/" - "protocol/openid-connect/certs"): - return FakeResponse(FAKE_CERTS) - else: - raise Exception("Unknown URL %s" % url) - - class FakeResponse: def __init__(self, json_dict): self._json = json_dict @@ -71,6 +61,25 @@ class FakeResponse: return self._json +def mock_get(url, params=None, **kwargs): + if url == ("https://my.oidc.provider/auth/realms/realm-one/" + ".well-known/openid-configuration"): + return FakeResponse(FAKE_WELL_KNOWN_CONFIG) + else: + raise Exception("Unknown URL %s" % url) + + +def mock_urlopen(url, *args, **kwargs): + if url == ("https://my.oidc.provider/auth/realms/realm-one/" + "protocol/openid-connect/certs"): + io = StringIO() + json.dump(FAKE_CERTS, io) + io.seek(0) + return io + else: + raise Exception("Unknown URL %s" % url) + + class TestOpenIDConnectAuthenticator(BaseTestCase): def test_decodeToken(self): """Test the decoding workflow""" @@ -92,6 +101,9 @@ class TestOpenIDConnectAuthenticator(BaseTestCase): algorithm='RS256', headers={'kid': 'OwO'}) with mock.patch('requests.get', side_effect=mock_get): - decoded = OIDCAuth.decodeToken(token) - for claim in payload.keys(): - self.assertEqual(payload[claim], decoded[claim]) + # patching call in PyJWKClient's fetch_data + with mock.patch('urllib.request.urlopen', + side_effect=mock_urlopen): + decoded = OIDCAuth.decodeToken(token) + for claim in payload.keys(): + self.assertEqual(payload[claim], decoded[claim]) diff --git a/tests/unit/test_client.py b/tests/unit/test_client.py index ea94382b0d..7982e06cce 100644 --- a/tests/unit/test_client.py +++ b/tests/unit/test_client.py @@ -136,9 +136,9 @@ class TestWebTokenClient(BaseClientTestCase): key=self.config.get( 'auth zuul_operator', 'secret'), - algorithm=self.config.get( + algorithms=[self.config.get( 'auth zuul_operator', - 'driver'), + 'driver')], audience=self.config.get( 'auth zuul_operator', 'client_id'),) diff --git a/tests/unit/test_web.py b/tests/unit/test_web.py index e58d6d743d..68e485a986 100644 --- a/tests/unit/test_web.py +++ b/tests/unit/test_web.py @@ -1351,7 +1351,7 @@ class TestTenantScopedWebApi(BaseTestWeb): }, 'exp': time.time() + 3600} token = jwt.encode(authz, key='OnlyZuulNoDana', - algorithm='HS256').decode('utf-8') + algorithm='HS256') resp = self.post_url( "api/tenant/tenant-one/project/org/project/autohold", headers={'Authorization': 'Bearer %s' % token}, @@ -1386,7 +1386,7 @@ class TestTenantScopedWebApi(BaseTestWeb): }, 'exp': time.time() - 3600} token = jwt.encode(authz, key='NoDanaOnlyZuul', - algorithm='HS256').decode('utf-8') + algorithm='HS256') resp = self.post_url( "api/tenant/tenant-one/project/org/project/autohold", headers={'Authorization': 'Bearer %s' % token}, @@ -1421,7 +1421,7 @@ class TestTenantScopedWebApi(BaseTestWeb): }, 'exp': time.time() + 3600} token = jwt.encode(authz, key='NoDanaOnlyZuul', - algorithm='HS256').decode('utf-8') + algorithm='HS256') resp = self.post_url( "api/tenant/tenant-one/project/org/project/autohold", headers={'Authorization': 'Bearer %s' % token}, @@ -1461,7 +1461,7 @@ class TestTenantScopedWebApi(BaseTestWeb): 'ref': None, 'node_hold_expiration': None} good_token = jwt.encode(good_authz, key='NoDanaOnlyZuul', - algorithm='HS256').decode('utf-8') + algorithm='HS256') req = self.post_url( 'api/tenant/tenant-one/project/org/project/autohold', headers={'Authorization': 'Bearer %s' % good_token}, @@ -1495,7 +1495,7 @@ class TestTenantScopedWebApi(BaseTestWeb): }, 'exp': time.time() + 3600} token = jwt.encode(authz, key='NoDanaOnlyZuul', - algorithm='HS256').decode('utf-8') + algorithm='HS256') req = self.post_url( 'api/tenant/tenant-one/project/org/project/autohold', headers={'Authorization': 'Bearer %s' % token}, @@ -1521,7 +1521,7 @@ class TestTenantScopedWebApi(BaseTestWeb): def _init_autohold_delete(self, authz): token = jwt.encode(authz, key='NoDanaOnlyZuul', - algorithm='HS256').decode('utf-8') + algorithm='HS256') client = zuul.rpcclient.RPCClient('127.0.0.1', self.gearman_server.port) @@ -1559,7 +1559,7 @@ class TestTenantScopedWebApi(BaseTestWeb): }, 'exp': time.time() + 3600} bad_token = jwt.encode(bad_authz, key='NoDanaOnlyZuul', - algorithm='HS256').decode('utf-8') + algorithm='HS256') resp = self.delete_url( "api/tenant/tenant-one/autohold/%s" % request_id, headers={'Authorization': 'Bearer %s' % bad_token}) @@ -1603,7 +1603,7 @@ class TestTenantScopedWebApi(BaseTestWeb): }, 'exp': time.time() + 3600} token = jwt.encode(authz, key='NoDanaOnlyZuul', - algorithm='HS256').decode('utf-8') + algorithm='HS256') path = "api/tenant/%(tenant)s/project/%(project)s/enqueue" enqueue_args = {'tenant': 'tenant-one', 'project': 'org/project', } @@ -1655,7 +1655,7 @@ class TestTenantScopedWebApi(BaseTestWeb): }, 'exp': time.time() + 3600} token = jwt.encode(authz, key='NoDanaOnlyZuul', - algorithm='HS256').decode('utf-8') + algorithm='HS256') req = self.post_url(path % enqueue_args, headers={'Authorization': 'Bearer %s' % token}, json=ref) @@ -1696,7 +1696,7 @@ class TestTenantScopedWebApi(BaseTestWeb): }, 'exp': time.time() + 3600} token = jwt.encode(authz, key='NoDanaOnlyZuul', - algorithm='HS256').decode('utf-8') + algorithm='HS256') path = "api/tenant/%(tenant)s/project/%(project)s/dequeue" dequeue_args = {'tenant': 'tenant-one', 'project': 'org/project', } @@ -1818,7 +1818,7 @@ class TestTenantScopedWebApi(BaseTestWeb): 'exp': time.time() + 3600, 'iat': time.time()} token = jwt.encode(authz, key='NoDanaOnlyZuul', - algorithm='HS256').decode('utf-8') + algorithm='HS256') req = self.post_url( 'api/tenant/tenant-one/promote', headers={'Authorization': 'Bearer %s' % token}, @@ -1892,7 +1892,7 @@ class TestTenantScopedWebApiWithAuthRules(BaseTestWeb): }, 'exp': time.time() + 3600} token = jwt.encode(authz, key='NoDanaOnlyZuul', - algorithm='HS256').decode('utf-8') + algorithm='HS256') req = self.post_url( 'api/tenant/tenant-one/project/org/project/autohold', headers={'Authorization': 'Bearer %s' % token}, @@ -1915,7 +1915,7 @@ class TestTenantScopedWebApiWithAuthRules(BaseTestWeb): 'project': project, } token = jwt.encode(authz, key='NoDanaOnlyZuul', - algorithm='HS256').decode('utf-8') + algorithm='HS256') req = self.post_url(path % enqueue_args, headers={'Authorization': 'Bearer %s' % token}, json=change) @@ -1959,7 +1959,7 @@ class TestTenantScopedWebApiWithAuthRules(BaseTestWeb): 'groups': ['ghostbusters', 'secretary'], 'exp': time.time() + 3600} token = jwt.encode(authz, key='NoDanaOnlyZuul', - algorithm='HS256').decode('utf-8') + algorithm='HS256') path = "api/tenant/%(tenant)s/project/%(project)s/enqueue" enqueue_args = {'tenant': 'tenant-one', 'project': 'org/project2', } @@ -1985,7 +1985,7 @@ class TestTenantScopedWebApiWithAuthRules(BaseTestWeb): 'car': 'ecto-1'}, 'exp': time.time() + 3600} token = jwt.encode(authz, key='NoDanaOnlyZuul', - algorithm='HS256').decode('utf-8') + algorithm='HS256') path = "api/tenant/%(tenant)s/project/%(project)s/enqueue" enqueue_args = {'tenant': 'tenant-one', 'project': 'org/project', } @@ -2007,7 +2007,7 @@ class TestTenantScopedWebApiWithAuthRules(BaseTestWeb): 'zuul': {'admin': admin_tenants}, 'exp': time.time() + 3600} token = jwt.encode(authz, key='NoDanaOnlyZuul', - algorithm='HS256').decode('utf-8') + algorithm='HS256') # TODO(mhu) deprecated, remove after next release req = self.get_url('/api/user/authorizations', headers={'Authorization': 'Bearer %s' % token}) @@ -2049,7 +2049,7 @@ class TestTenantScopedWebApiWithAuthRules(BaseTestWeb): authz = test_user['authz'] authz['exp'] = time.time() + 3600 token = jwt.encode(authz, key='NoDanaOnlyZuul', - algorithm='HS256').decode('utf-8') + algorithm='HS256') # TODO(mhu) deprecated, remove after next release req = self.get_url('/api/user/authorizations', headers={'Authorization': 'Bearer %s' % token}) @@ -2116,7 +2116,7 @@ class TestTenantScopedWebApiTokenWithExpiry(BaseTestWeb): }, 'exp': time.time() + 3600} token = jwt.encode(authz, key='NoDanaOnlyZuul', - algorithm='HS256').decode('utf-8') + algorithm='HS256') resp = self.post_url( "api/tenant/tenant-one/project/org/project/autohold", headers={'Authorization': 'Bearer %s' % token}, @@ -2152,7 +2152,7 @@ class TestTenantScopedWebApiTokenWithExpiry(BaseTestWeb): 'exp': time.time() + 7200, 'iat': time.time() + 3600} token = jwt.encode(authz, key='NoDanaOnlyZuul', - algorithm='HS256').decode('utf-8') + algorithm='HS256') resp = self.post_url( "api/tenant/tenant-one/project/org/project/autohold", headers={'Authorization': 'Bearer %s' % token}, @@ -2188,7 +2188,7 @@ class TestTenantScopedWebApiTokenWithExpiry(BaseTestWeb): 'exp': time.time() + 3600, 'iat': time.time()} token = jwt.encode(authz, key='NoDanaOnlyZuul', - algorithm='HS256').decode('utf-8') + algorithm='HS256') time.sleep(10) resp = self.post_url( "api/tenant/tenant-one/project/org/project/autohold", @@ -2232,7 +2232,7 @@ class TestTenantScopedWebApiTokenWithExpiry(BaseTestWeb): 'exp': time.time() + 3600, 'iat': time.time()} token = jwt.encode(authz, key='NoDanaOnlyZuul', - algorithm='HS256').decode('utf-8') + algorithm='HS256') req = self.post_url( 'api/tenant/tenant-one/project/org/project/autohold', headers={'Authorization': 'Bearer %s' % token}, @@ -2336,7 +2336,7 @@ class TestCLIViaWebApi(BaseTestWeb): }, 'exp': time.time() + 3600} token = jwt.encode(authz, key='NoDanaOnlyZuul', - algorithm='HS256').decode('utf-8') + algorithm='HS256') p = subprocess.Popen( [os.path.join(sys.prefix, 'bin/zuul'), '--zuul-url', self.base_url, '--auth-token', token, @@ -2375,7 +2375,7 @@ class TestCLIViaWebApi(BaseTestWeb): }, 'exp': time.time() + 3600} token = jwt.encode(authz, key='NoDanaOnlyZuul', - algorithm='HS256').decode('utf-8') + algorithm='HS256') p = subprocess.Popen( [os.path.join(sys.prefix, 'bin/zuul'), '--zuul-url', self.base_url, '--auth-token', token, @@ -2404,7 +2404,7 @@ class TestCLIViaWebApi(BaseTestWeb): }, 'exp': time.time() + 3600} token = jwt.encode(authz, key='NoDanaOnlyZuul', - algorithm='HS256').decode('utf-8') + algorithm='HS256') p = subprocess.Popen( [os.path.join(sys.prefix, 'bin/zuul'), '--zuul-url', self.base_url, '--auth-token', token, @@ -2440,7 +2440,7 @@ class TestCLIViaWebApi(BaseTestWeb): }, 'exp': time.time() + 3600} token = jwt.encode(authz, key='NoDanaOnlyZuul', - algorithm='HS256').decode('utf-8') + algorithm='HS256') p = subprocess.Popen( [os.path.join(sys.prefix, 'bin/zuul'), '--zuul-url', self.base_url, '--auth-token', token, @@ -2491,7 +2491,7 @@ class TestCLIViaWebApi(BaseTestWeb): }, 'exp': time.time() + 3600} token = jwt.encode(authz, key='NoDanaOnlyZuul', - algorithm='HS256').decode('utf-8') + algorithm='HS256') p = subprocess.Popen( [os.path.join(sys.prefix, 'bin/zuul'), '--zuul-url', self.base_url, '--auth-token', token, diff --git a/tests/zuul_client/test_zuulclient.py b/tests/zuul_client/test_zuulclient.py index 13b18ab61b..2873d94486 100644 --- a/tests/zuul_client/test_zuulclient.py +++ b/tests/zuul_client/test_zuulclient.py @@ -169,7 +169,7 @@ class TestZuulClientAdmin(BaseTestWeb): }, 'exp': time.time() + 3600} token = jwt.encode(authz, key='NoDanaOnlyZuul', - algorithm='HS256').decode('utf-8') + algorithm='HS256') p = subprocess.Popen( ['zuul-client', '--zuul-url', self.base_url, '--auth-token', token, '-v', @@ -209,7 +209,7 @@ class TestZuulClientAdmin(BaseTestWeb): }, 'exp': time.time() + 3600} token = jwt.encode(authz, key='NoDanaOnlyZuul', - algorithm='HS256').decode('utf-8') + algorithm='HS256') p = subprocess.Popen( ['zuul-client', '--zuul-url', self.base_url, '--auth-token', token, '-v', @@ -245,7 +245,7 @@ class TestZuulClientAdmin(BaseTestWeb): }, 'exp': time.time() + 3600} token = jwt.encode(authz, key='NoDanaOnlyZuul', - algorithm='HS256').decode('utf-8') + algorithm='HS256') p = subprocess.Popen( ['zuul-client', '--zuul-url', self.base_url, '--auth-token', token, '-v', @@ -287,7 +287,7 @@ class TestZuulClientAdmin(BaseTestWeb): }, 'exp': time.time() + 3600} token = jwt.encode(authz, key='NoDanaOnlyZuul', - algorithm='HS256').decode('utf-8') + algorithm='HS256') p = subprocess.Popen( ['zuul-client', '--zuul-url', self.base_url, '--auth-token', token, '-v', @@ -338,7 +338,7 @@ class TestZuulClientAdmin(BaseTestWeb): }, 'exp': time.time() + 3600} token = jwt.encode(authz, key='NoDanaOnlyZuul', - algorithm='HS256').decode('utf-8') + algorithm='HS256') p = subprocess.Popen( ['zuul-client', '--zuul-url', self.base_url, '--auth-token', token, '-v', diff --git a/zuul/cmd/client.py b/zuul/cmd/client.py index 8863934140..ccd879f29c 100755 --- a/zuul/cmd/client.py +++ b/zuul/cmd/client.py @@ -581,7 +581,7 @@ class Client(zuul.cmd.ZuulApp): try: auth_token = jwt.encode(token, key=key, - algorithm=driver).decode('utf-8') + algorithm=driver) print("Bearer %s" % auth_token) err_code = 0 except Exception as e: diff --git a/zuul/driver/auth/jwt.py b/zuul/driver/auth/jwt.py index 5df48971b6..7fe23f2ffe 100644 --- a/zuul/driver/auth/jwt.py +++ b/zuul/driver/auth/jwt.py @@ -18,7 +18,6 @@ import math import time import jwt import requests -import json from urllib.parse import urljoin from zuul import exceptions @@ -74,16 +73,16 @@ class JWTAuthenticator(AuthenticatorInterface): except jwt.exceptions.InvalidSignatureError: raise exceptions.AuthTokenInvalidSignatureException( realm=self.realm) - except jwt.DecodeError: + except jwt.exceptions.DecodeError: raise exceptions.AuthTokenUndecodedException( realm=self.realm) except jwt.exceptions.ExpiredSignatureError: raise exceptions.TokenExpiredError( realm=self.realm) - except jwt.InvalidIssuerError: + except jwt.exceptions.InvalidIssuerError: raise exceptions.IssuerUnknownError( realm=self.realm) - except jwt.InvalidAudienceError: + except jwt.exceptions.InvalidAudienceError: raise exceptions.IncorrectAudienceError( realm=self.realm) except Exception as e: @@ -154,7 +153,7 @@ class HS256Authenticator(JWTAuthenticator): def _decode(self, rawToken): return jwt.decode(rawToken, self.secret, issuer=self.issuer_id, audience=self.audience, - algorithms=self.algorithm) + algorithms=[self.algorithm]) class RS256Authenticator(JWTAuthenticator): @@ -172,7 +171,7 @@ class RS256Authenticator(JWTAuthenticator): def _decode(self, rawToken): return jwt.decode(rawToken, self.public_key, issuer=self.issuer_id, audience=self.audience, - algorithms=self.algorithm) + algorithms=[self.algorithm]) class OpenIDConnectAuthenticator(JWTAuthenticator): @@ -203,9 +202,9 @@ class OpenIDConnectAuthenticator(JWTAuthenticator): raise exceptions.JWKSException( realm=self.realm, msg=msg) - # TODO keys can probably be cached + jwks_client = jwt.PyJWKClient(keys_url) try: - certs = requests.get(keys_url).json() + signing_key = jwks_client.get_signing_key(kid=key_id) except Exception as e: msg = 'Could not fetch Identity Provider keys at %s: %s' logger.error(msg % (keys_url, e)) @@ -213,20 +212,9 @@ class OpenIDConnectAuthenticator(JWTAuthenticator): realm=self.realm, msg='There was an error while fetching ' 'keys for Identity Provider, check logs for details') - for key_dict in certs['keys']: - if key_dict.get('kid') == key_id: - # TODO: theoretically two other types of keys are - # supported by the JWKS standard. We should raise an error - # in the unlikely case 'kty' is not RSA. - # (see https://tools.ietf.org/html/rfc7518#section-6.1) - key = jwt.algorithms.RSAAlgorithm.from_jwk( - json.dumps(key_dict)) - algorithm = key_dict.get('alg', None) or self.algorithm - return key, algorithm - raise exceptions.JWKSException( - self.realm, - 'Cannot verify token: public key %s ' - 'not listed by Identity Provider' % key_id) + algorithm = signing_key._jwk_data.get("alg", None) or self.algorithm + key = signing_key.key + return key, algorithm def get_well_known_config(self): issuer = self.issuer_id @@ -258,7 +246,7 @@ class OpenIDConnectAuthenticator(JWTAuthenticator): key, algorithm = self.get_key(key_id) return jwt.decode(rawToken, key, issuer=self.issuer_id, audience=self.audience, - algorithms=algorithm) + algorithms=[algorithm]) AUTHENTICATORS = { diff --git a/zuul/driver/github/githubconnection.py b/zuul/driver/github/githubconnection.py index ac9073ce69..8c1ceecccb 100644 --- a/zuul/driver/github/githubconnection.py +++ b/zuul/driver/github/githubconnection.py @@ -947,7 +947,7 @@ class GithubClientManager: data = {'iat': now, 'exp': expiry, 'iss': self.app_id} app_token = jwt.encode(data, self.app_key, - algorithm='RS256').decode('utf-8') + algorithm='RS256') headers = {'Accept': PREVIEW_JSON_ACCEPT, 'Authorization': 'Bearer %s' % app_token} diff --git a/zuul/lib/auth.py b/zuul/lib/auth.py index effb4bc5ea..a5866e908d 100644 --- a/zuul/lib/auth.py +++ b/zuul/lib/auth.py @@ -74,7 +74,7 @@ class AuthenticatorRegistry(object): cpb.capabilities_registry.register_capabilities('auth', capabilities) def authenticate(self, rawToken): - unverified = jwt.decode(rawToken, verify=False) + unverified = jwt.decode(rawToken, options={'verify_signature': False}) for auth_name in self.authenticators: authenticator = self.authenticators[auth_name] if authenticator.issuer_id == unverified.get('iss', ''):