Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Config to disable discovery initiated SAMLBackend flows #322

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

skoranda
Copy link
Contributor

@skoranda skoranda commented Apr 13, 2020

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.

All Submissions:

  • Have you checked to ensure there aren't other open Pull Requests for the same update/change?
  • Have you added an explanation of what problem you are trying to solve with this PR?
  • Have you added information on what your changes do and why you chose this as your solution?
  • Have you written new tests for your changes?
  • Does your submission pass tests?
  • This project follows PEP8 style guide. Have you run your code against the 'flake8' linter?

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.
@peppelinux
Copy link
Member

Really usefull

@skoranda
Copy link
Contributor Author

Here is a use case where allowing discovery initiated SAMLBackend flows remains important.

Users do bookmark the discovery service (at the time they do it, it includes the return URL to SATOSA so the bookmark is in some sense valid). Yes, they should not do that, and education can help, but it cannot stop all users from bookmarking the discovery service.

So rather than stopping the flow entirely, it is helpful to be able to allow the user to go to the IdP and authenticate. Then when the user returns, a microservice can detect there is no SP requestor, and direct the user to the VO dashboard, which contains a list of all SPs to which the user can connect. They can then click on the SP and go through the SSO flow and get to the SP, even though they started the day from the discovery service.

I have one deployment that wants to support this scenario to help reduce the communications to the HelpDesk.

@peppelinux
Copy link
Member

Exactly, the same use case here.
But I'd prefer to stop the flow first of all. Because users get frustration in the case of a failed Auth.
Moreover in case of SAML2 forceAuthn or MFA the frustration could be come more obvious. Just drop it whit an eloquent message (yes, redirect page) that remember to user that they are should initialize session from a real SP

@peppelinux
Copy link
Member

This feature Is really important, keep It on top of the agenda!

@c00kiemon5ter
Copy link
Member

The problem with starting a flow from the discovery-endpoint is that the proxy does not know what the originating SP really is. The proxy is lost and crashes.

To resolve this, a configurable handler will be registered. The handler by default will stop the flow. A user will be able to configure this handler to provide different behaviour - for example redirecting to an error page or a service-selection page.

This is similar to #324 and those two will be handled by a similar mechanism.

@peppelinux
Copy link
Member

To resolve this, a configurable handler will be registered. The handler by default will stop the flow. A user will be able to configure this handler to provide different behaviour - for example redirecting to an error page or a service-selection page.

or a default SP?

This is similar to #324 and those two will be handled by a similar mechanism.

completely agree

KEY_DISCO_SRV = 'disco_srv'
KEY_SAML_DISCOVERY_INITIATED = 'saml_discovery_initiated'
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@skoranda could we adopt KEY_ALLOW_DISCO_INIT_CONFIG only, without KEY_SAML_DISCOVERY_INITIATED?

SAMLBackend.KEY_SAML_DISCOVERY_INITIATED,
False)

if (not self.config.get(SAMLBackend.KEY_ALLOW_DISCO_INIT_CONFIG, True)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Without KEY_SAML_DISCOVERY_INITIATED it would be

if not self.config.get(SAMLBackend.KEY_ALLOW_DISCO_INIT_CONFIG, False):
    # ....

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants