credentials: implement file-based JWT Call Credentials (part 1 for A97) #8431

dimpavloff · 2025-07-07T09:27:50Z

Part one for grpc/proposal#492 (A97).
This is done in a new 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 .

There will be a separate PR which uses it in xds/bootstrap.

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.

Relates to istio/istio#53532

RELEASE NOTES:

credentials: add credentials/jwt package providing file-based JWT PerRPCCredentials (A97)

codecov · 2025-07-07T09:32:57Z

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 81.97%. Comparing base (dd718e4) to head (330d9a8).
⚠️ Report is 56 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #8431      +/-   ##
==========================================
- Coverage   82.27%   81.97%   -0.31%     
==========================================
  Files         414      414              
  Lines       40424    40587     +163     
==========================================
+ Hits        33259    33271      +12     
- Misses       5795     5930     +135     
- Partials     1370     1386      +16

Files with missing lines	Coverage Δ
credentials/jwt/jwt_file_reader.go	`100.00% <100.00%> (ø)`
credentials/jwt/jwt_token_file_call_creds.go	`100.00% <100.00%> (ø)`

... and 250 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 · 2025-07-09T11:25:26Z

@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 · 2025-07-22T08:38:37Z

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

dfawley · 2025-07-25T20:40:45Z

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!

easwars · 2025-07-28T19:50:58Z

Thank you for your contribution @dimpavloff. Yes, it would be nice if you can split this into smaller PRs. I will continue to use this PR to review the JWT call credentials implementation. If you can move the xDS implementation out to one or more PRs, I would greatly appreciate that and would be happy to review them as well.

credentials/jwt/doc.go

credentials/jwt/jwt_token_file.go

credentials/jwt/jwt_token_file_test.go

easwars · 2025-07-28T22:44:48Z

credentials/jwt/jwt_token_file_test.go

+
+	// Verify cached expiration is 30 seconds before actual token expiration
+	impl := creds.(*jwtTokenFileCallCreds)
+	impl.mu.RLock()


Please only test the API surface. Relying on implementation internals in tests makes them brittle and would result in test changes when any changes to implementation is made.

I assume you are referring to using a private field rather than obtaining mu specifically.
In general I agree -- white box tests may get fragile and break during a refactor. However, this test and the next couple of ones are about the caching behaviour -- it is meant to be transparent to the external API. If I don't make assertions about the private fields, the tests may pass trivially and become more flaky (e.g. when testing the backoff in the next test).
One alternative could be factoring out these behaviours out into a separate private struct with "public" functions which expose the same information. Given that it would require shifting the majority of the implementation into that struct, I'm not sure it is an improvement from the current approach.
Please do let me know your thoughts and if you have other suggestions.

dimpavloff

@easwars , I managed to iterate on top of your suggestion to simplify it further, we no longer need the condition variable, just one lock! LMK what you think

credentials/jwt/jwt_token_file.go

credentials/jwt/jwt_file_reader.go

easwars

Looks quite good. Most of the comments are nits.

I still have to review some of the tests in credentials/jwt/jwt_token_file_call_creds_test.go though (starting with TestTokenFileCallCreds_CacheExpirationIsBeforeTokenExpiration).

credentials/jwt/jwt_file_reader.go

credentials/jwt/jwt_token_file_call_creds_test.go

credentials/jwt/jwt_file_reader_test.go

easwars · 2025-08-25T23:17:17Z

credentials/jwt/jwt_token_file_call_creds_test.go

+				if err == nil {
+					t.Fatalf("GetRequestMetadata() expected error, got nil")
+				}
+				if !strings.Contains(err.Error(), tt.wantErrContains) {


Here as well, we should be checking for the status code instead of the actual contents of the error string.

Also, using status.Code(err) would work with nil error and return status.OK which would make the test logic much simpler as it would mostly the same for success and failure cases.

Nice point, updated.

Btw, this has helped uncover that credentials.CheckSecurityLevel and its call in GetRequestMetadata doesn't return a grpc code. This is also true in credentials/oauth/oauth.go and credentials/sts/sts.go. Therefore, this will map to codes.Unknown is this appropriate?

Will check with folks on the team and will get back to you soon on this. Thanks.

credentials/jwt/jwt_token_file_call_creds_test.go

easwars

Thanks for taking care of all of my comments. I think I'm mostly happy with where we are at now. I'll follow up on the two open issues and get back to you soon.

easwars · 2025-08-29T01:00:43Z

credentials/jwt/jwt_token_file_call_creds_test.go

+	}
+}
+
+// createTestJWT creates a test JWT token with the specified audience and


Nit: remove mention of audience from the comment.

