Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Calculate changed roles on roles.yml reload #33525

Merged
merged 2 commits into from
Sep 26, 2018
Merged

Calculate changed roles on roles.yml reload #33525

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
37c47b5
Calculate changed roles on roles.yml reload
jaymode Sep 7, 2018
9bb1e8e
Merge branch 'master' into file_reload_cache_invalidate
jaymode Sep 25, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 18 additions & 3 deletions ...urity/src/main/java/org/elasticsearch/xpack/security/authz/store/CompositeRolesStore.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -356,6 +354,23 @@ public void invalidate(String role) {
negativeLookupCache.remove(role);
}

public void invalidate(Set<String> 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<Set<String>> keyIter = roleCache.keys().iterator();
while (keyIter.hasNext()) {
Set<String> key = keyIter.next();
if (Sets.haveEmptyIntersection(key, roles) == false) {
keyIter.remove();
}
}
}

negativeLookupCache.removeAll(roles);
}

public void usageStats(ActionListener<Map<String, Object>> listener) {
final Map<String, Object> usage = new HashMap<>(2);
usage.put("file", fileRolesStore.usageStats());
Expand Down
45 changes: 31 additions & 14 deletions ...n/security/src/main/java/org/elasticsearch/xpack/security/authz/store/FileRolesStore.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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;
Expand All @@ -52,16 +56,16 @@ public class FileRolesStore extends AbstractComponent {

private final Path file;
private final XPackLicenseState licenseState;
private final List<Runnable> listeners = new ArrayList<>();
private final List<Consumer<Set<String>>> listeners = new ArrayList<>();

private volatile Map<String, RoleDescriptor> 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<Set<String>> listener,
XPackLicenseState licenseState) throws IOException {
super(settings);
this.file = resolveFile(env);
Expand All @@ -76,9 +80,10 @@ public FileRolesStore(Settings settings, Environment env, ResourceWatcherService
}

public Set<RoleDescriptor> roleDescriptors(Set<String> roleNames) {
final Map<String, RoleDescriptor> localPermissions = permissions;
Set<RoleDescriptor> descriptors = new HashSet<>();
roleNames.forEach((name) -> {
RoleDescriptor descriptor = permissions.get(name);
RoleDescriptor descriptor = localPermissions.get(name);
if (descriptor != null) {
descriptors.add(descriptor);
}
Expand All @@ -87,12 +92,13 @@ public Set<RoleDescriptor> roleDescriptors(Set<String> roleNames) {
}

public Map<String, Object> usageStats() {
final Map<String, RoleDescriptor> localPermissions = permissions;
Map<String, Object> 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;
Expand All @@ -107,17 +113,22 @@ public Map<String, Object> usageStats() {
return usageStats;
}

public void addListener(Runnable runnable) {
Objects.requireNonNull(runnable);
public void addListener(Consumer<Set<String>> consumer) {
Objects.requireNonNull(consumer);
synchronized (this) {
listeners.add(runnable);
listeners.add(consumer);
}
}

public Path getFile() {
return file;
}

// package private for testing
Set<String> getAllRoleNames() {
return permissions.keySet();
}

public static Path resolveFile(Environment env) {
return XPackPlugin.resolveConfigFile(env, "roles.yml");
}
Expand Down Expand Up @@ -319,21 +330,27 @@ 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<String, RoleDescriptor> 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(
"could not reload roles file [{}]. Current roles remain unmodified", file.toAbsolutePath()), e);
return;
}

synchronized (FileRolesStore.this) {
listeners.forEach(Runnable::run);
}
final Set<String> changedOrMissingRoles = Sets.difference(previousPermissions.entrySet(), permissions.entrySet())
.stream()
.map(Map.Entry::getKey)
.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));
}
}
}
Expand Down
3 changes: 2 additions & 1 deletion .../src/test/java/org/elasticsearch/xpack/security/authz/store/CompositeRolesStoreTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;

Expand Down Expand Up @@ -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<Role> future = new PlainActionFuture<>();
Expand Down
53 changes: 51 additions & 2 deletions ...urity/src/test/java/org/elasticsearch/xpack/security/authz/store/FileRolesStoreTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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<String> modifiedRoles = new HashSet<>();
FileRolesStore store = new FileRolesStore(settings, env, watcherService, roleSet -> {
modifiedRoles.addAll(roleSet);
latch.countDown();
}, new XPackLicenseState(Settings.EMPTY));

Set<RoleDescriptor> descriptors = store.roleDescriptors(Collections.singleton("role1"));
assertThat(descriptors, notNullValue());
Expand All @@ -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());
Expand All @@ -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<String> 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<String> 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<String> 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();
Expand Down