From 547399f72ca4d910687edc38299b5a9615db8362 Mon Sep 17 00:00:00 2001 From: Scott Koranda Date: Mon, 13 Apr 2020 13:39:11 -0500 Subject: [PATCH] Config to disable discovery initiated SAMLBackend flows The current SAMLBackend allows a flow to start with the disco_response() endpoint, i.e., a client passing in the entityID of the IdP to be used for authentication. In most deployments the flow will fail after the backend receives the SAMLResponse, parses it, and passes control to the frontend since the frontend does not know to which relying party to redirect the browser. But it is possible for some deployments to make a flow that starts with the disco_response() work so just preventing such flows is not acceptable. This commit enables configuring the SAMLBackend so that flows are not allowed to be started at disco_response(), and when so configured a flow that starts at disco_response() will raise an exception with a useful message rather than continuing on to a frontend and failing with UnknownError. --- .../backends/saml2_backend.yaml.example | 5 ++ src/satosa/backends/saml2.py | 23 +++++++- tests/satosa/backends/test_saml2.py | 55 +++++++++++++++++++ 3 files changed, 81 insertions(+), 2 deletions(-) diff --git a/example/plugins/backends/saml2_backend.yaml.example b/example/plugins/backends/saml2_backend.yaml.example index a71dfd0d4..0b536f324 100644 --- a/example/plugins/backends/saml2_backend.yaml.example +++ b/example/plugins/backends/saml2_backend.yaml.example @@ -59,3 +59,8 @@ config: name_id_format_allow_create: true # disco_srv must be defined if there is more than one IdP in the metadata specified above disco_srv: http://disco.example.com + + # Allow flows to start at the discovery response endpoint. + # Not recommend but allowed by default for backwards compatiblity. + # Default will change to false in a later release. + # allow_discovery_initiated: true diff --git a/src/satosa/backends/saml2.py b/src/satosa/backends/saml2.py index 13349ee37..128b815eb 100644 --- a/src/satosa/backends/saml2.py +++ b/src/satosa/backends/saml2.py @@ -23,7 +23,7 @@ from satosa.context import Context from satosa.internal import AuthenticationInformation from satosa.internal import InternalData -from satosa.exception import SATOSAAuthenticationError +from satosa.exception import SATOSAAuthenticationError, SATOSAStateError from satosa.response import SeeOther, Response from satosa.saml_util import make_saml_response from satosa.metadata_creation.description import ( @@ -75,7 +75,9 @@ class SAMLBackend(BackendModule, SAMLBaseModule): """ A saml2 backend module (acting as a SP). """ + KEY_ALLOW_DISCO_INIT_CONFIG = 'allow_discovery_initiated' KEY_DISCO_SRV = 'disco_srv' + KEY_SAML_DISCOVERY_INITIATED = 'saml_discovery_initiated' KEY_SAML_DISCOVERY_SERVICE_URL = 'saml_discovery_service_url' KEY_SAML_DISCOVERY_SERVICE_POLICY = 'saml_discovery_service_policy' KEY_SP_CONFIG = 'sp_config' @@ -168,6 +170,7 @@ def start_auth(self, context, internal_req): :type internal_req: satosa.internal.InternalData :rtype: satosa.response.Response """ + context.state[self.name] = {} entity_id = self.get_idp_entity_id(context) if entity_id is None: @@ -210,6 +213,10 @@ def disco_query(self, context): loc = self.sp.create_discovery_service_request( disco_url, self.sp.config.entityid, **args ) + + state_dict = context.state[self.name] + state_dict[SAMLBackend.KEY_SAML_DISCOVERY_INITIATED] = True + return SeeOther(loc) def construct_requested_authn_context(self, entity_id): @@ -363,12 +370,24 @@ def disco_response(self, context): """ info = context.request state = context.state + session_id = lu.get_session_id(state) + state_dict = state.get(self.name, {}) + disco_initiated = state_dict.get( + SAMLBackend.KEY_SAML_DISCOVERY_INITIATED, + False) + + if (not self.config.get(SAMLBackend.KEY_ALLOW_DISCO_INIT_CONFIG, True) + and not disco_initiated): + msg = "IdP discovery initiated flow not allowed" + logline = lu.LOG_FMT.format(id=session_id, message=msg) + logger.debug(logline, exc_info=True) + raise SATOSAStateError(state, msg) try: entity_id = info["entityID"] except KeyError as err: msg = "No IDP chosen for state" - logline = lu.LOG_FMT.format(id=lu.get_session_id(state), message=msg) + logline = lu.LOG_FMT.format(id=session_id, message=msg) logger.debug(logline, exc_info=True) raise SATOSAAuthenticationError(state, "No IDP chosen") from err diff --git a/tests/satosa/backends/test_saml2.py b/tests/satosa/backends/test_saml2.py index e5e2d905c..0ad5fc910 100644 --- a/tests/satosa/backends/test_saml2.py +++ b/tests/satosa/backends/test_saml2.py @@ -19,6 +19,7 @@ from satosa.backends.saml2 import SAMLBackend from satosa.context import Context +from satosa.exception import SATOSAStateError from satosa.internal import InternalData from tests.users import USERS from tests.util import FakeIdP, create_metadata_from_config_dict, FakeSP @@ -348,6 +349,60 @@ def test_get_metadata_desc_with_logo_without_lang(self, sp_conf, idp_conf): assert ui_info["description"] == expected_ui_info["description"] assert ui_info["logo"] == expected_ui_info["logo"] + def test_allow_discovery_initiated(self, sp_conf, context, idp_conf): + + # Test that with allow_discovery_initiated set to True can initiate + # flow at disco_response() and be directed to the correct IdP. + config = {"sp_config": sp_conf, + "disco_srv": DISCOSRV_URL, + "allow_discovery_initiated": True} + + samlbackend = SAMLBackend( + Mock(), + INTERNAL_ATTRIBUTES, + config, + "base_url", + "samlbackend", + ) + + context.request = {'entityID': idp_conf["entityid"]} + resp = samlbackend.disco_response(context) + assert_redirect_to_idp(resp, idp_conf) + + # Test that with allow_discovery_initiated set to False can not + # initiate flow at disco_response() and instead raises exception. + config = {"sp_config": sp_conf, + "disco_srv": DISCOSRV_URL, + SAMLBackend.KEY_ALLOW_DISCO_INIT_CONFIG: False} + + samlbackend = SAMLBackend( + Mock(), + INTERNAL_ATTRIBUTES, + config, + "base_url", + "samlbackend", + ) + + context.request = {'entityID': idp_conf["entityid"]} + + with pytest.raises(SATOSAStateError): + resp = samlbackend.disco_response(context) + + # Test that with allow_discovery_initiated set to False a flow + # that begins in the backend with start_auth() marks the state + # as having been initiated properly through the disco_query() + # method on the backend and that the flow succeeds in redirecting + # to the IdP selected during discovery. + samlbackend.start_auth(context, InternalData()) + name = samlbackend.name + key = SAMLBackend.KEY_SAML_DISCOVERY_INITIATED + initiated = context.state[name][key] + assert initiated is True + + context.request = {'entityID': idp_conf["entityid"]} + resp = samlbackend.disco_response(context) + assert_redirect_to_idp(resp, idp_conf) + class TestSAMLBackendRedirects: def test_default_redirect_to_discovery_service_if_using_mdq(