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

Add OIDC end session endpoint and custom query params #72

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

Conversation

shawnhankim
Copy link
Contributor

@shawnhankim shawnhankim commented Dec 22, 2022

Issue:

Summary:

  • Added the IdP's end session endpoint to terminate the user session on the IdP's side.
  • Added the customizable variable to support different query parameters from each IdP.
  • Enhanced RP's callback URI for NGINX to clean cookies and redirect to the OIDC logout landing page.

Description:

  • Added a map variable of $oidc_end_session_endpoint as same as authorization and token endpoints in the openid_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:

    • Add query parameters using $oidc_end_session_query_params for the $oidc_end_session_endpoint.
    • NGINX Plus: cleared tokens.
    • Redirected to the $oidc_end_session_endpoint to start ending session in the IdP.
  • Enhanced /_logout location:

    • Redirected by IdP when IdP successfully finished the session.
    • Clean cookies
    • NGINX Plus: Redirect to the $oidc_logout_landing_page.

@shawnhankim
Copy link
Contributor Author

shawnhankim commented Dec 22, 2022

@route443 :

  • Thanks for your review in detail for the PR.
  • This PR is to simplify from the previous PR.
  • For you to easily manage this repo to reduce any concerns from the enhancements based on the reviews on the PR, I have divided a big PR into small PRs, and this is one of PRs.
  • Let me know if you have any questions.

@shawnhankim shawnhankim changed the title feat: integrated OIDC logout endpoint feat: integrate OIDC logout endpoint Dec 22, 2022
@shawnhankim shawnhankim changed the title feat: integrate OIDC logout endpoint feat: integrate OIDC logout endpoint and custom query params Dec 24, 2022
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.

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
Copy link
Contributor

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.

Copy link
Contributor Author

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.
Copy link
Contributor

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.

Copy link
Contributor Author

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.

@@ -253,11 +256,39 @@ function validateIdToken(r) {
}
}

// Default RP-Initiated or Custom Logout w/ OP.
//
Copy link
Contributor

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure! Addressed it!

// 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
//
Copy link
Contributor

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Addressed it!

@@ -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
Copy link
Contributor

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Addressed it!

default "/_logout"; # Built-in, simple logout page
}

map $host $post_logout_return_uri {
Copy link
Contributor

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_.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Addressed it.

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
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
# 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.

Copy link
Contributor Author

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.


default "";

# Enable if you want to redirect to the landing page
Copy link
Contributor

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.

Copy link
Contributor Author

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.

# 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;
Copy link
Contributor

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.

Copy link
Contributor Author

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.

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);
Copy link
Contributor

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.

Copy link
Contributor Author

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!

@shawnhankim shawnhankim force-pushed the 021-oidc-logout-endpoint branch from a769f11 to 24a8acb Compare December 30, 2022 16:09
Copy link
Contributor Author

@shawnhankim shawnhankim left a comment

Choose a reason for hiding this comment

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

@route443

  • 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
Copy link
Contributor Author

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.
Copy link
Contributor Author

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.

@@ -253,11 +256,39 @@ function validateIdToken(r) {
}
}

// Default RP-Initiated or Custom Logout w/ OP.
//
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure! Addressed it!

@@ -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
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Addressed it!

// 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
//
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Addressed it!

openid_connect.server_conf Show resolved Hide resolved
default "http://127.0.0.1:8080/auth/realms/master/protocol/openid-connect/logout";
}

map $host $oidc_logout_query_params_option {
Copy link
Contributor Author

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"
    }
  • 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"
    }
  • 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"
    }

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.

default 0;
}

map $host $oidc_logout_query_params {
Copy link
Contributor Author

@shawnhankim shawnhankim Dec 30, 2022

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)

@shawnhankim shawnhankim changed the title feat: integrate OIDC logout endpoint and custom query params integrate OIDC logout endpoint and custom query params Dec 30, 2022
@shawnhankim shawnhankim force-pushed the 021-oidc-logout-endpoint branch 5 times, most recently from 7bee7aa to 7de0b09 Compare December 30, 2022 18:05
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
Copy link
Contributor

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.

Suggested change
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

Copy link
Contributor Author

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
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Revised!

