Add include_data_streams flag for authorization #58154

danhermann · 2020-06-16T00:29:56Z

Most of the work around the addition of an includeDataStreams flag was done in #58381. The remainder of this PR adds the flag to the appropriate request classes, updates IndexNameExpressionResolver to rely on the flag rather than a separate boolean flag, and updates tests.

This PR adds an includeDataStreams flag to the IndicesRequest interface so that it is available when AuthorizationEngine::loadAuthorizedIndices is called. This is necessary to avoid different behaviors when security is enabled and disabled.

The disparate behavior is described in the referenced issue. When a GET */_alias request (or any other request that does not operate on data streams) is sent without security enabled, any data streams present are ignored by the IndexNameExpressionResolver class and the correct HTTP 200 response is returned.

When security is enabled, the star in GET */_alias is first resolved to all authorized indices, aliases, and data streams. Because the _alias endpoint does not understand data streams, the first data stream it encounters among the list of authorized indices is treated as an alias and an incorrect 404 for "unknown alias" is returned.

This PR changes AuthorizationService to selectively exclude (in the case of most requests) or include (and the case of ResolveIndexAction.Request and other requests that opt in) data streams during the authorized index resolution phase.

~~Especially interested in feedback from @elastic/es-security.~~

Fixes #57712

Relates to #53100

elasticmachine · 2020-06-16T00:29:59Z

Pinging @elastic/es-core-features (:Core/Features/Data streams)

danhermann · 2020-06-16T01:10:13Z

I'll resolve the test failures. I'm still interested in feedback on the general approach.

martijnvg

The general approach looks good to me. A number of other requests classes
would need to implement this method as well.

I think we should also make sure that the includeDataStreams() method is used in
IndexNameExpressionResolver#concreteIndexNames(...) and IndexNameExpressionResolver#concreteIndices(...) then the overloaded method that
also accepts includeDataStreams boolean parameter can be removed.

martijnvg · 2020-06-16T08:20:06Z

server/src/main/java/org/elasticsearch/action/IndicesRequest.java

