From 37e6af85d0f8621bae00a17b0737f183500b0d23 Mon Sep 17 00:00:00 2001 From: BGuga Date: Thu, 16 May 2024 12:06:08 +0900 Subject: [PATCH 1/4] =?UTF-8?q?refactor:=20CachedAppleOpenIdKeyProvider.ja?= =?UTF-8?q?va=20=EC=82=AD=EC=A0=9C=20=EB=B0=8F=20prototype=20Scope=20?= =?UTF-8?q?=EC=A0=81=EC=9A=A9?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- .../openid/AppleOpenIdPublicKeyLocator.java | 2 +- .../openid/CachedAppleOpenIdKeyProvider.java | 63 ------------------- .../openid/CachedOpenIdKeyProvider.java | 2 + .../AppleOpenIdPublicKeyLocatorTest.java | 31 +++++++++ 4 files changed, 34 insertions(+), 64 deletions(-) delete mode 100644 backend/src/main/java/com/festago/auth/infrastructure/openid/CachedAppleOpenIdKeyProvider.java create mode 100644 backend/src/test/java/com/festago/auth/infrastructure/openid/AppleOpenIdPublicKeyLocatorTest.java diff --git a/backend/src/main/java/com/festago/auth/infrastructure/openid/AppleOpenIdPublicKeyLocator.java b/backend/src/main/java/com/festago/auth/infrastructure/openid/AppleOpenIdPublicKeyLocator.java index a0a20c09e..34a427d9c 100644 --- a/backend/src/main/java/com/festago/auth/infrastructure/openid/AppleOpenIdPublicKeyLocator.java +++ b/backend/src/main/java/com/festago/auth/infrastructure/openid/AppleOpenIdPublicKeyLocator.java @@ -13,7 +13,7 @@ public class AppleOpenIdPublicKeyLocator implements Locator { private final AppleOpenIdJwksClient appleOpenIdJwksClient; - private final CachedAppleOpenIdKeyProvider cachedOpenIdKeyProvider; + private final CachedOpenIdKeyProvider cachedOpenIdKeyProvider; @Override public Key locate(Header header) { diff --git a/backend/src/main/java/com/festago/auth/infrastructure/openid/CachedAppleOpenIdKeyProvider.java b/backend/src/main/java/com/festago/auth/infrastructure/openid/CachedAppleOpenIdKeyProvider.java deleted file mode 100644 index c336dbc0c..000000000 --- a/backend/src/main/java/com/festago/auth/infrastructure/openid/CachedAppleOpenIdKeyProvider.java +++ /dev/null @@ -1,63 +0,0 @@ -package com.festago.auth.infrastructure.openid; - -import com.festago.common.exception.UnexpectedException; -import io.jsonwebtoken.security.JwkSet; -import jakarta.annotation.Nullable; -import java.security.Key; -import java.util.HashMap; -import java.util.Map; -import java.util.concurrent.TimeUnit; -import java.util.concurrent.locks.ReentrantLock; -import java.util.function.Supplier; -import lombok.extern.slf4j.Slf4j; -import org.springframework.stereotype.Component; - -@Slf4j -@Component -public class CachedAppleOpenIdKeyProvider { - - private final Map cache = new HashMap<>(); - private final ReentrantLock lock = new ReentrantLock(); - - /** - * OpenId Key를 캐싱하여 반환하는 클래스
OpenID Id Token 헤더의 kid 값을 key로 가지도록 구현
Id Token을 검증할 때, 매번 공개키 목록을 조회하면 - * 요청이 차단될 수 있으므로 캐싱하는 과정이 필요.
따라서 kid에 대한 Key를 찾을 수 없으면, fallback을 통해 캐시를 업데이트함
이때, 동시에 여러 요청이 들어오면 동시성 - * 문제가 발생할 수 있으므로 ReentrantLock을 사용하여 상호 배제 구현
데드락을 방지하기 위해 ReentrantLock.tryLock() 메서드를 사용하였음
또한 반드시 - * fallback에서 Timeout에 대한 예외 발생을 구현 해야함
존재하지 않는 kid로 계속 요청 시 fallback이 계속 호출되므로 공격 가능성이 있음.
- * - * @param kid 캐시의 Key로 사용될 OpenId Id Token 헤더의 kid 값 - * @param fallback 캐시 미스 발생 시 캐시에 Key를 등록할 JwkSet을 반환하는 함수 - * @return 캐시 Hit의 경우 Key 반환, 캐시 Miss에서 fallback으로 반환된 JwkSet에 Key가 없으면 null 반환 - */ - @Nullable - public Key provide(String kid, Supplier fallback) { - Key key = cache.get(kid); - if (key != null) { - return key; - } - log.info("kid에 대한 OpenId Key를 찾지 못해 Key 목록 조회를 시도합니다. kid={}", kid); - try { - if (lock.tryLock(5, TimeUnit.SECONDS)) { - try { - key = cache.get(kid); - if (key != null) { - return key; - } - JwkSet jwkSet = fallback.get(); - jwkSet.forEach(jwk -> cache.put(jwk.getId(), jwk.toKey())); - key = cache.get(kid); - if (key == null) { - log.warn("OpenId kid에 대한 Key를 찾을 수 없습니다. kid={}", kid); - } - return key; - } finally { - lock.unlock(); - } - } - } catch (InterruptedException e) { - Thread.currentThread().interrupt(); - log.warn("스레드가 인터럽트 되었습니다.", e); - } - throw new UnexpectedException("OpenId Key를 가져오는 중, 락 대기로 인해 Key를 획득하지 못했습니다. kid=" + kid); - } -} diff --git a/backend/src/main/java/com/festago/auth/infrastructure/openid/CachedOpenIdKeyProvider.java b/backend/src/main/java/com/festago/auth/infrastructure/openid/CachedOpenIdKeyProvider.java index 14fc6feab..7a3619305 100644 --- a/backend/src/main/java/com/festago/auth/infrastructure/openid/CachedOpenIdKeyProvider.java +++ b/backend/src/main/java/com/festago/auth/infrastructure/openid/CachedOpenIdKeyProvider.java @@ -10,10 +10,12 @@ import java.util.concurrent.locks.ReentrantLock; import java.util.function.Supplier; import lombok.extern.slf4j.Slf4j; +import org.springframework.context.annotation.Scope; import org.springframework.stereotype.Component; @Slf4j @Component +@Scope("prototype") public class CachedOpenIdKeyProvider { private final Map cache = new HashMap<>(); diff --git a/backend/src/test/java/com/festago/auth/infrastructure/openid/AppleOpenIdPublicKeyLocatorTest.java b/backend/src/test/java/com/festago/auth/infrastructure/openid/AppleOpenIdPublicKeyLocatorTest.java new file mode 100644 index 000000000..8dc31f029 --- /dev/null +++ b/backend/src/test/java/com/festago/auth/infrastructure/openid/AppleOpenIdPublicKeyLocatorTest.java @@ -0,0 +1,31 @@ +package com.festago.auth.infrastructure.openid; + +import static org.assertj.core.api.Assertions.assertThat; + +import com.festago.support.ApplicationIntegrationTest; +import org.junit.jupiter.api.DisplayNameGeneration; +import org.junit.jupiter.api.DisplayNameGenerator.ReplaceUnderscores; +import org.junit.jupiter.api.Test; +import org.springframework.beans.factory.annotation.Autowired; + +@DisplayNameGeneration(ReplaceUnderscores.class) +@SuppressWarnings("NonAsciiCharacters") +class AppleOpenIdPublicKeyLocatorTest extends ApplicationIntegrationTest { + + @Autowired + AppleOpenIdPublicKeyLocator appleOpenIdPublicKeyLocator; + + @Autowired + KakaoOpenIdPublicKeyLocator kakaoOpenIdPublicKeyLocator; + + @Test + void 소셜별_Locator_들은_캐싱을_공유하지_않는다() { + + // given & when & then + assertThat(appleOpenIdPublicKeyLocator) + .usingRecursiveComparison() + .comparingOnlyFields("cachedOpenIdKeyProvider") + .isNotEqualTo(kakaoOpenIdPublicKeyLocator); + } + +} From 24bdbce8508fa6815c70f8ad512f73e6b8003854 Mon Sep 17 00:00:00 2001 From: BGuga Date: Thu, 16 May 2024 12:09:01 +0900 Subject: [PATCH 2/4] =?UTF-8?q?refactor:=20Audience=20=EB=A5=BC=20OpenIdTo?= =?UTF-8?q?kenParser=20=EC=97=90=EC=84=9C=20=EA=B2=80=EC=A6=9D=ED=95=98?= =?UTF-8?q?=EB=8F=84=EB=A1=9D=20=EB=B3=80=EA=B2=BD?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- .../infrastructure/openid/AppleOpenIdClient.java | 15 +-------------- 1 file changed, 1 insertion(+), 14 deletions(-) diff --git a/backend/src/main/java/com/festago/auth/infrastructure/openid/AppleOpenIdClient.java b/backend/src/main/java/com/festago/auth/infrastructure/openid/AppleOpenIdClient.java index df228713c..a64433ab3 100644 --- a/backend/src/main/java/com/festago/auth/infrastructure/openid/AppleOpenIdClient.java +++ b/backend/src/main/java/com/festago/auth/infrastructure/openid/AppleOpenIdClient.java @@ -4,13 +4,10 @@ import com.festago.auth.domain.OpenIdNonceValidator; import com.festago.auth.domain.SocialType; import com.festago.auth.domain.UserInfo; -import com.festago.common.exception.ErrorCode; -import com.festago.common.exception.UnauthorizedException; import io.jsonwebtoken.Claims; import io.jsonwebtoken.Jwts; import java.time.Clock; import java.util.Date; -import java.util.Set; import lombok.extern.slf4j.Slf4j; import org.springframework.beans.factory.annotation.Value; import org.springframework.stereotype.Component; @@ -34,6 +31,7 @@ public AppleOpenIdClient( this.openIdNonceValidator = openIdNonceValidator; this.idTokenParser = new OpenIdIdTokenParser(Jwts.parser() .keyLocator(appleOpenIdPublicKeyLocator) + .requireAudience(clientId) .requireIssuer(ISSUER) .clock(() -> Date.from(clock.instant())) .build()); @@ -43,23 +41,12 @@ public AppleOpenIdClient( public UserInfo getUserInfo(String idToken) { Claims payload = idTokenParser.parse(idToken); openIdNonceValidator.validate(payload.get("nonce", String.class), payload.getExpiration()); - validateAudience(payload.getAudience()); return UserInfo.builder() .socialType(SocialType.APPLE) .socialId(payload.getSubject()) .build(); } - private void validateAudience(Set audiences) { - for (String audience : audiences) { - if (clientId.equals(audience)) { - return; - } - } - log.info("허용되지 않는 id 토큰의 audience 값이 요청되었습니다. audiences={}", audiences); - throw new UnauthorizedException(ErrorCode.OPEN_ID_INVALID_TOKEN); - } - @Override public SocialType getSocialType() { return SocialType.APPLE; From e482fabbd87ed03a4b2f2b43184487c52018bc19 Mon Sep 17 00:00:00 2001 From: BGuga Date: Thu, 16 May 2024 12:12:42 +0900 Subject: [PATCH 3/4] =?UTF-8?q?chore:=20=EB=81=9D=EC=A4=84=20=EA=B0=9C?= =?UTF-8?q?=ED=96=89=20=EC=A0=9C=EA=B1=B0?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- .../auth/infrastructure/openid/AppleOpenIdClientTest.java | 2 -- .../auth/infrastructure/openid/AppleOpenIdJwksClientTest.java | 2 -- .../infrastructure/openid/AppleOpenIdPublicKeyLocatorTest.java | 1 - 3 files changed, 5 deletions(-) diff --git a/backend/src/test/java/com/festago/auth/infrastructure/openid/AppleOpenIdClientTest.java b/backend/src/test/java/com/festago/auth/infrastructure/openid/AppleOpenIdClientTest.java index bd28c0965..a46105aeb 100644 --- a/backend/src/test/java/com/festago/auth/infrastructure/openid/AppleOpenIdClientTest.java +++ b/backend/src/test/java/com/festago/auth/infrastructure/openid/AppleOpenIdClientTest.java @@ -185,6 +185,4 @@ void setUp() { // then assertThat(expect.socialId()).isEqualTo(socialId); } - - } diff --git a/backend/src/test/java/com/festago/auth/infrastructure/openid/AppleOpenIdJwksClientTest.java b/backend/src/test/java/com/festago/auth/infrastructure/openid/AppleOpenIdJwksClientTest.java index 559721c81..72afd648a 100644 --- a/backend/src/test/java/com/festago/auth/infrastructure/openid/AppleOpenIdJwksClientTest.java +++ b/backend/src/test/java/com/festago/auth/infrastructure/openid/AppleOpenIdJwksClientTest.java @@ -106,6 +106,4 @@ class AppleOpenIdJwksClientTest { .map(Identifiable::getId) .containsExactlyInAnyOrder("pyaRQpAbnY", "lVHdOx8ltR", "Bh6H7rHVmb"); } - - } diff --git a/backend/src/test/java/com/festago/auth/infrastructure/openid/AppleOpenIdPublicKeyLocatorTest.java b/backend/src/test/java/com/festago/auth/infrastructure/openid/AppleOpenIdPublicKeyLocatorTest.java index 8dc31f029..5eb9ea0ca 100644 --- a/backend/src/test/java/com/festago/auth/infrastructure/openid/AppleOpenIdPublicKeyLocatorTest.java +++ b/backend/src/test/java/com/festago/auth/infrastructure/openid/AppleOpenIdPublicKeyLocatorTest.java @@ -27,5 +27,4 @@ class AppleOpenIdPublicKeyLocatorTest extends ApplicationIntegrationTest { .comparingOnlyFields("cachedOpenIdKeyProvider") .isNotEqualTo(kakaoOpenIdPublicKeyLocator); } - } From ecb8c2a093604685caf21da098587d057605fab3 Mon Sep 17 00:00:00 2001 From: BGuga Date: Thu, 16 May 2024 15:42:07 +0900 Subject: [PATCH 4/4] =?UTF-8?q?test:=20=EB=B6=88=ED=95=84=EC=9A=94=20?= =?UTF-8?q?=ED=85=8C=EC=8A=A4=ED=8A=B8=20=EC=82=AD=EC=A0=9C?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- .../AppleOpenIdPublicKeyLocatorTest.java | 30 ------------------- 1 file changed, 30 deletions(-) delete mode 100644 backend/src/test/java/com/festago/auth/infrastructure/openid/AppleOpenIdPublicKeyLocatorTest.java diff --git a/backend/src/test/java/com/festago/auth/infrastructure/openid/AppleOpenIdPublicKeyLocatorTest.java b/backend/src/test/java/com/festago/auth/infrastructure/openid/AppleOpenIdPublicKeyLocatorTest.java deleted file mode 100644 index 5eb9ea0ca..000000000 --- a/backend/src/test/java/com/festago/auth/infrastructure/openid/AppleOpenIdPublicKeyLocatorTest.java +++ /dev/null @@ -1,30 +0,0 @@ -package com.festago.auth.infrastructure.openid; - -import static org.assertj.core.api.Assertions.assertThat; - -import com.festago.support.ApplicationIntegrationTest; -import org.junit.jupiter.api.DisplayNameGeneration; -import org.junit.jupiter.api.DisplayNameGenerator.ReplaceUnderscores; -import org.junit.jupiter.api.Test; -import org.springframework.beans.factory.annotation.Autowired; - -@DisplayNameGeneration(ReplaceUnderscores.class) -@SuppressWarnings("NonAsciiCharacters") -class AppleOpenIdPublicKeyLocatorTest extends ApplicationIntegrationTest { - - @Autowired - AppleOpenIdPublicKeyLocator appleOpenIdPublicKeyLocator; - - @Autowired - KakaoOpenIdPublicKeyLocator kakaoOpenIdPublicKeyLocator; - - @Test - void 소셜별_Locator_들은_캐싱을_공유하지_않는다() { - - // given & when & then - assertThat(appleOpenIdPublicKeyLocator) - .usingRecursiveComparison() - .comparingOnlyFields("cachedOpenIdKeyProvider") - .isNotEqualTo(kakaoOpenIdPublicKeyLocator); - } -}