feat: access token & enhanced endpoints (/login, /userinfo, /logout) #62

shawnhankim · 2022-11-17T22:10:36Z

Issue Item:

feat: access token & enhanced endpoints (/login, /userinfo, /logout) #62

Description:

1. access token

Enhance the NJS Code to capture the access_token sent by the IdP.
Store the access_token in the k/v store as same as we store id_token and refresh_token

2. new endpoints

Add /userinfo endpoint:
- Add a map variable of $oidc_userinfo_endpoint as same as authz and token endpoints here (openid_connect_configuration.conf) .
- Expose /userinfo endpoint here(openid_connect.server_conf) in a location block of NGINX Plus to interact with IdP's userinfo_endpoint which is defined in the endpoint ofwell-known/openid-configuration.
- The nginx location block should proxy to the IdP’s userinfo_endpoint by adding access_token as a bearer token.
```
Authorization : Bearer <access_token>
```
- The response coming from IdP should be returned back to the caller as it is.
Expose /login endpoint:
- Expose the /login endpoint as a location block here (openid_connect.server_conf)
- Proxy it to the IdP's authorization_endpoint configured in the map variable of $oidc_authz_endpoint in (openid_connect_configuration.conf).
- This would outsource the login function to IdP as its configured.
Enhance /logout endpoint:
- Add a map variable of $oidc_end_session_endpoint as same as authz and token endpoints here (openid_connect_configuration.conf) .
- Proxy it to the IdP's end_session_endpoint to finish the session by IdP.
Enhance /_logout endpoint:
- Enhance /_logout endpoint which is a callback from IdP as a location block here (openid_connect.server_conf) to handle the following sequences.
  - 1. Redirected by IdP when IdP successfully finished the session.
  - 1. NGINX Plus: Clear session cookies.
  - 1. NGINX Plus: Redirect to either the landing page or the custom logout page by calling

3. add endpoints in configure.sh

IdP's userinfo endpoint
IdP's end session endpoint

Compatibility:

This PR does not block the existing customers as it just adds endpoints and features.

shawnhankim · 2022-11-17T23:46:43Z

@route443 : Per our discussions,

I have consolidated 3 PRs into 1 PR with 1 commit here.
I have separated the PR for doc.
I am going to separately provide a demo files which can be merged into a feature branch like demo once I have access to the branch, or if you create the branch. Until then, you can test this PR using this repo.

route443 · 2022-12-08T16:14:44Z

frontend.conf

@@ -1,3 +1,16 @@
+# -----------------------------------------------------------------------------#


In general, the current style does not imply the use of any ascii-frames. I'm not sure if this improves the readability of the config files, so I would remove it.

Sure, removed it!

route443 · 2022-12-08T16:17:28Z

frontend.conf

+
+# -----------------------------------------------------------------------------#
+#                                                                              #
+# 1. Basic Example: Landing page starts OIDC workflow w/o login/logout button. #


This description is not entirely correct, we still provide all logout-related locations in the openid_connect.server_conf, so there is nothing stopping you from assigning the appropriate logout event to your application.

Addressed it. Thx!

route443 · 2022-12-08T16:18:22Z

frontend.conf

@@ -33,4 +46,106 @@ server {
    }
 }

+# -----------------------------------------------------------------------------#
+#                                                                              #
+# 2. Advanced Example: Landing page, login/logout button to handle OIDC kflow  #


I think it's better to have one example with commented out directives than to have multiple examples which can lead to additional complexity or potential security risks. More details below.

Great comment and idea to have one example!

Consolidated it as one example!

route443 · 2022-12-08T16:19:12Z

frontend.conf

+#  - Landing page shows 'login' button                                         #
+#  - 'login' button calls `/login` endpoint to start OIDC flow by validating 
+#    'id_token' w/ IdP's JWK.  #
+#  - Landing page calls `/userinfo` to show user info using 'access_token`.    #


