Skip to content
This repository has been archived by the owner on Dec 7, 2020. It is now read-only.

Logout redirect should use the end session endpoint from the discovery doc if it exists #620

Open
gps035 opened this issue May 22, 2020 · 4 comments
Assignees
Labels

Comments

@gps035
Copy link

gps035 commented May 22, 2020

Currently, when the enable-logout-redirect option is on, a user is redirected to $ISSUER/protocol/openid-connect/logout. This is not always the correct endpoint, and I believe that the end_session_endpoint from the discovery doc should be used if it is specified.

I have opened a PR(#619) to demonstrate how I think it should work, but I'm not 100% confident that this is the right approach

@abstractj
Copy link

@gps035 I was reading the description from your PR and I believe we need more details. Could you please provide more details like:

## Environment
<!-- 
Version of everything that it's running in your environment:
- OS:
- Kernel (e.g. `uname -a`):
- Go (e.g. go version)
- Server (e.g. Keycloak or any other IdP):
- Louketo:
-->

## Expected Results
<!-- 
What you expected to happen?
-->

## Actual Results

<!-- 
What happened?
-->

## Steps to reproduce

<!-- 
All the detailed steps required to reproduce the issue
-->

## Additional Information

<!-- 
Any additional information that you believe worth mentioning
-->

@abstractj abstractj self-assigned this Jun 3, 2020
@stianst
Copy link
Contributor

stianst commented Jun 4, 2020

@abstractj as part of making Louketo support multiple OIDC vendors it is important that it does not hard-code endpoints, and it should use end_session_endpoint from the Discovery URL as stated here. No assumption can be made on auth/token/logout or any endpoint and these should all be loaded from the Discovery URL.

@gps035
Copy link
Author

gps035 commented Jun 4, 2020

Basically what @stianst said is the motivation. Currently the logouts make an assumption about the logout url, and that assumption is incorrect for the internal auth server we have (I believe it is based on https://github.com/IdentityServer/IdentityServer4, or possibly 3, but I can't confirm as it is owned by another team).
As for the environment, it is currently running in docker, the image being a FROM scratch image with just the louketo proxy binary and the required ca certificates.
The code is built in the Dockerfile using Go 1.14, and the code we are building is a local copy with this fix applied, but originally based on #538 (which now seems to be superseeded by #630) as we need the updated OAuth2 dependency in order to work around another bug (coreos/go-oidc#178).

In the PR, you have mentioned adding tests around the change, but I will probably need a bit of guidance on this. I'm not on the project where this change is needed anymore, but also those are the first 3 lines of Go that I've ever written

@abstractj
Copy link

abstractj commented Jun 4, 2020

@gps035 what you and @stianst makes perfect sense to me now, thanks for clarifying. I will give your PR a try tomorrow. Meanwhile would you like to be the reviewer of #630 ? Just give it a try to make sure that things are working as expected?

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

No branches or pull requests

3 participants