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

feat: Add access token support in the OIDC #3474

Merged

Conversation

shawnhankim
Copy link
Contributor

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 shawnhankim requested a review from a team as a code owner January 23, 2023 21:39
@github-actions github-actions bot added documentation Pull requests/issues for documentation helm_chart Pull requests that update the Helm Chart labels Jan 23, 2023
@shawnhankim 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 shaun-nx self-assigned this Jan 25, 2023
@shawnhankim 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 shawnhankim changed the title Add access token support in the OIDC feat: Add access token support in the OIDC Jan 28, 2023
@shawnhankim 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
Copy link
Contributor Author

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

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

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

shawnhankim commented Feb 14, 2023

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

@brianehlert brianehlert added the backlog Pull requests/issues that are backlog items label Feb 23, 2023
@lucacome lucacome linked an issue Mar 9, 2023 that may be closed by this pull request
internal/configs/oidc/openid_connect.js Outdated Show resolved Hide resolved
internal/configs/version2/nginx-plus.virtualserver.tmpl Outdated Show resolved Hide resolved
internal/configs/version2/nginx-plus.virtualserver.tmpl Outdated Show resolved Hide resolved
@shawnhankim 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
Copy link
Member

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

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

@shawnhankim
Copy link
Contributor Author

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

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

@shawnhankim shawnhankim reopened this Mar 14, 2023
Copy link
Member

@lucacome lucacome left a comment

Choose a reason for hiding this comment

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

Thanks @shawnhankim !

@lucacome lucacome removed the backlog Pull requests/issues that are backlog items label Mar 14, 2023
@lucacome lucacome added this to the v3.1.0 milestone Mar 14, 2023
Copy link
Member

@ciarams87 ciarams87 left a comment

Choose a reason for hiding this comment

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

LGTM, thanks so much @shawnhankim!

@codecov
Copy link

codecov bot commented Mar 17, 2023

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

@lucacome lucacome merged commit 4dfccbe into nginxinc:main Mar 17, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Pull requests/issues for documentation helm_chart Pull requests that update the Helm Chart
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

OIDC: option to pass access token as authorization header to upstream
6 participants