+        /**
+         * Determines whether the action should be applied to data streams
+         */
+        default boolean includeDataStreams() {


I think we can move this method to IndicesRequest?

danhermann · 2020-06-16T19:27:29Z

@elasticmachine run elasticsearch-ci/bwc

danhermann · 2020-06-16T19:48:29Z

@elasticmachine update branch

danhermann · 2020-06-16T21:25:30Z

@elasticmachine run elasticsearch-ci/default-distro

martijnvg

I left some comments.

martijnvg · 2020-06-17T06:02:22Z

server/src/main/java/org/elasticsearch/action/IndicesRequest.java

+     * Determines whether the action should be applied to data streams
+     */
+    default boolean includeDataStreams() {
+        return false;


I think we should change the default here, because most indices based action will be supporting data streams.

Let's keep the default to false. After all apis that need to support data stream resolution have been changed to do that, we can revisit what the default should be.

martijnvg · 2020-06-17T06:20:43Z

rest-api-spec/src/main/resources/rest-api-spec/test/indices.data_stream/20_unsupported_apis.yml

@@ -65,11 +65,6 @@
      indices.create:
        index: logs-foobarbaz

-  - do:


why is this removed?

I'll mute that portion of the test for now since that one call has different behavior when security is enabled or not. Once this PR and #57900 land, its behavior will be the same.

martijnvg · 2020-06-17T07:22:49Z

server/src/main/java/org/elasticsearch/action/search/TransportSearchAction.java

@@ -457,8 +457,8 @@ private void executeLocalSearch(Task task, SearchTimeProvider timeProvider, Sear
        if (localIndices == null) {
            return Index.EMPTY_ARRAY; //don't search on any local index (happens when only remote indices were specified)
        }
-        return indexNameExpressionResolver.concreteIndices(clusterState, indicesOptions, true,
-            timeProvider.getAbsoluteStartMillis(), localIndices.indices());
+        return indexNameExpressionResolver.concreteIndices(clusterState, indicesOptions, localIndices,


I think we should change this resolveLocalIndices() method here. I think we should change the indicesOptions parameter to request (and pass down the request from where it is being invoked).

Then change the concreteIndices() method this method invokes. Remove the options and indexExpressions parameters and add request parameter (of type IndicesRequest). Then
in the body of this method derive the indicesOptions and indices from the request parameter.

albertzaharovits · 2020-06-18T18:40:23Z

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

-                indicesAndAliases.add(aliasOrIndex);
+                if (entry.getValue().getType() != IndexAbstraction.Type.DATA_STREAM || includeDataStreams) {
+                    indicesAndAliases.add(aliasOrIndex);
+                }
            }


I gave it more thought than it probably warrants.

Overall I am OK with the change, but I would suggest we move this whole logic( if (entry.getValue().getType() != IndexAbstraction.Type.DATA_STREAM || includeDataStreams) {) to

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

Line 113 in 2925056

final ResolvedIndices.Builder resolvedIndicesBuilder = new ResolvedIndices.Builder();

This is the place where we deal with the entities (open/close indices and aliases) behind the "authorised" names, and I believe it would help us to keep complexity under control in an already messy spaghetti.

Would moving the logic to IndicesAndAliasesResolver still achieve the desired result? The problem is that all requests that implement IndicesRequest.Replaceable have their requested indices replaced with the result of the loadAuthorizedIndices call and this causes problems for the requests such as GET /_alias that do not (and should not) operate on data streams.

What I mean is to move the whole processing on loadAuthorizedIndices, as you've proposed, over to the method in IndicesAndAliasesResolver. I think the outcome is the same, but it feels to me like a better place for these kind of processing on index names based on requests. Makes sense?

Ah, thanks for the clarification. I will make that change.

albertzaharovits

I have left a style suggestion that I feel kinda strongly about.
If you agree to follow it, I don't believe another review round is necessary.

martijnvg

LGTM

danhermann · 2020-06-26T18:24:53Z

This is likely to be included in #58381 since the two concerns are tightly related.

danhermann added >non-issue :Security/Authorization Roles, Privileges, DLS/FLS, RBAC/ABAC v8.0.0 :Data Management/Data streams Data streams and their lifecycles v7.9.0 labels Jun 16, 2020

danhermann requested review from martijnvg and henningandersen June 16, 2020 00:29

elasticmachine added the Team:Security Meta label for security team label Jun 16, 2020

elasticmachine added the Team:Data Management Meta label for data/management team label Jun 16, 2020

martijnvg reviewed Jun 16, 2020

View reviewed changes

danhermann mentioned this pull request Jun 16, 2020

Ignore matching data streams if include_data_streams is false #57900

Merged

martijnvg reviewed Jun 17, 2020

View reviewed changes

tvernum requested a review from albertzaharovits June 18, 2020 08:01

albertzaharovits reviewed Jun 18, 2020

View reviewed changes

albertzaharovits approved these changes Jun 18, 2020

View reviewed changes

martijnvg approved these changes Jun 23, 2020

View reviewed changes

danhermann force-pushed the include_ds_flag_for_authorization branch from d11c6c9 to 279f4f4 Compare July 2, 2020 18:29

add include_data_streams flag for authorization

9125043

danhermann force-pushed the include_ds_flag_for_authorization branch from 9950d0a to 9125043 Compare July 3, 2020 03:00

fix test

3bd06a8

danhermann merged commit e592a9a into elastic:master Jul 3, 2020

danhermann added the backport pending label Jul 3, 2020

danhermann added a commit to danhermann/elasticsearch that referenced this pull request Jul 3, 2020

Add include_data_streams flag for authorization (elastic#58154)

01cfe20

danhermann mentioned this pull request Jul 3, 2020

[7.x] Add include_data_streams flag for authorization #59008

Merged

danhermann deleted the include_ds_flag_for_authorization branch July 3, 2020 17:06

danhermann removed the backport pending label Jan 27, 2021

jakelandis added v8.0.0-alpha1 and removed v8.0.0 labels Jul 26, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add include_data_streams flag for authorization #58154

Add include_data_streams flag for authorization #58154

danhermann commented Jun 16, 2020 •

edited

Loading

elasticmachine commented Jun 16, 2020

danhermann commented Jun 16, 2020

martijnvg left a comment

martijnvg Jun 16, 2020 •

edited

Loading

danhermann commented Jun 16, 2020

danhermann commented Jun 16, 2020

danhermann commented Jun 16, 2020

martijnvg left a comment

martijnvg Jun 17, 2020

martijnvg Jun 17, 2020

martijnvg Jun 17, 2020

danhermann Jun 17, 2020

martijnvg Jun 17, 2020

albertzaharovits Jun 18, 2020 •

edited

Loading

danhermann Jun 18, 2020

albertzaharovits Jun 18, 2020

danhermann Jun 18, 2020

albertzaharovits left a comment

martijnvg left a comment

danhermann commented Jun 26, 2020

Add include_data_streams flag for authorization #58154

Add include_data_streams flag for authorization #58154

Conversation

danhermann commented Jun 16, 2020 • edited Loading

elasticmachine commented Jun 16, 2020

danhermann commented Jun 16, 2020

martijnvg left a comment

Choose a reason for hiding this comment

martijnvg Jun 16, 2020 • edited Loading

Choose a reason for hiding this comment

danhermann commented Jun 16, 2020

danhermann commented Jun 16, 2020

danhermann commented Jun 16, 2020

martijnvg 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 Jun 18, 2020 • edited Loading

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 left a comment

Choose a reason for hiding this comment

martijnvg left a comment

Choose a reason for hiding this comment

danhermann commented Jun 26, 2020

danhermann commented Jun 16, 2020 •

edited

Loading

martijnvg Jun 16, 2020 •

edited

Loading

albertzaharovits Jun 18, 2020 •

edited

Loading