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

feat: Add optional compatibility option to support non-complaint IDPs that do not sign their LogOut responses. #1178

Merged

Conversation

henrikottesorensen
Copy link
Contributor

We got hit by issue #446, after our IDP began to returning unsigned LogOut responses. I have added a compatibility switch to support this specification violating behaviour, while we work with the IDP provider to get them in compliance with the spec.

@AndersAbel
Copy link
Member

I'm a bit confused about what the scope is of this PR. The title mentions "Logout Responses". The code excludes both logout responses and incoming logout requests from the signature check. The test case is for a logout request.

Do you want this to apply only to logout responses? Or only to requests? Or both?

From a security perspective I can't think of why a missing signature on a logout response is a problem. A missing signature/sender validation for an incoming logout request however might be part of an attack vector. If we compare to OpenID Connect they authenticate incoming logout requests (by including the id_token) but not the redirect back.

@henrikottesorensen
Copy link
Contributor Author

My apologies for the late reply, been putting out various brush fires and then easter holiday hit us.

Being unfamilar with the Saml2 protocol and the codebase, so the overly aggressive patch is my fault.

For our use case, until the IDP becomes spec complient, we only need to support unsigned logout RESPONSES. I can supply you with an example HAR archive of the traffic, if needed.

@AndersAbel
Copy link
Member

@henrikottesorensen Thank you for your response. I will go ahead and merge this with a modification to only allow unsigned logout responses.

@AndersAbel AndersAbel added this to the v2.7.0 milestone Apr 14, 2020
@AndersAbel AndersAbel merged commit 341c884 into Sustainsys:master Apr 14, 2020
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.

2 participants