Commands GET `cat/aliases` and POST `_rollover` will work on specified indices #3305

derek-ho · 2023-09-05T13:33:22Z

Description

Updates Get Aliases Request and Rollover to correctly look at the indices specified for these requests. This means that they will now work when a user has access to some specified set of indices, whereas before they would fail unless a user had permissions on all indices.

Issues Resolved

#1861

Is this a backport? If so, please add backport PR # and/or commits #

Testing

[Please provide details of testing done: unit testing, integration testing and manual testing]

Check List

New functionality includes testing
New functionality has been documented
Commits are signed per the DCO using --signoff

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

Signed-off-by: Derek Ho <[email protected]>

derek-ho · 2023-09-05T13:34:23Z

src/main/java/org/opensearch/security/privileges/PrivilegesEvaluator.java

@@ -108,7 +108,8 @@ public class PrivilegesEvaluator {
            "indices:admin/shards/search_shards",
            "indices:admin/resolve/index",
            "indices:monitor/settings/get",
-            "indices:monitor/stats"
+            "indices:monitor/stats",
+            "indices:admin/aliases/get"


It seems to me that the get aliases should also be a DNFOF like behavior - should I keep this in the PR and add a test for it?

codecov · 2023-09-05T13:38:12Z

Codecov Report

Merging #3305 (fc5f916) into main (0338cdd) will decrease coverage by 19.13%.
Report is 73 commits behind head on main.
The diff coverage is 42.85%.

@@              Coverage Diff              @@
##               main    #3305       +/-   ##
=============================================
- Coverage     63.15%   44.03%   -19.13%     
+ Complexity     3448     2754      -694     
=============================================
  Files           263      282       +19     
  Lines         20024    24271     +4247     
  Branches       3341     4517     +1176     
=============================================
- Hits          12647    10688     -1959     
- Misses         5748    12150     +6402     
+ Partials       1629     1433      -196

Files	Coverage Δ
...earch/security/privileges/PrivilegesEvaluator.java	`60.53% <ø> (-12.68%)`	⬇️
...earch/security/resolver/IndexResolverReplacer.java	`61.66% <42.85%> (-6.64%)`	⬇️

... and 182 files with indirect coverage changes

Signed-off-by: Derek Ho <[email protected]>

derek-ho · 2023-09-05T19:44:30Z

src/integrationTest/java/org/opensearch/security/DoNotFailOnForbiddenTests.java

+            assertThat(getAliasesResponse.getStatusLine().getStatusCode(), equalTo(200));
+            assertThat(aliases.size(), equalTo(1));
+            assertThat(aliases.get(0), containsString("marvelous_songs"));
+            assertThat(aliases.get(0), not(containsString("horrible_songs")));


@peternied @cwperks can you folks confirm this is the expected behavior (DNFOF returns alias even when user doesn't have index permissions on that alias when that alias contains an index that that user has access to (but the alias doesn't reveal any information about indices the user doesn't have access to))?

Index names themselves are considered protected by the access control systems, so an alias that exposes an index name would violate the access control checks. Therefore; Users see aliases for indices I they have access to see, and the inverse if the user does not have access to an index they should not see any aliases associated with it.

cwperks · 2023-09-06T14:47:43Z

src/main/java/org/opensearch/security/resolver/IndexResolverReplacer.java

-            String[] newIndices = provider.provide(((Replaceable) request).indices(), request, true);
-            if (checkIndices(request, newIndices, false, allowEmptyIndices) == false) {
+            String[] newIndicesOrAliases;
+            if (request instanceof GetAliasesRequest) {


I see that GetAliasesRequest implements AliasesRequest. Can this use the more generic AliasesRequest? What other AliasesRequests are there and would they be impacted by the same issue?

Looks like the only other AliasesRequest I could find is within the IndicesAliasRequest: https://github.com/opensearch-project/OpenSearch/blob/main/server/src/main/java/org/opensearch/action/admin/indices/alias/IndicesAliasesRequest.java#L100. From the description it is: A request to add/remove aliases for one or more indices.. IMO we can change this to instaceof AliasesRequest, wdyt?

AliasesRequests extends IndicesRequest.Replaceable which means an AliasesRequest is also an IndicesRequest. In that case would the provider.provide call need to consider both .aliases() and .indices() from the request object?

Is it possible that a GetAliasesRequest can contain .aliases() and .indices() at the same time?

+1 . Will there be a case where indices() and aliases() are present at the same time. Which should be given higher priority in that case or should they both be considered ?

peternied · 2023-09-10T17:39:15Z

src/integrationTest/java/org/opensearch/security/DoNotFailOnForbiddenTests.java

+    public void shouldPerformCatAliases_positive() throws IOException {
+        try (RestHighLevelClient restHighLevelClient = cluster.getRestHighLevelClient(LIMITED_USER)) {
+            Request getAliasesRequest = new Request("GET", "/_cat/aliases");
+            // High level client doesn't support _cat/_indices API


This comment isn't correct, let's remove it.

If you feel strongly that it is 'weird' this API isn't on the high-level client API, I'd recommend opening an issue.

DarshitChanpura · 2023-09-27T22:24:05Z

src/main/java/org/opensearch/security/resolver/IndexResolverReplacer.java

-            String[] newIndices = provider.provide(((Replaceable) request).indices(), request, true);
-            if (checkIndices(request, newIndices, false, allowEmptyIndices) == false) {
+            String[] newIndicesOrAliases;
+            if (request instanceof GetAliasesRequest) {


+1 . Will there be a case where indices() and aliases() are present at the same time. Which should be given higher priority in that case or should they both be considered ?

DarshitChanpura · 2023-09-27T22:34:01Z

src/main/java/org/opensearch/security/resolver/IndexResolverReplacer.java

@@ -781,6 +788,8 @@ private boolean getOrReplaceAllIndices(final Object request, final IndicesProvid
        } else if (request instanceof ReindexRequest) {
            result = getOrReplaceAllIndices(((ReindexRequest) request).getDestination(), provider, false) && result;
            result = getOrReplaceAllIndices(((ReindexRequest) request).getSearchRequest(), provider, false) && result;
+        } else if (request instanceof RolloverRequest) {


should admins be allowed to rollover all indices with * permission or should these be index pattern specific? I'm not aware of any risks in allowing admin to rollover all indices, but would like to confirm.

peternied · 2023-10-13T19:47:09Z

@derek-ho looks like this PR has stalled, would you like to keep working on it, or would you prefer to close it out?

peternied · 2023-10-13T19:48:36Z

Oops meant to hit comment, not close. I've reopened - let us know what you are thinking - thanks!

derek-ho · 2023-10-25T18:36:12Z

@peternied @DarshitChanpura closing this until I get more bandwidth to see it through to completion. Will keep the branch in its current state for now

derek-ho added 2 commits August 30, 2023 15:17

fix permissioning around get alias and rollovers

dc0a162

Signed-off-by: Derek Ho <[email protected]>

refactor and spotless

d20c602

Signed-off-by: Derek Ho <[email protected]>

derek-ho commented Sep 5, 2023

View reviewed changes

derek-ho changed the title ~~Fix alias rollover~~ Commands GET cat/aliases and POST _rollover will work on specified indices Sep 5, 2023

derek-ho marked this pull request as ready for review September 5, 2023 13:53

derek-ho requested review from cliu123, cwperks, DarshitChanpura, davidlago, peternied, RyanL1997, stephen-crawford, reta and willyborankin as code owners September 5, 2023 13:53

derek-ho added 2 commits September 5, 2023 15:41

add dnfof test for get aliases

02e9127

Signed-off-by: Derek Ho <[email protected]>

spotlessApply

fc5f916

Signed-off-by: Derek Ho <[email protected]>

derek-ho commented Sep 5, 2023

View reviewed changes

cwperks reviewed Sep 6, 2023

View reviewed changes

peternied reviewed Sep 10, 2023

View reviewed changes

DarshitChanpura reviewed Sep 27, 2023

View reviewed changes

peternied closed this Oct 13, 2023

peternied reopened this Oct 13, 2023

derek-ho closed this Oct 25, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Commands GET `cat/aliases` and POST `_rollover` will work on specified indices #3305

Commands GET `cat/aliases` and POST `_rollover` will work on specified indices #3305

derek-ho commented Sep 5, 2023 •

edited

Loading

derek-ho Sep 5, 2023

codecov bot commented Sep 5, 2023 •

edited

Loading

derek-ho Sep 5, 2023

peternied Sep 10, 2023

cwperks Sep 6, 2023

derek-ho Sep 7, 2023

cwperks Sep 7, 2023

DarshitChanpura Sep 27, 2023

peternied Sep 10, 2023

DarshitChanpura Sep 27, 2023

DarshitChanpura Sep 27, 2023

peternied commented Oct 13, 2023

peternied commented Oct 13, 2023

derek-ho commented Oct 25, 2023

Commands GET cat/aliases and POST _rollover will work on specified indices #3305

Commands GET cat/aliases and POST _rollover will work on specified indices #3305

Conversation

derek-ho commented Sep 5, 2023 • edited Loading

Description

Issues Resolved

Testing

Check List

Choose a reason for hiding this comment

codecov bot commented Sep 5, 2023 • edited Loading

Codecov Report

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

peternied commented Oct 13, 2023

peternied commented Oct 13, 2023

derek-ho commented Oct 25, 2023

Commands GET `cat/aliases` and POST `_rollover` will work on specified indices #3305

Commands GET `cat/aliases` and POST `_rollover` will work on specified indices #3305

derek-ho commented Sep 5, 2023 •

edited

Loading

codecov bot commented Sep 5, 2023 •

edited

Loading