Skip to content

Commit bf96f1a

Browse files
committed
fix: prevent data races in concurrent token introspection
- Clone result templates in gRPC introspection to prevent shared state across concurrent calls - Clone singleflight results before returning to prevent caller modifications affecting concurrent requests - Add comprehensive concurrency tests covering gRPC introspection, cache paths, and singleflight scenarios This ensures each introspection request receives an isolated result instance, preventing data races when multiple goroutines perform concurrent introspections with custom result templates.
1 parent 75767e0 commit bf96f1a

File tree

4 files changed

+452
-5
lines changed

4 files changed

+452
-5
lines changed

idptoken/grpc_client.go

Lines changed: 9 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -165,10 +165,17 @@ func (c *GRPCClient) IntrospectToken(
165165
c.setSessionID(sessionIDMeta[0])
166166
}
167167

168-
result := c.resultTemplate
169-
if result == nil {
168+
// Create a new result instance for this introspection request.
169+
// If a custom result template is configured, clone it to get a fresh instance.
170+
// This is critical for thread-safety: without cloning, concurrent gRPC introspection
171+
// calls would share the same resultTemplate instance, leading to data races.
172+
var result IntrospectionResult
173+
if c.resultTemplate != nil {
174+
result = c.resultTemplate.Clone()
175+
} else {
170176
result = &DefaultIntrospectionResult{}
171177
}
178+
172179
result.SetIsActive(resp.GetActive())
173180
if !result.IsActive() {
174181
return result, nil

idptoken/grpc_client_test.go

Lines changed: 162 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -348,6 +348,168 @@ func TestGRPCClient_IntrospectToken_CustomClaims(t *gotesting.T) {
348348
require.Equal(t, 42, clonedCustomResult.CustomIntField)
349349
}
350350

351+
// TestGRPCClient_IntrospectToken_ConcurrentCloning tests that concurrent
352+
// introspection calls don't cause data races with result template cloning.
353+
func TestGRPCClient_IntrospectToken_ConcurrentCloning(t *gotesting.T) {
354+
const validAccessToken = "access-token-with-introspection-permission"
355+
const numGoroutines = 100
356+
357+
opaqueToken := "opaque-token-" + uuid.NewString()
358+
opaqueTokenScope := []jwt.AccessPolicy{{
359+
TenantUUID: uuid.NewString(),
360+
ResourceNamespace: "account-server",
361+
Role: "admin",
362+
ResourcePath: "resource-" + uuid.NewString(),
363+
}}
364+
opaqueTokenRegClaims := jwtgo.RegisteredClaims{
365+
ExpiresAt: jwtgo.NewNumericDate(time.Now().Add(time.Hour)),
366+
}
367+
368+
jwtScopeToGRPC := func(jwtScope []jwt.AccessPolicy) []*pb.AccessTokenScope {
369+
grpcScope := make([]*pb.AccessTokenScope, len(jwtScope))
370+
for i, scope := range jwtScope {
371+
grpcScope[i] = &pb.AccessTokenScope{
372+
TenantUuid: scope.TenantUUID,
373+
ResourceNamespace: scope.ResourceNamespace,
374+
RoleName: scope.Role,
375+
ResourcePath: scope.ResourcePath,
376+
}
377+
}
378+
return grpcScope
379+
}
380+
381+
t.Run("custom result template", func(t *gotesting.T) {
382+
grpcServerTokenIntrospector := testing.NewGRPCServerTokenIntrospectorMock()
383+
grpcServerTokenIntrospector.SetAccessTokenForIntrospection(validAccessToken)
384+
grpcServerTokenIntrospector.SetResultForToken(opaqueToken, &pb.IntrospectTokenResponse{
385+
Active: true,
386+
TokenType: idputil.TokenTypeBearer,
387+
Aud: opaqueTokenRegClaims.Audience,
388+
Exp: opaqueTokenRegClaims.ExpiresAt.Unix(),
389+
Scope: jwtScopeToGRPC(opaqueTokenScope),
390+
CustomClaimsJson: `{"custom_string_field":"custom_value","custom_int_field":42}`,
391+
}, nil)
392+
393+
grpcIDPSrv := idptest.NewGRPCServer(idptest.WithGRPCTokenIntrospector(grpcServerTokenIntrospector))
394+
require.NoError(t, grpcIDPSrv.StartAndWaitForReady(time.Second))
395+
defer func() { grpcIDPSrv.GracefulStop() }()
396+
397+
grpcClient, err := idptoken.NewGRPCClientWithOpts(
398+
grpcIDPSrv.Addr(),
399+
insecure.NewCredentials(),
400+
idptoken.GRPCClientOpts{
401+
ResultTemplate: &CustomIntrospectionResult{},
402+
},
403+
)
404+
require.NoError(t, err)
405+
defer func() { require.NoError(t, grpcClient.Close()) }()
406+
407+
// Run concurrent introspection requests
408+
errChan := make(chan error, numGoroutines)
409+
startCh := make(chan struct{})
410+
for i := 0; i < numGoroutines; i++ {
411+
go func(index int) {
412+
<-startCh // Wait for signal to start all goroutines simultaneously
413+
result, introspectErr := grpcClient.IntrospectToken(context.Background(), opaqueToken, nil, validAccessToken)
414+
if introspectErr != nil {
415+
errChan <- introspectErr
416+
return
417+
}
418+
419+
customResult, ok := result.(*CustomIntrospectionResult)
420+
if !ok {
421+
errChan <- fmt.Errorf("goroutine %d: result should be of type CustomIntrospectionResult", index)
422+
return
423+
}
424+
425+
if customResult.CustomStringField != "custom_value" {
426+
errChan <- fmt.Errorf("goroutine %d: expected CustomStringField='custom_value', got '%s'",
427+
index, customResult.CustomStringField)
428+
return
429+
}
430+
431+
if customResult.CustomIntField != 42 {
432+
errChan <- fmt.Errorf("goroutine %d: expected CustomIntField=42, got %d",
433+
index, customResult.CustomIntField)
434+
return
435+
}
436+
437+
// Modify the result - this would cause a race if the same object is shared
438+
customResult.CustomStringField = fmt.Sprintf("modified_by_goroutine_%d", index)
439+
customResult.CustomIntField = index
440+
441+
errChan <- nil
442+
}(i)
443+
}
444+
445+
close(startCh) // Start all goroutines at once
446+
447+
// Collect results
448+
for i := 0; i < numGoroutines; i++ {
449+
err := <-errChan
450+
require.NoError(t, err)
451+
}
452+
})
453+
454+
t.Run("default result template", func(t *gotesting.T) {
455+
grpcServerTokenIntrospector := testing.NewGRPCServerTokenIntrospectorMock()
456+
grpcServerTokenIntrospector.SetAccessTokenForIntrospection(validAccessToken)
457+
grpcServerTokenIntrospector.SetResultForToken(opaqueToken, &pb.IntrospectTokenResponse{
458+
Active: true,
459+
TokenType: idputil.TokenTypeBearer,
460+
Aud: opaqueTokenRegClaims.Audience,
461+
Exp: opaqueTokenRegClaims.ExpiresAt.Unix(),
462+
Scope: jwtScopeToGRPC(opaqueTokenScope),
463+
}, nil)
464+
465+
grpcIDPSrv := idptest.NewGRPCServer(idptest.WithGRPCTokenIntrospector(grpcServerTokenIntrospector))
466+
require.NoError(t, grpcIDPSrv.StartAndWaitForReady(time.Second))
467+
defer func() { grpcIDPSrv.GracefulStop() }()
468+
469+
grpcClient, err := idptoken.NewGRPCClient(grpcIDPSrv.Addr(), insecure.NewCredentials())
470+
require.NoError(t, err)
471+
defer func() { require.NoError(t, grpcClient.Close()) }()
472+
473+
// Run concurrent introspection requests
474+
errChan := make(chan error, numGoroutines)
475+
startCh := make(chan struct{})
476+
for i := 0; i < numGoroutines; i++ {
477+
go func(index int) {
478+
<-startCh // Wait for signal to start all goroutines simultaneously
479+
result, introspectErr := grpcClient.IntrospectToken(context.Background(), opaqueToken, nil, validAccessToken)
480+
if introspectErr != nil {
481+
errChan <- introspectErr
482+
return
483+
}
484+
485+
if !result.IsActive() {
486+
errChan <- fmt.Errorf("goroutine %d: expected active token", index)
487+
return
488+
}
489+
490+
if result.GetTokenType() != idputil.TokenTypeBearer {
491+
errChan <- fmt.Errorf("goroutine %d: expected token type %s, got %s",
492+
index, idputil.TokenTypeBearer, result.GetTokenType())
493+
return
494+
}
495+
496+
// Modify the result - this would cause a race if the same object is shared
497+
result.SetTokenType(fmt.Sprintf("modified_by_goroutine_%d", index))
498+
499+
errChan <- nil
500+
}(i)
501+
}
502+
503+
close(startCh) // Start all goroutines at once
504+
505+
// Collect results
506+
for i := 0; i < numGoroutines; i++ {
507+
err := <-errChan
508+
require.NoError(t, err)
509+
}
510+
})
511+
}
512+
351513
func TestGRPCClient_ExchangeToken(t *gotesting.T) {
352514
tokenExpiresIn := time.Hour
353515
tokenExpiresAt := time.Now().Add(time.Hour)

idptoken/introspector.go

Lines changed: 14 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -369,10 +369,14 @@ func (i *Introspector) IntrospectToken(ctx context.Context, token string) (Intro
369369
if err := i.validateClaims(cachedItem.IntrospectionResult.GetClaims()); err != nil {
370370
return nil, fmt.Errorf("validate claims of cached introspection result: %w", err)
371371
}
372+
// Clone the cached result to prevent data races. Without cloning, concurrent calls
373+
// would receive the same cached instance, and modifications by one goroutine could
374+
// affect other goroutines reading the same object.
372375
return cachedItem.IntrospectionResult.Clone(), nil
373376
}
374377

375378
if cachedItem, ok := i.NegativeCache.Get(ctx, cacheKey); ok {
379+
// Clone the cached result for thread-safety (same reason as above).
376380
return cachedItem.IntrospectionResult.Clone(), nil
377381
}
378382

@@ -382,11 +386,11 @@ func (i *Introspector) IntrospectToken(ctx context.Context, token string) (Intro
382386
return nil, err
383387
}
384388
if !result.IsActive() {
385-
i.NegativeCache.Add(ctx, cacheKey, IntrospectionCacheItem{IntrospectionResult: result.Clone(), CreatedAt: time.Now()})
389+
i.NegativeCache.Add(ctx, cacheKey, IntrospectionCacheItem{IntrospectionResult: result, CreatedAt: time.Now()})
386390
return result, nil
387391
}
388392
result.GetClaims().ApplyScopeFilter(i.scopeFilter)
389-
i.ClaimsCache.Add(ctx, cacheKey, IntrospectionCacheItem{IntrospectionResult: result.Clone(), CreatedAt: time.Now()})
393+
i.ClaimsCache.Add(ctx, cacheKey, IntrospectionCacheItem{IntrospectionResult: result, CreatedAt: time.Now()})
390394
if err = i.validateClaims(result.GetClaims()); err != nil {
391395
return nil, fmt.Errorf("validate claims of received introspection result: %w", err)
392396
}
@@ -395,7 +399,10 @@ func (i *Introspector) IntrospectToken(ctx context.Context, token string) (Intro
395399
if err != nil {
396400
return nil, err // already wrapped
397401
}
398-
return resultVal.(IntrospectionResult), nil
402+
// Clone the result before returning to ensure thread-safety.
403+
// This prevents the caller from modifying the singleflight-shared result object,
404+
// which could affect other concurrent callers waiting for the same singleflight operation.
405+
return resultVal.(IntrospectionResult).Clone(), nil
399406
}
400407

401408
// AddTrustedIssuer adds trusted issuer with specified name and URL.
@@ -633,6 +640,10 @@ func (i *Introspector) makeIntrospectFuncHTTP(introspectionEndpointURL string) i
633640
return nil, unexpectedCodeErr
634641
}
635642

643+
// Create a new result instance for this HTTP introspection request.
644+
// If a custom result template is configured, clone it to get a fresh instance.
645+
// This is critical for thread-safety: without cloning, concurrent HTTP introspection
646+
// calls would share the same resultTemplate instance, leading to data races.
636647
var res IntrospectionResult
637648
if i.resultTemplate != nil {
638649
res = i.resultTemplate.Clone()

0 commit comments

Comments
 (0)