Skip to content

ticket-777: Fix the bug when IMDS fails during auth token refresh whi… #780

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
k2chogori wants to merge 1 commit into
base: legacy/v2/master
Choose a base branch
from
Open

ticket-777: Fix the bug when IMDS fails during auth token refresh whi… #780

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
98 changes: 53 additions & 45 deletions bmc-common/src/main/java/com/oracle/bmc/auth/internal/X509FederationClient.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -237,55 +237,14 @@ private String refreshAndGetSecurityTokenInner(
}

if (iAmTheLeader) {
LOG.info("[Leader] About to refresh security token.");

if (refreshKeys) {
LOG.info("Refreshing session keys.");
sessionKeySupplier.refreshKeys();
}
if (leafCertificateSupplier instanceof Refreshable) {
try {
((Refreshable) leafCertificateSupplier).refresh();
} catch (RefreshFailedException ex) {
throw new BmcException(
false, "Can't refresh the leaf certification!", ex, null);
}
// When using default purpose (ex, instance principals), the token request should always be signed with the same tenant id as the certificate.
// For other purposes, the tenant id can be different.
if (this.purpose.equals(DEFAULT_PURPOSE)) {
String newTenancyId =
AuthUtils.getTenantIdFromCertificate(
leafCertificateSupplier
.getCertificateAndKeyPair()
.getCertificate());

if (!this.tenancyId.equals(newTenancyId)) {
throw new IllegalArgumentException(
"The tenancy id should never be changed in cert file!");
}
}
}

for (X509CertificateSupplier supplier : intermediateCertificateSuppliers) {
if (supplier instanceof Refreshable) {
try {
((Refreshable) supplier).refresh();
} catch (RefreshFailedException ex) {
throw new BmcException(
false, "Can't refresh the intermediate certification!", ex, null);
}
}
}

try {
securityTokenAdapter = getSecurityTokenFromServer();
String token = securityTokenAdapter.getSecurityToken();
LOG.info("[Leader] About to refresh security token.");
String token = refreshSecurityToken(refreshKeys);
future.complete(token);
return token;
} catch (Exception e) {
LOG.error("Error refreshing security token", e);
} catch (RuntimeException e) {
future.completeExceptionally(e);
throw new BmcException(false, "Error refreshing security token.", e, null);
throw e;
} finally {
inFlightRefresh = null;
}
Expand All @@ -310,6 +269,55 @@ private String refreshAndGetSecurityTokenInner(
}
}

private String refreshSecurityToken(boolean refreshKeys) {
if (refreshKeys) {
LOG.info("Refreshing session keys.");
sessionKeySupplier.refreshKeys();
}
if (leafCertificateSupplier instanceof Refreshable) {
try {
((Refreshable) leafCertificateSupplier).refresh();
} catch (RefreshFailedException ex) {
throw new BmcException(
false,
"Unable to refresh the client certificate used to obtain an OCI security token.",
ex,
null);
}
// When using default purpose (ex, instance principals), the token request should always be signed with the same tenant id as the certificate.
// For other purposes, the tenant id can be different.
if (this.purpose.equals(DEFAULT_PURPOSE)) {
String newTenancyId =
AuthUtils.getTenantIdFromCertificate(
leafCertificateSupplier.getCertificateAndKeyPair().getCertificate());

if (!this.tenancyId.equals(newTenancyId)) {
throw new IllegalArgumentException(
"The tenancy id should never be changed in cert file!");
}
}
}

for (X509CertificateSupplier supplier : intermediateCertificateSuppliers) {
if (supplier instanceof Refreshable) {
try {
((Refreshable) supplier).refresh();
} catch (RefreshFailedException ex) {
throw new BmcException(
false, "Can't refresh the intermediate certification!", ex, null);
}
}
}

try {
securityTokenAdapter = getSecurityTokenFromServer();
return securityTokenAdapter.getSecurityToken();
} catch (Exception e) {
LOG.error("Error refreshing security token", e);
throw new BmcException(false, "Error refreshing security token.", e, null);
}
}

