feat: Add access token support in the OIDC by shawnhankim · Pull Request #3474 · nginx/kubernetes-ingress

shawnhankim · 2023-01-23T21:39:26Z

Proposed changes

The current OIDC only support ID token for user authentication and does not support access token for the API authorization yet. The feature of access token has been recently added in the NGINX Plus OIDC repo, and this PR is to synchronize the feature into NGINX Ingress Controller.
This change is to add $access_token support for authorizing protected resources in the OpenID Connect (OIDC).
This PR is also to resolve the issue OIDC: option to pass access token as authorization header to upstream #2635 to enable OIDC option (accessTokenEnable) to pass $access_token as authorization header to upstream.

Checklist

Before creating a PR, run through this checklist and mark each as complete.

I have read the CONTRIBUTING doc
I have added tests that prove my fix is effective or that my feature works
I have checked that all unit tests pass after adding my changes
I have updated necessary documentation
I have rebased my branch onto main
I will ensure my PR is targeting the main branch and pulling from my branch from my own fork

shawnhankim · 2023-02-12T13:08:33Z

@brianehlert : This PR is one of OIDC enhancements which are discussed in the Slack channel.
@shaun-nx, @brianehlert : I would appreciate it if you could review this PR. Please let me know if I miss anything.

brianehlert · 2023-02-14T17:51:23Z

@shawnhankim
This appears to support passing the access token to the upstream / backend. Is that correct?
When this project takes updates from the OIDC repository, we try to take all the changes and therefore maintain the OIDC reference implementation as a source of truth, and not that this is some random fork. Does this PR include all enhancements? (Whether exposed or not)

shawnhankim · 2023-02-14T22:42:53Z

@brianehlert

This appears to support passing the access token to the upstream / backend. Is that correct?

Yes, it is correct.
So, we can answer to the question of ID token (for user authentication) / access token (for API/protected-resource authorization) such as this kind of article based on this additional feature of access token as we already have the session_jwt which is the id_token.

shawnhankim · 2023-02-14T22:53:03Z

@brianehlert

When this project takes updates from the OIDC repository, we try to take all the changes and therefore maintain the OIDC reference implementation as a source of truth, and not that this is some random fork. Does this PR include all enhancements? (Whether exposed or not)

This PR is to take all the changes from the latest version of OIDC reference implementation which is the source of truth.
The rest of enhancements are being reviewed by NGINX Plus Team. Those PRs are going to be separately synchronized here once they are merged in the source repo of truth based on N+ team's schedule.

Please let me know if I miss anything about your question.

lucacome · 2023-03-14T06:40:48Z

@shawnhankim can you rebase the PR with main and run make update-crds?

shawnhankim · 2023-03-14T12:10:54Z

shawnhankim · 2023-03-14T12:12:41Z

@shawnhankim can you rebase the PR with main and run make update-crds?

Thanks @lucacome for this feedback and letting me know. Addressed it.

lucacome

Thanks @shawnhankim !

ciarams87

LGTM, thanks so much @shawnhankim!

fix: virtual server template for OIDC.AccessTokenEnable

fix: compare OIDC option to enable access token and intercepting error fix: revert

codecov · 2023-03-17T04:38:38Z

Codecov Report

Merging #3474 (e6af55d) into main (2b40efe) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##             main    #3474   +/-   ##
=======================================
  Coverage   52.23%   52.23%           
=======================================
  Files          59       59           
  Lines       16877    16878    +1     
=======================================
+ Hits         8816     8817    +1     
  Misses       7766     7766           
  Partials      295      295

Impacted Files	Coverage Δ
internal/configs/version2/http.go	`0.00% <ø> (ø)`
internal/configs/virtualserver.go	`95.08% <100.00%> (+<0.01%)`	⬆️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

shawnhankim requested a review from a team as a code owner January 23, 2023 21:39

shawnhankim requested review from haywoodsh, jjngx and lucacome January 23, 2023 21:39

github-actions Bot added documentation Pull requests/issues for documentation helm_chart Pull requests that update the Helm Chart labels Jan 23, 2023

shawnhankim force-pushed the 001-add-access-token-support branch 4 times, most recently from 3e72a42 to 1e07a19 Compare January 24, 2023 18:06

shaun-nx self-assigned this Jan 25, 2023

shawnhankim force-pushed the 001-add-access-token-support branch 7 times, most recently from e2cf2de to d86cf30 Compare January 28, 2023 03:56

