Empty GetAliases authorization fix (#34444)

This fixes a bug about aliases authorization.
That is, a user might see aliases which he is not authorized to see.
This manifests when the user is not authorized to see any aliases
and the `GetAlias` request is empty which normally is a marking
that all aliases are requested. In this case, no aliases should be
returned, but due to this bug, all aliases will have been returned.

server/src/main/java/org/elasticsearch/cluster/metadata/MetaData.java

@@ -249,53 +249,45 @@ public class MetaData implements Iterable<IndexMetaData>, Diffable<MetaData>, To
     }
 
     /**
-     * Finds the specific index aliases that point to the specified concrete indices or match partially with the indices via wildcards.
+     * Finds the specific index aliases that point to the requested concrete indices directly
+     * or that match with the indices via wildcards.
      *
-     * @param concreteIndices The concrete indexes the index aliases must point to order to be returned.
+     * @param concreteIndices The concrete indices that the aliases must point to in order to be returned.
-     * @return a map of index to a list of alias metadata, the list corresponding to a concrete index will be empty if no aliases are
+     * @return A map of index name to the list of aliases metadata. If a concrete index does not have matching
-     * present for that index
+     * aliases then the result will <b>not</b> include the index's key.
      */
-    public ImmutableOpenMap<String, List<AliasMetaData>> findAllAliases(String[] concreteIndices) {
+    public ImmutableOpenMap<String, List<AliasMetaData>> findAllAliases(final String[] concreteIndices) {
-        return findAliases(Strings.EMPTY_ARRAY, Strings.EMPTY_ARRAY, concreteIndices);
+        return findAliases(Strings.EMPTY_ARRAY, concreteIndices);
     }
 
     /**
-     * Finds the specific index aliases that match with the specified aliases directly or partially via wildcards and
+     * Finds the specific index aliases that match with the specified aliases directly or partially via wildcards, and
-     * that point to the specified concrete indices or match partially with the indices via wildcards.
+     * that point to the specified concrete indices (directly or matching indices via wildcards).
      *
      * @param aliasesRequest The request to find aliases for
-     * @param concreteIndices The concrete indexes the index aliases must point to order to be returned.
+     * @param concreteIndices The concrete indices that the aliases must point to in order to be returned.
-     * @return a map of index to a list of alias metadata, the list corresponding to a concrete index will be empty if no aliases are
+     * @return A map of index name to the list of aliases metadata. If a concrete index does not have matching
-     * present for that index
+     * aliases then the result will <b>not</b> include the index's key.
      */
-    public ImmutableOpenMap<String, List<AliasMetaData>> findAliases(final AliasesRequest aliasesRequest, String[] concreteIndices) {
+    public ImmutableOpenMap<String, List<AliasMetaData>> findAliases(final AliasesRequest aliasesRequest, final String[] concreteIndices) {
-        return findAliases(aliasesRequest.getOriginalAliases(), aliasesRequest.aliases(), concreteIndices);
+        return findAliases(aliasesRequest.aliases(), concreteIndices);
     }
 
     /**
-     * Finds the specific index aliases that match with the specified aliases directly or partially via wildcards and
+     * Finds the specific index aliases that match with the specified aliases directly or partially via wildcards, and
-     * that point to the specified concrete indices or match partially with the indices via wildcards.
+     * that point to the specified concrete indices (directly or matching indices via wildcards).
      *
-     * @param aliases The aliases to look for
+     * @param aliases The aliases to look for. Might contain include or exclude wildcards.
-     * @param originalAliases The original aliases that the user originally requested
+     * @param concreteIndices The concrete indices that the aliases must point to in order to be returned
-     * @param concreteIndices The concrete indexes the index aliases must point to order to be returned.
+     * @return A map of index name to the list of aliases metadata. If a concrete index does not have matching
-     * @return a map of index to a list of alias metadata, the list corresponding to a concrete index will be empty if no aliases are
+     * aliases then the result will <b>not</b> include the index's key.
-     * present for that index
      */
-    private ImmutableOpenMap<String, List<AliasMetaData>> findAliases(String[] originalAliases, String[] aliases,
+    private ImmutableOpenMap<String, List<AliasMetaData>> findAliases(final String[] aliases, final String[] concreteIndices) {
-                                                                      String[] concreteIndices) {
         assert aliases != null;
-        assert originalAliases != null;
         assert concreteIndices != null;
         if (concreteIndices.length == 0) {
             return ImmutableOpenMap.of();
         }
-
-        //if aliases were provided but they got replaced with empty aliases, return empty map
-        if (originalAliases.length > 0 && aliases.length == 0) {
-            return ImmutableOpenMap.of();
-        }
-
         String[] patterns = new String[aliases.length];
         boolean[] include = new boolean[aliases.length];
         for (int i = 0; i < aliases.length; i++) {
@@ -331,7 +323,6 @@ public class MetaData implements Iterable<IndexMetaData>, Diffable<MetaData>, To
                     filteredValues.add(value);
                 }
             }
-
             if (filteredValues.isEmpty() == false) {
                 // Make the list order deterministic
                 CollectionUtil.timSort(filteredValues, Comparator.comparing(AliasMetaData::alias));

server/src/test/java/org/elasticsearch/cluster/metadata/IndexCreationTaskTests.java

@@ -51,6 +51,7 @@ import org.elasticsearch.index.mapper.MapperService;
 import org.elasticsearch.index.mapper.RoutingFieldMapper;
 import org.elasticsearch.index.shard.IndexEventListener;
 import org.elasticsearch.indices.IndicesService;
+import org.elasticsearch.indices.InvalidAliasNameException;
 import org.elasticsearch.test.ESTestCase;
 import org.hamcrest.Matchers;
 import org.mockito.ArgumentCaptor;
@@ -82,7 +83,7 @@ import static org.mockito.Mockito.when;
 public class IndexCreationTaskTests extends ESTestCase {
 
     private final IndicesService indicesService = mock(IndicesService.class);
-    private final AliasValidator aliasValidator = mock(AliasValidator.class);
+    private final AliasValidator aliasValidator = new AliasValidator(Settings.EMPTY);
     private final NamedXContentRegistry xContentRegistry = mock(NamedXContentRegistry.class);
     private final CreateIndexClusterStateUpdateRequest request = mock(CreateIndexClusterStateUpdateRequest.class);
     private final Logger logger = mock(Logger.class);
@@ -149,6 +150,12 @@ public class IndexCreationTaskTests extends ESTestCase {
         assertThat(getMappingsFromResponse(), Matchers.hasKey("mapping1"));
     }
 
+    public void testInvalidAliasName() throws Exception {
+        final String[] invalidAliasNames = new String[] { "-alias1", "+alias2", "_alias3", "a#lias", "al:ias", ".", ".." };
+        setupRequestAlias(new Alias(randomFrom(invalidAliasNames)));
+        expectThrows(InvalidAliasNameException.class, this::executeTask);
+    }
+
     public void testRequestDataHavePriorityOverTemplateData() throws Exception {
         final CompressedXContent tplMapping = createMapping("text");
         final CompressedXContent reqMapping = createMapping("keyword");

server/src/test/java/org/elasticsearch/cluster/metadata/MetaDataTests.java

@@ -66,6 +66,14 @@ public class MetaDataTests extends ESTestCase {
             assertThat(aliases.size(), equalTo(0));
         }
         {
+            final GetAliasesRequest request;
+            if (randomBoolean()) {
+                request = new GetAliasesRequest();
+            } else {
+                request = new GetAliasesRequest(randomFrom("alias1", "alias2"));
+                // replacing with empty aliases behaves as if aliases were unspecified at request building
+                request.replaceAliases(Strings.EMPTY_ARRAY);
+            }
             ImmutableOpenMap<String, List<AliasMetaData>> aliases = metaData.findAliases(new GetAliasesRequest(), new String[]{"index"});
             assertThat(aliases.size(), equalTo(1));
             List<AliasMetaData> aliasMetaDataList = aliases.get("index");
@@ -73,12 +81,6 @@ public class MetaDataTests extends ESTestCase {
             assertThat(aliasMetaDataList.get(0).alias(), equalTo("alias1"));
             assertThat(aliasMetaDataList.get(1).alias(), equalTo("alias2"));
         }
-        {
-            GetAliasesRequest getAliasesRequest = new GetAliasesRequest("alias1");
-            getAliasesRequest.replaceAliases(Strings.EMPTY_ARRAY);
-            ImmutableOpenMap<String, List<AliasMetaData>> aliases = metaData.findAliases(getAliasesRequest, new String[]{"index"});
-            assertThat(aliases.size(), equalTo(0));
-        }
         {
             ImmutableOpenMap<String, List<AliasMetaData>> aliases =
                 metaData.findAliases(new GetAliasesRequest("alias*"), new String[]{"index"});

x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authz/IndicesAndAliasesResolver.java

@@ -46,9 +46,9 @@ import static org.elasticsearch.xpack.core.security.authz.IndicesAndAliasesResol
 
 class IndicesAndAliasesResolver {
 
-    //`*,-*` what we replace indices with if we need Elasticsearch to return empty responses without throwing exception
+    //`*,-*` what we replace indices and aliases with if we need Elasticsearch to return empty responses without throwing exception
-    private static final String[] NO_INDICES_ARRAY = new String[] { "*", "-*" };
+    static final String[] NO_INDICES_OR_ALIASES_ARRAY = new String[] { "*", "-*" };
-    static final List<String> NO_INDICES_LIST = Arrays.asList(NO_INDICES_ARRAY);
+    static final List<String> NO_INDICES_OR_ALIASES_LIST = Arrays.asList(NO_INDICES_OR_ALIASES_ARRAY);
 
     private final IndexNameExpressionResolver nameExpressionResolver;
     private final RemoteClusterResolver remoteClusterResolver;
@@ -165,7 +165,7 @@ class IndicesAndAliasesResolver {
                     //this is how we tell es core to return an empty response, we can let the request through being sure
                     //that the '-*' wildcard expression will be resolved to no indices. We can't let empty indices through
                     //as that would be resolved to _all by es core.
-                    replaceable.indices(NO_INDICES_ARRAY);
+                    replaceable.indices(NO_INDICES_OR_ALIASES_ARRAY);
                     indicesReplacedWithNoIndices = true;
                     resolvedIndicesBuilder.addLocal(NO_INDEX_PLACEHOLDER);
                 } else {
@@ -176,8 +176,6 @@ class IndicesAndAliasesResolver {
             }
         } else {
             if (containsWildcards(indicesRequest)) {
-                //an alias can still contain '*' in its name as of 5.0. Such aliases cannot be referred to when using
-                //the security plugin, otherwise the following exception gets thrown
                 throw new IllegalStateException("There are no external requests known to support wildcards that don't support replacing " +
                         "their indices");
             }
@@ -198,8 +196,6 @@ class IndicesAndAliasesResolver {
             if (aliasesRequest.expandAliasesWildcards()) {
                 List<String> aliases = replaceWildcardsWithAuthorizedAliases(aliasesRequest.aliases(),
                         loadAuthorizedAliases(authorizedIndices.get(), metaData));
-                //it may be that we replace aliases with an empty array, in case there are no authorized aliases for the action.
-                //MetaData#findAliases will return nothing when some alias was originally requested, which was replaced with empty.
                 aliasesRequest.replaceAliases(aliases.toArray(new String[aliases.size()]));
             }
             if (indicesReplacedWithNoIndices) {
@@ -213,6 +209,13 @@ class IndicesAndAliasesResolver {
             } else {
                 resolvedIndicesBuilder.addLocal(aliasesRequest.aliases());
             }
+            // if no aliases are authorized, then fill in an expression that
+            // MetaData#findAliases evaluates to the empty alias list. You cannot put
+            // "nothing" (the empty list) explicitly because this is resolved by es core to
+            // _all
+            if (aliasesRequest.aliases().length == 0) {
+                aliasesRequest.replaceAliases(NO_INDICES_OR_ALIASES_ARRAY);
+            }
         }
         return resolvedIndicesBuilder.build();
     }

x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/authz/AuthorizationServiceTests.java

@@ -818,7 +818,7 @@ public class AuthorizationServiceTests extends ESTestCase {
         final SearchRequest searchRequest = new SearchRequest("_all");
         authorize(authentication, SearchAction.NAME, searchRequest);
         assertEquals(2, searchRequest.indices().length);
-        assertEquals(IndicesAndAliasesResolver.NO_INDICES_LIST, Arrays.asList(searchRequest.indices()));
+        assertEquals(IndicesAndAliasesResolver.NO_INDICES_OR_ALIASES_LIST, Arrays.asList(searchRequest.indices()));
     }
 
     public void testGrantedNonXPackUserCanExecuteMonitoringOperationsAgainstSecurityIndex() {

x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/authz/IndicesAndAliasesResolverTests.java

@@ -778,11 +778,11 @@ public class IndicesAndAliasesResolverTests extends ESTestCase {
     public void testResolveAliasesWildcardsIndicesAliasesRequestDeleteActionsNoAuthorizedIndices() {
         IndicesAliasesRequest request = new IndicesAliasesRequest();
         request.addAliasAction(AliasActions.remove().index("foo*").alias("foo*"));
-        //no authorized aliases match bar*, hence aliases are replaced with empty string for that action
+        //no authorized aliases match bar*, hence aliases are replaced with no-aliases-expression for that action
         request.addAliasAction(AliasActions.remove().index("*bar").alias("bar*"));
         resolveIndices(request, buildAuthorizedIndices(user, IndicesAliasesAction.NAME));
         assertThat(request.getAliasActions().get(0).aliases(), arrayContainingInAnyOrder("foofoobar", "foobarfoo"));
-        assertThat(request.getAliasActions().get(1).aliases().length, equalTo(0));
+        assertThat(request.getAliasActions().get(1).aliases(), arrayContaining(IndicesAndAliasesResolver.NO_INDICES_OR_ALIASES_ARRAY));
     }
 
     public void testResolveWildcardsIndicesAliasesRequestAddAndDeleteActions() {
@@ -1084,11 +1084,11 @@ public class IndicesAndAliasesResolverTests extends ESTestCase {
 
     public void testResolveAliasesWildcardsGetAliasesRequestNoAuthorizedIndices() {
         GetAliasesRequest request = new GetAliasesRequest();
-        //no authorized aliases match bar*, hence aliases are replaced with empty array
+        //no authorized aliases match bar*, hence aliases are replaced with the no-aliases-expression
         request.aliases("bar*");
         request.indices("*bar");
         resolveIndices(request, buildAuthorizedIndices(user, GetAliasesAction.NAME));
-        assertThat(request.aliases().length, equalTo(0));
+        assertThat(request.aliases(), arrayContaining(IndicesAndAliasesResolver.NO_INDICES_OR_ALIASES_ARRAY));
     }
 
     public void testResolveAliasesExclusionWildcardsGetAliasesRequest() {
@@ -1112,10 +1112,11 @@ public class IndicesAndAliasesResolverTests extends ESTestCase {
             request.aliases("_all");
         }
         request.indices("non_existing");
-        //current user is not authorized for any index, foo* resolves to no indices, aliases are replaced with empty array
+        //current user is not authorized for any index, aliases are replaced with the no-aliases-expression
         ResolvedIndices resolvedIndices = resolveIndices(request, buildAuthorizedIndices(userNoIndices, GetAliasesAction.NAME));
         assertThat(resolvedIndices.getLocal(), contains("non_existing"));
-        assertThat(request.aliases().length, equalTo(0));
+        assertThat(Arrays.asList(request.indices()), contains("non_existing"));
+        assertThat(request.aliases(), arrayContaining(IndicesAndAliasesResolver.NO_INDICES_OR_ALIASES_ARRAY));
     }
 
     /**
@@ -1372,7 +1373,7 @@ public class IndicesAndAliasesResolverTests extends ESTestCase {
         final List<String> localIndices = resolvedIndices.getLocal();
         assertEquals(1, localIndices.size());
         assertEquals(IndicesAndAliasesResolverField.NO_INDEX_PLACEHOLDER, localIndices.iterator().next());
-        assertEquals(IndicesAndAliasesResolver.NO_INDICES_LIST, Arrays.asList(request.indices()));
+        assertEquals(IndicesAndAliasesResolver.NO_INDICES_OR_ALIASES_LIST, Arrays.asList(request.indices()));
         assertEquals(0, resolvedIndices.getRemote().size());
     }