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

SAML 2.0 Logout filters should consider RelyingPartyRegistration logout properties #11182

Open
marcusdacoregio opened this issue May 4, 2022 · 3 comments
Assignees
Labels
in: saml2 An issue in SAML2 modules type: bug A general bug
Milestone

Comments

@marcusdacoregio
Copy link
Contributor

marcusdacoregio commented May 4, 2022

Describe the bug
Each RelyingParty can specify its own logout URL properties, but doing that results in having to change the RequestMatcher used by Saml2LogoutRequest/ResponseFilter manually to match those different URLs. It could be confusing to allow multiple relying parties to be defined with different locations that ultimately have no effect (or an undefined effect) on the behavior.

In both the logout request and logout response filters, the payload validation is going to check the URI against the registration's configured URI anyway. With that said, we should resolve the RequestMatcher based on what's in the RelyingPartyRegistration.

Expected behavior
Saml2LogoutRequestFilter and Saml2LogoutResponseFilter should consider singleLogoutServiceLocation and singleLogoutServiceResponseLocation in the RequestMatcher.

Related:

@marcusdacoregio marcusdacoregio added type: bug A general bug in: saml2 An issue in SAML2 modules labels May 4, 2022
@marcusdacoregio marcusdacoregio added this to the 5.8.0-M1 milestone May 4, 2022
@marcusdacoregio marcusdacoregio self-assigned this May 4, 2022
@marcusdacoregio marcusdacoregio modified the milestones: 5.8.0-M1, 5.7.0 May 4, 2022
@marcusdacoregio
Copy link
Contributor Author

Both the login filters and logout filters match on requests that contain either SAMLRequest or SAMLResponse as a parameter. If the Logout filters match on any request, they will try to process requests for login as well, causing problems.

@marcusdacoregio marcusdacoregio removed this from the 5.7.0 milestone May 6, 2022
@marcusdacoregio marcusdacoregio added status: declined A suggestion or change that we don't feel we should currently apply and removed type: bug A general bug labels May 6, 2022
@marcusdacoregio marcusdacoregio changed the title SAML 2.0 Logout filters should match on any request by default SAML 2.0 Logout filters should consider RelyingPartyRegistration logout properties May 10, 2022
@marcusdacoregio marcusdacoregio added type: bug A general bug and removed status: declined A suggestion or change that we don't feel we should currently apply labels May 10, 2022
@marcusdacoregio marcusdacoregio added this to the 5.8.0-M1 milestone May 10, 2022
@jzheaux
Copy link
Contributor

jzheaux commented May 11, 2022

With that said, we should resolve the RequestMatcher based on what's in the RelyingPartyRegistration.

Though it's likely that the path for the filter and the path in the registration will be the same, it seems valuable to consider scenarios when they could reasonably be different. One example is if a legacy URL, like /saml/SingleLogout from Spring Security SAML Extension, is forwarded to /logout/saml2/slo. In that case, the RP metadata still says that the endpoint is /saml/SingleLogout.

Whether this is the ideal way to do something like that remains to be seen. I think it would be good to complete spring-projects/spring-security-samples#76 before proceeding with this ticket.

@marcusdacoregio marcusdacoregio modified the milestones: 5.8.0-M1, 5.8.x May 11, 2022
@jzheaux
Copy link
Contributor

jzheaux commented May 23, 2022

Another scenario to consider is that sometimes the logout link is clicked when the user is already logged out. In this circumstance, the relying party must be looked up from the URL in order to still provide a logout response to the IdP.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
in: saml2 An issue in SAML2 modules type: bug A general bug
Projects
None yet
Development

No branches or pull requests

2 participants