-
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 landing page for nginx to redirect after successful OIDC login #74
base: main
Are you sure you want to change the base?
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.
frontend.conf
Outdated
@@ -31,6 +31,20 @@ server { | |||
|
|||
access_log /var/log/nginx/access.log main_jwt; | |||
} | |||
|
|||
location = /login { |
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 is not entirely clear why we need this example.
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 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 intoopenid_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.
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 list of reasons is not very convincing.
- 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. - The
openid_connect.server_conf
file contains OIDC functional locations, that is, by default, the user does not need to change anything in it. - 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.
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.
-
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 forproxy_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.
- SPA landing page to show default data using public APIs without login:
-
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.
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
/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.
108564c
to
fe88c96
Compare
a72793e
to
68a0e86
Compare
remove login endpoint
Issue:
Description:
$oidc_landing_page
to determine where to send browser after successful login from the IdP.