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

support specify jwt requirement #2733

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

wulianglongrd
Copy link
Member

@wulianglongrd wulianglongrd commented Mar 17, 2023

issue: istio/istio#43982

JwtRule add a failure_mode field to specify a Jwt requirement. This is an optional field and no breaking changes.

@wulianglongrd wulianglongrd requested a review from a team as a code owner March 17, 2023 16:33
@istio-policy-bot
Copy link

😊 Welcome @wulianglongrd! This is either your first contribution to the Istio api repo, or it's been
awhile since you've been here.

You can learn more about the Istio working groups, code of conduct, and contributing guidelines
by referring to Contributing to Istio.

Thanks for contributing!

Courtesy of your friendly welcome wagon.

@istio-testing istio-testing added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Mar 17, 2023
@wulianglongrd wulianglongrd changed the title JWTRule add allow field to specify jwt requirements JWTRule add allow field to specify a jwt requirement Mar 17, 2023
security/v1beta1/jwt.proto Outdated Show resolved Hide resolved
security/v1beta1/jwt.proto Outdated Show resolved Hide resolved
@wulianglongrd wulianglongrd deleted the JwtRule-allow branch April 22, 2023 04:29
@wulianglongrd wulianglongrd restored the JwtRule-allow branch April 22, 2023 09:28
@wulianglongrd wulianglongrd reopened this Apr 22, 2023
@wulianglongrd wulianglongrd changed the title JWTRule add allow field to specify a jwt requirement JWTRule add requirement field to specify a jwt requirement Apr 22, 2023
@istio-testing istio-testing added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Apr 22, 2023
@wulianglongrd wulianglongrd changed the title JWTRule add requirement field to specify a jwt requirement support specify jwt requirement Apr 22, 2023
@wulianglongrd
Copy link
Member Author

cc @ramaraochavali

@wulianglongrd wulianglongrd requested a review from linsun April 22, 2023 09:54
@wulianglongrd
Copy link
Member Author

If this api can be merged into master, I will implement it in the istio repository.

@ramaraochavali
Copy link
Contributor

@wulianglongrd Thank you . LGTM. @linsun Can you please review?

security/v1/jwt.proto Outdated Show resolved Hide resolved
security/v1/jwt.proto Outdated Show resolved Hide resolved
security/v1/jwt.proto Outdated Show resolved Hide resolved
security/v1/jwt.proto Outdated Show resolved Hide resolved
@wulianglongrd
Copy link
Member Author

@howardjohn @ramaraochavali @aryan16 thanks for your review.

To be honest, I also think the naming doesn't look very good. What I want to express are three levels of tolerance for authentication(authn) results:

  1. jwt must be provided and must meet the RequestAuthentication(RA) rules (for example, signature, audiences, issuer, expiration, etc... need to be verified successfully)
  2. It is allowed if jwt is not provided, but if it is provided, it must meet the RA rules (this is the current behavior, and currently only supports this behavior)
  3. It is allowed if jwt is not provided, and it is also allowed if it is provided and does not meet the RA rules

Would it be clearer to name it mode?

  • STRICT: (1)
  • PERMISSIVE: (2)
  • ?: (3) what should it be named ?

What do you think?

@ramaraochavali
Copy link
Contributor

Would it be clearer to name it mode?

STRICT: (1)
PERMISSIVE: (2)
?: (3) what should it be named ?

Yes. Mode/FailureMode makes sense

I like STRICT and PERMISSIVE. 3 can be IGNORE - that is ignore if jwt is missing or fails validation?

@wulianglongrd
Copy link
Member Author

3 can be IGNORE - that is ignore if jwt is missing or fails validation?

Yes, and I think IGNORE is good, let's wait for what others opinion.

@costinm
Copy link
Contributor

costinm commented Apr 25, 2023 via email

@costinm
Copy link
Contributor

costinm commented Apr 25, 2023 via email

@costinm
Copy link
Contributor

costinm commented Apr 26, 2023 via email

@wulianglongrd
Copy link
Member Author

wulianglongrd commented Apr 27, 2023

Thanks for your reply, it's very helpful to me.

Our usage scenario is to use JWT for intranet service-to-service authentication (mTLS is not used for some other reasons), not for end-user authentication.

