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 landing page for nginx to redirect after successful OIDC login #74

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:

Description:

  • Added a map variable of $oidc_landing_page to determine where to send browser after successful login from the IdP.

@shawnhankim
Copy link
Contributor Author

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

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.

frontend.conf Outdated
@@ -31,6 +31,20 @@ server {

access_log /var/log/nginx/access.log main_jwt;
}

location = /login {
Copy link
Contributor

Choose a reason for hiding this comment

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

It is not entirely clear why we need this example.

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.

This endpoint requirement was originally requested by PMs of NGINX Management Suite - API Connectivity Manager (a.k.a. NMS-ACM) as follows. In the meantime, I have added another common purpose.

Product Requirements of NMS-ACM:

  • Problem: Current NJS implementation doesn’t have login endpoints for client apps (SPA) to interact with. Client Apps require /login function as part of relying party when user clicks on login button.
  • Change Required: Expose the /login nginx location block here (openid_connect.server_conf) & proxy it to existing authz endpoint configured in the map variable(openid_connect_configuration.conf) of $oidc_authz_endpoint. This would outsource the login function to IdP as its configured.

Reason why we need for the general purpose regardless of NMS-ACM requirement:

  • The current example only shows the scenario where / location always need to start with user authentication.
  • There are many web sites (which are proxied from / location) that do not have to be started with user authentication by using IdP's login GUI.
  • For those cases, this endpoint can be used by the SPA when users click on login button.

To Sum Up:

  • To prevent confusion with an additional example, I have removed this example in this frontend.conf. The endpoint has been moved into openid_connect_configuration.conf that can be used by customers based on their requirements instead.
  • Let me know if this reason is not enough for you.

Copy link
Contributor

Choose a reason for hiding this comment

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

The list of reasons is not very convincing.

  1. JWT authentication in NGINX works in such a way that if you specify /login location, then only it will be protected. The principle of "explicit allow" is a security principle that states that only actions or activities that have been explicitly permitted should be allowed to take place. This means that anything that is not specifically allowed is prohibited.
  2. The openid_connect.server_conf file contains OIDC functional locations, that is, by default, the user does not need to change anything in it.
  3. We work as a proxy, that is, the more locations we have, the less predictable the end application can be, simply because locations can overlap.

Your /login example is app specific, that is, nothing prevents you from creating the configuration that NMS-ACM need, we, in turn, pursue the goal of being as predictable (simple) as possible.
We don't make a Swiss Army knife for all occasions, we provide a simple solution that should meet the standard OIDC requirements and not be overly complicated.

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.

  • FYI. This feature was originally requested by the PM in the team who managed this OIDC repo before you started working on this repo. Hence, it was firstly implemented into the NGINX Management Suite - API Connectivity Manager(NMS-ACM), and the PM's plan was to donate it here to be used by more customers.

  • The openid_connect.server_conf already have exceptions (resolver, /_jwks_uri and /_token locations for proxy_set_header Accept-Encoding "gzip") despite no changes are usually required. Hence, /login is added for customers who want to use the endpoint from the login button in UI per request from one of PMs in the OIDC mgmt. team although you already mentioned to protect resources.

  • In the meantime, I understand what you pointed out. We may need to find another repo to add more app specific solutions if the purpose of this repo is just to provide a simple reference implementation.

  • Otherwise, customers need more time to find how to implement the followings:

    • SPA landing page to show default data using public APIs without login: / should not start OIDC flow.
    • SPA to start user authentication via OIDC flow when clicking login button.
    • SPA (JS) to call private APIs using Fetch and restart OIDC flow when invalid token. There is no guide for this here as general JS fetch don't work with OIDC authZ endpoint.
    • So we need to provide much more convenient and easy solutions to customers to influence this OIDC solution over the world.
  • Let me think of how to provide the simpler solution of the feature here as I can understand what this repo wants. I appreciate your thoughts.

Copy link
Contributor Author

@shawnhankim shawnhankim Jan 9, 2023

Choose a reason for hiding this comment

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

@route443 :

  • The /login endpoint has been removed as this repo is only to provide reference implementation for a simple proxy. The application level reference implementation (that contains /login) will be separately considered in other repo after discussing with the PM of NGINX Management Suite as this repo is only for simple implementation for a proxy.
  • Hence, the $oidc_landing_page is only kept as I like your suggestion of variable name.
  • Feel free to let me know if you have other thoughts.

openid_connect.js Show resolved Hide resolved
openid_connect_configuration.conf Outdated Show resolved Hide resolved
@shawnhankim shawnhankim changed the title feat: add login endpoint Add OIDC landing page and login endpoint Dec 30, 2022
@shawnhankim shawnhankim force-pushed the 022-login branch 2 times, most recently from a72793e to 68a0e86 Compare December 30, 2022 21:39
@shawnhankim shawnhankim changed the title Add OIDC landing page and login endpoint Add OIDC landing page for nginx to redirect after successful OIDC login Jan 9, 2023
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