Skip to content

Fix refresh remote JWKS logic #42662

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

Merged
merged 4 commits into from
May 30, 2019
Merged

Fix refresh remote JWKS logic #42662

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
fd6cc8c
Fix refresh remote JWKS logic
jkakavas May 29, 2019
e0650af
Merge remote-tracking branch 'origin/master' into oidc-exception-hand…
jkakavas May 29, 2019
bfdc19b
Merge remote-tracking branch 'origin/master' into oidc-exception-hand…
jkakavas May 29, 2019
ef31690
Merge remote-tracking branch 'origin/master' into oidc-exception-hand…
jkakavas May 30, 2019
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
6 changes: 4 additions & 2 deletions ...src/main/java/org/elasticsearch/xpack/security/authc/oidc/OpenIdConnectAuthenticator.java
View file
Open in desktop
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) {
Copy link
Contributor

Choose a reason for hiding this comment

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

@jkakavas Can you please explain why BadJOSEException is not retry-able anymore?
If I read the code correctly, https://bitbucket.org/connect2id/nimbus-jose-jwt/src/8bc51863af3c36ec3631878ac69c2ab59936cfae/src/main/java/com/nimbusds/jose/proc/DefaultJOSEProcessor.java#lines-234 , a BadJOSEException is triggered when there is no candidate key to do the signature verification, so it must be the case of a key rotation?

Copy link
Member Author

Choose a reason for hiding this comment

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

@albertzaharovits you are right about the above. The problem - which I tried to address - is that BadJWTException is also a BadJOSEException and that a BadJWTException can be thrown for many reasons ( Expired JWTs, wrong nonce, wrong issuer etc. etc. ) that have nothing to do with key rotation or reloadability of keys, so it didn't make sense to try and reload keys for any error that could happen in the context of the OIDC realm configuration or JWT parsing.

I was under the impression that the error of not finding a suitable key would throw a BadJWSException and not a BadJOSEException but you are right. It feels like the only way to try and reload only on errors that can be caused by key rotation, we'd have to both match the Exception class and the Exception message. This is what I was doing to start with here and we agreed it was not an elegant solution

Another solution would be to add the possibility to refresh a remote JWKSet at will on configurable intervals. This way, if you're using an OP that has a frequent key rotation schedule you can adjust the polling frequency accordingly. This fails to address the situation where an OP rotates their keys because of a compromise though - which is what the original implementation would try to cater for with the simplistic retry on error approach.

Copy link
Contributor

Choose a reason for hiding this comment

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

@jkakavas

BadJWSException is also a BadJOSEException

I see, that I haven't noticed.

I think we might get a BadJWSException as well as a BadJOSEException when the OP rotated the keys, depending on if the OP uses the kid header parameter https://tools.ietf.org/html/rfc7515#section-4.1.4 . If it does I think the library would throw a BadJOSEException; if the OP simply updates the key we will get a BadJWSException.
Therefore, I think we should reload for the BadJOSEException even though BadJOSEException can also be caused by a user typo in the realm configuration. If we are concerned about doubling error messages for user misconfiguration, then we can only proceed with the claim validation retrial if the newly retrieved keys are different. I would not be concerned that a realm misconfiguration incurs an additional call to the OP.
What do you think?

Copy link
Member Author

Choose a reason for hiding this comment

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

@jkakavas
BadJWSException is also a BadJOSEException
I see, that I haven't noticed.

It's BadJWTException being a BadJOSEException that is problematic, tbc.

I would not be concerned that a realm misconfiguration incurs an additional call to the OP.

Agreed, this is better than not reloading keys in case of a valid key rotation - this was an unintended consequence of the changes in this PR. I failed to notice that the library throws a BadJOSEException in some cases too.

I'll raise a PR to change this

Copy link
Member Author

Choose a reason for hiding this comment

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

Addressed in #42662

} 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
Open in desktop
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