Security should not reload files that haven't changed (elastic#50207)

In security we currently monitor a set of files for changes:

- config/role_mapping.yml (or alternative configured path)
- config/roles.yml
- config/users
- config/users_roles

This commit prevents unnecessary reloading when the file change actually doesn't change the internal structure.

Closes: elastic#50063

Co-authored-by: Anton Shuvaev <anton.shuvaev91@gmail.com>

server/src/main/java/org/elasticsearch/common/util/Maps.java

@@ -99,4 +99,23 @@ public static <K, V> Map<K, V> ofEntries(final Collection<Map.Entry<K, V>> entri
         return map;
     }
 
+    /**
+     * Returns {@code true} if the two specified maps are equal to one another. Two maps are considered equal if both represent identical
+     * mappings where values are checked with Objects.deepEquals. The primary use case is to check if two maps with array values are equal.
+     *
+     * @param left  one map to be tested for equality
+     * @param right the other map to be tested for equality
+     * @return {@code true} if the two maps are equal
+     */
+    public static <K, V> boolean deepEquals(Map<K, V> left, Map<K, V> right) {
+        if (left == right) {
+            return true;
+        }
+        if (left == null || right == null || left.size() != right.size()) {
+            return false;
+        }
+        return left.entrySet().stream()
+                .allMatch(e -> right.containsKey(e.getKey()) && Objects.deepEquals(e.getValue(), right.get(e.getKey())));
+    }
+
 }

server/src/test/java/org/elasticsearch/common/util/MapsTests.java

@@ -22,13 +22,18 @@
 import org.elasticsearch.test.ESTestCase;
 
 import java.util.ArrayList;
+import java.util.Arrays;
 import java.util.Collection;
+import java.util.HashMap;
 import java.util.HashSet;
 import java.util.Map;
+import java.util.function.Supplier;
 import java.util.stream.Collectors;
+import java.util.stream.IntStream;
 import java.util.stream.Stream;
 
 import static java.util.Map.entry;
+import static java.util.stream.Collectors.toMap;
 import static org.hamcrest.Matchers.equalTo;
 import static org.hamcrest.Matchers.hasItem;
 
@@ -103,6 +108,31 @@ public void testOfEntries() {
         assertMapEntriesAndImmutability(map, entries);
     }
 
