[Security] Get Alias API wildcard exclusion with Security #34144

albertzaharovits · 2018-09-28T14:52:38Z

This builds on #33518 .
Wildcard index expressions are tricky because when security is enabled,
widcards (inclusions and exclusions) have to be expanded in the scope
of the authenticated user.

Relates: #33805

EDIT:
The security plugin authorizes actions on indices. This implies that a
request with the indices expression containing wildcards has to be
first evaluated, in the authorization layer, before the request is
handled. For authorization purposes, wildcards in expressions will
only be expanded to indices visible by the authenticated user.
However, this "constrained" evaluation has to be compatible with
the expression evaluation that a cluster without the security plugin
installed would do. Therefore every change in the evaluation logic
in any of the to sites has to be mirrored in the other.

#33518 added support forwildcard exclusion (-...*) at the core logic
site. This PR mirrors that at the IndicesAndAliasesResolver site,
which is the site that does the affore mentioned "constrained" expression
evaluation.

elasticmachine · 2018-09-28T14:52:40Z

Pinging @elastic/es-core-infra

javanna

I left some questions

javanna · 2018-10-01T08:56:52Z

server/src/main/java/org/elasticsearch/rest/action/admin/indices/RestGetAliasesAction.java

@@ -77,9 +74,6 @@ public String getName() {

    @Override
    public RestChannelConsumer prepareRequest(final RestRequest request, final NodeClient client) throws IOException {
-        // The TransportGetAliasesAction was improved do the same post processing as is happening here.
-        // We can't remove this logic yet to support mixed clusters. We should be able to remove this logic here
-        // in when 8.0 becomes the new version in the master branch.


why remove this comment ?

It is not clear to me what post processing is handled here as well as in the transport action.

In the transport action, TransportGetAliasesAction#postProcess handles the cases when the get aliases request is of the form /{index}/_alias, when no alias is specified in the request, only index names.
While the code here deals with requests of the form /_alias/{name} or /{index}/_alias/{name}, taking care that only requested aliases are printed (indicesToDisplay and namesProvided) and any wildcard expressions which have not expanded to any aliases are called out in a 404 message.

The comment is not so clear I agree, I believe it has to do with this: #29538 (comment) . In the linked PR we have made the result returned from transport client more similar to the one at REST, with the addition of empty entries for requested indices when no specific aliases were requested. Such thing was previously only happening at REST, now it happens in transport, but I believe it also still happens at REST for bw comp reasons. The idea behind the comment was "watch out, you may think something is redundant here, and ideally the post processing would happen in one place only, but it still happens in two places for bw comp reasons". Do you see what I mean? We should have done a better job at highlighting what piece of code is specifically redundant and we will be able to remove in the future. I am not sold on just removing the comment.

And yes, the code for this API is quite complicated and hard to follow.

Okay, I understand the history behind it, and I will update the comments in between the code lines.

Such thing was previously only happening at REST, now it happens in transport, but I believe it also still happens at REST for bw comp reasons.

In the REST layer I see that

elasticsearch/server/src/main/java/org/elasticsearch/rest/action/admin/indices/RestGetAliasesAction.java

Line 104 in d25e3ef

if (namesProvided) {

is working to remove entries that have been added in the transport. So I agree, there is an interplay here. Again, given the conflicting requirements I think it's better not to push all this logic on the transport layer (or suggest it in the comments).

javanna · 2018-10-01T08:57:15Z

server/src/main/java/org/elasticsearch/rest/action/admin/indices/RestGetAliasesAction.java

@@ -89,56 +83,59 @@ public RestChannelConsumer prepareRequest(final RestRequest request, final NodeC
        getAliasesRequest.indicesOptions(IndicesOptions.fromRequest(request, getAliasesRequest.indicesOptions()));
        getAliasesRequest.local(request.paramAsBoolean("local", getAliasesRequest.local()));

-        //we may want to move this logic to TransportGetAliasesAction but it is based on the original provided aliases, which will
-        //not always be available there (they may get replaced so retrieving request.aliases is not quite the same).


why remove this comment?

This ties in with the other comments.

I think the RestHandler here tries to identify which of the many URl patterns have been invoked. For example, if the URI had index names, make sure those index names are printed even if they have no aliases. On the other hand, if aliases were requested (maybe alongside indices), than make sure all the requested aliases are returned and no empty indices without aliases will be returned.

I don't agree that the TransportAction should handle this types of corner cases. I think the TransportAction should return a minimal set of aliases (no extraneous placeholders) and the REST handler should package it with empty or null placeholders as well as to do any existence checks that are required to meet each individual REST spec.

TransportGetAliasesAction but it is based on the original provided aliases, which will not always be available there (they may get replaced so retrieving request.aliases is not quite the same).

GetAliasesRequest#originalAliases contains the original provided aliases, but again I don't think a single transport action should handle several different rest actions, especially when those rest actions have conflicting requirements, like for example the inclusion of indices with no aliases.

In fewer words, I think the logic is best were it stands, in the rest layer.

The other comment above is more about "we have duplicated logic which should be in one place, but there's a good reason". This comment is more about whether some parts of the REST action should be moved to the transport action. I generally prefer rest actions with almost no logic, but that has to do with trying to keep rest responses and transport client responses the same and have REST layer only doing the printing. With the transport client being deprecated, this is less and less important.

The reason why this comment is there is because we have tried in the past to move all the logic to the transport action, but that did not quite work, also it made bw compatibility hard. We have added this comment to remember especially that request.aliases may get replaced, which is a tricky thing that can be easily missed. originalAliases were not there yet when the comment was written which means that it's a good time now to re-evaluate if this comment is still valid.

In general, this API does quite unique things, I always found it weird that the REST layer had more information about the request than transport (till originalAliases were added to the request) and I had envisioned that one day maybe it would become more similar to the other API, with logic in the transport action and a very simple REST action. But I see how this comment does not add much value at this point, so I am fine with it being removed. Maybe we should re-evaluate what changes we want to make long-term to this API in a different PR though. See also #30536 .

javanna · 2018-10-01T08:58:51Z

server/src/main/java/org/elasticsearch/rest/action/admin/indices/RestGetAliasesAction.java

                            indicesToDisplay.add(cursor.key);
                        }
+                        returnedAliasNames.add(aliasMetaData.alias());


no substantial change was made up until here, or am I missing something?

Yes, no substantial changes until here.

javanna · 2018-10-01T09:01:23Z

server/src/main/java/org/elasticsearch/rest/action/admin/indices/RestGetAliasesAction.java

+                        if (false == returnedAliasNames.contains(aliases[i])) {
+                            missingAliases.add(aliases[i]);
+                        }
+                    }


I am not following what changes were made here, can you maybe explain? I was expecting changes needed only in IndicesAndAliasesResolver to add support for pattern exclusion, why do we need to make changes to the transport action too?

I am not following what changes were made here, can you maybe explain?

Sorry, I've been rushing it on Friday. I will add another comment explaining the complete changes in this PR, as well as trying to explain this hereunder.

In the present code, GetAlias is returning 404 if any alias pattern in the comma delimited list in the request does not match some returned alias. In other words, if a wildcard expression in the request did not return any alias, 404 will be returned, alongside a message indicating the culprit pattern(s).
The proposed change here tries to preserver some of this behavior.
In the proposed change, when exclude wildcard expressions (-...*) are allowed, an include wildcard expression can expand to aliases that are removed from the final result by subsequent exclude wildcards, even if the excluding wildcard is a subset of the including one, in the general case.
For example, given the request expression foo*,-foobar* and the foobar1 and foobar2 aliases the returned result will be empty, and the current code will return 404 with "aliases [foo*,-foobar*] missing". The proposed change will return 200.

The proposed change will return 404 if an explicitly requested alias that is not excluded by any subsequent exclude wildcard is not returned as a response. It will not return a 404 for wildcards.

what you explain sounds like a bug in the current wildcard exclusion implementation, besides the fact that security does not support pattern exclusions. Is that correct?

Yes it is a bug in the current implementation. #33518 should have added these bits as well.

ok can we please split this into two PRs then? One for addressing the bug in the original implementation, and the new tests, the other one for adapting the security resolution code? That would make reviewing much easier.

Okay, will do!

javanna

LGTM thanks @albertzaharovits

albertzaharovits · 2018-10-03T16:55:56Z

The failure
https://elasticsearch-ci.elastic.co/job/elastic+elasticsearch+pull-request/1235/console
looks like a fluke, I cannot reproduce it locally and I cannot fathom how the changes
herein could have such a ripple effect. I'll be pushing a master merge 🤞

The Security plugin authorizes actions on indices. Authorization happens on a per index/alias basis. Therefore a request with a Multi Index Expression (containing wildcards) has to be first evaluated in the authorization layer, before the request is handled. For authorization purposes, wildcards in expressions will only be expanded to indices/aliases that are visible by the authenticated user. However, this "constrained" evaluation has to be compatible with the expression evaluation that a cluster without the Security plugin would do. Therefore any change in the evaluation logic in any of these sites has to be mirrored in the other site. This commit mirrors the changes in core from #33518 that allowed for Multi Index Expression in the Get Alias API, loosely speaking.

* master: (25 commits) HLRC: ML Adding get datafeed stats API (elastic#34271) Small fixes to the HLRC watcher documentation. (elastic#34306) Tasks: Document that status is not semvered (elastic#34270) HLRC: ML Add preview datafeed api (elastic#34284) [CI] Fix bogus ScheduleWithFixedDelayTests.testRunnableRunsAtMostOnceAfterCancellation Fix error in documentation for activete watch SCRIPTING: Terms set query expression (elastic#33856) Logging: Drop remaining Settings log ctor (elastic#34149) [ML] Remove unused last_data_time member from Job (elastic#34262) Docs: Allow skipping response assertions (elastic#34240) HLRC: Add activate watch action (elastic#33988) [Security] Multi Index Expression alias wildcard exclusion (elastic#34144) [ML] Label anomalies with multi_bucket_impact (elastic#34233) Document smtp.ssl.trust configuration option (elastic#34275) Support PKCS#11 tokens as keystores and truststores (elastic#34063) Fix sporadic failure in NestedObjectMapperTests [Authz] Allow update settings action for system user (elastic#34030) Replace version with reader cache key in IndicesRequestCache (elastic#34189) [TESTS] Set SO_LINGER and SO_REUSEADDR on the mock socket (elastic#34211) Security: upgrade unboundid ldapsdk to 4.0.8 (elastic#34247) ...

* rename-ccr-stats: (25 commits) HLRC: ML Adding get datafeed stats API (elastic#34271) Small fixes to the HLRC watcher documentation. (elastic#34306) Tasks: Document that status is not semvered (elastic#34270) HLRC: ML Add preview datafeed api (elastic#34284) [CI] Fix bogus ScheduleWithFixedDelayTests.testRunnableRunsAtMostOnceAfterCancellation Fix error in documentation for activete watch SCRIPTING: Terms set query expression (elastic#33856) Logging: Drop remaining Settings log ctor (elastic#34149) [ML] Remove unused last_data_time member from Job (elastic#34262) Docs: Allow skipping response assertions (elastic#34240) HLRC: Add activate watch action (elastic#33988) [Security] Multi Index Expression alias wildcard exclusion (elastic#34144) [ML] Label anomalies with multi_bucket_impact (elastic#34233) Document smtp.ssl.trust configuration option (elastic#34275) Support PKCS#11 tokens as keystores and truststores (elastic#34063) Fix sporadic failure in NestedObjectMapperTests [Authz] Allow update settings action for system user (elastic#34030) Replace version with reader cache key in IndicesRequestCache (elastic#34189) [TESTS] Set SO_LINGER and SO_REUSEADDR on the mock socket (elastic#34211) Security: upgrade unboundid ldapsdk to 4.0.8 (elastic#34247) ...

The Security plugin authorizes actions on indices. Authorization happens on a per index/alias basis. Therefore a request with a Multi Index Expression (containing wildcards) has to be first evaluated in the authorization layer, before the request is handled. For authorization purposes, wildcards in expressions will only be expanded to indices/aliases that are visible by the authenticated user. However, this "constrained" evaluation has to be compatible with the expression evaluation that a cluster without the Security plugin would do. Therefore any change in the evaluation logic in any of these sites has to be mirrored in the other site. This commit mirrors the changes in core from #33518 that allowed for Multi Index Expression in the Get Alias API, loosely speaking.

albertzaharovits added 6 commits September 20, 2018 17:55

WIP main code

f66d163

Main code

fc0a185

GetAlias 404 for requested indices

348a2e1

Darn indicesToDisplay BS

7e29895

Get Alias wildcard rest-api tests

ed2ceba

Almost all tests pass

f711b91

albertzaharovits added >bug review :Data Management/Indices APIs APIs to create and manage indices and templates v7.0.0 v6.5.0 labels Sep 28, 2018

albertzaharovits self-assigned this Sep 28, 2018

albertzaharovits requested review from javanna and jaymode September 28, 2018 14:52

All IndicesAndAliasesResolverTests done!

ca58af3

javanna reviewed Oct 1, 2018

View reviewed changes

Version guards for mixed cluster IT

6f5f2cb

albertzaharovits mentioned this pull request Oct 2, 2018

Get Aliases with wildcard exclusion expression #34230

Merged

Revert changes delegated to elastic#34230

9c1e3da

albertzaharovits mentioned this pull request Oct 3, 2018

Revise get aliases response #30536

Open

Merge branch 'master' into fix-get-alias

27fc7eb

javanna approved these changes Oct 3, 2018

View reviewed changes

Merge branch 'master' into fix-get-alias

209b4be

Merge branch 'master' into fix-get-alias

6086bea

albertzaharovits merged commit f817bc5 into elastic:master Oct 4, 2018

albertzaharovits deleted the fix-get-alias branch October 4, 2018 08:19

albertzaharovits mentioned this pull request Oct 5, 2018

Pattern exclusion not working properly on aliases when security is enabled #33805

Closed

colings86 added v7.0.0-beta1 and removed v7.0.0 labels Feb 7, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Security] Get Alias API wildcard exclusion with Security #34144

[Security] Get Alias API wildcard exclusion with Security #34144

albertzaharovits commented Sep 28, 2018 •

edited

elasticmachine commented Sep 28, 2018

javanna left a comment

javanna Oct 1, 2018

albertzaharovits Oct 1, 2018

javanna Oct 1, 2018

albertzaharovits Oct 1, 2018

javanna Oct 1, 2018

albertzaharovits Oct 1, 2018 •

edited

albertzaharovits Oct 1, 2018

javanna Oct 1, 2018

javanna Oct 1, 2018

albertzaharovits Oct 1, 2018

javanna Oct 1, 2018

albertzaharovits Oct 1, 2018 •

edited

javanna Oct 1, 2018

albertzaharovits Oct 1, 2018

javanna Oct 1, 2018

albertzaharovits Oct 1, 2018

javanna left a comment

albertzaharovits commented Oct 3, 2018

[Security] Get Alias API wildcard exclusion with Security #34144

[Security] Get Alias API wildcard exclusion with Security #34144

Conversation

albertzaharovits commented Sep 28, 2018 • edited

elasticmachine commented Sep 28, 2018

javanna left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

albertzaharovits Oct 1, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

albertzaharovits Oct 1, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

javanna left a comment

Choose a reason for hiding this comment

albertzaharovits commented Oct 3, 2018

albertzaharovits commented Sep 28, 2018 •

edited

albertzaharovits Oct 1, 2018 •

edited

albertzaharovits Oct 1, 2018 •

edited