From 1776d6e0552bfa749ddfc7a78d46564f1583d462 Mon Sep 17 00:00:00 2001 From: Ioannis Kakavas Date: Tue, 11 Jun 2019 10:55:50 +0300 Subject: [PATCH] Refresh remote JWKs on all errors (#42850) 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 ) --- .../oidc/OpenIdConnectAuthenticator.java | 5 ++- .../oidc/OpenIdConnectAuthenticatorTests.java | 32 +++++++++++++++---- 2 files changed, 28 insertions(+), 9 deletions(-) diff --git a/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authc/oidc/OpenIdConnectAuthenticator.java b/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authc/oidc/OpenIdConnectAuthenticator.java index 6de933804f3..35a01d15dda 100644 --- a/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authc/oidc/OpenIdConnectAuthenticator.java +++ b/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authc/oidc/OpenIdConnectAuthenticator.java @@ -12,7 +12,6 @@ import com.nimbusds.jose.jwk.JWKSelector; 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; @@ -241,7 +240,7 @@ public class OpenIdConnectAuthenticator { } 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 @@ -257,7 +256,7 @@ public class OpenIdConnectAuthenticator { } 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)); } } diff --git a/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/authc/oidc/OpenIdConnectAuthenticatorTests.java b/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/authc/oidc/OpenIdConnectAuthenticatorTests.java index 7a2fa9af039..5c31ad850be 100644 --- a/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/authc/oidc/OpenIdConnectAuthenticatorTests.java +++ b/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/authc/oidc/OpenIdConnectAuthenticatorTests.java @@ -320,7 +320,11 @@ public class OpenIdConnectAuthenticatorTests extends OpenIdConnectTestCase { 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 { @@ -360,7 +364,11 @@ public class OpenIdConnectAuthenticatorTests extends OpenIdConnectTestCase { 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 { @@ -399,7 +407,11 @@ public class OpenIdConnectAuthenticatorTests extends OpenIdConnectTestCase { 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 { @@ -438,7 +450,11 @@ public class OpenIdConnectAuthenticatorTests extends OpenIdConnectTestCase { 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 { @@ -611,7 +627,7 @@ public class OpenIdConnectAuthenticatorTests extends OpenIdConnectTestCase { 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 { @@ -648,7 +664,11 @@ public class OpenIdConnectAuthenticatorTests extends OpenIdConnectTestCase { 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 {