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

Disallow mapping updates for doc ingestion privileges #58784

Merged
Merged
Show file tree
Hide file tree
Changes from 33 commits
Commits
Show all changes
43 commits
Select commit Hold shift + click to select a range
d62250d
Done maybe
albertzaharovits Jun 30, 2020
547faa3
Merge branch 'master' into remove_put_mapping_priv
albertzaharovits Jun 30, 2020
9dce3fc
Merge branch 'master' into remove_put_mapping_priv
albertzaharovits Jul 6, 2020
00bbb0f
Merge fallout
albertzaharovits Jul 6, 2020
f4fb1c7
Auto put mapping is also bwc supported
albertzaharovits Jul 6, 2020
ac0966d
IndicesPermission
albertzaharovits Jul 7, 2020
63e7ade
WIP
albertzaharovits Jul 7, 2020
905e458
Maybeee?
albertzaharovits Jul 7, 2020
e2dc3f9
authorizeMappingUpdateBwcSpecialCase
albertzaharovits Jul 7, 2020
9a5ff34
Merge branch 'master' into remove_put_mapping_priv
albertzaharovits Jul 7, 2020
ec96511
ReservedRolesStoreTests
albertzaharovits Jul 7, 2020
e155998
Merge branch 'master' into remove_put_mapping_priv
albertzaharovits Jul 7, 2020
04f95a9
WIP adjust tests
albertzaharovits Jul 7, 2020
a99f9f0
Existing tests compiled
albertzaharovits Jul 7, 2020
0e7efed
Merge branch 'master' into remove_put_mapping_priv
albertzaharovits Jul 7, 2020
4dfe44a
Checkstyle
albertzaharovits Jul 7, 2020
66e244d
Checkstyle
albertzaharovits Jul 7, 2020
2d34b46
Merge branch 'master' into remove_put_mapping_priv
albertzaharovits Jul 7, 2020
44e0517
Merge branch 'master' into remove_put_mapping_priv
albertzaharovits Jul 8, 2020
3c53f41
XPackRestIT
albertzaharovits Jul 8, 2020
008ec5a
index -> write for ml yaml tests
albertzaharovits Jul 8, 2020
aa62846
SmokeTestSecurityWithMustacheClientYamlTestSuiteIT
albertzaharovits Jul 8, 2020
c09a20b
Merge branch 'master' into remove_put_mapping_priv
albertzaharovits Jul 8, 2020
3ec62d3
Data stream tests
albertzaharovits Jul 8, 2020
774a423
Merge branch 'master' into remove_put_mapping_priv
albertzaharovits Jul 8, 2020
4c6383c
IndexPrivilege tests
albertzaharovits Jul 8, 2020
75bb67e
Nit
albertzaharovits Jul 8, 2020
b114c13
IndicesMatcher action
albertzaharovits Jul 8, 2020
4a2394c
Almost
albertzaharovits Jul 8, 2020
e08624a
Done testing
albertzaharovits Jul 8, 2020
4071e95
Review
albertzaharovits Jul 8, 2020
e6bd53f
Merge branch 'master' into remove_put_mapping_priv
albertzaharovits Jul 13, 2020
eb50c54
Merge fallout
albertzaharovits Jul 13, 2020
49b92c6
Review partway
albertzaharovits Jul 13, 2020
50f393b
Merge branch 'master' into remove_put_mapping_priv
albertzaharovits Jul 14, 2020
f5a85d7
Review
albertzaharovits Jul 14, 2020
3d3f64d
Merge branch 'master' into remove_put_mapping_priv
albertzaharovits Jul 14, 2020
44c3473
Review partway
albertzaharovits Jul 14, 2020
fd45d7b
Merge branch 'master' into remove_put_mapping_priv
albertzaharovits Jul 14, 2020
3e4f130
Message
albertzaharovits Jul 14, 2020
148057c
Deprecation msgs fallout
albertzaharovits Jul 14, 2020
9e12382
Update more deprecation logs
albertzaharovits Jul 14, 2020
f3fec33
Merge branch 'master' into remove_put_mapping_priv
albertzaharovits Jul 14, 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 @@ -10,11 +10,14 @@
import org.apache.lucene.util.automaton.Operations;
import org.apache.lucene.util.automaton.TooComplexToDeterminizeException;
import org.elasticsearch.ElasticsearchSecurityException;
import org.elasticsearch.action.admin.indices.mapping.put.AutoPutMappingAction;
import org.elasticsearch.action.admin.indices.mapping.put.PutMappingAction;
import org.elasticsearch.cluster.metadata.IndexAbstraction;
import org.elasticsearch.cluster.metadata.IndexMetadata;
import org.elasticsearch.common.Nullable;
import org.elasticsearch.common.Strings;
import org.elasticsearch.common.bytes.BytesReference;
import org.elasticsearch.common.logging.DeprecationLogger;
import org.elasticsearch.common.regex.Regex;
import org.elasticsearch.xpack.core.security.authz.accesscontrol.IndicesAccessControl;
import org.elasticsearch.xpack.core.security.authz.privilege.IndexPrivilege;
Expand Down Expand Up @@ -44,16 +47,38 @@
*/
public final class IndicesPermission {

private static final DeprecationLogger deprecationLogger = DeprecationLogger.getLogger(IndicesPermission.class);

public static final IndicesPermission NONE = new IndicesPermission();

private final ConcurrentMap<String, Predicate<String>> allowedIndicesMatchersForAction = new ConcurrentHashMap<>();
private static final Set<String> privilegeNameSetBwcAllowMappingUpdate = Set.of("create", "create_doc", "index", "write");

private final ConcurrentMap<String, Predicate<IndexAbstraction>> allowedIndicesMatchersForAction = new ConcurrentHashMap<>();

private final Group[] groups;

public IndicesPermission(Group... groups) {
this.groups = groups;
}

private static Predicate<String> indexMatcher(Collection<String> ordinaryIndices, Collection<String> restrictedIndices) {
Predicate<String> namePredicate;
if (ordinaryIndices.isEmpty()) {
namePredicate = indexMatcher(restrictedIndices);
} else {
namePredicate = indexMatcher(ordinaryIndices)
.and(index -> false == RestrictedIndicesNames.isRestricted(index));
if (restrictedIndices.isEmpty() == false) {
namePredicate = indexMatcher(restrictedIndices).or(namePredicate);
}
}
return namePredicate;
}

/**
* Given a collection of index names and patterns, this constructs a {@code Predicate} that tests
* {@code true} for the names in the collection as well as for any names matching the patterns in the collection.
*/
public static Predicate<String> indexMatcher(Collection<String> indices) {
Set<String> exactMatch = new HashSet<>();
List<String> nonExactMatch = new ArrayList<>();
Expand Down Expand Up @@ -105,7 +130,7 @@ public Group[] groups() {
* @return A predicate that will match all the indices that this permission
* has the privilege for executing the given action on.
*/
public Predicate<String> allowedIndicesMatcher(String action) {
public Predicate<IndexAbstraction> allowedIndicesMatcher(String action) {
return allowedIndicesMatchersForAction.computeIfAbsent(action, a -> Group.buildIndexMatcherPredicateForAction(a, groups));
}

Expand All @@ -116,8 +141,10 @@ public Predicate<String> allowedIndicesMatcher(String action) {
* checked on the coordinating node), and properly authorized later at the shard level checking their indices as well.
*/
public boolean check(String action) {
final boolean isMappingUpdateAction = action.equals(PutMappingAction.NAME) || action.equals(AutoPutMappingAction.NAME);
for (Group group : groups) {
if (group.check(action)) {
if (group.checkAction(action) || (isMappingUpdateAction &&
group.privilege().name().stream().anyMatch(privilegeNameSetBwcAllowMappingUpdate::contains))) {
return true;
}
}
Expand Down Expand Up @@ -202,43 +229,83 @@ public Map<String, IndicesAccessControl.IndexAccessControl> authorize(String act
Map<String, DocumentLevelPermissions> roleQueriesByIndex = new HashMap<>();
Map<String, Boolean> grantedBuilder = new HashMap<>();

final boolean isMappingUpdateAction = action.equals(PutMappingAction.NAME) || action.equals(AutoPutMappingAction.NAME);

for (String indexOrAlias : requestedIndicesOrAliases) {
boolean granted = false;
Set<String> concreteIndices = new HashSet<>();
IndexAbstraction indexAbstraction = lookup.get(indexOrAlias);
final boolean isBackingIndex;
final boolean isDataStream;
final Set<String> concreteIndices = new HashSet<>();
final IndexAbstraction indexAbstraction = lookup.get(indexOrAlias);
if (indexAbstraction != null) {
for (IndexMetadata indexMetadata : indexAbstraction.getIndices()) {
concreteIndices.add(indexMetadata.getIndex().getName());
}
isBackingIndex = indexAbstraction.getType() == IndexAbstraction.Type.CONCRETE_INDEX &&
indexAbstraction.getParentDataStream() != null;
isDataStream = indexAbstraction.getType() == IndexAbstraction.Type.DATA_STREAM;
} else {
isBackingIndex = isDataStream = false;
}

// true if ANY group covers the given index AND the given action
boolean granted = false;
// true if ANY group, which contains certain ingest privileges, covers the given index AND the action is a mapping update for
// an index or an alias (but not for a data stream)
boolean bwcGrantMappingUpdate = false;
final List<Runnable> bwcDeprecationLogActions = new ArrayList<>();

for (Group group : groups) {
// 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<>());
fieldPermissionsByIndex.put(indexOrAlias, fieldPermissions);
fieldPermissions.add(group.getFieldPermissions());
DocumentLevelPermissions permissions =
roleQueriesByIndex.computeIfAbsent(index, (k) -> new DocumentLevelPermissions());
roleQueriesByIndex.putIfAbsent(indexOrAlias, permissions);
if (group.hasQuery()) {
permissions.addAll(group.getQuery());
} else {
// if more than one permission matches for a concrete index here and if
// a single permission doesn't have a role query then DLS will not be
// applied even when other permissions do have a role query
permissions.setAllowAll(true);
// the group covers the given index OR the given index is a backing index and the group covers the parent data stream
final boolean indexCheck = group.checkIndex(indexOrAlias) ||
(isBackingIndex && group.checkIndex(indexAbstraction.getParentDataStream().getName()));
if (indexCheck) {
boolean actionCheck = group.checkAction(action);
// mapping updates are allowed for certain privileges on indices and aliases (but not on data streams),
// outside of the privilege definition
boolean bwcMappingActionCheck = isMappingUpdateAction && false == isDataStream && false == isBackingIndex &&
group.privilege().name().stream().anyMatch(privilegeNameSetBwcAllowMappingUpdate::contains);
Copy link
Member

Choose a reason for hiding this comment

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

I think this can be optimised as

boolean bwcMappingActionCheck = actionCheck ? false : isMappingUpdateAction && false == isDataStream && false == isBackingIndex &&
        group.privilege().name().stream().anyMatch(privilegeNameSetBwcAllowMappingUpdate::contains);

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't like this minor optimisation because it makes it harder to reason about the meaning of the check.
There is some overlapping between the actionCheck and the bwcMappingActionCheck flags (they can both be true) because it is easier to express them.

Copy link
Member

Choose a reason for hiding this comment

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

I do agree this by itself makes the code harder to read. Since I made a few comments around this code block, I think what I am really suggesting is to re-arrange in a broader context, e.g.:

if (group.checkAction(action)) {
    granted = true;
    propagateDlsAndFlsPermissionsOvertheConcreteIndices(...);
} else if (isMappingUpdateAction && false == isDataStream && false == isBackingIndex &&
        group.privilege().name().stream().anyMatch(privilegeNameSetBwcAllowMappingUpdate::contains)) {
    bwcGrantMappingUpdate = true;
    propagateDlsAndFlsPermissionsOvertheConcreteIndices(...);
    populateDeprecationLogs(...);
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I understand now, but I don't see an improvement. bwcGrantMappingUpdate is updated to true in the if branch, but I slightly prefer it when its value is the expression of the if branch itself; I find it easier to follow like this.
Also, you need a similar flag for the if (group.checkAction(action)) if branch, because deprecation logs must only be present if the action is exclusively granted by the deprecation behaviour.

Copy link
Member

Choose a reason for hiding this comment

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

I don't quite follow.

Once the logic is inside the else if branch, we can already be sure the action is granted by bwc and can go ahead and prepare the deprecation logs within the branch (i.e. populateDeprecationLogs(...)). With this approach, we can get rid of two variables actionCheck and bwcMappingActionCheck. We still need granted and bwcGrantMappingUpdate, but only one of them needs to be updated for each loop.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry, you're right, I got lost.
I'm tempted to not change the code as you're suggesting, because the goal to get rid of actionCheck and bwcMappingActionCheck is not worthwhile IMO. To me these look like different approaches with no substantial differences.

if (actionCheck || bwcMappingActionCheck) {
// propagate DLS and FLS permissions over the concrete indices
for (String index : concreteIndices) {
Set<FieldPermissions> fieldPermissions = fieldPermissionsByIndex.computeIfAbsent(index, (k) -> new HashSet<>());
fieldPermissionsByIndex.put(indexOrAlias, fieldPermissions);
fieldPermissions.add(group.getFieldPermissions());
DocumentLevelPermissions permissions =
roleQueriesByIndex.computeIfAbsent(index, (k) -> new DocumentLevelPermissions());
roleQueriesByIndex.putIfAbsent(indexOrAlias, permissions);
if (group.hasQuery()) {
permissions.addAll(group.getQuery());
} else {
// if more than one permission matches for a concrete index here and if
// a single permission doesn't have a role query then DLS will not be
// applied even when other permissions do have a role query
permissions.setAllowAll(true);
}
}
}
granted |= actionCheck;
bwcGrantMappingUpdate |= bwcMappingActionCheck;
if (false == actionCheck && bwcMappingActionCheck) {
for (String privilegeName : group.privilege.name()) {
if (privilegeNameSetBwcAllowMappingUpdate.contains(privilegeName)) {
bwcDeprecationLogActions.add(() -> {
deprecationLogger.deprecate("[" + indexOrAlias + "] mapping update for ingest privilege [" +
privilegeName + "]", "the mapping update action [" + action + "] on the [" +
indexOrAlias + "] index, is granted by the [" + privilegeName + "] privilege," +
" but the privilege has been tightened to not allow it in the next major release");
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not a fan of this wording - I'll propose an alternative during my day.

});
}
Copy link
Member

Choose a reason for hiding this comment

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

I wonder whether it would be better to display a single deprecation log per indexOrAlias, e.g. ... is granted by [privA, privB] ....

Also this if block can be placed inside the previous if (actionCheck || bwcMappingActionCheck) so its own if check can become just if (false == actionCheck).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I wonder whether it would be better to display a single deprecation log per indexOrAlias, e.g. ... is granted by [privA, privB] ....

I dunno. I've chosen the current variant because it is easier to code.

The deprecationLogger.deprecate method has built-in logic to deduplicate frequent log entries with the same key.
If a log message contains a list of privileges, instead of a single one, the deduplication logic would have to be smarter to compare that "all (or some of) the privileges (given the index) on this deprecation log entry have appeared in (some) previous entries". It sounds too complicated to implement.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Also this if block can be placed inside the previous if (actionCheck || bwcMappingActionCheck) so its own if check can become just if (false == actionCheck).

I can make the move, I don't know of any heuristic that could help me choose between the two.

}
}
}
}

if (false == granted && bwcGrantMappingUpdate) {
// the action is granted only due to the deprecated behaviour of certain privileges
granted = true;
bwcDeprecationLogActions.forEach(deprecationLogAction -> deprecationLogAction.run());
}

if (concreteIndices.isEmpty()) {
grantedBuilder.put(indexOrAlias, granted);
} else {
Expand Down Expand Up @@ -322,14 +389,13 @@ public FieldPermissions getFieldPermissions() {
return fieldPermissions;
}

private boolean check(String action) {
private boolean checkAction(String action) {
return actionMatcher.test(action);
}

private boolean check(String action, String index) {
private boolean checkIndex(String index) {
assert index != null;
return check(action) && indexNameMatcher.test(index)
&& (allowRestrictedIndices || (false == RestrictedIndicesNames.isRestricted(index)));
return indexNameMatcher.test(index) && (allowRestrictedIndices || (false == RestrictedIndicesNames.isRestricted(index)));
}

boolean hasQuery() {
Expand All @@ -349,30 +415,39 @@ public static Automaton buildIndexMatcherAutomaton(boolean allowRestrictedIndice
}
}

private static Predicate<String> buildIndexMatcherPredicateForAction(String action, Group... groups) {
private static Predicate<IndexAbstraction> buildIndexMatcherPredicateForAction(String action, Group... groups) {
final Set<String> ordinaryIndices = new HashSet<>();
final Set<String> restrictedIndices = new HashSet<>();
final Set<String> grantMappingUpdatesOnIndices = new HashSet<>();
final Set<String> grantMappingUpdatesOnRestrictedIndices = new HashSet<>();
final boolean isMappingUpdateAction = action.equals(PutMappingAction.NAME) || action.equals(AutoPutMappingAction.NAME);
for (final Group group : groups) {
if (group.actionMatcher.test(action)) {
if (group.allowRestrictedIndices) {
restrictedIndices.addAll(Arrays.asList(group.indices()));
} else {
ordinaryIndices.addAll(Arrays.asList(group.indices()));
}
} else if (isMappingUpdateAction &&
group.privilege().name().stream().anyMatch(privilegeNameSetBwcAllowMappingUpdate::contains)) {
// special BWC case for certain privileges: allow put mapping on indices and aliases (but not on data streams), even if
// the privilege definition does not currently allow it
if (group.allowRestrictedIndices) {
grantMappingUpdatesOnRestrictedIndices.addAll(Arrays.asList(group.indices()));
} else {
grantMappingUpdatesOnIndices.addAll(Arrays.asList(group.indices()));
}
}
}
final Predicate<String> predicate;
if (restrictedIndices.isEmpty()) {
predicate = indexMatcher(ordinaryIndices)
.and(index -> false == RestrictedIndicesNames.isRestricted(index));
} else if (ordinaryIndices.isEmpty()) {
predicate = indexMatcher(restrictedIndices);
} else {
predicate = indexMatcher(restrictedIndices)
.or(indexMatcher(ordinaryIndices)
.and(index -> false == RestrictedIndicesNames.isRestricted(index)));
}
return predicate;
final Predicate<String> namePredicate = indexMatcher(ordinaryIndices, restrictedIndices);
final Predicate<String> bwcSpecialCaseNamePredicate = indexMatcher(grantMappingUpdatesOnIndices,
grantMappingUpdatesOnRestrictedIndices);
return indexAbstraction -> {
return namePredicate.test(indexAbstraction.getName()) ||
(indexAbstraction.getType() != IndexAbstraction.Type.DATA_STREAM &&
(indexAbstraction.getParentDataStream() == null) &&
bwcSpecialCaseNamePredicate.test(indexAbstraction.getName()));
};
}
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -72,8 +72,8 @@ public IndicesAccessControl authorize(String action, Set<String> requestedIndice
* action on.
*/
@Override
public Predicate<String> allowedIndicesMatcher(String action) {
Predicate<String> predicate = super.indices().allowedIndicesMatcher(action);
public Predicate<IndexAbstraction> allowedIndicesMatcher(String action) {
Predicate<IndexAbstraction> predicate = super.indices().allowedIndicesMatcher(action);
predicate = predicate.and(limitedBy.indices().allowedIndicesMatcher(action));
return predicate;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -83,7 +83,7 @@ public static Builder builder(RoleDescriptor rd, FieldPermissionsCache fieldPerm
* @return A predicate that will match all the indices that this role
* has the privilege for executing the given action on.
*/
public Predicate<String> allowedIndicesMatcher(String action) {
public Predicate<IndexAbstraction> allowedIndicesMatcher(String action) {
return indices.allowedIndicesMatcher(action);
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,6 @@
import org.elasticsearch.action.admin.indices.mapping.get.GetFieldMappingsAction;
import org.elasticsearch.action.admin.indices.mapping.get.GetMappingsAction;
import org.elasticsearch.action.admin.indices.mapping.put.AutoPutMappingAction;
import org.elasticsearch.action.admin.indices.mapping.put.PutMappingAction;
import org.elasticsearch.action.admin.indices.settings.get.GetSettingsAction;
import org.elasticsearch.action.admin.indices.validate.query.ValidateQueryAction;
import org.elasticsearch.common.Strings;
Expand Down Expand Up @@ -51,15 +50,13 @@ public final class IndexPrivilege extends Privilege {
private static final Automaton READ_AUTOMATON = patterns("indices:data/read/*");
private static final Automaton READ_CROSS_CLUSTER_AUTOMATON = patterns("internal:transport/proxy/indices:data/read/*",
ClusterSearchShardsAction.NAME);
private static final Automaton CREATE_AUTOMATON = patterns("indices:data/write/index*", "indices:data/write/bulk*",
PutMappingAction.NAME, AutoPutMappingAction.NAME);
private static final Automaton CREATE_AUTOMATON = patterns("indices:data/write/index*", "indices:data/write/bulk*");
private static final Automaton CREATE_DOC_AUTOMATON = patterns("indices:data/write/index", "indices:data/write/index[*",
"indices:data/write/index:op_type/create", "indices:data/write/bulk*", PutMappingAction.NAME, AutoPutMappingAction.NAME);
"indices:data/write/index:op_type/create", "indices:data/write/bulk*");
private static final Automaton INDEX_AUTOMATON = patterns("indices:data/write/index*", "indices:data/write/bulk*",
"indices:data/write/update*", PutMappingAction.NAME, AutoPutMappingAction.NAME);
"indices:data/write/update*");
private static final Automaton DELETE_AUTOMATON = patterns("indices:data/write/delete*", "indices:data/write/bulk*");
private static final Automaton WRITE_AUTOMATON = patterns("indices:data/write/*", PutMappingAction.NAME,
AutoPutMappingAction.NAME);
private static final Automaton WRITE_AUTOMATON = patterns("indices:data/write/*", AutoPutMappingAction.NAME);
private static final Automaton MONITOR_AUTOMATON = patterns("indices:monitor/*");
private static final Automaton MANAGE_AUTOMATON =
unionAndMinimize(Arrays.asList(MONITOR_AUTOMATON, patterns("indices:admin/*")));
Expand Down
Loading