From 29cbb184bb0475f8deca8f4d8239570ebfe9ddaa Mon Sep 17 00:00:00 2001 From: jason Date: Wed, 23 Dec 2020 14:24:51 -0700 Subject: [PATCH] Allow special characters in issuer value The recommended value for issuer is a URL with an https:// prefix. The current strategy of defining keys using an uppercase issuer as the variable suffix does not allow for special characters in the name. This change moves the key values to a dictionary keyed by the issuer string under the new `JWT_KEYS` setting while deprecating and maintaining backward compatibility for the original setting. --- README.md | 40 ++++++++++++++-------- oauth2_provider_jwt/utils.py | 49 ++++++++++++++++++++++----- oauth2_provider_jwt/views.py | 12 +++++-- tests/settings.py | 11 ++++-- tests/test_authentication.py | 10 +++--- tests/test_utils.py | 65 +++++++++++++++++++++++++++++++++--- tests/test_views.py | 13 +++++--- 7 files changed, 162 insertions(+), 38 deletions(-) diff --git a/README.md b/README.md index 883542c..7bf0e66 100644 --- a/README.md +++ b/README.md @@ -108,8 +108,8 @@ AUTHENTICATION_BACKENDS = ( Now we need to set up a `JWT_ISSUER` variable in our config, which will be the name of the issuer. Take the private key that we generated before -and store it in a `JWT_PRIVATE_KEY_` variable. Finally, define the -ID attribute you wish to use in `JWT_ID_ATTRIBUTE`. While this can be any +and store it in a `JWT_KEYS[]['private']` variable. Finally, define +the ID attribute you wish to use in `JWT_ID_ATTRIBUTE`. While this can be any attribute on your user model it should be unique. Also you have to set your JWT-encoding Algorithm if it's different than @@ -119,26 +119,39 @@ For example: ```python # settings.py - -JWT_ISSUER = 'OneIssuer' -JWT_ID_ATTRIBUTE = 'email' -JWT_PRIVATE_KEY_ONEISSUER = """ +jwt_private_key = """ -----BEGIN RSA PRIVATE KEY----- MIIBOAIBAAJAbCmbRUsLrsv0/Cq7DVDpUooPS1V2sr0EhTZAZmJhid2o/+ya/28m ... 6D0+csaGDlZ9GbrTpTJUObNENNHqfrHGfqzDxQ== -----END RSA PRIVATE KEY----- """ -``` +jwt_public_key = """ +-----BEGIN PUBLIC KEY----- +MIIBIjANBgkqhkiG9w0BAQEFAAOCAQ8AMIIBCgKCAQEAmeym/g734dlLr3bEeVSR +... +6wIDAQAB +-----END PUBLIC KEY----- +""" +JWT_ISSUER = 'OneIssuer' +JWT_ID_ATTRIBUTE = 'email' -\* *Note that storing hardcoded secrets in the settings is a bad practice and +JWT_KEYS = { + 'OneIssuer': { # Or JWT_ISSUER + 'private': jwt_private_key + 'public': jwt_public_key + } +} + +``` +* NOTE: storing hardcoded secrets in the settings is a bad practice and can lead to severe security breaches in your code. We recommend using -environment variables for this purpose.* +environment variables for this purpose. -\** *Note that you can configure only **one** JWT-Encoding Algorithm in +* NOTE: you can configure only **one** JWT-Encoding Algorithm in `JWT_ENC_ALGORITHM`. But you can set multiple allowed decoding(verifying) Algorithms in `JWT_JWS_ALGORITHMS` as an array of Strings. It is only useful -if the JWT is from a 3rd Party and you don't know which Algorithm is used.* +if the JWT is from a 3rd Party and you don't know which Algorithm is used. The payload of messages will be by default something like: @@ -216,11 +229,12 @@ REST_FRAMEWORK = { ``` Also, you will need to add to the settings every public key of all the -possible token issuers, if configured, using a variable `JWT_PUBLIC_KEY_`: +possible token issuers, if configured, using a variable +`JWT_KEYS[]['public']`: ```python # settings.py -JWT_PUBLIC_KEY_ONEISSUER = """ +JWT_KEYS['OneIssuer']['public'] = """ -----BEGIN PUBLIC KEY----- MFswDQYJKoZIhvcNAQEBBQADSgAwRwJAbCmbRUsLrsv0/Cq7DVDpUooPS1V2sr0E hTZAZmJhid2o/+ya/28muuoQgknEoJz32bKeWuYZrFkRKUrGFnlxHwIDAQAB diff --git a/oauth2_provider_jwt/utils.py b/oauth2_provider_jwt/utils.py index f34c88b..caa0f8d 100644 --- a/oauth2_provider_jwt/utils.py +++ b/oauth2_provider_jwt/utils.py @@ -1,6 +1,7 @@ import base64 from datetime import datetime, timedelta import json +import warnings from django.conf import settings from django.core.exceptions import ImproperlyConfigured @@ -48,11 +49,27 @@ def encode_jwt(payload, issuer=None, headers=None): # RS256 in default, because hardcoded legacy algorithm = getattr(settings, 'JWT_ENC_ALGORITHM', 'RS256') - private_key_name = 'JWT_PRIVATE_KEY_{}'.format(issuer.upper()) - private_key = getattr(settings, private_key_name, None) + try: + private_key = getattr(settings, 'JWT_KEYS', {})[issuer]['private'] + except KeyError: + private_key = None + if not private_key: - raise ImproperlyConfigured('Missing setting {}'.format( - private_key_name)) + # Check the legacy setting + private_key_name = 'JWT_PRIVATE_KEY_{}'.format(issuer.upper()) + try: + private_key = getattr(settings, private_key_name) + if private_key.strip() == '': + # Little hack to pick up the test case where the key is an + # empty string. + raise AttributeError + warnings.warn( + f'{private_key_name} has been deprecated in favor of JWT_KEYS', + DeprecationWarning + ) + except AttributeError: + raise ImproperlyConfigured(f'Missing private key for {issuer}') + encoded = jwt.encode(payload, private_key, algorithm=algorithm, headers=headers) return encoded.decode("utf-8") @@ -78,11 +95,27 @@ def decode_jwt(jwt_value, issuer=None): 'Unable to determine issuer. Token missing iss claim') algorithms = getattr(settings, 'JWT_JWS_ALGORITHMS', ['HS256', 'RS256']) - public_key_name = 'JWT_PUBLIC_KEY_{}'.format(issuer.upper()) - public_key = getattr(settings, public_key_name, None) + + try: + public_key = getattr(settings, 'JWT_KEYS', {})[issuer]['public'] + except KeyError: + public_key = None + if not public_key: - raise ImproperlyConfigured('Missing setting {}'.format( - public_key_name)) + # Check the legacy setting + public_key_name = 'JWT_PUBLIC_KEY_{}'.format(issuer.upper()) + try: + public_key = getattr(settings, public_key_name) + if public_key.strip() == '': + # Little hack to pick up the test case where the key is an + # empty string. + raise AttributeError + warnings.warn( + f'{public_key_name} has been deprecated in favor of JWT_KEYS', + DeprecationWarning + ) + except AttributeError: + raise ImproperlyConfigured(f'Missing public key for {issuer}') decoded = jwt.decode(jwt_value, public_key, algorithms=algorithms) return decoded diff --git a/oauth2_provider_jwt/views.py b/oauth2_provider_jwt/views.py index e8ab753..98a6440 100644 --- a/oauth2_provider_jwt/views.py +++ b/oauth2_provider_jwt/views.py @@ -82,8 +82,16 @@ def _get_access_token_jwt(self, request, content): @staticmethod def _is_jwt_config_set(): issuer = getattr(settings, 'JWT_ISSUER', '') - private_key_name = 'JWT_PRIVATE_KEY_{}'.format(issuer.upper()) - private_key = getattr(settings, private_key_name, None) + try: + private_key = getattr(settings, 'JWT_KEYS', {})[issuer]['private'] + except KeyError: + private_key = None + + # Deprecated legacy setting + if not private_key: + private_key_name = 'JWT_PRIVATE_KEY_{}'.format(issuer.upper()) + private_key = getattr(settings, private_key_name, None) + id_attribute = getattr(settings, 'JWT_ID_ATTRIBUTE', None) if issuer and private_key and id_attribute: return True diff --git a/tests/settings.py b/tests/settings.py index c023c8e..f0f1f44 100644 --- a/tests/settings.py +++ b/tests/settings.py @@ -129,7 +129,7 @@ # JWT_ENC_ALGORITHM = 'HS256' # 'RS256' is still default -JWT_PRIVATE_KEY_ISSUER = """ +priv_key = """ -----BEGIN RSA PRIVATE KEY----- MIIBOAIBAAJAbCmbRUsLrsv0/Cq7DVDpUooPS1V2sr0EhTZAZmJhid2o/+ya/28m uuoQgknEoJz32bKeWuYZrFkRKUrGFnlxHwIDAQABAkBILcO2DAxxyx1jIcjNbA8n @@ -141,13 +141,20 @@ -----END RSA PRIVATE KEY----- """ -JWT_PUBLIC_KEY_ISSUER = """ +pub_key = """ -----BEGIN PUBLIC KEY----- MFswDQYJKoZIhvcNAQEBBQADSgAwRwJAbCmbRUsLrsv0/Cq7DVDpUooPS1V2sr0E hTZAZmJhid2o/+ya/28muuoQgknEoJz32bKeWuYZrFkRKUrGFnlxHwIDAQAB -----END PUBLIC KEY----- """ +JWT_KEYS = { + JWT_ISSUER: { + 'private': priv_key, + 'public': pub_key, + } +} + REST_FRAMEWORK = { 'DEFAULT_AUTHENTICATION_CLASSES': ( 'oauth2_provider_jwt.authentication.JWTAuthentication', diff --git a/tests/test_authentication.py b/tests/test_authentication.py index 4221a7d..80b3a62 100644 --- a/tests/test_authentication.py +++ b/tests/test_authentication.py @@ -1,8 +1,9 @@ from datetime import datetime, timedelta import json -from django.test import TestCase +from django.conf import settings from django.contrib.auth import get_user_model +from django.test import TestCase from django.test.utils import override_settings from rest_framework.test import APIClient @@ -134,7 +135,8 @@ def test_post_valid_jwt_with_auth_and_scope_is_valid(self): class ECDSAJWTAuth(JWTAuthenticationTests): @override_settings(JWT_ENC_ALGORITHM="ES256") - @override_settings(JWT_PRIVATE_KEY_ISSUER="""-----BEGIN OPENSSH PRIVATE KEY----- + @override_settings(JWT_KEYS={settings.JWT_ISSUER: { + 'private': """-----BEGIN OPENSSH PRIVATE KEY----- b3BlbnNzaC1rZXktdjEAAAAABG5vbmUAAAAEbm9uZQAAAAAAAAABAAAAaAAAABNlY2RzYS 1zaGEyLW5pc3RwMjU2AAAACG5pc3RwMjU2AAAAQQTRAzrSN7dFmGa05mrQaKfP2xplEFo/ 1InJc3P+pcy1zgN427Y96mmPMMkx1zBk9S0uDFX5WlEJ9UuRxmf+yceVAAAAsJLFlSOSxZ @@ -142,8 +144,8 @@ class ECDSAJWTAuth(JWTAuthenticationTests): atBop8/bGmUQWj/Uiclzc/6lzLXOA3jbtj3qaY8wyTHXMGT1LS4MVflaUQn1S5HGZ/7Jx5 UAAAAhAJlPEfG7s+7zces2RHc1txeyyjwZxCqUqs25kvtO36nrAAAAFnRob21hc0B3b3Jr c3RhdGlvbi1kZWIB ------END OPENSSH PRIVATE KEY-----""") - @override_settings(JWT_PUBLIC_KEY_ISSUER="ecdsa-sha2-nistp256 AAAAE2VjZHNhLXNoYTItbmlzdHAyNTYAAAAIbmlzdHAyNTYAAABBBNEDOtI3t0WYZrTmatBop8/bGmUQWj/Uiclzc/6lzLXOA3jbtj3qaY8wyTHXMGT1LS4MVflaUQn1S5HGZ/7Jx5U= thomas@workstation-deb") # noqa: E501 +-----END OPENSSH PRIVATE KEY-----""", + 'public': 'ecdsa-sha2-nistp256 AAAAE2VjZHNhLXNoYTItbmlzdHAyNTYAAAAIbmlzdHAyNTYAAABBBNEDOtI3t0WYZrTmatBop8/bGmUQWj/Uiclzc/6lzLXOA3jbtj3qaY8wyTHXMGT1LS4MVflaUQn1S5HGZ/7Jx5U= thomas@workstation-deb'}}) # noqa: E501 def setUp(self): self.client = APIClient(enforce_csrf_checks=True) User = get_user_model() diff --git a/tests/test_utils.py b/tests/test_utils.py index e4a664f..e8ac105 100644 --- a/tests/test_utils.py +++ b/tests/test_utils.py @@ -4,11 +4,16 @@ from unittest.mock import patch from unittest import TestCase as PythonTestCase +from django.conf import settings from django.core.exceptions import ImproperlyConfigured from django.test import override_settings import jwt from oauth2_provider_jwt import utils +issuer_special_chars = 'https://api.service.cloud' +privkey = settings.JWT_KEYS[settings.JWT_ISSUER]['private'] +pubkey = settings.JWT_KEYS[settings.JWT_ISSUER]['public'] + class GeneratePayloadTest(PythonTestCase): def _get_payload_args(self): @@ -71,7 +76,7 @@ def _get_payload(self): 'org': 'some_org', } - @override_settings(JWT_PRIVATE_KEY_ISSUER='') + @override_settings(JWT_KEYS={}) def test_encode_jwt_no_private_key_in_setting(self): payload = self._get_payload() self.assertRaises(ImproperlyConfigured, @@ -90,6 +95,22 @@ def test_encode_jwt_rs256(self): json.loads(base64.b64decode(payload).decode("utf-8")), payload_in) + @override_settings(JWT_ISSUER=issuer_special_chars) + @override_settings(JWT_KEYS={issuer_special_chars: {'private': privkey}}) + def test_encode_jwt__issuer_special_chars(self): + payload_in = self._get_payload() + payload_in['iss'] = issuer_special_chars + encoded = utils.encode_jwt(payload_in) + self.assertIn(type(encoded).__name__, ('unicode', 'str')) + headers, payload, verify_signature = encoded.split(".") + self.assertDictEqual( + json.loads(base64.b64decode(headers)), + {"typ": "JWT", "alg": "RS256"}) + payload += '=' * (-len(payload) % 4) # add padding + self.assertEqual( + json.loads(base64.b64decode(payload).decode("utf-8")), + payload_in) + def test_encode_jwt_explicit_issuer(self): payload_in = self._get_payload() payload_in['iss'] = 'different-issuer' @@ -104,7 +125,7 @@ def test_encode_jwt_explicit_issuer(self): json.loads(base64.b64decode(payload).decode("utf-8")), payload_in) - @override_settings(JWT_PRIVATE_KEY_ISSUER='test') + @override_settings(JWT_KEYS={settings.JWT_ISSUER: {'private': 'test'}}) @override_settings(JWT_ENC_ALGORITHM='HS256') def test_encode_jwt_hs256(self): payload_in = self._get_payload() @@ -119,6 +140,21 @@ def test_encode_jwt_hs256(self): json.loads(base64.b64decode(payload).decode('utf-8')), payload_in) + @override_settings(JWT_KEYS={}) + @override_settings(JWT_PRIVATE_KEY_ISSUER=privkey) + def test_encode_jwt__legacy_settings_private_key(self): + payload_in = self._get_payload() + encoded = utils.encode_jwt(payload_in) + self.assertIn(type(encoded).__name__, ('unicode', 'str')) + headers, payload, verify_signature = encoded.split(".") + self.assertDictEqual( + json.loads(base64.b64decode(headers)), + {"typ": "JWT", "alg": "RS256"}) + payload += '=' * (-len(payload) % 4) # add padding + self.assertEqual( + json.loads(base64.b64decode(payload).decode("utf-8")), + payload_in) + class DecodeJWTTest(PythonTestCase): def _get_payload(self): @@ -135,7 +171,7 @@ def _get_payload(self): def test_decode_jwt_invalid(self): self.assertRaises(jwt.InvalidTokenError, utils.decode_jwt, 'abc') - @override_settings(JWT_PUBLIC_KEY_ISSUER='') + @override_settings(JWT_KEYS={settings.JWT_ISSUER: {'private': privkey}}) def test_decode_jwt_public_key_not_found(self): payload = self._get_payload() jwt_value = utils.encode_jwt(payload) @@ -157,6 +193,16 @@ def test_decode_jwt_rs256(self): payload_out = utils.decode_jwt(jwt_value) self.assertDictEqual(payload, payload_out) + @override_settings(JWT_ISSUER=issuer_special_chars) + @override_settings(JWT_KEYS={ + issuer_special_chars: {'private': privkey, 'public': pubkey}}) + def test_decode_jwt__issuer_special_chars(self): + payload = self._get_payload() + payload['iss'] = issuer_special_chars + jwt_value = utils.encode_jwt(payload) + payload_out = utils.decode_jwt(jwt_value) + self.assertDictEqual(payload, payload_out) + def test_decode_jwt_explicit_issuer(self): payload = self._get_payload() payload['iss'] = 'different-issuer' @@ -164,11 +210,20 @@ def test_decode_jwt_explicit_issuer(self): payload_out = utils.decode_jwt(jwt_value, 'issuer') self.assertDictEqual(payload, payload_out) - @override_settings(JWT_PRIVATE_KEY_ISSUER='test') - @override_settings(JWT_PUBLIC_KEY_ISSUER='test') + @override_settings(JWT_KEYS={ + settings.JWT_ISSUER: {'private': 'test', 'public': 'test'}}) @override_settings(JWT_ENC_ALGORITHM='HS256') def test_decode_jwt_hs256(self): payload = self._get_payload() jwt_value = utils.encode_jwt(payload) payload_out = utils.decode_jwt(jwt_value) self.assertDictEqual(payload, payload_out) + + @override_settings(JWT_KEYS={}) + @override_settings(JWT_PRIVATE_KEY_ISSUER=privkey) + @override_settings(JWT_PUBLIC_KEY_ISSUER=pubkey) + def test_decode_jwt__legacy_settings_public_key(self): + payload = self._get_payload() + jwt_value = utils.encode_jwt(payload) + payload_out = utils.decode_jwt(jwt_value) + self.assertDictEqual(payload, payload_out) diff --git a/tests/test_views.py b/tests/test_views.py index c391ea9..06b8770 100644 --- a/tests/test_views.py +++ b/tests/test_views.py @@ -80,23 +80,28 @@ def tearDown(self): self.dev_user.delete() @override_settings(JWT_ISSUER='api') - @override_settings(JWT_PRIVATE_KEY_API='somevalue') + @override_settings(JWT_KEYS={'api': {'private': 'somevalue'}}) def test_is_jwt_config_set(self): self.assertTrue(TokenView._is_jwt_config_set()) - @override_settings(JWT_ISSUER='') + @override_settings(JWT_ISSUER='api') @override_settings(JWT_PRIVATE_KEY_API='somevalue') + def test_is_jwt_config_set_legacy(self): + self.assertTrue(TokenView._is_jwt_config_set()) + + @override_settings(JWT_ISSUER='') + @override_settings(JWT_KEYS={'api': {'private': 'somevalue'}}) def test_is_jwt_config_not_set_missing_issuer(self): self.assertFalse(TokenView._is_jwt_config_set()) @override_settings() - @override_settings(JWT_PRIVATE_KEY_API='somevalue') + @override_settings(JWT_KEYS={'api': {'private': 'somevalue'}}) def test_is_jwt_config_not_set_none_issuer(self): del settings.JWT_ISSUER self.assertFalse(TokenView._is_jwt_config_set()) @override_settings(JWT_ISSUER='api') - @override_settings(JWT_PRIVATE_KEY_API='') + @override_settings(JWT_KEYS={'api': {'private': ''}}) def test_is_jwt_config_not_set_missing_private_key(self): self.assertFalse(TokenView._is_jwt_config_set())