Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
38 commits
Select commit Hold shift + click to select a range
b90165e
resolve expressions from role per group
jakelandis Jan 22, 2025
7b2d256
fix variable shadow
jakelandis Jan 22, 2025
5b15f40
basic manual tests pass
jakelandis Jan 22, 2025
eb45a0d
assertions and comments
jakelandis Jan 23, 2025
f02d184
wildcard in role mostly working (test*::*) still fails
jakelandis Jan 23, 2025
6270503
fixed wildcard in role (i think)
jakelandis Jan 23, 2025
f2e45a2
spotless and precommit
jakelandis Jan 23, 2025
445ba6d
don't support hasPrivileges for failure restrictions (still need vali…
jakelandis Jan 23, 2025
cbfedd1
clean up and organize thoughts (comment on direction change)
jakelandis Jan 24, 2025
37821a8
refactor to only support ::failures (simple cases work, but doing unc…
jakelandis Jan 24, 2025
bb90832
greatly simplify
jakelandis Jan 24, 2025
4e86384
nits
jakelandis Jan 24, 2025
9893ccf
[CI] Auto commit changes from spotless
Jan 24, 2025
a5b6d14
handle deny of .fs* indices
jakelandis Jan 25, 2025
f3f7eca
Merge remote-tracking branch 'upstream/main' into alt_119915_hack
jakelandis Jan 25, 2025
fb18a5f
Merge remote-tracking branch 'upstream/main' into alt_119915_hack
jakelandis Jan 28, 2025
c8e9fd6
fix tests that expect failure store indices to be authorized
jakelandis Jan 28, 2025
01e7add
minor clean up
jakelandis Jan 28, 2025
2a544b6
[CI] Auto commit changes from spotless
Jan 28, 2025
03a1591
fix parathensis bug
jakelandis Jan 29, 2025
7944d60
fix test and minor clean up
jakelandis Jan 29, 2025
337f6e1
minor clean up and test fix
jakelandis Jan 29, 2025
4fd53cd
fix direct access to .fs and .ds indices
jakelandis Jan 30, 2025
81f32a9
Merge remote-tracking branch 'upstream/main' into alt_119915_hack
jakelandis Jan 30, 2025
dc5eaf5
initial black box test (copied from Nikolaj's PoC)
jakelandis Jan 31, 2025
b34965d
[CI] Auto commit changes from spotless
Jan 31, 2025
7077de9
support has privs
jakelandis Jan 31, 2025
56a8c76
Merge remote-tracking branch 'upstream/main' into alt_119915_hack
jakelandis Jan 31, 2025
4c51737
Tweaks
n1v0lg Feb 5, 2025
87ef806
Fix predicate
n1v0lg Feb 5, 2025
1619d26
Merge branch 'main' into allow-failure-store-access-flag-poc
n1v0lg Feb 5, 2025
a03f63a
More
n1v0lg Feb 5, 2025
7abcb0a
Smaller diff
n1v0lg Feb 5, 2025
b6635d0
No logging needed
n1v0lg Feb 5, 2025
4e358d0
Merge branch 'main' into allow-failure-store-access-flag-poc
n1v0lg Feb 5, 2025
c4e8b5a
Fix intersection logic
n1v0lg Feb 5, 2025
ee155ea
More fixes
n1v0lg Feb 5, 2025
bc4d768
Tests
n1v0lg Feb 5, 2025
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 @@ -76,6 +76,10 @@ default Index getWriteIndex(IndexRequest request, Metadata metadata) {
return getWriteIndex();
}

default boolean isFailureIndexOfDataStream() {
return false;
}

/**
* @return the data stream to which this index belongs or <code>null</code> if this is not a concrete index or
* if it is a concrete index that does not belong to a data stream.
Expand Down Expand Up @@ -193,6 +197,11 @@ public boolean isSystem() {
return isSystem;
}

@Override
public boolean isFailureIndexOfDataStream() {
return dataStream != null && dataStream.isFailureStoreIndex(getName());
}

@Override
public boolean equals(Object o) {
if (this == o) return true;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@
import java.util.HashSet;
import java.util.List;
import java.util.Set;
import java.util.function.Predicate;
import java.util.function.BiPredicate;
import java.util.function.Supplier;

public class IndexAbstractionResolver {
Expand All @@ -37,7 +37,7 @@ public List<String> resolveIndexAbstractions(
IndicesOptions indicesOptions,
Metadata metadata,
Supplier<Set<String>> allAuthorizedAndAvailable,
Predicate<String> isAuthorized,
BiPredicate<String, String> isAuthorized,
boolean includeDataStreams
) {
List<String> finalIndices = new ArrayList<>();
Expand Down Expand Up @@ -103,7 +103,7 @@ && isIndexVisible(
resolveSelectorsAndCombine(indexAbstraction, selectorString, indicesOptions, resolvedIndices, metadata);
if (minus) {
finalIndices.removeAll(resolvedIndices);
} else if (indicesOptions.ignoreUnavailable() == false || isAuthorized.test(indexAbstraction)) {
} else if (indicesOptions.ignoreUnavailable() == false || isAuthorized.test(indexAbstraction, selectorString)) {
// Unauthorized names are considered unavailable, so if `ignoreUnavailable` is `true` they should be silently
// discarded from the `finalIndices` list. Other "ways of unavailable" must be handled by the action
// handler, see: https://github.com/elastic/elasticsearch/issues/90215
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -236,6 +236,6 @@ private List<String> resolveAbstractionsSelectorAllowed(List<String> expressions
}

private List<String> resolveAbstractions(List<String> expressions, IndicesOptions indicesOptions, Supplier<Set<String>> mask) {
return indexAbstractionResolver.resolveIndexAbstractions(expressions, indicesOptions, metadata, mask, (idx) -> true, true);
return indexAbstractionResolver.resolveIndexAbstractions(expressions, indicesOptions, metadata, mask, (idx, sel) -> true, true);
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -295,7 +295,11 @@ interface AuthorizedIndices {
/**
* Checks if an index-like resource name is authorized, for an action by a user. The resource might or might not exist.
*/
boolean check(String name);
default boolean check(String name) {
return check(name, null);
}

boolean check(String name, String selector);
}

/**
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -89,7 +89,6 @@ public Builder addGroup(
public IndicesPermission build() {
return new IndicesPermission(restrictedIndices, groups.toArray(Group.EMPTY_ARRAY));
}

}

private IndicesPermission(RestrictedIndices restrictedIndices, Group[] groups) {
Expand Down Expand Up @@ -143,16 +142,27 @@ public boolean hasFieldOrDocumentLevelSecurity() {

private IsResourceAuthorizedPredicate buildIndexMatcherPredicateForAction(String action) {
final Set<String> ordinaryIndices = new HashSet<>();
final Set<String> ordinaryIndicesWithFailureStoreAccess = new HashSet<>();
final Set<String> restrictedIndices = new HashSet<>();
final Set<String> restrictedIndicesWithFailureStoreAccess = new HashSet<>();

final Set<String> grantMappingUpdatesOnIndices = new HashSet<>();
final Set<String> grantMappingUpdatesOnRestrictedIndices = new HashSet<>();
final boolean isMappingUpdateAction = isMappingUpdateAction(action);
for (final Group group : groups) {
if (group.actionMatcher.test(action)) {
if (group.allowRestrictedIndices) {
restrictedIndices.addAll(Arrays.asList(group.indices()));
if (group.allowFailureStoreAccess) {
restrictedIndicesWithFailureStoreAccess.addAll(Arrays.asList(group.indices()));
} else {
restrictedIndices.addAll(Arrays.asList(group.indices()));
}
} else {
ordinaryIndices.addAll(Arrays.asList(group.indices()));
if (group.allowFailureStoreAccess) {
ordinaryIndicesWithFailureStoreAccess.addAll(Arrays.asList(group.indices()));
} else {
ordinaryIndices.addAll(Arrays.asList(group.indices()));
}
}
} else if (isMappingUpdateAction && containsPrivilegeThatGrantsMappingUpdatesForBwc(group)) {
// special BWC case for certain privileges: allow put mapping on indices and aliases (but not on data streams), even if
Expand All @@ -164,7 +174,10 @@ private IsResourceAuthorizedPredicate buildIndexMatcherPredicateForAction(String
}
}
}
final StringMatcher nameMatcher = indexMatcher(ordinaryIndices, restrictedIndices);
final StringMatcher nameMatcher = indexMatcher(ordinaryIndices, restrictedIndices).and(
"<not-failure-store>",
name -> name.endsWith("::failures") == false
).or(indexMatcher(ordinaryIndicesWithFailureStoreAccess, restrictedIndicesWithFailureStoreAccess));
final StringMatcher bwcSpecialCaseMatcher = indexMatcher(grantMappingUpdatesOnIndices, grantMappingUpdatesOnRestrictedIndices);
return new IsResourceAuthorizedPredicate(nameMatcher, bwcSpecialCaseMatcher);
}
Expand All @@ -173,14 +186,19 @@ private IsResourceAuthorizedPredicate buildIndexMatcherPredicateForAction(String
* This encapsulates the authorization test for resources.
* There is an additional test for resources that are missing or that are not a datastream or a backing index.
*/
public static class IsResourceAuthorizedPredicate implements BiPredicate<String, IndexAbstraction> {
public static class IsResourceAuthorizedPredicate {

private final BiPredicate<String, IndexAbstraction> biPredicate;

// public for tests
public IsResourceAuthorizedPredicate(StringMatcher resourceNameMatcher, StringMatcher additionalNonDatastreamNameMatcher) {
this((String name, @Nullable IndexAbstraction indexAbstraction) -> {
assert indexAbstraction == null || name.equals(indexAbstraction.getName());
// TODO this is hacky
assert indexAbstraction == null
|| (name.equals(indexAbstraction.getName())
|| name.equals(
IndexNameExpressionResolver.combineSelector(indexAbstraction.getName(), IndexComponentSelector.FAILURES)
));
return resourceNameMatcher.test(name)
|| (isPartOfDatastream(indexAbstraction) == false && additionalNonDatastreamNameMatcher.test(name));
});
Expand All @@ -191,13 +209,12 @@ private IsResourceAuthorizedPredicate(BiPredicate<String, IndexAbstraction> biPr
}

/**
* Given another {@link IsResourceAuthorizedPredicate} instance in {@param other},
* return a new {@link IsResourceAuthorizedPredicate} instance that is equivalent to the conjunction of
* authorization tests of that other instance and this one.
*/
@Override
public final IsResourceAuthorizedPredicate and(BiPredicate<? super String, ? super IndexAbstraction> other) {
return new IsResourceAuthorizedPredicate(this.biPredicate.and(other));
* Given another {@link IsResourceAuthorizedPredicate} instance in {@param other},
* return a new {@link IsResourceAuthorizedPredicate} instance that is equivalent to the conjunction of
* authorization tests of that other instance and this one.
*/
public final IsResourceAuthorizedPredicate and(IsResourceAuthorizedPredicate other) {
return new IsResourceAuthorizedPredicate(this.biPredicate.and(other.biPredicate));
}

/**
Expand All @@ -215,11 +232,24 @@ public final boolean test(IndexAbstraction indexAbstraction) {
* if it doesn't.
* Returns {@code true} if access to the given resource is authorized or {@code false} otherwise.
*/
@Override
public boolean test(String name, @Nullable IndexAbstraction indexAbstraction) {
return biPredicate.test(name, indexAbstraction);
}

/**
* Similar to {@link #test(IndexAbstraction)} but for the failures component of a data stream. Adds ::failures to name of the
* index abstraction to test if ::failures are allowed
*/
public final boolean testDataStreamForFailureAccess(IndexAbstraction indexAbstraction) {
// TODO clean up
assert indexAbstraction != null && indexAbstraction.getType() == IndexAbstraction.Type.DATA_STREAM;
return biPredicate.test(
IndexNameExpressionResolver.combineSelector(indexAbstraction.getName(), IndexComponentSelector.FAILURES),
// this cannot be `null`, since otherwise this is not treated as a data stream
indexAbstraction
);
}

private static boolean isPartOfDatastream(IndexAbstraction indexAbstraction) {
return indexAbstraction != null
&& (indexAbstraction.getType() == IndexAbstraction.Type.DATA_STREAM || indexAbstraction.getParentDataStream() != null);
Expand Down Expand Up @@ -371,17 +401,20 @@ private static class IndexResource {

private IndexResource(String name, @Nullable IndexAbstraction abstraction, @Nullable IndexComponentSelector selector) {
assert name != null : "Resource name cannot be null";
assert abstraction == null || abstraction.getName().equals(name)
: "Index abstraction has unexpected name [" + abstraction.getName() + "] vs [" + name + "]";
assert abstraction == null
|| selector == null
|| IndexComponentSelector.FAILURES.equals(selector) == false
|| abstraction.isDataStreamRelated()
: "Invalid index component selector ["
+ selector.getKey()
+ "] applied to abstraction of type ["
+ abstraction.getType()
+ "]";
// assert abstraction == null || abstraction.getName().equals(name)
// : "Index abstraction has unexpected name [" + abstraction.getName() + "] vs [" + name + "]";
// assert abstraction == null
// || selector == null
// || IndexComponentSelector.FAILURES.equals(selector) == false
// || abstraction.isDataStreamRelated()
// : "Invalid index component selector ["
// + selector.getKey()
// + "] applied to abstraction of type ["
// + abstraction.getType()
// + "]";
var tuple = IndexNameExpressionResolver.splitSelectorExpression(name);
assert tuple.v2() == null || tuple.v2().equals(IndexComponentSelector.FAILURES.getKey())
: "Unexpected selector [" + tuple.v2() + "] in index name [" + name + "]";
this.name = name;
this.indexAbstraction = abstraction;
this.selector = selector;
Expand Down Expand Up @@ -411,7 +444,11 @@ public boolean isPartOfDataStream() {
public boolean checkIndex(Group group) {
final DataStream ds = indexAbstraction == null ? null : indexAbstraction.getParentDataStream();
if (ds != null) {
if (group.checkIndex(ds.getName())) {
boolean isFailureStoreIndex = ds.isFailureStoreIndex(indexAbstraction.getName());
if (isFailureStoreIndex
&& group.checkIndex(IndexNameExpressionResolver.combineSelector(ds.getName(), IndexComponentSelector.FAILURES))) {
return true;
} else if (isFailureStoreIndex == false && group.checkIndex(ds.getName())) {
return true;
}
}
Expand Down Expand Up @@ -500,13 +537,18 @@ public IndicesAccessControl authorize(
int totalResourceCount = 0;
Map<String, IndexAbstraction> lookup = metadata.getIndicesLookup();
for (String indexOrAlias : requestedIndicesOrAliases) {
// Remove any selectors from abstraction name. Discard them for this check as we do not have access control for them (yet)
Tuple<String, String> expressionAndSelector = IndexNameExpressionResolver.splitSelectorExpression(indexOrAlias);
indexOrAlias = expressionAndSelector.v1();
IndexComponentSelector selector = expressionAndSelector.v2() == null
? null
: IndexComponentSelector.getByKey(expressionAndSelector.v2());
final IndexResource resource = new IndexResource(indexOrAlias, lookup.get(indexOrAlias), selector);
// If the request has name::data, upstream code will remove ::data from the name
// If the request has name::* upstream code will convert to name::failures and name (without ::data)
assert selector != IndexComponentSelector.DATA : "Data selector is not allowed in this context";
assert selector != IndexComponentSelector.ALL_APPLICABLE : "All selector is not allowed in this context";
assert selector == null || selector == IndexComponentSelector.FAILURES
: "Only the failures selector " + "or none is not allowed in this context";
// look up the IndexAbstraction by the name without the selector, but leave the (::failures) selector for authorization
final IndexResource resource = new IndexResource(indexOrAlias, lookup.get(expressionAndSelector.v1()), selector);
resources.put(resource.name, resource);
totalResourceCount += resource.size(lookup);
}
Expand Down Expand Up @@ -790,6 +832,9 @@ private Map<Automaton, Automaton> indexGroupAutomatons(boolean combine) {

public static class Group {
public static final Group[] EMPTY_ARRAY = new Group[0];
// TODO this is just a hack to avoid implementing a new field in this POC; this would be set via allow_failure_store_access on
// the role descriptor
public static final String FAILURE_STORE_ACCESS_MARKER = ".failure_store_access_marker";

private final IndexPrivilege privilege;
private final Predicate<String> actionMatcher;
Expand All @@ -803,6 +848,7 @@ public static class Group {
// users. Setting this flag true eliminates the special status for the purpose of this permission - restricted indices still have
// to be covered by the "indices"
private final boolean allowRestrictedIndices;
public final boolean allowFailureStoreAccess;

public Group(
IndexPrivilege privilege,
Expand All @@ -816,13 +862,19 @@ public Group(
this.privilege = privilege;
this.actionMatcher = privilege.predicate();
this.indices = indices;
boolean allowFailureStoreAccess = allowFailureStoreAccess(indices);
this.allowFailureStoreAccess = allowFailureStoreAccess;
this.allowRestrictedIndices = allowRestrictedIndices;
ConcurrentHashMap<String[], Automaton> indexNameAutomatonMemo = new ConcurrentHashMap<>(1);
if (allowRestrictedIndices) {
this.indexNameMatcher = StringMatcher.of(indices);
this.indexNameMatcher = getIndexNameMatcher(allowFailureStoreAccess, indices);
// TODO handle this, too
this.indexNameAutomaton = () -> indexNameAutomatonMemo.computeIfAbsent(indices, k -> Automatons.patterns(indices));
} else {
this.indexNameMatcher = StringMatcher.of(indices).and(name -> restrictedIndices.isRestricted(name) == false);
this.indexNameMatcher = getIndexNameMatcher(allowFailureStoreAccess, indices).and(
name -> restrictedIndices.isRestricted(name) == false
);
// TODO handle this, too
this.indexNameAutomaton = () -> indexNameAutomatonMemo.computeIfAbsent(
indices,
k -> Automatons.minusAndMinimize(Automatons.patterns(indices), restrictedIndices.getAutomaton())
Expand All @@ -832,6 +884,17 @@ public Group(
this.query = query;
}

private static StringMatcher getIndexNameMatcher(boolean allowFailureStoreAccess, String[] indices) {
if (false == allowFailureStoreAccess) {
return StringMatcher.of(indices).and(name -> name.endsWith("::failures") == false);
}
return StringMatcher.of(indices);
}

private static boolean allowFailureStoreAccess(String... indices) {
return Arrays.stream(indices).anyMatch(index -> index.equals("*") || index.equals(FAILURE_STORE_ACCESS_MARKER));
}

public IndexPrivilege privilege() {
return privilege;
}
Expand Down
Loading