rmannibucau commented on code in PR #7: URL: https://github.com/apache/geronimo-jwt-auth/pull/7#discussion_r1557913665
########## src/main/java/org/apache/geronimo/microprofile/impl/jwtauth/jwt/KidMapper.java: ########## @@ -79,9 +101,48 @@ private void init() { .collect(Collectors.toSet())) .orElseGet(HashSet::new); ofNullable(config.read("issuer.default", config.read(Names.ISSUER, null))).ifPresent(defaultIssuers::add); + jwksUrl = config.read("mp.jwt.verify.publickey.location", null); + readerFactory = Json.createReaderFactory(emptyMap()); + ofNullable(jwksUrl).ifPresent(url -> { + HttpClient.Builder builder = HttpClient.newBuilder(); Review Comment: guess minimum is a `protected void customize(HttpClient.Builder)` method - to be transparent I can't replace by custom code fillung the key map with that cause of missing sslContext+sslParameters setup, not asking to include it by default but enable it - guess the protected method is the fastest but I'm fine with an event too. ########## src/main/java/org/apache/geronimo/microprofile/impl/jwtauth/jwt/KidMapper.java: ########## @@ -79,9 +101,48 @@ private void init() { .collect(Collectors.toSet())) .orElseGet(HashSet::new); ofNullable(config.read("issuer.default", config.read(Names.ISSUER, null))).ifPresent(defaultIssuers::add); + jwksUrl = config.read("mp.jwt.verify.publickey.location", null); + readerFactory = Json.createReaderFactory(emptyMap()); + ofNullable(jwksUrl).ifPresent(url -> { + HttpClient.Builder builder = HttpClient.newBuilder(); + if (getJwksRefreshInterval() != null) { + long secondsRefresh = getJwksRefreshInterval(); + backgroundThread = Executors.newSingleThreadScheduledExecutor(); + builder.executor(backgroundThread); + backgroundThread.scheduleAtFixedRate(this::reloadRemoteKeys, getJwksRefreshInterval(), secondsRefresh, SECONDS); + } + httpClient = builder.build(); + reloadJwksRequest = reloadRemoteKeys();// inital load, otherwise the background thread is too slow to start and serve + }); defaultKey = config.read("public-key.default", config.read(Names.VERIFIER_PUBLIC_KEY, null)); } + private Integer getJwksRefreshInterval() { + String interval = config.read("jwks.invalidation.interval",null); + if (interval != null) { + return Integer.parseInt(interval); + } else { + return null; + } + } + + private CompletableFuture<Void> reloadRemoteKeys() { + HttpRequest request = HttpRequest.newBuilder().GET().uri(URI.create(jwksUrl)).header("Accept", "application/json").build(); Review Comment: can be worth to be protected too, some servers are not that friendly (don't hit, not my choice ;)) ########## src/main/java/org/apache/geronimo/microprofile/impl/jwtauth/jwt/KidMapper.java: ########## @@ -16,43 +16,65 @@ */ package org.apache.geronimo.microprofile.impl.jwtauth.jwt; +import static java.util.Collections.emptyMap; import static java.util.Optional.ofNullable; +import static java.util.concurrent.TimeUnit.SECONDS; import static java.util.stream.Collectors.joining; +import static java.util.stream.Collectors.toList; +import org.apache.geronimo.microprofile.impl.jwtauth.config.GeronimoJwtAuthConfig; +import org.apache.geronimo.microprofile.impl.jwtauth.io.PropertiesLoader; +import org.eclipse.microprofile.jwt.config.Names; +import javax.annotation.PostConstruct; +import javax.annotation.PreDestroy; +import javax.enterprise.context.ApplicationScoped; +import javax.inject.Inject; +import javax.json.Json; +import javax.json.JsonArray; +import javax.json.JsonObject; +import javax.json.JsonReader; +import javax.json.JsonReaderFactory; +import javax.json.JsonValue; import java.io.BufferedReader; import java.io.File; import java.io.IOException; import java.io.InputStream; import java.io.InputStreamReader; +import java.io.StringReader; +import java.net.URI; +import java.net.http.HttpClient; +import java.net.http.HttpRequest; +import java.net.http.HttpResponse; import java.nio.file.Files; import java.util.Collection; import java.util.HashMap; import java.util.HashSet; +import java.util.List; import java.util.Map; import java.util.Set; +import java.util.concurrent.CompletableFuture; import java.util.concurrent.ConcurrentHashMap; import java.util.concurrent.ConcurrentMap; +import java.util.concurrent.ExecutionException; +import java.util.concurrent.Executors; +import java.util.concurrent.ScheduledExecutorService; import java.util.stream.Collectors; import java.util.stream.Stream; -import javax.annotation.PostConstruct; -import javax.enterprise.context.ApplicationScoped; -import javax.inject.Inject; - -import org.apache.geronimo.microprofile.impl.jwtauth.config.GeronimoJwtAuthConfig; -import org.apache.geronimo.microprofile.impl.jwtauth.io.PropertiesLoader; -import org.eclipse.microprofile.jwt.config.Names; - @ApplicationScoped public class KidMapper { @Inject private GeronimoJwtAuthConfig config; - private final ConcurrentMap<String, String> keyMapping = new ConcurrentHashMap<>(); + private ConcurrentMap<String, String> keyMapping = new ConcurrentHashMap<>(); private final Map<String, Collection<String>> issuerMapping = new HashMap<>(); private String defaultKey; + private String jwksUrl; private Set<String> defaultIssuers; - + private JsonReaderFactory readerFactory; + private CompletableFuture<Void> reloadJwksRequest; + HttpClient httpClient; Review Comment: private? ########## src/main/java/org/apache/geronimo/microprofile/impl/jwtauth/jwt/KidMapper.java: ########## @@ -79,9 +101,48 @@ private void init() { .collect(Collectors.toSet())) .orElseGet(HashSet::new); ofNullable(config.read("issuer.default", config.read(Names.ISSUER, null))).ifPresent(defaultIssuers::add); + jwksUrl = config.read("mp.jwt.verify.publickey.location", null); + readerFactory = Json.createReaderFactory(emptyMap()); + ofNullable(jwksUrl).ifPresent(url -> { + HttpClient.Builder builder = HttpClient.newBuilder(); + if (getJwksRefreshInterval() != null) { + long secondsRefresh = getJwksRefreshInterval(); + backgroundThread = Executors.newSingleThreadScheduledExecutor(); + builder.executor(backgroundThread); + backgroundThread.scheduleAtFixedRate(this::reloadRemoteKeys, getJwksRefreshInterval(), secondsRefresh, SECONDS); + } + httpClient = builder.build(); + reloadJwksRequest = reloadRemoteKeys();// inital load, otherwise the background thread is too slow to start and serve + }); defaultKey = config.read("public-key.default", config.read(Names.VERIFIER_PUBLIC_KEY, null)); } + private Integer getJwksRefreshInterval() { + String interval = config.read("jwks.invalidation.interval",null); + if (interval != null) { + return Integer.parseInt(interval); + } else { + return null; + } + } + + private CompletableFuture<Void> reloadRemoteKeys() { + HttpRequest request = HttpRequest.newBuilder().GET().uri(URI.create(jwksUrl)).header("Accept", "application/json").build(); + CompletableFuture<HttpResponse<String>> httpResponseCompletableFuture = httpClient.sendAsync(request, HttpResponse.BodyHandlers.ofString()); + CompletableFuture<Void> ongoingRequest = httpResponseCompletableFuture.thenApply(result -> { Review Comment: `.thenApplyAsync(...., httpClient.executor().orElseThrow())` otherwise it is ran in commonPool which is something we don't want nor we control there. ########## src/main/java/org/apache/geronimo/microprofile/impl/jwtauth/jwt/KidMapper.java: ########## @@ -79,9 +101,48 @@ private void init() { .collect(Collectors.toSet())) .orElseGet(HashSet::new); ofNullable(config.read("issuer.default", config.read(Names.ISSUER, null))).ifPresent(defaultIssuers::add); + jwksUrl = config.read("mp.jwt.verify.publickey.location", null); + readerFactory = Json.createReaderFactory(emptyMap()); + ofNullable(jwksUrl).ifPresent(url -> { + HttpClient.Builder builder = HttpClient.newBuilder(); + if (getJwksRefreshInterval() != null) { Review Comment: store the interval in a local var to avoid to call it N times ########## src/main/java/org/apache/geronimo/microprofile/impl/jwtauth/jwt/KidMapper.java: ########## @@ -79,9 +101,48 @@ private void init() { .collect(Collectors.toSet())) .orElseGet(HashSet::new); ofNullable(config.read("issuer.default", config.read(Names.ISSUER, null))).ifPresent(defaultIssuers::add); + jwksUrl = config.read("mp.jwt.verify.publickey.location", null); + readerFactory = Json.createReaderFactory(emptyMap()); + ofNullable(jwksUrl).ifPresent(url -> { + HttpClient.Builder builder = HttpClient.newBuilder(); + if (getJwksRefreshInterval() != null) { + long secondsRefresh = getJwksRefreshInterval(); + backgroundThread = Executors.newSingleThreadScheduledExecutor(); + builder.executor(backgroundThread); + backgroundThread.scheduleAtFixedRate(this::reloadRemoteKeys, getJwksRefreshInterval(), secondsRefresh, SECONDS); + } + httpClient = builder.build(); + reloadJwksRequest = reloadRemoteKeys();// inital load, otherwise the background thread is too slow to start and serve Review Comment: if interval is null or < 0 just drop the client (`httpClient=null`) ########## src/main/java/org/apache/geronimo/microprofile/impl/jwtauth/jwt/KidMapper.java: ########## @@ -79,9 +101,48 @@ private void init() { .collect(Collectors.toSet())) .orElseGet(HashSet::new); ofNullable(config.read("issuer.default", config.read(Names.ISSUER, null))).ifPresent(defaultIssuers::add); + jwksUrl = config.read("mp.jwt.verify.publickey.location", null); + readerFactory = Json.createReaderFactory(emptyMap()); + ofNullable(jwksUrl).ifPresent(url -> { + HttpClient.Builder builder = HttpClient.newBuilder(); + if (getJwksRefreshInterval() != null) { + long secondsRefresh = getJwksRefreshInterval(); + backgroundThread = Executors.newSingleThreadScheduledExecutor(); + builder.executor(backgroundThread); + backgroundThread.scheduleAtFixedRate(this::reloadRemoteKeys, getJwksRefreshInterval(), secondsRefresh, SECONDS); + } + httpClient = builder.build(); + reloadJwksRequest = reloadRemoteKeys();// inital load, otherwise the background thread is too slow to start and serve + }); defaultKey = config.read("public-key.default", config.read(Names.VERIFIER_PUBLIC_KEY, null)); } + private Integer getJwksRefreshInterval() { + String interval = config.read("jwks.invalidation.interval",null); + if (interval != null) { + return Integer.parseInt(interval); + } else { + return null; + } + } + + private CompletableFuture<Void> reloadRemoteKeys() { + HttpRequest request = HttpRequest.newBuilder().GET().uri(URI.create(jwksUrl)).header("Accept", "application/json").build(); + CompletableFuture<HttpResponse<String>> httpResponseCompletableFuture = httpClient.sendAsync(request, HttpResponse.BodyHandlers.ofString()); + CompletableFuture<Void> ongoingRequest = httpResponseCompletableFuture.thenApply(result -> { + List<JWK> jwks = parseKeys(result); + ConcurrentHashMap<String, String> newKeys = new ConcurrentHashMap<>(); + jwks.forEach(key -> newKeys.put(key.getKid(), key.toPemKey())); + keyMapping = newKeys; + return null; + }); + + ongoingRequest.thenRun(() -> { Review Comment: not needed, just set `reloadJwksRequest` in previous line ########## src/test/java/org/apache/geronimo/microprofile/impl/jwtauth/jwt/KidMapperTest.java: ########## @@ -0,0 +1,52 @@ +package org.apache.geronimo.microprofile.impl.jwtauth.jwt; + +import static javax.ws.rs.client.ClientBuilder.newClient; +import static org.testng.Assert.assertEquals; + +import java.io.IOException; +import java.net.URISyntaxException; +import java.net.URL; + +import org.jboss.arquillian.container.test.api.Deployment; +import org.jboss.arquillian.container.test.api.RunAsClient; +import org.jboss.arquillian.test.api.ArquillianResource; +import org.jboss.arquillian.testng.Arquillian; +import org.jboss.shrinkwrap.api.ShrinkWrap; +import org.jboss.shrinkwrap.api.spec.WebArchive; +import org.testng.annotations.AfterClass; +import org.testng.annotations.Test; + +public class KidMapperTest extends Arquillian { + + private static JwksServer jwksServer; + + @Deployment() + public static WebArchive createDeployment() throws Exception { + jwksServer = new JwksServer(); + jwksServer.start(); + System.setProperty("mp.jwt.verify.publickey.location", "http://localhost:" + jwksServer.getPort() + "/jwks.json"); Review Comment: where is it cleaned? tip: inject it in the war as a mp properties file side note: true for all tests ########## src/test/java/org/apache/geronimo/microprofile/impl/jwtauth/jwt/JwksServer.java: ########## @@ -0,0 +1,56 @@ +package org.apache.geronimo.microprofile.impl.jwtauth.jwt; + +import java.io.BufferedReader; +import java.io.IOException; +import java.io.InputStreamReader; +import java.io.PrintWriter; +import java.net.ServerSocket; +import java.net.Socket; + +class JwksServer { Review Comment: why not `HttpServer`? - not a blocker more curious ########## src/main/java/org/apache/geronimo/microprofile/impl/jwtauth/jwt/KidMapper.java: ########## @@ -120,7 +181,44 @@ private String tryLoad(final String value) { throw new IllegalArgumentException(e); } - // else direct value + // load jwks via url + if (jwksUrl != null) { + if(reloadJwksRequest != null) { + try { + reloadJwksRequest.get(); + } catch (InterruptedException | ExecutionException e) { + throw new IllegalStateException(e); + } + } + String key = keyMapping.get(value); + if (key != null) { + return key; + } + + } return value; } + + private List<JWK> parseKeys(HttpResponse<String> keyResponse) { + StringReader stringReader = new StringReader(keyResponse.body()); Review Comment: > try (final JsonReader reader = readerfactory.createReader(new StringReader(keyResponse.body()))) {...} otherwise you leak memory (jsonp) ########## src/main/java/org/apache/geronimo/microprofile/impl/jwtauth/jwt/KidMapper.java: ########## @@ -79,9 +101,48 @@ private void init() { .collect(Collectors.toSet())) .orElseGet(HashSet::new); ofNullable(config.read("issuer.default", config.read(Names.ISSUER, null))).ifPresent(defaultIssuers::add); + jwksUrl = config.read("mp.jwt.verify.publickey.location", null); + readerFactory = Json.createReaderFactory(emptyMap()); + ofNullable(jwksUrl).ifPresent(url -> { + HttpClient.Builder builder = HttpClient.newBuilder(); + if (getJwksRefreshInterval() != null) { + long secondsRefresh = getJwksRefreshInterval(); + backgroundThread = Executors.newSingleThreadScheduledExecutor(); Review Comment: `ThreadFactory` to name the thread (kill -3 friendly ;)) ########## src/main/java/org/apache/geronimo/microprofile/impl/jwtauth/jwt/KidMapper.java: ########## @@ -120,7 +181,44 @@ private String tryLoad(final String value) { throw new IllegalArgumentException(e); } - // else direct value + // load jwks via url + if (jwksUrl != null) { + if(reloadJwksRequest != null) { + try { + reloadJwksRequest.get(); + } catch (InterruptedException | ExecutionException e) { + throw new IllegalStateException(e); + } + } + String key = keyMapping.get(value); + if (key != null) { + return key; + } + + } return value; } + + private List<JWK> parseKeys(HttpResponse<String> keyResponse) { + StringReader stringReader = new StringReader(keyResponse.body()); + JsonReader jwksReader = readerFactory.createReader(stringReader); + JsonObject keySet = jwksReader.readObject(); + JsonArray keys = keySet.getJsonArray("keys"); + return keys.stream() + .map(JsonValue::asJsonObject) + .map(JWK::new) + .filter(it -> it.getUse() == null || "sig".equals(it.getUse())) + .collect(toList()); + } + + @PreDestroy + private void destroy() { + if (backgroundThread != null) { + backgroundThread.shutdown(); Review Comment: + await (1mn can be hardcoded since it should be almost immediate - one call duration) to try our best to not leak a thread before quitting this method (the class being the thread owner) side note: the cancel should be done before the await ########## src/main/java/org/apache/geronimo/microprofile/impl/jwtauth/jwt/KidMapper.java: ########## @@ -16,43 +16,65 @@ */ package org.apache.geronimo.microprofile.impl.jwtauth.jwt; +import static java.util.Collections.emptyMap; import static java.util.Optional.ofNullable; +import static java.util.concurrent.TimeUnit.SECONDS; import static java.util.stream.Collectors.joining; +import static java.util.stream.Collectors.toList; +import org.apache.geronimo.microprofile.impl.jwtauth.config.GeronimoJwtAuthConfig; +import org.apache.geronimo.microprofile.impl.jwtauth.io.PropertiesLoader; +import org.eclipse.microprofile.jwt.config.Names; +import javax.annotation.PostConstruct; +import javax.annotation.PreDestroy; +import javax.enterprise.context.ApplicationScoped; +import javax.inject.Inject; +import javax.json.Json; +import javax.json.JsonArray; +import javax.json.JsonObject; +import javax.json.JsonReader; +import javax.json.JsonReaderFactory; +import javax.json.JsonValue; import java.io.BufferedReader; import java.io.File; import java.io.IOException; import java.io.InputStream; import java.io.InputStreamReader; +import java.io.StringReader; +import java.net.URI; +import java.net.http.HttpClient; +import java.net.http.HttpRequest; +import java.net.http.HttpResponse; import java.nio.file.Files; import java.util.Collection; import java.util.HashMap; import java.util.HashSet; +import java.util.List; import java.util.Map; import java.util.Set; +import java.util.concurrent.CompletableFuture; import java.util.concurrent.ConcurrentHashMap; import java.util.concurrent.ConcurrentMap; +import java.util.concurrent.ExecutionException; +import java.util.concurrent.Executors; +import java.util.concurrent.ScheduledExecutorService; import java.util.stream.Collectors; import java.util.stream.Stream; -import javax.annotation.PostConstruct; -import javax.enterprise.context.ApplicationScoped; -import javax.inject.Inject; - -import org.apache.geronimo.microprofile.impl.jwtauth.config.GeronimoJwtAuthConfig; -import org.apache.geronimo.microprofile.impl.jwtauth.io.PropertiesLoader; -import org.eclipse.microprofile.jwt.config.Names; - @ApplicationScoped public class KidMapper { @Inject private GeronimoJwtAuthConfig config; - private final ConcurrentMap<String, String> keyMapping = new ConcurrentHashMap<>(); + private ConcurrentMap<String, String> keyMapping = new ConcurrentHashMap<>(); Review Comment: volatile (not critical at all, just want to ensure we thought about it and use it or not deliberatly) ########## src/main/java/org/apache/geronimo/microprofile/impl/jwtauth/jwt/KidMapper.java: ########## @@ -120,7 +181,44 @@ private String tryLoad(final String value) { throw new IllegalArgumentException(e); } - // else direct value + // load jwks via url + if (jwksUrl != null) { + if(reloadJwksRequest != null) { + try { + reloadJwksRequest.get(); + } catch (InterruptedException | ExecutionException e) { Review Comment: split `InterruptedException` and `Thread.currentThread().interrupt()` there ? catch of `ExecutionException` should `throw e.getCause() instanceof RuntimeException ? (RuntimeException) e.getCause() : new IllegalStateException(e.getCause());` probably ########## src/main/java/org/apache/geronimo/microprofile/impl/jwtauth/jwt/KidMapper.java: ########## @@ -120,7 +181,44 @@ private String tryLoad(final String value) { throw new IllegalArgumentException(e); } - // else direct value + // load jwks via url + if (jwksUrl != null) { + if(reloadJwksRequest != null) { + try { + reloadJwksRequest.get(); + } catch (InterruptedException | ExecutionException e) { + throw new IllegalStateException(e); + } + } + String key = keyMapping.get(value); + if (key != null) { + return key; + } + + } return value; } + + private List<JWK> parseKeys(HttpResponse<String> keyResponse) { + StringReader stringReader = new StringReader(keyResponse.body()); + JsonReader jwksReader = readerFactory.createReader(stringReader); + JsonObject keySet = jwksReader.readObject(); + JsonArray keys = keySet.getJsonArray("keys"); + return keys.stream() + .map(JsonValue::asJsonObject) + .map(JWK::new) + .filter(it -> it.getUse() == null || "sig".equals(it.getUse())) Review Comment: tip: filter should be before map generally speaking even if not critical ########## src/main/java/org/apache/geronimo/microprofile/impl/jwtauth/jwt/KidMapper.java: ########## @@ -79,9 +101,48 @@ private void init() { .collect(Collectors.toSet())) .orElseGet(HashSet::new); ofNullable(config.read("issuer.default", config.read(Names.ISSUER, null))).ifPresent(defaultIssuers::add); + jwksUrl = config.read("mp.jwt.verify.publickey.location", null); + readerFactory = Json.createReaderFactory(emptyMap()); + ofNullable(jwksUrl).ifPresent(url -> { + HttpClient.Builder builder = HttpClient.newBuilder(); + if (getJwksRefreshInterval() != null) { + long secondsRefresh = getJwksRefreshInterval(); + backgroundThread = Executors.newSingleThreadScheduledExecutor(); + builder.executor(backgroundThread); + backgroundThread.scheduleAtFixedRate(this::reloadRemoteKeys, getJwksRefreshInterval(), secondsRefresh, SECONDS); + } + httpClient = builder.build(); + reloadJwksRequest = reloadRemoteKeys();// inital load, otherwise the background thread is too slow to start and serve + }); defaultKey = config.read("public-key.default", config.read(Names.VERIFIER_PUBLIC_KEY, null)); } + private Integer getJwksRefreshInterval() { + String interval = config.read("jwks.invalidation.interval",null); + if (interval != null) { + return Integer.parseInt(interval); + } else { + return null; + } + } + + private CompletableFuture<Void> reloadRemoteKeys() { + HttpRequest request = HttpRequest.newBuilder().GET().uri(URI.create(jwksUrl)).header("Accept", "application/json").build(); + CompletableFuture<HttpResponse<String>> httpResponseCompletableFuture = httpClient.sendAsync(request, HttpResponse.BodyHandlers.ofString()); + CompletableFuture<Void> ongoingRequest = httpResponseCompletableFuture.thenApply(result -> { + List<JWK> jwks = parseKeys(result); + ConcurrentHashMap<String, String> newKeys = new ConcurrentHashMap<>(); Review Comment: > keyMapping = parseKeys(result).stream().collect(toMap(JWK::kid, JWK::toPemKey)); side note: should we tolerate kid-less keys if single and use the default id? -- 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: dev-unsubscr...@geronimo.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org