Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Remove aliases resolution limitations when security is enabled #31952

Merged
merged 7 commits into from
Jul 20, 2018
Merged

Remove aliases resolution limitations when security is enabled #31952

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
94a75b1
Remove aliases resolution limitations when security is enabled
javanna Jul 10, 2018
d8553ab
adjust version conditionals
javanna Jul 10, 2018
58dfae5
Merge branch 'master' into security/remove_resolve_alias_limitations
javanna Jul 16, 2018
5e927d6
add javadocsi
javanna Jul 16, 2018
a6b5c78
revert some of the changes
javanna Jul 17, 2018
85f6c11
update docs
javanna Jul 17, 2018
b9925da
Merge branch 'master' into security/remove_resolve_alias_limitations
javanna Jul 18, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions server/src/main/java/org/elasticsearch/action/AliasesRequest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,8 @@ public interface AliasesRequest extends IndicesRequest.Replaceable {
*/
String[] aliases();

String[] getOriginalAliases();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can you add javadocs for this method?


/**
* Replaces current aliases with the provided aliases.
*
Expand Down
16 changes: 15 additions & 1 deletion server/src/main/java/org/elasticsearch/action/admin/indices/alias/IndicesAliasesRequest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -214,6 +214,7 @@ private static ObjectParser<AliasActions, Void> parser(String name, Supplier<Ali
private final AliasActions.Type type;
private String[] indices;
private String[] aliases = Strings.EMPTY_ARRAY;
private String[] originalAliases = Strings.EMPTY_ARRAY;
private String filter;
private String routing;
private String indexRouting;
Expand All @@ -238,6 +239,9 @@ public AliasActions(StreamInput in) throws IOException {
if (in.getVersion().onOrAfter(Version.V_6_4_0)) {
writeIndex = in.readOptionalBoolean();
}
if (in.getVersion().onOrAfter(Version.V_7_0_0_alpha1)) {
originalAliases = in.readStringArray();
}
}

@Override
Expand All @@ -252,6 +256,9 @@ public void writeTo(StreamOutput out) throws IOException {
if (out.getVersion().onOrAfter(Version.V_6_4_0)) {
out.writeOptionalBoolean(writeIndex);
}
if (out.getVersion().onOrAfter(Version.V_7_0_0_alpha1)) {
out.writeStringArray(originalAliases);
}
}

/**
Expand Down Expand Up @@ -315,6 +322,7 @@ public AliasActions aliases(String... aliases) {
}
}
this.aliases = aliases;
this.originalAliases = aliases;
return this;
}

Expand All @@ -329,6 +337,7 @@ public AliasActions alias(String alias) {
throw new IllegalArgumentException("[alias] can't be empty string");
}
this.aliases = new String[] {alias};
this.originalAliases = aliases;
return this;
}

Expand Down Expand Up @@ -432,6 +441,11 @@ public void replaceAliases(String... aliases) {
this.aliases = aliases;
}

@Override
public String[] getOriginalAliases() {
return originalAliases;
}

@Override
public boolean expandAliasesWildcards() {
//remove operations support wildcards among aliases, add operations don't
Expand Down Expand Up @@ -579,7 +593,7 @@ public XContentBuilder toXContent(XContentBuilder builder, Params params) throws
}, AliasActions.PARSER, new ParseField("actions"));
}

public static IndicesAliasesRequest fromXContent(XContentParser parser) throws IOException {
public static IndicesAliasesRequest fromXContent(XContentParser parser) {
return PARSER.apply(parser, null);
}
}
4 changes: 2 additions & 2 deletions ...main/java/org/elasticsearch/action/admin/indices/alias/TransportIndicesAliasesAction.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -95,7 +95,7 @@ protected void masterOperation(final IndicesAliasesRequest request, final Cluste
Set<String> aliases = new HashSet<>();
for (AliasActions action : actions) {
String[] concreteIndices = indexNameExpressionResolver.concreteIndexNames(state, request.indicesOptions(), action.indices());
Collections.addAll(aliases, action.aliases());
Collections.addAll(aliases, action.getOriginalAliases());
for (String index : concreteIndices) {
switch (action.actionType()) {
case ADD:
Expand Down Expand Up @@ -142,7 +142,7 @@ private static String[] concreteAliases(AliasActions action, MetaData metaData,
if (action.expandAliasesWildcards()) {
//for DELETE we expand the aliases
String[] indexAsArray = {concreteIndex};
ImmutableOpenMap<String, List<AliasMetaData>> aliasMetaData = metaData.findAliases(action.aliases(), indexAsArray);
ImmutableOpenMap<String, List<AliasMetaData>> aliasMetaData = metaData.findAliases(action, indexAsArray);
List<String> finalAliases = new ArrayList<>();
for (ObjectCursor<List<AliasMetaData>> curAliases : aliasMetaData.values()) {
for (AliasMetaData aliasMeta: curAliases.value) {
Expand Down
2 changes: 1 addition & 1 deletion ...main/java/org/elasticsearch/action/admin/indices/alias/get/TransportGetAliasesAction.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -63,7 +63,7 @@ protected GetAliasesResponse newResponse() {
@Override
protected void masterOperation(GetAliasesRequest request, ClusterState state, ActionListener<GetAliasesResponse> listener) {
String[] concreteIndices = indexNameExpressionResolver.concreteIndexNames(state, request);
ImmutableOpenMap<String, List<AliasMetaData>> aliases = state.metaData().findAliases(request.aliases(), concreteIndices);
ImmutableOpenMap<String, List<AliasMetaData>> aliases = state.metaData().findAliases(request, concreteIndices);
listener.onResponse(new GetAliasesResponse(postProcess(request, concreteIndices, aliases)));
}

Expand Down
5 changes: 2 additions & 3 deletions server/src/main/java/org/elasticsearch/action/admin/indices/get/TransportGetIndexAction.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,15 +32,14 @@
import org.elasticsearch.cluster.metadata.IndexNameExpressionResolver;
import org.elasticsearch.cluster.metadata.MappingMetaData;
import org.elasticsearch.cluster.service.ClusterService;
import org.elasticsearch.common.Strings;
import org.elasticsearch.common.collect.ImmutableOpenMap;
import org.elasticsearch.common.inject.Inject;
import org.elasticsearch.common.settings.IndexScopedSettings;
import org.elasticsearch.common.settings.Settings;
import org.elasticsearch.common.settings.SettingsFilter;
import org.elasticsearch.indices.IndicesService;
import org.elasticsearch.threadpool.ThreadPool;
import org.elasticsearch.transport.TransportService;
import org.elasticsearch.common.settings.IndexScopedSettings;

import java.io.IOException;
import java.util.List;
Expand Down Expand Up @@ -110,7 +109,7 @@ protected void doMasterOperation(final GetIndexRequest request, String[] concret
break;
case ALIASES:
if (!doneAliases) {
aliasesResult = state.metaData().findAliases(Strings.EMPTY_ARRAY, concreteIndices);
aliasesResult = state.metaData().findAllAliases(concreteIndices);
doneAliases = true;
}
break;
Expand Down
37 changes: 35 additions & 2 deletions server/src/main/java/org/elasticsearch/cluster/metadata/MetaData.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@
import com.carrotsearch.hppc.cursors.ObjectObjectCursor;
import org.apache.logging.log4j.Logger;
import org.apache.lucene.util.CollectionUtil;
import org.elasticsearch.action.AliasesRequest;
import org.elasticsearch.cluster.ClusterState;
import org.elasticsearch.cluster.ClusterState.FeatureAware;
import org.elasticsearch.cluster.Diff;
Expand Down Expand Up @@ -247,22 +248,54 @@ public SortedMap<String, AliasOrIndex> getAliasAndIndexLookup() {
return aliasAndIndexLookup;
}

/**
* Finds the specific index aliases that point to the specified concrete indices or match partially 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
*/
public ImmutableOpenMap<String, List<AliasMetaData>> findAllAliases(String[] concreteIndices) {
return findAliases(Strings.EMPTY_ARRAY, 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.
*
* @param aliases The names of the index aliases to find
* @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
*/
public ImmutableOpenMap<String, List<AliasMetaData>> findAliases(final AliasesRequest aliasesRequest, String[] concreteIndices) {
return findAliases(aliasesRequest.getOriginalAliases(), 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.
*
* @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
*/
public ImmutableOpenMap<String, List<AliasMetaData>> findAliases(final String[] aliases, String[] concreteIndices) {
private ImmutableOpenMap<String, List<AliasMetaData>> findAliases(String[] originalAliases, String[] aliases,
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();
}

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@javanna I think this is wrong.
If the originalAliases were empty and they got replaced by empty aliases because the user is not authorized for any, then he'll be seeing all of them.
Example:

# create index with 2 random aliases
curl -u elastic-admin:elastic-password -X PUT "localhost:9200/role-index-1" -H "Content-Type: application/json" -d'
{
  "aliases": {
    "some-other-alias": {},
    "some-alias": {}
  }
}
'
# create role that only authorizes on indices and not on any aliases
 curl -u elastic-admin:elastic-password -X POST "localhost:9200/_xpack/security/role/role" -H 'Content-Type: application/json' -d'
{
  "indices": [
    {
      "names": [ "role-index-*" ],
      "privileges": [ "all" ]
    }
  ]
}
'
# assign user to role
curl -u elastic-admin:elastic-password -X POST "localhost:9200/_xpack/security/user/user" -H 'Content-Type: application/json' -d'
{
  "password" : "elastic",
  "roles" : [ "role" ]
}
'
# all aliases are being returned, although only for authorized indices
curl -u user:elastic -X GET "localhost:9200/_alias/*?pretty"
{
  "role-index-1" : {
    "aliases" : {
      "some-other-alias" : { },
      "some-alias" : { }
    }
  }
}
# non-empty aliases list returns an empty response (in the broken way that it does, but it is empty)
curl -u user:elastic -X GET "localhost:9200/_alias/some-*?pretty"
{
  "error" : "alias [some-*] missing",
  "status" : 404
}

I think this should be: if (aliases.length == 0) {
What do you think?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ops, you raise a pretty bad issue. I think that if (aliases.length == 0) { would be too strict, as we would assume that empty always means "none" (the following boolean matchAllAliases = patterns.length == 0; would never be true in that case. The problem is that empty may mean "none" or "all", and it turns out that looking at the original aliases helps in some cases but not always. Maybe we should have some kind of placeholder like we do with indices (Some expression that cannot be used as an alias) to indicate that we should resolve to none, so we can distinguish between these two scenarios.

Copy link
Contributor

@albertzaharovits albertzaharovits Oct 11, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe we should have some kind of placeholder like we do with indices (Some expression that cannot be used as an alias) to indicate that we should resolve to none, so we can distinguish between these two scenarios.

I think that *, -* should work in 7.0 ?
'-' validation for alias name was added in 3787ea2 (5.1). I am assuming '-' alias names are still existent in 6.x due to bwc but we should migrate the names in 7.0 ? Do we ever enforce name changes for indices if the user ever does rolling upgrades?

Copy link
Contributor

@albertzaharovits albertzaharovits Oct 11, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I feel my previous comment requires some clarifications.

I am now certain that in 6.0 you are not allowed to create aliases starting with '-'.
The proof lies underneath:

elasticsearch/core/src/main/java/org/elasticsearch/cluster/metadata/MetaDataCreateIndexService.java

Line 176 in 7bb79ed

if (index.charAt(0) == '_' || index.charAt(0) == '-' || index.charAt(0) == '+') {

(A test might though be nice in MetaDataCreateIndexServiceTests, I will see to add one too)

Given that reindex requires first creating the index, and that any index created in < 5.1, when '-' was a valid char in the name, requires reindexing in 6.x before upgrading to 7.x, I think it is safe to assume '-' cannot be part of alias names in 7.x. Therefore *, -* works as a placeholder for no alias in 7.0 .

I will raise the PR soon!

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree @albertzaharovits I think this simplifies things, I had not realized that we backported the breaking change to 5.x too. Thanks for checking.

boolean matchAllAliases = matchAllAliases(aliases);
ImmutableOpenMap.Builder<String, List<AliasMetaData>> mapBuilder = ImmutableOpenMap.builder();
for (String index : concreteIndices) {
Expand Down
59 changes: 59 additions & 0 deletions server/src/test/java/org/elasticsearch/cluster/metadata/MetaDataTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@
package org.elasticsearch.cluster.metadata;

import org.elasticsearch.Version;
import org.elasticsearch.action.admin.indices.alias.get.GetAliasesRequest;
import org.elasticsearch.cluster.ClusterModule;
import org.elasticsearch.common.Strings;
import org.elasticsearch.common.UUIDs;
Expand All @@ -41,6 +42,7 @@
import java.io.IOException;
import java.util.HashMap;
import java.util.HashSet;
import java.util.List;
import java.util.Map;
import java.util.Set;

Expand All @@ -50,6 +52,63 @@

public class MetaDataTests extends ESTestCase {

public void testFindAliases() {
MetaData metaData = MetaData.builder().put(IndexMetaData.builder("index")
.settings(Settings.builder().put(IndexMetaData.SETTING_VERSION_CREATED, Version.CURRENT))
.numberOfShards(1)
.numberOfReplicas(0)
.putAlias(AliasMetaData.builder("alias1").build())
.putAlias(AliasMetaData.builder("alias2").build())).build();

{
ImmutableOpenMap<String, List<AliasMetaData>> aliases = metaData.findAliases(new GetAliasesRequest(), Strings.EMPTY_ARRAY);
assertThat(aliases.size(), equalTo(0));
}
{
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"});
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"));
}
{
ImmutableOpenMap<String, List<AliasMetaData>> aliases =
metaData.findAliases(new GetAliasesRequest("alias1"), new String[]{"index"});
assertThat(aliases.size(), equalTo(1));
List<AliasMetaData> aliasMetaDataList = aliases.get("index");
assertThat(aliasMetaDataList.size(), equalTo(1));
assertThat(aliasMetaDataList.get(0).alias(), equalTo("alias1"));
}
{
ImmutableOpenMap<String, List<AliasMetaData>> aliases = metaData.findAllAliases(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"));
}
{
ImmutableOpenMap<String, List<AliasMetaData>> aliases = metaData.findAllAliases(Strings.EMPTY_ARRAY);
assertThat(aliases.size(), equalTo(0));
}
}

public void testIndexAndAliasWithSameName() {
IndexMetaData.Builder builder = IndexMetaData.builder("index")
.settings(Settings.builder().put(IndexMetaData.SETTING_VERSION_CREATED, Version.CURRENT))
Expand Down
2 changes: 0 additions & 2 deletions x-pack/docs/en/security/limitations.asciidoc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,8 +19,6 @@ with {security} enabled.
Elasticsearch clusters with {security} enabled apply the `/_all` wildcard, and
all other wildcards, to the indices that the current user has privileges for, not
the set of all indices on the cluster.
While creating or retrieving aliases by providing wildcard expressions for alias names, if there are no existing authorized aliases
that match the wildcard expression provided an IndexNotFoundException is returned.

[float]
=== Multi Document APIs
Expand Down
20 changes: 4 additions & 16 deletions ...urity/src/main/java/org/elasticsearch/xpack/security/authz/IndicesAndAliasesResolver.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,6 @@
import org.elasticsearch.cluster.metadata.IndexNameExpressionResolver;
import org.elasticsearch.cluster.metadata.MetaData;
import org.elasticsearch.cluster.service.ClusterService;
import org.elasticsearch.common.Strings;
import org.elasticsearch.common.collect.ImmutableOpenMap;
import org.elasticsearch.common.regex.Regex;
import org.elasticsearch.common.settings.ClusterSettings;
Expand Down Expand Up @@ -200,6 +199,8 @@ 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 Down Expand Up @@ -240,8 +241,7 @@ static String getPutMappingIndexOrAlias(PutMappingRequest request, AuthorizedInd
} else {
// the user is not authorized to put mappings for this index, but could have been
// authorized for a write using an alias that triggered a dynamic mapping update
ImmutableOpenMap<String, List<AliasMetaData>> foundAliases =
metaData.findAliases(Strings.EMPTY_ARRAY, new String[] { concreteIndexName });
ImmutableOpenMap<String, List<AliasMetaData>> foundAliases = metaData.findAllAliases(new String[] { concreteIndexName });
List<AliasMetaData> aliasMetaData = foundAliases.get(concreteIndexName);
if (aliasMetaData != null) {
Optional<String> foundAlias = aliasMetaData.stream()
Expand Down Expand Up @@ -279,14 +279,12 @@ private List<String> replaceWildcardsWithAuthorizedAliases(String[] aliases, Lis
List<String> finalAliases = new ArrayList<>();

//IndicesAliasesRequest doesn't support empty aliases (validation fails) but GetAliasesRequest does (in which case empty means _all)
boolean matchAllAliases = aliases.length == 0;
if (matchAllAliases) {
if (aliases.length == 0) {
finalAliases.addAll(authorizedAliases);
}

for (String aliasPattern : aliases) {
if (aliasPattern.equals(MetaData.ALL)) {
matchAllAliases = true;
finalAliases.addAll(authorizedAliases);
} else if (Regex.isSimpleMatchPattern(aliasPattern)) {
for (String authorizedAlias : authorizedAliases) {
Expand All @@ -298,16 +296,6 @@ private List<String> replaceWildcardsWithAuthorizedAliases(String[] aliases, Lis
finalAliases.add(aliasPattern);
}
}

//Throw exception if the wildcards expansion to authorized aliases resulted in no indices.
//We always need to replace wildcards for security reasons, to make sure that the operation is executed on the aliases that we
//authorized it to execute on. Empty set gets converted to _all by es core though, and unlike with indices, here we don't have
//a special expression to replace empty set with, which gives us the guarantee that nothing will be returned.
//This is because existing aliases can contain all kinds of special characters, they are only validated since 5.1.
if (finalAliases.isEmpty()) {
String indexName = matchAllAliases ? MetaData.ALL : Arrays.toString(aliases);
throw new IndexNotFoundException(indexName);
}
return finalAliases;
}

Expand Down
Loading