Remove aliases resolution limitations when security is enabled

[javanna]

Resolving wildcards in aliases expression is challenging as we may end
up with no aliases to replace the original expression with, but if we
replace with an empty array that means _all which is quite the opposite.
Now that we support and serialize the original requested aliases,
whenever aliases are replaced we will be able to know what was
initially requested. MetaData#findAliases can then be updated to not
return anything in case it gets empty aliases, but the original aliases
were not empty. That means that empty aliases are interpreted as _all
only if they were originally requested that way.

Relates to #31516

[elasticmachine]

Pinging @elastic/es-security

[javanna]

I think that I need to add a note to the breaking changes list for xpack, but I am not sure where the list is located.

[jaymode]

LGTM. @lcawl do you know where the x-pack breaking changes for 7.0 get documented?

[jaymode]

can you add javadocs for this method?

[jaymode]

👍 the list gets smaller and smaller

[lcawl]

The X-Pack-related breaking changes are now integrated in the product-specific documentation (e.g. https://www.elastic.co/guide/en/elasticsearch/reference/master/breaking-changes-7.0.html)

[javanna]

I am still not sure where I should add this note. @lcawl could you please point me to the file that I need to add this breaking change to? Thanks!

[lcawl]

@javanna I suggest https://github.com/elastic/elasticsearch/edit/master/docs/reference/migration/migrate_7_0/api.asciidoc

[javanna]

thanks @lcawl for the pointers! would you mind double checking the note that I added?

[lcawl]

I recommend using "{security}" instead of "xpack security", since the X-Pack terminology is changing in the near future.

[lcawl]

In my opinion "vanilla" might be confusing, since X-Pack is included in Elasticsearch by default now. Maybe something like this might be clearer?: "The behavior and response codes of the get aliases API no longer vary depending on whether {security} is enabled. Previously..."

[javanna]

agreed, that sounds great, thank you.

[lcawl]

LGTM, thanks!

[albertzaharovits]

@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?

[javanna]

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.

[albertzaharovits]

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?

[albertzaharovits]

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!

[javanna]

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.