shawnhankim commented Jan 28, 2023

View reviewed changes

Comment thread docs/content/configuration/policy-resource.md

shawnhankim force-pushed the 001-add-access-token-support branch from 252fa10 to 731394d Compare January 28, 2023 04:31

shawnhankim changed the title ~~Add access token support in the OIDC~~ feat: Add access token support in the OIDC Jan 28, 2023

shawnhankim force-pushed the 001-add-access-token-support branch 2 times, most recently from af2c3fc to bf5fe17 Compare February 2, 2023 02:51

shawnhankim force-pushed the 001-add-access-token-support branch from bf5fe17 to e30a4b6 Compare February 12, 2023 13:05

shawnhankim force-pushed the 001-add-access-token-support branch from e30a4b6 to 5a3f013 Compare February 14, 2023 22:17

shawnhankim force-pushed the 001-add-access-token-support branch from 38ccd50 to 800590d Compare February 17, 2023 11:05

brianehlert added the backlog Pull requests/issues that are backlog items label Feb 23, 2023

tomasohaodha assigned lucacome Mar 9, 2023

lucacome unassigned shaun-nx Mar 9, 2023

lucacome linked an issue Mar 9, 2023 that may be closed by this pull request

OIDC: option to pass access token as authorization header to upstream #2635

Closed

lucacome suggested changes Mar 10, 2023

View reviewed changes

Comment thread internal/configs/oidc/openid_connect.js Outdated

Comment thread internal/configs/version2/nginx-plus.virtualserver.tmpl Outdated

Comment thread internal/configs/version2/nginx-plus.virtualserver.tmpl Outdated

shawnhankim force-pushed the 001-add-access-token-support branch 2 times, most recently from eebcd9e to ab03765 Compare March 10, 2023 05:24

lucacome reviewed Mar 14, 2023

View reviewed changes

Comment thread docs/content/configuration/policy-resource.md

Comment thread internal/configs/version2/nginx-plus.virtualserver.tmpl Outdated

shawnhankim force-pushed the 001-add-access-token-support branch 2 times, most recently from 30e4042 to 353d072 Compare March 14, 2023 12:10

shawnhankim closed this Mar 14, 2023

shawnhankim reopened this Mar 14, 2023

lucacome approved these changes Mar 14, 2023

View reviewed changes

lucacome removed the backlog Pull requests/issues that are backlog items label Mar 14, 2023

lucacome added this to the v3.1.0 milestone Mar 14, 2023

shaun-nx approved these changes Mar 16, 2023

View reviewed changes

tomasohaodha requested a review from vepatel March 16, 2023 10:30

ciarams87 approved these changes Mar 16, 2023

View reviewed changes

lucacome suggested changes Mar 17, 2023

View reviewed changes

Comment thread internal/configs/version2/nginx-plus.virtualserver.tmpl Outdated

shawnhankim added 4 commits March 16, 2023 19:41

feat: add access token support in the OIDC

5c9f9ed

fix: virtual server template for OIDC.AccessTokenEnable

feat: add intercept error enable and fix checking OIDC config data

a9ac466

fix: compare OIDC option to enable access token and intercepting error fix: revert

fix: remove intercept error from access token feature and update CRD

e5982ad

fix: move the position of access token enable option

e6af55d

shawnhankim force-pushed the 001-add-access-token-support branch from 9fd54f5 to e6af55d Compare March 17, 2023 03:26

lucacome approved these changes Mar 17, 2023

View reviewed changes

lucacome merged commit 4dfccbe into nginx:main Mar 17, 2023

Uh oh!

Conversation

shawnhankim commented Jan 23, 2023

Proposed changes

Checklist

Uh oh!

Uh oh!

shawnhankim commented Feb 12, 2023

Uh oh!

brianehlert commented Feb 14, 2023

Uh oh!

shawnhankim commented Feb 14, 2023

Uh oh!

shawnhankim commented Feb 14, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

lucacome commented Mar 14, 2023

Uh oh!

shawnhankim commented Mar 14, 2023

Uh oh!

shawnhankim commented Mar 14, 2023

Uh oh!

lucacome left a comment

Choose a reason for hiding this comment

Uh oh!

ciarams87 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

codecov Bot commented Mar 17, 2023

Codecov Report

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

shawnhankim commented Feb 14, 2023 •

edited

Loading