From bb5f6bcccacd1a2134ff369488eef6e807e77eef Mon Sep 17 00:00:00 2001 From: Johan Lundberg Date: Tue, 17 Dec 2024 10:44:57 +0100 Subject: [PATCH 1/8] switch from default deny to default allow for security keys a security key no longer needs to be in metadata to be valid for mfa --- src/eduid/webapp/security/settings/common.py | 20 +++---- .../webapp/security/webauthn_proofing.py | 56 +++++-------------- 2 files changed, 22 insertions(+), 54 deletions(-) diff --git a/src/eduid/webapp/security/settings/common.py b/src/eduid/webapp/security/settings/common.py index ce1bb1c7b..ef63448c1 100644 --- a/src/eduid/webapp/security/settings/common.py +++ b/src/eduid/webapp/security/settings/common.py @@ -42,11 +42,11 @@ class SecurityConfig( # webauthn webauthn_proofing_method: str = Field(default="webauthn metadata") - webauthn_proofing_version: str = Field(default="2022v1") + webauthn_proofing_version: str = Field(default="2024v1") webauthn_max_allowed_tokens: int = 10 webauthn_attestation: AttestationConveyancePreference | None = None webauthn_user_verification: UserVerificationRequirement = UserVerificationRequirement.PREFERRED - webauthn_allowed_user_verification_methods: list[str] = Field( + webauthn_recommended_user_verification_methods: list[str] = Field( default=[ "faceprint_internal", "passcode_external", @@ -59,18 +59,16 @@ class SecurityConfig( "apple", ] ) - webauthn_allowed_key_protection: list[str] = Field( + webauthn_recommended_key_protection: list[str] = Field( default=["remote_handle", "hardware", "secure_element", "tee", "apple"] ) - webauthn_allowed_status: list[AuthenticatorStatus] = Field( + webauthn_disallowed_status: list[AuthenticatorStatus] = Field( default=[ - AuthenticatorStatus.FIDO_CERTIFIED, - AuthenticatorStatus.FIDO_CERTIFIED_L1, - AuthenticatorStatus.FIDO_CERTIFIED_L2, - AuthenticatorStatus.FIDO_CERTIFIED_L3, - AuthenticatorStatus.FIDO_CERTIFIED_L1plus, - AuthenticatorStatus.FIDO_CERTIFIED_L2plus, - AuthenticatorStatus.FIDO_CERTIFIED_L3plus, + AuthenticatorStatus.USER_VERIFICATION_BYPASS, + AuthenticatorStatus.ATTESTATION_KEY_COMPROMISE, + AuthenticatorStatus.USER_KEY_REMOTE_COMPROMISE, + AuthenticatorStatus.USER_KEY_PHYSICAL_COMPROMISE, + AuthenticatorStatus.REVOKED, ] ) diff --git a/src/eduid/webapp/security/webauthn_proofing.py b/src/eduid/webapp/security/webauthn_proofing.py index 1731b34dc..137ffd15d 100644 --- a/src/eduid/webapp/security/webauthn_proofing.py +++ b/src/eduid/webapp/security/webauthn_proofing.py @@ -134,54 +134,24 @@ def is_authenticator_mfa_approved(authenticator_info: AuthenticatorInformation) """ This is our current policy for determine if a FIDO2 authenticator can do multi-factor authentications. """ - # If there is no attestation we can not trust the authenticator info - if authenticator_info.attestation_format == AttestationFormat.NONE: - return False + if authenticator_info.user_verified: + current_app.logger.info( + f"AttestationFormat {authenticator_info.attestation_format}: user verified - authenticator is mfa capable" + ) - # Our current policy is that Apple is capable of mfa - if authenticator_info.status is OtherAuthenticatorStatus.APPLE: - current_app.logger.debug("apple device is mfa capable") - return True + # check status in metadata (if any) and disallow incident statuses + if authenticator_info.status and authenticator_info.status in current_app.conf.webauthn_disallowed_status: + current_app.logger.debug(f"status {authenticator_info.status} is not mfa capable") + return False - # check status in metadata and disallow uncertified and incident statuses - if authenticator_info.status not in current_app.conf.webauthn_allowed_status: - current_app.logger.debug(f"status {authenticator_info.status} is not mfa capable") - return False - - # true if the authenticator supports any of the user verification methods we allow - is_accepted_user_verification = any( - [ - method - for method in authenticator_info.user_verification_methods - if method in current_app.conf.webauthn_allowed_user_verification_methods - ] - ) - # a typical token has key protection ["hardware"] or ["hardware", "tee"] but some also support software, so - # we have to check that all key protections supported is in our allow list - is_accepted_key_protection = all( - [ - protection - for protection in authenticator_info.key_protection - if protection in current_app.conf.webauthn_allowed_key_protection - ] - ) - current_app.logger.debug(f"is_accepted_user_verification: {is_accepted_user_verification}") - current_app.logger.debug(f"is_accepted_key_protection: {is_accepted_key_protection}") - if is_accepted_user_verification and is_accepted_key_protection: return True + current_app.logger.info( + f"AttestationFormat {authenticator_info.attestation_format}: user NOT verified - authenticator is not mfa capable" + ) return False def save_webauthn_proofing_log(eppn: str, authenticator_info: AuthenticatorInformation) -> bool: - user_verification_methods_match = set(authenticator_info.user_verification_methods) & set( - current_app.conf.webauthn_allowed_user_verification_methods - ) - current_app.logger.debug(f"user verifications methods that match config: {user_verification_methods_match}") - key_protection_match = set(authenticator_info.key_protection) & set( - current_app.conf.webauthn_allowed_key_protection - ) - current_app.logger.debug(f"user verifications methods that match config: {user_verification_methods_match}") - proofing_element = WebauthnMfaCapabilityProofingLog( created_by=current_app.conf.app_name, eppn=eppn, @@ -189,8 +159,8 @@ def save_webauthn_proofing_log(eppn: str, authenticator_info: AuthenticatorInfor proofing_method=current_app.conf.webauthn_proofing_method, authenticator_id=authenticator_info.authenticator_id, attestation_format=authenticator_info.attestation_format, - user_verification_methods=list(user_verification_methods_match), - key_protection=list(key_protection_match), + user_verification_methods=authenticator_info.user_verification_methods, + key_protection=authenticator_info.key_protection, ) current_app.logger.debug(f"webauthn mfa capability proofing element: {proofing_element}") return current_app.proofing_log.save(proofing_element) From f2592bb296e37740091f75efd70d2e1e4cfece91 Mon Sep 17 00:00:00 2001 From: Johan Lundberg Date: Thu, 23 Jan 2025 10:33:20 +0100 Subject: [PATCH 2/8] implement old mfa approved checks for security recommendation list --- src/eduid/webapp/security/helpers.py | 56 +++++++++++++++++----------- 1 file changed, 34 insertions(+), 22 deletions(-) diff --git a/src/eduid/webapp/security/helpers.py b/src/eduid/webapp/security/helpers.py index 5de3c9239..b80902172 100644 --- a/src/eduid/webapp/security/helpers.py +++ b/src/eduid/webapp/security/helpers.py @@ -240,40 +240,52 @@ def update_user_official_name(security_user: SecurityUser, navet_data: NavetData @cache def get_approved_security_keys() -> dict[str, Any]: # a way to reuse is_authenticator_mfa_approved() from security app - parsed_entries: list[AuthenticatorInformation] = [] + parsed_entries: list[str] = [] for metadata_entry in current_app.fido_mds.metadata.entries: user_verification_methods = [ detail.user_verification_method for detail in metadata_entry.metadata_statement.get_user_verification_details() ] - # simulated to fit AuthenticatorInformation format - attestation_format = AttestationFormat.PACKED - if not metadata_entry.metadata_statement.attestation_types: - attestation_format = AttestationFormat.NONE - - authenticator_info = AuthenticatorInformation( - authenticator_id=metadata_entry.aaguid or metadata_entry.aaid, - attestation_format=attestation_format, - user_present=False, # simulated to fit AuthenticatorInformation required fields - user_verified=False, # simulated to fit AuthenticatorInformation required fields - status=metadata_entry.status_reports[0].status, - last_status_change=metadata_entry.time_of_last_status_change, - user_verification_methods=user_verification_methods, - key_protection=metadata_entry.metadata_statement.key_protection, - description=metadata_entry.metadata_statement.description, + # check latest status in metadata and disallow incident statuses + # TODO: user latest_status_report method on Entry when available in python-fido-mds + latest_status_report = max(metadata_entry.status_reports, key=lambda sr: sr.effective_date) + if latest_status_report.status in current_app.conf.webauthn_disallowed_status: + current_app.logger.debug(f"status {latest_status_report.status} is not mfa capable") + continue + + # true if the authenticator supports any of the user verification methods we allow + is_accepted_user_verification = any( + [ + method + for method in user_verification_methods + if method in current_app.conf.webauthn_recommended_user_verification_methods + ] ) - parsed_entries.append(authenticator_info) - - approved_keys_list: list[str] = [ - entry.description for entry in parsed_entries if entry.description and is_authenticator_mfa_approved(entry) - ] + # a typical token has key protection ["hardware"] or ["hardware", "tee"] but some also support software, so + # we have to check that all key protections supported is in our allow list + is_accepted_key_protection = all( + [ + protection + for protection in metadata_entry.metadata_statement.key_protection + if protection in current_app.conf.webauthn_recommended_key_protection + ] + ) + current_app.logger.debug(f"security key description: {metadata_entry.metadata_statement.description}") + current_app.logger.debug(f"is_accepted_user_verification: {is_accepted_user_verification}") + current_app.logger.debug(f"is_accepted_key_protection: {is_accepted_key_protection}") + if ( + is_accepted_user_verification + and is_accepted_key_protection + and metadata_entry.metadata_statement.description + ): + parsed_entries.append(metadata_entry.metadata_statement.description) # remove case-insensitive duplicates from list, while maintaining the original case marker = set() unique_approved_keys_list: list[str] = [] - for key in approved_keys_list: + for key in parsed_entries: lower_case_key = key.lower() if lower_case_key not in marker: # test presence marker.add(lower_case_key) From 469b5e9c79e6a046ac42a474f25af5164a1ea120 Mon Sep 17 00:00:00 2001 From: Johan Lundberg Date: Thu, 23 Jan 2025 10:33:52 +0100 Subject: [PATCH 3/8] we no longer fail on bad or missing metadata --- src/eduid/webapp/security/views/webauthn.py | 5 ----- src/eduid/webapp/security/webauthn_proofing.py | 16 +++++++++++++++- 2 files changed, 15 insertions(+), 6 deletions(-) diff --git a/src/eduid/webapp/security/views/webauthn.py b/src/eduid/webapp/security/views/webauthn.py index 38ef36acd..3757e32bc 100644 --- a/src/eduid/webapp/security/views/webauthn.py +++ b/src/eduid/webapp/security/views/webauthn.py @@ -167,11 +167,6 @@ def registration_complete( current_app.logger.info(f"attestation_object: {attestation_object}") current_app.logger.info(f"client_data: {client_data}") return error_response(message=SecurityMsg.webauthn_attestation_fail) - except MetadataValidationError: - current_app.logger.exception("metadata validation failed") - current_app.logger.info(f"attestation_object: {attestation_object}") - current_app.logger.info(f"client_data: {client_data}") - return error_response(message=SecurityMsg.webauthn_metadata_fail) # Move registration state from session to local variable to let users restart if something fails reg_state = session.security.webauthn_registration diff --git a/src/eduid/webapp/security/webauthn_proofing.py b/src/eduid/webapp/security/webauthn_proofing.py index 137ffd15d..078186ae8 100644 --- a/src/eduid/webapp/security/webauthn_proofing.py +++ b/src/eduid/webapp/security/webauthn_proofing.py @@ -74,11 +74,25 @@ def get_authenticator_information(attestation: str, client_data: str) -> Authent # verify attestation try: current_app.fido_mds.verify_attestation(attestation=att, client_data=websafe_decode(client_data)) - except (AttestationVerificationError, MetadataValidationError) as e: + except AttestationVerificationError as e: current_app.logger.debug(f"attestation: {att}") current_app.logger.debug(f"client_data: {client_data}") current_app.logger.exception("Failed to get authenticator information") raise e + except MetadataValidationError: + current_app.logger.debug(f"attestation: {att}") + current_app.logger.debug(f"client_data: {client_data}") + current_app.logger.exception( + "Failed to get authenticator information from metadata, continuing without metadata" + ) + # Continue even if the security key _should_ be found and validated with metadata as we have seen security keys + # in the wild that fails + return AuthenticatorInformation( + attestation_format=att.fmt, + authenticator_id=authenticator_id, + user_present=user_present, + user_verified=user_verified, + ) # There are no metadata entries for Apple devices, just create the authenticator information if att.fmt is AttestationFormat.APPLE: From db2c6f9aacc99bb9e5a3789b350fdf5879ed5aa4 Mon Sep 17 00:00:00 2001 From: Johan Lundberg Date: Thu, 23 Jan 2025 10:34:46 +0100 Subject: [PATCH 4/8] check latest date instead of relying on order --- src/eduid/webapp/security/webauthn_proofing.py | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/eduid/webapp/security/webauthn_proofing.py b/src/eduid/webapp/security/webauthn_proofing.py index 078186ae8..694301763 100644 --- a/src/eduid/webapp/security/webauthn_proofing.py +++ b/src/eduid/webapp/security/webauthn_proofing.py @@ -133,7 +133,9 @@ def get_authenticator_information(attestation: str, client_data: str) -> Authent return AuthenticatorInformation( attestation_format=att.fmt, authenticator_id=att.aaguid or att.certificate_key_identifier, - status=metadata_entry.status_reports[0].status, # latest status reports status + status=max( + metadata_entry.status_reports, key=lambda sr: sr.effective_date + ).status, # latest status reports status last_status_change=last_status_change, user_verification_methods=user_verification_methods, key_protection=metadata_entry.metadata_statement.key_protection, From 9e0170e320750d6cb4646653b3e0d04d13571e46 Mon Sep 17 00:00:00 2001 From: Johan Lundberg Date: Thu, 23 Jan 2025 10:35:42 +0100 Subject: [PATCH 5/8] python-fido-mds yubikey 5 data does not have user verification true --- src/eduid/webapp/security/tests/test_webauthn.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/eduid/webapp/security/tests/test_webauthn.py b/src/eduid/webapp/security/tests/test_webauthn.py index 432cfd428..5cf7bc3e6 100644 --- a/src/eduid/webapp/security/tests/test_webauthn.py +++ b/src/eduid/webapp/security/tests/test_webauthn.py @@ -637,9 +637,9 @@ def test_authenticator_information(self) -> None: with self.app.test_request_context(): res = is_authenticator_mfa_approved(authenticator_info=authenticator_info) - if authenticator in [YUBIKEY_4, NONE_ATTESTATION]: + if authenticator in [YUBIKEY_4, YUBIKEY_5_NFC]: # Yubikey 4 does not support any user verification we accept - # None attestations cannot be verified to support anything we accept + # The test data for Yubikey 5 do not include user verification assert res is False else: assert res is True From ecf3a841c05adc4f9afe750100cd10b44f35c108 Mon Sep 17 00:00:00 2001 From: Johan Lundberg Date: Thu, 23 Jan 2025 10:36:25 +0100 Subject: [PATCH 6/8] updated security test data to include a user verification true registration --- .../webapp/security/tests/test_webauthn.py | 51 ++++++++----------- 1 file changed, 22 insertions(+), 29 deletions(-) diff --git a/src/eduid/webapp/security/tests/test_webauthn.py b/src/eduid/webapp/security/tests/test_webauthn.py index 5cf7bc3e6..f76a41bcc 100644 --- a/src/eduid/webapp/security/tests/test_webauthn.py +++ b/src/eduid/webapp/security/tests/test_webauthn.py @@ -65,34 +65,27 @@ ) # CTAP2 security key -STATE_2 = {"challenge": "iW6wn2xAYUfBueKvhIyTsB6YRsQz9OIwaPfw1ZoCtNY", "user_verification": "discouraged"} - -ATTESTATION_OBJECT_2 = ( - b"o2NmbXRmcGFja2VkZ2F0dFN0bXSjY2FsZyZjc2lnWEcwRQIhANNvwZhaTvdSujKW3pUCfeYB_ABjCo2X" - b"dg8e5RowhAgZAiBmj8DH71y46Rg9W67BTG1MuBmaycK7osVy6g_ppmJGiGN4NWOBWQLBMIICvTCCAaWg" - b"AwIBAgIEHo-HNDANBgkqhkiG9w0BAQsFADAuMSwwKgYDVQQDEyNZdWJpY28gVTJGIFJvb3QgQ0EgU2Vy" - b"aWFsIDQ1NzIwMDYzMTAgFw0xNDA4MDEwMDAwMDBaGA8yMDUwMDkwNDAwMDAwMFowbjELMAkGA1UEBhMC" - b"U0UxEjAQBgNVBAoMCVl1YmljbyBBQjEiMCAGA1UECwwZQXV0aGVudGljYXRvciBBdHRlc3RhdGlvbjEn" - b"MCUGA1UEAwweWXViaWNvIFUyRiBFRSBTZXJpYWwgNTEyNzIyNzQwMFkwEwYHKoZIzj0CAQYIKoZIzj0D" - b"AQcDQgAEqHn4IzjtFJS6wHBLzH_GY9GycXFZdiQxAcdgURXXwVKeKBwcZzItOEtc1V3T6YGNX9hcIq8y" - b"bgxk_CCv4z8jZqNsMGowIgYJKwYBBAGCxAoCBBUxLjMuNi4xLjQuMS40MTQ4Mi4xLjcwEwYLKwYBBAGC" - b"5RwCAQEEBAMCBDAwIQYLKwYBBAGC5RwBAQQEEgQQL8BXn4ETR-qxFrtajbkgKjAMBgNVHRMBAf8EAjAA" - b"MA0GCSqGSIb3DQEBCwUAA4IBAQCGk_9i3w1XedR0jX_I0QInMYqOWA5qOlfBCOlOA8OFaLNmiU_OViS-" - b"Sj79fzQRiz2ZN0P3kqGYkWDI_JrgsE49-e4V4-iMBPyCqNy_WBjhCNzCloV3rnn_ZiuUc0497EWXMF1z" - b"5uVe4r65zZZ4ygk15TPrY4-OJvq7gXzaRB--mDGDKuX24q2ZL56720xiI4uPjXq0gdbTJjvNv55KV1UD" - b"cJiK1YE0QPoDLK22cjyt2PjXuoCfdbQ8_6Clua3RQjLvnZ4UgSY4IzxMpKhzufismOMroZFnYG4VkJ_N" - b"20ot_72uRiAkn5pmRqyB5IMtERn-v6pzGogtolp3gn1G0ZAXaGF1dGhEYXRhWMTc9wRxJipqDOIk0QJj" - b"FIEoxHTUnimbJIcg88Mz7jZh00EAAAAEL8BXn4ETR-qxFrtajbkgKgBAz6lLs6rFz6zm4IH73RUcSaVb" - b"C5v4-J6j8HGS-VwPYIhyYUi6d1mxHYZPuehFrC5-r4ZmFqd7gpMdoJCo4H1bT6UBAgMmIAEhWCBTzCFk" - b"fHJcW7ny9bUS4h8YqUadyw0q4kg91vgkScDscCJYICsxt49Uv09cN4OSw93GtjMLxgoaIfFhAK0vd9WL" - b"vOY8" -) - -CLIENT_DATA_JSON_2 = ( - b"eyJ0eXBlIjoid2ViYXV0aG4uY3JlYXRlIiwiY2hhbGxlbmdlIjoiaVc2d24yeEFZVWZCdWVLdmhJeVRz" - b"QjZZUnNRejlPSXdhUGZ3MVpvQ3ROWSIsIm9yaWdpbiI6Imh0dHBzOi8vZGFzaGJvYXJkLmVkdWlkLmRv" - b"Y2tlciIsImNyb3NzT3JpZ2luIjpmYWxzZX0" -) +STATE_2 = {"challenge": "yxHWG+ouoa8MLGSxOJJhM0NtiH5ubK3BPfx+6uE3QkU=", "user_verification": "required"} + +ATTESTATION_OBJECT_2 = b""" +o2NmbXRmcGFja2VkZ2F0dFN0bXSjY2FsZyZjc2lnWEYwRAIgHFOTISd2NExUtGr1GTkgImVIJx09yJfKdx7j1f714r0CIF6vTppPv1mRyism5kZjMq+pEvi3 +BATxv2m/kRvlD5zhY3g1Y4FZAsEwggK9MIIBpaADAgECAgQej4c0MA0GCSqGSIb3DQEBCwUAMC4xLDAqBgNVBAMTI1l1YmljbyBVMkYgUm9vdCBDQSBTZXJp +YWwgNDU3MjAwNjMxMCAXDTE0MDgwMTAwMDAwMFoYDzIwNTAwOTA0MDAwMDAwWjBuMQswCQYDVQQGEwJTRTESMBAGA1UECgwJWXViaWNvIEFCMSIwIAYDVQQL +DBlBdXRoZW50aWNhdG9yIEF0dGVzdGF0aW9uMScwJQYDVQQDDB5ZdWJpY28gVTJGIEVFIFNlcmlhbCA1MTI3MjI3NDAwWTATBgcqhkjOPQIBBggqhkjOPQMB +BwNCAASoefgjOO0UlLrAcEvMf8Zj0bJxcVl2JDEBx2BRFdfBUp4oHBxnMi04S1zVXdPpgY1f2FwirzJuDGT8IK/jPyNmo2wwajAiBgkrBgEEAYLECgIEFTEu +My42LjEuNC4xLjQxNDgyLjEuNzATBgsrBgEEAYLlHAIBAQQEAwIEMDAhBgsrBgEEAYLlHAEBBAQSBBAvwFefgRNH6rEWu1qNuSAqMAwGA1UdEwEB/wQCMAAw +DQYJKoZIhvcNAQELBQADggEBAIaT/2LfDVd51HSNf8jRAicxio5YDmo6V8EI6U4Dw4Vos2aJT85WJL5KPv1/NBGLPZk3Q/eSoZiRYMj8muCwTj357hXj6IwE +/IKo3L9YGOEI3MKWhXeuef9mK5RzTj3sRZcwXXPm5V7ivrnNlnjKCTXlM+tjj44m+ruBfNpEH76YMYMq5fbirZkvnrvbTGIji4+NerSB1tMmO82/nkpXVQNw +mIrVgTRA+gMsrbZyPK3Y+Ne6gJ91tDz/oKW5rdFCMu+dnhSBJjgjPEykqHO5+KyY4yuhkWdgbhWQn83bSi3/va5GICSfmmZGrIHkgy0RGf6/qnMaiC2iWneC +fUbRkBdoYXV0aERhdGFYxNz3BHEmKmoM4iTRAmMUgSjEdNSeKZskhyDzwzPuNmHTRQAAAAIvwFefgRNH6rEWu1qNuSAqAEDl1oXc7ZWmdlYmRhcoe5htjx6D ++mBRxGkyn3o/xCQfq0FrCveEmEHkG9YmfOxgM77SQrRfG9o3jHuZfpEBLZwopQECAyYgASFYIAFrblN2QOoc1mGVVyS5SvjcQsg2aDZrqfHnrb0iTTiJIlgg +PJvxSmTxEspL+STelnGuWKgEIiAGR9S+snHAGqkaYf4= +""" + +CLIENT_DATA_JSON_2 = b""" +eyJ0eXBlIjoid2ViYXV0aG4uY3JlYXRlIiwiY2hhbGxlbmdlIjoieXhIV0ctb3VvYThNTEdTeE9KSmhNME50aUg1dWJLM0JQZngtNnVFM1FrVSIsIm9yaWdp +biI6Imh0dHBzOi8vaHRtbC5lZHVpZC5kb2NrZXIiLCJjcm9zc09yaWdpbiI6ZmFsc2V9 +""" CREDENTIAL_ID_2 = ( "7bad3b59fa16e9e8840e2fd15c026a3c9a7d2877fd7d97c25a3b3158d1a9cba1e14b7c9913915" @@ -689,7 +682,7 @@ def test_authenticator_information_backdoor(self) -> None: with self.app.test_request_context(): res = is_authenticator_mfa_approved(authenticator_info=authenticator_info) - assert res is False + assert res is True def test_approved_security_keys(self) -> None: response = self.browser.get("/webauthn/approved-security-keys") From 7b58e707715cdb64f2ba6b59b772fe85c6482f1f Mon Sep 17 00:00:00 2001 From: Johan Lundberg Date: Thu, 23 Jan 2025 10:40:03 +0100 Subject: [PATCH 7/8] make reformat --- src/eduid/scimapi/testing.py | 12 ++--- src/eduid/scimapi/tests/test_scimuser.py | 12 ++--- src/eduid/webapp/common/api/testing.py | 18 +++---- src/eduid/webapp/common/proofing/testing.py | 54 +++++++++---------- src/eduid/webapp/freja_eid/tests/test_app.py | 6 +-- src/eduid/webapp/idp/tests/test_SSO.py | 6 +-- src/eduid/webapp/idp/tests/test_api.py | 12 ++--- .../webapp/personal_data/tests/test_app.py | 6 +-- src/eduid/webapp/security/helpers.py | 3 -- src/eduid/webapp/security/views/webauthn.py | 2 +- .../webapp/security/webauthn_proofing.py | 3 +- src/eduid/webapp/signup/tests/test_app.py | 6 +-- .../am/tests/test_proofing_fetchers.py | 12 ++--- 13 files changed, 75 insertions(+), 77 deletions(-) diff --git a/src/eduid/scimapi/testing.py b/src/eduid/scimapi/testing.py index 7f5ac69cc..b1df50655 100644 --- a/src/eduid/scimapi/testing.py +++ b/src/eduid/scimapi/testing.py @@ -288,9 +288,9 @@ def _assertName(db_name: ScimApiName, response_name: dict[str, str]) -> None: ] db_name_dict = asdict(db_name) for first, second in name_map: - assert db_name_dict.get(first) == response_name.get(second), ( - f"{first}:{db_name_dict.get(first)} != {second}:{response_name.get(second)}" - ) + assert db_name_dict.get(first) == response_name.get( + second + ), f"{first}:{db_name_dict.get(first)} != {second}:{response_name.get(second)}" @staticmethod def _assertResponse(response: Response, status_code: int = 200) -> None: @@ -300,6 +300,6 @@ def _assertResponse(response: Response, status_code: int = 200) -> None: _detail = response.json().get("detail", "No error detail in parsed_response") except JSONDecodeError: pass - assert response.status_code == status_code, ( - f"Response status was not {status_code} ({response.status_code}), {_detail}" - ) + assert ( + response.status_code == status_code + ), f"Response status was not {status_code} ({response.status_code}), {_detail}" diff --git a/src/eduid/scimapi/tests/test_scimuser.py b/src/eduid/scimapi/tests/test_scimuser.py index a1f4d9bce..38f603d14 100644 --- a/src/eduid/scimapi/tests/test_scimuser.py +++ b/src/eduid/scimapi/tests/test_scimuser.py @@ -204,9 +204,9 @@ def _assertUserUpdateSuccess(self, req: Mapping, response: Response, user: ScimA self._assertScimResponseProperties(response, resource=user, expected_schemas=expected_schemas) # Validate user update specifics - assert user.external_id == response.json().get("externalId"), ( - 'user.externalId != parsed_response.json().get("externalId")' - ) + assert user.external_id == response.json().get( + "externalId" + ), 'user.externalId != parsed_response.json().get("externalId")' self._assertName(user.name, response.json().get("name")) _expected_emails = filter_none(normalised_data([email.to_dict() for email in user.emails])) _obtained_emails = filter_none(normalised_data(response.json().get("emails", []))) @@ -214,9 +214,9 @@ def _assertUserUpdateSuccess(self, req: Mapping, response: Response, user: ScimA _expected_phones = filter_none(normalised_data([number.to_dict() for number in user.phone_numbers])) _obtained_phones = filter_none(normalised_data(response.json().get("phoneNumbers", []))) assert _obtained_phones == _expected_phones, 'parsed_response.json().get("phoneNumbers") != user.phone_numbers' - assert user.preferred_language == response.json().get("preferredLanguage"), ( - 'user.preferred_language != parsed_response.json().get("preferredLanguage")' - ) + assert user.preferred_language == response.json().get( + "preferredLanguage" + ), 'user.preferred_language != parsed_response.json().get("preferredLanguage")' # If the request has NUTID profiles, ensure they are present in the parsed_response if SCIMSchema.NUTID_USER_V1.value in req: diff --git a/src/eduid/webapp/common/api/testing.py b/src/eduid/webapp/common/api/testing.py index d8539f9b9..4658a0a6e 100644 --- a/src/eduid/webapp/common/api/testing.py +++ b/src/eduid/webapp/common/api/testing.py @@ -517,9 +517,9 @@ def _assure_not_in_dict(d: Mapping[str, Any], unwanted_key: str) -> None: if message is not None: assert "message" in _json["payload"], 'JSON payload has no "message" element' _message_value = _json["payload"]["message"] - assert message.value == _message_value, ( - f"Wrong message returned. expected: {message.value}, actual: {_message_value}" - ) + assert ( + message.value == _message_value + ), f"Wrong message returned. expected: {message.value}, actual: {_message_value}" if error is not None: assert _json["error"] is True, "The Flux response was supposed to have error=True" assert "error" in _json["payload"], 'JSON payload has no "error" element' @@ -527,9 +527,9 @@ def _assure_not_in_dict(d: Mapping[str, Any], unwanted_key: str) -> None: assert error == _error, f"Wrong error returned. expected: {error}, actual: {_error}" if payload is not None: for k, v in payload.items(): - assert k in _json["payload"], ( - f"The Flux response payload {_json['payload']} does not contain {repr(k)}" - ) + assert ( + k in _json["payload"] + ), f"The Flux response payload {_json['payload']} does not contain {repr(k)}" assert v == _json["payload"][k], ( f"The Flux response payload item {repr(k)} should be {repr(v)} " f"but is {repr(_json['payload'][k])}" @@ -540,9 +540,9 @@ def _assure_not_in_dict(d: Mapping[str, Any], unwanted_key: str) -> None: if meta is not None: for k, v in meta.items(): assert k in _json["meta"], f"The Flux response meta does not contain {repr(k)}" - assert v == _json["meta"][k], ( - f"The Flux response meta item {repr(k)} should be {repr(v)} but is {repr(_json['meta'][k])}" - ) + assert ( + v == _json["meta"][k] + ), f"The Flux response meta item {repr(k)} should be {repr(v)} but is {repr(_json['meta'][k])}" except (AssertionError, KeyError): if response.json: diff --git a/src/eduid/webapp/common/proofing/testing.py b/src/eduid/webapp/common/proofing/testing.py index 67c159b2c..e64bd0c66 100644 --- a/src/eduid/webapp/common/proofing/testing.py +++ b/src/eduid/webapp/common/proofing/testing.py @@ -77,49 +77,49 @@ def _verify_user_parameters( user_mfa_tokens = user.credentials.filter(FidoCredential) # Check token status - assert len(user_mfa_tokens) == num_mfa_tokens, ( - f"Unexpected number of FidoCredentials on user. {len(user_mfa_tokens)}, expected {num_mfa_tokens}" - ) + assert ( + len(user_mfa_tokens) == num_mfa_tokens + ), f"Unexpected number of FidoCredentials on user. {len(user_mfa_tokens)}, expected {num_mfa_tokens}" if user_mfa_tokens: - assert user_mfa_tokens[0].is_verified == token_verified, ( - f"User token was expected to be verified={token_verified}" - ) + assert ( + user_mfa_tokens[0].is_verified == token_verified + ), f"User token was expected to be verified={token_verified}" _log = getattr(self.app, "proofing_log") assert isinstance(_log, ProofingLog) - assert _log.db_count() == num_proofings, ( - f"Unexpected number of proofings in db. {_log.db_count()}, expected {num_proofings}" - ) + assert ( + _log.db_count() == num_proofings + ), f"Unexpected number of proofings in db. {_log.db_count()}, expected {num_proofings}" if identity is not None: # Check parameters of a specific nin user_identity = user.identities.find(identity.identity_type) if not identity_present: - assert (user_identity is None or user_identity.unique_value != identity.unique_value) is True, ( - f"identity {identity} not expected to be present on user" - ) + assert ( + user_identity is None or user_identity.unique_value != identity.unique_value + ) is True, f"identity {identity} not expected to be present on user" return None assert user_identity is not None, f"identity {identity} not present on user" - assert user_identity.unique_value == identity.unique_value, ( - "user_identity.unique_value != identity.unique_value" - ) - assert user_identity.is_verified == identity_verified, ( - f"{user_identity} was expected to be verified={identity_verified}" - ) + assert ( + user_identity.unique_value == identity.unique_value + ), "user_identity.unique_value != identity.unique_value" + assert ( + user_identity.is_verified == identity_verified + ), f"{user_identity} was expected to be verified={identity_verified}" if proofing_method is not None: - assert user_identity.proofing_method == proofing_method, ( - f"{user_identity.proofing_method} != {proofing_method}" - ) + assert ( + user_identity.proofing_method == proofing_method + ), f"{user_identity.proofing_method} != {proofing_method}" if proofing_version is not None: - assert user_identity.proofing_version == proofing_version, ( - f"{user_identity.proofing_version} != {proofing_version}" - ) + assert ( + user_identity.proofing_version == proofing_version + ), f"{user_identity.proofing_version} != {proofing_version}" if locked_identity is not None: # Check parameters of a specific locked nin user_locked_identity = user.locked_identity.find(locked_identity.identity_type) assert user_locked_identity is not None, f"locked identity {locked_identity} not present" - assert user_locked_identity.unique_value == locked_identity.unique_value, ( - f"locked identity {user_locked_identity.unique_value} not matching {locked_identity.unique_value}" - ) + assert ( + user_locked_identity.unique_value == locked_identity.unique_value + ), f"locked identity {user_locked_identity.unique_value} not matching {locked_identity.unique_value}" diff --git a/src/eduid/webapp/freja_eid/tests/test_app.py b/src/eduid/webapp/freja_eid/tests/test_app.py index a7fd4da1c..62e8a5381 100644 --- a/src/eduid/webapp/freja_eid/tests/test_app.py +++ b/src/eduid/webapp/freja_eid/tests/test_app.py @@ -342,9 +342,9 @@ def test_verify_identity_request(self) -> None: assert query["response_type"] == ["code"] assert query["client_id"] == ["test_client_id"] assert query["redirect_uri"] == ["http://test.localhost/authn-callback"] - assert query["scope"] == [" ".join(self.app.conf.freja_eid_client.scopes)], ( - f"{query['scope']} != {[' '.join(self.app.conf.freja_eid_client.scopes)]}" - ) + assert query["scope"] == [ + " ".join(self.app.conf.freja_eid_client.scopes) + ], f"{query['scope']} != {[' '.join(self.app.conf.freja_eid_client.scopes)]}" assert query["code_challenge_method"] == ["S256"] @patch("eduid.webapp.common.api.helpers.get_reference_nin_from_navet_data") diff --git a/src/eduid/webapp/idp/tests/test_SSO.py b/src/eduid/webapp/idp/tests/test_SSO.py index b6fd3e668..5d7cc49c5 100644 --- a/src/eduid/webapp/idp/tests/test_SSO.py +++ b/src/eduid/webapp/idp/tests/test_SSO.py @@ -259,9 +259,9 @@ def _check_login_response_authn( assert authn_result.message == message, f"Message: {authn_result.message}, Expected: {message}" if expect_success: assert authn_result.authn_info - assert authn_result.authn_info.class_ref == accr, ( - f"class_ref: {authn_result.authn_info.class_ref}, Expected: {accr}" - ) + assert ( + authn_result.authn_info.class_ref == accr + ), f"class_ref: {authn_result.authn_info.class_ref}, Expected: {accr}" if assurance_profile is not None: assert authn_result.authn_info.authn_attributes["eduPersonAssurance"] == [ item.value for item in assurance_profile diff --git a/src/eduid/webapp/idp/tests/test_api.py b/src/eduid/webapp/idp/tests/test_api.py index 03507ff87..359269e1c 100644 --- a/src/eduid/webapp/idp/tests/test_api.py +++ b/src/eduid/webapp/idp/tests/test_api.py @@ -376,9 +376,9 @@ def _call_mfa( response = client.post(target, json=data) payload = self.get_response_payload(response=response) - assert payload.get("webauthn_options") == mock_stv.return_value.webauthn_options, ( - f"webauthn_options: {payload.get('webauthn_options')}, Expected: {mock_stv.return_value.webauthn_options}" - ) + assert ( + payload.get("webauthn_options") == mock_stv.return_value.webauthn_options + ), f"webauthn_options: {payload.get('webauthn_options')}, Expected: {mock_stv.return_value.webauthn_options}" assert not payload.get("finished"), "Expected finished=False" logger.debug(f"MFA endpoint returned (challenge):\n{json.dumps(response.json, indent=4)}") @@ -599,9 +599,9 @@ def _check_login_result( if sso_cookie_val is True: assert result.sso_cookie_val is not None, "Expected sso_cookie_val but it is None" else: - assert result.sso_cookie_val == sso_cookie_val, ( - f"sso_cookie_val: {result.sso_cookie_val}, expected: {sso_cookie_val}" - ) + assert ( + result.sso_cookie_val == sso_cookie_val + ), f"sso_cookie_val: {result.sso_cookie_val}, expected: {sso_cookie_val}" if finish_result is not None: assert result.finished_result is not None, "Expected finished_result but it is None" diff --git a/src/eduid/webapp/personal_data/tests/test_app.py b/src/eduid/webapp/personal_data/tests/test_app.py index 4befd46c0..c7862a28b 100644 --- a/src/eduid/webapp/personal_data/tests/test_app.py +++ b/src/eduid/webapp/personal_data/tests/test_app.py @@ -560,6 +560,6 @@ def test_is_valid_chosen_given_name() -> None: ("Sverker Jr", "Jr", True), ] for param in params: - assert is_valid_chosen_given_name(param[0], param[1]) is param[2], ( - f"{param[0]}, {param[1]} did not return {param[2]}" - ) + assert ( + is_valid_chosen_given_name(param[0], param[1]) is param[2] + ), f"{param[0]}, {param[1]} did not return {param[2]}" diff --git a/src/eduid/webapp/security/helpers.py b/src/eduid/webapp/security/helpers.py index b80902172..13041150b 100644 --- a/src/eduid/webapp/security/helpers.py +++ b/src/eduid/webapp/security/helpers.py @@ -4,8 +4,6 @@ from functools import cache from typing import Any -from fido_mds.models.webauthn import AttestationFormat - from eduid.common.config.base import EduidEnvironment from eduid.common.misc.timeutil import utc_now from eduid.common.rpc.msg_relay import FullPostalAddress, NavetData @@ -21,7 +19,6 @@ from eduid.webapp.common.api.messages import TranslatableMsg from eduid.webapp.common.api.translation import get_user_locale from eduid.webapp.security.app import current_security_app as current_app -from eduid.webapp.security.webauthn_proofing import AuthenticatorInformation, is_authenticator_mfa_approved __author__ = "lundberg" diff --git a/src/eduid/webapp/security/views/webauthn.py b/src/eduid/webapp/security/views/webauthn.py index 3757e32bc..da4c99fb0 100644 --- a/src/eduid/webapp/security/views/webauthn.py +++ b/src/eduid/webapp/security/views/webauthn.py @@ -12,7 +12,7 @@ CollectedClientData, PublicKeyCredentialUserEntity, ) -from fido_mds.exceptions import AttestationVerificationError, MetadataValidationError +from fido_mds.exceptions import AttestationVerificationError from flask import Blueprint from eduid.common.config.base import FrontendAction diff --git a/src/eduid/webapp/security/webauthn_proofing.py b/src/eduid/webapp/security/webauthn_proofing.py index 694301763..d6a7b0283 100644 --- a/src/eduid/webapp/security/webauthn_proofing.py +++ b/src/eduid/webapp/security/webauthn_proofing.py @@ -162,7 +162,8 @@ def is_authenticator_mfa_approved(authenticator_info: AuthenticatorInformation) return True current_app.logger.info( - f"AttestationFormat {authenticator_info.attestation_format}: user NOT verified - authenticator is not mfa capable" + f"AttestationFormat {authenticator_info.attestation_format}: user NOT verified - authenticator is not mfa " + f"capable" ) return False diff --git a/src/eduid/webapp/signup/tests/test_app.py b/src/eduid/webapp/signup/tests/test_app.py index c0c19afaa..44f46e01a 100644 --- a/src/eduid/webapp/signup/tests/test_app.py +++ b/src/eduid/webapp/signup/tests/test_app.py @@ -744,9 +744,9 @@ def _accept_invite( assert response.json is not None, "response.json unexpected None" if expect_success: - assert response.json.get("error", False) is False, ( - f"expect_success {expect_success} but got error={response.json.get('error')}" - ) + assert ( + response.json.get("error", False) is False + ), f"expect_success {expect_success} but got error={response.json.get('error')}" if not expected_payload: payload = self.get_response_payload(response) assert payload["state"]["tou"]["completed"] is False diff --git a/src/eduid/workers/am/tests/test_proofing_fetchers.py b/src/eduid/workers/am/tests/test_proofing_fetchers.py index 126f91511..6c83dd976 100644 --- a/src/eduid/workers/am/tests/test_proofing_fetchers.py +++ b/src/eduid/workers/am/tests/test_proofing_fetchers.py @@ -94,16 +94,16 @@ def test_append_attributes_letter_proofing_data(self) -> None: "$unset": {"nins": None}, } - assert normalised_data(fetched) == expected, ( - f"Fetched letter proofing data has unexpected data: {DeepDiff(fetched, expected)}" - ) + assert ( + normalised_data(fetched) == expected + ), f"Fetched letter proofing data has unexpected data: {DeepDiff(fetched, expected)}" fetched2 = self.fetcher.fetch_attrs(proofing_user.user_id) # Don't repeat the letter_proofing_data - assert normalised_data(fetched2) == expected, ( - f"Fetched (2nd time) letter proofing data has unexpected data: {DeepDiff(fetched, expected)}" - ) + assert ( + normalised_data(fetched2) == expected + ), f"Fetched (2nd time) letter proofing data has unexpected data: {DeepDiff(fetched, expected)}" # Adding a new letter_proofing_data self.user_data["letter_proofing_data"].append( From a0c7a61a74e391d53e306d79a7157a6ec50ca61d Mon Sep 17 00:00:00 2001 From: Johan Lundberg Date: Thu, 23 Jan 2025 15:24:09 +0100 Subject: [PATCH 8/8] change attestation format to None for security keys that fails metadata validation --- src/eduid/webapp/security/webauthn_proofing.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/eduid/webapp/security/webauthn_proofing.py b/src/eduid/webapp/security/webauthn_proofing.py index d6a7b0283..ab19f7b07 100644 --- a/src/eduid/webapp/security/webauthn_proofing.py +++ b/src/eduid/webapp/security/webauthn_proofing.py @@ -88,7 +88,7 @@ def get_authenticator_information(attestation: str, client_data: str) -> Authent # Continue even if the security key _should_ be found and validated with metadata as we have seen security keys # in the wild that fails return AuthenticatorInformation( - attestation_format=att.fmt, + attestation_format=AttestationFormat.NONE, authenticator_id=authenticator_id, user_present=user_present, user_verified=user_verified,