Fix Has Privilege API check on restricted indices (#41226)

albertzaharovits · albertzaharovits · commit 36ad069e1427 · 2019-04-25T12:06:10.000+03:00
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.
diff --git 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
@@ -138,27 +138,40 @@ public ResourcePrivilegesMap checkResourcePrivileges(Set<String> checkForIndexPa
         final ResourcePrivilegesMap.Builder resourcePrivilegesMapBuilder = ResourcePrivilegesMap.builder();
         final Map<IndicesPermission.Group, Automaton> 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()));
+            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 {
-                        allowedIndexPrivilegesAutomaton = group.privilege().getAutomaton();
+                        resourcePrivilegesMapBuilder.addResourcePrivilege(forIndexPattern, privilege, Boolean.FALSE);
                     }
                 }
-            }
-            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 {
+            } 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
@@ -49,7 +49,6 @@ public class Role {
         this.runAs = Objects.requireNonNull(runAs);
     }
 
-
     public String[] names() {
         return names;
     }
@@ -120,7 +119,7 @@ public boolean checkIndicesAction(String action) {
      * @return an instance of {@link ResourcePrivilegesMap}
      */
     public ResourcePrivilegesMap checkIndicesPrivileges(Set<String> checkForIndexPatterns, boolean allowRestrictedIndices,
-                                                                 Set<String> checkForPrivileges) {
+                                                        Set<String> 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
@@ -48,6 +48,7 @@
 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 void testCheckingIndexPermissionsDefinedOnDifferentPatterns() throws Exce
         ));
     }
 
+    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<String, Boolean>())
+                        .put("index", true).put("monitor", false).map()).build(),
+                ResourcePrivileges.builder(explicitRestrictedIndex) // matches both ".sec*" and ".security*"
+                    .addPrivileges(MapBuilder.newMapBuilder(new LinkedHashMap<String, Boolean>())
+                        .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<String, Boolean>())
+                        .put("index", true).put("monitor", false).map()).build(),
+                ResourcePrivileges.builder(explicitRestrictedIndex) // matches both ".sec*" and ".security*"
+                    .addPrivileges(MapBuilder.newMapBuilder(new LinkedHashMap<String, Boolean>())
+                        .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<String, Boolean>())
+                    .put("index", true).put("monitor", false).map()).build(),
+            ResourcePrivileges.builder(".security*")
+                .addPrivileges(MapBuilder.newMapBuilder(new LinkedHashMap<String, Boolean>())
+                    .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<String, Boolean>())
+                    .put("index", false).put("monitor", false).map()).build(),
+            ResourcePrivileges.builder(".security*")
+                .addPrivileges(MapBuilder.newMapBuilder(new LinkedHashMap<String, Boolean>())
+                    .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<String, Boolean>())
+                    .put("index", true).put("monitor", false).map()).build(),
+            ResourcePrivileges.builder(".security*")
+                .addPrivileges(MapBuilder.newMapBuilder(new LinkedHashMap<String, Boolean>())
+                    .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<String, Boolean>())
+                    .put("index", true).put("monitor", false).map()).build(),
+            ResourcePrivileges.builder(".security*")
+                .addPrivileges(MapBuilder.newMapBuilder(new LinkedHashMap<String, Boolean>())
+                    .put("index", true).put("monitor", false).map()).build()
+        ));
+    }
+
     public void testCheckingApplicationPrivilegesOnDifferentApplicationsAndResources() throws Exception {
         List<ApplicationPrivilegeDescriptor> privs = new ArrayList<>();
         final ApplicationPrivilege app1Read = defineApplicationPrivilege(privs, "app1", "read", "data:read/*");

Original file line number	Diff line number	Diff line change
`@@ -49,7 +49,6 @@ public class Role {`
`49`	`49`	`this.runAs = Objects.requireNonNull(runAs);`
`50`	`50`	`}`
`51`	`51`
`52`		`-`
`53`	`52`	`public String[] names() {`
`54`	`53`	`return names;`
`55`	`54`	`}`
`@@ -120,7 +119,7 @@ public boolean checkIndicesAction(String action) {`
`120`	`119`	`* @return an instance of {@link ResourcePrivilegesMap}`
`121`	`120`	`*/`
`122`	`121`	`public ResourcePrivilegesMap checkIndicesPrivileges(Set<String> checkForIndexPatterns, boolean allowRestrictedIndices,`
`123`		`- Set<String> checkForPrivileges) {`
	`122`	`+ Set<String> checkForPrivileges) {`
`124`	`123`	`return indices.checkResourcePrivileges(checkForIndexPatterns, allowRestrictedIndices, checkForPrivileges);`
`125`	`124`	`}`
`126`	`125`