From d393cc27402b3a69fc1deba4ccc68ab96212c11c Mon Sep 17 00:00:00 2001 From: jaymode Date: Fri, 1 May 2015 10:26:56 -0400 Subject: [PATCH] do not attempt to resolve permissions in the esusers tool The esusers tool reads the list of roles to provide validation feedback to the user, however since we have added custom roles the tool doesn't know about these roles as they come from outside of Shield. When a custom role was found, a warning was printed that can be confusing to users. Now when validating roles, we only read the names from the roles.yml file. Closes elastic/elasticsearch#835 Original commit: elastic/x-pack-elasticsearch@89d0e3efce6343eda81d0b5c704a27916d1e3759 --- .../authc/esusers/tool/ESUsersTool.java | 18 +++----- .../shield/authz/store/FileRolesStore.java | 21 +++++++-- .../authc/esusers/tool/ESUsersToolTests.java | 46 +++++++++++++++++++ .../shield/authz/PermissionTests.java | 11 +++-- .../authz/store/FileRolesStoreTests.java | 21 +++++++-- 5 files changed, 95 insertions(+), 22 deletions(-) diff --git a/src/main/java/org/elasticsearch/shield/authc/esusers/tool/ESUsersTool.java b/src/main/java/org/elasticsearch/shield/authc/esusers/tool/ESUsersTool.java index 281d4365fab..3a7efb3af05 100644 --- a/src/main/java/org/elasticsearch/shield/authc/esusers/tool/ESUsersTool.java +++ b/src/main/java/org/elasticsearch/shield/authc/esusers/tool/ESUsersTool.java @@ -21,7 +21,6 @@ import org.elasticsearch.shield.authc.esusers.FileUserPasswdStore; import org.elasticsearch.shield.authc.esusers.FileUserRolesStore; import org.elasticsearch.shield.authc.support.Hasher; import org.elasticsearch.shield.authc.support.SecuredString; -import org.elasticsearch.shield.authz.Permission; import org.elasticsearch.shield.authz.store.FileRolesStore; import org.elasticsearch.shield.support.Validation; @@ -430,7 +429,7 @@ public class ESUsersTool extends CliTool { @Override public ExitStatus execute(Settings settings, Environment env) throws Exception { Settings esusersSettings = Realms.internalRealmSettings(settings, ESUsersRealm.TYPE); - ImmutableMap knownRoles = loadRoles(terminal, settings, env); + ImmutableSet knownRoles = loadRoleNames(terminal, settings, env); Path userRolesFilePath = FileUserRolesStore.resolveFile(esusersSettings, env); Map userRoles = FileUserRolesStore.parseFile(userRolesFilePath, null); Path userFilePath = FileUserPasswdStore.resolveFile(esusersSettings, env); @@ -444,7 +443,7 @@ public class ESUsersTool extends CliTool { if (userRoles.containsKey(username)) { String[] roles = userRoles.get(username); - Set unknownRoles = Sets.difference(Sets.newHashSet(roles), knownRoles.keySet()); + Set unknownRoles = Sets.difference(Sets.newHashSet(roles), knownRoles); String[] markedRoles = markUnknownRoles(roles, unknownRoles); terminal.println("%-15s: %s", username, Joiner.on(",").useForNull("-").join(markedRoles)); if (!unknownRoles.isEmpty()) { @@ -461,7 +460,7 @@ public class ESUsersTool extends CliTool { boolean usersExist = false; for (Map.Entry entry : userRoles.entrySet()) { String[] roles = entry.getValue(); - Set unknownRoles = Sets.difference(Sets.newHashSet(roles), knownRoles.keySet()); + Set unknownRoles = Sets.difference(Sets.newHashSet(roles), knownRoles); String[] markedRoles = markUnknownRoles(roles, unknownRoles); terminal.println("%-15s: %s", entry.getKey(), Joiner.on(",").join(markedRoles)); unknownRolesFound = unknownRolesFound || !unknownRoles.isEmpty(); @@ -492,10 +491,10 @@ public class ESUsersTool extends CliTool { } } - private static ImmutableMap loadRoles(Terminal terminal, Settings settings, Environment env) { + private static ImmutableSet loadRoleNames(Terminal terminal, Settings settings, Environment env) { Path rolesFile = FileRolesStore.resolveFile(settings, env); try { - return FileRolesStore.parseFile(rolesFile, null); + return FileRolesStore.parseFileForRoleNames(rolesFile, null); } catch (Throwable t) { // if for some reason, parsing fails (malformatted perhaps) we just warn terminal.println("Warning: Could not parse [%s] for roles verification. Please revise and fix it. Nonetheless, the user will still be associated with all specified roles", rolesFile.toAbsolutePath()); @@ -519,11 +518,8 @@ public class ESUsersTool extends CliTool { } private static void verifyRoles(Terminal terminal, Settings settings, Environment env, String[] roles) { - ImmutableMap knownRoles = loadRoles(terminal, settings, env); - if (knownRoles == null) { - return; - } - Set unknownRoles = Sets.difference(Sets.newHashSet(roles), knownRoles.keySet()); + ImmutableSet knownRoles = loadRoleNames(terminal, settings, env); + Set unknownRoles = Sets.difference(Sets.newHashSet(roles), knownRoles); if (!unknownRoles.isEmpty()) { Path rolesFile = FileRolesStore.resolveFile(settings, env); terminal.println("Warning: The following roles [%s] are unknown. Make sure to add them to the [%s] file. " + diff --git a/src/main/java/org/elasticsearch/shield/authz/store/FileRolesStore.java b/src/main/java/org/elasticsearch/shield/authz/store/FileRolesStore.java index 037f253a64c..e8cc6891ec4 100644 --- a/src/main/java/org/elasticsearch/shield/authz/store/FileRolesStore.java +++ b/src/main/java/org/elasticsearch/shield/authz/store/FileRolesStore.java @@ -100,11 +100,19 @@ public class FileRolesStore extends AbstractLifecycleComponent imple return env.homeFile().resolve(location); } - public static ImmutableMap parseFile(Path path, ESLogger logger) { - return parseFile(path, Collections.emptySet(), logger); + public static ImmutableSet parseFileForRoleNames(Path path, ESLogger logger) { + ImmutableMap roleMap = parseFile(path, Collections.emptySet(), logger, false); + if (roleMap == null) { + return ImmutableSet.builder().build(); + } + return roleMap.keySet(); } public static ImmutableMap parseFile(Path path, Set reservedRoles, ESLogger logger) { + return parseFile(path, reservedRoles, logger, true); + } + + public static ImmutableMap parseFile(Path path, Set reservedRoles, ESLogger logger, boolean resolvePermission) { if (logger == null) { logger = NoOpLogger.INSTANCE; } @@ -121,7 +129,7 @@ public class FileRolesStore extends AbstractLifecycleComponent imple List roleSegments = roleSegments(path); for (String segment : roleSegments) { - Permission.Global.Role role = parseRole(segment, path, logger); + Permission.Global.Role role = parseRole(segment, path, logger, resolvePermission); if (role != null) { if (SystemRole.NAME.equals(role.name())) { logger.warn("role [{}] is reserved to the system. the relevant role definition in the mapping file will be ignored", SystemRole.NAME); @@ -146,7 +154,7 @@ public class FileRolesStore extends AbstractLifecycleComponent imple return ImmutableMap.copyOf(roles); } - private static Permission.Global.Role parseRole(String segment, Path path, ESLogger logger) { + private static Permission.Global.Role parseRole(String segment, Path path, ESLogger logger, boolean resolvePermissions) { String roleName = null; try { XContentParser parser = YamlXContent.yamlXContent.createParser(segment); @@ -160,7 +168,12 @@ public class FileRolesStore extends AbstractLifecycleComponent imple logger.error("invalid role definition [{}] in roles file [{}]. invalid role name - {}. skipping role... ", roleName, path.toAbsolutePath(), validationError); return null; } + Permission.Global.Role.Builder permission = Permission.Global.Role.builder(roleName); + if (resolvePermissions == false) { + return permission.build(); + } + token = parser.nextToken(); if (token == XContentParser.Token.START_OBJECT) { String currentFieldName = null; diff --git a/src/test/java/org/elasticsearch/shield/authc/esusers/tool/ESUsersToolTests.java b/src/test/java/org/elasticsearch/shield/authc/esusers/tool/ESUsersToolTests.java index 0761851225f..692622634e7 100644 --- a/src/test/java/org/elasticsearch/shield/authc/esusers/tool/ESUsersToolTests.java +++ b/src/test/java/org/elasticsearch/shield/authc/esusers/tool/ESUsersToolTests.java @@ -213,6 +213,52 @@ public class ESUsersToolTests extends CliToolTestCase { assertThat(status, is(CliTool.ExitStatus.CODE_ERROR)); } + + @Test + public void testUseradd_CustomRole() throws Exception { + Path usersFile = createTempFile(); + Path userRolesFile = createTempFile(); + Path rolesFile = writeFile("plugin_admin:\n" + + " manage_plugin"); + Settings settings = Settings.builder() + .put("shield.authc.realms.esusers.type", "esusers") + .put("shield.authc.realms.esusers.files.users", usersFile) + .put("shield.authc.realms.esusers.files.users_roles", userRolesFile) + .put("shield.authz.store.files.roles", rolesFile) + .put("path.home", createTempDir()) + .build(); + + final CaptureOutputTerminal terminal = new CaptureOutputTerminal(); + ESUsersTool.Useradd cmd = new ESUsersTool.Useradd(terminal, "user1", SecuredStringTests.build("changeme"), "plugin_admin"); + + CliTool.ExitStatus status = execute(cmd, settings); + assertThat(status, is(CliTool.ExitStatus.OK)); + assertThat(terminal.getTerminalOutput(), hasSize(0)); + } + + @Test + public void testUseradd_NonExistantRole() throws Exception { + Path usersFile = createTempFile(); + Path userRolesFile = createTempFile(); + Path rolesFile = writeFile("plugin_admin:\n" + + " manage_plugin"); + Settings settings = Settings.builder() + .put("shield.authc.realms.esusers.type", "esusers") + .put("shield.authc.realms.esusers.files.users", usersFile) + .put("shield.authc.realms.esusers.files.users_roles", userRolesFile) + .put("shield.authz.store.files.roles", rolesFile) + .put("path.home", createTempDir()) + .build(); + + final CaptureOutputTerminal terminal = new CaptureOutputTerminal(); + ESUsersTool.Useradd cmd = new ESUsersTool.Useradd(terminal, "user1", SecuredStringTests.build("changeme"), "plugin_admin_2"); + + CliTool.ExitStatus status = execute(cmd, settings); + assertThat(status, is(CliTool.ExitStatus.OK)); + assertThat(terminal.getTerminalOutput(), hasSize(1)); + assertThat(terminal.getTerminalOutput().get(0), containsString("[plugin_admin_2]")); + } + @Test public void testUserdel_Parse() throws Exception { ESUsersTool tool = new ESUsersTool(); diff --git a/src/test/java/org/elasticsearch/shield/authz/PermissionTests.java b/src/test/java/org/elasticsearch/shield/authz/PermissionTests.java index b6e3c6299cc..47ca7aea1ee 100644 --- a/src/test/java/org/elasticsearch/shield/authz/PermissionTests.java +++ b/src/test/java/org/elasticsearch/shield/authz/PermissionTests.java @@ -15,9 +15,7 @@ import org.junit.Test; import java.util.Iterator; import static org.elasticsearch.shield.authz.Privilege.Index.*; -import static org.hamcrest.Matchers.equalTo; -import static org.hamcrest.Matchers.instanceOf; -import static org.hamcrest.Matchers.is; +import static org.hamcrest.Matchers.*; /** * @@ -64,6 +62,13 @@ public class PermissionTests extends ElasticsearchTestCase { assertThat(count, is(equalTo(permission.indices().groups().length))); } + @Test + public void buildEmptyRole() { + Permission.Global.Role.Builder permission = Permission.Global.Role.builder("some_role"); + Permission.Global.Role role = permission.build(); + assertThat(role, notNullValue()); + } + // "baz_*foo", "/fool.*bar/" private void testAllowedIndicesMatcher(Predicate indicesMatcher) { assertThat(indicesMatcher.apply("foobar"), is(false)); diff --git a/src/test/java/org/elasticsearch/shield/authz/store/FileRolesStoreTests.java b/src/test/java/org/elasticsearch/shield/authz/store/FileRolesStoreTests.java index 4042ba05ee8..e6b6c29118c 100644 --- a/src/test/java/org/elasticsearch/shield/authz/store/FileRolesStoreTests.java +++ b/src/test/java/org/elasticsearch/shield/authz/store/FileRolesStoreTests.java @@ -41,7 +41,7 @@ public class FileRolesStoreTests extends ElasticsearchTestCase { @Test public void testParseFile() throws Exception { Path path = getDataPath("roles.yml"); - Map roles = FileRolesStore.parseFile(path, logger); + Map roles = FileRolesStore.parseFile(path, Collections.emptySet(), logger); assertThat(roles, notNullValue()); assertThat(roles.size(), is(4)); @@ -107,7 +107,7 @@ public class FileRolesStoreTests extends ElasticsearchTestCase { @Test public void testDefaultRolesFile() throws Exception { Path path = getDataPath("default_roles.yml"); - Map roles = FileRolesStore.parseFile(path, logger); + Map roles = FileRolesStore.parseFile(path, Collections.emptySet(), logger); assertThat(roles, notNullValue()); assertThat(roles.size(), is(8)); @@ -187,7 +187,7 @@ public class FileRolesStoreTests extends ElasticsearchTestCase { public void testThatEmptyFileDoesNotResultInLoop() throws Exception { Path file = createTempFile(); Files.write(file, ImmutableList.of("#"), Charsets.UTF_8); - Map roles = FileRolesStore.parseFile(file, logger); + Map roles = FileRolesStore.parseFile(file, Collections.emptySet(), logger); assertThat(roles.keySet(), is(empty())); } @@ -195,7 +195,7 @@ public class FileRolesStoreTests extends ElasticsearchTestCase { public void testThatInvalidRoleDefinitions() throws Exception { Path path = getDataPath("invalid_roles.yml"); CapturingLogger logger = new CapturingLogger(CapturingLogger.Level.ERROR); - Map roles = FileRolesStore.parseFile(path, logger); + Map roles = FileRolesStore.parseFile(path, Collections.emptySet(), logger); assertThat(roles.size(), is(1)); assertThat(roles, hasKey("valid_role")); Permission.Global.Role role = roles.get("valid_role"); @@ -211,6 +211,19 @@ public class FileRolesStoreTests extends ElasticsearchTestCase { assertThat(entries.get(4).text, startsWith("invalid role definition [role4] in roles file [" + path.toAbsolutePath() + "]. could not resolve indices privileges [al;kjdlkj;lkj]")); } + @Test + public void testThatRoleNamesDoesNotResolvePermissions() throws Exception { + Path path = getDataPath("invalid_roles.yml"); + CapturingLogger logger = new CapturingLogger(CapturingLogger.Level.ERROR); + ImmutableSet roleNames = FileRolesStore.parseFileForRoleNames(path, logger); + assertThat(roleNames.size(), is(5)); + assertThat(roleNames, containsInAnyOrder("valid_role", "role1", "role2", "role3", "role4")); + + List entries = logger.output(CapturingLogger.Level.ERROR); + assertThat(entries, hasSize(1)); + assertThat(entries.get(0).text, startsWith("invalid role definition [$dlk39] in roles file [" + path.toAbsolutePath() + "]. invalid role name")); + } + @Test public void testReservedRoles() throws Exception { Set reservedRoles = ImmutableSet.builder()