Skip to content

Commit 0de5a71

Browse files
committed
feat(auth): fix ClientSideCredentialAccessBoundary race condition
This change addresses a race condition in ClientSideCredentialAccessBoundaryFactory that occurred when multiple concurrent calls were made to generateToken. The fix involves: - Waiting on the RefreshTask itself rather than its internal task. - Using a single listener in RefreshTask to ensure finishRefreshTask completes before the outer future unblocks waiting threads. - Adding a regression test generateToken_freshInstance_concurrent_noNpe.
1 parent 4cb170d commit 0de5a71

File tree

2 files changed

+62
-21
lines changed

2 files changed

+62
-21
lines changed

google-auth-library-java/cab-token-generator/java/com/google/auth/credentialaccessboundary/ClientSideCredentialAccessBoundaryFactory.java

Lines changed: 7 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -248,7 +248,7 @@ void refreshCredentialsIfRequired() throws IOException {
248248
}
249249
try {
250250
// Wait for the refresh task to complete.
251-
currentRefreshTask.task.get();
251+
currentRefreshTask.get();
252252
} catch (InterruptedException e) {
253253
// Restore the interrupted status and throw an exception.
254254
Thread.currentThread().interrupt();
@@ -495,31 +495,17 @@ class RefreshTask extends AbstractFuture<IntermediateCredentials> implements Run
495495
this.task = task;
496496
this.isNew = isNew;
497497

498-
// Add listener to update factory's credentials when the task completes.
498+
// Single listener to guarantee that finishRefreshTask updates the internal state BEFORE
499+
// the outer future completes and unblocks waiters.
499500
task.addListener(
500501
() -> {
501502
try {
502503
finishRefreshTask(task);
504+
RefreshTask.this.set(Futures.getDone(task));
503505
} catch (ExecutionException e) {
504-
Throwable cause = e.getCause();
505-
RefreshTask.this.setException(cause);
506-
}
507-
},
508-
MoreExecutors.directExecutor());
509-
510-
// Add callback to set the result or exception based on the outcome.
511-
Futures.addCallback(
512-
task,
513-
new FutureCallback<IntermediateCredentials>() {
514-
@Override
515-
public void onSuccess(IntermediateCredentials result) {
516-
RefreshTask.this.set(result);
517-
}
518-
519-
@Override
520-
public void onFailure(@Nullable Throwable t) {
521-
RefreshTask.this.setException(
522-
t != null ? t : new IOException("Refresh failed with null Throwable."));
506+
RefreshTask.this.setException(e.getCause());
507+
} catch (Exception e) {
508+
RefreshTask.this.setException(e);
523509
}
524510
},
525511
MoreExecutors.directExecutor());

google-auth-library-java/cab-token-generator/javatests/com/google/auth/credentialaccessboundary/ClientSideCredentialAccessBoundaryFactoryTest.java

Lines changed: 55 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -988,4 +988,59 @@ void generateToken_withMalformSessionKey_failure() throws Exception {
988988

989989
assertThrows(GeneralSecurityException.class, () -> factory.generateToken(accessBoundary));
990990
}
991+
992+
@Test
993+
void generateToken_freshInstance_concurrent_noNpe() throws Exception {
994+
for (int run = 0; run < 10; run++) { // Run 10 times in a single test instance to save time
995+
GoogleCredentials sourceCredentials =
996+
getServiceAccountSourceCredentials(mockTokenServerTransportFactory);
997+
ClientSideCredentialAccessBoundaryFactory factory =
998+
ClientSideCredentialAccessBoundaryFactory.newBuilder()
999+
.setSourceCredential(sourceCredentials)
1000+
.setHttpTransportFactory(mockStsTransportFactory)
1001+
.build();
1002+
1003+
CredentialAccessBoundary.Builder cabBuilder = CredentialAccessBoundary.newBuilder();
1004+
CredentialAccessBoundary accessBoundary =
1005+
cabBuilder
1006+
.addRule(
1007+
CredentialAccessBoundary.AccessBoundaryRule.newBuilder()
1008+
.setAvailableResource("resource")
1009+
.setAvailablePermissions(ImmutableList.of("role"))
1010+
.build())
1011+
.build();
1012+
1013+
int numThreads = 5;
1014+
Thread[] threads = new Thread[numThreads];
1015+
CountDownLatch latch = new CountDownLatch(numThreads);
1016+
java.util.concurrent.atomic.AtomicInteger npeCount =
1017+
new java.util.concurrent.atomic.AtomicInteger();
1018+
1019+
for (int i = 0; i < numThreads; i++) {
1020+
threads[i] =
1021+
new Thread(
1022+
() -> {
1023+
try {
1024+
latch.countDown();
1025+
latch.await();
1026+
factory.generateToken(accessBoundary);
1027+
} catch (NullPointerException e) {
1028+
npeCount.incrementAndGet();
1029+
} catch (Exception e) {
1030+
// Ignore other exceptions for the sake of the race reproduction
1031+
}
1032+
});
1033+
threads[i].start();
1034+
}
1035+
1036+
for (Thread thread : threads) {
1037+
thread.join();
1038+
}
1039+
1040+
org.junit.jupiter.api.Assertions.assertEquals(
1041+
0,
1042+
npeCount.get(),
1043+
"Expected zero NullPointerExceptions due to the race condition, but some were thrown.");
1044+
}
1045+
}
9911046
}

0 commit comments

Comments
 (0)