Evolve oidc logout #87
Open
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
6 tasks
route443
requested changes
Apr 13, 2024
There was a problem hiding this comment.
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:
- 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. - If you are using
configure.shfor auto-configuration via OIDC discovery and your OP doesn't provide/support logout endpoint. You will getdefault nullfor$oidc_logout_endpoinvar, this is obviously also a non-working approach. - In most of the implementations I'm familiar with, the endpoint is called
end_session_endpoint, but you are usinglogout_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 ofend_session_endpointin 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** |
There was a problem hiding this comment.
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 | |||
There was a problem hiding this comment.
- 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 thelogout_endpointused? In addition, the OpenID Connect Discovery 1.0 refers specifically toend_session_endpoint, and there is not a single mention oflogout_endpoint. - The
end_session_endpointparameter 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 |
There was a problem hiding this comment.
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 { |
There was a problem hiding this comment.
I have some doubts about the practical usefulness of this functionality for several reasons:
- The description of
redir_post_logoutandoidc_logout_redirectis a little confusing and makes me think that they are about the same thing. - Why do we need a redirect somewhere else when the UA has successfully landed in a
/_logoutlocation? 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 |
There was a problem hiding this comment.
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
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Hello,
Following this pull request.
I'm doing a port of the code on this repository.