Because it is an intranet service, stability is more important to us than security. That is to say, we can tolerate the lack of security protection capabilities for a short period of time, but we cannot tolerate the interruption of production traffic due to incorrect security policies. (By the way, traffic from the external network to the internal network will pass through our gateway, and we have self-developed security components for strict security protection)

At present, our intranet services do not have standardized security protection components, and most of them use ak/sk + rbac hard-coded security protection, which brings a lot of repetitive work. Therefore, we want to borrow the security capabilities of istio to transparently replace this part of the functionality.

Because it is to add security capabilities to services running in the production environment, stability is what we are most worried about. A small mistake may cause serious online accidents. Because there are a large number of very important services with very complex dependencies, for example, a service may have more than 200+ callers (downstream service), and the number of requests per second exceeds 150,000 (150k+ QPS).

So, how to ensure the stability of introducing istio security?

dry-run, dry-run mode allows to adjust the policy and send it to the istio-proxy for execution, but when the policy is misconfigured due to a mistake, the process will not be interrupted. When we have enough confidence to determine that the security policy is correct, we turn off dry-run.

At present, authz already supports dry-run, but authn does not. This RP hopes that authn supports a capability similar to dry-run, which is also the motivation for me to submit this PR.

Do you have any suggestions? Thanks.

@istio-testing istio-testing added the needs-rebase Indicates a PR needs to be rebased before being merged label Jun 22, 2023
@istio-policy-bot istio-policy-bot added the lifecycle/automatically-closed Indicates a PR or issue that has been closed automatically. label Aug 30, 2023
@hexiaodai
Copy link
Member

Is it possible to reopen this PR?

@wulianglongrd wulianglongrd reopened this Mar 11, 2024
@wulianglongrd wulianglongrd added the do-not-merge/work-in-progress Block merging of a PR because it isn't ready yet. label Mar 11, 2024
@istio-testing istio-testing added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed needs-rebase Indicates a PR needs to be rebased before being merged size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Mar 12, 2024
@wulianglongrd wulianglongrd removed do-not-merge/work-in-progress Block merging of a PR because it isn't ready yet. lifecycle/automatically-closed Indicates a PR or issue that has been closed automatically. labels Mar 12, 2024
@wulianglongrd
Copy link
Member Author

@hanxiaop
Copy link
Member

I think my case involves handling tokens from an unknown issuer under the current policy. For example, I originally required some requests to be validated by a specific issuer, but now I wish to allow requests with k8s tokens to bypass the policy, with validation occurring in the app. It is hard to write a new rule to validate that token, especially in the multicluster senario. Without an option to ignore the failure, the requests cannot pass the RA and be validated by the app.

@wulianglongrd
Copy link
Member Author

I think the main purpose of this API is to provide the ability to "tolerate" authn failure. We will always need to tolerate authn failure in some cases for one reason or another.

@wulianglongrd
Copy link
Member Author

I think the main purpose of this API is to provide the ability to "tolerate" authn failure. We will always need to tolerate authn failure in some cases for one reason or another.

@howardjohn @linsun @aryan16 @costinm @ramaraochavali
What do you think? If I miss any questions and haven't replied, please let me know. Thanks.

@flpanbin
Copy link

flpanbin commented Mar 13, 2024

I have the same problem as @hanxiaop , we have a proxy service to forward k8s request, and this proxy service is on the backend of istio gateway. But the k8s token sent by the client cannot be authenticated by RA. There is no good solution to this problem currently. We urgently need this feature!

@istio-testing istio-testing added the needs-rebase Indicates a PR needs to be rebased before being merged label May 14, 2024
@istio-testing
Copy link
Collaborator

PR needs rebase.

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-sigs/prow repository.

@chanakya-svt
Copy link

Any update on when/if this would be considered and merged?

@wulianglongrd
Copy link
Member Author

Any update on when/if this would be considered and merged?

Sorry, I didn't push this PR because I don't have an urgent need for it right now (I implemented it in my company's private branch a long time ago), if you want to get this feature, please convince TOC to accept it.

If you have any suggestions for improvements to this PR, feel free to comment, I'll be happy to improve it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs-rebase Indicates a PR needs to be rebased before being merged size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.