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 97.63780% with 3 lines in your changes missing coverage. Please review.
✅ Project coverage is 82.04%. Comparing base (dd718e4) to head (12fedd5).
⚠️ Report is 51 commits behind head on master.

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

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #8431      +/-   ##
==========================================
- Coverage   82.27%   82.04%   -0.24%     
==========================================
  Files         414      414              
  Lines       40424    40647     +223     
==========================================
+ Hits        33259    33347      +88     
- Misses       5795     5910     +115     
- Partials     1370     1390      +20

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

... and 254 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:41:16Z

credentials/jwt/jwt_token_file_test.go

+		AuthInfo: &testAuthInfo{secLevel: credentials.PrivacyAndIntegrity},
+	})
+
+	// First call should read from file


Here and elsewhere in this PR, please use appropriate punctuations in comment sentences. See: https://google.github.io/styleguide/go/decisions#comment-sentences

easwars · 2025-07-28T22:43:12Z

credentials/jwt/jwt_token_file_test.go

+				t.Fatalf("NewTokenFileCallCredentials() failed: %v", err)
+			}
+
+			ctx, cancel := context.WithTimeout(context.Background(), 5*time.Second)


Please define a const for default test timeout and use it in all tests that require a timeout. We use this pattern a lot in our tests. Please search for defaultTestTimeout and defaultTestShortTimeout for commonly used patterns.

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 · 2025-08-11T16:47:22Z

Apologies for the delay on this. Have been busy with some other stuff. Will definitely try to make another pass today.

FYI, we recommend authors to not mark comments as resolved. Instead, we recommend the reviewer to do that once the comment has been satisfactorily addressed, because now I have to unresolve every comment to see what I commented and then verify that the comment has been satisfactorily addressed. Thanks for understanding.

easwars · 2025-08-12T14:00:41Z

credentials/jwt/doc.go

+// - Errors in reading tokens or parsing JWTs will result in RPC UNAVAILALBE or
+// UNAUTHENTICATED errors
+// - These errors are cached and retried with exponential backoff.


Nit: It looks like these two bullet points can/should be merged?

credentials/jwt/jwt_token_file.go

easwars · 2025-08-12T14:16:54Z

credentials/jwt/jwt_token_file.go

+}
+
+// isTokenValid checks if the cached token is still valid.
+// Caller must hold c.mu.RLock().


Actually, I personally haven't considered it so far, because I tend to avoid RWLocks after being bitten by them once (in a bad way). But, I'm not opposed to it and it does sound more explicit when using RWLocks.

easwars · 2025-08-12T14:20:07Z

credentials/jwt/jwt_token_file.go

+// invalid or expired, the next RPC will handle synchronous refresh instead.
+// Caller must hold c.mu.RLock().
+func (c *jwtTokenFileCallCreds) needsPreemptiveRefreshLocked() bool {
+	return c.isTokenValidLocked() && time.Until(c.cachedExpiration) < time.Minute


Can we make a const for the one minute pre-emptive refresh trigger?

easwars · 2025-08-12T21:44:25Z

credentials/jwt/jwt_token_file.go

+	nextRetryTime   time.Time        // When next retry is allowed
+
+	// Pre-emptive refresh mutex
+	refreshMu sync.Mutex


That's a valid point. Even if it is a single goroutine, we cannot leak it, because our tests check for leaked goroutines and therefore unit tests will fail. Thanks for trying though.

easwars · 2025-08-12T21:44:35Z

credentials/jwt/jwt_token_file.go

+// Multiple concurrent calls are safe - only one refresh will run at a time.
+// The refresh runs in a separate goroutine and does not block the caller.
+func (c *jwtTokenFileCallCreds) triggerPreemptiveRefresh() {
+	go func() {


easwars · 2025-08-12T22:17:28Z

Again, apologies for the delay in the review.

I did a little bit of thinking and I feel we can avoid the second mutex and also avoid locking and unlocking multiple times in several codepaths with the following approach.

(this is mostly going to be pseudocode ... so please bear with me)

Separate out the file reading and expiration extracting functionality into a separate type, JWTFileReader or some other name
- This would have a NewXxx function that would take the file name
- This type would also have a method to read the token from file: ReadToken() (token string, expiry time.Time, err error)
- This type would be very simple and would not know or care about any of the other stuff happening in the creds (like preemptive refreshes, ongoing refreshes etc)
- It would be the responsibility of the caller to ensure that they don't call the ReadToken() method concurrently. Nothing bad will happen if they call it concurrently, just that there would unnecessary file reads.
We would maintain the following state in the creds (all protected by a single sync.Mutex)
- token
- expiry time
- previously seen error
- retry attempt number
- next retry time
- pending refresh (boolean)
- a condition variable for starting a token refresh (sync.Cond)
The GetRequestMetadata would work as follows:
- Lock the mutex
- Defer a call to unlock the mutex
- If the token is valid:
  - if we need a pre-emptive refresh:
    - if there is no pending refresh:
      - set the pending bit
      - spawn a goroutine to refresh the token
  - return token
- If we are in backoff:
  - return error
- If there is no pending request:
  - set the pending bit
  - spawn a goroutine to refresh the token
- Wait for the condition that (pending == false), using the condition variable. This will be something like:
```
  for pending == true {
    cond.Wait()
  }
```
  - When there is an ongoing refresh, this will block the current goroutine without holding the mutex
- Once we get past the condition variable, we can safely return the result from the our state
  - Once we get past cond.Wait(), we will again have possession of the mutex, and can therefore safely access our state
  - we either have a valid token or a valid error
The goroutine to refresh the token would do the following:
- Call fileReader.ReadToken()
  - (the above call is now happening without the mutex), but since spawning of this goroutine is controlled by the pending bit, we are always guaranteed that only one instance of this goroutine is ever running)
- Lock mutex
  - Update internal state (token, expiry, error, backoff state)
  - reset the pending bit
- Unlock mutex
- Call Broadcast on the condition variable to unblock all goroutines waiting on this condition

Let me know what you think.

github-actions · 2025-08-18T22:45:55Z

This PR is labeled as requiring an update from the reporter, and no update has been received after 6 days. If no update is provided in the next 7 days, this issue will be automatically closed.

dimpavloff · 2025-08-21T13:39:46Z

Hi @easwars , apologies for the delay, I was on holiday. Thanks for the review and for the suggested approach. I don't have the code very fresh in my mind anymore 😅 but I think that makes sense. I will see what I can do in the coming days and get back to you.

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

dimpavloff · 2025-08-21T17:21:37Z

credentials/jwt/jwt_token_file.go

+	return map[string]string{
+		"authorization": "Bearer " + token,
+	}, nil


FYI, I've cached it just in case

dimpavloff · 2025-08-21T17:39:29Z

credentials/jwt/jwt_file_reader.go

+}
+
+// newJWTFileReader creates a new JWTFileReader for the specified file path.
+func newJWTFileReader(tokenFilePath string) *jWTFileReader {


Just to highlight, I made both the struct and function private. LMK if you intended for them to be public

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

easwars added Type: Feature New features or improvements in behavior Area: Auth Includes regular credentials API and implementation. Also includes advancedtls, authz, rbac etc. labels Aug 11, 2025

easwars added this to the 1.76 Release milestone Aug 11, 2025

eshitachandwani removed their request for review August 12, 2025 05:12

eshitachandwani removed their assignment Aug 12, 2025

easwars reviewed Aug 12, 2025

View reviewed changes

easwars assigned dimpavloff and unassigned easwars Aug 12, 2025

easwars added the Status: Requires Reporter Clarification label Aug 12, 2025

github-actions bot added the stale label Aug 18, 2025

rename files

1d95fa2

github-actions bot removed the stale label Aug 21, 2025

dimpavloff added 5 commits August 21, 2025 18:10

use cond variable

a797ed9

refactor to no longer need cond

fd388d1

fix docstring comment

790a2d9

cache authorization header instead of token

6713190

remove internal/ and xds/ changes

3f563eb

dimpavloff commented Aug 21, 2025

View reviewed changes

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

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

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!

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!

easwars commented Aug 11, 2025

Uh oh!

Choose a reason for hiding this comment

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!

Choose a reason for hiding this comment

Uh oh!

easwars commented Aug 12, 2025

Uh oh!

github-actions bot commented Aug 18, 2025

Uh oh!

dimpavloff commented Aug 21, 2025

Uh oh!

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

Uh oh!

dimpavloff commented Jul 7, 2025 •

edited

Loading

codecov bot commented Jul 7, 2025 •

edited

Loading