Skip to content

Commit

Permalink
Fix refresh remote JWKS logic (#42662)
Browse files Browse the repository at this point in the history 
This change ensures that:

- We only attempt to refresh the remote JWKS when there is a
signature related error only ( BadJWSException instead of the
geric BadJOSEException )
- We do call OpenIDConnectAuthenticator#getUserClaims upon
successful refresh.
- We test this in OpenIdConnectAuthenticatorTests.

Without this fix, when using the OpenID Connect realm with a remote
JWKSet configured in `op.jwks_path`, the refresh would be triggered
for most configuration errors ( i.e. wrong value for `op.issuer` )
and the kibana wouldn't get a response and timeout since
`getUserClaims` wouldn't be called because
`ReloadableJWKSource#reloadAsync` wouldn't call `onResponse` on the
future.
  • Loading branch information
@jkakavas
jkakavas committed May 30, 2019
1 parent 05bc599 commit 64e9c88
Show file tree
Hide file tree
Showing 2 changed files with 19 additions and 2 deletions.
6 changes: 4 additions & 2 deletions ...src/main/java/org/elasticsearch/xpack/security/authc/oidc/OpenIdConnectAuthenticator.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@
import com.nimbusds.jose.jwk.JWKSet;
import com.nimbusds.jose.jwk.source.JWKSource;
import com.nimbusds.jose.proc.BadJOSEException;
import com.nimbusds.jose.proc.BadJWSException;
import com.nimbusds.jose.proc.JWSVerificationKeySelector;
import com.nimbusds.jose.proc.SecurityContext;
import com.nimbusds.jose.util.IOUtils;
Expand Down Expand Up @@ -240,7 +241,7 @@ private void getUserClaims(@Nullable AccessToken accessToken, JWT idToken, Nonce
}
claimsListener.onResponse(enrichedVerifiedIdTokenClaims);
}
} catch (BadJOSEException e) {
} catch (BadJWSException e) {
// We only try to update the cached JWK set once if a remote source is used and
// RSA or ECDSA is used for signatures
if (shouldRetry
Expand All @@ -256,7 +257,7 @@ private void getUserClaims(@Nullable AccessToken accessToken, JWT idToken, Nonce
} else {
claimsListener.onFailure(new ElasticsearchSecurityException("Failed to parse or validate the ID Token", e));
}
} catch (com.nimbusds.oauth2.sdk.ParseException | ParseException | JOSEException e) {
} catch (com.nimbusds.oauth2.sdk.ParseException | ParseException | BadJOSEException | JOSEException e) {
claimsListener.onFailure(new ElasticsearchSecurityException("Failed to parse or validate the ID Token", e));
}
}
Expand Down Expand Up @@ -777,6 +778,7 @@ public void completed(HttpResponse result) {
StandardCharsets.UTF_8));
reloadFutureRef.set(null);
LOGGER.trace("Successfully refreshed and cached remote JWKSet");
future.onResponse(null);
} catch (IOException | ParseException e) {
failed(e);
}
Expand Down
15 changes: 15 additions & 0 deletions ...est/java/org/elasticsearch/xpack/security/authc/oidc/OpenIdConnectAuthenticatorTests.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -88,13 +88,15 @@ public class OpenIdConnectAuthenticatorTests extends OpenIdConnectTestCase {
private Settings globalSettings;
private Environment env;
private ThreadContext threadContext;
private int callsToReloadJwk;

@Before
public void setup() {
globalSettings = Settings.builder().put("path.home", createTempDir())
.put("xpack.security.authc.realms.oidc.oidc-realm.ssl.verification_mode", "certificate").build();
env = TestEnvironment.newEnvironment(globalSettings);
threadContext = new ThreadContext(globalSettings);
callsToReloadJwk = 0;
}

@After
Expand Down Expand Up @@ -278,6 +280,7 @@ public void testClockSkewIsHonored() throws Exception {
authenticator.authenticate(token, future);
JWTClaimsSet claimsSet = future.actionGet();
assertThat(claimsSet.getSubject(), equalTo(subject));
assertThat(callsToReloadJwk, equalTo(0));
}

public void testImplicitFlowFailsWithExpiredToken() throws Exception {
Expand Down Expand Up @@ -317,6 +320,7 @@ public void testImplicitFlowFailsWithExpiredToken() throws Exception {
assertThat(e.getMessage(), containsString("Failed to parse or validate the ID Token"));
assertThat(e.getCause(), instanceOf(BadJWTException.class));
assertThat(e.getCause().getMessage(), containsString("Expired JWT"));
assertThat(callsToReloadJwk, equalTo(0));
}

public void testImplicitFlowFailsNotYetIssuedToken() throws Exception {
Expand Down Expand Up @@ -356,6 +360,7 @@ public void testImplicitFlowFailsNotYetIssuedToken() throws Exception {
assertThat(e.getMessage(), containsString("Failed to parse or validate the ID Token"));
assertThat(e.getCause(), instanceOf(BadJWTException.class));
assertThat(e.getCause().getMessage(), containsString("JWT issue time ahead of current time"));
assertThat(callsToReloadJwk, equalTo(0));
}

public void testImplicitFlowFailsInvalidIssuer() throws Exception {
Expand Down Expand Up @@ -394,6 +399,7 @@ public void testImplicitFlowFailsInvalidIssuer() throws Exception {
assertThat(e.getMessage(), containsString("Failed to parse or validate the ID Token"));
assertThat(e.getCause(), instanceOf(BadJWTException.class));
assertThat(e.getCause().getMessage(), containsString("Unexpected JWT issuer"));
assertThat(callsToReloadJwk, equalTo(0));
}

public void testImplicitFlowFailsInvalidAudience() throws Exception {
Expand Down Expand Up @@ -432,6 +438,7 @@ public void testImplicitFlowFailsInvalidAudience() throws Exception {
assertThat(e.getMessage(), containsString("Failed to parse or validate the ID Token"));
assertThat(e.getCause(), instanceOf(BadJWTException.class));
assertThat(e.getCause().getMessage(), containsString("Unexpected JWT audience"));
assertThat(callsToReloadJwk, equalTo(0));
}

public void testAuthenticateImplicitFlowFailsWithForgedRsaIdToken() throws Exception {
Expand All @@ -456,6 +463,7 @@ public void testAuthenticateImplicitFlowFailsWithForgedRsaIdToken() throws Excep
assertThat(e.getMessage(), containsString("Failed to parse or validate the ID Token"));
assertThat(e.getCause(), instanceOf(BadJWSException.class));
assertThat(e.getCause().getMessage(), containsString("Signed JWT rejected: Invalid signature"));
assertThat(callsToReloadJwk, equalTo(1));
}

public void testAuthenticateImplicitFlowFailsWithForgedEcsdsaIdToken() throws Exception {
Expand All @@ -480,6 +488,7 @@ public void testAuthenticateImplicitFlowFailsWithForgedEcsdsaIdToken() throws Ex
assertThat(e.getMessage(), containsString("Failed to parse or validate the ID Token"));
assertThat(e.getCause(), instanceOf(BadJWSException.class));
assertThat(e.getCause().getMessage(), containsString("Signed JWT rejected: Invalid signature"));
assertThat(callsToReloadJwk, equalTo(1));
}

public void testAuthenticateImplicitFlowFailsWithForgedHmacIdToken() throws Exception {
Expand All @@ -503,6 +512,7 @@ public void testAuthenticateImplicitFlowFailsWithForgedHmacIdToken() throws Exce
assertThat(e.getMessage(), containsString("Failed to parse or validate the ID Token"));
assertThat(e.getCause(), instanceOf(BadJWSException.class));
assertThat(e.getCause().getMessage(), containsString("Signed JWT rejected: Invalid signature"));
assertThat(callsToReloadJwk, equalTo(0));
}

public void testAuthenticateImplicitFlowFailsWithForgedAccessToken() throws Exception {
Expand Down Expand Up @@ -532,6 +542,7 @@ public void testAuthenticateImplicitFlowFailsWithForgedAccessToken() throws Exce
assertThat(e.getMessage(), containsString("Failed to verify access token"));
assertThat(e.getCause(), instanceOf(InvalidHashException.class));
assertThat(e.getCause().getMessage(), containsString("Access token hash (at_hash) mismatch"));
assertThat(callsToReloadJwk, equalTo(0));
}

public void testImplicitFlowFailsWithNoneAlgorithm() throws Exception {
Expand Down Expand Up @@ -569,6 +580,7 @@ public void testImplicitFlowFailsWithNoneAlgorithm() throws Exception {
assertThat(e.getMessage(), containsString("Failed to parse or validate the ID Token"));
assertThat(e.getCause(), instanceOf(BadJOSEException.class));
assertThat(e.getCause().getMessage(), containsString("Another algorithm expected, or no matching key(s) found"));
assertThat(callsToReloadJwk, equalTo(0));
}

/**
Expand Down Expand Up @@ -599,6 +611,7 @@ public void testImplicitFlowFailsWithAlgorithmMixupAttack() throws Exception {
assertThat(e.getMessage(), containsString("Failed to parse or validate the ID Token"));
assertThat(e.getCause(), instanceOf(BadJOSEException.class));
assertThat(e.getCause().getMessage(), containsString("Another algorithm expected, or no matching key(s) found"));
assertThat(callsToReloadJwk, equalTo(0));
}

public void testImplicitFlowFailsWithUnsignedJwt() throws Exception {
Expand Down Expand Up @@ -635,6 +648,7 @@ public void testImplicitFlowFailsWithUnsignedJwt() throws Exception {
assertThat(e.getMessage(), containsString("Failed to parse or validate the ID Token"));
assertThat(e.getCause(), instanceOf(BadJWTException.class));
assertThat(e.getCause().getMessage(), containsString("Signed ID token expected"));
assertThat(callsToReloadJwk, equalTo(0));
}

public void testJsonObjectMerging() throws Exception {
Expand Down Expand Up @@ -832,6 +846,7 @@ private OpenIdConnectAuthenticator.ReloadableJWKSource mockSource(JWK jwk) {
Mockito.doAnswer(invocation -> {
@SuppressWarnings("unchecked")
ActionListener<Void> listener = (ActionListener<Void>) invocation.getArguments()[0];
callsToReloadJwk += 1;
listener.onResponse(null);
return null;
}).when(jwkSource).triggerReload(any(ActionListener.class));
Expand Down

0 comments on commit 64e9c88

Please sign in to comment.