Skip to content

Commit

Permalink
Command cat/indices will filter results per the Do Not Fail On Forb…
Browse files Browse the repository at this point in the history 
…idden setting (opensearch-project#3236)

This change allows for DNFOF behavior on the _cat/_indices API. It adds
the required index permissions into the DNFOF regex to be picked up in
the DNFOF code path. Previously it was being skipped/returning 403,
since the index permissions were not in the regex.

Fix: opensearch-project#1815

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

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

- [ ] 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](https://github.com/opensearch-project/OpenSearch/blob/main/CONTRIBUTING.md#developer-certificate-of-origin).

---------

Signed-off-by: Derek Ho <dxho@amazon.com>
(cherry picked from commit 4c095d2)
Signed-off-by: Derek Ho <dxho@amazon.com>
  • Loading branch information
@derek-ho
derek-ho committed Aug 29, 2023
1 parent 9ac5a04 commit 7faf072
Show file tree
Hide file tree
Showing 3 changed files with 120 additions and 7 deletions.
19 changes: 13 additions & 6 deletions src/main/java/org/opensearch/security/privileges/PrivilegesEvaluator.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -35,8 +35,8 @@
import java.util.Map;
import java.util.Set;
import java.util.StringJoiner;
import java.util.regex.Pattern;

import com.google.common.collect.ImmutableList;
import com.google.common.collect.ImmutableSet;
import org.apache.logging.log4j.LogManager;
import org.apache.logging.log4j.Logger;
Expand Down Expand Up @@ -101,12 +101,19 @@

public class PrivilegesEvaluator {

private static final WildcardMatcher ACTION_MATCHER = WildcardMatcher.from("indices:data/read/*search*");

private static final Pattern DNFOF_PATTERNS = Pattern.compile(
"indices:(data/read/.*|(admin/(mappings/fields/get.*|shards/search_shards|resolve/index)))"
static final WildcardMatcher DNFOF_MATCHER = WildcardMatcher.from(
ImmutableList.of(
"indices:data/read/*",
"indices:admin/mappings/fields/get*",
"indices:admin/shards/search_shards",
"indices:admin/resolve/index",
"indices:monitor/settings/get",
"indices:monitor/stats"
)
);

private static final WildcardMatcher ACTION_MATCHER = WildcardMatcher.from("indices:data/read/*search*");

private static final IndicesOptions ALLOW_EMPTY = IndicesOptions.fromOptions(true, true, false, false);

protected final Logger log = LogManager.getLogger(this.getClass());
Expand Down Expand Up @@ -474,7 +481,7 @@ public PrivilegesEvaluatorResponse evaluate(
}
}

if (dnfofEnabled && DNFOF_PATTERNS.matcher(action0).matches()) {
if (dnfofEnabled && DNFOF_MATCHER.test(action0)) {

if (requestedResolved.getAllIndices().isEmpty()) {
presponse.missingPrivileges.clear();
Expand Down
12 changes: 12 additions & 0 deletions src/test/java/org/opensearch/security/IntegrationTests.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -793,6 +793,18 @@ public void testDnfof() throws Exception {
(resc = rh.executeGetRequest("_all/_mapping/field/*", encodeBasicHeader("nagilum", "nagilum"))).getStatusCode()
);
System.out.println(resc.getBody());

Assert.assertEquals(
HttpStatus.SC_OK,
(resc = rh.executeGetRequest("_cat/_indices", encodeBasicHeader("user_a", "user_a"))).getStatusCode()
);
Assert.assertEquals(
resc,
(resc = rh.executeGetRequest("_cat/_indices", encodeBasicHeader("user_a", "user_a"))).getStatusCode()
);
Assert.assertTrue(resc.getBody(), resc.getBody().contains("indexa"));
Assert.assertFalse(resc.getBody(), resc.getBody().contains("indexb"));
System.out.println(resc.getBody());
}

@Test
Expand Down
96 changes: 95 additions & 1 deletion src/test/java/org/opensearch/security/privileges/PrivilegesEvaluatorUnitTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -8,14 +8,94 @@

package org.opensearch.security.privileges;

import com.google.common.collect.ImmutableList;
import org.junit.Test;

import java.util.List;

import static org.hamcrest.MatcherAssert.assertThat;
import static org.hamcrest.Matchers.equalTo;
import static org.junit.Assert.assertFalse;
import static org.junit.Assert.assertTrue;
import static org.opensearch.security.privileges.PrivilegesEvaluator.isClusterPerm;
import static org.opensearch.security.privileges.PrivilegesEvaluator.*;

public class PrivilegesEvaluatorUnitTest {

private static final List<String> allowedDnfof = ImmutableList.of(
"indices:admin/mappings/fields/get",
"indices:admin/resolve/index",
"indices:admin/shards/search_shards",
"indices:data/read/explain",
"indices:data/read/field_caps",
"indices:data/read/get",
"indices:data/read/mget",
"indices:data/read/msearch",
"indices:data/read/msearch/template",
"indices:data/read/mtv",
"indices:data/read/plugins/replication/file_chunk",
"indices:data/read/plugins/replication/changes",
"indices:data/read/scroll",
"indices:data/read/scroll/clear",
"indices:data/read/search",
"indices:data/read/search/template",
"indices:data/read/tv",
"indices:monitor/settings/get",
"indices:monitor/stats"
);

private static final List<String> disallowedDnfof = ImmutableList.of(
"indices:admin/aliases",
"indices:admin/aliases/exists",
"indices:admin/aliases/get",
"indices:admin/analyze",
"indices:admin/cache/clear",
"indices:admin/close",
"indices:admin/create",
"indices:admin/data_stream/create",
"indices:admin/data_stream/delete",
"indices:admin/data_stream/get",
"indices:admin/delete",
"indices:admin/exists",
"indices:admin/flush",
"indices:admin/forcemerge",
"indices:admin/get",
"indices:admin/mapping/put",
"indices:admin/mappings/get",
"indices:admin/open",
"indices:admin/plugins/replication/index/setup/validate",
"indices:admin/plugins/replication/index/start",
"indices:admin/plugins/replication/index/pause",
"indices:admin/plugins/replication/index/resume",
"indices:admin/plugins/replication/index/stop",
"indices:admin/plugins/replication/index/update",
"indices:admin/plugins/replication/index/status_check",
"indices:admin/refresh",
"indices:admin/rollover",
"indices:admin/seq_no/global_checkpoint_sync",
"indices:admin/settings/update",
"indices:admin/shrink",
"indices:admin/synced_flush",
"indices:admin/template/delete",
"indices:admin/template/get",
"indices:admin/template/put",
"indices:admin/types/exists",
"indices:admin/upgrade",
"indices:admin/validate/query",
"indices:data/write/bulk",
"indices:data/write/delete",
"indices:data/write/delete/byquery",
"indices:data/write/plugins/replication/changes",
"indices:data/write/index",
"indices:data/write/reindex",
"indices:data/write/update",
"indices:data/write/update/byquery",
"indices:monitor/data_stream/stats",
"indices:monitor/recovery",
"indices:monitor/segments",
"indices:monitor/shard_stores",
"indices:monitor/upgrade"
);

@Test
public void testClusterPerm() {
String multiSearchTemplate = "indices:data/read/msearch/template";
Expand All @@ -33,4 +113,18 @@ public void testClusterPerm() {
assertFalse(isClusterPerm(adminClose));
assertFalse(isClusterPerm(monitorUpgrade));
}

@Test
public void testDnfofPermissions_negative() {
for (final String permission : disallowedDnfof) {
assertThat(DNFOF_MATCHER.test(permission), equalTo(false));
}
}

@Test
public void testDnfofPermissions_positive() {
for (final String permission : allowedDnfof) {
assertThat(DNFOF_MATCHER.test(permission), equalTo(true));
}
}
}

0 comments on commit 7faf072

Please sign in to comment.