diff --git a/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authz/store/CompositeRolesStore.java b/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authz/store/CompositeRolesStore.java index beb2ca60fb2ae..7e1cc49e2c0bc 100644 --- a/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authz/store/CompositeRolesStore.java +++ b/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authz/store/CompositeRolesStore.java @@ -97,9 +97,7 @@ public CompositeRolesStore(Settings settings, FileRolesStore fileRolesStore, Nat ThreadContext threadContext, XPackLicenseState licenseState) { super(settings); this.fileRolesStore = fileRolesStore; - // invalidating all on a file based role update is heavy handed to say the least, but in general this should be infrequent so the - // impact isn't really worth the added complexity of only clearing the changed values - fileRolesStore.addListener(this::invalidateAll); + fileRolesStore.addListener(this::invalidate); this.nativeRolesStore = nativeRolesStore; this.reservedRolesStore = reservedRolesStore; this.privilegeStore = privilegeStore; @@ -356,6 +354,23 @@ public void invalidate(String role) { negativeLookupCache.remove(role); } + public void invalidate(Set roles) { + numInvalidation.incrementAndGet(); + + // the cache cannot be modified while doing this operation per the terms of the cache iterator + try (ReleasableLock ignored = writeLock.acquire()) { + Iterator> keyIter = roleCache.keys().iterator(); + while (keyIter.hasNext()) { + Set key = keyIter.next(); + if (Sets.haveEmptyIntersection(key, roles) == false) { + keyIter.remove(); + } + } + } + + negativeLookupCache.removeAll(roles); + } + public void usageStats(ActionListener> listener) { final Map usage = new HashMap<>(2); usage.put("file", fileRolesStore.usageStats()); diff --git a/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authz/store/FileRolesStore.java b/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authz/store/FileRolesStore.java index 59bc8042fbaff..868a7076b8b1f 100644 --- a/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authz/store/FileRolesStore.java +++ b/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authz/store/FileRolesStore.java @@ -12,6 +12,7 @@ import org.elasticsearch.common.Nullable; import org.elasticsearch.common.component.AbstractComponent; import org.elasticsearch.common.settings.Settings; +import org.elasticsearch.common.util.set.Sets; import org.elasticsearch.common.xcontent.LoggingDeprecationHandler; import org.elasticsearch.common.xcontent.NamedXContentRegistry; import org.elasticsearch.common.xcontent.XContentParser; @@ -34,13 +35,16 @@ import java.nio.file.Files; import java.nio.file.Path; import java.util.ArrayList; +import java.util.Collections; import java.util.HashMap; import java.util.HashSet; import java.util.List; import java.util.Map; import java.util.Objects; import java.util.Set; +import java.util.function.Consumer; import java.util.regex.Pattern; +import java.util.stream.Collectors; import static java.util.Collections.emptyMap; import static java.util.Collections.unmodifiableMap; @@ -52,16 +56,16 @@ public class FileRolesStore extends AbstractComponent { private final Path file; private final XPackLicenseState licenseState; - private final List listeners = new ArrayList<>(); + private final List>> listeners = new ArrayList<>(); private volatile Map permissions; public FileRolesStore(Settings settings, Environment env, ResourceWatcherService watcherService, XPackLicenseState licenseState) throws IOException { - this(settings, env, watcherService, () -> {}, licenseState); + this(settings, env, watcherService, null, licenseState); } - FileRolesStore(Settings settings, Environment env, ResourceWatcherService watcherService, Runnable listener, + FileRolesStore(Settings settings, Environment env, ResourceWatcherService watcherService, Consumer> listener, XPackLicenseState licenseState) throws IOException { super(settings); this.file = resolveFile(env); @@ -76,9 +80,10 @@ public FileRolesStore(Settings settings, Environment env, ResourceWatcherService } public Set roleDescriptors(Set roleNames) { + final Map localPermissions = permissions; Set descriptors = new HashSet<>(); roleNames.forEach((name) -> { - RoleDescriptor descriptor = permissions.get(name); + RoleDescriptor descriptor = localPermissions.get(name); if (descriptor != null) { descriptors.add(descriptor); } @@ -87,12 +92,13 @@ public Set roleDescriptors(Set roleNames) { } public Map usageStats() { + final Map localPermissions = permissions; Map usageStats = new HashMap<>(3); - usageStats.put("size", permissions.size()); + usageStats.put("size", localPermissions.size()); boolean dls = false; boolean fls = false; - for (RoleDescriptor descriptor : permissions.values()) { + for (RoleDescriptor descriptor : localPermissions.values()) { for (IndicesPrivileges indicesPrivileges : descriptor.getIndicesPrivileges()) { fls = fls || indicesPrivileges.getGrantedFields() != null || indicesPrivileges.getDeniedFields() != null; dls = dls || indicesPrivileges.getQuery() != null; @@ -107,10 +113,10 @@ public Map usageStats() { return usageStats; } - public void addListener(Runnable runnable) { - Objects.requireNonNull(runnable); + public void addListener(Consumer> consumer) { + Objects.requireNonNull(consumer); synchronized (this) { - listeners.add(runnable); + listeners.add(consumer); } } @@ -118,6 +124,11 @@ public Path getFile() { return file; } + // package private for testing + Set getAllRoleNames() { + return permissions.keySet(); + } + public static Path resolveFile(Environment env) { return XPackPlugin.resolveConfigFile(env, "roles.yml"); } @@ -319,11 +330,13 @@ public void onFileDeleted(Path file) { } @Override - public void onFileChanged(Path file) { + public synchronized void onFileChanged(Path file) { if (file.equals(FileRolesStore.this.file)) { + final Map previousPermissions = permissions; try { permissions = parseFile(file, logger, settings, licenseState); - logger.info("updated roles (roles file [{}] {})", file.toAbsolutePath(), Files.exists(file) ? "changed" : "removed"); + logger.info("updated roles (roles file [{}] {})", file.toAbsolutePath(), + Files.exists(file) ? "changed" : "removed"); } catch (Exception e) { logger.error( (Supplier) () -> new ParameterizedMessage( @@ -331,9 +344,13 @@ public void onFileChanged(Path file) { return; } - synchronized (FileRolesStore.this) { - listeners.forEach(Runnable::run); - } + final Set changedOrMissingRoles = Sets.difference(previousPermissions.entrySet(), permissions.entrySet()) + .stream() + .map(Map.Entry::getKey) + .collect(Collectors.toSet()); + final Set addedRoles = Sets.difference(permissions.keySet(), previousPermissions.keySet()); + final Set changedRoles = Collections.unmodifiableSet(Sets.union(changedOrMissingRoles, addedRoles)); + listeners.forEach(c -> c.accept(changedRoles)); } } } diff --git a/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/authz/store/CompositeRolesStoreTests.java b/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/authz/store/CompositeRolesStoreTests.java index 0c2ab1ecc7650..9f1490856d67b 100644 --- a/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/authz/store/CompositeRolesStoreTests.java +++ b/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/authz/store/CompositeRolesStoreTests.java @@ -53,6 +53,7 @@ import java.util.concurrent.ExecutionException; import java.util.concurrent.atomic.AtomicInteger; import java.util.function.BiConsumer; +import java.util.function.Consumer; import java.util.function.Function; import java.util.function.Predicate; @@ -213,7 +214,7 @@ public void testNegativeLookupsAreCached() { new CompositeRolesStore(SECURITY_ENABLED_SETTINGS, fileRolesStore, nativeRolesStore, reservedRolesStore, mock(NativePrivilegeStore.class), Collections.emptyList(), new ThreadContext(SECURITY_ENABLED_SETTINGS), new XPackLicenseState(SECURITY_ENABLED_SETTINGS)); - verify(fileRolesStore).addListener(any(Runnable.class)); // adds a listener in ctor + verify(fileRolesStore).addListener(any(Consumer.class)); // adds a listener in ctor final String roleName = randomAlphaOfLengthBetween(1, 10); PlainActionFuture future = new PlainActionFuture<>(); diff --git a/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/authz/store/FileRolesStoreTests.java b/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/authz/store/FileRolesStoreTests.java index 5cb93b898ba52..0763ff65ec562 100644 --- a/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/authz/store/FileRolesStoreTests.java +++ b/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/authz/store/FileRolesStoreTests.java @@ -37,6 +37,7 @@ import java.nio.file.Path; import java.nio.file.StandardOpenOption; import java.util.Collections; +import java.util.HashSet; import java.util.List; import java.util.Map; import java.util.Set; @@ -319,8 +320,11 @@ public void testAutoReload() throws Exception { threadPool = new TestThreadPool("test"); watcherService = new ResourceWatcherService(settings, threadPool); final CountDownLatch latch = new CountDownLatch(1); - FileRolesStore store = new FileRolesStore(settings, env, watcherService, latch::countDown, - new XPackLicenseState(Settings.EMPTY)); + final Set modifiedRoles = new HashSet<>(); + FileRolesStore store = new FileRolesStore(settings, env, watcherService, roleSet -> { + modifiedRoles.addAll(roleSet); + latch.countDown(); + }, new XPackLicenseState(Settings.EMPTY)); Set descriptors = store.roleDescriptors(Collections.singleton("role1")); assertThat(descriptors, notNullValue()); @@ -344,6 +348,8 @@ public void testAutoReload() throws Exception { fail("Waited too long for the updated file to be picked up"); } + assertEquals(1, modifiedRoles.size()); + assertTrue(modifiedRoles.contains("role5")); final TransportRequest request = mock(TransportRequest.class); descriptors = store.roleDescriptors(Collections.singleton("role5")); assertThat(descriptors, notNullValue()); @@ -354,6 +360,49 @@ public void testAutoReload() throws Exception { assertThat(role.cluster().check("cluster:monitor/foo/bar", request), is(true)); assertThat(role.cluster().check("cluster:admin/foo/bar", request), is(false)); + // truncate to remove some + final Set truncatedFileRolesModified = new HashSet<>(); + final CountDownLatch truncateLatch = new CountDownLatch(1); + store = new FileRolesStore(settings, env, watcherService, roleSet -> { + truncatedFileRolesModified.addAll(roleSet); + truncateLatch.countDown(); + }, new XPackLicenseState(Settings.EMPTY)); + + final Set allRolesPreTruncate = store.getAllRoleNames(); + try (BufferedWriter writer = Files.newBufferedWriter(tmp, StandardCharsets.UTF_8, StandardOpenOption.TRUNCATE_EXISTING)) { + writer.append("role5:").append(System.lineSeparator()); + writer.append(" cluster:").append(System.lineSeparator()); + writer.append(" - 'MONITOR'"); + } + + truncateLatch.await(); + assertEquals(allRolesPreTruncate.size() - 1, truncatedFileRolesModified.size()); + assertTrue(allRolesPreTruncate.contains("role5")); + assertFalse(truncatedFileRolesModified.contains("role5")); + descriptors = store.roleDescriptors(Collections.singleton("role5")); + assertThat(descriptors, notNullValue()); + assertEquals(1, descriptors.size()); + + // modify + final Set modifiedFileRolesModified = new HashSet<>(); + final CountDownLatch modifyLatch = new CountDownLatch(1); + store = new FileRolesStore(settings, env, watcherService, roleSet -> { + modifiedFileRolesModified.addAll(roleSet); + modifyLatch.countDown(); + }, new XPackLicenseState(Settings.EMPTY)); + + try (BufferedWriter writer = Files.newBufferedWriter(tmp, StandardCharsets.UTF_8, StandardOpenOption.TRUNCATE_EXISTING)) { + writer.append("role5:").append(System.lineSeparator()); + writer.append(" cluster:").append(System.lineSeparator()); + writer.append(" - 'ALL'"); + } + + modifyLatch.await(); + assertEquals(1, modifiedFileRolesModified.size()); + assertTrue(modifiedFileRolesModified.contains("role5")); + descriptors = store.roleDescriptors(Collections.singleton("role5")); + assertThat(descriptors, notNullValue()); + assertEquals(1, descriptors.size()); } finally { if (watcherService != null) { watcherService.stop();