Skip to content

Refresh remote JWK when unknown KID error occurs #11638

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 2 commits into from
Aug 18, 2022
Merged

Refresh remote JWK when unknown KID error occurs #11638

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
14ae464
Add tests for unknown KID error
tinolazreg Jul 27, 2022
e38b6e7
Refresh remote JWK when unknown KID error occurs
Jul 27, 2022
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
70 changes: 33 additions & 37 deletions ...2/oauth2-jose/src/main/java/org/springframework/security/oauth2/jwt/NimbusJwtDecoder.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
/*
* Copyright 2002-2021 the original author or authors.
* Copyright 2002-2022 the original author or authors.
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
Expand Down Expand Up @@ -338,8 +338,8 @@ JWKSource<SecurityContext> jwkSource(ResourceRetriever jwkSetRetriever) {
if (this.cache == null) {
return new RemoteJWKSet<>(toURL(this.jwkSetUri), jwkSetRetriever);
}
ResourceRetriever cachingJwkSetRetriever = new CachingResourceRetriever(this.cache, jwkSetRetriever);
return new RemoteJWKSet<>(toURL(this.jwkSetUri), cachingJwkSetRetriever, new NoOpJwkSetCache());
JWKSetCache jwkSetCache = new SpringJWKSetCache(this.jwkSetUri, this.cache);
return new RemoteJWKSet<>(toURL(this.jwkSetUri), jwkSetRetriever, jwkSetCache);
}

JWTProcessor<SecurityContext> processor() {
Expand Down Expand Up @@ -371,52 +371,48 @@ private static URL toURL(String url) {
}
}

private static class NoOpJwkSetCache implements JWKSetCache {
private static final class SpringJWKSetCache implements JWKSetCache {

@Override
public void put(JWKSet jwkSet) {
private final String jwkSetUri;

private final Cache cache;

private JWKSet jwkSet;

SpringJWKSetCache(String jwkSetUri, Cache cache) {
this.jwkSetUri = jwkSetUri;
this.cache = cache;
this.updateJwkSetFromCache();
}

@Override
public JWKSet get() {
return null;
private void updateJwkSetFromCache() {
String cachedJwkSet = this.cache.get(this.jwkSetUri, String.class);
if (cachedJwkSet != null) {
try {
this.jwkSet = JWKSet.parse(cachedJwkSet);
}
catch (ParseException ignored) {
// Ignore invalid cache value
}
}
}

// Note: Only called from inside a synchronized block in RemoteJWKSet.
@Override
public boolean requiresRefresh() {
return true;
public void put(JWKSet jwkSet) {
this.jwkSet = jwkSet;
this.cache.put(this.jwkSetUri, jwkSet.toString(false));
}

}

private static class CachingResourceRetriever implements ResourceRetriever {

private final Cache cache;

private final ResourceRetriever resourceRetriever;
@Override
public JWKSet get() {
return (!requiresRefresh()) ? this.jwkSet : null;

CachingResourceRetriever(Cache cache, ResourceRetriever resourceRetriever) {
this.cache = cache;
this.resourceRetriever = resourceRetriever;
}

@Override
public Resource retrieveResource(URL url) throws IOException {
try {
String jwkSet = this.cache.get(url.toString(),
() -> this.resourceRetriever.retrieveResource(url).getContent());
return new Resource(jwkSet, "UTF-8");
}
catch (Cache.ValueRetrievalException ex) {
Throwable thrownByValueLoader = ex.getCause();
if (thrownByValueLoader instanceof IOException) {
throw (IOException) thrownByValueLoader;
}
throw new IOException(thrownByValueLoader);
}
catch (Exception ex) {
throw new IOException(ex);
}
public boolean requiresRefresh() {
return this.cache.get(this.jwkSetUri) == null;
}

}
Expand Down
118 changes: 113 additions & 5 deletions ...th2-jose/src/test/java/org/springframework/security/oauth2/jwt/NimbusJwtDecoderTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
/*
* Copyright 2002-2021 the original author or authors.
* Copyright 2002-2022 the original author or authors.
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
Expand Down Expand Up @@ -32,10 +32,10 @@
import java.util.Date;
import java.util.List;
import java.util.Map;
import java.util.concurrent.Callable;

import javax.crypto.SecretKey;

import com.nimbusds.jose.JOSEException;
import com.nimbusds.jose.JOSEObjectType;
import com.nimbusds.jose.JWSAlgorithm;
import com.nimbusds.jose.JWSHeader;
Expand Down Expand Up @@ -82,6 +82,7 @@
import static org.mockito.ArgumentMatchers.eq;
import static org.mockito.BDDMockito.given;
import static org.mockito.Mockito.mock;
import static org.mockito.Mockito.times;
import static org.mockito.Mockito.verify;
import static org.mockito.Mockito.verifyNoInteractions;
import static org.mockito.Mockito.verifyNoMoreInteractions;
Expand All @@ -97,10 +98,14 @@ public class NimbusJwtDecoderTests {

private static final String JWK_SET = "{\"keys\":[{\"p\":\"49neceJFs8R6n7WamRGy45F5Tv0YM-R2ODK3eSBUSLOSH2tAqjEVKOkLE5fiNA3ygqq15NcKRadB2pTVf-Yb5ZIBuKzko8bzYIkIqYhSh_FAdEEr0vHF5fq_yWSvc6swsOJGqvBEtuqtJY027u-G2gAQasCQdhyejer68zsTn8M\",\"kty\":\"RSA\",\"q\":\"tWR-ysspjZ73B6p2vVRVyHwP3KQWL5KEQcdgcmMOE_P_cPs98vZJfLhxobXVmvzuEWBpRSiqiuyKlQnpstKt94Cy77iO8m8ISfF3C9VyLWXi9HUGAJb99irWABFl3sNDff5K2ODQ8CmuXLYM25OwN3ikbrhEJozlXg_NJFSGD4E\",\"d\":\"FkZHYZlw5KSoqQ1i2RA2kCUygSUOf1OqMt3uomtXuUmqKBm_bY7PCOhmwbvbn4xZYEeHuTR8Xix-0KpHe3NKyWrtRjkq1T_un49_1LLVUhJ0dL-9_x0xRquVjhl_XrsRXaGMEHs8G9pLTvXQ1uST585gxIfmCe0sxPZLvwoic-bXf64UZ9BGRV3lFexWJQqCZp2S21HfoU7wiz6kfLRNi-K4xiVNB1gswm_8o5lRuY7zB9bRARQ3TS2G4eW7p5sxT3CgsGiQD3_wPugU8iDplqAjgJ5ofNJXZezoj0t6JMB_qOpbrmAM1EnomIPebSLW7Ky9SugEd6KMdL5lW6AuAQ\",\"e\":\"AQAB\",\"use\":\"sig\",\"kid\":\"one\",\"qi\":\"wdkFu_tV2V1l_PWUUimG516Zvhqk2SWDw1F7uNDD-Lvrv_WNRIJVzuffZ8WYiPy8VvYQPJUrT2EXL8P0ocqwlaSTuXctrORcbjwgxDQDLsiZE0C23HYzgi0cofbScsJdhcBg7d07LAf7cdJWG0YVl1FkMCsxUlZ2wTwHfKWf-v4\",\"dp\":\"uwnPxqC-IxG4r33-SIT02kZC1IqC4aY7PWq0nePiDEQMQWpjjNH50rlq9EyLzbtdRdIouo-jyQXB01K15-XXJJ60dwrGLYNVqfsTd0eGqD1scYJGHUWG9IDgCsxyEnuG3s0AwbW2UolWVSsU2xMZGb9PurIUZECeD1XDZwMp2s0\",\"dq\":\"hra786AunB8TF35h8PpROzPoE9VJJMuLrc6Esm8eZXMwopf0yhxfN2FEAvUoTpLJu93-UH6DKenCgi16gnQ0_zt1qNNIVoRfg4rw_rjmsxCYHTVL3-RDeC8X_7TsEySxW0EgFTHh-nr6I6CQrAJjPM88T35KHtdFATZ7BCBB8AE\",\"n\":\"oXJ8OyOv_eRnce4akdanR4KYRfnC2zLV4uYNQpcFn6oHL0dj7D6kxQmsXoYgJV8ZVDn71KGmuLvolxsDncc2UrhyMBY6DVQVgMSVYaPCTgW76iYEKGgzTEw5IBRQL9w3SRJWd3VJTZZQjkXef48Ocz06PGF3lhbz4t5UEZtdF4rIe7u-977QwHuh7yRPBQ3sII-cVoOUMgaXB9SHcGF2iZCtPzL_IffDUcfhLQteGebhW8A6eUHgpD5A1PQ-JCw_G7UOzZAjjDjtNM2eqm8j-Ms_gqnm4MiCZ4E-9pDN77CAAPVN7kuX6ejs9KBXpk01z48i9fORYk9u7rAkh1HuQw\"}]}";

private static final String NEW_KID_JWK_SET = "{\"keys\":[{\"kty\":\"RSA\",\"e\":\"AQAB\",\"kid\":\"two\",\"n\":\"ra9UJw4I0fCHuOqr1xWJsh-qcVeZWtKEU3uoqq1sAg5fG67dujNCm_Q16yuO0ZdDiU0vlJkbc_MXFAvm4ZxdJ_qR7PAneV-BOGNtLpSaiPclscCy3m7zjRWkaqwt9ZZEsdK5UqXyPlBpcYhNKsmnQGjnX4sYb7d8b2jSCM_qto48-6451rbyEhXXywtFy_JqtTpbsw_IIdQHMr1O-MdSjsQxX9kkvZwPU8LsC-CcqlcsZ7mnpOhmIXaf4tbRwAaluXwYft0yykFsp8e5C4t9mMs9Vu8AB5gT8o-D_ovXd2qh4k3ejzVpYLtzD4nbfvPJA_TXmjhn-9GOPAqkzfON2Q\"}]}";

private static final String MALFORMED_JWK_SET = "malformed";

private static final String SIGNED_JWT = "eyJhbGciOiJSUzI1NiJ9.eyJzdWIiOiJ0ZXN0LXN1YmplY3QiLCJzY3AiOlsibWVzc2FnZTpyZWFkIl0sImV4cCI6NDY4Mzg5Nzc3Nn0.LtMVtIiRIwSyc3aX35Zl0JVwLTcQZAB3dyBOMHNaHCKUljwMrf20a_gT79LfhjDzE_fUVUmFiAO32W1vFnYpZSVaMDUgeIOIOpxfoe9shj_uYenAwIS-_UxqGVIJiJoXNZh_MK80ShNpvsQwamxWEEOAMBtpWNiVYNDMdfgho9n3o5_Z7Gjy8RLBo1tbDREbO9kTFwGIxm_EYpezmRCRq4w1DdS6UDW321hkwMxPnCMSWOvp-hRpmgY2yjzLgPJ6Aucmg9TJ8jloAP1DjJoF1gRR7NTAk8LOGkSjTzVYDYMbCF51YdpojhItSk80YzXiEsv1mTz4oMM49jXBmfXFMA";

private static final String NEW_KID_SIGNED_JWT = "eyJraWQiOiJ0d28iLCJhbGciOiJSUzI1NiJ9.eyJleHAiOjIxMzMyNzg4MjV9.DQJn_qg0HfZ_sjlx9MJkdCjkp9t-0zOj3FzVp_UPzx6RCcBb8Jk373dNgcyfOP5CS29wv5gKX6geWEDj5cgqcJdTS53zqOaLETdNnKACd056SkPqgTLJv12gdJx7tr5WbBqRB9Y0ce96vbH6wwQGfqU_1Lz1RhZ7ZZuvIuWLp75ujld7dOshScg728Z9BQsiFOH_yFp09XraO15spwTXp9RO5TJRUSLih-5V3sdxHa5rPTm6by7me8I_l4iMJN81Z95_O7sbLeYH-4zZ-3T49uPyAC5suEOd-P5aFP89zPKh9Y3Uviu2OyvpUuXmpUjTtdAKf3p96dOEeLJvT3hkSg";

private static final String MALFORMED_JWT = "eyJhbGciOiJSUzI1NiJ9.eyJuYmYiOnt9LCJleHAiOjQ2ODQyMjUwODd9.guoQvujdWvd3xw7FYQEn4D6-gzM_WqFvXdmvAUNSLbxG7fv2_LLCNujPdrBHJoYPbOwS1BGNxIKQWS1tylvqzmr1RohQ-RZ2iAM1HYQzboUlkoMkcd8ENM__ELqho8aNYBfqwkNdUOyBFoy7Syu_w2SoJADw2RTjnesKO6CVVa05bW118pDS4xWxqC4s7fnBjmZoTn4uQ-Kt9YSQZQk8YQxkJSiyanozzgyfgXULA6mPu1pTNU3FVFaK1i1av_xtH_zAPgb647ZeaNe4nahgqC5h8nhOlm8W2dndXbwAt29nd2ZWBsru_QwZz83XSKLhTPFz-mPBByZZDsyBbIHf9A";

private static final String UNSIGNED_JWT = "eyJhbGciOiJub25lIiwidHlwIjoiSldUIn0.eyJleHAiOi0yMDMzMjI0OTcsImp0aSI6IjEyMyIsInR5cCI6IkpXVCJ9.";
Expand Down Expand Up @@ -644,22 +649,104 @@ public void decodeWhenCacheThenStoreRetrievedJwkSetToCache() {
}

@Test
public void decodeWhenCacheThenRetrieveFromCache() {
public void decodeWhenCacheStoredThenAbleToRetrieveJwkSetFromCache() {
Cache cache = new ConcurrentMapCache("test-jwk-set-cache");
RestOperations restOperations = mock(RestOperations.class);
given(restOperations.exchange(any(RequestEntity.class), eq(String.class)))
.willReturn(new ResponseEntity<>(JWK_SET, HttpStatus.OK));
// @formatter:off
NimbusJwtDecoder jwtDecoder1 = NimbusJwtDecoder.withJwkSetUri(JWK_SET_URI)
.restOperations(restOperations)
.cache(cache)
.build();
// @formatter:on
jwtDecoder1.decode(SIGNED_JWT);
assertThat(cache.get(JWK_SET_URI, String.class)).isEqualTo(JWK_SET);
verify(restOperations).exchange(any(RequestEntity.class), eq(String.class));

// @formatter:off
NimbusJwtDecoder jwtDecoder2 = NimbusJwtDecoder.withJwkSetUri(JWK_SET_URI)
.restOperations(restOperations)
.cache(cache)
.build();
// @formatter:on
jwtDecoder2.decode(SIGNED_JWT);
verifyNoMoreInteractions(restOperations);
}

// gh-11621
@Test
public void decodeWhenCacheThenRetrieveFromCache() throws Exception {
RestOperations restOperations = mock(RestOperations.class);
Cache cache = mock(Cache.class);
given(cache.get(eq(JWK_SET_URI), any(Callable.class))).willReturn(JWK_SET);
given(cache.get(eq(JWK_SET_URI), eq(String.class))).willReturn(JWK_SET);
given(cache.get(eq(JWK_SET_URI))).willReturn(mock(Cache.ValueWrapper.class));
// @formatter:off
NimbusJwtDecoder jwtDecoder = NimbusJwtDecoder.withJwkSetUri(JWK_SET_URI)
.cache(cache)
.restOperations(restOperations)
.build();
// @formatter:on
jwtDecoder.decode(SIGNED_JWT);
verify(cache).get(eq(JWK_SET_URI), any(Callable.class));
verify(cache).get(eq(JWK_SET_URI), eq(String.class));
verify(cache, times(2)).get(eq(JWK_SET_URI));
verifyNoMoreInteractions(cache);
verifyNoInteractions(restOperations);
}

// gh-11621
@Test
public void decodeWhenCacheAndUnknownKidShouldTriggerFetchOfJwkSet() throws JOSEException {
RestOperations restOperations = mock(RestOperations.class);
Cache cache = mock(Cache.class);
given(cache.get(eq(JWK_SET_URI), eq(String.class))).willReturn(JWK_SET);
given(restOperations.exchange(any(RequestEntity.class), eq(String.class)))
.willReturn(new ResponseEntity<>(NEW_KID_JWK_SET, HttpStatus.OK));

// @formatter:off
NimbusJwtDecoder jwtDecoder = NimbusJwtDecoder.withJwkSetUri(JWK_SET_URI)
.cache(cache)
.restOperations(restOperations)
.build();
// @formatter:on

// Decode JWT with new KID
jwtDecoder.decode(NEW_KID_SIGNED_JWT);

ArgumentCaptor<RequestEntity> requestEntityCaptor = ArgumentCaptor.forClass(RequestEntity.class);
verify(restOperations).exchange(requestEntityCaptor.capture(), eq(String.class));
verifyNoMoreInteractions(restOperations);
assertThat(requestEntityCaptor.getValue().getHeaders().getAccept()).contains(MediaType.APPLICATION_JSON,
APPLICATION_JWK_SET_JSON);
}

// gh-11621
@Test
public void decodeWithoutCacheSpecifiedAndUnknownKidShouldTriggerFetchOfJwkSet() throws JOSEException {
RestOperations restOperations = mock(RestOperations.class);
given(restOperations.exchange(any(RequestEntity.class), eq(String.class))).willReturn(
new ResponseEntity<>(JWK_SET, HttpStatus.OK), new ResponseEntity<>(NEW_KID_JWK_SET, HttpStatus.OK));

// @formatter:off
NimbusJwtDecoder jwtDecoder = NimbusJwtDecoder.withJwkSetUri(JWK_SET_URI)
.restOperations(restOperations)
.build();
// @formatter:on
jwtDecoder.decode(SIGNED_JWT);

// Decode JWT with new KID
jwtDecoder.decode(NEW_KID_SIGNED_JWT);

ArgumentCaptor<RequestEntity> requestEntityCaptor = ArgumentCaptor.forClass(RequestEntity.class);
verify(restOperations, times(2)).exchange(requestEntityCaptor.capture(), eq(String.class));
verifyNoMoreInteractions(restOperations);
List<RequestEntity> requestEntities = requestEntityCaptor.getAllValues();
assertThat(requestEntities.get(0).getHeaders().getAccept()).contains(MediaType.APPLICATION_JSON,
APPLICATION_JWK_SET_JSON);
assertThat(requestEntities.get(1).getHeaders().getAccept()).contains(MediaType.APPLICATION_JSON,
APPLICATION_JWK_SET_JSON);
}

@Test
public void decodeWhenCacheIsConfiguredAndValueLoaderErrorsThenThrowsJwtException() {
Cache cache = new ConcurrentMapCache("test-jwk-set-cache");
Expand All @@ -678,6 +765,27 @@ public void decodeWhenCacheIsConfiguredAndValueLoaderErrorsThenThrowsJwtExceptio
// @formatter:on
}

// gh-11621
@Test
public void decodeWhenCacheIsConfiguredAndParseFailsOnCachedValueThenExceptionIgnored() {
RestOperations restOperations = mock(RestOperations.class);
Cache cache = mock(Cache.class);
given(cache.get(eq(JWK_SET_URI), eq(String.class))).willReturn(JWK_SET);
given(cache.get(eq(JWK_SET_URI))).willReturn(mock(Cache.ValueWrapper.class));
// @formatter:off
NimbusJwtDecoder jwtDecoder = NimbusJwtDecoder.withJwkSetUri(JWK_SET_URI)
.cache(cache)
.restOperations(restOperations)
.build();
// @formatter:on
jwtDecoder.decode(SIGNED_JWT);
verify(cache).get(eq(JWK_SET_URI), eq(String.class));
verify(cache, times(2)).get(eq(JWK_SET_URI));
verifyNoMoreInteractions(cache);
verifyNoInteractions(restOperations);

}

// gh-8730
@Test
public void withJwkSetUriWhenUsingCustomTypeHeaderThenRefuseOmittedType() throws Exception {
Expand Down