Skip to content

Support validating first party JWT for platforms without third party JWT#20179

Merged
istio-testing merged 1 commit intoistio:masterfrom
lei-tang:support-legacy-jwt
Jan 18, 2020
Merged

Support validating first party JWT for platforms without third party JWT#20179
istio-testing merged 1 commit intoistio:masterfrom
lei-tang:support-legacy-jwt

Conversation

@lei-tang
Copy link
Copy Markdown
Contributor

@lei-tang lei-tang commented Jan 14, 2020

Please provide a description for what this PR is for: #20178

To make the PR shorter and easier to review, this PR focues on adding the support of validating first party JWT. More PRs will follow up on this topic.

[ ] Configuration Infrastructure
[ ] Docs
[ ] Installation
[X ] Networking
[ ] Performance and Scalability
[ ] Policies and Telemetry
[X ] Security
[ ] Test and Release
[ ] User Experience
[ ] Developer Infrastructure

@lei-tang lei-tang requested review from a team as code owners January 14, 2020 22:00
@googlebot googlebot added the cla: yes Set by the Google CLA bot to indicate the author of a PR has signed the Google CLA. label Jan 14, 2020
@istio-testing istio-testing added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Jan 14, 2020
@lei-tang lei-tang added this to the 1.5 milestone Jan 14, 2020
Copy link
Copy Markdown
Member

@howardjohn howardjohn left a comment

Choose a reason for hiding this comment

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

This only solves a small portion of the problem. We still mount the trustworthy jwt, which will fail on clusters without support, and we still send XDS config referencing it.

I would suggest looking at the PR that removed support and working backwards as a starting point possibly?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

We used to have a setting useTrustworthyJwt, do you think it makes sense to go back to using the same setting?

If not, should we call this first and third party jwt?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Changed to call it first and third party JWT.
Here jwtPolicy is used to express different JWT policies; the binary setting of useTrustworthyJwt makes it awkward to express different JWT policies while reducing the code readability.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Line 391 is going to fail if your cluster doesn't have this enabled. If you want to test it just run kind create cluster, that will start a cluster without support. No pods will start

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Added {{- if eq .Values.global.jwtPolicy "third-party-jwt" }} before line 391.

@lei-tang lei-tang changed the title Support legacy JWT for platforms without trustworthy JWT Support first party JWT for platforms without third party JWT Jan 14, 2020
@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 Jan 14, 2020
@istio istio deleted a comment from istio-testing Jan 14, 2020
@istio istio deleted a comment from istio-testing Jan 14, 2020
@lei-tang lei-tang changed the title Support first party JWT for platforms without third party JWT Support validation of first party JWT for platforms without third party JWT Jan 14, 2020
@lei-tang
Copy link
Copy Markdown
Contributor Author

lei-tang commented Jan 14, 2020

@howardjohn To support first party JWT, there will be multiple PRs. To make the PR shorter and easier to review, this PR focuses on adding the support of validating first party JWT.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

you need to remove the volume too, not just the volume mount

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Added the conditional flag to the volume too. The mounting part will be addressed in follow up PRs.

@lei-tang lei-tang changed the title Support validation of first party JWT for platforms without third party JWT Support validating first party JWT for platforms without third party JWT Jan 14, 2020
Copy link
Copy Markdown
Member

@howardjohn howardjohn left a comment

Choose a reason for hiding this comment

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

lgtm other than my one comment. can be a followup pr

@lei-tang lei-tang force-pushed the support-legacy-jwt branch 2 times, most recently from 23319a3 to 19cc5c9 Compare January 15, 2020 00:02
@istio istio deleted a comment from istio-testing Jan 15, 2020
@lei-tang
Copy link
Copy Markdown
Contributor Author

/test release-test_istio

@lei-tang
Copy link
Copy Markdown
Contributor Author

/test integ-security-k8s-tests_istio

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Done.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Please also set condition for the "projected volume"

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Done.

@lei-tang
Copy link
Copy Markdown
Contributor Author

/test gencheck_istio

@lei-tang lei-tang force-pushed the support-legacy-jwt branch 2 times, most recently from 76bc815 to d51fa72 Compare January 15, 2020 23:31
@lei-tang lei-tang added the do-not-merge/hold Block automatic merging of a PR. label Jan 16, 2020
@istio istio deleted a comment from istio-testing Jan 16, 2020
@istio istio deleted a comment from istio-testing Jan 16, 2020
@lei-tang lei-tang force-pushed the support-legacy-jwt branch 2 times, most recently from 1b7df8f to cbf6945 Compare January 16, 2020 17:55
@istio istio deleted a comment from istio-testing Jan 16, 2020
@lei-tang lei-tang force-pushed the support-legacy-jwt branch 3 times, most recently from b6e6132 to dd22696 Compare January 17, 2020 23:39
@lei-tang lei-tang removed the do-not-merge/hold Block automatic merging of a PR. label Jan 17, 2020
- Support validating first party JWT for platforms without third party JWT.
- Add the JWT policy configuration to Operator.
- Config grpc calling credentials.
@istio-testing istio-testing merged commit cb3935b into istio:master Jan 18, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area/networking area/security cla: yes Set by the Google CLA bot to indicate the author of a PR has signed the Google CLA. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants