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 64.58333% with 51 lines in your changes missing coverage. Please review.
✅ Project coverage is 80.65%. Comparing base (dd718e4) to head (75fbc02).
⚠️ Report is 67 commits behind head on master.

Files with missing lines	Patch %	Lines
credentials/jwt/jwt_token_file_call_creds.go	66.66%	19 Missing and 13 partials ⚠️
credentials/jwt/jwt_file_reader.go	60.41%	10 Missing and 9 partials ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #8431      +/-   ##
==========================================
- Coverage   82.27%   80.65%   -1.62%     
==========================================
  Files         414      415       +1     
  Lines       40424    40969     +545     
==========================================
- Hits        33259    33044     -215     
- Misses       5795     6244     +449     
- Partials     1370     1681     +311

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

... and 261 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 · 2025-09-10T12:39:04Z

@arjan-bal ping

credentials/jwt/jwt_file_reader.go

arjan-bal · 2025-09-10T14:07:07Z

credentials/jwt/jwt_file_reader.go

+}
+
+// jWTFileReader handles reading and parsing JWT tokens from files.
+type jWTFileReader struct {


style: This struct should be named jwtFileReader. See https://google.github.io/styleguide/go/decisions#initialisms

arjan-bal · 2025-09-10T14:15:09Z

credentials/jwt/jwt_file_reader.go

+func (r *jWTFileReader) extractExpiration(token string) (time.Time, error) {
+	parts := strings.Split(token, ".")
+	if len(parts) != 3 {
+		return time.Time{}, fmt.Errorf("expected 3 parts, got %d: %w", len(parts), errJWTValidation)


This method seems to be wrapping errJWTValidation in every error it returns. I think we should move the wrapping to the calling function to avoid redundancy.

If there are reasons to perform the wrapping in this method, we should mention that the cases in which the returned error is an instance of errJWTValidation in the godoc.

Good point 👍

arjan-bal · 2025-09-10T14:28:00Z

credentials/jwt/jwt_file_reader.go

+}
+
+// extractExpiration parses the JWT token to extract the expiration time.
+func (r *jWTFileReader) extractExpiration(token string) (time.Time, error) {


JWT parsing seems like a fairly complex task. Even though we aren't verifying the token, my opinion is that we should use a popular library like golang-jwt. I would like to get others' opinions on this.

@dfawley @gtcooke94 @matthewstevenson88

I scanned through that library. Looks like it has a lot of functionality. In the past, we have decided to implement things ourselves when we only need a small amount of functionality from a library. Also, we already have this functionality in the existing GCP JWT token call creds. So, I'm leaning against not taking a dependency on an external library (even though it seems to be a 1.x). Want to hear Doug's thoughts on this though.

This functionality is preset in the go std lib.

import "golang.org/x/oauth2/jws" func (r *jwtFileReader) extractExpiration(token string) (time.Time, error) { cs, err := jws.Decode(token) if err != nil { return time.Time{}, err } expTime := time.Unix(cs.Exp, 0) if expTime.Before(time.Now()) { return time.Time{}, fmt.Errorf("expired token: %w", errJWTValidation) } return expTime, nil }

This is what the Google oauth2 implementation uses internally. The jws package is marked as deprecated with the following message:

Deprecated: this package is not intended for public use and might be removed in the future. It exists for internal use only. Please switch to another JWS package or copy this package into your own source tree.

I think we can take inspiration from their implementation.

I think we can take inspiration from their implementation.

Their implementation's jws.Decode:

Parses the token by splitting on the . character https://cs.opensource.google/go/x/oauth2/+/refs/tags/v0.31.0:jws/jws.go;drc=696f7b31289a98558822be146698b7834e477e63;l=182

Calls base64.RawURLEncoding.DecodeString(claims)

Decodes the JWT Claims json.NewDecoder(bytes.NewBuffer(decoded)).Decode(c)

This is exactly what the current function does, barring minor detail of what exact functions are called to achieve these steps. So I am struggling to understand what your suggestion is here – copy paste the code verbatim so that it's exactly the same as in golang.org/x/oauth2/jws? If yes, why is that preferred? Or ignore golang.org/x/oauth2/jws's deprecation notice and do what you have in the snippet?

Apologies for the confusion, this thread is not actionable now. I left it open for other reviewers to see. I left comments about replacing strings.Split for efficiency and using RawURLEncoding below after going through the std lib implementation.

arjan-bal · 2025-09-10T14:31:02Z

credentials/jwt/jwt_token_file_call_creds.go

+ *
+ */
+
+// Package jwt implements gRPC credentials using JWT tokens from files.


Only a single file in the package should have the package level godoc. Since it's there in doc.go, we should remove it from here.

credentials/jwt/jwt_file_reader.go

arjan-bal · 2025-09-10T15:03:34Z

credentials/jwt/jwt_token_file_call_creds.go

+		if needsPreemptiveRefresh {
+			// Start refresh if not pending (handling the prior RPC may have
+			// just spawned a goroutine).
+			if !c.pendingRefresh {


nit: Can we combine this with the outer check to reduce one level of indentation?

if needsPreemptiveRefresh && !c.pendingRefresh {

arjan-bal · 2025-09-10T15:11:10Z

credentials/jwt/jwt_token_file_call_creds.go

+
+// refreshToken reads the token from file and updates the cached data.
+func (c *jwtTokenFileCallCreds) refreshToken() {
+	// Deliberately not locking c.mu here


Is the mutex lock omitted here to avoid doing file IO while holding the lock? If that's the case, we should state this in the comment for the benefit of readers.

I think this is safe only as long fileReader can handle concurrent calls. It can presently, since it doesn't have any mutable state. We should probably this requirement in the godoc of fileReader to serve as a good warning in case state is ever added to fileReader. Something like "It is safe to call methods on this type concurrently" should work.

Is the mutex lock omitted here to avoid doing file IO while holding the lock?

Yes, exactly.

We should probably this requirement in the godoc of fileReader to serve as a good warning in case state is ever added to fileReader.

Good idea! 👍

arjan-bal · 2025-09-10T15:20:29Z

credentials/jwt/jwt_token_file_call_creds.go

+	c.mu.Lock()
+	defer c.mu.Unlock()
+	c.updateCacheLocked(token, expiry, err)
+


nit: Please omit empty line here, it doesn't seem to improve readability here.

arjan-bal · 2025-09-10T15:48:09Z

credentials/jwt/jwt_token_file_call_creds.go

+	c.nextRetryTime = time.Time{}
+
+	c.cachedAuthHeader = "Bearer " + token
+	// Per RFC A97: consider token invalid if it expires within the next 30


nit: This should be Per gRFC instead of Per RFC.

arjan-bal · 2025-09-10T15:53:58Z

credentials/jwt/jwt_token_file_call_creds.go

+func (c *jwtTokenFileCallCreds) GetRequestMetadata(ctx context.Context, _ ...string) (map[string]string, error) {
+	ri, _ := credentials.RequestInfoFromContext(ctx)
+	if err := credentials.CheckSecurityLevel(ri.AuthInfo, credentials.PrivacyAndIntegrity); err != nil {
+		return nil, fmt.Errorf("cannot send secure credentials on an insecure connection: %w", err)


nit: Use %v instead of %w to format the embedded error here. Using %w can leak the type of the embedded error and make it part of the API which people may start depending on. See https://google.github.io/styleguide/go/best-practices#adding-information-to-errors

It is best to avoid using %w unless you also document (and have tests that validate) the underlying errors that you expose. If you do not expect your caller to call errors.Unwrap, errors.Is and so on, don’t bother with %w.

arjan-bal · 2025-09-10T18:50:15Z

credentials/jwt/jwt_token_file_call_creds_test.go

+	impl.mu.Unlock()
+
+	if !nextRetryTime2.Equal(nextRetryTime) {
+		t.Errorf("nextRetryTime should not change due to backoff. Got: %v, Want: %v", nextRetryTime2, nextRetryTime)


This check seems to rely on the fact that time b/w the two GetRequestMetadata calls is always <= 0.8s. I think we should try to avoid relying on time like this because overloaded CI infra can cause test execution to slow down occasionally. Can we change the nextRetryTime to something larger than defaultTestTimeout before this verification?

Agreed; I've removed the reliance on the backoff strategy implementation entirely and now we control the nextRetryTime whenever we want to retry / ensure no retries are made.

arjan-bal · 2025-09-10T18:54:31Z

credentials/jwt/jwt_token_file_call_creds_test.go

+	// Third call should retry but still fail with UNAVAILABLE.
+	_, err3 := creds.GetRequestMetadata(ctx)
+	if err3 == nil {
+		t.Fatal("Expected cached error")


style: Use the word want instead of expect, e.g.:

t.Fatalf("creds.GetRequestMetadata() = %v, want non-nil". err)

See https://google.github.io/styleguide/go/decisions#got-before-want

arjan-bal · 2025-09-10T18:56:33Z

credentials/jwt/jwt_token_file_call_creds_test.go

+	impl.mu.Unlock()
+
+	// Third call should retry but still fail with UNAVAILABLE.
+	_, err3 := creds.GetRequestMetadata(ctx)


nit: Since we're not using the old err variable again, we can re-use the err names instead of adding numbers as suffixes.

arjan-bal · 2025-09-10T18:59:33Z

credentials/jwt/jwt_token_file_call_creds_test.go

+		t.Errorf("nextRetryTime should not change due to backoff. Got: %v, Want: %v", nextRetryTime4, nextRetryTime3)
+	}
+	if retryAttempt4 != retryAttempt3 {
+		t.Error("retry attempt should not change due to backoff")


style: Log message should being with a capital letter, like regular sentences.

arjan-bal · 2025-09-10T19:02:40Z

credentials/jwt/jwt_token_file_call_creds_test.go

+		t.Errorf("after creating valid token file, GetRequestMetadata() should eventually succeed, but got: %v", err5)
+		t.Error("backoff should expire and trigger new attempt on next RPC")


style: Can we combine these two into a single t.Fatalf call? This should also allow us to un-indent the else block below. It is fine if the log message takes up more horizontal space.
See https://google.github.io/styleguide/go/guide#line-length

arjan-bal · 2025-09-11T07:21:56Z

credentials/jwt/jwt_file_reader.go

+}
+
+// extractExpiration parses the JWT token to extract the expiration time.
+func (r *jWTFileReader) extractExpiration(token string) (time.Time, error) {


This functionality is preset in the go std lib.

import "golang.org/x/oauth2/jws" func (r *jwtFileReader) extractExpiration(token string) (time.Time, error) { cs, err := jws.Decode(token) if err != nil { return time.Time{}, err } expTime := time.Unix(cs.Exp, 0) if expTime.Before(time.Now()) { return time.Time{}, fmt.Errorf("expired token: %w", errJWTValidation) } return expTime, nil }

This is what the Google oauth2 implementation uses internally. The jws package is marked as deprecated with the following message:

Deprecated: this package is not intended for public use and might be removed in the future. It exists for internal use only. Please switch to another JWS package or copy this package into your own source tree.

I think we can take inspiration from their implementation.

arjan-bal · 2025-09-11T07:26:07Z

credentials/jwt/jwt_file_reader.go

+	// Add padding if necessary for base64 decoding.
+	if m := len(payload) % 4; m != 0 {
+		payload += strings.Repeat("=", 4-m)
+	}
+
+	payloadBytes, err := base64.URLEncoding.DecodeString(payload)


We can use base64.RawURLEncoding to handle padding for us. See the std lib implementation.

arjan-bal · 2025-09-11T07:34:52Z

credentials/jwt/jwt_file_reader.go

+	parts := strings.Split(token, ".")
+	if len(parts) != 3 {
+		return time.Time{}, fmt.Errorf("expected 3 parts, got %d: %w", len(parts), errJWTValidation)


We should use the peel-off style used by the std lib implementation as its more efficient. It avoids extra allocations only re-slices the existing string a couple of times.

…eReader

…hed value; also explain preemptive refresh test

dimpavloff

Thanks for the review @arjan-bal . Could you please take another look?

dimpavloff · 2025-09-11T10:26:38Z

credentials/jwt/jwt_token_file_call_creds.go

+
+// refreshToken reads the token from file and updates the cached data.
+func (c *jwtTokenFileCallCreds) refreshToken() {
+	// Deliberately not locking c.mu here


Is the mutex lock omitted here to avoid doing file IO while holding the lock?

Yes, exactly.

We should probably this requirement in the godoc of fileReader to serve as a good warning in case state is ever added to fileReader.

Good idea! 👍

dimpavloff · 2025-09-11T10:27:52Z

credentials/jwt/jwt_token_file_call_creds_test.go

+	grpctest.Tester
+}
+
+func TestTokenFileCallCreds(t *testing.T) {


Oh, I see, nicely spotted. Thanks

dimpavloff · 2025-09-11T10:29:55Z

credentials/jwt/jwt_token_file_call_creds_test.go

+			name:         "insufficient_security_level",
+			tokenContent: createTestJWT(t, now.Add(time.Hour)),
+			authInfo:     &testAuthInfo{secLevel: credentials.NoSecurity},
+			wantCode:     codes.Unknown, // http2Client.getCallAuthData actually transforms such errors into into Unauthenticated


I was referring to a few lines above --

grpc-go/internal/transport/http2_client.go

Line 691 in c122250

return nil, status.Error(codes.Unauthenticated, "transport: cannot send secure credentials on an insecure connection")

dimpavloff · 2025-09-11T10:33:06Z

credentials/jwt/jwt_token_file_call_creds_test.go

+		{
+			name:             "unreachable_token_file",
+			invalidTokenPath: true,
+			tokenContent:     "",


Agree but because I am varying the value across the test cases, I thought it would be clearer to the reader what the intent of each test is as it's easier to compare visually.
The rule you quoted supports this: Zero-value fields may be omitted from struct literals when clarity is not lost as a result.

My preference would be to keep it as is but happy to remove if you insist.

dimpavloff · 2025-09-12T13:22:02Z

credentials/jwt/jwt_token_file_call_creds_test.go

+		t.Fatal("token should be cached after successful GetRequestMetadata")
+	}
+
+	if !shouldTriggerRefresh {


It just explicitly confirms the precondition necessary for the rest of the test -- the test assumes that the subsequent call (the second call) will trigger a refresh. So this test will fail sooner if this doesn't happen.

I've updated the failure message to something more useful but happy to remove it, too, as it's not strictly needed.

dimpavloff · 2025-09-12T15:24:10Z

credentials/jwt/jwt_token_file_call_creds_test.go

+	impl.mu.Unlock()
+
+	if !nextRetryTime2.Equal(nextRetryTime) {
+		t.Errorf("nextRetryTime should not change due to backoff. Got: %v, Want: %v", nextRetryTime2, nextRetryTime)


Agreed; I've removed the reliance on the backoff strategy implementation entirely and now we control the nextRetryTime whenever we want to retry / ensure no retries are made.

dimpavloff · 2025-09-12T15:24:58Z

credentials/jwt/jwt_token_file_call_creds_test.go

+	if retryAttempt != 1 {
+		t.Errorf("Expected retry attempt to be 1, got %d", retryAttempt)
+	}
+	if nextRetryTime.IsZero() || nextRetryTime.Before(time.Now()) {


Hmm, doesn't this make the test a bit more noisy as it will have to be aware of the backoff details, which is another implementation detail, calculate the above, and explain it?

I've attempted to improve the test flow a bit. Please lmk what you think.

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

arjan-bal reviewed Sep 10, 2025

View reviewed changes

arjan-bal assigned gtcooke94 and matthewstevenson88 Sep 10, 2025

arjan-bal reviewed Sep 10, 2025

View reviewed changes

dimpavloff added 3 commits September 10, 2025 16:05

rename jWTFileReader to jwtFileReader

2c9a06d

move error wrapping

42c6804

remove leftover package docstring

f1a1cd3

dimpavloff requested a review from arjan-bal September 10, 2025 15:20

arjan-bal reviewed Sep 10, 2025

View reviewed changes

arjan-bal reviewed Sep 11, 2025

View reviewed changes

arjan-bal assigned dimpavloff and unassigned gtcooke94, matthewstevenson88 and arjan-bal Sep 11, 2025

dimpavloff added 13 commits September 11, 2025 10:34

use RawURLEncoding.DecodeString

2b8ae01

%v instead of %w in credentials.CheckSecurityLevel error string

1b5a609

single check for preemptive refresh

4c30c68

clarify why lock is not used and document concurrent calls for jwtFil…

0c15d73

…eReader

rename test suite function name

7d5f578

trailing brace in comment

c8852fc

add comments to clarify we do not trigger refresh on updating the cac…

0be0243

…hed value; also explain preemptive refresh test

shouldTriggerRefresh failure message update

36042db

re-use err instead of err1,2,3,4,5

7e50e3e

improve err==nil failure message in test

8e3b91b

t.Fatal and t.Error message capitalisation where possible

4ac6f4c

combine t.Error into a single t.Fatal and indent

e83fbee

attempt to make the token referesh retry backoff test more readable

75fbc02

dimpavloff commented Sep 12, 2025

View reviewed changes

dimpavloff requested a review from arjan-bal September 12, 2025 15:28

		t.Errorf("after creating valid token file, GetRequestMetadata() should eventually succeed, but got: %v", err5)
		t.Error("backoff should expire and trigger new attempt on next RPC")

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 commented Sep 10, 2025

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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

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!

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!

Choose a reason for hiding this comment

Uh oh!

arjan-bal Sep 10, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dimpavloff commented Jul 7, 2025 •

edited

Loading

codecov bot commented Jul 7, 2025 •

edited

Loading

dimpavloff Sep 11, 2025 •

edited

Loading

arjan-bal Sep 10, 2025 •

edited

Loading

arjan-bal Sep 11, 2025 •

edited

Loading