The Userinfo Claims are needed by the Client and is not meant to be provided to the User Agent. Why would we disclose this information to the end user?

Reason 1: Product Requirements
You could find the following product requirements from NGINX Management Suite(NMS) - API Connectivity Manager(ACM).

Capture the /userinfo endpoint from the IDP’s openid-config endpoint in a map variable as same as authz and token endpoints in the the openid_connect_configuration.conf

The nginx location block to expose /userinfo in the openid_connect.server_conf.

The nginx location block should proxy to the IdP’s /userinfo endpoint by adding access_token as bearer token - Authorization : Bearer <access_token> .

The response coming from IdP should be returned back to the caller as it is.

The details are shared into your Slack.
Here is another document of Dev Portal of NMS.

Reason 2: Common Use Case

It is used when User Agent needs to show the user profile in detail such as picture, phone and prefered_name beyond name/email.

The fields are configurable and flexible by IdP based on the customer requirements. So, the PM of the above NMS-ACM wanted to proxy pass to the AS, and we thought that it would be common use case.

So many web sites can show the user profile without storing the Personal Identifiable Information (PII) in their system as it is managed by IdP.

route443 · 2022-12-08T16:20:48Z

frontend.conf

+#    'id_token' w/ IdP's JWK.  #
+#  - Landing page calls `/userinfo` to show user info using 'access_token`.    #
+#  - 'logout' button to be finished OIDC session by IdP.                       #
+#  - API authorization by validating `access_token` w/ IdP's JWK               #


The approach to the validation process as well as the need to validate the Access Token on the Client is a big question. (more details below)

Removed this comment, and modified the configuration!

route443 · 2022-12-08T16:21:28Z

frontend.conf

+# -----------------------------------------------------------------------------#
+
+#
+# Upstream server for proxing to the frontend site.


It seems that this description is an artifact, since it refers to some kind of bundle that is not presented in this PR. Please, correct me if I am wrong. Plus, I wouldn't use any links to third-party resources/projects without a very good reason.

Yes, you are right!

The purpose is for customers to easily follow the example of demo which we are going to create the feature branch (e.g, demo) instead of the 3rd-party link as discussed.

Given the kind of demo, customers can quickly set up IdP, configure NGINX Plus, and develop their application using NGINX Plus OIDC.

Therefore, I just added the link as the demo branch is not created yet.

However, I will remove the link as for now.

Let me know once you create a feature branch such as demo so that I can add it to there.

Removed it!

route443 · 2022-12-08T16:21:53Z

frontend.conf

+}
+
+#
+# Upstream sample for proxing to the backend API server.


The previous remark is true for this example as well.

Removed it!

route443

As you can see, we are not using Conventional Commits for this project. So I wouldn't use this approach unnecessarily.

route443 · 2022-12-08T16:30:28Z

frontend.conf

