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

Mirror privileges over data streams to their backing indices #58381

Merged
merged 25 commits into from
Jul 3, 2020
Merged
Show file tree
Hide file tree
Changes from 24 commits
Commits
Show all changes
25 commits
Select commit Hold shift + click to select a range
d30476c
Change IndicesPermission::authorize to check parent data stream if re…
danhermann Jun 17, 2020
bc0e9b1
add REST test
danhermann Jun 18, 2020
1f03278
add comment to test
danhermann Jun 19, 2020
3c6bac8
Merge branch 'master' into grant_privs_to_backing_indices
elasticmachine Jun 19, 2020
bd716a5
Merge branch 'grant_privs_to_backing_indices' of https://github.com/d…
danhermann Jun 19, 2020
6073197
Merge branch 'master' into grant_privs_to_backing_indices
elasticmachine Jun 23, 2020
948209e
Merge branch 'master' into grant_privs_to_backing_indices
elasticmachine Jun 26, 2020
e0965e4
recommended security integration implementation
danhermann Jun 27, 2020
80d7caf
add tests for isIndexVisible
danhermann Jun 28, 2020
6caaa1e
add test for resolveAuthorizedIndicesFromRole
danhermann Jun 28, 2020
f8305c0
add test for IndicesPermission::authorize
danhermann Jun 28, 2020
233c043
checkstyle!
danhermann Jun 28, 2020
dd8794f
improve javadoc
danhermann Jun 29, 2020
79c5e09
add test case for requests not supporting data streams
danhermann Jun 30, 2020
b26c874
add includeDataStreams flag to fix failing test
danhermann Jun 30, 2020
2988f7a
unit test fix
danhermann Jun 30, 2020
c380031
remove tests that are no longer valid
danhermann Jun 30, 2020
6491913
Merge branch 'master' into grant_privs_to_backing_indices
elasticmachine Jun 30, 2020
fa0d1d6
fix failing test
danhermann Jun 30, 2020
12466c5
Merge branch 'master' into grant_privs_to_backing_indices
danhermann Jun 30, 2020
129d01c
fix merge conflict
danhermann Jun 30, 2020
8f1d8ee
Merge branch 'master' into grant_privs_to_backing_indices
elasticmachine Jul 2, 2020
98f222b
fix test
danhermann Jul 2, 2020
022c7c1
fix another test
danhermann Jul 2, 2020
2ba65c9
review comments
danhermann Jul 3, 2020
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
Original file line number Diff line number Diff line change
Expand Up @@ -47,15 +47,6 @@
indices.delete:
index: logs-foobar

- do:
indices.create:
index: logs-foobarbaz

- do:
catch: bad_request
indices.close:
index: logs-*

