-
Notifications
You must be signed in to change notification settings - Fork 94
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
Add OIDC end session endpoint and custom query params #72
base: main
Are you sure you want to change the base?
Add OIDC end session endpoint and custom query params #72
Conversation
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi Shawn. Thank you for your contribution to our nginx oidc project.
As I said in previous PRs, we are not using Conventional Commits for this project. So I wouldn't use this approach unnecessarily. Please change the commit name.
README.md
Outdated
* The authorization code flow is in use | ||
* NGINX Plus is configured as a relying party | ||
* The IdP knows NGINX Plus as a confidential client or a public client using PKCE | ||
- The identity provider (IdP) supports OpenID Connect 1.0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did these changes come from the Markdown auto-formatter? In any case, they are not relevant to the current PR and should be addressed separately.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes it is automatically changed by Markdown Formatter. Addressed this with a separate PR.
README.md
Outdated
@@ -36,7 +36,7 @@ If a [refresh token](https://openid.net/specs/openid-connect-core-1_0.html#Refre | |||
|
|||
### Logout | |||
|
|||
Requests made to the `/logout` location invalidate both the ID token and refresh token by erasing them from the key-value store. Therefore, subsequent requests to protected resources will be treated as a first-time request and send the client to the IdP for authentication. Note that the IdP may issue cookies such that an authenticated session still exists at the IdP. | |||
Requests made to the `/logout` location invalidate both the ID token, access token and refresh token by erasing them from the key-value store. Therefore, subsequent requests to protected resources will be treated as a first-time request and send the client to the IdP for authentication. By interacting with `$oidc_logout_endpoint` which is the end session endpoint of IdP, the authenticated session is ended at the IdP. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Access token support should be addressed separately.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure thing! Addressed separately in the access token PR.
openid_connect.js
Outdated
@@ -253,11 +256,39 @@ function validateIdToken(r) { | |||
} | |||
} | |||
|
|||
// Default RP-Initiated or Custom Logout w/ OP. | |||
// |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Style: no need for empty line.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure! Addressed it!
openid_connect.js
Outdated
// end-user's User Agent to the OP's Logout endpoint. | ||
// - https://openid.net/specs/openid-connect-rpinitiated-1_0.html#RPLogout | ||
// - https://openid.net/specs/openid-connect-rpinitiated-1_0.html#RedirectionAfterLogout | ||
// |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Style: no need for empty line.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Addressed it!
openid_connect.js
Outdated
@@ -253,11 +256,39 @@ function validateIdToken(r) { | |||
} | |||
} | |||
|
|||
// Default RP-Initiated or Custom Logout w/ OP. | |||
// | |||
// - An RP requests that the OP log out the end-user by redirecting the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Style: no need for hyphen character.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Addressed it!
openid_connect_configuration.conf
Outdated
default "/_logout"; # Built-in, simple logout page | ||
} | ||
|
||
map $host $post_logout_return_uri { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All OIDC-related directives/vars are prefixed with oidc_
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Addressed it.
openid_connect_configuration.conf
Outdated
default "/_logout"; # Built-in, simple logout page | ||
} | ||
|
||
map $host $post_logout_return_uri { | ||
# Where to send browser after the RP requests /logout to the OP, and after |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
# Where to send browser after the RP requests /logout to the OP, and after | |
# Where to redirect browser after the successful logout. If empty, redirects User Agent | |
# to the /_logout location. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for your suggestion. Consolidated this into $oidc_logout_landing_page
based on the review.
openid_connect_configuration.conf
Outdated
|
||
default ""; | ||
|
||
# Enable if you want to redirect to the landing page |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Too many examples, one will suffice.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! Addressed this into $oidc_logout_landing_page
.
openid_connect.server_conf
Outdated
# Enable oidc.redirectPostLogout(), and disable the above built-in logout | ||
# page if you want to redirect to either the landing page or custom | ||
# logout page using the map of $post_logout_return_uri. | ||
#js_content oidc.redirectPostLogout; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The redirectPostLogout
function can be left. We need to be able to clear cookies after logout.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Addressed it. But, the cookies are cleared here and redirect browser to the $oidc_logout_landing_page
as this location is called by IdP after successful logout from the IdP. Let me know if you have other thoughts.
openid_connect.js
Outdated
if (r.variables.post_logout_return_uri) { | ||
r.return(302, r.variables.post_logout_return_uri); | ||
} else { | ||
r.return(302, r.variables.redirect_base + r.variables.cookie_auth_redir); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The idea of redirecting the User Agent back to cookie_auth_redir
is not entirely clear to me.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was to follow the existing pattern. But, I have revised this as I addressed the idea of $oidc_logout_landing_page
. Thanks!
a769f11
to
24a8acb
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- Thanks for your review in detail.
- I have addressed some of comments and am going to address rest of them.
- In the meantime, I would appreciate it if you could review this PR(Rename variable from args to query params for authz endpoint #78) to see if we can change the variable name for consistency between NGINX Plus and NGINX Management Suite as
$oidc_authz_extra_args
has been recently merged.
README.md
Outdated
* The authorization code flow is in use | ||
* NGINX Plus is configured as a relying party | ||
* The IdP knows NGINX Plus as a confidential client or a public client using PKCE | ||
- The identity provider (IdP) supports OpenID Connect 1.0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes it is automatically changed by Markdown Formatter. Addressed this with a separate PR.
README.md
Outdated
@@ -36,7 +36,7 @@ If a [refresh token](https://openid.net/specs/openid-connect-core-1_0.html#Refre | |||
|
|||
### Logout | |||
|
|||
Requests made to the `/logout` location invalidate both the ID token and refresh token by erasing them from the key-value store. Therefore, subsequent requests to protected resources will be treated as a first-time request and send the client to the IdP for authentication. Note that the IdP may issue cookies such that an authenticated session still exists at the IdP. | |||
Requests made to the `/logout` location invalidate both the ID token, access token and refresh token by erasing them from the key-value store. Therefore, subsequent requests to protected resources will be treated as a first-time request and send the client to the IdP for authentication. By interacting with `$oidc_logout_endpoint` which is the end session endpoint of IdP, the authenticated session is ended at the IdP. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure thing! Addressed separately in the access token PR.
openid_connect.js
Outdated
@@ -253,11 +256,39 @@ function validateIdToken(r) { | |||
} | |||
} | |||
|
|||
// Default RP-Initiated or Custom Logout w/ OP. | |||
// |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure! Addressed it!
openid_connect.js
Outdated
@@ -253,11 +256,39 @@ function validateIdToken(r) { | |||
} | |||
} | |||
|
|||
// Default RP-Initiated or Custom Logout w/ OP. | |||
// | |||
// - An RP requests that the OP log out the end-user by redirecting the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Addressed it!
openid_connect.js
Outdated
// end-user's User Agent to the OP's Logout endpoint. | ||
// - https://openid.net/specs/openid-connect-rpinitiated-1_0.html#RPLogout | ||
// - https://openid.net/specs/openid-connect-rpinitiated-1_0.html#RedirectionAfterLogout | ||
// |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Addressed it!
openid_connect_configuration.conf
Outdated
default "http://127.0.0.1:8080/auth/realms/master/protocol/openid-connect/logout"; | ||
} | ||
|
||
map $host $oidc_logout_query_params_option { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
-
Extra parameters over the default parameters (that the NJS provided) make sense. However, it is not fully customizable for customer need as the following examples, or there would be unnecessary parameters which cause additional network packet although it is not too big.
-
example 1. Default RP-Initiated Logout Parameter
{ "post_logout_redirect_uri": "$redirect_base/_logout", "id_token_hint": "$session_jwt" }
-
example 2. AWS Cognito Logout Parameter
- If the following parameters are just added as extra arguments, then the unnecessary default parameters are sent to the IdP, and
post_logout_redirect_uri
&logout_uri
values are also redundant.
{ "client_id" : "$oidc_client", "logout_uri" : "$redirect_base/_logout" }
- If the following parameters are just added as extra arguments, then the unnecessary default parameters are sent to the IdP, and
-
example 3. AWS Cognito Logout Parameter to prompt a user to sign in as another user.
- If the following parameters are just added as extra arguments, then the unnecessary default parameters are sent to the IdP, and
post_logout_redirect_uri
&redirect_uri
values are also redundant.
{ "response_type": "code", "client_id" : "$oidc_client", "redirect_uri" : "$redirect_base$redir_location", "state" : "STATE", "scope" : "$oidc_scopes" }
- If the following parameters are just added as extra arguments, then the unnecessary default parameters are sent to the IdP, and
-
example 4. Auth0 Logout Parameter
- If the following parameters are just added as extra arguments, then the unnecessary default parameters are sent to the IdP, and
post_logout_redirect_uri
&returnTo
values are also redundant.
{ "client_id": "$oidc_client", "returnTo" : "$redirect_base/_logout" }
- If the following parameters are just added as extra arguments, then the unnecessary default parameters are sent to the IdP, and
Let me know if you just want extra parameters although unnecessary parameters can be sent to the IdP. I will address this based on the NGINX Plus Team's decision.
openid_connect_configuration.conf
Outdated
default 0; | ||
} | ||
|
||
map $host $oidc_logout_query_params { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- Yes, I am aware of it.
- I have suggested the naming change in the Rename variable from args to query params for authz endpoint #78 for consistency between NGINX Plus and NGINX Management Suite.
- I wanted to synchronize the variable name as it has been recently merged if possible between NGINX Plus and NGINX Management Suite.
- NGINX Plus OIDC:
$oidc_authz_extra_args
is merged (Dec/8/2022) - NGINX Management Suite:
$oidc_authz_query_params
is released (Jul/20/2022)
- NGINX Plus OIDC:
7bee7aa
to
7de0b09
Compare
configure.sh
Outdated
@@ -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_jwks_uri \(.jwks_uri)\n$oidc_logout_endpoint \(.logout_endpoint)"' < /tmp/${COMMAND}_$$_json > /tmp/${COMMAND}_$$_conf |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like a "Find and Replace" bug, since it worked in the previous iteration. Must be .end_session_endpoint
. It's probably worth having variable names like in OIDC well-known json so that there is no confusion:
oidc_end_session_endpoint = end_session_endpoint.
jq -r '. | "$oidc_authz_endpoint \(.authorization_endpoint)\n$oidc_token_endpoint \(.token_endpoint)\n$oidc_jwks_uri \(.jwks_uri)\n$oidc_logout_endpoint \(.logout_endpoint)"' < /tmp/${COMMAND}_$$_json > /tmp/${COMMAND}_$$_conf | |
jq -r '. | "$oidc_authz_endpoint \(.authorization_endpoint)\n$oidc_token_endpoint \(.token_endpoint)\n$oidc_jwks_uri \(.jwks_uri)\n$oidc_end_session_endpoint \(.end_session_endpoint)"' < /tmp/${COMMAND}_$$_json > /tmp/${COMMAND}_$$_conf |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah! I had changed in my mind.
- For the first time, it was
$oidc_logout_endpoint
. - But I had changed it with
$oidc_end_session_endpoint
to match with the OIDC well-known endpoint. - However, there would be another work to sync variable name with NGINX Management Suite, and OIDC spec is using between logout and end_session.
- Therefore, I have changed it again. 😺
To sum up,
- It makes sense to use same variable name in the OIDC well-known JSON.
- So, I have changed it again.
configure.sh
Outdated
@@ -178,7 +178,7 @@ fi | |||
|
|||
# Loop through each configuration variable | |||
echo "$COMMAND: NOTICE: Configuring $CONFDIR/openid_connect_configuration.conf" | |||
for OIDC_VAR in \$oidc_authz_endpoint \$oidc_token_endpoint \$oidc_jwt_keyfile \$oidc_hmac_key $CLIENT_ID_VAR $CLIENT_SECRET_VAR $PKCE_ENABLE_VAR; do | |||
for OIDC_VAR in \$oidc_authz_endpoint \$oidc_token_endpoint \$oidc_jwt_keyfile \$oidc_logout_endpoint \$oidc_hmac_key $CLIENT_ID_VAR $CLIENT_SECRET_VAR $PKCE_ENABLE_VAR; do |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
for OIDC_VAR in \$oidc_authz_endpoint \$oidc_token_endpoint \$oidc_jwt_keyfile \$oidc_logout_endpoint \$oidc_hmac_key $CLIENT_ID_VAR $CLIENT_SECRET_VAR $PKCE_ENABLE_VAR; do | |
for OIDC_VAR in \$oidc_authz_endpoint \$oidc_token_endpoint \$oidc_jwt_keyfile \$oidc_ end_session_endpoint \$oidc_hmac_key $CLIENT_ID_VAR $CLIENT_SECRET_VAR $PKCE_ENABLE_VAR; do |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Revised!
openid_connect.js
Outdated
if (r.variables.oidc_logout_query_params) { | ||
queryParams = '?' + r.variables.oidc_logout_query_params; | ||
} | ||
r.variables.request_id = '-'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is no need to clear request_id
variable as it's an embedded var.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure! Revised!
README.md
Outdated
@@ -105,7 +105,7 @@ Manual configuration involves reviewing the following files so that they match y | |||
|
|||
* **openid_connect_configuration.conf** - this contains the primary configuration for one or more IdPs in `map{}` blocks | |||
* Modify all of the `map…$oidc_` blocks to match your IdP configuration | |||
* Modify the URI defined in `map…$oidc_logout_redirect` to specify an unprotected resource to be displayed after requesting the `/logout` location | |||
* Modify the URI defined in `map…$oidc_logout_landing_page` to redirect browser after successful logout. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The period char at the end of the sentence is not required
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! Removed!
openid_connect_configuration.conf
Outdated
default 0; | ||
} | ||
|
||
map $host $oidc_logout_query_params { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If I've caught your logic, then the name should be extra_query_params
because query_params is everything that comes after the question mark. Since we already have some list of required query parameters declared in authZArgs
.
# Each IdP may use different query params of the $oidc_logout_endpoint. For | ||
# example, Amazon Cognito requires `client_id` and `logout_uri`, and Auth0 | ||
# requires `client_id` and `returnTo` instead of the default query params. | ||
default "post_logout_redirect_uri=$redirect_base/_logout&id_token_hint=$session_jwt"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this approach can still confuse the inexperienced user:
- We cannot replace the
post_logout_redirect_uri
parameter with something of our own, because the_logout
location will perform additional functions. - We already have 2 similar parameters:
post_logout_redirect_uri
and$oidc_logout_landing_page
.
Perhaps it makes sense to hide post_logout_redirect_uri=$redirect_base/_logout&
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Background:
-
The RP-Initiated Logout of OIDC spec uses
post_logout_redirect_uri
for most of IdPs. -
However, all of fields are not required, and several IdPs use different variable name as the following example.
-
Example 1. Default RP-Initiated Logout Parameter
{ "post_logout_redirect_uri": "$redirect_base/_logout", "id_token_hint": "$session_jwt" }
-
Example 2. AWS Cognito Logout Parameter
If the following parameters are just added as extra arguments, then the unnecessary default parameters are sent to the IdP, andpost_logout_redirect_uri
&logout_uri
values are also redundant.{ "client_id" : "$oidc_client", "logout_uri" : "$redirect_base/_logout" }
-
Example 3. AWS Cognito Logout Parameter to prompt a user to sign in as another user.
If the following parameters are just added as extra arguments, then the unnecessary default parameters are sent to the IdP, andpost_logout_redirect_uri
&redirect_uri
values are also redundant.{ "response_type": "code", "client_id" : "$oidc_client", "redirect_uri" : "$redirect_base$redir_location", "state" : "STATE", "scope" : "$oidc_scopes" }
-
Example 4. Auth0 Logout Parameter
If the following parameters are just added as extra arguments, then the unnecessary default parameters are sent to the IdP, andpost_logout_redirect_uri
&returnTo
values are also redundant.{ "client_id": "$oidc_client", "returnTo" : "$redirect_base/_logout" }
-
Summary:
In the file of openid_connect.server_conf
:
$oidc_logout_callback_param
had been added so the key/value are hidden as you suggested.$oidc_logout_callback_uri
has been added so customers can reuse it as a value of different key of query param per each IdP.
7de0b09
to
c0a1b61
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for your review. Revised and added some one cent thoughts.
README.md
Outdated
@@ -105,7 +105,7 @@ Manual configuration involves reviewing the following files so that they match y | |||
|
|||
* **openid_connect_configuration.conf** - this contains the primary configuration for one or more IdPs in `map{}` blocks | |||
* Modify all of the `map…$oidc_` blocks to match your IdP configuration | |||
* Modify the URI defined in `map…$oidc_logout_redirect` to specify an unprotected resource to be displayed after requesting the `/logout` location | |||
* Modify the URI defined in `map…$oidc_logout_landing_page` to redirect browser after successful logout. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! Removed!
configure.sh
Outdated
@@ -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_jwks_uri \(.jwks_uri)\n$oidc_logout_endpoint \(.logout_endpoint)"' < /tmp/${COMMAND}_$$_json > /tmp/${COMMAND}_$$_conf |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah! I had changed in my mind.
- For the first time, it was
$oidc_logout_endpoint
. - But I had changed it with
$oidc_end_session_endpoint
to match with the OIDC well-known endpoint. - However, there would be another work to sync variable name with NGINX Management Suite, and OIDC spec is using between logout and end_session.
- Therefore, I have changed it again. 😺
To sum up,
- It makes sense to use same variable name in the OIDC well-known JSON.
- So, I have changed it again.
configure.sh
Outdated
@@ -178,7 +178,7 @@ fi | |||
|
|||
# Loop through each configuration variable | |||
echo "$COMMAND: NOTICE: Configuring $CONFDIR/openid_connect_configuration.conf" | |||
for OIDC_VAR in \$oidc_authz_endpoint \$oidc_token_endpoint \$oidc_jwt_keyfile \$oidc_hmac_key $CLIENT_ID_VAR $CLIENT_SECRET_VAR $PKCE_ENABLE_VAR; do | |||
for OIDC_VAR in \$oidc_authz_endpoint \$oidc_token_endpoint \$oidc_jwt_keyfile \$oidc_logout_endpoint \$oidc_hmac_key $CLIENT_ID_VAR $CLIENT_SECRET_VAR $PKCE_ENABLE_VAR; do |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Revised!
openid_connect.js
Outdated
if (r.variables.oidc_logout_query_params) { | ||
queryParams = '?' + r.variables.oidc_logout_query_params; | ||
} | ||
r.variables.request_id = '-'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure! Revised!
# Each IdP may use different query params of the $oidc_logout_endpoint. For | ||
# example, Amazon Cognito requires `client_id` and `logout_uri`, and Auth0 | ||
# requires `client_id` and `returnTo` instead of the default query params. | ||
default "post_logout_redirect_uri=$redirect_base/_logout&id_token_hint=$session_jwt"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Background:
-
The RP-Initiated Logout of OIDC spec uses
post_logout_redirect_uri
for most of IdPs. -
However, all of fields are not required, and several IdPs use different variable name as the following example.
-
Example 1. Default RP-Initiated Logout Parameter
{ "post_logout_redirect_uri": "$redirect_base/_logout", "id_token_hint": "$session_jwt" }
-
Example 2. AWS Cognito Logout Parameter
If the following parameters are just added as extra arguments, then the unnecessary default parameters are sent to the IdP, andpost_logout_redirect_uri
&logout_uri
values are also redundant.{ "client_id" : "$oidc_client", "logout_uri" : "$redirect_base/_logout" }
-
Example 3. AWS Cognito Logout Parameter to prompt a user to sign in as another user.
If the following parameters are just added as extra arguments, then the unnecessary default parameters are sent to the IdP, andpost_logout_redirect_uri
&redirect_uri
values are also redundant.{ "response_type": "code", "client_id" : "$oidc_client", "redirect_uri" : "$redirect_base$redir_location", "state" : "STATE", "scope" : "$oidc_scopes" }
-
Example 4. Auth0 Logout Parameter
If the following parameters are just added as extra arguments, then the unnecessary default parameters are sent to the IdP, andpost_logout_redirect_uri
&returnTo
values are also redundant.{ "client_id": "$oidc_client", "returnTo" : "$redirect_base/_logout" }
-
Summary:
In the file of openid_connect.server_conf
:
$oidc_logout_callback_param
had been added so the key/value are hidden as you suggested.$oidc_logout_callback_uri
has been added so customers can reuse it as a value of different key of query param per each IdP.
openid_connect_configuration.conf
Outdated
default 0; | ||
} | ||
|
||
map $host $oidc_logout_query_params { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- It makes sense to use
extra_query_params
rather thanextra_args
although there is no required query parameters declared for the$oidc_end_session_endpoint
. - FYI. The required query parameters declared in
authZArgs
for the authz endpoint are not fully enough to be reconfigurable by customers unless they change codes. Hence, we can think of how the required fields' value can be configurable in the future. (Therefore, I used three ways ofextra
,replace
andbuilt-in
although this way is also not the best approach.)
To sum up: changed it as extra_query_params
at this moment.
c0a1b61
to
5ed5521
Compare
README.md
Outdated
@@ -36,7 +36,7 @@ If a [refresh token](https://openid.net/specs/openid-connect-core-1_0.html#Refre | |||
|
|||
### Logout | |||
|
|||
Requests made to the `/logout` location invalidate both the ID token and refresh token by erasing them from the key-value store. Therefore, subsequent requests to protected resources will be treated as a first-time request and send the client to the IdP for authentication. Note that the IdP may issue cookies such that an authenticated session still exists at the IdP. | |||
Requests made to the `/logout` location invalidate both the ID token and refresh token by erasing them from the key-value store. Therefore, subsequent requests to protected resources will be treated as a first-time request and send the client to the IdP for authentication. By interacting with `$oidc_logout_endpoint` which is the end session endpoint of IdP, the authenticated session is ended at the IdP. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Requests made to the `/logout` location invalidate both the ID token and refresh token by erasing them from the key-value store. Therefore, subsequent requests to protected resources will be treated as a first-time request and send the client to the IdP for authentication. By interacting with `$oidc_logout_endpoint` which is the end session endpoint of IdP, the authenticated session is ended at the IdP. | |
Requests made to the `/logout` location invalidate both the ID token and refresh token by erasing them from the key-value store. Next, the User Agent is redirected to the `$oidc_end_session_endpoint` in order to terminate the user session on the IdP's side. After the session is successfully terminated on the IdP side, the User Agent will be redirected to the `$oidc_logout_landing_page`. Note that the $oidc_logout_landing_page endpoint must not require authentication, otherwise the user authentication process may be initiated from the beginning. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the suggestion. Revised it.
openid_connect.js
Outdated
r.variables.refresh_token = "-"; | ||
r.return(302, r.variables.oidc_logout_redirect); | ||
var queryParams = ''; | ||
if (r.variables.oidc_logout_extra_query_params) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if (r.variables.oidc_logout_extra_query_params) { | |
if (r.variables.oidc_logout_query_params) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great. Extra itself was not enough. 😄 Revised it!
openid_connect.js
Outdated
r.return(302, r.variables.oidc_logout_redirect); | ||
var queryParams = ''; | ||
if (r.variables.oidc_logout_extra_query_params) { | ||
queryParams = '?' + r.variables.oidc_logout_extra_query_params; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
queryParams = '?' + r.variables.oidc_logout_extra_query_params; | |
queryParams = '?' + r.variables.oidc_logout_query_params; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Revised it.
openid_connect.server_conf
Outdated
@@ -1,6 +1,8 @@ | |||
# Advanced configuration START | |||
set $internal_error_message "NGINX / OpenID Connect login failure\n"; | |||
set $pkce_id ""; | |||
set $oidc_logout_callback_uri "$redirect_base/_logout"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Introducing a new variable does not make the process any easier.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for your thoughts. Removed it.
openid_connect.server_conf
Outdated
@@ -1,6 +1,8 @@ | |||
# Advanced configuration START | |||
set $internal_error_message "NGINX / OpenID Connect login failure\n"; | |||
set $pkce_id ""; | |||
set $oidc_logout_callback_uri "$redirect_base/_logout"; | |||
set $oidc_logout_callback_param "post_logout_redirect_uri=$oidc_logout_callback_uri"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Introducing a new variable does not make the process any easier.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for your thoughts. Removed it.
openid_connect_configuration.conf
Outdated
default "http://127.0.0.1:8080/auth/realms/master/protocol/openid-connect/logout"; | ||
} | ||
|
||
map $host $oidc_logout_extra_query_params { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
map $host $oidc_logout_extra_query_params { | |
map $host $oidc_logout_query_params { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Revised it!
openid_connect_configuration.conf
Outdated
# Each IdP may use different query params of the $oidc_end_session_endpoint. | ||
# For example, Amazon Cognito requires `logout_uri=xxx`, and Auth0 requires | ||
# `returnTo=xxx` instead of $oidc_logout_callback_param. | ||
default "$oidc_logout_callback_param&id_token_hint=$session_jwt";; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The proposed version did not add simplicity and clarity, but rather the opposite. It's better to return as it was.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah. Returned as it was!
5ed5521
to
ad7b9f0
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Addressed the comments. Let me know if I miss anything. Thanks!
README.md
Outdated
@@ -36,7 +36,7 @@ If a [refresh token](https://openid.net/specs/openid-connect-core-1_0.html#Refre | |||
|
|||
### Logout | |||
|
|||
Requests made to the `/logout` location invalidate both the ID token and refresh token by erasing them from the key-value store. Therefore, subsequent requests to protected resources will be treated as a first-time request and send the client to the IdP for authentication. Note that the IdP may issue cookies such that an authenticated session still exists at the IdP. | |||
Requests made to the `/logout` location invalidate both the ID token and refresh token by erasing them from the key-value store. Therefore, subsequent requests to protected resources will be treated as a first-time request and send the client to the IdP for authentication. By interacting with `$oidc_logout_endpoint` which is the end session endpoint of IdP, the authenticated session is ended at the IdP. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the suggestion. Revised it.
openid_connect.js
Outdated
r.variables.refresh_token = "-"; | ||
r.return(302, r.variables.oidc_logout_redirect); | ||
var queryParams = ''; | ||
if (r.variables.oidc_logout_extra_query_params) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great. Extra itself was not enough. 😄 Revised it!
openid_connect.js
Outdated
r.return(302, r.variables.oidc_logout_redirect); | ||
var queryParams = ''; | ||
if (r.variables.oidc_logout_extra_query_params) { | ||
queryParams = '?' + r.variables.oidc_logout_extra_query_params; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Revised it.
openid_connect.server_conf
Outdated
@@ -1,6 +1,8 @@ | |||
# Advanced configuration START | |||
set $internal_error_message "NGINX / OpenID Connect login failure\n"; | |||
set $pkce_id ""; | |||
set $oidc_logout_callback_uri "$redirect_base/_logout"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for your thoughts. Removed it.
openid_connect.server_conf
Outdated
@@ -1,6 +1,8 @@ | |||
# Advanced configuration START | |||
set $internal_error_message "NGINX / OpenID Connect login failure\n"; | |||
set $pkce_id ""; | |||
set $oidc_logout_callback_uri "$redirect_base/_logout"; | |||
set $oidc_logout_callback_param "post_logout_redirect_uri=$oidc_logout_callback_uri"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for your thoughts. Removed it.
openid_connect_configuration.conf
Outdated
default "http://127.0.0.1:8080/auth/realms/master/protocol/openid-connect/logout"; | ||
} | ||
|
||
map $host $oidc_logout_extra_query_params { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Revised it!
openid_connect_configuration.conf
Outdated
# Each IdP may use different query params of the $oidc_end_session_endpoint. | ||
# For example, Amazon Cognito requires `logout_uri=xxx`, and Auth0 requires | ||
# `returnTo=xxx` instead of $oidc_logout_callback_param. | ||
default "$oidc_logout_callback_param&id_token_hint=$session_jwt";; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah. Returned as it was!
69e28e7
to
92d267c
Compare
6cc94b4
to
b79661c
Compare
rename oidc_end_session_query_params fix: typo
b79661c
to
fc05d36
Compare
Issue:
Summary:
Description:
Added a map variable of
$oidc_end_session_endpoint
as same as authorization and token endpoints in theopenid_connect_configuration.conf
.Added a map variable of
$oidc_logout_landing_page
to determine where to redirect browser after successful logout from the IdP.Added a map variable of
$oidc_end_session_query_params
to support different query parameters per each IdP.Enhanced
/logout
location:$oidc_end_session_query_params
for the$oidc_end_session_endpoint
.$oidc_end_session_endpoint
to start ending session in the IdP.Enhanced
/_logout
location:$oidc_logout_landing_page
.