saml: allow specification of the IdP entityid

[benbz]

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 when generating a redirect URL:

  File "/home/ben/Work/local/synapse_deploy/deploy/env/lib/python3.8/site-packages/synapse/rest/client/v1/login.py", line 469, in get_sso_url
    return self._saml_handler.handle_redirect_request(client_redirect_url)
  File "/home/ben/Work/local/synapse_deploy/deploy/env/lib/python3.8/site-packages/synapse/handlers/saml_handler.py", line 128, in handle_redirect_request
    reqid, info = self._saml_client.prepare_for_authenticate(
  File "/home/ben/Work/local/synapse_deploy/deploy/env/lib/python3.8/site-packages/saml2/client.py", line 65, in prepare_for_authenticate
    self.prepare_for_negotiated_authenticate(
  File "/home/ben/Work/local/synapse_deploy/deploy/env/lib/python3.8/site-packages/saml2/client.py", line 115, in prepare_for_negotiated_authenticate
    destination = self._sso_location(entityid, binding)
  File "/home/ben/Work/local/synapse_deploy/deploy/env/lib/python3.8/site-packages/saml2/client_base.py", line 224, in _sso_location
    raise IdpUnspecified("Too many IdPs to choose from: %s" % eids)
saml2.client_base.IdpUnspecified: 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.
So I'm populating that configuration option and attempting to use the idp
attribute. saml2_sp_config.getattr('idp') returns None if the config option
isn't set, which is equivalent to what was there previously.

However https://pysaml2.readthedocs.io/en/latest/howto/config.html#idp
suggests this setting should be a list, but Saml2Client._sso_location
appears to require a single entity id. With this PR if I configure idp as a list, as the
docs suggest, i.e:

    service:
      sp:
        idp:
        - https://my-idp-domain/idp/shibboleth

then when attempting to get the redirect URL I get the following:

  File "/home/ben/Work/local/synapse_deploy/deploy/env/lib/python3.8/site-packages/synapse/rest/client/v1/login.py", line 469, in get_sso_url
    return self._saml_handler.handle_redirect_request(client_redirect_url)
  File "/home/ben/Work/local/synapse_deploy/deploy/env/lib/python3.8/site-packages/synapse/handlers/saml_handler.py", line 127, in handle_redirect_request
    reqid, info = self._saml_client.prepare_for_authenticate(
  File "/home/ben/Work/local/synapse_deploy/deploy/env/lib/python3.8/site-packages/saml2/client.py", line 65, in prepare_for_authenticate
    self.prepare_for_negotiated_authenticate(
  File "/home/ben/Work/local/synapse_deploy/deploy/env/lib/python3.8/site-packages/saml2/client.py", line 115, in prepare_for_negotiated_authenticate
    destination = self._sso_location(entityid, binding)
  File "/home/ben/Work/local/synapse_deploy/deploy/env/lib/python3.8/site-packages/saml2/client_base.py", line 213, in _sso_location
    srvs = self.metadata.single_sign_on_service(entityid, binding)
  File "/home/ben/Work/local/synapse_deploy/deploy/env/lib/python3.8/site-packages/saml2/mdstore.py", line 1126, in single_sign_on_service
    return self.service(entity_id, "idpsso_descriptor",
  File "/home/ben/Work/local/synapse_deploy/deploy/env/lib/python3.8/site-packages/saml2/mdstore.py", line 1073, in service
    srvs = _md.service(entity_id, typ, service, binding)
  File "/home/ben/Work/local/synapse_deploy/deploy/env/lib/python3.8/site-packages/saml2/mdstore.py", line 596, in service
    for t in self[entity_id][typ]:
  File "/home/ben/Work/local/synapse_deploy/deploy/env/lib/python3.8/site-packages/saml2/mdstore.py", line 497, in __getitem__
    return self.entity[item]
TypeError: unhashable type: 'list'

If using this PR, I configure idp as a string, i.e:

    service:
      sp:
        idp: https://my-idp-domain/idp/shibboleth

then I get a valid redirect URL back.

I could configure service.sp.idp as a list and update this PR to only
use the first entry in the Saml2Client.prepare_for_authenticate call
but what's the point of accepting a list then?

In summary this seems to work, but I'm not convinced I'm doing it right.

Changelog entry not added yet until we're happy that a string is fine
rather than a list.

cc @clokep given we were discussing this yesterday.

Pull Request Checklist

• Pull request is based on the develop branch
• Pull request includes a changelog file. The entry should:
  • Be a short description of your change which makes sense to users. "Fixed a bug that prevented receiving messages from other servers." instead of "Moved X method from EventStore to EventWorkerStore.".
  • Use markdown where necessary, mostly for code blocks.
  • End with either a period (.) or an exclamation mark (!).
  • Start with a capital letter.
• Pull request includes a sign off
• Code style is correct (run the linters)

[clokep]

I'm setting review on this so we remember to come back to it!

[clokep]

My reading of 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. So I'm populating that configuration option and attempting to use the idp attribute. saml2_sp_config.getattr('idp') returns None if the config option isn't set, which is equivalent to what was there previously.

Is there a particular section of this document that makes you think the service.sp.idp parameter is meant to be used for redirects? Was it more than just the idp docs:

Defines the set of IdPs that this SP is allowed to use; if unset, all listed IdPs may be used. If set, then the value is expected to be a list with entity identifiers for the allowed IdPs.

Looking through the pyaml2 code this configuration setting seems to be completely unused. The only examples of calling prepare_for_authenticate (or prepare_for_negotiated_authenticate) seem to be in the tests which consistently call the API by providing an entity ID as a string. Those eventually call into Base._sso_location which has comments describing the following:

1. If an entity ID is provided, ensure that it exists in the metadata provided. If it does not exist, raise an exception. If it exists, return it.
2. If no entity ID is provided, but it is ambiguous which entity to use (i.e. there is more than one entity in the metadata provided), raise an exception. (This is the "Too many IdPs to choose from" exception we're seeing.)
3. If no entity ID is provided and there is only a single entity in the metadata, return it.

In summary this seems to work, but I'm not convinced I'm doing it right.

I'm pretty confident the change to call prepare_for_authenticate with a given entity ID is reasonable, I'm not sure I agree that pulling it out from service.sp.idp makes sense. In particular I'm nervous about having people set this to a string if pysaml2 starts using this field for some reason. It might make sense to use a completely separate setting. What do you think?

[benbz]

Is there a particular section of this document that makes you think the service.sp.idp parameter is meant to be used for redirects? Was it more than just the idp docs:

Defines the set of IdPs that this SP is allowed to use; if unset, all listed IdPs may be used. If set, then the value is expected to be a list with entity identifiers for the allowed IdPs.

It was just this quoted section above that made me think it should be this setting. But that was before I realised it wasn't being used.

I'm pretty confident the change to call prepare_for_authenticate with a given entity ID is reasonable, I'm not sure I agree that pulling it out from service.sp.idp makes sense. In particular I'm nervous about having people set this to a string if pysaml2 starts using this field for some reason. It might make sense to use a completely separate setting. What do you think?

Options I see:

• either carry on as now or use a list for service.sp.idp and extract the 1st value only but either way, as you say, pysaml2 could start using it in a way we didn't expect
• use a parameter under saml2_config but not under sp_config but that feels somewhat wrong given everything else related is under sp_config
• pick a parameter name under sp_config that isn't used by pysaml2 at present and attempt to find a way to use that here but there's always the risk that then pysaml2 will start using that parameter name in the future. This may or may not even be possible with how pysaml2 does its config, would need to test

I don't have strong feelings about those options other than that they all appear to have risks.

[clokep]

I filed IdentityPython/pysaml2#739 upstream about the option being unused.

@richvdh and I discussed this and we think the best option is 2:

use a parameter under saml2_config but not under sp_config

I think this will give us the greatest flexibility in the future.

[benbz]

Ack, thanks for digging into this and for the steer. I'll update the PR early next week as I'm off for the rest of the week after today

[clokep]

Great! I'm going to mark this as request changes to get it out of the queue. 👍

[erikjohnston]

@clokep sounds like you were happy with the concept? If so then this PR looks good, otherwise I need to spend a bit of time getting up to speed.

[erikjohnston]

(You'll need to update the sample config too if you're not already doing that)

[benbz]

(You'll need to update the sample config too if you're not already doing that)

Yup, unfortunately doing this from mobile so will have to do it tomorrow

[erikjohnston]

Ah, then I can do that for you so we can let the tests chuuuuuuuuurn

[clokep]

@clokep sounds like you were happy with the concept? If so then this PR looks good, otherwise I need to spend a bit of time getting up to speed.

Yep. I think the code changes look fine. The sample config comment could use a once over from someone who didn't do a deep dive of this. It read ok to me though!

[clokep]

@benbz / @erikjohnston Did this get dropped on the floor? Looks like there's nothing left to do here except sync with develop?

[benbz]

@clokep thanks for following up on this. I've resynced with develop. There seem to be some sytest failures, but I don't think they relate to this PR?

[clokep]

LGTM! Thanks!