From 431e92e88df6226fcf7646aed22481122db01260 Mon Sep 17 00:00:00 2001 From: Simon Oliver Tveit Date: Mon, 6 Feb 2023 08:39:14 +0100 Subject: [PATCH 01/38] Add config for nav issued tokens --- python/nav/etc/jwt.conf | 16 +++++++++++--- python/nav/jwtconf.py | 49 ++++++++++++++++++++++++++++++++++++++++- 2 files changed, 61 insertions(+), 4 deletions(-) diff --git a/python/nav/etc/jwt.conf b/python/nav/etc/jwt.conf index 4b4ad4cbfe..cac1b73f2c 100644 --- a/python/nav/etc/jwt.conf +++ b/python/nav/etc/jwt.conf @@ -1,7 +1,17 @@ -# This file contains configuration for JWT issuers that this NAV instance -# should accept tokens from. +# This file contains configuration for JWT tokens. +# This includes tokens generated by NAV itself and external issuers. -# Each issuer is defined in a section. The name of the section should be +# These settings are for tokens issued by NAV itself. +# nav_private_key and nav_public_key should be absolute paths to the related keys in PEM format. +# nav_name will be used for the 'iss' and 'aud' claims. +# The section name should NOT be changed. + +[nav-config] +private_key= +public_key= +name= + +# Custom JWT issuers can be defined in a section. The name of the section should be # the issuers name, the same value as the 'iss' claim will be in tokens # generated by this issuer. This value is often a URL, but does not # have to be. diff --git a/python/nav/jwtconf.py b/python/nav/jwtconf.py index 453392ba67..0aef2f0cf0 100644 --- a/python/nav/jwtconf.py +++ b/python/nav/jwtconf.py @@ -11,10 +11,13 @@ class JWTConf(NAVConfigParser): """jwt.conf config parser""" DEFAULT_CONFIG_FILES = ('jwt.conf',) + NAV_SECTION = "nav-config" def get_issuers_setting(self): - issuers_settings = dict() + issuers_settings = self._get_settings_for_nav_issued_tokens() for section in self.sections(): + if section == self.NAV_SECTION: + continue try: get = partial(self.get, section) key = self._validate_key(get('key')) @@ -54,3 +57,47 @@ def _validate_audience(self, audience): if not audience: raise ConfigurationError("Invalid 'aud': 'aud' must not be empty") return audience + + def _get_nav_issued_token_settings(self): + return partial(self.get, self.NAV_SECTION) + + def get_nav_private_key(self): + get = self._get_nav_issued_token_settings() + path = get('private_key') + return self._read_file(path) + + def get_nav_public_key(self): + get = self._get_nav_issued_token_settings() + path = get('public_key') + return self._read_file(path) + + def get_nav_name(self): + get = self._get_nav_issued_token_settings() + name = get('name') + if not name: + raise ConfigurationError("Invalid 'name': 'name' must not be empty") + return name + + def _get_settings_for_nav_issued_tokens(self): + try: + name = self.get_nav_name() + claims_options = { + 'aud': {'values': [name], 'essential': True}, + 'token_type': {'values': ['access_token'], 'essential': True}, + } + settings = { + name: { + 'type': "PEM", + 'key': self.get_nav_public_key(), + 'claims_options': claims_options, + } + } + return settings + except ( + FileNotFoundError, + ConfigurationError, + configparser.NoSectionError, + configparser.NoOptionError, + ) as error: + _logger.error('Error reading config for NAV issued token: %s', error) + return {} From a34c1bac0c7512a98446ef0ba9b6d407b73d5f09 Mon Sep 17 00:00:00 2001 From: Simon Oliver Tveit Date: Mon, 6 Feb 2023 08:40:24 +0100 Subject: [PATCH 02/38] Validate issuer name --- python/nav/jwtconf.py | 21 ++++++++++++++++++++- 1 file changed, 20 insertions(+), 1 deletion(-) diff --git a/python/nav/jwtconf.py b/python/nav/jwtconf.py index 0aef2f0cf0..fb281d1270 100644 --- a/python/nav/jwtconf.py +++ b/python/nav/jwtconf.py @@ -20,6 +20,7 @@ def get_issuers_setting(self): continue try: get = partial(self.get, section) + issuer = self._validate_issuer(section) key = self._validate_key(get('key')) aud = self._validate_audience(get('aud')) key_type = self._validate_type(get('keytype')) @@ -28,7 +29,7 @@ def get_issuers_setting(self): claims_options = { 'aud': {'values': [aud], 'essential': True}, } - issuers_settings[section] = { + issuers_settings[issuer] = { 'key': key, 'type': key_type, 'claims_options': claims_options, @@ -53,6 +54,24 @@ def _validate_type(self, key_type): ) return key_type + def _validate_issuer(self, section): + if not section: + raise ConfigurationError("Invalid 'issuer': 'issuer' must not be empty") + try: + if section == self.get_nav_name(): + raise ConfigurationError( + "Invalid 'issuer': {} collides with NAVs internal issuer name".format( + section + ) + ) + except ( + configparser.NoSectionError, + configparser.NoOptionError, + ConfigurationError, + ): + pass + return section + def _validate_audience(self, audience): if not audience: raise ConfigurationError("Invalid 'aud': 'aud' must not be empty") From 0763c00f0d40eebb400520f392e8a76516f03f5a Mon Sep 17 00:00:00 2001 From: Simon Oliver Tveit Date: Mon, 6 Feb 2023 08:41:55 +0100 Subject: [PATCH 03/38] Make log message match the context --- python/nav/jwtconf.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/python/nav/jwtconf.py b/python/nav/jwtconf.py index fb281d1270..0e4da4965d 100644 --- a/python/nav/jwtconf.py +++ b/python/nav/jwtconf.py @@ -35,7 +35,7 @@ def get_issuers_setting(self): 'claims_options': claims_options, } except (configparser.Error, ConfigurationError) as error: - _logger.error('Error collecting stats for %s: %s', section, error) + _logger.error('Error reading config for %s: %s', section, error) return issuers_settings def _read_file(self, file): From 8d38a8bae6f9cf38dac0ec6fad5c90a0d83300d1 Mon Sep 17 00:00:00 2001 From: Simon Oliver Tveit Date: Mon, 6 Feb 2023 08:48:39 +0100 Subject: [PATCH 04/38] Add default config --- python/nav/jwtconf.py | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/python/nav/jwtconf.py b/python/nav/jwtconf.py index 0e4da4965d..0e718c28ea 100644 --- a/python/nav/jwtconf.py +++ b/python/nav/jwtconf.py @@ -12,6 +12,12 @@ class JWTConf(NAVConfigParser): DEFAULT_CONFIG_FILES = ('jwt.conf',) NAV_SECTION = "nav-config" + DEFAULT_CONFIG = u""" +[nav-config] +private_key= +public_key= +name=NAV +""" def get_issuers_setting(self): issuers_settings = self._get_settings_for_nav_issued_tokens() From c2044bb0433ffa9a88727a491ed4159b1fcef39e Mon Sep 17 00:00:00 2001 From: Simon Oliver Tveit Date: Mon, 6 Feb 2023 08:53:19 +0100 Subject: [PATCH 05/38] Shorten error message --- python/nav/jwtconf.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/python/nav/jwtconf.py b/python/nav/jwtconf.py index 0e718c28ea..a9ad4d238b 100644 --- a/python/nav/jwtconf.py +++ b/python/nav/jwtconf.py @@ -66,7 +66,7 @@ def _validate_issuer(self, section): try: if section == self.get_nav_name(): raise ConfigurationError( - "Invalid 'issuer': {} collides with NAVs internal issuer name".format( + "Invalid 'issuer': {} collides with internal issuer name".format( section ) ) From 81d0234ad58d7cae19c186b1a262e3ef1f9e2803 Mon Sep 17 00:00:00 2001 From: Simon Oliver Tveit Date: Mon, 6 Feb 2023 15:57:21 +0100 Subject: [PATCH 06/38] Rename function to be more distinct --- python/nav/jwtconf.py | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/python/nav/jwtconf.py b/python/nav/jwtconf.py index a9ad4d238b..599a22884c 100644 --- a/python/nav/jwtconf.py +++ b/python/nav/jwtconf.py @@ -83,21 +83,21 @@ def _validate_audience(self, audience): raise ConfigurationError("Invalid 'aud': 'aud' must not be empty") return audience - def _get_nav_issued_token_settings(self): + def _get_nav_token_config(self): return partial(self.get, self.NAV_SECTION) def get_nav_private_key(self): - get = self._get_nav_issued_token_settings() + get = self._get_nav_token_config() path = get('private_key') return self._read_file(path) def get_nav_public_key(self): - get = self._get_nav_issued_token_settings() + get = self._get_nav_token_config() path = get('public_key') return self._read_file(path) def get_nav_name(self): - get = self._get_nav_issued_token_settings() + get = self._get_nav_token_config() name = get('name') if not name: raise ConfigurationError("Invalid 'name': 'name' must not be empty") From 25f116ffe34220cef6450085185f48c3b0699c71 Mon Sep 17 00:00:00 2001 From: Simon Oliver Tveit Date: Mon, 6 Feb 2023 17:04:24 +0100 Subject: [PATCH 07/38] Return blank config if any error --- python/nav/jwtconf.py | 73 +++++++++++++++------------------ tests/unittests/jwtconf_test.py | 57 +++++++++++++++++-------- 2 files changed, 71 insertions(+), 59 deletions(-) diff --git a/python/nav/jwtconf.py b/python/nav/jwtconf.py index 599a22884c..4467240291 100644 --- a/python/nav/jwtconf.py +++ b/python/nav/jwtconf.py @@ -20,11 +20,11 @@ class JWTConf(NAVConfigParser): """ def get_issuers_setting(self): - issuers_settings = self._get_settings_for_nav_issued_tokens() - for section in self.sections(): - if section == self.NAV_SECTION: - continue - try: + try: + issuers_settings = self._get_settings_for_nav_issued_tokens() + for section in self.sections(): + if section == self.NAV_SECTION: + continue get = partial(self.get, section) issuer = self._validate_issuer(section) key = self._validate_key(get('key')) @@ -40,9 +40,16 @@ def get_issuers_setting(self): 'type': key_type, 'claims_options': claims_options, } - except (configparser.Error, ConfigurationError) as error: - _logger.error('Error reading config for %s: %s', section, error) - return issuers_settings + return issuers_settings + except ( + FileNotFoundError, + configparser.Error, + configparser.NoSectionError, + configparser.NoOptionError, + ConfigurationError, + ) as error: + _logger.error('Error reading jwtconfig %s', error) + return dict() def _read_file(self, file): with open(file, "r") as f: @@ -63,19 +70,12 @@ def _validate_type(self, key_type): def _validate_issuer(self, section): if not section: raise ConfigurationError("Invalid 'issuer': 'issuer' must not be empty") - try: - if section == self.get_nav_name(): - raise ConfigurationError( - "Invalid 'issuer': {} collides with internal issuer name".format( - section - ) + if section == self.get_nav_name(): + raise ConfigurationError( + "Invalid 'issuer': {} collides with internal issuer name".format( + section ) - except ( - configparser.NoSectionError, - configparser.NoOptionError, - ConfigurationError, - ): - pass + ) return section def _validate_audience(self, audience): @@ -104,25 +104,16 @@ def get_nav_name(self): return name def _get_settings_for_nav_issued_tokens(self): - try: - name = self.get_nav_name() - claims_options = { - 'aud': {'values': [name], 'essential': True}, - 'token_type': {'values': ['access_token'], 'essential': True}, - } - settings = { - name: { - 'type': "PEM", - 'key': self.get_nav_public_key(), - 'claims_options': claims_options, - } + name = self.get_nav_name() + claims_options = { + 'aud': {'values': [name], 'essential': True}, + 'token_type': {'values': ['access_token'], 'essential': True}, + } + settings = { + name: { + 'type': "PEM", + 'key': self.get_nav_public_key(), + 'claims_options': claims_options, } - return settings - except ( - FileNotFoundError, - ConfigurationError, - configparser.NoSectionError, - configparser.NoOptionError, - ) as error: - _logger.error('Error reading config for NAV issued token: %s', error) - return {} + } + return settings diff --git a/tests/unittests/jwtconf_test.py b/tests/unittests/jwtconf_test.py index abd9114628..315146feb0 100644 --- a/tests/unittests/jwtconf_test.py +++ b/tests/unittests/jwtconf_test.py @@ -10,27 +10,38 @@ def setUp(self): def test_valid_jwks_config_should_pass(self): config = u""" + [nav-config] + private_key=key + public_key=key + name=issuer-name [jwks-issuer] keytype=JWKS aud=nav key=www.example.com """ expected_settings = { - 'jwks-issuer': { - 'key': 'www.example.com', - 'type': 'JWKS', - 'claims_options': { - 'aud': {'values': ['nav'], 'essential': True}, - }, - } + 'key': 'www.example.com', + 'type': 'JWKS', + 'claims_options': { + 'aud': {'values': ['nav'], 'essential': True}, + }, } + + def read_file_patch(self, file): + return "key" + with patch.object(JWTConf, 'DEFAULT_CONFIG', config): - jwtconf = JWTConf() - settings = jwtconf.get_issuers_setting() - self.assertEqual(settings, expected_settings) + with patch.object(JWTConf, '_read_file', read_file_patch): + jwtconf = JWTConf() + settings = jwtconf.get_issuers_setting() + self.assertEqual(settings['jwks-issuer'], expected_settings) def test_valid_pem_config_should_pass(self): config = u""" + [nav-config] + private_key=key + public_key=key + name=nav-issuer [pem-issuer] keytype=PEM aud=nav @@ -38,13 +49,11 @@ def test_valid_pem_config_should_pass(self): """ pem_key = "PEM KEY" expected_settings = { - 'pem-issuer': { - 'key': pem_key, - 'type': 'PEM', - 'claims_options': { - 'aud': {'values': ['nav'], 'essential': True}, - }, - } + 'key': pem_key, + 'type': 'PEM', + 'claims_options': { + 'aud': {'values': ['nav'], 'essential': True}, + }, } def read_file_patch(self, file): @@ -54,10 +63,14 @@ def read_file_patch(self, file): with patch.object(JWTConf, '_read_file', read_file_patch): jwtconf = JWTConf() settings = jwtconf.get_issuers_setting() - self.assertEqual(settings, expected_settings) + self.assertEqual(settings['pem-issuer'], expected_settings) def test_invalid_ketype_should_fail(self): config = u""" + [nav-config] + private_key=key + public_key=key + name=issuer-name [pem-issuer] keytype=Fake aud=nav @@ -70,6 +83,10 @@ def test_invalid_ketype_should_fail(self): def test_empty_key_should_fail(self): config = u""" + [nav-config] + private_key=key + public_key=key + name=issuer-name [pem-issuer] keytype=JWKS aud=nav @@ -82,6 +99,10 @@ def test_empty_key_should_fail(self): def test_empty_aud_should_fail(self): config = u""" + [nav-config] + private_key=key + public_key=key + name=issuer-name [pem-issuer] keytype=JWKS aud= From b50dcd3e25c607a212bde75248635d3a52399c30 Mon Sep 17 00:00:00 2001 From: Simon Oliver Tveit Date: Mon, 6 Feb 2023 17:06:18 +0100 Subject: [PATCH 08/38] Remove default name value --- python/nav/jwtconf.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/python/nav/jwtconf.py b/python/nav/jwtconf.py index 4467240291..97e0339b4d 100644 --- a/python/nav/jwtconf.py +++ b/python/nav/jwtconf.py @@ -16,7 +16,7 @@ class JWTConf(NAVConfigParser): [nav-config] private_key= public_key= -name=NAV +name= """ def get_issuers_setting(self): From cc67a4651c8d651eef2f2e677ee1482ad581e7df Mon Sep 17 00:00:00 2001 From: Simon Oliver Tveit Date: Mon, 6 Feb 2023 17:07:12 +0100 Subject: [PATCH 09/38] Remove redundant tests other tests cover these things more directly --- tests/unittests/jwtconf_test.py | 48 --------------------------------- 1 file changed, 48 deletions(-) diff --git a/tests/unittests/jwtconf_test.py b/tests/unittests/jwtconf_test.py index 315146feb0..54bce57a07 100644 --- a/tests/unittests/jwtconf_test.py +++ b/tests/unittests/jwtconf_test.py @@ -65,54 +65,6 @@ def read_file_patch(self, file): settings = jwtconf.get_issuers_setting() self.assertEqual(settings['pem-issuer'], expected_settings) - def test_invalid_ketype_should_fail(self): - config = u""" - [nav-config] - private_key=key - public_key=key - name=issuer-name - [pem-issuer] - keytype=Fake - aud=nav - key=key - """ - with patch.object(JWTConf, 'DEFAULT_CONFIG', config): - jwtconf = JWTConf() - settings = jwtconf.get_issuers_setting() - self.assertEqual(settings, dict()) - - def test_empty_key_should_fail(self): - config = u""" - [nav-config] - private_key=key - public_key=key - name=issuer-name - [pem-issuer] - keytype=JWKS - aud=nav - key= - """ - with patch.object(JWTConf, 'DEFAULT_CONFIG', config): - jwtconf = JWTConf() - settings = jwtconf.get_issuers_setting() - self.assertEqual(settings, dict()) - - def test_empty_aud_should_fail(self): - config = u""" - [nav-config] - private_key=key - public_key=key - name=issuer-name - [pem-issuer] - keytype=JWKS - aud= - key=key - """ - with patch.object(JWTConf, 'DEFAULT_CONFIG', config): - jwtconf = JWTConf() - settings = jwtconf.get_issuers_setting() - self.assertEqual(settings, dict()) - def test_validate_key_should_raise_error_if_key_is_empty(self): jwtconf = JWTConf() with self.assertRaises(ConfigurationError): From 285451b6dbaf5ceee05657c4143d62cf83742fe0 Mon Sep 17 00:00:00 2001 From: Simon Oliver Tveit Date: Mon, 6 Feb 2023 17:18:44 +0100 Subject: [PATCH 10/38] Add more tests --- tests/unittests/jwtconf_test.py | 84 +++++++++++++++++++++++++++++++++ 1 file changed, 84 insertions(+) diff --git a/tests/unittests/jwtconf_test.py b/tests/unittests/jwtconf_test.py index 54bce57a07..a7f96d878e 100644 --- a/tests/unittests/jwtconf_test.py +++ b/tests/unittests/jwtconf_test.py @@ -103,3 +103,87 @@ def test_PEM_should_be_a_valid_type(self): jwtconf = JWTConf() validated_type = jwtconf._validate_type(type) self.assertEqual(validated_type, type) + + def test_validate_issuer_should_fail_if_external_name_matches_local_name(self): + config = u""" + [nav-config] + private_key=key + public_key=key + name=issuer-name + [issuer-name] + keytype=PEM + aud=aud + key=key + """ + key = "key_value" + + def read_file_patch(self, file): + return key + + with patch.object(JWTConf, 'DEFAULT_CONFIG', config): + with patch.object(JWTConf, '_read_file', read_file_patch): + jwtconf = JWTConf() + with self.assertRaises(ConfigurationError): + jwtconf._validate_issuer('issuer-name') + + def test_validate_issuer_should_raise_error_if_issuer_is_empty(self): + jwtconf = JWTConf() + with self.assertRaises(ConfigurationError): + jwtconf._validate_issuer("") + + def test_get_nav_private_key_returns_correct_private_key(self): + config = u""" + [nav-config] + private_key=key + public_key=key + name=issuer-name + """ + key = "private-key" + + def read_file_patch(self, file): + return key + + with patch.object(JWTConf, 'DEFAULT_CONFIG', config): + with patch.object(JWTConf, '_read_file', read_file_patch): + jwtconf = JWTConf() + self.assertEqual(jwtconf.get_nav_private_key(), key) + + def test_get_nav_public_key_returns_correct_public_key(self): + config = u""" + [nav-config] + private_key=key + public_key=key + name=issuer-name + """ + key = "private-key" + + def read_file_patch(self, file): + return key + + with patch.object(JWTConf, 'DEFAULT_CONFIG', config): + with patch.object(JWTConf, '_read_file', read_file_patch): + jwtconf = JWTConf() + self.assertEqual(jwtconf.get_nav_public_key(), key) + + def test_get_nav_name_should_raise_error_if_name_empty(self): + config = u""" + [nav-config] + private_key=key + public_key=key + name= + """ + with patch.object(JWTConf, 'DEFAULT_CONFIG', config): + jwtconf = JWTConf() + with self.assertRaises(ConfigurationError): + jwtconf.get_nav_name() + + def test_get_nav_name_returns_configured_name(self): + config = u""" + [nav-config] + private_key=key + public_key=key + name=nav + """ + with patch.object(JWTConf, 'DEFAULT_CONFIG', config): + jwtconf = JWTConf() + self.assertEqual(jwtconf.get_nav_name(), "nav") From 57b5f85f92495aec4b7c324ba0efc40f2ee491eb Mon Sep 17 00:00:00 2001 From: Simon Oliver Tveit Date: Mon, 6 Feb 2023 17:28:14 +0100 Subject: [PATCH 11/38] Move external settings parsing to new function --- python/nav/jwtconf.py | 46 ++++++++++++++++++++++++------------------- 1 file changed, 26 insertions(+), 20 deletions(-) diff --git a/python/nav/jwtconf.py b/python/nav/jwtconf.py index 97e0339b4d..534dfdf0b9 100644 --- a/python/nav/jwtconf.py +++ b/python/nav/jwtconf.py @@ -21,26 +21,10 @@ class JWTConf(NAVConfigParser): def get_issuers_setting(self): try: - issuers_settings = self._get_settings_for_nav_issued_tokens() - for section in self.sections(): - if section == self.NAV_SECTION: - continue - get = partial(self.get, section) - issuer = self._validate_issuer(section) - key = self._validate_key(get('key')) - aud = self._validate_audience(get('aud')) - key_type = self._validate_type(get('keytype')) - if key_type == 'PEM': - key = self._read_file(key) - claims_options = { - 'aud': {'values': [aud], 'essential': True}, - } - issuers_settings[issuer] = { - 'key': key, - 'type': key_type, - 'claims_options': claims_options, - } - return issuers_settings + external_settings = self._get_settings_for_external_tokens() + local_settings = self._get_settings_for_nav_issued_tokens() + external_settings.update(local_settings) + return external_settings except ( FileNotFoundError, configparser.Error, @@ -51,6 +35,28 @@ def get_issuers_setting(self): _logger.error('Error reading jwtconfig %s', error) return dict() + def _get_settings_for_external_tokens(self): + settings = dict() + for section in self.sections(): + if section == self.NAV_SECTION: + continue + get = partial(self.get, section) + issuer = self._validate_issuer(section) + key = self._validate_key(get('key')) + aud = self._validate_audience(get('aud')) + key_type = self._validate_type(get('keytype')) + if key_type == 'PEM': + key = self._read_file(key) + claims_options = { + 'aud': {'values': [aud], 'essential': True}, + } + settings[issuer] = { + 'key': key, + 'type': key_type, + 'claims_options': claims_options, + } + return settings + def _read_file(self, file): with open(file, "r") as f: return f.read() From 10b9f852d895191bf05e6668f7b82dbc60f69b06 Mon Sep 17 00:00:00 2001 From: Simon Oliver Tveit Date: Mon, 6 Feb 2023 17:30:29 +0100 Subject: [PATCH 12/38] Use string format --- python/nav/jwtconf.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/python/nav/jwtconf.py b/python/nav/jwtconf.py index 534dfdf0b9..55c7f7f958 100644 --- a/python/nav/jwtconf.py +++ b/python/nav/jwtconf.py @@ -32,7 +32,7 @@ def get_issuers_setting(self): configparser.NoOptionError, ConfigurationError, ) as error: - _logger.error('Error reading jwtconfig %s', error) + _logger.error('Error reading jwtconfig {}'.format(error)) return dict() def _get_settings_for_external_tokens(self): From d1a3f1979239843fad60a362374c83100e925f43 Mon Sep 17 00:00:00 2001 From: Simon Oliver Tveit Date: Wed, 8 Feb 2023 08:52:38 +0100 Subject: [PATCH 13/38] Wrap errors as ConfigurationError --- python/nav/jwtconf.py | 85 ++++++++++++++++++--------------- tests/unittests/jwtconf_test.py | 10 ++-- 2 files changed, 52 insertions(+), 43 deletions(-) diff --git a/python/nav/jwtconf.py b/python/nav/jwtconf.py index 55c7f7f958..9143660bd2 100644 --- a/python/nav/jwtconf.py +++ b/python/nav/jwtconf.py @@ -25,41 +25,45 @@ def get_issuers_setting(self): local_settings = self._get_settings_for_nav_issued_tokens() external_settings.update(local_settings) return external_settings - except ( - FileNotFoundError, - configparser.Error, - configparser.NoSectionError, - configparser.NoOptionError, - ConfigurationError, - ) as error: + except ConfigurationError as error: _logger.error('Error reading jwtconfig {}'.format(error)) return dict() def _get_settings_for_external_tokens(self): settings = dict() - for section in self.sections(): - if section == self.NAV_SECTION: - continue - get = partial(self.get, section) - issuer = self._validate_issuer(section) - key = self._validate_key(get('key')) - aud = self._validate_audience(get('aud')) - key_type = self._validate_type(get('keytype')) - if key_type == 'PEM': - key = self._read_file(key) - claims_options = { - 'aud': {'values': [aud], 'essential': True}, - } - settings[issuer] = { - 'key': key, - 'type': key_type, - 'claims_options': claims_options, - } + try: + for section in self.sections(): + if section == self.NAV_SECTION: + continue + get = partial(self.get, section) + issuer = self._validate_issuer(section) + key = self._validate_key(get('key')) + aud = self._validate_audience(get('aud')) + key_type = self._validate_type(get('keytype')) + if key_type == 'PEM': + key = self._read_key_from_path(key) + claims_options = { + 'aud': {'values': [aud], 'essential': True}, + } + settings[issuer] = { + 'key': key, + 'type': key_type, + 'claims_options': claims_options, + } + except ( + configparser.Error, + configparser.NoSectionError, + configparser.NoOptionError, + ) as error: + raise ConfigurationError(error) return settings - def _read_file(self, file): - with open(file, "r") as f: - return f.read() + def _read_key_from_path(self, path): + try: + with open(path, "r") as f: + return f.read() + except FileNotFoundError: + raise ConfigurationError("Could not read PEM key from file {}".format(path)) def _validate_key(self, key): if not key: @@ -89,22 +93,27 @@ def _validate_audience(self, audience): raise ConfigurationError("Invalid 'aud': 'aud' must not be empty") return audience - def _get_nav_token_config(self): - return partial(self.get, self.NAV_SECTION) + def _get_nav_token_config_option(self, option): + try: + get = partial(self.get, self.NAV_SECTION) + return get(option) + except ( + configparser.Error, + configparser.NoSectionError, + configparser.NoOptionError, + ) as error: + raise ConfigurationError(error) def get_nav_private_key(self): - get = self._get_nav_token_config() - path = get('private_key') - return self._read_file(path) + path = self._get_nav_token_config_option('private_key') + return self._read_key_from_path(path) def get_nav_public_key(self): - get = self._get_nav_token_config() - path = get('public_key') - return self._read_file(path) + path = self._get_nav_token_config_option('public_key') + return self._read_key_from_path(path) def get_nav_name(self): - get = self._get_nav_token_config() - name = get('name') + name = self._get_nav_token_config_option('name') if not name: raise ConfigurationError("Invalid 'name': 'name' must not be empty") return name diff --git a/tests/unittests/jwtconf_test.py b/tests/unittests/jwtconf_test.py index a7f96d878e..b507c8ac79 100644 --- a/tests/unittests/jwtconf_test.py +++ b/tests/unittests/jwtconf_test.py @@ -31,7 +31,7 @@ def read_file_patch(self, file): return "key" with patch.object(JWTConf, 'DEFAULT_CONFIG', config): - with patch.object(JWTConf, '_read_file', read_file_patch): + with patch.object(JWTConf, '_read_key_from_path', read_file_patch): jwtconf = JWTConf() settings = jwtconf.get_issuers_setting() self.assertEqual(settings['jwks-issuer'], expected_settings) @@ -60,7 +60,7 @@ def read_file_patch(self, file): return pem_key with patch.object(JWTConf, 'DEFAULT_CONFIG', config): - with patch.object(JWTConf, '_read_file', read_file_patch): + with patch.object(JWTConf, '_read_key_from_path', read_file_patch): jwtconf = JWTConf() settings = jwtconf.get_issuers_setting() self.assertEqual(settings['pem-issuer'], expected_settings) @@ -121,7 +121,7 @@ def read_file_patch(self, file): return key with patch.object(JWTConf, 'DEFAULT_CONFIG', config): - with patch.object(JWTConf, '_read_file', read_file_patch): + with patch.object(JWTConf, '_read_key_from_path', read_file_patch): jwtconf = JWTConf() with self.assertRaises(ConfigurationError): jwtconf._validate_issuer('issuer-name') @@ -144,7 +144,7 @@ def read_file_patch(self, file): return key with patch.object(JWTConf, 'DEFAULT_CONFIG', config): - with patch.object(JWTConf, '_read_file', read_file_patch): + with patch.object(JWTConf, '_read_key_from_path', read_file_patch): jwtconf = JWTConf() self.assertEqual(jwtconf.get_nav_private_key(), key) @@ -161,7 +161,7 @@ def read_file_patch(self, file): return key with patch.object(JWTConf, 'DEFAULT_CONFIG', config): - with patch.object(JWTConf, '_read_file', read_file_patch): + with patch.object(JWTConf, '_read_key_from_path', read_file_patch): jwtconf = JWTConf() self.assertEqual(jwtconf.get_nav_public_key(), key) From fff9a7c2f90729675c852fe601cf11cdd2528f19 Mon Sep 17 00:00:00 2001 From: Simon Oliver Tveit Date: Wed, 8 Feb 2023 08:52:59 +0100 Subject: [PATCH 14/38] Add new test --- tests/unittests/jwtconf_test.py | 21 +++++++++++++++++++++ 1 file changed, 21 insertions(+) diff --git a/tests/unittests/jwtconf_test.py b/tests/unittests/jwtconf_test.py index b507c8ac79..5d44f9708f 100644 --- a/tests/unittests/jwtconf_test.py +++ b/tests/unittests/jwtconf_test.py @@ -65,6 +65,27 @@ def read_file_patch(self, file): settings = jwtconf.get_issuers_setting() self.assertEqual(settings['pem-issuer'], expected_settings) + def test_invalid_config_should_return_empty_dict(self): + config = u""" + [wrong-section-name] + private_key=key + public_key=key + name=nav-issuer + [pem-issuer] + keytype=INVALID + aud=nav + key=key_path + """ + + def read_file_patch(self, file): + return "key" + + with patch.object(JWTConf, 'DEFAULT_CONFIG', config): + with patch.object(JWTConf, '_read_key_from_path', read_file_patch): + jwtconf = JWTConf() + settings = jwtconf.get_issuers_setting() + self.assertEqual(settings, dict()) + def test_validate_key_should_raise_error_if_key_is_empty(self): jwtconf = JWTConf() with self.assertRaises(ConfigurationError): From 675e099ed41dd1029e13d7299fca60810c6c0797 Mon Sep 17 00:00:00 2001 From: Simon Oliver Tveit Date: Wed, 8 Feb 2023 09:25:19 +0100 Subject: [PATCH 15/38] Test internal/external error separately --- tests/unittests/jwtconf_test.py | 23 ++++++++++++++++++++++- 1 file changed, 22 insertions(+), 1 deletion(-) diff --git a/tests/unittests/jwtconf_test.py b/tests/unittests/jwtconf_test.py index 5d44f9708f..96cc499621 100644 --- a/tests/unittests/jwtconf_test.py +++ b/tests/unittests/jwtconf_test.py @@ -65,13 +65,34 @@ def read_file_patch(self, file): settings = jwtconf.get_issuers_setting() self.assertEqual(settings['pem-issuer'], expected_settings) - def test_invalid_config_should_return_empty_dict(self): + def test_invalid_config_for_interal_tokens_should_return_empty_dict(self): config = u""" [wrong-section-name] private_key=key public_key=key name=nav-issuer [pem-issuer] + keytype=PEM + aud=nav + key=key_path + """ + + def read_file_patch(self, file): + return "key" + + with patch.object(JWTConf, 'DEFAULT_CONFIG', config): + with patch.object(JWTConf, '_read_key_from_path', read_file_patch): + jwtconf = JWTConf() + settings = jwtconf.get_issuers_setting() + self.assertEqual(settings, dict()) + + def test_invalid_config_for_external_tokens_should_return_empty_dict(self): + config = u""" + [nav-config] + private_key=key + public_key=key + name=nav-issuer + [pem-issuer] keytype=INVALID aud=nav key=key_path From 0b833708020723ace7889eb0028cbcbc5fafb23e Mon Sep 17 00:00:00 2001 From: Simon Oliver Tveit Date: Thu, 9 Feb 2023 16:29:34 +0100 Subject: [PATCH 16/38] Add tests for more coverage --- tests/unittests/jwtconf_test.py | 35 +++++++++++++++++++++++++++++++++ 1 file changed, 35 insertions(+) diff --git a/tests/unittests/jwtconf_test.py b/tests/unittests/jwtconf_test.py index 96cc499621..8b0f8585e4 100644 --- a/tests/unittests/jwtconf_test.py +++ b/tests/unittests/jwtconf_test.py @@ -229,3 +229,38 @@ def test_get_nav_name_returns_configured_name(self): with patch.object(JWTConf, 'DEFAULT_CONFIG', config): jwtconf = JWTConf() self.assertEqual(jwtconf.get_nav_name(), "nav") + + def test_missing_option_should_raise_error(self): + config_with_missing_keytype = u""" + [nav-config] + private_key=key + public_key=key + name=nav-issuer + [pem-issuer] + aud=nav + key=key_path + """ + + def read_file_patch(self, file): + return "key" + + with patch.object(JWTConf, 'DEFAULT_CONFIG', config_with_missing_keytype): + with patch.object(JWTConf, '_read_key_from_path', read_file_patch): + jwtconf = JWTConf() + with self.assertRaises(ConfigurationError): + jwtconf._get_settings_for_external_tokens() + + def test_non_existing_file_should_raise_error(self): + config = u""" + [nav-config] + private_key=key + public_key=key + name=nav-issuer + [pem-issuer] + aud=nav + key=key_path + """ + with patch.object(JWTConf, 'DEFAULT_CONFIG', config): + jwtconf = JWTConf() + with self.assertRaises(ConfigurationError): + jwtconf._read_key_from_path("fakepath") From 1655b707bd275a218271ab51c11ddc5b8c7934f3 Mon Sep 17 00:00:00 2001 From: Simon Oliver Tveit Date: Thu, 9 Feb 2023 17:28:19 +0100 Subject: [PATCH 17/38] Add test for correctly reading file --- tests/unittests/jwtconf_test.py | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/tests/unittests/jwtconf_test.py b/tests/unittests/jwtconf_test.py index 8b0f8585e4..51d995727c 100644 --- a/tests/unittests/jwtconf_test.py +++ b/tests/unittests/jwtconf_test.py @@ -1,4 +1,4 @@ -from mock import patch +from mock import patch, mock_open from unittest import TestCase from nav.jwtconf import JWTConf from nav.config import ConfigurationError @@ -264,3 +264,9 @@ def test_non_existing_file_should_raise_error(self): jwtconf = JWTConf() with self.assertRaises(ConfigurationError): jwtconf._read_key_from_path("fakepath") + + def test_return_correct_key_if_file_exists(self): + jwtconf = JWTConf() + mock_key = "key" + with patch("builtins.open", mock_open(read_data=mock_key)): + self.assertEqual(jwtconf._read_key_from_path("path"), mock_key) From ff9f5997fe64cba2c595f7520ce5cf0e386635a7 Mon Sep 17 00:00:00 2001 From: Simon Oliver Tveit Date: Wed, 22 Feb 2023 12:45:28 +0100 Subject: [PATCH 18/38] Rename nav internal token config --- python/nav/etc/jwt.conf | 2 +- python/nav/jwtconf.py | 4 ++-- tests/unittests/jwtconf_test.py | 20 ++++++++++---------- 3 files changed, 13 insertions(+), 13 deletions(-) diff --git a/python/nav/etc/jwt.conf b/python/nav/etc/jwt.conf index cac1b73f2c..6ed77b3053 100644 --- a/python/nav/etc/jwt.conf +++ b/python/nav/etc/jwt.conf @@ -6,7 +6,7 @@ # nav_name will be used for the 'iss' and 'aud' claims. # The section name should NOT be changed. -[nav-config] +[nav] private_key= public_key= name= diff --git a/python/nav/jwtconf.py b/python/nav/jwtconf.py index 9143660bd2..a681c77b43 100644 --- a/python/nav/jwtconf.py +++ b/python/nav/jwtconf.py @@ -11,9 +11,9 @@ class JWTConf(NAVConfigParser): """jwt.conf config parser""" DEFAULT_CONFIG_FILES = ('jwt.conf',) - NAV_SECTION = "nav-config" + NAV_SECTION = "nav" DEFAULT_CONFIG = u""" -[nav-config] +[nav] private_key= public_key= name= diff --git a/tests/unittests/jwtconf_test.py b/tests/unittests/jwtconf_test.py index 51d995727c..495781d445 100644 --- a/tests/unittests/jwtconf_test.py +++ b/tests/unittests/jwtconf_test.py @@ -10,7 +10,7 @@ def setUp(self): def test_valid_jwks_config_should_pass(self): config = u""" - [nav-config] + [nav] private_key=key public_key=key name=issuer-name @@ -38,7 +38,7 @@ def read_file_patch(self, file): def test_valid_pem_config_should_pass(self): config = u""" - [nav-config] + [nav] private_key=key public_key=key name=nav-issuer @@ -88,7 +88,7 @@ def read_file_patch(self, file): def test_invalid_config_for_external_tokens_should_return_empty_dict(self): config = u""" - [nav-config] + [nav] private_key=key public_key=key name=nav-issuer @@ -148,7 +148,7 @@ def test_PEM_should_be_a_valid_type(self): def test_validate_issuer_should_fail_if_external_name_matches_local_name(self): config = u""" - [nav-config] + [nav] private_key=key public_key=key name=issuer-name @@ -175,7 +175,7 @@ def test_validate_issuer_should_raise_error_if_issuer_is_empty(self): def test_get_nav_private_key_returns_correct_private_key(self): config = u""" - [nav-config] + [nav] private_key=key public_key=key name=issuer-name @@ -192,7 +192,7 @@ def read_file_patch(self, file): def test_get_nav_public_key_returns_correct_public_key(self): config = u""" - [nav-config] + [nav] private_key=key public_key=key name=issuer-name @@ -209,7 +209,7 @@ def read_file_patch(self, file): def test_get_nav_name_should_raise_error_if_name_empty(self): config = u""" - [nav-config] + [nav] private_key=key public_key=key name= @@ -221,7 +221,7 @@ def test_get_nav_name_should_raise_error_if_name_empty(self): def test_get_nav_name_returns_configured_name(self): config = u""" - [nav-config] + [nav] private_key=key public_key=key name=nav @@ -232,7 +232,7 @@ def test_get_nav_name_returns_configured_name(self): def test_missing_option_should_raise_error(self): config_with_missing_keytype = u""" - [nav-config] + [nav] private_key=key public_key=key name=nav-issuer @@ -252,7 +252,7 @@ def read_file_patch(self, file): def test_non_existing_file_should_raise_error(self): config = u""" - [nav-config] + [nav] private_key=key public_key=key name=nav-issuer From 913ff7207b276988add760b259ceaff176c035e6 Mon Sep 17 00:00:00 2001 From: Simon Oliver Tveit Date: Wed, 22 Feb 2023 12:46:04 +0100 Subject: [PATCH 19/38] Fix config comments using incorrent names --- python/nav/etc/jwt.conf | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/python/nav/etc/jwt.conf b/python/nav/etc/jwt.conf index 6ed77b3053..eeac819f3d 100644 --- a/python/nav/etc/jwt.conf +++ b/python/nav/etc/jwt.conf @@ -2,8 +2,8 @@ # This includes tokens generated by NAV itself and external issuers. # These settings are for tokens issued by NAV itself. -# nav_private_key and nav_public_key should be absolute paths to the related keys in PEM format. -# nav_name will be used for the 'iss' and 'aud' claims. +# private_key and public_key should be absolute paths to the related keys in PEM format. +# name will be used for the 'iss' and 'aud' claims. # The section name should NOT be changed. [nav] From 800ee6a1a66793b6dd945b5ce2951e6a9494348b Mon Sep 17 00:00:00 2001 From: Simon Oliver Tveit Date: Wed, 22 Feb 2023 12:47:45 +0100 Subject: [PATCH 20/38] Explain config option directly above the option --- python/nav/etc/jwt.conf | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/python/nav/etc/jwt.conf b/python/nav/etc/jwt.conf index eeac819f3d..0d7fbab66a 100644 --- a/python/nav/etc/jwt.conf +++ b/python/nav/etc/jwt.conf @@ -2,13 +2,14 @@ # This includes tokens generated by NAV itself and external issuers. # These settings are for tokens issued by NAV itself. -# private_key and public_key should be absolute paths to the related keys in PEM format. -# name will be used for the 'iss' and 'aud' claims. # The section name should NOT be changed. [nav] +# Absolute path to private key in PEM format private_key= +# Absolute path to public key in PEM format. public_key= +# Used for the 'iss' and 'aud' claims of generated tokens. name= # Custom JWT issuers can be defined in a section. The name of the section should be From 683e24fc9f574122a6fd8f54da99ff81a89afe6c Mon Sep 17 00:00:00 2001 From: Simon Oliver Tveit Date: Wed, 22 Feb 2023 13:01:10 +0100 Subject: [PATCH 21/38] Reword comment --- python/nav/etc/jwt.conf | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/python/nav/etc/jwt.conf b/python/nav/etc/jwt.conf index 0d7fbab66a..349b909d1a 100644 --- a/python/nav/etc/jwt.conf +++ b/python/nav/etc/jwt.conf @@ -1,7 +1,7 @@ # This file contains configuration for JWT tokens. # This includes tokens generated by NAV itself and external issuers. -# These settings are for tokens issued by NAV itself. +# The settings in the `nav` section are for tokens issued by NAV itself. # The section name should NOT be changed. [nav] From de26c68e26e9cb47b59bfa0132a20e3b266d5178 Mon Sep 17 00:00:00 2001 From: Simon Oliver Tveit Date: Wed, 22 Feb 2023 13:03:01 +0100 Subject: [PATCH 22/38] Use built in logging message formatting --- python/nav/jwtconf.py | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-) diff --git a/python/nav/jwtconf.py b/python/nav/jwtconf.py index a681c77b43..4a75fa79b8 100644 --- a/python/nav/jwtconf.py +++ b/python/nav/jwtconf.py @@ -26,7 +26,7 @@ def get_issuers_setting(self): external_settings.update(local_settings) return external_settings except ConfigurationError as error: - _logger.error('Error reading jwtconfig {}'.format(error)) + _logger.error('Error reading jwtconfig %s', error) return dict() def _get_settings_for_external_tokens(self): @@ -63,7 +63,7 @@ def _read_key_from_path(self, path): with open(path, "r") as f: return f.read() except FileNotFoundError: - raise ConfigurationError("Could not read PEM key from file {}".format(path)) + raise ConfigurationError("Could not read PEM key from file %s", path) def _validate_key(self, key): if not key: @@ -82,9 +82,7 @@ def _validate_issuer(self, section): raise ConfigurationError("Invalid 'issuer': 'issuer' must not be empty") if section == self.get_nav_name(): raise ConfigurationError( - "Invalid 'issuer': {} collides with internal issuer name".format( - section - ) + "Invalid 'issuer': {} collides with internal issuer name %s", section ) return section From d7c784d4d999e16c5c381e4416a9d1198a34b2a5 Mon Sep 17 00:00:00 2001 From: Simon Oliver Tveit Date: Wed, 22 Feb 2023 13:09:03 +0100 Subject: [PATCH 23/38] Handle permission error for reading pem keys --- python/nav/jwtconf.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/python/nav/jwtconf.py b/python/nav/jwtconf.py index 4a75fa79b8..96fda3062d 100644 --- a/python/nav/jwtconf.py +++ b/python/nav/jwtconf.py @@ -62,8 +62,8 @@ def _read_key_from_path(self, path): try: with open(path, "r") as f: return f.read() - except FileNotFoundError: - raise ConfigurationError("Could not read PEM key from file %s", path) + except (FileNotFoundError, PermissionError) as error: + raise ConfigurationError(error) def _validate_key(self, key): if not key: From 17f8471804de6d98b3e7e98dde1bfe43379d0b4b Mon Sep 17 00:00:00 2001 From: Simon Oliver Tveit Date: Wed, 22 Feb 2023 13:19:01 +0100 Subject: [PATCH 24/38] Add colon to error message --- python/nav/jwtconf.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/python/nav/jwtconf.py b/python/nav/jwtconf.py index 96fda3062d..6aa8b7c0a5 100644 --- a/python/nav/jwtconf.py +++ b/python/nav/jwtconf.py @@ -26,7 +26,7 @@ def get_issuers_setting(self): external_settings.update(local_settings) return external_settings except ConfigurationError as error: - _logger.error('Error reading jwtconfig %s', error) + _logger.error('Error reading jwtconfig: %s', error) return dict() def _get_settings_for_external_tokens(self): From 2a3a0fe3e8ee4cb0b7aefc99613079a5fc5a81d4 Mon Sep 17 00:00:00 2001 From: Simon Oliver Tveit Date: Wed, 22 Feb 2023 13:35:48 +0100 Subject: [PATCH 25/38] Add test for permissionerror --- tests/unittests/jwtconf_test.py | 17 +++++++++++++++++ 1 file changed, 17 insertions(+) diff --git a/tests/unittests/jwtconf_test.py b/tests/unittests/jwtconf_test.py index 495781d445..a6ebb77180 100644 --- a/tests/unittests/jwtconf_test.py +++ b/tests/unittests/jwtconf_test.py @@ -270,3 +270,20 @@ def test_return_correct_key_if_file_exists(self): mock_key = "key" with patch("builtins.open", mock_open(read_data=mock_key)): self.assertEqual(jwtconf._read_key_from_path("path"), mock_key) + + def test_file_with_permission_problems_should_raise_error(self): + config = u""" + [nav] + private_key=key + public_key=key + name=nav-issuer + [pem-issuer] + aud=nav + key=key_path + """ + with patch("builtins.open", mock_open(read_data="key")) as mocked_open: + mocked_open.side_effect = PermissionError + with patch.object(JWTConf, 'DEFAULT_CONFIG', config): + jwtconf = JWTConf() + with self.assertRaises(ConfigurationError): + jwtconf._read_key_from_path("fakepath") From 714c4afd9a730a5fdd1e157bdeb023a2561ab720 Mon Sep 17 00:00:00 2001 From: Simon Oliver Tveit Date: Wed, 22 Feb 2023 14:01:30 +0100 Subject: [PATCH 26/38] use %s instead of {} --- python/nav/jwtconf.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/python/nav/jwtconf.py b/python/nav/jwtconf.py index 6aa8b7c0a5..8fbca2043b 100644 --- a/python/nav/jwtconf.py +++ b/python/nav/jwtconf.py @@ -82,7 +82,7 @@ def _validate_issuer(self, section): raise ConfigurationError("Invalid 'issuer': 'issuer' must not be empty") if section == self.get_nav_name(): raise ConfigurationError( - "Invalid 'issuer': {} collides with internal issuer name %s", section + "Invalid 'issuer': %s collides with internal issuer name %s", section ) return section From 12d43eaa51b8f94f4dd6c592aac1763da60026d7 Mon Sep 17 00:00:00 2001 From: Simon Oliver Tveit Date: Thu, 9 Mar 2023 15:19:19 +0100 Subject: [PATCH 27/38] Separate exception for permission/notfound --- python/nav/jwtconf.py | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/python/nav/jwtconf.py b/python/nav/jwtconf.py index 8fbca2043b..d1afb52f0b 100644 --- a/python/nav/jwtconf.py +++ b/python/nav/jwtconf.py @@ -62,8 +62,14 @@ def _read_key_from_path(self, path): try: with open(path, "r") as f: return f.read() - except (FileNotFoundError, PermissionError) as error: - raise ConfigurationError(error) + except FileNotFoundError: + raise ConfigurationError( + "Could not find file %s to read PEM key from", path + ) + except PermissionError: + raise ConfigurationError( + "Could not access file %s to read PEM key from", path + ) def _validate_key(self, key): if not key: From 025a25c9fd4fb0908094afdd444f5335f0406fc6 Mon Sep 17 00:00:00 2001 From: Simon Oliver Tveit Date: Wed, 24 May 2023 09:56:49 +0200 Subject: [PATCH 28/38] Allow local jwt tokens to not be configured --- python/nav/jwtconf.py | 8 ++------ 1 file changed, 2 insertions(+), 6 deletions(-) diff --git a/python/nav/jwtconf.py b/python/nav/jwtconf.py index d1afb52f0b..15da897a10 100644 --- a/python/nav/jwtconf.py +++ b/python/nav/jwtconf.py @@ -12,12 +12,6 @@ class JWTConf(NAVConfigParser): DEFAULT_CONFIG_FILES = ('jwt.conf',) NAV_SECTION = "nav" - DEFAULT_CONFIG = u""" -[nav] -private_key= -public_key= -name= -""" def get_issuers_setting(self): try: @@ -123,6 +117,8 @@ def get_nav_name(self): return name def _get_settings_for_nav_issued_tokens(self): + if not self.has_section(self.NAV_SECTION): + return {} name = self.get_nav_name() claims_options = { 'aud': {'values': [name], 'essential': True}, From f201c0da75e4a541f94eea873e78a70244529e39 Mon Sep 17 00:00:00 2001 From: Simon Oliver Tveit Date: Wed, 24 May 2023 09:57:16 +0200 Subject: [PATCH 29/38] Do not have config for local jwt token by default --- python/nav/etc/jwt.conf | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/python/nav/etc/jwt.conf b/python/nav/etc/jwt.conf index 349b909d1a..ab308998c1 100644 --- a/python/nav/etc/jwt.conf +++ b/python/nav/etc/jwt.conf @@ -2,15 +2,16 @@ # This includes tokens generated by NAV itself and external issuers. # The settings in the `nav` section are for tokens issued by NAV itself. +# Comment this in and fill out the values if you want to activate local JWT tokens. # The section name should NOT be changed. -[nav] +#[nav] # Absolute path to private key in PEM format -private_key= +#private_key= # Absolute path to public key in PEM format. -public_key= +#public_key= # Used for the 'iss' and 'aud' claims of generated tokens. -name= +#name= # Custom JWT issuers can be defined in a section. The name of the section should be # the issuers name, the same value as the 'iss' claim will be in tokens From b2e2f52db7971d475e30a2769ace0f053d4efa47 Mon Sep 17 00:00:00 2001 From: Simon Oliver Tveit Date: Wed, 24 May 2023 09:58:12 +0200 Subject: [PATCH 30/38] Add test for empty config --- tests/unittests/jwtconf_test.py | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/tests/unittests/jwtconf_test.py b/tests/unittests/jwtconf_test.py index a6ebb77180..7e203555ef 100644 --- a/tests/unittests/jwtconf_test.py +++ b/tests/unittests/jwtconf_test.py @@ -287,3 +287,12 @@ def test_file_with_permission_problems_should_raise_error(self): jwtconf = JWTConf() with self.assertRaises(ConfigurationError): jwtconf._read_key_from_path("fakepath") + + def test_empty_config_should_produce_empty_settings(self): + config = u""" + """ + expected_settings = {} + with patch.object(JWTConf, 'DEFAULT_CONFIG', config): + jwtconf = JWTConf() + settings = jwtconf.get_issuers_setting() + self.assertEqual(settings, expected_settings) From 5d5b88efa2566e3c7828d79d69c8e20cddbc8c3e Mon Sep 17 00:00:00 2001 From: Simon Oliver Tveit Date: Wed, 24 May 2023 11:09:49 +0200 Subject: [PATCH 31/38] Pass issuer validation if local nav name is not set --- python/nav/jwtconf.py | 14 ++++++++++---- 1 file changed, 10 insertions(+), 4 deletions(-) diff --git a/python/nav/jwtconf.py b/python/nav/jwtconf.py index 15da897a10..c72c9c7009 100644 --- a/python/nav/jwtconf.py +++ b/python/nav/jwtconf.py @@ -80,10 +80,16 @@ def _validate_type(self, key_type): def _validate_issuer(self, section): if not section: raise ConfigurationError("Invalid 'issuer': 'issuer' must not be empty") - if section == self.get_nav_name(): - raise ConfigurationError( - "Invalid 'issuer': %s collides with internal issuer name %s", section - ) + try: + nav_name = self.get_nav_name() + except ConfigurationError: + pass + else: + if section == nav_name: + raise ConfigurationError( + "Invalid 'issuer': %s collides with internal issuer name %s", + section, + ) return section def _validate_audience(self, audience): From 5a28a3f13a0cb23b9185bb1e5f92a1fe2fb95591 Mon Sep 17 00:00:00 2001 From: Simon Oliver Tveit Date: Wed, 24 May 2023 11:12:48 +0200 Subject: [PATCH 32/38] Handle only expected configparser exceptions aka removes the "general" exception check --- python/nav/jwtconf.py | 2 -- 1 file changed, 2 deletions(-) diff --git a/python/nav/jwtconf.py b/python/nav/jwtconf.py index c72c9c7009..42c50b561d 100644 --- a/python/nav/jwtconf.py +++ b/python/nav/jwtconf.py @@ -45,7 +45,6 @@ def _get_settings_for_external_tokens(self): 'claims_options': claims_options, } except ( - configparser.Error, configparser.NoSectionError, configparser.NoOptionError, ) as error: @@ -102,7 +101,6 @@ def _get_nav_token_config_option(self, option): get = partial(self.get, self.NAV_SECTION) return get(option) except ( - configparser.Error, configparser.NoSectionError, configparser.NoOptionError, ) as error: From 87399350dcdcc776b020e041d4947bf67912af1e Mon Sep 17 00:00:00 2001 From: Simon Oliver Tveit Date: Wed, 24 May 2023 11:53:36 +0200 Subject: [PATCH 33/38] Update tests for allowing non-configured local jwt --- tests/unittests/jwtconf_test.py | 101 +++++++++++++++++++++++--------- 1 file changed, 72 insertions(+), 29 deletions(-) diff --git a/tests/unittests/jwtconf_test.py b/tests/unittests/jwtconf_test.py index 7e203555ef..4cd46e4c3d 100644 --- a/tests/unittests/jwtconf_test.py +++ b/tests/unittests/jwtconf_test.py @@ -10,10 +10,6 @@ def setUp(self): def test_valid_jwks_config_should_pass(self): config = u""" - [nav] - private_key=key - public_key=key - name=issuer-name [jwks-issuer] keytype=JWKS aud=nav @@ -38,10 +34,6 @@ def read_file_patch(self, file): def test_valid_pem_config_should_pass(self): config = u""" - [nav] - private_key=key - public_key=key - name=nav-issuer [pem-issuer] keytype=PEM aud=nav @@ -65,16 +57,38 @@ def read_file_patch(self, file): settings = jwtconf.get_issuers_setting() self.assertEqual(settings['pem-issuer'], expected_settings) + def test_valid_local_config_should_pass(self): + config = u""" + [nav] + private_key=key + public_key=key + name=nav + """ + key = "PEM KEY" + expected_settings = { + 'key': key, + 'type': 'PEM', + 'claims_options': { + 'aud': {'values': ['nav'], 'essential': True}, + 'token_type': {'values': ['access_token'], 'essential': True}, + }, + } + + def read_file_patch(self, file): + return key + + with patch.object(JWTConf, 'DEFAULT_CONFIG', config): + with patch.object(JWTConf, '_read_key_from_path', read_file_patch): + jwtconf = JWTConf() + settings = jwtconf.get_issuers_setting() + self.assertEqual(settings['nav'], expected_settings) + def test_invalid_config_for_interal_tokens_should_return_empty_dict(self): config = u""" [wrong-section-name] private_key=key public_key=key name=nav-issuer - [pem-issuer] - keytype=PEM - aud=nav - key=key_path """ def read_file_patch(self, file): @@ -88,10 +102,6 @@ def read_file_patch(self, file): def test_invalid_config_for_external_tokens_should_return_empty_dict(self): config = u""" - [nav] - private_key=key - public_key=key - name=nav-issuer [pem-issuer] keytype=INVALID aud=nav @@ -232,10 +242,6 @@ def test_get_nav_name_returns_configured_name(self): def test_missing_option_should_raise_error(self): config_with_missing_keytype = u""" - [nav] - private_key=key - public_key=key - name=nav-issuer [pem-issuer] aud=nav key=key_path @@ -252,10 +258,6 @@ def read_file_patch(self, file): def test_non_existing_file_should_raise_error(self): config = u""" - [nav] - private_key=key - public_key=key - name=nav-issuer [pem-issuer] aud=nav key=key_path @@ -273,10 +275,6 @@ def test_return_correct_key_if_file_exists(self): def test_file_with_permission_problems_should_raise_error(self): config = u""" - [nav] - private_key=key - public_key=key - name=nav-issuer [pem-issuer] aud=nav key=key_path @@ -288,7 +286,7 @@ def test_file_with_permission_problems_should_raise_error(self): with self.assertRaises(ConfigurationError): jwtconf._read_key_from_path("fakepath") - def test_empty_config_should_produce_empty_settings(self): + def test_empty_config_should_give_empty_issuer_settings(self): config = u""" """ expected_settings = {} @@ -296,3 +294,48 @@ def test_empty_config_should_produce_empty_settings(self): jwtconf = JWTConf() settings = jwtconf.get_issuers_setting() self.assertEqual(settings, expected_settings) + + def test_empty_config_should_give_empty_external_settings(self): + config = u""" + """ + expected_settings = {} + with patch.object(JWTConf, 'DEFAULT_CONFIG', config): + jwtconf = JWTConf() + settings = jwtconf._get_settings_for_external_tokens() + self.assertEqual(settings, expected_settings) + + def test_empty_config_should_give_empty_local_settings(self): + config = u""" + """ + expected_settings = {} + with patch.object(JWTConf, 'DEFAULT_CONFIG', config): + jwtconf = JWTConf() + settings = jwtconf._get_settings_for_nav_issued_tokens() + self.assertEqual(settings, expected_settings) + + def test_settings_should_include_local_and_external_settings(self): + config = u""" + [nav] + private_key=key + public_key=key + name=local-issuer + [jwks-issuer] + keytype=JWKS + aud=nav + key=www.example.com + [pem-issuer] + keytype=PEM + aud=aud + key=key + """ + + def read_file_patch(self, file): + return "key" + + with patch.object(JWTConf, 'DEFAULT_CONFIG', config): + with patch.object(JWTConf, '_read_key_from_path', read_file_patch): + jwtconf = JWTConf() + settings = jwtconf.get_issuers_setting() + assert 'jwks-issuer' in settings + assert 'pem-issuer' in settings + assert 'local-issuer' in settings From 35079b7ba709468f187442a38f18853406c09e7f Mon Sep 17 00:00:00 2001 From: Simon Oliver Tveit Date: Wed, 24 May 2023 11:57:13 +0200 Subject: [PATCH 34/38] Update test names --- tests/unittests/jwtconf_test.py | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/tests/unittests/jwtconf_test.py b/tests/unittests/jwtconf_test.py index 4cd46e4c3d..d79f50704c 100644 --- a/tests/unittests/jwtconf_test.py +++ b/tests/unittests/jwtconf_test.py @@ -8,7 +8,7 @@ class TestJWTConf(TestCase): def setUp(self): pass - def test_valid_jwks_config_should_pass(self): + def test_issuer_settings_include_valid_jwks_issuer(self): config = u""" [jwks-issuer] keytype=JWKS @@ -32,7 +32,7 @@ def read_file_patch(self, file): settings = jwtconf.get_issuers_setting() self.assertEqual(settings['jwks-issuer'], expected_settings) - def test_valid_pem_config_should_pass(self): + def test_issuer_settings_include_valid_pem_issuer(self): config = u""" [pem-issuer] keytype=PEM @@ -57,7 +57,7 @@ def read_file_patch(self, file): settings = jwtconf.get_issuers_setting() self.assertEqual(settings['pem-issuer'], expected_settings) - def test_valid_local_config_should_pass(self): + def test_issuer_settings_include_valid_local_issuer(self): config = u""" [nav] private_key=key @@ -83,7 +83,7 @@ def read_file_patch(self, file): settings = jwtconf.get_issuers_setting() self.assertEqual(settings['nav'], expected_settings) - def test_invalid_config_for_interal_tokens_should_return_empty_dict(self): + def test_invalid_config_for_internal_tokens_should_return_empty_dict(self): config = u""" [wrong-section-name] private_key=key From ef3aded626e717b96a54165ff887d5182707670c Mon Sep 17 00:00:00 2001 From: Simon Oliver Tveit Date: Mon, 4 Sep 2023 11:35:07 +0200 Subject: [PATCH 35/38] Move config file to webfront directory --- python/nav/etc/{ => webfront}/jwt.conf | 0 python/nav/jwtconf.py | 3 ++- 2 files changed, 2 insertions(+), 1 deletion(-) rename python/nav/etc/{ => webfront}/jwt.conf (100%) diff --git a/python/nav/etc/jwt.conf b/python/nav/etc/webfront/jwt.conf similarity index 100% rename from python/nav/etc/jwt.conf rename to python/nav/etc/webfront/jwt.conf diff --git a/python/nav/jwtconf.py b/python/nav/jwtconf.py index 42c50b561d..46f2b5d383 100644 --- a/python/nav/jwtconf.py +++ b/python/nav/jwtconf.py @@ -1,4 +1,5 @@ import logging +from os.path import join from functools import partial import configparser @@ -10,7 +11,7 @@ class JWTConf(NAVConfigParser): """jwt.conf config parser""" - DEFAULT_CONFIG_FILES = ('jwt.conf',) + DEFAULT_CONFIG_FILES = [join('webfront', 'jwt.conf')] NAV_SECTION = "nav" def get_issuers_setting(self): From 1e99af89b2a009a6db46405b6504d8fc4983f67b Mon Sep 17 00:00:00 2001 From: Simon Oliver Tveit Date: Mon, 4 Sep 2023 11:58:27 +0200 Subject: [PATCH 36/38] Update docstrings --- python/nav/jwtconf.py | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/python/nav/jwtconf.py b/python/nav/jwtconf.py index 46f2b5d383..309f53b43b 100644 --- a/python/nav/jwtconf.py +++ b/python/nav/jwtconf.py @@ -9,12 +9,14 @@ class JWTConf(NAVConfigParser): - """jwt.conf config parser""" + """webfront/jwt.conf config parser""" DEFAULT_CONFIG_FILES = [join('webfront', 'jwt.conf')] NAV_SECTION = "nav" def get_issuers_setting(self): + """Parses the webfront/jwt.conf config file and produces a dictionary that can + be used as settings for the `drf-oidc-auth` django extension""" try: external_settings = self._get_settings_for_external_tokens() local_settings = self._get_settings_for_nav_issued_tokens() From b381bbac57a2730c7c97ac456322f937c37c18f5 Mon Sep 17 00:00:00 2001 From: Simon Oliver Tveit Date: Mon, 4 Sep 2023 20:25:33 +0200 Subject: [PATCH 37/38] Add type hints --- python/nav/jwtconf.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/python/nav/jwtconf.py b/python/nav/jwtconf.py index 309f53b43b..d684659738 100644 --- a/python/nav/jwtconf.py +++ b/python/nav/jwtconf.py @@ -2,6 +2,7 @@ from os.path import join from functools import partial import configparser +from typing import Dict, Any from nav.config import ConfigurationError, NAVConfigParser @@ -14,7 +15,7 @@ class JWTConf(NAVConfigParser): DEFAULT_CONFIG_FILES = [join('webfront', 'jwt.conf')] NAV_SECTION = "nav" - def get_issuers_setting(self): + def get_issuers_setting(self) -> Dict[str, Any]: """Parses the webfront/jwt.conf config file and produces a dictionary that can be used as settings for the `drf-oidc-auth` django extension""" try: From 8c8e00fa392dcd9a73a7f6d6e0a6bc9267ac8121 Mon Sep 17 00:00:00 2001 From: Simon Oliver Tveit Date: Mon, 4 Sep 2023 20:30:56 +0200 Subject: [PATCH 38/38] Revise docstring --- python/nav/jwtconf.py | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/python/nav/jwtconf.py b/python/nav/jwtconf.py index d684659738..5d8287aba5 100644 --- a/python/nav/jwtconf.py +++ b/python/nav/jwtconf.py @@ -16,8 +16,10 @@ class JWTConf(NAVConfigParser): NAV_SECTION = "nav" def get_issuers_setting(self) -> Dict[str, Any]: - """Parses the webfront/jwt.conf config file and produces a dictionary that can - be used as settings for the `drf-oidc-auth` django extension""" + """Parses the webfront/jwt.conf config file and returns a dictionary that can + be used as settings for the `drf-oidc-auth` django extension. + If the parsing fails, an empty dict is returned. + """ try: external_settings = self._get_settings_for_external_tokens() local_settings = self._get_settings_for_nav_issued_tokens()