From b9120af869d139df0f8d364336bbc33f00833a3e Mon Sep 17 00:00:00 2001 From: Ben Banfield-Zanin Date: Wed, 21 Oct 2020 15:56:09 +0100 Subject: [PATCH 1/9] saml: allow specification of the IdP entityid I'm working with a remote metadata endpoint that contains multiple IDPSSODescriptor that support Saml2, so entityid needs to be passed into Saml2Client.prepare_for_authenticate or an error will be thrown saying "Too many IdPs to choose from". My reading of https://pysaml2.readthedocs.io/en/latest/howto/config.html suggests that service.sp.idp is the setting for this but it doesn't seem to do anything other than populate the `idp` attribute in saml2_sp_config. Also https://pysaml2.readthedocs.io/en/latest/howto/config.html#idp suggests this setting should be a list, but Saml2Client._sso_location wants a single entity id. In summary this seems to work, but I'm not convinced I'm doing it right. --- synapse/handlers/saml_handler.py | 2 ++ 1 file changed, 2 insertions(+) diff --git a/synapse/handlers/saml_handler.py b/synapse/handlers/saml_handler.py index 285c481a9604..6dc6ec3a63f5 100644 --- a/synapse/handlers/saml_handler.py +++ b/synapse/handlers/saml_handler.py @@ -61,6 +61,7 @@ class SamlHandler: def __init__(self, hs: "synapse.server.HomeServer"): self.hs = hs self._saml_client = Saml2Client(hs.config.saml2_sp_config) + self._saml_idp_entityid = hs.config.saml2_sp_config.getattr('idp') self._auth = hs.get_auth() self._auth_handler = hs.get_auth_handler() self._registration_handler = hs.get_registration_handler() @@ -124,6 +125,7 @@ def handle_redirect_request( URL to redirect to """ reqid, info = self._saml_client.prepare_for_authenticate( + entityid=self._saml_idp_entityid, relay_state=client_redirect_url ) From ec15ee1793fcebd06f71c83fcc77d80e2dd6e551 Mon Sep 17 00:00:00 2001 From: Ben Banfield-Zanin Date: Thu, 22 Oct 2020 16:51:56 +0100 Subject: [PATCH 2/9] Correct code style --- synapse/handlers/saml_handler.py | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/synapse/handlers/saml_handler.py b/synapse/handlers/saml_handler.py index 6dc6ec3a63f5..967081f29d5e 100644 --- a/synapse/handlers/saml_handler.py +++ b/synapse/handlers/saml_handler.py @@ -125,8 +125,7 @@ def handle_redirect_request( URL to redirect to """ reqid, info = self._saml_client.prepare_for_authenticate( - entityid=self._saml_idp_entityid, - relay_state=client_redirect_url + entityid=self._saml_idp_entityid, relay_state=client_redirect_url ) # Since SAML sessions timeout it is useful to log when they were created. From 6c75abe2e624d4cb564da8fff29425b5af0b70ec Mon Sep 17 00:00:00 2001 From: Ben Banfield-Zanin Date: Thu, 22 Oct 2020 16:52:04 +0100 Subject: [PATCH 3/9] Add Newsfile --- changelog.d/8630.feature | 1 + 1 file changed, 1 insertion(+) create mode 100644 changelog.d/8630.feature diff --git a/changelog.d/8630.feature b/changelog.d/8630.feature new file mode 100644 index 000000000000..7b4d0436351e --- /dev/null +++ b/changelog.d/8630.feature @@ -0,0 +1 @@ +Allow specification of the SAML IdP if the metadata returns multiple IdPs From d2fc6fbdc1abe2a4ec01ef03535a33df870067d2 Mon Sep 17 00:00:00 2001 From: Ben Banfield-Zanin Date: Thu, 22 Oct 2020 16:56:15 +0100 Subject: [PATCH 4/9] Correct the Newsfile --- changelog.d/8630.feature | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/changelog.d/8630.feature b/changelog.d/8630.feature index 7b4d0436351e..706051f1316e 100644 --- a/changelog.d/8630.feature +++ b/changelog.d/8630.feature @@ -1 +1 @@ -Allow specification of the SAML IdP if the metadata returns multiple IdPs +Allow specification of the SAML IdP if the metadata returns multiple IdPs. From 213cf34271d34a54d393a32f6ed0e7d67b856061 Mon Sep 17 00:00:00 2001 From: Ben Banfield-Zanin Date: Fri, 23 Oct 2020 11:36:38 +0100 Subject: [PATCH 5/9] Fix more codestyle that I missed --- synapse/handlers/saml_handler.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/synapse/handlers/saml_handler.py b/synapse/handlers/saml_handler.py index 967081f29d5e..f9c310c883b3 100644 --- a/synapse/handlers/saml_handler.py +++ b/synapse/handlers/saml_handler.py @@ -61,7 +61,7 @@ class SamlHandler: def __init__(self, hs: "synapse.server.HomeServer"): self.hs = hs self._saml_client = Saml2Client(hs.config.saml2_sp_config) - self._saml_idp_entityid = hs.config.saml2_sp_config.getattr('idp') + self._saml_idp_entityid = hs.config.saml2_sp_config.getattr("idp") self._auth = hs.get_auth() self._auth_handler = hs.get_auth_handler() self._registration_handler = hs.get_registration_handler() From 705806a560b8cf1e900c06590405e4739ccf90af Mon Sep 17 00:00:00 2001 From: Ben Banfield-Zanin Date: Mon, 2 Nov 2020 13:17:39 +0000 Subject: [PATCH 6/9] Specify the IdP entity id outside of sp_config --- synapse/config/saml2_config.py | 10 ++++++++++ synapse/handlers/saml_handler.py | 2 +- 2 files changed, 11 insertions(+), 1 deletion(-) diff --git a/synapse/config/saml2_config.py b/synapse/config/saml2_config.py index 99aa8b3bf123..37d53fdee308 100644 --- a/synapse/config/saml2_config.py +++ b/synapse/config/saml2_config.py @@ -90,6 +90,8 @@ def read_config(self, config, **kwargs): "grandfathered_mxid_source_attribute", "uid" ) + self.saml2_idp_entityid = saml2_config.get("idp_entityid", None) + # user_mapping_provider may be None if the key is present but has no value ump_dict = saml2_config.get("user_mapping_provider") or {} @@ -350,6 +352,14 @@ def generate_config_section(self, config_dir_path, server_name, **kwargs): # value: "staff" # - attribute: department # value: "sales" + + # Most metadata XML only contains a single IdP entity. However if the + # metadata XML contains multiple IdPs Synapse needs to know which IdP to + # redirect users to. `idp_entityid` can be populated with the entity of + # the IdP that should be used. For most deployments, this parameter should + # be omitted. + # + #idp_entityid: 'https://our_idp/entityid' """ % { "config_dir_path": config_dir_path } diff --git a/synapse/handlers/saml_handler.py b/synapse/handlers/saml_handler.py index f9c310c883b3..9f93502fbb39 100644 --- a/synapse/handlers/saml_handler.py +++ b/synapse/handlers/saml_handler.py @@ -61,7 +61,7 @@ class SamlHandler: def __init__(self, hs: "synapse.server.HomeServer"): self.hs = hs self._saml_client = Saml2Client(hs.config.saml2_sp_config) - self._saml_idp_entityid = hs.config.saml2_sp_config.getattr("idp") + self._saml_idp_entityid = hs.config.saml2_idp_entityid self._auth = hs.get_auth() self._auth_handler = hs.get_auth_handler() self._registration_handler = hs.get_registration_handler() From bc45f23102732ddd4abb287d701c62728defa4d4 Mon Sep 17 00:00:00 2001 From: Ben Banfield-Zanin Date: Mon, 2 Nov 2020 13:28:14 +0000 Subject: [PATCH 7/9] Update sample config --- docs/sample_config.yaml | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/docs/sample_config.yaml b/docs/sample_config.yaml index 061226ea6fc1..e071646d8f97 100644 --- a/docs/sample_config.yaml +++ b/docs/sample_config.yaml @@ -1640,6 +1640,14 @@ saml2_config: # - attribute: department # value: "sales" + # Most metadata XML only contains a single IdP entity. However if the + # metadata XML contains multiple IdPs Synapse needs to know which IdP to + # redirect users to. `idp_entityid` can be populated with the entity of + # the IdP that should be used. For most deployments, this parameter should + # be omitted. + # + #idp_entityid: 'https://our_idp/entityid' + # OpenID Connect integration. The following settings can be used to make Synapse # use an OpenID Connect Provider for authentication, instead of its internal From 1510218c2945afc27c7e880c2f22cab341e9aa0d Mon Sep 17 00:00:00 2001 From: Ben Banfield-Zanin Date: Mon, 2 Nov 2020 17:35:53 +0000 Subject: [PATCH 8/9] Update synapse/config/saml2_config.py Co-authored-by: Erik Johnston --- synapse/config/saml2_config.py | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/synapse/config/saml2_config.py b/synapse/config/saml2_config.py index 37d53fdee308..db3d67a4b711 100644 --- a/synapse/config/saml2_config.py +++ b/synapse/config/saml2_config.py @@ -353,11 +353,11 @@ def generate_config_section(self, config_dir_path, server_name, **kwargs): # - attribute: department # value: "sales" - # Most metadata XML only contains a single IdP entity. However if the - # metadata XML contains multiple IdPs Synapse needs to know which IdP to - # redirect users to. `idp_entityid` can be populated with the entity of - # the IdP that should be used. For most deployments, this parameter should - # be omitted. + # If the metadata XML contains multiple IdP entities then the `idp_entityid` + # option must be set to the entity to redirect users to. + # + # Most deployments only have a single IdP entity and so should omit this + # option. # #idp_entityid: 'https://our_idp/entityid' """ % { From 1d2994899d6330f7a8498b4ffdc1ab9b2726840e Mon Sep 17 00:00:00 2001 From: Erik Johnston Date: Mon, 2 Nov 2020 17:48:48 +0000 Subject: [PATCH 9/9] Update sample config --- docs/sample_config.yaml | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/docs/sample_config.yaml b/docs/sample_config.yaml index e071646d8f97..9975ddddfedc 100644 --- a/docs/sample_config.yaml +++ b/docs/sample_config.yaml @@ -1640,11 +1640,11 @@ saml2_config: # - attribute: department # value: "sales" - # Most metadata XML only contains a single IdP entity. However if the - # metadata XML contains multiple IdPs Synapse needs to know which IdP to - # redirect users to. `idp_entityid` can be populated with the entity of - # the IdP that should be used. For most deployments, this parameter should - # be omitted. + # If the metadata XML contains multiple IdP entities then the `idp_entityid` + # option must be set to the entity to redirect users to. + # + # Most deployments only have a single IdP entity and so should omit this + # option. # #idp_entityid: 'https://our_idp/entityid'