+    public void testDeepEquals() {
+        final Supplier<String> keyGenerator = () -> randomAlphaOfLengthBetween(1, 5);
+        final Supplier<int[]> arrayValueGenerator = () -> random().ints(randomInt(5)).toArray();
+        final Map<String, int[]> map = randomMap(randomInt(5), keyGenerator, arrayValueGenerator);
+        final Map<String, int[]> mapCopy = map.entrySet().stream()
+                .collect(toMap(Map.Entry::getKey, e -> Arrays.copyOf(e.getValue(), e.getValue().length)));
+
+        assertTrue(Maps.deepEquals(map, mapCopy));
+
+        final Map<String, int[]> mapModified = mapCopy;
+        if (mapModified.isEmpty()) {
+            mapModified.put(keyGenerator.get(), arrayValueGenerator.get());
+        } else {
+            if (randomBoolean()) {
+                final String randomKey = mapModified.keySet().toArray(new String[0])[randomInt(mapModified.size() - 1)];
+                final int[] value = mapModified.get(randomKey);
+                mapModified.put(randomKey, randomValueOtherThanMany((v) -> Arrays.equals(v, value), arrayValueGenerator));
+            } else {
+                mapModified.put(randomValueOtherThanMany(mapModified::containsKey, keyGenerator), arrayValueGenerator.get());
+            }
+        }
+
+        assertFalse(Maps.deepEquals(map, mapModified));
+    }
+
     private void assertMapEntries(final Map<String, String> map, final Collection<Map.Entry<String, String>> entries) {
         for (var entry : entries) {
             assertThat("map [" + map + "] does not contain key [" + entry.getKey() + "]", map.keySet(), hasItem(entry.getKey()));
@@ -160,4 +190,10 @@ private void assertMapEntriesAndImmutability(
         assertMapImmutability(map);
     }
 
+    private static <K, V> Map<K, V> randomMap(int size, Supplier<K> keyGenerator, Supplier<V> valueGenerator) {
+        final Map<K, V> map = new HashMap<>();
+        IntStream.range(0, size).forEach(i -> map.put(keyGenerator.get(), valueGenerator.get()));
+        return map;
+    }
+
 }

x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authc/file/FileUserPasswdStore.java

@@ -5,14 +5,15 @@
  */
 package org.elasticsearch.xpack.security.authc.file;
 
-import org.apache.logging.log4j.Logger;
 import org.apache.logging.log4j.LogManager;
+import org.apache.logging.log4j.Logger;
 import org.apache.logging.log4j.message.ParameterizedMessage;
 import org.apache.logging.log4j.util.Supplier;
 import org.elasticsearch.ElasticsearchException;
 import org.elasticsearch.common.Nullable;
 import org.elasticsearch.common.settings.SecureString;
 import org.elasticsearch.common.settings.Settings;
+import org.elasticsearch.common.util.Maps;
 import org.elasticsearch.env.Environment;
 import org.elasticsearch.watcher.FileChangesListener;
 import org.elasticsearch.watcher.FileWatcher;
@@ -191,9 +192,13 @@ public void onFileDeleted(Path file) {
         @Override
         public void onFileChanged(Path file) {
             if (file.equals(FileUserPasswdStore.this.file)) {
-                logger.info("users file [{}] changed. updating users... )", file.toAbsolutePath());
+                final Map<String, char[]> previousUsers = users;
                 users = parseFileLenient(file, logger, settings);
-                notifyRefresh();
+
+                if (Maps.deepEquals(previousUsers, users) == false) {
+                    logger.info("users file [{}] changed. updating users... )", file.toAbsolutePath());
+                    notifyRefresh();
+                }
             }
         }
     }

x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authc/file/FileUserRolesStore.java

@@ -5,13 +5,14 @@
  */
 package org.elasticsearch.xpack.security.authc.file;
 
-import org.apache.logging.log4j.Logger;
 import org.apache.logging.log4j.LogManager;
+import org.apache.logging.log4j.Logger;
 import org.apache.logging.log4j.message.ParameterizedMessage;
 import org.apache.logging.log4j.util.Supplier;
 import org.elasticsearch.ElasticsearchException;
 import org.elasticsearch.common.Nullable;
 import org.elasticsearch.common.Strings;
+import org.elasticsearch.common.util.Maps;
 import org.elasticsearch.env.Environment;
 import org.elasticsearch.watcher.FileChangesListener;
 import org.elasticsearch.watcher.FileWatcher;
@@ -206,9 +207,13 @@ public void onFileDeleted(Path file) {
         @Override
         public void onFileChanged(Path file) {
             if (file.equals(FileUserRolesStore.this.file)) {
-                logger.info("users roles file [{}] changed. updating users roles...", file.toAbsolutePath());
+                final Map<String, String[]> previousUserRoles = userRoles;
                 userRoles = parseFileLenient(file, logger);
-                notifyRefresh();
+
+                if (Maps.deepEquals(previousUserRoles, userRoles) == false) {
+                    logger.info("users roles file [{}] changed. updating users roles...", file.toAbsolutePath());
+                    notifyRefresh();
+                }
             }
         }
     }

x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authc/support/DnRoleMapper.java

@@ -223,10 +223,14 @@ public void onFileDeleted(Path file) {
         @Override
         public void onFileChanged(Path file) {
             if (file.equals(DnRoleMapper.this.file)) {
-                logger.info("role mappings file [{}] changed for realm [{}/{}]. updating mappings...", file.toAbsolutePath(),
-                        config.type(), config.name());
+                final Map<String, List<String>> previousDnRoles = dnRoles;
                 dnRoles = parseFileLenient(file, logger, config.type(), config.name());
-                notifyRefresh();
+
+                if (previousDnRoles.equals(dnRoles) == false) {
+                    logger.info("role mappings file [{}] changed for realm [{}/{}]. updating mappings...", file.toAbsolutePath(),
+                            config.type(), config.name());
+                    notifyRefresh();
+                }
             }
         }
     }

x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authz/store/FileRolesStore.java

@@ -368,8 +368,6 @@ public synchronized void onFileChanged(Path file) {
                 final Map<String, RoleDescriptor> previousPermissions = permissions;
                 try {
                     permissions = parseFile(file, logger, settings, licenseState, xContentRegistry);
-                    logger.info("updated roles (roles file [{}] {})", file.toAbsolutePath(),
-                        Files.exists(file) ? "changed" : "removed");
                 } catch (Exception e) {
                     logger.error(
                             (Supplier<?>) () -> new ParameterizedMessage(
@@ -383,7 +381,11 @@ public synchronized void onFileChanged(Path file) {
                         .collect(Collectors.toSet());
                 final Set<String> addedRoles = Sets.difference(permissions.keySet(), previousPermissions.keySet());
                 final Set<String> changedRoles = Collections.unmodifiableSet(Sets.union(changedOrMissingRoles, addedRoles));
-                listeners.forEach(c -> c.accept(changedRoles));
+                if (changedRoles.isEmpty() == false) {
+                    logger.info("updated roles (roles file [{}] {})", file.toAbsolutePath(),
+                            Files.exists(file) ? "changed" : "removed");
+                    listeners.forEach(c -> c.accept(changedRoles));
+                }
             }
         }
     }

x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/authc/file/FileUserPasswdStoreTests.java

@@ -53,7 +53,7 @@ public class FileUserPasswdStoreTests extends ESTestCase {
     @Before
     public void init() {
         settings = Settings.builder()
-            .put("resource.reload.interval.high", "2s")
+            .put("resource.reload.interval.high", "100ms")
             .put("path.home", createTempDir())
             .put("xpack.security.authc.password_hashing.algorithm", randomFrom("bcrypt", "bcrypt11", "pbkdf2", "pbkdf2_1000",
                 "pbkdf2_50000"))
@@ -103,6 +103,20 @@ public void testStore_AutoReload() throws Exception {
 
         watcherService.start();
 
+        try (BufferedWriter writer = Files.newBufferedWriter(file, StandardCharsets.UTF_8, StandardOpenOption.APPEND)) {
+            writer.append("\n");
+        }
+
+        watcherService.notifyNow(ResourceWatcherService.Frequency.HIGH);
+        if (latch.getCount() != 1) {
+            fail("Listener should not be called as users passwords are not changed.");
+        }
+
+        assertThat(store.userExists(username), is(true));
+        result = store.verifyPassword(username, new SecureString("test123"), () -> user);
+        assertThat(result.getStatus(), is(AuthenticationResult.Status.SUCCESS));
+        assertThat(result.getUser(), is(user));
+
         try (BufferedWriter writer = Files.newBufferedWriter(file, StandardCharsets.UTF_8, StandardOpenOption.APPEND)) {
             writer.newLine();
             writer.append("foobar:").append(new String(hasher.hash(new SecureString("barfoo"))));

x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/authc/file/FileUserRolesStoreTests.java

@@ -54,7 +54,7 @@ public class FileUserRolesStoreTests extends ESTestCase {
     @Before
     public void init() {
         settings = Settings.builder()
-                .put("resource.reload.interval.high", "2s")
+                .put("resource.reload.interval.high", "100ms")
                 .put("path.home", createTempDir())
                 .build();
         env = TestEnvironment.newEnvironment(settings);
@@ -102,6 +102,17 @@ public void testStoreAutoReload() throws Exception {
 
         watcherService.start();
 
+        try (BufferedWriter writer = Files.newBufferedWriter(tmp, StandardCharsets.UTF_8, StandardOpenOption.APPEND)) {
+            writer.append("\n");
+        }
+
+        watcherService.notifyNow(ResourceWatcherService.Frequency.HIGH);
+        if (latch.getCount() != 1) {
+            fail("Listener should not be called as users roles are not changed.");
+        }
+
+        assertThat(store.roles("user1"), arrayContaining("role1", "role2", "role3"));
+
         try (BufferedWriter writer = Files.newBufferedWriter(tmp, StandardCharsets.UTF_8, StandardOpenOption.APPEND)) {
             writer.newLine();
             writer.append("role4:user4\nrole5:user4\n");

x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/authc/support/DnRoleMapperTests.java

@@ -112,6 +112,18 @@ public void testMapper_AutoReload() throws Exception {
 
         watcherService.start();
 
+        try (BufferedWriter writer = Files.newBufferedWriter(file, StandardCharsets.UTF_8, StandardOpenOption.APPEND)) {
+            writer.append("\n");
+        }
+
+        watcherService.notifyNow(ResourceWatcherService.Frequency.HIGH);
+        if (latch.getCount() != 1) {
+            fail("Listener should not be called as roles mapping is not changed.");
+        }
+
+        roles = mapper.resolveRoles("", Collections.singletonList("cn=shield,ou=marvel,o=superheros"));
+        assertThat(roles, contains("security"));
+
         try (BufferedWriter writer = Files.newBufferedWriter(file, StandardCharsets.UTF_8, StandardOpenOption.APPEND)) {
             writer.newLine();
             writer.append("fantastic_four:\n")

x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/authz/store/FileRolesStoreTests.java

@@ -332,7 +332,7 @@ public void testAutoReload() throws Exception {
             }
 
             Settings.Builder builder = Settings.builder()
-                    .put("resource.reload.interval.high", "500ms")
+                    .put("resource.reload.interval.high", "100ms")
                     .put("path.home", home);
             Settings settings = builder.build();
             Environment env = TestEnvironment.newEnvironment(settings);
@@ -354,6 +354,19 @@ public void testAutoReload() throws Exception {
 
             watcherService.start();
 
+            try (BufferedWriter writer = Files.newBufferedWriter(tmp, StandardCharsets.UTF_8, StandardOpenOption.APPEND)) {
+                writer.append("\n");
+            }
+
+            watcherService.notifyNow(ResourceWatcherService.Frequency.HIGH);
+            if (latch.getCount() != 1) {
+                fail("Listener should not be called as roles are not changed.");
+            }
+
+            descriptors = store.roleDescriptors(Collections.singleton("role1"));
+            assertThat(descriptors, notNullValue());
+            assertEquals(1, descriptors.size());
+
             try (BufferedWriter writer = Files.newBufferedWriter(tmp, StandardCharsets.UTF_8, StandardOpenOption.APPEND)) {
                 writer.newLine();
                 writer.newLine();