diff --git a/app/assets/javascripts/registerSecurityKey.js b/app/assets/javascripts/registerSecurityKey.js index bd6826b4c..2e3280e40 100644 --- a/app/assets/javascripts/registerSecurityKey.js +++ b/app/assets/javascripts/registerSecurityKey.js @@ -27,7 +27,16 @@ }) .then((response) => { if (!response.ok) { - throw Error(response.statusText); + return response.arrayBuffer() + .then((cbor) => { + return Promise.resolve(window.CBOR.decode(cbor)); + }) + .catch(() => { + throw Error(response.statusText); + }) + .then((text) => { + throw Error(text); + }); } window.location.reload(); diff --git a/app/main/views/webauthn_credentials.py b/app/main/views/webauthn_credentials.py index 5304f9d74..e212374d2 100644 --- a/app/main/views/webauthn_credentials.py +++ b/app/main/views/webauthn_credentials.py @@ -3,7 +3,7 @@ from flask import current_app, request, session from flask_login import current_user from app.main import main -from app.models.webauthn_credential import WebAuthnCredential +from app.models.webauthn_credential import RegistrationError, WebAuthnCredential from app.notify_client.user_api_client import user_api_client from app.utils import user_is_platform_admin @@ -34,13 +34,19 @@ def webauthn_begin_register(): @main.route('/webauthn/register', methods=['POST']) @user_is_platform_admin def webauthn_complete_register(): - credential = WebAuthnCredential.from_registration( - session.pop("webauthn_registration_state"), - cbor.decode(request.get_data()), - ) + if 'webauthn_registration_state' not in session: + return cbor.encode("No registration in progress"), 400 + + try: + credential = WebAuthnCredential.from_registration( + session.pop("webauthn_registration_state"), + cbor.decode(request.get_data()), + ) + except RegistrationError as e: + return cbor.encode(str(e)), 400 user_api_client.create_webauthn_credential_for_user( current_user.id, credential ) - return '' + return cbor.encode('') diff --git a/app/models/webauthn_credential.py b/app/models/webauthn_credential.py index 97abb9dc1..d9cfdfe67 100644 --- a/app/models/webauthn_credential.py +++ b/app/models/webauthn_credential.py @@ -2,12 +2,17 @@ import base64 from fido2 import cbor from fido2.client import ClientData +from fido2.cose import UnsupportedKey from fido2.ctap2 import AttestationObject, AttestedCredentialData from flask import current_app from app.models import JSONModel +class RegistrationError(Exception): + pass + + class WebAuthnCredential(JSONModel): ALLOWED_PROPERTIES = { 'id', @@ -22,11 +27,17 @@ class WebAuthnCredential(JSONModel): def from_registration(cls, state, response): server = current_app.webauthn_server - auth_data = server.register_complete( - state, - ClientData(response["clientDataJSON"]), - AttestationObject(response["attestationObject"]), - ) + try: + auth_data = server.register_complete( + state, + ClientData(response["clientDataJSON"]), + AttestationObject(response["attestationObject"]), + ) + except ValueError as e: + raise RegistrationError(e) + + if isinstance(auth_data.credential_data.public_key, UnsupportedKey): + raise RegistrationError("Encryption algorithm not supported") return cls({ 'name': 'Unnamed key', diff --git a/tests/app/main/views/test_webauthn_credentials.py b/tests/app/main/views/test_webauthn_credentials.py index f41aa62f7..769725634 100644 --- a/tests/app/main/views/test_webauthn_credentials.py +++ b/tests/app/main/views/test_webauthn_credentials.py @@ -2,7 +2,7 @@ import pytest from fido2 import cbor from flask import url_for -from app import webauthn_server +from app.models.webauthn_credential import RegistrationError @pytest.mark.parametrize('endpoint', [ @@ -20,18 +20,10 @@ def test_begin_register_returns_encoded_options( mocker, platform_admin_user, platform_admin_client, + webauthn_dev_server, ): - # override base URL so it's consistent on CI and locally - mocker.patch.dict( - app_.config, - values={'ADMIN_BASE_URL': 'http://localhost:6012'} - ) - webauthn_server.init_app(app_) mocker.patch('app.user_api_client.get_webauthn_credentials_for_user', return_value=[]) - - response = platform_admin_client.get( - url_for('main.webauthn_begin_register') - ) + response = platform_admin_client.get(url_for('main.webauthn_begin_register')) assert response.status_code == 200 @@ -49,7 +41,7 @@ def test_begin_register_returns_encoded_options( relying_party_options = webauthn_options['rp'] assert relying_party_options['name'] == 'GOV.UK Notify' - assert relying_party_options['id'] == 'localhost' + assert relying_party_options['id'] == 'webauthn.io' def test_begin_register_includes_existing_credentials( @@ -132,3 +124,37 @@ def test_complete_register_clears_session( with platform_admin_client.session_transaction() as session: assert 'webauthn_registration_state' not in session + + +def test_complete_register_handles_library_errors( + platform_admin_client, + mocker, +): + with platform_admin_client.session_transaction() as session: + session['webauthn_registration_state'] = 'state' + + mocker.patch( + 'app.models.webauthn_credential.WebAuthnCredential.from_registration', + side_effect=RegistrationError('error') + ) + + response = platform_admin_client.post( + url_for('main.webauthn_complete_register'), + data=cbor.encode('public_key_credential'), + ) + + assert response.status_code == 400 + assert cbor.decode(response.data) == 'error' + + +def test_complete_register_handles_missing_state( + platform_admin_client, + mocker, +): + response = platform_admin_client.post( + url_for('main.webauthn_complete_register'), + data=cbor.encode('public_key_credential'), + ) + + assert response.status_code == 400 + assert cbor.decode(response.data) == 'No registration in progress' diff --git a/tests/app/models/test_webauthn_credential.py b/tests/app/models/test_webauthn_credential.py index 49e50bab9..07a7a2006 100644 --- a/tests/app/models/test_webauthn_credential.py +++ b/tests/app/models/test_webauthn_credential.py @@ -4,8 +4,7 @@ import pytest from fido2 import cbor from fido2.cose import ES256 -from app import webauthn_server -from app.models.webauthn_credential import WebAuthnCredential +from app.models.webauthn_credential import RegistrationError, WebAuthnCredential # noqa adapted from https://github.com/duo-labs/py_webauthn/blob/90e3d97e0182899a35a70fc510280b4082cce19b/tests/test_webauthn.py#L14-L24 SESSION_STATE = {'challenge': 'bPzpX3hHQtsp9evyKYkaZtVc9UN07PUdJ22vZUdDp94', 'user_verification': 'discouraged'} @@ -14,21 +13,11 @@ CLIENT_DATA_JSON = b'{"type": "webauthn.create", "clientExtensions": {}, "challe # had to use the cbor2 library to re-encode the attestationObject due to implementation differences ATTESTATION_OBJECT = base64.b64decode(b'o2NmbXRoZmlkby11MmZnYXR0U3RtdKJjc2lnWEgwRgIhAI1qbvWibQos/t3zsTU05IXw1Ek3SDApATok09uc4UBwAiEAv0fB/lgb5Ot3zJ691Vje6iQLAtLhJDiA8zDxaGjcE3hjeDVjgVkCUzCCAk8wggE3oAMCAQICBDxoKU0wDQYJKoZIhvcNAQELBQAwLjEsMCoGA1UEAxMjWXViaWNvIFUyRiBSb290IENBIFNlcmlhbCA0NTcyMDA2MzEwIBcNMTQwODAxMDAwMDAwWhgPMjA1MDA5MDQwMDAwMDBaMDExLzAtBgNVBAMMJll1YmljbyBVMkYgRUUgU2VyaWFsIDIzOTI1NzM0ODExMTE3OTAxMFkwEwYHKoZIzj0CAQYIKoZIzj0DAQcDQgAEvd9nk9t3lMNQMXHtLE1FStlzZnUaSLql2fm1ajoggXlrTt8rzXuSehSTEPvEaEdv/FeSqX22L6Aoa8ajIAIOY6M7MDkwIgYJKwYBBAGCxAoCBBUxLjMuNi4xLjQuMS40MTQ4Mi4xLjUwEwYLKwYBBAGC5RwCAQEEBAMCBSAwDQYJKoZIhvcNAQELBQADggEBAKrADVEJfuwVpIazebzEg0D4Z9OXLs5qZ/ukcONgxkRZ8K04QtP/CB5x6olTlxsj+SXArQDCRzEYUgbws6kZKfuRt2a1P+EzUiqDWLjRILSr+3/o7yR7ZP/GpiFKwdm+czb94POoGD+TS1IYdfXj94mAr5cKWx4EKjh210uovu/pLdLjc8xkQciUrXzZpPR9rT2k/q9HkZhHU+NaCJzky+PTyDbq0KKnzqVhWtfkSBCGw3ezZkTS+5lrvOKbIa24lfeTgu7FST5OwTPCFn8HcfWZMXMSD/KNU+iBqJdAwTLPPDRoLLvPTl29weCAIh+HUpmBQd0UltcPOrA/LFvAf61oYXV0aERhdGFYwnSm6pITyZwvdLIkkrMgz0AmKpTBqVCgOX8pJQtghB7wQQAAAAAAAAAAAAAAAAAAAAAAAAAAAECKU1ppjl9gmhHWyDkgHsUvZmhr6oF3/lD3llzLE2SaOSgOGIsIuAQqgp8JQSUu3r/oOaP8RS44dlQjrH+ALfYtpAECAyYhWCAxnqAfESXOYjKUc2WACuXZ3ch0JHxV0VFrrTyjyjIHXCJYIFnx8H87L4bApR4M+hPcV+fHehEOeW+KCyd0H+WGY8s6') # noqa - -@pytest.fixture -def disable_webauthn_origin_verification(app_, mocker): - mocker.patch.dict( - app_.config, values={ - 'NOTIFY_ENVIRONMENT': 'development', - 'ADMIN_BASE_URL': 'https://webauthn.io', - } - ) - - # disable origin verification for non-HTTPS test - webauthn_server.init_app(app_) +# manually adapted by working out which character in the encoded CBOR corresponds to the public key algorithm ID +UNSUPPORTED_ATTESTATION_OBJECT = base64.b64decode(b'o2NmbXRoZmlkby11MmZnYXR0U3RtdKJjc2lnWEgwRgIhAI1qbvWibQos/t3zsTU05IXw1Ek3SDApATok09uc4UBwAiEAv0fB/lgb5Ot3zJ691Vje6iQLAtLhJDiA8zDxaGjcE3hjeDVjgVkCUzCCAk8wggE3oAMCAQICBDxoKU0wDQYJKoZIhvcNAQELBQAwLjEsMCoGA1UEAxMjWXViaWNvIFUyRiBSb290IENBIFNlcmlhbCA0NTcyMDA2MzEwIBcNMTQwODAxMDAwMDAwWhgPMjA1MDA5MDQwMDAwMDBaMDExLzAtBgNVBAMMJll1YmljbyBVMkYgRUUgU2VyaWFsIDIzOTI1NzM0ODExMTE3OTAxMFkwEwYHKoZIzj0CAQYIKoZIzj0DAQcDQgAEvd9nk9t3lMNQMXHtLE1FStlzZnUaSLql2fm1ajoggXlrTt8rzXuSehSTEPvEaEdv/FeSqX22L6Aoa8ajIAIOY6M7MDkwIgYJKwYBBAGCxAoCBBUxLjMuNi4xLjQuMS40MTQ4Mi4xLjUwEwYLKwYBBAGC5RwCAQEEBAMCBSAwDQYJKoZIhvcNAQELBQADggEBAKrADVEJfuwVpIazebzEg0D4Z9OXLs5qZ/ukcONgxkRZ8K04QtP/CB5x6olTlxsj+SXArQDCRzEYUgbws6kZKfuRt2a1P+EzUiqDWLjRILSr+3/o7yR7ZP/GpiFKwdm+czb94POoGD+TS1IYdfXj94mAr5cKWx4EKjh210uovu/pLdLjc8xkQciUrXzZpPR9rT2k/q9HkZhHU+NaCJzky+PTyDbq0KKnzqVhWtfkSBCGw3ezZkTS+5lrvOKbIa24lfeTgu7FST5OwTPCFn8HcfWZMXMSD/KNU+iBqJdAwTLPPDRoLLvPTl29weCAIh+HUpmBQd0UltcPOrA/LFvAf61oYXV0aERhdGFYwnSm6pITyZwvdLIkkrMgz0AmKpTBqVCgOX8pJQtghB7wQQAAAAAAAAAAAAAAAAAAAAAAAAAAAECKU1ppjl9gmhHWyDkgHsUvZmhr6oF3/lD3llzLE2SaOSgOGIsIuAQqgp8JQSUu3r/oOaP8RS44dlQjrH+ALfYtpAECAyUhWCAxnqAfESXOYjKUc2WACuXZ3ch0JHxV0VFrrTyjyjIHXCJYIFnx8H87L4bApR4M+hPcV+fHehEOeW+KCyd0H+WGY8s6') # noqa -def test_from_registration_verifies_response(disable_webauthn_origin_verification): +def test_from_registration_verifies_response(webauthn_dev_server): registration_response = { 'clientDataJSON': CLIENT_DATA_JSON, 'attestationObject': ATTESTATION_OBJECT, @@ -44,7 +33,7 @@ def test_from_registration_verifies_response(disable_webauthn_origin_verificatio assert credential_data.public_key[3] == ES256.ALGORITHM -def test_from_registration_encodes_as_unicode(disable_webauthn_origin_verification): +def test_from_registration_encodes_as_unicode(webauthn_dev_server): registration_response = { 'clientDataJSON': CLIENT_DATA_JSON, 'attestationObject': ATTESTATION_OBJECT, @@ -56,3 +45,27 @@ def test_from_registration_encodes_as_unicode(disable_webauthn_origin_verificati assert type(serialized_credential['credential_data']) == str assert type(serialized_credential['registration_response']) == str + + +def test_from_registration_handles_library_errors(app_): + registration_response = { + 'clientDataJSON': CLIENT_DATA_JSON, + 'attestationObject': ATTESTATION_OBJECT, + } + + with pytest.raises(RegistrationError) as exc_info: + WebAuthnCredential.from_registration(SESSION_STATE, registration_response) + + assert 'Invalid origin' in str(exc_info.value) + + +def test_from_registration_handles_unsupported_keys(webauthn_dev_server): + registration_response = { + 'clientDataJSON': CLIENT_DATA_JSON, + 'attestationObject': UNSUPPORTED_ATTESTATION_OBJECT, + } + + with pytest.raises(RegistrationError) as exc_info: + WebAuthnCredential.from_registration(SESSION_STATE, registration_response) + + assert 'Encryption algorithm not supported' in str(exc_info.value) diff --git a/tests/app/test_webauthn_server.py b/tests/app/test_webauthn_server.py index 23093378b..c2bc70381 100644 --- a/tests/app/test_webauthn_server.py +++ b/tests/app/test_webauthn_server.py @@ -3,33 +3,36 @@ import pytest from app import webauthn_server +@pytest.fixture +def app_with_mock_config(mocker): + app = mocker.Mock() + + app.config = { + 'ADMIN_BASE_URL': 'https://www.notify.works', + 'NOTIFY_ENVIRONMENT': 'development' + } + + return app + + @pytest.mark.parametrize(('environment, allowed'), [ ('development', True), ('production', False) ]) def test_server_origin_verification( - app_, - mocker, + app_with_mock_config, environment, allowed ): - mocker.patch.dict( - app_.config, - values={'NOTIFY_ENVIRONMENT': environment} - ) - webauthn_server.init_app(app_) - assert app_.webauthn_server._verify('fake-domain') == allowed + app_with_mock_config.config['NOTIFY_ENVIRONMENT'] = environment + webauthn_server.init_app(app_with_mock_config) + assert app_with_mock_config.webauthn_server._verify('fake-domain') == allowed def test_server_relying_party_id( - app_, + app_with_mock_config, mocker, ): - mocker.patch.dict( - app_.config, - values={'ADMIN_BASE_URL': 'https://www.notify.works'} - ) - - webauthn_server.init_app(app_) - assert app_.webauthn_server.rp.id == 'www.notify.works' + webauthn_server.init_app(app_with_mock_config) + assert app_with_mock_config.webauthn_server.rp.id == 'www.notify.works' diff --git a/tests/conftest.py b/tests/conftest.py index 36950f562..c9fe05cac 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -13,7 +13,7 @@ from flask import Flask, url_for from notifications_python_client.errors import HTTPError from notifications_utils.url_safe_token import generate_token -from app import create_app +from app import create_app, webauthn_server from . import ( TestClient, @@ -3245,6 +3245,20 @@ def set_config_values(app, dict): app.config[key] = old_values[key] +@pytest.fixture +def webauthn_dev_server(app_, mocker): + overrides = { + 'NOTIFY_ENVIRONMENT': 'development', + 'ADMIN_BASE_URL': 'https://webauthn.io', + } + + with set_config_values(app_, overrides): + webauthn_server.init_app(app_) + yield + + webauthn_server.init_app(app_) + + @pytest.fixture(scope='function') def valid_token(app_, fake_uuid): return generate_token( diff --git a/tests/javascripts/registerSecurityKey.test.js b/tests/javascripts/registerSecurityKey.test.js index 94e756361..53e13e9d3 100644 --- a/tests/javascripts/registerSecurityKey.test.js +++ b/tests/javascripts/registerSecurityKey.test.js @@ -98,9 +98,10 @@ describe('Register security key', () => { }) test.each([ - ['network'], - ['server'], - ])('alerts if sending WebAuthn credentials fails (%s error)', ({errorType}, done) => { + ['network error'], + ['internal server error'], + ['bad request'], + ])('alerts if sending WebAuthn credentials fails (%s)', (errorType, done) => { jest.spyOn(window.navigator.credentials, 'create').mockImplementation(() => { // fake PublicKeyCredential response from WebAuthn API return Promise.resolve({ response: {} }) @@ -117,10 +118,15 @@ describe('Register security key', () => { // subsequent POST of credential data to server } else { - if (errorType == 'network') { - return Promise.reject('error') - } else { - return Promise.resolve({ ok: false, statusText: 'error' }) + switch (errorType) { + case 'network error': + return Promise.reject('error') + case 'bad request': + message = Promise.resolve(window.CBOR.encode('error')) + return Promise.resolve({ ok: false, arrayBuffer: () => message }) + case 'internal server error': + message = Promise.reject('encoding error') + return Promise.resolve({ ok: false, arrayBuffer: () => message, statusText: 'error' }) } } })