Skip to content

feat: validate JWT token and use projected token #5871

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

Merged
merged 18 commits into from
May 9, 2025

Conversation

cnvergence
Copy link
Member

@cnvergence cnvergence commented Apr 30, 2025

What type of PR is this?

What this PR does / why we need it:

  • Validate JWT token against intercepted nodeId and check if the service account name matches irkey stored in the snapshot cache.
  • Use the projected service account token and validate against EG audience

Which issue(s) this PR fixes:

Fixes #5863
Followup to the #5137

Release Notes: Yes

@cnvergence cnvergence requested a review from a team as a code owner April 30, 2025 14:34
@cnvergence cnvergence changed the title feat: validate JWT token with metadata headers and controller namespace value in infra feat: validate JWT token with metadata headers and add controller namespace value in infra Apr 30, 2025
@cnvergence cnvergence changed the title feat: validate JWT token with metadata headers and add controller namespace value in infra feat: validate JWT token with metadata headers and add controller namespace in infra Apr 30, 2025
Copy link

codecov bot commented Apr 30, 2025

Codecov Report

Attention: Patch coverage is 32.29167% with 65 lines in your changes missing coverage. Please review.

Project coverage is 65.81%. Comparing base (9b78828) to head (e792930).
Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
internal/xds/server/kubejwt/jwtinterceptor.go 0.00% 33 Missing ⚠️
internal/xds/server/kubejwt/tokenreview.go 0.00% 21 Missing ⚠️
internal/xds/cache/snapshotcache.go 0.00% 9 Missing ⚠️
internal/xds/server/runner/runner.go 0.00% 2 Missing ⚠️

❌ Your patch status has failed because the patch coverage (32.29%) is below the target coverage (60.00%). You can increase the patch coverage or adjust the target coverage.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #5871      +/-   ##
==========================================
+ Coverage   65.80%   65.81%   +0.01%     
==========================================
  Files         217      217              
  Lines       36020    36077      +57     
==========================================
+ Hits        23703    23745      +42     
- Misses      10836    10857      +21     
+ Partials     1481     1475       -6     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@cnvergence cnvergence force-pushed the update-gateway-mode branch 3 times, most recently from c4308f3 to 92ad344 Compare May 6, 2025 12:55
@cnvergence
Copy link
Member Author

cnvergence commented May 6, 2025

I will seperate feature with infrastructure controller namespace into separate PR
xref: #5937

@cnvergence cnvergence changed the title feat: validate JWT token with metadata headers and add controller namespace in infra feat: validate JWT token with metadata headers and use projected token May 6, 2025
@cnvergence cnvergence force-pushed the update-gateway-mode branch from 9be1dad to a13a272 Compare May 6, 2025 13:50
@zhaohuabing
Copy link
Member

zhaohuabing commented May 7, 2025

@cnvergence Talked with @arkodg offline, and he mentioned that we don't need to support gatewaynamespace mode for merged gateway. In that case, the irKey will always follow the "gateway-namespace/gateway-name" format, so we should be able to verify the service account in the jwt claim against the irKey.

Example:

JWT claim: "username": "system:serviceaccount:default:envoy-default-eg-e41e7b31"
IrKey: "default/eg"

We can construct the expected service account using the IrKey, and compare it with the username claim.

@arkodg feel free to add anything if I haven't captured our discussion correctly.

@cnvergence cnvergence changed the title feat: validate JWT token with metadata headers and use projected token feat: validate JWT token with ServiceAccount name and use projected token May 8, 2025
@cnvergence cnvergence force-pushed the update-gateway-mode branch from d77b9e6 to 8ed4cda Compare May 8, 2025 10:01
@cnvergence cnvergence force-pushed the update-gateway-mode branch 2 times, most recently from 99dcc03 to 726f64f Compare May 8, 2025 18:48
cnvergence and others added 4 commits May 9, 2025 12:29
Signed-off-by: Karol Szwaj <[email protected]>
Signed-off-by: Huabing (Robin) Zhao <[email protected]>
Signed-off-by: Karol Szwaj <[email protected]>
Signed-off-by: Karol Szwaj <[email protected]>
@zirain zirain force-pushed the update-gateway-mode branch from 726f64f to 6f0a844 Compare May 9, 2025 04:29
zirain
zirain previously approved these changes May 9, 2025
@zirain zirain added this to the v1.4.0 milestone May 9, 2025
zhaohuabing
zhaohuabing previously approved these changes May 9, 2025
Copy link
Member

@zhaohuabing zhaohuabing left a comment

Choose a reason for hiding this comment

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

LGTM.

Discussed with @arkodg , to prevent an envoy from requesting xDS not belong to it, we also need to enforce authz, which can be addressed in a follow-up PR.

Or you can simply revert b0748a0 to add pod check back @cnvergence

@zhaohuabing zhaohuabing dismissed stale reviews from zirain and themself via 654a617 May 9, 2025 07:19
This reverts commit b0748a0.

Signed-off-by: Huabing (Robin) Zhao <[email protected]>
@zhaohuabing zhaohuabing force-pushed the update-gateway-mode branch from 654a617 to 7049ed6 Compare May 9, 2025 07:20
zhaohuabing
zhaohuabing previously approved these changes May 9, 2025
Signed-off-by: Karol Szwaj <[email protected]>
@cnvergence cnvergence changed the title feat: validate JWT token with ServiceAccount name and use projected token feat: validate JWT token and use projected token May 9, 2025
@zirain zirain merged commit 57f0058 into envoyproxy:main May 9, 2025
24 of 25 checks passed
melsal13 referenced this pull request in melsal13/gatewayPersonal May 9, 2025
* Add proxyMetadata to xds config and validate JWT

Signed-off-by: Karol Szwaj <[email protected]>

* Add controller namespace to infra

Signed-off-by: Karol Szwaj <[email protected]>

* Add Metadata envoy bootstrap struct

Signed-off-by: Karol Szwaj <[email protected]>

* Add release note

Signed-off-by: Karol Szwaj <[email protected]>

* fix lint

Signed-off-by: Karol Szwaj <[email protected]>

* fix doc

Signed-off-by: Karol Szwaj <[email protected]>

* use projected service account tokens with eg audience

Signed-off-by: Karol Szwaj <[email protected]>

* lint code

Signed-off-by: Karol Szwaj <[email protected]>

* make gen

Signed-off-by: Karol Szwaj <[email protected]>

* make gen

Signed-off-by: Karol Szwaj <[email protected]>

* Revert "Add controller namespace to infra"

This reverts commit b2fa2caf58982432e5d5b31bd7d95a5ad523ed5e.

Signed-off-by: Karol Szwaj <[email protected]>

* fetch the node id and initial metadata from first msg

Signed-off-by: Karol Szwaj <[email protected]>

* update codegen

Signed-off-by: Karol Szwaj <[email protected]>

* verify service account

Signed-off-by: Huabing (Robin) Zhao <[email protected]>

* validate only sa

Signed-off-by: Karol Szwaj <[email protected]>

* add local hash name func

Signed-off-by: Karol Szwaj <[email protected]>

* Verify pod name for authz

This reverts commit b0748a066c9f6a41920df95f728ace5f84ed1acb.

Signed-off-by: Huabing (Robin) Zhao <[email protected]>

* lint code

Signed-off-by: Karol Szwaj <[email protected]>

---------

Signed-off-by: Karol Szwaj <[email protected]>
Signed-off-by: Huabing (Robin) Zhao <[email protected]>
Co-authored-by: Huabing (Robin) Zhao <[email protected]>
Signed-off-by: melsal13 <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

validate the identity in the Envoy JWT token
4 participants