+server {
+    # Enable when debugging is needed.
+    error_log  /var/log/nginx/error.log  debug; # Reduce severity level as required
+    access_log /var/log/nginx/access.log main;


Typo, looks like it should be main_jwt.

Thanks for finding it.

route443 · 2022-12-08T16:31:10Z

frontend.conf

+    server_name nginx.oidc.test;
+
+    # Replace the following files with your certificate.
+    ssl_certificate     /etc/ssl/nginx/nginx-repo.crt;


This configuration will fail validation if these files are not present on the file system. Plus, based on the path, you use the public and private keys from the NGINX Plus repo. It looks at least strange.

Thanks for your opinion. Per the request of one example, I will remove this to just guide to use SSL/TLS in production using the existing comment.

Removed it!

route443 · 2022-12-08T16:32:03Z

frontend.conf

+    #  - The site is protected with OpenID Connect(OIDC) by calling the API 
+    #    endpoint of `/login` when users click 'login' button.
+    #
+    location / {


As far as I understand the proposed configuration, in this case we protect only two location using OIDC auth, while everything else except /login and /api is unprotected. It seems to me that examples should be as safe as possible by default.

Thanks for your opinion.

Per the requirement of NMS-ACM Developer Portal:

The landing page just show default contents should not be protected by OIDC using public APIs before login.

After logged-in, there are more information that can be shown using partner/private APIs that are protected.

There are also many services that need to be shown as a landing page from the / location that is not protected by OIDC which means before login.

In case of this, / and public APIs are not required to be protected, and private/public APIs need to be protected.

I will revise the example and show it to you once I am ready. In the meantime, feel free to let me know if you have any better idea or comment.

Revised the example.

route443 · 2022-12-08T16:32:50Z

frontend.conf

+    #
+    location / {
+        proxy_pass http://my_frontend_site;
+        access_log /var/log/nginx/access.log main_jwt;


We use the access_log directive with the same value both at the server level and at the individual location level, this looks redundant. However, we need to remember that the $jwt_claim_sub variable can be evaluated as an empty string or as a different value (if the sub in the tokens does not match) depending on the location. This may lead to additional questions and issues.

Sure! Consolidated it as one example per the request!

route443 · 2022-12-08T16:34:02Z

frontend.conf

+    #
+    # Backend API example to interact with proxied backend service:
+    #
+    #  - This API resource is protected by access token which is received by IdP


I don't quite understand the meaning of this sentence. As far as I know, the Access Token is obtained from the AS.

Yes, the access token is obtained from the AS. I will revise the comment.

Revised it!

route443 · 2022-12-08T16:38:05Z

frontend.conf

+    #      endpoints (like /userinfo) as well as customer's endpoints.
+    #    + But Azure AD generate two types of access token for API authorization
+    #      of Microsoft graph API endpoints and customers' endpoints.
+    #    + Therefore, we recommend that you use $session_jwt for Azure AD and


I don’t think that we should make such recommendations based on all of the above.

Sure, I will remove it.

route443 · 2022-12-08T16:39:50Z

frontend.conf

+    #      OneLogin, Ping Identity, etc as for now.
+    #
+    location /v1/api/example {
+        auth_jwt "" token=$access_token;      # Use $session_jwt for Azure AD


As I noted earlier, we cannot replace the use of an ID Token with an Access Token. The second important fact is the embedded variables that we expose based on headers/claims, each request updates these variables. In other words, if there is no token => there are no variables, there is an Access Token, there are variables only from it, without taking into account the fact that we already process the ID Token in another location. This is not always obvious and must be handled in a special way.

I also forgot to note. That if we didn't find an Access Token in the tokenset response from the AS? We must properly handle this case, and only in cases where it is necessary.

Good point of tokenset response!

Fixed as the previous my reply to your comments!

route443 · 2022-12-08T16:46:15Z

openid_connect.server_conf

@@ -66,6 +69,70 @@
        error_page 500 502 504 @oidc_error;
    }

+    #
+    # User information endpoint for the following purposes:
+    # - Browser to periodically check if your are signed-in based on status code.


How can the browser be forced to do this? Using custom JS code? What happens if we get a 401 Unauthorized, what does that mean? Access Token not valid? Whether it can be updated using a Refresh Token? I'm not sure that such tasks should be solved through the User Agent.

When we get 401 from /userinfo or customer's endpoints because access token isn't valid, User Agent can call /login to relogin or refresh token based on the status of IdP's token expiration.

We need to separately discuss this item which I wanted to talk to you. I will show you the use case later.

Hence, I will remove this comment to avoid confusion as for now.

Removed this.

route443 · 2022-12-08T16:46:59Z

openid_connect.server_conf

+    #
+    # User information endpoint for the following purposes:
+    # - Browser to periodically check if your are signed-in based on status code.
+    # - Browser to show the signed-in user information.


What's the use case? What if I don't want this information to be exposed to the end user?

Use Case:

It is used when customer system needs to show user profile (e.g., photo, last name, first name, email, address, etc.) by requesting to IdP. So customers can show them and don't need to store PII into their system.

The NGINX Management Suite - API Connectivity Manager(NMS-ACM) Dev Portal uses prefered_username and name, and it is going to use email later.

What if I don't want this information to be exposed to the end user?

The infrastructure owner of customer is responsible for and can configure the fields when setting up user pool in the IdP.

So, it should be configurable not only in NGINX conf but also in IdP by the customer.

The $oidc_userinfo_response_data is empty as default value, then the information is not exposed for the scenario if customers don't want this info to be exposed to the end user.

route443 · 2022-12-08T16:50:05Z

openid_connect.server_conf

+    # - Browser to show the signed-in user information.
+    # - https://openid.net/specs/openid-connect-core-1_0.html#UserInfo
+    #
+    location = /userinfo {


We cannot say that we support the Userinfo endpoint until such time as:

These requests must be made through the back channel communication (client/RP/nginx <-> Userinfo).

We need to parse JSON response.

Perform response validation in accordance with the requirement.

Save keys and values in a keyval.
At the same time, we are not required to validate the Access Token to perform this request.

Yeah, I originally thought and tried your approach.

But, the requirement of NMS-ACM was to proxy and return the IdP's response itself to the SPA back then.

Yeah, it is good point of access token validation too. If we validate it using local JWK file in the backend, and if it isn't invalid, then it reduces latency. However, if it is valid, then the IdP validates it using Bearer token again.

I will try achieving both requirements between NGINX Plus and NMS-ACM. Afterwards, you can review, and I will revise more things. Thanks!

For the use case where SPA wants to show user profile in detail like www.example.com "sub,name,preferred_username,given_name,family_name,email,photo", I have extracted only configured claims in $oidc_userinfo_response_data and return the JSON rather than directly returning all claims from the OP.

So, if $oidc_userinfo_response_data is empty as default value, then the information is not exposed.

Let me know if you have other thoughts.

shawnhankim · 2022-12-11T01:36:11Z

Thanks @route443 for your time and great review. 👍 I will address them next week.

route443 · 2022-12-12T20:45:49Z

openid_connect.js

+// Get query parameter of ID token after sucessful login:
+//
+// - For the variable of `returnTokenToClientOnLogin` of the APIM, this config


At least the first time you mention a new abbreviation, it's better to unfold it. Not everyone knows what APIM is. But it's best not to get attached to any proprietary 3rd party solutions at all and use a more general terminology like SPA, User Agent, etc.

Removed it!

route443 · 2022-12-12T21:53:33Z

openid_connect.js

+//   is only effective for /login endpoint. By default, our implementation MUST
+//   not return any token back to the client app.
+// - If its configured it can send id_token in the request uri as 


Can you explain the use case a bit deep please? Why do you want to pass id_token to the User Agent?

Per our separate discussion, I have removed it as this moment although it was required to the NGINX Management Suite - API Connectivity Manager (NMS-ACM). Let's discuss this later per the requirements between NGINX Plus PM and NMS-ACM PM.

route443 · 2022-12-12T22:02:20Z

openid_connect.js

+//
+function getIDTokenArgsAfterLogin(r) {
+    if (r.variables.return_token_to_client_on_login == 'id_token') {


Are you planning to have more than one possible value, like access_token? Also, be more careful with the terminology, I think that the Client is NGINX in OIDC flow and you are talking about User Agent?

Per our separate discussion, I have removed it as this moment although it was required to the NGINX Management Suite - API Connectivity Manager (NMS-ACM). Let's discuss this later per the requirements between NGINX Plus PM and NMS-ACM PM.

route443 · 2022-12-13T03:20:13Z

openid_connect.js

+//
+// Redirect URI after logging in the IDP.
+function redirectPostLogin(r) {


Since we already have the logic of redirecting User Agent to the request_uri via the auth_redir cookie, which does not imply a redirect after successful authentication to the landing page. I'd recommend introducing a new variable / config parameter via openid_connect_configuration.conf. That is, we could introduce new parameter/variable like $oidc_landing_page or $oidc_relay_state (by analogy with SAML), and if it is defined, then redirect to it, if not, then to request_uri.
In any case, the current approach looks strange as it allows you to redirect User Agent to the root relative to redirect_base with or without a token.

Introduced $oidc_landing_page. Thanks!

route443 · 2022-12-13T04:49:47Z

openid_connect.js

+// - https://openid.net/specs/openid-connect-rpinitiated-1_0.html#RedirectionAfterLogout
+//
+function v2logout(r) {


I'd prefer not to produce unnecessary entities and combine this logic with logout() function. At least from the description it is not entirely clear why we introduced a new logout function.
Here we need to think carefully about how not to break the previous functionality, but at least we can use $oidc_end_session_endpoint as a hint for ourselves.
Plus, in addition to id_token_hint, we need to think about adding other parameters from the specification.

Combined the logic with logout() function that I also wanted. Thanks!

The custom parameters are considered with the next PR. But they are added in this PR.

route443 · 2022-12-13T05:30:41Z

openid_connect.server_conf

+    }
+
+    #
+    # V2 Logout: The following features are added in the NGINX R29.


There is nothing specific to NGINX Plus R29 here, I think we should not mislead anyone.

Sure, removed it!

route443 · 2022-12-13T05:32:22Z

openid_connect.server_conf

+    # - Sample logout page for your OIDC simulation.
+    # - TODO: Custom logout parameters will be separately added.
+    #
+    location = /v2/logout {


As I mentioned earlier, we should merge this location with /logout. In other words, the location for initializing the logout process must be unified.

route443 · 2022-12-13T05:37:37Z

openid_connect.server_conf

+        js_content oidc.v2logout;
+    }
+
+    location = /v2/_logout {


The same is true for /v2/_logout as well. We need to think about unification with /_logout.

route443 · 2022-12-13T05:39:11Z

openid_connect.server_conf

+    }
+
+    #
+    # V1 Logout (NGINX R28): 


It's not worth mentioning the NGINX Plus version here either.

Removed it!

route443 · 2022-12-13T05:52:02Z

openid_connect_configuration.conf

+    # default https://www.nginx.com;
+}
+
+map $host $return_token_to_client_on_login {


Since the ID Token is stored in a variable, it can also be returned from the app-specific config. Not sure if we need this option.

Removed this per discussion as the requirement of NGINX Plus although it is the requirements from the NGINX Management Suite (API Connectivity Manager).

Let's consider this later when all products are required this feature.

shawnhankim · 2022-12-18T06:08:13Z

@route443

Thanks for your review in detail.
Addressed most of comments, and added some context.
Feel free to let me know if you need more changes, or if I miss anything.
FYI: I will squash the commits after your review.

shawnhankim · 2022-12-18T06:11:17Z

@route443
FYI: If you want to test the changes, you could either find the following link for your convenience or use your test environment:

https://github.com/nginx-openid-connect/nginx-oidc-examples/tree/main/001-oidc-local-test

**Issue Item:** - nginxinc#62 **Description:** **1. access token** - Enhance the NJS Code to capture the `access_token` sent by the IdP. - Store the `access_token` in the k/v store as same as we store `id_token` and `refresh_token` **2. new endpoints** - Add `/userinfo` endpoint: - Add a map variable of `$oidc_userinfo_endpoint` as same as authz and token endpoints here (`openid_connect_configuration.conf`) . - Expose `/userinfo` endpoint here(`openid_connect.server_conf`) in a location block of NGINX Plus to interact with IdP's `userinfo_endpoint` which is defined in the endpoint of`well-known/openid-configuration`. - The nginx location block should proxy to the IdP’s `userinfo_endpoint` by adding `access_token` as a bearer token. ``` Authorization : Bearer <access_token> ``` - The response coming from IdP should be returned back to the caller as it is. - Expose `/login` endpoint: - Expose the `/login` endpoint as a location block here (`openid_connect.server_conf`) - Proxy it to the IdP's `authorization_endpoint` configured in the map variable of `$oidc_authz_endpoint` in (`openid_connect_configuration.conf`). - This would outsource the login function to IdP as its configured. - Enhance `/logout` endpoint: - Add a map variable of `$oidc_end_session_endpoint` as same as authz and token endpoints here (`openid_connect_configuration.conf`) . - Proxy it to the IdP's `end_session_endpoint` to finish the session by IdP. - Enhance `/_logout` endpoint: - Enhance `/_logout` endpoint which is a callback from IdP as a location block here (`openid_connect.server_conf`) to handle the following sequences. - 1. Redirected by IdP when IdP successfully finished the session. - 2. NGINX Plus: Clear session cookies. - 3. NGINX Plus: Redirect to either the landing page or the custom logout page by calling **3. add endpoints in `configure.sh`** - IdP's userinfo endpoint - IdP's end session endpoint **Compatibility:** - This PR does not block the existing customers as it just adds endpoints and features.

shawnhankim force-pushed the 009-access-token-new-endpoints branch 2 times, most recently from a5b8863 to db0ec31 Compare November 17, 2022 22:22

shawnhankim force-pushed the 009-access-token-new-endpoints branch from db0ec31 to de4bab2 Compare November 17, 2022 22:43

shawnhankim mentioned this pull request Nov 17, 2022

[WIP] docs: access token new endpoints(/login, /userinfo, /v2/logout) #64

Open

route443 reviewed Dec 8, 2022

View reviewed changes

route443 requested changes Dec 8, 2022

View reviewed changes

shawnhankim mentioned this pull request Dec 12, 2022

Add access token support nginx-openid-connect/nginx-oidc-core-v1#17

Open

route443 reviewed Dec 13, 2022

View reviewed changes

shawnhankim force-pushed the 009-access-token-new-endpoints branch from af1aadd to c8e3e63 Compare December 18, 2022 06:02

shawnhankim changed the title ~~feat: access token & new endpoints (/login, /userinfo, /v2/logout)~~ feat: access token & enhanced endpoints (/login, /userinfo, /logout) Dec 18, 2022

shawnhankim requested a review from route443 December 18, 2022 06:05

shawnhankim force-pushed the 009-access-token-new-endpoints branch from c8e3e63 to fdc447c Compare December 19, 2022 07:45

shawnhankim force-pushed the 009-access-token-new-endpoints branch from fdc447c to 2a31b35 Compare December 19, 2022 07:54

shawnhankim force-pushed the 009-access-token-new-endpoints branch from 2a31b35 to 248ef07 Compare December 19, 2022 08:09

shawnhankim closed this Dec 19, 2022

shawnhankim force-pushed the 009-access-token-new-endpoints branch from 248ef07 to 6ad8ec6 Compare December 19, 2022 08:10

		@@ -1,3 +1,16 @@
		# -----------------------------------------------------------------------------#

feat: access token & enhanced endpoints (/login, /userinfo, /logout) #62

feat: access token & enhanced endpoints (/login, /userinfo, /logout) #62

Conversation

shawnhankim commented Nov 17, 2022 • edited

shawnhankim commented Nov 17, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

route443 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shawnhankim Dec 12, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shawnhankim Dec 18, 2022 • edited

Choose a reason for hiding this comment

shawnhankim commented Dec 11, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shawnhankim commented Dec 18, 2022

shawnhankim commented Dec 18, 2022

shawnhankim commented Nov 17, 2022 •

edited

shawnhankim Dec 12, 2022 •

edited

shawnhankim Dec 18, 2022 •

edited