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
Original file line number Diff line number Diff line change
@@ -0,0 +1,167 @@
/*
* 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.privileges;

import java.util.ArrayList;
import java.util.Arrays;
import java.util.HashMap;
import java.util.HashSet;
import java.util.List;
import java.util.Map;
import java.util.Set;
import java.util.stream.Collectors;

import com.google.common.collect.ImmutableSet;
import org.apache.logging.log4j.LogManager;
import org.apache.logging.log4j.Logger;

import org.opensearch.action.ActionRequest;
import org.opensearch.action.admin.indices.segments.PitSegmentsRequest;
import org.opensearch.action.search.DeletePitRequest;
import org.opensearch.action.search.GetAllPitNodesRequest;
import org.opensearch.action.search.GetAllPitNodesResponse;
import org.opensearch.action.search.ListPitInfo;
import org.opensearch.action.search.SearchRequest;
import org.opensearch.cluster.metadata.IndexNameExpressionResolver;
import org.opensearch.cluster.service.ClusterService;
import org.opensearch.security.OpenSearchSecurityPlugin;
import org.opensearch.security.resolver.IndexResolverReplacer;
import org.opensearch.security.securityconf.SecurityRoles;
import org.opensearch.security.user.User;

/**
* This class evaluates privileges for point in time (Delete and List all) operations
*/
public class PitPrivilegesEvaluator {

protected final Logger log = LogManager.getLogger(this.getClass());
private boolean isDebugEnabled = log.isDebugEnabled();

public PrivilegesEvaluatorResponse evaluate(final ActionRequest request, final ClusterService clusterService,
final User user, final SecurityRoles securityRoles, final String action,
final IndexNameExpressionResolver resolver,
boolean dnfOfEmptyResultsEnabled, final PrivilegesEvaluatorResponse presponse) {

// Skip custom evaluation for "NodesGetAllPITs" action, since it fetches all PITs across the cluster
// for privilege evaluation - still this action will be evaluated in the generic PrivilegesEvaluator flow
if(action.startsWith("cluster:admin/point_in_time")) {
return presponse;
}
if (request instanceof GetAllPitNodesRequest) {
return handleGetAllPitsAccess(request, clusterService, user, securityRoles,
action, resolver, dnfOfEmptyResultsEnabled, presponse);
} else if (request instanceof DeletePitRequest) {
DeletePitRequest deletePitRequest = (DeletePitRequest) request;
return handleExplicitPitsAccess(deletePitRequest.getPitIds(), clusterService, user, securityRoles,
action, resolver, presponse);
} else if (request instanceof PitSegmentsRequest) {
PitSegmentsRequest pitSegmentsRequest = (PitSegmentsRequest) request;
return handleExplicitPitsAccess(pitSegmentsRequest.getPitIds(), clusterService, user, securityRoles,
action, resolver, presponse);
}
return presponse;
}

/**
* Handle access for Get All PITs access
*/
private PrivilegesEvaluatorResponse handleGetAllPitsAccess(final ActionRequest request, final ClusterService clusterService,
final User user, SecurityRoles securityRoles, final String action,
IndexNameExpressionResolver resolver,
boolean dnfOfEmptyResultsEnabled, PrivilegesEvaluatorResponse presponse) {
List<ListPitInfo> pitInfos = ((GetAllPitNodesRequest) request).getGetAllPitNodesResponse().getPitInfos();
// if cluster has no PITs, then allow the operation to pass with empty response if dnfOfEmptyResultsEnabled
// config property is true, otherwise fail the operation
if(pitInfos.isEmpty()) {
if(dnfOfEmptyResultsEnabled) {
presponse.allowed = true;
presponse.markComplete();
}
return presponse;
}
List<String> pitIds = new ArrayList<>();
pitIds.addAll(pitInfos.stream().map(ListPitInfo::getPitId).collect(Collectors.toList()));
Map<String, String[]> pitToIndicesMap = OpenSearchSecurityPlugin.GuiceHolder.getPitService().getIndicesForPits(pitIds);
Map<String, ListPitInfo> pitToPitInfoMap = new HashMap<>();

for(ListPitInfo pitInfo : pitInfos) {
pitToPitInfoMap.put(pitInfo.getPitId(), pitInfo);
}
List<ListPitInfo> permittedPits = new ArrayList<>();

Set<String> allPitIndices = new HashSet<>();
for(String[] indices: pitToIndicesMap.values()) {
allPitIndices.addAll(Arrays.asList(indices));
}
final Set<String> allPermittedPitIndices = getPermittedIndices(allPitIndices, clusterService, user,
securityRoles, action, resolver);

for (String pitId : pitIds) {
final String[] indices = pitToIndicesMap.get(pitId);
final HashSet<String> pitIndicesSet = new HashSet<>(Arrays.asList(indices));
if(isDebugEnabled) {
log.debug("Evaluating PIT ID : " + pitId );
}

if (allPermittedPitIndices.containsAll(pitIndicesSet)) {
if(isDebugEnabled) {
log.debug(" Permitting PIT ID : " + pitId);
}
permittedPits.add(pitToPitInfoMap.get(pitId));
}
}
if (permittedPits.size() > 0) {
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 reading this as if any Pits are allowed the request is authorized, this seems vulnerable bugs where a check is skipped. For sensitive operates granting access, inverting the check so only if no Pits are denied is the request allowed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We are making this whole change because we want the pit permission to be granular - only the allowed PITs are shown to the user ( by setting the permitted pits in request )

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So the pattern is similar to search. If user passes * to search api, then we show results from allowed indices, whereas if indices are explicitly passed to search api, then we'll make sure user permission is denied even if one index permission is not present.

Similarly when pit ids are explicitly passed, we do deny even if one pit is denied.

This logic is specific for * operation

Copy link
Member

Choose a reason for hiding this comment

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

By hiding the inaccessible PITs we are making it harder to understand what "ALL" means. While we are just getting this feature off the ground I'd like us to use the most restrictive framing of permissions that we could open up in future revisions.

I understand this change requires more work from the user configuring the cluster; however, I think adding a role like the following better communicates what is secured and what isn't to the operator of the cluster.

point_in_time_all_index_manage:
  index_permissions:
    - index_patterns:
        - "*"
      allowed_actions:
        - "manage_point_in_time"

((GetAllPitNodesRequest) request).setGetAllPitNodesResponse(new GetAllPitNodesResponse(permittedPits,
((GetAllPitNodesRequest) request).getGetAllPitNodesResponse()));
presponse.allowed = true;
presponse.markComplete();
}
return presponse;
}

/**
* Handle access for delete operation / pit segments operation where PIT IDs are explicitly passed
*/
private PrivilegesEvaluatorResponse handleExplicitPitsAccess(List<String> pitIds, ClusterService clusterService,
User user, SecurityRoles securityRoles, final String action,
IndexNameExpressionResolver resolver, PrivilegesEvaluatorResponse presponse) {
Map<String, String[]> pitToIndicesMap = OpenSearchSecurityPlugin.
GuiceHolder.getPitService().getIndicesForPits(pitIds);
Set<String> pitIndices = new HashSet<>();
// add indices across all PITs to a set and evaluate if user has access to all indices
for(String[] indices: pitToIndicesMap.values()) {
pitIndices.addAll(Arrays.asList(indices));
}
Set<String> allPermittedIndices = getPermittedIndices(pitIndices, clusterService, user,
securityRoles, action, resolver);
// In this case, PIT IDs are explicitly passed.
// So, only if user has access to all PIT's indices, allow delete operation, otherwise fail.
if(pitIndices.size() == allPermittedIndices.size()) {
presponse.allowed = true;
presponse.markComplete();
}
return presponse;
}

/**
* This method returns list of permitted indices for the PIT indices passed
*/
private Set<String> getPermittedIndices(Set<String> pitIndices, ClusterService clusterService,
User user, SecurityRoles securityRoles, final String action,
IndexNameExpressionResolver resolver) {
final ImmutableSet<String> pitImmutableIndices = ImmutableSet.copyOf(pitIndices);
final IndexResolverReplacer.Resolved pitResolved =
new IndexResolverReplacer.Resolved(pitImmutableIndices, pitImmutableIndices, pitImmutableIndices,
ImmutableSet.of(), SearchRequest.DEFAULT_INDICES_OPTIONS);
return securityRoles.reduce(pitResolved,
user, new String[]{action}, resolver, clusterService);
}
}
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"
Loading