Skip to content

Split JWT verification from forwarding logic.#1754

Closed
nickrmc83 wants to merge 2 commits intoistio:masterfrom
ThalesIgnite:master
Closed

Split JWT verification from forwarding logic.#1754
nickrmc83 wants to merge 2 commits intoistio:masterfrom
ThalesIgnite:master

Conversation

@nickrmc83
Copy link

What this PR does / why we need it:

This is the first PR in a set that will slowly introduce OpenID Connect end-user authentication and session management.
Splitting logic previously in jwt_authenticator into 2 parts.

  • jwt_authenticator just contains logic for verifying/authenticating JWTs
  • JWT forwarding logic has been moved into jwt_auth filter.

The changes enable greater re-usability of the JWT verification code across filters.
All tests updated and passing.

Special notes for your reviewer:

Envoy related PR: envoyproxy/envoy#3428
Design discussions:

Release note:

Splitting logic previously in jwt_authenticator into 2 parts.
- jwt_authenticator just contains logic for verifying/authenticating JWTs
- JWT forwarding logic has been moved into jwt_auth filter.

The changes enable greater re-usability of the JWT verification code across filters.
All tests updated and passing.
@googlebot
Copy link
Collaborator

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here (e.g. I signed it!) and we'll verify it.


What to do if you already signed the CLA

Individual signers
Corporate signers

@istio-testing istio-testing requested review from lizan and qiwzhang May 17, 2018 21:50
@googlebot googlebot added the cla: no Set by the Google CLA bot to indicate the author of a PR has not signed the Google CLA. label May 17, 2018
@istio-testing
Copy link
Collaborator

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: nickrmc83
To fully approve this pull request, please assign additional approvers.
We suggest the following additional approver: lizan

Assign the PR to them by writing /assign @lizan in a comment when ready.

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@istio-testing
Copy link
Collaborator

Hi @nickrmc83. Thanks for your PR.

I'm waiting for a istio member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

I understand the commands that are listed here.

Details

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@qiwzhang
Copy link
Contributor

@nickrmc83 I am in the middle of porting jwt_authn code from Isito/proxy to Envoy.

Here is: envoyproxy/envoy#3339

The jwt_authn code in istio/proxy is frozen now.

I strongly suggest you make your change after I done the porting.

If you don't want to wait, you can create a private branch in istio/proxy to submit your change. We may not have time to review it since it is in the private branch.

@nickrmc83
Copy link
Author

nickrmc83 commented May 22, 2018

@qiwzhang I'll wait until you've finished porting and then migrate the changes directly to envoy. I'll leave this PR open for the time being in case anyone wants to give some general feedback as what will appear in envoy will closely resemble these changes.

Additionally if there's anything I can do to help envoyproxy/envoy#3339 along let me know.

@istio-testing
Copy link
Collaborator

@nickrmc83: PR needs rebase.

Details

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@istio-testing istio-testing added the needs-rebase Indicates a PR needs to be rebased before being merged label May 27, 2018
@qiwzhang
Copy link
Contributor

Hi @nickrmc83 could you help to review this config change in Envoy: envoyproxy/envoy#3381

@dmadams
Copy link

dmadams commented Aug 31, 2018

Hi @nickrmc83 I was wondering where this was at. Are you still working on this? We are also interested in introducing OIDC to Envoy.

@nickrmc83
Copy link
Author

Closing as the world has moved on as has the approach.

@nickrmc83 nickrmc83 closed this Sep 4, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

cla: no Set by the Google CLA bot to indicate the author of a PR has not signed the Google CLA. needs-rebase Indicates a PR needs to be rebased before being merged

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants