Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Changes to make PIT security model granular #2053

Closed
Original file line number Diff line number Diff line change
Expand Up @@ -66,6 +66,7 @@
import org.opensearch.Version;
import org.opensearch.action.ActionRequest;
import org.opensearch.action.ActionResponse;
import org.opensearch.action.search.PitService;
import org.opensearch.action.search.SearchScrollAction;
import org.opensearch.action.support.ActionFilter;
import org.opensearch.client.Client;
Expand Down Expand Up @@ -1160,13 +1161,15 @@ public static class GuiceHolder implements LifecycleComponent {
private static RepositoriesService repositoriesService;
private static RemoteClusterService remoteClusterService;
private static IndicesService indicesService;
private static PitService pitService;

@Inject
public GuiceHolder(final RepositoriesService repositoriesService,
final TransportService remoteClusterService, IndicesService indicesService) {
final TransportService remoteClusterService, IndicesService indicesService, PitService pitService) {
GuiceHolder.repositoriesService = repositoriesService;
GuiceHolder.remoteClusterService = remoteClusterService.getRemoteClusterService();
GuiceHolder.indicesService = indicesService;
GuiceHolder.pitService = pitService;
}

public static RepositoriesService getRepositoriesService() {
Expand All @@ -1180,6 +1183,10 @@ public static RemoteClusterService getRemoteClusterService() {
public static IndicesService getIndicesService() {
return indicesService;
}

public static PitService getPitService() {
return pitService;
}

@Override
public void close() {
Expand Down

Large diffs are not rendered by default.

Original file line number Diff line number Diff line change
Expand Up @@ -130,6 +130,7 @@ public class PrivilegesEvaluator {
private final SecurityIndexAccessEvaluator securityIndexAccessEvaluator;
private final ProtectedIndexAccessEvaluator protectedIndexAccessEvaluator;
private final TermsAggregationEvaluator termsAggregationEvaluator;
private final PitPrivilegesEvaluator pitPrivilegesEvaluator;
private final boolean dlsFlsEnabled;
private final boolean dfmEmptyOverwritesAll;
private DynamicConfigModel dcm;
Expand Down Expand Up @@ -158,6 +159,7 @@ public PrivilegesEvaluator(final ClusterService clusterService, final ThreadPool
securityIndexAccessEvaluator = new SecurityIndexAccessEvaluator(settings, auditLog, irr);
protectedIndexAccessEvaluator = new ProtectedIndexAccessEvaluator(settings, auditLog);
termsAggregationEvaluator = new TermsAggregationEvaluator();
pitPrivilegesEvaluator = new PitPrivilegesEvaluator();
this.namedXContentRegistry = namedXContentRegistry;
this.dlsFlsEnabled = dlsFlsEnabled;
this.dfmEmptyOverwritesAll = settings.getAsBoolean(ConfigConstants.SECURITY_DFM_EMPTY_OVERRIDES_ALL, false);
Expand Down Expand Up @@ -282,6 +284,12 @@ public PrivilegesEvaluatorResponse evaluate(final User user, String action0, fin
return presponse;
}

// check access for point in time requests
if(pitPrivilegesEvaluator.evaluate(request, clusterService, user, securityRoles,
action0, resolver, dcm.isDnfofForEmptyResultsEnabled(), presponse).isComplete()) {
return presponse;
}

final boolean dnfofEnabled = dcm.isDnfofEnabled();

final boolean isTraceEnabled = log.isTraceEnabled();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -370,11 +370,11 @@ public final static class Resolved {
private final boolean isLocalAll;
private final IndicesOptions indicesOptions;

private Resolved(final ImmutableSet<String> aliases,
final ImmutableSet<String> allIndices,
final ImmutableSet<String> originalRequested,
final ImmutableSet<String> remoteIndices,
IndicesOptions indicesOptions) {
public Resolved(final ImmutableSet<String> aliases,
final ImmutableSet<String> allIndices,
final ImmutableSet<String> originalRequested,
final ImmutableSet<String> remoteIndices,
IndicesOptions indicesOptions) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please avoid the format changes if not necessary.

this.aliases = aliases;
this.allIndices = allIndices;
this.originalRequested = originalRequested;
Expand Down
8 changes: 5 additions & 3 deletions src/main/resources/static_config/static_action_groups.yml
Original file line number Diff line number Diff line change
Expand Up @@ -233,8 +233,10 @@ manage_point_in_time:
static: true
allowed_actions:
- "indices:data/read/point_in_time/create"
- "cluster:admin/point_in_time/delete"
- "cluster:admin/point_in_time/read*"
- "indices:data/read/point_in_time/delete"
- "indices:data/read/point_in_time/readall"
- "indices:data/read/search"
- "cluster:admin/point_in_time/read_from_nodes"

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, for my understanding what is read_from_nodes?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the parent action that basically reads all active Point in time searches from nodes.

This the action for NodesGetAllPitAction.

We can rename it as well, if the name is confusing.

- "indices:monitor/point_in_time/segments"
type: "cluster"
type: "index"
description: "Manage point in time actions"
176 changes: 176 additions & 0 deletions src/test/java/org/opensearch/security/PitIntegrationTests.java
Original file line number Diff line number Diff line change
@@ -0,0 +1,176 @@
/*
bharath-techie marked this conversation as resolved.
Show resolved Hide resolved
* Copyright 2015-2018 _floragunn_ GmbH
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/

/*
* SPDX-License-Identifier: Apache-2.0
*
* The OpenSearch Contributors require contributions made to
* this file be licensed under the Apache-2.0 license or a
* compatible open source license.
*
* Modifications Copyright OpenSearch Contributors. See
* GitHub history for details.
*/
package org.opensearch.security;

import org.apache.http.HttpStatus;
import org.junit.Assert;
import org.junit.Test;

import org.opensearch.action.index.IndexRequest;
import org.opensearch.action.support.WriteRequest;
import org.opensearch.client.Client;
import org.opensearch.common.xcontent.XContentType;
import org.opensearch.security.test.SingleClusterTest;
import org.opensearch.security.test.helper.rest.RestHelper;

/**
* Integration tests to test point in time APIs permission model
*/
public class PitIntegrationTests extends SingleClusterTest {

@Test
public void pitCreateWithDeleteAll() throws Exception {
setup();
RestHelper rh = nonSslRestHelper();

// Create two indices
try (Client tc = getClient()) {
tc.index(new IndexRequest("pit_1").id("1").setRefreshPolicy(WriteRequest.RefreshPolicy.IMMEDIATE).
source("{\"content\":1}", XContentType.JSON)).actionGet();
tc.index(new IndexRequest("pit_2").id("2").setRefreshPolicy(WriteRequest.RefreshPolicy.IMMEDIATE).
source("{\"content\":2}", XContentType.JSON)).actionGet();
}

RestHelper.HttpResponse resc;

// Create point in time in index should be successful since the user has permission for index
resc = rh.executePostRequest("/pit_1/_search/point_in_time?keep_alive=100m", "",
encodeBasicHeader("pit-1", "nagilum"));
Assert.assertEquals(HttpStatus.SC_OK, resc.getStatusCode());

// Create point in time in index for which the user does not have permission
resc = rh.executePostRequest("/pit_2/_search/point_in_time?keep_alive=100m", "",
encodeBasicHeader("pit-1", "nagilum"));
Assert.assertEquals(HttpStatus.SC_FORBIDDEN, resc.getStatusCode());

// Create point in time in index for which the user has permission for
resc = rh.executePostRequest("/pit_2/_search/point_in_time?keep_alive=100m", "",
encodeBasicHeader("pit-2", "nagilum"));
Assert.assertEquals(HttpStatus.SC_OK, resc.getStatusCode());

// Delete all PITs should work since there is atleast one PIT for which user has access for
resc = rh.executeDeleteRequest("/_search/point_in_time/_all",
encodeBasicHeader("pit-1", "nagilum"));
Assert.assertEquals(HttpStatus.SC_OK, resc.getStatusCode());

// Delete all PITs should throw error since there are no PITs for which the user has access for
resc = rh.executeDeleteRequest("/_search/point_in_time/_all",
encodeBasicHeader("pit-1", "nagilum"));
Assert.assertEquals(HttpStatus.SC_FORBIDDEN, resc.getStatusCode());

// Delete all PITs should work since there is atleast one PIT for which user has access for
resc = rh.executeDeleteRequest("/_search/point_in_time/_all",
encodeBasicHeader("pit-2", "nagilum"));
Assert.assertEquals(HttpStatus.SC_OK, resc.getStatusCode());

// Delete all PITs throws forbidden error since there are no PITs in the cluster
resc = rh.executeDeleteRequest("/_search/point_in_time/_all",
encodeBasicHeader("pit-1", "nagilum"));
Assert.assertEquals(HttpStatus.SC_FORBIDDEN, resc.getStatusCode());
}

@Test
public void pitCreateWithGetAll() throws Exception {
setup();
RestHelper rh = nonSslRestHelper();

// Create two indices
try (Client tc = getClient()) {
tc.index(new IndexRequest("pit_1").id("1").setRefreshPolicy(WriteRequest.RefreshPolicy.IMMEDIATE).
source("{\"content\":1}", XContentType.JSON)).actionGet();
tc.index(new IndexRequest("pit_2").id("2").setRefreshPolicy(WriteRequest.RefreshPolicy.IMMEDIATE).
source("{\"content\":2}", XContentType.JSON)).actionGet();
}

RestHelper.HttpResponse resc;

// Create point in time in index should be successful since the user has permission for index
resc = rh.executePostRequest("/pit_1/_search/point_in_time?keep_alive=100m", "",
encodeBasicHeader("pit-1", "nagilum"));
Assert.assertEquals(HttpStatus.SC_OK, resc.getStatusCode());
bharath-techie marked this conversation as resolved.
Show resolved Hide resolved

// Create point in time in index for which the user does not have permission
resc = rh.executePostRequest("/pit_2/_search/point_in_time?keep_alive=100m", "",
encodeBasicHeader("pit-1", "nagilum"));
Assert.assertEquals(HttpStatus.SC_FORBIDDEN, resc.getStatusCode());

// Create point in time in index for which the user has permission for
resc = rh.executePostRequest("/pit_2/_search/point_in_time?keep_alive=100m", "",
encodeBasicHeader("pit-2", "nagilum"));
Assert.assertEquals(HttpStatus.SC_OK, resc.getStatusCode());

// List all PITs should work since there is atleast one PIT for which user has access for
resc = rh.executeGetRequest("/_search/point_in_time/_all",
encodeBasicHeader("pit-1", "nagilum"));
Assert.assertEquals(HttpStatus.SC_OK, resc.getStatusCode());

// PIT segments should work since there is atleast one PIT for which user has access for
resc = rh.executeGetRequest("/_cat/pit_segments/_all",
encodeBasicHeader("pit-1", "nagilum"));
Assert.assertEquals(HttpStatus.SC_OK, resc.getStatusCode());

// Delete all PITs should work since there is atleast one PIT for which user has access for
resc = rh.executeDeleteRequest("/_search/point_in_time/_all",
encodeBasicHeader("pit-1", "nagilum"));
Assert.assertEquals(HttpStatus.SC_OK, resc.getStatusCode());


// List all PITs should throw error since there are no PITs for which the user has access for
resc = rh.executeGetRequest("/_search/point_in_time/_all",
encodeBasicHeader("pit-1", "nagilum"));
Assert.assertEquals(HttpStatus.SC_FORBIDDEN, resc.getStatusCode());

// PIT segments should throw error since there are PITs in system but no PIT for which user has access for
resc = rh.executeGetRequest("/_cat/pit_segments/_all",
encodeBasicHeader("pit-1", "nagilum"));
Assert.assertEquals(HttpStatus.SC_FORBIDDEN, resc.getStatusCode());

// List all PITs should work since there is atleast one PIT for which user has access for
resc = rh.executeGetRequest("/_search/point_in_time/_all",
encodeBasicHeader("pit-2", "nagilum"));
Assert.assertEquals(HttpStatus.SC_OK, resc.getStatusCode());

// PIT segments should work since there is atleast one PIT for which user has access for
resc = rh.executeGetRequest("/_cat/pit_segments/_all",
encodeBasicHeader("pit-2", "nagilum"));
Assert.assertEquals(HttpStatus.SC_OK, resc.getStatusCode());

// Delete all PITs should work since there is atleast one PIT for which user has access for
resc = rh.executeDeleteRequest("/_search/point_in_time/_all",
encodeBasicHeader("pit-2", "nagilum"));
Assert.assertEquals(HttpStatus.SC_OK, resc.getStatusCode());

// List all PITs throws forbidden error since there are no PITs in the cluster
resc = rh.executeGetRequest("/_search/point_in_time/_all",
encodeBasicHeader("pit-1", "nagilum"));
Assert.assertEquals(HttpStatus.SC_FORBIDDEN, resc.getStatusCode());

// PIT segments API throws forbidden error since there are no PITs in the cluster
resc = rh.executeGetRequest("/_search/point_in_time/_all",
encodeBasicHeader("pit-1", "nagilum"));
Assert.assertEquals(HttpStatus.SC_FORBIDDEN, resc.getStatusCode());
}
}
6 changes: 6 additions & 0 deletions src/test/resources/internal_users.yml
Original file line number Diff line number Diff line change
Expand Up @@ -346,6 +346,12 @@ ds2:
ds3:
hash: $2a$12$n5nubfWATfQjSYHiWtUyeOxMIxFInUHOAx8VMmGmxFNPGpaBmeB.m
#password is: nagilum
pit-1:
hash: $2a$12$n5nubfWATfQjSYHiWtUyeOxMIxFInUHOAx8VMmGmxFNPGpaBmeB.m
#password is: nagilum
pit-2:
hash: $2a$12$n5nubfWATfQjSYHiWtUyeOxMIxFInUHOAx8VMmGmxFNPGpaBmeB.m
#password is: nagilum
hidden_test:
hash: $2a$12$n5nubfWATfQjSYHiWtUyeOxMIxFInUHOAx8VMmGmxFNPGpaBmeB.m
opendistro_security_roles:
Expand Down
30 changes: 30 additions & 0 deletions src/test/resources/roles.yml
Original file line number Diff line number Diff line change
Expand Up @@ -1128,7 +1128,37 @@ data_stream_3:
- "*"
allowed_actions:
- "DATASTREAM_ALL"
point_in_time_1:
reserved: true
hidden: false
description: "Migrated from v6 (all types mapped)"
cluster_permissions:
- "cluster:admin/point_in_time/read_from_nodes"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am concerned about adding cluster level permission it blurs the line between the index based permissions and PIT which are tightly coupled to indices, I know there is some discussion around it and I'd like to focus that discussion separately from the rest of this work to help get this PR merged more quickly - is that possible?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Without cluster level permission we'd not be able to get all PITs from the cluster, as the request doesn't have indices as part of it. Any suggestions ?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a place where all of the different Rest APIs are documented in the form below? I would like to be able to cross-reference the actions and associated permissions that are needed to perform them, I think that might help our understanding.

Create point in time

POST "/<target indices>/point_in_time?*keep_alive*=1h&routing=&expand_wildcards=&preference=" 

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

opensearch-project/OpenSearch#3960 -- is the design doc

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The doc and the comments on this PR have evolved over time and it is not clear to me what the permission setup, user action, and result should be for the many scenarios embodied - please make this as clear as possible.

Not only would this help us understand the specific scope and bounds of your request, this would be of great value for writing the end-user facing permissions documentation for this feature

- "cluster_monitor"
index_permissions:
- index_patterns:
- "pit_1"
dls: null
fls: null
masked_fields: null
allowed_actions:
- "manage_point_in_time"

point_in_time_2:
reserved: true
hidden: false
description: "Migrated from v6 (all types mapped)"
cluster_permissions:
- "cluster:admin/point_in_time/read_from_nodes"
- "cluster_monitor"
index_permissions:
- index_patterns:
- "pit_2"
dls: null
fls: null
masked_fields: null
allowed_actions:
- "manage_point_in_time"
hidden_test:
cluster_permissions:
- SGS_CLUSTER_COMPOSITE_OPS
Expand Down
10 changes: 10 additions & 0 deletions src/test/resources/roles_mapping.yml
Original file line number Diff line number Diff line change
Expand Up @@ -413,6 +413,16 @@ data_stream_3:
hidden: false
users:
- "ds3"
point_in_time_1:
reserved: false
hidden: false
users:
- "pit-1"
point_in_time_2:
reserved: false
hidden: false
users:
- "pit-2"
sem-role:
reserved: false
hidden: false
Expand Down