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

❌ Patch coverage is 96.74797% with 4 lines in your changes missing coverage. Please review.
✅ Project coverage is 81.92%. Comparing base (dd718e4) to head (3f9195e).
⚠️ Report is 62 commits behind head on master.

Files with missing lines	Patch %	Lines
credentials/jwt/jwt_token_file_call_creds.go	95.06%	3 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #8431      +/-   ##
==========================================
- Coverage   82.27%   81.92%   -0.35%     
==========================================
  Files         414      415       +1     
  Lines       40424    40643     +219     
==========================================
+ Hits        33259    33297      +38     
- Misses       5795     5966     +171     
- Partials     1370     1380      +10

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

... 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.

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.

credentials/jwt/jwt_token_file_call_creds_test.go

credentials/jwt/jwt_file_reader.go

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.

credentials/jwt/jwt_file_reader.go

credentials/jwt/jwt_file_reader_test.go

credentials/jwt/jwt_token_file_call_creds.go

easwars · 2025-09-03T19:42:41Z

credentials/jwt/jwt_token_file_call_creds_test.go

+	impl := creds.(*jwtTokenFileCallCreds)
+	impl.mu.Lock()
+	cachedErr := impl.cachedError
+	retryAttempt := impl.retryAttempt
+	nextRetryTime := impl.nextRetryTime
+	impl.mu.Unlock()


Please consider declaring the backoff function as a package level variable that can be overridden from here. That way, you can easily verify the retry attempt and inject any value that you want from the backoff function. This is an example of a test that overrides the backoff function used by the code under test. See:

grpc-go/internal/xds/clients/xdsclient/test/ads_stream_backoff_test.go

Line 65 in 29ba001

func (s) TestADS_BackoffAfterStreamFailure(t *testing.T) {

I'm not sure if that would help much. The struct is storing the backoff duration in nextRetryTime which I am already able to mutate directly in the tests which would be simpler than through an overridden backoff function.

credentials/jwt/jwt_token_file_call_creds_test.go

easwars · 2025-09-03T19:45:23Z

credentials/jwt/jwt_token_file_call_creds_test.go

+	// Fast-forward the backoff retry time to allow next retry attempt.
+	impl.mu.Lock()
+	impl.nextRetryTime = time.Now().Add(-1 * time.Minute)
+	impl.mu.Unlock()


So, if we override the backoff function, we wont need to change the internal fields of the creds. We can simply control the returned value from the overridden backoff function.

The backoff function will need to be configured what delay to return before GetMetadata is called. This result is then stored in nextRetryTime in the implementation. Subsequent calls will continue to consult nextRetryTime until another attempt is made.
Therefore, in this test, before the first call, I would have to estimate what is a good delay such that it's long enough for the first and second call to be covered (ie second call is cached) but short enough for the third call to trigger a retry. Obviously, this would be extremely flaky. Alternatively, it would require that I create a deadline in the backoff function which is shared with the rest of the test and the deadline is awaited on this line. To me this seems more complicated than mutating the private field to a negative value to force the retry and it will make the test slower because I still need to overestimate the duration for the first and second calls.

LMK if you still want to proceed.

credentials/jwt/jwt_token_file_call_creds_test.go

easwars

Could you please add a TODO in the tests that access/mutate internal state to rewrite/change them to not do that. We can handle it as a low priority task at some point in time.

Thank you for taking care of all the comments.

easwars · 2025-09-04T21:53:52Z

Moving to @arjan-bal for second set of eyes.

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 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

easwars assigned dimpavloff and unassigned easwars Aug 29, 2025

refactor readToken() and tests to use error values

774d83e

dimpavloff requested a review from easwars September 1, 2025 10:25

dimpavloff removed their assignment Sep 1, 2025

arjan-bal assigned easwars Sep 3, 2025

easwars reviewed Sep 3, 2025

View reviewed changes

easwars assigned dimpavloff and unassigned easwars Sep 3, 2025

dimpavloff added 6 commits September 3, 2025 20:58

remove errJWTFormat in favour of validation error

b9dcfcb

error wrapping

6ee5ba7

subtests with underscores only

14c5ccd

change credentials.CheckSecurityLevel error mgs; success path identation

ca8227d

re-order assertions

ff50123

remove string comparisons

c05da9f

dimpavloff requested a review from easwars September 3, 2025 22:32

dimpavloff removed their assignment Sep 3, 2025

easwars approved these changes Sep 4, 2025

View reviewed changes

easwars assigned arjan-bal Sep 4, 2025

easwars requested review from dfawley and arjan-bal and removed request for dfawley September 4, 2025 21:53

add TODO to tests

3f9195e

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

Uh oh!

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!

easwars Jul 28, 2025

Choose a reason for hiding this comment

Uh oh!

dimpavloff Jul 29, 2025

Choose a reason for hiding this comment

Uh oh!

easwars left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

easwars Aug 29, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

easwars Sep 3, 2025

Choose a reason for hiding this comment

Uh oh!

dimpavloff Sep 3, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

easwars Sep 3, 2025

Choose a reason for hiding this comment

Uh oh!

dimpavloff Sep 3, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

easwars left a comment

Choose a reason for hiding this comment

Uh oh!

easwars commented Sep 4, 2025

Uh oh!

Uh oh!

dimpavloff commented Jul 7, 2025 •

edited

Loading

codecov bot commented Jul 7, 2025 •

edited

Loading