easwars · 2025-08-29T01:14:42Z

credentials/jwt/jwt_file_reader.go

+	}
+}
+
+// ReadToken reads and parses a JWT token from the configured file.


Will check with folks on the team and will get back to you soon on this. Thanks.

easwars · 2025-08-29T01:14:47Z

credentials/jwt/jwt_token_file_call_creds_test.go

+				if err == nil {
+					t.Fatalf("GetRequestMetadata() expected error, got nil")
+				}
+				if !strings.Contains(err.Error(), tt.wantErrContains) {


Will check with folks on the team and will get back to you soon on this. Thanks.

dimpavloff added 3 commits July 6, 2025 18:47

xds: read JWT credentials from file as per A97

6e63daa

remove example

3268ea5

refactor test creation

b18a1f5

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

eshitachandwani requested a review from dfawley July 11, 2025 18:04

eshitachandwani assigned dfawley Jul 11, 2025

eshitachandwani assigned eshitachandwani and unassigned dfawley Jul 22, 2025

dfawley self-assigned this Jul 22, 2025

dfawley requested review from easwars and eshitachandwani and removed request for dfawley July 25, 2025 20:39

dfawley assigned easwars and unassigned dfawley Jul 25, 2025

refactor token string padding

eb391af

easwars reviewed Jul 28, 2025

View reviewed changes

easwars assigned dimpavloff and unassigned easwars Jul 28, 2025

dimpavloff added 7 commits July 29, 2025 08:20

remove example; mark as experimental

d43893a

reorganise struct attributes

167b86e

rename methods with Locked suffix

439d28c

remove context param from refreshTokenSync

b36d4b6

reformat comments; remove redundant cachedErrorTime field

26e0451

add defaultTestTimeout const

da2de8c

refactor test to use wantErr string only

51ce34c

dimpavloff commented Aug 21, 2025

View reviewed changes

credentials/jwt/jwt_token_file.go Outdated Show resolved Hide resolved

dimpavloff added 2 commits August 21, 2025 18:35

remove xds/bootstrap

a38573b

fix comment docstrings

12fedd5

dimpavloff commented Aug 21, 2025

View reviewed changes

credentials/jwt/jwt_file_reader.go Outdated Show resolved Hide resolved

dimpavloff changed the title ~~xds: implement file-based JWT Call Credentials (A97)~~ credentials: implement file-based JWT Call Credentials (part 1 for A97) Aug 22, 2025

dimpavloff requested a review from easwars August 22, 2025 11:27

This was referenced Aug 22, 2025

xds bootstrap: enable using JWT Call Credentials (part 2 for A97) #8535

Closed

xds bootstrap: enable using JWT Call Credentials (part 2 for A97) #8536

Open

dimpavloff removed their assignment Aug 22, 2025

easwars self-assigned this Aug 25, 2025

easwars reviewed Aug 25, 2025

View reviewed changes

easwars assigned dimpavloff and unassigned easwars Aug 25, 2025

dimpavloff added 11 commits August 26, 2025 11:54

remove newJWTFileReader

52445c7

make ReadToken private method

1678016

use subtests

1be843b

use writeTempFile

8ac3296

add comment about RPC queue behaviour

b0bdc70

remove needsPreemptiveRefreshLocked method

e4f955c

split NewTokenFileCallCredentials tests

f78178c

remove leftover os.MkdirTemp

bbeb759

remove audience parameter and do not set it at all for test tokens

bba5d34

test for grpc codes in TestTokenFileCallCreds_GetRequestMetadata

607868b

use cmp.Diff in TestTokenFileCallCreds_TokenCaching

bc2d327

dimpavloff requested a review from easwars August 26, 2025 17:52

dimpavloff removed their assignment Aug 26, 2025

easwars removed the Status: Requires Reporter Clarification label Aug 29, 2025

easwars self-assigned this Aug 29, 2025

easwars reviewed Aug 29, 2025

View reviewed changes

fix createTestJWT docstring

330d9a8

credentials: implement file-based JWT Call Credentials (part 1 for A97) #8431

Are you sure you want to change the base?

credentials: implement file-based JWT Call Credentials (part 1 for A97) #8431

Conversation

dimpavloff commented Jul 7, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov bot commented Jul 7, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

dimpavloff commented Jul 9, 2025

Uh oh!

eshitachandwani commented Jul 22, 2025

Uh oh!

dfawley commented Jul 25, 2025

Uh oh!

easwars commented Jul 28, 2025

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dimpavloff left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

easwars left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

easwars left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

dimpavloff commented Jul 7, 2025 •

edited

Loading

codecov bot commented Jul 7, 2025 •

edited

Loading