From 3ab4d4e46f1ad46e1dba8ed5942679820ec7bc29 Mon Sep 17 00:00:00 2001 From: Thijs Kinkhorst Date: Mon, 8 Aug 2022 16:07:38 +0200 Subject: [PATCH 1/3] Turn not verifying any signature into an error Not checking any signature is completely insecure and we'd want to go through great lenghts to avoid this. Just logging a warning does not suffice, since will read logs when it doesn't work, but it does. --- src/saml2/client_base.py | 8 +++----- tests/test_51_client.py | 12 ++++++++---- 2 files changed, 11 insertions(+), 9 deletions(-) diff --git a/src/saml2/client_base.py b/src/saml2/client_base.py index cf88dee9c..42e4bfc8f 100644 --- a/src/saml2/client_base.py +++ b/src/saml2/client_base.py @@ -10,7 +10,6 @@ import time import logging from typing import Mapping -from warnings import warn as _warn from saml2.entity import Entity @@ -194,15 +193,14 @@ def __init__(self, config=None, identity_cache=None, state_cache=None, self.want_assertions_or_response_signed, ] ): - warn_msg = ( + error_msg = ( "The SAML service provider accepts " "unsigned SAML Responses and Assertions. " "This configuration is insecure. " - "Consider setting want_assertions_signed, want_response_signed " + "Set at least one of want_assertions_signed, want_response_signed " "or want_assertions_or_response_signed configuration options." ) - logger.warning(warn_msg) - _warn(warn_msg) + raise SAMLError(error_msg) self.artifact2response = {} diff --git a/tests/test_51_client.py b/tests/test_51_client.py index a323de793..8d85ddcdf 100644 --- a/tests/test_51_client.py +++ b/tests/test_51_client.py @@ -1860,7 +1860,8 @@ def set_client_want(response, assertion, either): parse_authn_response(response) set_client_want(False, False, False) - parse_authn_response(response) + with raises(SAMLError): + parse_authn_response(response) # Response is not signed but assertion is signed. kwargs["sign_response"] = False @@ -1893,7 +1894,8 @@ def set_client_want(response, assertion, either): parse_authn_response(response) set_client_want(False, False, False) - parse_authn_response(response) + with raises(SAMLError): + parse_authn_response(response) # Both response and assertion are signed. kwargs["sign_response"] = True @@ -1922,7 +1924,8 @@ def set_client_want(response, assertion, either): parse_authn_response(response) set_client_want(False, False, False) - parse_authn_response(response) + with raises(SAMLError): + parse_authn_response(response) # Neither response nor assertion is signed. kwargs["sign_response"] = False @@ -1958,7 +1961,8 @@ def set_client_want(response, assertion, either): parse_authn_response(response) set_client_want(False, False, False) - parse_authn_response(response) + with raises(SAMLError): + parse_authn_response(response) class TestClientNonAsciiAva: From 8c2f880bee4d39cc3c5fa7a9b0be5dd5eaa70c86 Mon Sep 17 00:00:00 2001 From: Thijs Kinkhorst Date: Mon, 8 Aug 2022 16:38:25 +0200 Subject: [PATCH 2/3] Default want_assertions_or_response_signed to True Given that the other signature verification options want_assertions_signed and want_response_signed will overrule this setting, defaulting it to true does not impact any configuration that has one of those settings. It does however catch the situation where someone has disabled response signature checking. This prevents that user from landing on an error about an insecure config, and rather employs this reasonable fallback scenario. --- docs/howto/config.rst | 2 +- src/saml2/client_base.py | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/docs/howto/config.rst b/docs/howto/config.rst index c224cb2a1..eebbb5331 100644 --- a/docs/howto/config.rst +++ b/docs/howto/config.rst @@ -1174,7 +1174,7 @@ want_assertions_or_response_signed Indicates that *either* the Authentication Response *or* the assertions contained within the response to this SP must be signed. -Valid values are True or False. Default value is False. +Valid values are True or False. Default value is True. This configuration directive **does not** override ``want_response_signed`` or ``want_assertions_signed``. For example, if ``want_response_signed`` is True diff --git a/src/saml2/client_base.py b/src/saml2/client_base.py index 42e4bfc8f..d3b512e16 100644 --- a/src/saml2/client_base.py +++ b/src/saml2/client_base.py @@ -173,7 +173,7 @@ def __init__(self, config=None, identity_cache=None, state_cache=None, "authn_requests_signed": False, "want_assertions_signed": False, "want_response_signed": True, - "want_assertions_or_response_signed": False, + "want_assertions_or_response_signed": True, } for attr, val_default in attribute_defaults.items(): val_config = self.config.getattr(attr, "sp") From 22186976712723e6a48f1e430240a4f063e9cf2f Mon Sep 17 00:00:00 2001 From: Thijs Kinkhorst Date: Fri, 12 Aug 2022 09:51:09 +0200 Subject: [PATCH 3/3] Reordering the phrases to put emphasis on insecure.Update src/saml2/client_base.py Co-authored-by: Ivan Kanakarakis --- src/saml2/client_base.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/saml2/client_base.py b/src/saml2/client_base.py index d3b512e16..592059df7 100644 --- a/src/saml2/client_base.py +++ b/src/saml2/client_base.py @@ -194,9 +194,9 @@ def __init__(self, config=None, identity_cache=None, state_cache=None, ] ): error_msg = ( + "This configuration is insecure. " "The SAML service provider accepts " "unsigned SAML Responses and Assertions. " - "This configuration is insecure. " "Set at least one of want_assertions_signed, want_response_signed " "or want_assertions_or_response_signed configuration options." )