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

Evolve oidc logout #87

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Conversation

llomgui
Copy link

@llomgui llomgui commented Jan 30, 2024

Hello,

Following this pull request.
I'm doing a port of the code on this repository.

Copy link
Contributor

@route443 route443 left a comment

Choose a reason for hiding this comment

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

Thank you for your contribution to our project and I apologize for the delay in the review.

Your implementation of RP-initiated logout has some serious issues that need to be taken care of:

  1. You don't care about backwards compatibility. RP-initiated logout support (and specifically end_session_endpoint) is optional and many providers still do not support it. In this case, you'll get a non-working configuration and you wont be able to use the traditional logout approach, where the security context is destroyed only on the NGINX side.
  2. If you are using configure.sh for auto-configuration via OIDC discovery and your OP doesn't provide/support logout endpoint. You will get default null for $oidc_logout_endpoin var, this is obviously also a non-working approach.
  3. In most of the implementations I'm familiar with, the endpoint is called end_session_endpoint, but you are using logout_endpoint. This leads me to believe that this is also not something standardized, and providers are still using their own implementations. Meanwhile, I've only found one mention of end_session_endpoint in OpenID Connect Discovery 1.0.


* If your IdP supports OpenID Connect Discovery (usually at the URI `/.well-known/openid-configuration`) then use the `configure.sh` script to complete configuration. In this case you can skip the next section. Otherwise:
* Obtain the URL for `jwks_uri` or download the JWK file to your NGINX Plus instance
* Obtain the URL for the **authorization endpoint**
* Obtain the URL for the **token endpoint**
* Obtain the URL for the **logout endpoint**
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd replace logout endpoint with end session endpoint, since this is most likely how it will be reflected in the IdP documentation.

@@ -120,7 +120,7 @@ fi
# Build an intermediate configuration file
# File format is: <NGINX variable name><space><IdP value>
#
jq -r '. | "$oidc_authz_endpoint \(.authorization_endpoint)\n$oidc_token_endpoint \(.token_endpoint)\n$oidc_jwks_uri \(.jwks_uri)"' < /tmp/${COMMAND}_$$_json > /tmp/${COMMAND}_$$_conf
jq -r '. | "$oidc_authz_endpoint \(.authorization_endpoint)\n$oidc_token_endpoint \(.token_endpoint)\n$oidc_logout_endpoint \(.logout_endpoint)\n$oidc_jwks_uri \(.jwks_uri)"' < /tmp/${COMMAND}_$$_json > /tmp/${COMMAND}_$$_conf
Copy link
Contributor

Choose a reason for hiding this comment

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

  1. In most implementations with which I'm familiar, the endpoint is called end_session_endpoint. I’m curious, what implementation of OP did you work with, where is the logout_endpoint used? In addition, the OpenID Connect Discovery 1.0 refers specifically to end_session_endpoint, and there is not a single mention of logout_endpoint.
  2. The end_session_endpoint parameter is optional and many implementations do not support it, but you do not handle these cases in any way.

# IDP after closing user session in the IDP.

# Clean cookies
add_header Set-Cookie "auth_nonce=; $oidc_cookie_flags"; # Send empty cookie
Copy link
Contributor

Choose a reason for hiding this comment

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

What is the practical meaning of your approach when you initiate security context destruction at the time of the /logout call, but clear the cookies only in the callback (when the UA has returned from the OP)?

default "http://127.0.0.1:8080/auth/realms/master/protocol/openid-connect/logout";
}

map $host $redir_post_logout {
Copy link
Contributor

Choose a reason for hiding this comment

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

I have some doubts about the practical usefulness of this functionality for several reasons:

  1. The description of redir_post_logout and oidc_logout_redirect is a little confusing and makes me think that they are about the same thing.
  2. Why do we need a redirect somewhere else when the UA has successfully landed in a /_logout location? At this point, the user can be shown text about the successful logout, replace it with an app unprotected page, or implement any other logic within this location.


# Clean cookies
add_header Set-Cookie "auth_nonce=; $oidc_cookie_flags"; # Send empty cookie
add_header Set-Cookie "auth_token=; $oidc_cookie_flags"; # Erase original cookie
Copy link
Contributor

Choose a reason for hiding this comment

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

These comments make no sense, so I'd remove them.

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