From fe5789ada1d2cef8ce569a5847c4ce0573584558 Mon Sep 17 00:00:00 2001 From: Albert Zaharovits Date: Thu, 25 Apr 2019 12:02:09 +0300 Subject: [PATCH] Fix Has Privilege API check on restricted indices (#41226) The Has Privileges API allows to tap into the authorization process, to validate privileges without actually running the operations to be authorized. This commit fixes a bug, in which the Has Privilege API returned spurious results when checking for index privileges over restricted indices (currently .security, .security-6, .security-7). The actual authorization process is not affected by the bug. --- .../rest-api/security/has-privileges.asciidoc | 12 +- .../authz/permission/IndicesPermission.java | 51 ++++--- .../core/security/authz/permission/Role.java | 3 +- .../xpack/security/authz/RBACEngineTests.java | 125 ++++++++++++++++++ 4 files changed, 165 insertions(+), 26 deletions(-) diff --git a/x-pack/docs/en/rest-api/security/has-privileges.asciidoc b/x-pack/docs/en/rest-api/security/has-privileges.asciidoc index 92f1081bc2b..984a8e36f0a 100644 --- a/x-pack/docs/en/rest-api/security/has-privileges.asciidoc +++ b/x-pack/docs/en/rest-api/security/has-privileges.asciidoc @@ -29,11 +29,13 @@ privilege is assigned to the user. `index`:: `names`::: (list) A list of indices. -`allow_restricted_indices`::: (boolean) If `names` contains internal restricted -that also have to be covered by the has-privilege check, then this has to be -set to `true`. By default this is `false` because restricted indices should -generaly not be "visible" to APIs. For most use cases it is safe to ignore -this parameter. +`allow_restricted_indices`::: (boolean) This needs to be set to `true` (default +is `false`) if using wildcards or regexps for patterns that cover restricted +indices. Implicitly, restricted indices do not match index patterns because +restricted indices usually have limited privileges and including them in +pattern tests would render most such tests `false`. If restricted indices are +explicitly included in the `names` list, privileges will be checked against +them regardless of the value of `allow_restricted_indices`. `privileges`::: (list) A list of the privileges that you want to check for the specified indices. diff --git a/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/authz/permission/IndicesPermission.java b/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/authz/permission/IndicesPermission.java index 356e80c4975..aab6bab8110 100644 --- a/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/authz/permission/IndicesPermission.java +++ b/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/authz/permission/IndicesPermission.java @@ -138,27 +138,40 @@ public final class IndicesPermission { final ResourcePrivilegesMap.Builder resourcePrivilegesMapBuilder = ResourcePrivilegesMap.builder(); final Map predicateCache = new HashMap<>(); for (String forIndexPattern : checkForIndexPatterns) { - final Automaton checkIndexAutomaton = IndicesPermission.Group.buildIndexMatcherAutomaton(allowRestrictedIndices, - forIndexPattern); - Automaton allowedIndexPrivilegesAutomaton = null; - for (Group group : groups) { - final Automaton groupIndexAutomaton = predicateCache.computeIfAbsent(group, - g -> IndicesPermission.Group.buildIndexMatcherAutomaton(g.allowRestrictedIndices(), g.indices())); - if (Operations.subsetOf(checkIndexAutomaton, groupIndexAutomaton)) { - if (allowedIndexPrivilegesAutomaton != null) { - allowedIndexPrivilegesAutomaton = Automatons - .unionAndMinimize(Arrays.asList(allowedIndexPrivilegesAutomaton, group.privilege().getAutomaton())); - } else { - allowedIndexPrivilegesAutomaton = group.privilege().getAutomaton(); + Automaton checkIndexAutomaton = Automatons.patterns(forIndexPattern); + if (false == allowRestrictedIndices && false == RestrictedIndicesNames.RESTRICTED_NAMES.contains(forIndexPattern)) { + checkIndexAutomaton = Automatons.minusAndMinimize(checkIndexAutomaton, RestrictedIndicesNames.NAMES_AUTOMATON); + } + if (false == Operations.isEmpty(checkIndexAutomaton)) { + Automaton allowedIndexPrivilegesAutomaton = null; + for (Group group : groups) { + final Automaton groupIndexAutomaton = predicateCache.computeIfAbsent(group, + g -> IndicesPermission.Group.buildIndexMatcherAutomaton(g.allowRestrictedIndices(), g.indices())); + if (Operations.subsetOf(checkIndexAutomaton, groupIndexAutomaton)) { + if (allowedIndexPrivilegesAutomaton != null) { + allowedIndexPrivilegesAutomaton = Automatons + .unionAndMinimize(Arrays.asList(allowedIndexPrivilegesAutomaton, group.privilege().getAutomaton())); + } else { + allowedIndexPrivilegesAutomaton = group.privilege().getAutomaton(); + } } } - } - for (String privilege : checkForPrivileges) { - IndexPrivilege indexPrivilege = IndexPrivilege.get(Collections.singleton(privilege)); - if (allowedIndexPrivilegesAutomaton != null - && Operations.subsetOf(indexPrivilege.getAutomaton(), allowedIndexPrivilegesAutomaton)) { - resourcePrivilegesMapBuilder.addResourcePrivilege(forIndexPattern, privilege, Boolean.TRUE); - } else { + for (String privilege : checkForPrivileges) { + IndexPrivilege indexPrivilege = IndexPrivilege.get(Collections.singleton(privilege)); + if (allowedIndexPrivilegesAutomaton != null + && Operations.subsetOf(indexPrivilege.getAutomaton(), allowedIndexPrivilegesAutomaton)) { + resourcePrivilegesMapBuilder.addResourcePrivilege(forIndexPattern, privilege, Boolean.TRUE); + } else { + resourcePrivilegesMapBuilder.addResourcePrivilege(forIndexPattern, privilege, Boolean.FALSE); + } + } + } else { + // the index pattern produced the empty automaton, presumably because the requested pattern expands exclusively inside the + // restricted indices namespace - a namespace of indices that are normally hidden when granting/checking privileges - and + // the pattern was not marked as `allowRestrictedIndices`. We try to anticipate this by considering _explicit_ restricted + // indices even if `allowRestrictedIndices` is false. + // TODO The `false` result is a _safe_ default but this is actually an error. Make it an error. + for (String privilege : checkForPrivileges) { resourcePrivilegesMapBuilder.addResourcePrivilege(forIndexPattern, privilege, Boolean.FALSE); } } diff --git a/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/authz/permission/Role.java b/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/authz/permission/Role.java index 817a9e41eab..207a5ab0567 100644 --- a/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/authz/permission/Role.java +++ b/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/authz/permission/Role.java @@ -49,7 +49,6 @@ public class Role { this.runAs = Objects.requireNonNull(runAs); } - public String[] names() { return names; } @@ -116,7 +115,7 @@ public class Role { * @return an instance of {@link ResourcePrivilegesMap} */ public ResourcePrivilegesMap checkIndicesPrivileges(Set checkForIndexPatterns, boolean allowRestrictedIndices, - Set checkForPrivileges) { + Set checkForPrivileges) { return indices.checkResourcePrivileges(checkForIndexPatterns, allowRestrictedIndices, checkForPrivileges); } diff --git a/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/authz/RBACEngineTests.java b/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/authz/RBACEngineTests.java index e43ca6bbc0b..5c2e964c743 100644 --- a/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/authz/RBACEngineTests.java +++ b/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/authz/RBACEngineTests.java @@ -48,6 +48,7 @@ import org.elasticsearch.xpack.core.security.authz.privilege.ApplicationPrivileg import org.elasticsearch.xpack.core.security.authz.privilege.ApplicationPrivilegeDescriptor; import org.elasticsearch.xpack.core.security.authz.privilege.ClusterPrivilege; import org.elasticsearch.xpack.core.security.authz.privilege.ConditionalClusterPrivileges.ManageApplicationPrivileges; +import org.elasticsearch.xpack.core.security.index.RestrictedIndicesNames; import org.elasticsearch.xpack.core.security.authz.privilege.IndexPrivilege; import org.elasticsearch.xpack.core.security.authz.privilege.Privilege; import org.elasticsearch.xpack.core.security.user.User; @@ -493,6 +494,130 @@ public class RBACEngineTests extends ESTestCase { )); } + public void testCheckExplicitRestrictedIndexPermissions() throws Exception { + User user = new User(randomAlphaOfLengthBetween(4, 12)); + Authentication authentication = mock(Authentication.class); + when(authentication.getUser()).thenReturn(user); + final boolean restrictedIndexPermission = randomBoolean(); + final boolean restrictedMonitorPermission = randomBoolean(); + Role role = Role.builder("role") + .add(FieldPermissions.DEFAULT, null, IndexPrivilege.INDEX, restrictedIndexPermission, ".sec*") + .add(FieldPermissions.DEFAULT, null, IndexPrivilege.MONITOR, restrictedMonitorPermission, ".security*") + .build(); + RBACAuthorizationInfo authzInfo = new RBACAuthorizationInfo(role, null); + + String explicitRestrictedIndex = randomFrom(RestrictedIndicesNames.RESTRICTED_NAMES); + HasPrivilegesResponse response = hasPrivileges(RoleDescriptor.IndicesPrivileges.builder() + .indices(new String[] {".secret-non-restricted", explicitRestrictedIndex}) + .privileges("index", "monitor") + .allowRestrictedIndices(false) // explicit false for test + .build(), authentication, authzInfo, Collections.emptyList(), Strings.EMPTY_ARRAY); + assertThat(response.isCompleteMatch(), is(false)); + assertThat(response.getIndexPrivileges(), Matchers.iterableWithSize(2)); + assertThat(response.getIndexPrivileges(), containsInAnyOrder( + ResourcePrivileges.builder(".secret-non-restricted") // matches ".sec*" but not ".security*" + .addPrivileges(MapBuilder.newMapBuilder(new LinkedHashMap()) + .put("index", true).put("monitor", false).map()).build(), + ResourcePrivileges.builder(explicitRestrictedIndex) // matches both ".sec*" and ".security*" + .addPrivileges(MapBuilder.newMapBuilder(new LinkedHashMap()) + .put("index", restrictedIndexPermission).put("monitor", restrictedMonitorPermission).map()).build())); + + explicitRestrictedIndex = randomFrom(RestrictedIndicesNames.RESTRICTED_NAMES); + response = hasPrivileges(RoleDescriptor.IndicesPrivileges.builder() + .indices(new String[] {".secret-non-restricted", explicitRestrictedIndex}) + .privileges("index", "monitor") + .allowRestrictedIndices(true) // explicit true for test + .build(), authentication, authzInfo, Collections.emptyList(), Strings.EMPTY_ARRAY); + assertThat(response.isCompleteMatch(), is(false)); + assertThat(response.getIndexPrivileges(), Matchers.iterableWithSize(2)); + assertThat(response.getIndexPrivileges(), containsInAnyOrder( + ResourcePrivileges.builder(".secret-non-restricted") // matches ".sec*" but not ".security*" + .addPrivileges(MapBuilder.newMapBuilder(new LinkedHashMap()) + .put("index", true).put("monitor", false).map()).build(), + ResourcePrivileges.builder(explicitRestrictedIndex) // matches both ".sec*" and ".security*" + .addPrivileges(MapBuilder.newMapBuilder(new LinkedHashMap()) + .put("index", restrictedIndexPermission).put("monitor", restrictedMonitorPermission).map()).build())); + } + + public void testCheckRestrictedIndexWildcardPermissions() throws Exception { + User user = new User(randomAlphaOfLengthBetween(4, 12)); + Authentication authentication = mock(Authentication.class); + when(authentication.getUser()).thenReturn(user); + Role role = Role.builder("role") + .add(FieldPermissions.DEFAULT, null, IndexPrivilege.INDEX, false, ".sec*") + .add(FieldPermissions.DEFAULT, null, IndexPrivilege.MONITOR, true, ".security*") + .build(); + RBACAuthorizationInfo authzInfo = new RBACAuthorizationInfo(role, null); + + HasPrivilegesResponse response = hasPrivileges(RoleDescriptor.IndicesPrivileges.builder() + .indices(".sec*", ".security*") + .privileges("index", "monitor") + .build(), authentication, authzInfo, Collections.emptyList(), Strings.EMPTY_ARRAY); + assertThat(response.isCompleteMatch(), is(false)); + assertThat(response.getIndexPrivileges(), Matchers.iterableWithSize(2)); + assertThat(response.getIndexPrivileges(), containsInAnyOrder( + ResourcePrivileges.builder(".sec*") + .addPrivileges(MapBuilder.newMapBuilder(new LinkedHashMap()) + .put("index", true).put("monitor", false).map()).build(), + ResourcePrivileges.builder(".security*") + .addPrivileges(MapBuilder.newMapBuilder(new LinkedHashMap()) + .put("index", true).put("monitor", true).map()).build() + )); + + response = hasPrivileges(RoleDescriptor.IndicesPrivileges.builder() + .indices(".sec*", ".security*") + .privileges("index", "monitor") + .allowRestrictedIndices(true) + .build(), authentication, authzInfo, Collections.emptyList(), Strings.EMPTY_ARRAY); + assertThat(response.isCompleteMatch(), is(false)); + assertThat(response.getIndexPrivileges(), Matchers.iterableWithSize(2)); + assertThat(response.getIndexPrivileges(), containsInAnyOrder( + ResourcePrivileges.builder(".sec*") + .addPrivileges(MapBuilder.newMapBuilder(new LinkedHashMap()) + .put("index", false).put("monitor", false).map()).build(), + ResourcePrivileges.builder(".security*") + .addPrivileges(MapBuilder.newMapBuilder(new LinkedHashMap()) + .put("index", false).put("monitor", true).map()).build() + )); + + role = Role.builder("role") + .add(FieldPermissions.DEFAULT, null, IndexPrivilege.INDEX, true, ".sec*") + .add(FieldPermissions.DEFAULT, null, IndexPrivilege.MONITOR, false, ".security*") + .build(); + authzInfo = new RBACAuthorizationInfo(role, null); + + response = hasPrivileges(RoleDescriptor.IndicesPrivileges.builder() + .indices(".sec*", ".security*") + .privileges("index", "monitor") + .build(), authentication, authzInfo, Collections.emptyList(), Strings.EMPTY_ARRAY); + assertThat(response.isCompleteMatch(), is(false)); + assertThat(response.getIndexPrivileges(), Matchers.iterableWithSize(2)); + assertThat(response.getIndexPrivileges(), containsInAnyOrder( + ResourcePrivileges.builder(".sec*") + .addPrivileges(MapBuilder.newMapBuilder(new LinkedHashMap()) + .put("index", true).put("monitor", false).map()).build(), + ResourcePrivileges.builder(".security*") + .addPrivileges(MapBuilder.newMapBuilder(new LinkedHashMap()) + .put("index", true).put("monitor", true).map()).build() + )); + + response = hasPrivileges(RoleDescriptor.IndicesPrivileges.builder() + .indices(".sec*", ".security*") + .privileges("index", "monitor") + .allowRestrictedIndices(true) + .build(), authentication, authzInfo, Collections.emptyList(), Strings.EMPTY_ARRAY); + assertThat(response.isCompleteMatch(), is(false)); + assertThat(response.getIndexPrivileges(), Matchers.iterableWithSize(2)); + assertThat(response.getIndexPrivileges(), containsInAnyOrder( + ResourcePrivileges.builder(".sec*") + .addPrivileges(MapBuilder.newMapBuilder(new LinkedHashMap()) + .put("index", true).put("monitor", false).map()).build(), + ResourcePrivileges.builder(".security*") + .addPrivileges(MapBuilder.newMapBuilder(new LinkedHashMap()) + .put("index", true).put("monitor", false).map()).build() + )); + } + public void testCheckingApplicationPrivilegesOnDifferentApplicationsAndResources() throws Exception { List privs = new ArrayList<>(); final ApplicationPrivilege app1Read = defineApplicationPrivilege(privs, "app1", "read", "data:read/*");