- do:
indices.delete_data_stream:
name: logs-foobar
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,15 @@ public interface IndicesRequest {
*/
IndicesOptions indicesOptions();

/**
* Determines whether the request should be applied to data streams. When {@code false}, none of the names or
* wildcard expressions in {@link #indices} should be applied to or expanded to any data streams. All layers
* involved in the request's fulfillment including security, name resolution, etc., should respect this flag.
*/
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggestion: I would work on the naming and/or javadoc for this to make it clear that such requests over wildcards are meant to (not) expand to data stream names too.

default boolean includeDataStreams() {
return false;
}

interface Replaceable extends IndicesRequest {
/**
* Sets the indices that the action relates to.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -112,6 +112,11 @@ public ClusterSearchShardsRequest indicesOptions(IndicesOptions indicesOptions)
return this;
}

@Override
public boolean includeDataStreams() {
return true;
}

/**
* A comma separated list of routing values to control the shards the search will be executed on.
*/
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -281,4 +281,9 @@ public void writeTo(StreamOutput out) throws IOException {
super.writeTo(out);
flags.writeTo(out);
}

@Override
public boolean includeDataStreams() {
return true;
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -157,6 +157,11 @@ public IndicesOptions indicesOptions() {
return indicesOptions;
}

@Override
public boolean includeDataStreams() {
return true;
}

public boolean includeUnmapped() {
return includeUnmapped;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -348,6 +348,11 @@ public SearchRequest indicesOptions(IndicesOptions indicesOptions) {
return this;
}

@Override
public boolean includeDataStreams() {
return true;
}

/**
* Returns whether network round-trips should be minimized when executing cross-cluster search requests.
* Defaults to <code>true</code>.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -194,7 +194,7 @@ public Automaton allowedActionsMatcher(String index) {
* Authorizes the provided action against the provided indices, given the current cluster metadata
*/
public Map<String, IndicesAccessControl.IndexAccessControl> authorize(String action, Set<String> requestedIndicesOrAliases,
Map<String, IndexAbstraction> allAliasesAndIndices,
Map<String, IndexAbstraction> lookup,
FieldPermissionsCache fieldPermissionsCache) {
// now... every index that is associated with the request, must be granted
// by at least one indices permission group
Expand All @@ -205,15 +205,20 @@ public Map<String, IndicesAccessControl.IndexAccessControl> authorize(String act
for (String indexOrAlias : requestedIndicesOrAliases) {
boolean granted = false;
Set<String> concreteIndices = new HashSet<>();
IndexAbstraction indexAbstraction = allAliasesAndIndices.get(indexOrAlias);
IndexAbstraction indexAbstraction = lookup.get(indexOrAlias);
if (indexAbstraction != null) {
for (IndexMetadata indexMetadata : indexAbstraction.getIndices()) {
concreteIndices.add(indexMetadata.getIndex().getName());
}
}

for (Group group : groups) {
if (group.check(action, indexOrAlias)) {
// check for privilege granted directly on the requested index/alias
if (group.check(action, indexOrAlias) ||
// check for privilege granted on parent data stream if a backing index
(indexAbstraction != null && indexAbstraction.getType() == IndexAbstraction.Type.CONCRETE_INDEX &&
indexAbstraction.getParentDataStream() != null &&
group.check(action, indexAbstraction.getParentDataStream().getName()))) {
granted = true;
for (String index : concreteIndices) {
Set<FieldPermissions> fieldPermissions = fieldPermissionsByIndex.computeIfAbsent(index, (k) -> new HashSet<>());
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -130,7 +130,7 @@ ResolvedIndices resolveIndicesAndAliases(IndicesRequest indicesRequest, Metadata
if (IndexNameExpressionResolver.isAllIndices(indicesList(indicesRequest.indices()))) {
if (replaceWildcards) {
for (String authorizedIndex : authorizedIndices) {
if (isIndexVisible("*", authorizedIndex, indicesOptions, metadata)) {
if (isIndexVisible("*", authorizedIndex, indicesOptions, metadata, indicesRequest.includeDataStreams())) {
resolvedIndicesBuilder.addLocal(authorizedIndex);
}
}
Expand All @@ -145,7 +145,7 @@ ResolvedIndices resolveIndicesAndAliases(IndicesRequest indicesRequest, Metadata
split = new ResolvedIndices(Arrays.asList(indicesRequest.indices()), Collections.emptyList());
}
List<String> replaced = replaceWildcardsWithAuthorizedIndices(split.getLocal(), indicesOptions, metadata,
authorizedIndices, replaceWildcards);
authorizedIndices, replaceWildcards, indicesRequest.includeDataStreams());
if (indicesOptions.ignoreUnavailable()) {
//out of all the explicit names (expanded from wildcards and original ones that were left untouched)
//remove all the ones that the current user is not authorized for and ignore them
Expand Down Expand Up @@ -337,7 +337,8 @@ private boolean containsWildcards(IndicesRequest indicesRequest) {

//TODO Investigate reusing code from vanilla es to resolve index names and wildcards
private List<String> replaceWildcardsWithAuthorizedIndices(Iterable<String> indices, IndicesOptions indicesOptions, Metadata metadata,
List<String> authorizedIndices, boolean replaceWildcards) {
List<String> authorizedIndices, boolean replaceWildcards,
boolean includeDataStreams) {
//the order matters when it comes to exclusions
List<String> finalIndices = new ArrayList<>();
boolean wildcardSeen = false;
Expand All @@ -359,7 +360,7 @@ private List<String> replaceWildcardsWithAuthorizedIndices(Iterable<String> indi
// continue
aliasOrIndex = dateMathName;
} else if (authorizedIndices.contains(dateMathName) &&
isIndexVisible(aliasOrIndex, dateMathName, indicesOptions, metadata, true)) {
isIndexVisible(aliasOrIndex, dateMathName, indicesOptions, metadata, includeDataStreams, true)) {
if (minus) {
finalIndices.remove(dateMathName);
} else {
Expand All @@ -377,7 +378,7 @@ private List<String> replaceWildcardsWithAuthorizedIndices(Iterable<String> indi
Set<String> resolvedIndices = new HashSet<>();
for (String authorizedIndex : authorizedIndices) {
if (Regex.simpleMatch(aliasOrIndex, authorizedIndex) &&
isIndexVisible(aliasOrIndex, authorizedIndex, indicesOptions, metadata)) {
isIndexVisible(aliasOrIndex, authorizedIndex, indicesOptions, metadata, includeDataStreams)) {
resolvedIndices.add(authorizedIndex);
}
}
Expand Down Expand Up @@ -412,13 +413,17 @@ private List<String> replaceWildcardsWithAuthorizedIndices(Iterable<String> indi
return finalIndices;
}

private static boolean isIndexVisible(String expression, String index, IndicesOptions indicesOptions, Metadata metadata) {
return isIndexVisible(expression, index, indicesOptions, metadata, false);
private static boolean isIndexVisible(String expression, String index, IndicesOptions indicesOptions, Metadata metadata,
boolean includeDataStreams) {
return isIndexVisible(expression, index, indicesOptions, metadata, includeDataStreams, false);
}

private static boolean isIndexVisible(String expression, String index, IndicesOptions indicesOptions, Metadata metadata,
boolean dateMathExpression) {
boolean includeDataStreams, boolean dateMathExpression) {
IndexAbstraction indexAbstraction = metadata.getIndicesLookup().get(index);
if (indexAbstraction == null) {
return false;
Copy link
Contributor

Choose a reason for hiding this comment

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

I would make it an IllegalStateException.

}
final boolean isHidden = indexAbstraction.isHidden();
if (indexAbstraction.getType() == IndexAbstraction.Type.ALIAS) {
//it's an alias, ignore expandWildcardsOpen and expandWildcardsClosed.
Expand All @@ -433,12 +438,7 @@ private static boolean isIndexVisible(String expression, String index, IndicesOp
}
}
if (indexAbstraction.getType() == IndexAbstraction.Type.DATA_STREAM) {
// If indicesOptions.includeDataStreams() returns false then we fail later in IndexNameExpressionResolver.
if (isHidden == false || indicesOptions.expandWildcardsHidden()) {
return true;
} else {
return false;
}
return includeDataStreams;
}
assert indexAbstraction.getIndices().size() == 1 : "concrete index must point to a single index";
IndexMetadata indexMetadata = indexAbstraction.getIndices().get(0);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -86,6 +86,7 @@
import java.util.Set;
import java.util.TreeSet;
import java.util.function.Predicate;
import java.util.stream.Collectors;

import static org.elasticsearch.common.Strings.arrayToCommaDelimitedString;
import static org.elasticsearch.xpack.security.action.user.TransportHasPrivilegesAction.getApplicationNames;
Expand Down Expand Up @@ -343,7 +344,7 @@ public void loadAuthorizedIndices(RequestInfo requestInfo, AuthorizationInfo aut
Map<String, IndexAbstraction> indicesLookup, ActionListener<List<String>> listener) {
if (authorizationInfo instanceof RBACAuthorizationInfo) {
final Role role = ((RBACAuthorizationInfo) authorizationInfo).getRole();
listener.onResponse(resolveAuthorizedIndicesFromRole(role, requestInfo.getAction(), indicesLookup));
listener.onResponse(resolveAuthorizedIndicesFromRole(role, requestInfo, indicesLookup));
} else {
listener.onFailure(
new IllegalArgumentException("unsupported authorization info:" + authorizationInfo.getClass().getSimpleName()));
Expand Down Expand Up @@ -500,18 +501,29 @@ GetUserPrivilegesResponse buildUserPrivilegesResponseObject(Role userRole) {
return new GetUserPrivilegesResponse(cluster, conditionalCluster, indices, application, runAs);
}

static List<String> resolveAuthorizedIndicesFromRole(Role role, String action, Map<String, IndexAbstraction> aliasAndIndexLookup) {
Predicate<String> predicate = role.allowedIndicesMatcher(action);
static List<String> resolveAuthorizedIndicesFromRole(Role role, RequestInfo requestInfo, Map<String, IndexAbstraction> lookup) {
Predicate<String> predicate = role.allowedIndicesMatcher(requestInfo.getAction());

List<String> indicesAndAliases = new ArrayList<>();
// do not include data streams for actions that do not operate on data streams
TransportRequest request = requestInfo.getRequest();
boolean includeDataStreams = (request instanceof IndicesRequest) && ((IndicesRequest) request).includeDataStreams();

Set<String> indicesAndAliases = new HashSet<>();
// TODO: can this be done smarter? I think there are usually more indices/aliases in the cluster then indices defined a roles?
for (Map.Entry<String, IndexAbstraction> entry : aliasAndIndexLookup.entrySet()) {
String aliasOrIndex = entry.getKey();
if (predicate.test(aliasOrIndex)) {
indicesAndAliases.add(aliasOrIndex);
for (Map.Entry<String, IndexAbstraction> entry : lookup.entrySet()) {
String indexAbstraction = entry.getKey();
if (predicate.test(indexAbstraction)) {
if (entry.getValue().getType() != IndexAbstraction.Type.DATA_STREAM) {
indicesAndAliases.add(indexAbstraction);
} else if (includeDataStreams) {
// add data stream and its backing indices for any authorized data streams
indicesAndAliases.addAll(entry.getValue().getIndices().stream()
.map(i -> i.getIndex().getName()).collect(Collectors.toList()));
indicesAndAliases.add(indexAbstraction);
}
}
}
return Collections.unmodifiableList(indicesAndAliases);
return Collections.unmodifiableList(new ArrayList<>(indicesAndAliases));
}

private void buildIndicesAccessControl(Authentication authentication, String action,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,8 @@
import org.elasticsearch.common.settings.Settings;
import org.elasticsearch.common.util.set.Sets;
import org.elasticsearch.test.ESTestCase;
import org.elasticsearch.transport.TransportRequest;
import org.elasticsearch.xpack.core.security.authz.AuthorizationEngine;
import org.elasticsearch.xpack.core.security.authz.RoleDescriptor;
import org.elasticsearch.xpack.core.security.authz.RoleDescriptor.IndicesPrivileges;
import org.elasticsearch.xpack.core.security.authz.permission.FieldPermissions;
Expand All @@ -35,7 +37,7 @@ public class AuthorizedIndicesTests extends ESTestCase {

Copy link
Contributor

Choose a reason for hiding this comment

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

We need to test that RBACEngine.resolveAuthorizedIndicesFromRole:

  • returns the list containing the backing indices if the role grants access to a data stream (granting as part of a wildcard or a simple name)
  • indices which are not backing indices are not returned when some data stream is covered by the role permission
  • granting access to backing indices (as a wildcard or simple name) does not include the data stream in the list of authorised names

We need to repeat the tests for when the request works with data streams or not.

public void testAuthorizedIndicesUserWithoutRoles() {
List<String> authorizedIndices =
RBACEngine.resolveAuthorizedIndicesFromRole(Role.EMPTY, "", Metadata.EMPTY_METADATA.getIndicesLookup());
RBACEngine.resolveAuthorizedIndicesFromRole(Role.EMPTY, getRequestInfo(""), Metadata.EMPTY_METADATA.getIndicesLookup());
assertTrue(authorizedIndices.isEmpty());
}

Expand Down Expand Up @@ -71,7 +73,7 @@ public void testAuthorizedIndicesUserWithSomeRoles() {
CompositeRolesStore.buildRoleFromDescriptors(descriptors, new FieldPermissionsCache(Settings.EMPTY), null, future);
Role roles = future.actionGet();
List<String> list =
RBACEngine.resolveAuthorizedIndicesFromRole(roles, SearchAction.NAME, metadata.getIndicesLookup());
RBACEngine.resolveAuthorizedIndicesFromRole(roles, getRequestInfo(SearchAction.NAME), metadata.getIndicesLookup());
assertThat(list, containsInAnyOrder("a1", "a2", "aaaaaa", "b", "ab"));
assertFalse(list.contains("bbbbb"));
assertFalse(list.contains("ba"));
Expand All @@ -81,15 +83,15 @@ public void testAuthorizedIndicesUserWithSomeRoles() {

public void testAuthorizedIndicesUserWithSomeRolesEmptyMetadata() {
Role role = Role.builder("role").add(IndexPrivilege.ALL, "*").build();
List<String> authorizedIndices =
RBACEngine.resolveAuthorizedIndicesFromRole(role, SearchAction.NAME, Metadata.EMPTY_METADATA.getIndicesLookup());
List<String> authorizedIndices = RBACEngine.resolveAuthorizedIndicesFromRole(role, getRequestInfo(SearchAction.NAME),
Metadata.EMPTY_METADATA.getIndicesLookup());
assertTrue(authorizedIndices.isEmpty());
}

public void testSecurityIndicesAreRemovedFromRegularUser() {
Role role = Role.builder("user_role").add(IndexPrivilege.ALL, "*").cluster(Set.of("all"), Set.of()).build();
List<String> authorizedIndices =
RBACEngine.resolveAuthorizedIndicesFromRole(role, SearchAction.NAME, Metadata.EMPTY_METADATA.getIndicesLookup());
List<String> authorizedIndices = RBACEngine.resolveAuthorizedIndicesFromRole(role, getRequestInfo(SearchAction.NAME),
Metadata.EMPTY_METADATA.getIndicesLookup());
assertTrue(authorizedIndices.isEmpty());
}

Expand All @@ -114,7 +116,7 @@ public void testSecurityIndicesAreRestrictedForDefaultRole() {
.build();

List<String> authorizedIndices =
RBACEngine.resolveAuthorizedIndicesFromRole(role, SearchAction.NAME, metadata.getIndicesLookup());
RBACEngine.resolveAuthorizedIndicesFromRole(role, getRequestInfo(SearchAction.NAME), metadata.getIndicesLookup());
assertThat(authorizedIndices, containsInAnyOrder("an-index", "another-index"));
assertThat(authorizedIndices, not(contains(internalSecurityIndex)));
assertThat(authorizedIndices, not(contains(RestrictedIndicesNames.SECURITY_MAIN_ALIAS)));
Expand All @@ -140,13 +142,21 @@ public void testSecurityIndicesAreNotRemovedFromUnrestrictedRole() {
.build();

List<String> authorizedIndices =
RBACEngine.resolveAuthorizedIndicesFromRole(role, SearchAction.NAME, metadata.getIndicesLookup());
RBACEngine.resolveAuthorizedIndicesFromRole(role, getRequestInfo(SearchAction.NAME), metadata.getIndicesLookup());
assertThat(authorizedIndices, containsInAnyOrder(
"an-index", "another-index", RestrictedIndicesNames.SECURITY_MAIN_ALIAS, internalSecurityIndex));

List<String> authorizedIndicesSuperUser =
RBACEngine.resolveAuthorizedIndicesFromRole(role, SearchAction.NAME, metadata.getIndicesLookup());
RBACEngine.resolveAuthorizedIndicesFromRole(role, getRequestInfo(SearchAction.NAME), metadata.getIndicesLookup());
assertThat(authorizedIndicesSuperUser, containsInAnyOrder(
"an-index", "another-index", RestrictedIndicesNames.SECURITY_MAIN_ALIAS, internalSecurityIndex));
}

public static AuthorizationEngine.RequestInfo getRequestInfo(String action) {
return getRequestInfo(TransportRequest.Empty.INSTANCE, action);
}

public static AuthorizationEngine.RequestInfo getRequestInfo(TransportRequest request, String action) {
return new AuthorizationEngine.RequestInfo(null, request, action);
}
}
Loading