Skip to content

feat(auth): fix ClientSideCredentialAccessBoundary race condition #12681

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
vverman wants to merge 1 commit into
base: main
Choose a base branch
from
Open

feat(auth): fix ClientSideCredentialAccessBoundary race condition #12681

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
28 changes: 7 additions & 21 deletions ...a/com/google/auth/credentialaccessboundary/ClientSideCredentialAccessBoundaryFactory.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -248,7 +248,7 @@ void refreshCredentialsIfRequired() throws IOException {
}
try {
// Wait for the refresh task to complete.
currentRefreshTask.task.get();
currentRefreshTask.get();
} catch (InterruptedException e) {
// Restore the interrupted status and throw an exception.
Thread.currentThread().interrupt();
Expand Down Expand Up @@ -495,31 +495,17 @@ class RefreshTask extends AbstractFuture<IntermediateCredentials> implements Run
this.task = task;
this.isNew = isNew;

// Add listener to update factory's credentials when the task completes.
// Single listener to guarantee that finishRefreshTask updates the internal state BEFORE
// the outer future completes and unblocks waiters.
task.addListener(
() -> {
try {
finishRefreshTask(task);
RefreshTask.this.set(Futures.getDone(task));
} catch (ExecutionException e) {
Throwable cause = e.getCause();
RefreshTask.this.setException(cause);
}
},
MoreExecutors.directExecutor());

// Add callback to set the result or exception based on the outcome.
Futures.addCallback(
task,
new FutureCallback<IntermediateCredentials>() {
@Override
public void onSuccess(IntermediateCredentials result) {
RefreshTask.this.set(result);
}

@Override
public void onFailure(@Nullable Throwable t) {
RefreshTask.this.setException(
t != null ? t : new IOException("Refresh failed with null Throwable."));
RefreshTask.this.setException(e.getCause());
} catch (Exception e) {
RefreshTask.this.setException(e);
}
Comment on lines 505 to 509
Copy link
Copy Markdown
Contributor

@gemini-code-assist gemini-code-assist bot Apr 5, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

The error handling in this listener is less robust than the previous implementation. Specifically:

  1. Catching Exception instead of Throwable: If finishRefreshTask or Futures.getDone throws an Error (e.g., OutOfMemoryError or NoClassDefFoundError), it will not be caught. This would cause the listener to terminate abruptly without completing the RefreshTask future, leading to all threads waiting on currentRefreshTask.get() hanging indefinitely.
  2. Potential NullPointerException: RefreshTask.this.setException(e.getCause()) will throw a NullPointerException if e.getCause() is null (which is technically possible for an ExecutionException, though rare from Future.get()). Since AbstractFuture.setException does not allow null arguments, this would also cause the listener to crash and the future to hang.

Using Throwable and adding a null check for the cause ensures the future is always completed even in extreme failure scenarios.

            } catch (ExecutionException e) {
              Throwable cause = e.getCause();
              RefreshTask.this.setException(cause != null ? cause : e);
            } catch (Throwable t) {
              RefreshTask.this.setException(t);
            }

},
MoreExecutors.directExecutor());
Expand Down
55 changes: 55 additions & 0 deletions ...m/google/auth/credentialaccessboundary/ClientSideCredentialAccessBoundaryFactoryTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -988,4 +988,59 @@ void generateToken_withMalformSessionKey_failure() throws Exception {

assertThrows(GeneralSecurityException.class, () -> factory.generateToken(accessBoundary));
}

@Test
void generateToken_freshInstance_concurrent_noNpe() throws Exception {
for (int run = 0; run < 10; run++) { // Run 10 times in a single test instance to save time
GoogleCredentials sourceCredentials =
getServiceAccountSourceCredentials(mockTokenServerTransportFactory);
ClientSideCredentialAccessBoundaryFactory factory =
ClientSideCredentialAccessBoundaryFactory.newBuilder()
.setSourceCredential(sourceCredentials)
.setHttpTransportFactory(mockStsTransportFactory)
.build();

CredentialAccessBoundary.Builder cabBuilder = CredentialAccessBoundary.newBuilder();
CredentialAccessBoundary accessBoundary =
cabBuilder
.addRule(
CredentialAccessBoundary.AccessBoundaryRule.newBuilder()
.setAvailableResource("resource")
.setAvailablePermissions(ImmutableList.of("role"))
.build())
.build();

int numThreads = 5;
Thread[] threads = new Thread[numThreads];
CountDownLatch latch = new CountDownLatch(numThreads);
java.util.concurrent.atomic.AtomicInteger npeCount =
new java.util.concurrent.atomic.AtomicInteger();

for (int i = 0; i < numThreads; i++) {
threads[i] =
new Thread(
() -> {
try {
latch.countDown();
latch.await();
factory.generateToken(accessBoundary);
} catch (NullPointerException e) {
npeCount.incrementAndGet();
} catch (Exception e) {
// Ignore other exceptions for the sake of the race reproduction
}
});
threads[i].start();
}

for (Thread thread : threads) {
thread.join();
}

org.junit.jupiter.api.Assertions.assertEquals(
0,
npeCount.get(),
"Expected zero NullPointerExceptions due to the race condition, but some were thrown.");
}
}
}
Loading