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

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

Merged
merged 4 commits into from
Jan 6, 2020
Merged

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

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
69b0f4d
Security should not reload files that haven't changed
AntonShuvaev Dec 15, 2019
c37b64f
Fix review issues
AntonShuvaev Dec 16, 2019
50895b0
Fix unused imports
AntonShuvaev Dec 16, 2019
e3eb627
Merge branch 'master' into not_reload_files_if_not_changed
elasticmachine Jan 6, 2020
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
22 changes: 22 additions & 0 deletions server/src/main/java/org/elasticsearch/common/util/Maps.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@
import java.util.Collection;
import java.util.Map;
import java.util.Objects;
import java.util.function.BiPredicate;
import java.util.stream.Collectors;
import java.util.stream.Stream;

Expand Down Expand Up @@ -99,4 +100,25 @@ 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 contain
* the same number of entries, and all values ​​of the corresponding keys are equal by the specified equivalence relationship
* 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
* @param valueEquivalence the equivalence relationship for comparing values
* @return {@code true} if the two maps are equal
*/
public static <K, V> boolean equals(Map<K, V> left, Map<K, V> right, BiPredicate<V, V> valueEquivalence) {
AntonShuvaev marked this conversation as resolved.
Show resolved Hide resolved
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()) && valueEquivalence.test(e.getValue(), right.get(e.getKey())));
}

}
57 changes: 57 additions & 0 deletions server/src/test/java/org/elasticsearch/common/util/MapsTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;

Expand Down Expand Up @@ -103,6 +108,52 @@ public void testOfEntries() {
assertMapEntriesAndImmutability(map, entries);
}

public void testEqualsStringValue() {
Supplier<String> keyGenerator = () -> randomAlphaOfLengthBetween(1, 5);
Supplier<String> stringValueGenerator = () -> randomAlphaOfLengthBetween(1, 5);
Map<String, String> map = randomMap(randomInt(5), keyGenerator, stringValueGenerator);
Map<String, String> mapCopy = new HashMap<>(map);
Map<String, String> mapModified = new HashMap<>(map);
if (mapModified.isEmpty()) {
mapModified.put(keyGenerator.get(), stringValueGenerator.get());
} else {
if (randomBoolean()) {
String randomKey = mapModified.keySet().toArray(new String[0])[randomInt(mapModified.size() - 1)];
mapModified.put(randomKey, randomValueOtherThan(mapModified.get(randomKey), stringValueGenerator));
} else {
mapModified.put(randomValueOtherThanMany(mapModified::containsKey, keyGenerator), stringValueGenerator.get());
}
}

assertFalse(Maps.equals(map, mapModified, String::equals));
assertTrue(Maps.equals(map, mapCopy, String::equals));
}

public void testEqualsArrayValue() {
Supplier<String> keyGenerator = () -> randomAlphaOfLengthBetween(1, 5);
Supplier<int[]> arrayValueGenerator = () -> random().ints(randomInt(5)).toArray();
Map<String, int[]> map = randomMap(randomInt(5), keyGenerator, arrayValueGenerator);
Map<String, int[]> mapCopy = map.entrySet().stream()
.collect(toMap(Map.Entry::getKey, e -> Arrays.copyOf(e.getValue(), e.getValue().length)));

assertTrue(Maps.equals(map, mapCopy, Arrays::equals));

Map<String, int[]> mapModified = mapCopy;
if (mapModified.isEmpty()) {
mapModified.put(keyGenerator.get(), arrayValueGenerator.get());
} else {
if (randomBoolean()) {
String randomKey = mapModified.keySet().toArray(new String[0])[randomInt(mapModified.size() - 1)];
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.equals(map, mapModified, Arrays::equals));
}

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()));
Expand Down Expand Up @@ -160,4 +211,10 @@ private void assertMapEntriesAndImmutability(
assertMapImmutability(map);
}

