Skip to content

Revert "fix(auth): Address ClientSideCredentialAccessBoundary RefreshTask race condition" #12770

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

Closed
lqiu96 wants to merge 1 commit into from
+22 −62
Closed

Revert "fix(auth): Address ClientSideCredentialAccessBoundary RefreshTask race condition" #12770

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
29 changes: 22 additions & 7 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 @@ -53,6 +53,7 @@
import com.google.common.annotations.VisibleForTesting;
import com.google.common.base.Strings;
import com.google.common.util.concurrent.AbstractFuture;
import com.google.common.util.concurrent.FutureCallback;
import com.google.common.util.concurrent.Futures;
import com.google.common.util.concurrent.ListenableFuture;
import com.google.common.util.concurrent.ListenableFutureTask;
Expand All @@ -78,6 +79,7 @@
import java.util.Date;
import java.util.List;
import java.util.concurrent.ExecutionException;
import javax.annotation.Nullable;

/**
* A factory for generating downscoped access tokens using a client-side approach.
Expand Down Expand Up @@ -246,7 +248,7 @@ void refreshCredentialsIfRequired() throws IOException {
}
try {
// Wait for the refresh task to complete.
currentRefreshTask.get();
currentRefreshTask.task.get();
Copy link
Copy Markdown
Contributor

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

Choose a reason for hiding this comment

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

high

Reverting this to wait on currentRefreshTask.task.get() instead of currentRefreshTask.get() re-introduces a race condition. The internal task completes as soon as the network request is done, but before the factory state has been updated via finishRefreshTask. This allows concurrent calls to proceed with potentially stale or incomplete state.

Suggested change
currentRefreshTask.task.get();
currentRefreshTask.get();

} catch (InterruptedException e) {
// Restore the interrupted status and throw an exception.
Thread.currentThread().interrupt();
Expand Down Expand Up @@ -493,18 +495,31 @@ class RefreshTask extends AbstractFuture<IntermediateCredentials> implements Run
this.task = task;
this.isNew = isNew;

// Single listener to guarantee that finishRefreshTask updates the internal state BEFORE
// the outer future completes and unblocks waiters.
// Add listener to update factory's credentials when the task completes.
task.addListener(
() -> {
try {
finishRefreshTask(task);
RefreshTask.this.set(Futures.getDone(task));
} catch (ExecutionException e) {
Throwable cause = e.getCause();
RefreshTask.this.setException(cause != null ? cause : e);
} catch (Throwable t) {
RefreshTask.this.setException(t);
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."));
}
},
MoreExecutors.directExecutor());
Comment on lines +498 to 525
Copy link
Copy Markdown
Contributor

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

Choose a reason for hiding this comment

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

high

This implementation re-introduces a race condition by using two separate listeners. There is no guarantee that finishRefreshTask (which updates the factory state) will execute before RefreshTask.this.set(result) (which unblocks waiters). Additionally, line 505 is not null-safe and will throw a NullPointerException if cause is null. It is highly recommended to use a single listener to ensure correct sequencing.

      // 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 != null ? cause : e);
            } catch (Throwable t) {
              RefreshTask.this.setException(t);
            }
          },
          MoreExecutors.directExecutor());

Expand Down
55 changes: 0 additions & 55 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,59 +988,4 @@ 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;
CountDownLatch latch = new CountDownLatch(numThreads);
java.util.concurrent.atomic.AtomicInteger npeCount =
new java.util.concurrent.atomic.AtomicInteger();
java.util.concurrent.ExecutorService executor =
java.util.concurrent.Executors.newFixedThreadPool(numThreads);

try {
for (int i = 0; i < numThreads; i++) {
executor.submit(
() -> {
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
}
});
}
} finally {
executor.shutdown();
executor.awaitTermination(5, java.util.concurrent.TimeUnit.SECONDS);
}

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

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

Choose a reason for hiding this comment

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

medium

The removal of the generateToken_freshInstance_concurrent_noNpe regression test is discouraged. This test was specifically designed to catch the race condition that is being re-introduced by this revert. Keeping the test ensures that the concurrency issue is properly tracked and eventually fixed without regression.

Loading