/**
* Gets a security token from the federation server
* @return the security token, which is basically a JWT token string
Expand Down
145 changes: 145 additions & 0 deletions bmc-common/src/test/java/com/oracle/bmc/auth/internal/X509FederationClientTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,9 +14,12 @@
import com.oracle.bmc.model.BmcException;
import com.oracle.bmc.requests.BmcRequest;
import java.security.KeyPairGenerator;
import java.security.cert.X509Certificate;
import java.util.ArrayList;
import java.util.concurrent.CountDownLatch;
import java.util.concurrent.TimeUnit;
import java.util.concurrent.atomic.AtomicInteger;
import java.util.concurrent.atomic.AtomicReference;
import org.junit.Before;
import org.junit.Test;
import org.junit.runner.RunWith;
Expand All @@ -30,6 +33,8 @@

import javax.ws.rs.client.Invocation;
import javax.ws.rs.core.Response;
import javax.security.auth.RefreshFailedException;
import javax.security.auth.Refreshable;

import java.io.IOException;
import java.net.URI;
Expand Down Expand Up @@ -312,4 +317,144 @@ public void jacksonCanRoundTripSecurityToken() throws IOException {
1,
serverCallCount.get());
}

/**
* Regression test for:
*
* <ul>
* <li><a href="https://jira.oci.oraclecorp.com/browse/TOSIM-3623">TOSIM-3623</a></li>
* <li><a href="https://github.com/oracle/oci-java-sdk/issues/777">OCI Java SDK issue 777</a></li>
* <li><a href="https://jira.oci.oraclecorp.com/browse/DEX-25556">DEX-25556</a></li>
* </ul>
*
* <p>This test simulates the Instance Metadata Service (IMDS) returning HTTP 404 while the
* leader refreshes {@code cert.pem}. The leader has already created {@code inFlightRefresh}; a
* concurrent follower therefore waits on that Future instead of starting another refresh.
*
* <p>The test verifies that the leader completes {@code inFlightRefresh} exceptionally before
* it returns. The follower must then receive the same error promptly, rather than waiting for
* the one-minute single-flight timeout.
*/
@Test
public void
refreshFailureBeforeTokenRequest_completesInFlightRefreshSoFollowerDoesNotWait()
throws Exception {
CountDownLatch leaderStartedCertificateRefresh = new CountDownLatch(1);
CountDownLatch failCertificateRefresh = new CountDownLatch(1);
AtomicReference<Throwable> leaderFailure = new AtomicReference<>();
AtomicReference<Throwable> followerFailure = new AtomicReference<>();

class RefreshableFailingLeafCertificateSupplier
implements X509CertificateSupplier, Refreshable {
@Override
public boolean isCurrent() {
return false;
}

@Override
public void refresh() throws RefreshFailedException {
leaderStartedCertificateRefresh.countDown();
try {
if (!failCertificateRefresh.await(5, TimeUnit.SECONDS)) {
throw new RefreshFailedException(
"Timed out waiting to simulate cert.pem failure");
}
} catch (InterruptedException e) {
Thread.currentThread().interrupt();
throw new RefreshFailedException("Interrupted while simulating cert.pem failure");
}
throw new RefreshFailedException("IMDS GET cert.pem returned HTTP 404");
}

@Override
@Deprecated
public X509Certificate getCertificate() {
return null;
}

@Override
@Deprecated
public java.security.interfaces.RSAPrivateKey getPrivateKey() {
return null;
}

@Override
public CertificateAndPrivateKeyPair getCertificateAndKeyPair() {
return null;
}
}

X509CertificateSupplier refreshableLeafCertificateSupplier =
new RefreshableFailingLeafCertificateSupplier();

X509FederationClient client =
new X509FederationClient(
"https://auth.example.com",
"testTenantId",
refreshableLeafCertificateSupplier,
mock(SessionKeySupplier.class),
Collections.emptySet(),
mock(ClientConfigurator.class),
Collections.emptyList(),
mock(CircuitBreakerConfiguration.class));

Thread leader =
new Thread(
() -> {
try {
client.getSecurityToken();
} catch (Throwable e) {
leaderFailure.set(e);
}
},
"refresh-leader");
leader.start();
assertEquals(
"Leader should begin the simulated cert.pem refresh",
true,
leaderStartedCertificateRefresh.await(5, TimeUnit.SECONDS));

Thread follower =
new Thread(
() -> {
try {
client.getSecurityToken();
} catch (Throwable e) {
followerFailure.set(e);
}
},
"refresh-follower");
follower.start();

try {
final Object waitObject = new Object();
long deadline = System.currentTimeMillis() + 5_000;
while (follower.getState() != Thread.State.TIMED_WAITING
&& System.currentTimeMillis() < deadline) {
synchronized (waitObject) {
waitObject.wait(10);
}
}
assertEquals(
"Follower should wait on the leader's in-flight refresh before it fails",
Thread.State.TIMED_WAITING,
follower.getState());

failCertificateRefresh.countDown();
leader.join(5_000);
follower.join(2_000);

assertFalse("Leader should return the certificate refresh error", leader.isAlive());
assertFalse(
"Follower must receive the leader failure instead of waiting for inFlightRefresh",
follower.isAlive());
assertEquals(BmcException.class, leaderFailure.get().getClass());
assertEquals(BmcException.class, followerFailure.get().getClass());
} finally {
failCertificateRefresh.countDown();
follower.interrupt();
leader.join(5_000);
follower.join(5_000);
}
}
}