private static <K, V> Map<K, V> randomMap(int size, Supplier<K> keyGenerator, Supplier<V> valueGenerator) {
Map<K, V> map = new HashMap<>();
IntStream.range(0, size).forEach(i -> map.put(keyGenerator.get(), valueGenerator.get()));
return map;
}

}
12 changes: 9 additions & 3 deletions ...curity/src/main/java/org/elasticsearch/xpack/security/authc/file/FileUserPasswdStore.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -32,6 +33,7 @@
import java.nio.charset.StandardCharsets;
import java.nio.file.Files;
import java.nio.file.Path;
import java.util.Arrays;
import java.util.Collections;
import java.util.HashMap;
import java.util.List;
Expand Down Expand Up @@ -191,9 +193,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());
Map<String, char[]> previousUsers = users;
AntonShuvaev marked this conversation as resolved.
Show resolved Hide resolved
users = parseFileLenient(file, logger, settings);
notifyRefresh();

if (Maps.equals(previousUsers, users, Arrays::equals) == false) {
logger.info("users file [{}] changed. updating users... )", file.toAbsolutePath());
notifyRefresh();
}
}
}
}
Expand Down
12 changes: 9 additions & 3 deletions ...ecurity/src/main/java/org/elasticsearch/xpack/security/authc/file/FileUserRolesStore.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -27,6 +28,7 @@
import java.nio.file.Files;
import java.nio.file.Path;
import java.util.ArrayList;
import java.util.Arrays;
import java.util.Collections;
import java.util.HashMap;
import java.util.List;
Expand Down Expand Up @@ -206,9 +208,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());
Map<String, String[]> previousUserRoles = userRoles;
userRoles = parseFileLenient(file, logger);
notifyRefresh();

if (Maps.equals(previousUserRoles, userRoles, Arrays::equals) == false) {
logger.info("users roles file [{}] changed. updating users roles...", file.toAbsolutePath());
notifyRefresh();
}
}
}
}
Expand Down
10 changes: 7 additions & 3 deletions ...n/security/src/main/java/org/elasticsearch/xpack/security/authc/support/DnRoleMapper.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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());
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();
}
}
}
}
Expand Down
8 changes: 5 additions & 3 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 @@ -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(
Expand All @@ -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));
}
}
}
}
Expand Down
15 changes: 14 additions & 1 deletion ...y/src/test/java/org/elasticsearch/xpack/security/authc/file/FileUserPasswdStoreTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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"))
Expand Down Expand Up @@ -103,6 +103,19 @@ public void testStore_AutoReload() throws Exception {

watcherService.start();

try (BufferedWriter writer = Files.newBufferedWriter(file, StandardCharsets.UTF_8, StandardOpenOption.APPEND)) {
writer.append("\n");
}

if (latch.await(200, TimeUnit.MILLISECONDS)) {
fail("Listener should not be called as users passwords are not changed.");
}
AntonShuvaev marked this conversation as resolved.
Show resolved Hide resolved

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"))));
Expand Down
12 changes: 11 additions & 1 deletion ...ty/src/test/java/org/elasticsearch/xpack/security/authc/file/FileUserRolesStoreTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down Expand Up @@ -102,6 +102,16 @@ public void testStoreAutoReload() throws Exception {

watcherService.start();

try (BufferedWriter writer = Files.newBufferedWriter(tmp, StandardCharsets.UTF_8, StandardOpenOption.APPEND)) {
writer.append("\n");
}

if (latch.await(200, TimeUnit.MILLISECONDS)) {
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");
Expand Down
11 changes: 11 additions & 0 deletions ...urity/src/test/java/org/elasticsearch/xpack/security/authc/support/DnRoleMapperTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -112,6 +112,17 @@ public void testMapper_AutoReload() throws Exception {

watcherService.start();

try (BufferedWriter writer = Files.newBufferedWriter(file, StandardCharsets.UTF_8, StandardOpenOption.APPEND)) {
writer.append("\n");
}

if (latch.await(200, TimeUnit.MILLISECONDS)) {
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")
Expand Down
14 changes: 13 additions & 1 deletion ...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 @@ -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);
Expand All @@ -354,6 +354,18 @@ public void testAutoReload() throws Exception {

watcherService.start();

try (BufferedWriter writer = Files.newBufferedWriter(tmp, StandardCharsets.UTF_8, StandardOpenOption.APPEND)) {
writer.append("\n");
}

if (latch.await(200, TimeUnit.MILLISECONDS)) {
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();
Expand Down