Skip to content

xds: implement file-based JWT Call Credentials (A97) #8431

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

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

dimpavloff
Copy link

@dimpavloff dimpavloff commented Jul 7, 2025

Implement grpc/proposal#492 (A97). This is achieved via:

  • add credentials/jwt package to provide file-based PerRPCCallCredentials. It can be used beyond XDS. The package handles token reloading, caching, and validation as per A97
  • add internal/xds/bootstrap/jwtcreds package to provide a credentials.Bundle implementation using the new jwt package.
  • update internal/xds/bootstrap with support for loading multiple PerRPCCallCredentials specifed in a new call_creds field in the boostrap file as per A97
  • adjust xds/internal/xdsclient/clientimpl.goto use the call credentials when constructing the client
  • update xds/bootstrap to register the jwtcreds call credentials and make them available if GRPC_EXPERIMENTAL_XDS_BOOTSTRAP_CALL_CREDS is enabled

Whilst implementing the above, I considered credentials/oauth and credentials/xds packages instead of creating a new one. The former package has NewJWTAccessFromKey and jwtAccess which seem very relevant at first. However, I think the jwtAccess behaviour seems more tailored towards Google services. Also, the refresh, caching, and error behaviour for A97 is quite different than what's already there and therefore a separate implementation would have still made sense.
WRT credentials/xds, it could have been extended to both handle transport and call credentials. However, this is a bit at odds with A97 which says that the implementation should be non-XDS specific and, from reading between the lines, usable beyond XDS.
I think the current approach makes review easier but because of the similarities with the other two packages, it is a bit confusing to navigate. Please let me know whether the structure should change.

WRT the internal bootstrap implementation, I have added DialOptionsWithCallCredsForTransport because, even though current and future call credentials are likely to all expect secure transport, I thought it would be safer to check of insecure transport just in case. If you prefer, I can just update DialOptions to use all call credentials regardless of the transport.

Lastly, I appreciate this is a large PR so please lmk if you would rather I split it (e.g. credentials/ and xds/ parts).

Relates to istio/istio#53532

RELEASE NOTES:

  • credentials: add credentials/jwt package providing file-based JWT PerRPCCredentials (A97)
  • xds bootstrap: add support for loading a JWT token from file (guarded by GRPC_EXPERIMENTAL_XDS_BOOTSTRAP_CALL_CREDS )

Copy link

codecov bot commented Jul 7, 2025

Codecov Report

❌ Patch coverage is 88.58447% with 25 lines in your changes missing coverage. Please review.
✅ Project coverage is 82.51%. Comparing base (dd718e4) to head (eb391af).
⚠️ Report is 28 commits behind head on master.

Files with missing lines Patch % Lines
credentials/jwt/jwt_token_file.go 89.36% 10 Missing and 5 partials ⚠️
internal/xds/bootstrap/bootstrap.go 89.74% 3 Missing and 1 partial ⚠️
internal/xds/bootstrap/jwtcreds/bundle.go 89.28% 2 Missing and 1 partial ⚠️
xds/bootstrap/bootstrap.go 0.00% 2 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #8431      +/-   ##
==========================================
+ Coverage   82.27%   82.51%   +0.24%     
==========================================
  Files         414      415       +1     
  Lines       40424    40731     +307     
==========================================
+ Hits        33259    33610     +351     
+ Misses       5795     5757      -38     
+ Partials     1370     1364       -6     
Files with missing lines Coverage Δ
xds/bootstrap/credentials.go 100.00% <100.00%> (ø)
xds/internal/xdsclient/clientimpl.go 82.85% <100.00%> (+0.33%) ⬆️
internal/xds/bootstrap/jwtcreds/bundle.go 89.28% <89.28%> (ø)
xds/bootstrap/bootstrap.go 70.00% <0.00%> (-30.00%) ⬇️
internal/xds/bootstrap/bootstrap.go 68.27% <89.74%> (+2.92%) ⬆️
credentials/jwt/jwt_token_file.go 89.36% <89.36%> (ø)

... and 42 files with indirect coverage changes

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

@dimpavloff dimpavloff changed the title xds: implement file-based JWT authentication (A97) xds: implement file-based JWT Call Credentials (A97) Jul 7, 2025
@dimpavloff
Copy link
Author

@dfawley hey 👋 Given you approved A97, would you mind having a cursory look at the PR to confirm if at least at a high level the approach looks good?

@eshitachandwani
Copy link
Member

I will take a look at this , I need to go through the gRFC first.

@dfawley dfawley self-assigned this Jul 22, 2025
@dfawley dfawley requested review from easwars and eshitachandwani and removed request for dfawley July 25, 2025 20:39
@dfawley dfawley assigned easwars and unassigned dfawley Jul 25, 2025
@dfawley
Copy link
Member

dfawley commented Jul 25, 2025

Sorry for the delay here.

@easwars would you be able to review this change? I think you have more background into some of the things than I do, like the bootstrap integration. Thank you!

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.

4 participants