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.

Sign up for GitHub

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
wants to merge 8 commits into from
Closed

Changes to make PIT security model granular #2053

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
d5c482e
Changes to make PIT security model granular
bharath-techie Aug 28, 2022
953748f
Addressing comments , using dnfOfEnabled
bharath-techie Sep 5, 2022
dd830b4
Addressing comments , using dnfOfEmptyResultsEnabled
bharath-techie Sep 5, 2022
eeff45e
Changes to separate list all pits call from security plugin
bharath-techie Sep 6, 2022
4a8ad4c
Addressing comments
bharath-techie Sep 6, 2022
4380e77
Addressing comments
bharath-techie Sep 6, 2022
e440c35
Addressing comments - enhancing integ tests
bharath-techie Sep 6, 2022
54d80cf
Addressing comments - correcting roles permissions
bharath-techie Sep 6, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 8 additions & 1 deletion src/main/java/org/opensearch/security/OpenSearchSecurityPlugin.java
View file
Open in desktop
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
193 changes: 193 additions & 0 deletions src/main/java/org/opensearch/security/privileges/PitPrivilegesEvaluator.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,193 @@
/*
* 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.
*/
peternied marked this conversation as resolved.
Show resolved Hide resolved

/*
* 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.concurrent.CountDownLatch;
import java.util.concurrent.TimeUnit;
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.OpenSearchException;
import org.opensearch.action.ActionListener;
import org.opensearch.action.ActionRequest;
import org.opensearch.action.LatchedActionListener;
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.common.unit.TimeValue;
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 pit evaluation for "NodesGetAllPITs" action, since it fetches all PITs across the cluster
peternied marked this conversation as resolved.
Show resolved Hide resolved
// for privilege evaluation
if(action.startsWith("cluster:admin")) {
peternied marked this conversation as resolved.
Show resolved Hide resolved
return presponse;
}
try {
if (request instanceof GetAllPitNodesRequest) {
peternied marked this conversation as resolved.
Show resolved Hide resolved
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, dnfOfEmptyResultsEnabled, presponse);
} else if (request instanceof PitSegmentsRequest) {
PitSegmentsRequest pitSegmentsRequest = (PitSegmentsRequest) request;
return handleExplicitPitsAccess(pitSegmentsRequest.getPitIds(), clusterService, user, securityRoles,
action, resolver, dnfOfEmptyResultsEnabled, presponse);
}
} catch(InterruptedException e) {
Thread.currentThread().interrupt();
log.error(e.toString());
}
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) throws InterruptedException {
peternied marked this conversation as resolved.
Show resolved Hide resolved
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<>();
for (String pitId : pitIds) {
peternied marked this conversation as resolved.
Show resolved Hide resolved
String[] indices = pitToIndicesMap.get(pitId);
HashSet<String> indicesSet = new HashSet<>(Arrays.asList(indices));

final ImmutableSet<String> INDICES_SET = ImmutableSet.copyOf(indicesSet);
peternied marked this conversation as resolved.
Show resolved Hide resolved
final IndexResolverReplacer.Resolved pitResolved =
peternied marked this conversation as resolved.
Show resolved Hide resolved
new IndexResolverReplacer.Resolved(INDICES_SET, INDICES_SET, INDICES_SET,
ImmutableSet.of(), SearchRequest.DEFAULT_INDICES_OPTIONS);
peternied marked this conversation as resolved.
Show resolved Hide resolved

final Set<String> allPermittedIndices = securityRoles.reduce(pitResolved,
user, new String[]{action}, resolver, clusterService);
if(isDebugEnabled) {
log.debug("Evaluating PIT ID : " + pitId );
}
if (allPermittedIndices.size() == INDICES_SET.size()) {
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,
boolean dnfOfEmptyResultsEnabled, 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> INDICES_SET = ImmutableSet.copyOf(pitIndices);
final IndexResolverReplacer.Resolved pitResolved =
new IndexResolverReplacer.Resolved(INDICES_SET, INDICES_SET, INDICES_SET,
ImmutableSet.of(), SearchRequest.DEFAULT_INDICES_OPTIONS);
return securityRoles.reduce(pitResolved,
user, new String[]{action}, resolver, clusterService);
}
}
8 changes: 8 additions & 0 deletions src/main/java/org/opensearch/security/privileges/PrivilegesEvaluator.java
View file
Open in desktop
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
10 changes: 5 additions & 5 deletions src/main/java/org/opensearch/security/resolver/IndexResolverReplacer.java
View file
Open in desktop
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
View file
Open in desktop
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"
Copy link

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