Skip to content

Commit

Permalink
Empty GetAliases authorization fix (#34444)
Browse files Browse the repository at this point in the history 
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.
  • Loading branch information
@albertzaharovits @kcm
albertzaharovits authored and kcm committed Oct 30, 2018
1 parent 1fcfb84 commit 75665ab
Show file tree
Hide file tree
Showing 6 changed files with 57 additions and 53 deletions.
51 changes: 21 additions & 30 deletions server/src/main/java/org/elasticsearch/cluster/metadata/MetaData.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -249,53 +249,45 @@ public SortedMap<String, AliasOrIndex> getAliasAndIndexLookup() {
}

/**
* 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.
* @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
* present for that index
* @param concreteIndices The concrete indices that the aliases must point to in order to be returned.
* @return A map of index name to the list of aliases metadata. If a concrete index does not have matching
* aliases then the result will <b>not</b> include the index's key.
*/
public ImmutableOpenMap<String, List<AliasMetaData>> findAllAliases(String[] concreteIndices) {
return findAliases(Strings.EMPTY_ARRAY, Strings.EMPTY_ARRAY, concreteIndices);
public ImmutableOpenMap<String, List<AliasMetaData>> findAllAliases(final String[] concreteIndices) {
return findAliases(Strings.EMPTY_ARRAY, concreteIndices);
}

/**
* 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.
* Finds the specific index aliases that match with the specified aliases directly or partially via wildcards, and
* 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.
* @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
* present for that index
* @param concreteIndices The concrete indices that the aliases must point to in order to be returned.
* @return A map of index name to the list of aliases metadata. If a concrete index does not have matching
* aliases then the result will <b>not</b> include the index's key.
*/
public ImmutableOpenMap<String, List<AliasMetaData>> findAliases(final AliasesRequest aliasesRequest, String[] concreteIndices) {
return findAliases(aliasesRequest.getOriginalAliases(), aliasesRequest.aliases(), concreteIndices);
public ImmutableOpenMap<String, List<AliasMetaData>> findAliases(final AliasesRequest aliasesRequest, final String[] concreteIndices) {
return findAliases(aliasesRequest.aliases(), concreteIndices);
}

/**
* 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.
* Finds the specific index aliases that match with the specified aliases directly or partially via wildcards, and
* that point to the specified concrete indices (directly or matching indices via wildcards).
*
* @param aliases The aliases to look for
* @param originalAliases The original aliases that the user originally requested
* @param concreteIndices The concrete indexes the index aliases must point to 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
* present for that index
* @param aliases The aliases to look for. Might contain include or exclude wildcards.
* @param concreteIndices The concrete indices that the aliases must point to in order to be returned
* @return A map of index name to the list of aliases metadata. If a concrete index does not have matching
* aliases then the result will <b>not</b> include the index's key.
*/
private ImmutableOpenMap<String, List<AliasMetaData>> findAliases(String[] originalAliases, String[] aliases,
String[] concreteIndices) {
private ImmutableOpenMap<String, List<AliasMetaData>> findAliases(final String[] aliases, final 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++) {
Expand Down Expand Up @@ -331,7 +323,6 @@ private ImmutableOpenMap<String, List<AliasMetaData>> findAliases(String[] origi
filteredValues.add(value);
}
}

if (filteredValues.isEmpty() == false) {
// Make the list order deterministic
CollectionUtil.timSort(filteredValues, Comparator.comparing(AliasMetaData::alias));
Expand Down
9 changes: 8 additions & 1 deletion server/src/test/java/org/elasticsearch/cluster/metadata/IndexCreationTaskTests.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,7 @@
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;
Expand Down Expand Up @@ -82,7 +83,7 @@
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);
Expand Down Expand Up @@ -149,6 +150,12 @@ public void testApplyDataFromRequest() throws Exception {
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");
Expand Down
14 changes: 8 additions & 6 deletions server/src/test/java/org/elasticsearch/cluster/metadata/MetaDataTests.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -66,19 +66,21 @@ public void testFindAliases() {
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");
assertThat(aliasMetaDataList.size(), equalTo(2));
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"});
Expand Down
19 changes: 11 additions & 8 deletions ...urity/src/main/java/org/elasticsearch/xpack/security/authz/IndicesAndAliasesResolver.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -46,9 +46,9 @@

class IndicesAndAliasesResolver {

//`*,-*` what we replace indices with if we need Elasticsearch to return empty responses without throwing exception
private static final String[] NO_INDICES_ARRAY = new String[] { "*", "-*" };
static final List<String> NO_INDICES_LIST = Arrays.asList(NO_INDICES_ARRAY);
//`*,-*` what we replace indices and aliases with if we need Elasticsearch to return empty responses without throwing exception
static final String[] NO_INDICES_OR_ALIASES_ARRAY = new String[] { "*", "-*" };
static final List<String> NO_INDICES_OR_ALIASES_LIST = Arrays.asList(NO_INDICES_OR_ALIASES_ARRAY);

private final IndexNameExpressionResolver nameExpressionResolver;
private final RemoteClusterResolver remoteClusterResolver;
Expand Down Expand Up @@ -165,7 +165,7 @@ ResolvedIndices resolveIndicesAndAliases(IndicesRequest indicesRequest, MetaData
//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 {
Expand All @@ -176,8 +176,6 @@ ResolvedIndices resolveIndicesAndAliases(IndicesRequest indicesRequest, MetaData
}
} 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");
}
Expand All @@ -198,8 +196,6 @@ ResolvedIndices resolveIndicesAndAliases(IndicesRequest indicesRequest, MetaData
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) {
Expand All @@ -213,6 +209,13 @@ ResolvedIndices resolveIndicesAndAliases(IndicesRequest indicesRequest, MetaData
} 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();
}
Expand Down
2 changes: 1 addition & 1 deletion ...urity/src/test/java/org/elasticsearch/xpack/security/authz/AuthorizationServiceTests.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -818,7 +818,7 @@ public void testNonXPackUserCannotExecuteOperationAgainstSecurityIndex() {
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() {
Expand Down
15 changes: 8 additions & 7 deletions .../src/test/java/org/elasticsearch/xpack/security/authz/IndicesAndAliasesResolverTests.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -778,11 +778,11 @@ public void testResolveAllAliasesWildcardsIndicesAliasesRequestDeleteActions() {
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() {
Expand Down Expand Up @@ -1084,11 +1084,11 @@ public void testResolveAliasesWildcardsGetAliasesRequest() {

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() {
Expand All @@ -1112,10 +1112,11 @@ public void testResolveAliasesAllGetAliasesRequestNoAuthorizedIndices() {
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));
}

/**
Expand Down Expand Up @@ -1372,7 +1373,7 @@ private static void assertNoIndices(IndicesRequest.Replaceable request, Resolved
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());
}

Expand Down

0 comments on commit 75665ab

Please sign in to comment.