if (r.variables.oidc_logout_query_params) {
queryParams = '?' + r.variables.oidc_logout_query_params;
}
r.variables.request_id = '-';
Copy link
Contributor

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.

Copy link
Contributor Author

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.
Copy link
Contributor

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks! Removed!

default 0;
}

map $host $oidc_logout_query_params {
Copy link
Contributor

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";
Copy link
Contributor

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:

  1. We cannot replace the post_logout_redirect_uri parameter with something of our own, because the _logout location will perform additional functions.
  2. 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&.

Copy link
Contributor Author

@shawnhankim shawnhankim Jan 5, 2023

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, and post_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, 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"
      }
    • 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"
      }

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.

@shawnhankim shawnhankim force-pushed the 021-oidc-logout-endpoint branch from 7de0b09 to c0a1b61 Compare January 6, 2023 00:26
Copy link
Contributor Author

@shawnhankim shawnhankim left a 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.
Copy link
Contributor Author

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
Copy link
Contributor Author

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
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Revised!

if (r.variables.oidc_logout_query_params) {
queryParams = '?' + r.variables.oidc_logout_query_params;
}
r.variables.request_id = '-';
Copy link
Contributor Author

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";
Copy link
Contributor Author

@shawnhankim shawnhankim Jan 5, 2023

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, and post_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, 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"
      }
    • 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"
      }

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.

default 0;
}

map $host $oidc_logout_query_params {
Copy link
Contributor Author

@shawnhankim shawnhankim Jan 6, 2023

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 than extra_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 and built-in although this way is also not the best approach.)

To sum up: changed it as extra_query_params at this moment.

@shawnhankim shawnhankim force-pushed the 021-oidc-logout-endpoint branch from c0a1b61 to 5ed5521 Compare January 6, 2023 00:34
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.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
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.

Copy link
Contributor Author

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.

r.variables.refresh_token = "-";
r.return(302, r.variables.oidc_logout_redirect);
var queryParams = '';
if (r.variables.oidc_logout_extra_query_params) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
if (r.variables.oidc_logout_extra_query_params) {
if (r.variables.oidc_logout_query_params) {

Copy link
Contributor Author

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!

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;
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
queryParams = '?' + r.variables.oidc_logout_extra_query_params;
queryParams = '?' + r.variables.oidc_logout_query_params;

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Revised it.

@@ -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";
Copy link
Contributor

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.

Copy link
Contributor Author

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.

@@ -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";
Copy link
Contributor

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.

Copy link
Contributor Author

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.

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

map $host $oidc_logout_extra_query_params {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
map $host $oidc_logout_extra_query_params {
map $host $oidc_logout_query_params {

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Revised it!

# 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";;
Copy link
Contributor

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.

Copy link
Contributor Author

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!

@shawnhankim shawnhankim force-pushed the 021-oidc-logout-endpoint branch from 5ed5521 to ad7b9f0 Compare January 6, 2023 04:02
Copy link
Contributor Author

@shawnhankim shawnhankim left a 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.
Copy link
Contributor Author

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.

r.variables.refresh_token = "-";
r.return(302, r.variables.oidc_logout_redirect);
var queryParams = '';
if (r.variables.oidc_logout_extra_query_params) {
Copy link
Contributor Author

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!

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;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Revised it.

@@ -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";
Copy link
Contributor Author

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.

@@ -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";
Copy link
Contributor Author

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.

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

map $host $oidc_logout_extra_query_params {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Revised it!

# 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";;
Copy link
Contributor Author

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!

@shawnhankim shawnhankim force-pushed the 021-oidc-logout-endpoint branch 2 times, most recently from 69e28e7 to 92d267c Compare January 9, 2023 20:41
@shawnhankim shawnhankim changed the title integrate OIDC logout endpoint and custom query params Add OIDC end session endpoint and custom query params Jan 9, 2023
@shawnhankim shawnhankim force-pushed the 021-oidc-logout-endpoint branch 2 times, most recently from 6cc94b4 to b79661c Compare January 9, 2023 21:26
rename oidc_end_session_query_params

fix: typo
@shawnhankim shawnhankim force-pushed the 021-oidc-logout-endpoint branch from b79661c to fc05d36 Compare January 13, 2023 17:22
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