Update PKC JWKS reload settings names. Add tests for JWKS loader task stop.

Author: ebarlas

x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/authc/jwt/JwtRealmSettings.java

@@ -262,25 +262,25 @@ private static Set<Setting.AffixSetting<SecureString>> getSecureSettings() {
 
     public static final Setting.AffixSetting<Boolean> PKC_JWKSET_RELOAD_ENABLED = Setting.affixKeySetting(
         RealmSettings.realmSettingPrefix(TYPE),
-        "pkc_jwkset_reload_enabled",
+        "pkc_jwkset_reload.enabled",
         key -> Setting.boolSetting(key, false, Setting.Property.NodeScope)
     );
 
     public static final Setting.AffixSetting<TimeValue> PKC_JWKSET_RELOAD_FILE_INTERVAL = Setting.affixKeySetting(
         RealmSettings.realmSettingPrefix(TYPE),
-        "pkc_jwkset_reload_file_interval",
+        "pkc_jwkset_reload.file_interval",
         key -> Setting.timeSetting(key, TimeValue.timeValueMinutes(5), TimeValue.timeValueMinutes(5), Setting.Property.NodeScope)
     );
 
     public static final Setting.AffixSetting<TimeValue> PKC_JWKSET_RELOAD_URL_INTERVAL_MIN = Setting.affixKeySetting(
         RealmSettings.realmSettingPrefix(TYPE),
-        "pkc_jwkset_reload_url_interval_min",
+        "pkc_jwkset_reload.url_interval_min",
         key -> Setting.timeSetting(key, TimeValue.timeValueHours(1), TimeValue.timeValueMinutes(5), Setting.Property.NodeScope)
     );
 
     public static final Setting.AffixSetting<TimeValue> PKC_JWKSET_RELOAD_URL_INTERVAL_MAX = Setting.affixKeySetting(
         RealmSettings.realmSettingPrefix(TYPE),
-        "pkc_jwkset_reload_url_interval_max",
+        "pkc_jwkset_reload.url_interval_max",
         key -> Setting.timeSetting(key, TimeValue.timeValueDays(5), TimeValue.timeValueMinutes(5), Setting.Property.NodeScope)
     );
 

x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authc/jwt/JwkSetLoader.java

@@ -239,7 +239,7 @@ void reload(FileChangeWatcher fileWatcher) {
                 return;
             }
             try {
-                if (fileWatcher.changed() == false) {
+                if (fileWatcher.changedSinceLastCall() == false) {
                     logger.debug("No changes detected in PKC JWK set file [{}], aborting", jwkSetPath);
                     return;
                 }
@@ -267,6 +267,7 @@ public void load(ActionListener<byte[]> listener) {
 
         @Override
         public void stop() {
+            closed = true;
             if (task != null) {
                 task.cancel();
             }
@@ -394,7 +395,7 @@ static class FileChangeWatcher implements FileChangesListener {
             this.fileWatcher.addListener(this);
         }
 
-        boolean changed() throws IOException {
+        boolean changedSinceLastCall() throws IOException {
             fileWatcher.checkAndNotify(); // may call onFileInit, onFileChanged
             boolean c = changed;
             changed = false;

x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authc/jwt/JwtUtil.java

@@ -46,6 +46,7 @@
 import org.elasticsearch.common.settings.SecureString;
 import org.elasticsearch.common.settings.SettingsException;
 import org.elasticsearch.common.util.concurrent.ThreadContext;
+import org.elasticsearch.core.Nullable;
 import org.elasticsearch.env.Environment;
 import org.elasticsearch.xcontent.XContentBuilder;
 import org.elasticsearch.xcontent.XContentFactory;
@@ -498,10 +499,10 @@ private static boolean containsAtLeastTwoDots(SecureString secureString) {
         return false;
     }
 
-    record JwksResponse(byte[] content, Instant expires, Integer maxAgeSeconds) {
+    record JwksResponse(byte[] content, @Nullable Instant expires, @Nullable Integer maxAgeSeconds) {
         private static final Pattern MAX_AGE_PATTERN = Pattern.compile("\\bmax-age\\s*=\\s*(\\d+)\\b", Pattern.CASE_INSENSITIVE);
 
-        JwksResponse(byte[] content, String expires, String cacheControl) {
+        JwksResponse(byte[] content, @Nullable String expires, @Nullable String cacheControl) {
             this(content, parseExpires(expires), parseMaxAge(cacheControl));
         }
 
@@ -510,7 +511,7 @@ record JwksResponse(byte[] content, Instant expires, Integer maxAgeSeconds) {
          * The Expires header follows RFC 7231 format (e.g., "Thu, 01 Jan 2024 00:00:00 GMT").
          * @return the parsed Instant, or null if the header is null or cannot be parsed
          */
-        static Instant parseExpires(String expires) {
+        static Instant parseExpires(@Nullable String expires) {
             if (expires == null) {
                 return null;
             }
@@ -525,10 +526,10 @@ static Instant parseExpires(String expires) {
         }
 
         /**
-         * Parse the Cache-Control header to extract the max-age value.
+         * Parse the Cache-Control header to extract the max-age value defined in RFC 7234.
          * @return the parsed max-age value as Integer, or null if the header is null or cannot be parsed
          */
-        static Integer parseMaxAge(String cacheControl) {
+        static Integer parseMaxAge(@Nullable String cacheControl) {
             if (cacheControl == null) {
                 return null;
             }

x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/authc/jwt/JwkSetLoaderTests.java

@@ -157,13 +157,13 @@ public void testFileChangeWatcher() throws IOException {
         var now = Instant.now();
         int ticks = 0;
         var watcher = new JwkSetLoader.FileChangeWatcher(path);
-        assertThat(watcher.changed(), is(true));
+        assertThat(watcher.changedSinceLastCall(), is(true));
         Files.setLastModifiedTime(path, FileTime.from(now.plusSeconds(++ticks)));
-        assertThat(watcher.changed(), is(true));
-        assertThat(watcher.changed(), is(false));
+        assertThat(watcher.changedSinceLastCall(), is(true));
+        assertThat(watcher.changedSinceLastCall(), is(false));
         Files.setLastModifiedTime(path, FileTime.from(now.plusSeconds(++ticks)));
-        assertThat(watcher.changed(), is(true));
-        assertThat(watcher.changed(), is(false));
+        assertThat(watcher.changedSinceLastCall(), is(true));
+        assertThat(watcher.changedSinceLastCall(), is(false));
     }
 
     public void testFilePkcJwkSetLoader() throws IOException {
@@ -181,7 +181,9 @@ public void testFilePkcJwkSetLoader() throws IOException {
         ThreadPool threadPool = mock(ThreadPool.class);
         CountingCallback callback = new CountingCallback();
 
-        new JwkSetLoader.FilePkcJwkSetLoader(realmConfig, threadPool, path.toString(), callback).start(); // schedules task
+        var loader = new JwkSetLoader.FilePkcJwkSetLoader(realmConfig, threadPool, path.toString(), callback);
+        loader.start(); // schedules task
+
         ArgumentCaptor<Runnable> taskCaptor = ArgumentCaptor.forClass(Runnable.class);
         verify(threadPool, times(1)).scheduleWithFixedDelay(taskCaptor.capture(), any(TimeValue.class), isNull());
 
@@ -198,6 +200,15 @@ public void testFilePkcJwkSetLoader() throws IOException {
         taskCaptor.getValue().run(); // run third time, change detected
         assertThat(callback.content, is(equalTo(goodbye)));
         assertThat(callback.count, is(2));
+
+        loader.stop();
+
+        Files.writeString(path, "too late");
+        Files.setLastModifiedTime(path, FileTime.from(Instant.now().plusSeconds(1))); // ensure mod time changes
+
+        taskCaptor.getValue().run(); // run fourth time, callback not invoked due to closure
+        assertThat(callback.content, is(equalTo(goodbye)));
+        assertThat(callback.count, is(2));
     }
 
     public void testUrlPkcJwkSetLoader() throws IOException {
@@ -209,12 +220,18 @@ public void testUrlPkcJwkSetLoader() throws IOException {
 
         CountingCallback callback = new CountingCallback();
 
-        new JwkSetLoader.UrlPkcJwkSetLoader(realmConfig, threadPool, uri, httpClient, callback).start(); // schedules task
+        var loader = new JwkSetLoader.UrlPkcJwkSetLoader(realmConfig, threadPool, uri, httpClient, callback);
+        loader.start(); // schedules task
 
         int iterations = randomIntBetween(5, 10);
         for (int i = 0; i < iterations; i++) {
             verifySchedulingIteration(callback, threadPool, httpClient, i + 1);
         }
+
+        loader.stop();
+        verify(httpClient, times(1)).close();
+        verifySchedulingIteration(callback, threadPool, httpClient, iterations + 1); // last schedule call, no subsequent reschedule
+        verify(threadPool, never()).schedule(any(Runnable.class), any(TimeValue.class), isNull());
     }
 
     public void testUrlPkcJwkSetLoaderListenerException() throws IOException {
@@ -230,7 +247,7 @@ public void testUrlPkcJwkSetLoaderListenerException() throws IOException {
 
         int iterations = randomIntBetween(5, 10);
         for (int i = 0; i < iterations; i++) {
-            verifySchedulingIterationWithListenerException(threadPool, httpClient, i + 1);
+            verifySchedulingIterationWithListenerException(threadPool, httpClient);
         }
     }
 
@@ -263,16 +280,15 @@ private void verifySchedulingIteration(
         byte[] bytes = "x".repeat(iteration).getBytes(StandardCharsets.UTF_8);
         HttpResponse response = makeHttpResponse(bytes, randomBoolean());
 
-        reset(threadPool);
-        reset(httpClient);
+        reset(threadPool, httpClient);
 
         // invoke response handler
         responseFn.getValue().completed(response);
         assertThat(callback.content, is(equalTo(bytes)));
         assertThat(callback.count, is(iteration));
     }
 
-    private void verifySchedulingIterationWithListenerException(ThreadPool threadPool, CloseableHttpAsyncClient httpClient, int iteration)
+    private void verifySchedulingIterationWithListenerException(ThreadPool threadPool, CloseableHttpAsyncClient httpClient)
         throws IOException {
         // capture scheduled task and delay
         ArgumentCaptor<Runnable> taskCaptor = ArgumentCaptor.forClass(Runnable.class);
@@ -289,8 +305,7 @@ private void verifySchedulingIterationWithListenerException(ThreadPool threadPoo
         verify(httpClient, times(1)).execute(any(HttpGet.class), responseFn.capture());
         HttpResponse response = makeHttpResponse(new byte[0], randomBoolean());
 
-        reset(threadPool);
-        reset(httpClient);
+        reset(threadPool, httpClient);
 
         // invoke response handler
         responseFn.getValue().completed(response);