gnodet-bot commented on code in PR #29058: URL: https://github.com/apache/camel/pull/29058#discussion_r4467706639
########## components/camel-keycloak/src/test/java/org/apache/camel/component/keycloak/security/KeycloakJwksCacheStampedeTest.java: ########## @@ -0,0 +1,285 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.apache.camel.component.keycloak.security; + +import java.io.IOException; +import java.math.BigInteger; +import java.net.InetSocketAddress; +import java.nio.charset.StandardCharsets; +import java.security.KeyPair; +import java.security.KeyPairGenerator; +import java.security.interfaces.RSAPublicKey; +import java.util.Base64; +import java.util.concurrent.CountDownLatch; +import java.util.concurrent.ExecutorService; +import java.util.concurrent.Executors; +import java.util.concurrent.TimeUnit; +import java.util.concurrent.atomic.AtomicInteger; + +import com.sun.net.httpserver.HttpServer; +import org.apache.camel.CamelAuthorizationException; +import org.apache.camel.CamelContext; +import org.apache.camel.Exchange; +import org.apache.camel.impl.DefaultCamelContext; +import org.apache.camel.support.DefaultExchange; +import org.junit.jupiter.api.AfterEach; +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.Test; +import org.keycloak.jose.jws.JWSBuilder; +import org.keycloak.representations.AccessToken; + +import static org.assertj.core.api.Assertions.assertThat; +import static org.assertj.core.api.Assertions.assertThatThrownBy; +import static org.awaitility.Awaitility.await; + +/** + * Tests verifying that {@link KeycloakPublicKeyResolver} and {@link KeycloakSecurityProcessor} rate limit remote JWKS + * refreshes for unknown key IDs, preventing cache stampede. + */ +class KeycloakJwksCacheStampedeTest { + + private HttpServer server; + private int serverPort; + private AtomicInteger jwksRequestCount; + private KeyPair keyPair; + private String jwksResponse; + private CamelContext context; + + @BeforeEach + void setUp() throws Exception { + context = new DefaultCamelContext(); + context.start(); + + KeyPairGenerator keyGen = KeyPairGenerator.getInstance("RSA"); + keyGen.initialize(2048); + keyPair = keyGen.generateKeyPair(); + RSAPublicKey rsaPub = (RSAPublicKey) keyPair.getPublic(); + + jwksResponse = "{\"keys\":[" + String.format( + "{\"kty\":\"RSA\",\"use\":\"sig\",\"kid\":\"valid-key-1\",\"n\":\"%s\",\"e\":\"%s\"}", + b64url(rsaPub.getModulus()), b64url(rsaPub.getPublicExponent())) + "]}"; + + jwksRequestCount = new AtomicInteger(); + server = HttpServer.create(new InetSocketAddress("127.0.0.1", 0), 0); + server.createContext("/realms/test-realm/protocol/openid-connect/certs", httpExchange -> { + jwksRequestCount.incrementAndGet(); + byte[] bytes = jwksResponse.getBytes(StandardCharsets.UTF_8); + httpExchange.getResponseHeaders().set("Content-Type", "application/json"); + httpExchange.sendResponseHeaders(200, bytes.length); + httpExchange.getResponseBody().write(bytes); + httpExchange.close(); + }); + server.start(); + serverPort = server.getAddress().getPort(); + } + + @AfterEach + void tearDown() { + if (server != null) { + server.stop(0); + } + if (context != null) { + context.stop(); + } + } + + private static String b64url(BigInteger value) { + byte[] bytes = value.toByteArray(); + if (bytes.length > 1 && bytes[0] == 0) { + byte[] unsigned = new byte[bytes.length - 1]; + System.arraycopy(bytes, 1, unsigned, 0, unsigned.length); + bytes = unsigned; + } + return Base64.getUrlEncoder().withoutPadding().encodeToString(bytes); + } + + @Test + void testDirectResolverRateLimitsJwksRefreshOnUnknownKid() throws Exception { + String serverUrl = "http://127.0.0.1:"; + serverPort; + KeycloakPublicKeyResolver resolver = new KeycloakPublicKeyResolver(serverUrl, "test-realm"); + + // 1. Initial request with known kid loads the cache (1 network request) + assertThat(resolver.getPublicKey("valid-key-1")).isNotNull(); + assertThat(jwksRequestCount.get()).isEqualTo(1); + + // 2. Subsequent requests for known kid hit cache (no network requests) + assertThat(resolver.getPublicKey("valid-key-1")).isNotNull(); + assertThat(jwksRequestCount.get()).isEqualTo(1); + + // 3. Requests with unknown kids within the cooldown window do NOT trigger remote JWKS refreshes + int unknownKidAttempts = 5; + for (int i = 0; i < unknownKidAttempts; i++) { + final String unknownKid = "unknown-kid-" + i; + assertThatThrownBy(() -> resolver.getPublicKey(unknownKid)) + .isInstanceOf(IOException.class) + .hasMessageContaining("No public key found in Keycloak JWKS for kid: " + unknownKid); + } + + // Outbound request count must still be 1 (stampede prevented) + assertThat(jwksRequestCount.get()) + .as("Unknown kids within cooldown window did not trigger remote JWKS fetches") + .isEqualTo(1); + } + + @Test + void testConcurrentRequestsWithUnknownKidsTriggerSingleFetch() throws Exception { + String serverUrl = "http://127.0.0.1:"; + serverPort; + KeycloakPublicKeyResolver resolver = new KeycloakPublicKeyResolver(serverUrl, "test-realm"); + + // Initial fetch loads cache + assertThat(resolver.getPublicKey("valid-key-1")).isNotNull(); + assertThat(jwksRequestCount.get()).isEqualTo(1); + + int threadCount = 10; + ExecutorService executor = Executors.newFixedThreadPool(threadCount); + CountDownLatch startLatch = new CountDownLatch(1); + CountDownLatch doneLatch = new CountDownLatch(threadCount); + AtomicInteger failureCount = new AtomicInteger(); + + for (int i = 0; i < threadCount; i++) { + final String unknownKid = "concurrent-unknown-kid-" + i; + executor.submit(() -> { + try { + startLatch.await(); + resolver.getPublicKey(unknownKid); + } catch (IOException expected) { + failureCount.incrementAndGet(); + } catch (InterruptedException e) { + Thread.currentThread().interrupt(); + } finally { + doneLatch.countDown(); + } + }); + } + + // Release all threads simultaneously + startLatch.countDown(); + assertThat(doneLatch.await(5, TimeUnit.SECONDS)).isTrue(); + executor.shutdown(); + + assertThat(failureCount.get()).isEqualTo(threadCount); + // All concurrent threads for unknown kids must not trigger separate remote fetches + assertThat(jwksRequestCount.get()).isEqualTo(1); Review Comment: **`testConcurrentRequestsWithUnknownKidsTriggerSingleFetch` doesn't exercise the double-check under lock.** The warm-up fetch on line 144 (`resolver.getPublicKey("valid-key-1")`) sets `lastRefreshAttemptNanos`. The 10 concurrent threads then all see `elapsed <= minRefreshIntervalNanos` in the **outer** guard (the unlocked `if` check in `getPublicKey()`) and return without ever calling `refreshKeysIfStale()`. The test asserts `jwksRequestCount == 1`, which is correct — but the mutex re-check inside `refreshKeysIfStale()` (the core of the fix) is never reached. To pin the actual lock + re-check logic, use a short cooldown (e.g. 0ms or 1ms) and skip the warm-up, so all threads contend on the first fetch: ```java // Use 0ms cooldown so every thread passes the outer guard and contends on the lock KeycloakPublicKeyResolver resolver = new KeycloakPublicKeyResolver(serverUrl, "test-realm", 300_000, 0); // No warm-up — threads start with an empty cache and lastRefreshAttemptNanos == 0 // All 10 threads should queue on the lock; only one should trigger a fetch ``` The existing `testJwksRefreshAllowedAfterCooldown` covers the cooldown boundary correctly, but is single-threaded. ########## components/camel-keycloak/src/main/java/org/apache/camel/component/keycloak/security/KeycloakPublicKeyResolver.java: ########## @@ -197,7 +238,7 @@ public String getExpectedIssuer() { * Clears the key cache. */ public void clearCache() { - keyCache.clear(); - lastRefreshTime = 0; + keyCache = Map.of(); + lastRefreshAttemptNanos = 0; } Review Comment: **[Re-raised from prior review]** `clearCache()`: two volatile writes outside the lock. A concurrent `getPublicKey()` call racing between these two writes sees `keyCache.isEmpty() == true` (→ picks `minRefreshIntervalNanos`) while `lastRefreshAttemptNanos` still holds a stale non-zero value — the cooldown suppression blocks the refresh that was just needed. Acquire `lock.lock()` here, as `refreshKeysIfStale()` does for its own writes. ```suggestion public void clearCache() { lock.lock(); try { keyCache = Map.of(); lastRefreshAttemptNanos = 0; } finally { lock.unlock(); } } ``` ########## docs/user-manual/modules/ROOT/pages/camel-4x-upgrade-guide-4_24.adoc: ########## @@ -13,6 +13,14 @@ See the xref:camel-upgrade-recipes-tool.adoc[documentation] page for details. == Upgrading Camel 4.23 to 4.24 +=== camel-keycloak - JWKS public key resolution rate limiting + +`KeycloakSecurityPolicy` now enforces rate limiting on public key refresh calls to Keycloak's JWKS endpoint (`/protocol/openid-connect/certs`) to prevent cache stampedes during key rotation or when requests with unknown `kid` (key ID) headers are processed. + +The new configuration option `minJwksRefreshInterval` defines the minimum cooldown interval in seconds between consecutive JWKS refresh attempts, defaulting to `10` seconds. If an incoming token carries an unknown `kid` and a refresh was attempted within the cooldown window, the cached keys are reused without invoking the remote JWKS endpoint. Review Comment: **Inaccurate:** tokens with an unknown `kid` during the cooldown window are **rejected** (IOException from `selectKey()`), not served from the cache. The key isn't in `keyCache`, so the lookup fails closed. Suggested correction: ```suggestion The new configuration option `minJwksRefreshInterval` defines the minimum cooldown interval in seconds between consecutive JWKS refresh attempts, defaulting to `10` seconds. If an incoming token carries an unknown `kid` and a refresh was attempted within the cooldown window, the JWKS refresh is deferred and the token is rejected — the endpoint will not be contacted again until the cooldown elapses. ``` -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
