Refresh remote JWK when unknown KID error occurs

Closes gh-11621
spring-projects · Aug 18, 2022 · 7c7f938 · 7c7f938
1 parent 888715b
commit 7c7f938
Show file tree

Hide file tree

Showing 2 changed files with 107 additions and 83 deletions.
diff --git a/...2/oauth2-jose/src/main/java/org/springframework/security/oauth2/jwt/NimbusJwtDecoder.java b/...2/oauth2-jose/src/main/java/org/springframework/security/oauth2/jwt/NimbusJwtDecoder.java
@@ -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.
@@ -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() {
@@ -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;
 			}
 
 		}

diff --git a/...th2-jose/src/test/java/org/springframework/security/oauth2/jwt/NimbusJwtDecoderTests.java b/...th2-jose/src/test/java/org/springframework/security/oauth2/jwt/NimbusJwtDecoderTests.java
@@ -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.
@@ -32,7 +32,6 @@
 import java.util.Date;
 import java.util.List;
 import java.util.Map;
-import java.util.concurrent.Callable;
 
 import javax.crypto.SecretKey;
 
@@ -43,9 +42,6 @@
 import com.nimbusds.jose.JWSSigner;
 import com.nimbusds.jose.crypto.MACSigner;
 import com.nimbusds.jose.crypto.RSASSASigner;
-import com.nimbusds.jose.jwk.JWKSet;
-import com.nimbusds.jose.jwk.RSAKey;
-import com.nimbusds.jose.jwk.gen.RSAKeyGenerator;
 import com.nimbusds.jose.jwk.source.JWKSource;
 import com.nimbusds.jose.proc.BadJOSEException;
 import com.nimbusds.jose.proc.DefaultJOSEObjectTypeVerifier;
@@ -102,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.";
@@ -649,35 +649,59 @@ 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), any(Callable.class))).willReturn(JWK_SET);
-
-		RSAKey rsaJWK = new RSAKeyGenerator(2048)
-				.keyID("new_kid")
-				.generate();
-		String jwkSetWithNewKid = new JWKSet(rsaJWK).toPublicJWKSet().toString();
+		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<>(jwkSetWithNewKid, HttpStatus.OK));
+				.willReturn(new ResponseEntity<>(NEW_KID_JWK_SET, HttpStatus.OK));
 
 		// @formatter:off
 		NimbusJwtDecoder jwtDecoder = NimbusJwtDecoder.withJwkSetUri(JWK_SET_URI)
@@ -687,32 +711,21 @@ public void decodeWhenCacheAndUnknownKidShouldTriggerFetchOfJwkSet() throws JOSE
 		// @formatter:on
 
 		// Decode JWT with new KID
-		JWSSigner signer = new RSASSASigner(rsaJWK);
-		JWTClaimsSet claimsSet = new JWTClaimsSet.Builder()
-				.expirationTime(Date.from(Instant.now().plusSeconds(60)))
-				.build();
-		SignedJWT signedJWT = new SignedJWT(new JWSHeader.Builder(JWSAlgorithm.RS256).keyID(rsaJWK.getKeyID()).build(), claimsSet);
-		signedJWT.sign(signer);
-		String token = signedJWT.serialize();
-
-		jwtDecoder.decode(token);
+		jwtDecoder.decode(NEW_KID_SIGNED_JWT);
 
-		ArgumentCaptor<RequestEntity>  requestEntityCaptor = ArgumentCaptor.forClass(RequestEntity.class);
+		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);
+		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);
-
-		RSAKey rsaJWK = new RSAKeyGenerator(2048)
-				.keyID("new_kid")
-				.generate();
-		String jwkSetWithNewKid = new JWKSet(rsaJWK).toPublicJWKSet().toString();
-		given(restOperations.exchange(any(RequestEntity.class), eq(String.class)))
-				.willReturn(new ResponseEntity<>(JWK_SET, HttpStatus.OK), new ResponseEntity<>(jwkSetWithNewKid, HttpStatus.OK));
+		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)
@@ -722,22 +735,16 @@ public void decodeWithoutCacheSpecifiedAndUnknownKidShouldTriggerFetchOfJwkSet()
 		jwtDecoder.decode(SIGNED_JWT);
 
 		// Decode JWT with new KID
-		JWSSigner signer = new RSASSASigner(rsaJWK);
-		JWTClaimsSet claimsSet = new JWTClaimsSet.Builder()
-				.expirationTime(Date.from(Instant.now().plusSeconds(60)))
-				.build();
-		SignedJWT signedJWT = new SignedJWT(new JWSHeader.Builder(JWSAlgorithm.RS256).keyID(rsaJWK.getKeyID()).build(), claimsSet);
-		signedJWT.sign(signer);
-		String token = signedJWT.serialize();
-
-		jwtDecoder.decode(token);
+		jwtDecoder.decode(NEW_KID_SIGNED_JWT);
 
-		ArgumentCaptor<RequestEntity>  requestEntityCaptor = ArgumentCaptor.forClass(RequestEntity.class);
+		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);
+		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
@@ -758,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 {