From ba21d040809b836a6d204061ba69bae564c513c0 Mon Sep 17 00:00:00 2001 From: David McDonald Date: Fri, 24 Jan 2020 17:25:49 +0000 Subject: [PATCH 1/3] Add test case for uncaught jwt exception --- .../app/authentication/test_authentication.py | 23 +++++++++++++++++++ 1 file changed, 23 insertions(+) diff --git a/tests/app/authentication/test_authentication.py b/tests/app/authentication/test_authentication.py index 8fae6bdab..53c235261 100644 --- a/tests/app/authentication/test_authentication.py +++ b/tests/app/authentication/test_authentication.py @@ -126,6 +126,29 @@ def test_admin_auth_should_not_allow_request_with_no_iat(client, sample_api_key) assert exc.value.short_message == 'Invalid token: signature, api token is not valid' +def test_auth_should_not_allow_request_with_extra_claims(client, sample_api_key): + iss = str(sample_api_key.service_id) + key = get_unsigned_secrets(sample_api_key.service_id)[0] + + headers = { + "typ": 'JWT', + "alg": 'HS256' + } + + claims = { + 'iss': iss, + 'iat': int(time.time()), + 'aud': 'notifications.service.gov.uk' # extra claim that we don't support + } + + token = jwt.encode(payload=claims, key=key, headers=headers).decode() + + request.headers = {'Authorization': 'Bearer {}'.format(token)} + with pytest.raises(AuthError) as exc: + requires_auth() + assert exc.value.short_message == 'Invalid token: signature, api token is not valid' + + def test_should_not_allow_invalid_secret(client, sample_api_key): token = create_jwt_token( secret="not-so-secret", From 7a019df5a2d2fd38bf33f2a5e3a5d3aaccdc2016 Mon Sep 17 00:00:00 2001 From: David McDonald Date: Fri, 24 Jan 2020 17:29:43 +0000 Subject: [PATCH 2/3] Catch previously uncaught jwt exceptions added in python client 5.5.0 This fixes the test in the previous commit and means we will catch other unexpected jwt errors which are now raised as `TokenError`s and raise an AuthError based on this. This will stop us serving 5xx to users when we don't catch an exception. Also runs make freeze-requirements --- app/authentication/auth.py | 5 ++++- requirements.txt | 4 ++-- 2 files changed, 6 insertions(+), 3 deletions(-) diff --git a/app/authentication/auth.py b/app/authentication/auth.py index 9f564df80..f8c99fb4b 100644 --- a/app/authentication/auth.py +++ b/app/authentication/auth.py @@ -1,7 +1,7 @@ from flask import request, _request_ctx_stack, current_app, g from notifications_python_client.authentication import decode_jwt_token, get_token_issuer from notifications_python_client.errors import ( - TokenDecodeError, TokenExpiredError, TokenIssuerError, TokenAlgorithmError + TokenDecodeError, TokenExpiredError, TokenIssuerError, TokenAlgorithmError, TokenError ) from notifications_utils import request_helper from sqlalchemy.exc import DataError @@ -93,6 +93,9 @@ def requires_auth(): raise AuthError(err_msg, 403, service_id=service.id, api_key_id=api_key.id) except TokenDecodeError: continue + except TokenError: + err_msg = 'Invalid token: signature, api token is not valid' + raise AuthError(err_msg, 403, service_id=service.id, api_key_id=api_key.id) if api_key.expiry_date: raise AuthError("Invalid token: API key revoked", 403, service_id=service.id, api_key_id=api_key.id) diff --git a/requirements.txt b/requirements.txt index bacdf1dfd..22f42891e 100644 --- a/requirements.txt +++ b/requirements.txt @@ -36,13 +36,13 @@ alembic==1.4.0 amqp==1.4.9 anyjson==0.3.3 attrs==19.3.0 -awscli==1.17.16 +awscli==1.17.17 bcrypt==3.1.7 billiard==3.3.0.23 bleach==3.1.0 boto==2.49.0 boto3==1.10.38 -botocore==1.14.16 +botocore==1.14.17 certifi==2019.11.28 chardet==3.0.4 Click==7.0 From f861da184307641281e4c51ee31435b766d8e36f Mon Sep 17 00:00:00 2001 From: David McDonald Date: Fri, 24 Jan 2020 17:32:41 +0000 Subject: [PATCH 3/3] Improve text for error messages --- app/authentication/auth.py | 34 ++++++++++++------- app/errors.py | 4 +-- .../app/authentication/test_authentication.py | 20 +++++------ .../notifications/test_post_notifications.py | 2 +- 4 files changed, 34 insertions(+), 26 deletions(-) diff --git a/app/authentication/auth.py b/app/authentication/auth.py index f8c99fb4b..9f9df9436 100644 --- a/app/authentication/auth.py +++ b/app/authentication/auth.py @@ -10,6 +10,9 @@ from sqlalchemy.orm.exc import NoResultFound from app.dao.services_dao import dao_fetch_service_by_id_with_api_keys +GENERAL_TOKEN_ERROR_MESSAGE = 'Invalid token: make sure your API token matches the example at https://docs.notifications.service.gov.uk/rest-api.html#authorisation-header' # noqa + + class AuthError(Exception): def __init__(self, message, code, service_id=None, api_key_id=None): self.message = {"token": [message]} @@ -36,12 +39,12 @@ class AuthError(Exception): def get_auth_token(req): auth_header = req.headers.get('Authorization', None) if not auth_header: - raise AuthError('Unauthorized, authentication token must be provided', 401) + raise AuthError('Unauthorized: authentication token must be provided', 401) auth_scheme = auth_header[:7].title() if auth_scheme != 'Bearer ': - raise AuthError('Unauthorized, authentication bearer scheme must be used', 401) + raise AuthError('Unauthorized: authentication bearer scheme must be used', 401) return auth_header[7:] @@ -60,17 +63,17 @@ def requires_admin_auth(): g.service_id = current_app.config.get('ADMIN_CLIENT_USER_NAME') return handle_admin_key(auth_token, current_app.config.get('ADMIN_CLIENT_SECRET')) else: - raise AuthError('Unauthorized, admin authentication token required', 401) + raise AuthError('Unauthorized: admin authentication token required', 401) def requires_auth(): request_helper.check_proxy_header_before_request() auth_token = get_auth_token(request) - client = __get_token_issuer(auth_token) + issuer = __get_token_issuer(auth_token) # ie the `iss` claim which should be a service ID try: - service = dao_fetch_service_by_id_with_api_keys(client) + service = dao_fetch_service_by_id_with_api_keys(issuer) except DataError: raise AuthError("Invalid token: service id is not the right data type", 403) except NoResultFound: @@ -92,10 +95,15 @@ def requires_auth(): err_msg = "Invalid token: algorithm used is not HS256" raise AuthError(err_msg, 403, service_id=service.id, api_key_id=api_key.id) except TokenDecodeError: + # we attempted to validate the token but it failed meaning it was not signed using this api key. + # Let's try the next one + # TODO: Change this so it doesn't also catch `TokenIssuerError` or `TokenIssuedAtError` exceptions (which + # are children of `TokenDecodeError`) as these should cause an auth error immediately rather than + # continue on to check the next API key continue except TokenError: - err_msg = 'Invalid token: signature, api token is not valid' - raise AuthError(err_msg, 403, service_id=service.id, api_key_id=api_key.id) + # General error when trying to decode and validate the token + raise AuthError(GENERAL_TOKEN_ERROR_MESSAGE, 403, service_id=service.id, api_key_id=api_key.id) if api_key.expiry_date: raise AuthError("Invalid token: API key revoked", 403, service_id=service.id, api_key_id=api_key.id) @@ -103,7 +111,7 @@ def requires_auth(): g.service_id = api_key.service_id _request_ctx_stack.top.authenticated_service = service _request_ctx_stack.top.api_user = api_key - current_app.logger.info('API authorised for service {} with api key {}, using client {}'.format( + current_app.logger.info('API authorised for service {} with api key {}, using issuer {}'.format( service.id, api_key.id, request.headers.get('User-Agent') @@ -111,17 +119,17 @@ def requires_auth(): return else: # service has API keys, but none matching the one the user provided - raise AuthError("Invalid token: signature, api token not found", 403, service_id=service.id) + raise AuthError("Invalid token: API key not found", 403, service_id=service.id) def __get_token_issuer(auth_token): try: - client = get_token_issuer(auth_token) + issuer = get_token_issuer(auth_token) except TokenIssuerError: raise AuthError("Invalid token: iss field not provided", 403) except TokenDecodeError: - raise AuthError("Invalid token: signature, api token is not valid", 403) - return client + raise AuthError(GENERAL_TOKEN_ERROR_MESSAGE, 403) + return issuer def handle_admin_key(auth_token, secret): @@ -130,4 +138,4 @@ def handle_admin_key(auth_token, secret): except TokenExpiredError: raise AuthError("Invalid token: expired, check that your system clock is accurate", 403) except TokenDecodeError: - raise AuthError("Invalid token: signature, api token is not valid", 403) + raise AuthError("Invalid token: could not decode your API token", 403) diff --git a/app/errors.py b/app/errors.py index 09ff8d633..a699dffc8 100644 --- a/app/errors.py +++ b/app/errors.py @@ -88,12 +88,12 @@ def register_errors(blueprint): @blueprint.errorhandler(401) def unauthorized(e): - error_message = "Unauthorized, authentication token must be provided" + error_message = "Unauthorized: authentication token must be provided" return jsonify(result='error', message=error_message), 401, [('WWW-Authenticate', 'Bearer')] @blueprint.errorhandler(403) def forbidden(e): - error_message = "Forbidden, invalid authentication token provided" + error_message = "Forbidden: invalid authentication token provided" return jsonify(result='error', message=error_message), 403 @blueprint.errorhandler(429) diff --git a/tests/app/authentication/test_authentication.py b/tests/app/authentication/test_authentication.py index 53c235261..b3fec11e4 100644 --- a/tests/app/authentication/test_authentication.py +++ b/tests/app/authentication/test_authentication.py @@ -12,7 +12,7 @@ from notifications_python_client.authentication import create_jwt_token from app import api_user from app.dao.api_key_dao import get_unsigned_secrets, save_model_api_key, get_unsigned_secret, expire_api_key from app.models import ApiKey, KEY_TYPE_NORMAL -from app.authentication.auth import AuthError, requires_admin_auth, requires_auth +from app.authentication.auth import AuthError, requires_admin_auth, requires_auth, GENERAL_TOKEN_ERROR_MESSAGE from tests.conftest import set_config @@ -22,7 +22,7 @@ def test_should_not_allow_request_with_no_token(client, auth_fn): request.headers = {} with pytest.raises(AuthError) as exc: auth_fn() - assert exc.value.short_message == 'Unauthorized, authentication token must be provided' + assert exc.value.short_message == 'Unauthorized: authentication token must be provided' @pytest.mark.parametrize('auth_fn', [requires_auth, requires_admin_auth]) @@ -30,7 +30,7 @@ def test_should_not_allow_request_with_incorrect_header(client, auth_fn): request.headers = {'Authorization': 'Basic 1234'} with pytest.raises(AuthError) as exc: auth_fn() - assert exc.value.short_message == 'Unauthorized, authentication bearer scheme must be used' + assert exc.value.short_message == 'Unauthorized: authentication bearer scheme must be used' @pytest.mark.parametrize('auth_fn', [requires_auth, requires_admin_auth]) @@ -38,7 +38,7 @@ def test_should_not_allow_request_with_incorrect_token(client, auth_fn): request.headers = {'Authorization': 'Bearer 1234'} with pytest.raises(AuthError) as exc: auth_fn() - assert exc.value.short_message == 'Invalid token: signature, api token is not valid' + assert exc.value.short_message == GENERAL_TOKEN_ERROR_MESSAGE @pytest.mark.parametrize('auth_fn', [requires_auth, requires_admin_auth]) @@ -80,7 +80,7 @@ def test_auth_should_not_allow_request_with_no_iat(client, sample_api_key): request.headers = {'Authorization': 'Bearer {}'.format(token)} with pytest.raises(AuthError) as exc: requires_auth() - assert exc.value.short_message == 'Invalid token: signature, api token not found' + assert exc.value.short_message == 'Invalid token: API key not found' def test_auth_should_not_allow_request_with_non_hs256_algorithm(client, sample_api_key): @@ -123,7 +123,7 @@ def test_admin_auth_should_not_allow_request_with_no_iat(client, sample_api_key) request.headers = {'Authorization': 'Bearer {}'.format(token)} with pytest.raises(AuthError) as exc: requires_admin_auth() - assert exc.value.short_message == 'Invalid token: signature, api token is not valid' + assert exc.value.short_message == "Invalid token: could not decode your API token" def test_auth_should_not_allow_request_with_extra_claims(client, sample_api_key): @@ -146,7 +146,7 @@ def test_auth_should_not_allow_request_with_extra_claims(client, sample_api_key) request.headers = {'Authorization': 'Bearer {}'.format(token)} with pytest.raises(AuthError) as exc: requires_auth() - assert exc.value.short_message == 'Invalid token: signature, api token is not valid' + assert exc.value.short_message == GENERAL_TOKEN_ERROR_MESSAGE def test_should_not_allow_invalid_secret(client, sample_api_key): @@ -159,7 +159,7 @@ def test_should_not_allow_invalid_secret(client, sample_api_key): ) assert response.status_code == 403 data = json.loads(response.get_data()) - assert data['message'] == {"token": ['Invalid token: signature, api token not found']} + assert data['message'] == {"token": ['Invalid token: API key not found']} @pytest.mark.parametrize('scheme', ['bearer', 'Bearer']) @@ -276,7 +276,7 @@ def test_authentication_returns_error_when_admin_client_has_no_secrets(client): headers={'Authorization': 'Bearer {}'.format(token)}) assert response.status_code == 403 error_message = json.loads(response.get_data()) - assert error_message['message'] == {"token": ["Invalid token: signature, api token is not valid"]} + assert error_message['message'] == {"token": ["Invalid token: could not decode your API token"]} def test_authentication_returns_error_when_admin_client_secret_is_invalid(client): @@ -291,7 +291,7 @@ def test_authentication_returns_error_when_admin_client_secret_is_invalid(client headers={'Authorization': 'Bearer {}'.format(token)}) assert response.status_code == 403 error_message = json.loads(response.get_data()) - assert error_message['message'] == {"token": ["Invalid token: signature, api token is not valid"]} + assert error_message['message'] == {"token": ["Invalid token: could not decode your API token"]} current_app.config['ADMIN_CLIENT_SECRET'] = api_secret diff --git a/tests/app/v2/notifications/test_post_notifications.py b/tests/app/v2/notifications/test_post_notifications.py index 53b62d730..02cab3c15 100644 --- a/tests/app/v2/notifications/test_post_notifications.py +++ b/tests/app/v2/notifications/test_post_notifications.py @@ -256,7 +256,7 @@ def test_post_notification_returns_401_and_well_formed_auth_error(client, sample error_resp = json.loads(response.get_data(as_text=True)) assert error_resp['status_code'] == 401 assert error_resp['errors'] == [{'error': "AuthError", - 'message': 'Unauthorized, authentication token must be provided'}] + 'message': 'Unauthorized: authentication token must be provided'}] @pytest.mark.parametrize("notification_type, key_send_to, send_to",