Skip to content

Commit

Permalink
Refresh remote JWKs on all errors (elastic#42850)
Browse files Browse the repository at this point in the history 
It turns out that key rotation on the OP, can manifest as both
a BadJWSException and a BadJOSEException in nimbus-jose-jwt. As
such we cannot depend on matching only BadJWSExceptions to
determine if we should poll the remote JWKs for an update.

This has the side-effect that a remote JWKs source will be polled
exactly one additional time too for errors that have to do with
configuration, or for errors that might be caused by not synched
clocks, forged JWTs, etc. ( These will throw a BadJWTException
which extends BadJOSEException also )
  • Loading branch information
@jkakavas
jkakavas committed Jun 11, 2019
1 parent 6a77dde commit 1776d6e
Show file tree
Hide file tree
Showing 2 changed files with 28 additions and 9 deletions.
5 changes: 2 additions & 3 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,7 +12,6 @@
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 @@ -241,7 +240,7 @@ private void getUserClaims(@Nullable AccessToken accessToken, JWT idToken, Nonce
}
claimsListener.onResponse(enrichedVerifiedIdTokenClaims);
}
} catch (BadJWSException e) {
} catch (BadJOSEException 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 @@ -257,7 +256,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 | BadJOSEException | JOSEException e) {
} catch (com.nimbusds.oauth2.sdk.ParseException | ParseException | JOSEException e) {
claimsListener.onFailure(new ElasticsearchSecurityException("Failed to parse or validate the ID Token", e));
}
}
Expand Down
32 changes: 26 additions & 6 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 @@ -320,7 +320,11 @@ 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));
if (jwk.getAlgorithm().getName().startsWith("HS")) {
assertThat(callsToReloadJwk, equalTo(0));
} else {
assertThat(callsToReloadJwk, equalTo(1));
}
}

public void testImplicitFlowFailsNotYetIssuedToken() throws Exception {
Expand Down Expand Up @@ -360,7 +364,11 @@ 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));
if (jwk.getAlgorithm().getName().startsWith("HS")) {
assertThat(callsToReloadJwk, equalTo(0));
} else {
assertThat(callsToReloadJwk, equalTo(1));
}
}

public void testImplicitFlowFailsInvalidIssuer() throws Exception {
Expand Down Expand Up @@ -399,7 +407,11 @@ 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));
if (jwk.getAlgorithm().getName().startsWith("HS")) {
assertThat(callsToReloadJwk, equalTo(0));
} else {
assertThat(callsToReloadJwk, equalTo(1));
}
}

public void testImplicitFlowFailsInvalidAudience() throws Exception {
Expand Down Expand Up @@ -438,7 +450,11 @@ 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));
if (jwk.getAlgorithm().getName().startsWith("HS")) {
assertThat(callsToReloadJwk, equalTo(0));
} else {
assertThat(callsToReloadJwk, equalTo(1));
}
}

public void testAuthenticateImplicitFlowFailsWithForgedRsaIdToken() throws Exception {
Expand Down Expand Up @@ -611,7 +627,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));
assertThat(callsToReloadJwk, equalTo(1));
}

public void testImplicitFlowFailsWithUnsignedJwt() throws Exception {
Expand Down Expand Up @@ -648,7 +664,11 @@ 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));
if (jwk.getAlgorithm().getName().startsWith("HS")) {
assertThat(callsToReloadJwk, equalTo(0));
} else {
assertThat(callsToReloadJwk, equalTo(1));
}
}

public void testJsonObjectMerging() throws Exception {
Expand Down

0 comments on commit 1776d6e

Please sign in to comment.